review: bugfix-PR regression-coverage checklist, Phase 1 (#93) - #105
Conversation
#93 asks the reviewer to make regression coverage a formal duty on bugfix PRs. Phase 1, data-plane only, one duty stated once per mode: - Strict: a "Bugfix PR regression coverage" section in the injected checklist page (repos/vllm-omni/review/guides/strict-review-checklist.md) — pin the ORIGINAL failure path (failing pre-fix, root-cause-aligned, happy path does not count), check same-class entrypoints/boundaries, and conclude with the covering test or an explicit blocking/non-blocking finding. Fenced "only when the PR is a bugfix", so non-bugfix reviews are unaffected. Page stays inside its 7,000-char injection budget (6,991 after the edit — the next section added here must trim first). - Direct: one conditional item in _DIRECT_REVIEW_CHECKLIST with the same duty, guarded by the same bugfix condition. Guardrail test pins the Direct item; _index.md description extended; SPEC page re-verified. Knowledge validators, wiki lint, and the vllm-omni release audit (enforce, CLEAN) all pass. Per the house eval rule this is the cheap reversible trial route — checklist data, no prompt/pipeline changes; measurement before any Phase 2 (planner bugfix signal + dedicated manifest key). Refs #93. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165QsXAeLVPrKfj8Yk5zns3
![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: 2c1c63a275
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # unassisted baseline caught and Direct missed, and neither belongs to any component | ||
| # owner, so no knowledge route will surface them. | ||
| "When the diff adds or changes a test, check the assertions bind to real behavior and not to values the fixture, mock, or fake injected.", | ||
| "When the PR is a bugfix (title, labels, or linked issue), require a regression test that pins the original failure path; happy-path-only additions do not count, and a missing pin becomes an explicit blocking or non-blocking finding, never silence.", |
There was a problem hiding this comment.
The new Direct item asks the host to condition on labels or linked issues, but the Direct contract only tells callers to collect and pass title, body, and changed_files, and review() has no labels or linked-issues input. For a PR with a neutral title/body whose only bugfix signal is a bug label or a GitHub sidebar link, a conforming Direct host has no data with which to activate this requirement, so the regression-coverage check is skipped; extend the Direct snapshot/input contract to include these signals or classify them before returning the checklist.
Useful? React with 👍 / 👎.
| # unassisted baseline caught and Direct missed, and neither belongs to any component | ||
| # owner, so no knowledge route will surface them. | ||
| "When the diff adds or changes a test, check the assertions bind to real behavior and not to values the fixture, mock, or fake injected.", | ||
| "When the PR is a bugfix (title, labels, or linked issue), require a regression test that pins the original failure path; happy-path-only additions do not count, and a missing pin becomes an explicit blocking or non-blocking finding, never silence.", |
There was a problem hiding this comment.
This Direct item stops after requiring a test for the original failure and a finding when that test is absent. Unlike the Strict addition, it never asks reviewers to inspect same-class entrypoints and boundaries or to record the reason and residual risk for accepted gaps. A narrowly targeted regression test can therefore satisfy Direct while identical failure paths remain uncovered, so Direct does not carry the claimed coverage-completeness duty.
Useful? React with 👍 / 👎.
| Only when the PR is a bugfix (title/labels/linked issue): demand a regression | ||
| test pinning the ORIGINAL failure path — failing before the fix, matching the | ||
| root cause, not a happy path (a test that cannot fail counts as missing). | ||
| Check same-class entrypoints and boundaries; an accepted gap must state reason | ||
| and residual risk. Conclude by naming the covering test/case or filing an |
There was a problem hiding this comment.
This persists a new executable review-experience mandate in a guides/ page, even though the repository contract requires PR-learning and review-experience intake to produce executable rules in the nearest owner's rules.md. Keeping the demand/check/conclude requirement here bypasses owner-scoped rule routing; merge it into the appropriate owner rule and update the checklist injection/index without duplicating the conclusion.
AGENTS.md reference: AGENTS.md:L12-L17
Useful? React with 👍 / 👎.
1 participant
What
Makes regression coverage a formal review duty on bugfix PRs, in both modes, as asked
in #93 — Phase 1: data-plane only, no prompt or pipeline changes.
(
knowledge/repos/vllm-omni/review/guides/strict-review-checklist.md, theknowledge.review_checklistmechanism from the deep-engine RFC): demand a regressiontest pinning the original failure path (failing before the fix, aligned with the
root cause — happy-path-only does not count, and a test that cannot fail counts as
missing), check same-class entrypoints/boundaries with stated reason + residual risk for
accepted gaps, and conclude by naming the covering test or filing an explicit
blocking/non-blocking finding. The section is fenced "only when the PR is a bugfix
(title/labels/linked issue)" — non-bugfix PRs are unaffected (issue acceptance
criterion 1).
_DIRECT_REVIEW_CHECKLISTcarrying the same dutyunder the same bugfix condition, so both modes state one duty without a conflicting
channel (issue acceptance criterion 4).
Mapping to #93's acceptance criteria
coverage check. ✔
blocking/non-blocking conclusion duty. ✔
Out of scope, per the issue: generating test implementations; overriding human judgment on
flaky/environment-bound cases.
Budget note
The checklist page's own header caps Strict injection at its first 7,000 chars; the page
is at 6,991 after this edit. The commit records that the next section added there must
trim first.
House-rule note (measurement)
Review-behavior changes ship measured. This is deliberately the cheap, reversible trial
route — adapter-scoped checklist data. If it earns its keep on a measured trial, Phase 2
(a deterministic bugfix classification signal in
review/planner.py+ a dedicatedknowledge.bugfix_checklistmanifest key, so Strict injection itself becomes conditional)is a separate PR.
Verification
knowledge/tools/check_knowledge_tree.py+check_wiki_lint.py— 0 errorstools/audit_vllm_omni_release.py --mode enforce(baseline from→to, local vllm-omnicheckout) — RESULT: CLEAN
test_thin_mcp_server.py); full offline suite greendoc/architecture/SPEC/thin_mcp_server.mdre-verified 2026-08-25Refs #93.
🤖 Generated with Claude Code
https://claude.ai/code/session_0165QsXAeLVPrKfj8Yk5zns3