Skip to content

feat(pr-review): let a caller admit bot senders by numeric id - #734

Merged
kyle-sexton merged 3 commits into
mainfrom
feat/review-allowed-bot-sender-ids
Oct 11, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
feat/review-allowed-bot-sender-ids

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 11, 2026 •

Copy link
Copy Markdown
Member

No related issue: step 1 of Phase 17 in the claude-code-plugins pr-unblock plan (design T20/NEW-6, owner decision B2b); no tracker issue exists for it.

Hold: a human merges this.

No coordination step is owed: the CI/CD Conventions session has ended (owner, 2026-10-11), so the owner alone decides the merge.

Summary

pr-review.yml and pr-review-security.yml skip every [bot] actor, so a pull request the automation-lanes App pushes to is never AI-reviewed, and the merge lane then holds it forever. This adds a way for a caller to admit exactly one App by its numeric user id.

Branched from main b3a1d7dc01ae1a2e57dc8cf4ec535fa4e062c8f1, which was still the tip when the branch was cut.

Fix

  • New optional input allowed-bot-sender-ids (string, default '', comma-separated numeric ids, no spaces) on both reusables.
  • Job if: admits a [bot] actor only when inputs.allowed-bot-sender-ids is non-empty and well formed (no leading, trailing or doubled comma), github.event.sender.id is present, and contains(format(',{0},', ids), format(',{0},', github.event.sender.id)) matches. allowed_bots gets the sender login only on that same guarded match. Draft and fork terms are unchanged.
  • allowed_bots on claude-code-action gets github.event.sender.login only when that id matched, else ''.
  • SECURITY MODEL headers and the README inputs table and Skips paragraph describe the input.
  • R-INC: full. After a base merge that touches a PR file, the incremental review already falls back to a full review of the PR's files (pr-review.yml@ab83b01:247-272), so no incremental-scope change is needed. To ship it, cut a minor release under the existing names; this PR cuts no release.

With the input empty, the gate and allowed_bots behave as before, so existing callers are unchanged.

Verification

  • node --test .github/scripts/*.test.cjs: 312 pass, 0 fail (gate test 18/18, incl. senderless, near-miss ids and malformed lists). node --test .github/actions/report-lane-outcome/*.test.cjs: 21 pass. biome ci clean after 9d152cb.
  • claude-lane-job-gates.test.cjs now evaluates both job gates and allowed_bots against events: no list (bots skipped), human actor (reviewed, no bot allowed), listed sender (admitted, its login only), near-miss ids 22 and 2222 against list 111,222 (skipped), senderless event with empty input (skipped), listed bot on draft or fork PR (skipped). I checked that the tests can fail on a scratch copy: removing the empty-input guard fails the senderless case, and dropping the comma delimiters fails the near-miss case.
  • actionlint on both files: clean. zizmor --offline: the same 2 medium artipacked findings as main (claude-code-action#1236, documented in the header). typos and markdownlint-cli2 README.md: clean.
  • Phase 17 sanity check: grep -c allowed-bot-sender-ids prints 13 for each file; the inputs.allowed-bot-sender-ids != '' per-file zero count prints 0.

Related

  • Next: standards runner-policy contracts add allowed-bot-sender-ids to allowedInputs for the release SHA, and the claude-lanes callers pass ${{ vars.AUTOMATION_LANES_APP_SENDER_ID }}. Both happen after this merges and is released.

🤖 Generated with Claude Code

kyle-sexton and others added 3 commits October 11, 2026 12:21
Both review reusables skip every [bot] actor, so a pull request pushed by
an automation App is never reviewed. Add an optional input,
allowed-bot-sender-ids (comma-separated numeric user ids, default empty).
The job gate admits a bot only when the event sender's id is listed,
guarded so an empty list never matches an event without a sender, and
claude-code-action's allowed_bots gets that sender's login only when the
id matched. Draft and fork skips are unchanged; an empty input leaves
every existing caller as it was.

The job-gate test now evaluates both gates and allowed_bots against
listed, unlisted, near-miss, senderless, draft and fork events.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The empty-input guard caught only an exactly empty list. A list with a
leading, trailing or doubled comma still contains ',,', which a
senderless event formats to, so the gate would admit any bot there. Both
the job gate and allowed_bots now also reject such a list and require a
sender id. The job-gate test covers senderless events against those
lists and a listed sender against each malformed shape.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 11, 2026 17:02
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 11, 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-10-11T17:05:01.149181Z 9d152cb 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.

@claude

claude Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 2s —— View job


Code review in progress

  • Gather context (PR body, diff, CLAUDE.md/REVIEW.md)
  • Review changed files for correctness and maintainability
  • Dispatch a hunter subagent to independently stress-test the bot-sender-id gate expression
  • Post inline findings (if any) and summary

@claude

claude Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 37s —— View job


Security review — /review:security-review

  • Gather context (diff, changed files, instruction-surface check)
  • Review pr-review.yml and pr-review-security.yml gate logic
  • Review allowed_bots propagation to claude-code-action
  • Review test file changes
  • Review README changes
  • Check instruction-surface deletions register
  • Adversarial validation of candidate findings
  • Post final findings

No security issues found.

This PR adds an opt-in, per-caller allowlist (allowed-bot-sender-ids) that lets a reusable-workflow caller admit exactly one bot sender past the existing "skip all bots" gate in both pr-review.yml and pr-review-security.yml. I reviewed it against the trust-boundary and logic criteria this lane owns (not zizmor's Actions-hardening lane).

Trust-boundary analysis

  • The gate matches on github.event.sender.id, not github.actor or .login. sender.id is set by GitHub from the authenticated caller of the triggering API action — it isn't derived from PR body/commit content and can't be forged by a PR author, and (per the in-code comment) it survives login renames/squatting in a way a login string wouldn't. This is the right attribute to gate on.
    pr-review.yml#L168-L176
  • The new admission clause is OR'd into the bot-skip term but the result is still AND'd with the pre-existing draft check and fork check (head.repo.full_name == github.repository), so a listed bot's draft or fork PR is still skipped — confirmed by the explicit test still skips a listed bot's draft or fork PR. The "Draft and fork terms are unchanged" claim in the PR body holds.
  • The comma-delimited match uses contains(format(',{0},', list), format(',{0},', id)), which correctly prevents substring false-positives (e.g. id 22 or 2222 are not admitted by list 111,222). The additional guards (!startsWith(...,','), !endsWith(...,','), !contains(...,',,'), and the github.event.sender.id truthiness check) correctly close the degenerate case where a malformed list or a senderless event could otherwise produce a false ',,' contains ',,' match — this is exactly the bug the earlier commit in this branch (f586183, "admit no bot on a malformed sender-id list") fixed, and it's now covered by dedicated tests (admits no bot on an event without a sender, admits no bot when the id list is malformed).
  • allowed_bots fed to claude-code-action is the matched sender's login only when the id matched, '' otherwise, so the action still rejects every other bot — the allowlist can't be used to admit an unlisted bot via a side channel.
  • A caller can only exercise this by controlling the allowed-bot-sender-ids input on the uses:/with: call into this reusable workflow, which requires the same write access to the base repo that already lets someone edit any CI workflow; a fork PR can't reach this path because the fork check still applies regardless of bot admission. No new privilege is reachable from a fork or from PR-body/commit content.

Instruction-surface check

The security-model header comment "this lane allows none" (re: bot actors) is softened to describe the new conditional admission. docs/conventions/instruction-exception-register/README.md is not present in this checkout, so I used consequence-based recognition instead: the softened rule is paired with an actual enforcement mechanism (the unforgeable sender.id check plus the unchanged draft/fork/token-scope checks), not a bare removal — nothing that used to be blocked is now reachable without a corresponding check. I don't see a matching protected-class violation here, but flagging that the register itself wasn't consulted since it isn't checked out.

Scope note: the empty-input default ('') preserves existing caller behavior exactly, and the test suite (claude-lane-job-gates.test.cjs) exercises the gate and allowed_bots output against a real subset-matching evaluator of the Actions expression, not just string matching on the YAML — this is solid coverage for a logic change like this.

@github-actions

Copy link
Copy Markdown

claude-security-review has reviewed this pull request through 9d152cb; a later push is reviewed from there.

@github-actions

Copy link
Copy Markdown

claude-review has reviewed this pull request through 9d152cb; a later push is reviewed from there.

@kyle-sexton
kyle-sexton merged commit a920202 into main Oct 11, 2026
81 checks passed
@kyle-sexton
kyle-sexton deleted the feat/review-allowed-bot-sender-ids branch October 11, 2026 17:08
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.

1 participant