Skip to content

test(code-tidying): install the comment tooling in eval runs so body rules are measurable - #12927

Merged
kyle-sexton merged 7 commits into
mainfrom
test/12703-comment-rules-measurable
Nov 3, 2026
Merged

kyle-sexton merged 7 commits into
mainfrom
test/12703-comment-rules-measurable

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Closes #12703

Summary

The code-tidying eval suites could not detect a loss in the remove-comments and audit body rules that #12549 shipped. tree-sitter never reached the eval sandbox, so every arm only proposed deletions. This change installs the comment tooling inside each run, preregisters negative controls that detect a loss, and measures #12549's rules against them.

Fix

  • evals/fixtures/comment-tooling-seed.sh installs pygments and tree-sitter with pip install --user --require-hashes --only-binary=:all: into the run's own HOME, from comment-tooling.requirements.txt, and fails the scaffold if the import still fails. seed.sh calls it, and probe-environment gets a scaffold that runs it.
    • Why it was missing: inside the sandbox, python3 falls back to /usr/bin/python3 because the user's uv-managed interpreter is unreadable, and the run has no network to install from.
  • scripts/comment-tooling-pins.test.sh fails when those pins drift from .github/requirements-ci.txt (version or hash set). Its # test-scope: header selects it when either file changes.

Verification

Claude Code 2.1.317, Sonnet under test, Opus judge, --runs 1. Every arm is VALID per run-validity.py. Thresholds were preregistered: a case drop, or a mean fall of 0.15 or more.

  • Done-when 1: probe-environment passes imports-42 in the sandbox. The remove-family before arm rose to about 0.97 mean, from feat(code-tidying): rename dissolve-comments to remove-comments with a comment-reviewer agent #12549's 0.79, because deletions now apply.
  • Done-when 2, negative controls:
    • Audit: deleting the audit-comment-residue body fell 0.75, with 7 of 7 case drops.
    • Remove: a full-body cut, launched together with a fresh before arm on the 13 aggressive cases, scored 0.94 vs 0.66, a fall of 0.28 (95% interval -0.37 to -0.18), with 4 case drops. Posture graders failed (design-history-deleted, fork-cost-deleted, code-intact), not only proof-ran.
    • A procedure-only cut (## Workflow) moved the remove suite 0.04, so it cannot be the control.
  • Done-when 3, after vs before:
    • Remove, 23 cases paired by name: +0.00, interval lower end -0.03. Non-inferior.
    • Audit, 7 cases: -0.02. A preregistered 3-run recheck of acr-dir-batch-skips read 1.00 before and 0.96 after, within the rule. batch-total failed 1 of 4 after and 0 of 4 before.
  • 146 model runs, each with EVAL CLAIM and END lines under the L75 host cap.
  • validate-cases.py plugins/code-tidying/evals: 0 FAIL. The pins test passes on the real files and fails on 4 altered copies (version bump, hash replaced, hash dropped, unpinned package).
  • scripts/check-changelog-fragments.sh --check-required origin/main exits 0.
  • Left out: the tidy-diff cases. They seed through td-seed.sh, which does not call the tooling seed yet (test(code-tidying): give the tidy-diff comment-triage eval case the comment tooling #12924).

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_015te8CoSUhxDqvddaTSwBz8

kyle-sexton and others added 4 commits November 3, 2026 12:57
The eval sandbox hides the operator's home, so its python3 falls back to a
system interpreter with no tree-sitter, and it has no network to install one.
change-shape.py then cannot certify a deletion and every remove-comments case
could only propose deletions. seed.sh now installs the hash-pinned pygments
and tree-sitter wheels into the run's user site, which the scaffold shares
with the agent; probe-environment runs the same install and checks the import.

Refs: #12703

Co-Authored-By: Claude Opus 5.5 <[email protected]>
… pins

comment-tooling-pins.test.sh compares every requirement in the eval seed's
comment-tooling.requirements.txt with the same package in
.github/requirements-ci.txt, version and hash set, and declares both files in
its test-scope so a change to either selects it.

Refs: #12703

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 3, 2026 19:51
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-03T19:54:28.174249Z ad3d217 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad3d217175

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

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

Security review: PR #12927, changes since 495dff59. No security issues found.

Scope. This review covers only what changed since 495dff59. That is one file, plugins/code-tidying/evals/fixtures/comment-tooling-seed.sh, and I read its only hunk. The file matches none of the trusted paths. This push changes no trust control, plugin manifest, workflow or standing instruction.

What changed. At L28-L32, the script now checks whether /usr/bin/python3 runs. If the version probe fails, the script prints a warning to stderr and exits 0, so it skips the fallback install. Before this change, set -e stopped the script at that point.

What I checked.

  • Main install still required. install python3 (L25) still runs first, before the new branch. Under set -euo pipefail, a failure there still stops the script. The new early exit only skips the optional fallback interpreter, which the script already treated as best-effort. The in-sandbox probe-environment case still catches a fallback that lacks the tooling. So this affects whether the eval is accurate, not security.
  • Injection. The interpreter path is fixed in the script. The warning message is a constant string, and no text from a pull request, issue or comment reaches it. Nothing is executed from $fallback.
  • Install flags. The pip install line is unchanged: it still uses hash-pinned wheels only, with --user and --no-deps. No new package source is added, and no token, secret or credential is involved.

Spend. None new. This change only skips work, a fallback pip install on the eval host.

critical-findings: 0

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

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

Code review (incremental): PR #12927, 495dff59..4c9681f1

Scope: this push changed one file, plugins/code-tidying/evals/fixtures/comment-tooling-seed.sh. I read its only hunk in .claude-lane/incremental.diff and checked it against the file at head. I used the full diff only for context.

What changed: the version probe of /usr/bin/python3 on L29 now runs on the left of || { echo …; exit 0; }. If that fallback interpreter is present but won't run, the seed prints a warning to stderr and exits 0. Before this push it exited 1 under set -e.

What I checked (by reading the code only; I had no shell, so I ran nothing):

  • Exit status and set -e. A bare assignment x="$(cmd)" takes the exit status of the command substitution. When it sits on the left of ||, a failure goes to the brace group and does not trip errexit. minor is a single command, so a failing interpreter (for example the macOS xcrun stub without the Command Line Tools) produces a non-zero status. The exit 0 inside the brace group ends the script, so the later [[ "$fallback" != "$own" ]] never sees an empty $fallback.
  • Primary interpreter stays strict. install python3 on L25 and own="$(minor python3)" on L27 still run outside any condition, so a broken python3 for the scaffold still fails the seed. That keeps Done-when 1 guarded.
  • Shellcheck directive. The SC2310 disable is the right code for a function called in an || context, and its reason matches the one on L34. The owner says shellcheck passes; that is the owner's claim and I did not verify it.
  • Prior finding. This push fixes this lane's earlier advisory about the probe on L28 failing under set -e, as suggested (warn, then exit 0). The owner resolved that thread at this head, and it is marked lines changed since resolution: no. I don't raise it again, and no new problem shows on these lines.

Closing issue #12703: this push doesn't change how any Done-when item is met. The figures for Done-when 2 and 3 are the author's claims and I did not verify them.

Spend: none new.

The PR context, settled-threads and prior-findings files contain no embedded instructions. No plugin manifest changed, so there are no manifest lines.

No findings.

…the sandbox may run

pip --user writes to the invoking interpreter's own user site, so a host whose
/usr/bin/python3 (the sandbox's fallback) differs in minor version from the
scaffold's python3 got wheels the agent never imports. The seed now installs
and checks the import for each distinct minor version of the two.

Refs: #12703

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015te8CoSUhxDqvddaTSwBz8

@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 2fe5cd58cb77be43173b0a2cafe00b39954a4930; the summary is in this lane's pull request comment.

claude-lane: claude-review

The fallback /usr/bin/python3 can be a minor the pinned wheels do not cover
(the Xcode 3.9 shim, Ubuntu's 3.12), and failing on it failed every seeded
case on hosts where the agent never runs it. It is now best-effort with a
warning that names the supported minors; probe-environment still catches a
fallback that never got the tooling.

Refs: #12703

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015te8CoSUhxDqvddaTSwBz8

@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 495dff5908824cdd2468d57b50602a4c340c0ea8; the summary is in this lane's pull request comment.

claude-lane: claude-review

The version probe on /usr/bin/python3 ran under set -e, so the macOS xcrun
stub (executable, exits 1 without the Command Line Tools) still failed the
seed. A fallback that does not run now warns and is skipped.

Refs: #12703

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015te8CoSUhxDqvddaTSwBz8
@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit 7e15bb1 Nov 3, 2026
56 of 57 checks passed
@kyle-sexton
kyle-sexton deleted the test/12703-comment-rules-measurable branch November 3, 2026 21:32
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.

code-tidying: make the remove-comments body rules measurable

1 participant