Repository navigation
refactor(runtime): owner transports, the creation default and a focus-read ratchet - #27275
Conversation
…-read ratchet Adds runtimeTargetForOwnerEnvironment, runtimeTargetForOwnerHostId and runtimeTargetForWorkspaceOwner so routed call sites can get a transport from the resource's owner, and defaultCreationHost as the one sanctioned reader of the Active Server setting for creation flows with no source row. The owner-routing ratchet gains a second per-file counter for reads of the setting (member reads, the focus-routing helpers and defaultCreationHost), so swapping a helper for a direct read can no longer lower a count. Baseline rows may carry a trailing note that survives --prune.
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Unrelated setting writes or method calls can make the new ratchet reject otherwise valid changes. The PR is mergeable with owner awareness, but these counting errors should be corrected. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 41.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 8 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Description check | |
The description covers the required change, rationale, testing, visual proof, review, notes, and checklist sections. However, the required Linked Issue section states "None", while the template requir… | Add the actual linked issue under "Linked Issue", or confirm that the author is a maintainer and document the applicable exception. |
✅ Passed checks (3 passed)
-
Check name Status Explanation 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. Title check The title clearly summarizes the main changes: owner transport helpers, the creation default, and the Active Server read ratchet.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 41.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 8 files. (1 skipped: 1 unsupported.)
Full details: Description check
-
Explanation
The description covers the required change, rationale, testing, visual proof, review, notes, and checklist sections. However, the required Linked Issue section states "None", while the template requires an actual issue unless the author is a maintainer.
- 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:
c208b45d-40a6-449d-8773-12105fcbb1cf
📒 Files selected for processing (9)
config/focus-setting-read-baseline.txtconfig/scripts/check-owner-routing-ratchet.mjsconfig/scripts/check-owner-routing-ratchet.test.mjssrc/renderer/src/lib/default-creation-host.test.tssrc/renderer/src/lib/default-creation-host.tssrc/renderer/src/lib/resolve-owner.test.tssrc/renderer/src/lib/resolve-owner.tssrc/renderer/src/runtime/runtime-client-target.test.tssrc/renderer/src/runtime/runtime-client-target.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| const SETTING_MEMBER_READ = | ||
| /\??\.\s*activeRuntimeEnvironmentId\b|(?<!(?:\b[A-Z][\w$]*|>)\s*)\[\s*['"]activeRuntimeEnvironmentId['"]\s*\]/g |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exclude simple property assignments from the read count.
If code adds settings.activeRuntimeEnvironmentId = id, SETTING_MEMBER_READ counts the assignment as a read. The value of the existing property is not read. The new count can then fail checkRatchet for a setting write. Exclude assignment-only targets, but keep read-modify-write operations such as += in the count. Add a direct-assignment case to the write exclusions test.
…and-listing them A hand list let a look-alike such as getAutomationListTarget replace getActiveRuntimeTarget and lower both ratchets. Every exported top-level function whose body reads the setting or calls a seed helper now counts as a reader, and nested destructuring reads count too.
Readers are keyed by defining file and resolved through imports and re-exports, so a same-named function elsewhere is not mistaken for one, and a function that returns a reader's result (a wrapper of a wrapper) is found too. Components are not readers, and a declaration ends at the next top-level statement.
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:
0f32c6f2-4ae0-4138-9578-f1851651eb37
📒 Files selected for processing (3)
config/focus-setting-read-baseline.txtconfig/scripts/check-owner-routing-ratchet.mjsconfig/scripts/check-owner-routing-ratchet.test.mjs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| // Exported reader names, for `ns.reader(…)` through a namespace or dynamic import. | ||
| const memberNames = new Set(seeds) | ||
| const addReader = (key) => { | ||
| readerKeys.add(key) | ||
| memberNames.add(key.slice(key.indexOf('#') + 1)) | ||
| } | ||
| for (const key of readerKeys) { | ||
| addReader(key) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Member-name matching ignores the call's receiver.
memberNames stores bare export names from every reader module. members.some((member) => memberNames.has(member)) in discoverFocusReaders and the MEMBER_NAME filter in countFocusSettingReads both match any .name( call, whatever the receiver is. An unrelated method call that has the same name as a discovered reader therefore counts as a read. One example is foo.ownerTarget(x), where ownerTarget is a reader in harness.ts. The extra count can mark unrelated exported functions as readers, and the ratchet can then fail with a false positive. The test at Lines 59-128 depends on module resolution to tell the two ownerTarget functions apart, but the member path skips that resolution. Restrict member matches to receivers that resolve to a namespace import or dynamic-import binding of a module that exports the reader.
1 participant
ELI5
Orca is moving every remote action to "ask the thing you're acting on where it lives" instead of "use whatever server is picked in the Active Server setting". This PR adds the small building blocks the remaining call sites will switch to, and a second lint counter that stops new code from reading the Active Server setting directly. Nothing the user sees changes yet.
What Changed
runtimeTargetForOwnerEnvironment(environmentId)gives the connection for an owner's server id, wherenullmeans this app.runtimeTargetForOwnerHostId(hostId)gives it for a row's host id:runtime:Egoes to server E, andlocalorssh:tgo through this app, the way direct SSH already does. The "unresolved owner" placeholder id is never dialed.runtimeTargetForWorkspaceOwner(state, ref)(inresolve-owner.ts) isresolveOwnerplushostRouteForAuthority. It returnsnullwhen the rows name no owner or disagree, so a caller cannot quietly fall back to focus.defaultCreationHost(settings)(newlib/default-creation-host.ts) is the one approved reader of the Active Server setting. It is for creation flows that have no source row.check-owner-routing-ratchet.mjsnow keeps two per-file baselines. The existing one still counts the three focus-routing helpers (209 today). The newconfig/focus-setting-read-baseline.txt(1476 today) counts every read of the setting: member, element and destructuring reads ofactiveRuntimeEnvironmentId, plus every use of a function that reads it on the caller's behalf. Those functions are discovered, not hand-listed:defaultCreationHost.src/sharedwhose body reads the setting or calls a seed is added. Examples aregetSettingsFocusedExecutionHostId,getProviderRuntimeContextKey, andgetAutomationListTarget, which is a renamed copy ofgetActiveRuntimeTarget.getBrowserSettingsHostId.ns.reader(…)) or a destructured dynamic import (renamed, parenthesized or.then(({ … }) =>) count too.<Pane />doesn't read focus. Replacing a helper with a direct read of the setting, or with the creation default, therefore no longer lowers any count. Baseline rows can end with a# note, and--prunekeeps it.Why
The remaining ~200 focus-routed call sites are spread over many areas, so they will move in several PRs, one per area. This PR lands the shared building blocks once, so those PRs don't each invent their own.
The first ratchet alone could be lowered by a rename, for example by swapping
getActiveRuntimeTarget(settings)for something that readssettings.activeRuntimeEnvironmentIditself. The second counter closes that gap.Discovery follows returned values only. Following every call pulled in about 7,600 names, which is most of the renderer through the store. We considered folding everything into the existing count. That would make the helper count impossible to bring to zero, because legitimate writes and display code still mention the setting. So the helper ratchet stays the one that goes to zero, and the read ratchet can only go down.
Linked Issue
None. This is part of the host-ownership routing stack (D4 G1).
Visual Proof
N/A. There is no UI or behavior change; it only adds helpers and a lint counter.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
runtime-client-target.test.ts,resolve-owner.test.tsanddefault-creation-host.test.tscover the new helpers. They check that the owner wins over a focused server, that local and direct SSH owners go through this app, and that a missing owner has no connection.check-owner-routing-ratchet.test.mjscovers the read counter, including that a renamed reader, a wrapper of a wrapper,runtimeTargetForOwnerHostId(getSettingsFocusedExecutionHostId(s)), and flat or nested destructuring each still count. It also checks that a same-named harness function and a component are not counted, and that writes, props and indexed types don't. It also covers alias refusal and notes that survive a prune.Ran:
pnpm tc:web, focusedpnpm test,oxlint,pnpm run check:code-quality:changed, andpnpm check:owner-routing-ratchet(both counters OK).Review
Agent skill upstream boundary
Notes
No wire change; renderer only. SSH: direct SSH owners keep going through this app's IPC.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)