mirror of
https://github.com/trailofbits/skills.git
synced 2026-09-14 14:28:48 +08:00
Dan Guido
488d37c71c
* Pin the review model, raise effort to xhigh, stop invented evidence Three problems with the automated review, found while auditing what was actually reviewing PRs #253-#255. **The model was never specified.** `claude_review.sh` passed `--effort` but no `--model`, so the reviewer was whatever the CLI happened to default to at run time, and nothing in the logs said which. The CLI version was pinned to stop CI changing without a commit while the larger lever on review quality was left floating. Both tiers now pin `opus`, and the run line records model, effort and resolved CLI version. **The CLI is now deliberately unpinned**, in both this workflow and the loadability check, so they track releases as they ship. For loadability that is the point: the version worth proving plugins load against is the one users run. The logged version is what makes a surprising result attributable after the fact. **The review claimed to run things it cannot run.** Its allowlist is `gh pr` reads plus Read, Grep and Glob — no interpreter, no test runner. Across three PRs it reported a Python snippet it had "verified directly" whose regex cannot compile, a pytest suite it had "run locally" with a pass count that does not match reality, and a shell script three "independent verifiers" had supposedly executed. Every conclusion was correct and every proof was fabricated. The deep prompt already said "you cannot execute anything"; the prompt that actually runs never did. That statement moves into the shared prompt, extended to forbid reporting output no tool produced. Effort goes low -> xhigh on the tier that runs on every push, so the strongest review is the default rather than something to remember to ask for. The shared prompt also now says to rank on consequence rather than diff size, after a one-character fault that made a checker miss its own target was filed as a nit. The `fast` name is kept: it is the check name branch protection matches on, and it describes the trigger rather than the effort. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Collapse the two review tiers into one The split was a cheap pass on every push plus an xhigh adversarial pass behind a `deep-review` label. It did not survive contact: the label was never created, so the deep tier skipped 9 times and ran zero, and every review this repository has ever received came from the cheap tier. With both tiers now on opus at xhigh, the only thing left separating them was scope, and there was no reason to gate the better scope behind a label somebody has to remember to apply. So there is one job. It takes the deep tier's prompt and tools — reads beyond the diff, gets git history, checks the PR's claims against the files — and the cheap tier's `--edit-last` posting, which matters now that it runs on every push. `fast` is gone from the check name, the script's tier argument, and the concurrency key. The name was already inaccurate once effort went to xhigh, and the ruleset on main requires license/cla, Validate, Pre-commit and bats — not this check — so nothing depended on it. The elaborate label-keyed concurrency group goes too; it existed only to stop a push cancelling an in-flight deep review, and there is no second tier to collide with. Timeout follows the deep tier at 30 minutes, since xhigh on a large diff needs the room. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Address the review of this PR The new xhigh reviewer found real problems in its own configuration. **The API key was in the unpinned install step's environment.** It was there only to satisfy `if: env.ANTHROPIC_API_KEY != ''`, since `secrets` is not available in a step-level `if:`. Combined with `@latest`, that runs npm lifecycle scripts from a version nobody chose beside an org-wide credential. Presence is now reduced to a boolean in its own step, and the install step holds no secret at all — which is what makes @latest acceptable there. **The posting guard counted comments it did not write.** It matched any issue comment in the window, so a maintainer replying to the previous review could stand in for a review this run never posted: green check, no review. Measured on this PR's siblings — the old query counts 2 on #253 and #255, the new bot-only query counts 1. Also paginated, since the unfiltered call would miss a review past the first 30 comments. **`AGENTS.md` reaches the reviewer and tells it to run things.** CLAUDE.md is just `@AGENTS.md`, so it loads as project instructions telling the model to run `make check`, run `prek run -a`, and consult a `claude-code-guide` subagent — none of which it can do. That is the fabrication channel this PR exists to close, arriving by a route the prompt did not address. The prompt now names those files and says they are addressed to someone else. **validate.yml goes back to a pinned CLI.** Unpinning it was my extension, not what was asked, and the risk is misplaced: that job is a required check, so an upstream release renaming a field in `plugin list --json` reddens every open PR at once and blocks merges with no commit to explain it. The review job can go red harmlessly. This also re-aligns Codex and Claude, both pinned there again, and keeps dependabot.yml's note about "the pinned npm CLI versions" accurate. Smaller: the prompt described its allowlist as "only `gh pr` reads" while mandating `gh pr comment`, a write, and granting `git log`/`git diff`. And the comment over `MODEL` claimed an attributability the `opus` alias does not provide, since it tracks new Opus releases by design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Unpin the Claude Code CLI on the loadability check too Reverses the revert two commits back, deliberately and with the argument against it on the record. The review made the case for pinning here: `Validate plugins and skills` is a required check, so a Claude Code release that breaks plugin loading reddens every open PR at once with no commit to explain it. That reading is correct about the mechanics and wrong about which failure costs more. A pin nobody remembers to bump drifts until CI is proving loadability against a version no user runs, which is the one thing this check exists to establish — and it fails silently, by passing. The loud break is the preferable failure, and it is now the chosen one rather than an oversight. Dependabot does not track npm CLIs installed this way, so the real choice was a live version or a stale one, never a maintained one. The Codex CLI beside it stays pinned at 0.146.0. Same class of manual pin and arguably the same argument applies, but unpinning another vendor's CLI was not asked for and would widen this change past its subject. dependabot.yml's note is corrected to match: singular, and naming which one is pinned and why the other is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>