feat(desktop): add managed remote Runtime Host onboarding - #3236
feat(desktop): add managed remote Runtime Host onboarding#3236M4n5ter wants to merge 17 commits into
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughProblem solvedThis PR adds a guided Desktop flow for adding a Linux Runtime Host through SSH. The flow:
Source of truthThe PR extends the existing Desktop, CLI, Runtime Host, Profile, credential, OpenSSH, systemd, and IPC contracts. It does not add a separate credential authority or Profile system. The guided flow adds a new onboarding entry point over the existing managed setup path. Manual Profile configuration remains a separate supported path. Solution size and complexityThe implementation is the smallest coherent solution for the requested transactional behavior. The prepare/finalize credential flow prevents credential commitment before verification. Retry handling addresses connection loss and unknown commit outcomes. Profile rebinding and rollback protect existing Profiles. SSH cancellation escalation prevents setup processes and archive uploads from remaining active. The implementation adds coordination across the main process, preload bridge, renderer, CLI, and Runtime Host protocol. This scope matches the requested onboarding, security, recovery, and development-package behavior. The following code may be simplified later without weakening behavior if equivalent coverage remains:
Rollback, retry, cancellation, redaction, and failure-path tests should remain. They cover security-sensitive and transactional behavior. Validation and risksThe PR reports:
The tests cover credential and progress isolation, pairing finalization recovery, cancellation and forced termination, Profile replacement and rollback, credential preparation and revocation, development archive upload, service restart behavior, compatibility handling, and release-version handling. These checks were not independently verified in the available evidence. Required-check status remains unverified. Complexity deltaThe PR adds:
The PR removes or consolidates:
Total maintenance complexity increases locally. The increase is justified by the required credential isolation, transactional pairing, retry safety, cancellation guarantees, and packaged/development setup support. The reported validation supports this conclusion, but required checks remain unverified. Review-relevant risks
The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughDesktop adds guided SSH onboarding for remote Runtime Hosts. The change adds deferred credential pairing, durable pairing recovery, framed SSH setup, onboarding IPC, Settings UI, project selection, CLI package handling, and validation coverage. ChangesRemote Runtime Host onboarding
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to This PR adds managed Linux Runtime Host onboarding, pairing, and deployment, but the current implementation can expose pairing credentials in recovery files, resurrect revoked credentials after failed writes, ignore deployment cleanup failures, and leave onboarding stuck without an error message. These security, rollback, and usability risks should be fixed before merge. Sequence Diagram(s)sequenceDiagram
participant User
participant Settings
participant DesktopOnboarding
participant SSH
participant RuntimeHost
User->>Settings: Enter SSH destination
Settings->>DesktopOnboarding: Start onboarding
DesktopOnboarding->>SSH: Run setup package
SSH->>RuntimeHost: Upload and execute setup
RuntimeHost-->>SSH: Return progress and credential
SSH-->>DesktopOnboarding: Return setup result
DesktopOnboarding->>RuntimeHost: Verify and finalize pairing
DesktopOnboarding-->>Settings: Publish completed profile
Settings->>User: Open project picker
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
956157c to
0427296
Compare
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
710be23 to
e04eb16
Compare
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 56ff859 |
56ff859 to
9c4737a
Compare
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit a654cba |
c4082fc to
c4a54fd
Compare
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 37ec416 |
49996da to
b2543c8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
packages/cli/src/runtime-host-cli.ts (1)
184-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake
allowConfigurationdefault explicit.
allowConfigurationis optional and the guard tests=== false. A caller that omits the field gets configuration arguments enabled.parseSetupCommandrelies on that implicit default, whileparseServiceManagementCommandpasses the value explicitly. A future caller that forgets the field silently accepts--root,--project-root,--websocket-port, and--websocket-path.Disposition: optional. Destructure with a default so the intent is stated at one place.
♻️ Proposed refactor
function parseManagedServiceOptions( argv: string[], - input: { + { + valueOptions, + flagOptions, + allowConfiguration = true, + }: { readonly valueOptions?: Readonly<Record<string, (value: string) => RuntimeHostCliError | void>>; readonly flagOptions?: Readonly<Record<string, () => RuntimeHostCliError | void>>; readonly allowConfiguration?: boolean; } = {}, ): ManagedServiceOptions | RuntimeHostCliError {Then use
flagOptions,valueOptions, andif (!allowConfiguration)in the loop.packages/cli/src/__tests__/runtime-host-service-manager.test.ts (1)
89-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the duplicate
--defer-pairing-commitbranch.
parseSetupCommandinpackages/cli/src/runtime-host-cli.ts(Line 127) returnsDuplicate --defer-pairing-commitwhen the flag repeats. This test only covers the single-use case. The rejection branch has no assertion here.Disposition: optional. Add one case that passes the flag twice and asserts the error result.
packages/runtime-host/src/server/operation-dispatcher.ts (1)
239-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the five identical unavailable handlers.
Every entry returns the same outcome. A single factory removes the repetition and keeps the
Picktype check intact.♻️ Proposed refactor
+const accessCredentialsUnavailable = async () => ({ + ok: false as const, + error: { + code: 'operation_unavailable' as const, + message: 'Runtime Host access credentials are unavailable', + }, +}); + export function createUnavailableAccessAuthorityOperationHandlers(): AccessAuthorityOperationHandlerMap { return { - 'access.credential.issue': async () => ({ ... }), - 'access.credential.replace': async () => ({ ... }), - 'access.credential.prepare': async () => ({ ... }), - 'access.credential.revoke': async () => ({ ... }), - 'access.credential.finalize': async () => ({ ... }), + 'access.credential.issue': accessCredentialsUnavailable, + 'access.credential.replace': accessCredentialsUnavailable, + 'access.credential.prepare': accessCredentialsUnavailable, + 'access.credential.revoke': accessCredentialsUnavailable, + 'access.credential.finalize': accessCredentialsUnavailable, }; }Disposition: optional.
Source: Path instructions
packages/runtime-host/src/__tests__/host-profile.test.ts (1)
170-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the credential-only rebind path.
The test only exercises
bindingChanged === true(the transport URL changes). ThebindingChanged === falsebranch inrebindIfCurrentbehaves differently: it reuses the same credential slot, setsdisplacedCredentialtotarget.credential, and skips thecredentials.delete(stored)step. That branch is unverified.The
profile.id/profile.rootIdguard rejection is also unverified.💚 Suggested additional assertions
+ test('rebinds a credential without changing the transport binding', async () => { + const path = await profilePath(); + const credentials = memoryCredentials(); + const desktop = createFileRuntimeHostProfileCatalog(path, credentials); + const profile = remoteProfile('office', 'wss://host.example.com', ROOT_A); + await desktop.create(profile, 'old-token'); + const expected = await desktop.resolve(profile.id); + assert.equal((await desktop.rebindIfCurrent(expected, profile, 'new-token')).rebound, true); + assert.equal((await desktop.resolve(profile.id)).credential, 'new-token'); + }); + + test('rejects a rebind that changes the Host identity', async () => { + const path = await profilePath(); + const credentials = memoryCredentials(); + const desktop = createFileRuntimeHostProfileCatalog(path, credentials); + const profile = remoteProfile('office', 'wss://host.example.com', ROOT_A); + await desktop.create(profile, 'old-token'); + const expected = await desktop.resolve(profile.id); + await assert.rejects( + desktop.rebindIfCurrent(expected, remoteProfile('office', 'wss://host.example.com', ROOT_B), 'x'), + /must retain its Host identity/, + ); + });Disposition: follow-up.
Source: Path instructions
packages/cli/src/runtime-host-access-command.ts (1)
79-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: give the exported functions explicit, consistent signatures.
Two small inconsistencies in the new public surface:
- Line 79:
replaceRuntimeHostAccessCredentialisasyncbut only forwards a promise, whileissueRuntimeHostAccessCredentialandprepareRuntimeHostAccessCredentialare notasync.- Line 146:
revokeRuntimeHostAccessCredentialhas no declared return type.runtime-host-setup-command.tsbinds to it throughtypeof, so the exported contract is inferred fromconnection.requestinternals.♻️ Proposed change
-export async function replaceRuntimeHostAccessCredential( +export function replaceRuntimeHostAccessCredential( options: RuntimeHostAccessIssueOptions, ): Promise<ReplacedRuntimeHostAccessCredential> { return mutateRuntimeHostAccessCredential(options, 'access.credential.replace'); }-export async function revokeRuntimeHostAccessCredential(options: RuntimeHostAccessRevokeOptions) { +export async function revokeRuntimeHostAccessCredential( + options: RuntimeHostAccessRevokeOptions, +): Promise<OperationOutput<'access.credential.revoke'>> {Disposition: optional.
Also applies to: 146-146
Source: Path instructions
packages/cli/src/runtime-host-managed-deployment.ts (1)
219-240: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePass
versionsRootexplicitly topruneInactiveDevelopmentPackages. This avoids recomputing it withdirname(packageRoot)and removes unnecessary indirection.Source: Path instructions
apps/desktop/src/main/__tests__/runtime-host-ssh-terminal.test.ts (1)
181-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the shell-variable-name assertion.
assert.doesNotMatch(remoteCommand, /\bstatus=/u)asserts the internal name of a shell variable in the generated command. That name has no observable effect. The adjacent assertion on/maka_setup_exit/ualready covers the exit-code propagation contract. A rename would break this test without any behavior change.🧹 Suggested removal
assert.match(remoteCommand, /maka_setup_exit/u); - assert.doesNotMatch(remoteCommand, /\bstatus=/u);As per path instructions: "Flag tests that duplicate existing coverage, assert implementation details, or do not protect observable behavior."
Source: Path instructions
apps/desktop/src/main/__tests__/runtime-host-onboarding.test.ts (1)
62-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the cancellable path and for endpoint rejection.
The tests cover the non-cancellable commit phase and the success path. Two behaviors in
runtime-host-onboarding.tsremain untested: cancel during the SSH phase returnstrueand publishesidle, andrequireSetupEndpointrejects a non-loopback or non-ws:endpoint. Both are security-relevant guards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 35a99f27-6640-4c0a-9bbf-fdc85c2bf176
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (56)
CONTRIBUTING.mdCONTRIBUTING.zh-CN.mdapps/desktop/electron-builder.config.mjsapps/desktop/src/main/__tests__/runtime-host-desktop-manager.test.tsapps/desktop/src/main/__tests__/runtime-host-onboarding.test.tsapps/desktop/src/main/__tests__/runtime-host-profile-service.test.tsapps/desktop/src/main/__tests__/runtime-host-ssh-terminal.test.tsapps/desktop/src/main/runtime-host-boot.tsapps/desktop/src/main/runtime-host-client.tsapps/desktop/src/main/runtime-host-desktop-manager.tsapps/desktop/src/main/runtime-host-onboarding.tsapps/desktop/src/main/runtime-host-profile-service.tsapps/desktop/src/main/runtime-host-ssh-terminal.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/app-shell-overlays.tsxapps/desktop/src/renderer/app-shell.tsxapps/desktop/src/renderer/locales/settings-projects-copy.tsapps/desktop/src/renderer/settings/projects-settings-page.tsxapps/desktop/src/renderer/settings/runtime-host-onboarding-dialog.tsxapps/desktop/src/renderer/settings/runtime-host-profiles-section.tsxapps/desktop/src/renderer/settings/settings-modal.tsxapps/desktop/src/renderer/settings/settings-surface.tsxapps/desktop/src/renderer/styles/settings/runtime-host.cssapps/desktop/src/renderer/use-new-task-target.tsapps/desktop/stories/settings/settings-pages.stories.tsxdocs/astryx-surface-file-inventory.mddocs/astryx-surface-file-inventory.pathsdocs/runtime-host-remote-access.mddocs/runtime-host-remote-access.zh-CN.mdpackages/cli/package.jsonpackages/cli/src/__tests__/runtime-host-cli-context.test.tspackages/cli/src/__tests__/runtime-host-operator-command.test.tspackages/cli/src/__tests__/runtime-host-profile-command.test.tspackages/cli/src/__tests__/runtime-host-service-manager.test.tspackages/cli/src/__tests__/runtime-host-setup.test.tspackages/cli/src/cli-core.tspackages/cli/src/runtime-host-access-command.tspackages/cli/src/runtime-host-cli.tspackages/cli/src/runtime-host-managed-deployment.tspackages/cli/src/runtime-host-setup-command.tspackages/cli/src/runtime-host-systemd-service.tspackages/runtime-host/src/__tests__/authenticated-websocket.test.tspackages/runtime-host/src/__tests__/host-profile.test.tspackages/runtime-host/src/__tests__/websocket-listener.test.tspackages/runtime-host/src/client/host-profile.tspackages/runtime-host/src/protocol/access-authority.tspackages/runtime-host/src/protocol/index.tspackages/runtime-host/src/protocol/operations.tspackages/runtime-host/src/server/access-authority.tspackages/runtime-host/src/server/access-credential-store.tspackages/runtime-host/src/server/connection-session.tspackages/runtime-host/src/server/host-kernel.tspackages/runtime-host/src/server/operation-dispatcher.tsscripts/release-cli-package.mjsscripts/release-cli-publication.test.mjs
Included review availability: Your plan provides up to 3 included reviews per hour; 1 remains after this review.
PR Summary by QodoAdd managed remote Runtime Host onboarding to Desktop
AI Description
Diagram
High-Level Assessment
Files changed (57)
|
|
Code review by qodo was updated up to the latest commit b2543c8 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the substantial hardening here. The current head is green and the supplied screenshot covers the new connection flow, but two P2 correctness gaps remain: generated development archives are not recognized by the new version predicate, and a process exit after durable profile mutation but before credential finalization leaves pairing unrecoverable. The final substantive commit also needs the declared Codex Generated-by trailer. Please address these items, then request an exact-head rereview. Because this changes user-visible behavior, a public protocol, credential security, and release behavior, independent human review is still required by project policy.
Reviewed with Codex as an AI-assisted code review. I verified the current diff, relevant authority boundaries, tests, CI, screenshot, and commit provenance; no external model output was used.
中文说明
当前 head 的 CI 和截图都没有问题，但仍有两个 P2：新生成的 dev 版本与识别正则不一致；Profile 已持久化但 credential 尚未 finalize 时如果进程退出，重启后无法恢复，旧 Profile 甚至会丢失可用凭证。最后一个实质性 commit 还缺少已声明的 Codex Generated-by trailer。修复后请按新 head 重新 review；同时本 PR 涉及 UI、公开协议、credential security 和 release 行为，仍需独立人工审查。
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
An independent adversarial pass confirmed the two earlier P2 findings and found one additional user-visible cancellation gap, inline below. The rest of the prior review remains unchanged.
中文说明
独立交叉复核确认了前两个 P2，并额外发现一个用户可见的取消路径问题，见下面 inline。此前其余结论不变。
5f6bb39 to
0673602
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/desktop/src/main/__tests__/runtime-host-profile-service.test.ts (1)
336-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that rollback restores the previous enablement state.
The test verifies the credential is restored and the journal is cleared. It does not verify the
intent.wasEnabledbranch inrollbackPairingIntentatapps/desktop/src/main/runtime-host-profile-service.tsLines 339-354.stageInterruptedPairingenablesPROFILE.id, so rollback must re-add it toenabledRemoteProfileIdsand callinput.enablewith the restored target.Both effects can regress without failing this test. Add assertions on the persisted preferences and on the re-enable call.
💚 Sketch of the added assertions
const service = createDesktopRuntimeHostProfileService({ clientDataRoot: root, startup, catalog, states: () => [connectingLocal()], enable: async (target) => { + enabled.push(target.credential ?? ""); if (target.credential === "new-token") { throw new RuntimeHostPermanentReconnectError("pairing credential expired"); } },assert.equal((await catalog.resolve(PROFILE.id)).credential, "old-token"); + assert.deepEqual(enabled, ["new-token", "old-token"]); + const restored = await resolveDesktopRuntimeHostStartup(root, { catalog }); + assert.deepEqual(restored.preferences.enabledRemoteProfileIds, [PROFILE.id]); - assert.equal((await resolveDesktopRuntimeHostStartup(root, { catalog })).pairingIntents.length, 0); + assert.equal(restored.pairingIntents.length, 0);Declare
const enabled: string[] = [];above the service.packages/cli/src/__tests__/runtime-host-setup.test.ts (1)
178-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the dot-separated development version.
The packager emits
0.1.0-beta.2.dev-<hex>when the base version contains-. Add a fixture with this form to cover the\.branch inisRuntimeHostDevelopmentPackageVersionduring replacement and pruning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0487ef76-0312-44fe-bf07-5d8dfceca9c6
📒 Files selected for processing (13)
apps/desktop/src/main/__tests__/runtime-host-onboarding.test.tsapps/desktop/src/main/__tests__/runtime-host-profile-service.test.tsapps/desktop/src/main/__tests__/runtime-host-ssh-terminal.test.tsapps/desktop/src/main/runtime-host-boot.tsapps/desktop/src/main/runtime-host-onboarding.tsapps/desktop/src/main/runtime-host-pairing-journal.tsapps/desktop/src/main/runtime-host-profile-service.tsapps/desktop/src/main/runtime-host-ssh-terminal.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/renderer/settings/runtime-host-ssh-terminal-dialog.tsxpackages/cli/src/__tests__/runtime-host-setup.test.tspackages/cli/src/runtime-host-managed-deployment.tspackages/runtime-host/src/client/index.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 06736029a2e6e78cb51153c39768423efec8259c.
The earlier development-version and crash-window pairing findings are fixed: the generated and recognized dev-<digest> grammar is aligned, and Desktop now persists a bounded atomic pairing journal before profile mutation, replays enable plus idempotent finalization after restart, and restores the previous credential after a permanent failure. The requested screenshot and all ten Codex trailers are present, and all prior threads are resolved.
One current-main integration blocker remains inline. Please rebase, advance the compatibility epoch, update fixtures, and rerun the still-pending current-base checks. Independent human review remains required because this affects UI, a public protocol, credential security, and release behavior.
AI-assisted review disclosure: Codex reviewed the exact-head delta, prior findings and remediation, recovery tests, current-main protocol integration, live threads and CI, screenshot, and provenance metadata. No external model was used. Astro-Han authorized this review campaign.
中文说明
此前 dev 版本识别和崩溃窗口 pairing recovery 两个 P2 均已修复：版本语法统一，Desktop 会在 profile mutation 前原子持久化恢复 journal，并在重启后完成幂等 finalize 或永久失败回滚。截图、十个 Codex trailer 和旧线程都已核实。当前只剩 inline 所述的 main compatibility epoch 冲突；请 rebase 后升 epoch、更新 fixture 并重跑 CI。
26ddf61 to
ea3b69c
Compare
3f403af to
4bf6351
Compare
|
Release note on the CLI version used by Desktop onboarding: When this work began, the repository had not yet moved into the Apache organization, and the working assumption was that the next CLI version could be published immediately after merge. That assumption no longer holds after the migration: merging this PR does not itself make a new npm package available. The implementation has therefore been adjusted so a packaged Desktop build does not hard-code or implicitly resolve an unpublished or floating CLI version. The release workflow now requires an exact already-published
|
|
Release-policy clarification: the Runtime Host setup package input is an operator-controlled release decision. We trust an authorized Desktop publisher to select an exact, already-published Maka CLI version that is compatible with the Desktop build being released. It is not an untrusted runtime input, and the workflow intentionally does not try to infer semantic CLI compatibility from source versions or add a second capability/version authority. The existing checks keep the selected package immutable and verify that the exact version exists on npm; compatibility remains part of the publisher review and release checklist. |
33bfae8 to
6cc6cd1
Compare
|
@Astro-Han, thank you for the careful reviews. Could you please re-review the current exact head Since the previous round, the branch has been rebased onto the latest The rebased branch passes Automated by Codex on behalf of M4n5ter. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 6cc6cd1a9. I ran five scoped sub-reviews — problem definition, simplification, security, test quality, and a delta audit of my own 31 earlier findings — and re-derived everything published here myself.
First, the part that deserves to be said plainly: the response to review has been excellent. Of my 31 findings, 20 are fixed at this head, and several are fixed at the cause rather than the symptom — the shared-secret writer got a real fsync fence, the "profile becomes enabled" logic collapsed to one owner, the journal became a per-profile map so one stuck intent no longer blocks every other Host, and the npm branch is now pinned to a mktemp -d prefix. Two more you rebutted correctly and I withdraw. That is a high rate on hard findings.
And yet I am not going to file a thirty-second finding, because I do not think the remaining problem is a list of defects. Fifteen commits, of which one is the feature and fourteen are fix/harden/bound/reconcile/preserve/retain, almost all landing on pairing and recovery. The individual answers are mostly right and the count still climbs. Two facts show the shape directly rather than by size argument: babf67971 added active = undefined; to completePresentation() to release the terminal slot, and 6cc6cd1a9, the very next commit, deletes that exact line because releasing early meant a completed setup process was no longer owned by the terminal lifecycle; and 2d3887e26 routed finalization through #mutateTarget and added waits for in-flight pairing finalization before closing its target, after which 66baffa38 broke the wait that serialization created, b67803d35 deleted the test, and babf67971 added the timeout that actually bounds it — the asserted guarantee no longer exists at this head. Across those thirteen commits runtime-host-ssh-terminal.ts was touched eight times, runtime-host-profile-service.ts six, access-authority.ts five; access-authority.ts grew 71% and profile-service.ts 44%, and no commit ever removed a state.
The cause is a write order, and it is a choice this PR makes rather than one the Host imposes. addAndEnableVerified calls rebindIfCurrent to overwrite the existing profile's credential slot before the Host commits, so the pairing needs a rollback; the rollback needs the previous credential to be durable; that durability is the journal, and the journal is a second persistent coordinator for a transaction the Host already owns with pending/expiresAt/a 15-minute expiry. Every one of the last five commits is a boundary patch between those two coordinators, and their bounds are still unaligned — 30 s on the Desktop side against 15 minutes on the Host's, with nothing recording the coupling. The destructive step is not required: credential slots are keyed by profile id (host-profile.ts:626), so a new profile id never touches the old slot; the catalog already refuses target changes with "A Runtime Host profile target cannot be changed; create a new profile id" (host-profile.ts:380), which makes rebindIfCurrent a new exception to an existing rule rather than an extension of it; the Host's AccessCredentialFile.credentials is a list, so credentials coexist by design; and authenticate() grants a pending credential the same operationGrants as an active one while prepare revokes nothing, so both keys work throughout the window. That last point matters most: at rollback time the old credential was never invalidated and the new one is still usable, so the rollback is not recovering access, only bookkeeping. Create the profile, verify it, swap, and delete the old one on success — the journal, its recovery path, its two snapshot flags, its UI entry point, and the failure mode at profile-service.ts:408 ("pairing failed and its previous profile could not be restored", a state that exists only because rollback exists) all disappear together. I estimate around 500 lines, and the two open P2s below at :530 and :562 are both inside code that would no longer exist.
Two architecture-level points for a maintainer rather than for this thread. This PR's delivered artefact is a protocol change — two new wire operations, a new credential state with an expiry, a new error code, a new entry in REMOTE_OWNER_OPERATION_GRANTS next to a comment saying such grants must be updated deliberately, and an epoch bump that hard-rejects mismatched peers — while #3233 describes UI over an unchanged contract and does not mention any of it. Separately, release-desktop.yml gains a required: true input that binds every Desktop release, including releases unrelated to remote hosts, to the CLI release train; that is a release-process decision, it is not in #3233, and reverting this PR would revert it, which by the one-intent rule makes it a second intent.
Three notes that do not warrant their own threads. The rebind fix at b67803d35 removed the named-profile-with-remedy message that existed at 3f403af23:496, so the enabled-conflict case now fails with the opaque Runtime Host ${rootId} is already enabled — a State Root id, no profile name, no remedy — and it fires only after the full SSH setup has run; that is a regression caused by a fix. The mktemp -d cleanup added at babf67971 uses trap ... EXIT, which does not run on the SIGHUP the remote foreground process group receives when Desktop terminates ssh -tt, so cancelled setups leave a directory on every target machine; add HUP INT TERM. And requireSetupDestination accepts internal whitespace and control characters that the existing requireSshDestination rejects, so the same value has two validators with the weaker one at the front door — reuse the existing one.
I also want to correct something I published on this PR earlier: I argued that identical = N epoch text merges cleanly and lets two PRs silently share an epoch. That is wrong. main moved to 28 today and all six epoch-28 claimants went CONFLICTING immediately, because the differing comment block below the constant conflicts. Git blocks the second merge; the residual risk is only that a human resolving that conflict keeps both comment blocks under one number.
Carrying three P1s, so this is a COMMENT.
AI disclosure: reviewed with Claude Code (Opus 5), using five scoped sub-reviews as leads. Every finding published here I verified myself at 6cc6cd1a9: I reproduced the renderer credential leak by executing a verbatim port of createSetupOutputFilter; I read the setup-frame schema, the onboarding destructuring and the profile type to confirm credentialId is received and dropped; I read packages/runtime-host/src/protocol/index.ts:75 at all three PR heads for the epoch collision, and confirmed the conflict behaviour against today's main; I traced the resolvePairingRecovery call chain to writeRuntimeHostPreferences; I grepped both test files for the two uncovered error types; and I read the systemd unit and both systemctl call sites. Sub-review findings I could not confirm are not included, and I downgraded or reframed several that did not survive checking. Per AGENTS.md this is not independent human review.
|
Following up on my review with the concrete target design, because "the write order is wrong" is not something you can act on and I do not want to leave you patching the eight threads instead. Everything below uses APIs that already exist at The one fact the design rests on
Proposal 1 — create, verify, swap, delete. The journal disappears.
addAndEnableVerified(value) {
requireSaveInput(value);
return mutateProfiles(async () => {
const document = await catalog.read();
const superseded = document.profiles.find(
(p) => p.kind === 'remote' && p.rootId === value.profile.rootId,
);
const supersededWasEnabled =
superseded !== undefined &&
preferences.enabledRemoteProfileIds.includes(superseded.id);
// 1. Create under the fresh id onboarding already minted. Nothing is overwritten.
const created = await catalog.create(value.profile, value.credential);
const profile = created.profiles.find((c) => c.id === value.profile.id);
if (!profile) throw new Error('Runtime Host profile creation did not persist');
const target = { profile, credential: value.credential } as const;
try {
// 2. Free the at-most-one-enabled invariant.
if (superseded && supersededWasEnabled) await disableProfile(superseded.id);
// 3. Prove the new profile. assertRootIsNotEnabled now passes.
const error = await enable(profile.id);
if (error) throw error;
// 4. Commit. Only now is the old profile redundant.
if (superseded) {
if (preferences.defaultProfileId === superseded.id) await setDefaultProfile(profile.id);
await catalog.remove(superseded.id);
}
return { profileId: profile.id };
} catch (failure) {
await rollbackCreatedProfile(catalog, target, failure); // already exists, :639
if (superseded && supersededWasEnabled) {
await enable(superseded.id).catch(() => {}); // restore a flag, not a secret
}
throw failure;
}
});
}Why this needs no journal: nothing durable is ever overwritten. What a crash leaves behind, at every point in the sequence: the old profile intact, plus possibly a new profile that is disabled or unavailable. Both are ordinary rows in Settings. Re-running Add computer is idempotent — it mints another id and supersedes again. That is the entire behavioural delta versus today, and I want to state the cost honestly rather than pretend it is free: the current design self-heals a crashed pairing, this one leaves the user one visible row to delete. My argument is that ~500 lines, a second persistent coordinator, and fourteen hardening commits is a steep price for removing one click, but that is your product call to make explicitly, not mine to assume. Deleting, once this lands:
Both open P2s from my review — the untested interrupt branch at Proposal 2 — let the Host finalize itself.
|
EnglishThank you for the detailed re-review and, especially, for proposing a concrete alternative instead of only identifying complexity. The concern is valid: the pairing path has accumulated enough recovery logic that its state model deserves explicit re-evaluation. After tracing the complete setup and failure ordering, I do not think the two proposed architectural changes preserve the required guarantees. Why we are keeping the Desktop pairing journalThe proposed “create, verify, swap, delete” sequence does not remove the local transaction:
The Host and Desktop are not duplicating the same authority. The Host owns the credential transition from pending to active and the revocation of the previous credential. Desktop owns the durable Profile, credential binding, enablement and default selection. The journal records only the Desktop participant’s unfinished local transition so those two authorities can be reconciled after interruption. The proposed sequence also does not work for the common case where the superseded Profile is the current default: it cannot be disabled first, while the replacement cannot become the default until it has been enabled. For those reasons, we will retain the explicit journal and staged finalization rather than move the same intermediate state into ordinary Profile rows. Why finalization must remain explicitThe remote setup command verifies the pending credential before emitting the complete frame. That verification connects to the Host and successfully calls If the Host promoted a pending credential on its first successful operation, this verification would finalize the credential and revoke the previous one before Desktop had received or persisted the new credential. An SSH interruption between verification and delivery would then leave Desktop with the revoked old credential and no saved replacement—the exact handoff failure staged finalization is intended to prevent. Therefore Findings we agree should be addressedWe will address the confirmed issues with bounded changes:
The missing Credential IDI do not plan to add an otherwise unused The complete solution should be a Host-authoritative credential inventory and revocation flow, with Profile removal consuming that capability when remote management is implemented. Adding a field without that consumer would expand the persisted contract without delivering the claimed cleanup behavior. Scope observationsThe development-package path is separable in theory, but it is what permits real end-to-end onboarding verification before a compatible CLI can be published under the ASF release process. Splitting it now would increase coordination and review cost without changing the production architecture. Likewise, requiring the Desktop publisher to select an exact compatible CLI package is intentional: the release process trusts the publisher to choose the package included with that Desktop release. This keeps the explicit cross-process commit boundary while avoiding additional defensive machinery. The remaining work should close demonstrated gaps and remove ineffective tests, not add another recovery model. 简体中文感谢这次细致的重新审查，尤其是没有只指出复杂度问题，而是给出了具体替代方案。这个担忧是合理的：pairing 路径已经积累了足够多的恢复逻辑，确实应该重新明确审视它的状态模型。 在完整追踪 setup 顺序和各个失败窗口后，我认为两项架构替代方案无法保留当前所需的保证。 为什么保留 Desktop pairing journal建议的“create、verify、swap、delete”流程并没有消除本地事务：
Host 与 Desktop 也并非在维护同一份 authority。Host 负责 credential 从 pending 到 active 的迁移以及撤销旧 credential；Desktop 负责持久化 Profile、credential binding、enablement 和默认选择。Journal 只记录 Desktop 这一参与方尚未完成的本地迁移，以便中断后重新协调这两个 authority。 此外，该建议在旧 Profile 同时是当前默认 Host 这一常见场景下无法执行：旧 Profile 不能先被禁用，而新 Profile 在启用前又不能成为默认项。 因此，我们会保留显式 journal 与 staged finalization，而不是把相同的中间状态转移到普通 Profile 行中。 为什么 finalization 必须保持显式远程 setup 命令会在输出 complete frame 之前验证 pending credential。该验证会连接 Host，并成功调用 如果 Host 在 pending credential 的首次成功 operation 上自动完成 finalization，那么这次验证就会在 Desktop 收到并持久化新 credential 之前撤销旧 credential。若 SSH 在验证完成后、credential 交付前中断，Desktop 将只剩已失效的旧 credential，并且没有保存新的替代 credential——这正是 staged finalization 需要避免的交付失败窗口。 因此， 我们确认需要处理的 findings我们会通过有边界的修改处理确认成立的问题：
缺少 Credential ID我不计划在本 PR 中向通用 Profile schema 添加一个当前没有消费者的 完整方案应由 Host 提供权威的 credential inventory 与 revocation 流程；将来实现远程管理时，Profile 删除操作再消费这项能力。仅增加字段而不提供对应消费者，会扩大持久化契约，却不能真正实现评论中描述的清理行为。 关于范围的观察Development package 路径在概念上可以拆分，但在 ASF release 流程尚未发布兼容 CLI 时，它是进行真实端到端 onboarding 验证的必要能力。现在拆分只会增加协调与审查成本，不会改变生产架构。 同样，要求 Desktop 发布者选择一个精确且兼容的 CLI package 是有意的设计：发布流程信任发布者为该 Desktop release 选择正确的 package。 这一裁决保留了必要的跨进程显式提交边界，同时避免继续增加防御性机制。剩余工作应聚焦于关闭已有证据支持的缺口，并删除无效测试，而不是再引入另一套恢复模型。 AI disclosure: OpenAI Codex posted this maintainer-reviewed comment under M4n5ter's direction. |
Let Desktop install, pair, verify, and connect a Linux Runtime Host from one SSH destination while retaining manual profiles for advanced endpoints. Keep delivered credentials in the main process and continue successful setup directly to remote Project selection. Unpackaged builds may exercise the same service contract with a content-addressed local CLI archive instead of introducing a separate development Host mode. Generated-by: Codex
Keep the current credential valid until the newly persisted Desktop profile has established its own connection, then let that authenticated connection finalize the handoff. Re-pairing the same Host now atomically rebinds its transport and rolls back on connection failure. Pin packaged onboarding to an exact CLI package and give content-addressed development archives a safe dev-to-dev replacement path. Generated-by: Codex
Escalate cancelled or timed-out setup processes from graceful to forced termination and stop waiting after a bounded deadline. Reuse the same lifecycle for setup, development package upload, and terminal shutdown. Generated-by: Codex
Keep one expiring pending credential per principal while preserving immediate replacement for standalone CLI setup. Commit only the pending candidate, make retries harmless, and align SSH cancellation with the completion boundary. Generated-by: Codex
Reuse one per-principal staging path for development package uploads so a failed SSH handoff cannot accumulate orphaned archives. Successful setup still removes the staged package normally. Generated-by: Codex
Replay idempotent credential finalization after uncertain commits or connection replacement, while restoring the prior profile after conclusive failures. Share one exact setup package validator so valid build metadata is accepted consistently. Generated-by: Codex
Derive the Desktop setup package from the CLI release version so onboarding cannot target a stale command surface. Bound credential-authority shutdown and service replacement behavior, while keeping development archive setup portable and self-cleaning. Generated-by: Codex
Run the prepare-stage CLI fixture against the version declared by the repository manifest so release validation remains stable across intentional CLI version bumps. Generated-by: Codex
Terminate interactive SSH independently from onboarding settlement so shutdown cannot wait behind the process it must stop. Contain post-onboarding catalog refresh failures after the hook records the UI error state. Generated-by: Codex
Align managed development releases with the generated package version grammar. Preserve published credential authority after uncertain commits, persist pending Desktop pairing transactions across process loss, and dismiss interactive SSH presentation when cancellation begins. Generated-by: Codex
Serialize credential finalization with target shutdown so an in-flight commit can reconcile before its connection is retired. Make secret-file publication durable before dependent profile state is written, and remove the unused pairing intent UUID. Generated-by: Codex
Advance the compatibility epoch for staged credential operations and let Desktop shutdown interrupt only pairing reconnect waits. Interrupted finalization retains its durable journal so the next startup can reconcile an unknown outcome without rolling back a potentially committed credential. Generated-by: Codex
Keep setup metadata and pairing recovery scoped to onboarding so Desktop startup and existing remote Hosts remain available after recoverable failures. Model pairing as one durable intent, centralize profile activation, and preserve explicit repair and shutdown boundaries without duplicating defensive state. Generated-by: Codex
Keep pairing recovery independent per profile so an offline Host does not block other onboarding. Bound credential finalization and release the interactive terminal once its tunnel is established. Run remote npm setup from an isolated prefix, and require Desktop releases to select an exact CLI package that is already published. Generated-by: Codex
Keep a completed SSH setup process under the terminal lifecycle until it actually exits, while releasing tunneled processes only after ownership transfers to the connection resource. This lets application shutdown terminate a remote setup that stalls after sending its completion frame. Generated-by: Codex
Preserve the staged credential handoff while closing the demonstrated recovery and terminal-output gaps. Unify SSH destination validation, make finalization retries follow its idempotent contract, and let repeated Linux setup recover from systemd start limits. Generated-by: Codex
6cc6cd1 to
25bea91
Compare
Refresh the generated surface count after the onboarding UI additions so the repository inventory check matches the current product surface. Generated-by: Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Local verification used Node 22.22.1 and npm 11.19.0. Build, typecheck, lint, format, release checks, the Runtime Host suite (1040 tests), and the CLI suite (337 tests) passed. The Desktop suite reproducibly ends with 997 passes and 9 cancellations because of the timeout finding below.
Codex-assisted review performed under the maintainer-approved review workflow.
| () => timeout.abort(new RuntimeHostPairingFinalizationInterruptedError()), | ||
| this.pairingFinalizationTimeoutMs, | ||
| ); | ||
| timer.unref(); |
There was a problem hiding this comment.
[P1] Keep this deadline alive until finalization settles. Because the timer is unref'ed, the new defers pairing finalization when reconnect does not complete in time test reaches a pending reconnect with no referenced handles; Node exits the event loop before the 10 ms deadline, cancels that test, and cancels the next eight tests in the file. I reproduced this in both the full Desktop suite and a serial rerun: 997 pass, 9 are cancelledByParent with Promise resolution is still pending. Removing unref() is sufficient here because the timer is already cleared in finally and the operation is explicitly required to settle within the deadline.
| 'Description=Maka Runtime Host', | ||
| 'After=network.target', | ||
| 'StartLimitIntervalSec=60s', | ||
| 'StartLimitBurst=5', |
There was a problem hiding this comment.
[P2] Clear this start-limit state on the rollback path as well. A replacement that repeatedly exits can consume all five starts during the 45-second readiness wait; rollback then restores the previous unit but calls restart at line 291 without another reset-failed. The limit belongs to the unit name, so systemd rejects the restored service too and a failed update leaves the previously working Host offline. The install-side reset added for the earlier thread does not cover this path; reset before restarting an active snapshot and model the failed/start-limit state in the rollback test.
3 participants
Summary
English
Fixes #3233
简体中文
修复 #3233
Verification
English
npm run build:testnpm run typechecknpm run lintnpm run format:check简体中文
npm run build:testnpm run typechecknpm run lintnpm run format:checkAI use
Select exactly one:
Tool(s) and scope: OpenAI Codex implemented the Desktop onboarding flow, SSH orchestration, development-package path, tests, and documentation under maintainer direction
Checklist
Does this PR entail a change in behavior?