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 == []