fix(prepare-pr): re-mirror the local review gate onto the current CI contract - #2456
Conversation
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsAll new test assertions hold against the actual workflows ( No findings. [OPUS-REVIEWED] 4bf5ee2 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of I've verified the PR's claims against the repo: CI does pin Design-Verdict: PASS Real drift, fixed at the cause: the mirror claim is now test-pinned to the workflows instead of trusted prose, so recurrence fails loudly. Suggestions
[DESIGN-REVIEWED] 4bf5ee2 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
6f80b7b to
90bdfc9
Compare
90bdfc9 to
c746ea5
Compare
…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.
c746ea5 to
4bf5ee2
Compare
|
Re: Design Review 🟡 CONCERNS ( Watch — the drift guard pins only the model, so the hand-copied prose contract can silently re-drift: FIXED in The concern is correct and it was a hole in this PR's own thesis: I argued drift is the problem, then guarded one of the four things that drifted. Took your first suggestion (extend the guard) rather than the second (slim the charter), because the charter's restated numbers are what make the local reviewer's instructions self-contained — a subagent that has to go read the workflow to learn its own budget is a worse contract, not a better one. Added
Positive control: mutating the charter to
Not pinned, deliberately: the five blocking classes and the seven dimensions are prose enumerations with no single parseable line to anchor to, so a regex guard there would be brittle rather than protective. The budget numbers were the cheaply-parseable facts you identified, and those are now covered. |
…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>
Problem
prepare-prno longer mirrors CI. Its whole value is that local-green predicts server-green — and four things had drifted since the CI review sprint, so the local gate was mirroring a contract CI stopped running.The model was the reported symptom, but it was the smallest of the four:
claude-opus-5us.anthropic.claude-opus-4-8(#2339)≤2 BLOCKING5 BLOCKING+6 advisory FINDING(#2322, #2379)Why it matters
Two of these actively degrade the gate:
Fix
Model:
claude-opus-4.8, fallbackclaude-opus-4.7. CI has no fallback (#2339 removed it); the local tier stays because local model availability varies — and the charter now says that, so the difference reads as intentional rather than as drift.Budget:
≤5 BLOCKING, ≤6 advisory FINDING.Blocking contract: all 5 CI classes —
blocking: trueAUTOSDE rule, reachable security hole, crash/data-loss/corruption, removed guard with no replacement, and a correctness defect that is unconditional wrong behaviour on the NORMAL path.Dimensions: the charter now names the consequence-chain bar (
cause → mechanism → consequence, dropped rather than downgraded if it cannot complete) and the seven review dimensions. Previously the local reviewer had no instruction to look at correctness, resource/lifecycle, scope fidelity, consistency, maintainability, or observability at all.GPT charter: now states its
≤5 BLOCKINGbudget and that CI runs the lane as two passes (discovery + authoritative falsification), so a single local pass knows to apply the same falsification bar.Tests
test_opus_profile_model_matches_the_ci_workflow— a drift guard that parses--modelout ofclaude-review.ymland compares it to the profile pin, normalizing across the two id namespaces on purpose (CI uses the Bedrock regional inference profile, the local harness uses the kiro-cli id).Anchored to the real
claude_argsline: an unanchored regex matched the prose"--model below"in the comment above the job and produced a false failure — caught while writing it.Positive control: reverting the pin to
claude-opus-5makes the guard fail with both ids named; restoring it passes.18/18pass, flake8 + isort clean.Manual verification
origin/main'sclaude-review.yml/codex-review.ymlto enumerate the drift rather than assuming only the model moved.claude-opus-4.7is a served kiro-cli id before pinning it as the fallback.Screenshots
N/A — skill/profile text and a test.
Note: the installed copy under
~/.kiro/crew/skills/kirocrew-dev/prepare-pr/is a real directory, not a symlink to the repo, so it refreshes on the next install/update rather than immediately from this merge.