Recalibrate Claude PR review severity to cut false positives - #4406
Recalibrate Claude PR review severity to cut false positives#4406keppo-bot[bot] wants to merge 2 commits into
Conversation
An audit of the last 25 PRs by wwwillchen/keppo-bot (#4343-#4400) found 229 inline comments from dyad-assistant across 83 review runs. Re-judging each one against the code at the reviewed commit: 196 were factually true, but only 67 deserved to be inline comments. 135 MEDIUMs should have been LOW and 25 should not have been posted at all -- 71% miscalibrated. Corroborating signals: 117 of 176 resolved threads got no reply at all, 41 MEDIUMs were left open on PRs that merged anyway, and only 3 of 83 runs ever concluded "ready to merge". Three causes in this prompt, and the fixes: 1. No severity rubric. The only guidance was a pointer to the multi-persona standard, whose code-health reviewer says verbatim "Sloppy code that hurts maintainability is a MEDIUM severity issue, not LOW". Replaced with an impact-anchored rubric plus an explicit always-LOW list (duplication, dead code, naming, comments, missing tests, defensive guards, telemetry shape, prompt wording), and a calibration note that 0 HIGH / 0-2 MEDIUM is the expected result. 2. No admission gate. "Maintainability problems that can cause future defects" invited hazards that are not defects yet; all 60 speculative and all 48 scope-creep findings were over-posted. Findings now must name a concrete trigger in the first sentence of `body`, scope-creep remedies are LOW by rule, and base-branch behavior is out of scope. Constraint 4 becomes an admission gate rather than a disclaimer. 3. Three personas and a 7-bullet checklist acted as quotas. Collapsed to one senior-engineer voice; the Dyad-specific bullets are now gated triggers that apply only when the diff touches what they name. LOW stays out of the findings JSON: validate-review.mjs throws when findings exist with no Issues Summary table, and the posting step is continue-on-error, so a LOW-only run would silently drop all inline output. Verified the template still renders with the same three variables and that the summary/findings contract validate-review.mjs parses is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cf5bdf57f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| Before you assign HIGH or MEDIUM, write the trigger to yourself: the specific input or state, and the specific wrong outcome. If you cannot write it without the words "could", "may", "might", "if someone later", or "risks", the finding is LOW. | ||
|
|
||
| Findings about LLM system-prompt or instruction text are LOW by default. They may be MEDIUM only when the text over-promises a capability: it names a tool the model will not have, claims a provider is connected when the code gates it off, or instructs an action the runtime forbids. Under-specification, wording, ordering, duplication between prompt constants, and "this could confuse the model" are always LOW. |
There was a problem hiding this comment.
When a PR changes a local-agent system prompt to direct an available state-modifying tool to delete or overwrite user files, this rule still classifies the resulting data-loss defect as LOW because the instruction does not over-promise an unavailable capability. That finding is then excluded from the findings JSON and cannot affect the ready recommendation; only speculative wording concerns should be categorically demoted, while executable instructions need normal impact-based severity.
Useful? React with 👍 / 👎.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Prompt-only change to The two notes below are about the new policy text weakening the security/correctness net, not about the pipeline breaking. Neither blocks merge. Issues Summary
🟢 Low Priority Notes (5 items)
Generated by Dyadbot persona-based code review |
github-actions
Bot
added
the
needs-human:review-issue
Codex review on #4406 caught that the intent bullet was written as an unconditional suppression rule: an author who describes a security regression as intentional would force the reviewer to demote it, and since LOW never becomes an inline comment and the recommendation stays `ready`, an author-controlled description could hide a merge blocker. Split intent into two rules. Stated intent settles design disagreements -- disagreeing with a deliberate product decision stays LOW, which is what the 22 intent-misread findings in the audit needed. Intent never lowers the severity of a demonstrable defect: a crash, data loss, a corrupted write, a security or IPC boundary hole, a failing test suite, or a broken feature is reported at the severity its impact earns, with a note that the description calls it intentional. Reviewing the fix surfaced the same failure mode twice more in the rules I added, both of which would have cost confirmed must-catch findings: - "remedy is work beyond this PR's stated scope" hangs a suppression rule on an author-controlled phrase, and reads as covering [4381-1], where deleting AutoApproveSwitch stranded a Playwright helper in a file the diff never touched -- 121 call sites across 60 specs would time out. Now split explicitly: breakage is a defect wherever its remedy lives; only optional follow-up work is LOW. - The always-LOW list could contradict the HIGH definition, e.g. for [4381-8] where the remedy is "migrate the specs" but the outcome is a failing suite. Added a precedence rule -- impact always wins over category -- rather than trying to word each bullet defensively. Re-checked all 14 confirmed merge-blockers from the audit against the revised rules; all 14 still surface at HIGH. Re-checked the intent demotions that motivated the original bullet (#4398's fail-closed auth policy, #4394's picker restructure); both remain LOW, since neither crashes, breaks a feature, or fails a test. Template still renders with the same three variables. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a987e32eaa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Severity is decided by user-visible impact, not by code quality. | ||
|
|
||
| - HIGH - the diff as written breaks something. You can name the input or state that triggers it and the wrong outcome someone sees: a crash, data loss, a corrupted write, a security or IPC boundary hole, a test suite that will fail, a feature that will not work. | ||
| - MEDIUM - the diff as written degrades a real behavior you can demonstrate, but the feature still works: a wrong error message, a missing error state on a path users hit, a diagnostic that logs the wrong value, an edge case that produces a confusing result. |
There was a problem hiding this comment.
When a diff changes a diagnostic to log the wrong value, this line classifies the defect as MEDIUM, while line 37 says telemetry and logging are always LOW. Those mutually exclusive instructions make the same change eligible for an inline finding or mandatory suppression depending on which rule the reviewer follows, undermining the intended severity recalibration; choose one severity for incorrect diagnostic content.
Useful? React with 👍 / 👎.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Prompt-only change to The machine-checked contract survives intact. No new The issues below are about the new severity rubric's interaction with rules elsewhere in the same file. None of them break the pipeline, so none block merge. Because the deliverable is instruction text, the real effect is behavioral and can only be confirmed on live runs — I am flagging specific internal conflicts I can point at in the diff, not predicting an outcome. Issues Summary
🟢 Low Priority Notes (4 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 279 passed, 1 failed, 1 flaky, 12 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/local_agent_auto.spec.ts
|
1 participant
Cuts the false-positive rate of the Dyadbot PR reviewer by fixing its severity calibration. Prompt-only — no pipeline or workflow changes, so
codex-pr-review.ymlis unaffected.Why
I audited every
dyad-assistantinline comment on the last 25 PRs by wwwillchen/keppo-bot (#4343–#4400): 229 comments across 83 review runs. Each was re-judged against the actual code at the commit it was posted against.The reviewer is not hallucinating — 196 of 229 findings are factually true, only 2 are outright false. The problem is that it has no way to say "minor":
71% of inline comments did not deserve to be inline. Independent signals agree:
The three causes this PR fixes
1. There was no severity rubric. The only guidance was
Use the same review standard as Dyad's multi-persona PR review, which resolves tocode-health-reviewer.md:21: "Sloppy code that hurts maintainability is a MEDIUM severity issue, not LOW. We care about code health." The prompt never defined LOW at all, so the model had no criterion to demote anything.Replaced with an impact-anchored rubric (HIGH breaks something, MEDIUM degrades a demonstrable behavior, LOW is everything else), an explicit always-LOW list — duplication, dead code retained during a migration, naming/comments, missing test coverage, defensive guards for inputs no caller passes, telemetry shape, LLM prompt wording — and a calibration note that 0 HIGH / 0–2 MEDIUM is the expected result, and finding nothing above LOW is a good outcome.
2. There was no admission gate. The old line 3 licensed "maintainability problems that can cause future defects" — an invitation to report things that are not defects yet. All 60 speculative and all 48 scope-creep findings in the corpus were over-posted.
Now: the first sentence of
bodymust name the concrete trigger (input/state → wrong outcome); if you can't write it without "could/may/might/risks", it's LOW. Scope-creep remedies ("retire the setting", "migrate the other specs") are LOW by rule. Base-branch behavior is out of scope. Constraint 4 changes from a disclaimer ("say so plainly") into an admission gate ("uncertainty is a reason to demote").3. Three personas and a 7-bullet checklist acted as quotas. "Code Health Expert" maps almost exactly onto the style/duplication/dead-code categories; "UX Wizard" produced the cursor-pointer notes. The Dyad-specific bullets read as an audit checklist to run against every PR.
Collapsed to one senior-engineer voice, and the Dyad bullets are now gated triggers — "apply a check ONLY if the diff touches the thing it names".
Safety
Each rule was checked against the 14 confirmed merge-blockers in the corpus — every one names a concrete trigger and a concrete wrong outcome, so all 14 survive. The rubric also recovers the two HIGHs that shipped at MEDIUM.
Deliberately not done here: LOW findings stay out of the findings JSON.
validate-review.mjs:203throws when findings exist with no Issues Summary table, and the posting step iscontinue-on-error— a LOW-only run would silently drop all of its inline output.Testing
render-template.mjsrenders the new template successfully with the same three variables (CONTEXT_PATH,OUTPUT_MD_PATH,OUTPUT_FINDINGS_PATH); no new{{VAR}}introduced.validate-review.mjsparses is unchanged: verdict line, recommendation line,### Issues Summarytable shape, and findings JSON schema are all byte-identical to before. The example separator row still matches the row filter added in Fix PR review validation for hyphenated issue paths #4405.Follow-ups not in this PR
synchronizeand rebuilds the full diff every time, so Give Implementers capability-aware project guidance #4389 got 31 cold reviews and 72 inline comments. 52 findings were repeats of earlier runs. Fix: persist the last-reviewed SHA and build context fromcompare/{lastSha}...{headSha}, plus debounce the trigger.issues.createCommentnever upserts, so Give Implementers capability-aware project guidance #4389 carries 31 summary comments.#skip-bugbot(AGENTS.md:184) is not honored by this workflow, and Simplify model picker with recent models and nested catalog #4399 collected 4 inline comments 11 minutes after it was closed.🤖 Generated with Claude Code