nisse-review: read code from a git clone kept between runs

Archives were downloaded and unpacked afresh for every run, which doesn't
scale to very large repositories. Each repository is now cloned once, with
its whole history, under scratch/repos/<owner>/<name>/ (head/ the clone,
base/ a worktree when read). A run fetches every branch and the PR's
refs/pull/N/head, checks out the head exactly, and fails if that head is
gone. Only the latest repository is kept for now.

Git runs walled off: none of the host's git configuration, links checked
out 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; a cut-short first clone is
removed. The reader never shows .git. GiteaClient.archive is gone.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

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.
@@ -962,7 +966,10 @@ Two agents can argue forever, so a PR's exchanges are capped.
   room, it gets one too. Until then the operator stops it on a PR by commenting
   `stop` there (a review is asked for again with `review`: a fresh one of the
   whole PR, with the round count reset), and stops it entirely by stopping its
-  container. A command is an operator's whole top-level comment, acknowledged
+  container. Stopping it ends a review in progress at once: at its next turn,
+  or by breaking off the model call it is waiting on. Nothing drafted is
+  posted, and the PR is reviewed again when it starts, with no failure
+  counted. A command is an operator's whole top-level comment, acknowledged
   with a reaction rather than a comment the PR's author would have to answer.
 - **Its own identity** (L12): a Gitea account (`nisse-review`) with read access to
   the repositories it reviews, its own token and LiteLLM key, and its own

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
@@ -694,9 +696,27 @@ of earlier points, replies on threads, the round cap and its reset, `stop` and
 - **Tests:** `test_review_rounds.py`, including a no-dead-end walk over every
   position; the live smoke test adds a push and a question, and checks the
   later review at the new head.
+- **Follow-ups:** a stop (SIGTERM) ends a review in progress at its next turn,
+  or breaks off its model call (`Interruptible`, `nisse_core.models`), and the
+  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-core/src/nisse_core/models/interruptible.py

@@ -0,0 +1,51 @@
+"""A model call that a stop can break off (L2).
+
+A ``Cancel`` takes effect at the next tool boundary, but one model call can
+run for many minutes (``request_timeout_s``), longer than a service manager
+waits before it kills the process. ``Interruptible`` wraps the client and
+knows when a call is in flight; ``interrupt``, called from the signal
+handler, raises ``Stopped`` inside that call so it ends at once.
+
+Only a model call is ever broken off this way, never a post or a write:
+outside a call, ``interrupt`` does nothing and the run ends at its next
+boundary instead. Python runs signal handlers on the main thread, so this
+works when the calls are made there.
+"""
+
+from typing import Any
+
+from nisse_core.log.canonical import CanonicalEvent, StoredEvent
+from nisse_core.models.llm import LLMClient
+
+
+class Stopped(BaseException):
+    """A model call broken off by a stop. A ``BaseException``, so no handler
+    meant for failures (``except Exception``, ``except LLMError``) takes it
+    for one."""
+
+
+class Interruptible:
+    def __init__(self, client: LLMClient) -> None:
+        self._client = client
+        self._calling = False
+
+    def complete(
+        self,
+        events: list[StoredEvent],
+        tools: list[dict[str, Any]] | None,
+        model: str,
+        system: str | None = None,
+    ) -> list[CanonicalEvent]:
+        self._calling = True
+        try:
+            return self._client.complete(events, tools, model, system)
+        finally:
+            self._calling = False
+
+    def interrupt(self) -> None:
+        """From a signal handler: breaks off the call in flight, if any."""
+        if self._calling:
+            raise Stopped("a stop broke off the model call")
+
+    def __getattr__(self, name: str) -> Any:
+        return getattr(self._client, name)

nisse-dev/src/nisse_dev/loop/agent.py

@@ -2111,6 +2111,7 @@ class LoopAgent:
         result = delegation.run(
             self._store.transcript(item.id, "helper"), brief=brief, branch=branch
         )
+        lost: GitError | None = None
         try:
             # The milestone takes the helper's work (or, on failure, only
             # its own back) as uncommitted changes: harness branches never
@@ -2122,6 +2123,8 @@ class LoopAgent:
                 git._git("branch", "-q", "-D", branch)
         except GitError as exc:
             self._fact(item, f"after helper {number}: {exc}")
+            if isinstance(exc.__cause__, HostUnavailable):
+                lost = exc  # the VM went: the driver's run ends too (#137)
         for intent in self._store.pending_intents():
             # Cut off rather than finished: discarded all the same, never
             # resumed blindly (L8, L9).
@@ -2130,6 +2133,8 @@ class LoopAgent:
                 and intent.payload.get("branch") == branch
             ):
                 self._store.abort_intent(intent.id, cause=f"delegation {result.status}")
+        if lost is not None:
+            raise VMUnreachable(str(lost)) from lost
         if result.usable and result.handback is not None:
             self._fact(item, f"helper {number} finished: {result.handback.render()}")
             return f"the helper finished:\n{result.handback.render()}", True

nisse-dev/tests/test_loop_vm.py

@@ -338,3 +338,42 @@ def test_a_vm_lost_just_before_the_step_blocks_before_any_model_call(
     agent, _ = agent_for(world, project, model, clock=Clock())
     agent.intake()
     assert list(blocks(world).values()) == ["unreachable"]
+
+
+def test_a_vm_lost_as_a_helper_hands_back_ends_the_drivers_run(
+    world: dict[str, Any], project: str, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    # Found by the property walk (seed 468): taking the helper's work back
+    # onto the milestone's branch failed for the lost VM, was noted as a git
+    # error, and the driver's model was called with the VM gone.
+    from test_loop_help import Spy, hand_back, helpers
+
+    faults = Faults(monkeypatch, world["host"])
+
+    class Leaving(Spy):
+        def complete(self, events, tools, model, system=None) -> list[Any]:
+            out = super().complete(events, tools, model, system)
+            if not self.turns:  # gone as its work is taken back
+                faults.drop_on = "checkout -q -f -B"
+            return out
+
+    helper = Leaving(
+        [call("write_file", path="helped.py", content="H = 1\n")], [hand_back()]
+    )
+    model = ScriptedModel(
+        ready(),
+        RESTATE,
+        [
+            call("start_milestone", name="m"),
+            call("delegate", task="write helped.py"),
+        ],
+        [call("run_command", command="echo never")],  # never asked for
+    )
+    agent, transport = agent_for(
+        world, project, model, clock=Clock(), helpers=helpers(helper)
+    )
+    agent.intake()
+    say(transport, world, "go")
+    assert list(blocks(world).values()) == ["unreachable"]
+    assert the_item(world).position == READY
+    assert len(model.turns) == 1  # the driver's model wasn't called again

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
@@ -12,10 +11,12 @@ from nisse_core.loop.driver import Driver
 from nisse_core.loop.expert import ExpertDesk
 from nisse_core.models.catalog import ModelCatalog
 from nisse_core.models.context import BudgetedClient
+from nisse_core.models.interruptible import Interruptible
 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
@@ -36,6 +37,13 @@ class Built:
     repositories: list[str]
     store: StateStore
     gitea: GiteaClient
+    model: Interruptible
+
+    def stop(self) -> None:
+        """From the signal handler: no more rounds, the run in progress
+        ends at its next turn, and a model call in flight is broken off."""
+        self.scheduler.stop()
+        self.model.interrupt()
 
     def close(self) -> None:
         self.gitea.close()
@@ -97,7 +105,8 @@ def build(
     oplog = oplog or OpLog(AGENT)
     gitea = gitea or GiteaClient(config.gitea_url, config.gitea_token)
     account, repos = check_access(gitea, config.repositories, oplog)
-    client = client if client is not None else llm_client(config)
+    # Every model call, the reviewer's and ask_expert's, can be broken off.
+    model = Interruptible(client if client is not None else llm_client(config))  # type: ignore[arg-type]
     config.state_db.parent.mkdir(parents=True, exist_ok=True)
     store = StateStore(config.state_db, oplog=oplog)
     vault = (
@@ -116,30 +125,37 @@ def build(
         else None
     )
     tuning = config.tuning
+    cancel = Cancel()
     reviewer = Reviewer(
         store,
         gitea,
-        Driver(client, config.roles.resolve("driver"), cancel=Cancel()),  # type: ignore[arg-type]
+        Driver(model, config.roles.resolve("driver"), cancel=cancel),  # type: ignore[arg-type]
         account=account,
         gitea_url=config.gitea_url,
         oplog=oplog,
         knowledge=Knowledge(vault, memory, oplog),
         expert=ExpertDesk(
-            client,  # type: ignore[arg-type]
+            model,  # type: ignore[arg-type]
             config.roles,
             budget=tuning.expert.consult_budget,
         ),
         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,
@@ -147,5 +163,6 @@ def build(
         operators=config.gitea_operators,
         max_rounds=tuning.review.max_rounds,
         patterns=config.repositories,
+        cancel=cancel,
     )
-    return Built(scheduler, account, repos, store, gitea)
+    return Built(scheduler, account, repos, store, gitea, model)

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/main.py

@@ -26,7 +26,7 @@ def main() -> int:
 
     def shutdown(_signum: int, _frame: FrameType | None) -> None:
         print("nisse-review: stopping", file=sys.stderr)
-        built.scheduler.stop()
+        built.stop()
 
     signal.signal(signal.SIGINT, shutdown)
     signal.signal(signal.SIGTERM, shutdown)

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

@@ -18,22 +18,24 @@ comment quiets it until their `review` (R5), which asks for a fresh review.
 Crash safety (L8): an item found ``running`` was interrupted. Whether its
 review got posted is asked of Gitea, never assumed; until Gitea can say, the
 item waits rather than risk a second review. A reply carries the key of the
-point it answers, so a re-run never answers it twice.
+point it answers, so a re-run never answers it twice. A stop (SIGTERM) ends
+a run at its next turn, or breaks off its model call, and the item goes back
+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
 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,
@@ -88,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,
@@ -96,13 +98,14 @@ class Scheduler:
         ci_grace_s: float = 120.0,
         operators: frozenset[str] = frozenset(),
         max_rounds: int = 4,
+        cancel: Cancel | None = None,
         clock: Callable[[], float] = time.time,
     ) -> None:
         self._store = store
         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.
@@ -114,6 +117,9 @@ class Scheduler:
         self._clock = clock
         self._said_waiting: set[str] = set()  # "item@head", logged once
         self._stop = threading.Event()
+        # The reviewer's driver's flag: a stop ends a run at its next turn.
+        self._cancel = cancel or Cancel()
+        self._repo_failing: dict[str, str] = {}  # repo: why, logged on change
 
     # -- the loop ---------------------------------------------------------------
 
@@ -123,7 +129,10 @@ class Scheduler:
                 self._stop.wait(self._poll_s)
 
     def stop(self) -> None:
+        """No more rounds, and a run in progress ends at its next turn,
+        back to ready: a stop is no failure of the review."""
         self._stop.set()
+        self._cancel.set()
 
     def round(self) -> bool:
         """The wall, intake, then one run if one is due; True if one ran.
@@ -158,9 +167,14 @@ class Scheduler:
             except GiteaError as exc:
                 # One repository Gitea won't list (an empty one, one without
                 # pull requests) holds up none of the others; its items stay
-                # as they are.
+                # as they are. Said when it starts, or the reason changes.
-                self._oplog.emit("intake.repo_failed", cause=str(exc)[:300], repo=repo)
+                why = str(exc)[:300]
+                if self._repo_failing.get(repo) != why:
+                    self._repo_failing[repo] = why
+                    self._oplog.emit("intake.repo_failed", cause=why, repo=repo)
                 continue
+            if self._repo_failing.pop(repo, None) is not None:
+                self._oplog.emit("intake.repo_ok", cause="listed again", repo=repo)
             seen: set[str] = set()
             for pr in prs:
                 key = item_id(repo, pr.number)
@@ -372,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,
-                known.repo,
-                {"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.
@@ -392,6 +398,9 @@ class Scheduler:
                     "review.partial", cause=outcome.note, work_item=item.id
                 )
                 tool, summary = "post_review", PARTIAL.format(n=len(draft.comments))
+            elif outcome.kind == "stopped":
+                self._interrupted(item, known, outcome.note)
+                return
             elif outcome.kind != "wait":
                 self._failed(item, known, pr, outcome.note or outcome.kind)
                 return
@@ -404,6 +413,10 @@ class Scheduler:
                 posted = self._reviewer.post(
                     item.id, known.repo, pr, summary, kept, n=known.posts + 1
                 )
+        except Stopped as exc:
+            # A stop broke off a model call: a stop all the same.
+            self._interrupted(item, known, str(exc))
+            return
         except GiteaError as exc:
             self._failed(item, known, None, f"Gitea: {exc}")
             return
@@ -462,6 +475,18 @@ class Scheduler:
             cause=f"{tool}",
         )
 
+    def _interrupted(self, item: WorkItem, known: PR, why: str) -> None:
+        """A stop ended the run: nothing drafted is posted, no attempt is
+        counted, and it runs again as it was. Replies it posted carry their
+        markers, so the next run doesn't repeat them."""
+        self._oplog.emit("review.interrupted", cause=why[:300], work_item=item.id)
+        self._store.update_work_item(
+            item.id,
+            position=READY,
+            data=data(replace(known, run="")),
+            cause="stopped mid-run",
+        )
+
     def _failed(
         self, item: WorkItem, known: PR, pr: PullRequest | None, why: str
     ) -> None:

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."""
+
+    def __init__(self, gitea: "FakeGitea", root: Path) -> None:
+        self._gitea = gitea
+        self._root = root
+        self.base: dict[str, str] | None = None
+        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():
-            raw = text.encode()
+                (root / path).parent.mkdir(parents=True, exist_ok=True)
-            info = tarfile.TarInfo(f"{top}/{path}")
+                (root / path).write_text(text)
-            info.size = len(raw)
+            root.mkdir(parents=True, exist_ok=True)
-            tar.addfile(info, io.BytesIO(raw))
+        return root
-    return out.getvalue()
 
 
 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
@@ -15,6 +15,7 @@ from nisse_core.knowledge.vault import Vault
 from nisse_core.log.canonical import AssistantText
 from nisse_core.loop.driver import Driver
 from nisse_core.oplog import OpLog
+from nisse_core.state.cancel import Cancel
 from nisse_review.access import AccessRefused, check, reviewed
 from nisse_review.build import StartupError, check_access
 from nisse_review.review import Reviewer, _diff_lines, marker
@@ -38,28 +39,34 @@ def world(
 ) -> dict[str, Any]:
     gitea = gitea or FakeGitea(repos={REPO: [pr()]})
     oplog = OpLog("nisse-review", io.StringIO())
+    cancel = more.setdefault("cancel", Cancel())  # the driver's and the scheduler's
     store = StateStore(tmp_path / "state.db", oplog=oplog)
     vault = Vault(tmp_path / "vault", {"memory/nisse-review": "rw"}, agent="r")
     reviewer = Reviewer(
         store,
         gitea,  # type: ignore[arg-type]
-        Driver(model, "m"),  # type: ignore[arg-type]
+        Driver(model, "m", cancel=cancel),  # type: ignore[arg-type]
         account=gitea.account,
         gitea_url="https://gitea.test",
         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):
@@ -210,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:
@@ -507,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
@@ -562,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
@@ -617,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 == []

nisse-review/tests/test_review_stop.py

@@ -0,0 +1,170 @@
+"""A clean stop (L2): SIGTERM ends a review run at its next turn, or breaks
+off its model call, and the item goes back to ready with no failure counted;
+a repository Gitea won't list is logged when that changes, not every round."""
+
+import io
+import json
+from collections.abc import Callable
+from pathlib import Path
+from typing import Any
+
+from review_fakes import FakeGitea, Scripted, call, pr
+from test_review import REPO, REVIEW, item, world
+
+from nisse_core.models.interruptible import Interruptible, Stopped
+from nisse_core.oplog import OpLog
+from nisse_review.build import build
+from nisse_review.config import ReviewConfig
+from nisse_review.review import reply_marker
+from nisse_review.state import READY, REVIEWED, pr_of
+
+
+class Stopping(Scripted):
+    """Scripted, but at call ``at`` (0-based) it first runs ``then``: a stop
+    arriving while the model works on that turn."""
+
+    def __init__(self, *turns: list[Any], at: int) -> None:
+        super().__init__(*turns)
+        self.at = at
+        self.then: Callable[[], None] = lambda: None
+        self.calls = 0
+
+    def complete(self, events, tools, model, system=None):  # type: ignore[no-untyped-def]
+        now, self.calls = self.calls, self.calls + 1
+        if now == self.at:
+            self.then()
+        return super().complete(events, tools, model, system)
+
+
+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 logged(w: dict[str, Any]) -> io.StringIO:
+    out = io.StringIO()
+    w["scheduler"]._oplog = OpLog("t", out)
+    return out
+
+
+def test_a_stop_ends_the_run_at_its_next_turn(tmp_path: Path) -> None:
+    model = Stopping(*REVIEW, at=1)
+    w = world(tmp_path, model)
+    model.then = w["scheduler"].stop
+    out = logged(w)
+    assert w["scheduler"].round() is True
+    assert model.calls == 2  # the turn in flight finished; no third
+    assert (item(w).position, pr_of(item(w)).attempts) == (READY, 0)
+    assert pr_of(item(w)).run == "" and w["gitea"].posted == []
+    assert len(events(out, "review.interrupted")) == 1
+    assert events(out, "review.failed") == []
+    w["scheduler"].run_forever()  # stopped: returns at once
+
+
+def test_a_stop_breaks_off_the_model_call_in_flight(tmp_path: Path) -> None:
+    model = Stopping(*REVIEW, at=1)
+    wrapped = Interruptible(model)
+    w = world(tmp_path, wrapped)  # type: ignore[arg-type]
+
+    def sigterm() -> None:  # what Built.stop does, in the signal handler
+        w["scheduler"].stop()
+        wrapped.interrupt()
+
+    model.then = sigterm
+    out = logged(w)
+    assert w["scheduler"].round() is True
+    assert (item(w).position, pr_of(item(w)).attempts) == (READY, 0)
+    assert w["gitea"].posted == []
+    [said] = events(out, "review.interrupted")
+    assert "broke off the model call" in said["cause"]
+
+
+def test_outside_a_model_call_interrupt_does_nothing() -> None:
+    wrapped = Interruptible(Scripted([]))
+    wrapped.interrupt()  # nothing in flight: no Stopped
+    assert wrapped.complete([], None, "m") == []
+    wrapped.interrupt()  # the call is over: still nothing
+    assert wrapped.offered == [set()]  # the rest reaches the client
+
+
+def test_stopped_is_no_failure_handlers_would_take() -> None:
+    assert not issubclass(Stopped, Exception)
+
+
+def test_replies_posted_before_a_stop_are_not_posted_again(tmp_path: Path) -> None:
+    model = Stopping(
+        *REVIEW,
+        [call("reply", point="P1", text="Tabs never occur here.")],
+        [call("reply", point="P2", text="It counts words, yes.")],  # stopped here
+        [call("reply", point="P1", text="It counts words, yes.")],
+        [call("done")],
+        at=5,
+    )
+    gitea = FakeGitea(repos={REPO: [pr()]})
+    w = world(tmp_path, model, gitea, operators=frozenset({"don"}))
+    model.then = w["scheduler"].stop
+    assert w["scheduler"].round() is True
+    first = w["gitea"].say("nisse-dev", "Why flag split()?")
+    second = w["gitea"].say("nisse-dev", "Does it count words?")
+    assert w["scheduler"].round() is True  # replied to P1, then stopped
+    assert item(w).position == READY
+    [(_, _, posted)] = w["gitea"].comments
+    assert reply_marker(f"comment-{first}") in posted
+    w["scheduler"]._stop.clear()
+    w["scheduler"]._cancel.clear()
+    assert w["scheduler"].round() is True  # only the second is left
+    bodies = [body for _, _, body in w["gitea"].comments]
+    assert sum(reply_marker(f"comment-{first}") in b for b in bodies) == 1
+    assert sum(reply_marker(f"comment-{second}") in b for b in bodies) == 1
+    assert item(w).position == REVIEWED
+
+
+# --- a repository Gitea won't list ---------------------------------------------
+
+
+def test_a_repository_that_keeps_failing_is_logged_once(tmp_path: Path) -> None:
+    gitea = FakeGitea(repos={"a/empty": [], REPO: []}, unlisted={"a/empty"})
+    w = world(tmp_path, Scripted(), gitea)
+    out = logged(w)
+    for _ in range(3):
+        w["scheduler"].round()
+    assert len(events(out, "intake.repo_failed")) == 1
+    gitea.unlisted.clear()
+    w["scheduler"].round()
+    w["scheduler"].round()
+    [ok] = events(out, "intake.repo_ok")
+    assert ok["repo"] == "a/empty"
+    gitea.unlisted.add("a/empty")  # failing again is news again
+    w["scheduler"].round()
+    assert len(events(out, "intake.repo_failed")) == 2
+
+
+def test_build_wires_a_stop_to_every_model_call(tmp_path: Path) -> None:
+    config = ReviewConfig.from_tree(
+        {
+            "state_db": str(tmp_path / "state.db"),
+            "llm": {"base_url": "http://llm.test", "token": "t"},
+            "roles": {"reviewer": "m", "expert": "e"},
+            "gitea": {
+                "url": "https://gitea.test",
+                "token": "t",
+                "operators": ["don"],
+            },
+            "repositories": [REPO],
+        },
+        {},
+    )
+    built = build(
+        config,
+        gitea=FakeGitea(repos={REPO: []}),  # type: ignore[arg-type]
+        client=Scripted(),
+        oplog=OpLog("t", io.StringIO()),
+    )
+    try:
+        driver = built.scheduler._reviewer._driver
+        assert driver._client is built.model
+        assert built.scheduler._reviewer._expert._client is built.model
+        built.stop()  # no call in flight: nothing raised
+        assert driver._cancel.requested and built.scheduler._stop.is_set()
+    finally:
+        built.close()