From 40bab7eefcb101e21cdcc470a2a9fb059295b546 Mon Sep 17 00:00:00 2001 From: Robert Reynolds Date: Fri, 31 Jul 2026 17:30:27 -0400 Subject: [PATCH] worktrees: provision declared gitignored dependency dirs git worktree add starts clean, which is the point -- and also the trap: anything gitignored is absent from the new worktree, so a task whose build or check depends on node_modules, a vendored toolchain, or a data cache fails before the worker types a word, and the failure output points at the build, not at the empty directory that caused it. New run-level manifest field: worktree_provision, a list of repo-relative paths cloned from the primary checkout into each fresh worktree during taskdir preparation. Copying uses cp -Rc first (an APFS copy-on-write clone, near-instant for a 90k-file node_modules on macOS) with plain cp -R as the portable fallback. Declarations are validated where they can fail loudest: parsing rejects non-string entries, absolute paths (POSIX and Windows forms), '..' segments, and the field without worktrees mode or a repo; at provision time, realpath re-proves the source resolves inside the repo (so a symlinked entry cannot pull from outside it), a missing source fails taskdir preparation rather than handing the worker a half-provisioned tree, and an existing destination is left alone. Ten new tests cover the field's parsing and the provisioning behaviour, including the symlink-escape rejection and the existing-destination no-overwrite rule. Full suite matches the baseline on this host (one pre-existing, unrelated test_deliverables failure reproduces identically with and without this change -- same one noted on #85). --- README.md | 1 + ringer.py | 83 +++++++++++++- tests/test_worktree_provision.py | 189 +++++++++++++++++++++++++++++++ 3 files changed, 272 insertions(+), 1 deletion(-) create mode 100644 tests/test_worktree_provision.py diff --git a/README.md b/README.md index 9917d5f0..3a28715d 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,7 @@ Each task gets its own directory, its own worker, its own log, and its own verdi | `verified` | One plain-English sentence saying what the check proves — shown on the results page next to "finished & checked" | | `full_access` | Worker runs unsandboxed — required for workers that spawn their own sub-workers; must also be enabled in config | | `worktrees` (run-level) | Give each task an isolated git worktree of `repo` so parallel workers can't collide | +| `worktree_provision` (run-level) | Repo-relative gitignored dirs (e.g. `web/node_modules`) cloned into each fresh worktree, so builds that need them work; a declared path that can't be provisioned fails task preparation loudly | > **Worktree footgun:** on PASS the task's worktree is removed — including anything written inside it. In worktrees mode, worker logs live outside task worktrees in `workdir/logs/`; have workers write deliverables outside the worktree too, or have your `check` copy artifacts out before it exits 0. diff --git a/ringer.py b/ringer.py index 062615b7..1d73286a 100755 --- a/ringer.py +++ b/ringer.py @@ -40,7 +40,7 @@ from html import escape as html_escape from http import HTTPStatus from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer -from pathlib import Path +from pathlib import Path, PureWindowsPath from typing import Any, Iterable @@ -1720,6 +1720,7 @@ class Manifest: repo: Path | None tasks: tuple[TaskSpec, ...] source_path: Path | None = None + worktree_provision: tuple[str, ...] = () @classmethod def from_path(cls, path: Path) -> "Manifest": @@ -1735,6 +1736,7 @@ def from_path(cls, path: Path) -> "Manifest": repo=manifest.repo, tasks=manifest.tasks, source_path=path, + worktree_provision=manifest.worktree_provision, ) @classmethod @@ -1774,6 +1776,26 @@ def from_obj(cls, obj: dict[str, Any]) -> "Manifest": "task key(s) collide with reserved worktree logs directory " f"'logs': {', '.join(collisions)}" ) + provision_raw = obj.get("worktree_provision", []) + if not isinstance(provision_raw, list) or not all( + isinstance(item, str) and item.strip() for item in provision_raw + ): + raise ValueError("worktree_provision must be a list of non-empty strings") + worktree_provision = tuple(item.strip() for item in provision_raw) + if worktree_provision: + if not worktrees: + raise ValueError("worktree_provision requires worktrees: true") + if repo is None: + raise ValueError("worktree_provision requires repo") + for rel in worktree_provision: + if ( + Path(rel).is_absolute() + or PureWindowsPath(rel).is_absolute() + or ".." in Path(rel).parts + ): + raise ValueError( + f"worktree_provision paths must be repo-relative without '..': {rel!r}" + ) return cls( run_name=run_name, workdir=workdir, @@ -1781,6 +1803,7 @@ def from_obj(cls, obj: dict[str, Any]) -> "Manifest": worktrees=worktrees, repo=repo, tasks=tasks, + worktree_provision=worktree_provision, ) def with_max_parallel(self, value: int | None) -> "Manifest": @@ -1796,6 +1819,7 @@ def with_max_parallel(self, value: int | None) -> "Manifest": repo=self.repo, tasks=self.tasks, source_path=self.source_path, + worktree_provision=self.worktree_provision, ) @@ -8920,10 +8944,67 @@ async def _prepare_taskdir(self, runtime: TaskRuntime) -> tuple[bool, str | None message = stdout.decode("utf-8", errors="replace") append_text(runtime.log_path, f"[ringer.py] git worktree add failed:\n{message}\n") return False, message.strip() or "git worktree add failed" + provision_error = await self._provision_worktree_deps(runtime, taskdir) + if provision_error is not None: + append_text(runtime.log_path, f"[ringer.py] {provision_error}\n") + return False, provision_error return True, None taskdir.mkdir(parents=True, exist_ok=True) return True, None + async def _provision_worktree_deps( + self, runtime: TaskRuntime, taskdir: Path + ) -> str | None: + """Clone declared gitignored dependency dirs into a fresh worktree. + + `git worktree add` starts clean: anything gitignored -- node_modules, + a vendored toolchain, a build cache -- is absent, so a task whose + build or check needs it fails before the worker types a word. + Manifests name those paths in `worktree_provision` (repo-relative), + and each is cloned from the primary checkout into the new worktree. + + Returns None on success, or an error string: a declared dependency + that cannot be provisioned fails taskdir preparation loudly rather + than handing the worker a half-provisioned tree whose downstream + test failures point everywhere except here. Parsing already rejected + absolute paths and `..`; realpath re-proves the source stays inside + the repo so a symlinked entry cannot pull from outside it. + """ + for rel in self.manifest.worktree_provision: + repo_root = Path(os.path.realpath(self.manifest.repo)) + src = Path(os.path.realpath(repo_root / rel)) + if src != repo_root and repo_root not in src.parents: + return f"worktree_provision path escapes the repo: {rel!r}" + if not src.exists(): + return f"worktree_provision path not found in repo: {rel}" + dst = taskdir / rel + if dst.exists(): + continue + dst.parent.mkdir(parents=True, exist_ok=True) + # cp -Rc asks APFS for a copy-on-write clone, which makes a + # 90k-file node_modules near-instant on macOS; plain -R is the + # portable fallback everywhere else. + for args in ( + ("cp", "-Rc", str(src), str(dst)), + ("cp", "-R", str(src), str(dst)), + ): + proc = await asyncio.create_subprocess_exec( + *args, + stdin=asyncio.subprocess.DEVNULL, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.STDOUT, + ) + await proc.communicate() + if proc.returncode == 0: + append_text( + runtime.log_path, + f"[ringer.py] provisioned {rel} into the worktree via cp {args[1]}\n", + ) + break + else: + return f"failed to provision {rel} into the worktree" + return None + async def _cleanup_worktree_on_pass(self, runtime: TaskRuntime) -> None: if not (self.manifest.worktrees and self.manifest.repo is not None): return diff --git a/tests/test_worktree_provision.py b/tests/test_worktree_provision.py new file mode 100644 index 00000000..05ebdfae --- /dev/null +++ b/tests/test_worktree_provision.py @@ -0,0 +1,189 @@ +#!/usr/bin/env python3 +"""worktree_provision: declared gitignored dependency dirs for fresh worktrees. + +`git worktree add` starts clean, so anything gitignored (node_modules, build +caches) is absent and a task whose build or check depends on it fails before +the worker starts. Manifests declare those paths; the runner clones each from +the primary checkout into the new worktree, and a declaration that cannot be +honored fails taskdir preparation loudly instead of handing the worker a +half-provisioned tree. +""" +from __future__ import annotations + +import asyncio +import json +import os +import sys +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT)) + +from ringer import Manifest, RingerRunner, TaskRuntime, TaskSpec # noqa: E402 + + +def _runtime(tmp: Path) -> TaskRuntime: + log = tmp / "worker.log" + log.touch() + return TaskRuntime( + task=TaskSpec.from_obj({"key": "t", "spec": "s", "check": "true"}), + taskdir=tmp, + log_path=log, + ) + + +def _manifest(tmp: Path, repo: Path, **extra) -> Manifest: + src = tmp / "manifest.json" + src.write_text( + json.dumps( + { + "run_name": "t", + "workdir": str(tmp), + "max_parallel": 1, + "worktrees": True, + "repo": str(repo), + "tasks": [{"key": "t", "spec": "s", "check": "true"}], + **extra, + } + ), + encoding="utf-8", + ) + return Manifest.from_path(src) + + +def _runner(tmp: Path, repo: Path, **extra) -> RingerRunner: + runner = RingerRunner.__new__(RingerRunner) + runner.manifest = _manifest(tmp, repo, **extra) + return runner + + +class ManifestFieldTests(unittest.TestCase): + def test_declared_field_is_retained(self) -> None: + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + manifest = _manifest(tmp, tmp, worktree_provision=["node_modules"]) + self.assertEqual(manifest.worktree_provision, ("node_modules",)) + + def test_absent_field_defaults_empty(self) -> None: + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + self.assertEqual(_manifest(tmp, tmp).worktree_provision, ()) + + def test_field_must_be_a_list_of_nonempty_strings(self) -> None: + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + for bad in (["node_modules", 7], "node_modules", [""]): + with self.assertRaisesRegex(ValueError, "list of non-empty strings"): + _manifest(tmp, tmp, worktree_provision=bad) + + def test_absolute_and_traversal_paths_are_rejected(self) -> None: + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + for bad in ("/etc", "C:\\deps", "../outside", "a/../../b"): + with self.assertRaisesRegex(ValueError, "repo-relative"): + _manifest(tmp, tmp, worktree_provision=[bad]) + + def test_field_requires_worktrees_mode(self) -> None: + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + src = tmp / "manifest.json" + src.write_text( + json.dumps( + { + "run_name": "t", + "workdir": str(tmp), + "worktrees": False, + "tasks": [{"key": "t", "spec": "s", "check": "true"}], + "worktree_provision": ["node_modules"], + } + ), + encoding="utf-8", + ) + with self.assertRaisesRegex(ValueError, "requires worktrees"): + Manifest.from_path(src) + + +class ProvisionBehaviourTests(unittest.TestCase): + def test_declared_dir_is_cloned_with_contents(self) -> None: + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + repo, wt = tmp / "repo", tmp / "wt" + (repo / "web" / "node_modules" / "pkg").mkdir(parents=True) + (repo / "web" / "node_modules" / "pkg" / "index.js").write_text("x") + wt.mkdir() + runner = _runner(tmp, repo, worktree_provision=["web/node_modules"]) + err = asyncio.run(runner._provision_worktree_deps(_runtime(tmp), wt)) + self.assertIsNone(err) + self.assertEqual( + (wt / "web" / "node_modules" / "pkg" / "index.js").read_text(), + "x", + "contents must be cloned, not just the directory created", + ) + + def test_absent_field_is_a_noop(self) -> None: + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + repo, wt = tmp / "repo", tmp / "wt" + repo.mkdir() + wt.mkdir() + err = asyncio.run( + _runner(tmp, repo)._provision_worktree_deps(_runtime(tmp), wt) + ) + self.assertIsNone(err) + self.assertEqual(list(wt.iterdir()), []) + + def test_missing_source_fails_preparation_loudly(self) -> None: + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + repo, wt = tmp / "repo", tmp / "wt" + repo.mkdir() + wt.mkdir() + err = asyncio.run( + _runner(tmp, repo, worktree_provision=["nope"]) + ._provision_worktree_deps(_runtime(tmp), wt) + ) + self.assertIsNotNone( + err, "a half-provisioned worktree must not be reported as success" + ) + self.assertIn("nope", err) + + def test_existing_destination_is_left_alone(self) -> None: + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + repo, wt = tmp / "repo", tmp / "wt" + (repo / "deps").mkdir(parents=True) + (repo / "deps" / "new.txt").write_text("from repo") + (wt / "deps").mkdir(parents=True) + (wt / "deps" / "old.txt").write_text("already here") + err = asyncio.run( + _runner(tmp, repo, worktree_provision=["deps"]) + ._provision_worktree_deps(_runtime(tmp), wt) + ) + self.assertIsNone(err) + self.assertTrue((wt / "deps" / "old.txt").exists()) + self.assertFalse( + (wt / "deps" / "new.txt").exists(), + "an existing destination must not be overwritten", + ) + + def test_symlinked_source_may_not_escape_the_repo(self) -> None: + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + repo, wt, outside = tmp / "repo", tmp / "wt", tmp / "outside" + repo.mkdir() + wt.mkdir() + outside.mkdir() + (outside / "secret.txt").write_text("private") + os.symlink(outside, repo / "deps") + err = asyncio.run( + _runner(tmp, repo, worktree_provision=["deps"]) + ._provision_worktree_deps(_runtime(tmp), wt) + ) + self.assertIsNotNone(err, "a symlink out of the repo must be rejected") + self.assertFalse((wt / "deps").exists()) + + +if __name__ == "__main__": + unittest.main()