Mac Vm Updates#324
Conversation
… install, and credentials Move the macOS VM workflow out of the inline lane panel into a dedicated /vm tab. Add lifecycle/menu UI (MacVmPage, VmLifecycleMenu, FirstBootCard, CredentialsPromptDialog, PhaseStepper), a credentials store, ADE runtime bootstrap for guest VMs, and recovery for stale guest-created worktree dirs. Wire 20+ macos-vm adeActions and CLI subcommands; update docs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughAdds singleton macOS VM support and VM-backed lanes: new public types, IPC channels, DB migration, services (credentials, recovery, runtime bootstrap, VNC), lane attach/detach and launch-context SSH routing, preload/IPC routing, renderer /vm UI, CLI subcommands, tests, and skill docs. ChangesSingleton macOS VM and VM-backed lanes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Here's a review of the changes in this PR. OverallThis is a large, well-structured PR that moves the macOS VM workflow into a dedicated 🔴 Bugs / Latent Issues1. 2. 3. 🟡 Security Observations4. VNC password crosses the IPC boundary ( 5. VNC password in process argv ( 6. 🟡 Design / Maintainability7. 8. Module-level mutable state in 9. 10. Production gate is UI-only ( 11. Hardcoded IPSW URL ( 🟢 Positives
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (9)
apps/desktop/src/renderer/components/terminals/WorkStartSurface.test.tsx (1)
110-124: 💤 Low valueConsider checking all VM-related elements in both tests for consistency.
The test at lines 93-108 verifies the absence of three elements (
work-vm-banner,work-vm-not-ready, and"Open VM tab"text), while this test only checks two. For symmetry and completeness, consider either checking all three in both tests or adding a comment explaining why the"Open VM tab"assertion is omitted for local lanes.♻️ Suggested enhancement
expect(await screen.findByTestId("agent-chat-pane")).toBeTruthy(); expect(screen.queryByTestId("work-vm-banner")).toBeNull(); expect(screen.queryByTestId("work-vm-not-ready")).toBeNull(); + expect(screen.queryByText("Open VM tab")).toBeNull(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/terminals/WorkStartSurface.test.tsx` around lines 110 - 124, The local-lane test "does not render the VM banner for a local lane" is missing the third VM-related assertion for symmetry with the other test; update the WorkStartSurface test to also assert that the "Open VM tab" text is not present (i.e., check that screen.queryByText("Open VM tab") is null) or, if there's a reason to omit it, add a brief inline comment in that test explaining why the "Open VM tab" assertion is intentionally skipped so the test expectations remain explicit and consistent with the other VM-related assertions ("work-vm-banner" and "work-vm-not-ready").apps/desktop/src/renderer/types/novnc.d.ts (1)
1-24: 💤 Low valueConsider expanding the type definitions if additional noVNC features are used.
The current ambient declarations cover basic RFB construction and configuration. If your codebase uses additional noVNC features (keyboard events, clipboard, connection events like 'connect'/'disconnect', or methods like
sendKey/sendCtrlAltDel), consider adding those to the type definitions for better type safety.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/types/novnc.d.ts` around lines 1 - 24, Add missing noVNC RFB members and event typings to the ambient declaration so TypeScript covers additional features your code uses: extend the exported class RFB (constructor, viewOnly, etc.) with methods like sendKey, sendCtrlAltDel, grabKeyboard, ungrabKeyboard, clipboardPasteFrom, pasteString/clipboardCopy methods you use, and add strongly typed event names (e.g., 'connect', 'disconnect', 'clipboard', 'bell', 'clipboard', 'securityfailure', 'credentialsrequired') by augmenting the EventTarget typing or adding addEventListener/removeEventListener overloads for those string literals; also expand RfbOptions/RfbCredentials if you rely on extra fields. Update the declaration for class RFB and the RfbOptions/RfbCredentials types accordingly so callers of RFB.sendCredentials, RFB.sendKey, and event handlers are properly typed.apps/desktop/src/main/services/lanes/laneService.ts (2)
4247-4309: ⚡ Quick win
attachLaneToVmdoes not wrap the initial placement flip in a transaction.Unlike
detachVmLane,attachLaneToVmdirectly runs theupdate lanesstatement without a transaction. IflinkLaneToCurrentVmfails, the rollback UPDATE is issued, but a crash between the initial UPDATE and the rollback would leave the lane in an inconsistentmacos-vmstate without proper VM linkage.Wrapping the initial update in a transaction (committed only after a successful link) would make the operation more robust, though the current rollback approach is acceptable for non-crash scenarios.
♻️ Wrap attachment in a transaction for atomicity
async attachLaneToVm(args: { laneId: string }): Promise<void> { const laneId = String(args?.laneId ?? "").trim(); if (!laneId.length) throw new Error("laneId is required to attach a lane to the Mac VM."); const row = getLaneRow(laneId); if (!row) throw new Error(`Lane not found: ${laneId}`); const previous = normalizeRuntimePlacement(row.runtime_placement); if (previous === "macos-vm") return; + db.run("begin immediate"); db.run( ` update lanes set runtime_placement = 'macos-vm' where id = ? and project_id = ? `, [row.id, projectId], ); - invalidateLaneListCache(); const hooks = activeMacosVmHooks; if (hooks?.linkLaneToCurrentVm) { try { await hooks.linkLaneToCurrentVm({ laneId: row.id }); } catch (error) { logger.warn("laneService.attach_link_to_vm_failed_rolling_back", { laneId: row.id, error: error instanceof Error ? error.message : String(error), }); - db.run( - ` - update lanes - set runtime_placement = 'local' - where id = ? - and project_id = ? - `, - [row.id, projectId], - ); - invalidateLaneListCache(); + try { db.run("rollback"); } catch { /* swallow */ } throw error; } } + db.run("commit"); + invalidateLaneListCache();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/lanes/laneService.ts` around lines 4247 - 4309, attachLaneToVm currently updates runtime_placement before linking to the VM, leaving a crash window; wrap the initial placement flip in a DB transaction so the UPDATE to set runtime_placement = 'macos-vm' is only committed after hooks.linkLaneToCurrentVm succeeds. Concretely: in attachLaneToVm begin a transaction, perform the UPDATE there (instead of the standalone db.run), call hooks.linkLaneToCurrentVm while still in the transaction, commit the transaction on success and then run invalidateLaneListCache and emitPlacementChanged; on hook failure rollback the transaction (so no separate rollback UPDATE is needed) and then run invalidateLaneListCache and rethrow the error; call hooks.startMirrorSyncForLane after the commit. Reference: attachLaneToVm, linkLaneToCurrentVm, startMirrorSyncForLane (and mirror detachVmLane behavior for guidance).
4142-4240: 💤 Low valuePotential issue:
detachVmLanedoes not acquire a transaction lock before flipping placement.The
detachVmLanemethod usesbegin/commitfor the DB update but does not usebegin immediate. This could allow concurrent reads to see partial state in high-contention scenarios, though SQLite's default isolation should make this rare. For consistency with other critical updates in this file (e.g.,database_cleanupstep indelete), consider usingbegin immediate.However, since the operation is idempotent and the window is small, this is a minor concern.
♻️ Optional: Use `begin immediate` for consistency
- db.run("begin"); + db.run("begin immediate");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/lanes/laneService.ts` around lines 4142 - 4240, The DB update in detachVmLane uses db.run("begin") which can allow concurrent readers; change it to acquire a write lock by using db.run("begin immediate") before updating runtime_placement in detachVmLane (and keep the existing commit/rollback handling and invalidateLaneListCache flow), so the sequence surrounding the update of row.id/projectId is executed inside an immediate transaction to prevent concurrency races.apps/desktop/src/renderer/components/vm/MacVmComingSoon.tsx (1)
6-10: ⚡ Quick winRename
PREVIEW_ITEMSto camelCase to match the repo rule.This constant is newly introduced and violates the TS/JS naming guideline.
As per coding guidelines `**/*.{ts,tsx,js,jsx}`: Use camelCase for variable names in TypeScript/JavaScript files.♻️ Proposed rename
-const PREVIEW_ITEMS: ReadonlyArray<{ +const previewItems: ReadonlyArray<{ icon: Icon; title: string; body: string; }> = [ @@ - {PREVIEW_ITEMS.map(({ icon: Icon, title, body }) => ( + {previewItems.map(({ icon: Icon, title, body }) => (Also applies to: 76-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/vm/MacVmComingSoon.tsx` around lines 6 - 10, Rename the exported constant PREVIEW_ITEMS to camelCase (e.g., previewItems) throughout the file; update the declaration of PREVIEW_ITEMS to previewItems and update every usage/reference (including any JSX map or imports within MacVmComingSoon.tsx) to the new identifier to satisfy the TS/JS naming guideline and ensure all references (e.g., map calls, prop passes) compile.apps/desktop/src/renderer/lib/macosVmRuntimeReadiness.ts (1)
10-10: ⚡ Quick winUse camelCase for the storage-key constant.
This new constant name conflicts with the TS/JS variable naming rule in this repo.
As per coding guidelines `**/*.{ts,tsx,js,jsx}`: Use camelCase for variable names in TypeScript/JavaScript files.♻️ Proposed rename
-export const MACOS_VM_RUNTIME_AUTH_STORAGE_KEY = "ade.vm.runtimeAuthConfirmed.v1"; +export const macosVmRuntimeAuthStorageKey = "ade.vm.runtimeAuthConfirmed.v1"; @@ - return window.localStorage.getItem(MACOS_VM_RUNTIME_AUTH_STORAGE_KEY) === "1"; + return window.localStorage.getItem(macosVmRuntimeAuthStorageKey) === "1"; @@ - if (value) window.localStorage.setItem(MACOS_VM_RUNTIME_AUTH_STORAGE_KEY, "1"); - else window.localStorage.removeItem(MACOS_VM_RUNTIME_AUTH_STORAGE_KEY); + if (value) window.localStorage.setItem(macosVmRuntimeAuthStorageKey, "1"); + else window.localStorage.removeItem(macosVmRuntimeAuthStorageKey);Also applies to: 14-14, 22-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/lib/macosVmRuntimeReadiness.ts` at line 10, The constant MACOS_VM_RUNTIME_AUTH_STORAGE_KEY should be renamed to camelCase (e.g., macosVmRuntimeAuthStorageKey) to comply with the repo's TS/JS naming rules; update its declaration in macosVmRuntimeReadiness.ts and replace all references/usages (imports, local reads/writes, and any tests) to use the new name, and ensure any export remains consistent (export const macosVmRuntimeAuthStorageKey = ...). Also update the other related occurrences referenced in the review (the other constants/uses in the same file) to camelCase so naming is consistent across the module.apps/desktop/src/main/main.ts (1)
3402-3413: 💤 Low valueConsider defining an explicit interface for the optional VM hooks.
The broad type assertion
as unknown as {...}works but obscures the contract. A dedicated interface (e.g.,MacosVmServiceHooks) exported alongsidecreateMacosVmServicewould make the expected shape explicit and catch mismatches at compile time.This is a style improvement for maintainability rather than a correctness issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/main.ts` around lines 3402 - 3413, The inline broad assertion for macosVmSvcAny hides the expected optional VM hook shape; define and export a dedicated interface (e.g., MacosVmServiceHooks) describing markShareStale, stopMirrorSyncForLane, startMirrorSyncForLane, linkLaneToCurrentVm, getStatus, and getCredentials, then replace the ad-hoc cast with a typed reference (use MacosVmServiceHooks or a combined type for macosVmService) and update createMacosVmService’s exported types so the compiler can check the contract instead of using as unknown as {...}.apps/desktop/src/main/services/adeActions/registry.ts (1)
783-783: ⚡ Quick winFormat the action array across multiple lines for consistency.
The
macos_vmallowlist contains 20 actions on a single line, which reduces readability and makes diff reviews difficult. Other long allowlists in this file (e.g.,lane,git,conflicts) use multi-line formatting.📝 Suggested reformatting
- macos_vm: ["getStatus", "getStorageInfo", "provision", "start", "stop", "restart", "delete", "wipe", "installRuntime", "setCredentials", "getCredentials", "detachLane", "getAgentGuide", "getSharePolicy", "focusWindow", "getDisplaySession", "captureScreenshot", "click", "selectPoint", "typeText"], + macos_vm: [ + "captureScreenshot", + "click", + "delete", + "detachLane", + "getAgentGuide", + "getCredentials", + "getDisplaySession", + "getSharePolicy", + "getStatus", + "getStorageInfo", + "focusWindow", + "installRuntime", + "provision", + "restart", + "selectPoint", + "setCredentials", + "start", + "stop", + "typeText", + "wipe", + ],Note: Actions are sorted alphabetically in the suggestion above for easier scanning and maintenance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/adeActions/registry.ts` at line 783, The macos_vm allowlist in registry.ts is currently a single long line; split the array for the macos_vm key into multiple lines (one action per line or grouped sensibly) to match the multi-line style used by other allowlists like lane, git, and conflicts; while you’re editing, alphabetize the action strings for easier scanning and ensure you update the macos_vm entry name exactly as spelled so the rest of the registry (and any references to macos_vm) continue to work.apps/desktop/src/main/services/ipc/ipcTimeouts.ts (1)
7-22: ⚡ Quick winUse camelCase for the runtime action map variable.
Rename
RUNTIME_ACTION_CHANNELtoruntimeActionChannelto match repository naming rules for TypeScript variables.As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Use camelCase for variable names in TypeScript/JavaScript files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/ipc/ipcTimeouts.ts` around lines 7 - 22, Rename the constant RUNTIME_ACTION_CHANNEL to camelCase runtimeActionChannel everywhere it is defined and referenced; update the declaration const RUNTIME_ACTION_CHANNEL: Record<string, Record<string, string>> = { ... } to const runtimeActionChannel: Record<string, Record<string, string>> = { ... } and adjust any imports/uses that reference RUNTIME_ACTION_CHANNEL (e.g., in code that reads IPC mappings) to use runtimeActionChannel instead so tests/consumers compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 6790-6804: readVmTarget currently only reads the lane from flags
via readValue(args, ["--lane", "--lane-id"]) and ignores a positional lane
argument, causing commands like "ade macos-vm wipe <lane>" to drop the lane;
update readVmTarget to also consume the positional lane when laneId is undefined
(e.g. check args._ or call the existing positional reader used by sibling macOS
VM commands) so laneId is populated from a positional argument before throwing
or returning; modify readVmTarget (and use readVmName/readValue) to prefer
explicit flags but fall back to the positional lane value.
In `@apps/desktop/src/main/services/adeActions/registry.ts`:
- Line 783: The macos_vm allowlist includes an unimplemented action "detachLane"
which will cause runtime failures; remove "detachLane" from the macos_vm array
in the registry (the array containing "getStatus", "getStorageInfo", ...
"typeText") so only methods actually exported by createMacosVmService() are
allowed; verify the registry's macos_vm entry matches the methods implemented by
createMacosVmService() and update any tests or callers accordingly.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 2347-2355: resolveMacosVmProjectRootForEvent currently falls back
to getCtx().project?.rootPath without checking whether the user has actually
selected a project; change the fallback so you only return
getCtx().project.rootPath when getCtx().hasUserSelectedProject is true
(otherwise throw the same error). Locate the function
resolveMacosVmProjectRootForEvent and its use of getCtx(), add a guard that
checks getCtx().hasUserSelectedProject before reading getCtx().project.rootPath,
and keep the existing error message when the guard fails so the function fails
closed instead of returning another project's path.
- Around line 7684-7756: The handlers currently cast ensureMacosVm() and
getCtx().laneService to broad Record types which loses compile-time checking;
instead declare a narrow interface (e.g., MacosVmExtension with optional methods
restart, wipe, installRuntime, setCredentials, getCredentials, getStorageInfo)
and cast ensureMacosVm() to that interface in callMacosVmExtension so TypeScript
can verify signatures, then keep the existing runtime typeof checks before
invoking svc.restart(args) etc; likewise define a small LaneService interface
with optional detachVmLane and cast getCtx().laneService to it before checking
and calling detachVmLane inside the IPC.macosVmDetachLane handler.
In `@apps/desktop/src/main/services/macosVm/runtimeBootstrap.ts`:
- Around line 217-219: The temporary bootstrap script and directory (tempDir and
localScriptPath created with fs.mkdtempSync and GUEST_BOOTSTRAP_SCRIPT) are
never removed; wrap the install/bootstrap flow (the block that writes
localScriptPath and the subsequent install/exec logic in the runtimeBootstrap
routine) in a try/finally so that in finally you remove localScriptPath and
remove tempDir if it was created by this function (i.e., only delete tempDir
when args.tempDir was not supplied). Use fs.unlinkSync/fs.rmSync for the file
and fs.rmdirSync or fs.rmSync for the directory, and swallow non-fatal ENOENT
errors so cleanup is best-effort; apply the same cleanup around the code paths
handling lines ~221-274 to ensure no leaks after successful or failed install
attempts.
- Around line 23-24: Hardcoded SSHPASS_BINARY path breaks on systems where
sshpass is installed elsewhere; add a resolver (e.g., resolveSshpassPath) in
runtimeBootstrap.ts that checks an ordered list of candidate locations (e.g.,
"/opt/homebrew/bin/sshpass", "/usr/local/bin/sshpass", common MacPorts/source
locations) and falls back to a PATH lookup (which/where or
child_process.execSync('which sshpass')) and return undefined or null if not
found, then replace the SSHPASS_BINARY constant with the resolver result and
update usages that reference SSHPASS_BINARY (the calls around the previous line
156 and 177) to handle a missing sshpass gracefully (error or alternate auth
flow); keep SCP_BINARY as-is or optionally resolve similarly.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 5995-5998: getDisplaySession and the other macOS VM IPC methods
are currently invoking ipcRenderer.invoke(...) directly and must be routed
through the remote-runtime-first dispatcher; update getDisplaySession (and the
similar methods handling
restart/wipe/runtime/credentials/detach/storage/display) to call
callRemoteProjectRuntimeActionOr(...) (the same wrapper used elsewhere in
preload) so the request is resolved against the remote "macos_vm" target when
appropriate, passing the original IPC channel (e.g.
IPC.macosVmGetDisplaySession) and args through that helper and returning its
result.
In `@apps/desktop/src/renderer/components/app/App.tsx`:
- Around line 331-336: serializeProjectRoute currently omits the new "/vm" route
from its allowlist, so project-route persistence can drop users back to "/work";
update the allowlist used by serializeProjectRoute to include "/vm" (and any
equivalent route alias such as "/macos-vm" if you normalize there), and ensure
the corresponding deserialization logic (e.g., deserializeProjectRoute or route
deserializer) accepts/restores "/vm" so switching project tabs preserves the VM
route.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 2009-2019: prepareCreateDialog currently preselects "macos-vm"
even when readMacosVmRuntimeAuthConfirmed() is false, allowing downstream
VM-status fetches to bypass the auth-confirm gate; change the logic inside
prepareCreateDialog (and the other dialog-preparation handlers that preselect
macos-vm) to first read authConfirmed = readMacosVmRuntimeAuthConfirmed(), call
setCreateVmRuntimeAuthConfirmed(authConfirmed), and only call
setCreateRuntimePlacement(runtimePlacement) with the normalized "macos-vm" value
if authConfirmed is true—otherwise setCreateRuntimePlacement to a safe default
("primary") and avoid kicking off any VM-status loading/fetch (leave
setCreateVmStatusLoading false and setCreateVmRuntimeAvailable false). Ensure
the same guard is applied in the other preparer functions mentioned so
deep-links and dialog-bus opens cannot preselect macos-vm without confirmation.
- Around line 3961-3965: The onOpenVmLaneInWork callback is routing to "/work"
but the component elsewhere opens the Work page via "/project" (see onOpenVmTab
and the earlier usage at line ~3884); update the callback so after calling
selectLane(laneId) it navigates to the same Work route used elsewhere (replace
navigate("/work") with navigate("/project")) to ensure the VM CTA lands on the
correct page.
In `@apps/desktop/src/renderer/components/vm/FirstBootCard.tsx`:
- Around line 120-122: The code dereferences currentStep (used as
currentStep.number/title) without guarding an empty steps array: compute
safeIndex using Math.min/Math.max can yield -1 when steps.length === 0; update
the logic in FirstBootCard to early-handle an empty steps array (e.g., if
(steps.length === 0) return null or render an empty/placeholder state) or set
currentStep to null and guard all uses, and adjust canAdvance to check
steps.length > 0 before comparing (for example use const canAdvance =
steps.length > 0 && safeIndex < steps.length - 1); ensure all references to
currentStep.number/title are only accessed when currentStep is not null.
In `@apps/desktop/src/renderer/components/vm/MacVmPage.tsx`:
- Around line 1342-1344: The generated shell command `command` is unsafe because
wrapping paths in double quotes doesn't protect against shell metacharacters
like $(...) — update the construction used in MacVmPage (symbols: command,
paths, copy) to perform proper shell escaping: for each path, wrap it in single
quotes and escape any embedded single quotes by replacing each ' with the
three-token sequence that closes, escapes, and reopens the single-quoted string
(i.e. '\''), then join those safely-quoted path tokens with spaces and use that
safe string in the copy callback; keep the rest of the logic
(navigator.clipboard.writeText in copy) unchanged.
In `@apps/desktop/src/renderer/components/vm/PhaseStepper.tsx`:
- Around line 16-19: The type annotation React.CSSProperties will fail because
only ReactNode is imported; import CSSProperties from React (add CSSProperties
to the existing import) and change the baseStyle declaration to use
CSSProperties (e.g., const baseStyle: CSSProperties = { ... }); update any other
occurrences of React.CSSProperties in this file to CSSProperties and keep the
original baseStyle symbol and property names intact.
In `@apps/desktop/src/renderer/lib/macosVmRuntimeReadiness.ts`:
- Around line 139-140: Validate vm.currentPhase before returning it: ensure it's
a number within the bounds of MACOS_VM_PHASES (e.g., 0 <= vm.currentPhase <
MACOS_VM_PHASES.length) and not NaN; if it fails validation, fall back to the
existing computed state (readiness?.state ?? vm.guestReadiness?.state ??
"not_created") or convert that state into a safe phase index. Update the logic
around vm.currentPhase usage in macosVmRuntimeReadiness (where vm.currentPhase
is checked) and any later indexing into MACOS_VM_PHASES (also referenced around
lines 169-175) to perform the same guard so lookups cannot receive an
out-of-range or invalid index.
In `@apps/desktop/src/shared/types/macosVm.ts`:
- Around line 410-411: Change the property type for the public contract from a
boolean to the literal true by replacing "confirm: boolean" with "confirm: true"
so callers must pass true at compile time; update the type declaration that
contains the confirm field (the 'confirm' property shown in macosVm types) and
leave or add runtime validation where the wipe operation is invoked to guard
against accidental use.
---
Nitpick comments:
In `@apps/desktop/src/main/main.ts`:
- Around line 3402-3413: The inline broad assertion for macosVmSvcAny hides the
expected optional VM hook shape; define and export a dedicated interface (e.g.,
MacosVmServiceHooks) describing markShareStale, stopMirrorSyncForLane,
startMirrorSyncForLane, linkLaneToCurrentVm, getStatus, and getCredentials, then
replace the ad-hoc cast with a typed reference (use MacosVmServiceHooks or a
combined type for macosVmService) and update createMacosVmService’s exported
types so the compiler can check the contract instead of using as unknown as
{...}.
In `@apps/desktop/src/main/services/adeActions/registry.ts`:
- Line 783: The macos_vm allowlist in registry.ts is currently a single long
line; split the array for the macos_vm key into multiple lines (one action per
line or grouped sensibly) to match the multi-line style used by other allowlists
like lane, git, and conflicts; while you’re editing, alphabetize the action
strings for easier scanning and ensure you update the macos_vm entry name
exactly as spelled so the rest of the registry (and any references to macos_vm)
continue to work.
In `@apps/desktop/src/main/services/ipc/ipcTimeouts.ts`:
- Around line 7-22: Rename the constant RUNTIME_ACTION_CHANNEL to camelCase
runtimeActionChannel everywhere it is defined and referenced; update the
declaration const RUNTIME_ACTION_CHANNEL: Record<string, Record<string, string>>
= { ... } to const runtimeActionChannel: Record<string, Record<string, string>>
= { ... } and adjust any imports/uses that reference RUNTIME_ACTION_CHANNEL
(e.g., in code that reads IPC mappings) to use runtimeActionChannel instead so
tests/consumers compile.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 4247-4309: attachLaneToVm currently updates runtime_placement
before linking to the VM, leaving a crash window; wrap the initial placement
flip in a DB transaction so the UPDATE to set runtime_placement = 'macos-vm' is
only committed after hooks.linkLaneToCurrentVm succeeds. Concretely: in
attachLaneToVm begin a transaction, perform the UPDATE there (instead of the
standalone db.run), call hooks.linkLaneToCurrentVm while still in the
transaction, commit the transaction on success and then run
invalidateLaneListCache and emitPlacementChanged; on hook failure rollback the
transaction (so no separate rollback UPDATE is needed) and then run
invalidateLaneListCache and rethrow the error; call hooks.startMirrorSyncForLane
after the commit. Reference: attachLaneToVm, linkLaneToCurrentVm,
startMirrorSyncForLane (and mirror detachVmLane behavior for guidance).
- Around line 4142-4240: The DB update in detachVmLane uses db.run("begin")
which can allow concurrent readers; change it to acquire a write lock by using
db.run("begin immediate") before updating runtime_placement in detachVmLane (and
keep the existing commit/rollback handling and invalidateLaneListCache flow), so
the sequence surrounding the update of row.id/projectId is executed inside an
immediate transaction to prevent concurrency races.
In `@apps/desktop/src/renderer/components/terminals/WorkStartSurface.test.tsx`:
- Around line 110-124: The local-lane test "does not render the VM banner for a
local lane" is missing the third VM-related assertion for symmetry with the
other test; update the WorkStartSurface test to also assert that the "Open VM
tab" text is not present (i.e., check that screen.queryByText("Open VM tab") is
null) or, if there's a reason to omit it, add a brief inline comment in that
test explaining why the "Open VM tab" assertion is intentionally skipped so the
test expectations remain explicit and consistent with the other VM-related
assertions ("work-vm-banner" and "work-vm-not-ready").
In `@apps/desktop/src/renderer/components/vm/MacVmComingSoon.tsx`:
- Around line 6-10: Rename the exported constant PREVIEW_ITEMS to camelCase
(e.g., previewItems) throughout the file; update the declaration of
PREVIEW_ITEMS to previewItems and update every usage/reference (including any
JSX map or imports within MacVmComingSoon.tsx) to the new identifier to satisfy
the TS/JS naming guideline and ensure all references (e.g., map calls, prop
passes) compile.
In `@apps/desktop/src/renderer/lib/macosVmRuntimeReadiness.ts`:
- Line 10: The constant MACOS_VM_RUNTIME_AUTH_STORAGE_KEY should be renamed to
camelCase (e.g., macosVmRuntimeAuthStorageKey) to comply with the repo's TS/JS
naming rules; update its declaration in macosVmRuntimeReadiness.ts and replace
all references/usages (imports, local reads/writes, and any tests) to use the
new name, and ensure any export remains consistent (export const
macosVmRuntimeAuthStorageKey = ...). Also update the other related occurrences
referenced in the review (the other constants/uses in the same file) to
camelCase so naming is consistent across the module.
In `@apps/desktop/src/renderer/types/novnc.d.ts`:
- Around line 1-24: Add missing noVNC RFB members and event typings to the
ambient declaration so TypeScript covers additional features your code uses:
extend the exported class RFB (constructor, viewOnly, etc.) with methods like
sendKey, sendCtrlAltDel, grabKeyboard, ungrabKeyboard, clipboardPasteFrom,
pasteString/clipboardCopy methods you use, and add strongly typed event names
(e.g., 'connect', 'disconnect', 'clipboard', 'bell', 'clipboard',
'securityfailure', 'credentialsrequired') by augmenting the EventTarget typing
or adding addEventListener/removeEventListener overloads for those string
literals; also expand RfbOptions/RfbCredentials if you rely on extra fields.
Update the declaration for class RFB and the RfbOptions/RfbCredentials types
accordingly so callers of RFB.sendCredentials, RFB.sendKey, and event handlers
are properly typed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7af9bed0-317f-4357-83de-ee60220f5572
⛔ Files ignored due to path filters (9)
apps/desktop/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsondocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/computer-use/README.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**docs/perf/macos-vm-tab-action-inventory.mdis excluded by!docs/**docs/perf/work-tab-action-inventory.mdis excluded by!docs/**
📒 Files selected for processing (68)
.agents/skills/plan/SKILL.md.agents/skills/source-command-audit/SKILL.mdapps/ade-cli/README.mdapps/ade-cli/src/cli.tsapps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsxapps/ade-cli/src/tuiClient/components/Drawer.tsxapps/desktop/package.jsonapps/desktop/resources/agent-skills/ade-macos-vm/SKILL.mdapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/ipc/ipcTimeouts.test.tsapps/desktop/src/main/services/ipc/ipcTimeouts.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.test.tsapps/desktop/src/main/services/lanes/laneLaunchContext.test.tsapps/desktop/src/main/services/lanes/laneLaunchContext.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/macosVm/credentialsStore.tsapps/desktop/src/main/services/macosVm/macosVmRecovery.tsapps/desktop/src/main/services/macosVm/macosVmService.test.tsapps/desktop/src/main/services/macosVm/macosVmService.tsapps/desktop/src/main/services/macosVm/rfbDirectClient.tsapps/desktop/src/main/services/macosVm/runtimeBootstrap.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/TabNav.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialog.test.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsxapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/terminals/MacosVmPanel.test.tsxapps/desktop/src/renderer/components/terminals/MacosVmPanel.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.test.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.test.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/ui/TabBackground.tsxapps/desktop/src/renderer/components/vm/CredentialsPromptDialog.tsxapps/desktop/src/renderer/components/vm/CurrentVmLaneRow.tsxapps/desktop/src/renderer/components/vm/FirstBootCard.tsxapps/desktop/src/renderer/components/vm/MacMiniGlyph.tsxapps/desktop/src/renderer/components/vm/MacVmComingSoon.tsxapps/desktop/src/renderer/components/vm/MacVmPage.test.tsxapps/desktop/src/renderer/components/vm/MacVmPage.tsxapps/desktop/src/renderer/components/vm/PhaseStepper.tsxapps/desktop/src/renderer/components/vm/VmLifecycleMenu.tsxapps/desktop/src/renderer/index.cssapps/desktop/src/renderer/lib/macosVmRuntimeReadiness.tsapps/desktop/src/renderer/lib/visualContextFormatting.tsapps/desktop/src/renderer/lib/workPtyContextEvents.tsapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/renderer/types/novnc.d.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/lanes.tsapps/desktop/src/shared/types/macosVm.tsapps/ios/ADE/Resources/DatabaseBootstrap.sql
💤 Files with no reviewable changes (4)
- apps/desktop/src/renderer/lib/workPtyContextEvents.ts
- apps/desktop/src/renderer/components/terminals/MacosVmPanel.tsx
- apps/desktop/src/renderer/components/terminals/MacosVmPanel.test.tsx
- apps/desktop/src/renderer/components/terminals/WorkSidebar.tsx
- runtimeBootstrap stub: mark install state failed instead of advancing to runtime_ready - runtimeBootstrap: resolveSshpassBinary() walks Homebrew/MacPorts/PATH - runtimeBootstrap: try/finally cleans staged install scripts - credentialsStore: SIGTERM → SIGKILL 500ms; settle promise on timeout - macosVmService: .part.url sidecar so partial download recovery is URL-keyed - macosVmService: /usr/bin/rsync absolute path - registerIpc: hasUserSelectedProject guard on project-root fallback - registerIpc: requireMacosVmEnabledInProduction gates VM IPC in packaged builds - registerIpc: typed MacosVmExtensionService interface (removed unknown casts) - adeActions/registry: drop detachLane (it lives on laneService, not macosVmService) - preload: route new VM IPC through callRemoteProjectRuntimeActionOr - App: /vm joins serializeProjectRoute allowedRoots - LanesPage: auth-confirm gate on status fetch + createVmRuntimeAvailable - LanesPage: fresh submit recheck mirrors full availability predicate - LanesPage: open-in-Work CTA navigates to /project - ade-cli: readVmTarget consumes positional lane arg - FirstBootCard: empty-steps guard - MacVmPage: shellSingleQuote() for safe sudo rm -rf cleanup string - PhaseStepper: import CSSProperties directly - macosVmRuntimeReadiness: validate vm.currentPhase bounds - shared/types/macosVm: confirm: true literal on wipe args Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Address Greptile P1: defaultRunner's per-phase timeout sent SIGTERM but waited unconditionally for the exit event. If SSH/scp blocked in a kernel wait, SIGTERM was ignored and the promise never settled. Mirror the credentialsStore pattern: settle promise on timeout, schedule a SIGKILL 500ms after SIGTERM. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/ade-cli/src/cli.ts (1)
6798-6799: ⚡ Quick winReuse
readVmLaneId(false)here to keep VM target parsing in one place.This reimplements the lane parser that already exists in
readVmLaneId, which is the same kind of split that caused the positional-lane regression before. Reusing the helper here lowers the chance these code paths drift again.♻️ Proposed fix
- const laneId = - readValue(args, ["--lane", "--lane-id"]) ?? firstPositional(args); + const laneId = readVmLaneId(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/cli.ts` around lines 6798 - 6799, Replace the ad-hoc lane parsing that sets const laneId = readValue(args, ["--lane", "--lane-id"]) ?? firstPositional(args); with a call to the existing helper readVmLaneId(false) so VM target parsing is centralized; locate the code that declares laneId in this block and use readVmLaneId(false) to obtain the lane id instead of readValue/firstPositional (keeping the variable name laneId and the same downstream usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/adeActions/registry.ts`:
- Around line 783-786: The macos_vm allowlist currently exposes sensitive
actions setCredentials and getDisplaySession to any caller; move these actions
behind the CTO-only gate used by run_ade_action (ADE_ACTION_CTO_ONLY) or remove
them from the generic macos_vm allowlist so only callers validated as CTO can
invoke them. Locate the macos_vm array in registry.ts and either remove
"setCredentials" and "getDisplaySession" from that list and add them to the
CTO-only action set, or annotate/route their invocation to laneService calls
that enforce ADE_ACTION_CTO_ONLY (e.g., ensure run_ade_action checks
ADE_ACTION_CTO_ONLY for those action names) so that only CTO-authorized callers
can access them.
In `@apps/desktop/src/main/services/macosVm/credentialsStore.ts`:
- Around line 88-118: The promise is clearing the SIGKILL fallback timer too
early: in finish() it clears killTimer which prevents the SIGKILL from running
if SIGTERM is ignored. Remove the clearTimeout(killTimer) call from finish (or
only clear the main timeout there), and initialize killTimer so it can be set
later (e.g., let killTimer: ReturnType<typeof setTimeout> | undefined), ensuring
only the main timeout is cleared when resolving; leave killTimer untouched so
the 500ms SIGKILL fallback scheduled in the timeout handler (which calls
child.kill("SIGKILL")) can still run.
In `@apps/desktop/src/main/services/macosVm/runtimeBootstrap.ts`:
- Around line 105-130: The timeout handler arms a SIGKILL fallback via killTimer
then calls finish(), but finish() clears killTimer which prevents the SIGKILL
escalation; change the logic so that finish() does not unconditionally clear
killTimer (or accept a flag like finish(fromTimeout: boolean)) and only clear
killTimer when the child has actually exited normally. Specifically adjust the
functions/variables referenced (killTimer, finish, timeout, child.kill, settled,
stdout/stderr, options.timeoutMs) so the timeout path sets killTimer and calls
finish without cancelling that killTimer, while non-timeout/normal-exit paths
still clear the killTimer to avoid leaks.
---
Nitpick comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 6798-6799: Replace the ad-hoc lane parsing that sets const laneId
= readValue(args, ["--lane", "--lane-id"]) ?? firstPositional(args); with a call
to the existing helper readVmLaneId(false) so VM target parsing is centralized;
locate the code that declares laneId in this block and use readVmLaneId(false)
to obtain the lane id instead of readValue/firstPositional (keeping the variable
name laneId and the same downstream usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87238303-d471-4307-bcbc-a8e0eadd54de
📒 Files selected for processing (15)
apps/ade-cli/src/cli.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/macosVm/credentialsStore.tsapps/desktop/src/main/services/macosVm/macosVmService.test.tsapps/desktop/src/main/services/macosVm/macosVmService.tsapps/desktop/src/main/services/macosVm/runtimeBootstrap.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/vm/FirstBootCard.tsxapps/desktop/src/renderer/components/vm/MacVmPage.tsxapps/desktop/src/renderer/components/vm/PhaseStepper.tsxapps/desktop/src/renderer/lib/macosVmRuntimeReadiness.tsapps/desktop/src/shared/types/macosVm.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/components/vm/PhaseStepper.tsx
…ighten action allowlist - credentialsStore + runtimeBootstrap: prior fix cleared killTimer in finish() unconditionally, defeating the 500 ms SIGKILL fallback. Pass clearKill=false from the timeout path so the SIGKILL still fires if SIGTERM is ignored. (CodeRabbit major, credentialsStore.ts:118 + runtimeBootstrap.ts:130) - macosVmService.openExternalVncClient: vnc:// URL no longer embeds the credential — it appeared in argv to /usr/bin/open (ps aux visible) for the brief lifetime of the child. macOS Screen Sharing prompts when no Keychain entry exists; matches the SIGSAFE pattern used for the SSH path with sshpass -e. (Greptile P1 security, macosVmService.ts:1487) - adeActions/registry macos_vm allowlist: removed setCredentials and getDisplaySession from the generic agent-callable surface. setCredentials mutates Keychain-backed VM credentials; getDisplaySession returns the live VNC password. Typed CLI / IPC paths (ade macos-vm set-credentials, ade macos-vm display-session) still work and run under CTO-level auth. (CodeRabbit major, registry.ts:786) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile P1: registry.ts allowlists macos_vm.restart/wipe/installRuntime (plus focusWindow/click/selectPoint/typeText) for agent invocation, but the RUNTIME_ACTION_CHANNEL map only covered provision/start/stop/delete/ captureScreenshot. Agent calls through localRuntimeCallAction / remoteRuntimeCallAction were falling back to the 30s default — restart sequences stop+start (up to 122 min), wipe drives deleteVm (~2 min), and installRuntime SSHes into the guest. All three were racing-lose to 30s. Map each agent-callable action to its IPC channel so the existing per-channel budgets apply. Add the matching cases to the switch (restart/installRuntime share start's 120-min budget; wipe shares delete's 2-min; UI actions share captureScreenshot's 60s). Extend tests to cover restart / wipe / installRuntime via the runtime channel and a UI action via the remote runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…che TTL
Greptile P1 (ipcTimeouts.ts:54): macosVmWipe was capped at 2 min IPC budget,
but wipe() and deleteVm() both call runLume("delete", 10 * 60_000) internally.
The IPC's Promise.race in main.ts was firing first — renderer saw "timed out"
while the underlying lume process kept running, leaving the store record
unchanged. Lift both wipe and delete IPC budgets to 10 min so the renderer
sees the real outcome. Update test to match.
Greptile P1 (laneLaunchContext.ts:207): VM_LAUNCH_CONTEXT_TTL_MS was 5_000 ms,
but the only refresh path is the placement-changed handler that fires once
when a lane attaches to a VM. agentChatService and ptyService call the sync
resolveLaneLaunchContext on every turn — after 5 s the cache returns null and
throws VmNotReadyError even when the VM is fine. The cached SSH target
(IP + username + vmName) is stable for the running VM lifetime; lifecycle
events explicitly invalidate the cache. Bump TTL to 30 min so the cache stays
warm across normal agent sessions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| onEvent: (payload) => | ||
| emitProjectEvent(projectRoot, IPC.macosVmEvent, payload), | ||
| captureWindowSources: async () => { |
There was a problem hiding this comment.
setCredentials never refreshes the VM launch cache
After a user saves guest credentials via setCredentials, the vmLaunchContextCache is never invalidated. The most likely setup flow is: (1) create VM lane while VM is runtime_ready but credentials haven't been saved yet → refreshVmLaneLaunchCache caches not-ready; (2) user saves credentials; (3) agent turn fires → getCachedVmLaunchContextSync returns not-ready → VmNotReadyError thrown for every turn for up to 30 minutes until the TTL expires. invalidateVmLaneLaunchCache(laneId) (or a full refreshVmLaneLaunchCache) needs to be called from the setCredentials IPC path after a successful credential save.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/main.ts
Line: 3378-3380
Comment:
**`setCredentials` never refreshes the VM launch cache**
After a user saves guest credentials via `setCredentials`, the `vmLaunchContextCache` is never invalidated. The most likely setup flow is: (1) create VM lane while VM is `runtime_ready` but credentials haven't been saved yet → `refreshVmLaneLaunchCache` caches `not-ready`; (2) user saves credentials; (3) agent turn fires → `getCachedVmLaunchContextSync` returns `not-ready` → `VmNotReadyError` thrown for every turn for up to 30 minutes until the TTL expires. `invalidateVmLaneLaunchCache(laneId)` (or a full `refreshVmLaneLaunchCache`) needs to be called from the `setCredentials` IPC path after a successful credential save.
How can I resolve this? If you propose a fix, please make it concise.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
Greptile Summary
This PR adds full macOS VM support to the desktop app: provision/start/stop/wipe/restart lifecycle, IPSW-based provisioning with resume, SSH-backed lane execution, a WebSocket VNC display proxy, Keychain-backed credential storage, phase-based onboarding UI, and hardened lane deletion for guest-created unreadable files.
laneLaunchContext.ts): TTL raised to 30 minutes and the comment claims lifecycle events drive invalidation, but onlyplacement-changedevents actually callrefreshVmLaneLaunchCache. Credential saves and VM stop/start do not invalidate the cache, leaving agents blocked or pointing at stale SSH targets.credentialsStore.ts): New Keychain-backed store shells out tosecurity add-generic-password -w <json>, passing the full credential payload as a CLI argument where it is briefly visible in the process table.ipcTimeouts.ts): All three new VM operations now carry correct long timeouts, resolving previous review findings on that file.Confidence Score: 3/5
The VM feature is partially gated by a runtime-install stub that prevents
runtime_readyfrom being reached in production, but the cache wiring gaps will bite immediately once the stub is replaced.Two concrete correctness gaps exist in the launch-context cache: credential saves and VM lifecycle changes (stop/start) do not call
refreshVmLaneLaunchCacheorinvalidateVmLaneLaunchCache, meaning anot-readyentry can block agent turns for up to 30 minutes after credentials are saved, and areadyentry can persist after the VM stops. These gaps are currently masked by the runtime-install stub, but will cause immediate workflow breakage when the stub is completed. TheonRuntimeReadyhook is wired in main.ts but never called in the service, adding another gap in the same critical path.apps/desktop/src/main/main.ts (cache invalidation for setCredentials and VM lifecycle events), apps/desktop/src/main/services/macosVm/credentialsStore.ts (credential in CLI args), apps/desktop/src/main/services/macosVm/macosVmService.ts (onRuntimeReady never called)
Security Review
credentialsStore.tsline 173): The full credential payload —{"username":"…","password":"…","savedAt":"…"}— is passed as the-wargument to/usr/bin/security add-generic-password. It is briefly visible in the process table to any process running as the same macOS user. The SSH password is kept out of argv via theSSHPASSenv var, but no equivalent protection exists here.Important Files Changed
setCredentialsnever callsrefreshVmLaneLaunchCache, andonEventfor macosVmService doesn't invalidate the cache on VM lifecycle changes.securityCLI. Well-structured with SIGKILL fallback, but the full credential JSON (including plaintext password) is passed as-w payloadon the CLI.onRuntimeReadyhook declared but never called.Comments Outside Diff (4)
apps/desktop/src/main/services/macosVm/macosVmService.ts, line 1029-1031 (link)The VNC password is embedded directly in the
vnc://URL and passed as a command-line argument to/usr/bin/open. Any local process can read it from the process table viaps auxor/proc/<pid>/cmdlinefor the brief time theopencommand runs. This is the exact threat the codebase already mitigates for SSH passwords (usingSSHPASSenv var so the secret never appears in argv). The same precaution is missing here.Prompt To Fix With AI
apps/desktop/src/main/services/ipc/ipcTimeouts.ts, line 38-95 (link)macosVmInstallRuntime,macosVmRestart, andmacosVmWipeare not listed in the switch statement, so they all fall through to the 30-second default. Each of these operations far exceeds 30 s:installRuntimeruns an SSHrun-scriptphase with a 120-second internal timeout,restartsequences throughstop(2 min) +start(up to 120 min), andwipecallsdeleteVmwith a 10-minute lume-delete budget. Because the main process wraps everyipcMain.handleinPromise.race([listener, timeoutPromise]), all three handlers will always reject with "IPC handler timed out" on any non-trivial guest or VM, while the underlying SSH/lume process continues running in the background — leaving the record state out of sync with what the renderer observed.Prompt To Fix With AI
apps/desktop/src/main/services/ipc/ipcTimeouts.ts, line 46-54 (link)macosVmInstallRuntime,macosVmRestart, andmacosVmWipeare not in the switch, so they all get 30 s. Each exceeds that trivially:installRuntimedrives a 120-second SSHrun-scriptphase;restartsequences throughstop(2 min) thenstart(up to 120 min);wipecallsdeleteVmwhich has a 10-minute budget. Becausemain.tswraps everyipcMain.handleinPromise.race([listener, timeoutPromise]), all three handlers will always reject with "IPC handler timed out" on any real VM while the underlying SSH / lume process keeps running in the background, leaving record state out of sync with what the renderer observed.Prompt To Fix With AI
apps/desktop/src/main/main.ts, line 3374-3380 (link)onEventformacosVmServiceonly forwards payloads to the renderer viaemitProjectEvent. When the VM stops, areadycache entry (containing the SSH target) stays valid for up to 30 minutes. An agent starting a turn on a stopped VM will attempt SSH and hang until timeout, rather than getting a fastVmNotReadyError. The comment inlaneLaunchContext.tsclaims "lifecycle events (start, stop, restart, placement-changed, deleted) drive explicit invalidation" but onlyplacement-changedactually does. TheonEventhandler should callinvalidateVmLaneLaunchCache()onvm-updatedevents where the new state is notrunning/runtime_ready.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (6): Last reviewed commit: "ship: iter 5 — match wipe/delete IPC bud..." | Re-trigger Greptile