ci(review): expand Opus to correctness blocking + sync fork prompt by iamwhatever · Pull Request #2379 · kirodotdev/KiroCrew

iamwhatever · 2026-08-09T17:18:38Z

Problem

The Opus reviewer only enforced AUTOSDE security rules — no code logic, correctness, or quality review. Between the deterministic gates (mypy/eslint/coverage) and GPT, Opus was blind to semantic bugs that no linter can catch. The fork-PR reviewer (fork-opus-review.yml) had the same limitation and was drifting out of sync.

Why it matters

Logic bugs, resource leaks, and scope mismatches that survive both linters and GPT's narrower pass reach main uncaught. The strongest model available (Opus 4.8) was underutilized on both same-repo and fork PRs.

Fix (symptoms → root cause → change)

The prompt was scoped to only AUTOSDE + 3 residual defect classes. This created a two-tier system that was complex to reason about and prevented correctness bugs from blocking.

Redesigned as a simpler collect → classify flow (applied to both claude-review.yml and fork-opus-review.yml):

Phase A collects all semantic defect candidates (any category)
Phase B (FALSIFY & CLASSIFY) falsifies each, then classifies survivors as BLOCKING or FINDING based on a closed WHAT BLOCKS list

Key changes:

BLOCKING expanded: added correctness defects that are unconditional on the normal path (the feature literally doesn't work on well-formed input) — not edge cases
Budget raised from 2 → 5 BLOCKING (aligned with GPT), so all real blockers surface in one pass
Simplified taxonomy: removed numbered dimension lists and [dimension] tags — just BLOCKING and FINDING
"WHAT TO LOOK FOR" is non-exhaustive — the model may report any semantic defect that passes the consequence-chain bar, not just those explicitly listed
PR intent (same-repo only) consumed as nonce-fenced file (not interpolated into YAML — prevents Actions expression injection on a public repo)
Fork prompt synced — fork-opus-review.yml now shares the same WHAT BLOCKS, Phase A/B, budget, and calibration as the same-repo version (retaining its own fork-specific security hardening: SYSTEM RULES, INPUT DISCIPLINE, authenticated patch file)
Prompt contradictions fixed (dead-code ban vs maintainability, residual-only restriction vs quality)

WHAT BLOCKS (exhaustive, 5 items):

AUTOSDE blocking: true rule violation
Reachable security hole with named trigger
Crash / data-loss / corruption
Removed guard with no replacement
NEW: Correctness defect — unconditional wrong behavior on normal path (all three: on changed path, no upstream guard verified, feature doesn't work for stated purpose)

GPT reviewer unchanged — kept narrow (AUTOSDE + residual only) to complement Opus's broader coverage. Two reviewers with different scopes catch more than two with the same scope.

Tests

41/41 test_ai_review_workflows.py pass
Updated assertions for: Phase B rename, budget cap (2→5), dimension-tag removal, advisory-only wording, what-to-look-for section

Manual verification

N/A — CI workflow change, verified by test assertions matching prompt text + the gate's own execution on this PR.

github-actions · 2026-08-09T17:21:09Z

Design Review (Fable 5) — 🟡 CONCERNS

Advisory design-level review of 8c03c633bb66b4ae851cff41b2d3c7ffa08c1165 — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

Sync-by-hand fixes the drift symptom; nothing prevents the two prompts drifting apart again, and the fork copy already contradicts itself.

Watch

The stated problem includes "fork-opus-review.yml … was drifting out of sync," but the fix is a one-time manual re-sync: every new test in TestClaudeReviewQualityDimensions asserts against claude-review.yml only, so nothing pins the fork prompt → the next prompt edit lands in one file → the same drift the PR set out to fix resurfaces silently.
The synced fork prompt is internally contradictory: it now blocks on criterion 5(c) "does not work for its stated purpose" and lists "scope fidelity" under WHAT TO LOOK FOR, while its retained INPUT DISCIPLINE still forbids reading the PR title/description → on a fork PR the model has no stated purpose to judge against and resolves the conflict arbitrarily — exactly the "prompt contradictions" failure class this PR claims to fix.
Criterion 5(c) makes the author-controlled description an input to a merge-blocking decision on a public repo. Expression injection is handled well (nonce-fenced file, no ${{ }} interpolation), but the only defense against the description steering the blocking verdict is prompt instruction — a known-soft control.

Suggestions

Add a test that asserts the shared prompt sections (WHAT BLOCKS, FINDING BAR, BUDGET, PHASE A/B) are byte-identical across both workflow files — cheap, and it closes the drift root cause this PR only patches.
In the fork prompt, either drop "stated purpose"/"scope fidelity" wording or state explicitly that purpose is inferred from the code, so the model isn't left to reconcile the contradiction.

[DESIGN-REVIEWED] 8c03c63

github-actions · 2026-08-09T17:21:49Z

Opus 4.8 Review — ✅ no blocking findings

Reviewed 8c03c633bb66b4ae851cff41b2d3c7ffa08c1165 — this comment is updated in place on each push.

Review details

This PR only changes CI review prompts, test assertions, and adds one shell step (the "Fetch PR intent" step in claude-review.yml). Let me verify the one piece of real logic — the fetch step — is internally consistent with what the prompt reads, and that the fork workflow (which retains INPUT DISCIPLINE and no intent file) doesn't dangle a read to a file it never creates.

The fork workflow correctly keeps its INPUT DISCIPLINE (no intent file, deliberate per description), and doesn't reference the intent file. The same-repo fetch step writes to ${{ runner.temp }}/.review-pr-intent.txt, which matches the path the prompt instructs the model to Read. Both steps share the same if: guard, so if the fetch is skipped the review is too. The shell step uses set -uo pipefail with || true fallbacks and a correct truncation-marker check.

No executable product code changed; no AUTOSDE rule matches workflow/prompt text; the shell logic completes no defect consequence chain.

No findings.

[OPUS-REVIEWED] 8c03c63

Verdict parsed from the review's SHA-scoped output markers for commit 8c03c633bb66b4ae851cff41b2d3c7ffa08c1165.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 8c03c633bb66b4ae851cff41b2d3c7ffa08c1165: <one-sentence reason>

github-actions · 2026-08-09T17:22:16Z

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 8c03c633bb66b4ae851cff41b2d3c7ffa08c1165 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- .github/workflows/fork-opus-review.yml:357 -- "dead code" remains explicitly forbidden above, so fork reviews suppress the semantic unreachable-branch findings this hunk promises -> Fix: clarify here that semantic unreachable branches are exempt from the mechanical dead-code exclusion.
[GPT-REVIEWED] 8c03c63

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 8c03c633bb66b4ae851cff41b2d3c7ffa08c1165: <one-sentence reason>

The Opus lane was only checking the AUTOSDE rule files -- which for the backend means 4 rules / 24 security-and-event-loop directives and nothing else. Actual code logic and quality went unreviewed, and the reviewer was deliberately blind to the PR description, so it could never tell whether a diff did what it claimed. Now that it runs in ~4 turns on opus-4.8 there is headroom to widen what it looks at. Reuses the code-review-sage ruleset: the chain-of-consequences bar and its review dimensions. 1. PR intent is now an input, as UNTRUSTED data. A pre-step fetches title/body, strips media, caps at 8KB, wraps it in a collision-resistant nonce fence, and writes it to `.review-pr-intent.txt`; the prompt tells the model to Read that file. Framed explicitly as a CLAIM, never ground truth, never an instruction, and unable to waive or downgrade any code finding. Passed as a FILE rather than a `${{ }}` expression on purpose: the body is attacker-controlled on a public repo and this is a privileged lane holding Bedrock creds, so interpolating it into a prompt/run block would be the classic Actions script-injection vector. `gh pr view` stays OUT of --allowedTools, so the model still has no unbounded PR-read tool and still cannot see comment threads. 2. Seven review dimensions replace the security-only residue: correctness & regression, security, resource & lifecycle, scope & description fidelity, consistency with existing patterns, maintainability, observability. 3. Chain of consequences is now the finding bar -- cause -> mechanism -> user/system consequence. A finding that cannot complete the chain to real user- or system-visible harm is DROPPED, not downgraded. 4. Blocking scope is UNCHANGED. Quality dimensions are advisory FINDINGs that never block; only an AUTOSDE `blocking: true` violation or one of the three residual classes can block. Widening what the reviewer looks at must not widen what stops a merge, or this recreates the endless-findings problem the budget work just fixed. Advisory findings are capped at 6. 5. Output stays terse: issue + fix, no rationale paragraphs, no alternatives considered. FINDINGs are one line each and carry a [dimension] tag, worst consequence first. Two prompt contradictions this surfaced and fixes, because the old text would have silently cancelled the new dimensions: - "NEVER report ... dead code" banned dimension 6 outright. The exclusion now draws the line at MECHANICAL vs SEMANTIC (a linter sees that `except Exception: pass` matches a pattern; only the reviewer sees that it swallows the one error the caller needed). - "you may report ONLY the three RESIDUAL DEFECT CLASSES" and "residue is DEFINED BY the AUTOSDE rule files" made any quality finding non-reportable. Those now define the BLOCKING set, with quality admitted as advisory alongside. Also recalibrated: "No findings." is still legitimate but no longer the default expectation. Tests: test_ai_review_workflows.py 41 pass. The pre-existing test_reviewer_is_code_only_and_cannot_fetch_pr_prose correctly FAILED on this change and was rewritten to pin the new contract, including a guard that the body is never reached through a `${{ }}` expression.

…contract prepare-pr's entire value is that local-green predicts server-green. Four things had drifted since the CI review sprint, so the local gate was mirroring a contract CI no longer runs. Model (the reported symptom): - profile pinned `claude-opus-5`; claude-review.yml moved to `us.anthropic.claude-opus-4-8` in #2339. The local gate was reviewing with a DIFFERENT model than the gate it claims to mirror. - Now `claude-opus-4.8`, fallback `claude-opus-4.7`. CI has no fallback (#2339 removed it); the local tier stays because local model availability varies, and that difference is now stated in the charter instead of looking like drift. Three further drifts, all understating CI: - Budget said "≤2 BLOCKING". CI is 5 BLOCKING + 6 advisory FINDING (#2322, #2379). A local cap of 2 systematically under-predicts the server round -- the precise cause of the "one push turns into ten" failure this skill warns about. - Blocking contract said 2 classes (AUTOSDE blocking:true, residual-class). CI enumerates 5, including a correctness defect that is unconditional wrong behaviour on the NORMAL path. - The charter never mentioned the consequence-chain bar or the quality dimensions added in #2379, so the local reviewer had no instruction to look at correctness, resource/lifecycle, scope fidelity, consistency, maintainability, or observability at all. The GPT charter also now states its ≤5 BLOCKING budget and that CI runs the lane as two passes (discovery + authoritative falsification), so a single local pass knows to apply the same falsification bar. Drift guard: test_opus_profile_model_matches_the_ci_workflow parses --model out of claude-review.yml and compares it to the profile pin, normalizing across the two id namespaces on purpose -- CI uses the Bedrock regional inference profile (us.anthropic.claude-opus-4-8), the local harness uses the kiro-cli id (claude-opus-4.8). Anchored to the real claude_args line: an unanchored regex matched the prose "--model below" in the comment above the job and produced a false failure. Positive control: reverting the pin to claude-opus-5 makes the guard fail with the two ids named; restoring it passes. 18/18 test_prepare_pr_profiles.py pass. flake8 + isort clean.

…contract (#2456) prepare-pr's entire value is that local-green predicts server-green. Four things had drifted since the CI review sprint, so the local gate was mirroring a contract CI no longer runs. Model (the reported symptom): - profile pinned `claude-opus-5`; claude-review.yml moved to `us.anthropic.claude-opus-4-8` in #2339. The local gate was reviewing with a DIFFERENT model than the gate it claims to mirror. - Now `claude-opus-4.8`, fallback `claude-opus-4.7`. CI has no fallback (#2339 removed it); the local tier stays because local model availability varies, and that difference is now stated in the charter instead of looking like drift. Three further drifts, all understating CI: - Budget said "≤2 BLOCKING". CI is 5 BLOCKING + 6 advisory FINDING (#2322, #2379). A local cap of 2 systematically under-predicts the server round -- the precise cause of the "one push turns into ten" failure this skill warns about. - Blocking contract said 2 classes (AUTOSDE blocking:true, residual-class). CI enumerates 5, including a correctness defect that is unconditional wrong behaviour on the NORMAL path. - The charter never mentioned the consequence-chain bar or the quality dimensions added in #2379, so the local reviewer had no instruction to look at correctness, resource/lifecycle, scope fidelity, consistency, maintainability, or observability at all. The GPT charter also now states its ≤5 BLOCKING budget and that CI runs the lane as two passes (discovery + authoritative falsification), so a single local pass knows to apply the same falsification bar. Drift guard: test_opus_profile_model_matches_the_ci_workflow parses --model out of claude-review.yml and compares it to the profile pin, normalizing across the two id namespaces on purpose -- CI uses the Bedrock regional inference profile (us.anthropic.claude-opus-4-8), the local harness uses the kiro-cli id (claude-opus-4.8). Anchored to the real claude_args line: an unanchored regex matched the prose "--model below" in the comment above the job and produced a false failure. Positive control: reverting the pin to claude-opus-5 makes the guard fail with the two ids named; restoring it passes. 18/18 test_prepare_pr_profiles.py pass. flake8 + isort clean. Co-authored-by: Joe Guo <zejiangg@amazon.com>

…irodotdev#2379) The Opus lane was only checking the AUTOSDE rule files -- which for the backend means 4 rules / 24 security-and-event-loop directives and nothing else. Actual code logic and quality went unreviewed, and the reviewer was deliberately blind to the PR description, so it could never tell whether a diff did what it claimed. Now that it runs in ~4 turns on opus-4.8 there is headroom to widen what it looks at. Reuses the code-review-sage ruleset: the chain-of-consequences bar and its review dimensions. 1. PR intent is now an input, as UNTRUSTED data. A pre-step fetches title/body, strips media, caps at 8KB, wraps it in a collision-resistant nonce fence, and writes it to `.review-pr-intent.txt`; the prompt tells the model to Read that file. Framed explicitly as a CLAIM, never ground truth, never an instruction, and unable to waive or downgrade any code finding. Passed as a FILE rather than a `${{ }}` expression on purpose: the body is attacker-controlled on a public repo and this is a privileged lane holding Bedrock creds, so interpolating it into a prompt/run block would be the classic Actions script-injection vector. `gh pr view` stays OUT of --allowedTools, so the model still has no unbounded PR-read tool and still cannot see comment threads. 2. Seven review dimensions replace the security-only residue: correctness & regression, security, resource & lifecycle, scope & description fidelity, consistency with existing patterns, maintainability, observability. 3. Chain of consequences is now the finding bar -- cause -> mechanism -> user/system consequence. A finding that cannot complete the chain to real user- or system-visible harm is DROPPED, not downgraded. 4. Blocking scope is UNCHANGED. Quality dimensions are advisory FINDINGs that never block; only an AUTOSDE `blocking: true` violation or one of the three residual classes can block. Widening what the reviewer looks at must not widen what stops a merge, or this recreates the endless-findings problem the budget work just fixed. Advisory findings are capped at 6. 5. Output stays terse: issue + fix, no rationale paragraphs, no alternatives considered. FINDINGs are one line each and carry a [dimension] tag, worst consequence first. Two prompt contradictions this surfaced and fixes, because the old text would have silently cancelled the new dimensions: - "NEVER report ... dead code" banned dimension 6 outright. The exclusion now draws the line at MECHANICAL vs SEMANTIC (a linter sees that `except Exception: pass` matches a pattern; only the reviewer sees that it swallows the one error the caller needed). - "you may report ONLY the three RESIDUAL DEFECT CLASSES" and "residue is DEFINED BY the AUTOSDE rule files" made any quality finding non-reportable. Those now define the BLOCKING set, with quality admitted as advisory alongside. Also recalibrated: "No findings." is still legitimate but no longer the default expectation. Tests: test_ai_review_workflows.py 41 pass. The pre-existing test_reviewer_is_code_only_and_cannot_fetch_pr_prose correctly FAILED on this change and was rewritten to pin the new contract, including a guard that the body is never reached through a `${{ }}` expression. Co-authored-by: Joe Guo <zejiangg@amazon.com>

…contract (kirodotdev#2456) prepare-pr's entire value is that local-green predicts server-green. Four things had drifted since the CI review sprint, so the local gate was mirroring a contract CI no longer runs. Model (the reported symptom): - profile pinned `claude-opus-5`; claude-review.yml moved to `us.anthropic.claude-opus-4-8` in kirodotdev#2339. The local gate was reviewing with a DIFFERENT model than the gate it claims to mirror. - Now `claude-opus-4.8`, fallback `claude-opus-4.7`. CI has no fallback (kirodotdev#2339 removed it); the local tier stays because local model availability varies, and that difference is now stated in the charter instead of looking like drift. Three further drifts, all understating CI: - Budget said "≤2 BLOCKING". CI is 5 BLOCKING + 6 advisory FINDING (kirodotdev#2322, kirodotdev#2379). A local cap of 2 systematically under-predicts the server round -- the precise cause of the "one push turns into ten" failure this skill warns about. - Blocking contract said 2 classes (AUTOSDE blocking:true, residual-class). CI enumerates 5, including a correctness defect that is unconditional wrong behaviour on the NORMAL path. - The charter never mentioned the consequence-chain bar or the quality dimensions added in kirodotdev#2379, so the local reviewer had no instruction to look at correctness, resource/lifecycle, scope fidelity, consistency, maintainability, or observability at all. The GPT charter also now states its ≤5 BLOCKING budget and that CI runs the lane as two passes (discovery + authoritative falsification), so a single local pass knows to apply the same falsification bar. Drift guard: test_opus_profile_model_matches_the_ci_workflow parses --model out of claude-review.yml and compares it to the profile pin, normalizing across the two id namespaces on purpose -- CI uses the Bedrock regional inference profile (us.anthropic.claude-opus-4-8), the local harness uses the kiro-cli id (claude-opus-4.8). Anchored to the real claude_args line: an unanchored regex matched the prose "--model below" in the comment above the job and produced a false failure. Positive control: reverting the pin to claude-opus-5 makes the guard fail with the two ids named; restoring it passes. 18/18 test_prepare_pr_profiles.py pass. flake8 + isort clean. Co-authored-by: Joe Guo <zejiangg@amazon.com>

iamwhatever requested a review from a team as a code owner August 9, 2026 17:18

github-actions Bot added the readiness: checking Automated validation is still running label Aug 9, 2026

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 9, 2026

iamwhatever force-pushed the ci/opus-sage-dimensions branch from 579fede to 43ee834 Compare August 9, 2026 19:05

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 9, 2026

iamwhatever force-pushed the ci/opus-sage-dimensions branch 2 times, most recently from 74af1cc to c4addaf Compare August 9, 2026 19:22

iamwhatever changed the title ~~ci(review): give Opus the PR intent and the sage quality dimensions~~ ci(review): expand Opus to correctness blocking + sync fork prompt Aug 9, 2026

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 9, 2026

iamwhatever force-pushed the ci/opus-sage-dimensions branch from c4addaf to 8c03c63 Compare August 9, 2026 19:30

bolichen97 approved these changes Aug 10, 2026

View reviewed changes

bolichen97 merged commit 10009fb into main Aug 10, 2026
51 checks passed

bolichen97 deleted the ci/opus-sage-dimensions branch August 10, 2026 01:22

github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 10, 2026

iamwhatever mentioned this pull request Aug 10, 2026

fix(prepare-pr): re-mirror the local review gate onto the current CI contract #2456

Merged

CrysisDeu mentioned this pull request Aug 10, 2026

ci: split Opus review into discovery and validation passes #2332

Merged

Navigation Menu

Uh oh!

ci(review): expand Opus to correctness blocking + sync fork prompt - #2379

ci(review): expand Opus to correctness blocking + sync fork prompt#2379
bolichen97 merged 1 commit into
mainfrom
ci/opus-sage-dimensions

iamwhatever commented Aug 9, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 9, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 9, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 9, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

iamwhatever commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Why it matters

Fix (symptoms → root cause → change)

Tests

Manual verification

Uh oh!

github-actions Bot commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design Review (Fable 5) — 🟡 CONCERNS

Watch

Suggestions

Uh oh!

github-actions Bot commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Opus 4.8 Review — ✅ no blocking findings

Uh oh!

github-actions Bot commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GPT 5.6 Review — ✅ no blocking findings

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iamwhatever commented Aug 9, 2026 •

edited

Loading

github-actions Bot commented Aug 9, 2026 •

edited

Loading

github-actions Bot commented Aug 9, 2026 •

edited

Loading

github-actions Bot commented Aug 9, 2026 •

edited

Loading