Skip to content

Resolve trusted bare bot aliases canonically - #270

Merged
justin808 merged 1 commit into
mainfrom
fix/copilot-bot-identity-trust
Jul 27, 2026
Merged

Resolve trusted bare bot aliases canonically#270
justin808 merged 1 commit into
mainfrom
fix/copilot-bot-identity-trust

Conversation

@justin808

Copy link
Copy Markdown
Member

Why

GitHub exposes Copilot code review through two identities that share one global node ID: the pull request participants connection returns a bare, User-shaped Copilot alias, while resolving that node canonically returns the copilot-pull-request-reviewer Bot. The existing suffix-only bot check cannot safely recognize the hidden participant alias, so strict preflight blocks even when maintainers intend to trust Copilot reviews.

What changed

  • retain participant node IDs and resolve allowlisted bare aliases through GitHub's canonical node(id:) query
  • require both the bare alias and the canonical Bot login in trusted_bots
  • preserve the existing [bot]-suffix path
  • fail closed for human nodes, missing IDs, lookup failures, mismatched canonical identities, empty canonical logins, and malformed blank allowlist entries
  • cache canonical lookups by node ID

Security notes

This does not add Copilot—or any actor—to the packaged trust defaults. Consumers must still make an explicit, narrow trust decision for both identities. The canonical Bot proof prevents a human login that merely matches an allowlisted bare name from inheriting bot trust.

Validation

  • ruby skills/pr-batch/bin/pr-security-preflight-test.rb — 108 runs, 913 assertions, 0 failures
  • bin/validate — PASS, including 106 Ruby files with no RuboCop offenses
  • strict live replay against shakacode/react_on_rails#4804 with both Copilot identities in candidate trust config — SECURITY_PREFLIGHT_OK
  • independent Codex Sol/xhigh review found a blank-entry fail-open; fixed with a regression, then the final exact-head review reported no actionable findings
  • git diff --check and git show --check — clean

Churn and release notes

  • One substantive review cycle corrected the initial assumption that participants.__typename could prove Bot identity.
  • A second review cycle found and closed the blank/null allowlist fail-open.
  • Changelog classification: deferred_to_update_changelog; record this user-visible trust behavior before the next Agent Workflows release.

This is the upstream prerequisite for the React on Rails Copilot trust-policy update and unblocking react_on_rails#4804.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 47 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 53ab458c-9f1f-4b61-a009-297a23c947cc

📥 Commits

Reviewing files that changed from the base of the PR and between 162e168 and 461b5b4.

📒 Files selected for processing (2)
  • skills/pr-batch/bin/pr-security-preflight
  • skills/pr-batch/bin/pr-security-preflight-test.rb
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/copilot-bot-identity-trust

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review: Resolve trusted bare bot aliases canonically

Overview

This PR fixes a real gap: GitHub's Copilot code-review integration exposes two identities sharing one global node ID — a bare User-shaped Copilot participant alias, and the canonical copilot-pull-request-reviewer Bot behind it. The existing [bot]-suffix-only check couldn't recognize the alias, so strict preflight blocked legitimate Copilot review trust. The fix retains the participant's node id, and for logins that match trusted_bots but lack the [bot] suffix, resolves the canonical identity via node(id:) and requires both the bare alias and the canonical Bot login to be present in trusted_bots before trusting.

Code quality

  • canonical_bot_login/trusted_bare_bot_participant? are small, single-purpose, and fail closed at every branch: empty node id, lookup exception, non-Bot canonical type, and empty canonical login all correctly return "not trusted."
  • Caching by node ID (canonical_bot_cache) avoids redundant GraphQL calls when the same identity appears multiple times, including failed lookups (cached as nil), which is a reasonable perf/consistency tradeoff for a single run.
  • run_gh_json("api", "graphql", "-f", "id=#{node_id}", ...) uses array-form Open3.capture3 args elsewhere in the file, so there's no shell-injection risk from the interpolated node ID.
  • Style is consistent with the rest of the file (rescue-and-warn pattern, normalized_login/normalized_bot_login reuse, heredoc GraphQL constants).

Test coverage

Good coverage of the fail-closed paths: canonical match, canonical mismatch, lookup failure, missing node ID (asserting zero extra GraphQL calls), cache sharing across aliases, and the blank/null trusted_bots entry regression. One gap: there's no fixture where the canonical query returns {"__typename":"Bot"} with a missing/blank login — the code guards this correctly (return false if canonical_login.to_s.empty?), but it's currently only exercised indirectly (via the User-typename mismatch case), not via a Bot-typed node with an empty login.

Documentation gap

docs/trust-and-preflight.md and examples/trusted-github-actors.yml explain trusted_bots as "base bot name, strip the [bot] suffix" but don't mention the new requirement that bare-alias bots (like Copilot) need both the alias name and the canonical bot login listed in trusted_bots to be trusted. Since this PR's stated purpose is to unblock a downstream consumer configuring Copilot trust, it'd help to add a short note/example (e.g. under "Recommended Config Split") documenting that copilot + copilot-pull-request-reviewer both need to be present.

Security

  • The dual-identity requirement is a sound design: a human account whose login happens to match a trusted bare alias name can't inherit bot trust, because the canonical node(id:) lookup would resolve to __typename: "Bot" only for genuine bots — confirmed by the updated test_human_login_matching_bot_base_name_is_not_trusted_as_bot test.
  • Packaged trust defaults are untouched, so this doesn't silently widen trust for existing configs — consumers must opt in explicitly, as called out in the PR description.
  • Fail-closed behavior is correctly prioritized over convenience throughout.

Minor observations

  • Changing the participant dedup key from login alone to [login, node_id] (in participant_logins_and_unknown_count) is safe in practice since GitHub node IDs are stable per entity, but it's worth knowing this relies on that stability if participant pagination ever returns transiently-inconsistent id values for the same login across pages.
  • No functional issues found; the changes look correct and well-tested overall.

@justin808

Copy link
Copy Markdown
Member Author

head_sha: 461b5b4
triggered_gates:

  • security-auth-privacy
    rollback_disposition: Code-only rollback is established by reverting PR Resolve trusted bare bot aliases canonically #270; existing trust defaults remain unchanged unless a consumer explicitly allowlists both identities.
    decision: approve
    approved_by: justin808
    source: direct-user-task
    evidence: The operator set merge_authority to auto_merge_when_gates_pass and explicitly directed this task to add Copilot to the allowed whitelist and keep going; exact-head QA Evidence v2 passed at 461b5b4.
    ...

@justin808
justin808 merged commit 7ef1dca into main Jul 27, 2026
3 checks passed
@justin808
justin808 deleted the fix/copilot-bot-identity-trust branch July 27, 2026 17:07
justin808 added a commit that referenced this pull request Jul 31, 2026
…erge-hardening

* origin/main:
  Preserve unrelated skills in plugin-companion mode (#253)
  Add interactive PR walkthrough workflow (#278)
  Resolve trusted bare bot aliases canonically (#270)

# Conflicts:
#	CHANGELOG.md
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