diff --git a/README.md b/README.md index 9917d5f0..fb3ed149 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_ref` (run-level) | Commit the task worktrees are created at (default `HEAD`). Pin it when the work is anchored to an older immutable baseline — worktree creation runs orchestrator-side and unsandboxed, so this is free, whereas a sandboxed worker told to `git checkout` inside its worktree fails outright (the shared `.git/worktrees` metadata is outside its writable surface) | > **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/docs/MODEL-NOTES.md b/docs/MODEL-NOTES.md index b153b851..6eacfd56 100644 --- a/docs/MODEL-NOTES.md +++ b/docs/MODEL-NOTES.md @@ -406,6 +406,7 @@ checks and raw logs support — no vibes, no worker self-reports. - Both fixture-based checks passed while hiding 2 real-substrate bugs (EXIT-trap lock cleanup deleting a live lock; tilde-expansion in ${var#~/} — fixture used absolute paths). Codex not at fault; checks were fixture-only. Lesson: for ops scripts, add one real-repo scenario to the check or budget an acceptance pass after integration. ## gpt-5.6-sol (codex) +- 2026-07-31 (observer-triad, phase-1 closeout) SCOREBOARD CORRECTION: three FAIL rows stamped `failure_class=model` that day are orchestration/environment-class, not model. (1+2) Both lanes of `bounded-primordial-v4-independent-review` (13:34Z): the spec ordered `git checkout` INSIDE the Ringer worktree, which needs a write to the shared `.git/worktrees//index.lock` — outside workspace-write's writable roots — so the first command failed EPERM. Both workers did exactly the right thing: halted immediately, changed nothing, reported the blocked command and the fix verbatim (~14k tokens each). The check then failed on the absent baseline/report and the rows recorded as model failures. Same defect class as the cttc r1 correction above (sandbox write-boundary vs spec contract); fixed structurally by the manifest-level `worktree_ref` field (worktree creation is orchestrator-side and unsandboxed) and, spec-side, by the clone-into-taskdir pattern. (3) `contract-authority` attempt 1 in the first `adversarial-review` dispatch (22:23Z, FAIL at 9m): the check rejected the draft report, and the retry was still in flight when the whole run died with its parent session (network drop → session death → unhandled SIGHUP; the ghost-run signature that put SIGHUP into the shutdown handler). At most a first-try format slip with the retry mechanism never allowed to finish — the identical three-lane review re-run later that evening went 3/3 first-try, READY×3. Discount all three rows when reading Sol's code-review cell. - 2026-07-28 (meridian, cttc-idea-join-notif — live Apps Script family board, 6 rounds): medium effort 3/3 first-try on code-feature/code-fix (impl 49.5k tok/108s; five-finding fix round 40k/81s; N1 lock restructure 43.5k/86s) — every fix landed exactly per the reviewer's minimal-concrete-edit blocks including comment placement; medium stays the build lane. High effort earned its review tier again: r1 NO-SHIP with 5 findings ALL reproduced by throwaway fixtures (incl. a minor-recipient gap the spec's own invariant demanded and a quota-arithmetic case tied to that night's planned family nudge); r2 caught a NEW HIGH the fix round introduced (script lock held across sendEmail → silent family-write loss, proven via instrumented LockService stub); r3 verified the replacement design with zero-violation lock-state evidence and an explicit trade judgment (105k/73k tokens for r2/r3). This is what high is FOR: last-gate review of live-user deploys. SCOREBOARD CORRECTION: two recorded FAILs on Sol's code-review cell (r1, attempts 1+2) are orchestrator-class, not model — the rows are STAMPED failure_class=model and this note is the correction. The review manifest demanded the report at an absolute path outside the sandbox's writable roots (workspace-write covers [workdir,/tmp,$TMPDIR] only) and passed no writable_roots override; the worker's reports were substance-complete BOTH attempts and were harvested from the taskdir. Same defect class as harness-audit-remediation r1 earlier the same day — the lesson was already in the session record and was reproduced anyway. Do not read Sol's code-review first-try rate at face value until these rows are discounted. Check-craft: (1) absolute-path deliverables from sandboxed workers need writable_roots, or use a RELATIVE taskdir deliverable + harvest-in-check (checks run outside the sandbox — the pattern that fixed r2/r3 here); (2) grep -E with \| alternation silently matches nothing — two false "risk-area-unaddressed" FAILs in the r1 check were this; lint cannot catch your check's own regexes. - 2026-07-15 ringer-self-update run (3 serial tasks, direct-repo-edit mode): code-fix baseline-test repair 1/1 first-try (61k tokens, 1.6m); code-feature self-update mechanism (git fetch/ff-pull/re-exec + HUD staleness restart + 20-test suite) 1/1 first-try at high effort (153k, 8.1m); code-feature signal-contract (all 3 scoreboard surfaces + canonical-route lint enforcement) passed on retry (358k, 13.7m) — attempt 1 died on stale old-column assertions in pre-existing tests it hadn't finished updating; the retry prompt's injected FAIL list was enough to close it out. Lesson: when a task rewrites a display contract, name every test file asserting the old contract in the spec's ownership list AND tell it to update them FIRST. - 2026-07-09 code-feature/code-fix (ringside-overhaul): 4/4 first-try — a ringer.py logging change with tests, a 265-line stdlib backfill CLI (atomic rewrite, dry-run, idempotence all check-verified), a ~1500-line single-file HTML redesign (running-now pills + worker-card grid + multi-expansion refactor, 30KB patch, node --check + contract greps + unittest), and a render-gating change where it correctly UPDATED tests asserting the old behavior instead of gaming the check. Medium/high reasoning, 65–120k tokens/task. diff --git a/ringer.py b/ringer.py index 262286e4..6f209694 100755 --- a/ringer.py +++ b/ringer.py @@ -1727,6 +1727,14 @@ class Manifest: repo: Path | None tasks: tuple[TaskSpec, ...] source_path: Path | None = None + # The commit worktrees are created at. Defaults to HEAD, but a run whose + # candidate is anchored to an older immutable baseline can pin it here — + # worktree creation happens orchestrator-side, UNSANDBOXED, so this is the + # only place a non-HEAD base costs nothing. A sandboxed worker cannot + # `git checkout` inside its worktree (the shared .git/worktrees metadata + # is outside its writable surface), and the workaround it is forced into + # (cloning the repo into its own taskdir) burns tokens on plumbing. + worktree_ref: str = "HEAD" @classmethod def from_path(cls, path: Path) -> "Manifest": @@ -1734,15 +1742,7 @@ def from_path(cls, path: Path) -> "Manifest": if not isinstance(data, dict): raise ValueError("manifest root must be a JSON object") manifest = cls.from_obj(data) - return cls( - run_name=manifest.run_name, - workdir=manifest.workdir, - max_parallel=manifest.max_parallel, - worktrees=manifest.worktrees, - repo=manifest.repo, - tasks=manifest.tasks, - source_path=path, - ) + return dataclass_replace(manifest, source_path=path) @classmethod def from_obj(cls, obj: dict[str, Any]) -> "Manifest": @@ -1769,6 +1769,15 @@ def from_obj(cls, obj: dict[str, Any]) -> "Manifest": if duplicates: raise ValueError(f"duplicate task keys: {', '.join(duplicates)}") worktrees = bool(obj.get("worktrees", False)) + worktree_ref_raw = obj.get("worktree_ref") + if worktree_ref_raw is not None and not isinstance(worktree_ref_raw, str): + raise ValueError("worktree_ref must be a string") + worktree_ref = (worktree_ref_raw or "HEAD").strip() or "HEAD" + if worktree_ref != "HEAD" and not worktrees: + raise ValueError( + "worktree_ref is set but worktrees is false — the ref only pins " + "where task worktrees are created, so enable worktrees or drop it" + ) if worktrees: reserved_logs_dir = (workdir / "logs").resolve() collisions = [] @@ -1788,6 +1797,7 @@ def from_obj(cls, obj: dict[str, Any]) -> "Manifest": worktrees=worktrees, repo=repo, tasks=tasks, + worktree_ref=worktree_ref, ) def with_max_parallel(self, value: int | None) -> "Manifest": @@ -1795,15 +1805,7 @@ def with_max_parallel(self, value: int | None) -> "Manifest": return self if value <= 0: raise ValueError("--max-parallel must be positive") - return Manifest( - run_name=self.run_name, - workdir=self.workdir, - max_parallel=value, - worktrees=self.worktrees, - repo=self.repo, - tasks=self.tasks, - source_path=self.source_path, - ) + return dataclass_replace(self, max_parallel=value) FILE_TEST_OPS = {"-e", "-f", "-s", "-d", "-r", "-w", "-x", "-L"} @@ -9432,8 +9434,11 @@ async def _prepare_taskdir(self, runtime: TaskRuntime) -> tuple[bool, str | None str(self.manifest.repo), "worktree", "add", + # --detach so a branch-name ref cannot fail on "already + # checked out" — every taskdir gets a detached copy either way. + "--detach", str(taskdir), - "HEAD", + self.manifest.worktree_ref, stdin=asyncio.subprocess.DEVNULL, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, @@ -10456,7 +10461,7 @@ async def run_baseline(manifest: Manifest, *, config: AppConfig) -> int: "add", "--detach", str(taskdir), - "HEAD", + manifest.worktree_ref, stdin=asyncio.subprocess.DEVNULL, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, @@ -11123,7 +11128,20 @@ def request_shutdown() -> None: shutdown_started = True task.cancel() - for sig in (signal.SIGINT, signal.SIGTERM): + # SIGHUP is in the set because a run's parent is often an interactive + # agent session: when that session dies (network drop, app crash, closed + # terminal), the shell HUPs the group. Unhandled, the orchestrator died + # mid-flush and left a ghost — state live/unfinished forever, workers + # killed with no ERROR verdicts recorded (observed 2026-07-31, when a + # network interruption took down the session driving a 3-lane review). + # An INHERITED ignore is respected: `nohup ./ringer.py run` means + # "survive hangups", and installing a handler over SIG_IGN would turn + # nohup into a graceful shutdown — the opposite of what it promises. + shutdown_signals = [signal.SIGINT, signal.SIGTERM] + hup = getattr(signal, "SIGHUP", None) # absent on Windows + if hup is not None and signal.getsignal(hup) is not signal.SIG_IGN: + shutdown_signals.append(hup) + for sig in shutdown_signals: with contextlib.suppress(NotImplementedError, RuntimeError): loop.add_signal_handler(sig, request_shutdown) registered_signals.append(sig) diff --git a/tests/test_baseline_mode.py b/tests/test_baseline_mode.py index cacd8560..9e2395a6 100644 --- a/tests/test_baseline_mode.py +++ b/tests/test_baseline_mode.py @@ -240,6 +240,62 @@ def test_task_key_cannot_escape_baseline_scratch_root(self) -> None: self.assertIn("task key escapes the baseline scratch root", output) self.assertIn("0 pass, 0 fail, 1 error of 1 check(s)", output) + def test_baseline_worktrees_honor_the_pinned_worktree_ref(self) -> None: + # Baseline checks must run against the SAME tree the workers will get: + # a manifest pinned to an older commit whose baseline ran at HEAD + # would judge check-craft against the wrong sources. + import asyncio + import contextlib + import importlib.util + import io + + spec = importlib.util.spec_from_file_location("ringer_baseline_ref_test", ROOT / "ringer.py") + assert spec is not None and spec.loader is not None + ringer = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = ringer + spec.loader.exec_module(ringer) + + with tempfile.TemporaryDirectory() as temp_root: + root = Path(temp_root) + repo = root / "repo" + init_git_repo(repo) + env = os.environ.copy() + env.update({"GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t"}) + sha1 = subprocess.run( + ["git", "-C", str(repo), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + ).stdout.strip() + (repo / "README.md").write_text("changed at head\n", encoding="utf-8") + subprocess.run( + ["git", "-C", str(repo), "commit", "--quiet", "-am", "second"], + check=True, env=env, capture_output=True, + ) + + manifest = ringer.Manifest.from_obj( + { + "run_name": "baseline-pinned-ref", + "workdir": str(root / "work"), + "repo": str(repo), + "worktrees": True, + "worktree_ref": sha1, + "tasks": [ + { + "key": "pinned", + "spec": "Placeholder; baseline spawns nothing.", + # Passes ONLY in the first commit's tree. + "check": "grep -q 'hello baseline' README.md", + } + ], + } + ) + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + rc = asyncio.run(ringer.run_baseline(manifest, config=None)) + output = buffer.getvalue() + self.assertEqual(0, rc, output) + self.assertIn("1 pass, 0 fail, 0 error of 1 check(s)", output) + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/tests/test_lint.py b/tests/test_lint.py index f5ed4e13..f27ebaa5 100644 --- a/tests/test_lint.py +++ b/tests/test_lint.py @@ -86,6 +86,50 @@ def test_task_fields_must_be_strings(self) -> None: with self.assertRaisesRegex(ValueError, r"task key must be a string"): self.manifest([task]) + def test_worktree_ref_defaults_to_head_and_round_trips(self) -> None: + self.assertEqual("HEAD", self.manifest([self.task()]).worktree_ref) + manifest = self.manifest([self.task()], worktrees=True) + self.assertEqual("HEAD", manifest.worktree_ref) + # with_max_parallel must carry the field — it rebuilds the manifest. + obj = { + "run_name": "ref-test", + "workdir": manifest.workdir.as_posix(), + "repo": str(manifest.repo), + "worktrees": True, + "worktree_ref": "bd61708d195d6535b716ec9a8b1974464848b214", + "tasks": [self.task()], + } + parsed = Manifest.from_obj(obj) + self.assertEqual("bd61708d195d6535b716ec9a8b1974464848b214", parsed.worktree_ref) + self.assertEqual( + "bd61708d195d6535b716ec9a8b1974464848b214", + parsed.with_max_parallel(4).worktree_ref, + ) + + def test_worktree_ref_requires_worktrees(self) -> None: + with self.assertRaisesRegex(ValueError, r"worktree_ref is set but worktrees is false"): + Manifest.from_obj( + { + "run_name": "ref-test", + "workdir": "/tmp/ref-test-work", + "worktree_ref": "bd61708", + "tasks": [self.task()], + } + ) + + def test_worktree_ref_must_be_a_string(self) -> None: + with self.assertRaisesRegex(ValueError, r"worktree_ref must be a string"): + Manifest.from_obj( + { + "run_name": "ref-test", + "workdir": "/tmp/ref-test-work", + "worktrees": True, + "repo": "/tmp", + "worktree_ref": 123, + "tasks": [self.task()], + } + ) + def test_w1_unverifiable_check(self) -> None: manifest = self.manifest([self.task(check="echo ok && echo done")]) self.assertHasFinding( diff --git a/tests/test_ringer.py b/tests/test_ringer.py index 16fa3ee9..487df029 100644 --- a/tests/test_ringer.py +++ b/tests/test_ringer.py @@ -366,6 +366,142 @@ def test_second_signal_during_shutdown_does_not_cancel_cleanup(self) -> None: self.assertEqual(state["state"], "finished") self.assertEqual(state["tasks"][0]["status"], "fail") + def spawn_sleeping_run( + self, name: str, *, preexec_fn=None + ) -> tuple[subprocess.Popen[str], int]: + """Start a run whose worker sleeps, and wait for the worker pid.""" + manifest = self.write_manifest( + name, + self.manifest( + name, + { + "key": "term", + "engine": "sleep_then_write", + "spec": "Sleep until signalled.", + "expect_files": ["out.txt"], + "timeout_s": 30, + "check": 'test "$(cat out.txt 2>/dev/null)" = done', + }, + ), + ) + cmd = [ + sys.executable, + "-B", + str(RINGER_PATH), + "--config", + str(self.config_path), + "run", + str(manifest), + "--no-dashboard", + "--identity", + "test-runner", + ] + env = os.environ.copy() + env["PYTHONDONTWRITEBYTECODE"] = "1" + env["RINGER_NO_SELF_UPDATE"] = "1" + proc = subprocess.Popen( + cmd, + cwd=ROOT, + env=env, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + preexec_fn=preexec_fn, + ) + worker_pid_path = self.root / f"work-{name}" / "term" / "worker.pid" + deadline = time.time() + 10 + while time.time() < deadline and not worker_pid_path.exists(): + time.sleep(0.05) + self.assertTrue(worker_pid_path.exists()) + return proc, int(worker_pid_path.read_text(encoding="utf-8").strip()) + + @unittest.skipUnless(hasattr(signal, "SIGHUP"), "SIGHUP is a unix signal") + def test_sighup_cleans_up_like_sigterm(self) -> None: + # The signature this prevents: a run's parent agent session dies + # (network drop, closed terminal), the shell HUPs the group, and an + # unhandled orchestrator leaves state live/unfinished forever — a + # ghost run with no ERROR verdicts (observed 2026-07-31). + proc, worker_pid = self.spawn_sleeping_run("sighup") + try: + proc.send_signal(signal.SIGHUP) + stdout, _ = proc.communicate(timeout=10) + finally: + if proc.poll() is None: + proc.kill() + stdout, _ = proc.communicate(timeout=10) + + self.assertEqual(proc.returncode, 130, stdout) + self.assertFalse(self.pid_is_alive(worker_pid), stdout) + state = self.read_final_state() + self.assertTrue(state["finished"]) + self.assertEqual(state["state"], "finished") + + @unittest.skipUnless(hasattr(signal, "SIGHUP"), "SIGHUP is a unix signal") + def test_inherited_sighup_ignore_is_respected(self) -> None: + # `nohup ./ringer.py run` promises hangup survival by setting SIG_IGN; + # installing our handler over that would turn nohup into a graceful + # shutdown. An inherited ignore must stay ignored. + proc, worker_pid = self.spawn_sleeping_run( + "nohup", preexec_fn=lambda: signal.signal(signal.SIGHUP, signal.SIG_IGN) + ) + try: + proc.send_signal(signal.SIGHUP) + time.sleep(1.0) + self.assertIsNone(proc.poll(), "run must survive SIGHUP under nohup semantics") + self.assertTrue(self.pid_is_alive(worker_pid)) + proc.send_signal(signal.SIGTERM) + stdout, _ = proc.communicate(timeout=10) + finally: + if proc.poll() is None: + proc.kill() + stdout, _ = proc.communicate(timeout=10) + + self.assertEqual(proc.returncode, 130, stdout) + state = self.read_final_state() + self.assertTrue(state["finished"]) + + def test_worktree_ref_pins_task_worktrees_to_the_named_commit(self) -> None: + # Two commits; the manifest pins the FIRST. Without the field, task + # worktrees spawn at HEAD and a baseline-anchored candidate forces + # sandboxed workers into checkout gymnastics they cannot perform + # (shared .git/worktrees metadata is outside their writable roots). + repo = self.root / "repo" + git = ["git", "-C", str(repo), "-c", "user.email=t@t", "-c", "user.name=t"] + subprocess.run(["git", "init", "-q", str(repo)], check=True) + (repo / "a.txt").write_text("one", encoding="utf-8") + subprocess.run(git + ["add", "a.txt"], check=True) + subprocess.run(git + ["commit", "-q", "-m", "c1"], check=True) + sha1 = subprocess.run( + git + ["rev-parse", "HEAD"], capture_output=True, text=True, check=True + ).stdout.strip() + (repo / "a.txt").write_text("two", encoding="utf-8") + subprocess.run(git + ["commit", "-q", "-am", "c2"], check=True) + + manifest = self.write_manifest( + "pinned-ref", + self.manifest( + "pinned-ref", + { + "key": "probe", + "engine": "spec_shell", + "spec": "git rev-parse HEAD > out.txt && cat a.txt >> out.txt", + "expect_files": ["out.txt"], + "check": f'test "$(head -1 out.txt)" = {sha1} && test "$(tail -1 out.txt)" = one', + }, + worktrees=True, + repo=str(repo), + ), + ) + # Inject the ref via the manifest file (the from_obj path a real run takes). + data = json.loads(manifest.read_text(encoding="utf-8")) + data["worktree_ref"] = sha1 + manifest.write_text(json.dumps(data, indent=2), encoding="utf-8") + + result = self.run_ringer(manifest) + self.assertEqual(result.returncode, 0, result.stdout) + rows = self.read_rows() + self.assertEqual(["PASS"], [row["verdict"] for row in rows], result.stdout) + def test_custom_shell_engine_substitutes_spec_placeholder(self) -> None: manifest = self.write_manifest( "custom-shell",