From 4263d684e94d05427100112ac2b00915afa55c22 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Sat, 12 Sep 2026 17:49:10 -0600 Subject: [PATCH] feat(TASK-020): finding-verifier + step-runner fail-closed hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit finding-verifier: parse gains max_attempts 2 + parse_failure capture + fallback to a new parse_fault marker that rewrites verdicts to a single UNVERIFIABLE PIPELINE-FAULT entry and continues to done (sentinel preserved β€” the caller's verify lane degrades instead of dying); verify_one gains a branch-local verify_fault marker normalizing the engine failure text into a schema-conformant UNVERIFIABLE verdict. step-runner: implement gains fallback end_failure (no retry β€” reruns against a mutated tree are unsafe; STEP_FAILED renders the failure via coder_result); independent_review gains fallback write_handoff with the handoff instructions and route_review treating an "Agent node failed:" review_report as review-did-not-run (fail-visible, guard ahead of the πŸ”΄ grep); orient/edge_case_sweep/write_handoff capture their failure text via state_updates, shrinking the bundled fallback-capture warning baseline back to zero. R5 tests: fault-marker normalization + crash paths (raw non-JSON GRAPH_STATE), route_review.sh fault/critical/clean routing via bash+jq, and structural wiring tests pinning fallbacks, sentinels, and end_failure's initial_state-backed template keys. --- assets/agents/finding-verifier/graph.yaml | 21 + .../finding-verifier/scripts/parse_fault.py | 57 +++ .../finding-verifier/scripts/verify_fault.py | 63 +++ assets/agents/step-runner/graph.yaml | 25 + .../step-runner/scripts/route_review.sh | 10 + src/config/request_context.rs | 433 +++++++++++++++++- 6 files changed, 585 insertions(+), 24 deletions(-) create mode 100644 assets/agents/finding-verifier/scripts/parse_fault.py create mode 100644 assets/agents/finding-verifier/scripts/verify_fault.py diff --git a/assets/agents/finding-verifier/graph.yaml b/assets/agents/finding-verifier/graph.yaml index 1adc582..cebc2c6 100644 --- a/assets/agents/finding-verifier/graph.yaml +++ b/assets/agents/finding-verifier/graph.yaml @@ -32,6 +32,7 @@ initial_state: verdicts: [] gate_feedback: '' gate_attempts: 0 + parse_failure: '' start: parse @@ -92,8 +93,20 @@ nodes: type: string description: Repository directory named in the request (echo the prompt default when none). required: [findings_list, project_dir] + max_attempts: 2 + state_updates: + parse_failure: '{{output}}' + fallback: parse_fault next: verify_each + parse_fault: + id: parse_fault + type: script + description: Fail-closed marker for a dead parse stage (reached only via parse's fallback) β€” rewrites `verdicts` to a single UNVERIFIABLE PIPELINE-FAULT entry and jumps straight to done, so the FINDING_VERIFIER_RESULTS sentinel still renders and the caller's verify lane degrades instead of dying. + script: scripts/parse_fault.py + timeout: 30 + next: done + verify_each: id: verify_each type: map @@ -165,6 +178,7 @@ nodes: max_attempts: 2 state_updates: verdict: '{{output}}' + fallback: verify_fault next: verdict_gate verdict_gate: @@ -174,6 +188,13 @@ nodes: script: scripts/verdict_gate.py timeout: 30 + verify_fault: + id: verify_fault + type: script + description: Fail-closed marker for a dead verify branch (reached only via verify_one's fallback; branch-local). Normalizes the engine's failure text in `verdict` into a schema-conformant UNVERIFIABLE verdict the map collects β€” a dead verifier lane never sinks the whole map or masquerades as verified. + script: scripts/verify_fault.py + timeout: 30 + done: id: done type: end diff --git a/assets/agents/finding-verifier/scripts/parse_fault.py b/assets/agents/finding-verifier/scripts/parse_fault.py new file mode 100644 index 0000000..7a6a8db --- /dev/null +++ b/assets/agents/finding-verifier/scripts/parse_fault.py @@ -0,0 +1,57 @@ +#!/usr/bin/env python3 +"""Fail-closed marker for a dead parse stage of the finding-verifier 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 rewrites `verdicts` to a single schema-conformant UNVERIFIABLE +entry naming the fault and the graph jumps straight to `done` β€” the +FINDING_VERIFIER_RESULTS sentinel still renders, so the caller (e.g. +code-reviewer's verify lane) degrades instead of dying. Findings that were +never extracted cannot be verified; the caller's "UNVERIFIABLE = keep the +finding, marked unverified" semantics do the right thing with the entry. +""" + +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 fault_verdicts(detail): + return { + "verdicts": [ + { + "id": "pipeline-fault", + "verdict": "UNVERIFIABLE", + "evidence": "", + "note": ( + "PIPELINE-FAULT: parse failed β€” findings could not be extracted " + f"from the verification request; no finding was verified: {detail}" + ), + } + ] + } + + +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(fault_verdicts(detail))) + + +try: + main() +except Exception as e: # noqa: BLE001 β€” the fault marker itself must never crash + print(json.dumps(fault_verdicts(f"fault-marker script error: {e}"))) diff --git a/assets/agents/finding-verifier/scripts/verify_fault.py b/assets/agents/finding-verifier/scripts/verify_fault.py new file mode 100644 index 0000000..4067c1f --- /dev/null +++ b/assets/agents/finding-verifier/scripts/verify_fault.py @@ -0,0 +1,63 @@ +#!/usr/bin/env python3 +"""Fail-closed marker for a dead verify branch of the finding-verifier graph. + +Reached ONLY via verify_one's `fallback:` route inside the map branch. +Grounded in engine behavior: on llm-node failure the engine writes +"LLM node failed: " into `verdict` 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 schema-conformant UNVERIFIABLE +verdict (authoritative id stamped from the finding item, mirroring +verdict_gate.py) and the chain ends there β€” the map collects a valid +verdict, so a dead verifier lane never sinks the whole map or masquerades +as verified. +""" + +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 finding_id(state): + finding = state.get("finding") + if isinstance(finding, dict): + fid = finding.get("id") + if isinstance(fid, str) and fid.strip(): + return fid.strip() + return "unknown" + + +def fault_verdict(fid, detail): + return { + "verdict": { + "id": fid, + "verdict": "UNVERIFIABLE", + "evidence": "", + "note": f"PIPELINE-FAULT: verifier lane failed after retries β€” {detail}", + } + } + + +def main(): + state = load_state() + fid = finding_id(state) + raw = state.get("verdict") + detail = raw.strip().replace("\n", " ") if isinstance(raw, str) else "" + if not detail.startswith("LLM node"): + detail = "the verifier lane died without recording a failure detail" + if len(detail) > MAX_DETAIL_CHARS: + detail = detail[: MAX_DETAIL_CHARS - 1] + "…" + print(json.dumps(fault_verdict(fid, detail))) + + +try: + main() +except Exception as e: # noqa: BLE001 β€” the fault marker itself must never crash + print(json.dumps(fault_verdict("unknown", f"fault-marker script error: {e}"))) diff --git a/assets/agents/step-runner/graph.yaml b/assets/agents/step-runner/graph.yaml index 5c3c85b..16a1c31 100644 --- a/assets/agents/step-runner/graph.yaml +++ b/assets/agents/step-runner/graph.yaml @@ -64,6 +64,7 @@ initial_state: plan_summary: '' implementation_brief: '' staleness_report: '' + orient_failure: '' has_major_deviation: false deviation_summary: '' user_feedback: '' @@ -80,12 +81,14 @@ initial_state: tests_output: '' edge_case_report: '' downstream_updates: '' + sweep_failure: '' needs_independent_review: false review_report: '' review_attempts: 0 max_review_attempts: 1 handoff_attempts: 0 handoff_fix: '' + handoff_failure: '' step_summary: '' start: resolve_step @@ -191,6 +194,8 @@ nodes: has_major_deviation, deviation_summary, ] + state_updates: + orient_failure: '{{output}}' fallback: end_failure next: route_staleness @@ -270,6 +275,12 @@ nodes: timeout: 3600 state_updates: coder_result: '{{output}}' + # No max_attempts: rerunning a coder that died mid-edit against an + # already-mutated tree is unsafe. On failure the "Agent node failed:" + # text lands in coder_result (via state_updates above) and end_failure + # renders it; end_failure's other keys all have initial_state defaults, + # so the STEP_FAILED output degrades to empty build/tests sections. + fallback: end_failure next: route_coder_result route_coder_result: @@ -376,6 +387,8 @@ nodes: needs_independent_review: type: boolean required: [edge_case_report, downstream_updates, needs_independent_review] + state_updates: + sweep_failure: '{{output}}' fallback: write_handoff next: route_sweep @@ -408,6 +421,7 @@ nodes: max_attempts: 2 state_updates: review_report: '{{output}}' + fallback: write_handoff next: route_review route_review: @@ -451,6 +465,15 @@ nodes: If "Gate feedback" below is non-empty, a previous handoff attempt failed validation - fix exactly what it lists. + + PIPELINE-FAULT VISIBILITY: an "Independent review report" below that + starts with "Agent node failed:" means the independent review DID NOT + RUN - the graph routed here through the reviewer's failure fallback. + Never present that text as review findings. Flag it prominently + instead: add a Deviations entry "PIPELINE-FAULT: independent review + did not run - ", repeat it under Notes for next step, + and carry it into `step_summary` so the user sees it at the approval + gate. prompt: | ## Step {{step_number}} ({{step_title}}) - plan at {{step_plan_path}} @@ -502,6 +525,8 @@ nodes: type: string description: 3-6 sentence summary of the step for the user's approval decision - what was done, deviations, anything needing their attention required: [step_summary] + state_updates: + handoff_failure: '{{output}}' fallback: end_failure next: check_handoff diff --git a/assets/agents/step-runner/scripts/route_review.sh b/assets/agents/step-runner/scripts/route_review.sh index eb6b2b9..c432920 100755 --- a/assets/agents/step-runner/scripts/route_review.sh +++ b/assets/agents/step-runner/scripts/route_review.sh @@ -13,6 +13,16 @@ review_report=$(echo "$state" | jq -r '.review_report // ""') review_attempts=$(echo "$state" | jq -r '.review_attempts // 0') max_review_attempts=$(echo "$state" | jq -r '.max_review_attempts // 1') +# Fail-visible guard: a review_report holding the engine's "Agent node +# failed:" text means the independent review DID NOT RUN (its fallback +# normally routes straight to write_handoff, bypassing this script). Never +# treat that text as findings or spend a fix-loop attempt on it - +# write_handoff flags the fault prominently in the handoff instead. +if [[ "$review_report" == "Agent node failed:"* ]]; then + jq -nc '{"_next": "write_handoff"}' + exit 0 +fi + if ! grep -qF "πŸ”΄" <<< "$review_report"; then jq -nc '{"_next": "write_handoff"}' exit 0 diff --git a/src/config/request_context.rs b/src/config/request_context.rs index f0d3a6e..a27fd5a 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -8200,20 +8200,6 @@ mod tests { .collect() } - fn fallback_capture(node_ids: &[&str]) -> Vec { - node_ids - .iter() - .map(|id| { - format!( - "{id}: llm node declares `fallback` with `output_schema` but no \ - `state_updates`; on failure the \"…failed: \" string is \ - only written through `state_updates`, so the fallback node \ - cannot see why it was reached" - ) - }) - .collect() - } - let _guard = TestConfigDirGuard::new(); Agent::install_builtin_agents(false).unwrap(); @@ -8252,9 +8238,7 @@ mod tests { // nodes, plus variable-shadowing on `coder` and `step-runner` (they // declare variables whose names are also `initial_state` keys). A new // rule that fires on a shipped asset (or a new bundled graph) must be - // recorded here deliberately. The fallback-capture warnings on - // `step-runner` are true positives, recorded until those nodes gain - // `state_updates` that capture the failure string. + // recorded here deliberately. let expected_warnings = BTreeMap::from([ ("adversary".to_string(), Vec::new()), ("code-reviewer".to_string(), Vec::new()), @@ -8282,8 +8266,9 @@ mod tests { ("finding-verifier".to_string(), Vec::new()), ("librarian".to_string(), Vec::new()), ("review-gauntlet".to_string(), Vec::new()), - ("step-runner".to_string(), { - let mut lines = [ + ( + "step-runner".to_string(), + [ shadowed(&["plans_dir", "project_dir"]), unreachable(&[ "check_handoff", @@ -8305,12 +8290,9 @@ mod tests { "verify_tests", "write_handoff", ]), - fallback_capture(&["edge_case_sweep", "orient", "write_handoff"]), ] - .concat(); - lines.sort(); - lines - }), + .concat(), + ), ]); assert_eq!( warnings_by_agent, expected_warnings, @@ -9803,6 +9785,409 @@ mod tests { ); } + // ---- finding-verifier suite-script regression tests (R5) ---- + // + // Fail-closed fault paths of the finding-verifier's marker scripts, + // exercised the same way as the suites above: `python3