diff --git a/tests/tools/test_subagent_worktree.py b/tests/tools/test_subagent_worktree.py index 027f44df729bf..d452151063ba0 100644 --- a/tests/tools/test_subagent_worktree.py +++ b/tests/tools/test_subagent_worktree.py @@ -18,9 +18,9 @@ from tools import subagent_worktree as sw # noqa: E402 -def _git(args, cwd): +def _git(args, cwd, check=True): return subprocess.run( - ["git", *args], cwd=cwd, capture_output=True, text=True, check=True + ["git", *args], cwd=cwd, capture_output=True, text=True, check=check ) @@ -132,6 +132,38 @@ def test_finalize_keeps_dirty_worktree(self): self.assertTrue(payload["dirty"]) self.assertTrue(os.path.isdir(info["path"])) + def test_finalize_keeps_worktree_when_git_inspection_fails(self): + """#88113: a non-zero git status exit must not be read as "clean". + + Corrupting the index makes the real `git status --porcelain` probe + exit 128. The old code kept the payload defaults (commits=0, + dirty=False) and pruned on them — permanently deleting the child's + uncommitted work. A destructive cleanup requires affirmative proof + of a clean tree.""" + repo = _make_repo(self.tmp) + info = sw.create_subagent_worktree(str(repo), "inspect-fail1") + assert info is not None + wt = Path(info["path"]) + (wt / "UNCOMMITTED-WORK.txt").write_text( + "irreplaceable\n", encoding="utf-8" + ) + + 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") + # Sanity: the probe really fails now. + broken = _git(["status", "--porcelain"], wt, check=False) + self.assertNotEqual(broken.returncode, 0) + + payload = sw.finalize_subagent_worktree(info) + + self.assertFalse(payload["pruned"]) + self.assertTrue(os.path.isdir(info["path"])) + self.assertTrue((wt / "UNCOMMITTED-WORK.txt").exists()) + 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( {"path": str(self.tmp / "gone"), "branch": "b", "repo_root": "", diff --git a/tools/subagent_worktree.py b/tools/subagent_worktree.py index 4b1ab0b6e04c1..e401b3c1a901b 100644 --- a/tools/subagent_worktree.py +++ b/tools/subagent_worktree.py @@ -196,6 +196,7 @@ def finalize_subagent_worktree( payload["pruned"] = True # nothing on disk to review return payload + inspection_ok = True try: if base_commit: counted = _run_git( @@ -203,14 +204,33 @@ def finalize_subagent_worktree( ) if counted.returncode == 0: payload["commits"] = int(counted.stdout.strip() or 0) + else: + inspection_ok = False status = _run_git(["status", "--porcelain"], cwd=path) if status.returncode == 0: payload["dirty"] = bool(status.stdout.strip()) + 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 + if not inspection_ok: + # Fail-safe (#88113): a non-zero git exit proves nothing about the + # tree — the payload defaults (0 commits, clean) were never + # overwritten, and pruning on them permanently deleted uncommitted + # 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 + if prune and payload["commits"] == 0 and not payload["dirty"]: try: removed = _run_git(