feat(desktop): add managed remote Runtime Host onboarding - #3236
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
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.
Keep the pairing deadline referenced on every supported Node version and clear a systemd unit start-limit state before restoring an active deployment. Generated-by: Codex
|
@hqhq1025 Thank you for the review. The confirmed findings are addressed in
The release-package finding was adjudicated separately in its thread. We are retaining the protected manual release workflow as a trusted publisher boundary rather than adding a second compatibility mechanism or constraining the explicitly selected CLI version to the repository version. All review threads have replies and are resolved. Please re-review the current head when convenient. AI disclosure: OpenAI Codex posted this maintainer-directed summary after the fixes and validation were reviewed. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Two prior code findings are fixed on this revision. Keeping the pairing-finalization deadline referenced eliminated the deterministic test cancellations (the complete Desktop suite passed 1006/1006 with zero cancellations), and active systemd rollback now resets the failed/start-limit state before restarting the prior unit (the service-manager suite passed 10/10).
[P1] .github/workflows/release-desktop.yml:66 still validates only that the exact input exists on npm, then embeds it into the released Desktop unchanged. The only published beta is maka-agent@0.1.0-beta.1; it passes this check but has no runtime-host setup command and uses compatibility epoch 24 while this Desktop uses 30. The live release environment only enforces the main-branch policy, and no release checklist step validates this package, so a normal release dispatch can produce an app whose Add computer flow deterministically fails. Require the selected package to match the repository CLI version, or give published packages a machine-readable setup/epoch probe and verify it before packaging.
Codex-assisted review performed under the maintainer-approved review workflow.
Derive the Runtime Host setup package from the CLI manifest and require that exact artifact to be public before packaging Desktop. This removes a redundant release input and prevents a valid but incompatible package from entering a release. Generated-by: Codex
|
@hqhq1025 Thank you for pressing on the release-package finding and for providing the concrete beta.1 counterexample. We reconsidered the earlier trust-boundary decision. Trusting a release publisher is appropriate for decisions that require judgment, but this input only asked the publisher to repeat a value the repository already owns. We also found no current product requirement for pairing a Desktop release with a CLI version different from the CLI source in that release commit. The revised design in
This leaves one version authority and no separate compatibility probe. At the current head, The release workflow/publication policy tests, lint, and formatting checks pass. Please re-review the current head when convenient. AI disclosure: OpenAI Codex posted this maintainer-directed explanation after the design decision and resulting change were reviewed. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Approved at exact head 7127099bcd2b1180e28af6e1094697712a075e0d.
No actionable code findings remain from my re-review. The previous release blocker is fixed at the correct authority: Release desktop now derives maka-agent@<packages/cli version>, requires that exact version to exist publicly, and only then exports it into electron-builder metadata. The repository currently names 0.1.0-beta.2, while the public registry contains only 0.1.0-beta.1, so the workflow fails closed today instead of embedding the known-incompatible package.
Problem definition and mechanism: the PR adds one guided SSH onboarding transaction spanning package deployment, staged Host credentials, verified local profile activation, credential finalization, and project selection. The Host-owned pending credential and Desktop pairing journal are not duplicate authorities: one bounds remote credential validity, while the other preserves the local profile/credential state needed to recover a crash between local persistence and remote finalization. After the recovery, timeout, cancellation, output-redaction, systemd rollback, and release-version fixes, the implementation follows those ownership boundaries consistently.
First principles and Occam's razor: the final structure is justified by the distributed commit boundary. Replacing an existing profile through a temporary new profile could reduce some rollback code, but it would change stable profile identity and still require durable recovery for crashes around Host finalization; I do not see evidence that it is a simpler equivalent solution. I found no production code that can be safely deleted without removing the development-package path or weakening recovery. The failure-path tests are behavioral and valuable; I found no low-quality test that should be removed. No deeper refactor is required for this revision.
Verification on this head:
npm run build:test,npm run typecheck,npm run lint, andnpm run format:checkpassed.npm run check:releasepassed after building the renderer artifact it checks.- CLI passed 337/337 tests.
- The Runtime Host full suite passed 1038/1040 while run concurrently; both unrelated timeout failures passed when rerun serially.
- Desktop passed 1004/1006 while run concurrently. The shell-output timeout passed serially; the unrelated Rive child-reaping test still times out on this machine. GitHub's main
testcheck is green.
Merge verdict: the code is approved, but the revision is not ready to merge while the required Release Windows check is red. That job built and downloaded the update, then observed the old 0.1.11.0 process instead of the expected 0.1.12.<build> and could not remove the still-busy installation directory. The same verifier is currently failing on other branches with different launch/update symptoms, so I cannot attribute it to this PR from present evidence, but it must be rerun successfully before merge.
Residual verification gap: the PR's Windows check packages without MAKA_RUNTIME_HOST_SETUP_PACKAGE, so it does not exercise the exact setup-package metadata used by official releases. The release workflow itself is fail-closed and the development archive path was tested against a real Linux systemd user service, but the first release should still verify an actual packaged Add computer flow with the published 0.1.0-beta.2 CLI before publication.
|
The remaining red The Runtime Host onboarding review has converged and the current head is approved. The repository CI and dependency audit checks are green. The Windows job fails later in the existing automatic-update verifier: after That failure shape is already tracked as #3340: a surviving Runtime Host process can keep the installation directory in use, causing the NSIS handoff to leave the old version installed. The verifier-side instability and cleanup behavior are being addressed separately in #3327, which is approved and has a green end-to-end Windows lane at its current head. #3348 also investigated product-side residue cleanup, but its current revision intentionally removed the unsafe untracked-PID termination path after review exposed a PID-reuse identity hazard. It therefore does not yet close the underlying #3340 lifecycle problem. None of the failing verifier, updater, installer, or cleanup paths are changed by this PR. The failure should remain owned by the existing Windows update work rather than expanding this already-reviewed Remote Runtime Host onboarding change. AI disclosure: OpenAI Codex posted this maintainer-directed status summary after checking the failing run and the related issue and PR heads. |
|
PR #3327 is now merged into |
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?