Repository navigation
fix(check-pr-fully-clean): strip posted timestamp verdict citations in reviews - #2668
Merged
dem-extra1 merged 32 commits intoAug 31, 2026
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… resolved blocking mentions
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Claude Code CLI (local session) is taking over this PR to resolve the merge conflict and address the open review finding (resolution-wording gate on Posted by Claude Code (AI agent) --- not written by a human. |
Both sides rewrote RESOLVED_BLOCKING_SUFFIX and the _is_resolved_blocking_mention prefix (this PR's 2ced776 vs main's #2684). The resolution keeps main's tense-checked, sentence-scoped structure and extends it to cover this PR's cases: plural verbs (are/were, have been), a parenthesized aside admitted as one clause unit, an agent/no-new-issues tail, the round-N/earlier/prior-round prefixes, and non-sentence dots (filenames, decimals) no longer truncating the sentence scan.
… harden the union
The unconditional strip erased LIVE verdicts phrased "posted <ts>,
verdict **Needs more work**" -- including findings re-raised across
rounds -- before the not-clean scanners ran (the fail-open direction).
A plain RESOLUTION_WORDING gate would break the motivating case, whose
narration carries no resolution wording, so the strip now requires a
POSITIVE attribution signal like _SHA_CITATION does: the parenthetical
must sit right after a markdown link to the cited round, or follow an
"in response to ..." phrase. A veto lookahead additionally refuses the
strip on re-raise wording, with a window that crosses semantic line
breaks and glued filename dots, and covers "not been addressed",
"must be fixed", "persists", "stands". The timestamp is anchored to a
real ISO-8601 shape (was [0-9T:Z-]+).
Adversarial review of the union merge also found two defects neither
side's suite could see:
- a negation fail-open: "None of the earlier blocking findings were
resolved" was exempted as a resolved mention; a negator check on the
prefix window now refuses it.
- exponential backtracking in the paren-aside clause scan (measured
26.8s at 22 groups): "(" matched both alternation branches; excluding
"()" from the character branch makes them disjoint, and a stray
unmatched paren fails safe.
Extends check-verdict-scan-parity.py's generator axes so the instrument
can produce both new shapes -- its prior zero was a coverage statement,
per its own docstring. Adds twelve regression tests (350 total pass).
…acent negation guard
Second adversarial round, four minors:
- The forward-only veto window missed a re-raise stated BEFORE the
citation and one past its 120-char bound; the veto now runs in code
over the citation's whole containing sentence in both directions
(glued-dot sentence bounds), with the aside itself excluded so its own
cited verdict cannot self-veto, and the vocabulary extended
(ignored, needs to be, applies, reopened, recurs, outstanding).
- The positive gate now accepts narration inflections (responds to,
replied to), a semantic line break after the link tail, and one
intervening parenthetical in the attribution filler (branches kept
disjoint on "(" so a failing scan stays linear).
- The negation guard is anchored adjacent to the past-state marker:
the bag-of-words form false-blocked "with no new issues, both
round-2 blocking findings ... are resolved".
- The backtracking regression test now probes
_is_resolved_blocking_mention directly: classify_verdict
short-circuits on the body's leading "Needs more work" and passed
even with the exponential bug restored (0.001s fixed vs 63.6s buggy).
Ten new regression tests; 360 total pass.
…ot a glue whitelist
Third adversarial round found the adjacency anchor from the previous
commit was too tight, and regressed in the dangerous direction: a count
or modifier between the negator and the past-state marker fell outside
the "of|the|all|these|those" whitelist, so
None of the two earlier blocking findings were resolved
read as unnegated and classified clean (measured not-clean on the
parent commit). "None of the identified earlier ..." and "Not one of
the prior ..." failed the same way.
Punctuation is the right discriminator rather than a word list: the
negator must quantify the same noun phrase the marker modifies, and a
comma is what ends its scope. The glue is now any run of up to four
bare words, which admits the counts and modifiers while still refusing
the fronted "with no new issues, both round-2 blocking findings ...
are resolved" clause the whitelist was introduced to fix.
The regression test now varies the words BETWEEN the negator and the
marker (five phrasings) plus a punctuated control, rather than
asserting two literal sentences -- the suite passed 360/360 while this
bug shipped. 366 total pass.
Fourth adversarial round: the bounded word run still failed open in
three ways, each classifying a live negated rejection as clean --
more than four intervening words ("None of the several very recently
identified earlier ..."), negators outside the alternation ("Zero of
the ...", "Hardly any of the ..."), and punctuation sitting INSIDE the
negated noun phrase rather than at a clause boundary ("None of the many
previously-identified, still-outstanding earlier ...", and the
parenthetical and dash-aside forms). All three are not-clean on
origin/main, so each was a fail-open this PR's own prefix widening had
introduced.
The premise was wrong rather than the bound: punctuation does not end a
negator's scope. What separates the two readings is the negator's
grammatical ROLE. So the search is now deliberately wide -- any negator
earlier in the same sentence defeats the exemption -- and the guard
instead tests whether that negator is governed by a preposition, which
makes it an adjunct ("WITH no new issues, ... are resolved") rather
than the subject quantifier that negates the resolution. The negator
list gains zero/hardly/barely/scarcely/nobody/nor. Anything this
misjudges over-flags, leaving a resolved mention blocking, which is the
safe direction.
The test matrix now varies the intervening material across twelve
phrasings, including all three escape shapes, and pairs a
preposition-governed control with its ungoverned twin. 374 pass.
Fifth adversarial round: keying the adjunct exemption on the word
before the negator tested lexical adjacency rather than what the
negator quantifies, so prepending any of the nine governor words to the
exact phrasing the guard exists to catch defeated it --
With none of the previously blocking findings were resolved
classified clean on this branch and not-clean on origin/main, for all
nine governors.
Both conditions are now required: the negator must be governed by a
preposition AND separated from the mention by a clause boundary. Each
alone admits the other's failure -- governed-only admits the case
above, and boundary-only admits "None of the many
previously-identified, still-outstanding earlier ...", whose comma sits
inside the negated noun phrase. Together they admit only the fronted
adjunct, whose negator both hangs off a preposition and quantifies a
different noun phrase.
Also teaches the shared sentence-boundary pattern about abbreviation
dots: "e.g." mid-sentence restarted the sentence and hid every negator
before it, which was a fail-open reached by a second route. Merging two
sentences only widens the scan, so this errs toward over-flagging.
Ten new tests, one per governor plus the abbreviation case; 384 pass.
Widening audit against origin/main over the parity corpus: 8112
widenings, all the intended citation shape, zero unintended.
…olved reading
Sixth adversarial round: requiring the negator be both governed and
detached still failed open, because the clause boundary can sit inside
the negated noun phrase --
With none of the recently reported, previously blocking findings
were resolved by this round's diff
classified clean, as did the semicolon, colon, and possessive variants.
That is the third lexical proxy for "what does this negator scope
over?" to admit a fresh false clean, so this stops proxying. Any
negator earlier in the same sentence now defeats the exemption, with no
judgment of what it quantifies. Deciding that is a parsing problem, and
the retreat is deliberate rather than a first guess -- the comment
records all three failed rules so the next reader does not re-derive
them.
The cost is one over-flag: a resolved narration whose sentence opens
with an unrelated negated clause ("With no new issues, both round-2
blocking findings ... are resolved") stays blocking. A false not-clean
stalls a merge until a human looks; a false clean merges over a live
rejection, which is this file's stated dangerous direction. A negator
AFTER the mention is unaffected, so the common trailing form this PR
exists to support still reads as resolved, as does a negator in a
preceding sentence.
Drops the now-unused governor and clause-boundary patterns, and makes
the abbreviation lookbehinds case-insensitive (lowercase "resp." was
not suppressed; suppression only ever widens the scan).
Tests: 45 governed shapes (nine governors x five boundary placements),
the trailing and preceding-sentence controls, and the fronted over-flag
recorded as intended. 420 pass.
…ator
Seventh adversarial round: the sentence scope is now the only thing
between a negator and the mention, so anything that fakes a sentence end
is a fail-open. An ellipsis and a URL or path dot both did --
None of the... previously blocking finding is resolved
None of the http://x.io/a. previously blocking finding is resolved
classified clean, because the scan restarted the sentence mid-clause and
never saw the negator. Adds an ellipsis lookbehind and a code-level
check that discards a candidate whose preceding token looks like a URL
or path, factored into _sentence_start_before and shared with the
citation-aside veto, which had the same exposure.
Two shapes remain where a negator sits inside an inline code span or a
quoted span: the scan text blanks those before the guard runs, and the
guard has no access to the citation mask. Measured clean on origin/main
too, along with the unobstructed baseline, so this is the residue where
the guard is merely no better than main rather than a regression. The
fix belongs to the blanking mechanism rather than to the guard, and is
tracked as ai-config#2689.
The guard as it now stands is a strict improvement over main on every
case measured: baseline, ellipsis, URL, path, abbreviation dot, and the
nine governor shapes each flip from clean to not-clean, and none flips
the other way. Four new tests; 424 pass.
Eighth adversarial round: _sentence_start_before found each candidate's preceding token by slicing and splitting the whole prefix, which is quadratic over a body with many sentences. A comment at GitHub's 65536 character cap took 7.2s, measured, and the scan runs once per comment body -- so a PR with several long review rounds compounds it. Both origin/main and the parent commit are effectively linear here, so this was a defect the previous commit introduced rather than one it inherited. The token is now found by scanning back to the nearest whitespace, which costs the token's own length: 0.88s on the same input, correctness unchanged across the ellipsis, URL, path, www, preceding sentence and plain cases. Adds the max-length body as a regression test, since a quadratic scan is invisible to a suite of short fixtures. 425 pass.
… token
Ninth adversarial round: locating a candidate's preceding token by the
nearest space, tab or newline yields an EMPTY token when the
punctuation is itself preceded by whitespace, and the empty string
passes the URL check --
None of the http://x.io/a . previously blocking finding is resolved
None of the /etc/passwd . previously blocking finding is resolved
each accepted the faked sentence end, narrowing the scan past the
negator and classifying clean. Correct on the parent commit, wrong
here, so a regression the previous commit introduced.
The scan now skips the whitespace run first and then reads the word,
and tests str.isspace rather than three named characters, so a carriage
return or a non-breaking space is handled on the same path instead of
by luck. Both loops are bounded by the run and the word rather than by
the prefix, and the max-length body drops from 0.88s to 0.030s, since
the previous rfind form scanned back to offset 0 whenever the character
was absent.
Four new tests, one per whitespace-separated shape; 429 pass.
…token
Tenth adversarial round: testing str.isspace for the token boundary let
a carriage return, vertical tab, form feed or non-breaking space INSIDE
a URL end the token early, leaving a bare word whose dot then read as a
real sentence end --
None of the http://x.io/a\rx. previously blocking finding is resolved
classified clean, and the same shape in the citation veto stripped a
live **Needs more work** citation into a body stating no verdict at
all. Correct on the parent and on origin/main, wrong here.
The break set is now space, tab and newline, which is the parent's set,
while the whitespace-run skip this commit's parent added is kept -- so
both routes are closed at once rather than trading one for the other.
Treating exotic whitespace as part of the token only ever makes the
token longer, so it can only add URL matches and skip more candidates,
which widens the caller's scan.
Five new tests. The four whitespace-separated interrupters added last
round were checked against the parent and three of them discriminate;
the exotic-whitespace forms here are what discriminate for this fix,
plus a citation-veto case, since that path had no faked-sentence-end
coverage at all. 434 pass, and the max-length body stays linear at
0.058s.
…e two scans
Eleventh adversarial round: narrowing the break set to space, tab and
newline fixed the exotic-whitespace-inside-a-URL shape and reopened the
content-free-token shape by the same stroke, because BOTH loops took
the narrower set. A real space followed by a non-breaking space left
the exotic character as the whole token --
None of the /etc/passwd \xa0. previously blocking finding is resolved
classified clean, and the citation-veto form stripped a live
**Needs more work** citation into a body stating no verdict at all.
The two loops do different jobs and now use different sets. The skip
consumes any str.isspace run, so a trailing non-breaking space -- an
ordinary copy-paste artifact rather than an adversarial input -- cannot
be left behind as the token. The extent stops only at space, tab or
newline, so exotic whitespace inside a URL cannot end the token early.
Fixing one shape and breaking the other happened twice, so the test is
now a sweep rather than an enumeration: three URL-like tokens, four
exotic whitespace characters, seven arrangements against the
punctuation, on both routes that share the scan -- 168 cases, all
required not-clean, reported with the count. 435 pass, and the
max-length body stays linear at 0.034s.
… the scan truly linear Twelfth adversarial round, two findings, both shared with the parent rather than introduced by it. A character that is invisible but not str.isspace -- a zero-width space, a joiner, a BOM -- was neither skipped nor a token break, so one sitting between a URL and the punctuation became the whole token. That is the content-free-token fail-open again, by a character class outside both loops' notion of whitespace, and it is the exact character a tool inserts into a long URL so it can soft-wrap. The skip now treats those as gaps. The scan was also still quadratic, and fixing the backward walk had not fixed it: with no ascii break anywhere the token extends to the start of the text, so the SLICE and its regex grew with each candidate. Measured 5.4s for 256,000 characters separated by non-breaking spaces. Both the break and the URL marker are now tracked by pointers that only move forward, so the question "does this token carry a marker" is answered by comparing two indices and the token is never materialized. Same input: 0.081s. The sweep grows from four whitespace characters to sixteen, including the invisible ones and Unicode separators, at 768 cases across both routes, and gains a linearity check that fails visibly rather than slowly. 436 pass.
…ver the whole invisible class Thirteenth adversarial round, one finding plus the follow-up it named. str.startswith runs against the whole text rather than the token, so a token ending in "www" glued to the sentence's own period matched on that period and the real sentence end was discarded as a URL. The parity instrument reports the end-to-end body as a NARROWING against the parent, and a legitimately clean review comment read not-clean. It fails in the safe direction, and it is still a divergence from the regex this scan replaced, which is the invariant the previous commit claimed. The match is now bounded by token_end. _ZERO_WIDTH listed five characters, so the next invisible one escaped it -- a word joiner and a soft hyphen were both still admitted as token content, identically on the parent and on origin/main. The test is now the Unicode format category, which covers the class rather than an enumeration. The sweep grows to twenty gap characters and 960 cases, and gains the www-boundary pair. 438 pass, and a 500,000-character body with no ascii break stays linear at 0.15s.
…raph boundary scan
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
|
Claude finished review — View run Review: #2668 —
|
Contributor
dem-extra1
deleted the
fix/check-pr-fully-clean-posted-verdict-citation
branch
August 31, 2026 04:09
This was referenced Aug 31, 2026
Collaborator
|
Three review rounds landed on this PR after it merged (runs 33353932842, 33354703191, 33355156462), each returning Needs more work against an intermediate commit. Recording their disposition, since a merged PR carrying three open verdicts reads badly otherwise. All three are resolved in the merged state. Each found a fail-open in the veto's paragraph-window bounds --- a And all six were already pinned as regression tests on So the three verdicts are stale rather than open: real when written, superseded by a later commit in the same PR, and already covered. Nothing here needs reopening. The follow-up for what the squash did drop is #2726 --- six comment and dead-code items whose corrections did not survive, none of them behavioural. Posted by Claude Code (AI agent) --- not written by a human. |
d-morrison
added a commit
that referenced
this pull request
Aug 31, 2026
…from #2668 (#2726) Closes #2722 PR #2668 merged as 5dfcb6e. Six comment or dead-code items from its later review rounds, and six regression tests, did not survive the squash. All are still absent from main. The comments are the substance. Each describes a mechanism the code no longer has, and the assertions around them stay green because the current mechanism is a superset of the retired one -- so only a reader notices, and a reader is exactly who consults them before changing the thing. The block above _RERAISE_VOCAB said the veto scans "the containing sentence BACKWARD and its containing paragraph FORWARD" when it scans the whole containing section in both directions; the block above _SENTENCE_END_RE said it is shared with the citation veto, which stopped using it when that veto moved to bisecting heading positions; and a third still named _paragraph_starts and _paragraph_ends, deleted before #2668 merged. Each now says what the code does and records why the narrower bound was abandoned. Also removes _SENTENCE_END, an alias with no readers sitting three lines above the section-scan code, and the "| _orphan_markers" union, which is empty by construction under swallow_unclosed=True and so never did anything while implying orphan markers matter there. The restored tests cover the blank-line-gap sweep across widths two through eight on both sides, a "#" comment inside closed, unclosed, tilde and indented fences, and the real-heading control. They pass against merged main unchanged, which confirms the behaviours survived the squash even though their coverage did not. The indented case keeps its "#" at column 0, because the heading regex is anchored there and an indented "#" would make the case pass with fence-awareness entirely broken -- what it tests is that an indented FENCE still counts as one. 472 pass, up from 466.
This was referenced Aug 31, 2026
d-morrison
added a commit
that referenced
this pull request
Aug 31, 2026
Five siblings, not six (verified family size on main). Name #2668 as the PR whose review rounds reported the coverage gap. Drop backticks from a phrase that is review shorthand, not literal tool output. Separate the case's Do/Don't bullets from the section's standing list.
d-morrison
added a commit
that referenced
this pull request
Aug 31, 2026
… racing its own reconciliation (#2731) Proactive UMS pass banking learnings from #2668 (merged as squash 5dfcb6e) that PR #2698's earlier pass did not cover. - shared/coding/fact-check-code-logic.md: new section on a comment describing HOW a mechanism works surviving the mechanism, invisible to every test when the replacement is a superset. Three confirmed instances from #2668, and the sweep-method lesson (enumerate mechanisms, don't grep the current round's vocabulary). Also adds a third pass-count hazard (a duplicated, unregistered test block inflating the count), referencing the already-filed #2725. - shared/workflow/claim-pr.md: a "Third occurrence" addendum to the existing concurrent-session-collision section, recording that a posted claim comment did not stop a second driver, and that this session's own git-level reconciliation of the two designs was itself orphaned when the PR merged six seconds before that reconciliation commit was authored. - shared/workflow/check-before-pushing.md: one-line reinforcement citing the above, without restating the existing rule. Verification: python3 scripts/semantic-line-breaks.py (0 would change), grep for non-ASCII punctuation (0 hits), python3 scripts/check-links.py (0 broken), python3 scripts/check-context-closure.py (0 fragments over cap). An adversarial-reviewer subagent pass found two factual errors in an earlier draft (a described git-merge resolution that never actually shipped -- the PR's real squash used the bot's commit directly, six seconds before this session's reconciliation commit was authored; and an overreaching claim that in-progress fixes were "dropped by the squash" when two of the three stale comments were never attempted before a later retrospective sweep). Both are corrected in this diff, verified independently against git history and the GitHub API before and after the fix. Note on scope: this pass covers the three learnings named in the task (stale mechanism comments, concurrent-driver collision, and a pointer to the already-filed #2725 duplicate-test-block issue). It has not been re-reviewed by a second adversarial pass after the accuracy fixes above -- only independently re-verified by direct git/gh queries. The session is wrapping up under a quota constraint, so that second review round was not run; a fresh pair of eyes on this PR would be worthwhile before merging if anything reads as still under-supported.
d-morrison
added a commit
that referenced
this pull request
Aug 31, 2026
…nother (#2730) * ums: coverage claim about one instrument does not license one about another 2026-08-30, while closing out #2722 in #2726 (the follow-up to #2668): three review rounds correctly reported that check-verdict-scan-parity.py's fuzz corpus does not cross its vocabulary axes with fenced-code content. That true finding about one instrument was read as settling coverage for a second, different instrument (the test suite), which had never itself been checked -- and six regression tests were drafted duplicating tests main already carried under different variable names. An adversarial-reviewer subagent caught it before push. Banked in shared/workflow/metacognitive-monitoring.md's "A sound measurement does not license the claim standing next to it" section, as the general form the task identified (metacognitive-monitoring.md's scope-claim rule, close cousin of verify-the-right-artifact.md). Dupe-checked: no existing entry names this instrument pair or PR. A parallel in-flight UMS pass (worktree ums-pr2668-lessons, uncommitted) covers three separate #2668/#2726 lessons -- a stale mechanism comment, two agents racing one PR, and #2725's pass-count double-counting -- none of which is this cross-instrument-coverage inference error. Ran scripts/check-links.py (clean) and scripts/semantic-line-breaks.py --write (applied). Co-Authored-By: Claude Opus 5 <[email protected]> * ums: address adversarial-review findings and add boundary cross-ref Five siblings, not six (verified family size on main). Name #2668 as the PR whose review rounds reported the coverage gap. Drop backticks from a phrase that is review shorthand, not literal tool output. Separate the case's Do/Don't bullets from the section's standing list. --------- Co-authored-by: Claude Opus 5 <[email protected]>
d-morrison
added a commit
that referenced
this pull request
Aug 31, 2026
Second adversarial review of the fixes found six more, three of them
letting a not-clean artifact score clean.
- pre-push-review.py: the rewritten fingerprint anchor still backtracked
exponentially. Removing the empty `\s*` alternative was necessary and
not sufficient: `={3,}` is self-ambiguous under the outer `*`, since a
run of `=` splits into chunks of size >= 3 exponentially many ways.
Measured on this tool's OWN `"=" * 60` report banner followed by any
non-matching text: 0.50s at 36, 4.01s at 42, 14.18s at 45. Replaced the
nested-quantifier regex with a line scan, which is linear by
construction and cannot reacquire the class from a future alternative
(0.36ms at 4000 `=`).
- review_payload.py: `find_fence_spans` defaults to
`swallow_unclosed=False`, which records only an unclosed fence's OPENER
line and leaves its interior live -- so the mask failed in both
directions. A truncated review quoting a CLEAN template scored clean,
and a quoted NOT_CLEAN payload minted an undischargeable finding.
check-pr-fully-clean.py already passes `swallow_unclosed=True` for this
reason; now so does this.
- check-pr-fully-clean.py: a payload's self-declared `reviewer` no longer
confers identity. It is body text the reviewer writes about itself, so
trusting it inverts #2308's invariant -- and it is load-bearing for
quorum: measured, two clean comments from the single `github-actions[bot]`
login whose payloads named different reviewers satisfied `--quorum 2`,
which shared/workflow/fully-clean.md makes the normal invocation.
- Drop the `review-json` payload alias. Nothing in the corpus emitted or
documented it, so it was a second verdict-bearing input spelling with no
producer.
- Drop the dead `(oid and oid.lower() == sha_lower)` term from
`is_sha_match`. It is read only where `oid` is falsy, so it can never
contribute -- while reading as having made formal-review OID matching
case-insensitive, which `is_match = (oid == sha)` is not.
- check-verdict-scan-parity.py: the corpus carried no `review-data`
fragment, so its zero for this change was a coverage statement -- the
same failure its own VOCAB comment records for #2668. Added a bounded
payload arm crossing placement (bare, fenced, unclosed fence, code span,
indented), which payload is authoritative, and verdict/findings
agreement. Yielded FIRST, deliberately: `--limit N` truncates the
generator and the full product is ~242k bodies, so an arm yielded last
sat at index 241,920 and was unreachable at any limit run by hand.
Verified: 516 + 50 + 12 tests pass; parity over 600 bodies (all 32 payload
bodies among them) reports 0 widened, 0 narrowed, negative control 157
divergences -> DISCRIMINATES; links, memory-file-size, skills, test-suite
coverage, reviewer-placeholder and hook-catalog checks green;
semantic-line-breaks clean across all four changed markdown files.
d-morrison
added a commit
that referenced
this pull request
Aug 31, 2026
…clean (#2736) * ums: capture GIA learnings — heredoc hook, chapter wiring, empty promises - Security hook: don't heuristic-strip heredoc bodies; keep full scan - Quarto new chapter: format block, navbar, index, WORDLIST + lychee - Inline R: formatC with format="d" to avoid scientific notation - No empty promises: don't use 'will' for future work without mechanism; daytb covers GIA checkpoint * fix(plugin): use stable absolute path instead of extensionPath in hooks.json * fix(ums): address review findings (daytb claim, em-dashes, sembr) * fix(ums): apply sembr and do/dont formatting, use ascii dash * fix(ums): fix antigravity.md headings and verification * fix(ums): move quarto learnings to r-quarto.md to avoid max lines limit * memorize: check external repo guidelines and PR template before filing Do: fetch and follow external repo's CONTRIBUTING.md and .github/pull_request_template.md before opening PR Don't: file with internal/generic template without checking required sections Fixes compliance failure on anomalyco/opencode#46226 where PR body missed required template sections (needs:compliance). * fix(hooks): block unsupported CI forecasts * style: use standard markdown em-dashes in AGENTS.md * style(ums): semantic line breaks for GIA additions * feat(review): support structured review-bot output in check-pr-fully-clean * fix(check-pr-fully-clean): import Any and Set and add future annotations * fix(review): address adversarial review findings on prompt builder and SHA matching * fix(review): gate structured reviewer identity on bot login or agent marker * fix(pre-push-review): tolerate agent disclosure footer after fingerprint * fix(pre-push-review): increase Claude CLI timeout to 600s * fix(pre-push-review): bound Observations section regex to next heading * fix(review): drop fence from reviewer prompt review-data and restore docstrings * fix(review): address PR review findings on gha claim, trailing guard, and SHA matching * fix(review): ensure prose findings override structured clean and preserve agent marker precedence * fix(pre-push-review): allow trailing status banner after fingerprint * fix(pre-push-review): anchor trailing verdict alternative and add unit test * fix(review): preserve heading and fingerprint requirements for structured review body admission * fix(pre-push-review): allow heading suffixes in section extractor * fix(pre-push-review): accept 1-6 hashes in extract_section and clarify Verification Steps in prompt * fix(review): close structured-payload fail-opens found in post-merge review The post-merge adversarial review of the merged branch returned twelve findings, five of them letting a not-clean artifact score clean. check-pr-fully-clean.py: - Drop the two Claude Code disclosure-footer entries from REVIEW_AGENT_MARKERS. CLAUDE.md mandates that footer on EVERY agent-posted comment -- claims, status updates, replies -- not on reviews only, so admitting it made each of those a quorum-eligible automated review with identity "Claude". Measured: an owner comment ending in the footer superseded a real bot "Needs more work", and satisfied quorum on a PR carrying no automated review at all. That reopened #2308's invariant (approval authority comes from author identity, never body text) and, via is_non_review_notice, the #1719 skip-notice admission path. - The LAST valid payload now wins, not the first. The persona template a reviewer may quote hardcodes "verdict": "CLEAN" and the authoritative payload comes last, so first-match-wins let a NOT_CLEAN review score clean. - Code spans and indented code blocks are masked alongside fences, so a comment that merely mentions the format is no longer read as a review. - Remove the <details>-plus-JSON-fence path. It spanned arbitrary distance and skipped the fence mask, so a collapsed earlier payload minted a blocking finding no ARD round could discharge (#2482 class). pre-push-review.py: - Remove the empty-matching \s* alternative from the fingerprint anchor's * quantifier. It duplicated the group's own leading \s* and backtracked catastrophically: measured 1.26s at 12 whitespace characters and 21.4s at 14, ~3x per added character, with no timeout around parse_review_verdict. Now linear (0.48ms at 2000). - Validate the payload the built prompt asks for. HTML comments were stripped before every check, so a report whose payload said NOT_CLEAN parsed as "Verdict: CLEAN" locally while check-pr-fully-clean.py scored the same artifact blocking. Both consumers now read the payload through one extractor, scripts/lib/review_payload.py, so they cannot drift apart again. Findings block regardless of the stated verdict; prose findings still override a structured clean. Also: state the finding-object schema (file/line/category/message) in both persona files and in the built prompt rather than asking for unspecified "finding objects"; accept a finding's "summary" key as a fallback, since the persona's own ReportFindings contract uses it; restore the emit-nothing-after constraint to the prompt; and move the structured-data section in adversarial-self-review.md out of the middle of the preceding section, whose #2444 provenance parenthetical it had orphaned. Verified: 510 + 49 tests pass; check-verdict-scan-parity.py over 400 bodies reports 0 widened, 0 narrowed, with its negative control discriminating (103 divergences); links, memory-file-size, skills, test-suite coverage, reviewer-placeholder and hook-catalog checks green; semantic-line-breaks clean on every changed markdown file. * fix(pre-push-review): clean heading and section body separation in extract_section * test(review_payload): add dedicated unit tests and wire into validate.yml * fix(pre-push-review): support optional bold and hyphen/space in fingerprint regex * fix(review): close round-2 fail-opens in the structured-payload path Second adversarial review of the fixes found six more, three of them letting a not-clean artifact score clean. - pre-push-review.py: the rewritten fingerprint anchor still backtracked exponentially. Removing the empty `\s*` alternative was necessary and not sufficient: `={3,}` is self-ambiguous under the outer `*`, since a run of `=` splits into chunks of size >= 3 exponentially many ways. Measured on this tool's OWN `"=" * 60` report banner followed by any non-matching text: 0.50s at 36, 4.01s at 42, 14.18s at 45. Replaced the nested-quantifier regex with a line scan, which is linear by construction and cannot reacquire the class from a future alternative (0.36ms at 4000 `=`). - review_payload.py: `find_fence_spans` defaults to `swallow_unclosed=False`, which records only an unclosed fence's OPENER line and leaves its interior live -- so the mask failed in both directions. A truncated review quoting a CLEAN template scored clean, and a quoted NOT_CLEAN payload minted an undischargeable finding. check-pr-fully-clean.py already passes `swallow_unclosed=True` for this reason; now so does this. - check-pr-fully-clean.py: a payload's self-declared `reviewer` no longer confers identity. It is body text the reviewer writes about itself, so trusting it inverts #2308's invariant -- and it is load-bearing for quorum: measured, two clean comments from the single `github-actions[bot]` login whose payloads named different reviewers satisfied `--quorum 2`, which shared/workflow/fully-clean.md makes the normal invocation. - Drop the `review-json` payload alias. Nothing in the corpus emitted or documented it, so it was a second verdict-bearing input spelling with no producer. - Drop the dead `(oid and oid.lower() == sha_lower)` term from `is_sha_match`. It is read only where `oid` is falsy, so it can never contribute -- while reading as having made formal-review OID matching case-insensitive, which `is_match = (oid == sha)` is not. - check-verdict-scan-parity.py: the corpus carried no `review-data` fragment, so its zero for this change was a coverage statement -- the same failure its own VOCAB comment records for #2668. Added a bounded payload arm crossing placement (bare, fenced, unclosed fence, code span, indented), which payload is authoritative, and verdict/findings agreement. Yielded FIRST, deliberately: `--limit N` truncates the generator and the full product is ~242k bodies, so an arm yielded last sat at index 241,920 and was unreachable at any limit run by hand. Verified: 516 + 50 + 12 tests pass; parity over 600 bodies (all 32 payload bodies among them) reports 0 widened, 0 narrowed, negative control 157 divergences -> DISCRIMINATES; links, memory-file-size, skills, test-suite coverage, reviewer-placeholder and hook-catalog checks green; semantic-line-breaks clean across all four changed markdown files. * fix(pre-push-review): one fingerprint pattern for both checks `parse_review_verdict` matched the `Reviewed-Commit:` line with two separate literals -- once to harvest the SHA, once to scan for content after it -- and they drifted apart within a single session. 9230833 loosened the harvest to accept `**Reviewed-Commit**` and `Reviewed Commit`; 6fe7d21 had independently replaced the second site with a line scan (the regex it replaced backtracked exponentially on this tool's own `"=" * 60` banner). A report in the loosened form then passed the harvest and matched nothing in the scan, so `last_fp` stayed None and `last_fp.end()` raised. Hoist both onto one `_FINGERPRINT_RE` carrying the loosened form, add a fail-closed guard on the now-unreachable None, and cover all five spellings -- plain, bold, space, bold-and-space, padded colon -- against both checks, each also asserting that trailing chatter is still rejected rather than silently unlocatable. Verified: 516 + 51 + review_payload + parity suites pass. * fix(pre-push-review): use extracted section variables in critical findings validation * fix(pre-push-review): allow heading variations in section extraction while preserving persona delegation * fix(review): close round-3 findings in the structured-payload path Third adversarial review found seven more, two of them fail-opens. - pre-push-review.py: a verdict restated AFTER the fingerprint was tolerated in position and then read by nothing -- `verdict_matches` scans the Summary section only -- so a report ending `### Verdict: Needs more work` parsed as (True, True, 'Verdict: CLEAN') where the pre-line-scan regex had rejected it outright. Trailing verdict lines now go through the same evaluation the Summary verdict does, so a not-clean restatement blocks and an unrecognized one invalidates. - review_payload.py: a `findings` value that is PRESENT but not a list was folded to `[]`, letting a type deviation clear a payload the way an empty array does. Measured end to end: a bot comment whose payload read `"findings": "3 defects listed above"` satisfied quorum and the gate reported fully clean. `payload_findings_malformed` now blocks it. - pre-push-review.py: `_FINGERPRINT_RE` was `^`-anchored and so rejected the three-space-indented `Reviewed-Commit:` line this file's own `build_review_prompt` renders. Re-derived from `hooks/no-push-without-self-review.py`'s REVIEWED_COMMIT -- the pattern the persona file names as authoritative -- so bold-after-colon and a backticked sha are accepted too, plus leading indentation. - check-verdict-scan-parity.py: every payload body carried a prose `## Verdict:` line, which decides before control reaches `payload_is_clean` -- the only new acceptance-widening branch. Reached 0 of 32 times, so the arm's zero was still a coverage statement. Added bodies with no prose verdict, plus malformed and findings-absent payloads; `payload_is_clean` is now reached 22 of 57 and the arm produces three distinct outcome pairs. - test_check_pr_fully_clean.py: the two short-prefix SHA checks re-implemented `startswith` and measured a string length in the test body rather than calling `check_review_comments`. Replaced with three that call it, with the full sha ABSENT from the comment so only the structured prefix can match. Mutation-checked: deleting `len(struct_sha) >= 7` now fails the suite (it did not before). - Corrected a comment attributing the `"=" * 60` banner to `log_error`, which prints only `Error: {msg}`; it is `main`'s report separator. - test_review_payload.py: three imports were unused. Added direct tests for `code_region_mask` (per-region attribution plus offset arithmetic on CRLF, tabs, multi-byte and a missing trailing newline) and for the two verdict vocabularies being disjoint and pre-normalized. Verified: 517 + 52 + 14 + 12 tests pass; both new regression tests mutation-checked to fail against the pre-fix behaviour; parity over 700 bodies reports 0 widened, 0 narrowed, control 189 divergences -> DISCRIMINATES; links, memory-file-size, skills, test-suite coverage, reviewer-placeholder and hook-catalog checks green; semantic-line-breaks clean across all four changed markdown files. * fix(review): close round-4 findings in the structured-payload path Fourth adversarial review found nine more. Two were fail-opens, one was a documented invariant that is false where it matters most. - review_payload.py: the payload must now BEGIN A LINE. Masking code regions was not enough on its own -- a payload written mid-sentence in ordinary prose ("Reviewers must end with <!-- review-data: ... -->") or behind a `> ` blockquote marker, which is what GitHub's Quote reply emits, sits in no code region at all. Measured both directions: a mention of the format scored the comment CLEAN, and a narrated earlier round minted a structured finding no push could discharge. - review_payload.py: `payload_is_clean` now requires `findings` to be present, a list, and empty. Both persona files and the built prompt say a CLEAN payload requires an empty array and nothing enforced it, so a payload that simply omitted the key cleared -- inconsistent with the blocking treatment a wrong-typed value already got, and an omitted required key is the commoner model failure of the two. - review_payload.py: dropped `except Exception: fenced_lines = set()` around the fence scan. It under-masked -- the direction the same docstring calls unsafe -- silently, on any failure, disabling fence masking for the whole body. The sibling call in check-pr-fully-clean.py is unwrapped for the same reason (fail-fast.md). - pre-push-review.py: a malformed `findings` field was reported as "the payload reports verdict CLEAN", which is not a contradiction and is not why it blocked. The reason now names the actual cause and its type. - check-pr-fully-clean.py: removed the structured `commit_sha` term from `is_sha_match`. It was provably unable to change the result -- `sha_short` is the target's own 7-character prefix, so any payload `commit_sha` long enough to be accepted contains it, and the body-substring disjunct already matched. Mutation confirmed: deleting the term left every structured-sha test passing. A dead disjunct that reads as a feature is worse than no feature, since the tests appear to cover it. - pre-push-review.py: `_FINGERPRINT_RE` claimed to accept every form the pre-push hook's REVIEWED_COMMIT accepts. It does not -- the `^` anchor rejects inline, blockquoted and list-item fingerprints the hook matches. The anchor is deliberate (the match position decides where trailing content begins), so the comment now states the two narrowings and two widenings instead of claiming parity, and the `{7,40}` length bound the hook applies is restored. - pre-push-review.py: moved the "lines tolerated after the fingerprint" comment back above the constant it describes. - adversarial-self-review.md and both persona files: the documented "findings block regardless of the stated verdict" invariant is FALSE for hooks/no-push-without-self-review.py, which blanks HTML comments before parsing -- the guard that actually blocks the push. Measured: a clean prose verdict with a NOT_CLEAN payload parses there as clean. Corrected the docs to say so and to tell reviewers to state the prose verdict truthfully. Teaching the hook to read the payload is deliberately NOT done here, since this branch's own pushes are gated by that hook; tracked as ai-config#2749. - Covered the two behaviours that survived mutation with every suite green (the persona-path `_structured_contradiction` guard, and the `review-data:` REVIEW_BODY_MARKERS entry), and added direct tests for `code_region_mask`. Verified: 520 + 52 + 17 + 12 tests pass; all six round-4 fixes mutation-checked to fail the suite when reverted; parity over 700 bodies reports 0 widened, 0 narrowed, control 189 divergences -> DISCRIMINATES; every checker in validate.yml green; semantic-line-breaks clean across all four changed markdown files; no non-ASCII in added lines. Refs #2749 * fix(review): close round-5 findings; payload prefix must be whitespace, not column zero Fifth adversarial review found seven. One was a regression the previous round introduced. - review_payload.py: the column-zero rule was worse than the hole it closed. `build_review_prompt` and both persona files render the payload three spaces in, so a reviewer following the prompt's own layout had its payload silently dropped -- a NOT_CLEAN payload ignored and the guard reporting clean, the exact failure this path exists to prevent. One leading space was enough. The rule is now that only WHITESPACE may precede the payload on its line, which still rejects a `> ` blockquote (GitHub's Quote reply), a `- ` list item, and a mid-sentence mention. Three spaces stays readable while four does not, and that boundary is CommonMark's rather than an arbitrary one: four spaces is an indented code block, which `code_region_mask` masks. The prompt and both personas now render the payload FLUSH LEFT and say so explicitly, so a four-space payload is a deviation from a stated instruction reading as the code block it is. This also makes the mask's indented-block test live again (round 4's review had it dead), mutation-confirmed. - check-pr-fully-clean.py: RESTORED the structured `commit_sha` term that round 4 deleted as "provably" inert. The proof was wrong: `json.loads` resolves escapes, so a payload carrying `"commit_sha": "abc1234..."` decodes to a match while the literal body text reads `abc12...`, which neither substring disjunct sees. Deleting it made that case fail closed -- a real review stops counting as evaluating HEAD, reported as "no review posted". The counterexample is now a test, and the comment states what is actually true rather than a proof that does not hold. - check-pr-fully-clean.py: `_unresolved_finding_pattern` now checks for a malformed `findings` field BEFORE the verdict branch, for the reason its sibling in pre-push-review.py already states -- otherwise it printed "structured blocking verdict (CLEAN)", naming the payload's own CLEAN string as the reason it blocked. - check-verdict-scan-parity.py: the payload arm is now appended AFTER `--limit` is applied. Yielding it last put the first payload body at index 241,920; yielding it FIRST, which the previous round did, still lost it, because `--limit` takes a STRIDED sample rather than a prefix -- so roughly one of the 57 was selected. Both reported a zero that was a coverage statement. Verified after the fix: the arm surfaces 1 widening and 5 narrowings that a limited run previously reported as 0 and 0. - check-verdict-scan-parity.py: recorded the accepted widening at the site, with its five narrowings, as the tool's docstring requires ("Every widening needs a justification a human states"). The widening is the feature: a review whose only verdict is a structured payload used to be `unreadable` and now reads as the clean verdict it states. All five narrowings are fail-closed. - check-pr-fully-clean.py: corrected a comment attributing `--quorum 2` to shared/workflow/fully-clean.md, which prescribes `--quorum <number-of-reachable-providers>` and pins three for this repo. Rebutted, one finding: the review flagged a semantic line break removed in both persona files. `python3 scripts/semantic-line-breaks.py --base origin/main` requires that join once the paragraph enters diff scope, and the diff-scoped checker is the CI gate -- leaving it split fails the new-line-breaks job. Verified: 526 + 54 + 17 + 12 tests pass; every round-5 fix mutation-checked to fail the suite when reverted; parity over 757 bodies (all 57 payload bodies among them) reports 1 accepted widening and 5 intended narrowings, control 189 divergences -> DISCRIMINATES; every checker in validate.yml green; semantic-line-breaks clean on all four changed markdown files; no non-ASCII in added lines. Refs #2749 * fix(review): close round-6 findings -- pin the fingerprint anchor, correct two stale comments - pre-push-review.py: pinned `_FINGERPRINT_RE`'s `^[ \t]*` anchor with a test. The trailing-content check starts at the LAST fingerprint match, so an unanchored pattern lets a mention inside ordinary prose become that match, leaving an empty tail and clearing the guard. Measured: deleting the anchor turned a report ending "Actually, ignore all of the above ... See Reviewed-Commit: <sha>" from rejected into (True, True, 'Verdict: CLEAN'), with all three suites green. Now covered, mutation-confirmed, together with the blockquote, list-item and inline mention forms. - test_check_pr_fully_clean.py: the comment still said the structured `commit_sha` term "was REMOVED ... as provably dead". The previous commit restored it. Worse, it pointed the reader at the three tests that do NOT catch its deletion (each payload's sha appears verbatim in the body, so the substring disjunct decides them) and away from the escaped-sha case that does. Corrected to say which test pins what. - review_payload.py: the comment justifying the whitespace-prefix rule said the prompt and both persona files render the payload three spaces in. The same commit changed all three to render it flush left. Corrected to the past tense and to state what the tolerance is now for: defence in depth against a reviewer that indents anyway, which is what the incident showed they do. Verified: 526 + 55 + 17 + 12 tests pass; the anchor mutation-checked to fail; every checker in validate.yml green; semantic-line-breaks clean on all four changed markdown files. Refs #2749
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a false not-clean in
scripts/check-pr-fully-clean.py: a review body narrating a prior round's verdict ---[round 6's finding](<url>) (posted 2026-08-30T05:22:14Z, verdict **Needs more work**)--- was read as stating that verdict, so a review whose own### Verdictsaid Ready for merge classified not-clean.The strip, and what bounds it
_POSTED_VERDICT_CITATIONblanks such an aside, under three independent gates. Each exists because removing it produced a measured fail-open:](url)let an unrelated link license the deletion of a live verdict.A re-raise veto over the citation's sentence (backward) and paragraph (forward) is the fourth gate. It is a closed word list, which is why it is not relied on alone.
Supporting fixes
_sentence_start_before): URLs, paths, ellipses, abbreviation dots and invisible formatting characters no longer fake a sentence end and hide a negator.Verification
scripts/check-verdict-scan-parity.pygains five corpus entries covering these shapes. Its zero for them beforehand was a statement about the corpus rather than about the change.Not fixed here
#2688, #2689, #2696 and #2702 --- each filed with its measurement. #2696 is asserted in the suite alongside the control showing
mainbehaves identically once the citation is removed.