[kyle-sexton]

No related issue: owner rule 2026-10-27 (a person only for real HIGH/CRITICAL security)

Summary

The claude-security-review lane holds a pull request for a person (criteria=critical-security) on any CRITICAL finding. The reusable workflow leaves the definition to this skill: its prompt says critical-security: as the review command defines it (pr-review-cli.yml L1108 at v0.65.15). The skill rated by path and by key:

• "Each of these is CRITICAL" for any trust-control change;
• "unless the diff shows the change narrows" for broker-loaded files;
• "a plugin entry added ... is CRITICAL" for manifests.

It held four pull requests, and none of the holds was a real HIGH or CRITICAL:

PR	Hold reason	What the code shows	Real severity
#10224	Touches broker-loaded resolve.mjs and pr-pipeline.yaml. The lane wrote "I found no token-scope widening, but the lane rule treats this as CRITICAL".	effect: read is unchanged. Skills are regex-checked and must exist in base.	LOW (matches the owner decision on #8485)
#10429	"a new default-on core plugin entry"	The entry sets "defaultEnabled": false (marketplace.json:780 at 97302a5).	IMPORTANT manifest flag
#10423	Narrowed merge-adoption idle hold	human_veto_logins, the CHANGES_REQUESTED refusal and the ruleset's required thread resolution are untouched and still block.	Not a hold
#10451	Regex edge cases in babysit_risk.py, "a merge-hold control"	The findings are real, but the hold they weaken guards spend and is not a security gate.	MEDIUM and LOW

Fix

All changes are in plugins/review/skills/security-review/SKILL.md:

• Severity bar (new, under Criteria). A finding is CRITICAL only when the review shows all three, each with file and line:

  1. who reaches the change (an attacker or untrusted input);
  2. what security impact they get, where a bypassed gate counts only when it guards a security impact, not spend, quality or style;
  3. that no existing control stops it, checked rather than assumed.

  Touching a trust control, gate file or hold path is not a severity, because the hold paths already route those files to a person. A finding that rests on a configuration value quotes the line it read.

• Fail closed. If the review has read the code and still cannot tell whether a path is reachable or blocked, the finding is unsettled and stays CRITICAL. A finding with no security impact is never CRITICAL.

• Trust controls. These widenings meet "who" and "what" by themselves, so they stay CRITICAL unless a cited control blocks them:

  • who is trusted;
  • what a token may do;
  • what untrusted text or code reaches a lane;
  • the kill switch's reach.

  A weakened merge hold has to show "who" and "what" like any other finding, and part 3 checks the gates that still run after it. The broker bullet now asks whether the change widens what the signer grants. Before, it asked whether the change touches what the signer loads. It also covers two cases the old wording did not name:

  • a skill added to an activity's allowlist, judged by the tools it grants;
  • code added to the signer's load path that can read or send the key, a token or the environment.

• Manifests. Code that starts running for existing users on update with no step of theirs is CRITICAL whatever it does today, unless an opt-in or another cited control stops it:

  • a default-on entry added;
  • any key added to a plugin that already exists, whatever its defaultEnabled;
  • a source moved off the repository.

  For an added entry, the review quotes defaultEnabled. Only "defaultEnabled": false makes it opt-in, and an opt-in entry rates IMPORTANT. An entry without the line is treated as on (fail closed). The behavior of defaultEnabled is a pointer with an as-of date, not restated.

• Refute pass. Every CRITICAL candidate, fanned out or not, goes to a review:lane-verifier. The verifier looks for a blocking control, a missing part of the bar, or a misread config value. A confirmed or unsettled result keeps the finding CRITICAL. A verifier may lower a manifest flag's severity but never remove its line.

Vocabulary is unchanged (CRITICAL / IMPORTANT / SUGGESTION). The brief's "HIGH or CRITICAL" maps to CRITICAL, the only severity that holds.

Verification

There are 13 new claude plugin eval cases under plugins/review/evals/security-review-*:

• four negatives, one from each held PR's shape;
• nine positives that must still rate CRITICAL.

They use acme/ names, contain no transcript, and grade with regex graders only. Every case ran on Claude Code 2.1.311 with --scaffold --ablation none --runs 3 --judge-model opus, against a copy of plugins/review at base (main's SKILL.md) and at head. Scores are per run, 1.00 = pass.

Case	Kind	Base	Head
broker-skills-read-effect (#10224 shape)	must not be CRITICAL	0.50 x3 (CRITICAL every run)	1.00 x3
marketplace-entry-default-off (#10429 shape, also checks defaultEnabled is quoted)	must not be CRITICAL	0.33, 0.33, 0.67	1.00 x3
merge-hold-other-gates-block (#10423 shape)	must not be CRITICAL	0.50 x3 (CRITICAL every run)	1.00 x3
cost-hold-regex-narrowed (#10451 shape)	must not be CRITICAL	0.50 x3 (CRITICAL every run)	1.00 x3
kill-switch-narrowed (existing)	must not be CRITICAL	1.00 x3	1.00 x3
trusted-actor-prefix-match	must be CRITICAL	1.00 x3	1.00 x3
broker-effect-write (issue-driven activity read to write)	must be CRITICAL	1.00 x3	1.00 x3
broker-skill-grants-bash (issue-driven allowlist gains a skill granting Bash)	must be CRITICAL	1.00 x3	1.00 x3
merge-veto-dropped (CHANGES_REQUESTED no longer refuses)	must be CRITICAL	1.00 x3	1.00 x3
marketplace-entry-default-on (no defaultEnabled, adds a hook)	must be CRITICAL	1.00 x3	1.00 x3
hook-added-opt-in-plugin (hook added to an existing defaultEnabled: false plugin)	must be CRITICAL	1.00 x3	1.00 x3
marketplace-source-unpinned	must be CRITICAL	1.00 x3	1.00 x3
secret-logged-encoded (base64 token in a public log)	must be CRITICAL	1.00 x3	1.00 x3
prt-head-checkout-write (pull_request_target runs fork code with a write token)	must be CRITICAL	1.00 x3	1.00 x3
kill-switch-fail-open (existing)	must be CRITICAL	1.00 x3	1.00 x3
reads-named-diff-file (existing)	path traversal plus embedded directive	1.00 x3	1.00 x3

• The validity gate returned verdict: VALID on every pass. Head's broker and manifest rows come from the final text. Every other head row comes from the text before the last review fixes, which changed only the broker bullet and the manifest paragraph.
• No positive loses on head. Every negative moves from CRITICAL to clean.
• The base answers confirm the cause. For example: "The rule still makes this CRITICAL because the entry changes what installs."
• Calibration notes:
  • Broker case fixture. The first fixture left out the skill bodies, and head rated it "unsettled, so CRITICAL", which is correct fail-closed behavior. With the skills added to base, as the real lane had them, head clears it.
  • fix(source-control): hold the timeout and model-leg shapes the cost probe still passed #10451 case. An earlier head text counted a cost hold as "a merge gate bypassed". That text is now scoped to gates that guard a security impact.
  • default-on case. An earlier head text lowered this case once in three runs, because two rules conflicted. The manifest rule now says code that runs for users without an opt-in meets the bar by itself.
• Review findings on 4755000 (fixed here). Two came from claude-security-review, one from claude-review and one from Codex:
  • the skill allowlist and signer-environment cases in the broker bullet;
  • the opt-in downgrade, now limited to added entries;
  • the defaultEnabled restatement, now a pointer.
• Review finding on a2d5165 (fixed here). claude-review found that the merge-veto-dropped fixture showed no security impact, and that the fail-closed paragraph read both ways.
  • The fixture now makes CHANGES_REQUESTED the security lane's block on a merge to a published branch.
  • The paragraph now separates an impact shown to be non-security, which is never CRITICAL, from an unsettled one.
  • Re-run on the final text: veto-dropped, merge-hold-other-gates-block, cost-hold-regex-narrowed and broker-skills-read-effect each pass 3 of 3 on head, and veto-dropped passes 3 of 3 on base.
• check-skill.sh: PASS, 0 errors. The body grew from 284 lines (a WARN); the growth is the new criteria.

Related

• #10224, #10429, #10423, #10451: the four holds this calibrates against.
• Sibling branch fix/holdpath-clears-on-security-pass: lets the hold-path rule clear on a clean head-bound security pass. That is why a trust-control widening must stay CRITICAL here.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

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

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-27T16:17:55.914642Z	4755000	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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4755000361

ℹ️ 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]

claude-security-review: 2 inline finding(s) on 4755000361de55143ab002feefeefb01dbc61205; the summary is in this lane's pull request comment.

claude-lane: claude-security-review

[github-actions]

claude-security-review needs no new review of 1666b66caf53e65086af5bec90caa12ba4df7d4b: no file of this pull request changed since the review of 49d5d16.

Incremental security review of the change since a2d5165. I found no security issues.

• plugins/review/skills/security-review/SKILL.md (lines 149-150): The lane's own severity rule now reads "a finding the review has shown to carry no security impact (part 2), such as a hold that guards only spend, is never CRITICAL". Before, it said any finding "whose impact is not a security impact" was never CRITICAL, "settled or not". The new wording lets a finding drop out of CRITICAL only after the review has shown it has no security impact. Anything still unsettled stays CRITICAL under the fail-closed sentence just before it. That tightens the rule instead of loosening it. "Part 2" matches the severity list at lines 133-136, which already says a hold that guards only spend, quality or style is not a security impact. The file sits on a trusted path, so I read the whole change and what it gates. It changes no trust control, hold path, token scope or kill switch, and it removes no standing rule.
• evals/security-review-merge-veto-dropped/prompt.md: expected_outcome now says the merge lane "merges and publishes a pull request the security review lane blocked", where it used to say a reviewer blocked it. Eval metadata only.
• evals/security-review-merge-veto-dropped/scaffold.sh: The fixture comment on gate.py now says main is published on every merge, and that vetoed() is the only place the security lane's CHANGES_REQUESTED review stops a merge. The comment gives the eval's reviewer the context it needs to rate the planted finding CRITICAL. It does not change any executable fixture code.

No plugin manifests changed in this increment, so there are no manifest <field> lines.

critical-findings: 0

[github-actions]

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

claude-lane: claude-review

[github-actions]

claude-review needs no new review of 1666b66caf53e65086af5bec90caa12ba4df7d4b: no file of this pull request changed since the review of 49d5d16.

Incremental review of the three files changed since a2d5165. I found nothing to report.

• plugins/review/skills/security-review/SKILL.md (trusted path, whole change read): the fail-closed paragraph now says a finding is never CRITICAL only when the review has shown it carries no security impact, with a hold that guards only spend as the example. The old wording ("whose impact is not a security impact … settled or not") could be read as barring CRITICAL for a finding whose impact was still unknown. The new text fits severity part 2, which already says a spend, quality or style hold is not a security impact. The "unsettled → CRITICAL" rule is unchanged, so the lane still fails closed.
• plugins/review/evals/security-review-merge-veto-dropped/scaffold.sh: the new gate.py comment sets up the attacker model (the security review lane blocks with CHANGES_REQUESTED, vetoed() is the only stop, and main is published on every merge). This gives parts 1 to 3 of the CRITICAL bar a demonstrable path in the eval. The head-branch code is unchanged, so the planted defect is the same.
• prompt.md: expected_outcome now says "merges and publishes a pull request the security review lane blocked". That matches the scaffold comment and the SKILL.md part 2 wording.

The edited sentence in SKILL.md runs past the wrap width of its neighbours. MD013 is disabled in .markdownlint-cli2.jsonc, so I did not count it.

I only read the files. I did not run the eval, and I did not check how it scores a CRITICAL rating.

No hold: the change has no cost or critical-security effect.

[github-actions]

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

claude-lane: claude-review