Skip to content

feat(review): revert-pair lister, case builder and caught missed report for code-review evals - #13031

Merged
melodic-automation-lanes[bot] merged 2 commits into
mainfrom
work-resolve-issue/issue-12939-41743071136-1
Nov 4, 2026
Merged

melodic-automation-lanes[bot] merged 2 commits into
mainfrom
work-resolve-issue/issue-12939-41743071136-1

Conversation

@melodic-automation-lanes

Copy link
Copy Markdown
Contributor

Refs: #12939

Summary

Opened by the work-resolve-issue lane. The run's own summary:

Adds plugins/review/evals/revert-cases.sh, the tooling for a revert eval set on /review:code-review. list prints each revert pair in git history, found by a revert: or revert(<scope>): subject or a This reverts commit <sha> body line and resolved to a merged PR, then the pair count. It skips a pair whose original names no PR, whose revert removes no line the original added, or whose original no longer reverses cleanly onto the revert's parent. build <original> <revert> <slug> writes a revert-<pr>-<slug> case like the recall cases: fixtures at the base and merge, a case.yaml recording the PR, the revert, the lines and a pending confirmed-by line, a /review:code-review prompt, and a regex grader on the lines the PR added and the revert removed. The case's scaffold refuses to run until a verifier agent replaces pending. report <aggregate-result.json> prints caught or missed per revert- case and a total, and marks the total indicative below 5 comparable cases. Design choices and their basis: one script with subcommands, so the generated scaffold.sh is a one-line call to it (judgment). Reverted lines are matched by their text, because line numbers shift between the merge and the revert's parent; the verifier step drops false matches (judgment). The report is in node because the review plugin already requires node for its hook (plugins/review/scripts/check-hook-registration.sh). The minimum of 5 cases is judgment. This PR builds no revert- case. This lane's checkout is a one-commit shallow clone, and WebFetch returns only summaries of GitHub diffs, never the exact file contents a fixture needs. Building the cases is left in remaining.

Remaining work

  • Build the revert- cases: run bash plugins/review/evals/revert-cases.sh list on a clone with full history, then build <original> <revert> <slug> for each listed pair (this lane's checkout is a one-commit shallow clone)
  • Confirm each built case: a verifier agent checks its lines against the revert, drops text-only matches, and names itself on the case.yaml confirmed-by line

Not verified

  • plugins/review/evals/revert-cases.test.sh was not executed: this lane runs only the named wrappers, so the suite is left to the PR's CI test-bash leg
  • No revert- case was built, and list was not run over real history: the checkout has one commit and WebFetch gives only summaries of GitHub diffs. Both are listed in remaining
  • No paid claude plugin eval run of revert- cases: none exist yet
  • tidy skipped: the change is about 470 changed lines, over the 300-line tidy cutoff
  • check-evals-quality not run: it takes evals.json files, and plugins/review/evals uses case directories
  • Review findings left as is (non-blocking): reverted lines are matched by text, so a repeated line like fi can be listed at two places (the verifier step drops these); the grader keys on the basename, so two caught files with the same basename in different directories can cross-match; a path…
  • The decision comment's example SHAs (bee196a, 444ef9e, 398d0ac, 6d6b300, 1d1fca6) were not checked as pairs here. 6d6b300 (#8303) already backs the recall-pr-merge-merge-state-listing case

Fix

  • plugins/review/evals/revert-cases.sh: new script with subcommands list (revert pairs and count), build (writes a revert- case and its fixtures), scaffold (rebuilds a case offline and refuses an unconfirmed one) and report (caught or missed per case, a total, indicative below 5 cases)
  • plugins/review/evals/revert-cases.test.sh: new suite on a throwaway repository covering both revert forms, the no-added-line and does-not-reverse skips, build output, the confirmation gate, scaffold, grader matching and report totals
  • plugins/review/README.md: new Revert cases section covering how to list, build, confirm, run and report, and stating that no CI job or required check runs the cases
  • .changes/review/work-resolve-issue-issue-12939-41743071136-1-f9b9cbb5.md: minor changelog fragment for the review plugin

Verification

The runner ran each check from the base copy against 75850c4:

  • scripts/check-changelog-fragments.sh --check: passed
  • scripts/check-changed-skills.sh --merge-base: passed

The run reports:

  • /home/runner/work/claude-code-plugins/claude-code-plugins/.lane/checks/bin/check-changelog-fragments: all 31 fragments valid
  • /home/runner/work/claude-code-plugins/claude-code-plugins/.lane/checks/bin/check-changed-skills: no changed skills, nothing to gate
  • /home/runner/work/claude-code-plugins/claude-code-plugins/.lane/checks/bin/check-affected: 12 passed, 6 skipped over 4 changed files. Shellcheck first failed on SC2319, SC2310 and SC2016, which I fixed with a helper and reasoned disable directives, then passed
  • cant-fail-scan --file plugins/review/evals/revert-cases.test.sh --check: PASS, no gating findings
  • fresh-context review subagent over the whole diff: no blocking findings. Fixed: a doubled (#n) suffix on squash-merged revert PRs, a missing leading boundary in the grader regex, the prompt calling the partial diff the whole diff, the README overstating what the grader checks, and nothing enforcing…
  • git-read log: the checkout is a one-commit shallow clone (bf216e0), so list could not run over real history here
  • read https: //github.com/melodic-software/claude-code-plugins/commit/bee196a5e2: revert commit bee196a (#12247) restores four skill cuts from several PRs and has no 'This reverts commit' line. WebFetch returns a summary, not the verbatim diff, so the page could not supply fixture contents
  • read https: //github.com/melodic-software/claude-code-plugins/commit/bee196a5e2fda2c0ab61300b15142353364e8a9b.diff: WebFetch refused to return the raw diff verbatim
  • The lane run: https://github.com/melodic-software/claude-code-plugins/actions/runs/41743071136

Left out: the items under Not verified.

Related

#12939

Lane-Activity: work-resolve-issue/resolve-issue
@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review reviewed ae5b07e6cecc50f1918a35a357e8b58972150fd0: read 1 of 1 changed file(s); 0 finding(s), 0 as inline comments.

Security review (incremental): PR #13031

No security issues found. This review covers the changes since 75850c4e. I read the whole incremental change. It touches one listed file, plugins/review/README.md, in one hunk, and that file matches none of the trusted-path globs.

What changed. The Revert cases section of the README now gives the verifier's steps in more detail. When the verifier drops a falsely matched line, it must remove it from all three places build writes it:

  • the # lines: comment in case.yaml
  • expected_outcome in prompt.md
  • the line alternation in graders/caught.md

After that, the verifier names itself on the confirmed-by: line.

Why there is no finding.

  • This is operator documentation for eval tooling that a person runs by hand. No CI job or required check runs it.
  • The hunk does not remove, narrow or soften any instruction. It adds steps to the verification.
  • It touches no workflow, action, token request, merge hold, trust control or plugin manifest. No untrusted input reaches a lane that holds credentials.

Earlier review. My review of 75850c4e had no findings, so nothing carries over.

Spend. None in CI.

critical-findings: 0

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude-review: 1 inline finding(s) on 75850c4ec6e382078fa7f28034500ef451608def; the summary is in this lane's pull request comment.

claude-lane: claude-review

Comment thread plugins/review/README.md Resolved
@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed ae5b07e6cecc50f1918a35a357e8b58972150fd0: read 1 of 1 changed file(s); 0 finding(s), 0 as inline comments.

Code review (incremental): PR #13031, changes since 75850c4e

Result: no findings. I read every hunk of the incremental change. It touches one file, plugins/review/README.md.

What changed: commit ae5b07e6 fixes this lane's earlier advisory finding on the README's verifier step (thread discussion_r4462597431, now resolved). The README now says:

  • build writes the caught lines to three places, and nothing rebuilds one from another.
  • The verifier must drop each line from all three: the # lines: comment in case.yaml, expected_outcome in prompt.md, and the line alternation in graders/caught.md.
  • If none of a file's lines remain, the verifier removes that file's whole alternative.
  • A line left in the grader still grades a citation of it as caught.
  • The verifier names itself on confirmed-by: only after these edits.

Checked by reading the code (not run, since this lane has no shell): the new text matches revert-cases.sh.

  • L187-L190 builds one regex alternative per file and the where string from the same line list.
  • L197-L199 writes the # lines: and confirmed-by: lines.
  • L215 writes expected_outcome.
  • L228 writes the grader pattern.
  • L247 is the gate, which checks only confirmed-by, so the README's caution is needed.

Left for a person: the earlier finding's stronger fix is still open. That fix was a subcommand that rewrites all three files from the confirmed line list. The fix's author left it out of this round on purpose. The documented manual step covers the problem for now, so I did not raise it again.

Other checks:

  • Settled threads: one, the finding above. It is resolved, no lines changed since resolution, and I did not raise it again.
  • Prior findings: the only inline entry is that same thread, and this commit addressed it.
  • Round cap: not reached (1 of 3).
  • Closing issue: none. The PR says Refs: #12939 and closes no issue.
  • Plugin manifests: none changed in this increment.
  • Embedded instructions: none in the PR context or thread text.
  • Spend: none added. The change is documentation only.

@melodic-automation-lanes

Copy link
Copy Markdown
Contributor Author

The unattended merge path armed native auto-merge at head 75850c4ec6e382078fa7f28034500ef451608def.

Lane-Activity: pr-unblock/address-feedback
Lane-Thread: PRRT_kwDOTCGFQM6zaOfm
@melodic-automation-lanes

Copy link
Copy Markdown
Contributor Author

pr-unblock / address-feedback finished round 1 of max 4 on ae5b07e.

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.

0 participants