From 38ea711fd0f57e7c2ec92d805aa2cc3f56b22c8b Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 17 Aug 2026 18:18:26 +0530 Subject: [PATCH] fix(delegate): tell the parent when a worktree was preserved un-inspected The preserved worktree is invisible to the only consumer that can act on it. Completes the #88113 fix. That change correctly stops the destructive prune when a git probe fails, but still returns commits=0 / dirty=False -- values that were never measured. Those are the defaults the prune used to delete on, so the failure payload is byte-identical to "inspected fine, child left nothing": inspection FAILED, uncommitted work kept -> {commits: 0, dirty: False, pruned: False} inspected OK, child produced nothing -> {commits: 0, dirty: False, pruned: False} The only failure signal was a logger.warning, and the sole consumer of this payload is the parent agent reading the serialized delegate_task entry -- it cannot read logs (no in-repo code reads the key back). So the parent's rational reading of the failure case is "the child produced no work", which is the exact wrong conclusion: a worktree possibly full of uncommitted work is preserved and then never looked at. The data survives but nobody is told to recover it. Changes: - subagent_worktree: one _unproven() helper stamps inspection_failed + a note naming the worktree/branch, warns, and returns the payload. Both unproven exits route through it, so they cannot drift apart again. - subagent_worktree: the pre-existing exception path (timeout, OSError, a non-numeric rev-list stdout) produced the same unproven payload but logged at DEBUG -- effectively silent. It now takes the same flagged path as a non-zero exit; identical outcomes get identical reporting. - delegate_tool: the caller's finalize-raised fallback assigned the creation-side metadata dict (path/branch/repo_root/base_commit) -- a disjoint schema missing commits/dirty/pruned. It now emits the same flagged shape, and logs at WARNING. - Docs + docstring + module contract now state that pruning requires affirmative proof, so a future cleanup doesn't "fix" the preserved worktree by restoring the unconditional prune and reintroducing this P1. Purely additive: the happy-path payload shape is unchanged, so no existing reader can break. Validation: - 18/18 tests/tools/test_subagent_worktree.py; 127 passed across the delegation suites (test_delegate, batch_validation, control_actions, timeout_diagnostic). - 3 new guards mutation-checked: neutering the flag fails all three; reverting the production file to pre-fix main fails all three. Restores checksum-verified. - E2E on real git: inspection-failure now returns inspection_failed=true with work intact on disk; proven-clean still prunes (pruned=true). --- tests/tools/test_subagent_worktree.py | 59 +++++++++++++++++++ tools/delegate_tool.py | 19 +++++- tools/subagent_worktree.py | 54 +++++++++++++---- .../docs/user-guide/features/delegation.md | 4 ++ 4 files changed, 121 insertions(+), 15 deletions(-) diff --git a/tests/tools/test_subagent_worktree.py b/tests/tools/test_subagent_worktree.py index d452151063..f0a8e81771 100644 --- a/tests/tools/test_subagent_worktree.py +++ b/tests/tools/test_subagent_worktree.py @@ -163,6 +163,65 @@ class SubagentWorktreeTests(unittest.TestCase): self.assertTrue((wt / "UNCOMMITTED-WORK.txt").exists()) branches = _git(["branch", "--list", info["branch"]], repo).stdout self.assertNotEqual(branches.strip(), "") + # The parent agent only ever sees this payload (it cannot read logs), + # so the uncertainty must travel in the dict — otherwise "0 commits, + # clean" reads as "the child produced nothing" and the work we just + # preserved never gets looked at. + self.assertTrue(payload["inspection_failed"]) + self.assertIn("UNKNOWN", payload["note"]) + self.assertIn(info["path"], payload["note"]) + + def test_finalize_flags_unproven_state_distinguishably(self): + """#88113 follow-up: a failed inspection must not look like "no work". + + Without an explicit flag, "inspection failed, uncommitted work + preserved" and "inspected fine, child left nothing" serialize to the + byte-identical dict {commits: 0, dirty: False, pruned: False} — so the + parent agent's rational reading of the failure case is the exact wrong + conclusion. + """ + repo = _make_repo(self.tmp) + + # Case 1: inspection SUCCEEDED, tree genuinely clean, prune disabled. + ok_info = sw.create_subagent_worktree(str(repo), "proven-clean") + assert ok_info is not None + ok_payload = sw.finalize_subagent_worktree(ok_info, prune=False) + + # Case 2: inspection FAILED with real uncommitted work on disk. + bad_info = sw.create_subagent_worktree(str(repo), "unproven") + assert bad_info is not None + bad_wt = Path(bad_info["path"]) + (bad_wt / "WIP.txt").write_text("real work\n", encoding="utf-8") + git_dir = Path(_git(["rev-parse", "--git-dir"], bad_wt).stdout.strip()) + if not git_dir.is_absolute(): + git_dir = (bad_wt / git_dir).resolve() + (git_dir / "index").write_bytes(b"not-a-valid-git-index\n") + bad_payload = sw.finalize_subagent_worktree(bad_info) + + # The three state fields are identical — that is exactly the ambiguity. + for key in ("commits", "dirty", "pruned"): + self.assertEqual(ok_payload[key], bad_payload[key]) + # Only the flag separates them. + self.assertNotIn("inspection_failed", ok_payload) + self.assertNotIn("note", ok_payload) + self.assertTrue(bad_payload["inspection_failed"]) + + def test_finalize_flags_unproven_state_when_inspection_raises(self): + """A raising probe is the same unknown state as a non-zero exit.""" + repo = _make_repo(self.tmp) + info = sw.create_subagent_worktree(str(repo), "raises") + assert info is not None + + def _boom(*_a, **_k): + raise subprocess.TimeoutExpired(cmd="git", timeout=30) + + with mock.patch.object(sw, "_run_git", side_effect=_boom): + payload = sw.finalize_subagent_worktree(info) + + self.assertFalse(payload["pruned"]) + self.assertTrue(payload["inspection_failed"]) + self.assertIn("UNKNOWN", payload["note"]) + self.assertTrue(os.path.isdir(info["path"])) def test_finalize_missing_path_reports_pruned(self): payload = sw.finalize_subagent_worktree( diff --git a/tools/delegate_tool.py b/tools/delegate_tool.py index 403e53204c..c6eabbe4b5 100644 --- a/tools/delegate_tool.py +++ b/tools/delegate_tool.py @@ -2508,8 +2508,23 @@ def _run_single_child( subagent_worktree.finalize_subagent_worktree(_worktree_info) ) except Exception as e: - logger.debug("worktree finalize failed: %s", e) - entry_dict["worktree"] = dict(_worktree_info) + # finalize is written hard not to raise, but if it ever does the + # state is unknown — emit the SAME schema the parent expects, + # flagged, instead of leaking the creation-side metadata shape. + logger.warning("worktree finalize failed: %s", e) + entry_dict["worktree"] = { + "path": _worktree_info.get("path", ""), + "branch": _worktree_info.get("branch", ""), + "commits": 0, + "dirty": False, + "pruned": False, + "inspection_failed": True, + "note": ( + "worktree finalize raised; state unknown — inspect " + f"{_worktree_info.get('path', '')} manually before " + "assuming no work." + ), + } try: _heartbeat_thread.start() diff --git a/tools/subagent_worktree.py b/tools/subagent_worktree.py index e401b3c1a9..4952a11e41 100644 --- a/tools/subagent_worktree.py +++ b/tools/subagent_worktree.py @@ -26,7 +26,9 @@ Contract (mirrors Muse Code's documented semantics): dirty state so the parent can review or merge each branch. - **Clean worktrees are pruned.** A worktree with no new commits and a clean tree is removed automatically after the child finishes; anything - holding work is kept and reported. + holding work is kept and reported. Pruning requires affirmative proof: + if a git inspection probe fails the state is unknown, so the worktree is + kept and the result entry is flagged ``inspection_failed`` (#88113). Only the local terminal backend is supported: on docker/ssh/modal/etc. the worktree created on the host would not be visible inside the sandbox, so @@ -177,8 +179,15 @@ def finalize_subagent_worktree( Returns a result-entry payload: path, branch, ``commits`` ahead of the base, ``dirty`` (uncommitted changes present), and ``pruned``. A worktree - with zero commits and a clean tree is removed when *prune* is true; - anything holding work is always kept for the parent to review or merge. + with zero commits and a clean tree is removed when *prune* is true **and + both git probes succeeded**; anything holding work is always kept for the + parent to review or merge. + + If ``git rev-list``/``git status`` exits non-zero (or the inspection + raises), the tree state is unknown, so the worktree and branch are kept + and the payload carries ``inspection_failed: True`` plus a ``note``. + ``commits``/``dirty`` are then defaults, NOT measurements — the parent + must inspect the worktree instead of concluding the child did no work. """ path = info.get("path", "") branch = info.get("branch", "") @@ -196,6 +205,30 @@ def finalize_subagent_worktree( payload["pruned"] = True # nothing on disk to review return payload + def _unproven(reason: str) -> Dict[str, Any]: + """Flag the payload as un-inspected and keep the worktree (#88113). + + A failed probe proves nothing about the tree, so ``commits``/``dirty`` + are still their defaults. The parent agent only ever sees this dict — + it cannot read logs — so the uncertainty has to travel *in the + payload*, or "0 commits, clean" reads as "the child produced nothing" + and the work we just preserved is never looked at. + """ + payload["inspection_failed"] = True + payload["note"] = ( + f"git inspection failed ({reason}): 'commits' and 'dirty' are " + "UNKNOWN, not zero/clean. The worktree and branch were preserved " + f"— inspect {path} (branch {branch}) before assuming no work." + ) + logger.warning( + "subagent worktree: git inspection failed (%s) — keeping %s " + "(branch %s) for manual review", + reason, + path, + branch, + ) + return payload + inspection_ok = True try: if base_commit: @@ -212,9 +245,10 @@ def finalize_subagent_worktree( else: inspection_ok = False except Exception as exc: - logger.debug("subagent worktree: finalize inspection failed: %s", exc) - # Unknown state — keep the worktree rather than risk deleting work. - return payload + # Same unknown state as a non-zero exit (timeout, OSError, or a + # non-numeric rev-list stdout) — keep the worktree rather than risk + # deleting work, and tell the caller the numbers are unproven. + return _unproven(f"inspection raised: {exc}") if not inspection_ok: # Fail-safe (#88113): a non-zero git exit proves nothing about the @@ -223,13 +257,7 @@ def finalize_subagent_worktree( # child work. A destructive cleanup requires affirmative proof of # "zero commits + clean tree"; otherwise keep the worktree and # branch for manual inspection. - logger.warning( - "subagent worktree: git inspection failed (rev-list/status " - "non-zero) — keeping %s (branch %s) for manual review", - path, - branch, - ) - return payload + return _unproven("rev-list/status non-zero") if prune and payload["commits"] == 0 and not payload["dirty"]: try: diff --git a/website/docs/user-guide/features/delegation.md b/website/docs/user-guide/features/delegation.md index f7f3819f1e..b71c3bde4f 100644 --- a/website/docs/user-guide/features/delegation.md +++ b/website/docs/user-guide/features/delegation.md @@ -412,6 +412,10 @@ With isolation on: reviews or merges each branch (`git log `, `git merge `). - A worktree left with **no commits and a clean tree is pruned automatically** (`pruned: true`); anything holding work is kept. +- Pruning requires proof. If a git inspection probe fails, the worktree and + branch are kept and the entry carries `inspection_failed: true` plus a + `note` — `commits`/`dirty` are then defaults, not measurements, so inspect + the worktree rather than assuming the child produced nothing. Scope: opt-in, git-only, and local-terminal-backend-only. In a non-git directory, on docker/ssh/modal backends, or if worktree creation fails, the