Skip to content

refactor(github-app): shared real-permission scaffold for action commands (#824) - #839

Merged
JSONbored merged 2 commits into
mainfrom
feat/authorized-pr-action-scaffold
Jun 17, 2026
Merged

refactor(github-app): shared real-permission scaffold for action commands (#824)#839
JSONbored merged 2 commits into
mainfrom
feat/authorized-pr-action-scaffold

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Closes #824. Agent-layer write-safety prerequisite (the structural complement to #788).

Why

maybeProcessGateOverrideCommand and maybeProcessPrPanelRetrigger each inlined the same security-critical authorization flow — resolve the actor's real repo permission → isAuthorizedCommandActor — plus the same advisory build. As #778/#769 add more action commands (auto-merge / close / request-changes), that duplication is a safety risk: each new command would have to independently get the real-permission check right, and one could silently fall back to the spoofable author_association (the #788 hazard).

What

Two helpers in processors.ts:

  • authorizePrActionActor — the single real-permission authorization gate for action commands. Resolves the actor's real repo permission via resolveRealRepoPermissionAssociation (never author_association), runs the optional miner detection, and returns the isAuthorizedCommandActor decision. Every action command authorizes through here, so no future command can bypass it.
  • buildAuthorizedPrActionAdvisory — the common load-repo-context + buildPullRequestAdvisory step.

Both action commands refactored onto them; each keeps its own trigger / skip / deny / success handling (those legitimately differ — distinct skip strings asserted in tests, distinct mutations). #778's write-commands build on authorizePrActionActor.

No behavior change

Pure refactor — the extracted code is identical to what was inlined. Full suite 1996 passed, 1 skipped (same as before), typecheck clean. The gate-override + panel-retrigger + #788 Q&A-auth tests all stay green.

Relates #788 (the auth-hardening this centralizes), #778 / #769 (write-actions that build on it), #538.

…ion commands (#824)

maybeProcessGateOverrideCommand and maybeProcessPrPanelRetrigger each
inlined the same security-critical authorization flow (resolve real repo
permission → isAuthorizedCommandActor) plus the same advisory build. As
#778/#769 add more action commands (auto-merge/close/request-changes),
that duplication is a safety risk — each new command must independently
get the real-permission check right (the #788 hazard).

Extract two helpers in processors.ts:
- authorizePrActionActor: the SINGLE real-permission authorization gate.
  Resolves the actor's real repo permission via
  resolveRealRepoPermissionAssociation (never the spoofable
  author_association), runs the optional miner detection, and returns the
  isAuthorizedCommandActor decision. Every action command authorizes
  through here, so no future command can fall back to a weaker check.
- buildAuthorizedPrActionAdvisory: the common load-repo-context +
  buildPullRequestAdvisory step.

Both action commands refactored onto them; each keeps its own
trigger/skip/deny/success handling. No behavior change — full suite green
(1996 passed).
@dosubot dosubot Bot added the size:L label Jun 17, 2026
@ghost

ghost commented Jun 17, 2026

Copy link
Copy Markdown

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal Result Evidence Action
Gate result ⚠️ Skipped #839 is no longer open. No action.

💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

@ghost ghost added the gittensory:reviewed label Jun 17, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@ghost

ghost commented Jun 17, 2026

Copy link
Copy Markdown

gittensory · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Changed files: src/queue/processors.ts, test/unit/queue.test.ts

Suggested action:Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR introduces a shared authorization scaffold for GitHub App action commands, consolidating permission checks and advisory building into two helper functions and updates the existing command processors to use them. It adds a comprehensive test for the miner‑detection path, preserving existing behavior and improving code reuse.

Suggestions

  • Add a unit test for the unauthorized branch of authorizePrActionActor to confirm proper handling of denied actors.
  • Consider exporting authorizePrActionActor and buildAuthorizedPrActionAdvisory if they might be useful elsewhere, or explicitly mark them as internal to avoid accidental external use.
  • Run the TypeScript linter to verify that the ReturnType<typeof isAuthorizedCommandActor> usage does not raise any type errors.

Worth double-checking

  • If future commands forget to pass needsMinerDetection: true when required, miner detection may be skipped.
  • The new helpers are internal; accidental export could expose internal logic.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR refactors duplicated permission and advisory logic into two shared helper functions (authorizePrActionActor and buildAuthorizedPrActionAdvisory) for GitHub action commands (gate-override, PR-panel retrigger, and future agent-layer writes). It replaces manual permission resolution and advisory building with calls to these helpers, preserving original behavior while centralizing the real-permission check (avoiding spoofable author_association). The added test validates the miner-detection path for retrigger commands. The change is clean, reduces duplication, and strengthens security by ensuring all action commands use the same real-permission gate.

No blocking issues spotted.

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.58%. Comparing base (207df8d) to head (16e3e14).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #839      +/-   ##
==========================================
+ Coverage   96.56%   96.58%   +0.01%     
==========================================
  Files          98       98              
  Lines       14197    14196       -1     
  Branches     5174     5172       -2     
==========================================
+ Hits        13710    13711       +1     
  Misses        105      105              
+ Partials      382      380       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…thorizePrActionActor (#824)

The miner-detection branch of the extracted authorizePrActionActor helper
(a confirmed-miner PR author retriggering their own panel via review-now)
was previously untested — every panel-retrigger test authorized via
maintainer. Adds an end-to-end test exercising it, restoring the patch
coverage the refactor surfaced.
@JSONbored JSONbored self-assigned this Jun 17, 2026
@JSONbored
JSONbored merged commit b2162f7 into main Jun 17, 2026
20 checks passed
@JSONbored
JSONbored deleted the feat/authorized-pr-action-scaffold branch June 17, 2026 20:36
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.

refactor(github-app): extract a shared withAuthorizedPrAction scaffold for action commands

1 participant