Skip to content

fix(github): merge when the viewer can bypass branch protection - #47

Merged
nplez1 merged 1 commit into
nplez1/mainfrom
nplez1/Mergeability
Oct 1, 2026
Merged

nplez1 merged 1 commit into
nplez1/mainfrom
nplez1/Mergeability

Conversation

@nplez1

@nplez1 nplez1 commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner
Files Added Deleted Net
Test 13 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​764 $\color{#cf222e}{\Huge{\mathbf{−}}}$​17 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​747
Prod 36 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​785 $\color{#cf222e}{\Huge{\mathbf{−}}}$​139 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​646

ELI5

Orca refused to merge any pull request GitHub marked "review required" — even for people whose GitHub account is allowed to override that rule (a repository administrator, or anyone a branch ruleset lists as a bypass actor). Orca was reading "a review is required" as "this merge is impossible for everyone". It now also asks GitHub "may this user ignore that rule?", and when GitHub says yes it offers the merge, after a confirmation that spells out what it will skip.

What Changed

Before. An approval-gated pull request showed "Approval required" with the merge button disabled (or offering only "Enable auto-merge"), and merging from any surface — workspace sidebar, PR page, land-pull-request dialog, tasks — failed with "This pull request requires review approval before it can be merged." Meanwhile GitHub's own merge box let the same user merge it, because their account may bypass the rule.

After. That pull request keeps the honest "Approval required" label, and its merge button is enabled. Clicking it asks:

Bypass branch protection?
This pull request does not meet its branch protection rules. Merging will skip those rules using your admin privileges.
Cancel Merge anyway

Confirming merges it with admin privileges through GitHub; cancelling does nothing. A user who genuinely cannot bypass sees exactly what they see today: "Approval required", merge disabled.

Mechanism. PullRequest.viewerCanMergeAsAdmin is GitHub's viewer-scoped answer to "can I bypass branch protection and merge this immediately". The host fetches it — one gh api graphql request, only for a pull request whose review gate is unmet, cached for 60s with in-flight dedupe and through the existing rate-limit guard — and carries it to the presentations with the rest of the PR metadata. A new shared rule (src/shared/github/pull-request-admin-bypass.ts) decides what the flag may be used for; the presenters use it to enable the merge and require a confirmation, and the host applies the same rule before appending --admin to gh pr merge, so a caller cannot talk it into a bypass the UI would not have shown.

Why

  • Reading reviewDecision as a verdict is the bug. It is a fact about the pull request ("a review is required"), not a statement about the person asking. GitHub answers the viewer-scoped half separately, and only it knows about ruleset bypass lists and administrator waivers.
  • Alternative: trust mergeStateStatus alone (GitHub's merge box is viewer-aware, so an open box already means "you may merge"). Rejected on its own: it cannot distinguish a box that is open because nothing is required from one that is open because the viewer may waive something, so the app could neither name what it was about to skip nor refuse the cases where a bypass would reach further than the user agreed to.
  • Alternative: attempt the merge and surface GitHub's error. Smallest change, but it throws away the pre-existing clear verdict and turns every non-administrator's click into a failed request.
  • Nothing bypasses silently, mirroring GitHub's own checkbox. The bypass flag is sent only by a surface that confirmed with the user, and only when the host re-checks that GitHub reports the privilege, so the RPC parameter is intent, not proof.
  • The bypass is withheld where --admin would do more than the confirmation says: when the base branch requires a merge queue or its status is unknown (the flag also skips the queue — see gh's shouldAddToMergeQueue: mergeQueueRequired && !useAdmin), and while GitHub reports the merge box BLOCKED, which can mean required checks are failing.

Linked Issue

No issue filed — reported directly from the app. Say the word and I will open one and relink.

Visual Proof

Not captured, and here is exactly why rather than a shrug:

  • The original live reproduction (an approval-gated PR in this repo) was merged while the work was in progress, so no rendered before existed when I finished.
  • I then launched a background dev build (ORCA_BACKGROUND_LAUNCH=1 node config/scripts/run-electron-vite-dev.mjs --remote-debugging-port=9333) to capture the after. The instance started and bound the debugger port, but never exposed a renderer CDP target and stopped answering the DevTools HTTP endpoint, so no screenshot could be taken; I terminated that instance (the only dev bundle I started) and left the user's running app alone.
  • Easy to capture now: this PR is itself the reproduction. It is reviewDecision: REVIEW_REQUIRED, mergeStateStatus: UNSTABLE, mergeable: MERGEABLE, viewerCanMergeAsAdmin: true for the repo owner — so a build from this branch shows the fixed sidebar for this very workspace (merge button enabled, labelled "Squash and merge", confirmation on click), and a build from nplez1/main shows the old one (disabled, "Approval required").

What stands in for the screenshot:

  • HostedReviewActions.admin-bypass.test.tsx asserts the user-visible outcome: the primary button is enabled and relabelled "Squash and merge" for a bypassable approval-gated PR, stays disabled and labelled "Approval required" when the viewer cannot bypass, and never asks for a bypass on a stacked PR.
  • github-pr-merge-state.test.ts asserts the label stays "Approval required" while the merge becomes reachable.

Testing

  • New tests: 7 host-process cases around mergePR (bypass granted → --admin; denied, unanswered, unrequested, unknown merge queue, and BLOCKED → still blocked with no merge call), the RPC forwarding of the confirmed flag, the shared rule's full truth table, the presentation rules, the sidebar component contract, the mobile predicate, and the mobile engine's flag forwarding.
  • Suites: desktop pnpm test over src/shared, src/main/github, src/main/runtime, and src/renderer/src/components — 4552 files / 42,749 tests pass. Mobile pnpm test over src/components/pr-sidebar, src/session, src/tasks — 255 files / 2504 tests pass.
  • Gates: pnpm tc, oxlint on every changed file, check:code-quality:changed, verify:rpc-params-catalog, and all three localization verifications.
  • Not run: full pnpm lint (the slower repo-wide audits) and pnpm build. The rendered in-app check was attempted and did not complete — see Visual Proof. No live GitHub Enterprise or SSH-host run, so the probe is untested on those; it returns "unknown" on failure there, which reads as "no bypass".
  • Verified against the real API: viewerCanMergeAsAdmin is a real field, and its pairing with mergeStateStatus was confirmed on public repositories (a viewer without the privilege sees BLOCKED for the same unmet review that a bypassing viewer sees as UNSTABLE).

Review

Known scope limits, deliberate rather than overlooked:

  • Tasks list rows (desktop task page and the mobile tasks board) cannot offer the bypass: those list payloads deliberately omit review and merge metadata because, as the existing comments put it, that "fans out into expensive per-row GraphQL". Their item detail fetch does carry it. Merging from a list row still fails with the pre-existing review error; the sidebar and PR page work.
  • Stacked pull requests keep their existing per-entry review gating. The sidebar now refuses to offer a bypass for them rather than promising one the stack merge would not perform.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Security: the bypass requires an explicit caller flag and GitHub's own viewer-scoped confirmation, and the host re-checks it; merging remains GitHub's decision, so a forged flag cannot merge anything the account is not already allowed to merge. Cross-platform: the change is platform-neutral (no paths, no keybindings, no shell selection) and the probe uses the existing gh execution path, so local, WSL, SSH and runtime-host repos all take it. Backwards compatibility: the new RPC parameter is optional, so older clients behave exactly as before and older hosts strip it. Performance: one extra GraphQL request, only for pull requests whose review gate is unmet, cached 60s and budget-guarded.

Checklist

  • This PR is small and focused — focused in purpose, but 47 files: the flag has to reach four merge surfaces on two platforms, and the merge gate lives behind IPC and RPC.
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots attached — see Visual Proof for why not, and what stands in for them
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, pnpm build pass — typecheck, oxlint, changed-code quality, localization and RPC-catalog checks, and the affected test suites pass; full pnpm lint and pnpm build were not run locally (CI covers them)

`reviewDecision` reports that a review is required; it does not say whether the
requirement gates the person looking at the pull request. A ruleset bypass actor, or
an administrator waiving classic branch protection, therefore saw "Approval required"
with the merge refused, while GitHub's own merge box let them merge.

Orca now asks GitHub the viewer-scoped question (`PullRequest.viewerCanMergeAsAdmin`),
only for a pull request whose review gate is unmet, and offers the merge when GitHub
says yes. Nothing bypasses silently: the surface must confirm, and the host only adds
`gh pr merge --admin` when the flag was asked for *and* GitHub confirms the privilege.
The bypass is withheld whenever a merge queue is required or its status is unknown,
because `--admin` also skips the queue, and while GitHub reports the merge box blocked
- which can mean required checks are failing, something the confirmation never covers.

The rule lives in one shared module so the host and both presentations cannot drift.
@nplez1
nplez1 force-pushed the nplez1/Mergeability branch from 433fed7 to e76209e Compare October 1, 2026 08:41
@nplez1
nplez1 merged this pull request into nplez1/main Oct 1, 2026
26 checks passed
nplez1 added a commit that referenced this pull request Oct 1, 2026
`reviewDecision` reports that a review is required; it does not say whether the
requirement gates the person looking at the pull request. A ruleset bypass actor, or
an administrator waiving classic branch protection, therefore saw "Approval required"
with the merge refused, while GitHub's own merge box let them merge.

Orca now asks GitHub the viewer-scoped question (`PullRequest.viewerCanMergeAsAdmin`),
only for a pull request whose review gate is unmet, and offers the merge when GitHub
says yes. Nothing bypasses silently: the surface must confirm, and the host only adds
`gh pr merge --admin` when the flag was asked for *and* GitHub confirms the privilege.
The bypass is withheld whenever a merge queue is required or its status is unknown,
because `--admin` also skips the queue, and while GitHub reports the merge box blocked
- which can mean required checks are failing, something the confirmation never covers.

The rule lives in one shared module so the host and both presentations cannot drift.
nplez1 added a commit that referenced this pull request Oct 6, 2026
`reviewDecision` reports that a review is required; it does not say whether the
requirement gates the person looking at the pull request. A ruleset bypass actor, or
an administrator waiving classic branch protection, therefore saw "Approval required"
with the merge refused, while GitHub's own merge box let them merge.

Orca now asks GitHub the viewer-scoped question (`PullRequest.viewerCanMergeAsAdmin`),
only for a pull request whose review gate is unmet, and offers the merge when GitHub
says yes. Nothing bypasses silently: the surface must confirm, and the host only adds
`gh pr merge --admin` when the flag was asked for *and* GitHub confirms the privilege.
The bypass is withheld whenever a merge queue is required or its status is unknown,
because `--admin` also skips the queue, and while GitHub reports the merge box blocked
- which can mean required checks are failing, something the confirmation never covers.

The rule lives in one shared module so the host and both presentations cannot drift.
nplez1 pushed a commit that referenced this pull request Oct 19, 2026
…iew off the deprecated owner resolver (stablyai#30562)

* refactor(renderer): fold browser creation, file actions and doc-preview callers off the deprecated owner resolver

The creation-action policy and browser creation (#3, #48) fold together, so the provider the
policy offers is the one creation uses. Browser panes, page lifecycle and conversion, tab
duplication, the explorer tree pane, doc-preview actions and editor path moves (#21, #43, #45-#47,
#49, #50) read runtimeEnvironmentIdWithLegacyFallbackUnlessTwin. File-explorer writes on a folder
(#51) take the owner from the rows alone, never from the focused server. Deprecated importers
16 -> 6.

* refactor(renderer): fold file preview off the deprecated owner resolver; Open in Orca Browser refuses a twin

A project two hosts publish reads as no server for the doc-preview plan, but the file:// path now
refuses it with its own message instead of opening this machine's copy (#7). Deprecated importers
6 -> 5.

* fix(renderer): a twin's file preview and open-externally refuse instead of opening this machine's copy

Codex P2 on the F7 folds: a folder twin read as no server, so the preview plan built a local
file:// tab and open-externally handed the path to this machine's OS. Both now check the owner
for ambiguity first. Also fix two test fixtures that broke the renderer typecheck.

* refactor(renderer): move the file:// open target out of file-preview.ts

file-preview.ts crossed its line limit after main grew it; the file:// target, its type and its two
refusal messages move to workspace-file-browser-open-target.ts, which they never depended on
file-preview for.

* test: mock the replacement resolver in the quick-action availability suite (Codex P2)
@nplez1
nplez1 deleted the nplez1/Mergeability branch October 24, 2026 03:23
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