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.
|
📝 WalkthroughWalkthroughThis pull request introduces comprehensive enhancements across desktop app startup, authentication handling, service architecture, and UI components. Changes include reorganized service dependency injection, expanded Claude runtime error detection, new snapshot/state management APIs, refactored sync pairing with LAN/Tailscale support, simplified onboarding flow, and enhanced operator tools with additional context/process/PR capabilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
|
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
+2662
to
+2685
| // system:task_progress — running subagent summary/usage | ||
| if (msg.type === "system" && (msg as any).subtype === "task_progress") { | ||
| const taskMsg = msg as any; | ||
| const taskId = String(taskMsg.task_id ?? ""); | ||
| const existing = runtime.activeSubagents.get(taskId); | ||
| const description = String(taskMsg.description ?? existing?.description ?? ""); | ||
| if (taskId.length) { | ||
| runtime.activeSubagents.set(taskId, { taskId, description }); | ||
| } | ||
| emitChatEvent(managed, { | ||
| type: "subagent_progress", | ||
| taskId, | ||
| description, | ||
| summary: String(taskMsg.summary ?? ""), | ||
| usage: taskMsg.usage ? { | ||
| totalTokens: typeof taskMsg.usage.total_tokens === "number" ? taskMsg.usage.total_tokens : undefined, | ||
| toolUses: typeof taskMsg.usage.tool_uses === "number" ? taskMsg.usage.tool_uses : undefined, | ||
| durationMs: typeof taskMsg.usage.duration_ms === "number" ? taskMsg.usage.duration_ms : undefined, | ||
| } : undefined, | ||
| lastToolName: typeof taskMsg.last_tool_name === "string" ? taskMsg.last_tool_name : undefined, | ||
| turnId, | ||
| }); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Drop subagent_progress updates that have no task ID.
trackSubagentEvent() keys snapshots by event.taskId, so emitting this with taskId === "" creates a phantom subagent entry and lets unrelated progress updates overwrite each other. Bail out here when the SDK omits task_id, or synthesize a stable ID before emitting.
🤖 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 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.
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
+5724
to
5729
| if (prev !== next && managed.runtime?.kind === "claude" && (managed.runtime.v2Session || managed.runtime.v2WarmupDone)) { | ||
| cancelClaudeWarmup(managed, managed.runtime, "session_reset"); | ||
| try { managed.runtime.v2Session?.close(); } catch { /* ignore */ } | ||
| managed.runtime.v2Session = null; | ||
| managed.runtime.v2WarmupDone = null; | ||
| } |
There was a problem hiding this comment.
Don't close the live Claude session on a mid-turn reasoning change.
v2Session is the active session as well as the warmed one. If this branch runs during an in-flight Claude turn, it closes the session out from under the stream and forces the current turn down the failure/interrupted path. Defer the reset until the runtime is idle.
💡 Minimal guard
- if (prev !== next && managed.runtime?.kind === "claude" && (managed.runtime.v2Session || managed.runtime.v2WarmupDone)) {
+ if (
+ prev !== next
+ && managed.runtime?.kind === "claude"
+ && !managed.runtime.busy
+ && (managed.runtime.v2Session || managed.runtime.v2WarmupDone)
+ ) {
cancelClaudeWarmup(managed, managed.runtime, "session_reset");
try { managed.runtime.v2Session?.close(); } catch { /* ignore */ }
managed.runtime.v2Session = null;
managed.runtime.v2WarmupDone = null;
}🤖 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 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).
Comment on lines
+744
to
+775
| getStateSnapshot(laneId: string): LaneStateSnapshotSummary | null { | ||
| const row = db.get<LaneStateSnapshotRow>( | ||
| ` | ||
| select lane_id, agent_summary_json, mission_summary_json, updated_at | ||
| from lane_state_snapshots | ||
| where lane_id = ? | ||
| limit 1 | ||
| `, | ||
| [laneId], | ||
| ); | ||
| if (!row) return null; | ||
| return { | ||
| laneId: row.lane_id, | ||
| agentSummary: parseSummaryRecord(row.agent_summary_json), | ||
| missionSummary: parseSummaryRecord(row.mission_summary_json), | ||
| updatedAt: row.updated_at ?? null, | ||
| }; | ||
| }, | ||
|
|
||
| listStateSnapshots(): LaneStateSnapshotSummary[] { | ||
| return db.all<LaneStateSnapshotRow>( | ||
| ` | ||
| select lane_id, agent_summary_json, mission_summary_json, updated_at | ||
| from lane_state_snapshots | ||
| `, | ||
| ).map((row) => ({ | ||
| laneId: row.lane_id, | ||
| agentSummary: parseSummaryRecord(row.agent_summary_json), | ||
| missionSummary: parseSummaryRecord(row.mission_summary_json), | ||
| updatedAt: row.updated_at ?? null, | ||
| })); | ||
| }, |
There was a problem hiding this comment.
Scope snapshot reads to projectId.
createLaneService is project-scoped, but these new queries read lane_state_snapshots without joining lanes on project_id. In a shared DB, that will surface other projects' snapshots and orphaned rows for deleted lanes.
🔒 Suggested scoping fix
getStateSnapshot(laneId: string): LaneStateSnapshotSummary | null {
const row = db.get<LaneStateSnapshotRow>(
`
- select lane_id, agent_summary_json, mission_summary_json, updated_at
- from lane_state_snapshots
- where lane_id = ?
+ select s.lane_id, s.agent_summary_json, s.mission_summary_json, s.updated_at
+ from lane_state_snapshots s
+ join lanes l on l.id = s.lane_id
+ where s.lane_id = ?
+ and l.project_id = ?
limit 1
`,
- [laneId],
+ [laneId, projectId],
);
if (!row) return null;
return {
laneId: row.lane_id,
agentSummary: parseSummaryRecord(row.agent_summary_json),
@@
listStateSnapshots(): LaneStateSnapshotSummary[] {
return db.all<LaneStateSnapshotRow>(
`
- select lane_id, agent_summary_json, mission_summary_json, updated_at
- from lane_state_snapshots
+ select s.lane_id, s.agent_summary_json, s.mission_summary_json, s.updated_at
+ from lane_state_snapshots s
+ join lanes l on l.id = s.lane_id
+ where l.project_id = ?
`,
+ [projectId],
).map((row) => ({
laneId: row.lane_id,
agentSummary: parseSummaryRecord(row.agent_summary_json),
missionSummary: parseSummaryRecord(row.mission_summary_json),
updatedAt: row.updated_at ?? null,📝 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
| getStateSnapshot(laneId: string): LaneStateSnapshotSummary | null { | |
| const row = db.get<LaneStateSnapshotRow>( | |
| ` | |
| select lane_id, agent_summary_json, mission_summary_json, updated_at | |
| from lane_state_snapshots | |
| where lane_id = ? | |
| limit 1 | |
| `, | |
| [laneId], | |
| ); | |
| if (!row) return null; | |
| return { | |
| laneId: row.lane_id, | |
| agentSummary: parseSummaryRecord(row.agent_summary_json), | |
| missionSummary: parseSummaryRecord(row.mission_summary_json), | |
| updatedAt: row.updated_at ?? null, | |
| }; | |
| }, | |
| listStateSnapshots(): LaneStateSnapshotSummary[] { | |
| return db.all<LaneStateSnapshotRow>( | |
| ` | |
| select lane_id, agent_summary_json, mission_summary_json, updated_at | |
| from lane_state_snapshots | |
| `, | |
| ).map((row) => ({ | |
| laneId: row.lane_id, | |
| agentSummary: parseSummaryRecord(row.agent_summary_json), | |
| missionSummary: parseSummaryRecord(row.mission_summary_json), | |
| updatedAt: row.updated_at ?? null, | |
| })); | |
| }, | |
| getStateSnapshot(laneId: string): LaneStateSnapshotSummary | null { | |
| const row = db.get<LaneStateSnapshotRow>( | |
| ` | |
| select s.lane_id, s.agent_summary_json, s.mission_summary_json, s.updated_at | |
| from lane_state_snapshots s | |
| join lanes l on l.id = s.lane_id | |
| where s.lane_id = ? | |
| and l.project_id = ? | |
| limit 1 | |
| `, | |
| [laneId, projectId], | |
| ); | |
| if (!row) return null; | |
| return { | |
| laneId: row.lane_id, | |
| agentSummary: parseSummaryRecord(row.agent_summary_json), | |
| missionSummary: parseSummaryRecord(row.mission_summary_json), | |
| updatedAt: row.updated_at ?? null, | |
| }; | |
| }, | |
| listStateSnapshots(): LaneStateSnapshotSummary[] { | |
| return db.all<LaneStateSnapshotRow>( | |
| ` | |
| select s.lane_id, s.agent_summary_json, s.mission_summary_json, s.updated_at | |
| from lane_state_snapshots s | |
| join lanes l on l.id = s.lane_id | |
| where l.project_id = ? | |
| `, | |
| [projectId], | |
| ).map((row) => ({ | |
| laneId: row.lane_id, | |
| agentSummary: parseSummaryRecord(row.agent_summary_json), | |
| missionSummary: parseSummaryRecord(row.mission_summary_json), | |
| updatedAt: row.updated_at ?? null, | |
| })); | |
| }, |
🤖 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 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.
Comment on lines
+328
to
+333
| function parseWriteTextAtomicArgs(value: Record<string, unknown>): WriteTextAtomicArgs { | ||
| return { | ||
| laneId: requireString(value.laneId, "files.writeTextAtomic requires laneId."), | ||
| path: requireString(value.path, "files.writeTextAtomic requires path."), | ||
| text: typeof value.text === "string" ? value.text : "", | ||
| }; |
There was a problem hiding this comment.
Reject missing text instead of turning it into an empty write.
At Line 332, any non-string payload becomes "", so a client bug or protocol mismatch can silently truncate the target file rather than fail validation.
🛡️ Suggested fix
function parseWriteTextAtomicArgs(value: Record<string, unknown>): WriteTextAtomicArgs {
+ if (typeof value.text !== "string") {
+ throw new Error("files.writeTextAtomic requires text.");
+ }
return {
laneId: requireString(value.laneId, "files.writeTextAtomic requires laneId."),
path: requireString(value.path, "files.writeTextAtomic requires path."),
- text: typeof value.text === "string" ? value.text : "",
+ text: value.text,
};
}📝 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
| function parseWriteTextAtomicArgs(value: Record<string, unknown>): WriteTextAtomicArgs { | |
| return { | |
| laneId: requireString(value.laneId, "files.writeTextAtomic requires laneId."), | |
| path: requireString(value.path, "files.writeTextAtomic requires path."), | |
| text: typeof value.text === "string" ? value.text : "", | |
| }; | |
| function parseWriteTextAtomicArgs(value: Record<string, unknown>): WriteTextAtomicArgs { | |
| if (typeof value.text !== "string") { | |
| throw new Error("files.writeTextAtomic requires text."); | |
| } | |
| return { | |
| laneId: requireString(value.laneId, "files.writeTextAtomic requires laneId."), | |
| path: requireString(value.path, "files.writeTextAtomic requires path."), | |
| text: value.text, | |
| }; |
🤖 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
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.
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.
Comment on lines
+42
to
+53
| 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, | ||
| }); |
There was a problem hiding this comment.
Handle partial subagent_progress payloads defensively.
Line 46 already treats description as optional, but Line 50 still calls .trim() on summary unconditionally and Line 52 replaces the entire usage object. If a progress event omits summary or only sends one usage field, this path will either throw or discard previously captured counters.
🛡️ Suggested defensive merge
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,
+ summary: event.summary?.trim() || existing?.summary || null,
lastToolName: event.lastToolName ?? existing?.lastToolName,
- usage: event.usage ?? existing?.usage,
+ usage: event.usage ? { ...(existing?.usage ?? {}), ...event.usage } : existing?.usage,
});
continue;
}🤖 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 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.
| runtimeAvailable: claudeRuntimeAvailable, | ||
| usageAvailable: claudeRuntimeAvailable, | ||
| path: "/Users/arul/.local/bin/claude", | ||
| blocker: claudeRuntimeAvailable ? null : null, |
There was a problem hiding this comment.
Minor: blocker value is the same for both branches.
The expression blocker: claudeRuntimeAvailable ? null : null always evaluates to null. While the test passes because getStatusTone derives "Sign-In Required" from other fields, setting a realistic blocker message when claudeRuntimeAvailable is false would make the mock more representative of actual behavior.
Suggested fix
- blocker: claudeRuntimeAvailable ? null : null,
+ blocker: claudeRuntimeAvailable ? null : "Authentication required",📝 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
| blocker: claudeRuntimeAvailable ? null : null, | |
| blocker: claudeRuntimeAvailable ? null : "Authentication required", |
🤖 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.test.tsx` at
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.
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.
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
Release Notes
New Features
Improvements
Documentation