witness gate: requiring the :: payload silently drops de-marked markers that have no payload, and the changelog claims otherwise - #353
Conversation
…t changelog - Change _NEAR_MISS_RE from (?=.*::) to (?=[^:]*:) so de-marked markers whose separator is broken but that still contain a colon after the separator are caught, fixing a silent regression where '# WITNESS<ZWSP>: <path>' with no :: payload exited 0. - Keep ordinary prose without a colon invisible to the near-miss detector. - Note in the docstring that prose quoting the marker syntax (path::token) still trips the near-miss detector as a known residual. - Add Arm D (ZWSP, no :: payload) to test_near_miss_regex_spares_prose_arms so all four required arms live in one test. - Update _DEMARKED_MARKER_EXEMPTION line numbers to match the docstring. - Add changelog fragment tsk-y54vhk-witness-gate-near-miss-fix.md.
|
ⓘ 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: 3 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # They are intentionally de-marked and must not be reported; every other line | ||
| # in the same file (e.g. an appended genuine marker) still is. Greppable name. | ||
| _DEMARKED_MARKER_EXEMPTION = { | ||
| "scripts/check_witness_token.py": frozenset({4, 21, 38}), |
There was a problem hiding this comment.
WARNING: Hardcoded line numbers in _DEMARKED_MARKER_EXEMPTION
The frozenset {4, 21, 38} is hardcoded to match specific docstring lines. If the docstring in this file is modified in the future, these line numbers will drift, causing the near-miss detector to either falsely flag docstring examples or fail to flag genuine near-misses. Consider using a more robust mechanism (e.g., detecting de-marked markers dynamically, or adding a marker comment in the docstring).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| docstring examples are not. | ||
| - Corrected the near-miss detector to catch de-marked markers whose payload | ||
| is missing, fixing a silent regression introduced when the detector was | ||
| tightened to require the ``::`` payload. |
There was a problem hiding this comment.
SUGGESTION: Changelog claims old regex required :: payload
The changelog states the old regex required the :: payload, but the diff shows the previous regex was r"#\\s*WITNESS[^:]", which does not require ::. Consider revising the changelog to accurately describe the prior regex behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 208.4K · Output: 72.3K · Cached: 1M |
|
BLOCK — and to be unambiguous: the work in this PR is correct. The regex fix does close the defect it was filed for, verified below with both controls. This is blocked purely because the branch predates #344 and no longer merges. Revision card: Measured on a trial merge of The fix is rightRow 1 is the whole point: a de-marked marker carrying no Probe discipline note: my first pass at this table used prose strings like The blocker — the branch predates #344Cut from The PR's regex is the correct end state; it simply cannot be applied as-is. Not a problem, checked so it is not mistaken for one: the PR also re-adds The rebase hazard worth namingThe exemption set pins line numbers, and both sets are correct for their own file — they differ by 2 only because this PR adds two comment lines above them: After the rebase the merged docstring is a third arrangement, so these must be re-derived from the resolved file rather than taken from either side. They fail loud when wrong, so this is safe rather than silent — but it must actually be redone. A widening I flagged, then measured down to sizeRow 6 shows this PR flags a line master does not: prose with a plain colon and no Rather than leave that as a worry, I swept the real tree: Zero new flags and zero lost coverage on the current tree. So this is not a defect and not a reason to touch the regex — it warrants one accurate sentence in the comment, nothing more. Recording it because the comment currently reads as if Stated limitations
|
1 participant
CARD TITLE (intent, not commit subject): witness gate: requiring the :: payload silently drops de-marked markers that have no payload, and the changelog claims otherwise
Autonomous build of board card tsk-y54vhk.
whose separator is broken but that still contain a colon after the
separator are caught, fixing a silent regression where
'# WITNESS: ' with no :: payload exited 0.
still trips the near-miss detector as a known residual.
so all four required arms live in one test.
Files:
.../tsk-2k55kq-witness-gate-near-miss-hardening.md | 8 +++
.../tsk-y54vhk-witness-gate-near-miss-fix.md | 12 ++++
scripts/check_witness_token.py | 28 +++++---
tests/test_witness_gate.py | 78 ++++++++++++++++++++++
4 files changed, 118 insertions(+), 8 deletions(-)