From 3813ad55d6cffacb37160a2cf23161330eec3a99 Mon Sep 17 00:00:00 2001 From: Don Garrett Date: Sat, 17 Oct 2026 15:46:44 +0000 Subject: [PATCH] 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/// (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) Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk --- docs/Architecture.md | 6 +- docs/Development-Plan.md | 19 +- nisse-core/src/nisse_core/gitea.py | 6 +- nisse-review/README.md | 5 +- nisse-review/src/nisse_review/build.py | 14 +- nisse-review/src/nisse_review/clones.py | 253 +++++++++++++++++++++ nisse-review/src/nisse_review/reader.py | 105 ++++----- nisse-review/src/nisse_review/scheduler.py | 24 +- nisse-review/tests/review_fakes.py | 48 ++-- nisse-review/tests/test_clones.py | 237 +++++++++++++++++++ nisse-review/tests/test_review.py | 64 +++--- 11 files changed, 642 insertions(+), 139 deletions(-) create mode 100644 nisse-review/src/nisse_review/clones.py create mode 100644 nisse-review/tests/test_clones.py diff --git a/docs/Architecture.md b/docs/Architecture.md index 1cae445..a87329c 100644 --- a/docs/Architecture.md +++ b/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. diff --git a/docs/Development-Plan.md b/docs/Development-Plan.md index afd6bbf..b4af0b1 100644 --- a/docs/Development-Plan.md +++ b/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///` (`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` diff --git a/nisse-core/src/nisse_core/gitea.py b/nisse-core/src/nisse_core/gitea.py index ac52267..91358fe 100644 --- a/nisse-core/src/nisse_core/gitea.py +++ b/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}")) diff --git a/nisse-review/README.md b/nisse-review/README.md index eb8e9ab..896044f 100644 --- a/nisse-review/README.md +++ b/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 - archives: the run's executor. +- `clones`: each repository as a git clone kept between runs (the latest + 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. diff --git a/nisse-review/src/nisse_review/build.py b/nisse-review/src/nisse_review/build.py index 2b2e461..7112765 100644 --- a/nisse-review/src/nisse_review/build.py +++ b/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" - scratch.mkdir(exist_ok=True) + # The repositories' clones, kept between runs (R3). + 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, diff --git a/nisse-review/src/nisse_review/clones.py b/nisse-review/src/nisse_review/clones.py new file mode 100644 index 0000000..52ab0da --- /dev/null +++ b/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///head/ the clone, checked out at the PR's head + root///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 diff --git a/nisse-review/src/nisse_review/reader.py b/nisse-review/src/nisse_review/reader.py index 51d0f09..e0493c0 100644 --- a/nisse-review/src/nisse_review/reader.py +++ b/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 -API call each, unpacked into a scratch folder on the agent host and read in -Python: no git, no shell, nothing a model chose is ever run. Issues come -from the Gitea API, for the same repository. +Code comes from a clone kept on the agent host (``clones``), checked out at +the PR's head and, when asked, its base branch, and is read in Python: git +runs only the harness's fixed commands, and nothing a model chose is ever +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: - """``refs``: what ``head`` and ``base`` name (commits or branches). - ``scratch``: a folder of this run's own, removed by ``close``.""" +class Trees(Protocol): + """Where a run's code is: ``tree("head")`` or ``tree("base")``, each a + folder of plain files, made on first use (``clones.Checkout``).""" - def __init__( - self, - gitea: GiteaClient, - repo: str, - refs: Mapping[str, str], - scratch: Path, - ) -> None: + def tree(self, ref: str) -> Path: ... + + +class Sources(Protocol): + """Where runs get their code from (``clones.Clones``).""" + + 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._scratch = scratch - self._trees: dict[str, Path] = {} - - def close(self) -> None: - shutil.rmtree(self._scratch, ignore_errors=True) + self._trees = trees def tree(self, ref: str) -> Path: - """The repository at ``ref``, unpacked on first use.""" - if ref not in self._refs: + if ref not in ("head", "base"): raise ValueError(f"ref is head or base, not {ref!r}") - if ref not in self._trees: - 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] + return self._trees.tree(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( - str(p.relative_to(root)) - for p in start.rglob("*") - if p.is_file() and not p.is_symlink() - ) + found = sorted(str(p.relative_to(root)) for p in _files(start, root)) 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("*")): - if not path.is_file() or path.is_symlink(): - continue + for path in sorted(_files(root, root)): 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: - """Gitea's archive, without its top folder; only plain files and - folders, each inside ``root``.""" - with tarfile.open(fileobj=io.BytesIO(archive), mode="r:gz") as tar: - for member in tar.getmembers(): - if not (member.isfile() or member.isdir()): - continue # links and devices are never followed - parts = member.name.split("/", 1) - if len(parts) < 2 or not parts[1]: - continue - inner = _clean(parts[1]) - if inner is None: - continue - target = root / inner - if member.isdir(): - 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()) +def _files(under: Path, root: Path) -> list[Path]: + """The plain files under a folder; git's own folder is never walked.""" + found: list[Path] = [] + for folder, dirs, files in os.walk(under): + here = Path(folder) + if here == root: + dirs[:] = [d for d in dirs if d != ".git"] + found.extend( + here / name + for name in files + if not (here == root and name == ".git") + and (here / name).is_file() + and not (here / name).is_symlink() + ) + return found diff --git a/nisse-review/src/nisse_review/scheduler.py b/nisse-review/src/nisse_review/scheduler.py index 70c6ca6..a4ae166 100644 --- a/nisse-review/src/nisse_review/scheduler.py +++ b/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-")) - repo = Repository( - self._gitea, - known.repo, - {"head": pr.head_sha, "base": pr.base}, - scratch, + trees = self._sources.open(known.repo, pr.number, pr.head_sha, pr.base) + repo = Repository(self._gitea, known.repo, trees) + outcome, draft = self._reviewer.run( + item.id, pr, repo, kind=kind, known=known, said=said ) - 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. diff --git a/nisse-review/tests/review_fakes.py b/nisse-review/tests/review_fakes.py index 070c766..96d65de 100644 --- a/nisse-review/tests/review_fakes.py +++ b/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: - out = io.BytesIO() - with tarfile.open(fileobj=out, mode="w:gz") as tar: - for path, text in files.items(): - raw = text.encode() - info = tarfile.TarInfo(f"{top}/{path}") - info.size = len(raw) - tar.addfile(info, io.BytesIO(raw)) - return out.getvalue() +class FakeSources: + """The repository's code as plain folders: ``gitea.files`` at head, + 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(): + (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", ()) diff --git a/nisse-review/tests/test_clones.py b/nisse-review/tests/test_clones.py new file mode 100644 index 0000000..e18fad3 --- /dev/null +++ b/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 diff --git a/nisse-review/tests/test_review.py b/nisse-review/tests/test_review.py index 7d916b6..16fa28b 100644 --- a/nisse-review/tests/test_review.py +++ b/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" - scratch.mkdir() + sources = FakeSources(gitea, tmp_path / "trees") 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 == []