Conversation
Add a macOS universal build and signing flow to CI and the repo: introduce a build-mac-app matrix job that produces arm64 and x64 unsigned app bundles, archive/upload them, then merge into a universal app and run signing/notarization/publish during release. Add helper scripts (make-universal-mac.mjs, require-macos-release-secrets.cjs), entitlements plist files, and package.json scripts for per-arch dist, merging, and signed/universal releases; enable hardened runtime, entitlements, and notarization in electron-builder config. Update CONTRIBUTING.md and README with Apple signing/notarization instructions and the required GitHub secrets. Miscellaneous supporting changes across desktop code, tests, docs, and CTO identity metadata.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved the pack subsystem and many related pack APIs/tests; reworked main/runtime wiring to replace packService with fileService and processService; expanded sync/pairing/remote-command features (LAN/Tailscale/mDNS/QR), added CRSQLite availability gating and CRR repair, and extended agent-chat/CTO/operator tooling plus numerous renderer and test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Phone as Phone (Client)
participant Desktop as Desktop (SyncService)
participant Host as SyncHostService
participant DB as Local DB
participant MDNS as mDNS/Bonjour
Phone->>Desktop: request pairing / scan QR
Desktop->>DB: read localDevice & pairingSession
DB-->>Desktop: pairingConnectInfo (addresses, qrPayload)
Desktop->>MDNS: publish mDNS TCP service (port)
Desktop-->>Phone: present QR / pairingConnectInfo
Phone->>Host: connect to advertised host address
Host->>Desktop: hello/authenticate -> register peer (authKind/pairedDeviceId)
Host-->>Phone: hello_ok with commandRouting.actions (descriptors)
Phone->>Host: send remote command (e.g., lanes.getDetail)
Host->>Desktop: requireService(...) -> dispatch handler (git/diff/agentChat)
Desktop-->>Host: action result / structuredContent
Host-->>Phone: return command result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Owner
Author
|
@codex review this please |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a08f9a00d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let reasoningEffort, !reasoningEffort.isEmpty { | ||
| args["reasoningEffort"] = reasoningEffort | ||
| } | ||
| return try await sendDecodableCommand(action: "chat.create", args: args, as: AgentChatSessionSummary.self) |
There was a problem hiding this comment.
AgentChatSessionSummary from chat.create
The new iPhone chat launch flow decodes chat.create as AgentChatSessionSummary, but the host handler now returns the raw AgentChatSession from agentChatService.createSession (id/createdAt, no sessionId/startedAt). In practice, tapping Launch in LaneChatLaunchSheet will fail JSON decoding every time instead of opening the new chat. Either the sync command needs to map the result to a summary shape, or the iOS client needs to accept the session payload.
Useful? React with 👍 / 👎.
Comment on lines
+799
to
+803
| func fetchChatTranscript(sessionId: String, limit: Int = 200, maxChars: Int = 32_000) async throws -> [AgentChatTranscriptEntry] { | ||
| try await sendDecodableCommand( | ||
| action: "chat.getTranscript", | ||
| args: ["sessionId": sessionId, "limit": limit, "maxChars": maxChars], | ||
| as: [AgentChatTranscriptEntry].self |
There was a problem hiding this comment.
chat.getTranscript envelope before use
fetchChatTranscript now expects chat.getTranscript to return a bare [AgentChatTranscriptEntry], but the desktop sync handler returns an object with sessionId, entries, truncated, and totalEntries. That means opening a chat transcript—or reloading it after sending a message—will throw during decode on iPhone and leave the chat view unusable. The client has to unwrap entries, or the host needs to change the response shape.
Useful? React with 👍 / 👎.
Comment on lines
+2054
to
+2055
| if (callerCtx.role === "cto") { | ||
| return [...visibleBaseTools, ...CTO_OPERATOR_TOOL_SPECS, ...CTO_LINEAR_SYNC_TOOL_SPECS, ...externalToolSpecs]; |
There was a problem hiding this comment.
Checked headless MCP wiring in apps/mcp-server/src/bootstrap.ts: this runtime still sets agentChatService, linearSyncService, flowPolicyService, linearRoutingService, fileService, and processService to null. After this change, tools/list still advertises the full CTO tool sets for any caller initialized with role cto, so commands like spawnChat or getLinearSyncDashboard are now listed but fail immediately with internal errors from requireAgentChatService/requireLinearSyncService. That breaks headless MCP feature discovery and makes the new CTO role unusable outside the desktop-backed runtime.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/desktop/src/main/services/orchestrator/orchestratorService.ts (1)
7316-7322:⚠️ Potential issue | 🟠 MajorPropagate
permissionModeviachangePermissionMode(or remove from launch contract) to avoid permission drift.At line 7316, the call forwards
executionModeandreasoningEffortbut dropsManagedWorkerLaunch.permissionMode. This creates a contract mismatch: theManagedWorkerLaunchtype includespermissionMode, but it's never propagated to the session.
runSessionTurndoes not acceptpermissionModeas a parameter (its signature isAgentChatSendArgs, which excludes it). The permission mode must be set separately viachangePermissionModebefore or after the turn, or removed from theManagedWorkerLaunchcontract if it's not intended to be used.🔧 Suggested approach (if `permissionMode` should be propagated)
Call
changePermissionModebeforerunSessionTurn:if (sessionId && managedLaunch && agentChatService) { void (async () => { try { + if (managedLaunch.permissionMode) { + agentChatService.changePermissionMode({ + sessionId, + permissionMode: managedLaunch.permissionMode, + }); + } await agentChatService.runSessionTurn({ sessionId, text: managedLaunch.prompt, displayText: managedLaunch.displayText, ...(managedLaunch.reasoningEffort ? { reasoningEffort: managedLaunch.reasoningEffort } : {}), ...(managedLaunch.executionMode ? { executionMode: managedLaunch.executionMode } : {}), });Alternatively, remove
permissionModefromManagedWorkerLaunchif it's not used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/orchestrator/orchestratorService.ts` around lines 7316 - 7322, The ManagedWorkerLaunch.permissionMode is never applied when launching a turn, causing permission drift; update the orchestrator to either call changePermissionMode(sessionId, managedLaunch.permissionMode) before invoking agentChatService.runSessionTurn(...) (so permissionMode is set on the session prior to the turn) or remove permissionMode from the ManagedWorkerLaunch contract if it should not be used; locate the call site around agentChatService.runSessionTurn and add the changePermissionMode call referencing sessionId and managedLaunch.permissionMode (with a no-op if undefined) or delete the permissionMode field from the ManagedWorkerLaunch type and all producers/consumers.apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (1)
749-821:⚠️ Potential issue | 🟠 MajorThese navigation buttons are attached to the wrong render path.
Normal
tool_call+tool_resultpairs are collapsed intotool_invocation, so the common tool flow never rendersToolResultCard. That means the new operator-navigation suggestions only appear for unmatched rawtool_resultevents, which makes the feature effectively disappear in real transcripts.Move the suggestion rendering into the
tool_invocationbranch, or replace the collapsed row with atool_resultwhen navigation metadata is present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 749 - 821, Navigation suggestion buttons are currently rendered only inside ToolResultCard (used for raw tool_result events) but not when tool_call + tool_result are collapsed into a tool_invocation, so suggestions never appear for the common tool flow; update the rendering so navigationSuggestions are shown for the collapsed invocation branch. Specifically, in the component branch that handles "tool_invocation" (where tool_call/tool_result pairs are collapsed) detect navigationSuggestions = readNavigationSuggestions(...) for that invocation's result and either render the same suggestion button block used in ToolResultCard or, when navigation metadata exists, expand the collapsed invocation into a visible tool_result-like row that includes the suggestion buttons; touch the ToolResultCard, the tool_invocation render branch, and the call to readNavigationSuggestions to keep behavior consistent.apps/desktop/src/main/services/prs/prService.ts (1)
644-650:⚠️ Potential issue | 🟠 MajorDon't wipe cached snapshots on transient refresh failures.
jsonOrFallback()only preserves the previous DB value forundefined, butrefreshSnapshotData()still maps each GitHub read failure tonull. That turns a temporary fetch error into destructive cache eviction fordetail/status/checks/reviews/comments/files, solistSnapshots()starts returning empty data until a later refresh succeeds.Suggested fix
- getDetailSnapshot(prId).catch(() => null), - computeStatus(rowToSummary(requireRow(prId))).catch(() => null), - getChecks(prId).catch(() => null), - getReviews(prId).catch(() => null), - getComments(prId).catch(() => null), - getFilesSnapshot(prId).catch(() => null), + getDetailSnapshot(prId).catch(() => undefined), + computeStatus(rowToSummary(requireRow(prId))).catch(() => undefined), + getChecks(prId).catch(() => undefined), + getReviews(prId).catch(() => undefined), + getComments(prId).catch(() => undefined), + getFilesSnapshot(prId).catch(() => undefined),Also applies to: 668-673
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 644 - 650, The code currently uses jsonOrFallback(next, fallback) but refreshSnapshotData maps transient GitHub fetch failures to null which causes jsonOrFallback to store null and evict cached snapshot fields; change refreshSnapshotData so that on transient/read errors it passes undefined (not null) for the per-field "next" value so jsonOrFallback will preserve the existing DB value, and only pass null when the remote explicitly returned null; update all similar sites (including the other occurrences around the 668-673 block) to follow this pattern so listSnapshots() doesn't get wiped on transient failures.apps/mcp-server/src/mcpServer.ts (2)
2067-2077:⚠️ Potential issue | 🔴 CriticalDon't accept
ctoas a client-declared role.If
ADE_DEFAULT_ROLEis unset, any MCP client can sendidentity.role: "cto"duringinitializeand immediately gain access to the new CTO-only tools. That makes CTO access self-asserted instead of host-assigned.🔐 Suggested guard
const requestedRole = asTrimmedString(identity.role); + if (requestedRole === "cto" && envContext.role !== "cto") { + throw new JsonRpcError( + JsonRpcErrorCode.policyDenied, + "CTO role must be assigned by the host runtime.", + ); + } const validRole: SessionIdentity["role"] = envContext.role ?? ( - requestedRole === "cto" - || requestedRole === "orchestrator" + requestedRole === "orchestrator" || requestedRole === "agent" || requestedRole === "evaluator" ? requestedRole : "external" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mcp-server/src/mcpServer.ts` around lines 2067 - 2077, The current role resolution lets a client self-assert "cto" via requestedRole and assigns it when envContext.role is unset; change the logic in the validRole computation so that "cto" is never accepted from the client side—only use envContext.role if it equals "cto". Specifically, keep using asTrimmedString(identity.role) to get requestedRole, but in the ternary that builds validRole, only allow requestedRole values "orchestrator", "agent", or "evaluator" (not "cto"), and if envContext.role is set use that (including "cto"); otherwise default to "external". Update the code around requestedRole, envContext.role and validRole to enforce this guard.
2034-2056:⚠️ Potential issue | 🟠 MajorDon't advertise CTO tools that this runtime can't execute.
For
callerCtx.role === "cto",tools/listalways publishes both CTO tool groups, but the call path later hard-fails withinternalErrorwhen optional services likeagentChatService,linearSyncService,linearIngressService,flowPolicyService, orlinearRoutingServiceare unset. MCP clients treattools/listas the contract, so this exposes dead capabilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mcp-server/src/mcpServer.ts` around lines 2034 - 2056, listToolSpecsForSession is currently returning both CTO tool groups unconditionally for callerCtx.role === "cto", which advertises capabilities the runtime may not support; update that branch to only include CTO_OPERATOR_TOOL_SPECS and CTO_LINEAR_SYNC_TOOL_SPECS when the corresponding runtime services are present (check runtime.agentChatService, runtime.linearSyncService, runtime.linearIngressService, runtime.flowPolicyService, runtime.linearRoutingService as appropriate) before concatenating those arrays with visibleBaseTools and externalToolSpecs so the tools/list response only advertises executables the runtime can actually run.apps/desktop/src/main/services/chat/agentChatService.ts (1)
4416-4527:⚠️ Potential issue | 🟠 MajorUse a per-warmup token instead of a shared cancellation flag.
Promise.race([warmupTask, waitForCancel])settlesv2WarmupDoneimmediately, but the oldwarmupTaskcan still be insidesend()/stream(). A subsequent prewarm resetsruntime.v2WarmupCancelled = false, which lets that stale task resume and mutatev2Session,sdkSessionId, slash commands, and ready notices for the new session. Guard each warmup with its own generation/token orAbortController.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 4416 - 4527, The current warmup uses the shared runtime.v2WarmupCancelled flag which allows stale warmupTask instances to resume and mutate runtime.v2Session/sdkSessionId; replace that with a per-warmup token or AbortController: create a local const token = Symbol() or const controller = new AbortController() for each warmup, store it on runtime (e.g., runtime.v2WarmupToken or runtime.v2WarmupAbort = controller), have cancelWarmup call runtime.v2WarmupAbort?.abort() or clear the token, and change all checks that read runtime.v2WarmupCancelled inside warmupTask (before/after createSession, before/after send/stream loop and in catch/finally) to instead verify the local token/controller (e.g., if (runtime.v2WarmupToken !== token) return; or if (controller.signal.aborted) return;), ensuring only the task created with that token may mutate runtime.v2Session, runtime.sdkSessionId, applyClaudeSlashCommands, emit notices, and set v2WarmupDone/v2WarmupCancel.apps/desktop/src/main/services/sync/syncRemoteCommandService.ts (1)
72-87:⚠️ Potential issue | 🟠 MajorDon't advertise commands whose backing service is absent.
getDescriptors()now exposes the full registry, but the registry is populated unconditionally even though many dependencies inSyncRemoteCommandServiceArgsare optional. A host withoutagentChatService,gitService,diffService,conflictService,laneTemplateService,laneEnvironmentService, orprojectConfigServicecan still announce those actions and then fail at execute time with... service not available.If these services are effectively mandatory now, make them required inSyncRemoteCommandServiceArgs; otherwise gate registration/descriptors on availability.🔧 Suggested pattern
const register = ( action: SyncRemoteCommandAction, policy: SyncRemoteCommandPolicy, handler: (payload: Record<string, unknown>) => Promise<unknown>, ) => { registry.set(action, { descriptor: { action, policy }, handler, }); }; + + const registerIf = ( + enabled: boolean, + action: SyncRemoteCommandAction, + policy: SyncRemoteCommandPolicy, + handler: (payload: Record<string, unknown>) => Promise<unknown>, + ) => { + if (!enabled) return; + register(action, policy, handler); + }; ... - register("chat.listSessions", { viewerAllowed: true }, async (payload) => { + registerIf(Boolean(args.agentChatService), "chat.listSessions", { viewerAllowed: true }, async (payload) => { const agentChatService = requireService(args.agentChatService, "Agent chat service not available."); const parsed = parseAgentChatListArgs(payload); return agentChatService.listSessions(parsed.laneId, { includeAutomation: parsed.includeAutomation }); });Apply the same gating to the other optional-service command groups.
Also applies to: 762-774, 821-873, 897-999, 1037-1043
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts` around lines 72 - 87, getDescriptors() currently exposes commands for services that may be optional in SyncRemoteCommandServiceArgs, causing advertised actions to fail at execution when backing services (agentChatService, gitService, diffService, conflictService, laneTemplateService, laneEnvironmentService, projectConfigService, etc.) are absent; update the registry population inside createSyncRemoteCommandService (and any helper that populates the registry) to only register descriptors and commands when their corresponding optional service is non-null/defined (or alternatively make truly-mandatory services required on SyncRemoteCommandServiceArgs), i.e., gate each command group registration by checking the specific service (e.g., if (gitService) { register git-related descriptors } else skip), and apply this same gating pattern to the other optional-service blocks referenced (lines covering the groups around getDescriptors, and the blocks for 762-774, 821-873, 897-999, 1037-1043) so getDescriptors only advertises commands that will actually execute.
🧹 Nitpick comments (8)
apps/web/src/app/pages/HomePage.tsx (1)
414-414: Prefer artifact-agnostic wording in the quickstart summary.Line 414 hard-codes “DMG,” while release messaging elsewhere references both DMG/ZIP. Consider neutral wording to avoid confusion.
Suggested copy tweak
- Download the DMG, move ADE into Applications, and open. No account required. + Download the latest macOS release, move ADE into Applications, and open. No account required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/pages/HomePage.tsx` at line 414, The quickstart summary in the HomePage component currently hard-codes “DMG” in the text node ("Download the DMG, move ADE into Applications, and open. No account required."); update this copy to neutral, artifact-agnostic wording (e.g., "Download the app, install it, and open. No account required.") so it covers DMG/ZIP without naming a specific package; locate the string inside the HomePage JSX and replace it with the generalized phrase.apps/desktop/src/renderer/components/settings/ProvidersSection.tsx (1)
149-170: Consider extracting shared auth error patterns to avoid duplication.The
AUTH_ERROR_SIGNALSarray duplicates patterns from the backend'sisClaudeRuntimeAuthErrorinclaudeRuntimeProbe.ts. While renderer-side filtering avoids IPC overhead, maintaining two copies increases drift risk.Consider extracting these patterns to a shared constants file (e.g.,
shared/authPatterns.ts) that both backend and renderer can import, or document the intentional duplication with a comment referencing the backend counterpart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx` around lines 149 - 170, The renderer duplicates auth error patterns in AUTH_ERROR_SIGNALS used by isAuthRelatedChatMessage, which risks drift from the backend's isClaudeRuntimeAuthError; extract the shared patterns into a common module (e.g., create shared/authPatterns.ts exporting AUTH_ERROR_SIGNALS or AUTH_ERROR_PATTERNS) and import that module from both ProvidersSection.tsx and claudeRuntimeProbe.ts, or if extraction is infeasible add a concise comment above AUTH_ERROR_SIGNALS referencing the backend file and function name (isClaudeRuntimeAuthError in claudeRuntimeProbe.ts) to make the duplication intentional and traceable.apps/desktop/src/main/services/state/crsqliteExtension.ts (1)
18-39: Consider memoizing extension resolution to avoid repeated sync FS probes.Line 39 calls a resolver that performs multiple
existsSyncchecks (Line 29-Line 31). Caching the first resolved value (includingnull) avoids repeated main-thread filesystem scans.⚡ Suggested memoization
+let cachedCrsqlitePath: string | null | undefined; + export function resolveCrsqliteExtensionPath(): string | null { + if (cachedCrsqlitePath !== undefined) { + return cachedCrsqlitePath; + } const relativePath = path.join("vendor", "crsqlite", platformArchDir(), extensionFileName()); const candidates = [ process.resourcesPath ? path.join(process.resourcesPath, "app.asar.unpacked", relativePath) : null, @@ for (const candidate of candidates) { if (fs.existsSync(candidate)) { - return candidate; + cachedCrsqlitePath = candidate; + return cachedCrsqlitePath; } } - return null; + cachedCrsqlitePath = null; + return cachedCrsqlitePath; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/state/crsqliteExtension.ts` around lines 18 - 39, The resolver resolveCrsqliteExtensionPath performs multiple synchronous fs.existsSync probes on every call; memoize its result (including null) in a module-level variable (e.g., let cachedCrsqlitePath: string | null | undefined) so subsequent calls return the cached value instead of re-scanning the filesystem, and have isCrsqliteAvailable call the memoized resolveCrsqliteExtensionPath so it benefits from the cache; ensure the cache is set after the first computation and that undefined denotes “not yet computed.”apps/desktop/src/renderer/components/chat/chatExecutionSummary.test.ts (1)
101-148: Consider asserting the pre-result running snapshot explicitly.The test title says “before the final result arrives,” but current assertions only validate the post-result state. Add an intermediate assertion (e.g., with events up to Line 125) to lock down running-state behavior.
💡 Suggested assertion addition
it("updates running snapshots from progress events before the final result arrives", () => { const events: AgentChatEventEnvelope[] = [ // ... ]; + expect(deriveChatSubagentSnapshots(events.slice(0, 2))).toEqual([ + expect.objectContaining({ + taskId: "task-1", + status: "running", + summary: "Traced the send handler and found the blocking await.", + lastToolName: "functions.exec_command", + usage: expect.objectContaining({ totalTokens: 800, toolUses: 2 }), + }), + ]); + expect(deriveChatSubagentSnapshots(events)).toEqual([ expect.objectContaining({ taskId: "task-1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/chatExecutionSummary.test.ts` around lines 101 - 148, Add an explicit intermediate assertion that verifies the running snapshot produced after the progress event (before the final result) — call deriveChatSubagentSnapshots with events sliced up to the subagent_progress event (e.g., events.slice(0, 2)) and assert the returned snapshot for taskId "task-1" has status "running" (or equivalent running state), includes the progress summary "Traced the send handler and found the blocking await.", lastToolName "functions.exec_command", and usage.totalTokens/toolUses values; keep the existing final assertion unchanged so the test covers both the pre-result running snapshot and the final completed snapshot.apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx (3)
262-289: Consider usingqrPayloadTextas the effect dependency.The effect depends on
status?.pairingConnectInfo, which is an object. Since React compares by reference, this effect may re-run on every status update even when the QR payload hasn't changed. UsingpairingInfo.qrPayloadTextdirectly would be more precise.♻️ Use qrPayloadText as dependency
useEffect(() => { let cancelled = false; - const pairingInfo = status?.pairingConnectInfo; - if (!pairingInfo?.qrPayloadText) { + const qrPayloadText = status?.pairingConnectInfo?.qrPayloadText; + if (!qrPayloadText) { setPairingQrDataUrl(null); return; } - void QRCode.toDataURL(pairingInfo.qrPayloadText, { + void QRCode.toDataURL(qrPayloadText, { width: 240, margin: 1, errorCorrectionLevel: "M", color: { dark: "#F4F7FB", light: "#11151A", }, }).then((dataUrl) => { if (!cancelled) { setPairingQrDataUrl(dataUrl); } }).catch(() => { if (!cancelled) { setPairingQrDataUrl(null); } }); return () => { cancelled = true; }; - }, [status?.pairingConnectInfo]); + }, [status?.pairingConnectInfo?.qrPayloadText]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx` around lines 262 - 289, The effect in SyncDevicesSection uses status?.pairingConnectInfo (an object) as its dependency which can trigger unnecessary re-runs; change the dependency to the scalar QR payload (pairingInfo?.qrPayloadText or a local qrPayloadText const) so the effect only runs when the actual QR text changes, keeping the existing cancelled flag, setPairingQrDataUrl updates and QRCode.toDataURL usage intact (refer to the useEffect block, pairingInfo, pairingQrDataUrl state and QRCode.toDataURL).
512-517: Consider using<code>tags for inline code values.The backtick characters (
) are literal text in HTML and won't render with code styling. For consistency with typical UI patterns, consider using` tags.♻️ Use code tags for inline values
- <li>Scan the QR, or enter `{pairingInfo.pairingCode}` with host `{primaryPairAddress}` and port `{pairingInfo.port}`.</li> + <li>Scan the QR, or enter <code>{pairingInfo.pairingCode}</code> with host <code>{primaryPairAddress}</code> and port <code>{pairingInfo.port}</code>.</li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx` around lines 512 - 517, The inline backticks are being rendered as literal text; update the JSX in SyncDevicesSection to wrap the dynamic values pairingInfo.pairingCode, primaryPairAddress and pairingInfo.port with <code> elements instead of surrounding them with backticks (in the <ol> list item that currently reads "Scan the QR, or enter `{pairingInfo.pairingCode}` with host `{primaryPairAddress}` and port `{pairingInfo.port}`"). Keep the surrounding text intact and ensure JSX expression braces remain (e.g. <code>{pairingInfo.pairingCode}</code>) so the values render with code styling.
669-673: Duplicate disconnect button in advanced section.There's already a disconnect button at the top level (lines 451-457) that appears under the same conditions. Having two disconnect buttons with identical functionality could be confusing. Consider removing this duplicate or differentiating their purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx` around lines 669 - 673, The duplicate "Disconnect this desktop" button inside SyncDevicesSection.tsx (the button using outlineButton(), disabled={busy}, onClick={handleDisconnect} and conditional on status.role === "viewer" || status.client.state === "connected") should be removed to avoid duplicating the top-level disconnect control; locate the other existing disconnect button that uses the same conditions and handler (the one rendered near the top-level of SyncDevicesSection) and keep that single instance, ensuring you only delete the inner duplicate block so handleDisconnect, busy, status checks and any surrounding layout remain consistent.apps/desktop/src/main/services/state/kvDb.ts (1)
334-377: Consider simplifying the control flow.The nested conditionals with two
continuestatements (lines 338-340 and 345-347) make the logic harder to follow. The firstcontinueexits for non-repair-target tables that already have CRR metadata, and the second exits for non-repair-target tables after conversion.The current logic is correct, but could be clearer:
♻️ Optional: Consolidate continue conditions
function ensureCrrTables(db: DatabaseSyncType, logger?: Logger): void { const repairTargets = new Set<string>(PHONE_CRITICAL_CRR_TABLES); for (const tableName of listEligibleCrrTables(db)) { - if (rawHasTable(db, `${tableName}__crsql_clock`)) { - if (!repairTargets.has(tableName)) { - continue; - } - } else { + const hasClock = rawHasTable(db, `${tableName}__crsql_clock`); + const isRepairTarget = repairTargets.has(tableName); + + if (!hasClock) { getRow(db, "select crsql_as_crr(?) as ok", [tableName]); } - if (!repairTargets.has(tableName)) { + if (!isRepairTarget) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/state/kvDb.ts` around lines 334 - 377, Replace the nested conditionals in ensureCrrTables with clearer, early-evaluated boolean variables to remove the two separate continue points: for each table from listEligibleCrrTables compute hasClock = rawHasTable(db, `${tableName}__crsql_clock`) and isRepairTarget = PHONE_CRITICAL_CRR_TABLES.contains(tableName) (or repairTargets.has(tableName)), then if (hasClock && !isRepairTarget) continue; if (!hasClock) getRow(...); if (!isRepairTarget) continue; then proceed with tableNeedsCrrRepair/table rebuild and logging as before (functions to locate: ensureCrrTables, listEligibleCrrTables, rawHasTable, getRow, tableNeedsCrrRepair, rebuildCrrTableWithBackfill, logger).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts`:
- Around line 1212-1249: The exportContext tool currently treats any
deps.fetchContextPack response as success; add scope-specific input validation
in exportContext before calling deps.fetchContextPack (e.g., require missionId
when scope === "mission", laneId when scope === "lane", etc.) and return {
success: false, error: "<clear message>" } for missing/invalid inputs; only call
deps.fetchContextPack when inputs pass validation and propagate its structured
error result (do not unconditionally set success: true). Use the existing
symbols exportContext, deps.fetchContextPack, and
buildNavigationPayload/buildNavigationSuggestion to locate insertion points and
use deps.missionService?.get(...) only after validating missionId.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 5095-5115: The sendMessage function is currently fire-and-forget
because it does not await executePreparedSendMessage, causing callers like
spawnChat() and routeIssueToCto() to believe a message succeeded even if
dispatch fails; change sendMessage (the function defined at the top of this
diff) to return/await the promise from executePreparedSendMessage so it rejects
on failure (or alternatively add a separate fire-and-forget wrapper function and
keep sendMessage awaitable), ensuring you still log and
emitDispatchedSendFailure inside the executePreparedSendMessage.catch path but
do not swallow the rejection from sendMessage.
- Around line 2662-2685: The code currently still calls emitChatEvent with an
empty taskId even though runtime.activeSubagents only sets entries when taskId
is present; add a guard in the system/task_progress branch to skip emitting and
updating when task_id is missing (i.e. if taskId === "" then continue), or
alternatively synthesize a stable non-empty ID before calling emitChatEvent;
update the logic around runtime.activeSubagents, the local taskId/description
handling, and the emitChatEvent(managed, {...}) call in the system/task_progress
block so that trackSubagentEvent (which keys by event.taskId) never receives an
empty taskId.
- Around line 5724-5729: The code currently unconditionally calls
cancelClaudeWarmup(managed, managed.runtime, "session_reset") and closes/nulls
managed.runtime.v2Session when a mid-turn reasoning change occurs, which can
terminate an in-flight Claude turn; change this to defer session teardown until
the runtime is idle by checking a runtime idle/in-flight indicator (e.g. a
property such as managed.runtime.inFlight or managed.runtime.state === "idle")
before calling cancelClaudeWarmup and closing v2Session; if the runtime is busy,
set a reset-needed flag on the runtime (e.g. managed.runtime.pendingReset =
true) so the reset/close logic (cancelClaudeWarmup, v2Session.close(), v2Session
= null, v2WarmupDone = null) runs only when the runtime transitions to idle
(handle the pendingReset in that transition).
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 744-775: The queries in getStateSnapshot and listStateSnapshots
currently read lane_state_snapshots without scoping to the service's project,
which can surface other projects' data; modify both queries to restrict results
to snapshots belonging to the current project by joining lanes (e.g., join lanes
on lanes.id = lane_state_snapshots.lane_id and add where lanes.project_id = ?)
or by adding an EXISTS/subquery that checks lane.project_id = ?, and pass the
service's projectId (and laneId for getStateSnapshot) as query parameters;
update the parameter arrays for getStateSnapshot (include projectId) and
listStateSnapshots (include projectId) and keep using parseSummaryRecord for the
returned rows.
- Around line 818-836: The created lane is still parented to parent.id even when
useCustomBase is true, causing later status/rebase logic to resolve against
parent.branch_ref instead of the lane's baseRef (row.base_ref); update the
createWorktreeLane call so when useCustomBase is true you do not set the parent
(e.g., pass parentId: undefined or omit parentId) and when false pass parent.id,
ensuring createWorktreeLane receives the correct parentId and baseRef (refer to
trimmedBaseBranch, useCustomBase, requestedBaseRef, parentHeadSha, getHeadSha,
createWorktreeLane, parent.branch_ref, row.base_ref, parent.id) so subsequent
status/rebase flows use row.base_ref rather than the primary's branch_ref.
- Around line 777-785: refreshSnapshots can return stale data because it relies
on listLanes, which may short-circuit via laneListCache; modify refreshSnapshots
to force fresh computation by calling listLanes with a cache-bypass flag (e.g.,
forceRefresh/skipCache: true) or by invoking the status recomputation path
directly before running the upsert loop, then run the existing
upsertLaneStateSnapshot loop for each returned lane and return the correct
refreshedCount and lanes; update refreshSnapshots, listLanes call-site, and
ensure upsertLaneStateSnapshot is invoked for every lane to avoid stale
snapshots.
In `@apps/desktop/src/main/services/memory/hybridSearchService.test.ts`:
- Around line 4-23: The test file eagerly requires node:sqlite and constructs
DatabaseSync during module load which throws on runtimes without node:sqlite;
modify hasFts/ftsAvailable so the require and DatabaseSync construction are
guarded: attempt to require("node:sqlite") inside a try/catch and set
DatabaseSync to undefined on failure, then in hasFts check DatabaseSync is
defined before creating a new DatabaseSync(":memory:") and return false if it's
not available; update references to DatabaseSync, hasFts, and ftsAvailable
accordingly so unsupported runtimes skip cleanly.
In `@apps/desktop/src/main/services/state/kvDb.sync.test.ts`:
- Around line 265-284: The test's FTS detection is inverted: when isFts
(computed from db2.get on sqlite_master for unified_memories_fts) is true you
should run the real FTS query using "unified_memories_fts MATCH ?" and when
false run the fallback LIKE query against the plain table; update the
conditional in the test (or swap the two branches) so that isFts -> MATCH and
!isFts -> LIKE, ensuring you do not execute a MATCH against a non‑existent
unified_memories_fts table (references: isFts, db2.get, unified_memories_fts,
MATCH, LIKE).
In `@apps/desktop/src/main/services/sync/syncHostService.test.ts`:
- Around line 418-429: The snapshot `status` objects in syncHostService.test.ts
no longer match the PrStatus returned by prService.listSnapshots(); update both
snapshot mocks (the one shown and the second at the other location) to remove
legacy fields (mergeableState, reviewDecision, draft) and instead include the
current PrStatus fields such as isMergeable, mergeConflicts, and behindBaseBy
(and any other fields present in PrStatus), ensuring the two mocks use the exact
same shape as prService.listSnapshots() so the test covers the real sync payload
contract.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts`:
- Around line 675-685: buildLaneListSnapshots currently derives the
runtime.summary from a capped session sample (using sessionService.list with a
fixed limit), which can omit active sessions and make runtime.bucket/counts
stale; replace this by calling an uncapped/aggregate session summary API (e.g.,
sessionService.getActiveSessionSummary() or sessionService.summary()) or add a
new dedicated method that returns accurate active-session aggregates, and use
that result wherever runtime is constructed (references: buildLaneListSnapshots,
sessionService.list, and the runtime.bucket/count aggregation locations) so both
runtime builds use the uncapped/aggregate data instead of the capped list.
- Around line 213-220: parseRebaseStartArgs (and the other parsers that mirror
its pattern) currently forwards raw value.* into the returned object and casts
without validating normalized strings; instead, run asTrimmedString on each
optional union-like field (e.g., scope, pushMode, actor, reason), store the
result in a local (e.g., const scope = asTrimmedString(value.scope)), check its
presence, and then include that local in the returned object (not value.scope)
with the proper typed cast; follow the same validated/local-variable pattern
used by parseLandPrArgs and apply this change to the other parser functions
handling fields like status, toolType, provider, mode, and compareTo so only
trimmed/validated literals are forwarded to the service layer.
- Around line 328-333: The parser parseWriteTextAtomicArgs currently converts a
missing or non-string text into an empty string, which can silently truncate
files; change the validation so text is required and must be a string (similar
to laneId and path). Replace the current text assignment with a strict check
that throws or calls requireString for value.text with a clear message like
"files.writeTextAtomic requires text." and ensure the returned
WriteTextAtomicArgs.text is the validated string.
In `@apps/desktop/src/renderer/components/chat/chatExecutionSummary.ts`:
- Around line 42-53: The subagent_progress handler currently calls
event.summary.trim() unconditionally and replaces the entire usage object, which
can throw or discard prior counters; update the block that sets
snapshots.set(...) (for event.type === "subagent_progress" and taskId) to:
compute a safe summary by using event.summary?.trim() and only use it if it's
non-empty, otherwise fall back to existing?.summary or null; for usage, merge
existing?.usage with event.usage (e.g., shallow merge) only when event.usage is
provided so missing fields are preserved; keep description handling as-is and
ensure startedAt/updatedAt logic remains unchanged.
In `@apps/desktop/src/renderer/components/settings/ProvidersSection.test.tsx`:
- Line 30: The test's mock uses blocker: claudeRuntimeAvailable ? null : null
which always yields null; update the mock in ProvidersSection.test.tsx so the
blocker reflects a realistic message when claudeRuntimeAvailable is false (e.g.,
set blocker to a descriptive string like "Sign-in required" or similar) so
getStatusTone and related assertions behave like real runtime behavior; locate
the mock object where claudeRuntimeAvailable is defined and replace the ternary
expression with a null-or-descriptive-string value to make the test data
representative.
In `@apps/mcp-server/src/mcpServer.ts`:
- Around line 1730-1745: The code always computes defaultLaneId and passes it
into createCtoOperatorTools which forces non-lane-scoped tools to depend on lane
state; instead compute/resolve the default lane only for tools that actually
require a lane (or move that logic into runCtoOperatorBridgeTool). Update the
caller around createCtoOperatorTools and runCtoOperatorBridgeTool so you either
(A) detect the tool schema's requirement for laneId before resolving
defaultLaneId and only supply defaultLaneId for lane-scoped tools, or (B) remove
defaultLaneId resolution here and perform it lazily inside
runCtoOperatorBridgeTool when dispatching to a tool that declares laneId as
required; reference createCtoOperatorTools, defaultLaneId,
runCtoOperatorBridgeTool, and resolveDefaultLaneId in your changes.
---
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 4416-4527: The current warmup uses the shared
runtime.v2WarmupCancelled flag which allows stale warmupTask instances to resume
and mutate runtime.v2Session/sdkSessionId; replace that with a per-warmup token
or AbortController: create a local const token = Symbol() or const controller =
new AbortController() for each warmup, store it on runtime (e.g.,
runtime.v2WarmupToken or runtime.v2WarmupAbort = controller), have cancelWarmup
call runtime.v2WarmupAbort?.abort() or clear the token, and change all checks
that read runtime.v2WarmupCancelled inside warmupTask (before/after
createSession, before/after send/stream loop and in catch/finally) to instead
verify the local token/controller (e.g., if (runtime.v2WarmupToken !== token)
return; or if (controller.signal.aborted) return;), ensuring only the task
created with that token may mutate runtime.v2Session, runtime.sdkSessionId,
applyClaudeSlashCommands, emit notices, and set v2WarmupDone/v2WarmupCancel.
In `@apps/desktop/src/main/services/orchestrator/orchestratorService.ts`:
- Around line 7316-7322: The ManagedWorkerLaunch.permissionMode is never applied
when launching a turn, causing permission drift; update the orchestrator to
either call changePermissionMode(sessionId, managedLaunch.permissionMode) before
invoking agentChatService.runSessionTurn(...) (so permissionMode is set on the
session prior to the turn) or remove permissionMode from the ManagedWorkerLaunch
contract if it should not be used; locate the call site around
agentChatService.runSessionTurn and add the changePermissionMode call
referencing sessionId and managedLaunch.permissionMode (with a no-op if
undefined) or delete the permissionMode field from the ManagedWorkerLaunch type
and all producers/consumers.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 644-650: The code currently uses jsonOrFallback(next, fallback)
but refreshSnapshotData maps transient GitHub fetch failures to null which
causes jsonOrFallback to store null and evict cached snapshot fields; change
refreshSnapshotData so that on transient/read errors it passes undefined (not
null) for the per-field "next" value so jsonOrFallback will preserve the
existing DB value, and only pass null when the remote explicitly returned null;
update all similar sites (including the other occurrences around the 668-673
block) to follow this pattern so listSnapshots() doesn't get wiped on transient
failures.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts`:
- Around line 72-87: getDescriptors() currently exposes commands for services
that may be optional in SyncRemoteCommandServiceArgs, causing advertised actions
to fail at execution when backing services (agentChatService, gitService,
diffService, conflictService, laneTemplateService, laneEnvironmentService,
projectConfigService, etc.) are absent; update the registry population inside
createSyncRemoteCommandService (and any helper that populates the registry) to
only register descriptors and commands when their corresponding optional service
is non-null/defined (or alternatively make truly-mandatory services required on
SyncRemoteCommandServiceArgs), i.e., gate each command group registration by
checking the specific service (e.g., if (gitService) { register git-related
descriptors } else skip), and apply this same gating pattern to the other
optional-service blocks referenced (lines covering the groups around
getDescriptors, and the blocks for 762-774, 821-873, 897-999, 1037-1043) so
getDescriptors only advertises commands that will actually execute.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 749-821: Navigation suggestion buttons are currently rendered only
inside ToolResultCard (used for raw tool_result events) but not when tool_call +
tool_result are collapsed into a tool_invocation, so suggestions never appear
for the common tool flow; update the rendering so navigationSuggestions are
shown for the collapsed invocation branch. Specifically, in the component branch
that handles "tool_invocation" (where tool_call/tool_result pairs are collapsed)
detect navigationSuggestions = readNavigationSuggestions(...) for that
invocation's result and either render the same suggestion button block used in
ToolResultCard or, when navigation metadata exists, expand the collapsed
invocation into a visible tool_result-like row that includes the suggestion
buttons; touch the ToolResultCard, the tool_invocation render branch, and the
call to readNavigationSuggestions to keep behavior consistent.
In `@apps/mcp-server/src/mcpServer.ts`:
- Around line 2067-2077: The current role resolution lets a client self-assert
"cto" via requestedRole and assigns it when envContext.role is unset; change the
logic in the validRole computation so that "cto" is never accepted from the
client side—only use envContext.role if it equals "cto". Specifically, keep
using asTrimmedString(identity.role) to get requestedRole, but in the ternary
that builds validRole, only allow requestedRole values "orchestrator", "agent",
or "evaluator" (not "cto"), and if envContext.role is set use that (including
"cto"); otherwise default to "external". Update the code around requestedRole,
envContext.role and validRole to enforce this guard.
- Around line 2034-2056: listToolSpecsForSession is currently returning both CTO
tool groups unconditionally for callerCtx.role === "cto", which advertises
capabilities the runtime may not support; update that branch to only include
CTO_OPERATOR_TOOL_SPECS and CTO_LINEAR_SYNC_TOOL_SPECS when the corresponding
runtime services are present (check runtime.agentChatService,
runtime.linearSyncService, runtime.linearIngressService,
runtime.flowPolicyService, runtime.linearRoutingService as appropriate) before
concatenating those arrays with visibleBaseTools and externalToolSpecs so the
tools/list response only advertises executables the runtime can actually run.
---
Nitpick comments:
In `@apps/desktop/src/main/services/state/crsqliteExtension.ts`:
- Around line 18-39: The resolver resolveCrsqliteExtensionPath performs multiple
synchronous fs.existsSync probes on every call; memoize its result (including
null) in a module-level variable (e.g., let cachedCrsqlitePath: string | null |
undefined) so subsequent calls return the cached value instead of re-scanning
the filesystem, and have isCrsqliteAvailable call the memoized
resolveCrsqliteExtensionPath so it benefits from the cache; ensure the cache is
set after the first computation and that undefined denotes “not yet computed.”
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 334-377: Replace the nested conditionals in ensureCrrTables with
clearer, early-evaluated boolean variables to remove the two separate continue
points: for each table from listEligibleCrrTables compute hasClock =
rawHasTable(db, `${tableName}__crsql_clock`) and isRepairTarget =
PHONE_CRITICAL_CRR_TABLES.contains(tableName) (or repairTargets.has(tableName)),
then if (hasClock && !isRepairTarget) continue; if (!hasClock) getRow(...); if
(!isRepairTarget) continue; then proceed with tableNeedsCrrRepair/table rebuild
and logging as before (functions to locate: ensureCrrTables,
listEligibleCrrTables, rawHasTable, getRow, tableNeedsCrrRepair,
rebuildCrrTableWithBackfill, logger).
In `@apps/desktop/src/renderer/components/chat/chatExecutionSummary.test.ts`:
- Around line 101-148: Add an explicit intermediate assertion that verifies the
running snapshot produced after the progress event (before the final result) —
call deriveChatSubagentSnapshots with events sliced up to the subagent_progress
event (e.g., events.slice(0, 2)) and assert the returned snapshot for taskId
"task-1" has status "running" (or equivalent running state), includes the
progress summary "Traced the send handler and found the blocking await.",
lastToolName "functions.exec_command", and usage.totalTokens/toolUses values;
keep the existing final assertion unchanged so the test covers both the
pre-result running snapshot and the final completed snapshot.
In `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx`:
- Around line 149-170: The renderer duplicates auth error patterns in
AUTH_ERROR_SIGNALS used by isAuthRelatedChatMessage, which risks drift from the
backend's isClaudeRuntimeAuthError; extract the shared patterns into a common
module (e.g., create shared/authPatterns.ts exporting AUTH_ERROR_SIGNALS or
AUTH_ERROR_PATTERNS) and import that module from both ProvidersSection.tsx and
claudeRuntimeProbe.ts, or if extraction is infeasible add a concise comment
above AUTH_ERROR_SIGNALS referencing the backend file and function name
(isClaudeRuntimeAuthError in claudeRuntimeProbe.ts) to make the duplication
intentional and traceable.
In `@apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx`:
- Around line 262-289: The effect in SyncDevicesSection uses
status?.pairingConnectInfo (an object) as its dependency which can trigger
unnecessary re-runs; change the dependency to the scalar QR payload
(pairingInfo?.qrPayloadText or a local qrPayloadText const) so the effect only
runs when the actual QR text changes, keeping the existing cancelled flag,
setPairingQrDataUrl updates and QRCode.toDataURL usage intact (refer to the
useEffect block, pairingInfo, pairingQrDataUrl state and QRCode.toDataURL).
- Around line 512-517: The inline backticks are being rendered as literal text;
update the JSX in SyncDevicesSection to wrap the dynamic values
pairingInfo.pairingCode, primaryPairAddress and pairingInfo.port with <code>
elements instead of surrounding them with backticks (in the <ol> list item that
currently reads "Scan the QR, or enter `{pairingInfo.pairingCode}` with host
`{primaryPairAddress}` and port `{pairingInfo.port}`"). Keep the surrounding
text intact and ensure JSX expression braces remain (e.g.
<code>{pairingInfo.pairingCode}</code>) so the values render with code styling.
- Around line 669-673: The duplicate "Disconnect this desktop" button inside
SyncDevicesSection.tsx (the button using outlineButton(), disabled={busy},
onClick={handleDisconnect} and conditional on status.role === "viewer" ||
status.client.state === "connected") should be removed to avoid duplicating the
top-level disconnect control; locate the other existing disconnect button that
uses the same conditions and handler (the one rendered near the top-level of
SyncDevicesSection) and keep that single instance, ensuring you only delete the
inner duplicate block so handleDisconnect, busy, status checks and any
surrounding layout remain consistent.
In `@apps/web/src/app/pages/HomePage.tsx`:
- Line 414: The quickstart summary in the HomePage component currently
hard-codes “DMG” in the text node ("Download the DMG, move ADE into
Applications, and open. No account required."); update this copy to neutral,
artifact-agnostic wording (e.g., "Download the app, install it, and open. No
account required.") so it covers DMG/ZIP without naming a specific package;
locate the string inside the HomePage JSX and replace it with the generalized
phrase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe01f083-a847-40ac-ba10-00f549ee7315
📥 Commits
Reviewing files that changed from the base of the PR and between 2d585dfe311c672f5fc7b0cdca681650e928cc87 and a08f9a00d0175484a6ae07ac28efd6b04531ee46.
⛔ Files ignored due to path filters (35)
.ade/cto/identity.yaml is excluded by !.ade/** and included by none.github/workflows/release.yml is excluded by none and included by noneCONTRIBUTING.md is excluded by !*.md and included by noneREADME.md is excluded by !*.md and included by noneapps/desktop/build/entitlements.mac.inherit.plist is excluded by none and included by noneapps/desktop/build/entitlements.mac.plist is excluded by none and included by noneapps/desktop/package-lock.json is excluded by !**/package-lock.json and included by noneapps/desktop/package.json is excluded by none and included by noneapps/desktop/scripts/make-universal-mac.mjs is excluded by none and included by noneapps/desktop/scripts/require-macos-release-secrets.cjs is excluded by none and included by noneapps/ios/ADE.xcodeproj/project.pbxproj is excluded by none and included by noneapps/ios/ADE/App/ADEApp.swift is excluded by none and included by noneapps/ios/ADE/App/ContentView.swift is excluded by none and included by noneapps/ios/ADE/Assets.xcassets/BrandMark.imageset/Contents.json is excluded by none and included by noneapps/ios/ADE/Info.plist is excluded by none and included by noneapps/ios/ADE/Models/RemoteModels.swift is excluded by none and included by noneapps/ios/ADE/Services/Database.swift is excluded by none and included by noneapps/ios/ADE/Services/SyncService.swift is excluded by none and included by noneapps/ios/ADE/Views/FilesTabView.swift is excluded by none and included by noneapps/ios/ADE/Views/LanesTabView.swift is excluded by none and included by noneapps/ios/ADE/Views/PRsTabView.swift is excluded by none and included by noneapps/ios/ADE/Views/WorkTabView.swift is excluded by none and included by noneapps/ios/ADETests/ADETests.swift is excluded by none and included by nonedocs/architecture/AI_INTEGRATION.md is excluded by !docs/** and included by nonedocs/architecture/IOS_APP.md is excluded by !docs/** and included by nonedocs/architecture/MULTI_DEVICE_SYNC.md is excluded by !docs/** and included by nonedocs/architecture/UI_FRAMEWORK.md is excluded by !docs/** and included by nonedocs/features/CTO.md is excluded by !docs/** and included by nonedocs/features/LANES.md is excluded by !docs/** and included by nonedocs/final-plan/README.md is excluded by !docs/** and included by nonedocs/final-plan/appendix.md is excluded by !docs/** and included by nonedocs/final-plan/phase-6.md is excluded by !docs/** and included by nonedocs/final-plan/phase-7.md is excluded by !docs/** and included by nonedocs/reference/symphony_orchestrator.ex is excluded by !docs/** and included by noneplan.md is excluded by !*.md and included by none
📒 Files selected for processing (51)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.test.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.tsapps/desktop/src/main/services/ai/providerConnectionStatus.test.tsapps/desktop/src/main/services/ai/providerConnectionStatus.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/cto/ctoStateService.test.tsapps/desktop/src/main/services/cto/ctoStateService.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/memory/hybridSearchService.test.tsapps/desktop/src/main/services/memory/hybridSearchService.tsapps/desktop/src/main/services/orchestrator/orchestratorService.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/state/crsqliteExtension.tsapps/desktop/src/main/services/state/kvDb.sync.test.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/state/onConflictAudit.test.tsapps/desktop/src/main/services/sync/deviceRegistryService.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncHostService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/services/sync/syncService.test.tsapps/desktop/src/main/services/sync/syncService.tsapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/ChatSubagentStrip.test.tsxapps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsxapps/desktop/src/renderer/components/chat/chatExecutionSummary.test.tsapps/desktop/src/renderer/components/chat/chatExecutionSummary.tsapps/desktop/src/renderer/components/cto/CtoPage.test.tsxapps/desktop/src/renderer/components/cto/CtoPage.tsxapps/desktop/src/renderer/components/cto/CtoPromptPreview.tsxapps/desktop/src/renderer/components/cto/OnboardingBanner.tsxapps/desktop/src/renderer/components/cto/OnboardingWizard.tsxapps/desktop/src/renderer/components/missions/MissionsPage.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.test.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.tsxapps/desktop/src/renderer/components/settings/SyncDevicesSection.tsxapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/cto.tsapps/desktop/src/shared/types/lanes.tsapps/desktop/src/shared/types/sync.tsapps/mcp-server/src/bootstrap.tsapps/mcp-server/src/mcpServer.test.tsapps/mcp-server/src/mcpServer.tsapps/web/src/app/pages/DownloadPage.tsxapps/web/src/app/pages/HomePage.tsx
Comment on lines
+1212
to
+1249
| tools.exportContext = tool({ | ||
| description: "Export a bounded ADE context pack for a project, lane, mission, conflict, plan, or feature scope.", | ||
| inputSchema: z.object({ | ||
| scope: z.enum(["project", "lane", "conflict", "plan", "feature", "mission"]), | ||
| level: z.enum(["brief", "standard", "detailed"]).optional().default("standard"), | ||
| laneId: z.string().optional(), | ||
| missionId: z.string().optional(), | ||
| featureKey: z.string().optional(), | ||
| }), | ||
| execute: async ({ scope, level, laneId, missionId, featureKey }) => { | ||
| if (!deps.fetchContextPack) return { success: false, error: "Context export service is not available." }; | ||
| try { | ||
| const result = await deps.fetchContextPack({ | ||
| scope, | ||
| level, | ||
| laneId: laneId?.trim() || undefined, | ||
| missionId: missionId?.trim() || undefined, | ||
| featureKey: featureKey?.trim() || undefined, | ||
| }); | ||
| const mission = missionId?.trim() ? deps.missionService?.get(missionId.trim()) ?? null : null; | ||
| return { | ||
| success: true, | ||
| ...result, | ||
| ...buildNavigationPayload( | ||
| scope === "mission" | ||
| ? buildNavigationSuggestion({ | ||
| surface: "missions", | ||
| laneId: mission?.laneId ?? (laneId?.trim() || null), | ||
| missionId: missionId?.trim() || null, | ||
| }) | ||
| : scope === "lane" | ||
| ? buildNavigationSuggestion({ | ||
| surface: "lanes", | ||
| laneId: laneId?.trim() || null, | ||
| }) | ||
| : null, | ||
| ), | ||
| }; |
There was a problem hiding this comment.
exportContext reports failures as successful exports.
deps.fetchContextPack() already catches validation and service errors and returns "Failed to fetch ..." in content, so this wrapper currently returns success: true for invalid requests like mission scope without a missionId. That makes downstream callers treat an error blob as a real context pack. Validate the scope-specific inputs here, or have the dependency throw/return structured failures.
🛠️ Scope validation at the tool boundary
execute: async ({ scope, level, laneId, missionId, featureKey }) => {
if (!deps.fetchContextPack) return { success: false, error: "Context export service is not available." };
+ if ((scope === "lane" || scope === "conflict" || scope === "plan") && !laneId?.trim()) {
+ return { success: false, error: `${scope} context requires laneId.` };
+ }
+ if (scope === "mission" && !missionId?.trim()) {
+ return { success: false, error: "Mission context requires missionId." };
+ }
+ if (scope === "feature" && !featureKey?.trim()) {
+ return { success: false, error: "Feature context requires featureKey." };
+ }
try {
const result = await deps.fetchContextPack({
scope,
level,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tools.exportContext = tool({ | |
| description: "Export a bounded ADE context pack for a project, lane, mission, conflict, plan, or feature scope.", | |
| inputSchema: z.object({ | |
| scope: z.enum(["project", "lane", "conflict", "plan", "feature", "mission"]), | |
| level: z.enum(["brief", "standard", "detailed"]).optional().default("standard"), | |
| laneId: z.string().optional(), | |
| missionId: z.string().optional(), | |
| featureKey: z.string().optional(), | |
| }), | |
| execute: async ({ scope, level, laneId, missionId, featureKey }) => { | |
| if (!deps.fetchContextPack) return { success: false, error: "Context export service is not available." }; | |
| try { | |
| const result = await deps.fetchContextPack({ | |
| scope, | |
| level, | |
| laneId: laneId?.trim() || undefined, | |
| missionId: missionId?.trim() || undefined, | |
| featureKey: featureKey?.trim() || undefined, | |
| }); | |
| const mission = missionId?.trim() ? deps.missionService?.get(missionId.trim()) ?? null : null; | |
| return { | |
| success: true, | |
| ...result, | |
| ...buildNavigationPayload( | |
| scope === "mission" | |
| ? buildNavigationSuggestion({ | |
| surface: "missions", | |
| laneId: mission?.laneId ?? (laneId?.trim() || null), | |
| missionId: missionId?.trim() || null, | |
| }) | |
| : scope === "lane" | |
| ? buildNavigationSuggestion({ | |
| surface: "lanes", | |
| laneId: laneId?.trim() || null, | |
| }) | |
| : null, | |
| ), | |
| }; | |
| tools.exportContext = tool({ | |
| description: "Export a bounded ADE context pack for a project, lane, mission, conflict, plan, or feature scope.", | |
| inputSchema: z.object({ | |
| scope: z.enum(["project", "lane", "conflict", "plan", "feature", "mission"]), | |
| level: z.enum(["brief", "standard", "detailed"]).optional().default("standard"), | |
| laneId: z.string().optional(), | |
| missionId: z.string().optional(), | |
| featureKey: z.string().optional(), | |
| }), | |
| execute: async ({ scope, level, laneId, missionId, featureKey }) => { | |
| if (!deps.fetchContextPack) return { success: false, error: "Context export service is not available." }; | |
| if ((scope === "lane" || scope === "conflict" || scope === "plan") && !laneId?.trim()) { | |
| return { success: false, error: `${scope} context requires laneId.` }; | |
| } | |
| if (scope === "mission" && !missionId?.trim()) { | |
| return { success: false, error: "Mission context requires missionId." }; | |
| } | |
| if (scope === "feature" && !featureKey?.trim()) { | |
| return { success: false, error: "Feature context requires featureKey." }; | |
| } | |
| try { | |
| const result = await deps.fetchContextPack({ | |
| scope, | |
| level, | |
| laneId: laneId?.trim() || undefined, | |
| missionId: missionId?.trim() || undefined, | |
| featureKey: featureKey?.trim() || undefined, | |
| }); | |
| const mission = missionId?.trim() ? deps.missionService?.get(missionId.trim()) ?? null : null; | |
| return { | |
| success: true, | |
| ...result, | |
| ...buildNavigationPayload( | |
| scope === "mission" | |
| ? buildNavigationSuggestion({ | |
| surface: "missions", | |
| laneId: mission?.laneId ?? (laneId?.trim() || null), | |
| missionId: missionId?.trim() || null, | |
| }) | |
| : scope === "lane" | |
| ? buildNavigationSuggestion({ | |
| surface: "lanes", | |
| laneId: laneId?.trim() || null, | |
| }) | |
| : null, | |
| ), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts` around lines
1212 - 1249, The exportContext tool currently treats any deps.fetchContextPack
response as success; add scope-specific input validation in exportContext before
calling deps.fetchContextPack (e.g., require missionId when scope === "mission",
laneId when scope === "lane", etc.) and return { success: false, error: "<clear
message>" } for missing/invalid inputs; only call deps.fetchContextPack when
inputs pass validation and propagate its structured error result (do not
unconditionally set success: true). Use the existing symbols exportContext,
deps.fetchContextPack, and buildNavigationPayload/buildNavigationSuggestion to
locate insertion points and use deps.missionService?.get(...) only after
validating missionId.
Comment on lines
+5095
to
+5115
| const sendMessage = async (args: AgentChatSendArgs): Promise<void> => { | ||
| const dispatchStartedAt = Date.now(); | ||
| const prepared = prepareSendMessage(args); | ||
| if (!prepared) return; | ||
|
|
||
| logger.info("agent_chat.turn_dispatch_ack", { | ||
| sessionId: prepared.sessionId, | ||
| provider: prepared.managed.session.provider, | ||
| model: prepared.managed.session.model, | ||
| durationMs: Date.now() - dispatchStartedAt, | ||
| }); | ||
|
|
||
| void executePreparedSendMessage(prepared).catch((error) => { | ||
| logger.warn("agent_chat.turn_dispatch_failed", { | ||
| sessionId: prepared.sessionId, | ||
| provider: prepared.managed.session.provider, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| emitDispatchedSendFailure(prepared, error); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
sendMessage() now resolves before dispatch failures surface.
This helper no longer awaits executePreparedSendMessage(), so callers that await sendMessage() get a success even when runtime startup, auth, or thread resume fails a moment later. In this PR, spawnChat() and routeIssueToCto() both await sendChatMessage(), so they can report that the seed prompt was sent when it never landed. Keep sendMessage() rejectable, or split the fire-and-forget path into a separate API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 5095 -
5115, The sendMessage function is currently fire-and-forget because it does not
await executePreparedSendMessage, causing callers like spawnChat() and
routeIssueToCto() to believe a message succeeded even if dispatch fails; change
sendMessage (the function defined at the top of this diff) to return/await the
promise from executePreparedSendMessage so it rejects on failure (or
alternatively add a separate fire-and-forget wrapper function and keep
sendMessage awaitable), ensuring you still log and emitDispatchedSendFailure
inside the executePreparedSendMessage.catch path but do not swallow the
rejection from sendMessage.
Comment on lines
+213
to
+220
| function parseRebaseStartArgs(value: Record<string, unknown>): RebaseStartArgs { | ||
| return { | ||
| laneId: requireString(value.laneId, "lanes.rebaseStart requires laneId."), | ||
| ...(asTrimmedString(value.scope) ? { scope: value.scope as RebaseStartArgs["scope"] } : {}), | ||
| ...(asTrimmedString(value.pushMode) ? { pushMode: value.pushMode as RebaseStartArgs["pushMode"] } : {}), | ||
| ...(asTrimmedString(value.actor) ? { actor: asTrimmedString(value.actor)! } : {}), | ||
| ...(asTrimmedString(value.reason) ? { reason: asTrimmedString(value.reason)! } : {}), | ||
| }; |
There was a problem hiding this comment.
Normalize and validate union-like protocol fields before casting.
Fields like scope, pushMode, status, toolType, provider, mode, and compareTo are still accepted via unchecked casts. On Line 216, Line 217, Line 404, and Line 428 the raw, untrimmed input is even forwarded after the presence check. Since this sits on the sync protocol boundary, a skewed client payload can slip malformed literals through parsing and only fail deeper in the service layer. The validation style already used in parseLandPrArgs is the safer pattern to reuse here.
🔧 Suggested direction
+function requireOneOf<T extends string>(
+ value: unknown,
+ allowed: readonly T[],
+ message: string,
+): T {
+ const parsed = requireString(value, message);
+ if (!allowed.includes(parsed as T)) throw new Error(message);
+ return parsed as T;
+}Use the parsed local for each optional field so the trimmed value, not value.*, is what gets forwarded.
Also applies to: 236-263, 285-293, 401-429, 451-454
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts` around lines
213 - 220, parseRebaseStartArgs (and the other parsers that mirror its pattern)
currently forwards raw value.* into the returned object and casts without
validating normalized strings; instead, run asTrimmedString on each optional
union-like field (e.g., scope, pushMode, actor, reason), store the result in a
local (e.g., const scope = asTrimmedString(value.scope)), check its presence,
and then include that local in the returned object (not value.scope) with the
proper typed cast; follow the same validated/local-variable pattern used by
parseLandPrArgs and apply this change to the other parser functions handling
fields like status, toolType, provider, mode, and compareTo so only
trimmed/validated literals are forwarded to the service layer.
Comment on lines
+675
to
+685
| async function buildLaneListSnapshots( | ||
| args: SyncRemoteCommandServiceArgs, | ||
| lanes: Awaited<ReturnType<ReturnType<typeof createLaneService>["list"]>>, | ||
| ): Promise<LaneListSnapshot[]> { | ||
| const [sessions, rebaseSuggestions, autoRebaseStatuses, stateSnapshots, batchAssessment] = await Promise.all([ | ||
| Promise.resolve(args.sessionService.list({ limit: 500 })), | ||
| Promise.resolve(args.rebaseSuggestionService?.listSuggestions() ?? []), | ||
| Promise.resolve(args.autoRebaseService?.listStatuses() ?? []), | ||
| Promise.resolve(args.laneService.listStateSnapshots()), | ||
| args.conflictService?.getBatchAssessment().catch(() => null) ?? Promise.resolve(null), | ||
| ]); |
There was a problem hiding this comment.
Avoid deriving runtime summaries from capped session samples.
Line 680 and Line 726 both build runtime from truncated sessionService.list(...) results. Once those caps are hit, some sessions drop out of the aggregation and runtime.bucket / counts can go stale or regress to none. A dedicated active-session aggregate, or an uncapped summary query, would be safer.
Also applies to: 723-739
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts` around lines
675 - 685, buildLaneListSnapshots currently derives the runtime.summary from a
capped session sample (using sessionService.list with a fixed limit), which can
omit active sessions and make runtime.bucket/counts stale; replace this by
calling an uncapped/aggregate session summary API (e.g.,
sessionService.getActiveSessionSummary() or sessionService.summary()) or add a
new dedicated method that returns accurate active-session aggregates, and use
that result wherever runtime is constructed (references: buildLaneListSnapshots,
sessionService.list, and the runtime.bucket/count aggregation locations) so both
runtime builds use the uncapped/aggregate data instead of the capped list.
apps/desktop/src/renderer/components/settings/ProvidersSection.test.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+1730
to
+1745
| const agentChatService = requireAgentChatService(runtime); | ||
| const defaultLaneId = (extractLaneId(toolArgs) ?? await resolveDefaultLaneId(runtime)).trim(); | ||
| const ctoIdentity = runtime.ctoStateService.getIdentity(); | ||
| const preferredProvider = ctoIdentity.modelPreferences.provider.trim().toLowerCase(); | ||
| const fallbackModelId = preferredProvider.includes("claude") | ||
| ? (getDefaultModelDescriptor("claude")?.id ?? null) | ||
| : (getDefaultModelDescriptor("codex")?.id ?? null); | ||
| const defaultModelId = | ||
| (typeof ctoIdentity.modelPreferences.modelId === "string" && ctoIdentity.modelPreferences.modelId.trim().length | ||
| ? ctoIdentity.modelPreferences.modelId.trim() | ||
| : null) | ||
| ?? fallbackModelId; | ||
| const tools = createCtoOperatorTools({ | ||
| currentSessionId: session.identity.callerId || "mcp-cto", | ||
| defaultLaneId, | ||
| defaultModelId, |
There was a problem hiding this comment.
Resolve a lane only for lane-scoped CTO tools.
runCtoOperatorBridgeTool computes a default lane before dispatch, so tools whose schemas do not require laneId now fail whenever the repo has no active lanes. It also makes those tools inherit whichever lane happens to be first in laneService.list(), which is unrelated state for chat/session inspection calls.
🧭 Suggested fix
- const defaultLaneId = (extractLaneId(toolArgs) ?? await resolveDefaultLaneId(runtime)).trim();
+ const requestedLaneId = extractLaneId(toolArgs)?.trim() ?? null;
+ const laneScopedTools = new Set([
+ "spawnChat",
+ "routeLinearIssueToCto",
+ "routeLinearIssueToMission",
+ ]);
+ const defaultLaneId = laneScopedTools.has(name)
+ ? (requestedLaneId ?? await resolveDefaultLaneId(runtime))
+ : (requestedLaneId ?? "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mcp-server/src/mcpServer.ts` around lines 1730 - 1745, The code always
computes defaultLaneId and passes it into createCtoOperatorTools which forces
non-lane-scoped tools to depend on lane state; instead compute/resolve the
default lane only for tools that actually require a lane (or move that logic
into runCtoOperatorBridgeTool). Update the caller around createCtoOperatorTools
and runCtoOperatorBridgeTool so you either (A) detect the tool schema's
requirement for laneId before resolving defaultLaneId and only supply
defaultLaneId for lane-scoped tools, or (B) remove defaultLaneId resolution here
and perform it lazily inside runCtoOperatorBridgeTool when dispatching to a tool
that declares laneId as required; reference createCtoOperatorTools,
defaultLaneId, runCtoOperatorBridgeTool, and resolveDefaultLaneId in your
changes.
Remove legacy pack export/integration code and simplify context tooling: delete MCP bridge and team message tools, drop many pack builders/tests, rename projectPackBuilder → contextDocBuilder and move pack utilities to shared. Remove packService wiring from app main and IPC, and deprecate the "update-packs" automation action/validation. Improve agent/chat runtime: add "max" reasoning level, background task detection, richer subagent/collab/dynamic-tool/web-search event handling, enable prompt suggestions and tool-search env, and attempt model updates for live Claude v2 sessions. Also update CI (add mcp-server typecheck/lint/build), add Dependabot config, new vitest workspace and desktop test scripts, minor .gitignore additions, and assorted import/path fixes and test updates to reflect the refactor.
Owner
Author
|
@claude review recent push please, include old error message sin your review and see if theyre valid or not |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/main/services/config/projectConfigService.ts (1)
315-345:⚠️ Potential issue | 🟠 MajorFix type coercion and validation mismatch for AutomationActionType.
coerceAutomationAction(line 319) accepts"update-packs"and coerces it asAutomationActionType, but"update-packs"is not a valid member of theAutomationActionTypeunion (defined inapps/desktop/src/shared/types/config.ts:504-509). Additionally,coerceAutomationActiondoes not accept"agent-session"and"launch-mission", which are validAutomationActionTypevalues.Meanwhile,
validateEffectiveConfig(lines 2475-2482) only validates three action types ("predict-conflicts","run-tests","run-command"), ignoring the other valid union members.Either remove
"update-packs"fromcoerceAutomationActionor add it toAutomationActionTypeif it is intentionally supported. Ensure coercion and validation cover the same set of action types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/config/projectConfigService.ts` around lines 315 - 345, The coercion and validation sets for AutomationActionType are out of sync: remove the unsupported "update-packs" from coercion and update both coerceAutomationAction and validateEffectiveConfig to accept the actual union members defined in AutomationActionType (add "agent-session" and "launch-mission" if present in the type union); specifically, in coerceAutomationAction replace the typeRaw checks to only allow the union values (e.g., "predict-conflicts","run-tests","run-command","agent-session","launch-mission") and ensure validateEffectiveConfig performs the same membership checks so both coercion (coerceAutomationAction) and validation (validateEffectiveConfig) cover the identical set of action types declared in AutomationActionType.apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (1)
819-850:⚠️ Potential issue | 🟠 MajorRender navigation suggestions on the
tool_invocationpath too.This UI only runs for standalone
tool_resultrows, butappendCollapsedEvent()earlier in this file folds matchedtool_resultevents intotool_invocation. In the normaltool_call→tool_resultflow, the new navigation suggestions stay insideevent.resultand never get rendered.Also applies to: 853-866
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 819 - 850, The navigation suggestions are only computed/rendered for standalone tool_result rows but appended tool_result data may be folded into tool_invocation by appendCollapsedEvent; update AgentChatMessageList to compute and render navigation suggestions from event.result for both tool_result and collapsed tool_invocation cases by calling readNavigationSuggestions(event.result) (and using that value where navigationSuggestions is used) whenever event.result exists (not just when event.type === 'tool_result'); ensure the same change is applied to the other block around lines 853-866 so collapsed tool_result content inside tool_invocation shows the navigation suggestions.apps/desktop/src/main/services/orchestrator/orchestratorService.ts (1)
7300-7306:⚠️ Potential issue | 🟠 Major
runSessionTurnnow introduces an implicit 5-minute timeout on managed launches.This call path does not pass
timeoutMs, so long-running worker turns can fail with timeout even if execution is still valid. The priorsendMessagebehavior did not enforce this hard turn timeout.💡 Suggested fix
+ const launchTimeoutMs = Number.isFinite(Number(step.metadata?.timeoutMs)) + ? Math.max(15_000, Math.floor(Number(step.metadata?.timeoutMs))) + : Math.max(15_000, runtimeConfig.stepTimeoutDefaultMs); await agentChatService.runSessionTurn({ sessionId, text: managedLaunch.prompt, displayText: managedLaunch.displayText, ...(managedLaunch.reasoningEffort ? { reasoningEffort: managedLaunch.reasoningEffort } : {}), ...(managedLaunch.executionMode ? { executionMode: managedLaunch.executionMode } : {}), + timeoutMs: launchTimeoutMs, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/orchestrator/orchestratorService.ts` around lines 7300 - 7306, The call to agentChatService.runSessionTurn is omitting timeoutMs which causes the new implicit 5-minute turn timeout to apply to managed launches; update the call site to forward the intended timeout (e.g., use managedLaunch.timeoutMs or the existing turn timeout value) by including timeoutMs: managedLaunch.timeoutMs (or undefined/fallback when no hard timeout was intended) in the argument object so long-running managed launches preserve the prior behaviour; locate the invocation of agentChatService.runSessionTurn and add the timeoutMs property to the payload along with sessionId, text, displayText, reasoningEffort, and executionMode.apps/desktop/src/main/services/orchestrator/orchestratorService.test.ts (1)
3357-3376:⚠️ Potential issue | 🔴 Critical
laneExportCallsassertion is stale and now fails deterministically.Line 3357 initializes
laneExportCalls, but nothing increments it anymore; Line 3375 still expects1, so this test will fail independent of runtime behavior.🛠️ Proposed fix
- let laneExportCalls = 0; - const fixture = await createFixture({}); + const fixture = await createFixture(); @@ - expect(laneExportCalls).toBe(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/orchestrator/orchestratorService.test.ts` around lines 3357 - 3376, The test declares laneExportCalls but no longer increments it, so the assertion expect(laneExportCalls).toBe(1) is stale and fails; remove that assertion (the laneExportCalls variable and the expect(...) line) and rely on the existing snapshot assertion (checking snapshot?.cursor.contextSources for "context_export:project:") to validate export behavior, or alternatively update the fixture/mock that should increment laneExportCalls inside createFixture so it is actually incremented when the export happens; update references to laneExportCalls accordingly in orchestratorService.test.ts around the startRun/startAttempt/listContextSnapshots assertions.
♻️ Duplicate comments (3)
apps/desktop/src/main/services/chat/agentChatService.ts (3)
2664-2687:⚠️ Potential issue | 🟡 MinorIgnore
task_progressframes withouttask_id.
trackSubagentEvent()indexes snapshots bytaskId, so emittingsubagent_progresswith""creates a phantom entry and lets unrelated worker updates overwrite each other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 2664 - 2687, The handler for "system" subtype "task_progress" currently emits a subagent_progress event even when task_id is empty, creating phantom entries; update the logic in the task_progress block so you only call runtime.activeSubagents.set and emitChatEvent for subagent_progress when taskId is non-empty (i.e., check taskId.length before emitting), or alternatively return/continue early when taskId is falsy; reference the task_progress branch, the taskId variable, runtime.activeSubagents, and emitChatEvent to locate where to apply the change.
5296-5316:⚠️ Potential issue | 🟠 MajorDon't make
sendMessage()fire-and-forget.Callers awaiting this method now get a resolved promise even when runtime startup, auth, or thread resume fails moments later. Keep
sendMessage()rejectable and move the detached path into a separate helper if you need both behaviors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 5296 - 5316, sendMessage currently starts executePreparedSendMessage(prepared) in a fire-and-forget manner so callers get a resolved promise even if runtime startup/auth/thread resume fails; change sendMessage to return (or await) the executePreparedSendMessage(prepared) promise so failures propagate to callers (keep the existing logger.warn and emitDispatchedSendFailure in the .catch handler), and if you still need a detached behavior extract that detached invocation into a separate helper (e.g., dispatchPreparedSendDetached) and call it where truly fire-and-forget behavior is desired; update references to sendMessage to use the rejectable promise as needed.
5937-5942:⚠️ Potential issue | 🟠 MajorDefer Claude session reset until the turn is idle.
This still closes
v2Sessionwhileruntime.busymay be true, which can kill the in-flight Claude turn. Gate the reset on idle or queue a pending reset for after completion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 5937 - 5942, Resetting Claude's v2Session while a turn is in-flight can abort work; modify the block that currently calls cancelClaudeWarmup(managed, managed.runtime, "session_reset") and closes managed.runtime.v2Session to first check managed.runtime.busy, and if busy set a pending reset flag (e.g., managed.runtime.pendingReset = true) or enqueue the reset; then ensure elsewhere where runtime.busy transitions to false you perform the actual cancelClaudeWarmup and clear/close v2Session and v2WarmupDone (and clear pendingReset). Use the existing symbols managed.runtime, managed.runtime.busy, cancelClaudeWarmup, v2Session, v2WarmupDone and add/consume managed.runtime.pendingReset to defer the reset until idle.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/orchestrator/orchestratorContext.ts (1)
905-910: Normalize legacy provider tokens before populatingdefaultOrchestratorModelId.Line 909 currently falls back from
defaultPlannerProviderdirectly into a model-id field. If legacy config storesclaude/codex, this leaves a non-model value indefaultOrchestratorModelId.♻️ Proposed hardening
- const legacyProvider = asString(orchestrator.defaultPlannerProvider)?.trim() || null; - const defaultOrchestratorModelId = orcModelId || legacyProvider; + const legacyRaw = asString(orchestrator.defaultPlannerProvider)?.trim() || null; + const legacyModelId = legacyRaw && getModelById(legacyRaw) ? legacyRaw : null; + const legacyProviderModelId = + legacyRaw === "claude" + ? CALL_TYPE_DEFAULTS.coordinator.model + : legacyRaw === "codex" + ? CALL_TYPE_DEFAULTS.chat_response.model + : null; + const defaultOrchestratorModelId = orcModelId || legacyModelId || legacyProviderModelId;Also applies to: 918-918
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/orchestrator/orchestratorContext.ts` around lines 905 - 910, The fallback from orchestrator.defaultPlannerProvider into defaultOrchestratorModelId can populate non-model tokens (e.g., "claude" or "codex"); normalize legacy provider tokens before assignment by mapping legacy provider names to their canonical model IDs (e.g., map "claude" -> "claude-v1", "codex" -> appropriate model) and use that mapped value instead of the raw legacyProvider string when computing defaultOrchestratorModelId; update the logic around orcModelId, legacyProvider, and defaultOrchestratorModelId to apply this normalization (and reuse the same normalization for the similar occurrence at the later block around line 918).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts`:
- Around line 201-219: resolveWorkspaceIdForLane currently calls
deps.fileService.listWorkspaces({ includeArchived: true }) and picks the first
lane match which can return an archived workspace; change the lane lookup to
prefer active workspaces first: after obtaining workspaces, if no explicit
workspaceId is provided then compute laneId as done and search
workspaces.filter(w => !w.archived).find(w => w.laneId === laneId) and only if
that returns undefined fall back to searching archived ones (workspaces.find(w
=> w.laneId === laneId)); keep the explicit workspaceId path unchanged and throw
the same errors when no match is found.
- Around line 523-561: The input schemas for tools.resumeChat and tools.endChat
currently accept empty or whitespace-only sessionId values; update their
inputSchema definitions (the z.object containing sessionId) to validate
non-empty trimmed strings by replacing z.string() with z.string().trim().min(1)
so invalid IDs are rejected at the boundary; also apply the same pattern to
other similar schemas referenced (around lines 1009-1092, 1105-1162, 1193-1253)
to ensure consistent early validation across functions that accept
identifiers/paths.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 2919-2929: The current prompt_suggestion branch uses .find((v) =>
typeof v === "string") which stops on empty strings; change the predicate used
to select suggestionText to pick the first string whose trimmed length > 0 (e.g.
typeof v === "string" && v.trim().length > 0) and then use the trimmed value
when emitting; update the selection logic around suggestionMsg, suggestionText
and the emitChatEvent call so empty "" values no longer mask later candidates.
- Around line 4719-4728: The warmup Promise.race unblocks before warmupTask
finishes and can allow a new turn to install runtime.v2Session which the
canceled warmup later closes; fix it by (a) keeping a local session handle
inside the warmupTask (capture the session returned/used by warmupTask) and only
close/null runtime.v2Session if runtime.v2Session === localSession, and (b)
avoid treating the raced promise as the canonical completion token — set
runtime.v2WarmupDone to the actual warmupTask (or to warmupTask.then(...))
rather than the race result so runtime.v2WarmupDone remains pending until the
warmupTask fully exits; also ensure runtime.v2WarmupCancel is only cleared when
cancelWarmup matches the token that started the actual warmupTask.
- Line 365: validateReasoningEffort currently maps Claude's "max" to "high", so
even though UI sets { effort: "max" } (e.g., in agentChatService payloads) the
SDK never receives it; update validateReasoningEffort to stop aliasing "max" to
"high" and treat "max" as a distinct allowed value, and ensure any callers in
create/send flows (functions referencing validateReasoningEffort) pass the
unchecked value through or validate against the expanded set so "max" is
preserved end-to-end.
In `@apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts`:
- Around line 4799-4805: The branch that maps config.defaultOrchestratorModelId
to "claude" or "codex" misses legacy values like "anthropic" and "openai" so
older configs can fall through; update the logic in the block handling
config.defaultOrchestratorModelId (and the call to getModelById) to first treat
legacy provider keys ("anthropic" -> "claude", "openai" -> "codex") or accept
those strings directly, then fall back to using desc?.family to map families to
"claude" or "codex" as before (refer to config.defaultOrchestratorModelId,
getModelById, and desc.family to locate the code).
In `@apps/desktop/src/main/services/orchestrator/orchestratorService.ts`:
- Around line 139-146: The snapshot metadata is being written with raw
profile/policy export levels (args.contextProfile.projectExportLevel and
contextPolicy.projectExportLevel) instead of the effective, possibly adjusted
export level computed earlier (e.g., at the adjustment in the analysis step
around projectExportLevel). Update the snapshot creation and any
metadata/handoff builders (places producing context manifest,
CreateSnapshotResult fields, and the code paths referenced by the comment like
the uses at args.contextProfile.projectExportLevel /
contextPolicy.projectExportLevel) to use the resolved export level variable
(introduce/propagate a resolvedProjectExportLevel or resolvedContextProfile that
contains the adjusted projectExportLevel) so downstream metadata and
CreateSnapshotResult.projectExport/projectExportLevel consistently reflect the
effective export policy; replace direct reads of
args.contextProfile.projectExportLevel and contextPolicy.projectExportLevel with
the resolved value everywhere noted (including the other locations called out).
- Around line 2933-2941: projectPackVersionId is computed from
projectExport.content (projectVersionId) which is empty by default, causing
identical version IDs across different context compositions; update the code
that builds projectPackVersionId (and the OrchestratorContextSnapshotCursor
assignment) to use a deterministic serialization of the effective project
context instead of raw projectExport.content — e.g., compute sha256 over a
serialized composition payload (projectExport plus any active lane/context
metadata or a generated snapshot object) or include a stable changing field
(timestamp/nonce) in the hashed input so different compositions produce distinct
projectVersionId values; ensure the change touches the projectVersionId
computation and any code that consumes projectPackVersionId (reference symbols:
projectVersionId, projectExport, projectPackVersionId,
OrchestratorContextSnapshotCursor, laneExport, lanePackKey).
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 999-1027: The ghost suggestion overlay is aria-hidden and the
textarea placeholder is emptied when promptSuggestion exists, so screen readers
miss the suggestion and Tab hint; fix this by rendering a visually-hidden
element (e.g., a span with an id like suggestionHintId) containing the
suggestion text plus the "Tab" hint and reference it from the textarea via
aria-describedby (while keeping the placeholder stable instead of setting it to
""), and ensure that promptSuggestion updates are announced (use
aria-live="polite" on the hidden suggestion element) so assistive tech hears
changes; update the textarea props (value draft, ref textareaRef, and
placeholder logic that currently uses promptSuggestion) to include
aria-describedby when promptSuggestion is present and keep existing behavior for
slashPickerOpen and setSlashQuery unchanged.
- Around line 1017-1022: The slash-picker cursor can become out-of-range when
the query narrows; update the onChange handler in AgentChatComposer to reset or
clamp the slashCursor whenever the slash query changes. Specifically, inside the
block that runs when val.startsWith("/"), call setSlashQuery(newQuery) and then
reset the cursor (e.g. setSlashCursor(0) or setSlashCursor(prev =>
Math.min(prev, Math.max(0, filteredSlashCommands.length - 1)))) so slashCursor
never exceeds the available filteredSlashCommands; reference the onChange
handler, setSlashQuery, setSlashCursor, slashCursor and filteredSlashCommands
when making the change.
---
Outside diff comments:
In `@apps/desktop/src/main/services/config/projectConfigService.ts`:
- Around line 315-345: The coercion and validation sets for AutomationActionType
are out of sync: remove the unsupported "update-packs" from coercion and update
both coerceAutomationAction and validateEffectiveConfig to accept the actual
union members defined in AutomationActionType (add "agent-session" and
"launch-mission" if present in the type union); specifically, in
coerceAutomationAction replace the typeRaw checks to only allow the union values
(e.g.,
"predict-conflicts","run-tests","run-command","agent-session","launch-mission")
and ensure validateEffectiveConfig performs the same membership checks so both
coercion (coerceAutomationAction) and validation (validateEffectiveConfig) cover
the identical set of action types declared in AutomationActionType.
In `@apps/desktop/src/main/services/orchestrator/orchestratorService.test.ts`:
- Around line 3357-3376: The test declares laneExportCalls but no longer
increments it, so the assertion expect(laneExportCalls).toBe(1) is stale and
fails; remove that assertion (the laneExportCalls variable and the expect(...)
line) and rely on the existing snapshot assertion (checking
snapshot?.cursor.contextSources for "context_export:project:") to validate
export behavior, or alternatively update the fixture/mock that should increment
laneExportCalls inside createFixture so it is actually incremented when the
export happens; update references to laneExportCalls accordingly in
orchestratorService.test.ts around the
startRun/startAttempt/listContextSnapshots assertions.
In `@apps/desktop/src/main/services/orchestrator/orchestratorService.ts`:
- Around line 7300-7306: The call to agentChatService.runSessionTurn is omitting
timeoutMs which causes the new implicit 5-minute turn timeout to apply to
managed launches; update the call site to forward the intended timeout (e.g.,
use managedLaunch.timeoutMs or the existing turn timeout value) by including
timeoutMs: managedLaunch.timeoutMs (or undefined/fallback when no hard timeout
was intended) in the argument object so long-running managed launches preserve
the prior behaviour; locate the invocation of agentChatService.runSessionTurn
and add the timeoutMs property to the payload along with sessionId, text,
displayText, reasoningEffort, and executionMode.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 819-850: The navigation suggestions are only computed/rendered for
standalone tool_result rows but appended tool_result data may be folded into
tool_invocation by appendCollapsedEvent; update AgentChatMessageList to compute
and render navigation suggestions from event.result for both tool_result and
collapsed tool_invocation cases by calling
readNavigationSuggestions(event.result) (and using that value where
navigationSuggestions is used) whenever event.result exists (not just when
event.type === 'tool_result'); ensure the same change is applied to the other
block around lines 853-866 so collapsed tool_result content inside
tool_invocation shows the navigation suggestions.
---
Duplicate comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 2664-2687: The handler for "system" subtype "task_progress"
currently emits a subagent_progress event even when task_id is empty, creating
phantom entries; update the logic in the task_progress block so you only call
runtime.activeSubagents.set and emitChatEvent for subagent_progress when taskId
is non-empty (i.e., check taskId.length before emitting), or alternatively
return/continue early when taskId is falsy; reference the task_progress branch,
the taskId variable, runtime.activeSubagents, and emitChatEvent to locate where
to apply the change.
- Around line 5296-5316: sendMessage currently starts
executePreparedSendMessage(prepared) in a fire-and-forget manner so callers get
a resolved promise even if runtime startup/auth/thread resume fails; change
sendMessage to return (or await) the executePreparedSendMessage(prepared)
promise so failures propagate to callers (keep the existing logger.warn and
emitDispatchedSendFailure in the .catch handler), and if you still need a
detached behavior extract that detached invocation into a separate helper (e.g.,
dispatchPreparedSendDetached) and call it where truly fire-and-forget behavior
is desired; update references to sendMessage to use the rejectable promise as
needed.
- Around line 5937-5942: Resetting Claude's v2Session while a turn is in-flight
can abort work; modify the block that currently calls
cancelClaudeWarmup(managed, managed.runtime, "session_reset") and closes
managed.runtime.v2Session to first check managed.runtime.busy, and if busy set a
pending reset flag (e.g., managed.runtime.pendingReset = true) or enqueue the
reset; then ensure elsewhere where runtime.busy transitions to false you perform
the actual cancelClaudeWarmup and clear/close v2Session and v2WarmupDone (and
clear pendingReset). Use the existing symbols managed.runtime,
managed.runtime.busy, cancelClaudeWarmup, v2Session, v2WarmupDone and
add/consume managed.runtime.pendingReset to defer the reset until idle.
---
Nitpick comments:
In `@apps/desktop/src/main/services/orchestrator/orchestratorContext.ts`:
- Around line 905-910: The fallback from orchestrator.defaultPlannerProvider
into defaultOrchestratorModelId can populate non-model tokens (e.g., "claude" or
"codex"); normalize legacy provider tokens before assignment by mapping legacy
provider names to their canonical model IDs (e.g., map "claude" -> "claude-v1",
"codex" -> appropriate model) and use that mapped value instead of the raw
legacyProvider string when computing defaultOrchestratorModelId; update the
logic around orcModelId, legacyProvider, and defaultOrchestratorModelId to apply
this normalization (and reuse the same normalization for the similar occurrence
at the later block around line 918).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e31b680e-fd2f-4b66-b89e-2c926ca19a8d
⛔ Files ignored due to path filters (32)
.github/dependabot.ymlis excluded by none and included by none.github/workflows/ci.ymlis excluded by none and included by none.gitignoreis excluded by none and included by noneapps/desktop/package.jsonis excluded by none and included by noneapps/desktop/vitest.config.tsis excluded by none and included by noneapps/desktop/vitest.workspace.tsis excluded by none and included by noneapps/ios/ADE.xcodeproj/project.pbxprojis excluded by none and included by noneapps/ios/ADE/App/ADEApp.swiftis excluded by none and included by noneapps/ios/ADE/App/ContentView.swiftis excluded by none and included by noneapps/ios/ADE/Views/FilesTabView.swiftis excluded by none and included by noneapps/ios/ADE/Views/LanesTabView.swiftis excluded by none and included by noneapps/ios/ADE/Views/PRsTabView.swiftis excluded by none and included by noneapps/ios/ADE/Views/WorkTabView.swiftis excluded by none and included by noneapps/ios/Vendor/crsqlite/.tool-versionsis excluded by none and included by noneapps/ios/Vendor/crsqlite/crsqlite.xcframework/Info.plistis excluded by none and included by noneapps/ios/Vendor/crsqlite/crsqlite.xcframework/ios-arm64/crsqlite.framework/Info.plistis excluded by none and included by noneapps/ios/Vendor/crsqlite/crsqlite.xcframework/ios-arm64/crsqlite.framework/crsqliteis excluded by none and included by noneapps/ios/Vendor/crsqlite/crsqlite.xcframework/ios-arm64_x86_64-simulator/crsqlite.framework/Info.plistis excluded by none and included by noneapps/ios/Vendor/crsqlite/crsqlite.xcframework/ios-arm64_x86_64-simulator/crsqlite.framework/crsqliteis excluded by none and included by nonedocs/PRD.mdis excluded by!docs/**and included by nonedocs/architecture/MEMORY.mdis excluded by!docs/**and included by nonedocs/architecture/MULTI_DEVICE_SYNC.mdis excluded by!docs/**and included by nonedocs/features/CONFLICTS.mdis excluded by!docs/**and included by nonedocs/features/CTO.mdis excluded by!docs/**and included by nonedocs/features/HISTORY.mdis excluded by!docs/**and included by nonedocs/final-plan/README.mdis excluded by!docs/**and included by nonedocs/final-plan/appendix.mdis excluded by!docs/**and included by nonedocs/final-plan/phase-3.mdis excluded by!docs/**and included by nonedocs/final-plan/phase-4/README.mdis excluded by!docs/**and included by nonedocs/final-plan/phase-8.mdis excluded by!docs/**and included by nonedocs/final-plan/phases-1-2.mdis excluded by!docs/**and included by nonepackage.jsonis excluded by none and included by none
📒 Files selected for processing (92)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/agentExecutor.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/ai/tools/index.tsapps/desktop/src/main/services/ai/tools/mcpBridge.tsapps/desktop/src/main/services/ai/tools/teamMessageTool.tsapps/desktop/src/main/services/automations/automationPlannerService.tsapps/desktop/src/main/services/automations/automationService.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/config/projectConfigService.tsapps/desktop/src/main/services/conflicts/conflictService.tsapps/desktop/src/main/services/context/contextDocBuilder.tsapps/desktop/src/main/services/context/contextDocService.test.tsapps/desktop/src/main/services/context/contextDocService.tsapps/desktop/src/main/services/cto/workerAgentService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/missions/missionService.tsapps/desktop/src/main/services/missions/phaseEngine.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.tsapps/desktop/src/main/services/orchestrator/hardeningMissions.test.tsapps/desktop/src/main/services/orchestrator/missionBudgetService.test.tsapps/desktop/src/main/services/orchestrator/orchestratorContext.tsapps/desktop/src/main/services/orchestrator/orchestratorService.test.tsapps/desktop/src/main/services/orchestrator/orchestratorService.tsapps/desktop/src/main/services/orchestrator/orchestratorSmoke.test.tsapps/desktop/src/main/services/orchestrator/planningFlowAndHandoffs.test.tsapps/desktop/src/main/services/orchestrator/planningGapsFixes.test.tsapps/desktop/src/main/services/orchestrator/qualityGateService.tsapps/desktop/src/main/services/orchestrator/runtimeInterventionsSteeringErrors.test.tsapps/desktop/src/main/services/orchestrator/stateCoherence.test.tsapps/desktop/src/main/services/orchestrator/worktreeIsolation.test.tsapps/desktop/src/main/services/packs/conflictPackBuilder.tsapps/desktop/src/main/services/packs/lanePackTemplate.test.tsapps/desktop/src/main/services/packs/lanePackTemplate.tsapps/desktop/src/main/services/packs/missionPackBuilder.tsapps/desktop/src/main/services/packs/packDeltaDigest.test.tsapps/desktop/src/main/services/packs/packExports.test.tsapps/desktop/src/main/services/packs/packExports.tsapps/desktop/src/main/services/packs/packSections.test.tsapps/desktop/src/main/services/packs/packSections.tsapps/desktop/src/main/services/packs/packService.docsFreshness.test.tsapps/desktop/src/main/services/packs/packService.missionPack.test.tsapps/desktop/src/main/services/packs/packService.tsapps/desktop/src/main/services/packs/packUtils.tsapps/desktop/src/main/services/packs/transcriptInsights.test.tsapps/desktop/src/main/services/sessions/sessionDeltaService.tsapps/desktop/src/main/services/shared/packLegacyUtils.tsapps/desktop/src/main/services/shared/transcriptInsights.tsapps/desktop/src/main/services/state/onConflictAudit.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/ProjectSelector.tsxapps/desktop/src/renderer/components/automations/components/RuleEditorPanel.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatSubagentStrip.test.tsxapps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsxapps/desktop/src/renderer/components/chat/chatExecutionSummary.tsapps/desktop/src/renderer/components/history/EventDetailPanel.tsxapps/desktop/src/renderer/components/lanes/ImportBranchDialog.tsxapps/desktop/src/renderer/components/lanes/laneTreeBuilder.tsapps/desktop/src/renderer/components/missions/CreateMissionDialog.tsxapps/desktop/src/renderer/components/missions/ManageMissionDialog.tsxapps/desktop/src/renderer/components/missions/MissionSidebar.tsxapps/desktop/src/renderer/components/missions/MissionsPage.tsxapps/desktop/src/renderer/components/missions/ModelProfileSelector.tsxapps/desktop/src/renderer/components/missions/PhaseProgressBar.tsxapps/desktop/src/renderer/components/missions/missionHelpers.tsapps/desktop/src/renderer/components/missions/useMissionsStore.tsapps/desktop/src/renderer/components/prs/IntegrationStepDetail.tsxapps/desktop/src/renderer/components/prs/shared/ModelSelector.tsxapps/desktop/src/renderer/components/prs/shared/PipelineNode.tsxapps/desktop/src/renderer/components/prs/shared/prVisuals.tsxapps/desktop/src/renderer/components/settings/KeybindingsSection.tsxapps/desktop/src/renderer/components/ui/Kbd.tsxapps/desktop/src/renderer/components/ui/SplitPane.tsxapps/desktop/src/renderer/lib/celebrations.tsapps/desktop/src/renderer/lib/motion.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/automations.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/config.tsapps/desktop/src/shared/types/index.tsapps/desktop/src/shared/types/missions.tsapps/mcp-server/src/bootstrap.tsapps/mcp-server/src/mcpServer.tsapps/mcp-server/vitest.config.ts
💤 Files with no reviewable changes (48)
- apps/desktop/src/main/services/orchestrator/missionBudgetService.test.ts
- apps/desktop/src/main/services/ai/tools/mcpBridge.ts
- apps/desktop/src/main/services/orchestrator/hardeningMissions.test.ts
- apps/desktop/src/main/services/missions/phaseEngine.ts
- apps/desktop/src/main/services/ai/agentExecutor.ts
- apps/desktop/src/main/services/cto/workerAgentService.ts
- apps/desktop/src/renderer/components/app/ProjectSelector.tsx
- apps/desktop/src/main/services/orchestrator/worktreeIsolation.test.ts
- apps/desktop/src/renderer/components/prs/shared/prVisuals.tsx
- apps/desktop/src/main/services/orchestrator/planningGapsFixes.test.ts
- apps/desktop/src/main/services/packs/packExports.test.ts
- apps/desktop/src/main/services/orchestrator/stateCoherence.test.ts
- apps/desktop/src/main/services/orchestrator/qualityGateService.ts
- apps/desktop/src/main/services/automations/automationPlannerService.ts
- apps/desktop/src/main/services/packs/lanePackTemplate.test.ts
- apps/desktop/src/main/services/packs/packSections.test.ts
- apps/desktop/src/main/services/packs/packService.docsFreshness.test.ts
- apps/desktop/src/main/services/packs/packDeltaDigest.test.ts
- apps/desktop/src/renderer/components/prs/shared/PipelineNode.tsx
- apps/desktop/src/renderer/components/automations/components/RuleEditorPanel.tsx
- apps/desktop/src/renderer/components/lanes/laneTreeBuilder.ts
- apps/desktop/src/renderer/components/prs/shared/ModelSelector.tsx
- apps/desktop/src/main/services/orchestrator/runtimeInterventionsSteeringErrors.test.ts
- apps/desktop/src/main/services/automations/automationService.ts
- apps/desktop/src/main/services/packs/packService.missionPack.test.ts
- apps/desktop/src/renderer/components/missions/PhaseProgressBar.tsx
- apps/desktop/src/main/services/ai/tools/index.ts
- apps/desktop/src/main/services/orchestrator/planningFlowAndHandoffs.test.ts
- apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts
- apps/desktop/src/preload/preload.ts
- apps/desktop/src/main/services/packs/transcriptInsights.test.ts
- apps/desktop/src/main/services/orchestrator/orchestratorSmoke.test.ts
- apps/desktop/src/preload/global.d.ts
- apps/desktop/src/main/services/ai/tools/teamMessageTool.ts
- apps/desktop/src/renderer/components/lanes/ImportBranchDialog.tsx
- apps/desktop/src/renderer/browserMock.ts
- apps/desktop/src/renderer/components/settings/KeybindingsSection.tsx
- apps/desktop/src/renderer/components/missions/ModelProfileSelector.tsx
- apps/desktop/src/main/services/packs/packExports.ts
- apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx
- apps/desktop/src/main/services/packs/packSections.ts
- apps/desktop/src/main/services/packs/packUtils.ts
- apps/desktop/src/main/services/packs/lanePackTemplate.ts
- apps/desktop/src/renderer/components/prs/IntegrationStepDetail.tsx
- apps/desktop/src/main/services/packs/missionPackBuilder.ts
- apps/desktop/src/main/services/packs/conflictPackBuilder.ts
- apps/desktop/src/main/services/packs/packService.ts
- apps/desktop/src/main/services/ipc/registerIpc.ts
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src/main/services/context/contextDocBuilder.ts
- apps/desktop/src/main/services/shared/transcriptInsights.ts
- apps/desktop/src/main/services/conflicts/conflictService.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/renderer/components/chat/ChatSubagentStrip.test.tsx
- apps/desktop/src/main/services/state/onConflictAudit.test.ts
- apps/desktop/src/renderer/components/chat/chatExecutionSummary.ts
- apps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.ts
- apps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsx
Comment on lines
+4719
to
+4728
| const warmupPromise = Promise.race([warmupTask, waitForCancel]); | ||
| runtime.v2WarmupDone = warmupPromise; | ||
|
|
||
| void warmupPromise.finally(() => { | ||
| if (runtime.v2WarmupDone === warmupPromise) { | ||
| runtime.v2WarmupDone = null; | ||
| } | ||
| if (runtime.v2WarmupCancel === cancelWarmup) { | ||
| runtime.v2WarmupCancel = null; | ||
| } |
There was a problem hiding this comment.
Warmup cancellation can tear down the replacement Claude session.
Promise.race([warmupTask, waitForCancel]) unblocks waiters before warmupTask stops touching runtime.v2Session. A new turn can install a fresh session, and the canceled warmup task can later close/null that shared field. Keep a local session handle and only clean it up if it still matches, or keep v2WarmupDone pending until the warmup task fully exits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 4719 -
4728, The warmup Promise.race unblocks before warmupTask finishes and can allow
a new turn to install runtime.v2Session which the canceled warmup later closes;
fix it by (a) keeping a local session handle inside the warmupTask (capture the
session returned/used by warmupTask) and only close/null runtime.v2Session if
runtime.v2Session === localSession, and (b) avoid treating the raced promise as
the canonical completion token — set runtime.v2WarmupDone to the actual
warmupTask (or to warmupTask.then(...)) rather than the race result so
runtime.v2WarmupDone remains pending until the warmupTask fully exits; also
ensure runtime.v2WarmupCancel is only cleared when cancelWarmup matches the
token that started the actual warmupTask.
Comment on lines
+4799
to
4805
| if (config.defaultOrchestratorModelId === "claude" || config.defaultOrchestratorModelId === "codex") { | ||
| return config.defaultOrchestratorModelId as "claude" | "codex"; | ||
| } | ||
| if (config.defaultPlannerProvider) { | ||
| const desc = getModelById(config.defaultPlannerProvider); | ||
| if (config.defaultOrchestratorModelId) { | ||
| const desc = getModelById(config.defaultOrchestratorModelId); | ||
| if (desc?.family === "anthropic") return "claude"; | ||
| if (desc?.family === "openai") return "codex"; |
There was a problem hiding this comment.
Preserve backward compatibility for legacy provider keys.
If older configs still store "anthropic"/"openai", this branch no longer honors that preference and may fall through to generic availability.
💡 Suggested compatibility patch
- if (config.defaultOrchestratorModelId === "claude" || config.defaultOrchestratorModelId === "codex") {
- return config.defaultOrchestratorModelId as "claude" | "codex";
- }
- if (config.defaultOrchestratorModelId) {
- const desc = getModelById(config.defaultOrchestratorModelId);
+ const preferredModelId = toOptionalString(config.defaultOrchestratorModelId);
+ const preferredKey = preferredModelId?.toLowerCase() ?? "";
+ if (preferredKey === "claude" || preferredKey === "codex") {
+ return preferredKey;
+ }
+ // Backward compatibility for pre-model-id config values.
+ if (preferredKey === "anthropic") return "claude";
+ if (preferredKey === "openai") return "codex";
+ if (preferredModelId) {
+ const desc = getModelById(preferredModelId);
if (desc?.family === "anthropic") return "claude";
if (desc?.family === "openai") return "codex";
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts` around
lines 4799 - 4805, The branch that maps config.defaultOrchestratorModelId to
"claude" or "codex" misses legacy values like "anthropic" and "openai" so older
configs can fall through; update the logic in the block handling
config.defaultOrchestratorModelId (and the call to getModelById) to first treat
legacy provider keys ("anthropic" -> "claude", "openai" -> "codex") or accept
those strings directly, then fall back to using desc?.family to map families to
"claude" or "codex" as before (refer to config.defaultOrchestratorModelId,
getModelById, and desc.family to locate the code).
Comment on lines
139
to
146
| type CreateSnapshotResult = { | ||
| snapshotId: string; | ||
| cursor: OrchestratorContextSnapshotCursor; | ||
| laneExport: PackExport | null; | ||
| projectExport: PackExport; | ||
| laneExport: { content: string; truncated: boolean } | null; | ||
| projectExport: { content: string; truncated: boolean }; | ||
| docsRefs: OrchestratorDocsRef[]; | ||
| fullDocs: Array<{ path: string; content: string; truncated: boolean }>; | ||
| }; |
There was a problem hiding this comment.
Resolved export levels are not propagated consistently to downstream metadata.
At Line 2506, projectExportLevel is adjusted for analysis steps, but context manifest/handoff metadata still use raw profile levels (args.contextProfile.projectExportLevel / contextPolicy.projectExportLevel). This can drift persisted metadata from the snapshot’s effective export policy.
💡 Suggested fix
type CreateSnapshotResult = {
snapshotId: string;
cursor: OrchestratorContextSnapshotCursor;
laneExport: { content: string; truncated: boolean } | null;
projectExport: { content: string; truncated: boolean };
+ resolvedLaneExportLevel: OrchestratorContextPolicyProfile["laneExportLevel"];
+ resolvedProjectExportLevel: OrchestratorContextPolicyProfile["projectExportLevel"] | "standard";
docsRefs: OrchestratorDocsRef[];
fullDocs: Array<{ path: string; content: string; truncated: boolean }>;
}; export type OrchestratorExecutorStartArgs = {
@@
laneExport: { content: string; truncated: boolean } | null;
projectExport: { content: string; truncated: boolean };
+ resolvedLaneExportLevel: OrchestratorContextPolicyProfile["laneExportLevel"];
+ resolvedProjectExportLevel: OrchestratorContextPolicyProfile["projectExportLevel"] | "standard";- project: {
- packKey: "project",
- level: args.contextProfile.projectExportLevel,
+ project: {
+ packKey: "project",
+ level: args.resolvedProjectExportLevel,
approxTokens: null,
contentPreview: clipText(args.projectExport.content, 2_000)
}Also applies to: 198-200, 3819-3822, 6576-6577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/orchestrator/orchestratorService.ts` around
lines 139 - 146, The snapshot metadata is being written with raw profile/policy
export levels (args.contextProfile.projectExportLevel and
contextPolicy.projectExportLevel) instead of the effective, possibly adjusted
export level computed earlier (e.g., at the adjustment in the analysis step
around projectExportLevel). Update the snapshot creation and any
metadata/handoff builders (places producing context manifest,
CreateSnapshotResult fields, and the code paths referenced by the comment like
the uses at args.contextProfile.projectExportLevel /
contextPolicy.projectExportLevel) to use the resolved export level variable
(introduce/propagate a resolvedProjectExportLevel or resolvedContextProfile that
contains the adjusted projectExportLevel) so downstream metadata and
CreateSnapshotResult.projectExport/projectExportLevel consistently reflect the
effective export policy; replace direct reads of
args.contextProfile.projectExportLevel and contextPolicy.projectExportLevel with
the resolved value everywhere noted (including the other locations called out).
Comment on lines
+2933
to
+2941
| const laneVersionId = laneExport ? `live:${lanePackKey ?? "lane"}:${sha256(Buffer.from(laneExport.content))}` : null; | ||
| const projectVersionId = `live:project:${sha256(Buffer.from(projectExport.content))}`; | ||
| const cursor: OrchestratorContextSnapshotCursor = { | ||
| lanePackKey, | ||
| lanePackVersionId: laneVersionId, | ||
| lanePackVersionNumber: laneExport?.header.versionNumber ?? null, | ||
| lanePackVersionNumber: null, | ||
| projectPackKey: "project", | ||
| projectPackVersionId: projectVersionId, | ||
| projectPackVersionNumber: projectExport.header.versionNumber ?? null, | ||
| projectPackVersionNumber: null, |
There was a problem hiding this comment.
Project pack version ID is effectively static with empty export content.
projectPackVersionId is derived from projectExport.content, which is currently empty by default, so snapshots can collapse onto the same version identity even when context composition differs.
💡 Suggested fix
- const projectVersionId = `live:project:${sha256(Buffer.from(projectExport.content))}`;
+ const projectVersionSeed = JSON.stringify({
+ level: projectExportLevel,
+ docs: docsRefs.map((doc) => `${doc.path}:${doc.sha256}`),
+ handoffIds: missionHandoffIds,
+ handoffDigest: missionHandoffDigest,
+ docsMode: args.contextProfile.docsMode,
+ });
+ const projectVersionId = `live:project:${sha256(Buffer.from(projectVersionSeed))}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const laneVersionId = laneExport ? `live:${lanePackKey ?? "lane"}:${sha256(Buffer.from(laneExport.content))}` : null; | |
| const projectVersionId = `live:project:${sha256(Buffer.from(projectExport.content))}`; | |
| const cursor: OrchestratorContextSnapshotCursor = { | |
| lanePackKey, | |
| lanePackVersionId: laneVersionId, | |
| lanePackVersionNumber: laneExport?.header.versionNumber ?? null, | |
| lanePackVersionNumber: null, | |
| projectPackKey: "project", | |
| projectPackVersionId: projectVersionId, | |
| projectPackVersionNumber: projectExport.header.versionNumber ?? null, | |
| projectPackVersionNumber: null, | |
| const laneVersionId = laneExport ? `live:${lanePackKey ?? "lane"}:${sha256(Buffer.from(laneExport.content))}` : null; | |
| const projectVersionSeed = JSON.stringify({ | |
| level: projectExportLevel, | |
| docs: docsRefs.map((doc) => `${doc.path}:${doc.sha256}`), | |
| handoffIds: missionHandoffIds, | |
| handoffDigest: missionHandoffDigest, | |
| docsMode: args.contextProfile.docsMode, | |
| }); | |
| const projectVersionId = `live:project:${sha256(Buffer.from(projectVersionSeed))}`; | |
| const cursor: OrchestratorContextSnapshotCursor = { | |
| lanePackKey, | |
| lanePackVersionId: laneVersionId, | |
| lanePackVersionNumber: null, | |
| projectPackKey: "project", | |
| projectPackVersionId: projectVersionId, | |
| projectPackVersionNumber: null, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/orchestrator/orchestratorService.ts` around
lines 2933 - 2941, projectPackVersionId is computed from projectExport.content
(projectVersionId) which is empty by default, causing identical version IDs
across different context compositions; update the code that builds
projectPackVersionId (and the OrchestratorContextSnapshotCursor assignment) to
use a deterministic serialization of the effective project context instead of
raw projectExport.content — e.g., compute sha256 over a serialized composition
payload (projectExport plus any active lane/context metadata or a generated
snapshot object) or include a stable changing field (timestamp/nonce) in the
hashed input so different compositions produce distinct projectVersionId values;
ensure the change touches the projectVersionId computation and any code that
consumes projectPackVersionId (reference symbols: projectVersionId,
projectExport, projectPackVersionId, OrchestratorContextSnapshotCursor,
laneExport, lanePackKey).
Comment on lines
+999
to
+1027
| <div className="relative"> | ||
| {/* Ghost suggestion overlay */} | ||
| {promptSuggestion && !draft.length && !turnActive ? ( | ||
| <div | ||
| className="pointer-events-none absolute inset-0 flex items-start px-4 py-3" | ||
| aria-hidden="true" | ||
| > | ||
| <span className="text-[13px] leading-[1.6] text-fg/18 italic"> | ||
| {promptSuggestion} | ||
| <span className="ml-2 inline-flex items-center rounded border border-white/[0.06] bg-white/[0.03] px-1 py-px font-mono text-[9px] not-italic text-fg/20"> | ||
| Tab | ||
| </span> | ||
| </span> | ||
| </div> | ||
| ) : null} | ||
| <textarea | ||
| ref={textareaRef} | ||
| value={draft} | ||
| onChange={(event) => { | ||
| const val = event.target.value; | ||
| onDraftChange(val); | ||
| if (slashPickerOpen && !val.startsWith("/")) { setSlashPickerOpen(false); setSlashQuery(""); } | ||
| if (val.startsWith("/")) { setSlashQuery(val.slice(1)); } | ||
| }} | ||
| className={cn( | ||
| "min-h-[40px] max-h-[160px] w-full resize-none bg-transparent px-4 py-3 text-[13px] leading-[1.6] text-fg/88 outline-none transition-colors placeholder:text-muted-fg/25", | ||
| dragActive ? "opacity-30" : "", | ||
| )} | ||
| placeholder={turnActive ? "Steer the active turn..." : (promptSuggestion ? "" : (messagePlaceholder ?? "Message the agent..."))} |
There was a problem hiding this comment.
Expose the prompt suggestion to assistive tech.
The ghost text is aria-hidden and the placeholder becomes empty when a suggestion is present, so screen-reader users never hear the suggestion or the Tab shortcut. Please add an accessible description for this state and keep a stable label for the textarea.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` around lines
999 - 1027, The ghost suggestion overlay is aria-hidden and the textarea
placeholder is emptied when promptSuggestion exists, so screen readers miss the
suggestion and Tab hint; fix this by rendering a visually-hidden element (e.g.,
a span with an id like suggestionHintId) containing the suggestion text plus the
"Tab" hint and reference it from the textarea via aria-describedby (while
keeping the placeholder stable instead of setting it to ""), and ensure that
promptSuggestion updates are announced (use aria-live="polite" on the hidden
suggestion element) so assistive tech hears changes; update the textarea props
(value draft, ref textareaRef, and placeholder logic that currently uses
promptSuggestion) to include aria-describedby when promptSuggestion is present
and keep existing behavior for slashPickerOpen and setSlashQuery unchanged.
Rework GitHub Actions CI into staged jobs (install, parallel typecheck/lint/test, build, validate) with explicit npm cache restore, sharded vitest runs, and a gate job to ensure all required jobs pass. Add vitest pool/fork settings and extract shared workspace test config. Stability and bug fixes across services: - agentChatService: add pendingSessionReset flag to defer/flush Claude V2 session resets mid-turn, and trim/validate suggestion text; avoid emitting subagent updates for empty task IDs. - laneService: scope state snapshot queries to project_id, pass projectId parameter, invalidate lane list cache when refreshing snapshots. - crsqliteExtension: cache resolved extension path to avoid repeated FS lookups. - hybridSearchService.test: robustly load node:sqlite DatabaseSync and guard closing when unavailable. - memoryLifecycleService.test: wrap large insert loops in transactions and increase test timeout for heavy sweeps. - kvDb.sync.test: fix FTS test branches to query MATCH for real FTS and LIKE for fallback plain table. - syncHostService.test: update expected PR/merge fields to new shape (isMergeable, mergeConflicts, behindBaseBy). - syncRemoteCommandService: validate presence of text for writeTextAtomic and return it directly. - syncService.test: mock syncHostService to prevent real WebSocket binding during tests. - chatExecutionSummary: guard optional summary, merge usage objects rather than replacing, and avoid null usage regressions. - ProvidersSection.test: adjust blocker text for claude runtime status. - SyncDevicesSection: memoize QR payload, use it in effect deps, and render pairing info using code/host/port inline elements. Overall changes improve CI throughput, test reliability, and fix multiple runtime and DB scoping bugs.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/memory/hybridSearchService.test.ts (1)
685-714:⚠️ Potential issue | 🟡 MinorMissing
skipIf(!ftsAvailable)guard.This test calls
memoryService.searchMemories()(line 707) and expects results (line 713). When FTS is unavailable,readLexicalCandidatesreturns an empty array (per the service's silent fallback), causing thetoContain(original.id)assertion to fail. Add the guard for consistency with other search-dependent tests.🔧 Suggested fix
- it("re-embeds merged content updates so new terms become searchable", async () => { + it.skipIf(!ftsAvailable)("re-embeds merged content updates so new terms become searchable", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/memory/hybridSearchService.test.ts` around lines 685 - 714, This test must be skipped when full-text search is unavailable; wrap or guard the test using skipIf(!ftsAvailable) (the same pattern used in other search tests) so it doesn't run when readLexicalCandidates will silently return empty results — add the guard at the top of the it(...) block (or before invoking memoryService.searchMemories) referencing skipIf and the ftsAvailable flag to prevent the failing assertion that expects searchMemories to return the updated memory.
♻️ Duplicate comments (5)
apps/desktop/src/main/services/lanes/laneService.ts (1)
824-844:⚠️ Potential issue | 🟡 MinorRemove or clarify
useCustomBase: custombaseBranchis stored but ignored.When
parent.lane_type === "primary"andbaseBranchis provided, the lane is created from the custom branch (stored asbaseRef), but all status and rebase operations ignore it. Line 567 incomputeLaneStatususesparent?.branch_ref ?? row.base_ref—when a parent lane exists,row.base_refis never consulted. Similarly, rebase operations useparent.branch_refor the parent's HEAD, not the custom base.This means the custom
baseRefbecomes unused storage. Either removeuseCustomBaseentirely (enforce that parented lanes always derive status from parent), or adjustcomputeLaneStatusand rebase logic to respectrow.base_refwhen it differs from the parent's branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/laneService.ts` around lines 824 - 844, The pull request currently accepts a custom baseRef in createWorktreeLane (see variables useCustomBase, requestedBaseRef and createWorktreeLane call) but other code ignores row.base_ref when a parent exists; update the behavior so custom baseRef is either removed or consistently honored: either (A) eliminate useCustomBase and always derive baseRef from parent.branch_ref (remove requestedBaseRef logic and pass parent.branch_ref into createWorktreeLane), or (B) preserve custom base support by changing computeLaneStatus and all rebase logic to prefer row.base_ref when present — specifically modify computeLaneStatus (the branch selection currently using parent?.branch_ref ?? row.base_ref) to use row.base_ref if non-empty, and update rebase routines (any function like rebaseLane / performRebase or code paths that choose parent.branch_ref or parent HEAD) to use the stored base_ref/requestedBaseRef instead of parent.branch_ref/parentHeadSha; ensure error handling still validates the requestedBaseRef with runGit before using it.apps/desktop/src/main/services/sync/syncRemoteCommandService.ts (2)
678-688:⚠️ Potential issue | 🟡 MinorRuntime summaries are still derived from capped session samples.
Line 683 (
limit: 500) and Line 729 (limit: 200) can undercount active sessions;runtime.bucket/counts may become stale when caps are hit.Also applies to: 706-747
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts` around lines 678 - 688, The runtime summaries are computed from capped session samples (sessionService.list with fixed limits), causing undercounted runtime.bucket values when caps are hit; update buildLaneListSnapshots and any other places calling sessionService.list (e.g., the occurrences around the second limit: 200) to fetch all sessions instead of using a hard cap: implement pagination (iterate sessionService.list using page/offset or cursor until no more results) or use an API that returns the full set, then recompute runtime counts from the complete session set so runtime.bucket and counts are accurate. Ensure you modify the calls to sessionService.list and any aggregation logic in buildLaneListSnapshots to consume paginated results.
213-219:⚠️ Potential issue | 🟠 MajorValidate enum-like protocol fields before casting.
Line 216, Line 217, Line 238, Line 261, Line 288, Line 407, Line 429, Line 431, and Line 456 still cast trimmed strings to union-like types without checking allowed literals. At the sync boundary, malformed client payloads can still pass parse-time and fail deeper.
🔧 Suggested direction
+function requireOneOf<T extends string>( + value: unknown, + allowed: readonly T[], + message: string, +): T { + const parsed = requireString(value, message); + if (!allowed.includes(parsed as T)) throw new Error(message); + return parsed as T; +}Then use validated locals (not raw
value.*) for fields likescope,pushMode,status,toolType,provider,mode,compareTo, etc.Also applies to: 236-243, 247-263, 285-292, 404-408, 425-432, 454-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts` around lines 213 - 219, The parsing functions (starting with parseRebaseStartArgs) currently cast trimmed strings like value.scope, value.pushMode, status, toolType, provider, mode, compareTo, etc. directly to union/enum-like types; instead, validate each trimmed local against the allowed literal set before casting and only include the property when it matches a permitted value. Update parseRebaseStartArgs and the other parse/validate helpers in this file to create validated locals (e.g., let scope = asTrimmedString(value.scope); if (scope && isAllowedScope(scope)) { result.scope = scope as AllowedScope } ), use those validated locals (not raw value.*) when building the return object, and add small helper checks (isAllowedX functions or switch/Set lookups) for each enum-like field mentioned (scope, pushMode, status, toolType, provider, mode, compareTo, etc.) so malformed client payloads are rejected at parse-time.apps/desktop/src/main/services/chat/agentChatService.ts (2)
5306-5326:⚠️ Potential issue | 🟠 Major
sendMessage()now resolves before dispatch success/failure is known.Awaiters of
sendMessage()can get a fulfilled promise even when startup/auth/thread-resume fails moments later.Suggested fix (keep sendMessage awaitable and rejectable)
const sendMessage = async (args: AgentChatSendArgs): Promise<void> => { const dispatchStartedAt = Date.now(); const prepared = prepareSendMessage(args); if (!prepared) return; @@ - void executePreparedSendMessage(prepared).catch((error) => { + try { + await executePreparedSendMessage(prepared); + } catch (error) { logger.warn("agent_chat.turn_dispatch_failed", { sessionId: prepared.sessionId, provider: prepared.managed.session.provider, error: error instanceof Error ? error.message : String(error), }); emitDispatchedSendFailure(prepared, error); - }); + throw (error instanceof Error ? error : new Error(String(error))); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 5306 - 5326, sendMessage currently fires executePreparedSendMessage with "void" so sendMessage resolves before dispatch completes; change it to return the dispatch promise so callers can await and receive rejection on failure: after prepareSendMessage succeeds, return executePreparedSendMessage(prepared).catch(error => { logger.warn("agent_chat.turn_dispatch_failed", { sessionId: prepared.sessionId, provider: prepared.managed.session.provider, error: error instanceof Error ? error.message : String(error), }); emitDispatchedSendFailure(prepared, error); throw error; }); this keeps prepareSendMessage, executePreparedSendMessage, emitDispatchedSendFailure and the existing logging but makes sendMessage awaitable/rejectable.
4729-4731:⚠️ Potential issue | 🔴 CriticalCritical warmup-cancel race can still tear down a replacement Claude session.
v2WarmupDoneresolves fromPromise.race(...), so callers can proceed beforewarmupTaskfully exits.warmupTaskstill closes/nulls sharedruntime.v2Sessionafter cancel, which can clobber a newly created session.Suggested fix (preserve session ownership + completion semantics)
- const warmupPromise = Promise.race([warmupTask, waitForCancel]); - runtime.v2WarmupDone = warmupPromise; + runtime.v2WarmupDone = warmupTask; + const warmupPromise = Promise.race([warmupTask, waitForCancel]);- if (runtime.sdkSessionId) { - runtime.v2Session = unstable_v2_resumeSession(runtime.sdkSessionId, v2Opts as any) as unknown as ClaudeV2Session; - } else { - runtime.v2Session = unstable_v2_createSession(v2Opts as any) as unknown as ClaudeV2Session; - } + const warmupSession = runtime.sdkSessionId + ? (unstable_v2_resumeSession(runtime.sdkSessionId, v2Opts as any) as unknown as ClaudeV2Session) + : (unstable_v2_createSession(v2Opts as any) as unknown as ClaudeV2Session); + runtime.v2Session = warmupSession; ... - if (runtime.v2WarmupCancelled) { - try { runtime.v2Session?.close(); } catch { /* ignore */ } - runtime.v2Session = null; + if (runtime.v2WarmupCancelled) { + try { if (runtime.v2Session === warmupSession) runtime.v2Session?.close(); } catch { /* ignore */ } + if (runtime.v2Session === warmupSession) runtime.v2Session = null; return; }#!/bin/bash # Verify the race + shared-session cleanup pattern is present. rg -n -C3 'Promise\.race\(\[warmupTask,\s*waitForCancel\]\)|runtime\.v2WarmupDone\s*=\s*warmupPromise|runtime\.v2Session\s*=\s*null' apps/desktop/src/main/services/chat/agentChatService.tsAlso applies to: 4656-4660, 4688-4691, 4724-4725
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 4729 - 4731, The current pattern sets runtime.v2WarmupDone = Promise.race([warmupTask, waitForCancel]) which lets callers continue before warmupTask has finished its cleanup and allows warmupTask to null runtime.v2Session after cancellation, possibly clobbering a new session; change the logic so v2WarmupDone only resolves when warmupTask has completed cleanup (e.g., set runtime.v2WarmupDone to warmupTask.finally(...) or a wrapper that awaits warmupTask completion) and use the raced promise only to signal cancellation to warmupTask (not to short-circuit v2WarmupDone), and modify warmupTask to avoid nulling runtime.v2Session if it no longer owns the session (check identity/ownership before setting runtime.v2Session = null) so session ownership and completion semantics are preserved for warmupTask, waitForCancel, runtime.v2WarmupDone, and runtime.v2Session.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/memory/hybridSearchService.test.ts (1)
579-616: Consider addingskipIf(!ftsAvailable)for meaningful coverage.This test validates that embedding failures fall back to lexical ranking. When FTS is unavailable, both
lexicalFixtureandfallbackFixturereturn empty arrays, making the equality assertions pass vacuously without testing the actual fallback behavior. Adding the guard would ensure this test only runs when it can meaningfully verify the fallback path.💡 Suggested improvement
- it("falls back to the shipped lexical ranking when the embedding pipeline is unavailable", async () => { + it.skipIf(!ftsAvailable)("falls back to the shipped lexical ranking when the embedding pipeline is unavailable", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/memory/hybridSearchService.test.ts` around lines 579 - 616, Add a guard so this test only runs when full-text search is available: for the spec "falls back to the shipped lexical ranking when the embedding pipeline is unavailable" call skipIf(!ftsAvailable) (or wrap the it(...) with skipIf when your test framework supports it) before executing the test body; update the test that uses createFixture, lexicalFixture, fallbackFixture and memoryService.searchMemories to early-skip if ftsAvailable is false so the equality assertions aren't vacuously true when FTS is disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/components/chat/chatExecutionSummary.ts`:
- Around line 44-55: The upsert in the subagent_progress (and similarly in the
subagent_result) branch is replacing the whole snapshot and dropping an existing
background flag; update the snapshots.set call in the handler for event.type ===
"subagent_progress" (and the subagent_result branch around the same code) to
preserve background by including background: existing?.background ??
event.background ?? existingBackgroundDefault (e.g. false) so a previously set
background: true isn't lost when merging progress/result events; locate the
snapshots variable and the snapshots.set(...) calls to apply this merge.
---
Outside diff comments:
In `@apps/desktop/src/main/services/memory/hybridSearchService.test.ts`:
- Around line 685-714: This test must be skipped when full-text search is
unavailable; wrap or guard the test using skipIf(!ftsAvailable) (the same
pattern used in other search tests) so it doesn't run when readLexicalCandidates
will silently return empty results — add the guard at the top of the it(...)
block (or before invoking memoryService.searchMemories) referencing skipIf and
the ftsAvailable flag to prevent the failing assertion that expects
searchMemories to return the updated memory.
---
Duplicate comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 5306-5326: sendMessage currently fires executePreparedSendMessage
with "void" so sendMessage resolves before dispatch completes; change it to
return the dispatch promise so callers can await and receive rejection on
failure: after prepareSendMessage succeeds, return
executePreparedSendMessage(prepared).catch(error => {
logger.warn("agent_chat.turn_dispatch_failed", { sessionId: prepared.sessionId,
provider: prepared.managed.session.provider, error: error instanceof Error ?
error.message : String(error), }); emitDispatchedSendFailure(prepared, error);
throw error; }); this keeps prepareSendMessage, executePreparedSendMessage,
emitDispatchedSendFailure and the existing logging but makes sendMessage
awaitable/rejectable.
- Around line 4729-4731: The current pattern sets runtime.v2WarmupDone =
Promise.race([warmupTask, waitForCancel]) which lets callers continue before
warmupTask has finished its cleanup and allows warmupTask to null
runtime.v2Session after cancellation, possibly clobbering a new session; change
the logic so v2WarmupDone only resolves when warmupTask has completed cleanup
(e.g., set runtime.v2WarmupDone to warmupTask.finally(...) or a wrapper that
awaits warmupTask completion) and use the raced promise only to signal
cancellation to warmupTask (not to short-circuit v2WarmupDone), and modify
warmupTask to avoid nulling runtime.v2Session if it no longer owns the session
(check identity/ownership before setting runtime.v2Session = null) so session
ownership and completion semantics are preserved for warmupTask, waitForCancel,
runtime.v2WarmupDone, and runtime.v2Session.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 824-844: The pull request currently accepts a custom baseRef in
createWorktreeLane (see variables useCustomBase, requestedBaseRef and
createWorktreeLane call) but other code ignores row.base_ref when a parent
exists; update the behavior so custom baseRef is either removed or consistently
honored: either (A) eliminate useCustomBase and always derive baseRef from
parent.branch_ref (remove requestedBaseRef logic and pass parent.branch_ref into
createWorktreeLane), or (B) preserve custom base support by changing
computeLaneStatus and all rebase logic to prefer row.base_ref when present —
specifically modify computeLaneStatus (the branch selection currently using
parent?.branch_ref ?? row.base_ref) to use row.base_ref if non-empty, and update
rebase routines (any function like rebaseLane / performRebase or code paths that
choose parent.branch_ref or parent HEAD) to use the stored
base_ref/requestedBaseRef instead of parent.branch_ref/parentHeadSha; ensure
error handling still validates the requestedBaseRef with runGit before using it.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts`:
- Around line 678-688: The runtime summaries are computed from capped session
samples (sessionService.list with fixed limits), causing undercounted
runtime.bucket values when caps are hit; update buildLaneListSnapshots and any
other places calling sessionService.list (e.g., the occurrences around the
second limit: 200) to fetch all sessions instead of using a hard cap: implement
pagination (iterate sessionService.list using page/offset or cursor until no
more results) or use an API that returns the full set, then recompute runtime
counts from the complete session set so runtime.bucket and counts are accurate.
Ensure you modify the calls to sessionService.list and any aggregation logic in
buildLaneListSnapshots to consume paginated results.
- Around line 213-219: The parsing functions (starting with
parseRebaseStartArgs) currently cast trimmed strings like value.scope,
value.pushMode, status, toolType, provider, mode, compareTo, etc. directly to
union/enum-like types; instead, validate each trimmed local against the allowed
literal set before casting and only include the property when it matches a
permitted value. Update parseRebaseStartArgs and the other parse/validate
helpers in this file to create validated locals (e.g., let scope =
asTrimmedString(value.scope); if (scope && isAllowedScope(scope)) { result.scope
= scope as AllowedScope } ), use those validated locals (not raw value.*) when
building the return object, and add small helper checks (isAllowedX functions or
switch/Set lookups) for each enum-like field mentioned (scope, pushMode, status,
toolType, provider, mode, compareTo, etc.) so malformed client payloads are
rejected at parse-time.
---
Nitpick comments:
In `@apps/desktop/src/main/services/memory/hybridSearchService.test.ts`:
- Around line 579-616: Add a guard so this test only runs when full-text search
is available: for the spec "falls back to the shipped lexical ranking when the
embedding pipeline is unavailable" call skipIf(!ftsAvailable) (or wrap the
it(...) with skipIf when your test framework supports it) before executing the
test body; update the test that uses createFixture, lexicalFixture,
fallbackFixture and memoryService.searchMemories to early-skip if ftsAvailable
is false so the equality assertions aren't vacuously true when FTS is disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aeec565a-fa62-4462-9e56-953a27e19392
⛔ Files ignored due to path filters (3)
.github/workflows/ci.ymlis excluded by none and included by noneapps/desktop/vitest.config.tsis excluded by none and included by noneapps/desktop/vitest.workspace.tsis excluded by none and included by none
📒 Files selected for processing (12)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/memory/hybridSearchService.test.tsapps/desktop/src/main/services/memory/memoryLifecycleService.test.tsapps/desktop/src/main/services/state/crsqliteExtension.tsapps/desktop/src/main/services/state/kvDb.sync.test.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/services/sync/syncService.test.tsapps/desktop/src/renderer/components/chat/chatExecutionSummary.tsapps/desktop/src/renderer/components/settings/ProvidersSection.test.tsxapps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/main/services/state/kvDb.sync.test.ts
- apps/desktop/src/main/services/state/crsqliteExtension.ts
- apps/desktop/src/main/services/sync/syncHostService.test.ts
- apps/desktop/src/main/services/sync/syncService.test.ts
Comment on lines
+44
to
+55
| if (event.type === "subagent_progress") { | ||
| const existing = snapshots.get(event.taskId); | ||
| snapshots.set(event.taskId, { | ||
| taskId: event.taskId, | ||
| description: event.description?.trim() || existing?.description || "Subagent task", | ||
| status: "running", | ||
| startedAt: existing?.startedAt ?? envelope.timestamp, | ||
| updatedAt: envelope.timestamp, | ||
| summary: event.summary?.trim() || existing?.summary || null, | ||
| lastToolName: event.lastToolName ?? existing?.lastToolName, | ||
| usage: event.usage ? { ...(existing?.usage ?? {}), ...event.usage } : existing?.usage, | ||
| }); |
There was a problem hiding this comment.
Preserve background on progress/result upserts.
Both branches replace the full snapshot object but omit background, so a previously set background: true is lost after the first subagent_progress/subagent_result event.
🔧 Proposed fix
if (event.type === "subagent_progress") {
const existing = snapshots.get(event.taskId);
snapshots.set(event.taskId, {
taskId: event.taskId,
description: event.description?.trim() || existing?.description || "Subagent task",
status: "running",
startedAt: existing?.startedAt ?? envelope.timestamp,
updatedAt: envelope.timestamp,
summary: event.summary?.trim() || existing?.summary || null,
lastToolName: event.lastToolName ?? existing?.lastToolName,
+ background: existing?.background ?? false,
usage: event.usage ? { ...(existing?.usage ?? {}), ...event.usage } : existing?.usage,
});
continue;
}
if (event.type === "subagent_result") {
const existing = snapshots.get(event.taskId);
snapshots.set(event.taskId, {
taskId: event.taskId,
description: existing?.description ?? "Subagent task",
status: event.status,
startedAt: existing?.startedAt ?? envelope.timestamp,
updatedAt: envelope.timestamp,
summary: event.summary?.trim() || existing?.summary || null,
lastToolName: existing?.lastToolName,
+ background: existing?.background ?? false,
usage: event.usage ? { ...(existing?.usage ?? {}), ...event.usage } : existing?.usage,
});
}Based on learnings: Mission chat uses structured message records and runtime IDs; worker and orchestrator channels reuse shared chat message renderer patterns.
Also applies to: 67-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/chat/chatExecutionSummary.ts` around
lines 44 - 55, The upsert in the subagent_progress (and similarly in the
subagent_result) branch is replacing the whole snapshot and dropping an existing
background flag; update the snapshots.set call in the handler for event.type ===
"subagent_progress" (and the subagent_result branch around the same code) to
preserve background by including background: existing?.background ??
event.background ?? existingBackgroundDefault (e.g. false) so a previously set
background: true isn't lost when merging progress/result events; locate the
snapshots variable and the snapshots.set(...) calls to apply this merge.
Several cross-cutting changes: - CI: Make cache key Linux-specific (CACHE_KEY -> nm-linux-...) to stabilize caching behavior. - ctoOperatorTools: Add trimming and non-empty validations to many zod input schemas; rename workspace list var and prefer active workspaces before falling back to archived when resolving lane workspace. - Tests: Increase timeouts for two syncService tests to 30s to reduce flaky failures. - AgentChatComposer: Reset slash picker cursor when opening slash-based suggestions. - SyncDevicesSection: Remove conditional "Disconnect this desktop" button (no longer rendered in this branch). - vitest.workspace.ts: Simplify workspace to only run unit tests (remove integration and component test groups and the unit exclude entry). - HomePage copy: Update macOS download copy to reference the latest macOS release. These changes improve validation, reduce flakes, simplify test setup, and adjust UI/copy and CI caching.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts (1)
583-583:⚠️ Potential issue | 🟡 MinorInconsistent input validation on
sessionId.The
getChatTranscripttool usesz.string()forsessionId, while other chat tools (sendChatMessage,interruptChat,resumeChat,endChat,getChatStatus) usez.string().trim().min(1). Apply the same validation for consistency and to catch empty/whitespace-only session IDs early.🔧 Proposed fix
tools.getChatTranscript = tool({ description: "Read recent user and assistant turns for an ADE chat session without focusing the UI.", inputSchema: z.object({ - sessionId: z.string(), + sessionId: z.string().trim().min(1), limit: z.number().int().positive().max(100).optional().default(20), maxChars: z.number().int().positive().max(40000).optional().default(8000), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts` at line 583, Update the input schema for the getChatTranscript tool so its sessionId uses the same validation as the other chat tools: replace the current z.string() for sessionId with z.string().trim().min(1) to reject empty or whitespace-only IDs; locate the getChatTranscript tool definition in ctoOperatorTools.ts and change the sessionId field to match sendChatMessage, interruptChat, resumeChat, endChat, and getChatStatus.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (1)
999-1030:⚠️ Potential issue | 🟡 MinorAccessibility regression still present for prompt suggestions.
aria-hiddenon the ghost suggestion plus empty placeholder means assistive tech still misses the suggestion and the “Tab” affordance. This appears to be the same unresolved issue from earlier review feedback.Suggested fix
<div className="relative"> {/* Ghost suggestion overlay */} {promptSuggestion && !draft.length && !turnActive ? ( - <div - className="pointer-events-none absolute inset-0 flex items-start px-4 py-3" - aria-hidden="true" - > - <span className="text-[13px] leading-[1.6] text-fg/18 italic"> - {promptSuggestion} - <span className="ml-2 inline-flex items-center rounded border border-white/[0.06] bg-white/[0.03] px-1 py-px font-mono text-[9px] not-italic text-fg/20"> - Tab - </span> - </span> - </div> + <> + <div + className="pointer-events-none absolute inset-0 flex items-start px-4 py-3" + aria-hidden="true" + > + <span className="text-[13px] leading-[1.6] text-fg/18 italic"> + {promptSuggestion} + <span className="ml-2 inline-flex items-center rounded border border-white/[0.06] bg-white/[0.03] px-1 py-px font-mono text-[9px] not-italic text-fg/20"> + Tab + </span> + </span> + </div> + <span id="agent-chat-suggestion-hint" className="sr-only" aria-live="polite"> + Suggested prompt available. Press Tab to accept: {promptSuggestion} + </span> + </> ) : null} <textarea ref={textareaRef} value={draft} @@ - placeholder={turnActive ? "Steer the active turn..." : (promptSuggestion ? "" : (messagePlaceholder ?? "Message the agent..."))} + placeholder={turnActive ? "Steer the active turn..." : (messagePlaceholder ?? "Message the agent...")} + aria-describedby={promptSuggestion && !draft.length && !turnActive ? "agent-chat-suggestion-hint" : undefined} onKeyDown={handleKeyDown} onPaste={handlePaste} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` around lines 999 - 1030, The ghost suggestion overlay is marked aria-hidden so assistive tech can't read the promptSuggestion or the "Tab" affordance; fix by adding a screen-reader-only element that contains the suggestion text and affordance and reference it from the textarea via aria-describedby (or make the overlay not aria-hidden and give it role="status" + aria-live="polite"); update the JSX around the prompt suggestion overlay (the div that renders when promptSuggestion && !draft.length && !turnActive) and the textarea element (reflected by textareaRef, draft, turnActive) so the textarea includes aria-describedby pointing to the new SR-only id (or the overlay has role/aria-live) ensuring screen readers announce the suggestion while keeping the visual overlay unchanged.
🧹 Nitpick comments (6)
apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts (6)
68-69: Consider makingresumeChatanddisposeChatoptional.Unlike
prService,fileService, andprocessServicewhich are optional,resumeChatanddisposeChatare required in the interface. However, the tools using them (tools.resumeChat,tools.endChat) don't check for their availability before calling. If these dependencies might not always be provided, consider making them optional and adding availability checks in the tool execute functions, similar to the pattern used for other optional services.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts` around lines 68 - 69, The interface currently requires resumeChat and disposeChat but other services (prService, fileService, processService) are optional; update the declaration of resumeChat and disposeChat to be optional (e.g., resumeChat?: ..., disposeChat?: ...) and then update the tool execute logic that calls tools.resumeChat and tools.endChat to guard those calls with availability checks (if (!tools.resumeChat) return or throw/handle accordingly) following the same pattern used for prService/fileService/processService so callers don't assume those methods always exist.
1276-1276: Consider adding input validation to Linear tool schemas for consistency.The Linear workflow tools (
getLinearRunStatus,resolveLinearRunAction,cancelLinearRun,commentOnLinearIssue,updateLinearIssueState, etc.) use plainz.string()for identifiers likerunIdandissueId, while the newly added PR and chat tools usez.string().trim().min(1). Consider applying the same validation pattern to Linear tools for consistency across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts` at line 1276, Update the Linear tool input schemas to validate identifier strings the same way other tools do: replace plain z.string() for fields like runId and issueId in the Linear-related schemas (e.g., getLinearRunStatus, resolveLinearRunAction, cancelLinearRun, commentOnLinearIssue, updateLinearIssueState) with z.string().trim().min(1) so empty/whitespace-only IDs are rejected consistently across the codebase.
1085-1085: Consider trimmingbodyinput for consistency.The
updatePullRequestBodytool'sbodyfield usesz.string().min(1)without.trim(), which allows whitespace-only content. This differs fromupdatePullRequestTitlewhich uses.trim().min(1). If whitespace-only bodies should be rejected, add.trim()for consistency.💡 Proposed fix if whitespace-only should be rejected
tools.updatePullRequestBody = tool({ description: "Update a pull request body through ADE's PR service.", inputSchema: z.object({ prId: z.string().trim().min(1), - body: z.string().min(1), + body: z.string().trim().min(1), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts` at line 1085, The schema for the updatePullRequestBody tool currently uses body: z.string().min(1) which allows whitespace-only values; update the Zod schema for updatePullRequestBody to match updatePullRequestTitle by applying .trim() (i.e., body: z.string().trim().min(1)) so whitespace-only bodies are rejected and input handling is consistent with the updatePullRequestTitle validation.
381-381: Consider adding.trim().min(1)tolaneIdschema.The
inspectLanetool trimslaneIdin the execute function (line 385) but doesn't validate it at the schema level. Adding.trim().min(1)to the schema would provide clearer, earlier error messages consistent with other tools.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts` at line 381, The laneId input schema currently uses z.string() but the execute function in inspectLane trims laneId; update the tool's input Zod schema by replacing the laneId definition with a trimmed, non-empty string (e.g., z.string().trim().min(1)) so validation fails early and matches the runtime behavior in inspectLane; ensure any callers or type usages of the schema still accept the same string type and you can keep the runtime trim in inspectLane or remove it if you prefer relying on schema normalization.
1172-1172: Consider adding.trim().min(1)to optionallaneIdfor consistency.The
listManagedProcessestool usesz.string().optional()forlaneId, whilestartManagedProcess,stopManagedProcess, andgetManagedProcessLogusez.string().trim().min(1).optional(). While the execute function handles empty strings via fallback, applying consistent validation at the schema level is cleaner.💡 Proposed fix
tools.listManagedProcesses = tool({ description: "Inspect ADE-managed processes for a lane, including configured definitions and current runtime state.", inputSchema: z.object({ - laneId: z.string().optional(), + laneId: z.string().trim().min(1).optional(), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts` at line 1172, The optional laneId schema in listManagedProcesses is looser than the others; update the zod schema for laneId in the listManagedProcesses tool to use z.string().trim().min(1).optional() so it matches startManagedProcess, stopManagedProcess, and getManagedProcessLog; locate the laneId declaration inside the listManagedProcesses tool (same symbol name) and replace z.string().optional() with z.string().trim().min(1).optional() to keep validation consistent.
1178-1181: UnnecessaryPromise.resolve()wrapping.
listDefinitions()andlistRuntime()appear to be synchronous methods based on how they're called. Wrapping them inPromise.resolve()is unnecessary sincePromise.all()can accept non-promise values.💡 Simplified code
try { - const [definitions, runtime] = await Promise.all([ - Promise.resolve(deps.processService.listDefinitions()), - Promise.resolve(deps.processService.listRuntime(resolvedLaneId)), - ]); + const definitions = deps.processService.listDefinitions(); + const runtime = deps.processService.listRuntime(resolvedLaneId); return { success: true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts` around lines 1178 - 1181, The Promise.resolve(...) wrappers around deps.processService.listDefinitions() and deps.processService.listRuntime(resolvedLaneId) are unnecessary; remove the Promise.resolve calls and pass the raw results into Promise.all (i.e., use Promise.all([deps.processService.listDefinitions(), deps.processService.listRuntime(resolvedLaneId)]) ) so listDefinitions and listRuntime are awaited correctly without extra wrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts`:
- Line 583: Update the input schema for the getChatTranscript tool so its
sessionId uses the same validation as the other chat tools: replace the current
z.string() for sessionId with z.string().trim().min(1) to reject empty or
whitespace-only IDs; locate the getChatTranscript tool definition in
ctoOperatorTools.ts and change the sessionId field to match sendChatMessage,
interruptChat, resumeChat, endChat, and getChatStatus.
---
Duplicate comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 999-1030: The ghost suggestion overlay is marked aria-hidden so
assistive tech can't read the promptSuggestion or the "Tab" affordance; fix by
adding a screen-reader-only element that contains the suggestion text and
affordance and reference it from the textarea via aria-describedby (or make the
overlay not aria-hidden and give it role="status" + aria-live="polite"); update
the JSX around the prompt suggestion overlay (the div that renders when
promptSuggestion && !draft.length && !turnActive) and the textarea element
(reflected by textareaRef, draft, turnActive) so the textarea includes
aria-describedby pointing to the new SR-only id (or the overlay has
role/aria-live) ensuring screen readers announce the suggestion while keeping
the visual overlay unchanged.
---
Nitpick comments:
In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts`:
- Around line 68-69: The interface currently requires resumeChat and disposeChat
but other services (prService, fileService, processService) are optional; update
the declaration of resumeChat and disposeChat to be optional (e.g., resumeChat?:
..., disposeChat?: ...) and then update the tool execute logic that calls
tools.resumeChat and tools.endChat to guard those calls with availability checks
(if (!tools.resumeChat) return or throw/handle accordingly) following the same
pattern used for prService/fileService/processService so callers don't assume
those methods always exist.
- Line 1276: Update the Linear tool input schemas to validate identifier strings
the same way other tools do: replace plain z.string() for fields like runId and
issueId in the Linear-related schemas (e.g., getLinearRunStatus,
resolveLinearRunAction, cancelLinearRun, commentOnLinearIssue,
updateLinearIssueState) with z.string().trim().min(1) so empty/whitespace-only
IDs are rejected consistently across the codebase.
- Line 1085: The schema for the updatePullRequestBody tool currently uses body:
z.string().min(1) which allows whitespace-only values; update the Zod schema for
updatePullRequestBody to match updatePullRequestTitle by applying .trim() (i.e.,
body: z.string().trim().min(1)) so whitespace-only bodies are rejected and input
handling is consistent with the updatePullRequestTitle validation.
- Line 381: The laneId input schema currently uses z.string() but the execute
function in inspectLane trims laneId; update the tool's input Zod schema by
replacing the laneId definition with a trimmed, non-empty string (e.g.,
z.string().trim().min(1)) so validation fails early and matches the runtime
behavior in inspectLane; ensure any callers or type usages of the schema still
accept the same string type and you can keep the runtime trim in inspectLane or
remove it if you prefer relying on schema normalization.
- Line 1172: The optional laneId schema in listManagedProcesses is looser than
the others; update the zod schema for laneId in the listManagedProcesses tool to
use z.string().trim().min(1).optional() so it matches startManagedProcess,
stopManagedProcess, and getManagedProcessLog; locate the laneId declaration
inside the listManagedProcesses tool (same symbol name) and replace
z.string().optional() with z.string().trim().min(1).optional() to keep
validation consistent.
- Around line 1178-1181: The Promise.resolve(...) wrappers around
deps.processService.listDefinitions() and
deps.processService.listRuntime(resolvedLaneId) are unnecessary; remove the
Promise.resolve calls and pass the raw results into Promise.all (i.e., use
Promise.all([deps.processService.listDefinitions(),
deps.processService.listRuntime(resolvedLaneId)]) ) so listDefinitions and
listRuntime are awaited correctly without extra wrapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 431e07a6-2754-4917-8fa0-3e47a7c279a4
⛔ Files ignored due to path filters (2)
.github/workflows/ci.ymlis excluded by none and included by noneapps/desktop/vitest.workspace.tsis excluded by none and included by none
📒 Files selected for processing (5)
apps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/sync/syncService.test.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/settings/SyncDevicesSection.tsxapps/web/src/app/pages/HomePage.tsx
Drop legacy "context pack" functionality across the MCP server and update tests accordingly. Removed the read_context tool, pack-related resource entries, pack MIME constant, pack handling in spawn context resolution (packs removed, approxTokens set to 0), and helper functions for listing packs/features/missions. Tests and fixtures were updated: packService and packsDir were removed from the test runtime, several pack-related tests/expectations were deleted or changed, and syncService tests are skipped when crsqlite is unavailable. Overall effect: resources list and MCP handler no longer expose pack URIs; users should use the unified memory system instead of context packs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/sync/syncService.test.ts (1)
9-27: Mock implementation is comprehensive and correctly aligned with the real service.The stub covers all 10 methods exposed by
createSyncHostServiceper the implementation insyncHostService.ts.One minor observation:
getPairingSession()generates a freshexpiresAttimestamp on each call (line 17). If the sync service implementation calls this method multiple times internally, successive calls could return slightly different timestamps, potentially causing flaky assertions on line 355 (expect(parsedPayload.expiresAt).toBe(status.pairingSession?.expiresAt)). Consider caching the expiry timestamp if you encounter intermittent failures:💡 Optional: Cache the expiry timestamp
vi.mock("./syncHostService", () => ({ createSyncHostService: () => { + const expires = new Date(Date.now() + 600_000).toISOString(); return { async waitUntilListening() { return 8787; }, getPort() { return 8787; }, getBootstrapToken() { return "test-bootstrap-token"; }, getPairingSession() { - const expires = new Date(Date.now() + 600_000).toISOString(); return { code: "TEST1234", expiresAt: expires, pairedDevices: [] }; }, // ...rest }; }, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncService.test.ts` around lines 9 - 27, The mock's getPairingSession in createSyncHostService returns a new expiresAt each call which can make tests flaky; change the mock to compute and store a single expiresAt value (e.g., const expires = new Date(...).toISOString() assigned once in the mock scope) and have getPairingSession return that cached expiresAt so repeated calls (and the assertion comparing parsedPayload.expiresAt to status.pairingSession?.expiresAt) see the same timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/services/sync/syncService.test.ts`:
- Around line 9-27: The mock's getPairingSession in createSyncHostService
returns a new expiresAt each call which can make tests flaky; change the mock to
compute and store a single expiresAt value (e.g., const expires = new
Date(...).toISOString() assigned once in the mock scope) and have
getPairingSession return that cached expiresAt so repeated calls (and the
assertion comparing parsedPayload.expiresAt to status.pairingSession?.expiresAt)
see the same timestamp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6e117aa5-4c2e-4b92-a31a-f0fe44bdf4e6
⛔ Files ignored due to path filters (1)
.github/workflows/ci.ymlis excluded by none and included by none
📒 Files selected for processing (4)
apps/desktop/src/main/services/orchestrator/orchestratorService.test.tsapps/desktop/src/main/services/sync/syncService.test.tsapps/mcp-server/src/mcpServer.test.tsapps/mcp-server/src/mcpServer.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/services/orchestrator/orchestratorService.test.ts
This was referenced Mar 23, 2026
Merged
Merged
Merged
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
symphony_orchestrator.ex(no license), removed internalplan.mdscratch doc, regenerated iOS bootstrap SQLTest plan
tsc --noEmit)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Removals