Skip to content

refactor(legacy-relay): one ask-wait clamp and no needless exports - #29835

Merged
nwparker merged 1 commit into
mainfrom
nwparker/cleanup-legacy-relay-exports
Oct 16, 2026
Merged

nwparker merged 1 commit into
mainfrom
nwparker/cleanup-legacy-relay-exports

Conversation

@nwparker

@nwparker nwparker commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 0 0 0 0
Prod 6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​7 $\color{#cf222e}{\Huge{\mathbf{−}}}$​14 $\color{#cf222e}{\Huge{\mathbf{−}}}$​7

ELI5

Deletes a timeout helper that was an exact copy of another one, hides three functions that nothing outside their own file uses, and corrects a setting's description. Nothing changes for users.

What Changed

  • legacy-relay-cli-call-limits.ts had two clamps for a relay-era CLI wait: cappedWait and optionalAskTimeout. For undefined, both return undefined. For any number, both return min(max(0, t), ORCHESTRATION_ASK_MAX_TIMEOUT_MS) (clampOrchestrationAskTimeoutMs only differs when given undefined, and that case never reached it). The ask call now uses cappedWait, and optionalAskTimeout is removed.
  • Three functions lose their export because no other file imports them: createUnservedSshPaneTransport, listDesktopBrowserBridgeCandidates and isBrowserWorkspaceOwnedByEnvironment.
  • The doc comment on the per-host allowRemoteCliControl switch (src/shared/ssh-types.ts) said it only gates the desktop browser bridge. Since fix(ssh): relay-era terminals' orca orchestration commands work again, behind the per-host switch (STA-10765) #29571 it also gates relay-era terminals' orchestration commands, and the comment now says so. This is a comment change only; the switch's value and its consent stamp are untouched.

Why

This is part of a cleanup pass over today's SSH merges. Two copies of the same clamp let the next edit change one and not the other.

Security-adjacent files: the relay-era CLI bridge (#29571/#29596) and the per-host switch. The caller-scope check (legacy-relay-cli-caller.ts) is untouched, and so is the concurrency cap. The ask timeout bound is the same number for every input. Existing tests that cover these, all passing unchanged: legacy-relay-cli-call-limits.test.ts covers the clamp (MAX_SAFE_INTEGER becomes 1,800,000, -5 becomes 0, undefined stays undefined), and the legacy-relay-cli-* and legacy-relay-target-bridge suites cover the rest.

Linked Issue

N/A (maintainer cleanup).

Visual Proof

N/A: no UI or behaviour change.

Testing

  • pnpm test --maxWorkers=1 src/main/ssh src/main/ipc src/main/browser src/renderer/src/hooks src/renderer/src/components/terminal-pane/pty-connection/unserved*: all passed (1192 files).

  • pnpm tc, plus every step of the static-analysis job run locally: passed.

  • Codex review (gpt-6.1-sol, high): no findings.

  • Nothing was verified live; this is a pure refactor.

  • Automated tests added/updated, or explained why not below

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

No wire change. Older relays send the same flags and get the same bounds.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • N/A for visuals with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered

optionalAskTimeout computed exactly what cappedWait does (an undefined timeout stays undefined;
anything else clamps to 0..ORCHESTRATION_ASK_MAX_TIMEOUT_MS), so the ask call uses cappedWait.
Three functions used only inside their own file lose their export, and the per-host switch's
doc names the orchestration commands it has gated since #29571.

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 54247581-73d2-4a50-86c9-39c3e6bd6bfd
📥 Commits

Reviewing files that changed from the base of the PR and between 95d41af and f7f0e82.

📒 Files selected for processing: 6
  • src/main/ipc/desktop-browser-bridge-wiring.ts
  • src/main/ssh/legacy-relay-bridge/legacy-relay-cli-call-limits.ts
  • src/main/ssh/legacy-relay-bridge/legacy-relay-cli-server.ts
  • src/renderer/src/components/terminal-pane/pty-connection/unserved-ssh-pane-connect.ts
  • src/renderer/src/hooks/ipc-events/browser-request-workspace-owner.ts
  • src/shared/ssh-types.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

📝 Walkthrough

The orchestration ask timeout call site now uses cappedWait, and the optionalAskTimeout helper was removed. Three functions are now module-local instead of exported. The allowRemoteCliControl comment now includes relay-era terminal orchestration commands in its scope.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to f7f0e

This is a cleanup with no expected behavior change. The ask timeout clamp is consolidated, three unused exports are removed, and a comment is clarified. No remaining merge-blocking risk was identified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main changes: consolidating the ask-wait clamp and removing unnecessary exports.
Description check Passed The description is complete and aligned with the template. It explains the changes, rationale, testing, lack of visual impact, compatibility considerations, and maintainer exception for the linked iss…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description is complete and aligned with the template. It explains the changes, rationale, testing, lack of visual impact, compatibility considerations, and maintainer exception for the linked issue.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • 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.

❤️ Share

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

@nwparker
nwparker merged commit e0e2001 into main Oct 16, 2026
86 of 88 checks passed
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