Repository navigation
fix(ssh): keep existing hosts' orca CLI control off after the on-by-default flip - #29949
Conversation
…on-by-default flip Builds before the flip saved an explicit off as an absent value, so absent=on silently turned on hosts the user had turned off. On the first load after this upgrade, pin every saved host's absent switch to an explicit false and record a one-shot marker in settings so hosts added later keep the new default.
…t the call site too
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 7 files. (7 skipped: 7… | 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 change: preserving disabled CLI control for existing SSH hosts after the default changes to enabled. Description check The description is detailed and covers the user impact, implementation, rationale, alternatives, testing, known limitations, compatibility, and checklist items. It explains that the linked issue is no… 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 detailed and covers the user impact, implementation, rationale, alternatives, testing, known limitations, compatibility, and checklist items. It explains that the linked issue is not applicable and documents the untested managed orcad deployment path.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 7 files. (7 skipped: 7 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- 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.
ELI5
Older Orca versions saved "off" for the SSH host switch "Allow this host's orca CLI to control Orca" by leaving it unset. #29856 made unset mean "on", so anyone who had turned it off would have it switched back on without being asked. This PR turns every host that was already saved before the upgrade off, once. Hosts you add afterwards still start with the switch on.
What Changed
isRemoteCliControlAllowedreads that as on, so the host'sorcaCLI can drive this desktop's Orca browser after the upgrade. Live run on Linux: the desktop browser bridge accepted both the old "off" host and a new host.allowRemoteCliControl: false. Explicittrueandfalsevalues stay as they are. A one-time marker,settings.sshRemoteCliControlExistingHostsMigrated, is saved in the same profile so this runs once. Fresh profiles start with the marker already set. Hosts added afterwards through Settings or either sidebar add-host dialog are still saved with an explicittrue, which fix(ssh): turn a host's orca CLI control on by default and stop resetting saved choices #29856 already does.migrateExistingHostsRemoteCliControl(src/main/persistence/leasing-ssh-ptys/ssh-remote-cli-control-migration.ts), called fromnormalizeLoadedProfileState. That is the only load path for the profile Store. The desktop and the server (orca serve/ orcad, which keeps its SSH targets in the same Store) both use it, so both stores are migrated.Why
settings. Every shipped build spreads unknown settings keys through load and save, so the marker survives a downgrade. If a user downgrades and then upgrades again, the migration does not run a second time over hosts they added on the newer version. It sits in the same profile document assshTargets, so it is saved together with the pinned values. Adding a new top-level persisted key would also mean new SQLite domain, WSL-move and census work.falseas off, as before. An older client talking to a migrated server is sentallowRemoteCliControl: false, which it reads as off. Nothing an older client relies on flips to on.isRemoteCliControlAllowedis unchanged). After the migration, a missing value is written by new-build paths that don't pass the switch (SSH config import, managed-server provisioning). Those are hosts added after this release, which the product decision says start on. It can also come from an older build after a downgrade. The server-side rule from fix(ssh): turn a host's orca CLI control on by default and stop resetting saved choices #29856 (a missing value from an older client over RPC is saved as off) is a different boundary: there an older client submitted a form that showed the switch off. That rule still applies, so hosts an older client adds through a new server are stored as off.Linked Issue
N/A (user decision on remote CLI control default, follow-up to #29856)
Visual Proof
N/A. This is a load-time data migration plus a one-phrase help-text change; the toggle UI is unchanged.
Testing
New
src/main/persistence-ssh-remote-cli-control-migration.test.tsdrives the real Store with its SQLite profile authority. These fail on main:falseand is saved that way;trueandfalseare kept;trueacross a restart, while the pre-existing host staysfalse.A third test covers the downgrade case: a profile that already has the marker keeps a missing value as is.
Focused suites pass:
src/main/persistence/loading-store,leasing-ssh-ptys,persistence-settings-update,persistence-ssh-targets-and-pane-keys,ssh-connection-store,desktop-browser-bridge-wiring,ssh-target-management,src/shared/wsl-move, the ssh settings renderer tests andsrc/renderer/src/i18n.pnpm tcpasses. I ran the static-analysis job steps locally and they passed (lint, code-quality plugins, changed-code gate, ratchets, localization, generated catalogs). The one exception is thetypecheck:cibun-compiler wrapper, which fails on this machine with "Script not found check".pnpm tccovers the same projects.Live on omarchy (Linux), isolated
/tmpHOME and profile dir:Both processes ran the real
listDesktopBrowserBridgeCandidates. Managed-server fencing and environment resolution were stubbed so both hosts count as connected managed hosts.old:false,new:true bridge=new, the same after the restart.old:undefined,new:true bridge=old,new.orca tab createround-trip. The Docker-based e2e can't run on omarchy because there is no Docker.I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Codex (gpt-6.1-sol, high) review 1 found one P2: an unchecked type assertion in the new test, which failed the changed-code gate. Fixed. Re-reviews found nothing further.
Agent skill upstream boundary
Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)