fix(review): harden target failure boundaries

Author: elkaix

docs/superpowers/specs/2026-07-15-review-target-resolution-design.md

@@ -117,7 +117,9 @@ All Git operations use the existing async host process boundary with argv elemen
 The current best-effort Git-context helper deliberately collapses command failures to `None`; the
 resolver instead uses a checked wrapper at that same boundary so it can distinguish timeout,
 non-zero exit, and unavailable Git while still exposing only safe categorized errors. Reads are
-bounded, and every timed-out process is killed and reaped.
+bounded. Timeout and cancellation paths make bounded kill, drain, and reap attempts only when a
+process handle exists; cleanup failures never replace the primary failure, and a failed host kill
+can leave the child unreaped.
 
 Every user-provided ref must pass the shape rules above and resolve with
 `git rev-parse --verify --end-of-options <ref>^{commit}`. The explicit option boundary remains even

src/pythinker_code/subagents/git_context.py

@@ -108,27 +108,28 @@ async def run_git(
         raise ValueError("max_output_bytes must be positive")
     proc: HostProcess | None = None
     completion: asyncio.Task[tuple[int, tuple[bytes, bool], tuple[bytes, bool]]] | None = None
+    deadline = asyncio.get_running_loop().time() + timeout
     try:
-        proc = await pythinker_host.exec(
-            "git",
-            "--no-pager",
-            "--no-optional-locks",
-            "-c",
-            "core.fsmonitor=false",
-            "-c",
-            "log.showSignature=false",
-            "-C",
-            cwd,
-            *args,
-        )
-        proc.stdin.close()
-        completion = asyncio.create_task(_collect_process(proc, max_output_bytes))
         try:
-            returncode, stdout_result, stderr_result = await asyncio.wait_for(
-                asyncio.shield(completion), timeout=timeout
-            )
+            async with asyncio.timeout_at(deadline):
+                proc = await pythinker_host.exec(
+                    "git",
+                    "--no-pager",
+                    "--no-optional-locks",
+                    "-c",
+                    "core.fsmonitor=false",
+                    "-c",
+                    "log.showSignature=false",
+                    "-C",
+                    cwd,
+                    *args,
+                )
+                proc.stdin.close()
+                completion = asyncio.create_task(_collect_process(proc, max_output_bytes))
+                returncode, stdout_result, stderr_result = await asyncio.shield(completion)
         except TimeoutError as exc:
-            await _cleanup_process(proc, completion)
+            if proc is not None:
+                await _cleanup_process(proc, completion)
             raise GitCommandError("timeout", args[0] if args else "command") from exc
         stdout_bytes, stdout_truncated = stdout_result
         stderr_bytes, stderr_truncated = stderr_result

src/pythinker_code/subagents/review_target.py

@@ -24,10 +24,15 @@
 
 REVIEWER_AGENT_TYPES = frozenset({"review", "code-reviewer", "security-reviewer"})
 MAX_REVIEW_REF_CHARS = 1024
+_FULL_OID_LENGTHS = frozenset({40, 64})
 
 
 class ReviewTarget(BaseModel):
-    model_config = ConfigDict(frozen=True)
+    model_config = ConfigDict(
+        frozen=True,
+        extra="allow",
+        json_schema_extra={"additionalProperties": False},
+    )
 
     kind: ReviewTargetKind = Field(
         default="auto",
@@ -96,6 +101,12 @@ def __init__(self, code: ReviewTargetErrorCode, brief: str, message: str) -> Non
 
 
 def validate_review_target(target: ReviewTarget) -> None:
+    if target.model_extra:
+        raise ReviewTargetResolutionError(
+            ReviewTargetErrorCode.invalid_target,
+            "Invalid review target",
+            "review_target contains unsupported fields.",
+        )
     ref = target.ref
     if target.kind == "commit" and ref is None:
         raise ReviewTargetResolutionError(
@@ -158,6 +169,7 @@ def _require_oid(result: GitCommandResult, *, message: str) -> str:
         or not value
         or "\n" in value
         or "\r" in value
+        or len(value) not in _FULL_OID_LENGTHS
         or any(char not in string.hexdigits for char in value)
     ):
         raise ReviewTargetResolutionError(
@@ -168,26 +180,48 @@ def _require_oid(result: GitCommandResult, *, message: str) -> str:
     return value.lower()
 
 
+def _quiet_verification_is_missing(result: GitCommandResult) -> bool:
+    return (
+        result.returncode == 1
+        and not result.stdout
+        and not result.stderr
+        and not result.stdout_truncated
+        and not result.stderr_truncated
+    )
+
+
+def _raise_commit_verification_failed() -> None:
+    raise ReviewTargetResolutionError(
+        ReviewTargetErrorCode.git_failed,
+        "Review target unavailable",
+        "Git could not verify the requested commit.",
+    )
+
+
 async def _try_resolve_commit(cwd: str, ref: str) -> str | None:
     result = await _run_resolver_git(
-        ["rev-parse", "--verify", "--end-of-options", f"{ref}^{{commit}}"],
+        ["rev-parse", "--verify", "--quiet", "--end-of-options", f"{ref}^{{commit}}"],
         cwd,
     )
-    if result.returncode != 0:
+    if _quiet_verification_is_missing(result):
         return None
+    if result.returncode != 0:
+        _raise_commit_verification_failed()
     return _require_oid(result, message="Git returned an invalid commit identifier.")
 
 
 async def _resolve_commit(cwd: str, ref: str) -> str:
     result = await _run_resolver_git(
-        ["rev-parse", "--verify", "--end-of-options", f"{ref}^{{commit}}"], cwd
+        ["rev-parse", "--verify", "--quiet", "--end-of-options", f"{ref}^{{commit}}"], cwd
     )
-    if result.returncode != 0:
+    if _quiet_verification_is_missing(result):
         raise ReviewTargetResolutionError(
             ReviewTargetErrorCode.missing_ref,
             "Review target unavailable",
             "The requested Git ref does not resolve to a commit.",
         )
+    if result.returncode != 0:
+        _raise_commit_verification_failed()
     return _require_oid(result, message="Git returned an invalid commit identifier.")
 
 
@@ -293,14 +327,19 @@ async def _commit_details(cwd: str, target_sha: str) -> tuple[tuple[str, ...], s
     if parents_text.endswith("\r"):
         parents_text = parents_text[:-1]
     tokens = parents_text.split()
+    oid_length = len(target_sha)
     if (
         parents_result.returncode != 0
         or parents_result.stdout_truncated
         or not tokens
         or "\n" in parents_text
         or "\r" in parents_text
-        or tokens[0].lower() != target_sha
-        or any(any(char not in string.hexdigits for char in token) for token in tokens)
+        or oid_length not in _FULL_OID_LENGTHS
+        or tokens[0].lower() != target_sha.lower()
+        or any(
+            len(token) != oid_length or any(char not in string.hexdigits for char in token)
+            for token in tokens
+        )
     ):
         raise ReviewTargetResolutionError(
             ReviewTargetErrorCode.git_failed,

src/pythinker_code/tools/agent/__init__.py

@@ -557,6 +557,8 @@ async def __call__(self, params: Params) -> ToolReturnValue:
             # Malformed resume id (store.instance_dir validates [A-Za-z0-9_-]{1,64}).
             logger.warning("Foreground agent resume id was malformed: {err}", err=exc)
             return ToolError(message=str(exc), brief="Agent not found")
+        except ReviewTargetResolutionError as exc:
+            return ToolError(message=str(exc), brief=exc.brief)
         except RuntimeError as exc:
             if "cannot be resumed concurrently" in str(exc):
                 logger.warning("Foreground agent resume rejected: {err}", err=exc)

tests/background/test_manager.py

@@ -23,7 +23,12 @@
 from pythinker_code.soul.context import Context
 from pythinker_code.subagents import AgentLaunchSpec, AgentTypeDefinition, ToolPolicy
 from pythinker_code.subagents.core import SubagentRunSpec
-from pythinker_code.subagents.review_target import ResolvedReviewTarget, WorktreeChanges
+from pythinker_code.subagents.review_target import (
+    ResolvedReviewTarget,
+    ReviewTargetErrorCode,
+    ReviewTargetResolutionError,
+    WorktreeChanges,
+)
 from pythinker_code.wire.types import TextPart
 
 
@@ -464,6 +469,74 @@ async def capture_prepare_soul(spec, runtime, builder, store, on_stage=None):
     assert captured[0].resolved_review_target == target
 
 
+@pytest.mark.asyncio
+async def test_background_review_target_drift_records_safe_failed_state(
+    runtime,
+    monkeypatch,
+) -> None:
+    runtime.labor_market.add_builtin_type(
+        AgentTypeDefinition(
+            name="code-reviewer",
+            description="Test code reviewer.",
+            agent_file=runtime.subagent_store.root / "code-reviewer.yaml",
+            tool_policy=ToolPolicy(mode="inherit"),
+        )
+    )
+    runtime.subagent_store.create_instance(
+        agent_id="adriftbg",
+        description="review drift",
+        launch_spec=AgentLaunchSpec(
+            agent_id="adriftbg",
+            subagent_type="code-reviewer",
+            model_override=None,
+            effective_model=None,
+        ),
+    )
+    target = ResolvedReviewTarget(
+        requested_kind="base",
+        requested_ref="main",
+        kind="base",
+        head_sha="b" * 40,
+        base_ref="main",
+        base_sha="c" * 40,
+        merge_base_sha="a" * 40,
+        attempted_base_refs=("main",),
+        worktree_changes=WorktreeChanges(staged=False, unstaged=False, untracked=False),
+        worktree_state="live",
+        prompt="<review-target>runtime scope</review-target>",
+        hint="base main",
+    )
+    monkeypatch.setattr(
+        "pythinker_code.background.agent_runner.prepare_soul",
+        AsyncMock(
+            side_effect=ReviewTargetResolutionError(
+                ReviewTargetErrorCode.head_moved,
+                "Review target changed",
+                "HEAD changed before background execution.",
+            )
+        ),
+    )
+
+    view = runtime.background_tasks.create_agent_task(
+        agent_id="adriftbg",
+        subagent_type="code-reviewer",
+        prompt="review current changes",
+        description="review drift",
+        tool_call_id="tool-review-drift",
+        model_override=None,
+        resolved_review_target=target,
+    )
+    await runtime.background_tasks._live_agent_tasks[view.spec.id]
+
+    failed = runtime.background_tasks.store.merged_view(view.spec.id)
+    output = runtime.background_tasks.store.output_path(view.spec.id).read_text(encoding="utf-8")
+    assert failed.runtime.status == "failed"
+    assert failed.runtime.failure_reason == "HEAD changed before background execution."
+    assert runtime.subagent_store.require_instance("adriftbg").status == "failed"
+    assert "HEAD changed before background execution." in output
+    assert "[summary]" not in output
+
+
 @pytest.mark.asyncio
 async def test_background_agent_resume_restores_system_prompt_from_context(runtime, monkeypatch):
     runtime.labor_market.add_builtin_type(

tests/core/test_default_agent.py

@@ -488,6 +488,7 @@ async def test_default_agent_background_bash_guardrails(runtime: Runtime):
                 "review_target": {
                     "anyOf": [
                         {
+                            "additionalProperties": False,
                             "properties": {
                                 "kind": {
                                     "default": "auto",