Repository navigation
refactor(browser): one page gate and one owner for a host command's hold - #30652
Conversation
The revocation gate, the input cancel list and the hold's lifecycle each lived in two places. Sinks now call one gate, held input is recorded at the CDP send (and the one native key path), and runWithRemoteCliPageAdmission owns retire-on-revoke, release and error conversion.
c644234 to
36957bc
Compare
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 61.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 10 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main refactor: consolidating the page gate and host-command hold ownership. Description check The description is complete and directly matches the changes. It covers the user-facing behavior, implementation details, rationale, edge cases, testing, scope, and checklist. The Linked Issue section… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is complete and directly matches the changes. It covers the user-facing behavior, implementation details, rationale, edge cases, testing, scope, and checklist. The Linked Issue section states that no GitHub issue exists, which is a minor deviation from the template but does not make the otherwise detailed description incomplete.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
66c8e053-9183-410d-9395-5a4ea3a2b791
📒 Files selected for processing: 11
src/main/browser/agent-browser-bridge-host-revocation.test.tssrc/main/browser/agent-browser-bridge-queue.tssrc/main/browser/agent-input-injection.tssrc/main/browser/cdp-debugger-channel.tssrc/main/browser/cdp-page-input-commands.tssrc/main/browser/cdp-ws-proxy-host-admission.test.tssrc/main/browser/cdp-ws-proxy.tssrc/main/browser/guest-cdp-command.tssrc/main/browser/guest-direct-key-input.tssrc/main/browser/remote-cli-page-admission.tssrc/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts
💤 Files with no reviewable changes: 1
- src/main/browser/cdp-debugger-channel.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| let retiring: Promise<void> | undefined | ||
| const retire = (): Promise<void> | undefined => | ||
| hooks.retire && (retiring ??= runOutsideRemoteCliBrowserScope(hooks.retire)) | ||
| const onRevoked = (): void => void retire() | ||
| hooks.revoked?.addEventListener('abort', onRevoked, { once: true }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '35,165p' src/main/browser/remote-cli-page-admission.ts
sed -n '85,165p' src/main/browser/agent-browser-bridge-queue.tsRepository: stablyai/orca
Length of output: 7305
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- admission references and owners ---'
rg -n -F --glob '*.ts' -e 'admissions.delete' -e 'runWithRemoteCliPageAdmission' -e 'assertRemoteCliPageAccess' -e 'trackRemoteCliInput' src/main/browser
printf '%s\n' '--- queue processing and revocation/cleanup references ---'
rg -n -F --glob '*.ts' -e 'destroySession(' -e 'onRevoked' -e 'revoked.abort' -e 'releaseHeldPageInput' -e 'processQueue(' src/main/browser
printf '%s\n' '--- lifecycle implementation ---'
sed -n '150,235p' src/main/browser/agent-browser-bridge-lifecycle.ts
printf '%s\n' '--- queue owner and processing ---'
sed -n '1,240p' src/main/browser/agent-browser-bridge-queue.ts
printf '%s\n' '--- admission module complete ---'
sed -n '1,190p' src/main/browser/remote-cli-page-admission.tsRepository: stablyai/orca
Length of output: 22185
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- revocation and remote scope ---'
sed -n '1,125p' src/main/browser/desktop-browser-bridge/desktop-browser-bridge-connection.ts
sed -n '1,125p' src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts
printf '%s\n' '--- page input delivery and release ---'
sed -n '1,215p' src/main/browser/guest-direct-key-input.ts
printf '%s\n' '--- proxy admission boundary ---'
sed -n '150,215p' src/main/browser/cdp-ws-proxy.ts
printf '%s\n' '--- guest command and direct input paths ---'
sed -n '1,75p' src/main/browser/guest-cdp-command.ts
sed -n '1,175p' src/main/browser/guest-direct-key-input.tsRepository: stablyai/orca
Length of output: 27735
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- proxy admission caller ---'
rg -n -F --glob '*.ts' -e 'runUnderRemoteCliPageAdmission' -e 'runUnderRemoteCliPageAdmission(' src
printf '%s\n' '--- proxy dispatch context ---'
rg -n -F --glob '*.ts' -e 'runUnderRemoteCliPageAdmission' src/main/browser/cdp-ws-proxy.ts src/main/browser || test "$?" -eq 1
sed -n '1,170p' src/main/browser/cdp-ws-proxy.ts
sed -n '215,360p' src/main/browser/cdp-ws-proxy.ts
printf '%s\n' '--- all admission map writes and input tracking callers ---'
rg -n -F --glob '*.ts' -e 'admissions.set' -e 'trackRemoteCliInput(' -e 'runUnderRemoteCliPageAdmission(' src/mainRepository: stablyai/orca
Length of output: 10573
Handle retirement rejection when creating retiring.
If hooks.retire rejects, the finally block exits at await retire() before it deletes the admission or calls releaseAll. processQueue only rejects the command, so no other owner clears the stale admission or releases its held input. A delivered key can therefore remain held. A proxy message that reaches CdpWsProxy can also run under the stale admission and be denied. Direct desktop-owned page gates do not use this map, so the denial is specific to work routed through the stale proxy admission.
🛡️ Suggested fix
--- "a/src/main/browser/remote-cli-page-admission.ts"
+++ "b/src/main/browser/remote-cli-page-admission.ts"
@@ -44,9 +44,12 @@
hooks: PageHoldHooks = {}
): Promise<T> {
const admission: Admission = { admit, held: new Map() }
let retiring: Promise<void> | undefined
const retire = (): Promise<void> | undefined =>
- hooks.retire && (retiring ??= runOutsideRemoteCliBrowserScope(hooks.retire))
+ hooks.retire &&
+ (retiring ??= runOutsideRemoteCliBrowserScope(hooks.retire).catch(() => {
+ // Best effort: the helper may already be gone; the revocation is what gets reported.
+ }))
const onRevoked = (): void => void retire()
hooks.revoked?.addEventListener('abort', onRevoked, { once: true })
admissions.set(webContentsId, admission)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let retiring: Promise<void> | undefined | |
| const retire = (): Promise<void> | undefined => | |
| hooks.retire && (retiring ??= runOutsideRemoteCliBrowserScope(hooks.retire)) | |
| const onRevoked = (): void => void retire() | |
| hooks.revoked?.addEventListener('abort', onRevoked, { once: true }) | |
| let retiring: Promise<void> | undefined | |
| const retire = (): Promise<void> | undefined => | |
| hooks.retire && | |
| (retiring ??= runOutsideRemoteCliBrowserScope(hooks.retire).catch(() => { | |
| // Best effort: the helper may already be gone; the revocation is what gets reported. | |
| })) | |
| const onRevoked = (): void => void retire() | |
| hooks.revoked?.addEventListener('abort', onRevoked, { once: true }) |
ELI5
#30630 makes a cancelled or revoked SSH host's browser command stop on the desktop. To get there it grew two separate "is this host still allowed?" checks and two copies of the "what does this command still have pressed?" bookkeeping. The steps of a command's hold on a page were also split between two files. This PR folds each of those into one place. Behaviour doesn't change.
What Changed
Before:
{ method, params }argument just for that.After:
assertRemoteCliPageAccesschecks the host command's scope plus the hold that a proxy message arrived under. It runs at the guest CDP send, native input and edit commands, and the proxy's intake. The debugger channel's own check is gone, because every one of its sends reaches the CDP send.trackRemoteCliInput(id, method, params, deliver)records a press before it is delivered and a release only once it lands. It is called at the CDP send, and once on the single native widget-key path, which skips CDP.runWithRemoteCliPageAdmission(id, admit, run, { revoked, retire, releaseHeld })runs the whole sequence: hold the page; retire the helper as soon as the host is revoked; when the command settles, end the hold; replay held input off the page; report the revocation. The queue only supplies the hooks.captureRemoteCliBrowserScope()now also returns the revoked signal, which replacescaptureRemoteCliBrowserRevokedSignal.Production code is 45 lines smaller (+120 / −165). Test changes are limited to two probe call sites, which now use the renamed APIs. No wire change.
Edge cases that change:
Why
#30630's design is the right one:
cancelevent, gated on a capability;Its review rounds went to finding sinks and edge cases, and each fix landed where the bug was found. This PR keeps the model and removes the duplicate checks and bookkeeping that built up, so the next sink only has to call one gate and one recorder. A larger rewrite was considered: putting proxy messages into the host's own scope instead of a separate hold context. It was rejected because long-lived timers created while handling a message would then inherit the host's scope and refuse the desktop's later sends. That was the same class of leak #30630 fixed for the proxy itself.
ref-oss check: the cancel table, per-request token and "replay the input cancel list" cleanup match the standard remote-RPC and input-automation models. The approach didn't change. This PR only consolidates.
Linked Issue
Follow-up to #30630 (SSH hardening review 4, finding #6). No GitHub issue.
Visual Proof
N/A. No UI changes.
Testing
src/main/browser,src/main/runtime/desktop-browser-bridgeand the bridge RPC/IPC tests, single-worker: 242 files, 2299 tests passed.pnpm tc:node.Every step of the "static analysis and typecheck" job, run locally, including the ratchets.
Verified in unit tests only. Not exercised live against a real SSH host.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Agent skill upstream boundary
Notes
nwparker/browser-bridge-cancel). It will be rebased onto main once fix(ssh): a cancelled or revoked host's browser command stops on the desktop #30630 merges.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)