From f720e30035e20f3280cecdd2ed9fc08615298bf7 Mon Sep 17 00:00:00 2001 From: drew Date: Fri, 15 May 2026 01:21:38 -0400 Subject: [PATCH] fix(auto-agents): worker no longer deletes dispatcher pre-clone + dispatcher resilient to missing worktree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run-11 deep inspection (PR #30 cycle, 4 escalation tiers) traced the "worktree reset to pinned SHA ... failed ... exit 128" warnings — and the cycle's misleading ``rebase-failed`` outcome despite the worker's own ``{"outcome": "resolved"}`` — to two bugs that compounded: 1. task-implementor.md instructed the worker to ``rm -rf {repo_dir}`` after every ``pr_fix`` (rule #7 + procedure step 11). That rule was correct in the legacy ``git-isolator-util`` workflow where the worker created its own throwaway clone, but with the Phase 3 pre-clone the worktree is owned by the dispatcher and reused across tier escalation. Deleting it stranded every subsequent tier with no workspace AND blew up the dispatcher's between-tier reset step. 2. ``_reset_worktree_to_pinned_sha`` shelled out to ``git -C `` without checking the path existed; the resulting ``CalledProcessError`` was logged with ``%s`` (just "exit 128"), so the real cause was invisible without py-spy. Fixes: - task-implementor.md: rule #7 + ``pr_fix`` step 11 now spell out the conditional — delete only if YOU created the clone via ``git-isolator-util``; leave the dispatcher's pre-clone alone. ``issue_impl`` step 10 keeps an unconditional ``rm -rf`` (no PR pre-clone exists for new-issue work) plus a one-line clarifying note. - dispatch_implementer._reset_worktree_to_pinned_sha: detect missing worktree dir BEFORE shelling to git (returns False with a clear "disappeared before reset" warning naming the path + pinned SHA); remove a stale ``.git/index.lock`` if a SIGKILL'd previous-session git op left one behind; capture stderr from ``CalledProcessError`` and include it (truncated) in the warning so future diagnosis doesn't require a session-archive deep dive. Fail-soft policy preserved — escalation still continues, the next session's ``implementer-workspace.py discover`` will fall through to ``git-isolator-util`` when the worktree is gone. Tests: new tests/auto_agents/test_worktree_reset_resilience.py with 4 classes / 8 tests pinning the missing-dir detection, stale-lock cleanup, stderr-surfaced-on-failure, happy-path, and degenerate-input contracts. Suite: 1611 passed / 3 skipped. Not fixed in this commit (deferred — root cause not clear from one sample): - PR #29's first session at run-11 timed out at the 1800s worker timeout with ``turn 3 task subagent input=0tok output=0tok wallclock=1756s`` — task subagent stuck with no token usage. Could be LLM-provider hang, tool loop, or queue stall. Needs another reproduction + session-archive event-stream inspection to localise. Co-Authored-By: Claude Opus 4.7 (1M context) --- .opencode/agents/task-implementor.md | 6 +- .../test_worktree_reset_resilience.py | 195 ++++++++++++++++++ tools/dispatch_implementer.py | 78 ++++++- 3 files changed, 267 insertions(+), 12 deletions(-) create mode 100644 tests/auto_agents/test_worktree_reset_resilience.py diff --git a/.opencode/agents/task-implementor.md b/.opencode/agents/task-implementor.md index 0ca38d7e9..a8b73bbae 100644 --- a/.opencode/agents/task-implementor.md +++ b/.opencode/agents/task-implementor.md @@ -457,7 +457,7 @@ This is a **performance** change, not a correctness change: the pre-fetched data 9. **Post attempt comment** on the issue (see "Attempt Comments" section below). -10. **Clean up.** `rm -rf {repo_dir}` +10. **Clean up.** `rm -rf {repo_dir}` — `issue_impl` always uses `git-isolator-util` to create the clone (no PR exists yet, so no dispatcher pre-clone), so the cleanup is unambiguously yours to do. (Contrast with `pr_fix` step 11, which is conditional.) 11. **Emit terminal output JSON and exit.** Your final response MUST end with exactly one `{"outcome": ..., "files_touched": [...]}` object — see "Terminal output" below. This is required even when the cycle failed. @@ -488,7 +488,7 @@ This is a **performance** change, not a correctness change: the pre-fetched data 10. **Post attempt comment** on the PR (see "Attempt Comments" section below). -11. **Clean up.** `rm -rf {repo_dir}` +11. **Clean up — conditionally.** If step 6 took the **pre-clone path** (`discover` returned a non-empty `repo_dir=`), **DO NOT delete `{repo_dir}`** — the dispatcher owns that worktree and reuses it across tier escalation. Skip cleanup entirely; jump to step 12. If step 6 took the **`git-isolator-util` fallback path** (your own ad-hoc clone), then run `rm -rf {repo_dir}` to free the temp dir. See CRITICAL Rule #7 for the rationale. 12. **Emit terminal output JSON and exit.** Your final response MUST end with exactly one `{"outcome": ..., "files_touched": [...]}` object — see "Terminal output" below. This is required even when the cycle failed. @@ -863,7 +863,7 @@ Commit all staged changes and force-push with lease. 4. **All commands through nox.** Never run `pip install`, `pytest`, `behave`, or `robot` directly. 5. **Leave an attempt comment always.** Whether you succeeded or failed, post the structured attempt comment. This is how the supervisor tracks escalation state. 6. **Never merge.** Create PRs; the merge supervisor handles merging. Never call any merge endpoint. -7. **Clean up your clone.** Delete the temporary directory before exiting (`rm -rf {repo_dir}`). +7. **Clean up your clone ONLY IF you created it.** If step 6 (in `pr_fix`) used `implementer-workspace.py discover` and got a non-empty `repo_dir=`, the **dispatcher** pre-cloned that worktree and owns its lifecycle (it is reused across tier escalation and cleaned up at end-of-cycle by `pr_clone.cleanup_*`). **Do NOT `rm -rf {repo_dir}` in that case** — deleting it strands the next escalation tier with no worktree and forces it to re-clone from scratch (and confuses the dispatcher's reset step between tiers). Only delete `{repo_dir}` when YOUR step 6 called `git-isolator-util` to create the clone (or when running `issue_impl`, which always calls `git-isolator-util` because there is no pre-existing PR worktree to share). 8. **Never work in `/app`.** Always work in `/tmp/`. If `repo_dir` is not inside `/tmp/`, refuse and report an error. 9. **Bot signature on all Forgejo content:** ``` diff --git a/tests/auto_agents/test_worktree_reset_resilience.py b/tests/auto_agents/test_worktree_reset_resilience.py new file mode 100644 index 000000000..285e62990 --- /dev/null +++ b/tests/auto_agents/test_worktree_reset_resilience.py @@ -0,0 +1,195 @@ +"""Unit tests for ``dispatch_implementer._reset_worktree_to_pinned_sha``. + +Background — run-11 deep inspection found that the task-implementor +worker's prompt instructed it to ``rm -rf {repo_dir}`` at the end of +``pr_fix`` (CRITICAL Rule #7 + procedure step 11), which destroyed the +dispatcher's pre-clone. The dispatcher then ran ``git reset --hard +`` between tier escalations against a non-existent +directory, got exit code 128, and silently continued via a generic +``CalledProcessError`` warning that hid the real cause. Three of four +tier escalations on PR #30 fired the same warning; the cycle landed +``rebase-failed`` despite the worker's own ``{"outcome": "resolved"}``. + +The prompt-side fix is in ``.opencode/agents/task-implementor.md``; +this file pins the dispatcher-side defence-in-depth: + +- Missing worktree dir → return False without shelling to ``git``, + log a clear message naming the likely cause. +- Stale ``.git/index.lock`` (a SIGKILL'd git op in the previous + session) → remove it so the reset isn't blocked. +- ``git`` failures → surface the captured stderr in the warning so + future operators can diagnose without re-running py-spy + reading + session archives. +""" +from __future__ import annotations + +import logging +import subprocess +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from .conftest import load_tool_module + + +@pytest.fixture +def driver(): + return load_tool_module("dispatch_implementer", fresh=True) + + +def _handle(path: Path, sha: str = "a" * 40) -> SimpleNamespace: + """Minimal stand-in for ``_pr_clone.WorktreeHandle`` — only the + two attributes ``_reset_worktree_to_pinned_sha`` reads.""" + return SimpleNamespace(path=str(path), head_sha=sha) + + +class TestMissingWorktree: + """Worker self-destructs its workspace (run-11 pattern): + ``_reset_worktree_to_pinned_sha`` must detect this BEFORE invoking + ``git`` and log a diagnostic message rather than a bare + ``CalledProcessError`` whose ``__str__`` only shows the exit code. + """ + + def test_returns_false_when_worktree_dir_is_missing( + self, driver, tmp_path, monkeypatch + ): + # The path under handle does NOT exist on disk. + missing = tmp_path / "vanished-worktree" + assert not missing.exists() + # Subprocess MUST NOT be called — we never even shell out. + run_calls: list[tuple] = [] + + def fake_run(*args, **kw): + run_calls.append(args) + return subprocess.CompletedProcess(args, 0, "", "") + + monkeypatch.setattr(subprocess, "run", fake_run) + assert driver._reset_worktree_to_pinned_sha(_handle(missing)) is False + assert run_calls == [], ( + "subprocess.run must not be invoked when the worktree " + f"is missing; got: {run_calls!r}" + ) + + def test_logs_disappeared_message_with_path_and_sha( + self, driver, tmp_path, caplog + ): + """The warning must name the worktree path AND the pinned SHA + (truncated) so an operator inspecting the log knows which PR + and which baseline.""" + sha = "1234567890abcdef" + "0" * 24 + missing = tmp_path / "vanished" + with caplog.at_level(logging.WARNING, logger="dispatch_implementer"): + driver._reset_worktree_to_pinned_sha(_handle(missing, sha=sha)) + msgs = [r.getMessage() for r in caplog.records] + relevant = [m for m in msgs if "disappeared" in m or "vanished" in m] + assert relevant, f"no missing-worktree log found in {msgs!r}" + assert any(str(missing) in m for m in relevant) + # The 12-char short SHA prefix is what the warning reports. + assert any(sha[:12] in m for m in relevant) + + +class TestStaleLockCleanup: + """A SIGKILL'd git op in the previous session leaves + ``.git/index.lock`` behind, which then blocks every git operation + with ``Unable to create '...index.lock'``. The reset must clear + the orphan lock before retrying.""" + + def test_stale_index_lock_removed_before_reset( + self, driver, tmp_path, monkeypatch + ): + # Set up a worktree-shaped dir with a stale lock. + wt = tmp_path / "pr-30-implementer-deadbeef" + (wt / ".git").mkdir(parents=True) + lock = wt / ".git" / "index.lock" + lock.write_text("", encoding="utf-8") + assert lock.exists() + + # Stub subprocess to succeed; we only care that the lock is + # gone BEFORE the first git call. + observed_lock_state: list[bool] = [] + + def fake_run(args, **kw): + observed_lock_state.append(lock.exists()) + return subprocess.CompletedProcess(args, 0, "", "") + + monkeypatch.setattr(subprocess, "run", fake_run) + assert driver._reset_worktree_to_pinned_sha(_handle(wt)) is True + # The lock was present pre-call AND absent when ``git reset`` + # ran (the first observed state). + assert observed_lock_state, "subprocess.run was never invoked" + assert observed_lock_state[0] is False, ( + "stale .git/index.lock must be removed BEFORE the first " + f"git call; observed states: {observed_lock_state!r}" + ) + assert not lock.exists() + + +class TestGitFailureSurfaceStderr: + """The previous implementation logged ``CalledProcessError``'s + ``__str__`` which only contains the command + exit code — useless + for diagnosis. The new logging must include the captured stderr + (truncated) so the operator can see WHY git failed.""" + + def test_stderr_appears_in_warning(self, driver, tmp_path, caplog, monkeypatch): + wt = tmp_path / "pr-30-implementer-deadbeef" + (wt / ".git").mkdir(parents=True) + sentinel_stderr = "fatal: invalid reference: 1234deadbeef" + + def fake_run(args, **kw): + raise subprocess.CalledProcessError( + returncode=128, cmd=args, output="", stderr=sentinel_stderr, + ) + + monkeypatch.setattr(subprocess, "run", fake_run) + with caplog.at_level(logging.WARNING, logger="dispatch_implementer"): + ok = driver._reset_worktree_to_pinned_sha(_handle(wt)) + assert ok is False + msgs = [r.getMessage() for r in caplog.records] + assert any(sentinel_stderr in m for m in msgs), ( + f"git stderr not in warning; got: {msgs!r}" + ) + # exit code also surfaced for quick triage. + assert any("128" in m for m in msgs) + + +class TestHappyPath: + """Sanity: when the worktree exists and ``git`` succeeds, return + True and call BOTH ``git reset --hard`` and ``git clean -xfdq``.""" + + def test_returns_true_when_reset_and_clean_succeed( + self, driver, tmp_path, monkeypatch + ): + wt = tmp_path / "pr-30-implementer-deadbeef" + (wt / ".git").mkdir(parents=True) + calls: list[list[str]] = [] + + def fake_run(args, **kw): + calls.append(list(args)) + return subprocess.CompletedProcess(args, 0, "", "") + + monkeypatch.setattr(subprocess, "run", fake_run) + assert driver._reset_worktree_to_pinned_sha(_handle(wt)) is True + # Two git calls, in order, with the worktree path. + assert len(calls) == 2, calls + assert calls[0][:5] == ["git", "-C", str(wt), "reset", "--hard"] + assert calls[1][:5] == ["git", "-C", str(wt), "clean", "-xfdq"] + + +class TestDegenerateInputs: + """Defensive: ``handle is None`` / missing attrs returns False + without raising. Locks down the pre-existing fail-soft policy so + a future refactor that moves the validation can't drop it.""" + + def test_none_handle(self, driver): + assert driver._reset_worktree_to_pinned_sha(None) is False + + def test_missing_path_attr(self, driver): + h = SimpleNamespace(head_sha="a" * 40) # no .path + assert driver._reset_worktree_to_pinned_sha(h) is False + + def test_missing_sha_attr(self, driver, tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + h = SimpleNamespace(path=str(wt)) # no .head_sha + assert driver._reset_worktree_to_pinned_sha(h) is False diff --git a/tools/dispatch_implementer.py b/tools/dispatch_implementer.py index f0575ed83..daa31cded 100644 --- a/tools/dispatch_implementer.py +++ b/tools/dispatch_implementer.py @@ -2137,8 +2137,23 @@ def _reset_worktree_to_pinned_sha(handle: Any) -> bool: SHA explicitly. Returns ``True`` on success, ``False`` on any subprocess failure - (logged WARNING; escalation continues with a potentially-dirty - worktree, which is suboptimal but does not break the loop). + (logged WARNING; escalation continues — the next session's + ``implementer-workspace.py discover`` will see the vanished / + dirty worktree and fall through to ``git-isolator-util``, the + legacy pre-pre-clone code path). + + Run-11 deep inspection: the prior worker session can ``rm -rf`` + its own worktree (the task-implementor prompt at one point + explicitly instructed it to — fixed in the same commit as this + function). Defend against that and other state-corruption cases: + + - Missing worktree dir: log clearly and return False without + shelling out to ``git`` against a non-existent path. + - Stale ``.git/index.lock`` from a SIGKILL'd worker git op: + remove it so the reset isn't blocked by an orphan lock. + - On ``git`` failure, surface the captured stderr in the + warning — the bare ``CalledProcessError.__str__`` only shows + the exit code, which is useless for diagnosis. Note: the remote branch may carry commits the worker pushed in Tier N. The plan's silent-worst-case rule @@ -2146,6 +2161,7 @@ def _reset_worktree_to_pinned_sha(handle: Any) -> bool: short-circuits that case before we reach this reset. """ import subprocess + from pathlib import Path as _Path if handle is None: return False @@ -2153,22 +2169,66 @@ def _reset_worktree_to_pinned_sha(handle: Any) -> bool: pinned_sha = getattr(handle, "head_sha", None) if not path or not pinned_sha: return False + sha_short = str(pinned_sha)[:12] + + if not _Path(path).is_dir(): + _logger.warning( + "worktree disappeared before reset for SHA %s at %s — " + "the prior worker session likely deleted it. Escalation " + "will continue; the next session's " + "``implementer-workspace.py discover`` will fall through " + "to ``git-isolator-util`` for a fresh clone.", + sha_short, path, + ) + return False + + # Stale-lock cleanup. A SIGKILL'd or crashed git op in the + # previous session can leave ``.git/index.lock`` behind, which + # then blocks every subsequent git operation with + # ``Unable to create '...index.lock'``. The lock has no + # legitimate concurrent owner here (the worker session has + # already terminated by the time the dispatcher is between + # tiers), so removing it is safe and matches what an operator + # would do by hand. + try: + lock = _Path(path) / ".git" / "index.lock" + if lock.exists(): + lock.unlink() + _logger.info( + "removed stale .git/index.lock at %s before reset", + path, + ) + except OSError: + pass + try: subprocess.run( ["git", "-C", str(path), "reset", "--hard", str(pinned_sha)], - check=True, capture_output=True, timeout=30, + check=True, capture_output=True, timeout=30, text=True, ) subprocess.run( ["git", "-C", str(path), "clean", "-xfdq"], - check=True, capture_output=True, timeout=30, + check=True, capture_output=True, timeout=30, text=True, ) return True - except subprocess.SubprocessError as exc: + except subprocess.CalledProcessError as exc: + # ``CalledProcessError.__str__`` only shows the exit code — + # include the actual stderr so the operator can act on it + # (vanished SHA vs. lock contention vs. permissions issue, + # etc.). _logger.warning( - "worktree reset to pinned SHA %s failed at %s: %s; " - "escalation continues with current worktree state", - pinned_sha[:12] if pinned_sha else "", - path, exc, + "worktree reset to pinned SHA %s failed at %s: " + "exit=%s stderr=%r; escalation continues", + sha_short, path, exc.returncode, + (exc.stderr or "").strip()[:400], + ) + return False + except subprocess.SubprocessError as exc: + # Timeout / missing binary / etc. — same fail-soft policy. + _logger.warning( + "worktree reset to pinned SHA %s raised %s at %s; " + "escalation continues", + sha_short, type(exc).__name__, path, ) return False