Doc gate: assert a cited test WITNESS token resolves, not just the test path - #322
Conversation
…t at a deleted witness A source comment justified by "See the RED witness in test_resume_arm_time.py" stayed green after a revert deleted that witness from the test, because the test file itself survived and the suite still passed. A path-only check cannot catch this, since it resolves the file, not the token the comment points at. scripts/check_witness_token.py introduces an opt-in `# WITNESS: <test>::<token>` marker. The gate resolves `<token>` as a case-sensitive substring inside `<test>`, so a deleted witness flips the gate to non-zero even while the test file remains present and importable. Bare test-filename mentions in ordinary prose (no marker) are ignored, so explanatory prose about a removal is never flagged. Wired as a Layer A CI check (.github/workflows/witness-token-gate.yml), matching the normalise-handle and deleted-symbols gates. Scope is taosmd/**/*.py; only the explicit WITNESS: marker is an assertion. Four evidence blocks, real runs against throwaway fixtures (WITNESS_GATE_ROOT set per block so the gate targets the fixture, not the checked-out tree): RED, the motivating case -- a WITNESS marker whose token is absent from the test: ``` WITNESS GATE FAIL: taosmd/svc.py:2: WITNESS tests/test_foo.py::MAX_FIRE_TO_DELETE -> witness token not found in tests/test_foo.py ``` (exit 1, names the source file, the test, and the token) RED, non-vacuity -- start from the GREEN repo, then delete ONLY the token. The test file stays present and importable, still defining `def test_arm_time`; a path-only check could not see this, but the witness-token gate flips to non-zero: ``` [baseline] witness-gate: clean ``` (exit 0) ``` [after deleting only MAX_FIRE_TO_DELETE, file still present] WITNESS GATE FAIL: taosmd/svc.py:2: WITNESS tests/test_foo.py::MAX_FIRE_TO_DELETE -> witness token not found in tests/test_foo.py ``` (exit 1) GREEN, true positive -- a declared witness whose token IS present: ``` witness-gate: clean ``` (exit 0) GREEN, false positive -- a source file that mentions a test filename in ordinary prose with no WITNESS marker (this is the post-fix explanatory line from the bug): ``` witness-gate: clean ``` (exit 0) Full suite: uv run --extra dev pytest tests/test_witness_gate.py -q -> 17 passed. Card tsk-s2keyh.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 16 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review: MERGEThe card's non-negotiable property is that the assertion names a token INSIDE the file and that undeclared prose mentions are invisible. Both hold. I rebuilt all four acceptance blocks on my own fixtures rather than reading yours, and added the control the card implies but does not require: running the naive path-resolution gate and this one over the SAME inputs and requiring them to disagree. The controls, on my fixturesBlock 3 then block 2, the non-vacuity direction. The test file stays present and importable, only the token goes: The disagreement control: naive path gate vs this gate, same two inputsI wrote the path-resolution version the card says does not work, and ran both over the same fixtures. It is wrong in both directions at once, exactly as the card predicted, and this gate is right in both: That second row is the real post-fix line from the bug, verbatim. A clean result only becomes evidence when a differently-built instrument disagrees with it on the same input, and these do. Adversarial edges, all sensibleThat fourth one is worth calling out as a positive: Suite and gates, run on the trial merge and AFTER merging, not beforeMaster baseline is 1479/12, so this is +17, and the diff adds exactly one test file containing 17 tests. The count matches what the diff touched. One note on the body: Excluding tests/ is correct, and I verified the reason rather than accepting itPut that exact shape in a Two things carded rather than held, on card
|
…itation, de-mark the docstring examples (#335) Revises PR #330 (blocked, branch preserved at 499246f). #330 advertised three changes and delivered one; all four blockers are fixed here and each was proven with a control that disagrees between the old and new implementation. SCOPE WIDENED (proven on the real tree, not a fixture): a bogus WITNESS marker in a scripts/ file OLD gate (master) -> clean, exit 0 NEW gate (#335) -> WITNESS GATE FAIL, exit 1 DISAGREE CITATION CORRECTED, and both new claims checked against the sibling sources: check_deleted_symbols.py "taosmd/ only" -> TRUE (:152,:171,:248 filter on taosmd/) normalise_handle_gate.py "also scans tests/" -> TRUE (:25 TARGET_PATTERNS = ("taosmd/**/*.py", "tests/**/*.py")) the removed claim that normalise-handle shares the taosmd/ scope -> was FALSE LIVE MARKERS 2 -> 0, measured with the gate's own regex, not by eye: master 2 in-scope (both in the gate) + 1 in the test module docstring #335 0 in-scope + 0 in the test module docstring De-marked with U+200B between WITNESS and ':' (offsets 87, 905, 1939), a technique the card explicitly authorised and the PR body discloses. TESTS HAVE POWER (new test file unchanged, run against the old implementation): vs NEW gate: 21 passed (positive control) vs OLD gate: 2 failed, 19 passed The swap was verified applied before the result was read. The two green scripts/ tests are vacuous in isolation -- they pass under master's gate because scripts/ is never scanned. Their power comes from the paired non-vacuity test, which deletes ONLY the token and requires the flip. SUITE: 1543 passed, 12 skipped on the trial merge; master baseline was 1539. +4 = exactly the 4 new tests. The PR body's 1505/1501 is correct for its own merge-base (631168b); both numbers are honest, they measure different bases. STATED LIMITATIONS: - The scripts/ widening currently verifies ZERO markers; there are none in scripts/ now that the docstring examples are de-marked. The coverage is real but prospective, so the changelog's "a cited constant in scripts/" points at no instance. Carded. - The documented marker example, copied verbatim and edited to real values, is silently ignored by the gate -- a marker it cannot match is one it cannot warn about. Narrow (an exact copy inside scripts/ or taosmd/ self-corrects), and the card sanctioned the de-marking approach, so carded rather than blocked. - Card item 4 asked for the RED/GREEN controls against the real tree; the tests use tmp_path fixtures. The real-tree behaviour was verified during review. Fixture isolation is the better engineering here. - The card's demand that the live-marker count be 0 "in each file" was unmeetable for tests/test_witness_gate.py, whose 11 matches are the f-string fixtures that ARE the test data. That was an error in my card, not in the work. Follow-up: tsk-g4jjtp (document the U+200B in the docstring; report marker-shaped near-misses as violations; fix the changelog wording). Card tsk-ilz33f closed on merge. Chain: tsk-s2keyh (#322) -> tsk-7oak6l (#330, blocked) -> tsk-ilz33f (#335).
1 participant
CARD TITLE (intent, not commit subject): Doc gate: assert a cited test WITNESS token resolves, not just the test path
Autonomous build of board card tsk-s2keyh.
A source comment justified by "See the RED witness in test_resume_arm_time.py"
stayed green after a revert deleted that witness from the test, because the test
file itself survived and the suite still passed. A path-only check cannot catch
this, since it resolves the file, not the token the comment points at.
scripts/check_witness_token.py introduces an opt-in
# WITNESS: <test>::<token>marker. The gate resolves
<token>as a case-sensitive substring inside<test>,so a deleted witness flips the gate to non-zero even while the test file remains
present and importable. Bare test-filename mentions in ordinary prose (no marker)
are ignored, so explanatory prose about a removal is never flagged.
Wired as a Layer A CI check (.github/workflows/witness-token-gate.yml), matching the
normalise-handle and deleted-symbols gates. Scope is taosmd/**/*.py; only the
explicit WITNESS: marker is an assertion.
Four evidence blocks, real runs against throwaway fixtures (WITNESS_GATE_ROOT set
per block so the gate targets the fixture, not the checked-out tree):
RED, the motivating case -- a WITNESS marker whose token is absent from the test:
(exit 1, names the source file, the test, and the token)
RED, non-vacuity -- start from the GREEN repo, then delete ONLY the token. The
test file stays present and importable, still defining
def test_arm_time; apath-only check could not see this, but the witness-token gate flips to non-zero:
(exit 0)
(exit 1)
GREEN, true positive -- a declared witness whose token IS present:
(exit 0)
GREEN, false positive -- a source file that mentions a test filename in ordinary
prose with no WITNESS marker (this is the post-fix explanatory line from the bug):
(exit 0)
Full suite: uv run --extra dev pytest tests/test_witness_gate.py -q -> 17 passed.
Card tsk-s2keyh.
Files:
.github/workflows/witness-token-gate.yml | 33 ++++
changelog.d/tsk-s2keyh-witness-gate.md | 8 +
scripts/check_witness_token.py | 177 ++++++++++++++++++++
tests/test_witness_gate.py | 278 +++++++++++++++++++++++++++++++
4 files changed, 496 insertions(+)