Repository navigation
test(review): add revert eval cases and resolve reverts named by PR number - #13097
kyle-sexton wants to merge 6 commits into
Conversation
…umber revert-cases.sh list found 0 pairs in this repository's full history: every revert names its original by pull request number, not by the reverted subject or a "This reverts commit" line. List now tries each merged non-revert pull request a revert's message names, which finds 16 pairs. Adds the 16 cases a verifier agent confirmed; a 17th pairing was dropped because that revert names the pull request without undoing it. Closes #12939 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
They are byte copies of history, which the markdown, typos and exec-bit checks must not judge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
claude-security-review partial review of Chunk 1 of 3 (40 files)Security review: chunk 1 of 3 (PR #13097). Partial review: 32 of 40 files readNo security issues found in this chunk. What this chunk changes
Checks
Not reviewedI did not inspect the 8
One case for whoever runs the evals: the archives hide their contents from every review lane, and SpendSixteen added on-demand eval cases (max 40 turns, 900 s timeout, one run each) spend prepaid API credit when someone runs them. They are not a required check and cause no billable charge. critical-findings: 0 Chunk 2 of 3 (40 files)Security review, chunk 2 of 3 (PR #13097, head 969f511): partialI found no security issues in the text files of this chunk. The review is partial: I read every text file whole (32 of 40), but I could not see what the 8 critical-findings: 0 What I read
Why the archive contents matter (context from chunk 3, not a finding here)
Other notes
Chunk 3 of 3 (3 files)Security review: chunk 3 of 3 (PR #13097)No security issues found in this chunk. I read every hunk of all 3 files: What I checked:
Spend: the 16 new eval cases use prepaid model usage only when someone runs No prior findings apply: 0 rounds before this one, and the cap of 3 does not apply. critical-findings: 0 |
…ectory Under evals/fixtures/ the orphaned-fixtures check requires a grader or test to name each file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
claude-review partial review of Chunk 1 of 3 (40 files)Code review, chunk 1 of 3: PR #13097 at
|
…tory Scaffold runs git add -A, so in a real checkout it would commit stray untracked files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…read them They are line numbers at the historic merge, not citations of the current tree. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if [[ "$found" == 0 ]]; then | ||
| printf 'skip\t%s\tits original commit is not in this history\n' "$sha" >&2 |
There was a problem hiding this comment.
Advisory: wrong skip reason when a named pull request is found but fails a check.
In the new pull-request-number path, found stays 0 in three cases:
- The message names no merged non-revert pull request.
- It names one, but the revert removes no line that pull request added (
pair_linesis empty). - It names one, but that change no longer reverses cleanly.
All three print its original commit is not in this history. That is false for cases 2 and 3, where the original was resolved and is in history. The header promises skip<TAB><sha><TAB><why>, and the subject and body paths keep the two failure reasons apart (removes no line #<n> added, #<n> does not reverse cleanly onto its parent).
How to see it: commit feat: add q (#20) adding q=1, then commit revert: undo it (#21) with body Undoes #20. that edits an unrelated line of the same file. list resolves #20, pair_lines is empty, and stderr says the original is not in history. Someone sorting skipped reverts to recover pairs (as with the #9438/#9456 pairing in the PR body) is pointed at a history problem that does not exist.
Fix: record the last failure per candidate, for example removes no line #<n> added or #<n> does not reverse cleanly. Print not in this history only when named_originals yields nothing. Add a test case for a named pull request that fails pair_lines.
claude-lane: claude-review
| MD | ||
|
|
||
| printf -- "---\ntype: regex\npattern: '%s'\nflags: i\n---\n" "${alt//$q/$q$q}" >"$case_dir/graders/caught.md" | ||
| tar --sort=name --mtime=@0 --owner=0 --group=0 --numeric-owner -C "$fixtures" -c . | gzip -n >"$case_dir/fixtures.tar.gz" |
There was a problem hiding this comment.
Advisory: build reports success even when the archive step fails.
The script runs without set -e (line 2's shellcheck note says each caller checks the status it needs), but nothing checks this pipeline's status. If tar fails, gzip -n still writes a valid but empty gzip stream to fixtures.tar.gz. Then rm -rf deletes the only copy of the fixtures, and build prints $case_dir and exits 0.
The failure is easy to hit: --sort=name, --mtime=@0 and --owner/--group/--numeric-owner are GNU tar options. bsdtar, the default tar on macOS, rejects --sort.
The broken case only surfaces later, when scaffold runs tar -xzf under set -e. By then the case directory exists, so a rerun of build dies with already exists until it is deleted by hand.
Fix: check the status and clean up on failure:
if ! tar --sort=name --mtime=@0 --owner=0 --group=0 --numeric-owner -C "$fixtures" -c . | gzip -n >"$case_dir/fixtures.tar.gz"; then
rm -rf "${fixtures:?}" "${case_dir:?}"
die "could not archive the fixtures for $name (GNU tar is required)"
ficlaude-lane: claude-review
Closes #12939
Summary
Adds the first 16 revert cases for
review:code-reviewand their first run: the lane caught 11 of 16 on Opus at effort high.revert-cases.sh listcould not find any pair on this repository's history before this PR, because reverts here name the original by pull request number.Fix
revert-cases.sh listnow tries each merged non-revert pull request a revert's message names, when neither the subject nor aThis reverts commitline names its original. It found 16 pairs in 7,584 commits; before, 0 of 10 reverts resolved.revert-*cases built withbuild, each named as confirmed by a verifier agent on itsconfirmed-by:line.<case>/fixtures.tar.gz: they are byte copies of history, and as plain files they failed the markdown, typos and exec-bit checks. Reviewers can read the same files from the commits eachcase.yamlnames. A verifier scanned the unpacked files for home paths and token shapes and found none.listno longer pairs a revert with an earlier revert.Verification
claude plugin evalon a copy outside git,--case 'revert-*' --scaffold --ablation none --runs 1 -j 2,claude-opus-5-5, effort high (thepr-reviewprofile indocs/conventions/pr-pipeline.yaml), Claude Code 2.1.318.run-validity.py --runs 1 --scaffoldedat3e711ca460: VALID, no denied tool call in 16 traces. Its warning is only that no case has a skill-fired grader.revert-cases.sh report, pair count 16:Read the total with these limits:
park-ledgercases andrevert-1835revert whole files or large parts of a change, so almost any citation counts. Comparing two totals is sounder than reading one.revert-6500's seven copies,revert-1835'splugin.jsonfiles) a citation of a sibling file can count.git blamefrom each listed line to its pull request, which matched every line. No line was dropped from the 16.The eval ran before the fixtures were packed; I scaffolded one case from its archive afterward, and it rebuilt the change.
Left out:
revert-cases.test.shis the only local test run, since CI runs the suites; no second run for a before-and-after comparison, because no review-lane change is in this PR.Related
🤖 Generated with Claude Code