ci(ai-review): report review step failures honestly

[SebastianYuSun]

Problem / Motivation

AI-review lanes could publish confident but false explanations after an earlier workflow step failed:

• the fork first-principles lane treated an empty contract output (“the step never ran”) as false (“the base was checked and has no contract”);
• six review lanes said the model errored even when the model step was skipped and no model call occurred.

The original branch also fixed a fragile 8,000-byte intent cap. Main #6264 now contains that production fix, so this rebase keeps main's Perl implementation instead of reintroducing the older patch.

Why it matters

An infrastructure failure must not be reported as a fact about a contributor's code. False “no contract” and “model errored” messages send contributors to debug the wrong system and can make a green advisory check assert something nobody verified.

What changed

• Distinguish CONTRACT=false from an empty output. Only a completed lookup that found no contract yields NO_CONTRACT; a skipped lookup yields UNKNOWN and an incomplete/neutral review.
• Give fork scope-skip and missing-contract states separate, accurate messages.
• Feed steps.review.outcome into all six review-comment lanes and map skipped, failure, cancelled, success, and unknown states to distinct leak-safe explanations.
• Preserve main feat(governance): fetch the Level-1 ceiling from a central source #6264's intent-cap implementation.
• Restore behavior-level coverage for that cap. The 100KB body comes from a temporary file, not an environment variable, so the test is valid under Windows' 32,767-character environment-block limit.
• Pin UTF-8 on all eight text-mode subprocess probes in test_ai_review_workflows.py and remove its now-zero entry from the encoding ratchet. This fixes the deterministic CI failure instead of rerunning it.

No permissions or merge-blocking conclusions are weakened.

Tests

• Focused cases: 27 passed serial and 27 passed with two workers.
• Full test_ai_review_workflows.py: 203 collected; 199 passed, 4 capability skips.
• Related workflow matrix: 213 passed, 75 capability skips.
• Subprocess-encoding self-test: 9 flagged and 9 clean probes; actual gate passed with 242 known calls in 118 baseline files.
• Six modified YAML files parsed successfully.
• All 36 modified Bash run: blocks passed bash -n.
• isort, Flake8, Black ratchet, and diff-whitespace checks passed.

The behavior tests execute the actual workflow shell at 0/100/8000/8001/100000 bytes and across a split UTF-8 boundary. They use no retry, sleep, duration threshold, timeout relaxation, or warning filter.

Manual verification

The fork Stage-2 workflow definition runs from the default branch, so this PR cannot exercise its edited copy before merge. Instead, tests extract and execute each modified shell block directly, including the failing state combinations and long-body inputs.

Screenshots / video

Why no screenshot: This changes CI workflow reporting and tests only; it has no rendered product UI effect.

Related Issues

Fixes #6207

[SebastianYuSun]

@bolichen97 @iamwhatever — could one of you approve the workflow runs on this PR when you have a moment?

State of head b49131a4 (open ~15h): all 12 pull_request workflows are still run_attempt=1 with conclusion: action_required and 0s duration — not one of them has ever executed. So the red PR Readiness is reporting CI / Build / Code Review as blocking purely because they never started; nothing has actually failed. Both documented auto-approval preconditions look satisfied: the PR is not a draft, and Fork PR Description is green with "Description follows the PR template".

Two questions, in case this is a gap rather than just queue depth:

1. Is krishdhasmana the intended reviewer here? They were auto-assigned alongside the kirocrew-team request when the PR opened, and I would like to confirm the round-robin landed somewhere expected before pinging anyone further.

2. Is a fork PR that touches .github/workflows/ expected to be skipped by the auto-approval path? 6 of the 7 files here are workflow files. Related, and possibly the reason for the pause: because no run has been approved, GitHub has emitted no workflow_run event at all for this head, so Fork workflow-change guard has not evaluated either — approving would happen without that signal.

The scope and the one intentional state change are described in the PR body. Happy to split test/test_ai_review_workflows.py into its own PR, or to reduce this to the fork lanes only, if either makes review easier.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 7a6365dc94b081498f7c1a07977cdd12d615b8a9 via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- .github/workflows/design-review.yml:438 -- a response with a verdict header but a missing proof marker reaches success) why="...no verdict header", publishing a false cause across all six lanes -> Fix: say “returned no usable verdict” in each mapping. (origin: validation)
[GPT-REVIEWED] 7a6365d

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 7a6365dc94b081498f7c1a07977cdd12d615b8a9 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified against the base tree: the fork lane's intent fetch (step at line 304) genuinely sits between the scope gate and the contract step, so an intent-fetch failure did skip the contract lookup and surface as a false NO_CONTRACT; the contract step emits an explicit three-state available output; every lane has an id: review step whose outcome the new case blocks read; and all comment steps run under if: always(), so skipped is a reachable outcome. The description matches the diff bidirectionally, the fix targets the root cause (report the observed outcome instead of asserting one), and the six-lane duplication is pinned by parametrized tests that execute the actual shell.

Design-Verdict: PASS

Root-cause fix — states are distinguished at their source and every claim is now derived from an observed outcome, pinned by behavior-executing tests across all six lanes.

[DESIGN-REVIEWED] 7a6365d

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 7a6365dc94b081498f7c1a07977cdd12d615b8a9 via the fork AI-review pipeline; updated in place on each push.

Review details

I've verified the one load-bearing claim: the fork lane now emits verdict=UNKNOWN from the new CONTRACT-empty branch, and UNKNOWN was already a pre-existing verdict state (the SURFACE-empty branch and the marker-parse default both emit it) that the comment step's case default and the finalize step (NEUTRAL on errored / UNKNOWN) already handle. No new unhandled state is introduced.

The other changes:

• The REVIEW_OUTCOME case blocks assign why from a fixed vocabulary; $why interpolates into an echo with no shell-injection surface (no command substitution, set -uo pipefail + :- default handles unset).
• The non-fork != "true" → = "false" narrowing: the empty case is documented as unreachable in that lane, and I cannot ground reachability without assuming step-ordering code I did not open — so no groundable defect.
• Test-only encoding="utf-8" additions match the removed subprocess-encoding-baseline.txt line (a gate-compliance change, out of scope per the rules).

Nothing survives falsification, and I found no groundable Step‑2 addition.

No findings.

[OPUS-REVIEWED] 7a6365d

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 7a6365dc94b081498f7c1a07977cdd12d615b8a9 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification complete. Emitting the review.

First-Principles-Verdict: PASS

Every item traces to the reported defect (#6207): CI lanes asserted causes nobody observed, and each fix lands at the cause with all counted siblings covered.

What this change ships

Intent: stop AI-review lanes from publishing confident false explanations (a "no contract"/"model errored" claim) when the real failure was an earlier workflow step — a FIX.

1. Fork FP lane: an upstream step failure now finalizes NEUTRAL/incomplete, not a green "no contract on base commit" — justified
2. Fork FP comment: missing-contract message split from "your diff ships nothing" — justified, restores parity with same-repo lane
3. Same-repo FP guard tightened to = "false" — sibling fix of the same root cause; unreachable today, author says so in-diff
4. Six lanes' "could not complete" comment names the observed step outcome, not an asserted model error — justified
5. REVIEW_OUTCOME env in six comment steps — one consumer each (its own case block), not generalized
6. Behavior tests executing the intent cap at 0/100/8000/8001/100000 bytes and a split UTF-8 boundary — declared; the prior string-match tests provably missed this class
7. Three-valued-CONTRACT and outcome-mapping tests — declared
8. UTF-8 pins on 8 subprocess probes, baseline entry ratcheted out — derived from the subprocess-encoding gate

Counts run: the false "the model call errored or returned no verdict header" string exists in exactly the 6 lanes changed — 0 unfixed siblings. Fork lane step order verified (scope → intent fetch → contract), so empty CONTRACT is genuinely reachable there and genuinely not in the same-repo lane (scope → contract, nothing between); item 3 is the counted sibling, not a rider. Same-repo lane already distinguished NO_CONTRACT from SKIPPED (first-principles-review.yml:338-373), so item 2 is drift-repair toward an existing mechanism, not a second spelling. No product surface, no config keys, no changelog touch.

[FIRST-PRINCIPLES-REVIEWED] 7a6365d

[bolichen97]

Tier 1 auto-approve: chore/CI plumbing (8 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: ci — edits AI-review workflow lanes to name a review step's failure cause from steps.review.outcome (and distinguish an absent-contract false from a never-ran empty state) instead of asserting 'the model call errored'; advisory-comment wording and outcome-reading only, no credential/permission/gate change. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.

[bolichen97]

Approved after a full-diff review (scope match, no out-of-scope files, security surface checked, tests verified non-vacuous). Review notes available on request.