Skip to content

Commit 61e2c7c

Browse files
wongkclaude
andcommitted
fix(code-review): attribute run-prefix stage-failure diagnostics per stage
Addresses review of PR #173 (all MEDIUM, non-blocking): - Capture each in-process stage's stderr and fold its tail into the returned status message, so a failing stage's own diagnostic is attributed to that stage now that one process spans many stages (was: bare process stderr with no stage attribution). Written back to the real stderr so live visibility is unchanged. - Thread that diagnostic into _emit_prefix_stage_failure_finding: the continue_with_coverage_gap agent-failure finding now carries the actual rc/exception/stderr in its explanation instead of a generic string. - Log the full traceback on an unexpected stage crash (the broad except no longer discards it), so a Phase-1 crash is diagnosable without reproduction. - SCHEMA.md §7b: document that ready_for_route with resume_stage=null means the walk completed with no partition stage (skip Gate B), distinct from the normal partition boundary. The docstring subparser-count finding was already correct (46). Adds a regression test asserting the stderr diagnostic reaches the finding explanation. Full code-review suite 1300 passed / 3 skipped; ruff + pyright clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent ad2293b commit 61e2c7c

4 files changed

Lines changed: 76 additions & 16 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
99
#### Added
1010
- New `run-prefix` helper subcommand: a resumable, in-process runner for the deterministic review prefix (stages `setup` through `cache-check`). It reads `run_plan.json` and walks the stages in one process — resolving each stage's placeholder tokens from prior-stage artifacts, redirecting stdout per stage, and honoring `on_failure` policies and validation gates — instead of one orchestrator turn per stage. It pauses only at genuine decision points (the hygiene-only early exit, a signal-extraction or coverage-critic singleton that needs an agent, or the route/partition boundary), emitting a status JSON that tells the orchestrator what to do next, and resumes from a given stage on re-invocation. A failed `continue_with_coverage_gap` stage emits a canonical `agent-failure` system finding so the gap is auditable. Documented as the `run-prefix` result contract in `SCHEMA.md`.
1111
- Subprocess A/B parity oracle for the prefix: the golden-fixture harness now walks each fixture two ways — one subprocess per stage (reproducing the current per-stage orchestrator walk) versus the new `run-prefix` runner — and asserts byte-identical normalized artifacts through `cache-check` across all seven fixtures, plus a pause-sequence check pinning the resumable segment boundaries. Contract tests cover token resolution, resumable dependency reconstruction, singleton detection, `on_failure` handling, and the runner's error/boundary returns.
12+
- `run-prefix` failure diagnostics are attributed per stage: each in-process stage's stderr is captured and folded into the returned status message (and, for a `continue_with_coverage_gap` stage, into the emitted `agent-failure` finding's explanation), and an unexpected stage crash logs its full traceback — so a batched-runner failure stays diagnosable without reproducing it, now that one process spans many stages.
1213

1314
### code-review v3.1.1
1415

plugins/code-review/SCHEMA.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,13 @@ on disk (a prior stage counts as done iff its literal `expected_outputs` exist).
525525
| `ready_for_route` | reaching `stage_17_partition` | Run Gate B (`route`) + partition + the rest of the walk. |
526526
| `error` | a stage aborted / a gate failed | Fall back to the per-stage walk from `failed_stage`; partials are preserved. |
527527

528+
`ready_for_route` distinguishes its two cases by `resume_stage`: a non-null
529+
`resume_stage` (`stage_17_partition`) is the normal boundary — run Gate B +
530+
partition from there. A **null** `resume_stage` means the walk reached the end
531+
of the plan without a partition stage (e.g. a depth tier that filters partition
532+
out); there is nothing left to route, so the orchestrator skips Gate B and
533+
partition and proceeds directly to the reviewer fleet.
534+
528535
The exit code is `0` for every well-formed result (including `error`) — the
529536
`next_action` field is the contract. Route + partition (Segment 3) fold into the
530537
runner in Phase 2, at which point `ready_for_route` becomes `ready_for_reviewers`

plugins/code-review/tools/python/code_review_helpers.py

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import stat
2727
import subprocess
2828
import sys
29+
import traceback
2930
import uuid
3031
from collections.abc import Generator
3132
from dataclasses import dataclass, field
@@ -10566,14 +10567,27 @@ def _rp_dispatch(func: Any, ns: argparse.Namespace, stdout_path: Path | None) ->
1056610567
return int(rc or 0)
1056710568

1056810569

10569-
def _emit_prefix_stage_failure_finding(cr_dir: Path, stage_id: str) -> None:
10570+
def _emit_prefix_stage_failure_finding(
10571+
cr_dir: Path, stage_id: str, diagnostic: str | None = None,
10572+
) -> None:
1057010573
"""Write ``agent_<stage>-failed.json`` with a canonical agent-failure finding.
1057110574

1057210575
Mirrors start.md Walker Contract step 5 (``continue_with_coverage_gap``): a
1057310576
prefix stage that failed to run must surface as an operator-visible,
1057410577
collectable finding — ``collect-findings`` globs ``agent_*.json`` — rather
10575-
than vanish silently. Fail-open on write error (telemetry is observational).
10576-
"""
10578+
than vanish silently. ``diagnostic`` is the caller's captured failure detail
10579+
(rc / exception / the stage's own stderr tail); it is folded into the
10580+
finding's explanation so the gap is debuggable without reproducing the
10581+
failure. Fail-open on write error (telemetry is observational).
10582+
"""
10583+
explanation = (
10584+
f"The deterministic prefix stage {stage_id!r} exited non-zero or "
10585+
"produced no output. Its on_failure policy is "
10586+
"continue_with_coverage_gap, so the pipeline proceeds but the gap is "
10587+
"recorded here for auditability."
10588+
)
10589+
if diagnostic:
10590+
explanation += f" Diagnostic: {diagnostic}"
1057710591
finding = {
1057810592
"reviewer": "foundation",
1057910593
"source": "foundation",
@@ -10584,12 +10598,7 @@ def _emit_prefix_stage_failure_finding(cr_dir: Path, stage_id: str) -> None:
1058410598
"file": None,
1058510599
"line": None,
1058610600
"issue": f"Prefix stage {stage_id} failed; continuing with coverage gap.",
10587-
"explanation": (
10588-
f"The deterministic prefix stage {stage_id!r} exited non-zero or "
10589-
"produced no output. Its on_failure policy is "
10590-
"continue_with_coverage_gap, so the pipeline proceeds but the gap is "
10591-
"recorded here for auditability."
10592-
),
10601+
"explanation": explanation,
1059310602
"recommendation": (
1059410603
"Re-run the review once the underlying issue is resolved. Common "
1059510604
"causes: malformed diff input, a git error, or a taxonomy mismatch "
@@ -10650,28 +10659,45 @@ def _execute_stage_inprocess(
1065010659
on_failure = stage.get("on_failure", "abort")
1065110660
rc = 1
1065210661
message: str | None = None
10662+
# Capture the stage's stderr so a failing stage's own diagnostic (the
10663+
# cmd_* handlers report via ``print(..., file=sys.stderr)``) can be attributed
10664+
# to THIS stage in the batched model — one process now runs many stages, so
10665+
# bare process stderr no longer maps to an orchestrator turn. It is written
10666+
# back to the real stderr below, so live visibility is unchanged.
10667+
err_buf = io.StringIO()
1065310668
try:
10654-
resolved = _rp_resolve_args(stage.get("args", []) or [], ctx)
10655-
ns = parser.parse_args([stage["subcommand"], *resolved])
10656-
stdout_target = stage.get("stdout")
10657-
rc = _rp_dispatch(
10658-
ns.func, ns, Path(stdout_target) if stdout_target else None,
10659-
)
10669+
with contextlib.redirect_stderr(err_buf):
10670+
resolved = _rp_resolve_args(stage.get("args", []) or [], ctx)
10671+
ns = parser.parse_args([stage["subcommand"], *resolved])
10672+
stdout_target = stage.get("stdout")
10673+
rc = _rp_dispatch(
10674+
ns.func, ns, Path(stdout_target) if stdout_target else None,
10675+
)
1066010676
except SystemExit as exc: # argparse rejected the resolved args
1066110677
message = f"argparse rejected args for {stage_id}: {exc}"
1066210678
rc = 1
1066310679
except Exception as exc: # noqa: BLE001 — a crash is a stage failure; on_failure decides
1066410680
message = f"{type(exc).__name__} in {stage_id}: {exc}"
1066510681
rc = 1
10682+
# Preserve the full traceback on the real stderr — the short ``message``
10683+
# alone can't be debugged without reproducing the crash.
10684+
traceback.print_exc()
10685+
10686+
stage_stderr = err_buf.getvalue()
10687+
if stage_stderr:
10688+
sys.stderr.write(stage_stderr)
1066610689

1066710690
outputs_ok = message is None and _rp_outputs_present(stage)
1066810691
if rc != 0 or not outputs_ok:
1066910692
if message is None:
1067010693
message = f"stage {stage_id} failed (rc={rc}, outputs_ok={outputs_ok})"
10694+
stderr_tail = stage_stderr.strip().splitlines()
10695+
if stderr_tail:
10696+
message = f"{message}; stderr: {stderr_tail[-1][:300]}"
1067110697
if on_failure == "abort":
1067210698
return "failed_abort", message
1067310699
if on_failure == "continue_with_coverage_gap":
10674-
_emit_prefix_stage_failure_finding(ctx.cr_dir, stage_id)
10700+
_emit_prefix_stage_failure_finding(ctx.cr_dir, stage_id, message)
1067510701
return "failed_continue", message
1067610702

1067710703
_rp_apply_post_stage_overrides(stage_id, ctx)

plugins/code-review/tools/python/test_code_review_helpers.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22608,6 +22608,32 @@ def test_continue_with_coverage_gap_emits_agent_failure_finding(
2260822608
assert marker == "agent-failure"
2260922609
assert payload["findings"][0]["finding_scope"] == "system"
2261022610

22611+
def test_coverage_gap_finding_threads_stage_stderr_diagnostic(
22612+
self, tmp_path: Path,
22613+
) -> None:
22614+
from code_review_helpers import _execute_stage_inprocess
22615+
22616+
out = tmp_path / "o.json"
22617+
22618+
def _fail_with_stderr(_ns: argparse.Namespace) -> int:
22619+
print("Error: taxonomy mismatch after edit", file=sys.stderr)
22620+
return 1
22621+
22622+
parser = _fake_stage_parser(_fail_with_stderr)
22623+
status, message = _execute_stage_inprocess(
22624+
self._stage(out, id="stage_14_resolve_coverage",
22625+
on_failure="continue_with_coverage_gap"),
22626+
_rp_ctx(tmp_path), parser, set(),
22627+
)
22628+
assert status == "failed_continue"
22629+
# The stage's own stderr is attributed to the stage in the message ...
22630+
assert "taxonomy mismatch after edit" in (message or "")
22631+
# ... and folded into the emitted finding's explanation (not discarded).
22632+
payload = json.loads(
22633+
(tmp_path / "agent_stage_14_resolve_coverage-failed.json").read_text(),
22634+
)
22635+
assert "taxonomy mismatch after edit" in payload["findings"][0]["explanation"]
22636+
2261122637
def test_skip_when_disabled(self, tmp_path: Path) -> None:
2261222638
from code_review_helpers import _execute_stage_inprocess
2261322639

0 commit comments

Comments
 (0)