From 4e72c6f5e630d1e26e9772e5b5b417ec8e63261d Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Sat, 12 Sep 2026 17:16:48 -0600 Subject: [PATCH] feat(TASK-019): code-reviewer fail-closed review pipeline Every llm/agent stage that can die persistently now falls back to a machine-readable PIPELINE-FAULT sentinel instead of killing the graph: - review_domain -> branch-local domain_fault marker (banner-prefixed domain_report the map collects; completeness_gate precedent) - verify -> verdict (verifier fault renders findings unverified + a PIPELINE-FAULT attention line) - parse -> parse_fault -> render (complete NEEDS-HUMAN verdict_out; render's no-changes stub is bypassed for fault verdicts) - refine_groups -> cover_gate (deterministic-proposal fallback absorbs the failure; degradation noted in groups_note) - aux_lanes -> aux_fault -> synthesize (aux_note fault marker; visible, never blocking - aux is enrichment) - synthesize -> verdict (synth_failure marker; keyed off the failure prefix + non-empty diff, never findings==[], so clean zero-finding reviews stay MERGE-READY) verdict.py gains unified fault accounting (R2): any PIPELINE-FAULT in domain_reports / verifier_output / state fault keys forces NEEDS-HUMAN, prefix-anchored so a report merely quoting the marker mid-text never trips it; old 'PIPELINE FAULT' spelling migrated. Faults are prepended to attention so the [:5] cap can never hide one. Dedicated *_failure state keys capture engine failure text on every fallback+output_schema node, keeping the bundled warning baseline at zero. 15 python3-guarded R5 tests: fault paths, quoted-marker precedence, happy-path byte-compat regression, marker normalization, crash-path fail-closed (verdict/cover_gate/domain_fault), verifier-fault-on-empty- diff render, and the no-changes stub regression. --- assets/agents/code-reviewer/graph.yaml | 41 ++ .../agents/code-reviewer/scripts/aux_fault.py | 48 ++ .../code-reviewer/scripts/cover_gate.py | 5 + .../code-reviewer/scripts/domain_fault.py | 59 +++ .../code-reviewer/scripts/parse_fault.py | 58 +++ assets/agents/code-reviewer/scripts/render.py | 9 +- .../agents/code-reviewer/scripts/verdict.py | 44 +- src/config/request_context.rs | 427 ++++++++++++++++++ 8 files changed, 687 insertions(+), 4 deletions(-) create mode 100644 assets/agents/code-reviewer/scripts/aux_fault.py create mode 100644 assets/agents/code-reviewer/scripts/domain_fault.py create mode 100644 assets/agents/code-reviewer/scripts/parse_fault.py diff --git a/assets/agents/code-reviewer/graph.yaml b/assets/agents/code-reviewer/graph.yaml index 464628a..e81516e 100644 --- a/assets/agents/code-reviewer/graph.yaml +++ b/assets/agents/code-reviewer/graph.yaml @@ -91,6 +91,10 @@ initial_state: final_report: '' gate_feedback: '' gate_attempts: 0 + parse_failure: '' + refine_failure: '' + aux_failure: '' + synth_failure: '' start: parse @@ -139,8 +143,19 @@ nodes: required: [diff_spec, intent, rigor_in, surfaces_in, project_dir_in, extra_context] max_iterations: 3 max_attempts: 2 + state_updates: + parse_failure: '{{output}}' + fallback: parse_fault next: facts + parse_fault: + id: parse_fault + type: script + description: Fail-closed marker for a dead parse stage (reached only via parse's fallback) — emits a complete PIPELINE-FAULT verdict_out and jumps straight to render; a review that could not parse its request never reads as MERGE-READY. + script: scripts/parse_fault.py + timeout: 30 + next: render + facts: id: facts type: script @@ -209,6 +224,9 @@ nodes: required: [groups] max_iterations: 3 max_attempts: 2 + state_updates: + refine_failure: '{{output}}' + fallback: cover_gate next: cover_gate cover_gate: @@ -275,6 +293,7 @@ nodes: max_attempts: 2 state_updates: domain_report: '{{output}}' + fallback: domain_fault next: completeness_gate completeness_gate: @@ -284,6 +303,13 @@ nodes: script: scripts/completeness_gate.py timeout: 30 + domain_fault: + id: domain_fault + type: script + description: Fail-closed domain lane fault marker (reached only via review_domain's fallback route inside the map branch). Normalizes the engine's "Agent node failed" text in domain_report into a > ⚠️ PIPELINE-FAULT banner the map collects; verdict.py counts any faulted lane and forces NEEDS-HUMAN — a dead domain lane never reads as a clean slice. + script: scripts/domain_fault.py + timeout: 30 + aux_lanes: id: aux_lanes type: llm @@ -339,6 +365,17 @@ nodes: max_iterations: 10 max_attempts: 2 timeout: 3600 + state_updates: + aux_failure: '{{output}}' + fallback: aux_fault + next: synthesize + + aux_fault: + id: aux_fault + type: script + description: Fail-closed marker for dead aux context lanes (reached only via aux_lanes' fallback) — records the failure as a PIPELINE-FAULT in aux_note, which synthesize surfaces as a lane skip. Aux lanes are enrichment; the degradation is visible but never blocks. + script: scripts/aux_fault.py + timeout: 30 next: synthesize downstream_sweep: @@ -457,6 +494,9 @@ nodes: required: [walkthrough, changes_rows, findings, cross_file, operational_history, org_context_section, attention_flags, placement_notes] max_iterations: 6 max_attempts: 2 + state_updates: + synth_failure: '{{output}}' + fallback: verdict next: verify verify: @@ -476,6 +516,7 @@ nodes: max_attempts: 2 state_updates: verifier_output: '{{output}}' + fallback: verdict next: verdict verdict: diff --git a/assets/agents/code-reviewer/scripts/aux_fault.py b/assets/agents/code-reviewer/scripts/aux_fault.py new file mode 100644 index 0000000..76edd09 --- /dev/null +++ b/assets/agents/code-reviewer/scripts/aux_fault.py @@ -0,0 +1,48 @@ +#!/usr/bin/env python3 +"""Fail-closed marker for dead aux context lanes of the code-reviewer graph. + +Reached ONLY via aux_lanes' `fallback:` route. aux_lanes' state_updates wrote +an "LLM node …failed: " string into `aux_failure` before routing here; +this node records the failure as a PIPELINE-FAULT in `aux_note`, which flows +into synthesize's "lane skips" prompt line. Aux lanes are ENRICHMENT, not +review coverage: the degradation is made VISIBLE in the report's inputs but +never blocks — verdict.py deliberately does NOT scan aux_note for faults. +""" + +import json +import os + +MAX_DETAIL_CHARS = 500 + + +def load_state(): + if path := os.environ.get("GRAPH_STATE_FILE"): + with open(path) as f: + return json.load(f) + return json.loads(os.environ.get("GRAPH_STATE", "{}")) + + +def main(): + state = load_state() + raw = state.get("aux_failure") + detail = raw.strip().replace("\n", " ") if isinstance(raw, str) else "" + if len(detail) > MAX_DETAIL_CHARS: + detail = detail[: MAX_DETAIL_CHARS - 1] + "…" + print( + json.dumps( + {"aux_note": f"PIPELINE-FAULT: aux context lanes failed after retries — {detail}"} + ) + ) + + +try: + main() +except Exception as e: # noqa: BLE001 — the fault marker itself must never crash + print( + json.dumps( + { + "aux_note": "PIPELINE-FAULT: aux context lanes failed after retries — " + f"fault-marker script error: {e}" + } + ) + ) diff --git a/assets/agents/code-reviewer/scripts/cover_gate.py b/assets/agents/code-reviewer/scripts/cover_gate.py index 1ebfc23..717a2f0 100644 --- a/assets/agents/code-reviewer/scripts/cover_gate.py +++ b/assets/agents/code-reviewer/scripts/cover_gate.py @@ -54,6 +54,11 @@ def main(): else: chosen = [{"domain": g["domain"], "files": g["files"]} for g in refined] if chosen is None: + # refine_groups' fallback lands here with `groups` never set; make the + # degradation visible in the synthesis prompt via groups_note. + refine_failure = state.get("refine_failure") + if not refined and isinstance(refine_failure, str) and refine_failure.startswith("LLM node"): + note = "refinement lane failed after retries — fell back to the deterministic grouping" chosen = [{"domain": g["domain"], "files": g["files"]} for g in proposed if isinstance(g, dict)] print(json.dumps({"group_items": chosen, "groups_note": note})) diff --git a/assets/agents/code-reviewer/scripts/domain_fault.py b/assets/agents/code-reviewer/scripts/domain_fault.py new file mode 100644 index 0000000..2b307d5 --- /dev/null +++ b/assets/agents/code-reviewer/scripts/domain_fault.py @@ -0,0 +1,59 @@ +#!/usr/bin/env python3 +"""Fail-closed domain lane fault marker for the code-reviewer graph. + +Reached ONLY via review_domain's `fallback:` route inside the map branch. +Grounded in engine behavior: on agent-node failure the engine writes +"Agent node failed: {e:#}" into `domain_report` via the node's state_updates +BEFORE routing to the fallback, so the raw failure text is already in state. +This node normalizes it into a "> ⚠️ PIPELINE-FAULT: domain review lane +failed after retries — …" banner the map collects as the lane's report; +verdict.py counts any banner-prefixed report as a fault and forces +NEEDS-HUMAN — a dead domain lane never reads as a clean slice. + +Slice identity comes from `domain_group` (the map item cover_gate emitted, +which carries `domain` and `files`); a missing/malformed item degrades to +the failure detail alone. +""" + +import json +import os + +MAX_DETAIL_CHARS = 500 + +FAULT_PREFIX = "> ⚠️ PIPELINE-FAULT: domain review lane failed after retries — " + + +def load_state(): + if path := os.environ.get("GRAPH_STATE_FILE"): + with open(path) as f: + return json.load(f) + return json.loads(os.environ.get("GRAPH_STATE", "{}")) + + +def main(): + state = load_state() + slice_desc = "" + group = state.get("domain_group") + if isinstance(group, dict): + domain = group.get("domain") + files = [f for f in (group.get("files") or []) if isinstance(f, str)] + if isinstance(domain, str) and domain.strip(): + slice_desc = f"domain '{domain.strip()}' (files: {', '.join(files)}): " + raw = state.get("domain_report") + detail = raw.strip().replace("\n", " ") if isinstance(raw, str) else "" + if len(detail) > MAX_DETAIL_CHARS: + detail = detail[: MAX_DETAIL_CHARS - 1] + "…" + print(json.dumps({"domain_report": f"{FAULT_PREFIX}{slice_desc}{detail}"})) + + +try: + main() +except Exception as e: # noqa: BLE001 — the fault marker itself must never crash + print( + json.dumps( + { + "domain_report": "> ⚠️ PIPELINE-FAULT: domain review lane failed " + f"after retries — fault-marker script error: {e}" + } + ) + ) diff --git a/assets/agents/code-reviewer/scripts/parse_fault.py b/assets/agents/code-reviewer/scripts/parse_fault.py new file mode 100644 index 0000000..000815b --- /dev/null +++ b/assets/agents/code-reviewer/scripts/parse_fault.py @@ -0,0 +1,58 @@ +#!/usr/bin/env python3 +"""Fail-closed marker for a dead parse stage of the code-reviewer graph. + +Reached ONLY via parse's `fallback:` route. parse's state_updates wrote an +"LLM node …failed: " string into `parse_failure` before routing here; +this node emits a complete PIPELINE-FAULT `verdict_out` and the graph jumps +straight to render — a review that could not even parse its request never +reads as MERGE-READY. render.py renders a PIPELINE-FAULT verdict despite the +empty changed-file list (its "No changes to review." early return skips +fault-reason verdicts). +""" + +import json +import os + +MAX_DETAIL_CHARS = 500 + +ATTENTION_LINE = ( + "PIPELINE-FAULT: parse failed — the review request could not be parsed; " + "the review did not run" +) + + +def load_state(): + if path := os.environ.get("GRAPH_STATE_FILE"): + with open(path) as f: + return json.load(f) + return json.loads(os.environ.get("GRAPH_STATE", "{}")) + + +def fault_verdict(detail): + return { + "verdict": "NEEDS-HUMAN", + "reason": f"PIPELINE-FAULT: parse failed — {detail}", + "counts": {}, + "deferred_count": 0, + "dropped_count": 0, + "dropped_titles": [], + "attention": [ATTENTION_LINE], + "findings_final": [], + } + + +def main(): + state = load_state() + raw = state.get("parse_failure") + detail = raw.strip().replace("\n", " ") if isinstance(raw, str) else "" + if not detail.startswith("LLM node"): + detail = "the parse stage died without recording a failure detail" + if len(detail) > MAX_DETAIL_CHARS: + detail = detail[: MAX_DETAIL_CHARS - 1] + "…" + print(json.dumps({"verdict_out": fault_verdict(detail)})) + + +try: + main() +except Exception as e: # noqa: BLE001 — the fault marker itself must never crash + print(json.dumps({"verdict_out": fault_verdict(f"fault-marker script error: {e}")})) diff --git a/assets/agents/code-reviewer/scripts/render.py b/assets/agents/code-reviewer/scripts/render.py index cf0ac54..9951268 100644 --- a/assets/agents/code-reviewer/scripts/render.py +++ b/assets/agents/code-reviewer/scripts/render.py @@ -24,7 +24,14 @@ def main(): changed = state.get("changed_files") or [] rigor = state.get("resolved_rigor") or "production" - if not changed: + # A fault can leave changed_files empty (a parse fault dies before the + # diff is resolved; a verifier fault fires even on an empty diff) — a + # fault-degraded verdict must still render as a full report. parse_fault + # puts the marker in the reason; verdict.py prepends it to attention. + faulted = str(v.get("reason") or "").startswith("PIPELINE-FAULT:") or any( + isinstance(a, str) and a.startswith("PIPELINE-FAULT:") for a in v.get("attention") or [] + ) + if not changed and not faulted: print(json.dumps({"final_report": "# Code Review Summary\n\nNo changes to review."})) return diff --git a/assets/agents/code-reviewer/scripts/verdict.py b/assets/agents/code-reviewer/scripts/verdict.py index e992afa..6aa72ff 100644 --- a/assets/agents/code-reviewer/scripts/verdict.py +++ b/assets/agents/code-reviewer/scripts/verdict.py @@ -8,6 +8,9 @@ The model that wrote the findings never grades its own homework: markers, exactly per the published rules; - MERGE-READY/NEEDS-HUMAN is computed from counts + always-human triggers (deterministic signals ∪ the synthesis's ADDITIVE-only flags). +Fail-closed: any PIPELINE-FAULT recorded upstream (dead domain lane, dead +verifier, dead synthesis, missing domain reports) forces NEEDS-HUMAN — a +degraded run can never read MERGE-READY. The gate never crashes into a silent verdict: any internal error emits NEEDS-HUMAN naming the error. """ @@ -16,6 +19,9 @@ import json import os import re +MAX_DETAIL_CHARS = 500 + + def load_state(): if path := os.environ.get("GRAPH_STATE_FILE"): with open(path) as f: @@ -117,16 +123,48 @@ def main(): seen = set() attention = [a for a in attention if not (a.lower() in seen or seen.add(a.lower()))] - if (state.get("changed_files") or []) and not (state.get("domain_reports") or []): - attention.append( - "PIPELINE FAULT: the diff is non-empty but no domain reports were produced — " + # --- pipeline-fault accounting (fail closed) ----------------------------- + # PREFIX-anchored: a report that merely QUOTES a marker mid-text must not + # trip it. Any fault forces NEEDS-HUMAN. + faults = [] + changed = state.get("changed_files") or [] + reports = state.get("domain_reports") or [] + faulted = sum( + 1 for r in reports if isinstance(r, str) and r.lstrip().startswith("> ⚠️ PIPELINE-FAULT:") + ) + if faulted: + faults.append( + f"PIPELINE-FAULT: {faulted} of {len(reports)} domain review lane(s) " + "failed after retries — their slices were not reviewed" + ) + verifier_raw = state.get("verifier_output") + if isinstance(verifier_raw, str) and verifier_raw.lstrip().startswith("Agent node failed:"): + faults.append( + "PIPELINE-FAULT: finding verification failed — findings render as unverified" + ) + # Keyed off synth_failure + a non-empty diff, NEVER off findings == [] — + # a clean review legitimately has zero findings and stays MERGE-READY. + synth_failure = state.get("synth_failure") + if isinstance(synth_failure, str) and synth_failure.startswith("LLM node") and changed: + detail = synth_failure.strip().replace("\n", " ") + if len(detail) > MAX_DETAIL_CHARS: + detail = detail[: MAX_DETAIL_CHARS - 1] + "…" + faults.append(f"PIPELINE-FAULT: synthesis failed — findings unavailable; {detail}") + if changed and not reports: + faults.append( + "PIPELINE-FAULT: the diff is non-empty but no domain reports were produced — " "the review did not actually run; do not trust this verdict." ) + # faults go FIRST so the attention cap can never hide one + attention = faults + attention + reasons = [] if counts["🔴"]: reasons.append(f"{counts['🔴']} 🔴 CRITICAL finding(s)") if yellow_correctness: reasons.append(f"{yellow_correctness} 🟡 [correctness] finding(s) outside the deferred section") + if faults: + reasons.append("pipeline fault(s) recorded — degraded run") if attention: reasons.append("always-human trigger(s) fired") verdict = "NEEDS-HUMAN" if reasons else "MERGE-READY" diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 0980c5a..91130bc 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -9248,6 +9248,433 @@ mod tests { ); } + // ---- code-reviewer suite-script regression tests (R5) ---- + // + // Fail-closed fault paths of the code-reviewer's verdict/fault/render + // scripts, exercised the same way as the suites above: `python3