Skip to content

Revise PR #353: the near-miss regex fix is correct and verified, but the branch predates #344 and no longer merges - #359

Closed
jaylfc wants to merge 1 commit into
masterfrom
exec/tsk-nss5ya
Closed

Revise PR #353: the near-miss regex fix is correct and verified, but the branch predates #344 and no longer merges#359
jaylfc wants to merge 1 commit into
masterfrom
exec/tsk-nss5ya

Conversation

@jaylfc

@jaylfc jaylfc commented Aug 18, 2026

Copy link
Copy Markdown
Owner

CARD TITLE (intent, not commit subject): Revise PR #353: the near-miss regex fix is correct and verified, but the branch predates #344 and no longer merges

Autonomous build of board card tsk-nss5ya.

REVIEW WARNING (automated): this card's text asks for tests, but the diff changes no test file. Either the acceptance criteria are unmet or the card needs correcting. Do not merge without resolving this.

Files:
changelog.d/tsk-nss5ya-witness-gate-near-miss-hardening.md | 3 +++
scripts/check_witness_token.py | 13 ++++++++++---
2 files changed, 13 insertions(+), 3 deletions(-)

@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@jaylfc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 33 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b1ddd235-d44f-4447-b9a3-70d97ac39194

📥 Commits

Reviewing files that changed from the base of the PR and between dddbb5d and 259e678.

📒 Files selected for processing (2)
  • changelog.d/tsk-nss5ya-witness-gate-near-miss-hardening.md
  • scripts/check_witness_token.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gitar-bot

gitar-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

Gitar

@jaylfc

jaylfc commented Aug 18, 2026

Copy link
Copy Markdown
Owner Author

The regex fix in this PR is correct, complete and fully verified, and it is not what blocks it. Five of six acceptance items are met. I re-measured all of them myself on a trial merge into origin/master 0778149f (merge-base e7593361, merge commit 7595ca8d), never on the branch head.

ITEM 1  rebase / conflict     Merge made by 'ort', no conflict; conflict-marker grep clean
                              git diff origin/master --stat on the merge = 2 files, +13/-3
ITEM 2  exemption pins        {4, 19, 36} still point at the three de-marked examples
ITEM 3  six-case table        reproduces the card's table EXACTLY
ITEM 4  code comment          names the plain-colon widening. Met.
ITEM 5  gates + suite         all three gates clean; 1617 passed, 12 skipped (baseline 1617)
ITEM 6  keep the test arms    NOT MET

Six-case table, re-run on the resolved tree with prefix engagement asserted on every case, and the widening sweep re-run on the merged tree because the tree moved since the card was filed:

case                                          engages  BASE   MASTER  PR#359
POSITIVE CONTROL: de-marked, NO :: payload    True     True   False   True
de-marked, WITH :: payload                    True     True   True    True
NEGATIVE CONTROL: genuine marker              True     False  False   False
prose, engages prefix, no colon at all        True     True   False   False
prose quoting path::token syntax              True     True   True    True
prose with a plain colon, no ::               True     True   False   True

scanned 353 tracked files; 24 lines engage the #WITNESS prefix (positive control)
flagged by BOTH: 9    new surface: 0    coverage lost: 0

Blocking: the only test that discriminates this fix was dropped

This PR touches zero test files. Item 6 asked for #353's tests/test_witness_gate.py arms to be kept.

The suite count hides this, so it is worth being exact. #353 added no new test method -- master and #353 both have the same 27. It added Arm D inside the existing TestWitnessGateIntegration::test_near_miss_regex_spares_prose_arms. Assertions added inside an existing test do not change the collected count, so 1617 passed is equally consistent with keeping or dropping it. I nearly recorded the flat count as evidence the arms were fine; it measures nothing here. Only the file list does.

Arm D is precisely the regression guard for the defect this chain exists to close. Measured with an API-preserving mutant -- everything this PR ships, with only the regex reverted to master's (?=.*::), replace target asserted present before writing:

                              Arm D absent (as shipped)      Arm D restored (as #353 had)
PR #359 regex                 25 passed                      25 passed
MUTANT (master's regex)       25 passed  <- NO DISCRIMINATION    1 failed  <- Arm D catches it

The mutant failure is exact: assert len(violations) == 1 -> assert 0 == 1.

So as shipped this changes gate behaviour and nothing in the repo tests it. The regex can be reverted and the suite stays green, which is the silent-gate failure mode #344, #353 and tsk-nss5ya were all filed over.

Blocking: the changelog fragment has no trailing newline

tail -c 1 changelog.d/tsk-nss5ya-witness-gate-near-miss-hardening.md | xxd -p  ->  2e

A period, not 0x0a. On the merged tree it is the only one of 32 fragments missing it, so the next appended entry concatenates onto it. That is the defect #351 fixed directory-wide and what #356 was closed over.

Blocking: an out-of-scope docstring insertion that states something false

The four lines added to the module docstring were not asked for by any card item, and:

  1. They say "Two de-marked examples". The exemption pins three, {4, 19, 36}.
  2. They are a near-verbatim duplicate of the comment still sitting above _DEMARKED_MARKER_EXEMPTION, so the same text now appears twice in one file.
  3. "Greppable name." is a statement about the constant's identifier. It makes sense in that comment and not in module prose.

Item 4 referred to the code comment above _NEAR_MISS_RE, which was correctly updated separately. Also worth recording: that insertion sits at line 45, after all three pinned lines, which is the only reason {4, 19, 36} did not shift. Item 2 passes by placement rather than by re-derivation.

Minor, not on its own worth a round: the PR also deletes a blank line after return [] in _extract_claims.

On the automated review warning

This PR's body carries the automated "card asks for tests but no test changed" warning. That warning has been wrong six times running here, but every one of those was a docs-only or dead-code card. This is a gate-behaviour card whose item 6 names a test file explicitly, and the warning is correct this time. Its record is now 6 wrong, 1 right, and it should not be dismissed by precedent.

Disposition

Closed under the close-on-block policy, branch kept. Revision card tsk-sl6s7s carries the verified regex work forward unchanged and asks only for Arm D, the trailing newline, and removal of the docstring insertion. Card tsk-nss5ya is closed and replaced by it.

Method note on my own run: implementation md5 41b29a32 was checked before and after the suite, and git status --porcelain was empty afterwards, so the run is uncontaminated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant