Skip to content

fix(github): lead with the database's operators on command-rejection comments - #960

Merged
aparajon merged 2 commits into
mainfrom
armand/not-auth-operator-order
Aug 13, 2026
Merged

fix(github): lead with the database's operators on command-rejection comments#960
aparajon merged 2 commits into
mainfrom
armand/not-auth-operator-order

Conversation

@aparajon

@aparajon aparajon commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Why this matters

When a PR command is rejected as not authorized, the comment lists who can run it — but as one flat list where the database's own operators (the people the requester should actually ask) can end up buried behind the global admin principals. The review-required comment already solved this ordering; the rejection comment should read the same way.

What it does

  • Splits the authorized-principals lookup into the database's operators and the broader admins, deduped so a principal configured in both groups is listed only as an operator.
  • The rejection comment now leads with an Operators of <database> section, followed by Other authorized teams and users, and closes with what to do next (ask one of them, or request team membership). A database with no configured operators keeps a single flat list.
  • Principals still render as inline code, never @-mentions, so a rejection never notifies every admin.

🤖 Generated with Claude Code

…comments

The not-authorized comment listed every authorized principal in one flat
list ordered by how the authorizer consults them, which buried the
database's own operators — the people the blocked user should actually
ping — at the bottom. Group the list the way the review-required comment
already does: the database's operators lead in their own section, with the
deployment and repo admins following as an explicit fallback. A principal
configured in both groups lists only as an operator. Principals still
render as inline code, never @-mentions.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 7, 2026 19:02

Copilot AI 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.

Pull request overview

Improves the UX of PR command authorization rejection comments by surfacing the target database’s operators first (so the requester knows who to ask), while still listing broader admins as an explicit fallback and avoiding @-mentions.

Changes:

  • Split the authorized-principals lookup into OperatorPrincipals vs OtherPrincipals, with cross-group dedupe.
  • Update the rejection comment renderer (and preview) to lead with an “Operators of <db>” section when operators exist.
  • Update and expand unit tests and the templates documentation example to reflect the new ordering/format.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
TEMPLATES.md Updates the rendered example of the “not authorized” comment to show operators-first formatting.
pkg/webhook/templates/preview.go Updates preview data to pass operator vs other principal groups.
pkg/webhook/templates/apply_commands.go Renders operators-first rejection comments; introduces writePrincipalList helper.
pkg/webhook/templates/apply_commands_test.go Adjusts tests to validate operators-first sections and ordering.
pkg/webhook/actor_authorization.go Passes operator vs other principal groups into the rejection comment.
pkg/api/actor_authorization.go Changes config API to return (operators, others) with dedupe behavior.
pkg/api/actor_authorization_test.go Updates tests to validate grouping, ordering, normalization, and dedupe semantics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread TEMPLATES.md
Comment thread pkg/webhook/templates/apply_commands.go Outdated
The rejection comment's closing guidance offered "request membership in
one of the teams above" even when every listed principal was a plain
user login with no team to join.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aparajon
aparajon marked this pull request as ready for review August 7, 2026 20:31

@morgo morgo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Approving on Morgan's behalf (automated review, escalation rules apply).

Verified the authorization decision path is untouched — PRCommandAuthorizedPrincipals feeds only the rejection comment, and the enforcement gate above it is unchanged. The split into (operators, others) keeps the same normalization and cross-group dedup (shared seen map), with operators processed first so a dual-configured principal lists once, as an operator. Template handles all three shapes (both groups / others only / neither) with the old fallback sentence preserved, keeps the no-@-mentions invariant, and only offers "request membership" when a team is actually listed — all tested, including ordering assertions.

One cosmetic side effect worth knowing: dedup priority flipping means the operator-configured casing of a principal now wins over the admin-configured casing in the rendered list (the normalization test captures this). Display-only.

@aparajon
aparajon merged commit da09d48 into main Aug 13, 2026
32 checks passed
@aparajon
aparajon deleted the armand/not-auth-operator-order branch August 13, 2026 07:22
Kiran01bm added a commit that referenced this pull request Aug 13, 2026
…re-public-13n

* origin/main:
  refactor(storage): portable lease-guarded joined DML for the operation store (#1011)
  fix(github): align lint warnings formatting with issues and fold long lists (#959)
  fix(github): lead with the database's operators on command-rejection comments (#960)
  docs: regenerate stale tables of contents (#968)
  fix(engine): heartbeat the row a local drive actually owns (#915)
  fix(github): scope auto-plan to the schema a pull request proposes (#1016)
  fix(vitess): dispatch task-less VSchema-only work operations over gRPC (#961)
  feat(storage): add PostgreSQL dialect nucleus to the shared store core (#1010)
  refactor(storage): render joined UPDATEs through the dialect (#1009)
  fix(planetscale): hold the cutover when the operator defers it (#978)
  fix(observability): make telemetry resource schema-tolerant (#1014)
  feat(postgres): implement declarative planning via pg-sprite diffplan (#1008)
  refactor(storage): make remaining sqlstore SQL dialect-portable (#1007)
  test(e2e): deflake multi-table stop/start resume and MySQL cold starts (#1005)
  ci: verify golangci config against a vendored schema (#997)
  feat(api): fail-closed verdict gating for postgres plans (#1004)
  feat(tern): route postgres targets to the postgres engine (#1003)
  feat(storage): stamp remaining sqlstore timestamps explicitly (#1006)
Kiran01bm added a commit that referenced this pull request Aug 13, 2026
…lect-factory-14b

* origin/main:
  feat(github): flag destructive changes to tables another open PR owns (#1017)
  feat(storage): add public postgresstore constructor (#1012)
  refactor(storage): portable lease-guarded joined DML for the operation store (#1011)
  fix(github): align lint warnings formatting with issues and fold long lists (#959)
  fix(github): lead with the database's operators on command-rejection comments (#960)
  docs: regenerate stale tables of contents (#968)
  fix(engine): heartbeat the row a local drive actually owns (#915)
  fix(github): scope auto-plan to the schema a pull request proposes (#1016)
  fix(vitess): dispatch task-less VSchema-only work operations over gRPC (#961)
  feat(storage): add PostgreSQL dialect nucleus to the shared store core (#1010)
  refactor(storage): render joined UPDATEs through the dialect (#1009)
  fix(planetscale): hold the cutover when the operator defers it (#978)
  fix(observability): make telemetry resource schema-tolerant (#1014)
  feat(postgres): implement declarative planning via pg-sprite diffplan (#1008)
  refactor(storage): make remaining sqlstore SQL dialect-portable (#1007)
  test(e2e): deflake multi-table stop/start resume and MySQL cold starts (#1005)
  ci: verify golangci config against a vendored schema (#997)
  feat(api): fail-closed verdict gating for postgres plans (#1004)
  feat(tern): route postgres targets to the postgres engine (#1003)
  feat(storage): stamp remaining sqlstore timestamps explicitly (#1006)
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.

3 participants