Merge pull request 'nisse-review: read code from a git clone kept between runs' (#160) from review-clone into main

docs/Architecture.md

@@ -928,7 +928,11 @@ Two agents can argue forever, so a PR's exchanges are capped.
   administer, a repository it reviews is refused, and stops if granted that
   while running. Reads are
   bound by the harness to the item's repository, so what it might repeat in a
-  comment is already visible to that repository's readers.
+  comment is already visible to that repository's readers. The code is a git
+  clone kept on its host between runs, checked out at the PR's head (and its
+  base when asked); git runs only the harness's fixed commands, walled off from
+  the host's git configuration and from anything the code could make it run
+  (links, hooks, submodules).
 - **It writes only comments and reviews on the item's PR.** Its reviews never
   approve and never request changes: the operator's approval alone counts for
   merging (D5), and Gitea's branch protection enforces it.

docs/Development-Plan.md

@@ -616,7 +616,9 @@ waits until two exist to compare. Moved, unchanged in behaviour:
   `list_files`, `search_code`; a long file is read in parts, `start` giving
   the line, since a file cut at 20,000 characters left its end out of reach),
   and its issues (`read_issue`,
-  `list_issues`): no git, no shell, bound to the item's repository.
+  `list_issues`): no shell, bound to the item's repository. Since the M7.5
+  follow-ups, the code comes from a git clone kept between runs, not an
+  archive per run (see there).
 - **A review:** starts from the PR, its commits, diff, comments, the issues it
   closes and CI; `comment` drafts a point on any line of any file (checked to
   exist), `post_review` posts one COMMENT review at the head. Gitea takes
@@ -699,9 +701,22 @@ of earlier points, replies on threads, the round cap and its reset, `stop` and
   PR goes back to ready with no failure counted; a repository Gitea won't
   list is logged when that starts or changes (`intake.repo_failed`), and when
   it ends (`intake.repo_ok`), not every round. Tests: `test_review_stop.py`.
+- **Code from a kept clone** (operator: archives don't scale to very large
+  repositories): each repository is cloned once, with its whole history, under
+  `scratch/repos/<owner>/<name>/` (`head/` the clone, `base/` a worktree made
+  when read). A run fetches every branch and the PR's `refs/pull/N/head`,
+  checks out the head exactly (`checkout --force`, `clean -ffdx`), and fails if
+  that head is gone. Only the latest repository is kept for now; the layout
+  holds several. Git runs walled off: none of the host's git configuration,
+  links as plain files, no hooks, submodules or LFS, only the Gitea URL's
+  protocol, and the token as an `http.extraHeader` for that URL in git's
+  environment only. A stop ends a git command, and a cut-short first clone is
+  removed. `review.clone` / `review.fetch` log the time and size. No size
+  limits yet. Tests: `test_clones.py`, against local bare repositories.
 
 **M7.6 Deployment.** *Waits for the agent host (Phase 6).* Its image, container,
-Gitea account and secrets (R5).
+Gitea account and secrets (R5). The image needs `git`, and `scratch/` a
+persistent volume, so clones outlive a restart.
 
 ### Phase 8 — `nisse-ops`
 

nisse-core/src/nisse_core/gitea.py

@@ -135,7 +135,7 @@ class GiteaClient:
         return response.json() if response.content else None
 
     def _raw(self, path: str, **kwargs: Any) -> bytes:
-        """A GET whose answer isn't JSON: a diff, an archive."""
+        """A GET whose answer isn't JSON: a diff."""
         try:
             response = self._http.get(path, **kwargs)
         except httpx.HTTPError as exc:
@@ -466,10 +466,6 @@ class GiteaClient:
             found.append(f"{str(item.get('sha', ''))[:7]} {first}".strip())
         return found
 
-    def archive(self, repo: str, ref: str) -> bytes:
-        """The repository at ``ref`` (a commit or branch), as a .tar.gz."""
-        return self._raw(f"/repos/{repo}/archive/{ref}.tar.gz")
-
     def issue(self, repo: str, index: int) -> Issue:
         return _issue(self._request("GET", f"/repos/{repo}/issues/{index}"))
 

nisse-review/README.md

@@ -11,8 +11,9 @@ A thin application over `nisse-core`, with its own small scheduler:
 - `state`: its state store, and a review item (one per PR).
 - `scheduler`: the duty (ready PRs), one review at a time, recovery after a
   crash.
-- `reader`: the repository at the PR's head and base, read-only, from Gitea's
+- `clones`: each repository as a git clone kept between runs (the latest
-  archives: the run's executor.
+  one), checked out at the PR's head and, when read, its base.
+- `reader`: those trees, read-only, as the run's executor.
 - `review`: one review run, and posting it.
 - `build`, `main`: putting it together, and the access check at startup.
 

nisse-review/src/nisse_review/build.py

@@ -2,7 +2,6 @@
 before it reviews anything."""
 
 from dataclasses import dataclass
-from pathlib import Path
 
 from nisse_core.gitea import GiteaClient
 from nisse_core.knowledge.handlers import Knowledge
@@ -17,6 +16,7 @@ from nisse_core.models.llm import LiteLLMClient
 from nisse_core.oplog import OpLog
 from nisse_core.state.cancel import Cancel
 from nisse_review.access import AccessRefused, check, reviewed
+from nisse_review.clones import Clones
 from nisse_review.config import ReviewConfig
 from nisse_review.review import Reviewer
 from nisse_review.scheduler import Scheduler
@@ -142,14 +142,20 @@ def build(
         max_turns=tuning.review.max_turns,
         diff_chars=tuning.review.diff_chars,
     )
-    scratch = config.state_db.parent / "scratch"
+    # The repositories' clones, kept between runs (R3).
-    scratch.mkdir(exist_ok=True)
+    clones = Clones(
+        config.state_db.parent / "scratch" / "repos",
+        config.gitea_url,
+        config.gitea_token,
+        oplog=oplog,
+        cancel=cancel,
+    )
     scheduler = Scheduler(
         store,
         gitea,
         reviewer,
         oplog=oplog,
-        scratch=Path(scratch),
+        sources=clones,
         poll_s=tuning.review.poll_s,
         attempts=tuning.review.attempts,
         ci_wait_s=tuning.review.ci_wait_s,

nisse-review/src/nisse_review/clones.py

@@ -0,0 +1,253 @@
+"""The repositories the reviewer reads, as git clones kept between runs (R3).
+
+Each repository is cloned once, with its whole history, under ``root``:
+
+    root/<owner>/<name>/head/   the clone, checked out at the PR's head
+    root/<owner>/<name>/base/   a worktree at the base branch, when read
+
+A run fetches what changed since (every branch, and the PR's own ref, which
+also reaches a PR from a fork), checks out the head it reviews, and removes
+whatever an earlier PR left. Only the most recently used repositories are
+kept (``keep``); the rest are deleted when another is opened.
+
+Git runs on code nobody vetted, so it runs with none of this host's git
+configuration, links checked out as plain files (never pointing out of the
+clone), no hooks, no submodules, no LFS downloads, and only the configured
+Gitea's protocol. The token reaches git in its environment, as a header for
+Gitea's address alone: never on a command line, in a file, or in a URL.
+
+A stop (``cancel``) ends a git command at once: it raises ``Stopped``, and a
+first clone it cut short is deleted, to be made afresh next time.
+"""
+
+import os
+import shutil
+import subprocess
+import time
+from pathlib import Path
+
+from nisse_core.models.interruptible import Stopped
+from nisse_core.oplog import OpLog
+from nisse_core.state.cancel import Cancel
+
+_DONE = "cloned"  # in a repository's folder once its first fetch finished
+_USED = "used"  # touched each time it is opened: which to keep
+
+
+class CloneError(RuntimeError):
+    """Git couldn't give the code: the run fails, and is tried again."""
+
+
+class Clones:
+    def __init__(
+        self,
+        root: Path,
+        gitea_url: str,
+        token: str,
+        *,
+        oplog: OpLog,
+        cancel: Cancel | None = None,
+        keep: int = 1,
+    ) -> None:
+        self._root = root
+        self._url = gitea_url.rstrip("/")
+        self._token = token
+        self._oplog = oplog
+        self._cancel = cancel or Cancel()
+        self._keep = keep
+        root.mkdir(parents=True, exist_ok=True)
+
+    def open(self, repo: str, number: int, head: str, base: str) -> "Checkout":
+        """``repo``'s PR ``number``, at ``head``, merging into ``base``. Git
+        runs only when a tree is first read."""
+        return Checkout(self, repo, number, head, base)
+
+    # -- for Checkout -----------------------------------------------------------
+
+    def folder(self, repo: str) -> Path:
+        owner, _, name = repo.partition("/")
+        return self._root / owner / name
+
+    def prune(self, repo: str) -> None:
+        """Keep ``repo`` and the most recently used others, to ``keep``."""
+        mine = self.folder(repo)
+        mine.mkdir(parents=True, exist_ok=True)
+        (mine / _USED).touch()
+        others = [
+            folder
+            for owner in self._root.iterdir()
+            if owner.is_dir()
+            for folder in owner.iterdir()
+            if folder.is_dir() and folder != mine
+        ]
+        others.sort(key=_used, reverse=True)
+        for folder in others[self._keep - 1 :]:
+            shutil.rmtree(folder, ignore_errors=True)
+            self._oplog.emit(
+                "review.clone_removed", cause="not recent", path=str(folder)
+            )
+        for owner in self._root.iterdir():
+            if owner.is_dir() and not any(owner.iterdir()):
+                owner.rmdir()
+
+    def git(self, *args: str, cwd: Path) -> str:
+        """One git command, in the walled environment; its output."""
+        with subprocess.Popen(
+            ["git", *args],
+            cwd=cwd,
+            env=self._env(),
+            stdin=subprocess.DEVNULL,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            text=True,
+        ) as proc:
+            while True:
+                try:
+                    out, err = proc.communicate(timeout=0.5)
+                    break
+                except subprocess.TimeoutExpired:
+                    if self._cancel.requested:
+                        proc.terminate()  # git removes its lock files
+                        proc.communicate()
+                        raise Stopped(f"a stop ended git {args[0]}") from None
+        if proc.returncode != 0:
+            raise CloneError(f"git {args[0]} failed: {err.strip()[-500:]}")
+        return out
+
+    def remote(self, repo: str) -> str:
+        return f"{self._url}/{repo}.git"
+
+    def _env(self) -> dict[str, str]:
+        settings = {
+            "core.symlinks": "false",
+            "core.hooksPath": os.devnull,
+            "core.fsmonitor": "false",
+            "submodule.recurse": "false",
+            "fetch.recurseSubmodules": "false",
+            "advice.detachedHead": "false",
+            "gc.auto": "0",  # no background work left running after a stop
+        }
+        if self._token:
+            settings[f"http.{self._url}/.extraHeader"] = (
+                f"Authorization: token {self._token}"
+            )
+        env = {
+            "PATH": os.environ.get("PATH", "/usr/bin:/bin"),
+            "HOME": str(self._root),  # no ~/.gitconfig of this host's
+            "GIT_CONFIG_NOSYSTEM": "1",
+            "GIT_CONFIG_GLOBAL": os.devnull,
+            "GIT_TERMINAL_PROMPT": "0",
+            # Never another repository above a clone, should one be missing.
+            "GIT_CEILING_DIRECTORIES": str(self._root),
+            "GIT_LFS_SKIP_SMUDGE": "1",
+            "GIT_ALLOW_PROTOCOL": self._url.partition(":")[0],
+            "GIT_CONFIG_COUNT": str(len(settings)),
+        }
+        for n, (key, value) in enumerate(settings.items()):
+            env[f"GIT_CONFIG_KEY_{n}"] = key
+            env[f"GIT_CONFIG_VALUE_{n}"] = value
+        return env
+
+
+class Checkout:
+    """One run's view of a repository: ``tree("head")`` and
+    ``tree("base")``, each a folder of plain files."""
+
+    def __init__(
+        self, clones: Clones, repo: str, number: int, head: str, base: str
+    ) -> None:
+        self._clones = clones
+        self.repo = repo
+        self._number = number
+        self._head = head
+        self._base = base
+        self._trees: dict[str, Path] = {}
+
+    def tree(self, ref: str) -> Path:
+        if ref not in ("head", "base"):
+            raise ValueError(f"ref is head or base, not {ref!r}")
+        if ref not in self._trees:
+            if "head" not in self._trees:
+                self._trees["head"] = self._checkout_head()
+            if ref == "base":
+                self._trees["base"] = self._checkout_base()
+        return self._trees[ref]
+
+    def _checkout_head(self) -> Path:
+        clones, folder = self._clones, self._clones.folder(self.repo)
+        clones.prune(self.repo)
+        head = folder / "head"
+        first = not (folder / _DONE).exists()
+        if first:
+            shutil.rmtree(head, ignore_errors=True)
+            shutil.rmtree(folder / "base", ignore_errors=True)
+            head.mkdir()
+            clones.git("init", "-q", cwd=head)
+            clones.git("remote", "add", "origin", clones.remote(self.repo), cwd=head)
+        started = time.monotonic()
+        try:
+            clones.git(
+                "fetch",
+                "-q",
+                "--prune",
+                "--no-tags",
+                "origin",
+                "+refs/heads/*:refs/remotes/origin/*",
+                f"+refs/pull/{self._number}/head:refs/review/pull/{self._number}",
+                cwd=head,
+            )
+        except BaseException:
+            if first:  # half a clone is no clone
+                shutil.rmtree(folder, ignore_errors=True)
+            raise
+        if first:
+            (folder / _DONE).touch()
+        clones._oplog.emit(
+            "review.clone" if first else "review.fetch",
+            cause=self.repo,
+            seconds=round(time.monotonic() - started, 1),
+            size_kb=_size_kb(clones, head),
+        )
+        try:
+            clones.git("cat-file", "-e", f"{self._head}^{{commit}}", cwd=head)
+        except CloneError:
+            raise CloneError(
+                f"{self._head[:12]} is no longer in {self.repo} (force-pushed?)"
+            ) from None
+        _put(clones, head, self._head)
+        return head
+
+    def _checkout_base(self) -> Path:
+        clones, head = self._clones, self._clones.folder(self.repo) / "head"
+        base = head.parent / "base"
+        commit = f"refs/remotes/origin/{self._base}"
+        if not (base / ".git").exists():
+            shutil.rmtree(base, ignore_errors=True)
+            clones.git("worktree", "prune", cwd=head)
+            clones.git("worktree", "add", "-q", "--detach", str(base), commit, cwd=head)
+            return base
+        _put(clones, base, commit)
+        return base
+
+
+def _put(clones: Clones, tree: Path, commit: str) -> None:
+    """The work tree at ``commit``, exactly: nothing else in it."""
+    git_dir = Path(clones.git("rev-parse", "--absolute-git-dir", cwd=tree).strip())
+    lock = git_dir / "index.lock"
+    if lock.is_file():
+        lock.unlink()  # left by a run killed mid-checkout; it's the only user
+    clones.git("checkout", "-q", "--force", "--detach", commit, cwd=tree)
+    clones.git("clean", "-q", "-ffdx", cwd=tree)
+
+
+def _size_kb(clones: Clones, head: Path) -> int:
+    counted = clones.git("count-objects", "-v", cwd=head)
+    sizes = dict(line.split(": ", 1) for line in counted.splitlines() if ": " in line)
+    return int(sizes.get("size", 0)) + int(sizes.get("size-pack", 0))
+
+
+def _used(folder: Path) -> float:
+    try:
+        return (folder / _USED).stat().st_mtime
+    except OSError:
+        return 0.0

nisse-review/src/nisse_review/reader.py

@@ -1,19 +1,18 @@
 """The repository, read-only, as the reviewer's executor (R3, L13).
 
 Reads are bound to the item's repository by construction: no tool names one.
-Code comes from Gitea's archive of the PR's head and of its base branch, one
+Code comes from a clone kept on the agent host (``clones``), checked out at
-API call each, unpacked into a scratch folder on the agent host and read in
+the PR's head and, when asked, its base branch, and is read in Python: git
-Python: no git, no shell, nothing a model chose is ever run. Issues come
+runs only the harness's fixed commands, and nothing a model chose is ever
-from the Gitea API, for the same repository.
+run. Git's own folder is never shown. Issues come from the Gitea API, for
+the same repository.
 """
 
-import io
+import os
 import posixpath
-import shutil
-import tarfile
 from collections.abc import Mapping
 from pathlib import Path
-from typing import Any
+from typing import Any, Protocol
 
 from nisse_core.gitea import GiteaClient, GiteaError
 from nisse_core.loop.driver import ToolOutcome
@@ -59,37 +58,29 @@ LIST_ISSUES = tool(
 READER_TOOLS = [READ_FILE, LIST_FILES, SEARCH_CODE, READ_ISSUE, LIST_ISSUES]
 
 
-class Repository:
+class Trees(Protocol):
-    """``refs``: what ``head`` and ``base`` name (commits or branches).
+    """Where a run's code is: ``tree("head")`` or ``tree("base")``, each a
-    ``scratch``: a folder of this run's own, removed by ``close``."""
+    folder of plain files, made on first use (``clones.Checkout``)."""
 
-    def __init__(
+    def tree(self, ref: str) -> Path: ...
-        self,
+
-        gitea: GiteaClient,
+
-        repo: str,
+class Sources(Protocol):
-        refs: Mapping[str, str],
+    """Where runs get their code from (``clones.Clones``)."""
-        scratch: Path,
+
-    ) -> None:
+    def open(self, repo: str, number: int, head: str, base: str) -> Trees: ...
+
+
+class Repository:
+    def __init__(self, gitea: GiteaClient, repo: str, trees: Trees) -> None:
         self._gitea = gitea
         self.repo = repo
-        self._refs = dict(refs)
+        self._trees = trees
-        self._scratch = scratch
-        self._trees: dict[str, Path] = {}
-
-    def close(self) -> None:
-        shutil.rmtree(self._scratch, ignore_errors=True)
 
     def tree(self, ref: str) -> Path:
-        """The repository at ``ref``, unpacked on first use."""
+        if ref not in ("head", "base"):
-        if ref not in self._refs:
             raise ValueError(f"ref is head or base, not {ref!r}")
-        if ref not in self._trees:
+        return self._trees.tree(ref)
-            root = self._scratch / ref
-            shutil.rmtree(root, ignore_errors=True)
-            root.mkdir(parents=True)
-            _unpack(self._gitea.archive(self.repo, self._refs[ref]), root)
-            self._trees[ref] = root
-        return self._trees[ref]
 
     def lines(self, path: str, ref: str = "head") -> list[str] | None:
         """A file's lines, or None if there's no such file."""
@@ -162,11 +153,7 @@ class Repository:
         start = root if not folder else root / (_clean(folder) or "\0")
         if not start.is_dir():
             return ToolOutcome(f"{folder}: no such folder at {ref}", False)
-        found = sorted(
+        found = sorted(str(p.relative_to(root)) for p in _files(start, root))
-            str(p.relative_to(root))
-            for p in start.rglob("*")
-            if p.is_file() and not p.is_symlink()
-        )
         shown = found[:MAX_LISTED]
         more = f"\n[{len(found) - len(shown)} more]" if len(found) > len(shown) else ""
         return ToolOutcome(
@@ -184,9 +171,7 @@ class Repository:
         root = self.tree(ref)
         needle = text.lower()
         hits: list[str] = []
-        for path in sorted(root.rglob("*")):
+        for path in sorted(_files(root, root)):
-            if not path.is_file() or path.is_symlink():
-                continue
             try:
                 content = path.read_text()
             except (UnicodeDecodeError, OSError):
@@ -236,29 +221,23 @@ def _clean(path: str) -> str | None:
     if not path or path.startswith("/") or "\\" in path:
         return None
     clean = posixpath.normpath(path)
-    if clean == "." or clean.startswith(".."):
+    if clean == "." or clean.startswith("..") or clean.split("/")[0] == ".git":
         return None
     return clean
 
 
-def _unpack(archive: bytes, root: Path) -> None:
+def _files(under: Path, root: Path) -> list[Path]:
-    """Gitea's archive, without its top folder; only plain files and
+    """The plain files under a folder; git's own folder is never walked."""
-    folders, each inside ``root``."""
+    found: list[Path] = []
-    with tarfile.open(fileobj=io.BytesIO(archive), mode="r:gz") as tar:
+    for folder, dirs, files in os.walk(under):
-        for member in tar.getmembers():
+        here = Path(folder)
-            if not (member.isfile() or member.isdir()):
+        if here == root:
-                continue  # links and devices are never followed
+            dirs[:] = [d for d in dirs if d != ".git"]
-            parts = member.name.split("/", 1)
+        found.extend(
-            if len(parts) < 2 or not parts[1]:
+            here / name
-                continue
+            for name in files
-            inner = _clean(parts[1])
+            if not (here == root and name == ".git")
-            if inner is None:
+            and (here / name).is_file()
-                continue
+            and not (here / name).is_symlink()
-            target = root / inner
+        )
-            if member.isdir():
+    return found
-                target.mkdir(parents=True, exist_ok=True)
-                continue
-            target.parent.mkdir(parents=True, exist_ok=True)
-            source = tar.extractfile(member)
-            if source is not None:
-                target.write_bytes(source.read())

nisse-review/src/nisse_review/scheduler.py

@@ -24,12 +24,10 @@ to ready, with nothing drafted posted and no failure counted.
 """
 
 import re
-import tempfile
 import threading
 import time
 from collections.abc import Callable
 from dataclasses import replace
-from pathlib import Path
 
 from nisse_core.gitea import GiteaClient, GiteaError, PullRequest
 from nisse_core.models.interruptible import Stopped
@@ -37,7 +35,7 @@ from nisse_core.oplog import OpLog
 from nisse_core.state.cancel import Cancel
 from nisse_core.state.store import WorkItem
 from nisse_review.access import check, reviewed
-from nisse_review.reader import Repository
+from nisse_review.reader import Repository, Sources
 from nisse_review.review import Draft, Posted, Reviewer, Said, answered, said_on
 from nisse_review.state import (
     CAPPED,
@@ -92,7 +90,7 @@ class Scheduler:
         reviewer: Reviewer,
         *,
         oplog: OpLog,
-        scratch: Path,
+        sources: Sources,
         poll_s: float = 60.0,
         attempts: int = 2,
         patterns: frozenset[str] | None = None,
@@ -107,7 +105,7 @@ class Scheduler:
         self._gitea = gitea
         self._reviewer = reviewer
         self._oplog = oplog
-        self._scratch = scratch
+        self._sources = sources  # the repositories' code, as clones
         self._poll_s = poll_s
         self._attempts = attempts
         # The repositories reviewed, as patterns; None: every one it was given.
@@ -388,19 +386,11 @@ class Scheduler:
             if kind == REPLIES and not known.fresh:
                 self._settle(item, known, pr, "done", Draft(), posted=None)
                 return
-            scratch = Path(tempfile.mkdtemp(dir=self._scratch, prefix="review-"))
+            trees = self._sources.open(known.repo, pr.number, pr.head_sha, pr.base)
-            repo = Repository(
+            repo = Repository(self._gitea, known.repo, trees)
-                self._gitea,
+            outcome, draft = self._reviewer.run(
-                known.repo,
+                item.id, pr, repo, kind=kind, known=known, said=said
-                {"head": pr.head_sha, "base": pr.base},
-                scratch,
             )
-            try:
-                outcome, draft = self._reviewer.run(
-                    item.id, pr, repo, kind=kind, known=known, said=said
-                )
-            finally:
-                repo.close()
             if outcome.kind == "capped" and draft.comments:
                 # Out of turns with points drafted: they're worth more posted
                 # than lost (#156), as a review that says it's partial.

nisse-review/tests/review_fakes.py

@@ -1,8 +1,7 @@
 """A Gitea and a model to review against, in memory."""
 
-import io
-import tarfile
 from dataclasses import dataclass, field, replace
+from pathlib import Path
 from typing import Any
 
 from nisse_core.gitea import (
@@ -32,15 +31,39 @@ DIFF = """diff --git a/count.py b/count.py
 """
 
 
-def archive(files: dict[str, str], *, top: str = "demo") -> bytes:
+class FakeSources:
-    out = io.BytesIO()
+    """The repository's code as plain folders: ``gitea.files`` at head,
-    with tarfile.open(fileobj=out, mode="w:gz") as tar:
+    and at base unless ``base`` is given."""
-        for path, text in files.items():
+
-            raw = text.encode()
+    def __init__(self, gitea: "FakeGitea", root: Path) -> None:
-            info = tarfile.TarInfo(f"{top}/{path}")
+        self._gitea = gitea
-            info.size = len(raw)
+        self._root = root
-            tar.addfile(info, io.BytesIO(raw))
+        self.base: dict[str, str] | None = None
-    return out.getvalue()
+        self.opened: list[tuple[str, int, str, str]] = []
+
+    def open(self, repo: str, number: int, head: str, base: str) -> "FakeTrees":
+        self.opened.append((repo, number, head, base))
+        return FakeTrees(self, len(self.opened))
+
+
+class FakeTrees:
+    def __init__(self, sources: FakeSources, n: int) -> None:
+        self._sources = sources
+        self._n = n
+        self.made: list[str] = []
+
+    def tree(self, ref: str) -> Path:
+        root = self._sources._root / f"run{self._n}" / ref
+        if not root.exists():
+            self.made.append(ref)
+            files = self._sources._gitea.files
+            if ref == "base" and self._sources.base is not None:
+                files = self._sources.base
+            for path, text in files.items():
+                (root / path).parent.mkdir(parents=True, exist_ok=True)
+                (root / path).write_text(text)
+            root.mkdir(parents=True, exist_ok=True)
+        return root
 
 
 def pr(number: int = 7, **more: Any) -> PullRequest:
@@ -172,9 +195,6 @@ class FakeGitea:
     def checks(self, repo: str, sha: str) -> str:
         return self.ci.get(sha, "success")
 
-    def archive(self, repo: str, ref: str) -> bytes:
-        return archive(self.files)
-
     def issue(self, repo: str, index: int) -> Issue:
         return Issue(index, "Counting is wrong", f"in {repo}", "don", ())
 

nisse-review/tests/test_clones.py

@@ -0,0 +1,237 @@
+"""The reviewer's code as kept git clones (R3): cloned once, fetched after,
+checked out exactly at the PR's head, only the latest repository kept, and
+git walled off from the code it fetches. A local bare repository stands in
+for Gitea (``file://``: the configured URL's protocol is the one allowed)."""
+
+import io
+import json
+import os
+import subprocess
+import threading
+import time
+from pathlib import Path
+from typing import Any
+
+import pytest
+
+from nisse_core.models.interruptible import Stopped
+from nisse_core.oplog import OpLog
+from nisse_core.state.cancel import Cancel
+from nisse_review.clones import CloneError, Clones
+from nisse_review.reader import Repository
+
+TOKEN = "s3cret-t0ken-value"
+GIT_ENV = {
+    "GIT_CONFIG_GLOBAL": os.devnull,
+    "GIT_CONFIG_NOSYSTEM": "1",
+    "GIT_AUTHOR_NAME": "t",
+    "GIT_AUTHOR_EMAIL": "t@t",
+    "GIT_COMMITTER_NAME": "t",
+    "GIT_COMMITTER_EMAIL": "t@t",
+    "PATH": os.environ["PATH"],
+}
+
+
+def git(cwd: Path, *args: str) -> str:
+    return subprocess.run(
+        ["git", *args], cwd=cwd, env=GIT_ENV, check=True, capture_output=True, text=True
+    ).stdout.strip()
+
+
+class Remote:
+    """A Gitea stand-in: bare repositories under ``root``, and a work tree
+    per repository to make commits in."""
+
+    def __init__(self, tmp: Path) -> None:
+        self.root = tmp / "gitea"
+        self.url = f"file://{self.root}"
+        self._tmp = tmp
+
+    def repo(self, name: str, files: dict[str, str]) -> Path:
+        bare = self.root / f"{name}.git"
+        bare.mkdir(parents=True)
+        git(bare, "init", "-q", "--bare", "-b", "main")
+        work = self._tmp / "work" / name
+        work.mkdir(parents=True)
+        git(work, "init", "-q", "-b", "main")
+        git(work, "remote", "add", "origin", str(bare))
+        self.commit(work, files, "first")
+        git(work, "push", "-q", "origin", "main")
+        return work
+
+    def commit(self, work: Path, files: dict[str, str | None], message: str) -> str:
+        for path, text in files.items():
+            if text is None:
+                git(work, "rm", "-q", path)
+                continue
+            (work / path).parent.mkdir(parents=True, exist_ok=True)
+            (work / path).write_text(text)
+            git(work, "add", path)
+        git(work, "commit", "-q", "-m", message)
+        return git(work, "rev-parse", "HEAD")
+
+    def pull(self, work: Path, number: int, files: dict[str, str | None]) -> str:
+        """A PR's new head, as Gitea keeps it: refs/pull/N/head."""
+        git(work, "checkout", "-q", "-B", f"pr{number}", "main")
+        sha = self.commit(work, files, f"pr {number}")
+        git(work, "push", "-q", "-f", "origin", f"HEAD:refs/pull/{number}/head")
+        git(work, "checkout", "-q", "main")
+        return sha
+
+
+def setup(tmp_path: Path, **more: Any) -> tuple[Remote, Clones, io.StringIO]:
+    out = io.StringIO()
+    remote = Remote(tmp_path)
+    clones = Clones(
+        tmp_path / "repos", remote.url, TOKEN, oplog=OpLog("t", out), **more
+    )
+    return remote, clones, out
+
+
+def events(out: io.StringIO, name: str) -> list[dict[str, Any]]:
+    found = (json.loads(line) for line in out.getvalue().splitlines())
+    return [e for e in found if e["event"] == name]
+
+
+def test_a_first_read_clones_and_checks_out_the_head(tmp_path: Path) -> None:
+    remote, clones, out = setup(tmp_path)
+    work = remote.repo("o/demo", {"count.py": "old\n", "gone.py": "x\n"})
+    head = remote.pull(work, 7, {"count.py": "new\n", "gone.py": None})
+    tree = clones.open("o/demo", 7, head, "main").tree("head")
+    assert tree == tmp_path / "repos" / "o" / "demo" / "head"
+    assert (tree / "count.py").read_text() == "new\n"
+    assert not (tree / "gone.py").exists()
+    assert git(tree, "rev-parse", "HEAD") == head
+    [cloned] = events(out, "review.clone")
+    assert cloned["cause"] == "o/demo" and "seconds" in cloned
+    assert not (tree.parent / "base").exists()  # only made when read
+
+
+def test_a_later_run_fetches_and_leaves_nothing_of_the_last(tmp_path: Path) -> None:
+    remote, clones, out = setup(tmp_path)
+    work = remote.repo("o/demo", {"count.py": "one\n"})
+    first = remote.pull(work, 7, {"extra.py": "x\n"})
+    tree = clones.open("o/demo", 7, first, "main").tree("head")
+    (tree / "junk.txt").write_text("left by the last run\n")
+    (tree / "count.py").write_text("edited in place\n")
+    second = remote.pull(work, 8, {"count.py": "two\n"})
+    again = clones.open("o/demo", 8, second, "main").tree("head")
+    assert again == tree
+    assert (tree / "count.py").read_text() == "two\n"
+    assert not (tree / "junk.txt").exists() and not (tree / "extra.py").exists()
+    assert len(events(out, "review.clone")) == 1
+    assert len(events(out, "review.fetch")) == 1
+
+
+def test_another_repository_replaces_the_last(tmp_path: Path) -> None:
+    remote, clones, out = setup(tmp_path)
+    a = remote.repo("o/a", {"a.py": "a\n"})
+    b = remote.repo("p/b", {"b.py": "b\n"})
+    clones.open("o/a", 1, remote.pull(a, 1, {"a2.py": "a\n"}), "main").tree("head")
+    clones.open("p/b", 2, remote.pull(b, 2, {"b2.py": "b\n"}), "main").tree("head")
+    assert sorted(p.name for p in (tmp_path / "repos").iterdir()) == ["p"]
+    assert (tmp_path / "repos" / "p" / "b" / "head" / "b2.py").is_file()
+    [removed] = events(out, "review.clone_removed")
+    assert removed["path"].endswith("o/a")
+
+
+def test_the_base_is_a_worktree_at_its_branch(tmp_path: Path) -> None:
+    remote, clones, _ = setup(tmp_path)
+    work = remote.repo("o/demo", {"count.py": "base\n"})
+    head = remote.pull(work, 7, {"count.py": "head\n"})
+    checkout = clones.open("o/demo", 7, head, "main")
+    base = checkout.tree("base")
+    assert base == tmp_path / "repos" / "o" / "demo" / "base"
+    assert (base / "count.py").read_text() == "base\n"
+    assert (checkout.tree("head") / "count.py").read_text() == "head\n"
+    remote.commit(work, {"count.py": "base moved\n"}, "on main")
+    git(work, "push", "-q", "origin", "main")
+    later = clones.open("o/demo", 7, head, "main")
+    assert (later.tree("base") / "count.py").read_text() == "base moved\n"
+
+
+def test_a_link_arrives_as_a_plain_file(tmp_path: Path) -> None:
+    remote, clones, _ = setup(tmp_path)
+    work = remote.repo("o/demo", {"ok.py": "x\n"})
+    git(work, "checkout", "-q", "-B", "pr7")
+    os.symlink("/etc/passwd", work / "link")
+    git(work, "add", "link")
+    git(work, "commit", "-q", "-m", "a link")
+    git(work, "push", "-q", "-f", "origin", "HEAD:refs/pull/7/head")
+    head = git(work, "rev-parse", "HEAD")
+    tree = clones.open("o/demo", 7, head, "main").tree("head")
+    assert not (tree / "link").is_symlink()
+    assert (tree / "link").read_text() == "/etc/passwd"
+
+
+def test_a_head_no_longer_there_fails_the_run(tmp_path: Path) -> None:
+    remote, clones, _ = setup(tmp_path)
+    work = remote.repo("o/demo", {"ok.py": "x\n"})
+    remote.pull(work, 7, {"a.py": "a\n"})
+    with pytest.raises(CloneError, match="force-pushed"):
+        clones.open("o/demo", 7, "0" * 40, "main").tree("head")
+
+
+def test_the_token_is_never_on_a_command_line_or_on_disk(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    remote, clones, _ = setup(tmp_path)
+    work = remote.repo("o/demo", {"ok.py": "x\n"})
+    head = remote.pull(work, 7, {"a.py": "a\n"})
+    seen: list[tuple[list[str], dict[str, str]]] = []
+    real = subprocess.Popen
+
+    def watched(args: list[str], **kwargs: Any) -> Any:
+        seen.append((list(args), dict(kwargs["env"])))
+        return real(args, **kwargs)
+
+    monkeypatch.setattr(subprocess, "Popen", watched)
+    checkout = clones.open("o/demo", 7, head, "main")
+    checkout.tree("head")
+    checkout.tree("base")
+    assert seen and all(TOKEN not in " ".join(args) for args, _ in seen)
+    headers = [v for _, env in seen for v in env.values() if TOKEN in v]
+    assert headers and all(v == f"Authorization: token {TOKEN}" for v in headers)
+    for path in (tmp_path / "repos").rglob("*"):
+        if path.is_file():
+            assert TOKEN.encode() not in path.read_bytes(), path
+
+
+def test_a_stop_ends_a_clone_and_removes_it(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    # A git whose fetch hangs, as a huge first clone would.
+    fake = tmp_path / "bin"
+    fake.mkdir()
+    real_git = subprocess.run(
+        ["which", "git"], capture_output=True, text=True, check=True
+    ).stdout.strip()
+    (fake / "git").write_text(
+        f'#!/bin/sh\n[ "$1" = fetch ] && exec sleep 30\nexec {real_git} "$@"\n'
+    )
+    (fake / "git").chmod(0o755)
+    cancel = Cancel()
+    remote, clones, _ = setup(tmp_path, cancel=cancel)
+    work = remote.repo("o/demo", {"ok.py": "x\n"})
+    head = remote.pull(work, 7, {"a.py": "a\n"})
+    monkeypatch.setenv("PATH", f"{fake}:{os.environ['PATH']}")
+    threading.Timer(0.3, cancel.set).start()
+    started = time.monotonic()
+    with pytest.raises(Stopped, match="git fetch"):
+        clones.open("o/demo", 7, head, "main").tree("head")
+    assert time.monotonic() - started < 5
+    assert not (tmp_path / "repos" / "o" / "demo").exists()  # made afresh next
+
+
+def test_the_reader_never_shows_gits_own_folder(tmp_path: Path) -> None:
+    remote, clones, _ = setup(tmp_path)
+    work = remote.repo("o/demo", {"count.py": "split here\n"})
+    head = remote.pull(work, 7, {"a.py": "a\n"})
+    repo = Repository(None, "o/demo", clones.open("o/demo", 7, head, "main"))  # type: ignore[arg-type]
+    read = repo.dispatch("read_file", {"path": ".git/config"})
+    assert not read.success
+    listed = repo.dispatch("list_files", {"path": ""})
+    assert listed.success and ".git" not in listed.result
+    assert "a.py" in listed.result and "count.py" in listed.result
+    found = repo.dispatch("search_code", {"text": "origin"})
+    assert "nothing" in found.result  # .git/config names it; never searched

nisse-review/tests/test_review.py

@@ -6,7 +6,7 @@ from pathlib import Path
 from typing import Any
 
 import pytest
-from review_fakes import FakeGitea, Scripted, call, pr
+from review_fakes import FakeGitea, FakeSources, Scripted, call, pr
 
 from nisse_core.gitea import GiteaError
 from nisse_core.knowledge.handlers import Knowledge
@@ -51,17 +51,22 @@ def world(
         oplog=oplog,
         knowledge=Knowledge(vault, Memory(vault, "memory/nisse-review"), oplog),
     )
-    scratch = tmp_path / "scratch"
+    sources = FakeSources(gitea, tmp_path / "trees")
-    scratch.mkdir()
     scheduler = Scheduler(
         store,
         gitea,  # type: ignore[arg-type]
         reviewer,
         oplog=oplog,
-        scratch=scratch,
+        sources=sources,
         **more,
     )
-    return {"gitea": gitea, "store": store, "scheduler": scheduler, "tmp": tmp_path}
+    return {
+        "gitea": gitea,
+        "store": store,
+        "scheduler": scheduler,
+        "sources": sources,
+        "tmp": tmp_path,
+    }
 
 
 def item(w: dict[str, Any], number: int = 7):
@@ -212,7 +217,7 @@ def test_reads_stay_in_the_repository(tmp_path: Path) -> None:
     assert results[1] == ("ref is head or base, not 'other'", False)
     assert "count.py:2: return len(text.split())" in results[2][0]
     assert results[3][0].startswith("[o/demo#3, by don]")
-    assert list((tmp_path / "scratch").iterdir()) == []  # the trees are gone
+    assert w["sources"].opened == [(REPO, 7, "abc123def456", "main")]
 
 
 def test_memory_notes_go_under_the_repositorys_folder(tmp_path: Path) -> None:
@@ -509,30 +514,6 @@ def test_gitea_errors_while_posting_count_as_a_failed_run(tmp_path: Path) -> Non
     assert w["store"].pending_intents() == []
 
 
-def test_an_archive_unpacks_only_plain_files_inside(tmp_path: Path) -> None:
-    import tarfile
-
-    from nisse_review.reader import _unpack
-
-    raw = io.BytesIO()
-    with tarfile.open(fileobj=raw, mode="w:gz") as tar:
-        for name, kind in [
-            ("demo/ok.py", tarfile.REGTYPE),
-            ("demo/../../evil.py", tarfile.REGTYPE),
-            ("demo/link", tarfile.SYMTYPE),
-        ]:
-            info = tarfile.TarInfo(name)
-            info.type = kind
-            info.linkname = "/etc/passwd" if kind == tarfile.SYMTYPE else ""
-            info.size = 0 if kind == tarfile.SYMTYPE else 2
-            tar.addfile(info, io.BytesIO(b"x\n") if info.size else None)
-    root = tmp_path / "tree"
-    root.mkdir()
-    _unpack(raw.getvalue(), root)
-    assert sorted(p.name for p in root.rglob("*")) == ["ok.py"]
-    assert not (tmp_path / "evil.py").exists()
-
-
 def test_the_config_names_every_problem(tmp_path: Path) -> None:
     from nisse_core.config import ConfigError
     from nisse_review.config import ReviewConfig
@@ -564,7 +545,8 @@ def test_a_long_file_is_read_in_parts_to_its_end(tmp_path: Path) -> None:
 
     body = "".join(f"x{n} = {'y' * 60}\n" for n in range(1, 1001))
     gitea = FakeGitea(files={"long.py": body})
-    repo = Repository(gitea, REPO, {"head": "abc"}, tmp_path)  # type: ignore[arg-type]
+    trees = FakeSources(gitea, tmp_path).open(REPO, 7, "abc", "main")
+    repo = Repository(gitea, REPO, trees)  # type: ignore[arg-type]
     first = repo.dispatch("read_file", {"path": "long.py"}).result
     assert len(first) < MAX_FILE_CHARS + 200
     assert "lines 1-" in first and "of 1000" in first
@@ -619,3 +601,23 @@ def test_out_of_turns_with_nothing_drafted_is_a_failed_run(tmp_path: Path) -> No
     w["scheduler"].round()
     assert w["gitea"].posted == []
     assert (item(w).position, pr_of(item(w)).attempts) == (READY, 1)
+
+
+def test_code_git_cannot_give_fails_the_run(tmp_path: Path) -> None:
+    from nisse_review.clones import CloneError
+
+    class Broken(FakeSources):
+        def open(self, repo: str, number: int, head: str, base: str):  # type: ignore[no-untyped-def]
+            trees = super().open(repo, number, head, base)
+
+            def tree(ref: str) -> Path:
+                raise CloneError("git fetch failed: no route to host")
+
+            trees.tree = tree  # type: ignore[method-assign]
+            return trees
+
+    w = world(tmp_path, Scripted(*REVIEW))
+    w["scheduler"]._sources = Broken(w["gitea"], tmp_path / "trees")
+    w["scheduler"].round()
+    assert (item(w).position, pr_of(item(w)).attempts) == (READY, 1)
+    assert w["gitea"].posted == []