test(delegate): assert the unproven-state contract, not its prose
Review fold on the #88113 follow-up. The new guards asserted implementation details that a strictly-better future change would break, and the second producer of the payload schema had no coverage at all. - The distinguishability test asserted the failure payload was byte-identical to the genuinely-clean one (`for key in commits/dirty/pruned: assertEqual`). That freezes the AMBIGUITY as a required property: emitting `commits: None` for "unknown" would improve exactly what #88113 is about and fail the test. Now asserts what the parent actually depends on -- both keep the worktree, and only the flag separates them. - `assertNotIn("inspection_failed", ok_payload)` pinned key ABSENCE on the happy path, forbidding an always-present-but-False flag (a legitimately better JSON contract: stable key set for serializers). Now `assertFalse(...get("inspection_failed", False))` -- same coverage, tolerant of that refactor. - `assertIn("UNKNOWN", note)` coupled tests to one word of English prose, and was not even a cross-producer contract: delegate_tool's note said "state unknown" (lowercase), so a copy-edit broke the implied convention. Tests now assert the note names the worktree AND branch -- the actionable part for a human -- and both producers' notes were aligned to read as one contract. - The raises test never proved its patched seam ran (a future short-circuit before any git call would keep it green while proving nothing). Now checks `call_count` and mirrors the branch-survival + note-names-path legs its sibling had. - NEW `WorktreePayloadSchemaTests`: commit 2's whole point is the schema the parent reads, but delegate_tool's fallback -- the second producer -- was verified only by reading. It now AST-parses the real fallback dict literal and compares against live `finalize_subagent_worktree()` output, so the two producers cannot drift and the pre-fix leak (repo_root/base_commit, missing commits/dirty/pruned) cannot come back. - Docs/docstring drift: the flag has a second trigger (finalization itself raising, handled in delegate_tool), and the module docstring listed `inspection_failed` without `note`. Both corrected. - Extracted the duplicated 5-line "corrupt the index" setup into `_break_git_index()` beside the file's other module-level helpers. Validation: 19/19 tests/tools/test_subagent_worktree.py; ruff clean. New schema guard mutation-checked -- reverting delegate_tool's fallback to the pre-fix `dict(_worktree_info)` shape fails it. Restores checksum-verified.
This commit is contained in:
kshitij
@@ -4,10 +4,13 @@ Inspired by Muse Code's --subagent-worktree-isolation (clean-room
|
||||
implementation from documented behavior).
|
||||
"""
|
||||
|
||||
import ast
|
||||
import inspect
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
import textwrap
|
||||
import shutil
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
@@ -36,6 +39,14 @@ def _make_repo(root: Path) -> Path:
|
||||
return repo
|
||||
|
||||
|
||||
def _break_git_index(wt: Path) -> None:
|
||||
"""Corrupt a worktree's index so the real git probes exit non-zero."""
|
||||
git_dir = Path(_git(["rev-parse", "--git-dir"], wt).stdout.strip())
|
||||
if not git_dir.is_absolute():
|
||||
git_dir = (wt / git_dir).resolve()
|
||||
(git_dir / "index").write_bytes(b"not-a-valid-git-index\n")
|
||||
|
||||
|
||||
class SubagentWorktreeTests(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = Path(tempfile.mkdtemp(prefix="hermes-sw-test-"))
|
||||
@@ -166,19 +177,22 @@ class SubagentWorktreeTests(unittest.TestCase):
|
||||
# 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.
|
||||
# preserved never gets looked at. Assert the actionable invariants
|
||||
# (flag set; note names the worktree + branch), not the prose.
|
||||
self.assertTrue(payload["inspection_failed"])
|
||||
self.assertIn("UNKNOWN", payload["note"])
|
||||
self.assertIn(info["path"], payload["note"])
|
||||
self.assertIn(info["branch"], 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.
|
||||
preserved" and "inspected fine, child left nothing" are
|
||||
indistinguishable to the parent agent — so its rational reading of the
|
||||
failure case is the exact wrong conclusion. The contract asserted here
|
||||
is *distinguishability*, not the specific field values (a future
|
||||
change emitting ``commits: None`` for "unknown" would be strictly
|
||||
better and must not break this test).
|
||||
"""
|
||||
repo = _make_repo(self.tmp)
|
||||
|
||||
@@ -192,19 +206,22 @@ class SubagentWorktreeTests(unittest.TestCase):
|
||||
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")
|
||||
_break_git_index(bad_wt)
|
||||
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)
|
||||
# Both keep the worktree, so "pruned" alone cannot separate them...
|
||||
self.assertFalse(ok_payload["pruned"])
|
||||
self.assertFalse(bad_payload["pruned"])
|
||||
self.assertTrue(os.path.isdir(bad_info["path"]))
|
||||
self.assertTrue((bad_wt / "WIP.txt").exists())
|
||||
# ...the flag must. Tolerant of an always-present-but-False refactor.
|
||||
self.assertFalse(ok_payload.get("inspection_failed", False))
|
||||
self.assertTrue(bad_payload["inspection_failed"])
|
||||
self.assertNotEqual(
|
||||
bool(ok_payload.get("inspection_failed")),
|
||||
bool(bad_payload.get("inspection_failed")),
|
||||
)
|
||||
self.assertFalse((ok_payload.get("note") or "").strip())
|
||||
|
||||
def test_finalize_flags_unproven_state_when_inspection_raises(self):
|
||||
"""A raising probe is the same unknown state as a non-zero exit."""
|
||||
@@ -215,13 +232,18 @@ class SubagentWorktreeTests(unittest.TestCase):
|
||||
def _boom(*_a, **_k):
|
||||
raise subprocess.TimeoutExpired(cmd="git", timeout=30)
|
||||
|
||||
with mock.patch.object(sw, "_run_git", side_effect=_boom):
|
||||
with mock.patch.object(sw, "_run_git", side_effect=_boom) as m:
|
||||
payload = sw.finalize_subagent_worktree(info)
|
||||
|
||||
# Prove the patched seam was actually exercised.
|
||||
self.assertGreaterEqual(m.call_count, 1)
|
||||
self.assertFalse(payload["pruned"])
|
||||
self.assertTrue(payload["inspection_failed"])
|
||||
self.assertIn("UNKNOWN", payload["note"])
|
||||
self.assertIn(info["path"], payload["note"])
|
||||
self.assertIn(info["branch"], payload["note"])
|
||||
self.assertTrue(os.path.isdir(info["path"]))
|
||||
branches = _git(["branch", "--list", info["branch"]], repo).stdout
|
||||
self.assertNotEqual(branches.strip(), "")
|
||||
|
||||
def test_finalize_missing_path_reports_pruned(self):
|
||||
payload = sw.finalize_subagent_worktree(
|
||||
@@ -257,6 +279,68 @@ class SubagentWorktreeTests(unittest.TestCase):
|
||||
self.assertIn("WORKTREE ISOLATION", note)
|
||||
|
||||
|
||||
class WorktreePayloadSchemaTests(unittest.TestCase):
|
||||
"""The parent agent reads ONE schema under ``entry["worktree"]``.
|
||||
|
||||
Two producers write it: ``finalize_subagent_worktree`` and
|
||||
``delegate_tool``'s fallback for when finalize itself raises. Compare the
|
||||
fallback against the REAL finalize output (not a hardcoded key list) so
|
||||
the two can never drift apart.
|
||||
"""
|
||||
|
||||
def setUp(self):
|
||||
self.tmp = Path(tempfile.mkdtemp(prefix="hermes-sw-schema-"))
|
||||
self.addCleanup(shutil.rmtree, self.tmp, True)
|
||||
|
||||
def test_fallback_matches_finalize_schema(self):
|
||||
from tools import delegate_tool
|
||||
|
||||
repo = _make_repo(self.tmp)
|
||||
info = sw.create_subagent_worktree(str(repo), "schema")
|
||||
assert info is not None
|
||||
happy = sw.finalize_subagent_worktree(info, prune=False)
|
||||
|
||||
# Read the fallback's ACTUAL keys out of the source (not a hardcoded
|
||||
# copy), so drift in delegate_tool is what fails this test.
|
||||
tree = ast.parse(
|
||||
textwrap.dedent(inspect.getsource(delegate_tool._run_single_child))
|
||||
)
|
||||
fallback_keys = None
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Assign) or not isinstance(
|
||||
node.value, ast.Dict
|
||||
):
|
||||
continue
|
||||
target = node.targets[0]
|
||||
if (
|
||||
isinstance(target, ast.Subscript)
|
||||
and isinstance(target.slice, ast.Constant)
|
||||
and target.slice.value == "worktree"
|
||||
):
|
||||
fallback_keys = {
|
||||
k.value
|
||||
for k in node.value.keys
|
||||
if isinstance(k, ast.Constant)
|
||||
}
|
||||
break
|
||||
|
||||
self.assertIsNotNone(
|
||||
fallback_keys,
|
||||
"delegate_tool no longer assigns a dict literal to "
|
||||
'entry_dict["worktree"] — update this schema guard',
|
||||
)
|
||||
assert fallback_keys is not None # narrow for type checkers
|
||||
# Fallback must be a superset: every happy-path key plus the two
|
||||
# unproven-state keys, and nothing invented. The pre-fix version
|
||||
# emitted repo_root/base_commit and omitted commits/dirty/pruned.
|
||||
self.assertTrue(set(happy).issubset(fallback_keys))
|
||||
self.assertEqual(
|
||||
fallback_keys - set(happy), {"inspection_failed", "note"}
|
||||
)
|
||||
for leaked in ("repo_root", "base_commit"):
|
||||
self.assertNotIn(leaked, fallback_keys)
|
||||
|
||||
|
||||
class DelegationConfigGateTests(unittest.TestCase):
|
||||
def test_worktree_isolation_default_off(self):
|
||||
from tools import delegate_tool
|
||||
|
||||
@@ -2520,8 +2520,10 @@ def _run_single_child(
|
||||
"pruned": False,
|
||||
"inspection_failed": True,
|
||||
"note": (
|
||||
"worktree finalize raised; state unknown — inspect "
|
||||
f"{_worktree_info.get('path', '')} manually before "
|
||||
"worktree finalize raised: 'commits' and 'dirty' are "
|
||||
"UNKNOWN, not zero/clean. The worktree and branch were "
|
||||
f"preserved — inspect {_worktree_info.get('path', '')} "
|
||||
f"(branch {_worktree_info.get('branch', '')}) before "
|
||||
"assuming no work."
|
||||
),
|
||||
}
|
||||
|
||||
@@ -28,7 +28,7 @@ Contract (mirrors Muse Code's documented semantics):
|
||||
clean tree is removed automatically after the child finishes; anything
|
||||
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).
|
||||
kept and the result entry carries ``inspection_failed`` + ``note`` (#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
|
||||
|
||||
@@ -412,10 +412,11 @@ With isolation on:
|
||||
reviews or merges each branch (`git log <branch>`, `git merge <branch>`).
|
||||
- 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.
|
||||
- Pruning requires proof. If a git inspection probe fails — or finalization
|
||||
itself errors — 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
|
||||
|
||||
Reference in New Issue
Block a user