Repository navigation
feat(review): support structured review-bot output in check-pr-fully-clean - #2736
Merged
Merged
Conversation
…ises - 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
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).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Collaborator
Author
Addressed Findings for Commit
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`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.
…while preserving persona delegation
Collaborator
Author
Adversarial Self-Review Resolution & Rebase Update
Posted by Antigravity (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
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
…e, 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
…rrect 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
Collaborator
Author
|
Pushed six rounds of adversarial self-review fixes on top of a Five fail-opens are closed. In each case the checker reported a not-clean artifact as clean:
Two ReDoS fixes in
Deferred and tracked: Verified locally: 526 + 55 + 17 + 12 tests pass; every fix mutation-checked to fail the suite when reverted; every checker in Seven adversarial-reviewer rounds; the seventh returned Ready for merge on Posted by Claude Code (AI agent) --- not written by a human. |
This was referenced Aug 31, 2026
Contributor
|
Claude finished review — View run SummaryThis round ( The new commits fix real, subtly interacting bugs in
Independent verification performed (not just trusting the commit messages):
FindingsNo new findings. This round's changes are correct, narrowly scoped, well-tested (each fix ships a regression test, several with explicit mutation-confirmation notes in the commit message), and every checkable claim in the commit messages verified against the actual code and tooling. VerdictReady for merge — this round's four new commits correctly fix real edge cases in the structured-payload verdict path, are fully covered by passing tests, and I independently reproduced the key claims (parity tool output, restored/removed disjunct behavior, referenced issue). No new content defects found. Reviewed commit: c725c44 |
Contributor
Collaborator
Author
|
Intent to merge.
So I am applying the peer-PR discipline: the clean verdict landed at 12:29:47Z, and I will merge no earlier than 12:49:47Z (05:49 PDT) unless someone holds it off before then. If you are the session that pushed those commits, reply here or message me and I will hold. For the record, that interleaving already caused one real defect: Posted by Claude Code (AI agent) --- not written by a human. |
d-morrison
added a commit
that referenced
this pull request
Aug 31, 2026
* ums: bank PR #2736 review lifecycle learnings (Closes #2750) * fix(review): address adversarial self-review findings on SemBr, rounds, and attribution * fix(review): refine round-2 findings on headings, attribution, and scope * docs(coding): catalog regex-backtracking-pitfalls in CLAUDE.md and principles README * docs(mistake-patterns): cite tracking issue #2768 in Pattern 31 * docs(mistake-patterns): cite tracking issue #2769 in Pattern 32
d-morrison
added a commit
that referenced
this pull request
Sep 1, 2026
…ngs (#2939) Five learnings from the 2026-09-01 GIA session, each recorded where the next reader of that situation will look: - memories/hooks.md: a README hook row's matcher list must reproduce the hooks.json group order verbatim, because check-hook-catalog.py joins the groups in file order and compares the string (ai-config#2930 CI). - memories/reviewing-prs.md: search an issue's comments, not only its body, before rebutting a "no source exists" finding -- the permalink a reviewer asked for on wai#184 sat in an issue comment, and the rebuttal was a false state claim. - shared/workflow/fully-clean.cases.md: a CLEAN payload whose prose names a prior finding reads as not clean, by design (prose wins, #2736); the driver-side remedy is a fresh cleanly-worded round, not hand-reading. - CLAUDE.md: the doubled-backslash collapse also did not reproduce in a Linux remote Claude Code container, measured with the rule's own reproducer, so the rule's environment scoping now carries two negatives. - skills/ums/SKILL.md: run the memory-size, line-break, link, and markdownlint gates before committing a UMS change, since #2909 failed CI on a memory file crossing the 1250-line budget. Claude-Session: https://claude.ai/code/session_01GMS285msTfSxnF4Cy9RkTE Co-authored-by: Claude <[email protected]>
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.
Summary
Add backward-compatible support for structured JSON review data (
<!-- review-data: { ... } -->or<details>blocks) across review validation and generation tools.Details
scripts/check-pr-fully-clean.py:extract_structured_review(body)to parse machine-readable review JSON comments without fragile regex heuristics.classify_verdict(),_reviewer_identity(),_unresolved_finding_pattern(),_is_structured_review_body(), andcheck_review_comments()SHA matching.Reviewer Persona Prompts & Generators:
.claude/agents/adversarial-reviewer.md,.opencode/agents/adversarial-reviewer.md, andscripts/pre-push-review.pyto append the machine-readable<!-- review-data: ... -->block followingReviewed-Commit: <sha>.shared/workflow/adversarial-self-review.mdandskills/claude-review-workflow/SKILL.mddocumentation.Validation:
scripts/test_check_pr_fully_clean.pywith 25 new tests covering clean/not-clean verdicts, findings overrides, details block format, code-fence isolation, spoofing defenses, and SHA matching (360 passed, 0 failed).Closes #2663