fix(ce-code-review): add false-confidence guard for self-authored tests - #1276
fix(ce-code-review): add false-confidence guard for self-authored tests#1276danikdanik wants to merge 3 commits into
Conversation
When a builder writes both a fix and its tests in the same pass, the tests may pass even without the fix -- assertions look specific but don't actually depend on the changed code. The testing-reviewer persona now mentally reverts the implementation to its pre-fix state and checks whether the test would still pass. If it would, the test is theatrical. This is the single highest-risk verification gap when delegating implementation to a cheaper model that writes code and tests in one pass. Reviewed by DeepSeek V4 Flash via OpenCode before submission -- findings addressed: removed inline severity to let the confidence rubric govern, removed ambiguous phrasing, sharpened distinction from the existing false-confidence bullet.
![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: 9edb95a441
ℹ️ 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".
| - **Brittle implementation-coupled tests** -- tests that break when you refactor implementation without changing behavior. Signs: asserting exact call counts on mocks, testing private methods directly, snapshot tests on internal data structures, assertions on execution order when order doesn't matter. | ||
| - **Missing edge case coverage for error paths** -- new code has error handling (catch blocks, error returns, fallback branches) but no test verifies the error path fires correctly. The happy path is tested; the sad path is not. | ||
| - **Behavioral changes with no test additions** -- the diff modifies behavior (new logic branches, state mutations, changed API contracts, altered control flow, or error behavior) but adds or modifies zero test files. This is distinct from untested branches above, which checks coverage *within* code that has tests. This check flags when the diff contains behavioral changes with no corresponding test work at all. Non-behavioral changes (formatting, comments, type-only annotations, or dependency/config metadata that does not alter runtime behavior) are excluded. | ||
| - **Self-authored tests in the same pass as the fix (false-confidence guard)** -- when the same diff introduces both a fix and its tests, the test may pass even without the fix. Verify the test is a real guard by mentally reverting the implementation to its pre-fix state and checking whether the test would still pass. If it would, the test is theatrical. This is distinct from line 11 (tests that don't assert behavior): here the assertions look specific but don't actually depend on the fix. This is the single highest-risk verification gap when a builder writes both code and tests in one pass. |
There was a problem hiding this comment.
When this persona is injected into a reviewer prompt, the source file's line numbers are not part of the prompt contract, and this list will also drift as bullets are added or reordered. In that runtime context, line 11 is an unstable cross-reference even though the adjacent bullet title is the actual durable anchor; a literal reviewer can fail to distinguish this new guard from the existing false-confidence check or follow the wrong reference after a future edit. Refer to the named bullet instead of the line number and keep the wording to the falsifiable distinction.
AGENTS.md reference: AGENTS.md:L118-L123
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5ebd788. Replaced line 11 with a reference to the named bullet (Tests that don't assert behavior) and updated the contract test to assert the durable reference instead of the line number.
…lse-confidence guard Address review feedback on PR EveryInc#1276: - Title said 'same pass' while body used 'same diff' -- align to 'same diff' - Add contract test asserting the new bullet's presence and distinctness, matching the existing behavioral-changes contract check pattern
…t title Address second review nit on PR EveryInc#1276: 'line 11' reference breaks if bullet ordering shifts. Replace with the bullet's actual title 'Tests that don't assert behavior' for a stable cross-reference.
1 participant
Summary
testing-reviewerpersona ince-code-review: when a diff introduces both a fix and its tests, the reviewer mentally reverts the implementation to its pre-fix state and checks whether the test would still pass. If it would, the test is theatrical -- it provides false confidence without actually guarding the fix.Why
From a 2-week session retrospective: post-review fix commits are a steady tax (~25+ per 22 days). A meaningful share comes from builders writing tests that pass for the wrong reason in the same pass as the fix. The review gate catches the bug eventually, but tightening the reviewer's ability to detect theatrical tests cuts the second-pass rate at the source.
Review process
This change was reviewed by DeepSeek V4 Flash Free (via OpenCode, free tier) before submission. Findings addressed:
requires_verification: truemetadata -- pattern break; no other bullet prescribes output metadataTest plan
bun testpasses (all existing tests green)