From 4323c67dcc6048fc8e311cdff7600d3d6a17807f Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:29:02 +0530 Subject: [PATCH] fix(delegate): disclaim only the fields a failed probe actually left unmeasured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /simplify-code residual. The note hard-coded "'commits' and 'dirty' are UNKNOWN", but the two probes fail independently: a bad base_commit fails rev-list while `git status` still succeeds, so `dirty` is a REAL measurement being reported as unknown. Safety was never affected (the worktree is preserved either way), but telling the parent a measured value is untrustworthy is its own kind of misreport — and it would push a human toward re-inspecting something already proven. `mark_worktree_payload_unproven()` now takes an `unmeasured` argument, and finalize tracks which probe actually failed. The raising path still disclaims both, because which probe raised is unknowable there. Validation: 22/22 tests/tools/test_subagent_worktree.py; ruff + ty clean. New guard mutation-checked (hard-coding "commits/dirty" back fails it). --- tests/tools/test_subagent_worktree.py | 27 ++++++++++++++++ tools/subagent_worktree.py | 44 +++++++++++++++++++-------- 2 files changed, 58 insertions(+), 13 deletions(-) diff --git a/tests/tools/test_subagent_worktree.py b/tests/tools/test_subagent_worktree.py index ed2b8a79a6..571c5f374a 100644 --- a/tests/tools/test_subagent_worktree.py +++ b/tests/tools/test_subagent_worktree.py @@ -239,6 +239,33 @@ class SubagentWorktreeTests(unittest.TestCase): branches = _git(["branch", "--list", info["branch"]], repo).stdout self.assertNotEqual(branches.strip(), "") + def test_finalize_note_disclaims_only_the_unmeasured_field(self): + """A partial failure must not claim a MEASURED value is unknown. + + A bad base_commit fails `rev-list` while `status` still succeeds, so + ``dirty`` is a real measurement — the note should disclaim ``commits`` + only, or it misreports in the other direction. + """ + repo = _make_repo(self.tmp) + info = sw.create_subagent_worktree(str(repo), "partial") + assert info is not None + wt = Path(info["path"]) + (wt / "UNTRACKED.txt").write_text("dirty!\n", encoding="utf-8") + + payload = sw.finalize_subagent_worktree( + {**info, "base_commit": "deadbeef" * 5} + ) + + # status succeeded, so dirty is trustworthy and reported as such. + self.assertTrue(payload["dirty"]) + self.assertTrue(payload["inspection_failed"]) + self.assertFalse(payload["pruned"]) + # The note names ONLY the unmeasured field. + self.assertIn("commits UNKNOWN", payload["note"]) + self.assertNotIn("dirty UNKNOWN", payload["note"]) + self.assertNotIn("commits/dirty", payload["note"]) + self.assertTrue((wt / "UNTRACKED.txt").exists()) + def test_finalize_keeps_worktree_when_base_commit_missing(self): """An unmeasurable commit count must not authorize deletion. diff --git a/tools/subagent_worktree.py b/tools/subagent_worktree.py index b039086f5a..b09b1a55be 100644 --- a/tools/subagent_worktree.py +++ b/tools/subagent_worktree.py @@ -173,15 +173,20 @@ def create_subagent_worktree( def mark_worktree_payload_unproven( - payload: Dict[str, Any], reason: str + payload: Dict[str, Any], reason: str, *, unmeasured: str = "commits/dirty" ) -> Dict[str, Any]: """Flag a worktree result payload as un-inspected, in place (#88113). - A failed probe proves nothing about the tree, so ``commits``/``dirty`` keep - 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. + A failed probe proves nothing about the tree, so the fields it would have + filled keep 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. + + *unmeasured* names only the fields this failure actually left unproven: one + probe can succeed while the other fails (a bad ``base_commit`` fails + ``rev-list`` while ``status`` still reports a real ``dirty``), and claiming + a measured value is UNKNOWN would be its own kind of misreport. Shared by ``finalize_subagent_worktree`` and ``delegate_tool``'s finalize-raised fallback so the two producers of this schema cannot drift. @@ -190,8 +195,8 @@ def mark_worktree_payload_unproven( branch = payload.get("branch", "") 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"git inspection failed ({reason}): {unmeasured} UNKNOWN — not " + "proven zero/clean. The worktree and branch were preserved " f"— inspect {path} (branch {branch}) before assuming no work." ) logger.warning( @@ -259,17 +264,25 @@ def finalize_subagent_worktree( payload["pruned"] = True # nothing on disk to review return payload - def _unproven(reason: str) -> Dict[str, Any]: - return mark_worktree_payload_unproven(payload, reason) + def _unproven( + reason: str, *, unmeasured: str = "commits/dirty" + ) -> Dict[str, Any]: + return mark_worktree_payload_unproven( + payload, reason, unmeasured=unmeasured + ) # A worktree whose commit count was never measured must not be pruned # either: the prune condition reads payload["commits"], and without a base # commit that value is an unproven default, exactly the class of bug # #88113 is about. if not base_commit: - return _unproven("no base_commit recorded — commit count unmeasurable") + return _unproven( + "no base_commit recorded — commit count unmeasurable", + unmeasured="commits", + ) failed: list = [] + unmeasured: list = [] try: counted = _run_git( ["rev-list", "--count", f"{base_commit}..HEAD"], cwd=path @@ -281,6 +294,7 @@ def finalize_subagent_worktree( f"rev-list exit {counted.returncode}: " f"{counted.stderr.strip()[:200]}" ) + unmeasured.append("commits") status = _run_git(["status", "--porcelain"], cwd=path) if status.returncode == 0: payload["dirty"] = bool(status.stdout.strip()) @@ -289,16 +303,20 @@ def finalize_subagent_worktree( f"status exit {status.returncode}: " f"{status.stderr.strip()[:200]}" ) + unmeasured.append("dirty") except Exception as exc: # 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. + # deleting work, and tell the caller the numbers are unproven. Which + # probe raised is unknowable here, so neither value is trustworthy. return _unproven(f"inspection raised: {exc}") if failed: # Fail-safe (#88113): a destructive cleanup requires affirmative proof # of "zero commits + clean tree"; the defaults prove nothing. - return _unproven("; ".join(failed)) + return _unproven( + "; ".join(failed), unmeasured="/".join(unmeasured) + ) if prune and payload["commits"] == 0 and not payload["dirty"]: try: