fix(delegate): disclaim only the fields a failed probe actually left unmeasured
/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).
This commit is contained in:
@@ -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.
|
||||
|
||||
|
||||
+31
-13
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user