diff --git a/docs/quality/audit.md b/docs/quality/audit.md index 4bdd686c..88740086 100644 --- a/docs/quality/audit.md +++ b/docs/quality/audit.md @@ -393,7 +393,7 @@ CodeyBox ships these audit-type presets as YAML resources (see `docs/quality/pre | `quality` | LLM review focus for dead code, magic numbers, naming, error handling. | | `completeness` | LLM review focus for TODOs, missing tests, half-finished impls. | | `cheating` | Deterministic diff-patterns + LLM review focus for agent shortcuts. | -| `tests` | Deterministic diff-patterns for no-op assertions + LLM review focus for test meaningfulness. | +| `tests` | Deterministic diff-patterns for no-op assertions + LLM review focus for test assertion quality (anti-gaming). Test existence/completeness is owned by the deterministic `tests:coverage` gate below. | A project enables a preset by listing its name in `Audit.AuditTypes` (see `docs/concepts/projects.md`). @@ -467,6 +467,23 @@ tool auditor (`Required = None`), auto-included by the composer like diff. A project opts out via `ExcludedAuditors`. See [`coverage.md`](coverage.md) for the full configuration. +### Meaningfulness review vs coverage gate + +Test existence and test quality are owned by different auditors by design, so the +audit converges instead of re-litigating the same gap on every iteration: + +* `tests:coverage` (deterministic) owns **existence / completeness**: every executable + line changed in the item's diff must be exercised by a test. It is stateless per + iteration and never judges whether an assertion is any good. +* `tests:meaningfulness-review` (LLM) owns **assertion quality / anti-gaming only**, + bounded strictly to tests added or modified in the diff: tests that execute code but + assert nothing, tests that assert on a mock/stub instead of the result under test, + missing error/edge-path assertions inside those tests, and coverage-padding tests + written only to satisfy the gate. It must never enumerate untested internal methods, + demand tests for code outside the diff, or report the mere absence of a test — that + is the coverage gate's job. Like the gate, it is stateless / isolated per audit + iteration, with no cross-iteration memory. + ## Rework prompt When an audit iteration fails, `ReworkPromptBuilder` assembles a prompt diff --git a/src/CodeyBox.Audit.Llm/LlmReviewAuditor.cs b/src/CodeyBox.Audit.Llm/LlmReviewAuditor.cs index a1707fb9..23068020 100644 --- a/src/CodeyBox.Audit.Llm/LlmReviewAuditor.cs +++ b/src/CodeyBox.Audit.Llm/LlmReviewAuditor.cs @@ -115,9 +115,14 @@ public string? SelfReviewGuidance """; private const string TestsGuidance = """ -- **Adequacy**: Ensure each new public class, function, endpoint, and error path has at least one test. -- **Meaningfulness**: Avoid implementation-mirroring, pure-mock tests, no-assertion tests, and trivially-true assertions. -- **Edge cases & failures**: Cover boundaries, empty, null, unicode, timeouts, network errors, and resource exhaustion. +- **Assertion quality, not existence**: test existence and line coverage belong to the + deterministic tests:coverage gate — never ask for a test that does not exist and never + enumerate untested methods or branches. +- **Gaming tests**: flag added/modified tests that execute code but assert nothing, assert + on a mock/stub instead of the SUT-produced value, or pad coverage without verifying + behavior. +- **Edge assertions within the test**: the added test should assert the error/edge paths + it executes, not just the happy path. - **Heuristic**: Ask yourself: "if I introduced a plausible bug (off-by-one, inverted condition, forgotten null check), would this test catch it?" """; diff --git a/src/CodeyBox.Audit.Presets/Defaults/audit-types/tests.yaml b/src/CodeyBox.Audit.Presets/Defaults/audit-types/tests.yaml index bc99992e..550e9a1e 100644 --- a/src/CodeyBox.Audit.Presets/Defaults/audit-types/tests.yaml +++ b/src/CodeyBox.Audit.Presets/Defaults/audit-types/tests.yaml @@ -40,62 +40,56 @@ patterns: - regex: '^\s*assert_eq!\s*\(\s*(\w+)\s*,\s*\1\s*\)' description: 'assert_eq!(x, x) (no-op)' reviewFocus: |- - You are reviewing TESTS: are the tests in this diff MEANINGFUL, and is the change's - important functionality actually VALIDATED by tests that would fail if it were broken? - Read BOTH halves of the change: the production code AND the test code. - - Why this lane exists: mechanical line/branch coverage proves a line executed — it does NOT - prove the feature works. AI-written code frequently compiles and passes mock-only tests - while the real path is broken. An INTEGRATION/functional test that drives the real wiring - is the main defense. That gap is exactly what you are here to catch. - - SCOPE — IN-LANE: (1) meaningfulness of tests ADDED or MODIFIED in this diff; (2) validation - of the change's critical/major functionality, especially through real wiring; (3) tests - disabled, skipped, or removed in this diff. - OUT-OF-LANE: bare per-symbol coverage counting — "method/branch X has no test" with NO - behavioral claim — never file it; file per PIECE OF FUNCTIONALITY instead, one finding per - piece. Also out: production code style, architecture, and task completeness — except per - [CROSS-LANE-SECURITY]. - - NON-EXECUTABLE CHANGE EXEMPTION (applies before Step 2): when the diff changes only - documentation, prose, comments, static marker/content files, or declarative metadata that - cannot affect runtime/build/deploy behavior, do NOT require a new test and do NOT file a - missing-test finding. Existing build/validation commands or direct artifact inspection are - sufficient for this lane. Still apply Steps 1 and 3 normally if the diff adds, modifies, - disables, or removes tests. - - DEFINITIONS (closed — do not extend them): - - CRITICAL functionality = ANY of: - (i) a deliverable explicitly listed in the task text; if the task is prose with no - enumerated deliverables, ONLY the single primary behavior of its first sentence — - do not infer more; - (ii) a security / auth / integrity / limit guard; - (iii) a data-writing, state-transition, or persistence path — EXCLUDING - telemetry/metrics/log writes, which are ALWAYS non-critical; - (iv) an error / rollback / failure-recovery branch of a new operation; - (v) behavior whose correctness depends on REAL component interaction: a DB, - filesystem, network, subprocess, IPC or hardware/peripheral interface, - cross-module flow, or serialization round-trip (exactly where mock-only tests - lie). - - MAJOR = important, user-facing or relied-upon behavior that is not critical. - - NON-CRITICAL = everything else: happy-path variants, normalization, formatting, logging, - metrics off-branches, distribution assertions, behavior-preserving refactors. - - DOUBT (explicit): critical-vs-major → treat as MAJOR; major-vs-non → treat as - NON-critical. - - ===== SEVERITY RUBRIC — TESTS ===== - - STEP 1 — MISLEADING TEST (a test added or modified in this diff) → ERROR when it fails - either check: - - 1a ASSERTION EXISTENCE: it has NO assertion whose subject is a value PRODUCED by the - code under test (a return value, thrown exception, persisted/emitted output, or - observable state change). No assertions at all, "did not throw" on a value-returning - contract, or only tautologies (NotNull on a non-nullable return, Count >= 0, x == x, - Assert.True(true)) → FAIL. - - 1b REGRESSION SENSITIVITY (the assertion-dataflow mutation test): for each assertion in + You are reviewing TEST ASSERTION QUALITY: do the tests ADDED or MODIFIED in this diff + actually prove the behavior they execute — with assertions that would fail if the + behavior were broken? Read BOTH halves of the change: the production code AND the + test code. + + Why this lane exists: mechanical line coverage proves a line executed — it does NOT + prove the feature works. AI-written tests frequently execute the code and stay green + while asserting nothing, asserting on a mock instead of the result, or padding coverage + to satisfy the gate. That gaming is exactly what you are here to catch. + + DIVISION OF LABOR (hard rule): test EXISTENCE and line-coverage completeness belong to + the deterministic `tests:coverage` gate, which blocks on changed executable lines no + test exercised. That gate owns "is there a test". You own "does the test prove + anything". NEVER report the mere absence of a test — no finding whose substance is + "X has no test", "add a test for Y", or "method/branch Z is uncovered", however + phrased. If no test exists for some production code, that is the coverage gate's + subject, not yours: stay silent. + + STATELESS: every audit iteration runs in isolation with no cross-iteration memory. + Judge ONLY the current diff. Never assume, reference, or carry over findings, + promises, or missing tests from a previous iteration; the same input must yield the + same findings. + + SCOPE — strictly BOUNDED to tests ADDED or MODIFIED in this diff. Every finding must + cite a test file:line for such a test. File one finding per gaming test, never one + per uncovered production symbol. + OUT-OF-LANE (never file): enumerating untested internal methods, branches, or + production paths; demanding tests for production code outside this diff; reporting + mere absence of a test (the coverage gate owns that); production code style, + architecture, and task completeness — except per [CROSS-LANE-SECURITY]. + + NON-EXECUTABLE CHANGE EXEMPTION: when the diff changes only documentation, prose, + comments, static marker/content files, or declarative metadata that cannot affect + runtime/build/deploy behavior, do NOT require a new test and do NOT file any finding + for having no new test. Existing build/validation commands or direct artifact + inspection are sufficient for this lane. Still apply the finding categories below + normally if the diff adds, modifies, disables, or removes tests. + + ===== SEVERITY RUBRIC — TESTS (assertion quality only) ===== + + FINDING 1 — EXECUTES BUT ASSERTS NOTHING (a test added or modified in this diff) + → ERROR when it has NO assertion whose subject is a value PRODUCED by the code under + test (a return value, thrown exception, persisted/emitted output, or observable state + change). No assertions at all, "did not throw" on a value-returning contract, or only + tautologies (NotNull on a non-nullable return, Count >= 0, x == x, + Assert.True(true)) → FAIL. + - REGRESSION SENSITIVITY (the assertion-dataflow mutation test): for each assertion in the test, trace the nearest production statement that computes the asserted value. Mentally mutate THAT statement (delete it / invert it / replace it with a constant). The - test is misleading iff NO assertion flips red under ANY such single-statement mutation + test is gaming iff NO assertion flips red under ANY such single-statement mutation of the production code. Derive the mutation targets from what the assertions actually read — never from the test's name or docstring. When the test mocks a collaborator, mutate the production SUT, not the mock. The following are ILLUSTRATIONS of this one @@ -104,39 +98,45 @@ reviewFocus: |- returns success/true/visible for the exact failure the test claims to catch); wrong unit (the exercised path is not the unit named); claim mismatch (the name says X, no executed assertion depends on X). - NOT misleading: a white-box assertion (cache hit, retry count, ordering, the specific + NOT gaming: a white-box assertion (cache hit, retry count, ordering, the specific query issued) is legitimate when inverting the underlying production behavior would flip it. - STEP 2 — UNVALIDATED FUNCTIONALITY. For each coherent piece of NEW critical/major - functionality, ask: does any test EXERCISE it working — through its real wiring — such - that the test would FAIL if the feature were broken? A test that mocks away every - collaborator, IO, or boundary does NOT validate the real path: it can stay green while the - feature is broken. - - A CRITICAL piece with no such test (no test at all, or only mock-only/trivial tests) → - ERROR. Name the missing test concretely ("no test drives a real merge through git and - asserts the commit lands"). ONE finding per piece of functionality, never one per - uncovered symbol. - - A MAJOR piece with no such test → WARNING. - - SECURITY OVERRIDE → ERROR: the untested thing is a guard on a nameable path from - less-trusted input (external caller, another user/tenant, model-agent output, repo file - content) to a dangerous sink. Do NOT relabel arbitrary internal validation as a - security guard to force a block — without untrusted reach, grade it as - critical/major/non-critical per the definitions. - - A per-branch gap of an otherwise-validated operation (the happy path IS exercised for - real; one branch is not) → WARNING at most. - PURITY NOTE for Step 2 descriptions: when real validation is missing because the decision - logic is inseparable from IO/state (everything must be mocked to test anything), say so - and name the enabling refactor in the finding: extract the pure decision core — testable - directly, input to output, no mocks — and keep ONE integration test driving the thin IO - shell. Pure functions are orders of magnitude cheaper to verify than mutable state; logic - structured that way needs far less scaffolding to be convincingly validated. - - STEP 3 — DISABLED / SKIPPED / REMOVED tests in this diff: [Skip], [Ignore], + FINDING 2 — ASSERTS ON A MOCK/STUB INSTEAD OF THE RESULT-UNDER-TEST → ERROR. The test + drives the production SUT but its assertions read back the mock's canned behavior + (mock received a call, stub returned what the test told it to return) rather than a + value the production code computed. Such a test stays green when the SUT is broken: + name the assertion and the SUT-produced value it should have read instead. A test that + must mock a boundary (network, clock, filesystem) still passes when at least one + assertion depends on SUT logic applied on top of the mocked seam — e.g. mapping, + validation, retry, or ordering the SUT itself performs. + + FINDING 3 — MISSING OBVIOUS ERROR/EDGE-PATH ASSERTIONS → WARNING. A test added or + modified in this diff executes an error, failure-recovery, or boundary path (empty, + null, timeout, malformed input, exhausted resource) but asserts only the happy path, + leaving the executed edge path unasserted. This is bounded to the test in front of + you: require the assertion, never a new test file or coverage of production code with + no test at all. When the decision logic is inseparable from IO/state (everything must + be mocked to test anything), say so and name the enabling refactor: extract the pure + decision core — testable directly, input to output, no mocks — and keep ONE + integration test driving the thin IO shell. + + FINDING 4 — COVERAGE-PADDING TEST → ERROR. A test added or modified in this diff whose + apparent purpose is to execute lines for the coverage gate rather than to verify + behavior: it calls into changed code but every assertion would still pass if the + exercised production logic were replaced with a constant or a stub (apply the Finding + 1 mutation test). Name the padded lines and the behavior-specific assertion that + would make the test genuine. Per [PARALLEL], file it here even though the cheating + dimension may also see it — it may not be running, and a padded test corrupts the + coverage signal the gate depends on. + + DISABLED / SKIPPED / REMOVED tests in this diff: [Skip], [Ignore], [Fact(Skip=...)], @pytest.mark.skip, x-prefixed, a commented-out body, or a DELETED test - whose covered production path still exists: - - covering CRITICAL or security-relevant behavior → ERROR (it advertises coverage that - does not run); + whose covered production path still exists. Disabling advertises coverage that does not + run, which games both this lane and the coverage gate: + - the disabled test covered security-relevant behavior (a guard on a nameable path + from less-trusted input — external caller, another user/tenant, model-agent output, + repo file content — to a dangerous sink) → ERROR; - otherwise → WARNING. Never treat a disabled test as merely "absent". FLAKE SOURCES → WARNING: a test added or modified in this diff whose outcome depends on @@ -144,65 +144,60 @@ reviewFocus: |- network/filesystem state in a unit-scope test where a fake or injected dependency was available (name the deterministic substitute: injected clock, seeded randomness, isolated fixture, fake). Flaky gate tests erode the whole pipeline's signal. If the nondeterminism - means the test can PASS while the behavior it claims to verify is broken, that is Step 1 - → ERROR. + means the test can PASS while the behavior it claims to verify is broken, that is + Finding 1 → ERROR. - STEP 4 — INFO: non-critical validation gaps; happy-path-only breadth notes; test - organization (names that don't describe the behavior, unrelated assertions bundled in one - test, fixtures that obscure simple checks); dead or undrifted fixtures. + BACKSTOP (hard rule): an ERROR exists ONLY via Finding 1, 2, or 4 (a gaming test in + this diff) or a security-relevant disabled-or-removed test. NEVER Error on a + per-method or per-branch coverage gap; NEVER Error on the mere absence of a test — + existence and completeness are the coverage gate's job, and doubt de-escalates: if + you cannot show the test would stay green under the Finding 1 mutation, WARNING at + most. - BACKSTOP (hard rule): an ERROR for a MISSING test exists ONLY via Step 2 (a critical piece - wholly unvalidated, or the security override) or Step 3 (critical/security - disabled-or-removed). NEVER Error on a per-method or per-branch coverage gap; NEVER Error - on ambiguous criticality — the doubt rules de-escalate it first. - - MUTATION HINTS for 1b — plausible single-statement bugs a meaningful test must catch: + MUTATION HINTS for Finding 1 — plausible single-statement bugs a meaningful test must catch: off-by-one; inverted condition (== vs !=, < vs <=); dropped null/empty check; wrong field used (id vs userId); wrong order of operations; skipped failure-path handling. - GOOD PATTERNS to look for when judging Step 2 (their presence is what "validated" means): - integration tests that exercise multiple real components together, where each must work - for the test to pass; direct input-to-output tests of pure decision logic (no mocks - required); failure-mode tests (timeout, network error, malformed input, - exhausted resources, partial failure/retry); edge cases (empty, null, boundary, max/min, - unicode, concurrent, very large); round-trip tests for serialization/encoding/encryption; - property tests for parsing and validation where feasible; a regression test reproducing - any bug this change fixes. + GOOD PATTERNS in the diff's tests (their presence is what "meaningful" means): + direct input-to-output tests of pure decision logic (no mocks required); failure-mode + assertions (timeout, network error, malformed input, exhausted resources, + partial failure/retry) inside the added tests; edge-case assertions (empty, null, + boundary, max/min, unicode, concurrent, very large); round-trip assertions for + serialization/encoding/encryption; a regression assertion reproducing any bug this + change fixes. INFO at most for the rest: test organization (names that don't describe + the behavior, unrelated assertions bundled in one test, fixtures that obscure simple + checks); dead or undrifted fixtures. Trivially-true assertion patterns (Assert.True(true), x == x, expect(x).toBe(x)) may also be flagged by a deterministic pattern layer. Per [PARALLEL], still file every one YOU - judge misleading — never assume the pattern layer caught it or will run. - - LOCATION: cite the test file:line for a misleading or disabled test; for missing - validation, cite the PRODUCTION file:line of the unvalidated functionality. - - CALIBRATION EXAMPLES (each closes a real reviewer split): - - A new IGitHost merge path whose ONLY test mocks the git subprocess and asserts the mock - was called; no test drives a real merge and asserts the commit lands → ERROR (Step 2, - critical (v)). This is the "compiles and passes mocks but doesn't actually work" case. - - A new provider that talks to an external CLI, tested only with a hand-written fake - returning canned success → ERROR if the provider is the task's core deliverable; - WARNING if it is a secondary/major feature. - - A new REST endpoint with a real happy-path functional test but no test for its failure - handling → WARNING (a major branch gap; the feature itself IS validated). - - "Rollback of patch-apply failure is untested" where the operation's happy path IS - exercised by a real test → WARNING (a branch gap of a validated operation, not a whole - unvalidated feature). - - "CaptureResourceMetrics off-branch untested" → INFO (metrics; always non-critical). - - "Method Foo() has no unit test", with no behavioral claim, where Foo is not a coherent - critical/major piece → DO NOT FILE (bare per-symbol coverage counting). - - Untested NormalizeRulesPath rejection of ".." traversal / embedded \0 → ERROR (security - override). + judge gaming — never assume the pattern layer caught it or will run. + + LOCATION: cite the test file:line of the added-or-modified test. Never cite a + production file:line to demand a test that does not exist. + + CALIBRATION EXAMPLES (each closes a real reviewer split; all bounded to diff tests): - A test asserts req.UrlSettings.Auth == "sprite", mirroring new - SpritesUrlSettings("sprite") with nothing transforming the value → ERROR (1b, mirrored - literal). + SpritesUrlSettings("sprite") with nothing transforming the value → ERROR (Finding 1, + mirrored literal). - A Playwright stub whose isVisible falls through to true, so emit-then-replay stays green - on broken selectors → ERROR (1b, success-stub gap). + on broken selectors → ERROR (Finding 1, success-stub gap). - A test whose body is only `await Sut.DoThing()` on a value-returning contract → ERROR - (1a). - - [Fact(Skip="flaky")] on the new merge-gate integrity test → ERROR (Step 3, critical). + (Finding 1). + - A new merge test that mocks the git subprocess and asserts only that the mock was + called, never the commit the SUT produced → ERROR (Finding 2). + - A new test that calls into changed code and asserts only `Assert.NotNull(result)` on + a non-nullable return so the coverage gate sees the lines → ERROR (Finding 4). + - A new REST-endpoint test asserting the happy-path response but executing the 4xx + branch with no assertion on it → WARNING (Finding 3: require the assertion, not a + new test). + - [Fact(Skip="flaky")] added on the new merge-gate test → WARNING (disabled; ERROR + only if the skipped test covered security-relevant behavior). + - "Method Foo() has no unit test", with no diff test to judge → DO NOT FILE (mere + absence; the coverage gate owns it). Same for "branch X uncovered" and "no test + drives feature Y end to end" — never file what you cannot cite to a diff test. Tests that cannot execute due to ENVIRONMENT limits (a missing external service, an unsupported platform) are out of scope for scoring. This does NOT exempt tests DISABLED IN - SOURCE ([Skip]/[Ignore]/commented-out/removed) — those are graded per Step 3. + SOURCE ([Skip]/[Ignore]/commented-out/removed) — those are graded per the disabled-test + rule above. Tests which cannot be run in this environment are not part of the scoring or auditing criteria. diff --git a/tests/CodeyBox.Tests/PresetCatalogTests.cs b/tests/CodeyBox.Tests/PresetCatalogTests.cs index 2f8692db..92a82ff2 100644 --- a/tests/CodeyBox.Tests/PresetCatalogTests.cs +++ b/tests/CodeyBox.Tests/PresetCatalogTests.cs @@ -15,7 +15,40 @@ public void TestsReviewFocus_ExemptsNonExecutableOnlyChangesFromMissingTestFindi Assert.Contains("NON-EXECUTABLE CHANGE EXEMPTION", reviewFocus, StringComparison.Ordinal); Assert.Contains("static marker/content files", reviewFocus, StringComparison.Ordinal); Assert.Contains("do NOT require a new test", reviewFocus, StringComparison.Ordinal); - Assert.Contains("Still apply Steps 1 and 3 normally", reviewFocus, StringComparison.Ordinal); + Assert.Contains("Still apply the finding categories below", reviewFocus, StringComparison.Ordinal); + } + + [Fact] + public void TestsReviewFocus_IsBoundedToDiffTestsAndDefersExistenceToCoverageGate() + { + var reviewFocus = new PresetCatalog().GetAuditTypeReviewFocus("tests"); + + // Division of labor: existence/completeness belongs to tests:coverage. + Assert.Contains("tests:coverage", reviewFocus, StringComparison.Ordinal); + Assert.Contains("NEVER report the mere absence of a test", reviewFocus, StringComparison.Ordinal); + + // Bounded to added-or-modified tests; must-nots for existence reporting. + Assert.Contains("strictly BOUNDED to tests ADDED or MODIFIED in this diff", reviewFocus, StringComparison.Ordinal); + Assert.Contains("enumerating untested internal methods", reviewFocus, StringComparison.Ordinal); + Assert.Contains("demanding tests for production code outside this diff", reviewFocus, StringComparison.Ordinal); + + // Stateless / isolated per audit iteration. + Assert.Contains("STATELESS", reviewFocus, StringComparison.Ordinal); + Assert.Contains("no cross-iteration memory", reviewFocus, StringComparison.Ordinal); + } + + [Fact] + public void TestsReviewFocus_CoversTheFourAssertionQualityCategories() + { + var reviewFocus = new PresetCatalog().GetAuditTypeReviewFocus("tests"); + + Assert.Contains("FINDING 1 — EXECUTES BUT ASSERTS NOTHING", reviewFocus, StringComparison.Ordinal); + Assert.Contains("FINDING 2 — ASSERTS ON A MOCK/STUB INSTEAD OF THE RESULT-UNDER-TEST", reviewFocus, StringComparison.Ordinal); + Assert.Contains("FINDING 3 — MISSING OBVIOUS ERROR/EDGE-PATH ASSERTIONS", reviewFocus, StringComparison.Ordinal); + Assert.Contains("FINDING 4 — COVERAGE-PADDING TEST", reviewFocus, StringComparison.Ordinal); + + // Quality-only calibration: absence of a test must never be filed. + Assert.Contains("DO NOT FILE (mere", reviewFocus, StringComparison.Ordinal); } private sealed class FakeAgent : IAgentRunner