Work-tab Chat Orchestrator (lead/worker/validator)#361
Merged
Conversation
Implements the spec at goal.md end-to-end. One ADE chat becomes a planner
(lead) that spawns worker and validator chats in the same lane; they
coordinate through a lane-local bundle (manifest.json + plan.md +
artifacts/) owned by a new orchestrationService.
Why: gives the user a single Work-tab surface for multi-agent
coordination without leaving ADE's chat shell — replaces ad-hoc tiling
of multiple lanes for related work.
What landed (six build steps from spec §17):
1. Types, IPC, bundle persistence
- shared/types/orchestration.ts: manifest schema, ping primitives,
model routing, validation concerns, context items.
- main/services/orchestration/{orchestrationService,patchPolicy}.ts:
per-runId AsyncMutex, etag concurrency, atomic writes, chokidar
watcher with self-write suppression + runId validation,
RFC-6902 subset patches with id-predicate paths (numeric
indices rejected), patch-path whitelist enforcement, coordinated
transaction enforcement (status=done + required gate requires
humanOverride + UserOverrideEntry).
- ipc.ts + registerIpc.ts: 13 handlers + event broadcast.
- preload.ts + global.d.ts: window.ade.orchestration.* bridge.
2. Lead chat + skill plumbing
- chat.ts: AgentChatInteractionMode union extended with
orchestrator-{lead,worker,validator}; PendingInputKind adds
model_selection; OrchestrationSessionFields intersected with
AgentChatSession/Summary.
- agentChatService.ts: createSession accepts orchestration fields,
persistChatState writes them, deserializer + resume hydrate
them, setOrchestrationFields exposed for IPC stitch-back,
public readTranscript wrapper.
- systemPrompt.ts: buildOrchestratorRoleDirective; threaded
through Codex + Droid call sites.
3. Orchestration tools (orchestrationTools.ts)
- createOrchestrationToolSet returns role-appropriate tools.
- Lead: read-only base + spawnAgent, messageAgent, manifestPatch,
planAppend/planWrite, claimTask/releaseTask, registerAsset,
getAgentTranscript, askUserForModelSelection. No bash/edit/write.
- Worker/validator: full edit+bash with buildOrchestrationSandboxConfig
applied — manifest.json/plan.md protected, blockByDefault: true.
- Bash hardening in universalTools.ts: MUTATING_BASH_RE extended
with ln (hardlink), INTERPRETER_RE for python|node|ruby|perl -c/-e,
commandUsesInterpreterPayload exported.
4. Right plan panel + chrome
- OrchestrationPanel.tsx: unified view — run header, phases
accordion, task cards (id/tag/status/files/owner/elapsed/validation
badges), plan.md narrative, empty-state Q&A history. Subscribes
via window.ade.orchestration.subscribe.
- PlanMarkdown.tsx: react-markdown + remark-gfm + rehype-raw with
local rehype-slug visitor + lazy mermaid render + inline asset
embeds + spec-preview card for artifacts/ui/*.html.
- OrchestratorLeadFrame.tsx: conic-gradient rainbow ring around
lead chat surface, prefers-reduced-motion static fallback.
- SessionCard.tsx: purple LEAD / blue WORKER · <tag> / green
VALIDATOR pills.
5. Ephemeral annotations + spec card
- AnnotationPopover.tsx + SpecPreviewCard.tsx: select text/image/
iframe → comment → CustomEvent ade:agent-chat:add-plan-annotation
→ composer attachment merge in AgentChatPane.tsx (no manifest
persistence in v1).
- openExternal.ts: file:// URLs route through built-in browser.
6. Model routing + askUserForModelSelection
- ChatModelSelectionPendingCard.tsx: renders ModelPicker for
PendingInputKind: "model_selection" with hidePermissionRail prop.
- AgentChatComposer.tsx: orchestrationRole lock prop, "+ New
orchestrator chat" entry (purple-accent), composer locks
permission rail for orchestration sessions.
- SessionListPane.tsx + TerminalsPage.tsx: "Orchestrator" pill
opens chat-orchestrator draft → agentChat.create with
interactionMode: orchestrator-lead → orchestration.runCreate.
Hardening (spec §16): per-runId AsyncMutex, watcher self-write
suppression, leadState snapshot + history ring buffer, serverGeneration
counter survives git checkout, patch-path whitelist, coordinated-
transaction override enforcement, bundle bash blocklist.
Permission profiles (spec §12): claude bypassPermissions, codex
danger-full-access + approvals=never, cursor full-auto (not "agent"),
droid auto-high, opencode full-auto.
Tests: 97 orchestration-specific tests across 14 files (service,
patchPolicy, validationConcerns, watcherResilience, modelRouting,
permissionProfile canary, bashHardening, orchestrationTools, plus
OrchestrationPanel/PlanMarkdown/OrchestratorLeadFrame/AnnotationFlow/
ModelPickerPendingInput/OrchestratorComposerEntry). 929 tests pass
across the broader chat+terminals suite — zero regressions.
Known v1 gaps (deferred): provider-runtime integration of the
orchestration tool factory (factory exists, each provider's wiring is
follow-up), §16.2 cancellation watcher SIGTERM hook (cooperative-on-
next-turn fallback works), rehype-sanitize between rehype-raw and the
slug visitor (workers can only write plan.md via planAppend; threat
model is local-only), persistent run-discovery index (chat session
record carries orchestrationRunId; lazy-load on bundleRead), preload
integration tests (unit tests use injected source prop per spec §18).
Follow-up prompt in codexGoal.md (untracked).
Skill: .agents/skills/ade-orchestrator/SKILL.md (outside repo at
/Users/arul/ADE/.agents/skills/ade-orchestrator/) — full role protocol
including planning, developing, validating, ping discipline,
cancellation, spawn brief, forbidden actions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The orchestrator skill landed in /Users/arul/ADE/.agents/skills/ during implementation — a user-local path outside the repo, so it shipped with my machine but not with the ADE build. Move it to the canonical resources/agent-skills/ tree alongside ade-cto-missions, ade-browser, ade-lanes-git, ade-pr-workflows etc. so it's auto-discovered by the agentSkillRoots resolver on every install. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…194b # Conflicts: # apps/desktop/src/main/services/files/fileWatcherService.ts # apps/desktop/src/renderer/components/chat/AgentChatPane.tsx # apps/desktop/src/renderer/components/shared/ModelPicker/ModelPicker.tsx # apps/desktop/src/renderer/components/shared/ModelPicker/ModelPickerContent.tsx # goal.md
…194b # Conflicts: # apps/desktop/src/main/main.ts # apps/desktop/src/main/services/ai/tools/universalTools.ts # apps/desktop/src/main/services/ai/tools/workerSandboxDefaults.ts # apps/desktop/src/main/services/chat/agentChatService.ts # apps/desktop/src/main/services/chat/permissionMapping.ts # apps/desktop/src/main/services/ipc/registerIpc.ts # apps/desktop/src/preload/global.d.ts # apps/desktop/src/shared/adeCliGuidance.ts # apps/desktop/src/shared/chatContextAttachments.ts # apps/desktop/src/shared/types/chat.ts
…orchestration - Resolve conflicts keeping new orchestration/ service and removing old missions/orchestrator/ systems that main already deleted - Fix import paths from deleted orchestrator/orchestratorConstants to workerSandboxDefaults - Keep both parseLaneLinearIssueValue (from main) and OrchestrationContextItem (from this branch) imports in chatContextAttachments - Preserve model_selection PendingInputKind (used by orchestration tools) - Keep orchestration MCP and Cursor SDK interrupt check (both needed) - Fix type narrowing in chatContextAttachments.test.ts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@copilot review but do not make fixes |
|
Warning Review limit reached
More reviews will be available in 50 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds orchestration service and renderer panel, wires orchestration tools via MCP across providers, updates GitHub auth with GH CLI fallback, refactors ADE CLI serve lifecycle and TUI prompt/edit flows, and updates tests/docs accordingly. ChangesEnd-to-end orchestration, auth, and runtime refactors
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Copilot stopped work on behalf of
arul28 due to an error
May 26, 2026 03:20
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PR Review
Scope: 131 file(s), +19906 / −1613
Verdict: Needs changes
This PR adds a full lead/worker/validator orchestration stack (manifest bundle, IPC, agent tools, Work-tab UI, TUI hooks) on top of chat sessions. The architecture is thorough, but a few authorization and lifecycle gaps undermine the plan-approval gate and worker spawn bookkeeping.
🔒 Security
[High] Lead can self-approve the plan via manifestPatch
File: apps/desktop/src/main/services/orchestration/patchPolicy.ts:L109-L119
Issue: Lead patch policy only blocks a small immutable-path set. /leadState/planApprovedAt, /currentPhase, and planning-phase status are not denied, so a lead (or any caller that supplies actorRole: "lead" over IPC) can mark the plan approved without the user clicking Implement.
Attack path: Lead session calls manifestPatch with planApprovedAt + phase transitions → isOrchestrationPlanApproved() becomes true → spawnAgent runs with no UI approval.
Fix: Deny lead patches to approval/phase-transition paths (e.g. add /leadState/planApprovedAt, /leadState/planApprovedBySessionId, /currentPhase, /phases/{id:planning}/status to LEAD_DENY_PATTERNS) and expose a single server-side approvePlan() entry used only from the Implement / respondToInput path.
🐛 Functionality
[High] spawnAgent leaves orphan worker sessions when manifest patch fails
File: apps/desktop/src/main/services/ai/tools/orchestrationTools.ts:L228-L304
Issue: The tool creates a chat session first, then patches the manifest. On manifest_patch_failed it returns an error but does not delete the session or stop downstream use; the worker never appears in manifest.agents but still exists in Work.
Repro: Trigger an etag conflict or policy denial after createSession succeeds (e.g. concurrent manifest updates during spawn).
Fix: On patch failure, delete the created session (or register a compensating cleanup job) and avoid returning sessionId as success; retry patch in a transaction or create the session only after manifest patch succeeds.
[Medium] runCreate failure leaves a lead chat with no orchestration bundle
File: apps/desktop/src/renderer/components/chat/AgentChatPane.tsx:L4860-L4880
Issue: If orchestration.runCreate throws after agentChat.create, the lead session persists without orchestrationRunId / bundle path. Tools and the plan panel cannot mount; only a banner error is shown.
Repro: Create an orchestrator chat when lane worktree is missing or .ade/orchestration cannot be created.
Fix: Roll back the chat session on runCreate failure, or block sending until bundle allocation succeeds (with retry).
[Medium] claimTask does not require the session to be registered in the run
File: apps/desktop/src/main/services/orchestration/orchestrationService.ts:L928-L996
Issue: releaseTask and recordValidationRun reject unknown sessionId, but claimTask will set assigneeSessionId to any ID. A mistyped or forged IPC sessionId can assign tasks to a non-agent row (only agent status update is skipped).
Repro: Invoke orchestration.claimTask with a random sessionId not in manifest.agents.
Fix: Mirror releaseTask and return ok: false when sessionId is not in manifest.agents.
⚡ Performance
[Medium] Orchestration panel re-reads the full bundle on every event
File: apps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsx:L16-L20,L191-L205
Issue: Each manifest/plan event triggers bundleRead (full manifest + plan.md). For busy runs with frequent heartbeats/patches, this adds redundant main-process I/O and JSON parsing on the hot path.
Impact: Extra disk read + parse per event while the panel is open; scales with agent count and patch frequency, not bundle size alone.
Fix: Apply incremental manifest / planMd from event payloads when present; reserve full bundleRead for subscribe/resync and patch-only events without a body.
🎨 UI/UX
[Low] Lead orchestrator rainbow frame never activates
File: apps/desktop/src/renderer/components/chat/AgentChatPane.tsx:L7726
Issue: OrchestratorLeadFrame is always rendered with active={false}, so lead chats never get the documented visual distinction from worker/validator sessions.
Fix: Pass active={orchestrationRole === "lead"} (and respect prefers-reduced-motion via the component).
[Low] Relative plan images render with broken src when no asset resolver is wired
File: apps/desktop/src/renderer/components/orchestration/PlanMarkdown.tsx:L474-L477
Issue: HTML spec links fall back to file://${bundleRoot}/…, but img uses resolved?.url ?? srcStr only. AgentChatPane does not pass resolveAsset, and orchestrationBridge has no assetDataUrl, so relative image paths in plan.md show as broken icons.
Fix: Add the same bundleRoot prefix fallback used for HTML assets, or wire readAssetDataUrl through preload IPC.
Notes
Positive patterns worth keeping: etag-guarded manifest patches, role-based patch allowlists, spawn brief validation, orchestration worker sandbox hardening (blockByDefault, interpreter payload checks), and plan-approval gating in both tools and IPC spawn paths (once approval fields are locked down). Extensive unit tests cover patch policy, orchestration service, and bash sandbox edges. I did not run the full desktop test suite in this environment; CI should be the final gate.
Sent by Cursor Automation: BUGBOT in Versic
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+235
to
+241
| return ( | ||
| <div | ||
| data-mermaid-block="" | ||
| className="mb-3 overflow-auto rounded-md border border-white/[0.06] bg-white/[0.02] p-3" | ||
| dangerouslySetInnerHTML={{ __html: svg }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
dangerouslySetInnerHTML with mermaid SVG rendered from agent-controlled plan content
MermaidBlock injects the raw SVG string produced by mermaid into the DOM without any post-render sanitization. The source comes from plan.md which AI agents write via planAppend. Mermaid is initialized with securityLevel: "strict", but strict mode has had historical bypasses. Consider running the rendered SVG through DOMPurify or wrapping output in a sandboxed <iframe srcdoc>.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/orchestration/PlanMarkdown.tsx
Line: 235-241
Comment:
**`dangerouslySetInnerHTML` with mermaid SVG rendered from agent-controlled plan content**
`MermaidBlock` injects the raw SVG string produced by mermaid into the DOM without any post-render sanitization. The source comes from `plan.md` which AI agents write via `planAppend`. Mermaid is initialized with `securityLevel: "strict"`, but strict mode has had historical bypasses. Consider running the rendered SVG through DOMPurify or wrapping output in a sandboxed `<iframe srcdoc>`.
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/ade-cli/src/cli.ts (1)
10290-10305:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire the parent PID into spawned daemons.
runServe()only enables the new parent-process shutdown path whenADE_RUNTIME_PARENT_PIDis present, butspawnMachineRuntimeDaemon()never sets it. As written, spawned runtimes will never take the parent-exit fast path and can linger until idle timeout/manual shutdown.Suggested fix
const env: NodeJS.ProcessEnv = { ...process.env, ...(serviceCommand.env ?? {}), ADE_DEFAULT_ROLE: options.role, ADE_RPC_SOCKET_PATH: socketPath, ADE_RUNTIME_SOCKET_PATH: socketPath, + ADE_RUNTIME_PARENT_PID: String(process.pid), };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/cli.ts` around lines 10290 - 10305, spawnMachineRuntimeDaemon() does not set ADE_RUNTIME_PARENT_PID into the child env, so runtimes never take the parent-exit fast path checked by runServe(); fix by adding ADE_RUNTIME_PARENT_PID: String(process.pid) to the env object created in spawnMachineRuntimeDaemon() (the same place where ADE_DEFAULT_ROLE, ADE_RPC_SOCKET_PATH, ADE_RUNTIME_SOCKET_PATH, ADE_RUNTIME_IDLE_EXIT_MS, and ADE_RUNTIME_BUILD_HASH are set) so the child process can detect and use the parent PID and enable the new shutdown behavior.apps/desktop/src/main/services/opencode/openCodeRuntime.ts (1)
596-602:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward
mcpwhen creating the prompt session.
RunOpenCodePromptArgsnow acceptsmcp, but this call drops it, sorunOpenCodeTextPrompt()ignores the new config entirely.Suggested fix
const handle = await startOpenCodeSession({ directory: args.directory, title: args.title, projectConfig: args.projectConfig, + mcp: args.mcp, leaseKind: "shared", ownerKind: "oneshot", });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/opencode/openCodeRuntime.ts` around lines 596 - 602, The call to startOpenCodeSession is not forwarding the new mcp field from RunOpenCodePromptArgs so runOpenCodeTextPrompt ignores the mcp config; update the startOpenCodeSession invocation (the block creating `handle`) to include mcp: args.mcp (and ensure the `startOpenCodeSession` options structure accepts/forwards `mcp`) so the prompt session is created with the provided mcp setting and runOpenCodeTextPrompt receives it.apps/desktop/src/renderer/components/chat/ChatProposedPlanCard.tsx (1)
30-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle clipboard write failures explicitly.
Line 31 has no
.catch, so clipboard-denied cases can trigger unhandled promise rejections.Suggested patch
const handleCopy = useCallback(() => { - void navigator.clipboard.writeText(bodyText).then(() => { - setCopied(true); - setTimeout(() => setCopied(false), 1500); - }); + void navigator.clipboard.writeText(bodyText) + .then(() => { + setCopied(true); + setTimeout(() => setCopied(false), 1500); + }) + .catch(() => { + setCopied(false); + }); }, [bodyText]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/ChatProposedPlanCard.tsx` around lines 30 - 35, The handleCopy callback currently calls navigator.clipboard.writeText(bodyText) without handling failures; update handleCopy to explicitly catch promise rejections from navigator.clipboard.writeText (or use async/await with try/catch) and handle errors (e.g., show a user-facing error or fallback copy method and log the error) while still setting setCopied(true) only on success and not leaving unhandled promise rejections; reference the handleCopy function, navigator.clipboard.writeText, bodyText, and setCopied to locate where to add the .catch or try/catch and error handling logic.
🟡 Minor comments (7)
apps/ade-cli/src/tuiClient/app.tsx-2556-2563 (1)
2556-2563:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFinish migrating prompt writes to the cursor-aware setter.
The new cursor model only stays correct when prompt changes flow through
setPromptValue(), but this file still has directsetPrompt(...)writes at Line 4306, Line 4448, Line 6759, Line 8846, and Line 9628. Those paths reuse the old cursor/focus state, so opening a form or injecting text can leave the cursor in the wrong column or keep attachment focus active.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/app.tsx` around lines 2556 - 2563, Several places still call setPrompt(...) directly and bypass the new cursor-aware update; replace each direct call to setPrompt(newText) so it uses setPromptValue(newText, promptCursorRef.current) instead (or just setPromptValue(newText) to default to end), ensuring promptRef and promptCursorRef stay in sync and attachment focus is cleared by the same code path; search for all usages of setPrompt and update them to call setPromptValue, keeping clampPromptCursor, promptCursorRef, and setAttachmentFocusIndex(null) behavior provided by setPromptValue.apps/ade-cli/src/tuiClient/app.tsx-1725-1765 (1)
1725-1765:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire the ESC prefix in the mouse parsers.
All three regexes use
\x1b*, which also matches zero ESC characters. That lets ordinary text like[Mabcor[<0;1;1Mget consumed as a mouse event and never reach the prompt. These patterns should require a single ESC sequence instead.Minimal fix
- const sgr = /\x1b*\[<(\d+);(\d+);(\d+)([mM])/g; + const sgr = /\x1b\[<(\d+);(\d+);(\d+)([mM])/g; … - const rxvt = /\x1b*\[(\d+);(\d+);(\d+)M/g; + const rxvt = /\x1b\[(\d+);(\d+);(\d+)M/g; … - const x10 = /\x1b*\[M([\s\S])([\s\S])([\s\S])/g; + const x10 = /\x1b\[M([\s\S])([\s\S])([\s\S])/g;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/app.tsx` around lines 1725 - 1765, In parseTerminalMouseInputs, the three regexes (sgr, rxvt, x10) use '\x1b*' which allows zero ESC bytes; change each to require a single ESC by replacing '\x1b*' with '\x1b' (i.e. use /\x1b\[<.../g, /\x1b\[(\d+;... )/g and /\x1b\[M.../g respectively) so the parsers only match sequences prefixed by the ESC character.apps/ade-cli/src/tuiClient/components/RightPane.tsx-97-101 (1)
97-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPR interaction height is overestimated for zero-check PRs.
On Line 100,
prRow.heightis always3, but the non-selected PR block renders only2lines whencontent.pr.checksTotal === 0(see PR rendering branch). This can misalign row-to-action hit-testing.Suggested fix
let prRow: LaneDetailsInteractionLayout["prRow"] = null; if (content.pr) { row += 2; // PR section heading with marginTop. - prRow = { start: row, height: 3 }; + const prSelected = content.selectedActionIndex === LANE_DETAIL_PR_ACTION_INDEX; + const prHeight = prSelected || content.pr.checksTotal > 0 ? 3 : 2; + prRow = { start: row, height: prHeight }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/components/RightPane.tsx` around lines 97 - 101, The PR interaction row height is always set to 3 but the non-selected PR rendering uses only 2 lines when content.pr.checksTotal === 0; update the prRow assignment in the block that sets prRow (the code that currently sets prRow = { start: row, height: 3 }) to compute height based on content.pr.checksTotal (e.g., height = content.pr.checksTotal === 0 ? 2 : 3) so hit-testing aligns with the actual rendered PR block; keep the check inside the existing if (content.pr) branch and preserve the start = row logic.apps/desktop/src/main/services/ai/tools/orchestrationPlanQuality.ts-54-65 (1)
54-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten these regexes to avoid accidental gate passes.
/tags?/imatches unrelated words likestage/stages, and/view/imatchesreview/preview, so a plan can satisfy the agent/UI checks without actually containing those sections. Use word boundaries or more heading-shaped patterns here.Example fix
- /tags?/i, + /\btags?\b/i, @@ - /view/i, + /\bview\b/i,Also applies to: 90-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/ai/tools/orchestrationPlanQuality.ts` around lines 54 - 65, The regexes in the patterns array (the "patterns" constant in orchestrationPlanQuality.ts) are too loose and match substrings like "stage" or "preview"; update each pattern to use word boundaries and/or heading-like anchors so they only match standalone section headings (e.g. use \b for single-word tokens like "tag" => \btag(s)?\b and use start/line anchors or heading patterns for section names such as /^\s*#+\s*(assignee|owner|validators?)\b/i). Apply the same tightening to the other similar list around lines 90-105 (the other patterns array) so all checks use word boundaries or heading-shaped regexes rather than plain substrings.apps/desktop/src/main/services/orchestration/manifestNormalization.ts-506-511 (1)
506-511:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim supersede IDs before returning them.
Array entries keep their original whitespace here, so
" T-1 "never matches task idT-1incollectSupersededTaskIds().Suggested fix
export function taskSupersedesIds(task: OrchestrationManifest["tasks"][number]): string[] { const value = (task as unknown as { supersedes?: unknown }).supersedes; if (Array.isArray(value)) { - return value.filter((entry): entry is string => typeof entry === "string" && entry.trim().length > 0); + return value + .filter((entry): entry is string => typeof entry === "string" && entry.trim().length > 0) + .map((entry) => entry.trim()); } - return typeof value === "string" && value.trim() ? [value] : []; + return typeof value === "string" && value.trim() ? [value.trim()] : []; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/orchestration/manifestNormalization.ts` around lines 506 - 511, taskSupersedesIds currently returns supersede IDs without trimming whitespace so entries like " T-1 " won't match in collectSupersededTaskIds; update taskSupersedesIds to trim each string entry before filtering/returning (for the array branch: map each entry to (typeof entry === "string" ? entry.trim() : entry) then filter non-empty strings; for the single-string branch: trim the value before checking truthiness and returning) so all returned IDs are normalized and match collectSupersededTaskIds.apps/desktop/src/renderer/browserMock.ts-663-666 (1)
663-666:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDeduplicate
MOCK_SESSIONSbyidbefore returning session lists.Appending
MOCK_ORCH_SESSIONSunconditionally can produce duplicateids when snapshot data already includes those sessions, which can break keyed rendering and session selection consistency.Suggested fix
-const MOCK_SESSIONS: any[] = [ - ...(ADE_DB_SESSIONS.length > 0 ? ADE_DB_SESSIONS : BUILTIN_MOCK_SESSIONS), - ...MOCK_ORCH_SESSIONS, -]; +const MOCK_SESSIONS: any[] = (() => { + const base = + ADE_DB_SESSIONS.length > 0 ? ADE_DB_SESSIONS : BUILTIN_MOCK_SESSIONS; + const merged = [...base, ...MOCK_ORCH_SESSIONS]; + const seen = new Set<string>(); + return merged.filter((row) => { + const id = String(row?.id ?? ""); + if (!id || seen.has(id)) return false; + seen.add(id); + return true; + }); +})();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/browserMock.ts` around lines 663 - 666, MOCK_SESSIONS can contain duplicate session objects with the same id because MOCK_ORCH_SESSIONS is appended unconditionally; fix by deduplicating the combined array (the current spread expression that builds MOCK_SESSIONS from ADE_DB_SESSIONS, BUILTIN_MOCK_SESSIONS and MOCK_ORCH_SESSIONS) into a new array keyed by session.id before it’s used/returned. Implement a dedupe step after the spread that reduces into a Map or object by id (preserving the preferred source order — e.g., keep ADE_DB_SESSIONS entries first), then replace MOCK_SESSIONS with the Map.values() array so all returned session lists are unique by id.apps/desktop/src/renderer/browserMock.ts-5519-5534 (1)
5519-5534:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn cloned orchestration payloads from mock handlers.
Most handlers return the same manifest object instance. If any consumer mutates it, later calls observe mutated state and the mock becomes nondeterministic.
Suggested fix
+const clone = <T,>(value: T): T => structuredClone(value); + orchestration: { - runCreate: resolvedArg({ runId: MOCK_ORCH_RUN_ID, manifest: MOCK_ORCH_MANIFEST, etag: MOCK_ORCH_MANIFEST.etag }), - bundleRead: resolvedArg({ manifest: MOCK_ORCH_MANIFEST, planMd: MOCK_ORCH_PLAN_MD, etag: MOCK_ORCH_MANIFEST.etag }), + runCreate: resolvedArg({ runId: MOCK_ORCH_RUN_ID, manifest: clone(MOCK_ORCH_MANIFEST), etag: MOCK_ORCH_MANIFEST.etag }), + bundleRead: resolvedArg({ manifest: clone(MOCK_ORCH_MANIFEST), planMd: MOCK_ORCH_PLAN_MD, etag: MOCK_ORCH_MANIFEST.etag }), manifestReadSection: async (args: any) => ({ section: args?.section ?? "tasks", data: (MOCK_ORCH_MANIFEST as any)[args?.section ?? "tasks"] ?? [], etag: MOCK_ORCH_MANIFEST.etag }), - manifestPatch: resolvedArg({ ok: true, manifest: MOCK_ORCH_MANIFEST, etag: MOCK_ORCH_MANIFEST.etag }), + manifestPatch: resolvedArg({ ok: true, manifest: clone(MOCK_ORCH_MANIFEST), etag: MOCK_ORCH_MANIFEST.etag }), ... - spawnAgent: resolvedArg({ sessionId: "mock-orch-spawn-new", manifest: MOCK_ORCH_MANIFEST, etag: MOCK_ORCH_MANIFEST.etag }), + spawnAgent: resolvedArg({ sessionId: "mock-orch-spawn-new", manifest: clone(MOCK_ORCH_MANIFEST), etag: MOCK_ORCH_MANIFEST.etag }), ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/browserMock.ts` around lines 5519 - 5534, The mock handlers (resolvedArg calls and manifestReadSection) currently return the same MOCK_ORCH_MANIFEST / MOCK_ORCH_PLAN_MD object instances so mutations leak across calls; fix by returning deep clones instead of the original object—e.g., replace direct returns in runCreate, bundleRead, manifestReadSection, manifestPatch, planAppend, planWrite, spawnAgent, assetRegister, claimTask, releaseTask and the runList entry with structured clones (structuredClone(...) or a cloneDeep utility) of MOCK_ORCH_MANIFEST, MOCK_ORCH_PLAN_MD and any section arrays; ensure manifestReadSection clones the section data and runList builds its item from a cloned manifest snapshot to keep each mock response independent and deterministic.
🧹 Nitpick comments (5)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
8406-8450: 💤 Low valuePotential race in
ensureOrchestrationHttpMcpServerif called concurrently.If two callers race past the null check at line 8409 before either assigns to
managed.orchestrationHttpMcpServer, both will start servers and one will be orphaned. Given the current call sites appear serialized per session, this is low risk but worth noting.♻️ Optional: Add a guard promise to prevent double initialization
+const orchestrationMcpSetupPromises = new WeakMap<ManagedChatSession, Promise<ManagedChatSession["orchestrationHttpMcpServer"]>>(); + const ensureOrchestrationHttpMcpServer = async ( managed: ManagedChatSession, ): Promise<ManagedChatSession["orchestrationHttpMcpServer"]> => { if (managed.orchestrationHttpMcpServer) return managed.orchestrationHttpMcpServer; + const existing = orchestrationMcpSetupPromises.get(managed); + if (existing) return existing; + const promise = (async () => { const tools = createOrchestrationRuntimeToolMap(managed); if (!tools) return null; // ... rest of implementation + })(); + orchestrationMcpSetupPromises.set(managed, promise); + try { + return await promise; + } finally { + orchestrationMcpSetupPromises.delete(managed); + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 8406 - 8450, ensureOrchestrationHttpMcpServer currently has a race: two callers can pass the null check and both start servers; fix by using a guard promise on managed.orchestrationHttpMcpServer (or a separate managed._orchestrationInit Promise) so the first caller stores a pending promise immediately, subsequent callers await that promise, and only the resolved config is assigned as the final lease; ensure failures clear the guard promise and call close() on any orphaned server in the error path, and update closeOrchestrationHttpMcpServer to handle the promise-backed lease value.apps/desktop/src/preload/orchestrationBridge.ts (1)
48-51: ⚡ Quick winAvoid silently swallowing orchestration subscribe/unsubscribe IPC failures.
catch(() => undefined)hides wiring/runtime failures and makes subscription issues hard to diagnose. At minimum, surface these failures via logging.As per coding guidelines `apps/desktop/src/**`: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.Suggested patch
- void ipcRenderer.invoke(IPC.orchestrationSubscribe, args).catch(() => undefined); + void ipcRenderer.invoke(IPC.orchestrationSubscribe, args).catch((error) => { + console.warn("[orchestrationBridge] orchestrationSubscribe failed", error); + }); return () => { ipcRenderer.removeListener(IPC.orchestrationEvent, listener); - void ipcRenderer.invoke(IPC.orchestrationUnsubscribe, args).catch(() => undefined); + void ipcRenderer.invoke(IPC.orchestrationUnsubscribe, args).catch((error) => { + console.warn("[orchestrationBridge] orchestrationUnsubscribe failed", error); + }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/preload/orchestrationBridge.ts` around lines 48 - 51, The current ipcRenderer.invoke calls for IPC.orchestrationSubscribe and IPC.orchestrationUnsubscribe swallow errors with catch(() => undefined); change those catch handlers to surface failures by logging the error (e.g., console.error or the app's logger) with context about which IPC call failed and the args, so replace the silent catches on ipcRenderer.invoke(IPC.orchestrationSubscribe, args) and ipcRenderer.invoke(IPC.orchestrationUnsubscribe, args) to log the error and return/ignore as appropriate; keep the ipcRenderer.removeListener(IPC.orchestrationEvent, listener) behavior unchanged.apps/desktop/src/preload/preload.ts (1)
5196-5200: ⚡ Quick winKeep
agentChat.readTranscripton the same runtime routing path as other chat reads.This new method bypasses the runtime-aware path used by the rest of
agentChat, which can diverge behavior between local vs remote project bindings.♻️ Proposed change
- readTranscript: (args: { + readTranscript: async (args: { sessionId: string; limit?: number; since?: string; - }) => ipcRenderer.invoke(IPC.agentChatReadTranscript, args), + }): Promise<unknown> => { + const runtime = await callProjectRuntimeActionIfBound<unknown>( + "chat", + "readTranscript", + { args }, + ); + return runtime.handled + ? runtime.result + : ipcRenderer.invoke(IPC.agentChatReadTranscript, args); + },As per coding guidelines, "Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/preload/preload.ts` around lines 5196 - 5200, The new readTranscript handler calls ipcRenderer.invoke(IPC.agentChatReadTranscript, args) directly which bypasses the runtime-aware routing used by the other agentChat read methods; replace this direct ipcRenderer.invoke call in the readTranscript export with the same runtime-aware helper used by the rest of agentChat (the existing agentChat read routing helper used elsewhere in this file), keeping the same args shape and IPC channel (IPC.agentChatReadTranscript) so local vs remote project bindings follow the same path as other reads.apps/desktop/src/renderer/components/chat/ChatComposerShell.tsx (1)
56-57: ⚡ Quick winMove stylesheet injection out of render.
Line 56 performs a DOM side effect during render. This should run in an effect to keep render pure and avoid strict/concurrent-mode surprises.
Suggested patch
-import type { CSSProperties, ReactNode } from "react"; +import { useEffect, type CSSProperties, type ReactNode } from "react"; ... - if (orchestratorActive) ensureOrchestratorComposerStyles(); + useEffect(() => { + if (orchestratorActive) ensureOrchestratorComposerStyles(); + }, [orchestratorActive]);As per coding guidelines,
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/ChatComposerShell.tsx` around lines 56 - 57, The call ensureOrchestratorComposerStyles() is being invoked during render (guarded by orchestratorActive) in ChatComposerShell, causing a DOM side-effect inside the render path; move this logic into a React effect: replace the inline call with a useEffect that depends on orchestratorActive and calls ensureOrchestratorComposerStyles() when orchestratorActive is true (and clean up if necessary), so the render remains pure and side-effects run in effect hooks.apps/desktop/src/renderer/components/chat/OrchestratorLeadFrame.tsx (1)
112-113: ⚡ Quick winAvoid DOM mutation during render for keyframe injection.
Line 112 triggers a render-phase side effect. Move this to an effect hook so rendering stays pure.
Suggested patch
-import type { CSSProperties, ReactNode } from "react"; +import { useEffect, type CSSProperties, type ReactNode } from "react"; ... - if (active) ensureKeyframes(); + useEffect(() => { + if (active) ensureKeyframes(); + }, [active]);As per coding guidelines,
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/OrchestratorLeadFrame.tsx` around lines 112 - 113, The call to ensureKeyframes() is being invoked during render (if (active) ensureKeyframes()), causing render-phase DOM mutation; move that call into a React effect so rendering remains pure: remove the inline call and add a useEffect in the OrchestratorLeadFrame component that runs when the active prop/state changes (useEffect(() => { if (active) ensureKeyframes(); }, [active])), keeping ensureKeyframes as-is and not invoking it during JSX render; this confines DOM mutation to the effect lifecycle and preserves pure rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 11019-11044: The monitorRuntimeIdleExit function currently samples
connection state and can miss short-lived connections; instead add a
lastActivityAt timestamp/property to HeadlessRpcServerState and update that
timestamp from createHeadlessRpcServer whenever a connection opens or closes (or
when any request is served), then change monitorRuntimeIdleExit to compute the
most-recent activity by reading states.map(s => s.lastActivityAt) (or taking the
max) rather than relying on the local lastActiveAt snapshot and
hasActiveHeadlessConnections; keep the timer logic but compare Date.now()
against the most-recent lastActivityAt to decide when to call onIdle and
preserve stop/clearInterval behavior.
In `@apps/ade-cli/src/headlessLinearServices.ts`:
- Around line 477-507: readToken currently calls ghAuthToken on every invocation
(via getToken) causing repeated synchronous shells; cache the GH CLI result
returned by ghAuthToken in an in-memory structure with a short TTL (or an
explicit invalidate function) and make readToken/getToken consult that cache
instead of re-running ghAuthToken each time. Specifically, add a module-level
cachedGh lookup (value + expiry) and update readToken to return the cached gh
result when not expired, call ghAuthToken only on cache miss, and provide a way
to invalidate the cache when PAT/env changes or on explicit error; keep
token/source/patTokenStored/ghCliPath/ghAuthError shapes unchanged so callers of
readToken and getToken (and functions like apiRequest/publishCurrentProject) see
the same contract.
- Around line 328-352: The branch that checks spawnSync("gh", ...) uses only
result.status and assumes GH exists; instead check result.error (and status ===
null) from spawnSync and treat that as “gh missing” by returning ghCliPath: null
and a ghAuthError containing result.error.message (or String(result.error));
keep the existing behavior when result.status is non-zero or stdout is empty but
ensure you reference spawnSync's result.error, result.status, token, ghCliPath
and ghAuthError to update the returned object accordingly so machines without
the gh binary are reported correctly.
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 338-355: mergeOptimisticChatSessions currently mutates the
optimisticSessions Map (it deletes keys based on the incoming sessions), which
causes optimistic sessions to be evicted prematurely when used inside
setSessions updates; change mergeOptimisticChatSessions so it does not mutate
its optimisticSessions argument (remove the optimisticSessions.delete loop and
any in-place changes) and instead only reads from it to produce the merged array
(e.g., compute pending from a non-mutating iteration or a shallow copy), and
move any pruning of optimisticChatSessionsRef.current to refreshState where you
compare against the authoritative listChatSessions() result and remove stale
entries there.
In `@apps/desktop/src/main/main.ts`:
- Around line 2997-3006: The orchestrationService created via
createOrchestrationService (stored in orchestrationServiceRef) is not torn down
when a project context closes; update disposeContextResources() to call the
orchestrationService's teardown API (e.g., orchestrationService.dispose() or
orchestrationService.close()) and clear orchestrationServiceRef to undefined
when a context is evicted or shutdown so watchers/subscriptions are removed;
ensure you null-check orchestrationServiceRef before calling the dispose method
and handle errors (try/catch) to avoid blocking context teardown.
In `@apps/desktop/src/main/services/ai/tools/orchestrationTools.ts`:
- Around line 284-305: If createSession succeeds but subsequent
svc.manifestPatch (called with buildPatchRequest and ctx.bundlePath) fails, the
newly created session (created.id) must be cleaned up; update the error branch
after both patch attempts to call the session deletion API (e.g.,
svc.deleteSession(created.id) or svc.sessionDelete(created.id)) and await it
(log but do not throw if deletion fails), then return the existing
manifest_patch_failed response; ensure you reference the same created.id, run
the cleanup before returning, and keep the retry logic for manifestPatch intact.
In `@apps/desktop/src/main/services/github/githubService.ts`:
- Around line 33-53: readGitHubCliAuthToken currently calls spawnSync("gh",
["auth","token"]) on the Electron main thread and is synchronously invoked by
readAuthToken, apiRequest, getStatus, publishCurrentProject and getTokenOrThrow,
which can block the main loop for up to the timeout; change this to an async,
non-blocking design: replace the synchronous spawnSync call with an asynchronous
child-process invocation run from a background refresh task (e.g.,
spawn/execFile/child_process.promises) that periodically (or on demand) updates
a cached token and ghCliPath, keep readGitHubCliAuthToken (or a new getter like
getCachedGhToken) synchronous and returning the cached value immediately
(respecting ADE_DISABLE_GH_AUTH_FALLBACK), and update callers
(readAuthToken/apiRequest/getStatus/publishCurrentProject/getTokenOrThrow) to
use the cached getter and optionally trigger the background refresh without
blocking the main process.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 5850-5865: The current subscribeOrchestrationBroadcast sets up a
global listener and rebroadcasts every orchestration event to
BrowserWindow.getAllWindows(), but subscribe/unsubscribe never record the
subscribing sender; change the implementation so subscribeOrchestrationBroadcast
(and the subscribe handler that currently calls it) records subscriptions keyed
by webContents.id (or webContents) and runId (from the subscribe payload) in a
Map/Set, and have orchestrationService.on("event", ...) only forward
IPC.orchestrationEvent to webContents whose subscription set includes the
event's runId; also implement orchestrationUnsubscribe() to remove that
webContents/runId entry (and cleanup when a webContents is destroyed) so
releases only affect the subscribers for that runId instead of all windows.
- Around line 6009-6070: The code creates a chat session via
ctx.agentChatService.createSession (saved as created) before writing the
manifest entry (buildSpawnPatch + service.manifestPatch); if both manifestPatch
attempts fail the orphan session (created.id) must be torn down. Wrap the
manifest patch logic in a try/catch/finally (or ensure cleanup on error) and on
failure call the agent teardown API (e.g.,
ctx.agentChatService.deleteSession(created.id) or
ctx.agentChatService.endSession/closeSession if that’s the real method) to
remove the spawned session, then rethrow the original error; reference
createSession, created.id, buildSpawnPatch, service.manifestPatch and patchRes
when implementing the cleanup.
- Around line 733-735: pathJoinOrchestration currently joins an unvalidated
runId into path.join(worktree, ".ade", "orchestration", runId) allowing
traversal like "../" from the renderer; fix by validating runId in
pathJoinOrchestration (or a helper) against a strict token pattern (e.g., only
alphanumerics, dashes/underscores) and reject anything else, and additionally
after constructing the candidate path resolve it and ensure it remains under the
orchestration root (resolve(path) startsWith resolvedOrchestrationRoot) and
throw/fail closed if it escapes; reference function pathJoinOrchestration,
parameters worktree and runId, and the orchestration root ".ade/orchestration"
when implementing these checks.
In `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts`:
- Around line 307-316: The current unlinkSocketIfNotListening unconditionally
unlinks on any connect failure; change it to only unlink when the connect error
definitively indicates a stale/unbound socket (e.g. error.code ===
'ECONNREFUSED' for a UNIX socket with no listener). In
unlinkSocketIfNotListening, wrap the openSocketTransport call so the catch
captures the thrown error, test error.code and only call
fs.unlinkSync(socketPath) when it matches the stale-socket codes (do not unlink
on 'EACCES', timeouts, or other transient errors); otherwise swallow/return or
rethrow as appropriate. Reference: unlinkSocketIfNotListening,
openSocketTransport, and fs.unlinkSync.
In `@apps/desktop/src/main/services/orchestration/orchestrationService.ts`:
- Around line 622-627: persistPlan performs a "plan-only" write but does not
advance the bundle etag or related metadata, so planWrite's ifMatchEtag check
can be bypassed; update persistPlan (and analogous code paths referenced around
lines 845-893) to after writing PLAN_FILE also increment or update
runtime.serverGeneration, append an entry to runtime.history (or otherwise
record the change), and update runtime.manifest.etag to a new value (e.g.,
generate a new etag/nonce) and persist the manifest so concurrent writers will
detect the change; keep existing helpers markSelfWrite, atomicWrite, and
runtime.planMd assignment but ensure the manifest/etag/history/serverGeneration
updates happen atomically with the plan write (or in the same
transaction/sequenced update) so plan-only writes produce a new etag and
conflict with stale ifMatchEtag values.
- Around line 633-638: The handler runCreate currently trusts req.bundleRoot to
build bundlePath and write to disk (bundlePath, ensureBundleDir), which exposes
the main process to IPC path injection; change it to derive the bundle root
server-side using resolveLaneWorktree(req.laneId) (or a canonical server-side
lane worktree resolver) and compute bundlePath =
path.join(resolveLaneWorktree(req.laneId), ".ade", "orchestration", newRunId())
before calling ensureBundleDir; then remove or deprecate caller-supplied
bundlePath parameters from public read/write methods (or enforce that any
incoming bundlePath is inside resolveLaneWorktree(req.laneId) and reject
otherwise) so all orchestration I/O is constrained to the lane worktree.
- Around line 777-799: The current logic uses hasHumanOverride and
hasUserOverrideEntry booleans which only prove some human override and some
userOverride append exist anywhere in the patch set; update the checks to ensure
the userOverride append matches the same task being acted on. Specifically,
inside the isTaskStatusDonePatch handling (and the isValidationGateRequiredOff
branch) where you extract taskId via extractTaskIdFromPath and call
taskValidationGatesSatisfied, replace the global hasUserOverrideEntry check with
a predicate that looks for an isUserOverrideEntryAppend op whose path (or
payload) maps to the same taskId (e.g., compute
hasUserOverrideEntryForTask(taskId) by scanning req.patches with
isUserOverrideEntryAppend and matching extractTaskIdFromPath on those ops), and
use that per-task boolean together with hasHumanOverride (or make
hasHumanOverride per-task similarly if needed) to decide whether to allow the
operation.
In `@apps/desktop/src/main/services/orchestration/patchPolicy.ts`:
- Around line 70-73: The loop currently treats any "**" segment as an immediate
match; change the logic so "**" is only accepted when it is the final segment:
in the function that iterates parts (using variables parts, parsed.segments, and
the loop index i), when want === "**" only return true if i === parts.length - 1
(trailing); otherwise treat it as a mismatch (return false or continue to deny
this pattern) and add/update tests to cover a non-trailing "**" like
"/tasks/**/status" to ensure it does not over-grant.
- Around line 227-243: The current check allows writes to unassigned tasks
because it only rejects when task.assigneeSessionId exists and differs; update
the logic in the patch policy (the block using extractTaskId,
ctx.manifest.tasks, ctx.actorSessionId, task.assigneeSessionId and op.path) to
reject any write where the task exists and its assigneeSessionId is not exactly
equal to ctx.actorSessionId (treat undefined/null assigneeSessionId as "not
owned"), returning allowed: false with the same reason/path structure; ensure
you still only perform this check when extractTaskId(op.path) yields a taskId
and a matching task is found.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 7177-7179: OrchestratorLeadFrame is being passed a hard-coded
active={false} which prevents the lead chrome from showing; change the prop to
use the real derived state (isOrchestratorLead or another appropriate boolean
like selectedSession?.interactionMode === "orchestrator-lead") so that
OrchestratorLeadFrame receives active={isOrchestratorLead} (or equivalent)
instead of false, ensuring the lead UI appears for orchestrator-lead sessions.
- Around line 7660-7663: The right-side layout currently allows multiple panes
(proof/simulator/app-control/subagent/cursor-cloud) to render alongside
orchestration; change the ownership so orchestration becomes the sole right-pane
when active by updating the pane-open logic and render guards: compute
rightPaneOpen such that orchestrationPanelOpen takes precedence (e.g., if
orchestrationPanelOpen then ignore other flags), and in the render section
(where renderRightPane(...) calls occur around render line ~8008) wrap each call
with a check against activeRightPane (e.g., activeRightPane === 'proof' /
'simulator' / 'appControl' / 'subagent' / 'cursorCloud' / 'orchestration') and
ensure when orchestrationRunId/orchestrationPanelOpen is true the
activeRightPane value is set to the orchestration name so only the orchestration
pane renders; update references to orchestrationRunId, orchestrationPanelOpen,
activeRightPane, and renderRightPane accordingly.
In `@apps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsx`:
- Around line 71-80: The component seeds local state only on mount
(initialProvider, initialModelId, useState for modelId, provider,
reasoningEffort, codexFastMode) so when the suggested prop updates the UI can
show stale values; add a useEffect that watches suggested and calls
setModelId(suggested?.modelId ?? ""), setProvider(suggested?.provider ??
"claude"), setReasoningEffort(suggested?.reasoningEffort ?? null), and
setCodexFastMode(Boolean(suggested?.codexFastMode)) to resync local selection
state whenever suggested changes.
In `@apps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsx`:
- Around line 192-205: The fallback ds.read() can resolve out-of-order and
overwrite newer panel state; inside the useEffect subscription callback for
ds.subscribe (where payload.kind === "manifest" && !payload.manifest) protect
the async read by gating its dispatch: create a request token/counter or capture
the current runId/laneId/etag in a local variable before calling ds.read(), then
when the promise resolves compare that token/etag/current state to ensure the
response is still current; only then call dispatch({ type: "load-success",
manifest: res.manifest, planMd: res.planMd, etag: res.etag }). Use symbols:
useEffect, ds.subscribe, ds.read, dispatch, runId, laneId, payload, etag to find
and apply the guard so stale responses cannot overwrite newer state.
---
Outside diff comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 10290-10305: spawnMachineRuntimeDaemon() does not set
ADE_RUNTIME_PARENT_PID into the child env, so runtimes never take the
parent-exit fast path checked by runServe(); fix by adding
ADE_RUNTIME_PARENT_PID: String(process.pid) to the env object created in
spawnMachineRuntimeDaemon() (the same place where ADE_DEFAULT_ROLE,
ADE_RPC_SOCKET_PATH, ADE_RUNTIME_SOCKET_PATH, ADE_RUNTIME_IDLE_EXIT_MS, and
ADE_RUNTIME_BUILD_HASH are set) so the child process can detect and use the
parent PID and enable the new shutdown behavior.
In `@apps/desktop/src/main/services/opencode/openCodeRuntime.ts`:
- Around line 596-602: The call to startOpenCodeSession is not forwarding the
new mcp field from RunOpenCodePromptArgs so runOpenCodeTextPrompt ignores the
mcp config; update the startOpenCodeSession invocation (the block creating
`handle`) to include mcp: args.mcp (and ensure the `startOpenCodeSession`
options structure accepts/forwards `mcp`) so the prompt session is created with
the provided mcp setting and runOpenCodeTextPrompt receives it.
In `@apps/desktop/src/renderer/components/chat/ChatProposedPlanCard.tsx`:
- Around line 30-35: The handleCopy callback currently calls
navigator.clipboard.writeText(bodyText) without handling failures; update
handleCopy to explicitly catch promise rejections from
navigator.clipboard.writeText (or use async/await with try/catch) and handle
errors (e.g., show a user-facing error or fallback copy method and log the
error) while still setting setCopied(true) only on success and not leaving
unhandled promise rejections; reference the handleCopy function,
navigator.clipboard.writeText, bodyText, and setCopied to locate where to add
the .catch or try/catch and error handling logic.
---
Minor comments:
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 2556-2563: Several places still call setPrompt(...) directly and
bypass the new cursor-aware update; replace each direct call to
setPrompt(newText) so it uses setPromptValue(newText, promptCursorRef.current)
instead (or just setPromptValue(newText) to default to end), ensuring promptRef
and promptCursorRef stay in sync and attachment focus is cleared by the same
code path; search for all usages of setPrompt and update them to call
setPromptValue, keeping clampPromptCursor, promptCursorRef, and
setAttachmentFocusIndex(null) behavior provided by setPromptValue.
- Around line 1725-1765: In parseTerminalMouseInputs, the three regexes (sgr,
rxvt, x10) use '\x1b*' which allows zero ESC bytes; change each to require a
single ESC by replacing '\x1b*' with '\x1b' (i.e. use /\x1b\[<.../g,
/\x1b\[(\d+;... )/g and /\x1b\[M.../g respectively) so the parsers only match
sequences prefixed by the ESC character.
In `@apps/ade-cli/src/tuiClient/components/RightPane.tsx`:
- Around line 97-101: The PR interaction row height is always set to 3 but the
non-selected PR rendering uses only 2 lines when content.pr.checksTotal === 0;
update the prRow assignment in the block that sets prRow (the code that
currently sets prRow = { start: row, height: 3 }) to compute height based on
content.pr.checksTotal (e.g., height = content.pr.checksTotal === 0 ? 2 : 3) so
hit-testing aligns with the actual rendered PR block; keep the check inside the
existing if (content.pr) branch and preserve the start = row logic.
In `@apps/desktop/src/main/services/ai/tools/orchestrationPlanQuality.ts`:
- Around line 54-65: The regexes in the patterns array (the "patterns" constant
in orchestrationPlanQuality.ts) are too loose and match substrings like "stage"
or "preview"; update each pattern to use word boundaries and/or heading-like
anchors so they only match standalone section headings (e.g. use \b for
single-word tokens like "tag" => \btag(s)?\b and use start/line anchors or
heading patterns for section names such as
/^\s*#+\s*(assignee|owner|validators?)\b/i). Apply the same tightening to the
other similar list around lines 90-105 (the other patterns array) so all checks
use word boundaries or heading-shaped regexes rather than plain substrings.
In `@apps/desktop/src/main/services/orchestration/manifestNormalization.ts`:
- Around line 506-511: taskSupersedesIds currently returns supersede IDs without
trimming whitespace so entries like " T-1 " won't match in
collectSupersededTaskIds; update taskSupersedesIds to trim each string entry
before filtering/returning (for the array branch: map each entry to (typeof
entry === "string" ? entry.trim() : entry) then filter non-empty strings; for
the single-string branch: trim the value before checking truthiness and
returning) so all returned IDs are normalized and match
collectSupersededTaskIds.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 663-666: MOCK_SESSIONS can contain duplicate session objects with
the same id because MOCK_ORCH_SESSIONS is appended unconditionally; fix by
deduplicating the combined array (the current spread expression that builds
MOCK_SESSIONS from ADE_DB_SESSIONS, BUILTIN_MOCK_SESSIONS and
MOCK_ORCH_SESSIONS) into a new array keyed by session.id before it’s
used/returned. Implement a dedupe step after the spread that reduces into a Map
or object by id (preserving the preferred source order — e.g., keep
ADE_DB_SESSIONS entries first), then replace MOCK_SESSIONS with the Map.values()
array so all returned session lists are unique by id.
- Around line 5519-5534: The mock handlers (resolvedArg calls and
manifestReadSection) currently return the same MOCK_ORCH_MANIFEST /
MOCK_ORCH_PLAN_MD object instances so mutations leak across calls; fix by
returning deep clones instead of the original object—e.g., replace direct
returns in runCreate, bundleRead, manifestReadSection, manifestPatch,
planAppend, planWrite, spawnAgent, assetRegister, claimTask, releaseTask and the
runList entry with structured clones (structuredClone(...) or a cloneDeep
utility) of MOCK_ORCH_MANIFEST, MOCK_ORCH_PLAN_MD and any section arrays; ensure
manifestReadSection clones the section data and runList builds its item from a
cloned manifest snapshot to keep each mock response independent and
deterministic.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 8406-8450: ensureOrchestrationHttpMcpServer currently has a race:
two callers can pass the null check and both start servers; fix by using a guard
promise on managed.orchestrationHttpMcpServer (or a separate
managed._orchestrationInit Promise) so the first caller stores a pending promise
immediately, subsequent callers await that promise, and only the resolved config
is assigned as the final lease; ensure failures clear the guard promise and call
close() on any orphaned server in the error path, and update
closeOrchestrationHttpMcpServer to handle the promise-backed lease value.
In `@apps/desktop/src/preload/orchestrationBridge.ts`:
- Around line 48-51: The current ipcRenderer.invoke calls for
IPC.orchestrationSubscribe and IPC.orchestrationUnsubscribe swallow errors with
catch(() => undefined); change those catch handlers to surface failures by
logging the error (e.g., console.error or the app's logger) with context about
which IPC call failed and the args, so replace the silent catches on
ipcRenderer.invoke(IPC.orchestrationSubscribe, args) and
ipcRenderer.invoke(IPC.orchestrationUnsubscribe, args) to log the error and
return/ignore as appropriate; keep the
ipcRenderer.removeListener(IPC.orchestrationEvent, listener) behavior unchanged.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 5196-5200: The new readTranscript handler calls
ipcRenderer.invoke(IPC.agentChatReadTranscript, args) directly which bypasses
the runtime-aware routing used by the other agentChat read methods; replace this
direct ipcRenderer.invoke call in the readTranscript export with the same
runtime-aware helper used by the rest of agentChat (the existing agentChat read
routing helper used elsewhere in this file), keeping the same args shape and IPC
channel (IPC.agentChatReadTranscript) so local vs remote project bindings follow
the same path as other reads.
In `@apps/desktop/src/renderer/components/chat/ChatComposerShell.tsx`:
- Around line 56-57: The call ensureOrchestratorComposerStyles() is being
invoked during render (guarded by orchestratorActive) in ChatComposerShell,
causing a DOM side-effect inside the render path; move this logic into a React
effect: replace the inline call with a useEffect that depends on
orchestratorActive and calls ensureOrchestratorComposerStyles() when
orchestratorActive is true (and clean up if necessary), so the render remains
pure and side-effects run in effect hooks.
In `@apps/desktop/src/renderer/components/chat/OrchestratorLeadFrame.tsx`:
- Around line 112-113: The call to ensureKeyframes() is being invoked during
render (if (active) ensureKeyframes()), causing render-phase DOM mutation; move
that call into a React effect so rendering remains pure: remove the inline call
and add a useEffect in the OrchestratorLeadFrame component that runs when the
active prop/state changes (useEffect(() => { if (active) ensureKeyframes(); },
[active])), keeping ensureKeyframes as-is and not invoking it during JSX render;
this confines DOM mutation to the effect lifecycle and preserves pure rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c5ec6b9-4f27-476a-9877-4c912f1326a8
⛔ Files ignored due to path filters (5)
apps/ade-cli/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsonapps/desktop/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsoncodexGoal.mdis excluded by!*.mdgoal.mdis excluded by!*.mdorchestrator-next-agent-prompt.mdis excluded by!*.md
📒 Files selected for processing (126)
apps/ade-cli/package.jsonapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/headlessLinearServices.test.tsapps/ade-cli/src/headlessLinearServices.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/tuiClient/__tests__/ChatView.test.tsxapps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsxapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/connection.test.tsapps/ade-cli/src/tuiClient/__tests__/drawerSelection.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/components/ChatView.tsxapps/ade-cli/src/tuiClient/components/Drawer.tsxapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/connection.tsapps/ade-cli/src/tuiClient/drawerSelection.tsapps/ade-cli/src/tuiClient/eventDedup.tsapps/desktop/package.jsonapps/desktop/resources/agent-skills/ade-orchestrator/SKILL.mdapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/ai/tools/bashHardening.test.tsapps/desktop/src/main/services/ai/tools/orchestrationPlanQuality.tsapps/desktop/src/main/services/ai/tools/orchestrationRuntime.tsapps/desktop/src/main/services/ai/tools/orchestrationTools.test.tsapps/desktop/src/main/services/ai/tools/orchestrationTools.tsapps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/ai/tools/universalTools.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/cursorSdkPool.tsapps/desktop/src/main/services/chat/cursorSdkProtocol.tsapps/desktop/src/main/services/chat/cursorSdkWorker.tsapps/desktop/src/main/services/chat/droidSdkPool.tsapps/desktop/src/main/services/chat/droidSdkProtocol.tsapps/desktop/src/main/services/chat/droidSdkWorker.tsapps/desktop/src/main/services/cto/ctoStateService.tsapps/desktop/src/main/services/files/fileWatcherService.test.tsapps/desktop/src/main/services/files/fileWatcherService.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/opencode/openCodeRuntime.tsapps/desktop/src/main/services/orchestration/applyPatches.tsapps/desktop/src/main/services/orchestration/manifestNormalization.tsapps/desktop/src/main/services/orchestration/orchestrationService.test.tsapps/desktop/src/main/services/orchestration/orchestrationService.tsapps/desktop/src/main/services/orchestration/patchPolicy.test.tsapps/desktop/src/main/services/orchestration/patchPolicy.tsapps/desktop/src/main/services/orchestration/runtimeProfile.tsapps/desktop/src/main/services/processes/orphanedAgentProcessReaper.test.tsapps/desktop/src/main/services/processes/orphanedAgentProcessReaper.tsapps/desktop/src/main/services/projects/projectScaffoldService.test.tsapps/desktop/src/main/services/projects/projectScaffoldService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/orchestrationBridge.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/FeedbackReporterModal.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/automations/adeActionSchemas.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsxapps/desktop/src/renderer/components/chat/ChatComposerShell.tsxapps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsxapps/desktop/src/renderer/components/chat/ChatProposedPlanCard.tsxapps/desktop/src/renderer/components/chat/ModelPickerPendingInput.test.tsxapps/desktop/src/renderer/components/chat/OrchestratorLeadFrame.test.tsxapps/desktop/src/renderer/components/chat/OrchestratorLeadFrame.tsxapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/onboarding/ProjectSetupPage.tsxapps/desktop/src/renderer/components/orchestration/AnnotationFlow.test.tsxapps/desktop/src/renderer/components/orchestration/AnnotationPopover.tsxapps/desktop/src/renderer/components/orchestration/OrchestrationPanel.test.tsxapps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsxapps/desktop/src/renderer/components/orchestration/PanelChrome.tsxapps/desktop/src/renderer/components/orchestration/PhaseAccordion.tsxapps/desktop/src/renderer/components/orchestration/PlanMarkdown.test.tsxapps/desktop/src/renderer/components/orchestration/PlanMarkdown.tsxapps/desktop/src/renderer/components/orchestration/SpecPreviewCard.tsxapps/desktop/src/renderer/components/orchestration/TaskCard.tsxapps/desktop/src/renderer/components/orchestration/orchestrationDataSource.tsapps/desktop/src/renderer/components/orchestration/orchestrationTokens.tsapps/desktop/src/renderer/components/projects/CloneProjectForm.tsxapps/desktop/src/renderer/components/projects/PublishToGitHubDialog.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/run/RunPage.tsxapps/desktop/src/renderer/components/settings/GitHubSection.tsxapps/desktop/src/renderer/components/shared/ModelPicker/ModelPicker.test.tsxapps/desktop/src/renderer/components/shared/ModelPicker/ModelPicker.tsxapps/desktop/src/renderer/components/shared/ModelPicker/ModelPickerContent.tsxapps/desktop/src/renderer/components/terminals/OrchestratorComposerEntry.test.tsxapps/desktop/src/renderer/components/terminals/SessionCard.test.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/lib/openExternal.tsapps/desktop/src/renderer/lib/sessions.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/chatContextAttachments.test.tsapps/desktop/src/shared/chatContextAttachments.tsapps/desktop/src/shared/cliLaunch.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/git.tsapps/desktop/src/shared/types/orchestration.tsapps/desktop/src/shared/types/sessions.ts
Comment on lines
+11019
to
+11044
| function monitorRuntimeIdleExit( | ||
| states: readonly HeadlessRpcServerState[], | ||
| onIdle: () => void, | ||
| ): () => void { | ||
| const idleExitMs = readRuntimeIdleExitMs(); | ||
| if (idleExitMs == null) return () => {}; | ||
| let stopped = false; | ||
| let lastActiveAt = Date.now(); | ||
| const intervalMs = Math.max(1_000, Math.min(10_000, Math.floor(idleExitMs / 4))); | ||
| const check = () => { | ||
| if (stopped) return; | ||
| if (hasActiveHeadlessConnections(states)) { | ||
| lastActiveAt = Date.now(); | ||
| return; | ||
| } | ||
| if (Date.now() - lastActiveAt >= idleExitMs) { | ||
| onIdle(); | ||
| } | ||
| }; | ||
| const timer = setInterval(check, intervalMs); | ||
| timer.unref?.(); | ||
| return () => { | ||
| stopped = true; | ||
| clearInterval(timer); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Track idle time from real connection activity, not timer snapshots.
lastActiveAt is only refreshed when a connection happens to still be open during a polling tick. Short-lived CLI connections that open and close between checks never update that clock, so the daemon can exit much sooner than ADE_RUNTIME_IDLE_EXIT_MS after serving a request.
A safer approach is to store lastActivityAt on HeadlessRpcServerState and update it from createHeadlessRpcServer() on connection open/close, then have monitorRuntimeIdleExit() compare against that timestamp instead of sampled activeConnections.size.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ade-cli/src/cli.ts` around lines 11019 - 11044, The
monitorRuntimeIdleExit function currently samples connection state and can miss
short-lived connections; instead add a lastActivityAt timestamp/property to
HeadlessRpcServerState and update that timestamp from createHeadlessRpcServer
whenever a connection opens or closes (or when any request is served), then
change monitorRuntimeIdleExit to compute the most-recent activity by reading
states.map(s => s.lastActivityAt) (or taking the max) rather than relying on the
local lastActiveAt snapshot and hasActiveHeadlessConnections; keep the timer
logic but compare Date.now() against the most-recent lastActivityAt to decide
when to call onIdle and preserve stop/clearInterval behavior.
Comment on lines
328
to
+352
| try { | ||
| const result = spawnSync("gh", ["auth", "token"], { | ||
| encoding: "utf8", | ||
| timeout: 5_000, | ||
| }); | ||
| if (result.status !== 0) return null; | ||
| if (result.status !== 0) { | ||
| const stderr = typeof result.stderr === "string" ? result.stderr.trim() : ""; | ||
| return { | ||
| token: null, | ||
| ghCliPath: "gh", | ||
| ghAuthError: stderr || "GitHub CLI is installed, but `gh auth token` did not return a token.", | ||
| }; | ||
| } | ||
| const token = result.stdout?.trim() ?? ""; | ||
| return token.length > 0 ? token : null; | ||
| } catch { | ||
| return null; | ||
| return { | ||
| token: token.length > 0 ? token : null, | ||
| ghCliPath: "gh", | ||
| ghAuthError: token.length > 0 ? null : "GitHub CLI is installed, but `gh auth token` did not return a token.", | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| token: null, | ||
| ghCliPath: null, | ||
| ghAuthError: error instanceof Error ? error.message : String(error), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
const { spawnSync } = require('node:child_process');
const result = spawnSync('__definitely_missing_binary__', [], {
encoding: 'utf8',
});
console.log(JSON.stringify({
status: result.status,
errorCode: result.error && result.error.code,
errorMessage: result.error && result.error.message,
stdout: result.stdout,
stderr: result.stderr,
}, null, 2));
NODERepository: arul28/ADE
Length of output: 168
Guard against spawnSync("gh", ...).error when gh is missing
Node doesn’t throw when the executable is missing; spawnSync returns status: null with result.error (e.g., ENOENT). With the current if (result.status !== 0) logic, that will fall into the “GitHub CLI is installed, but … did not return a token” branch while still setting ghCliPath: "gh", making the returned status misleading on machines without gh.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ade-cli/src/headlessLinearServices.ts` around lines 328 - 352, The
branch that checks spawnSync("gh", ...) uses only result.status and assumes GH
exists; instead check result.error (and status === null) from spawnSync and
treat that as “gh missing” by returning ghCliPath: null and a ghAuthError
containing result.error.message (or String(result.error)); keep the existing
behavior when result.status is non-zero or stdout is empty but ensure you
reference spawnSync's result.error, result.status, token, ghCliPath and
ghAuthError to update the returned object accordingly so machines without the gh
binary are reported correctly.
Comment on lines
+477
to
+507
| const readToken = (): HeadlessGitHubTokenLookup => { | ||
| const patToken = readStoredPatToken(); | ||
| if (patToken) { | ||
| return { | ||
| token: patToken, | ||
| source: "pat", | ||
| patTokenStored: true, | ||
| ghCliPath: null, | ||
| ghAuthError: null, | ||
| }; | ||
| } | ||
| const env = envToken("ADE_GITHUB_TOKEN", "GITHUB_TOKEN", "GH_TOKEN"); | ||
| if (env) { | ||
| return { | ||
| token: env, | ||
| source: "environment", | ||
| patTokenStored: false, | ||
| ghCliPath: null, | ||
| ghAuthError: null, | ||
| }; | ||
| } | ||
| const gh = ghAuthToken(); | ||
| return { | ||
| ...gh, | ||
| source: gh.token ? "gh" : "none", | ||
| patTokenStored: false, | ||
| }; | ||
| }; | ||
|
|
||
| const getToken = (): string => readToken().token ?? ""; | ||
|
|
There was a problem hiding this comment.
Cache the GH CLI fallback instead of re-running it on every token read.
getToken() now calls readToken(), and the GH CLI branch shells out synchronously whenever PAT/env auth is absent. That means apiRequest, publishCurrentProject, and paginated GitHub calls can block the event loop and spawn gh auth token repeatedly for the same credential source. Memoize the GH lookup behind a short TTL or explicit invalidation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ade-cli/src/headlessLinearServices.ts` around lines 477 - 507, readToken
currently calls ghAuthToken on every invocation (via getToken) causing repeated
synchronous shells; cache the GH CLI result returned by ghAuthToken in an
in-memory structure with a short TTL (or an explicit invalidate function) and
make readToken/getToken consult that cache instead of re-running ghAuthToken
each time. Specifically, add a module-level cachedGh lookup (value + expiry) and
update readToken to return the cached gh result when not expired, call
ghAuthToken only on cache miss, and provide a way to invalidate the cache when
PAT/env changes or on explicit error; keep
token/source/patTokenStored/ghCliPath/ghAuthError shapes unchanged so callers of
readToken and getToken (and functions like apiRequest/publishCurrentProject) see
the same contract.
Comment on lines
+338
to
+355
| export function mergeOptimisticChatSessions( | ||
| sessions: AgentChatSessionSummary[], | ||
| optimisticSessions: Map<string, AgentChatSessionSummary>, | ||
| ): AgentChatSessionSummary[] { | ||
| if (optimisticSessions.size === 0) return sessions; | ||
| const seen = new Set(sessions.map((session) => session.sessionId)); | ||
| for (const sessionId of seen) { | ||
| optimisticSessions.delete(sessionId); | ||
| } | ||
| const pending = [...optimisticSessions.values()] | ||
| .filter((session) => !seen.has(session.sessionId)) | ||
| .sort((left, right) => { | ||
| const rightMs = Date.parse(right.lastActivityAt ?? right.startedAt); | ||
| const leftMs = Date.parse(left.lastActivityAt ?? left.startedAt); | ||
| return (Number.isFinite(rightMs) ? rightMs : 0) - (Number.isFinite(leftMs) ? leftMs : 0); | ||
| }); | ||
| return pending.length ? [...pending, ...sessions] : sessions; | ||
| } |
There was a problem hiding this comment.
Don't evict optimistic sessions while merging local state.
mergeOptimisticChatSessions() mutates optimisticSessions based on whatever sessions array it receives. When this runs from setSessions(current => ...), any already-rendered optimistic session gets deleted from the map before the backend has actually listed it. Creating chat A and then chat B before the next refresh reproduces it: A is removed from the map and disappears on the following refresh even if the server still hasn't returned it.
Suggested direction
export function mergeOptimisticChatSessions(
sessions: AgentChatSessionSummary[],
optimisticSessions: Map<string, AgentChatSessionSummary>,
): AgentChatSessionSummary[] {
if (optimisticSessions.size === 0) return sessions;
const seen = new Set(sessions.map((session) => session.sessionId));
- for (const sessionId of seen) {
- optimisticSessions.delete(sessionId);
- }
const pending = [...optimisticSessions.values()]
.filter((session) => !seen.has(session.sessionId))
.sort((left, right) => {
const rightMs = Date.parse(right.lastActivityAt ?? right.startedAt);
const leftMs = Date.parse(left.lastActivityAt ?? left.startedAt);
return (Number.isFinite(rightMs) ? rightMs : 0) - (Number.isFinite(leftMs) ? leftMs : 0);
});
return pending.length ? [...pending, ...sessions] : sessions;
}Then prune optimisticChatSessionsRef.current only against the authoritative listChatSessions() result inside refreshState().
📝 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
| export function mergeOptimisticChatSessions( | |
| sessions: AgentChatSessionSummary[], | |
| optimisticSessions: Map<string, AgentChatSessionSummary>, | |
| ): AgentChatSessionSummary[] { | |
| if (optimisticSessions.size === 0) return sessions; | |
| const seen = new Set(sessions.map((session) => session.sessionId)); | |
| for (const sessionId of seen) { | |
| optimisticSessions.delete(sessionId); | |
| } | |
| const pending = [...optimisticSessions.values()] | |
| .filter((session) => !seen.has(session.sessionId)) | |
| .sort((left, right) => { | |
| const rightMs = Date.parse(right.lastActivityAt ?? right.startedAt); | |
| const leftMs = Date.parse(left.lastActivityAt ?? left.startedAt); | |
| return (Number.isFinite(rightMs) ? rightMs : 0) - (Number.isFinite(leftMs) ? leftMs : 0); | |
| }); | |
| return pending.length ? [...pending, ...sessions] : sessions; | |
| } | |
| export function mergeOptimisticChatSessions( | |
| sessions: AgentChatSessionSummary[], | |
| optimisticSessions: Map<string, AgentChatSessionSummary>, | |
| ): AgentChatSessionSummary[] { | |
| if (optimisticSessions.size === 0) return sessions; | |
| const seen = new Set(sessions.map((session) => session.sessionId)); | |
| const pending = [...optimisticSessions.values()] | |
| .filter((session) => !seen.has(session.sessionId)) | |
| .sort((left, right) => { | |
| const rightMs = Date.parse(right.lastActivityAt ?? right.startedAt); | |
| const leftMs = Date.parse(left.lastActivityAt ?? left.startedAt); | |
| return (Number.isFinite(rightMs) ? rightMs : 0) - (Number.isFinite(leftMs) ? leftMs : 0); | |
| }); | |
| return pending.length ? [...pending, ...sessions] : sessions; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ade-cli/src/tuiClient/app.tsx` around lines 338 - 355,
mergeOptimisticChatSessions currently mutates the optimisticSessions Map (it
deletes keys based on the incoming sessions), which causes optimistic sessions
to be evicted prematurely when used inside setSessions updates; change
mergeOptimisticChatSessions so it does not mutate its optimisticSessions
argument (remove the optimisticSessions.delete loop and any in-place changes)
and instead only reads from it to produce the merged array (e.g., compute
pending from a non-mutating iteration or a shallow copy), and move any pruning
of optimisticChatSessionsRef.current to refreshState where you compare against
the authoritative listChatSessions() result and remove stale entries there.
Comment on lines
+2997
to
+3006
| const orchestrationService = createOrchestrationService({ | ||
| resolveLaneWorktree: (laneId: string): string | undefined => { | ||
| try { | ||
| return laneService.getLaneWorktreePath(laneId); | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| }, | ||
| }); | ||
| orchestrationServiceRef = orchestrationService; |
There was a problem hiding this comment.
Dispose orchestrationService when a project context closes.
This service is created per project and stored on AppContext, but disposeContextResources() never tears it down. That will leave orchestration watchers/subscriptions alive after project eviction or shutdown of a single context.
Suggested teardown
@@
try {
+ await ctx.orchestrationService?.dispose?.();
+ } catch {
+ // ignore
+ }
+ try {
await ctx.agentChatService.disposeAll();
} catch {
// ignore
}Also applies to: 4042-4042
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/main.ts` around lines 2997 - 3006, The
orchestrationService created via createOrchestrationService (stored in
orchestrationServiceRef) is not torn down when a project context closes; update
disposeContextResources() to call the orchestrationService's teardown API (e.g.,
orchestrationService.dispose() or orchestrationService.close()) and clear
orchestrationServiceRef to undefined when a context is evicted or shutdown so
watchers/subscriptions are removed; ensure you null-check
orchestrationServiceRef before calling the dispose method and handle errors
(try/catch) to avoid blocking context teardown.
Comment on lines
+227
to
+243
| if (op.path.includes("/tasks/{id:")) { | ||
| const taskId = extractTaskId(op.path); | ||
| if (taskId) { | ||
| const task = ctx.manifest.tasks.find((t) => t.id === taskId); | ||
| if ( | ||
| task && | ||
| task.assigneeSessionId && | ||
| task.assigneeSessionId !== ctx.actorSessionId | ||
| ) { | ||
| return { | ||
| allowed: false, | ||
| reason: `worker ${ctx.actorSessionId} does not own task ${taskId}`, | ||
| path: op.path, | ||
| }; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Require strict task ownership for worker task writes.
Line 233 only rejects when assigneeSessionId exists and differs, so unassigned (or missing) tasks are writable by any worker. That conflicts with the “tasks they have claimed” policy and weakens main-process patch authorization.
Suggested fix
if (ctx.actorRole === "worker") {
+ if (!ctx.actorSessionId) {
+ return {
+ allowed: false,
+ reason: "worker session id is required",
+ path: op.path,
+ };
+ }
if (matchesAny(parsed, WORKER_DENY_PATTERNS)) {
return {
allowed: false,
@@
if (op.path.includes("/tasks/{id:")) {
const taskId = extractTaskId(op.path);
if (taskId) {
const task = ctx.manifest.tasks.find((t) => t.id === taskId);
- if (
- task &&
- task.assigneeSessionId &&
- task.assigneeSessionId !== ctx.actorSessionId
- ) {
+ if (!task || task.assigneeSessionId !== ctx.actorSessionId) {
return {
allowed: false,
reason: `worker ${ctx.actorSessionId} does not own task ${taskId}`,
path: op.path,
};
}
}
}
return { allowed: true };
}As per coding guidelines, apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/services/orchestration/patchPolicy.ts` around lines 227
- 243, The current check allows writes to unassigned tasks because it only
rejects when task.assigneeSessionId exists and differs; update the logic in the
patch policy (the block using extractTaskId, ctx.manifest.tasks,
ctx.actorSessionId, task.assigneeSessionId and op.path) to reject any write
where the task exists and its assigneeSessionId is not exactly equal to
ctx.actorSessionId (treat undefined/null assigneeSessionId as "not owned"),
returning allowed: false with the same reason/path structure; ensure you still
only perform this check when extractTaskId(op.path) yields a taskId and a
matching task is found.
Comment on lines
+7177
to
+7179
| const activeOrchestrationRole = selectedSession?.orchestrationRole ?? null; | ||
| const isOrchestratorLead = selectedSession?.interactionMode === "orchestrator-lead"; | ||
| const isOrchestratorDraft = forceDraft && workDraftKind === "chat-orchestrator" && selectedSessionId == null; |
There was a problem hiding this comment.
Pass the real lead-session state into OrchestratorLeadFrame.
isOrchestratorLead is already derived above, but Line 7726 hard-codes active={false}. That means the lead chrome never appears for orchestrator-lead sessions.
Suggested fix
- <OrchestratorLeadFrame active={false} className="flex h-full min-h-0 w-full min-w-0 flex-col">
+ <OrchestratorLeadFrame active={isOrchestratorLead} className="flex h-full min-h-0 w-full min-w-0 flex-col">Also applies to: 7726-7726
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines
7177 - 7179, OrchestratorLeadFrame is being passed a hard-coded active={false}
which prevents the lead chrome from showing; change the prop to use the real
derived state (isOrchestratorLead or another appropriate boolean like
selectedSession?.interactionMode === "orchestrator-lead") so that
OrchestratorLeadFrame receives active={isOrchestratorLead} (or equivalent)
instead of false, ensuring the lead UI appears for orchestrator-lead sessions.
Comment on lines
+71
to
+80
| const initialProvider: AgentChatProvider = suggested?.provider ?? "claude"; | ||
| const initialModelId = suggested?.modelId ?? ""; | ||
| const [modelId, setModelId] = useState<string>(initialModelId); | ||
| const [provider, setProvider] = useState<AgentChatProvider>(initialProvider); | ||
| const [reasoningEffort, setReasoningEffort] = useState<string | null>( | ||
| suggested?.reasoningEffort ?? null, | ||
| ); | ||
| const [codexFastMode, setCodexFastMode] = useState<boolean>( | ||
| Boolean(suggested?.codexFastMode), | ||
| ); |
There was a problem hiding this comment.
Resync local selection state when suggested changes.
Lines 71-80 only seed state on first mount. If this card is reused with a new suggested value, the UI can submit stale provider/model settings.
Suggested patch
-import { memo, useCallback, useMemo, useState } from "react";
+import { memo, useCallback, useEffect, useMemo, useState } from "react";
...
const [codexFastMode, setCodexFastMode] = useState<boolean>(
Boolean(suggested?.codexFastMode),
);
+
+ useEffect(() => {
+ setProvider(suggested?.provider ?? "claude");
+ setModelId(suggested?.modelId ?? "");
+ setReasoningEffort(suggested?.reasoningEffort ?? null);
+ setCodexFastMode(Boolean(suggested?.codexFastMode));
+ }, [suggested]);As per coding guidelines, apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const initialProvider: AgentChatProvider = suggested?.provider ?? "claude"; | |
| const initialModelId = suggested?.modelId ?? ""; | |
| const [modelId, setModelId] = useState<string>(initialModelId); | |
| const [provider, setProvider] = useState<AgentChatProvider>(initialProvider); | |
| const [reasoningEffort, setReasoningEffort] = useState<string | null>( | |
| suggested?.reasoningEffort ?? null, | |
| ); | |
| const [codexFastMode, setCodexFastMode] = useState<boolean>( | |
| Boolean(suggested?.codexFastMode), | |
| ); | |
| import { memo, useCallback, useEffect, useMemo, useState } from "react"; | |
| const initialProvider: AgentChatProvider = suggested?.provider ?? "claude"; | |
| const initialModelId = suggested?.modelId ?? ""; | |
| const [modelId, setModelId] = useState<string>(initialModelId); | |
| const [provider, setProvider] = useState<AgentChatProvider>(initialProvider); | |
| const [reasoningEffort, setReasoningEffort] = useState<string | null>( | |
| suggested?.reasoningEffort ?? null, | |
| ); | |
| const [codexFastMode, setCodexFastMode] = useState<boolean>( | |
| Boolean(suggested?.codexFastMode), | |
| ); | |
| useEffect(() => { | |
| setProvider(suggested?.provider ?? "claude"); | |
| setModelId(suggested?.modelId ?? ""); | |
| setReasoningEffort(suggested?.reasoningEffort ?? null); | |
| setCodexFastMode(Boolean(suggested?.codexFastMode)); | |
| }, [suggested]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/components/chat/ChatModelSelectionPendingCard.tsx`
around lines 71 - 80, The component seeds local state only on mount
(initialProvider, initialModelId, useState for modelId, provider,
reasoningEffort, codexFastMode) so when the suggested prop updates the UI can
show stale values; add a useEffect that watches suggested and calls
setModelId(suggested?.modelId ?? ""), setProvider(suggested?.provider ??
"claude"), setReasoningEffort(suggested?.reasoningEffort ?? null), and
setCodexFastMode(Boolean(suggested?.codexFastMode)) to resync local selection
state whenever suggested changes.
Comment on lines
+192
to
+205
| useEffect(() => { | ||
| const off = ds.subscribe({ runId, laneId }, (payload) => { | ||
| // If a manifest event omits the full manifest (small patch), refresh. | ||
| if (payload.kind === "manifest" && !payload.manifest) { | ||
| ds.read({ runId, laneId }) | ||
| .then((res) => | ||
| dispatch({ | ||
| type: "load-success", | ||
| manifest: res.manifest, | ||
| planMd: res.planMd, | ||
| etag: res.etag, | ||
| }), | ||
| ) | ||
| .catch(() => undefined); |
There was a problem hiding this comment.
Guard event-triggered ds.read from stale async overwrite.
At Line 196, the fallback ds.read() can resolve after a newer refresh and still dispatch load-success, which may replace newer panel state with stale data.
Suggested fix
+ const eventReadSeq = useRef(0);
useEffect(() => {
+ let disposed = false;
const off = ds.subscribe({ runId, laneId }, (payload) => {
// If a manifest event omits the full manifest (small patch), refresh.
if (payload.kind === "manifest" && !payload.manifest) {
+ const seq = ++eventReadSeq.current;
ds.read({ runId, laneId })
- .then((res) =>
- dispatch({
- type: "load-success",
- manifest: res.manifest,
- planMd: res.planMd,
- etag: res.etag,
- }),
- )
+ .then((res) => {
+ if (disposed || seq !== eventReadSeq.current) return;
+ dispatch({
+ type: "load-success",
+ manifest: res.manifest,
+ planMd: res.planMd,
+ etag: res.etag,
+ });
+ })
.catch(() => undefined);
return;
}
+ if (disposed) return;
dispatch({ type: "event", payload });
});
- return off;
+ return () => {
+ disposed = true;
+ off();
+ };
}, [ds, runId, laneId]);As per coding guidelines apps/desktop/src/**: "Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."
📝 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
| useEffect(() => { | |
| const off = ds.subscribe({ runId, laneId }, (payload) => { | |
| // If a manifest event omits the full manifest (small patch), refresh. | |
| if (payload.kind === "manifest" && !payload.manifest) { | |
| ds.read({ runId, laneId }) | |
| .then((res) => | |
| dispatch({ | |
| type: "load-success", | |
| manifest: res.manifest, | |
| planMd: res.planMd, | |
| etag: res.etag, | |
| }), | |
| ) | |
| .catch(() => undefined); | |
| const eventReadSeq = useRef(0); | |
| useEffect(() => { | |
| let disposed = false; | |
| const off = ds.subscribe({ runId, laneId }, (payload) => { | |
| // If a manifest event omits the full manifest (small patch), refresh. | |
| if (payload.kind === "manifest" && !payload.manifest) { | |
| const seq = ++eventReadSeq.current; | |
| ds.read({ runId, laneId }) | |
| .then((res) => { | |
| if (disposed || seq !== eventReadSeq.current) return; | |
| dispatch({ | |
| type: "load-success", | |
| manifest: res.manifest, | |
| planMd: res.planMd, | |
| etag: res.etag, | |
| }); | |
| }) | |
| .catch(() => undefined); | |
| return; | |
| } | |
| if (disposed) return; | |
| dispatch({ type: "event", payload }); | |
| }); | |
| return () => { | |
| disposed = true; | |
| off(); | |
| }; | |
| }, [ds, runId, laneId]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/components/orchestration/OrchestrationPanel.tsx`
around lines 192 - 205, The fallback ds.read() can resolve out-of-order and
overwrite newer panel state; inside the useEffect subscription callback for
ds.subscribe (where payload.kind === "manifest" && !payload.manifest) protect
the async read by gating its dispatch: create a request token/counter or capture
the current runId/laneId/etag in a local variable before calling ds.read(), then
when the promise resolves compare that token/etag/current state to ensure the
response is still current; only then call dispatch({ type: "load-success",
manifest: res.manifest, planMd: res.planMd, etag: res.etag }). Use symbols:
useEffect, ds.subscribe, ds.read, dispatch, runId, laneId, payload, etag to find
and apply the guard so stale responses cannot overwrite newer state.
…dings CI fix: - Add settling delay in localRuntimeConnectionPool.test.ts to prevent EPIPE race during socket reconnection Review fixes: - Add agent-registration guard to claimTask (cursor + greptile P1) - Block lead from self-approving plan via manifestPatch deny patterns, add dedicated approvePlan service method (cursor High) - Clean up orphan sessions in spawnAgent on manifest patch failure (cursor High) - Add spawnSync error handling in headlessLinearServices (coderabbit Critical) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 26, 2026 03:50
Clean up orphaned chat session when manifest patch fails in the registerIpc spawn_agent handler (same pattern as orchestrationTools fix). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…194b # Conflicts: # apps/ade-cli/src/multiProjectRpcServer.ts # apps/ade-cli/src/tuiClient/connection.ts # apps/desktop/src/renderer/components/chat/AgentChatPane.tsx # apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx
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
1 participant
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
ade-orchestratorSKILL.md alongside bundled agent skillsTest plan
apps/desktopandapps/ade-cliapps/ade-clitest suite: 48 files, 808 tests passingapps/desktoptest suite: 326 files, 5380 tests passing (1 flaky socket test unrelated to this PR)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Greptile Summary
This PR introduces a complete multi-agent orchestration system for Work-tab chats, replacing the prior missions/orchestrator system. Orchestration runs are coordinated through a manifest-driven plan (lead/worker/validator roles), with etag-based optimistic concurrency, heartbeat monitoring, task leasing, and cancellation support exposed through both IPC and HTTP MCP servers for cross-runtime orchestration.
orchestrationService.ts,patchPolicy.ts,applyPatches.ts,manifestNormalization.ts): atomic manifest persistence, role-scoped patch policy, per-run mutexes, chokidar file watchers, and a lane-indexed run registry.orchestrationTools.ts,orchestrationRuntime.ts): lead/worker/validator tool factories with heartbeat wrappers, bash cancellation registration, and model routing; an HTTP MCP server bridges droid/cursor SDK sessions to the same tool surface.detached:truetodetached:falsewith parent-PID tracking, a SIGTERM→SIGKILL cleanup helper, and a new orphaned-process reaper for codex exec orphans reparented to init.Confidence Score: 5/5
Safe to merge; the orchestration core is well-structured with per-run mutexes, etag-based OCC, and role-scoped patch policy that correctly guard the main mutation paths.
The new orchestration system is large but thoroughly tested (981-test orchestrationTools suite, 5380 desktop tests passing). The claimTask unregistered-agent gap flagged in a prior review is fixed in this PR. The remaining findings are a dead lane-filter in runList and a misleading op:add label on the heartbeat event patch, neither of which affects correctness of the running system.
apps/desktop/src/main/services/orchestration/orchestrationService.ts — both minor issues are isolated to this file.
Important Files Changed
Sequence Diagram
sequenceDiagram participant R as Renderer participant IPC as registerIpc participant OS as OrchestrationService participant ACS as AgentChatService participant Lead as Lead Session participant Worker as Worker Session R->>IPC: orchestrationRunCreate(laneId, goal, title) IPC->>OS: runCreate(req) OS-->>IPC: runId manifest etag IPC->>ACS: setOrchestrationFields leadSessionId IPC-->>R: runId manifest R->>IPC: orchestrationSubscribe(runId, laneId) IPC->>OS: subscribe(runId, bundlePath) OS-->>IPC: ok Lead->>OS: planAppend(section, body) OS-->>Lead: planMd etag OS-)R: event kind plan planMd Lead->>OS: requestPlanApproval via manifestPatch OS-->>Lead: etag_conflict or ok R->>IPC: orchestrationManifestPatch user approves IPC->>OS: manifestPatch patches actorRole lead OS-->>IPC: ok etag OS-)R: event kind manifest Lead->>OS: spawnAgent role worker tag initialMessage OS->>ACS: createSession orchestrationMode orchestrator-worker ACS-->>OS: id workerSessionId OS->>OS: manifestPatch agents add sessionId role worker OS->>ACS: sendMessage workerSessionId initialMessage ACS-->>Worker: initial brief Worker->>OS: claimTask(taskId, leaseMs) OS->>OS: verify agent registered check lease OS-->>Worker: ok manifest etag OS-)R: event kind manifest loop Heartbeat Worker->>OS: agentHeartbeat(runId, sessionId) OS->>OS: update lastHeartbeatAt no etag bump OS-)R: event kind manifest patch replace lastHeartbeatAt end Worker->>OS: releaseTask(taskId, status done) OS->>OS: verify validation gates build phase transitions OS-->>Worker: manifest etag OS-)R: event kind manifestComments Outside Diff (1)
apps/desktop/src/main/services/ipc/registerIpc.ts, line 1 (link)actorRoleis trusted verbatim from the renderer inorchestrationManifestPatchThe IPC handler for
orchestrationManifestPatchforwardsarg(includingarg.actorRoleandarg.actorSessionId) directly toservice.manifestPatch()without any cross-validation against a known session record. A renderer that sendsactorRole: "lead"would pass the lead's broader patch-policy whitelist regardless of the caller's true session role. For a desktop Electron app with context isolation this risk is limited to compromised renderer code, but it leaves the role-enforcement boundary incomplete.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile