From f8744a18e1588bdd4131f8675fc555638ab36088 Mon Sep 17 00:00:00 2001 From: Mark Lavercombe Date: Wed, 15 Jul 2026 08:39:42 +1000 Subject: [PATCH 1/2] Name the failure when task setup dies before the worker spawns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failed tasks keep their worktrees (by design, for post-mortems), so a follow-up run with the same run_name collides with the stale taskdir. In the field that surfaced as verdict ERROR at 0.0s with an empty log, empty check output, and no message anywhere — a full diagnosis cycle to discover what one sentence could have said. Three surfaces now carry the reason: - the collision message names the exact unblocking command (git worktree remove --force ) instead of a bare "already exists" - _record_prepare_error stores the reason on the task record as setup_error (included in the run-state payload for the HUD and post-mortems) and appends it to the worker log, which is where log_tail, activity, and humans look first - the run summary prints a "setup failures (no worker was spawned)" section listing each affected task with its reason Test: end-to-end worktrees run against a temp git repo with a pre-existing stale taskdir — asserts the ERROR verdict now travels with the collision message in the summary, the worker log, and the run-state record's setup_error field. Co-Authored-By: Claude Fable 5 --- ringer.py | 29 +++- tests/test_setup_error_diagnostics.py | 213 ++++++++++++++++++++++++++ 2 files changed, 241 insertions(+), 1 deletion(-) create mode 100644 tests/test_setup_error_diagnostics.py diff --git a/ringer.py b/ringer.py index aa75f84..1f32fc0 100755 --- a/ringer.py +++ b/ringer.py @@ -1052,6 +1052,10 @@ class TaskRuntime: last_check_returncode: int | None = None last_check_timed_out: bool = False last_check_output: str = "" + # Why task setup failed before any worker could spawn (e.g. a stale + # worktree from a previous failed run). Without this an ERROR verdict at + # 0.0s carries no diagnostics anywhere the operator looks. + setup_error: str | None = None last_worker_command: list[str] = field(default_factory=list) steering: dict[str, Any] | None = None @@ -1245,6 +1249,7 @@ def snapshot(self) -> dict[str, Any]: "check_returncode": runtime.last_check_returncode, "check_timed_out": runtime.last_check_timed_out, "check_output_tail": shorten(runtime.last_check_output, 4000), + "setup_error": runtime.setup_error, "timeout_s": runtime.task.timeout_s, "taskdir": str(runtime.taskdir), "log_path": str(runtime.log_path), @@ -7565,7 +7570,15 @@ async def _prepare_taskdir(self, runtime: TaskRuntime) -> tuple[bool, str | None if self.manifest.worktrees and self.manifest.repo is not None: taskdir.parent.mkdir(parents=True, exist_ok=True) if taskdir.exists(): - return False, f"worktree taskdir already exists: {taskdir}" + # Failed tasks keep their worktrees for post-mortems, so a + # re-run with the same run_name lands here. Name the exact + # command that unblocks it — the bare "already exists" cost a + # full diagnosis cycle in the field. + return False, ( + f"worktree taskdir already exists (left by a previous " + f"failed run?): {taskdir} — remove it with " + f"`git worktree remove --force {taskdir}` and re-run" + ) proc = await asyncio.create_subprocess_exec( "git", "-C", @@ -7635,7 +7648,16 @@ async def _record_prepare_error(self, runtime: TaskRuntime, error: str) -> None: runtime.attempts = 1 runtime.status = "fail" runtime.final_verdict = "ERROR" + runtime.setup_error = error runtime.ended_at_monotonic = time.monotonic() + # The worker log is where every other surface (HUD activity, + # log_tail, post-mortems) looks first — leave the reason there too. + with contextlib.suppress(Exception): + append_text( + runtime.log_path, + f"[ringer.py] task setup failed before any worker could " + f"spawn: {error}\n", + ) verify = VerifyResult( ok=False, check_returncode=None, @@ -8615,6 +8637,11 @@ def print_summary(run_id: str, runtimes: list[TaskRuntime]) -> None: f"{(runtime.final_verdict or ''):<8} {runtime.attempts:>8} " f"{tokens:>10} {runtime.elapsed_s(now):>10.1f}" ) + setup_failures = [r for r in runtimes if r.setup_error] + if setup_failures: + print("\nsetup failures (no worker was spawned):") + for runtime in setup_failures: + print(f" {runtime.task.key}: {runtime.setup_error}") def create_demo_manifest() -> Path: diff --git a/tests/test_setup_error_diagnostics.py b/tests/test_setup_error_diagnostics.py new file mode 100644 index 0000000..11b71bf --- /dev/null +++ b/tests/test_setup_error_diagnostics.py @@ -0,0 +1,213 @@ +#!/usr/bin/env python3 +"""Pre-spawn setup failures must say what happened and how to fix it. + +Observed in the field (2026-07-14): failed tasks keep their worktrees (by +design, for post-mortems), and a follow-up run with the same run_name then +died at 0.0s with verdict ERROR, an empty log, empty check output, and no +message anywhere naming the collision. A full diagnosis cycle later: +`git worktree list` showed the stale taskdir. + +These tests pin the diagnostics: the collision message names the exact +unblocking command, the reason reaches the worker log and the run-state +record, and the summary lists setup failures explicitly. +""" +from __future__ import annotations + +import json +import os +import re +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] + + +def toml_string(value: object) -> str: + return json.dumps(str(value)) + + +def init_git_repo(path: Path) -> None: + path.mkdir(parents=True, exist_ok=True) + env = os.environ.copy() + env.update( + { + "GIT_AUTHOR_NAME": "test", + "GIT_AUTHOR_EMAIL": "test@example.com", + "GIT_COMMITTER_NAME": "test", + "GIT_COMMITTER_EMAIL": "test@example.com", + } + ) + for args in ( + ["git", "-C", str(path), "init", "--quiet"], + ["git", "-C", str(path), "commit", "--allow-empty", "--quiet", "-m", "init"], + ): + subprocess.run(args, check=True, env=env, capture_output=True) + + +class SetupErrorDiagnosticsTests(unittest.TestCase): + def test_stale_worktree_collision_is_named_everywhere(self) -> None: + with tempfile.TemporaryDirectory() as temp_root: + root = Path(temp_root) + home = root / "home" + ringer_home = root / "ringer-home" + state_dir = root / "state" + workdir = root / "work" + repo = root / "repo" + config_path = root / "config.toml" + manifest_path = root / "manifest.json" + + home.mkdir() + ringer_home.mkdir() + init_git_repo(repo) + + # Simulate the stale worktree a previous failed run leaves behind. + stale_taskdir = workdir / "stale-task" + stale_taskdir.mkdir(parents=True) + + config_path.write_text( + "\n".join( + [ + f"state_dir = {toml_string(state_dir)}", + "", + "[eval]", + 'backend = "jsonl"', + f"jsonl_path = {toml_string(root / 'runs.jsonl')}", + "", + "[artifact]", + "enabled = false", + "", + "[engines.mock]", + f"bin = {toml_string(sys.executable)}", + "args_template = [", + f" {toml_string(ROOT / 'engines' / 'mock_worker.py')},", + ' "{spec}",', + "]", + "sandbox_args = []", + "full_access_args = []", + "", + ] + ), + encoding="utf-8", + ) + + manifest_path.write_text( + json.dumps( + { + "run_name": "setup-error-test", + "workdir": str(workdir), + "max_parallel": 1, + "worktrees": True, + "repo": str(repo), + "tasks": [ + { + "key": "stale-task", + "engine": "mock", + "spec": ( + "You are the deterministic mock worker. This spec " + "would succeed if the worker ran — the point of the " + "test is that setup fails first.\n" + "MOCK_FILE: hello.txt\n" + "hello\n" + "MOCK_END" + ), + "check": ( + "grep -q hello hello.txt || " + "{ echo FAIL: hello.txt missing; exit 1; }" + ), + "expect_files": ["hello.txt"], + } + ], + }, + indent=2, + ), + encoding="utf-8", + ) + + env = os.environ.copy() + env["HOME"] = str(home) + env["RINGER_HOME"] = str(ringer_home) + env["XDG_CONFIG_HOME"] = str(root / "xdg-config") + + proc = subprocess.run( + [ + sys.executable, + "ringer.py", + "run", + str(manifest_path), + "--config", + str(config_path), + "--no-dashboard", + "--identity", + "setup-error-test", + ], + cwd=ROOT, + env=env, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=False, + timeout=30, + ) + + combined_output = proc.stdout + proc.stderr + + # Verdict ERROR as before — but no longer naked. + self.assertRegex( + combined_output, + re.compile(r"^stale-task\s+fail\s+ERROR\s+1\s+", re.MULTILINE), + combined_output, + ) + + # The summary lists the setup failure with the unblocking command. + self.assertIn( + "setup failures (no worker was spawned):", combined_output + ) + self.assertIn("worktree taskdir already exists", combined_output) + # macOS tempdirs surface as /var/... but resolve to /private/var/...; + # accept either rendering of the same taskdir. + self.assertTrue( + f"git worktree remove --force {stale_taskdir}" in combined_output + or f"git worktree remove --force {stale_taskdir.resolve()}" + in combined_output, + combined_output, + ) + + # The reason reaches the worker log, where post-mortems look first. + worker_log_candidates = list(workdir.rglob("*.log")) + self.assertTrue(worker_log_candidates, "no worker log written") + logged = "".join( + p.read_text(encoding="utf-8") for p in worker_log_candidates + ) + self.assertIn( + "task setup failed before any worker could spawn", logged + ) + + # The run-state record carries setup_error for the HUD/post-mortem. + state_files = [ + p + for p in state_dir.rglob("*.json") + if "stale-task" in p.read_text(encoding="utf-8", errors="replace") + ] + self.assertTrue(state_files, "no run state file mentions the task") + found_setup_error = False + for state_file in state_files: + data = json.loads(state_file.read_text(encoding="utf-8")) + tasks = data.get("tasks") if isinstance(data, dict) else None + for task in tasks or []: + if ( + task.get("key") == "stale-task" + and "worktree taskdir already exists" + in (task.get("setup_error") or "") + ): + found_setup_error = True + self.assertTrue( + found_setup_error, + f"setup_error missing from run state: {state_files}", + ) + + +if __name__ == "__main__": + unittest.main(verbosity=2) From 8a94f2b7fb715f950e999bd302ca973b1f0aa062 Mon Sep 17 00:00:00 2001 From: Jonathan Edwards Date: Thu, 16 Jul 2026 12:00:40 -0400 Subject: [PATCH 2/2] Only print the worktree-remove command when it will actually work A linked worktree has a .git file; only then is `git worktree remove` the right recovery, now printed repo-qualified and shell-quoted so it is paste-safe from anywhere. A plain directory in the way gets named as exactly that. Tests cover both branches with a real registered stale worktree and a bare directory. Co-Authored-By: Claude Fable 5 --- ringer.py | 20 ++++++-- tests/test_ringer.py | 2 +- tests/test_setup_error_diagnostics.py | 67 ++++++++++++++++++++------- 3 files changed, 67 insertions(+), 22 deletions(-) diff --git a/ringer.py b/ringer.py index 355aa53..5579162 100755 --- a/ringer.py +++ b/ringer.py @@ -8229,11 +8229,23 @@ async def _prepare_taskdir(self, runtime: TaskRuntime) -> tuple[bool, str | None # Failed tasks keep their worktrees for post-mortems, so a # re-run with the same run_name lands here. Name the exact # command that unblocks it — the bare "already exists" cost a - # full diagnosis cycle in the field. + # full diagnosis cycle in the field. A linked worktree has a + # .git *file*; only then is `git worktree remove` the right + # command, and it must be repo-qualified and quoted to be + # paste-safe from anywhere. + if (taskdir / ".git").is_file(): + remove_cmd = ( + f"git -C {shlex.quote(str(self.manifest.repo))} " + f"worktree remove --force {shlex.quote(str(taskdir))}" + ) + return False, ( + f"worktree taskdir already exists (left by a previous " + f"failed run?): {taskdir} — remove it with " + f"`{remove_cmd}` and re-run" + ) return False, ( - f"worktree taskdir already exists (left by a previous " - f"failed run?): {taskdir} — remove it with " - f"`git worktree remove --force {taskdir}` and re-run" + f"taskdir already exists but is not a registered git " + f"worktree: {taskdir} — move or delete it, then re-run" ) proc = await asyncio.create_subprocess_exec( "git", diff --git a/tests/test_ringer.py b/tests/test_ringer.py index 01bdff0..16fa3ee 100644 --- a/tests/test_ringer.py +++ b/tests/test_ringer.py @@ -506,7 +506,7 @@ def test_worktree_prepare_failure_logs_error_row(self) -> None: self.assertEqual(result.returncode, 1, result.stdout) rows = self.read_rows() self.assertEqual(rows[0]["verdict"], "ERROR") - self.assertIn("worktree taskdir already exists", rows[0]["notes"]) + self.assertIn("taskdir already exists but is not a registered git worktree", rows[0]["notes"]) def test_task_key_cannot_escape_workdir(self) -> None: manifest = self.write_manifest( diff --git a/tests/test_setup_error_diagnostics.py b/tests/test_setup_error_diagnostics.py index 11b71bf..a1b421f 100644 --- a/tests/test_setup_error_diagnostics.py +++ b/tests/test_setup_error_diagnostics.py @@ -48,7 +48,7 @@ def init_git_repo(path: Path) -> None: class SetupErrorDiagnosticsTests(unittest.TestCase): - def test_stale_worktree_collision_is_named_everywhere(self) -> None: + def run_with_stale_taskdir(self, *, registered_worktree: bool) -> tuple[str, Path, Path, Path, Path]: with tempfile.TemporaryDirectory() as temp_root: root = Path(temp_root) home = root / "home" @@ -63,9 +63,18 @@ def test_stale_worktree_collision_is_named_everywhere(self) -> None: ringer_home.mkdir() init_git_repo(repo) - # Simulate the stale worktree a previous failed run leaves behind. + # Simulate what a previous failed run leaves behind: either a real + # registered worktree (the field case) or a plain directory. stale_taskdir = workdir / "stale-task" - stale_taskdir.mkdir(parents=True) + if registered_worktree: + stale_taskdir.parent.mkdir(parents=True) + subprocess.run( + ["git", "-C", str(repo), "worktree", "add", "--quiet", str(stale_taskdir)], + check=True, + capture_output=True, + ) + else: + stale_taskdir.mkdir(parents=True) config_path.write_text( "\n".join( @@ -130,6 +139,7 @@ def test_stale_worktree_collision_is_named_everywhere(self) -> None: env["HOME"] = str(home) env["RINGER_HOME"] = str(ringer_home) env["XDG_CONFIG_HOME"] = str(root / "xdg-config") + env["RINGER_NO_SELF_UPDATE"] = "1" proc = subprocess.run( [ @@ -153,6 +163,11 @@ def test_stale_worktree_collision_is_named_everywhere(self) -> None: ) combined_output = proc.stdout + proc.stderr + expected_reason = ( + "worktree taskdir already exists" + if registered_worktree + else "taskdir already exists but is not a registered git worktree" + ) # Verdict ERROR as before — but no longer naked. self.assertRegex( @@ -161,19 +176,11 @@ def test_stale_worktree_collision_is_named_everywhere(self) -> None: combined_output, ) - # The summary lists the setup failure with the unblocking command. + # The summary lists the setup failure explicitly. self.assertIn( "setup failures (no worker was spawned):", combined_output ) - self.assertIn("worktree taskdir already exists", combined_output) - # macOS tempdirs surface as /var/... but resolve to /private/var/...; - # accept either rendering of the same taskdir. - self.assertTrue( - f"git worktree remove --force {stale_taskdir}" in combined_output - or f"git worktree remove --force {stale_taskdir.resolve()}" - in combined_output, - combined_output, - ) + self.assertIn(expected_reason, combined_output) # The reason reaches the worker log, where post-mortems look first. worker_log_candidates = list(workdir.rglob("*.log")) @@ -197,10 +204,8 @@ def test_stale_worktree_collision_is_named_everywhere(self) -> None: data = json.loads(state_file.read_text(encoding="utf-8")) tasks = data.get("tasks") if isinstance(data, dict) else None for task in tasks or []: - if ( - task.get("key") == "stale-task" - and "worktree taskdir already exists" - in (task.get("setup_error") or "") + if task.get("key") == "stale-task" and expected_reason in ( + task.get("setup_error") or "" ): found_setup_error = True self.assertTrue( @@ -208,6 +213,34 @@ def test_stale_worktree_collision_is_named_everywhere(self) -> None: f"setup_error missing from run state: {state_files}", ) + return ( + combined_output, + stale_taskdir.resolve(), + workdir.resolve(), + state_dir.resolve(), + repo.resolve(), + ) + + def test_stale_registered_worktree_names_the_exact_remove_command(self) -> None: + combined_output, stale_taskdir, _, _, repo = self.run_with_stale_taskdir( + registered_worktree=True + ) + # The command must be paste-safe from anywhere: repo-qualified and + # pointing at the resolved taskdir. + self.assertIn( + f"git -C {repo} worktree remove --force {stale_taskdir}", + combined_output, + ) + + def test_plain_directory_collision_does_not_claim_a_worktree_command(self) -> None: + combined_output, _, _, _, _ = self.run_with_stale_taskdir( + registered_worktree=False + ) + # `git worktree remove` would fail on a plain directory — never + # print a recovery command that does not work. + self.assertNotIn("git worktree remove", combined_output) + self.assertIn("move or delete it, then re-run", combined_output) + if __name__ == "__main__": unittest.main(verbosity=2)