From 042125c71a3ca4d709df96aee51e974bef66e1bd Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Thu, 6 Aug 2026 11:19:20 -0400 Subject: [PATCH 1/6] docs(skills): make bundled skills describe ADE, not dictate how to work ADE hosts harnesses; it does not replace them. Every provider already ships its own operating instructions, so ADE's bundled skills adding a second layer of "how to think" competes with the harness the user chose. Strip generic workflow guidance from the bundled skills and keep only what is true and ADE-specific: what a command does, what it returns, and the facts an agent cannot infer from the repo. Destructive-operation notes stay, restated as facts about consequences rather than instructions about conduct. Co-Authored-By: Claude Opus 5 --- .../agent-skills/ade-app-control/SKILL.md | 6 +- .../agent-skills/ade-browser/SKILL.md | 6 +- .../ade-cli-control-plane/SKILL.md | 94 +++++-------------- .../agent-skills/ade-deeplinks/SKILL.md | 16 ++-- .../agent-skills/ade-ios-simulator/SKILL.md | 13 ++- .../agent-skills/ade-lanes-git/SKILL.md | 8 +- .../agent-skills/ade-linear/SKILL.md | 59 +++++------- .../agent-skills/ade-mosaic/SKILL.md | 9 +- .../agent-skills/ade-pr-workflows/SKILL.md | 86 +++++++---------- .../agent-skills/ade-proof-artifacts/SKILL.md | 3 +- .../agent-skills/ade-search/SKILL.md | 2 +- 11 files changed, 115 insertions(+), 187 deletions(-) diff --git a/apps/desktop/resources/agent-skills/ade-app-control/SKILL.md b/apps/desktop/resources/agent-skills/ade-app-control/SKILL.md index 02d240ad7..b8be117db 100644 --- a/apps/desktop/resources/agent-skills/ade-app-control/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-app-control/SKILL.md @@ -1,13 +1,13 @@ --- name: ade-app-control -description: Use this skill when inspecting, launching, logging, clicking, typing, or selecting context from Electron apps through ADE App Control and the `ade app-control` CLI. +description: Use this skill when you need to run or drive a local Electron/desktop app and capture what it does — launch it or attach to a running renderer, read its logs or answer its terminal prompts, click and type in it, or pull screenshot-backed DOM/source context into the chat — through `ade app-control`. --- # ADE App Control ## Use socket mode -App Control is a live desktop drawer service. Prefer socket-backed commands: +App Control is a live desktop drawer service, so every command below uses `--socket` (the general rule is in the **ade-cli-control-plane** skill): ```bash ade help app-control @@ -18,7 +18,7 @@ ade --socket app-control connect --cdp-port --text ``` ADE sets `ADE_APP_CONTROL_CDP_PORT` and `ADE_APP_CONTROL_DEBUG_FLAGS` for launches. Custom Electron launchers should forward one of those values to `--remote-debugging-port`. -ADE-launched agents pass `ADE_LANE_ID` / `ADE_CHAT_SESSION_ID` through `launch`, `connect`, and `claim`; use `claim` when attaching to a renderer that is already running so the Work tools pane attributes it to the agent's lane instead of the visible chat. +`launch`, `connect`, and `claim` all carry lane/session ownership; see "Owning a drawer surface" in the **ade-cli-control-plane** skill for when `claim` is required. ## Inspect diff --git a/apps/desktop/resources/agent-skills/ade-browser/SKILL.md b/apps/desktop/resources/agent-skills/ade-browser/SKILL.md index 4385bf3bc..a61d48a3e 100644 --- a/apps/desktop/resources/agent-skills/ade-browser/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-browser/SKILL.md @@ -1,6 +1,6 @@ --- name: ade-browser -description: Use this skill when using ADE's built-in browser pane, shared browser tabs, screenshots, page inspection, or browser context selection through `ade browser`. +description: Use this skill for any browser behavior at all — opening a URL, checking a localhost page, clicking or filling a form, logging in, screenshotting, inspecting the DOM, or verifying a page renders — before reaching for an external browser or tool. ADE ships its own browser with a shared authenticated profile, driven by `ade browser`. --- # ADE browser @@ -9,7 +9,7 @@ description: Use this skill when using ADE's built-in browser pane, shared brows The ADE browser uses one persistent, machine-global authentication profile per ADE installation/channel. Cookies, cache, local storage, IndexedDB, service workers, and normal authenticated session state are shared across ADE projects. Visible tabs are not global: each ADE window/project keeps an independent tab collection, and personal chat keeps a separate personal collection. Use socket mode so CLI calls route to the correct project collection; the bridge carries the runtime project root for tab routing, never for storage partitioning. If a task needs any browser behavior — opening a URL, checking localhost, clicking, filling a form, logging in, screenshotting, inspecting DOM, or verifying a page — use `ade --socket browser ...` before trying an external browser/tool. -Within a project, attribution is per tab and per chat agent. `ade browser open`, `new-tab`, `claim`, browser sessions, and agent page actions carry `ADE_LANE_ID` / `ADE_CHAT_SESSION_ID` automatically when ADE launched the agent, and each `BuiltInBrowserTab` reports `ownerLaneId`, `ownerChatSessionId`, `ownerClaimedAt`, and `ownerLeaseExpiresAt`. `ade browser panel` and plain `ade browser switch` are passive view operations, so they must not claim or block the Browser view just because another lane owns the active tab. If you attach to an already-open tab, run `ade --socket browser claim --tab --lane --text` first so the tab strip shows the right owner lane. +Within a project, attribution is per tab and per chat agent: `ade browser open`, `new-tab`, `claim`, browser sessions, and agent page actions all carry lane/session ownership (see "Owning a drawer surface" in the **ade-cli-control-plane** skill), and each `BuiltInBrowserTab` reports `ownerLaneId`, `ownerChatSessionId`, `ownerClaimedAt`, and `ownerLeaseExpiresAt`. The browser-specific claim form is `ade --socket browser claim --tab --lane --text`. `ade browser panel` and plain `ade browser switch` are passive view operations, so they must not claim or block the Browser view just because another lane owns the active tab. ## How `ade browser` reaches the desktop @@ -79,7 +79,6 @@ ade --socket browser clear-selection --text - For repeated agent work, start a browser session with `ade --socket browser session start --tab --text`, then use either `--browser-session ` on observe/click/fill/clear-field/press/wait/trace/proof/screenshot/reload/back/forward/stop/select or the shorthand `ade --socket browser session ...`. The session is a lightweight pointer to one tab plus owner/last observation/last trace metadata; it ends explicitly with `browser session end ` or automatically when the tab closes. - Browser click/select/scroll coordinates are viewport coordinates. Prefer `click --selector`, `click --text-match`, `click --test-id`, `click --element `, or `click --handle ` from the current DOM list when available; ADE scrolls located elements into view before dispatching the click. - Use `wait`, `fill`, `clear-field`, and `press` for Playwright-like agent actions. These commands focus located elements and reject disabled targets; `wait` can target selectors/text/test ids, URL substrings, or load state. `wait --network-idle` waits for `document.readyState === "complete"`, no pending browser requests, and a quiet window controlled by `--network-idle-ms` (default 500). -- Prefer `observe --map --text` before precise work. The text output gives numbered elements plus `handle` values; use `--handle` for the next click/fill/press/wait so ADE does not have to infer from brittle coordinates. - Lane/chat-owned browser tabs are leased. ADE injects the launched agent's browser actor capability plus `ADE_LANE_ID` and `ADE_CHAT_SESSION_ID` into status, session, observation, screenshot, trace, navigation-control, selection, and page-action calls. A bound agent cannot impersonate another owner or use `--force`; recovery takeovers and global administration stay on trusted human renderer paths. Passive panel reveal stays usable without claiming the active tab. - A global authenticated profile is a security boundary. Every non-local origin, and any local origin with an allowed privileged permission, requires a native human grant per chat/lane before an agent can inspect or control it. Use `browser authorize` when a synchronous status/session command asks for approval; navigation commands request the same approval directly. Grants are memory-only and disappear on ADE restart. Agent-triggered cross-origin navigation and redirects are stopped at the boundary; approved normal navigations may continue, while approved redirects require retrying the initiating action so ADE never blindly replays a redirected request. Sensitive popups are blocked until the agent navigates there explicitly and the human approves. - Browser tab URLs and the active index restore from a bounded machine-local store. Agent leases, lightweight browser sessions, and session-cookie values are never synthesized or restored by that store, so a restarted agent must claim a tab again and sites retain control over logout/expiry semantics. @@ -92,4 +91,3 @@ ade --socket browser clear-selection --text - Project browser observations are scratch files under `.ade/cache/browser-observations/`; personal-tab observations use the current ADE channel's machine-local `browser-observations/personal/` scratch root. Both prune aggressively to the latest 3 observations per tab by default. Use `--no-dom` for image-only scratch captures and proof commands only for reviewer-facing evidence. - Use `ade --socket browser proof --tab --caption "..."` or `ade --socket browser proof --browser-session --caption "..."` to promote a fresh browser observation into the durable proof drawer. - Signed packaged macOS builds embed ADE's Developer ID provisioning profile and enable Electron's Touch ID WebAuthn authenticator with the matching keychain access-group entitlement; source builds require `ADE_ENABLE_TOUCH_ID_WEBAUTHN=1` for explicit testing, and `ADE_ENABLE_TOUCH_ID_WEBAUTHN=0` disables it. These credentials are device-bound to the Mac's Secure Enclave and the global Electron browser session; they are not iCloud-synced passkeys. ADE does not claim Apple Passwords extension support. -- If there is no active browser panel/session, report the blocker rather than pretending to inspect the page. diff --git a/apps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.md b/apps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.md index ead454b8c..e3b433851 100644 --- a/apps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.md @@ -1,6 +1,6 @@ --- name: ade-cli-control-plane -description: Use this skill when an agent needs to inspect or operate ADE itself through the `ade` CLI, including lanes, chats, actions, proof, runtime/socket state, or help/flag discovery. +description: Use this skill when you need to hand work to another ADE agent, wait for or check whether delegated work has finished, read another agent's chat transcript, report status or raise a question on the Work row, schedule a wake-up for later, or spawn a child lane — and generally whenever you need ADE's own state (lanes, chats, actions, proof, secrets, runtime/socket status) through the `ade` CLI. --- # ADE CLI control plane @@ -44,38 +44,23 @@ ade actions list --domain project_secret --text ## Socket mode -Use `--socket` when the CLI and ADE desktop drawer must share live state. This matters for App Control, iOS Simulator, Preview Lab, browser tabs, terminal logs, context selection, and proof drawer updates. +Use `--socket` when the CLI and ADE desktop drawer must share live state. This matters for App Control, iOS Simulator, Preview Lab, browser tabs, terminal logs, context selection, and proof drawer updates. The **ade-app-control**, **ade-ios-simulator**, and **ade-browser** skills all assume it. + +### Owning a drawer surface + +ADE injects `ADE_LANE_ID` and `ADE_CHAT_SESSION_ID` into every agent it launches, and the drawer services (App Control, iOS Simulator, browser) carry them so the Work tools pane attributes what you drive to your lane rather than to the visible chat. When you *attach to something already running* instead of starting it yourself, run that surface's `claim` subcommand first — `ade --socket app-control claim`, `ade --socket ios-sim claim`, `ade --socket browser claim` — or Work will keep showing the previous owner. ## Runtime daemon vs. desktop bridge Most domains (`lane`, `git`, `chat`, `app_control`, `ios_simulator`, etc.) run **inside the runtime daemon** at `~/.ade/sock/ade.sock` and work whether or not the desktop is open. -A small set of domains require the **desktop bridge** because the underlying service needs real Electron APIs. Today that is just `built_in_browser` (it owns a `WebContentsView`), but expect the list to grow if more Electron-only services get exposed to the CLI. The runtime forwards these calls over `/sock/desktop-bridge.sock` (override with `ADE_DESKTOP_BRIDGE_SOCKET_PATH`). - -When no desktop is running, calls into a bridge-backed domain surface as `Domain unavailable` or `Desktop browser bridge not running at . Open ADE Desktop with a project to enable \`ade browser\` commands.` — report the blocker and continue with the rest of the control plane, which is unaffected. +A small set of domains require the **desktop bridge** because the underlying service needs real Electron APIs. Today that is just `built_in_browser`; the **ade-browser** skill documents that hop, its socket path, and the exact failure text when no desktop is running. A bridge-backed domain with no desktop also surfaces as `Domain unavailable` — report the blocker and continue with the rest of the control plane, which is unaffected. ## Linear issues attached to your session -See the **ade-linear** skill for the full read/write workflow on an attached issue; the essentials: - -When ADE launches you with an attached Linear issue, it injects two env vars into your session: `ADE_CHAT_SESSION_ID` (your session) and `ADE_LINEAR_ISSUE_IDS` (comma-separated attached issue ids). You read and write that issue through the **daemon bridge** — `ade linear ...` routes over the daemon to the desktop runtime, which holds the Linear credentials. You never need a Linear token. - -Read/write your attached issue (id defaults to your session's first attached issue, so you can omit it): +The **ade-linear** skill is the home for working an attached Linear issue: the env vars ADE injects, the read/write commands, and attach/detach. You never need a Linear token — `ade linear ...` routes over the daemon to the desktop runtime, which holds the credentials. Two attachment commands live outside that skill's `ade linear` surface: ``` -ade linear issues --this-session --text # what is attached to me -ade linear issue --text # read the attached issue -ade linear comment "Pushed a fix; CI running" -ade linear set-state ENG-431 # move workflow state -ade linear assign ENG-431 -ade linear label ENG-431 needs-review -``` - -Manage attachments: - -``` -ade linear attach --this-session --issue-id ENG-431 # attach to my session -ade linear detach --this-session [--issue-id ENG-431] # detach one or all ade chat attach-linear-issue --issue-id ENG-431 ade lanes link-linear-issue --linear-issue-json '{...}' ``` @@ -116,13 +101,9 @@ through `ADE_CHAT_SESSION_ID`. The type is required for every parented agent spawn; omitting it is a hard error whose message includes the decision rule. There is no silent `none` type. -- Use `subagent` whenever the parent will need, join, read, or review the - result. Parallel fan-out that later joins is subagent work. -- Use `peer` only for fire-and-forget work whose result the parent does not - expect to join, read, or review. Peer turns leave quiet completion notes and - do not wake the parent. -- Use `--no-parent` only for a genuinely independent top-level session. Do not - use it to avoid choosing a type. +The type controls what happens to the parent when the child finishes: a +`subagent` turn wakes or steers the parent, a `peer` turn only leaves a quiet +completion note. `--no-parent` creates an unparented top-level session. For persistent Work chats, every parent-dispatched subagent turn reports back with its child turn id and latest assistant summary. ADE steers an active parent @@ -224,47 +205,24 @@ What to do instead when you finish: say so in your final message, and use `ade chat note ""` to leave a durable status line on the Work row. If you are blocked, `ade chat ask ""` raises the row's hand. -#### Work-row status protocol +#### What `note` and `ask` do to the Work row -Treat the status line and hand-raise as separate signals: +They are two separate signals on the row the user is looking at: -- **`ade chat note` explains the current state.** Write one concrete, - present-tense summary aiming for **6 words or fewer** — a guideline, not a - hard limit. ADE truncates past **72 characters**, so put the decisive state - first and never write a full sentence, but a long note still beats no note. +- **`ade chat note` sets the row's status line.** It shows a durable one-line + summary of the current state; it does not change the row's phase. Length + guidance and the hard display bound live in + `apps/desktop/src/shared/sessionStatusNote.ts` (a note past that bound is + truncated with an ellipsis, so put the decisive state first). Good: `CI green; awaiting Codex review` Bad: `Working`, `Still looking`, `Blocked`, or `Done`. -- **`ade chat ask` means work cannot continue without a user answer.** Ask the - exact question that unlocks the next action. Include the meaningful choices - and consequence when there is a tradeoff. -- **When user input blocks progress, call `note` and then `ask`.** The note - preserves the operational context; the ask raises the Work row to **Needs - you**. A note alone never changes the canonical phase and an idle row can - otherwise appear **Done**. -- **The next accepted user message clears the prior hand-raise.** While the - reply is being handled, the row should be **Working**, not **Needs you**. If - the reply resolves the blocker, continue normally. If it does not, leave an - updated note and call `ask` again with the exact information still missing - before ending the turn. -- **Do not use `ask` for external waiting.** If CI, review, a build, or another - service is still running and no user action is required, leave a specific - note and either keep polling or snooze the session. -- **Do not ask the user to classify a failure the agent can investigate.** - Diagnose and recover autonomously. Raise an ask only when recovery requires - new authority, unavailable credentials, or a product choice. - -| Situation | Required action | Example | -|---|---|---| -| Actively working | `note` when the phase materially changes | `Fixing live branch refresh` | -| Waiting on external work | `note`, then poll or snooze | `PR #977 CI running` | -| Blocked on user input | `note`, then `ask` | Note: `Waiting for migration choice` Ask: `Use the reversible in-place migration, or create a new store and copy records?` | -| Recoverable error | `note`, investigate, continue | `Rerunning timed-out desktop shard` | -| Unrecoverable error needing user action | `note`, then `ask` | Note: `Writable GitHub credential missing` Ask: `Authenticate gh, or should I use the stored PAT?` | -| Delivered | final response plus `note` | `PR #977 merged; fixes shipped` | - -Before ending any non-delivered turn, ask: **Can useful work continue without -the user?** If yes, continue or snooze—do not hand-raise. If no, ensure both a -specific note and an exact ask were sent. +- **`ade chat ask` raises the row's hand**, moving it to **Needs you**. Because + a note alone never changes the phase, an idle row with only a note can read as + **Done** — when you are genuinely blocked on the user, call `note` for the + context and then `ask` for the exact question. +- **The next accepted user message clears the hand-raise** and the row returns + to **Working** while the reply is handled. If the reply does not unblock you, + leave an updated note and `ask` again. Snooze is the lifecycle verb you *do* own. The typed family takes the session id as a positional, also accepts `--session`, and falls back to @@ -366,8 +324,6 @@ ade shell start-cli codex --lane --model --prompt "Fix" # `minimal`, `low`, `medium`, `high`, `xhigh`, `max`, and `ultracode`; confirm model-specific support with `ade actions run chat.modelCatalog --json`. -Report what you actually did back to the issue with `ade linear comment` as you progress — that comment is how reviewers and the issue's watchers see status. Use `ade help linear` for the full flag set. - ## Fallback path If `command -v ade` fails: diff --git a/apps/desktop/resources/agent-skills/ade-deeplinks/SKILL.md b/apps/desktop/resources/agent-skills/ade-deeplinks/SKILL.md index 09ab89251..7fb43aecd 100644 --- a/apps/desktop/resources/agent-skills/ade-deeplinks/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-deeplinks/SKILL.md @@ -198,17 +198,21 @@ ade link linear-issue ADE-123 --no-clipboard ade linear comment ADE-123 "Created via ADE. Open in ADE: " ``` -Agents should still include a user-facing ADE PR link when handing off a newly -created or adopted PR. Use the GitHub PR URL for the browser link and the -`adeUrl` printed by `ade prs create`. If the PR came from another path, mint -the ADE link with: +### PR closeout links + +When handing off a newly created or adopted PR, include both links: the GitHub +PR URL (`githubUrl` / `html_url`) and the ADE PR link. `ade prs create` prints +the ADE link as `adeUrl`; for a PR that came from another path, mint it: ```bash ade link pr --no-clipboard ``` -Use that output for the ADE link. Prefer the default HTTPS form in chat and terminal output -because it is clickable, shareable, and upgrades into the ADE PRs tab. +Prefer the default HTTPS form in chat, PR comments, and terminal output: it is +clickable and shareable, and on a machine with ADE installed it upgrades into +`ade://pr///` and lands in the PRs tab. The PR body also +gets the automatic "Open in ADE" footer, but that footer is not a substitute for +the links in your final message. When you copy a deeplink from the desktop UI, the lane right-click menu offers Copy lane link, Copy branch link (cross-machine), Copy PR link, and Copy diff --git a/apps/desktop/resources/agent-skills/ade-ios-simulator/SKILL.md b/apps/desktop/resources/agent-skills/ade-ios-simulator/SKILL.md index 70630e5e5..bbf108d15 100644 --- a/apps/desktop/resources/agent-skills/ade-ios-simulator/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-ios-simulator/SKILL.md @@ -1,13 +1,13 @@ --- name: ade-ios-simulator -description: Use this skill when working with ADE iOS Simulator, Preview Lab, SwiftUI preview rendering, simulator screenshots, taps, streams, or iOS drawer context via `ade ios-sim`. +description: Use this skill when you need to see an iOS or SwiftUI change actually running on a simulator — launching the app, tapping/dragging/typing in it, screenshotting or streaming the screen, inspecting on-screen elements, or rendering a SwiftUI preview through Preview Lab — via `ade ios-sim`. --- # ADE iOS Simulator and Preview Lab ## Start here -Use socket mode so CLI actions and the desktop drawer share one simulator session: +Use `--socket` so CLI actions and the desktop drawer share one simulator session (the general rule is in the **ade-cli-control-plane** skill): ```bash ade --socket ios-sim status --text @@ -23,7 +23,7 @@ Launch with a target from `apps`: ade --socket ios-sim launch --target --text ``` -ADE-launched agents pass `ADE_LANE_ID` / `ADE_CHAT_SESSION_ID` through `launch` and `claim`; use `claim` when taking over an already-running simulator drawer so Work shows the lane that actually owns the visible simulator content. +`launch` and `claim` carry lane/session ownership; see "Owning a drawer surface" in the **ade-cli-control-plane** skill for when `claim` is required. ## Inspect and interact @@ -73,11 +73,10 @@ To bridge the current simulator screen into Preview Lab, first select a source-b Use `preview-match` when you only need the target decision without rendering. The selected simulator element's `sourceFile` and optional `sourceLine` bias matching; `--label` / `--component-id` are only hints for naming a missing-preview suggestion. Use `preview-ensure` when Xcode Preview Lab is not ready; it opens this lane's iOS project in Xcode and waits for MCP readiness. -Add or refine a preview only when no useful nearby preview exists or the match is too far from the selected element. Preview fixtures must not require live sync, keychain, network, push, sockets, or production databases. +Preview Lab fixtures must not require live sync, keychain, network, push, sockets, or production databases — add or refine a preview only when no useful nearby preview exists. ## Gotchas -- Do not create symlink projects, fake schemes, or repo-layout shims as the first fix for app detection. Re-run `ade --socket ios-sim apps --text` and report the selected project, scheme, and build output. -- If `preview-current` or `preview-match` returns `no-context`, do not guess the screen from stale code. Run `ade --socket ios-sim snapshot --text`, select a source-backed element, or pass an explicit source file/line. -- If no simulator/session/snapshot exists, report the exact blocker instead of guessing the screen. +- `apps` drives project/scheme detection. If it does not find your app, re-run `ade --socket ios-sim apps --text` and report the selected project, scheme, and build output rather than working around it with symlink projects, fake schemes, or repo-layout shims. +- `preview-current` / `preview-match` returning `no-context` means nothing on screen is source-backed. Run `ade --socket ios-sim snapshot --text` and select a source-backed element, or pass an explicit `--source` / `--line`. - When you own the simulator session and the task no longer needs it, run `ade --socket ios-sim shutdown --text`. diff --git a/apps/desktop/resources/agent-skills/ade-lanes-git/SKILL.md b/apps/desktop/resources/agent-skills/ade-lanes-git/SKILL.md index c0c5cd578..8cb7517aa 100644 --- a/apps/desktop/resources/agent-skills/ade-lanes-git/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-lanes-git/SKILL.md @@ -28,6 +28,10 @@ ade lanes child --lane --name --text ade lanes archive --text ``` +`archive` is destructive to a user's workspace: it retires the lane and its +worktree, taking any uncommitted work in it with them. Archive a lane only when +the user asked for cleanup, or a workflow that owns lane lifecycle requires it. + ## ADE-aware Git Use ADE git commands when the operation should update ADE operation state and refresh lane status: @@ -50,10 +54,10 @@ ade git conflict show --lane --text ade git rebase continue --lane --text ``` -For conflicts, inspect both sides and preserve intent from both branches. Do not blindly accept ours/theirs. +`--ai` runs the rebase with ADE's AI-assisted conflict support; +`ade git conflict show` prints ADE's view of the conflicted files. ## Gotchas - Use `--lane` for anything other than the active workspace. - Use `ade diff changes --lane --text` when you need ADE's view of file changes. -- Do not archive or delete lanes unless the user asked for cleanup or the release workflow explicitly requires it. diff --git a/apps/desktop/resources/agent-skills/ade-linear/SKILL.md b/apps/desktop/resources/agent-skills/ade-linear/SKILL.md index 81c5582ce..70e911f5d 100644 --- a/apps/desktop/resources/agent-skills/ade-linear/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-linear/SKILL.md @@ -101,29 +101,16 @@ Notes: ## ADE deeplinks in Linear comments -ADE posts deterministic Linear attachments/cards for these flows when the -runtime owns the Linear connection: +ADE posts its own Linear attachments/cards for lane, chat, and PR flows. The +**ade-deeplinks** skill holds the matrix of which flow ADE covers automatically +and which one leaves the link to you, plus the `ade link ...` commands and the +HTTPS-vs-`ade://` guidance. -| Flow | ADE handles it | -| --- | --- | -| Create a lane from a Linear issue | Lane attachment + one-time ADE branch comment | -| Attach a Linear issue to an existing lane | Lane attachment + one-time ADE branch comment | -| Create or attach a chat/CLI session with a Linear issue | ADE chat attachment | -| Open/create a PR from a linked lane | ADE PR attachment/footer | - -For direct issue actions (`comment`, `set-state`, `assign`, `label`, `graphql`) -include the relevant ADE link in any user-facing Linear comment you write, -especially when the action creates new Linear state or hands work to a human. -Use the **ade-deeplinks** skill to mint links: - -```bash -ade link linear-issue ENG-431 --no-clipboard -ade link branch --no-clipboard -ade link session "$ADE_CHAT_SESSION_ID" --lane --no-clipboard -ade link pr --no-clipboard -``` - -Example when creating a new Linear issue via GraphQL: +The short version for this skill's commands: the direct issue actions +(`comment`, `set-state`, `assign`, `label`, `graphql`) are the ones ADE does +*not* link for you, so include the relevant ADE link yourself in any user-facing +Linear comment — especially when the action creates new Linear state or hands +work to a human. ```bash ade linear graphql --query-file create-issue.graphql --variables-file vars.json @@ -138,19 +125,21 @@ ade linear detach --this-session --issue-id ENG-431 # detach one issue ade linear detach --this-session # detach every issue from your session ``` -## Recommended workflow - -1. When you start real work on the issue, move it to **In Progress** - (`ade linear set-state `), so watchers see it's being worked. -2. As you make progress, **comment** what you did and link the PR - (`ade linear comment "..."`). Include ADE branch/session/PR links when a card - was not already posted automatically. That comment is how reviewers and the - issue's watchers see status — report what you actually did, not what you - intend to do. -3. When you finish, set the **appropriate final state** (e.g. Done / In Review). - Defer the exact final-state policy to the user's workflow — if you're unsure - whether to mark Done vs. In Review, comment your result and ask rather than - guessing. +Two commands outside the `ade linear` surface also attach an issue: +`ade chat attach-linear-issue --issue-id ` and +`ade lanes link-linear-issue --linear-issue-json '{...}'`. + +## What lands where + +An `ade linear comment` is the issue's status channel — it is what reviewers and +the issue's watchers see, and it is visible to people who never open ADE. The +workflow state you set with `set-state` is what shows in the team's board. +Neither is inferred from your ADE activity; if you do not write them, the issue +does not move. + +`set-state` takes a workflow state id, and the valid states are per-team, so +resolve the id with `ade linear picker-data --text` rather than assuming a +Done/In Review convention. ## Discovery diff --git a/apps/desktop/resources/agent-skills/ade-mosaic/SKILL.md b/apps/desktop/resources/agent-skills/ade-mosaic/SKILL.md index 7957415cc..b1ab5dd46 100644 --- a/apps/desktop/resources/agent-skills/ade-mosaic/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-mosaic/SKILL.md @@ -1,6 +1,6 @@ --- name: ade-mosaic -description: Use this skill when you want to ask the user for structured input (choices, toggles, numbers, short text, approvals) inside an ADE chat. Emit a fenced mosaic code block and ADE renders it as a native interactive card; the user's answers come back as your next user message. Prefer a mosaic card over prose bullet-lists of options whenever the answer is a selection or a small form. +description: Use this skill when you want to ask the user for structured input (choices, toggles, numbers, short text, approvals) inside an ADE chat. Emit a fenced mosaic code block and ADE renders it as a native interactive card; the user's answers come back as your next user message. --- # ADE Mosaic cards @@ -60,9 +60,8 @@ Limits: 40 options per select, 50 table rows, labels ≤ 200 chars, values ## Guidance -- One card per decision. Don't stack multiple cards in one reply. -- Keep it small: a card is for a decision, not a settings page. -- Always include at least one interactive element; use `text`/`table` rows - for the context the user needs to decide. +- `text` and `table` are display-only, so a card needs at least one + interactive element to be submittable; use the display rows for the context + the user needs to decide. - Do NOT use a mosaic card for plan approval or permission prompts — ADE has dedicated native surfaces for those. diff --git a/apps/desktop/resources/agent-skills/ade-pr-workflows/SKILL.md b/apps/desktop/resources/agent-skills/ade-pr-workflows/SKILL.md index 3c569a6a8..abe259e6e 100644 --- a/apps/desktop/resources/agent-skills/ade-pr-workflows/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-pr-workflows/SKILL.md @@ -1,6 +1,6 @@ --- name: ade-pr-workflows -description: Use this skill when working with ADE PR workflows including PR tab data, GitHub stacked PRs, checks/comments, rebase resolution, CI fixes, or merge readiness. +description: Use this skill when working with ADE PR workflows including PR tab data, GitHub stacked PRs, checks/comments, rebase resolution, CI fixes, or landing a PR. --- # ADE PR workflows @@ -16,35 +16,11 @@ ade prs comments --text Use `ade help prs` and `ade help git rebase` before guessing PR or rebase flags. -## Offer stacked PRs for layered work +## GitHub stacked PRs -When a request naturally divides into two or more dependency-ordered, -independently reviewable changes, offer a GitHub stack before implementation. -If the user accepts and has not already chosen how to run it, ask which -execution model they prefer: - -- **One lane per PR** — create a child ADE lane and owning agent for every - layer. Recommend this when layers are still changing or several agents will - edit in parallel, because each PR gets an isolated worktree and ship loop. -- **Coordinated delegation** — one lead keeps the branch chain and delegates - only stable, non-overlapping work to subagents. Recommend this for a tightly - coupled stack where one agent should own rebases and commits. - -Then: - -1. Describe the layers bottom to top and keep foundations below their consumers. -2. Create one deliberate branch and PR per layer. -3. Run focused quality and tests for each layer before starting the next one. -4. Create the GitHub stack only after every PR base matches the previous PR's - head branch. -5. Run the ship loop for every PR. Fix a root-layer failure once, then rebase - and repoll every layer above it instead of applying duplicate fixes. -6. Apply review feedback to the lowest layer that owns the behavior, then - rebase every layer above it. -7. Keep one stack progress card current and report readiness bottom to top. - -Use ADE's built-in commands; users and agents do not need the `gh-stack` -extension: +A stack is two or more dependency-ordered, independently reviewable PRs. ADE +creates and tracks them through built-in commands; users and agents do not need +the `gh-stack` extension: ```bash ade prs stacks list --text @@ -54,24 +30,34 @@ ade prs stacks add --stack 8 --pulls 123 --text ade prs stacks unstack --stack 8 --text ``` -GitHub owns stack membership, review requirements, rebases performed on -GitHub, merge queue state, and final merging. Send final review and merge -decisions to GitHub instead of using ADE's legacy synchronous merge path. +Mechanics specific to ADE stacks: + +1. Order the layers bottom to top and keep foundations below their consumers. +2. Create one deliberate branch and PR per layer. Each layer can own a child + ADE lane (`ade lanes child`) and its own agent, so every layer gets an + isolated worktree and ship loop; alternatively one lead keeps the whole + branch chain and delegates only non-overlapping work. +3. `ade prs stacks create` expects every PR base to already match the previous + PR's head branch — create the stack only after the bases line up. +4. Fix a root-layer failure once, then rebase and repoll every layer above it + instead of applying duplicate fixes. +5. Report readiness bottom to top from one stack progress view. + +GitHub owns stack membership, review requirements, rebases performed on GitHub, +merge queue state, and final merging. ADE enforces this: `ade prs land` (the +`pr.land` action) refuses any PR that ADE knows is in a GitHub stack, failing +with `github_stack_requires_github_merge` and "PR #N is in GitHub Stack #M. +Review and merge the stack on GitHub." For an unstacked PR, `land` still merges +directly via `gh pr merge`. So send review and merge decisions for a stacked PR +to GitHub. ## PR creation closeout links -When you create or adopt a GitHub PR, include two links in your final handoff: - -- GitHub PR: use the PR's `githubUrl` / `html_url`. -- ADE PR: use the `adeUrl` printed by `ade prs create`; if you created or - adopted the PR through another path, run `ade link pr - --no-clipboard` and include the printed `https://ade-app.dev/open?...` URL. - -Prefer the HTTPS ADE link in chat, PR comments, and terminal output because it -unfurls and upgrades into `ade://pr///` on machines with -ADE installed. The PR body already gets an automatic "Open in ADE" footer, but -the final agent message should still include both links so the user can jump -straight to either GitHub or the ADE PRs tab. +When you create or adopt a GitHub PR, include both the GitHub URL +(`githubUrl` / `html_url`) and the ADE PR link in your final handoff. The +`adeUrl` printed by `ade prs create` is the ADE link; for a PR adopted through +another path, mint it as described in the **ade-deeplinks** skill, which is also +where the HTTPS-vs-`ade://` guidance lives. ## Use actions for niche surfaces @@ -80,13 +66,7 @@ ade actions list --domain pr --text ade actions run --input-json '{"key":"value"}' ``` -## Resolver rules - -- Preserve both the lane's intent and main's intent during conflicts. -- Read conflict files and surrounding call sites before choosing a side. -- For review-thread or CI work, fetch current checks/comments first; do not rely on stale PR tab state. -- Prefer focused fixes and rerun the smallest relevant check before escalating to broader validation. - -## Release readiness +## Freshness -Before treating a PR as merge-ready, verify working tree cleanliness, pushed branch status, required checks, unresolved review threads, and whether rebasing/merging main introduced conflicts or semantic drift. +For review-thread or CI work, fetch current checks/comments with +`ade prs checks` / `ade prs comments` first; the PR tab snapshot can be stale. diff --git a/apps/desktop/resources/agent-skills/ade-proof-artifacts/SKILL.md b/apps/desktop/resources/agent-skills/ade-proof-artifacts/SKILL.md index a9e25eb8e..e3f673e6f 100644 --- a/apps/desktop/resources/agent-skills/ade-proof-artifacts/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-proof-artifacts/SKILL.md @@ -30,6 +30,5 @@ ade help proof ## Gotchas - Do not leave proof as an unregistered local file when the user expects ADE to show it. -- ADE browser observations are scratch state under `.ade/cache/browser-observations/`; promote only reviewer-facing checkpoints with `ade --socket browser proof ...` or `ade proof attach`. +- Browser observations are scratch state, not proof; promote only reviewer-facing checkpoints with `ade --socket browser proof ...` or `ade proof attach`. The **ade-browser** skill documents where those scratch files live and how aggressively they prune. - Include enough context in the artifact name/description to understand what was verified. -- Clean up stale processes you started before declaring proof complete. diff --git a/apps/desktop/resources/agent-skills/ade-search/SKILL.md b/apps/desktop/resources/agent-skills/ade-search/SKILL.md index 57b540580..05dd006a3 100644 --- a/apps/desktop/resources/agent-skills/ade-search/SKILL.md +++ b/apps/desktop/resources/agent-skills/ade-search/SKILL.md @@ -110,7 +110,7 @@ on a usage error such as an unknown `--kind`. `ade search "Z" --lane --text` across files, commits, and chats. - **Index looks stale or empty** → `ade search --status --text` to see `docCount`, per-kind counts, and `backfillComplete`; rebuild with - `ade search --rebuild` (CTO-only) only if backfill is genuinely broken. + `ade search --rebuild` only if backfill is genuinely broken. Reach for `ade search` before grepping `.ade/` internals or asking the user to locate something. Remember the boundary: chats span registered projects; From b94b3acdefc1d113289a34cd96cb371de9c14136 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Thu, 6 Aug 2026 11:19:29 -0400 Subject: [PATCH 2/6] fix(orchestrator): enforce the lead read-only rule on every provider An orchestrator lead may run on any provider the user picks, but only Claude's native tool names were denied. A Codex, Cursor, Droid, or OpenCode lead kept full edit and shell access while holding danger-full-access with approvals set to never, so the "leads never touch files" rule held on exactly one runtime. Add provider-native deny lists in orchestrationRuntimePolicy and apply them at each runtime's own gate: Cursor loses the setting layers that carry writable tools and MCP servers, OpenCode gets only ADE's managed lease, and Droid's protocol denies its editing tools. MCP calls on a gated lead fail closed. Known gap: OpenCode project-level config is not yet isolated. The MCP map ADE hands it is gated, but a project config it reads on its own is not. Co-Authored-By: Claude Opus 5 --- .../services/chat/agentChatService.test.ts | 441 +++++++++++++++++- .../main/services/chat/agentChatService.ts | 116 ++++- .../services/chat/cursorSdkPolicy.test.ts | 49 ++ .../src/main/services/chat/cursorSdkPolicy.ts | 47 +- .../main/services/chat/cursorSdkPool.test.ts | 5 + .../src/main/services/chat/cursorSdkPool.ts | 2 + .../main/services/chat/cursorSdkProtocol.ts | 6 + .../src/main/services/chat/cursorSdkWorker.ts | 4 +- .../services/chat/droidSdkProtocol.test.ts | 56 +++ .../main/services/chat/droidSdkProtocol.ts | 33 ++ .../src/main/services/chat/droidSdkWorker.ts | 28 ++ .../services/opencode/openCodeRuntime.test.ts | 36 ++ .../main/services/opencode/openCodeRuntime.ts | 16 +- .../shared/orchestrationRuntimePolicy.test.ts | 66 +++ .../src/shared/orchestrationRuntimePolicy.ts | 257 ++++++++++ 15 files changed, 1138 insertions(+), 24 deletions(-) create mode 100644 apps/desktop/src/main/services/chat/droidSdkProtocol.test.ts diff --git a/apps/desktop/src/main/services/chat/agentChatService.test.ts b/apps/desktop/src/main/services/chat/agentChatService.test.ts index 414d7fec4..dc8342d87 100644 --- a/apps/desktop/src/main/services/chat/agentChatService.test.ts +++ b/apps/desktop/src/main/services/chat/agentChatService.test.ts @@ -12,6 +12,7 @@ import { resolveOpenCodeExecutablePath, startOpenCodeSession, } from "../opencode/openCodeRuntime"; +import { cursorSdkSettingSources, evaluateCursorSdkHook, summarizeCursorHook } from "./cursorSdkPolicy"; import { openKvDb } from "../state/kvDb"; import { createCtoStateService } from "../cto/ctoStateService"; import { createCtoMemoryService } from "../cto/ctoMemoryService"; @@ -289,12 +290,22 @@ vi.mock("../ai/codexExecutable", () => ({ resolveCodexExecutable: vi.fn(() => ({ path: "codex", source: "fallback-command" })), })); -vi.mock("../opencode/openCodeRuntime", () => ({ +vi.mock("../opencode/openCodeRuntime", async () => { + const { orchestrationLeadOpenCodeToolSelection } = await vi.importActual< + typeof import("../../../shared/orchestrationRuntimePolicy") + >("../../../shared/orchestrationRuntimePolicy"); + return { buildOpenCodePromptParts: vi.fn(({ prompt, files = [] }: { prompt: string; files?: Array> }) => [ { type: "text", text: prompt }, ...files, ]), - refreshOpenCodeSessionToolSelection: vi.fn(async () => null), + // Faithful stand-in for the real resolver: the E2E assertions below check the + // `tools` map that actually reaches OpenCode's prompt body, and + // openCodeRuntime.test.ts covers the real implementation. + refreshOpenCodeSessionToolSelection: vi.fn(async ( + _handle: unknown, + options?: { orchestrationLead?: boolean }, + ) => (options?.orchestrationLead ? orchestrationLeadOpenCodeToolSelection() : null)), mapPermissionModeToOpenCodeAgent: vi.fn((mode: string) => { if (mode === "plan") return "ade-plan"; if (mode === "full-auto") return "ade-full-auto"; @@ -537,7 +548,8 @@ vi.mock("../opencode/openCodeRuntime", () => ({ } })(); }), -})); + }; +}); vi.mock("../opencode/openCodeInventory", () => ({ clearOpenCodeInventoryCache: vi.fn(), @@ -7157,9 +7169,11 @@ describe("createAgentChatService", () => { expect(toolNames).not.toContain("writeFile"); expect(toolNames).not.toContain("bash"); expect(dynamicTools.every((entry: { namespace?: string }) => entry.namespace === "ade_orchestration")).toBe(true); + // Codex exposes no tool allow/deny list, so the lead's "no edits, no + // shell" invariant is carried by the thread sandbox instead. expect(startPayload?.params).toMatchObject({ approvalPolicy: "never", - sandbox: "danger-full-access", + sandbox: "read-only", }); const spawnCall = vi.mocked(spawn).mock.calls.find((call) => call[0] === "codex" && Array.isArray(call[1]) && call[1].includes("app-server") @@ -38564,3 +38578,422 @@ describe("explicit provider-thread continuity recovery", () => { expect(readPersistedChatState(none.id).continuityRecovery).toMatchObject({ state: "required", reason: "unknown" }); }); }); + +// --------------------------------------------------------------------------- +// Orchestrator-lead provider-native tool denial +// +// The lead plans and delegates; it never edits code or runs shell. ADE's own +// orchestration toolset already withholds editFile/writeFile/bash from leads, +// but that toolset is additive — it rides alongside each provider's built-in +// tools. Asserting only that ADE's tools are absent proves nothing, so each +// test below pins the provider-native denial ADE actually sends, and checks a +// worker on the SAME provider and the SAME permissive profile still has it. +// --------------------------------------------------------------------------- + +describe("orchestrator-lead provider-native tool denial", () => { + const leadArgs = (created: { runId: string; manifest: { bundlePath: string } }) => ({ + interactionMode: "orchestrator-lead" as const, + orchestrationRunId: created.runId, + orchestrationRole: "lead" as const, + orchestrationBundlePath: created.manifest.bundlePath, + }); + const workerArgs = (created: { runId: string; manifest: { bundlePath: string } }) => ({ + interactionMode: "orchestrator-worker" as const, + orchestrationRunId: created.runId, + orchestrationRole: "worker" as const, + orchestrationBundlePath: created.manifest.bundlePath, + }); + + it("Claude: denies the SDK's own Edit/Write/Bash/Task tools for a lead only", async () => { + const { orchestrationService, created } = await createLoadedOrchestrationRun("S-lead"); + try { + const { service } = createService({ getOrchestrationService: () => orchestrationService }); + + const lead = await service.createSession({ + laneId: "lane-1", + provider: "claude", + model: "claude-opus-4-5", + ...leadArgs(created), + }); + await service.sendMessage({ sessionId: lead.id, text: "Plan the work." }, { awaitDispatch: true }); + + const leadOptions = vi.mocked(query).mock.calls.at(-1)?.[0]?.options as + { disallowedTools?: string[]; canUseTool?: Function } | undefined; + expect(leadOptions?.disallowedTools).toEqual( + expect.arrayContaining(["Bash", "Edit", "MultiEdit", "Write", "NotebookEdit", "Task", "Agent"]), + ); + // Belt-and-braces: the runtime gate denies the same tools mid-turn. + await expect(leadOptions?.canUseTool?.("Write", { file_path: "README.md" }, {})) + .resolves.toMatchObject({ behavior: "deny" }); + await expect(leadOptions?.canUseTool?.("Bash", { command: "echo hi > README.md" }, {})) + .resolves.toMatchObject({ behavior: "deny" }); + + vi.mocked(query).mockClear(); + const worker = await service.createSession({ + laneId: "lane-1", + provider: "claude", + model: "claude-opus-4-5", + ...workerArgs(created), + }); + await service.sendMessage({ sessionId: worker.id, text: "Do the work." }, { awaitDispatch: true }); + + const workerOptions = vi.mocked(query).mock.calls.at(-1)?.[0]?.options as + { disallowedTools?: string[]; canUseTool?: Function } | undefined; + expect(workerOptions?.disallowedTools ?? []).not.toContain("Write"); + expect(workerOptions?.disallowedTools ?? []).not.toContain("Bash"); + await expect(workerOptions?.canUseTool?.("Write", { file_path: "README.md" }, {})) + .resolves.not.toMatchObject({ behavior: "deny" }); + } finally { + await orchestrationService.dispose(); + } + }); + + it("Codex: starts a lead thread read-only with approvals off, workers full-access", async () => { + const { orchestrationService, created } = await createLoadedOrchestrationRun("S-lead"); + try { + const { service } = createService({ getOrchestrationService: () => orchestrationService }); + + const lead = await service.createSession({ + laneId: "lane-1", + provider: "codex", + model: "gpt-5.4", + ...leadArgs(created), + }); + await service.sendMessage({ sessionId: lead.id, text: "Plan the work." }); + await vi.waitFor(() => { + expect(mockState.codexRequestPayloads.some((p) => p.method === "thread/start")).toBe(true); + }); + + // Codex's app-server exposes no tool allow/deny list, so `apply_patch` + // and `shell` cannot be removed from the model's toolset. The sandbox is + // the enforcement point: read-only blocks every write those tools make, + // and `approvalPolicy: never` stops the lead escalating past it. + const leadStart = mockState.codexRequestPayloads.find((p) => p.method === "thread/start") as any; + expect(leadStart?.params).toMatchObject({ approvalPolicy: "never", sandbox: "read-only" }); + + // The per-turn policy resolves through the same path, so a turn cannot + // re-grant write access after the thread starts. + await vi.waitFor(() => { + expect(mockState.codexRequestPayloads.some((p) => p.method === "turn/start")).toBe(true); + }); + const leadTurn = mockState.codexRequestPayloads.find((p) => p.method === "turn/start") as any; + expect(leadTurn?.params?.approvalPolicy).toBe("never"); + expect(leadTurn?.params?.sandboxPolicy?.type).toBe("readOnly"); + + mockState.codexRequestPayloads.length = 0; + const worker = await service.createSession({ + laneId: "lane-1", + provider: "codex", + model: "gpt-5.4", + ...workerArgs(created), + }); + await service.sendMessage({ sessionId: worker.id, text: "Do the work." }); + await vi.waitFor(() => { + expect(mockState.codexRequestPayloads.some((p) => p.method === "thread/start")).toBe(true); + }); + const workerStart = mockState.codexRequestPayloads.find((p) => p.method === "thread/start") as any; + expect(workerStart?.params).toMatchObject({ approvalPolicy: "never", sandbox: "danger-full-access" }); + } finally { + await orchestrationService.dispose(); + } + }); + + it("Droid: withholds Droid's own edit/execute tool categories from a lead only", async () => { + const { orchestrationService, created } = await createLoadedOrchestrationRun("S-lead"); + try { + const { service } = createService({ getOrchestrationService: () => orchestrationService }); + + const lead = await service.createSession({ + laneId: "lane-1", + provider: "droid", + model: "custom:claude-sonnet-5-thinking-32000", + modelId: "droid/custom:claude-sonnet-5-thinking-32000", + ...leadArgs(created), + }); + await service.sendMessage({ sessionId: lead.id, text: "Plan the work." }, { awaitDispatch: true }); + + // `disabledToolCategories` is resolved to concrete `disabledToolIds` + // against Droid's live tool list in the worker (see + // droidSdkProtocol.test.ts) — ids are build-specific, categories are not. + expect(mockState.droidAcquireCalls.at(-1)?.settings).toMatchObject({ + disabledToolCategories: ["edit", "execute"], + }); + expect(mockState.droidPromptCalls.at(-1)?.settings).toMatchObject({ + disabledToolCategories: ["edit", "execute"], + }); + + const worker = await service.createSession({ + laneId: "lane-1", + provider: "droid", + model: "custom:claude-sonnet-5-thinking-32000", + modelId: "droid/custom:claude-sonnet-5-thinking-32000", + ...workerArgs(created), + }); + await service.sendMessage({ sessionId: worker.id, text: "Do the work." }, { awaitDispatch: true }); + expect(mockState.droidPromptCalls.at(-1)?.settings) + .not.toHaveProperty("disabledToolCategories"); + } finally { + await orchestrationService.dispose(); + } + }); + + it("OpenCode: switches off OpenCode's own bash/edit/write/patch tools for a lead only", async () => { + vi.mocked(streamText).mockImplementation(() => ({ + fullStream: (async function* () { + yield { type: "finish", usage: {} }; + })(), + } as any)); + const { orchestrationService, created } = await createLoadedOrchestrationRun("S-lead"); + try { + const { service } = createService({ getOrchestrationService: () => orchestrationService }); + + const lead = await service.createSession({ + laneId: "lane-1", + provider: "opencode", + model: "", + modelId: "opencode/openai/gpt-5.4", + ...leadArgs(created), + }); + await service.sendMessage({ sessionId: lead.id, text: "Plan the work." }, { awaitDispatch: true }); + + const leadState = [...mockState.openCodeSessions.values()].at(-1)!; + await vi.waitFor(() => { + expect(leadState.promptBodies.length).toBeGreaterThan(0); + }); + expect(leadState.promptBodies.at(-1)?.tools).toMatchObject({ + bash: false, + edit: false, + write: false, + patch: false, + task: false, + }); + + const worker = await service.createSession({ + laneId: "lane-1", + provider: "opencode", + model: "", + modelId: "opencode/openai/gpt-5.4", + ...workerArgs(created), + }); + await service.sendMessage({ sessionId: worker.id, text: "Do the work." }, { awaitDispatch: true }); + + const workerState = [...mockState.openCodeSessions.values()].at(-1)!; + await vi.waitFor(() => { + expect(workerState.promptBodies.length).toBeGreaterThan(0); + }); + // No `tools` field at all: the worker keeps OpenCode's full default set. + expect(workerState.promptBodies.at(-1)).not.toHaveProperty("tools"); + } finally { + await orchestrationService.dispose(); + } + }); +}); + +// --------------------------------------------------------------------------- +// Orchestrator-lead MCP isolation +// +// Provider-native tool denials only cover each provider's built-in tools. A +// user-configured MCP server (filesystem, shell, git, …) hands the same +// capability back through another door, so a lead must see ADE-managed servers +// only. Each test pins the MCP configuration ADE actually sends for a lead and +// checks a worker on the SAME provider still receives the user's servers. +// Droid is deliberately absent: it has no session-scoped MCP restriction (see +// ORCHESTRATION_LEAD_MCP_ISOLATION.droid). +// --------------------------------------------------------------------------- + +describe("orchestrator-lead MCP isolation", () => { + const leadArgs = (created: { runId: string; manifest: { bundlePath: string } }) => ({ + interactionMode: "orchestrator-lead" as const, + orchestrationRunId: created.runId, + orchestrationRole: "lead" as const, + orchestrationBundlePath: created.manifest.bundlePath, + }); + const workerArgs = (created: { runId: string; manifest: { bundlePath: string } }) => ({ + interactionMode: "orchestrator-worker" as const, + orchestrationRunId: created.runId, + orchestrationRole: "worker" as const, + orchestrationBundlePath: created.manifest.bundlePath, + }); + + it("Claude: ignores ~/.claude.json and project .mcp.json for a lead only", async () => { + const { orchestrationService, created } = await createLoadedOrchestrationRun("S-lead"); + try { + const { service } = createService({ getOrchestrationService: () => orchestrationService }); + + const lead = await service.createSession({ + laneId: "lane-1", + provider: "claude", + model: "claude-opus-4-5", + ...leadArgs(created), + }); + await service.sendMessage({ sessionId: lead.id, text: "Plan the work." }, { awaitDispatch: true }); + + const leadOptions = vi.mocked(query).mock.calls.at(-1)?.[0]?.options as { + strictMcpConfig?: boolean; + mcpServers?: Record; + managedSettings?: { allowManagedMcpServersOnly?: boolean; allowedMcpServers?: Array<{ serverName: string }> }; + } | undefined; + // strictMcpConfig drops every on-disk MCP source; the managed allow-list + // keeps ADE's own programmatic server reachable. + expect(leadOptions?.strictMcpConfig).toBe(true); + expect(leadOptions?.managedSettings?.allowManagedMcpServersOnly).toBe(true); + expect(leadOptions?.managedSettings?.allowedMcpServers?.map((entry) => entry.serverName)) + .toContain("ade-orchestration"); + expect(Object.keys(leadOptions?.mcpServers ?? {})).toEqual(["ade-orchestration"]); + + vi.mocked(query).mockClear(); + const worker = await service.createSession({ + laneId: "lane-1", + provider: "claude", + model: "claude-opus-4-5", + ...workerArgs(created), + }); + await service.sendMessage({ sessionId: worker.id, text: "Do the work." }, { awaitDispatch: true }); + + const workerOptions = vi.mocked(query).mock.calls.at(-1)?.[0]?.options as { + strictMcpConfig?: boolean; + settingSources?: string[]; + } | undefined; + // The worker keeps the user's MCP servers: no strict flag, and the + // setting sources that load them are still on. + expect(workerOptions?.strictMcpConfig).toBeUndefined(); + expect(workerOptions?.settingSources).toEqual(expect.arrayContaining(["user", "project"])); + } finally { + await orchestrationService.dispose(); + } + }); + + it("Codex: switches off every configured MCP server for a lead only", async () => { + const { orchestrationService, created } = await createLoadedOrchestrationRun("S-lead"); + try { + const signedClient = codexComputerUseClientCandidates(path.join(tmpHomeRoot, ".codex"))[0]!; + const { service } = createService({ + getOrchestrationService: () => orchestrationService, + resolveCodexComputerUseMcp: async () => ({ command: signedClient, args: ["mcp"], enabled: true }), + resolveCodexConfiguredMcpServerNames: () => ["filesystem", "computer_use"], + }); + + const lead = await service.createSession({ + laneId: "lane-1", + provider: "codex", + model: "gpt-5.4", + ...leadArgs(created), + }); + await service.sendMessage({ sessionId: lead.id, text: "Plan the work." }); + await vi.waitFor(() => { + expect(mockState.codexRequestPayloads.some((p) => p.method === "thread/start")).toBe(true); + }); + + // Codex merges this overlay into config.toml rather than replacing it, so + // the isolation has to name each server. Computer Use is an MCP server + // too, so the lead does not get it either. + const leadStart = mockState.codexRequestPayloads.find((p) => p.method === "thread/start") as any; + expect(leadStart?.params?.config?.mcp_servers).toEqual({ + filesystem: { enabled: false }, + computer_use: { enabled: false }, + }); + + mockState.codexRequestPayloads.length = 0; + const worker = await service.createSession({ + laneId: "lane-1", + provider: "codex", + model: "gpt-5.4", + ...workerArgs(created), + }); + await service.sendMessage({ sessionId: worker.id, text: "Do the work." }); + await vi.waitFor(() => { + expect(mockState.codexRequestPayloads.some((p) => p.method === "thread/start")).toBe(true); + }); + const workerStart = mockState.codexRequestPayloads.find((p) => p.method === "thread/start") as any; + // Nothing disabled: the worker keeps the user's Codex MCP config intact + // (only ADE's own Computer Use merge is present). + expect(workerStart?.params?.config?.mcp_servers).toEqual({ + computer_use: { command: signedClient, args: ["mcp"], enabled: true }, + }); + } finally { + await orchestrationService.dispose(); + } + }); + + it("Cursor: runs a lead without the MCP-carrying setting layers, and denies MCP calls", async () => { + process.env.CURSOR_API_KEY = "cursor-test-key"; + const { orchestrationService, created } = await createLoadedOrchestrationRun("S-lead"); + try { + const { service } = createService({ getOrchestrationService: () => orchestrationService }); + const lead = await service.createSession({ + laneId: "lane-1", + provider: "cursor", + model: "composer-2", + modelId: "cursor/composer-2", + ...leadArgs(created), + }); + await service.sendMessage({ sessionId: lead.id, text: "Plan the work." }, { awaitDispatch: true }); + + // The worker turns `policy.orchestrationLead` into the trimmed + // `local.settingSources` (cursorSdkSettingSources); the pool call is the + // last thing ADE controls in this process. + const leadAcquire = mockState.cursorSdkAcquireCalls.at(-1) as any; + expect(leadAcquire?.policy?.orchestrationLead).toBe(true); + expect(cursorSdkSettingSources(leadAcquire?.policy)).toEqual(["user", "team", "mdm"]); + // MCP servers ADE hands Cursor are its own lease only. + expect(Object.keys(leadAcquire?.mcpServers ?? {})).toEqual(["ade-orchestration"]); + + // Cursor routes MCP tool calls through the same preToolUse gate as any + // other tool, named `MCP:` — an unknown risk class, denied. + const mcpCall = summarizeCursorHook( + { toolName: "MCP:write_file", toolInput: { path: "README.md" } }, + "/tmp/lane", + ); + expect(mcpCall.risk).toBe("unknown"); + expect(evaluateCursorSdkHook({ + request: mcpCall, + policy: { ...(leadAcquire?.policy as any) }, + laneRoot: "/tmp/lane", + userHomeDir: tmpHomeRoot, + })).toBe("deny"); + + const worker = await service.createSession({ + laneId: "lane-1", + provider: "cursor", + model: "composer-2", + modelId: "cursor/composer-2", + ...workerArgs(created), + }); + await service.sendMessage({ sessionId: worker.id, text: "Do the work." }, { awaitDispatch: true }); + const workerAcquire = mockState.cursorSdkAcquireCalls.at(-1) as any; + expect(workerAcquire?.policy?.orchestrationLead).toBe(false); + expect(cursorSdkSettingSources(workerAcquire?.policy)).toEqual(["all"]); + } finally { + await orchestrationService.dispose(); + } + }); + + it("OpenCode: hands every session an ADE-authored config with ADE's servers only", async () => { + vi.mocked(streamText).mockImplementation(() => ({ + fullStream: (async function* () { + yield { type: "finish", usage: {} }; + })(), + } as any)); + const { orchestrationService, created } = await createLoadedOrchestrationRun("S-lead"); + try { + const { service } = createService({ getOrchestrationService: () => orchestrationService }); + const lead = await service.createSession({ + laneId: "lane-1", + provider: "opencode", + model: "", + modelId: "opencode/openai/gpt-5.4", + ...leadArgs(created), + }); + await service.sendMessage({ sessionId: lead.id, text: "Plan the work." }, { awaitDispatch: true }); + + const leadStart = vi.mocked(startOpenCodeSession).mock.calls.at(-1)?.[0] as any; + // The MCP map ADE hands OpenCode for a lead carries ADE's lease only. + // `buildOpenCodeConfig`'s own behavior (that this map becomes the whole + // config the server sees) is covered directly in openCodeRuntime.test.ts + // — it cannot be asserted here because this suite mocks that module. + expect(Object.keys(leadStart?.mcp ?? {})).toEqual(["ade-orchestration"]); + } finally { + await orchestrationService.dispose(); + } + }); +}); diff --git a/apps/desktop/src/main/services/chat/agentChatService.ts b/apps/desktop/src/main/services/chat/agentChatService.ts index 83896e4f2..640c23ccd 100644 --- a/apps/desktop/src/main/services/chat/agentChatService.ts +++ b/apps/desktop/src/main/services/chat/agentChatService.ts @@ -607,8 +607,12 @@ import { CURSOR_AVAILABLE_MODE_IDS } from "../../../shared/cursorModes"; import { getApiKey } from "../ai/apiKeyStore"; import type { createOrchestrationService } from "../orchestration/orchestrationService"; import { + ORCHESTRATION_LEAD_CODEX_POLICY, ORCHESTRATION_LEAD_DENIED_CLAUDE_TOOLS, + ORCHESTRATION_LEAD_DENIED_DROID_TOOL_CATEGORIES, applyOrchestrationPermissionProfile, + codexConfiguredMcpServerNames, + orchestrationLeadCodexMcpOverrides, isOrchestrationInteractionMode, isOrchestrationLeadSession, lockedOrchestrationPermissionMode, @@ -3220,17 +3224,27 @@ function codexServiceTierArgs(session: AgentChatSession): { serviceTier: CodexSe function codexThreadConfigArgs( reasoningEffort: string | null | undefined, computerUse: CodexComputerUseMcpConfig | null = null, + /** + * Orchestrator leads plan and delegate — they get no MCP at all. Codex merges + * this overlay into the user's config.toml instead of replacing it, so the + * isolation is an explicit `enabled = false` per configured server (see + * ORCHESTRATION_LEAD_MCP_ISOLATION.codex). Computer Use is an MCP server too, + * so a lead does not receive it either. + */ + leadConfiguredMcpServerNames: readonly string[] | null = null, ): { config?: Record } { const effort = typeof reasoningEffort === "string" ? reasoningEffort.trim() : ""; - if (!effort && !computerUse) return {}; + const leadMcpOverrides = leadConfiguredMcpServerNames + ? orchestrationLeadCodexMcpOverrides(leadConfiguredMcpServerNames) + : null; + const mcpServers = leadMcpOverrides + ? (Object.keys(leadMcpOverrides).length ? leadMcpOverrides : null) + : (computerUse ? { computer_use: computerUse } : null); + if (!effort && !mcpServers) return {}; return { config: { ...(effort ? { model_reasoning_effort: effort } : {}), - ...(computerUse ? { - mcp_servers: { - computer_use: computerUse, - }, - } : {}), + ...(mcpServers ? { mcp_servers: mcpServers } : {}), }, }; } @@ -5949,8 +5963,15 @@ function enforceOrchestrationLockedPermissionMode( : null; // Use the per-provider profile directly — each provider has its own // "most permissive" mode name (bypassPermissions, danger-full-access, - // full-auto, auto-high, etc.). The lead's security comes from the tool-set - // restriction, not the permission mode. + // full-auto, auto-high, etc.). + // + // This profile is intentionally permissive for EVERY orchestration role, + // leads included. A lead's protection is a per-provider tool deny applied at + // the runtime boundary — Claude `canUseTool`/`disallowedTools`, OpenCode's + // prompt `tools` map, Droid's `disabledToolIds`, Cursor's hook risk gate, and + // (Codex having no tool knob) a read-only thread sandbox. Narrowing this + // profile instead was considered and rejected; see + // shared/orchestrationRuntimePolicy.ts for the per-provider deny sets. const profile = applyOrchestrationPermissionProfile(session.provider); Object.assign(session, profile); session.permissionMode = lockedMode; @@ -6723,6 +6744,12 @@ export function createAgentChatService(args: { | CodexComputerUseMcpConfig | null | Promise; + /** + * Names of the MCP servers the user configured in Codex's `config.toml`. + * Orchestrator leads carry an explicit `enabled = false` for each of them + * (see ORCHESTRATION_LEAD_MCP_ISOLATION.codex). + */ + resolveCodexConfiguredMcpServerNames?: () => readonly string[] | Promise; claudeSubprocessReaper?: ClaudeSubprocessReaper; createScheduledWorkScheduler?: typeof createChatScheduledWorkScheduler; onEvent?: (event: AgentChatEventEnvelope) => void; @@ -6771,6 +6798,7 @@ export function createAgentChatService(args: { getAdeCliAgentEnv, getLocalGitHubToken, resolveCodexComputerUseMcp: resolveCodexComputerUseMcpOverride, + resolveCodexConfiguredMcpServerNames: resolveCodexConfiguredMcpServerNamesOverride, claudeSubprocessReaper: injectedClaudeSubprocessReaper, createScheduledWorkScheduler = createChatScheduledWorkScheduler, onEvent, @@ -6782,6 +6810,30 @@ export function createAgentChatService(args: { } = args; const resolveCodexComputerUseMcp = resolveCodexComputerUseMcpOverride ?? resolveCodexComputerUseMcpConfig; + const resolveCodexConfiguredMcpServerNames = resolveCodexConfiguredMcpServerNamesOverride + ?? ((): string[] => { + const codexHome = process.env.CODEX_HOME?.trim() || path.join(os.homedir(), ".codex"); + try { + return codexConfiguredMcpServerNames(fs.readFileSync(path.join(codexHome, "config.toml"), "utf8")); + } catch { + return []; + } + }); + + /** + * Thread-level Codex config for one session. Orchestrator leads never get MCP + * (user servers are switched off one by one and Computer Use is skipped); + * every other session keeps the user's Codex config untouched. + */ + const codexThreadConfigArgsFor = async ( + managed: ManagedChatSession, + reasoningEffort: string | null | undefined, + ): Promise<{ config?: Record }> => { + if (isOrchestrationLeadSession(managed.session)) { + return codexThreadConfigArgs(reasoningEffort, null, await resolveCodexConfiguredMcpServerNames()); + } + return codexThreadConfigArgs(reasoningEffort, await resolveCodexComputerUseMcp()); + }; const notifiedSettledTurns = new Set(); const cancelledQueueRecoveries = new Map [ lease.serverName, @@ -21548,7 +21606,9 @@ export function createAgentChatService(args: { filename: path.basename(attachment._resolvedPath), })) .filter((entry) => fs.existsSync(entry.path)); - const toolSelection = await refreshOpenCodeSessionToolSelection(runtime.handle); + const toolSelection = await refreshOpenCodeSessionToolSelection(runtime.handle, { + orchestrationLead: isOrchestrationLeadSession(managed.session), + }); const openCodeReasoningVariant = managed.session.reasoningEffort && runtime.modelDescriptor.reasoningTiers?.includes(managed.session.reasoningEffort) @@ -26868,8 +26928,13 @@ export function createAgentChatService(args: { const config = resolveChatConfig(); const lockedMode = lockedOrchestrationPermissionMode(managed.session); if (lockedMode) { - const codexPolicy: CodexPolicy = lockedMode === "plan" - ? { approvalPolicy: "on-request", sandbox: "read-only" } + // Codex exposes no tool allow/deny list (see ORCHESTRATION_LEAD_CODEX_POLICY), + // so the lead's "never edits code, never runs shell" invariant is enforced + // by running its thread read-only with approvals off. Workers and + // validators keep full access. This covers `turn/start` too — the turn + // path resolves its policy through this same function. + const codexPolicy: CodexPolicy = isOrchestrationLeadSession(managed.session) + ? { ...ORCHESTRATION_LEAD_CODEX_POLICY } : { approvalPolicy: "never", sandbox: "danger-full-access" }; managed.session.codexConfigSource = "flags"; managed.session.codexApprovalPolicy = codexPolicy.approvalPolicy; @@ -26939,7 +27004,7 @@ export function createAgentChatService(args: { const startResponse = await runtime.request("thread/start", { model: managed.session.model, cwd: managed.laneWorktreePath, - ...codexThreadConfigArgs(reasoningEffort, await resolveCodexComputerUseMcp()), + ...(await codexThreadConfigArgsFor(managed, reasoningEffort)), developerInstructions: buildCodexDeveloperInstructions({ laneWorktreePath: managed.laneWorktreePath, session: managed.session, @@ -32180,6 +32245,7 @@ export function createAgentChatService(args: { stableStringify((modelParams ?? []).map((entry) => [entry.id, entry.value])), policy.chatMode, policy.approvalPolicy, + policy.orchestrationLead ? "lead-gated" : "unrestricted", policy.force ? "force" : "guarded", buildOrchestrationSessionContext(managed) ? "orchestration-mcp" : "standard", ].join(":"); @@ -32827,6 +32893,11 @@ export function createAgentChatService(args: { modelId, autonomyLevel: resolveDroidSdkAutonomyLevel(managed.session), interactionMode, + // Droid's own editor/terminal tools live outside ADE's toolset, so a lead + // has to have them withheld natively as well. + ...(isOrchestrationLeadSession(managed.session) + ? { disabledToolCategories: ORCHESTRATION_LEAD_DENIED_DROID_TOOL_CATEGORIES } + : {}), ...(reasoningEffort ? { reasoningEffort } : {}), ...(interactionMode === "spec" ? { @@ -33295,6 +33366,10 @@ export function createAgentChatService(args: { } const cursorMcpLeases = await ensureHttpMcpLeases(managed); + // These inline servers are additive. A lead's isolation from the user's own + // MCP servers rides on `policy.orchestrationLead`, which the worker turns + // into a trimmed `local.settingSources` (cursorSdkSettingSources) — see + // ORCHESTRATION_LEAD_MCP_ISOLATION.cursor. const cursorOrchestrationMcpServers = cursorMcpLeases.length ? Object.fromEntries(cursorMcpLeases.map((lease) => [ lease.serverName, @@ -34561,6 +34636,15 @@ export function createAgentChatService(args: { const auth = await detectAuth(); throwIfDroidSetupInterrupted(); const droidMcpLeases = await ensureHttpMcpLeases(managed); + // KNOWN GAP — orchestrator leads are NOT isolated from the user's MCP + // servers on Droid. `mcpServers` here only *adds* ADE's lease; Droid's + // SDK has no session-scoped way to withhold the servers it loads from the + // user's Factory config. `disabledToolIds` covers the exec tool catalog + // only, and toggleMcpServer/toggleMcpTool persist to the user's global + // settings (settingsLevel is pinned to `User`), which would disable a + // server for every other droid session on the machine. ADE will not + // mutate user config to fake a session gate — see + // ORCHESTRATION_LEAD_MCP_ISOLATION.droid. const droidOrchestrationMcpServers = droidMcpLeases.length ? droidMcpLeases.map((lease) => lease.config) : undefined; @@ -35102,7 +35186,7 @@ export function createAgentChatService(args: { threadId: threadIdToResume, model: managed.session.model, cwd: managed.laneWorktreePath, - ...codexThreadConfigArgs(resumeReasoningEffort, await resolveCodexComputerUseMcp()), + ...(await codexThreadConfigArgsFor(managed, resumeReasoningEffort)), developerInstructions: buildCodexDeveloperInstructions({ laneWorktreePath: managed.laneWorktreePath, session: managed.session, @@ -36853,7 +36937,7 @@ export function createAgentChatService(args: { threadId, model: managed.session.model, cwd: managed.laneWorktreePath, - ...codexThreadConfigArgs(managed.session.reasoningEffort, await resolveCodexComputerUseMcp()), + ...(await codexThreadConfigArgsFor(managed, managed.session.reasoningEffort)), ...codexServiceTierArgs(managed.session), ...codexPolicyArgs(codexPolicy), excludeTurns: true, @@ -37166,7 +37250,7 @@ export function createAgentChatService(args: { threadId: originalThreadId, model: managed.session.model, cwd: managed.laneWorktreePath, - ...codexThreadConfigArgs(managed.session.reasoningEffort, await resolveCodexComputerUseMcp()), + ...(await codexThreadConfigArgsFor(managed, managed.session.reasoningEffort)), developerInstructions: buildCodexDeveloperInstructions({ laneWorktreePath: managed.laneWorktreePath, session: managed.session, @@ -42874,7 +42958,7 @@ export function createAgentChatService(args: { threadId, model: managed.session.model, cwd: managed.laneWorktreePath, - ...codexThreadConfigArgs(resumeReasoningEffort, await resolveCodexComputerUseMcp()), + ...(await codexThreadConfigArgsFor(managed, resumeReasoningEffort)), developerInstructions: buildCodexDeveloperInstructions({ laneWorktreePath: managed.laneWorktreePath, session: managed.session, diff --git a/apps/desktop/src/main/services/chat/cursorSdkPolicy.test.ts b/apps/desktop/src/main/services/chat/cursorSdkPolicy.test.ts index f9776c110..5d24751bf 100644 --- a/apps/desktop/src/main/services/chat/cursorSdkPolicy.test.ts +++ b/apps/desktop/src/main/services/chat/cursorSdkPolicy.test.ts @@ -31,6 +31,55 @@ describe("Cursor SDK policy", () => { }); }); + it("denies Cursor's own edit/shell/subagent tools for an orchestrator lead", () => { + const laneRoot = "/tmp/ade-lane"; + // Leads run under the same permissive full-auto profile as workers; only + // the lead gate distinguishes them. + const leadPolicy = resolveCursorSdkPolicy({ + cursorModeId: "full-auto", + interactionMode: "orchestrator-lead", + orchestrationRole: "lead", + }); + const workerPolicy = resolveCursorSdkPolicy({ + cursorModeId: "full-auto", + interactionMode: "orchestrator-worker", + orchestrationRole: "worker", + }); + expect(leadPolicy).toMatchObject({ approvalPolicy: "never", orchestrationLead: true }); + expect(workerPolicy).toMatchObject({ approvalPolicy: "never", orchestrationLead: false }); + + const nativeWriteTools = [ + { toolName: "write", toolInput: { path: "README.md", contents: "x" } }, + { toolName: "edit", toolInput: { path: "README.md" } }, + { toolName: "apply_patch", toolInput: { path: "README.md" } }, + { toolName: "delete", toolInput: { path: "README.md" } }, + ]; + const nativeShellTools = [ + { toolName: "shell", toolInput: { command: "echo hi > README.md" } }, + { toolName: "run_command", toolInput: { command: "npm test" } }, + ]; + const nativeTaskTools = [{ toolName: "task", toolInput: { prompt: "edit README" } }]; + + for (const raw of [...nativeWriteTools, ...nativeShellTools, ...nativeTaskTools]) { + const request = summarizeCursorHook(raw, laneRoot); + expect(evaluateCursorSdkHook({ request, policy: leadPolicy, laneRoot })).toBe("deny"); + // Same tool, same permissive profile, worker role: still allowed. + const workerRequest = summarizeCursorHook(raw, laneRoot); + expect(evaluateCursorSdkHook({ request: workerRequest, policy: workerPolicy, laneRoot })).toBe("allow"); + } + + // Reads stay available so the lead can still plan. + const read = summarizeCursorHook({ toolName: "read", toolInput: { path: "src/app.ts" } }, laneRoot); + expect(evaluateCursorSdkHook({ request: read, policy: leadPolicy, laneRoot })).toBe("allow"); + + // Unrecognised tool names classify as risk "unknown" and must fail closed + // for a lead even though `approvalPolicy: never` would wave them through. + const unknown = summarizeCursorHook({ toolName: "some_future_write_tool", toolInput: {} }, laneRoot); + expect(evaluateCursorSdkHook({ request: unknown, policy: leadPolicy, laneRoot })).toBe("deny"); + const unknownForWorker = summarizeCursorHook({ toolName: "some_future_write_tool", toolInput: {} }, laneRoot); + expect(evaluateCursorSdkHook({ request: unknownForWorker, policy: workerPolicy, laneRoot })).toBe("allow"); + }); + it("allows reads, asks for risky tools, and denies protected paths", () => { const policy = resolveCursorSdkPolicy({ cursorModeId: "agent" }); const laneRoot = "/tmp/ade-lane"; diff --git a/apps/desktop/src/main/services/chat/cursorSdkPolicy.ts b/apps/desktop/src/main/services/chat/cursorSdkPolicy.ts index 997a3618f..d4a86ee21 100644 --- a/apps/desktop/src/main/services/chat/cursorSdkPolicy.ts +++ b/apps/desktop/src/main/services/chat/cursorSdkPolicy.ts @@ -2,6 +2,11 @@ import fs from "node:fs"; import path from "node:path"; import type { AgentChatSession } from "../../../shared/types"; import { cursorProjectSlug } from "../../../shared/cursorProjectSlug"; +import { + isOrchestrationLeadSession, + ORCHESTRATION_LEAD_ALLOWED_CURSOR_TOOL_RISKS, + ORCHESTRATION_LEAD_CURSOR_SETTING_SOURCES, +} from "../../../shared/orchestrationRuntimePolicy"; import type { CursorSdkApprovalPolicy, CursorSdkChatMode, @@ -10,7 +15,11 @@ import type { CursorSdkPermissionPolicy, } from "./cursorSdkProtocol"; -type CursorSessionModeInput = Pick; +type CursorSessionModeInput = Pick + & Partial>; + +/** Mirrors `SettingSource` from `@cursor/sdk` (`local.settingSources`). */ +export type CursorSdkSettingSource = "project" | "user" | "team" | "mdm" | "plugins" | "all"; const READ_TOOL_NAMES = new Set([ "read", @@ -56,6 +65,10 @@ export function resolveCursorSdkChatMode(session: CursorSessionModeInput): Curso } export function resolveCursorSdkPolicy(session: CursorSessionModeInput): CursorSdkPermissionPolicy { + // Leads run under the same permissive `full-auto` profile as workers by + // design; their protection is the risk gate in `evaluateCursorSdkHook`, which + // this flag switches on. + const orchestrationLead = isOrchestrationLeadSession(session); const explicit = typeof session.cursorModeId === "string" ? session.cursorModeId.trim().toLowerCase() : ""; const legacyFullAuto = !explicit.length @@ -67,6 +80,7 @@ export function resolveCursorSdkPolicy(session: CursorSessionModeInput): CursorS sandbox: "off", force: true, hardGuards: true, + orchestrationLead, }; } @@ -78,6 +92,7 @@ export function resolveCursorSdkPolicy(session: CursorSessionModeInput): CursorS sandbox: "ade", force: false, hardGuards: true, + orchestrationLead, }; } @@ -87,9 +102,28 @@ export function resolveCursorSdkPolicy(session: CursorSessionModeInput): CursorS sandbox: "ade", force: false, hardGuards: true, + orchestrationLead, }; } +/** + * Ambient Cursor setting layers an agent may load (`local.settingSources`). + * + * The Cursor SDK derives `includeProjectMcp` / `includePluginMcp` from these + * sources, so an orchestrator lead — which must see ADE-managed MCP servers + * only — drops the `project` and `plugins` layers. `user` has to stay: ADE's + * own preToolUse tool-gate hook is installed under the user's `~/.cursor` and + * is the enforcement point for every Cursor lead denial (an MCP call arrives at + * the gate as `MCP:`, classifies as risk `unknown`, and fails closed). + * See ORCHESTRATION_LEAD_MCP_ISOLATION.cursor. + */ +export function cursorSdkSettingSources( + policy: Pick, +): CursorSdkSettingSource[] { + if (!policy.orchestrationLead) return ["all"]; + return [...ORCHESTRATION_LEAD_CURSOR_SETTING_SOURCES]; +} + function asRecord(value: unknown): Record | null { return value && typeof value === "object" && !Array.isArray(value) ? value as Record @@ -492,6 +526,17 @@ export function evaluateCursorSdkHook(args: { } const normalized = normalizeToolName(args.request.toolName); + // An orchestrator lead never edits code or runs shell, whatever its approval + // policy says. Allow-list by risk so an unrecognised tool name (risk + // "unknown") is denied rather than waved through by `approvalPolicy: never`. + if (args.policy.orchestrationLead) { + if ((ORCHESTRATION_LEAD_ALLOWED_CURSOR_TOOL_RISKS as readonly string[]).includes(args.request.risk)) { + return "allow"; + } + args.request.reason = + "Orchestrator lead sessions cannot use Cursor's edit, shell, or subagent tools. Delegate the work to a worker instead."; + return "deny"; + } if (args.policy.approvalPolicy === "never") return "allow"; if (args.policy.approvalPolicy === "read-only") { return args.request.risk === "read" ? "allow" : "deny"; diff --git a/apps/desktop/src/main/services/chat/cursorSdkPool.test.ts b/apps/desktop/src/main/services/chat/cursorSdkPool.test.ts index 90b74abef..48d5bdfe1 100644 --- a/apps/desktop/src/main/services/chat/cursorSdkPool.test.ts +++ b/apps/desktop/src/main/services/chat/cursorSdkPool.test.ts @@ -395,6 +395,7 @@ describe("Cursor SDK pool paths", () => { sandbox: "ade" as const, force: false, hardGuards: true, + orchestrationLead: false, }, }; @@ -430,6 +431,7 @@ describe("Cursor SDK pool paths", () => { sandbox: "ade" as const, force: false, hardGuards: true, + orchestrationLead: false, }, }); @@ -468,6 +470,7 @@ describe("Cursor SDK pool paths", () => { sandbox: "ade" as const, force: false, hardGuards: true, + orchestrationLead: false, }, }; @@ -501,6 +504,7 @@ describe("Cursor SDK pool paths", () => { sandbox: "ade" as const, force: false, hardGuards: true, + orchestrationLead: false, }, })).rejects.toThrow("Cursor SDK worker exited (1)."); }); @@ -521,6 +525,7 @@ describe("Cursor SDK pool paths", () => { sandbox: "ade" as const, force: false, hardGuards: true, + orchestrationLead: false, }, })).rejects.toThrow(/NGHTTP2_ENHANCE_YOUR_CALM/); }); diff --git a/apps/desktop/src/main/services/chat/cursorSdkPool.ts b/apps/desktop/src/main/services/chat/cursorSdkPool.ts index be43a9b62..d1fe1a8fc 100644 --- a/apps/desktop/src/main/services/chat/cursorSdkPool.ts +++ b/apps/desktop/src/main/services/chat/cursorSdkPool.ts @@ -920,6 +920,7 @@ export async function runCursorSdkCatalogRequest( sandbox: "off", force: true, hardGuards: false, + orchestrationLead: false, }, logger: args.logger, }); @@ -970,6 +971,7 @@ export async function runCursorSdkCloudRequest( sandbox: "off", force: true, hardGuards: false, + orchestrationLead: false, }, logger: args.logger, }); diff --git a/apps/desktop/src/main/services/chat/cursorSdkProtocol.ts b/apps/desktop/src/main/services/chat/cursorSdkProtocol.ts index d6c568c6a..759b05fbf 100644 --- a/apps/desktop/src/main/services/chat/cursorSdkProtocol.ts +++ b/apps/desktop/src/main/services/chat/cursorSdkProtocol.ts @@ -27,6 +27,12 @@ export type CursorSdkPermissionPolicy = { sandbox: CursorSdkSandboxMode; force: boolean; hardGuards: boolean; + /** + * Orchestrator-lead sessions may only ever run read-risk tools. Carried on + * the policy (rather than derived at the hook) so it reaches the out-of-band + * hook server in `cursorSdkWorker` through the existing policy plumbing. + */ + orchestrationLead: boolean; }; export type CursorSdkModelParameterValue = { diff --git a/apps/desktop/src/main/services/chat/cursorSdkWorker.ts b/apps/desktop/src/main/services/chat/cursorSdkWorker.ts index ca29fa165..56056521f 100644 --- a/apps/desktop/src/main/services/chat/cursorSdkWorker.ts +++ b/apps/desktop/src/main/services/chat/cursorSdkWorker.ts @@ -33,6 +33,7 @@ import { } from "./cursorSdkErrors"; import { allowCursorHook, + cursorSdkSettingSources, denyCursorHook, evaluateCursorSdkHook, summarizeCursorHook, @@ -219,6 +220,7 @@ function cursorSdkAgentMode(policy: CursorSdkPermissionPolicy): CursorSdkAgentMo return policy.chatMode === "agent" ? "agent" : "plan"; } + function cursorRunRequestId(run: unknown): string | undefined { const value = run && typeof run === "object" ? (run as { requestId?: unknown }).requestId : undefined; return typeof value === "string" && value.trim() ? value.trim() : undefined; @@ -428,7 +430,7 @@ async function initWorker(init: CursorSdkWorkerInit): Promise<{ agentId: string; name: init.agentName ?? undefined, local: { cwd: init.laneRoot, - settingSources: ["all"], + settingSources: cursorSdkSettingSources(init.policy), sandboxOptions: { enabled: false }, enableAgentRetries: true, }, diff --git a/apps/desktop/src/main/services/chat/droidSdkProtocol.test.ts b/apps/desktop/src/main/services/chat/droidSdkProtocol.test.ts new file mode 100644 index 000000000..5b4d1da05 --- /dev/null +++ b/apps/desktop/src/main/services/chat/droidSdkProtocol.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from "vitest"; +import { droidDisabledToolIdsForCategories } from "./droidSdkProtocol"; +import { ORCHESTRATION_LEAD_DENIED_DROID_TOOL_CATEGORIES } from "../../../shared/orchestrationRuntimePolicy"; + +// Shape mirrors Droid's `session.listTools()` result. Ids are build-specific +// (`edit_file`, `apply-patch-cli`, `create-cli`, …), which is exactly why the +// deny is expressed as categories and resolved against the live tool list. +const DROID_TOOLS = [ + { id: "view_file", llmId: "Read", category: "read" }, + { id: "grep_tool", llmId: "Grep", category: "read" }, + { id: "edit_file", llmId: "edit_file", category: "edit" }, + { id: "apply_patch", llmId: "apply_patch", category: "edit" }, + { id: "apply-patch-cli", llmId: "ApplyPatch", category: "edit" }, + { id: "create-cli", llmId: "Create", category: "edit" }, + { id: "execute_terminal_command", llmId: "Execute", category: "execute" }, + { id: "task-cli", llmId: "Task", category: "execute" }, + { id: "todo_write", llmId: "TodoWrite", category: "other" }, +]; + +describe("droidDisabledToolIdsForCategories", () => { + it("selects every Droid-native edit and execute tool for an orchestrator lead", () => { + const disabled = droidDisabledToolIdsForCategories( + DROID_TOOLS, + ORCHESTRATION_LEAD_DENIED_DROID_TOOL_CATEGORIES, + ); + expect(disabled).toEqual([ + "edit_file", + "apply_patch", + "apply-patch-cli", + "create-cli", + "execute_terminal_command", + "task-cli", + ]); + // Reads survive so the lead can still plan. + expect(disabled).not.toContain("view_file"); + expect(disabled).not.toContain("grep_tool"); + // Every tool Droid classifies as edit/execute is covered — no id list to + // drift out of date when Droid renames or adds one. + for (const tool of DROID_TOOLS) { + if (tool.category === "edit" || tool.category === "execute") { + expect(disabled).toContain(tool.id); + } + } + }); + + it("disables nothing when no category is denied", () => { + expect(droidDisabledToolIdsForCategories(DROID_TOOLS, [])).toEqual([]); + }); + + it("ignores malformed entries rather than emitting empty tool ids", () => { + expect(droidDisabledToolIdsForCategories( + [{ id: "", category: "edit" }, { category: "edit" }, { id: "edit_file", category: "edit" }], + ["edit"], + )).toEqual(["edit_file"]); + }); +}); diff --git a/apps/desktop/src/main/services/chat/droidSdkProtocol.ts b/apps/desktop/src/main/services/chat/droidSdkProtocol.ts index 45ed99806..477201f74 100644 --- a/apps/desktop/src/main/services/chat/droidSdkProtocol.ts +++ b/apps/desktop/src/main/services/chat/droidSdkProtocol.ts @@ -1,3 +1,7 @@ +import type { DroidToolCategory } from "../../../shared/orchestrationRuntimePolicy"; + +export type { DroidToolCategory }; + export type DroidSdkAutonomyLevel = "off" | "low" | "medium" | "high"; // `agi` puts Droid in orchestrator mode: it decomposes a mission into features // and spawns worker sub-sessions (surfaced to ADE as subagents) while keeping @@ -21,8 +25,37 @@ export type DroidSdkSessionSettings = { reasoningEffort?: DroidSdkReasoningEffort | null; specModeModelId?: string | null; specModeReasoningEffort?: DroidSdkReasoningEffort | null; + /** + * Droid tool categories to withhold from the model, resolved to concrete + * `disabledToolIds` in the worker (tool ids are build-specific, categories + * are not). Set for orchestrator-lead sessions so Droid's own editor and + * terminal tools are dropped alongside ADE's. + */ + disabledToolCategories?: readonly DroidToolCategory[] | null; }; +/** + * Reduces a `session.listTools()` result to the ids ADE must disable. + * + * Droid's built-in tool ids vary by build and model (`edit_file`, + * `apply-patch-cli`, `create-cli`, …), so ADE selects by the category Droid + * itself reports rather than pinning a brittle id list. + */ +export function droidDisabledToolIdsForCategories( + tools: ReadonlyArray<{ id?: unknown; category?: unknown }>, + categories: readonly DroidToolCategory[], +): string[] { + const denied = new Set(categories); + const ids: string[] = []; + for (const tool of tools) { + const id = typeof tool?.id === "string" ? tool.id.trim() : ""; + const category = typeof tool?.category === "string" ? tool.category : ""; + if (!id || !denied.has(category as DroidToolCategory)) continue; + if (!ids.includes(id)) ids.push(id); + } + return ids; +} + export type DroidSdkWorkerInit = { sessionId: string; laneRoot: string; diff --git a/apps/desktop/src/main/services/chat/droidSdkWorker.ts b/apps/desktop/src/main/services/chat/droidSdkWorker.ts index 928ac2d97..e3a869dc8 100644 --- a/apps/desktop/src/main/services/chat/droidSdkWorker.ts +++ b/apps/desktop/src/main/services/chat/droidSdkWorker.ts @@ -10,6 +10,7 @@ import type { DroidSdkWorkerRequest, DroidSdkWorkerResponse, } from "./droidSdkProtocol"; +import { droidDisabledToolIdsForCategories } from "./droidSdkProtocol"; import { loadDroidSdk } from "../ai/droidSdkLoader"; import { summarizeDroidAskUser } from "./droidSdkAskUser"; import { ensureDroidSpawnsAreWindowless } from "./droidSdkWindowsHide"; @@ -246,14 +247,33 @@ function buildReady(): DroidSdkReady { }; } +/** + * Resolves the concrete `disabledToolIds` for a settings bag, if it asks for + * any category to be withheld. Droid reports tool ids and their categories at + * runtime, so the lookup happens here rather than against a pinned id list. + * Applied on every settings push so a resumed session cannot drift back to the + * full toolset. + */ +async function resolveDisabledToolIds( + settings: DroidSdkSessionSettings, +): Promise { + const categories = settings.disabledToolCategories ?? null; + if (!session || !categories?.length) return null; + const listed = await session.listTools(); + const tools = Array.isArray(listed?.tools) ? listed.tools : []; + return droidDisabledToolIdsForCategories(tools, categories); +} + async function applySettings(settings: DroidSdkSessionSettings): Promise { if (!session) throw new Error("Droid SDK worker is not initialized."); const sdk = await getSdk(); + const disabledToolIds = await resolveDisabledToolIds(settings); if (settings.interactionMode === "spec") { await session.enterSpecMode({ specModeModelId: settings.specModeModelId?.trim() || settings.modelId, specModeReasoningEffort: coerceReasoning(settings.specModeReasoningEffort ?? settings.reasoningEffort), }); + if (disabledToolIds?.length) await session.updateSettings({ disabledToolIds }); return; } await session.updateSettings({ @@ -261,6 +281,7 @@ async function applySettings(settings: DroidSdkSessionSettings): Promise { autonomyLevel: settings.autonomyLevel as DroidSdkTypes.AutonomyLevel, interactionMode: toDroidInteractionMode(sdk, settings.interactionMode), reasoningEffort: coerceReasoning(settings.reasoningEffort), + ...(disabledToolIds ? { disabledToolIds } : {}), }); } @@ -290,6 +311,13 @@ async function initWorker(init: DroidSdkWorkerInit): Promise { } else { session = await sdk.createSession(sessionOptions(sdk, init, init.settings)); } + // `createSession`/`resumeSession` take `disabledToolIds`, but the ids are + // only discoverable from the live session, so the lead's denial is pushed + // immediately after the session exists and before any prompt can run. + if (init.settings.disabledToolCategories?.length) { + const disabledToolIds = await resolveDisabledToolIds(init.settings); + if (disabledToolIds?.length) await session.updateSettings({ disabledToolIds }); + } const ready = buildReady(); post({ type: "ready", ready }); return ready; diff --git a/apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts b/apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts index 05d1b3261..b2fa2865d 100644 --- a/apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts +++ b/apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts @@ -116,6 +116,7 @@ import { __resetOpenCodeRuntimeDiagnosticsForTests, buildOpenCodeConfig, getOpenCodeRuntimeSnapshot, + refreshOpenCodeSessionToolSelection, runOpenCodeTextPrompt, startOpenCodeSession, } from "./openCodeRuntime"; @@ -306,3 +307,38 @@ describe("buildOpenCodeConfig provider injection", () => { expect(provider.mysteryco).toBeUndefined(); }); }); + +describe("refreshOpenCodeSessionToolSelection", () => { + const handleOf = () => ({ toolSelection: null } as { toolSelection: Record | null }); + + it("withholds OpenCode's native write and shell tools from an orchestrator lead", async () => { + const handle = handleOf(); + const selection = await refreshOpenCodeSessionToolSelection( + handle as never, + { orchestrationLead: true }, + ); + // These are OpenCode's own built-ins, not ADE's toolset — they are what a + // lead could otherwise use to edit code or run shell directly. + expect(selection).toMatchObject({ + bash: false, + edit: false, + write: false, + patch: false, + task: false, + }); + // Reads stay available so the lead can still plan. + expect(selection).not.toHaveProperty("read"); + expect(selection).not.toHaveProperty("grep"); + expect(selection).not.toHaveProperty("glob"); + // The handle carries the same map, so a resumed prompt cannot drop it. + expect(handle.toolSelection).toEqual(selection); + }); + + it("leaves workers and validators on OpenCode's default toolset", async () => { + for (const options of [undefined, { orchestrationLead: false }]) { + const handle = handleOf(); + await expect(refreshOpenCodeSessionToolSelection(handle as never, options)).resolves.toBeNull(); + expect(handle.toolSelection).toBeNull(); + } + }); +}); diff --git a/apps/desktop/src/main/services/opencode/openCodeRuntime.ts b/apps/desktop/src/main/services/opencode/openCodeRuntime.ts index 3a114f7d5..aeb6ae972 100644 --- a/apps/desktop/src/main/services/opencode/openCodeRuntime.ts +++ b/apps/desktop/src/main/services/opencode/openCodeRuntime.ts @@ -27,6 +27,7 @@ import type { OpenCodeRuntimeSnapshot, ProjectConfigFile, } from "../../../shared/types"; +import { orchestrationLeadOpenCodeToolSelection } from "../../../shared/orchestrationRuntimePolicy"; import { stableStringify } from "../shared/utils"; import { resolveOpenCodeBinaryPath } from "./openCodeBinaryManager"; import type { PermissionMode } from "../ai/tools/universalTools"; @@ -725,11 +726,22 @@ export async function openCodeEventStream(args: { return result.stream as AsyncGenerator; } +/** + * Resolves the `tools` map ADE sends with every OpenCode prompt. + * + * OpenCode has no server-side role model: whatever it exposes, the model may + * call. `session.prompt`'s `tools` map is the only lever, so an orchestrator + * lead gets every write/shell tool explicitly switched off here. Every other + * session keeps OpenCode's defaults (`null` — field omitted entirely). + */ export async function refreshOpenCodeSessionToolSelection( handle: OpenCodeSessionHandle, + options?: { orchestrationLead?: boolean }, ): Promise | null> { - handle.toolSelection = null; - return null; + handle.toolSelection = options?.orchestrationLead + ? orchestrationLeadOpenCodeToolSelection() + : null; + return handle.toolSelection; } export async function runOpenCodeTextPrompt( diff --git a/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts b/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts index 48e3a2a27..808645a82 100644 --- a/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts +++ b/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts @@ -1,7 +1,12 @@ import { describe, expect, it } from "vitest"; import { + ORCHESTRATION_LEAD_CURSOR_SETTING_SOURCES, ORCHESTRATION_LEAD_DENIED_CLAUDE_TOOLS, + ORCHESTRATION_LEAD_MCP_ISOLATION, applyOrchestrationPermissionProfile, + codexConfiguredMcpServerNames, + orchestrationLeadCodexMcpOverrides, + orchestrationLeadMcpIsolation, effectiveOrchestrationPermissionMode, isOrchestrationInteractionMode, isOrchestrationLeadSession, @@ -59,6 +64,67 @@ describe("orchestrationRuntimePolicy", () => { } }); + it("registers an MCP isolation mechanism for every provider that receives MCP", () => { + // A provider added to ADE without an entry here is a compile error; this + // asserts the runtime shape and pins the one provider that has no mechanism + // so the gap cannot quietly become "gated" without someone editing a test. + for (const provider of ["claude", "codex", "cursor", "droid", "opencode"] as const) { + const isolation = orchestrationLeadMcpIsolation(provider); + expect(isolation?.mechanism).toBeTruthy(); + expect(isolation?.note.length).toBeGreaterThan(0); + } + expect(ORCHESTRATION_LEAD_MCP_ISOLATION.droid.gated).toBe(false); + for (const provider of ["claude", "codex", "cursor", "opencode"] as const) { + expect(ORCHESTRATION_LEAD_MCP_ISOLATION[provider].gated).toBe(true); + } + expect(orchestrationLeadMcpIsolation("gemini")).toBeNull(); + }); + + it("drops Cursor's MCP-carrying setting layers for a lead but keeps the ADE hook layer", () => { + // `user` carries ADE's own preToolUse tool-gate hook (~/.cursor/hooks.json); + // dropping it would disable every Cursor lead denial. + expect(ORCHESTRATION_LEAD_CURSOR_SETTING_SOURCES).toContain("user"); + expect(ORCHESTRATION_LEAD_CURSOR_SETTING_SOURCES).not.toContain("project"); + expect(ORCHESTRATION_LEAD_CURSOR_SETTING_SOURCES).not.toContain("plugins"); + expect(ORCHESTRATION_LEAD_CURSOR_SETTING_SOURCES).not.toContain("all"); + }); + + it("reads Codex's configured MCP server names from every config.toml shape", () => { + expect(codexConfiguredMcpServerNames([ + "model = \"gpt-5.4\"", + "", + "[mcp_servers.filesystem]", + "command = \"npx\"", + "args = [\"-y\", \"@modelcontextprotocol/server-filesystem\"]", + "", + "[mcp_servers.filesystem.env]", + "TOKEN = \"x\"", + "", + "[mcp_servers.\"my shell\"]", + "command = \"sh\"", + "", + "mcp_servers.git.command = \"git-mcp\"", + "# mcp_servers.commented.command = \"nope\"", + ].join("\n"))).toEqual(["filesystem", "my shell", "git"]); + + expect(codexConfiguredMcpServerNames( + "mcp_servers = { linear = { command = \"linear\", args = [\"mcp\"] }, \"pg\" = { url = \"http://x\" } }", + )).toEqual(["linear", "pg"]); + + expect(codexConfiguredMcpServerNames("model = \"gpt-5.4\"")).toEqual([]); + }); + + it("turns Codex's configured servers into an explicit per-server disable overlay", () => { + // Codex merges the thread config overlay into config.toml rather than + // replacing it, so `mcp_servers = {}` would be a no-op — the only honest + // isolation is naming each server with `enabled = false`. + expect(orchestrationLeadCodexMcpOverrides(["filesystem", "git", " "])).toEqual({ + filesystem: { enabled: false }, + git: { enabled: false }, + }); + expect(orchestrationLeadCodexMcpOverrides([])).toEqual({}); + }); + it("denies Claude-native direct-work tools for orchestrator leads", () => { expect(ORCHESTRATION_LEAD_DENIED_CLAUDE_TOOLS).toEqual(expect.arrayContaining([ "Agent", diff --git a/apps/desktop/src/shared/orchestrationRuntimePolicy.ts b/apps/desktop/src/shared/orchestrationRuntimePolicy.ts index e90cc2ec7..0caac334d 100644 --- a/apps/desktop/src/shared/orchestrationRuntimePolicy.ts +++ b/apps/desktop/src/shared/orchestrationRuntimePolicy.ts @@ -35,6 +35,19 @@ export type OrchestrationPermissionProfile = Partial { + const selection: Record = {}; + for (const name of ORCHESTRATION_LEAD_DENIED_OPENCODE_TOOLS) selection[name] = false; + return selection; +} + +/** Droid tool categories (`ListToolsResult.tools[].category`) withheld from leads. */ +export const ORCHESTRATION_LEAD_DENIED_DROID_TOOL_CATEGORIES = ["edit", "execute"] as const; + +export type DroidToolCategory = "read" | "edit" | "execute" | "other"; + +/** + * Cursor tool-call risk classes a lead may use. Cursor's SDK has no tool + * allow/deny list, so the denial is enforced in ADE's own hook evaluator + * (`evaluateCursorSdkHook`), which classifies every tool call by risk. Leads + * are allow-listed rather than deny-listed so an unrecognised tool name + * (risk `"unknown"`) fails closed. + */ +export const ORCHESTRATION_LEAD_ALLOWED_CURSOR_TOOL_RISKS = ["read"] as const; + +/** + * Codex has no tool allow/deny knob: the pinned app-server protocol + * (`ThreadStartParams`) exposes no tools field, and the `tools` config table + * only covers `web_search` / `experimental_request_user_input`. Its sandbox is + * the only enforcement point, so leads run threads read-only with approvals + * off (so the lead cannot escalate a write through an approval prompt). + */ +export const ORCHESTRATION_LEAD_CODEX_POLICY = { + approvalPolicy: "never" satisfies AgentChatCodexApprovalPolicy, + sandbox: "read-only" satisfies AgentChatCodexSandbox, +} as const; + +// --------------------------------------------------------------------------- +// Orchestrator-lead MCP isolation +// +// The tool denials above only cover each provider's *built-in* tools. A +// user-configured MCP server (filesystem, shell, git, …) hands the same +// capability back through a different door, so a lead must see ADE-managed MCP +// servers only. Every provider that receives MCP configuration is registered +// below with the mechanism it is isolated by — a provider added to ADE without +// an entry here is a compile error, not a silent hole. +// --------------------------------------------------------------------------- + +export type OrchestrationLeadMcpIsolation = { + /** How user/project MCP servers are withheld from a lead on this provider. */ + mechanism: string; + /** False when no mechanism exists — the residual hole is described in `note`. */ + gated: boolean; + note: string; +}; + +export type McpCapableProvider = "claude" | "codex" | "cursor" | "droid" | "opencode"; + +export const ORCHESTRATION_LEAD_MCP_ISOLATION = { + claude: { + mechanism: "strictMcpConfig + managedSettings.allowManagedMcpServersOnly", + gated: true, + note: + "strictMcpConfig makes the Agent SDK ignore ~/.claude.json and project .mcp.json even though " + + "settingSources still loads the user's rules/commands; the programmatic ADE server stays.", + }, + codex: { + mechanism: "thread config override: mcp_servers..enabled = false", + gated: true, + note: + "Codex merges the thread `config` overlay into config.toml rather than replacing it " + + "(verified against codex-cli: `-c mcp_servers={}` is a no-op, `-c mcp_servers.x.enabled=false` " + + "disables x), so ADE enumerates the configured servers and disables each one. Codex leads " + + "receive no ADE-managed MCP server at all — their ADE tools ride the app-server dynamicTools " + + "channel — so the lead's MCP surface is empty. Residual: a server contributed by a Codex " + + "*plugin* is not listed in config.toml's mcp_servers table and cannot be enumerated.", + }, + cursor: { + mechanism: "local.settingSources (project + plugin layers dropped)", + gated: true, + note: + "The Cursor SDK derives includeProjectMcp/includePluginMcp from settingSources, so dropping " + + "those layers drops their MCP servers while `mcpServers` (ADE's inline lease) is unaffected. " + + "The `user` layer must stay: ADE's own preToolUse tool-gate hook is a user-layer artifact " + + "(~/.cursor/hooks.json) and is the enforcement point for every Cursor lead denial. Residual: " + + "user-level MCP servers still load, but their calls reach the gate as `MCP:`, classify " + + "as risk `unknown`, and are denied by the fail-closed allow-list above.", + }, + droid: { + mechanism: "none", + gated: false, + note: + "Droid's SDK exposes no session-scoped MCP restriction. `disabledToolIds` covers the exec tool " + + "catalog only, and toggleMcpServer/toggleMcpTool write the user's global Factory settings " + + "(ToggleMcpServerRequestParams pins settingsLevel: User), which would disable a server for " + + "every other droid session on the machine. ADE will not mutate user config to fake a " + + "session gate, so a droid lead still sees the user's MCP servers.", + }, + opencode: { + mechanism: "ADE-authored server config + OPENCODE_DISABLE_PROJECT_CONFIG", + gated: true, + note: + "Every ADE OpenCode server runs with an ADE-owned XDG_CONFIG_HOME, OPENCODE_CONFIG_CONTENT " + + "built by buildOpenCodeConfig, and OPENCODE_DISABLE_PROJECT_CONFIG=1, so the only MCP servers " + + "any session (lead or worker) can see are ADE's own leases.", + }, +} as const satisfies Record; + +export function orchestrationLeadMcpIsolation( + provider: AgentChatProvider | string, +): OrchestrationLeadMcpIsolation | null { + return (ORCHESTRATION_LEAD_MCP_ISOLATION as Record)[provider] + ?? null; +} + +/** + * Cursor setting layers a lead may load. `project` and `plugins` are dropped + * (they carry MCP servers); `user` stays because ADE's tool-gate hook lives + * there, and `team`/`mdm` stay because they only ever *restrict*. + */ +export const ORCHESTRATION_LEAD_CURSOR_SETTING_SOURCES = ["user", "team", "mdm"] as const; + +/** + * Extracts the server names declared under Codex's `mcp_servers` config table. + * + * Codex has no "managed servers only" switch; the only per-server knob is + * `mcp_servers..enabled`, so a lead's isolation is expressed as an + * explicit `enabled = false` for every configured server. Handles the three + * shapes `config.toml` can use: `[mcp_servers.name]` headers, dotted + * `mcp_servers.name.key = …` assignments, and an inline + * `mcp_servers = { name = { … } }` table. + */ +export function codexConfiguredMcpServerNames(configText: string): string[] { + const names: string[] = []; + const add = (raw: string): void => { + const name = raw.trim().replace(/^["']|["']$/g, "").trim(); + if (!name || names.includes(name)) return; + names.push(name); + }; + const firstSegment = (path: string): string | null => { + const trimmed = path.trim(); + if (trimmed.startsWith("\"") || trimmed.startsWith("'")) { + const quote = trimmed[0]!; + const end = trimmed.indexOf(quote, 1); + return end > 0 ? trimmed.slice(1, end) : null; + } + const segment = trimmed.split(".")[0]?.trim() ?? ""; + return segment.length ? segment : null; + }; + + for (const line of configText.replace(/\r\n?/g, "\n").split("\n")) { + const withoutComment = line.replace(/^\s*#.*$/, "").trim(); + if (!withoutComment.length) continue; + + const header = withoutComment.match(/^\[\[?\s*mcp_servers\s*\.\s*(.+?)\s*\]\]?$/)?.[1]; + if (header) { + const segment = firstSegment(header); + if (segment) add(segment); + continue; + } + + const dotted = withoutComment.match(/^mcp_servers\s*\.\s*(.+?)\s*=/)?.[1]; + if (dotted) { + const segment = firstSegment(dotted); + if (segment) add(segment); + continue; + } + + const inline = withoutComment.match(/^mcp_servers\s*=\s*\{(.*)\}\s*$/)?.[1]; + if (inline !== undefined) { + for (const key of inlineTableKeys(inline)) add(key); + } + } + return names; +} + +/** Top-level `key =` names of a single-line TOML inline table body. */ +function inlineTableKeys(body: string): string[] { + const keys: string[] = []; + let depth = 0; + let quote: string | null = null; + let token = ""; + for (let index = 0; index < body.length; index += 1) { + const char = body[index]!; + if (quote) { + if (char === quote && body[index - 1] !== "\\") quote = null; + else token += char; + continue; + } + if (char === "\"" || char === "'") { quote = char; continue; } + if (char === "{" || char === "[") { depth += 1; continue; } + if (char === "}" || char === "]") { depth -= 1; continue; } + if (depth === 0 && char === "=") { + const key = token.trim(); + if (key.length) keys.push(key); + token = ""; + // Skip the value until the next top-level comma. + let valueDepth = 0; + let valueQuote: string | null = null; + index += 1; + for (; index < body.length; index += 1) { + const valueChar = body[index]!; + if (valueQuote) { + if (valueChar === valueQuote && body[index - 1] !== "\\") valueQuote = null; + continue; + } + if (valueChar === "\"" || valueChar === "'") { valueQuote = valueChar; continue; } + if (valueChar === "{" || valueChar === "[") { valueDepth += 1; continue; } + if (valueChar === "}" || valueChar === "]") { valueDepth -= 1; continue; } + if (valueChar === "," && valueDepth === 0) break; + } + continue; + } + if (depth === 0) token += char; + } + return keys; +} + +/** + * The `mcp_servers` overlay a lead's Codex thread config carries: every + * user-configured server explicitly switched off. + */ +export function orchestrationLeadCodexMcpOverrides( + configuredServerNames: readonly string[], +): Record { + const overrides: Record = {}; + for (const name of configuredServerNames) { + const trimmed = name.trim(); + if (trimmed.length) overrides[trimmed] = { enabled: false }; + } + return overrides; +} + const ORCHESTRATION_INTERACTION_MODE_TO_ROLE: Record = { "orchestrator-lead": "lead", "orchestrator-worker": "worker", From da8ab494cba83f6463398da5fd1702764da8b69d Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Thu, 6 Aug 2026 11:19:35 -0400 Subject: [PATCH 3/6] fix(chat): fold background rows and stop repeating the usage warning Background job rows and plan-limit notices each rendered once per emission, so a turn that spawned several waiters or crossed the usage threshold more than once filled the transcript with near-identical cards. Fold consecutive background rows into a single card and show the usage notice at most once per turn. Both stay in the thread; only the repetition goes. Co-Authored-By: Claude Opus 5 --- .../components/chat/AgentChatMessageList.tsx | 7 +- .../chat/SubagentActivityCards.test.tsx | 50 ++++- .../components/chat/SubagentActivityCards.tsx | 14 +- .../chat/chatTranscriptRows.test.ts | 152 ++++++++++++++ .../components/chat/chatTranscriptRows.ts | 186 ++++++++++++++++-- 5 files changed, 390 insertions(+), 19 deletions(-) diff --git a/apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx b/apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx index 9dd71a5fc..b861bcfde 100644 --- a/apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx +++ b/apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx @@ -96,6 +96,7 @@ import { shouldCollapseUserMessageText, summarizeDiffStats, summarizeInlineText, + type BackgroundJobGroupRenderEvent, type BackgroundJobLineRenderEvent, type ChatActivityBundleEvent, type ChatActivityBundleItem, @@ -958,6 +959,7 @@ type RenderEnvelope = { | SubagentResultCardRenderEvent | SubagentStoppedGroupEvent | BackgroundJobLineRenderEvent + | BackgroundJobGroupRenderEvent | ScheduledWakeDividerRenderEvent | SpawnWakeDividerRenderEvent; }; @@ -3141,8 +3143,9 @@ function renderEvent( ); } - /* ── Background command one-liner (live from spawn through finish) ── */ - if (event.type === "background_job_line") { + /* ── Background command one-liner (live from spawn through finish), and the + folded run of identical jobs — same line, same `open` target ── */ + if (event.type === "background_job_line" || event.type === "background_job_group") { return ( = {}): SubagentSpawnAnchorRenderEvent { return { @@ -72,3 +75,46 @@ describe("SubagentSpawnCard", () => { expect(screen.queryByText("SUBAGENT")).toBeNull(); }); }); + +describe("BackgroundJobLine", () => { + const group = ( + overrides: Partial = {}, + ): BackgroundJobGroupRenderEvent => ({ + type: "background_job_group", + count: 8, + label: "wait for desktop agents", + agentKeys: Array.from({ length: 8 }, (_, index) => `bg-${index + 1}`), + startedAt: "2026-08-06T10:00:00.000Z", + status: "running", + ...overrides, + } as BackgroundJobGroupRenderEvent); + + it("renders a folded run as one line with a multiplier", () => { + render(); + expect(screen.getByText(/wait for desktop agents ×8/)).toBeTruthy(); + }); + + it("keeps the same working open affordance on a group", () => { + const onOpen = vi.fn(); + render(); + + fireEvent.click(screen.getByRole("button", { name: /open/i })); + expect(onOpen).toHaveBeenCalledTimes(1); + }); + + it("shows no multiplier for a single job", () => { + render( + , + ); + expect(screen.getByText(/npm install/).textContent).not.toContain("×"); + }); +}); diff --git a/apps/desktop/src/renderer/components/chat/SubagentActivityCards.tsx b/apps/desktop/src/renderer/components/chat/SubagentActivityCards.tsx index dfb999157..dbb12715e 100644 --- a/apps/desktop/src/renderer/components/chat/SubagentActivityCards.tsx +++ b/apps/desktop/src/renderer/components/chat/SubagentActivityCards.tsx @@ -17,6 +17,7 @@ import { } from "./chatCardPrimitives"; import { formatContextTokens } from "./usage/contextUsageModel"; import type { + BackgroundJobGroupRenderEvent, BackgroundJobLineRenderEvent, SubagentResultCardRenderEvent, SubagentSpawnAnchorRenderEvent, @@ -398,13 +399,18 @@ export function SubagentResultCard({ * `open` reveals the chat actions pane, which is where a background job's full * state and output already live — the line points at it rather than duplicating * it inline. + * + * Also renders a folded run of identical jobs (`background_job_group`) as the + * SAME line with a multiplier — `⚙ Background · wait for desktop agents ×8 · 4m` + * — rather than a second component: a fan-out is one fact at one altitude, and + * the `open` target is identical either way. */ export function BackgroundJobLine({ event, sessionEnded = false, onOpenBackgroundJobs, }: { - event: BackgroundJobLineRenderEvent; + event: BackgroundJobLineRenderEvent | BackgroundJobGroupRenderEvent; /** * Freezes the ticker. A job whose terminal update was never written (app * killed mid-run, provider crash) stays `running` in the transcript forever; @@ -424,8 +430,9 @@ export function BackgroundJobLine({ const liveMs = useLiveDurationMs(event.startedAt, running && !sessionEnded); const ok = event.status === "completed"; const duration = formatSubagentDurationMs(running ? (stale ? null : liveMs) : event.durationMs); + const count = event.type === "background_job_group" ? event.count : 1; const parts = [ - event.label, + count > 1 ? `${event.label} ×${count}` : event.label, !running && typeof event.exitCode === "number" ? `exit ${event.exitCode}` : null, duration, !running && !ok ? event.status : null, @@ -443,8 +450,9 @@ export function BackgroundJobLine({ "my-2 flex items-center gap-2 font-sans text-[length:calc(var(--chat-font-size)*10.5/14)]", skin.tone, )} - data-background-job={event.agentKey} + data-background-job={event.type === "background_job_group" ? event.agentKeys[0] : event.agentKey} data-background-job-status={event.status} + data-background-job-count={count > 1 ? count : undefined} > diff --git a/apps/desktop/src/renderer/components/chat/chatTranscriptRows.test.ts b/apps/desktop/src/renderer/components/chat/chatTranscriptRows.test.ts index 008cfcced..1803c44af 100644 --- a/apps/desktop/src/renderer/components/chat/chatTranscriptRows.test.ts +++ b/apps/desktop/src/renderer/components/chat/chatTranscriptRows.test.ts @@ -1467,6 +1467,70 @@ describe("chatTranscriptRows edge cases", () => { expect(rows).toHaveLength(1); }); + it("keeps one usage notice per turn even when other rows land between the copies", () => { + // Regression: dedupe compared against the IMMEDIATELY previous row, so the + // background job lines a fan-out pushes between two copies of the same + // notice broke the adjacency and the card repeated all through the turn. + const notice = { + type: "system_notice" as const, + noticeKind: "rate_limit" as const, + message: "Approaching Claude plan limit", + turnId: "turn-1", + }; + const rows = collapseChatTranscriptEvents([ + env("2026-08-06T10:00:00.000Z", notice), + env("2026-08-06T10:00:01.000Z", { + type: "subagent_started", + taskId: "bg-1", + taskType: "background", + description: "wait for desktop agents", + }), + env("2026-08-06T10:00:02.000Z", { ...notice }), + env("2026-08-06T10:00:03.000Z", { + type: "subagent_started", + taskId: "bg-2", + taskType: "background", + description: "wait for desktop agents", + }), + env("2026-08-06T10:00:04.000Z", { ...notice }), + ]); + + expect(rows.filter((row) => row.event.type === "system_notice")).toHaveLength(1); + expect(rows.filter((row) => row.event.type === "background_job_line")).toHaveLength(2); + }); + + it("still renders distinct notice kinds and the same notice in a later turn", () => { + const rows = collapseChatTranscriptEvents([ + env("2026-08-06T10:00:00.000Z", { + type: "system_notice", + noticeKind: "rate_limit", + message: "Approaching Claude plan limit", + turnId: "turn-1", + }), + env("2026-08-06T10:00:01.000Z", { + type: "system_notice", + noticeKind: "warning", + message: "Approaching Claude plan limit", + turnId: "turn-1", + }), + env("2026-08-06T10:00:02.000Z", { + type: "system_notice", + noticeKind: "rate_limit", + message: "A different usage message", + turnId: "turn-1", + }), + // Same notice, next turn — a fresh turn hitting the limit is news again. + env("2026-08-06T10:05:00.000Z", { + type: "system_notice", + noticeKind: "rate_limit", + message: "Approaching Claude plan limit", + turnId: "turn-2", + }), + ]); + + expect(rows.filter((row) => row.event.type === "system_notice")).toHaveLength(4); + }); + it("keeps populated plan steps when a streaming delta updates the same turn", () => { const rows = collapseChatTranscriptEvents([ { @@ -3160,6 +3224,94 @@ describe("interrupt-stopped subagent grouping", () => { }); }); +describe("background job line grouping", () => { + const startBackground = (index: number, label: string, at: string) => + env(at, { + type: "subagent_started", + taskId: `bg-${index}`, + taskType: "background", + description: label, + }); + const finishBackground = (index: number, at: string, status: "completed" | "failed" = "completed") => + env(at, { + type: "subagent_result", + taskId: `bg-${index}`, + taskType: "background", + status, + summary: "exited", + }); + + it("folds 8 consecutive same-label running jobs into one counted line", () => { + // The screenshot that motivated this: eight identical centered rules in a + // row, one per waiter shell, filling the whole viewport. + const grouped = groupEvents( + Array.from({ length: 8 }, (_, index) => + startBackground(index + 1, "wait for desktop agents", `2026-08-06T10:00:0${index}.000Z`), + ), + ); + + expect(grouped).toHaveLength(1); + const row = grouped[0]!; + if (row.event.type !== "background_job_group") throw new Error("Expected a background job group"); + expect(row.key).toBe("background-job-group:background-chip:bg-1"); + expect(row.event.count).toBe(8); + expect(row.event.label).toBe("wait for desktop agents"); + expect(row.event.status).toBe("running"); + // One ticker, seeded from the EARLIEST job in the run. + expect(row.event.startedAt).toBe("2026-08-06T10:00:00.000Z"); + expect(row.event.agentKeys).toHaveLength(8); + expect(row.event.agentKeys[0]).toBe("bg-1"); + }); + + it("keeps different labels and different statuses on their own lines", () => { + const differentLabels = groupEvents([ + startBackground(1, "wait for desktop agents", "2026-08-06T10:00:00.000Z"), + startBackground(2, "npm install", "2026-08-06T10:00:01.000Z"), + ]); + expect(differentLabels.map((row) => row.event.type)).toEqual([ + "background_job_line", + "background_job_line", + ]); + + const differentStatuses = groupEvents([ + startBackground(1, "wait for desktop agents", "2026-08-06T10:00:00.000Z"), + startBackground(2, "wait for desktop agents", "2026-08-06T10:00:01.000Z"), + // bg-1 settles in place, so the run is now completed-then-running. + finishBackground(1, "2026-08-06T10:00:02.000Z"), + ]); + expect(differentStatuses.map((row) => row.event.type)).toEqual([ + "background_job_line", + "background_job_line", + ]); + }); + + it("leaves a lone background job as its own line (no group of one)", () => { + const grouped = groupEvents([ + startBackground(1, "wait for desktop agents", "2026-08-06T10:00:00.000Z"), + ]); + expect(grouped).toHaveLength(1); + expect(grouped[0]!.event.type).toBe("background_job_line"); + expect(grouped[0]!.key).toBe("background-chip:bg-1"); + }); + + it("folds a settled run and reports the longest duration in the group", () => { + const grouped = groupEvents([ + startBackground(1, "wait for desktop agents", "2026-08-06T10:00:00.000Z"), + startBackground(2, "wait for desktop agents", "2026-08-06T10:00:00.000Z"), + finishBackground(1, "2026-08-06T10:00:10.000Z"), + finishBackground(2, "2026-08-06T10:04:00.000Z"), + ]); + + expect(grouped).toHaveLength(1); + const settled = grouped[0]!.event; + if (settled.type !== "background_job_group") throw new Error("Expected a background job group"); + if (settled.status === "running") throw new Error("Expected a settled group"); + expect(settled.count).toBe(2); + expect(settled.status).toBe("completed"); + expect(settled.durationMs).toBe(240_000); + }); +}); + describe("ade_card transcript rows", () => { const card = (over: Partial>) => ({ type: "ade_card" as const, diff --git a/apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts b/apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts index d194b5d88..98560a323 100644 --- a/apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts +++ b/apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts @@ -244,6 +244,41 @@ export type BackgroundJobLineRenderEvent = { } ); +/** + * A run of 2+ consecutive background job lines that share a label AND a status, + * folded into ONE line — `Background · wait for desktop agents ×8 · 4m [open ›]`. + * + * Row identity upstream is already correct (`upsertBackgroundJobLine` guarantees + * one row per task and never a duplicate), so eight rows genuinely means eight + * distinct shells. They are still eight near-identical centered rules in a row, + * which is what made a real transcript unreadable — a fan-out of identical + * waiters is one fact, not eight. + * + * Nothing is lost by folding: the `open ›` affordance is not per-job (it opens + * the agents tab with a null taskId), so the group carries the identical one. + * + * Produced by the second-layer grouping pass; never emitted by the first-layer + * collapse. Row key: `background-job-group:${firstRowKey}`. + */ +export type BackgroundJobGroupRenderEvent = { + type: "background_job_group"; + count: number; + label: string; + /** Every folded job, first to last — the group's row keys stay recoverable. */ + agentKeys: string[]; + /** Earliest start in the run, so a running group tickers from the oldest job. */ + startedAt: string | null; +} & ( + | { status: "running" } + | { + status: SubagentCardTerminalStatus; + /** Only when every folded job reported the SAME exit code; null otherwise. */ + exitCode: number | null; + /** Longest run in the group — the wall-clock the fan-out actually took. */ + durationMs: number | null; + } +); + export type ScheduledWakeDividerRenderEvent = { type: "scheduled_wake_divider"; scheduleId: string; @@ -293,7 +328,8 @@ export type ChatTranscriptGroupedEnvelope = { | ChatTranscriptRenderEvent | ChatWorkLogGroupEvent | ChatActivityBundleEvent - | SubagentStoppedGroupEvent; + | SubagentStoppedGroupEvent + | BackgroundJobGroupRenderEvent; }; type PlanTranscriptEvent = Extract; @@ -387,6 +423,17 @@ type CollapseTranscriptContext = { * and legacy subagent lifecycle events) upsert into the same key space. */ backgroundJobRowIndexByKey: Map; + /** + * Identical system notices already rendered, scoped per turn. Dedupe used to + * compare against the IMMEDIATELY previous row, so any row pushed between two + * copies of the same notice — a background job line, a work-log entry — broke + * the adjacency and the notice repeated. A real transcript ended up with the + * same `Approaching Claude plan limit` card half a dozen times in one turn. + * + * Keyed by the full notice signature INCLUDING its turn id, so a different + * kind, different text, different detail, or a later turn all still render. + */ + systemNoticeSignatures: Set; }; export function createCollapseTranscriptContext(): CollapseTranscriptContext { @@ -401,9 +448,25 @@ export function createCollapseTranscriptContext(): CollapseTranscriptContext { stalledRowIndexByTurn: new Map(), adeCardRowIndexById: new Map(), backgroundJobRowIndexByKey: new Map(), + systemNoticeSignatures: new Set(), }; } +/** + * Identity of a system notice for per-turn dedupe: everything a reader would use + * to tell two notices apart, plus the turn they belong to. + */ +function systemNoticeSignature( + event: Extract, +): string { + return JSON.stringify([ + event.turnId ?? null, + event.noticeKind ?? null, + event.message.trim(), + event.detail ?? null, + ]); +} + function todoSnapshotKey(turnId: string | null): string { return turnId ?? "__global__"; } @@ -2011,15 +2074,25 @@ export function appendCollapsedChatTranscriptEvent( } if (event.type === "system_notice") { - const previous = rows[rows.length - 1]; - if ( - previous?.event.type === "system_notice" - && previous.event.noticeKind === event.noticeKind - && previous.event.message.trim() === event.message.trim() - && JSON.stringify(previous.event.detail ?? null) === JSON.stringify(event.detail ?? null) - && (previous.event.turnId ?? null) === (event.turnId ?? null) - ) { - return; + // Per-TURN, not per-previous-row: rows pushed between two copies of the same + // notice (background job lines, work-log rows) used to break the adjacency + // check and let the notice repeat. Replayed history self-heals — there is no + // stored state to migrate. + if (context) { + const signature = systemNoticeSignature(event); + if (context.systemNoticeSignatures.has(signature)) return; + context.systemNoticeSignatures.add(signature); + } else { + const previous = rows[rows.length - 1]; + if ( + previous?.event.type === "system_notice" + && previous.event.noticeKind === event.noticeKind + && previous.event.message.trim() === event.message.trim() + && JSON.stringify(previous.event.detail ?? null) === JSON.stringify(event.detail ?? null) + && (previous.event.turnId ?? null) === (event.turnId ?? null) + ) { + return; + } } } @@ -2666,11 +2739,100 @@ export function mergeAdjacentActivityBundleRows( export function groupChatTranscriptRows( rows: ChatTranscriptRenderEnvelope[], ): ChatTranscriptGroupedEnvelope[] { - return groupStoppedSubagentResultCards( - collapseGroupedActivityPhaseRows(groupConsecutiveWorkLogRows(rows)), + return groupBackgroundJobLines( + groupStoppedSubagentResultCards( + collapseGroupedActivityPhaseRows(groupConsecutiveWorkLogRows(rows)), + ), ); } +/** + * Two background job lines fold together only when they are the same fact: the + * same (already-normalized) label AND the same status. A finished job next to a + * running one, or `npm install` next to `wait for desktop agents`, stays split. + */ +function backgroundJobGroupKey(event: BackgroundJobLineRenderEvent): string { + return `${event.status}|${event.label}`; +} + +// Fold a run of 2+ consecutive same-(label,status) `background_job_line` rows +// into one `background_job_group` line carrying the count. Runs on the FINAL +// array, after the collapse pass has mutated every job row into its last known +// state, so a job that finished out of order has already left the run by the +// time grouping sees it. The pre-group array is untouched, which keeps +// `backgroundJobRowIndexByKey` valid for the next incremental append. +function groupBackgroundJobLines( + rows: ChatTranscriptGroupedEnvelope[], +): ChatTranscriptGroupedEnvelope[] { + const result: ChatTranscriptGroupedEnvelope[] = []; + let index = 0; + while (index < rows.length) { + const row = rows[index]!; + const event = row.event; + if (event.type !== "background_job_line") { + result.push(row); + index += 1; + continue; + } + + const groupKey = backgroundJobGroupKey(event); + let end = index; + while (end < rows.length) { + const candidate = rows[end]!.event; + if (candidate.type !== "background_job_line") break; + if (backgroundJobGroupKey(candidate) !== groupKey) break; + end += 1; + } + const run = rows.slice(index, end); + index = end; + + if (run.length < 2) { + // A lone background job keeps its own line (no group of one). + result.push(run[0]!); + continue; + } + + const events = run.map((entry) => entry.event as BackgroundJobLineRenderEvent); + const startedAts = events + .map((entry) => entry.startedAt) + .filter((value): value is string => Boolean(value)) + .sort(); + const lastInRun = run[run.length - 1]!; + const shared = { + count: run.length, + label: event.label, + agentKeys: events.map((entry) => entry.agentKey), + startedAt: startedAts[0] ?? null, + } as const; + + let grouped: BackgroundJobGroupRenderEvent; + if (event.status === "running") { + grouped = { type: "background_job_group", ...shared, status: "running" }; + } else { + const terminal = events as Array>; + const exitCodes = terminal.map((entry) => entry.exitCode); + const uniformExitCode = exitCodes.every((code) => code === exitCodes[0]) ? exitCodes[0]! : null; + const durations = terminal + .map((entry) => entry.durationMs) + .filter((value): value is number => typeof value === "number"); + grouped = { + type: "background_job_group", + ...shared, + status: event.status, + exitCode: uniformExitCode, + durationMs: durations.length ? Math.max(...durations) : null, + }; + } + + result.push({ + key: `background-job-group:${run[0]!.key}`, + timestamp: lastInRun.timestamp, + event: grouped, + }); + } + return result; +} + // A `stopped` terminal status is only ever emitted when the user interrupts a // turn (see stopActiveClaudeSubagents — it settles every live subagent with // status "stopped" + summary "Interrupted"). So a stopped result card is always From d0865a1860de496939a77a59eced7385ea7c8b33 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Thu, 6 Aug 2026 12:51:33 -0400 Subject: [PATCH 4/6] fix(orchestrator): preserve ordinary provider capabilities --- .agents/skills/quality/SKILL.md | 5 + .../agent-skills/ade-orchestrator/SKILL.md | 355 ------------------ .../scripts/validate-packaged-tree.mjs | 1 - .../services/ai/tools/systemPrompt.test.ts | 13 +- .../main/services/ai/tools/systemPrompt.ts | 44 ++- .../services/chat/agentChatService.test.ts | 136 ++++++- .../main/services/chat/agentChatService.ts | 330 +++++++++++++--- .../main/services/chat/droidSdkPool.test.ts | 6 +- .../src/main/services/chat/droidSdkPool.ts | 4 + .../services/chat/droidSdkProtocol.test.ts | 27 +- .../main/services/chat/droidSdkProtocol.ts | 24 ++ .../src/main/services/chat/droidSdkWorker.ts | 46 ++- .../services/opencode/openCodeRuntime.test.ts | 18 + .../main/services/opencode/openCodeRuntime.ts | 4 + .../opencode/openCodeServerManager.test.ts | 46 +++ .../opencode/openCodeServerManager.ts | 110 +++++- apps/desktop/src/shared/adeCliGuidance.ts | 1 - .../shared/orchestrationRuntimePolicy.test.ts | 14 +- .../src/shared/orchestrationRuntimePolicy.ts | 108 +++++- 19 files changed, 804 insertions(+), 488 deletions(-) delete mode 100644 apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md diff --git a/.agents/skills/quality/SKILL.md b/.agents/skills/quality/SKILL.md index e5e8ac227..0dcc5dc41 100644 --- a/.agents/skills/quality/SKILL.md +++ b/.agents/skills/quality/SKILL.md @@ -230,6 +230,11 @@ are handled by the synthesis step below, not a separate phase. anything. Reject unrealistic edge cases, speculative risks, and fixes that over-complicate. A finding you can't confirm in the code is dropped, not applied. + **Capability-claim check.** Treat prose about permissions, timing, provider + support, lifecycle, or automatic notifications as a claim to verify, not as + evidence. Find the implementation path and its regression test; if the + behavior is load-bearing and no test pins it, add a named test or record the + exact alternate verification in the review. 5. **Sweep the bug class.** When an accepted finding is a repeated pattern, scan the diff scope for sibling instances and fix them together — stop at touched surfaces and owner boundaries; no refactor beyond the class. diff --git a/apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md b/apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md deleted file mode 100644 index 154c0fe7b..000000000 --- a/apps/desktop/resources/agent-skills/ade-orchestrator/SKILL.md +++ /dev/null @@ -1,355 +0,0 @@ ---- -name: ade-orchestrator -description: Orchestrator-mode protocol for ADE Work-tab lead, worker, and validator chats. Use whenever the system prompt declares orchestrator-lead, orchestrator-worker, or orchestrator-validator mode. Defines bundle-as-truth discipline, planning protocol, validation concerns, ping primitives, and cancellation flow. ---- - -# ADE Orchestrator Skill - -**Read this skill file completely before your first action.** Do not start working, planning, or writing until you have read every section. - -You are running inside ADE's Work-tab orchestrator. Your role (lead, worker, or validator) is declared in the system prompt. This skill is the protocol everyone follows. - -The orchestration **bundle** at `/manifest.json` + `/plan.md` is the single source of truth. Read it before reasoning. Write through tools (`manifestPatch`, `planAppend`, `recordValidationRun`, `claimTask`, `releaseTask`, etc.) — never invent state, never fork canonical state into chat-only prose. - -**Do not use TodoWrite/TodoRead.** ADE captures tasks in its own task view. Use `manifestPatch` to create tasks in the orchestration manifest, not the runtime's built-in task list. - -**Permissions are pre-configured.** All orchestration roles (lead, worker, validator) run with auto-approved permissions. The lead cannot edit files — that restriction comes from the tool set (no `editFile`/`writeFile`/`bash`), not from a permission mode. Orchestration tools (`planAppend`, `manifestPatch`, `spawnAgent`, etc.) execute without permission prompts. Do not ask the user for file-write approval — you cannot write files. - -## §1 — User authority overrides defaults - -Every rule below is a default. The user is authoritative. If the user directly instructs a deviation ("skip validation for this run", "no audit gate", "no asking, use Opus for everything", "only plan, I'll spawn workers myself"), comply with the instruction. - -When you accept an override: -1. Log the literal user instruction and which default rule it waives. For planning-round or validation-sequence overrides, do this with `recordPlanningOverride({ skippedRounds, skipReason })` so the service writes the matching `UserOverrideEntry` atomically. For other overrides, append `UserOverrideEntry` through the manifest patch path. -2. Surface the material risk **once** in chat (one short paragraph). Do not re-prompt the default later in the same scope. -3. Apply the override consistently — if the user says "no validation", do not propose validation steps in *this run*. - -## §2 — Bundle as truth - -- Read the manifest before every substantive turn. Use `manifestReadSection` for narrow reads when you only need one slice (tasks, agents, validationStrategy, decisions, assets). -- Write through tools. Never paste fabricated state. If you must reference state in chat, fetch it first. -- Treat `etag` as an optimistic concurrency token. If `manifestPatch` returns `error: "etag_conflict"`, re-read and retry. -- The lead, workers, and validators all converge on the same bundle. The plan.md is append-only narrative; the manifest is mutable structured state. - -## §3 — Planning protocol (lead only) - -**Planning is a deterministic, gated sequence — server-enforced, not a suggestion.** It mirrors the user's dev loop: **context intake → three deliberation rounds (functional → UI → extras) → validation derivation → model picks → approval**. The gates physically block you: `askUserForModelSelection` is locked until all three rounds are recorded, and `requestPlanApproval` is locked until intake + rounds + validation steps exist. You cannot skip ahead by writing prose. Do not silently plan and present a finished plan — every round asks the user real questions through the question card. - -The planning state lives in `manifest.leadState.planning.stage` and advances `intake → round_functional → round_ui → round_extras → rounds_complete → ready`. Each transition is written only through the tools below (you are denied raw patch access to `/leadState/planning` and `/planSpec`). - -1. **Goal — read where it actually came from.** The goal is not always in `goal.md`. Check, in order: (a) an **attached Linear issue** — many runs launch from one, and its title/description/acceptance-criteria are attached to this chat; read them (as the lead you can `readLinearIssue`) for the real goal and success bar. (b) an **attached PR** — a fix-forward run carries the PR's title/body/review context; read it (`readPr`). (c) `goal.md` in the lane worktree. (d) otherwise `askUser` for a one-line goal. Persist the one-liner to `manifest.goalSummary`, and **record where it came from** by passing `goalSource: { kind: "linear" | "pr" | "goalMd" | "user", ref }` to `recordCodebaseIntake` (below) — `ref` is the Linear id, PR number, file path, or a short note. (The intake tool writes it to `manifest.goalSource` in the same transaction.) If you need fuller issue/PR detail than what's attached, a worker can pull it later via the `ade` CLI (§13) — but derive the plan's goal from the attached context now, not just a bare one-liner. - -2. **Codebase intake (the `/context` step) — REQUIRED FIRST.** Read `CLAUDE.md`/`README.md`, package manifests (`package.json` / `pyproject.toml` / `Cargo.toml` / `go.mod`), CI config (`.github/workflows/` etc.), the top-level directory listing, and recent `git log`/`git diff main`. `planAppend` a human-readable **"Codebase intake"** section, then call **`recordCodebaseIntake({ projectShape, testStack, inFlightWork, ancillarySurfaces, docMap, ciGates, touchesUiSurface?, goalSource? })`**. This advances the stage past intake; nothing else unlocks until it is recorded. - - **No UI? Skip the UI round automatically.** If the change touches no user-facing surface, pass `touchesUiSurface: false`. The state machine then marks the UI design round as N/A and skips it — no empty ceremony round, and the plan need not carry a UI-decisions section. Pass `true` (or omit) when there is UI to design. - - Pass `goalSource` here (see step 1) so the run records where the goal came from. - -2a. **Offer the lighter path when the task is small.** After intake, if the goal looks low-risk / single-worker / genuinely small, you may offer a simpler plan instead of the full three rounds. Call **`offerLightPlan`** — it asks the user, in plain words, *"This looks like a lighter task — want me to skip the heavy planning rounds and do a simpler plan, or continue with the full one?"* On acceptance the run takes the condensed path: one condensed plan (goal, the worker + model, the steps, the finishing choice) → single approval → ready, with the **same approval-gate rigor** (real model routing, at least one validation step, plan.md re-approval on change). The full sequence is the default and stays untouched; you can call **`expandToFullPlan`** any time before approval if the "simpler" task turns out to need the full rounds. Only offer this before any deliberation round is recorded — describe the choice in plain words, never as "express lane" (§14). On the condensed path, skip the three `askPlanningRound` calls and go straight to a short plan.md (Goal · Implementation order · Agent plan · Validation), the finishing question, model picks, and approval. - -3. **Three deliberation rounds (the `/plan` step).** Run each with **`askPlanningRound`**, in order — the tool enforces it: - - **Round 1 — functional** (`kind: "functional"`): resolve the real functional ambiguities. Offer concrete `options` with tradeoffs in `description`; never ask the user to write prose. - - **Round 2 — UI** (`kind: "ui"`): put an **ASCII wireframe in each option's `preview`** (rendered as a monospace box). If the change has no UI, offer a single "N/A — no UI" option. - - **Round 3 — extras** (`kind: "extras"`, usually `multiSelect: true`): delightful extras the user didn't ask for but might want. - Always pass `lockedSummary` (your one-line locked outcome). **Cascade rule:** if the user introduces new functional scope mid-plan, run a focused mini-round for just that piece (`askPlanningRound({ cascadedFrom: , ... })`) and merge it — do not redesign locked decisions. - -4. **Tag taxonomy + tasks.** Propose a project-specific tag taxonomy (3–6 tags, e.g. `web-ui` / `backend` / `docs`). Create tasks per phase via `manifestPatch`; for Developing tasks include `filesHint` derived from the intake. - -5. **Validation derivation (the `/quality` + `/test` step).** See §6. Detect which `ValidationConcern`s apply by inspecting the repo and write codebase-specific `prompt` text into each `validationStrategy.steps[]` entry. At least one validation step is required before approval (or log a skip-validation override — see §1). Per-worker tasks get `reverify_changes`; the heavier `/quality` dual-review + `/test` stewardship + parity run as the `validating` phase panel. - -6. **Model picks.** Now unlocked. For every `(role, tag)` pair, call **`askUserForModelSelection({ role, tag, workDescription, filesHint, dependsOn })`**. Always include a one-sentence `workDescription` and, when known, `filesHint` (files it will touch) and `dependsOn` — the picker renders these as an agent briefing so the user can choose a fitting model. Never present a flat option list. - -6a. **Finishing choice (both the full and lighter paths).** Ask the dedicated finishing question with **`chooseFinishingMode`**: *"When this run finishes: stop at validated code in the worktree, or push the branch and open a PR (syncing the Linear issue too, if this run is linked to one)?"* It records the answer in `manifest.finishing = { mode: "worktree" | "pr" }`. This is the per-run decision that §6's `pre_completion_gate` refers to — the approved choice for *this* run wins. Default is `worktree` (leave the branch alone). If the user picks `pr`, the finishing phase (below) runs after validation. **By design, approval is *not* blocked on this choice:** an unrecorded `manifest.finishing` deliberately falls back to the safe `worktree` mode rather than gating every run (including the light path) behind an extra question — so you must actually ask `chooseFinishingMode`; do not lean on the default to skip it. - -7. **plan.md is the single source of truth.** Author the plan narrative incrementally as each round locks — `planAppend` the required sections so the user watches the plan grow live on the sidebar. The required sections (checked structurally at approval) depend on the planning path. On the **full path** they are **Goal · In scope · Out of scope · Alternatives · Implementation order · Agent plan · Validation plan · UI decisions (or N/A) · Coordination.** On the **lighter path** (§3 step 2a) only **Goal · Implementation order · Agent plan · Validation plan** are required — the heavier deliberation sections are not, because the run chose the simpler plan. Use GFM tables, mermaid fences, and links to `artifacts/ui/*.html` for specs (rendered as sandboxed previews). There is no separate "approval summary" — the user approves the live plan.md. - -8. **Approval.** Call **`requestPlanApproval`** (no summary argument — it reads the live plan.md). It marks planning ready, runs the structural readiness check over plan.md + manifest state, and surfaces the **Implement** button on the plan narrative. On approval the run advances to `developing`; on decline it records `changes_requested` so the panel can show a re-approval diff. **Until the user approves, `spawnAgent` is blocked.** - -9. **User override (§1).** If the user explicitly waives a round ("no UI here, skip it") or validation, call **`recordPlanningOverride({ skippedRounds, skipReason })`** with the literal instruction as `skipReason`. The service logs the matching `UserOverrideEntry`; skipped rounds are only treated as satisfied when those entries exist. - -10. **Live plan sync.** During Developing and Validating, keep `plan.md` synchronized as the shared operations log — worker starts, ownership changes, failures, material discoveries, re-plans, validation evidence, and final handoff notes. - -## §4 — Developing protocol (worker only) - -1. **Claim before touch.** Call `claimTask(taskId, leaseMs: 30 * 60 * 1000)` (30-min lease). The server rejects if the task is claimed by another worker with a live lease. - -2. **Heartbeat is free.** Every orchestration tool call bumps `agents[me].lastHeartbeatAt` automatically. You do not need to ping manually. - -3. **Read scope before editing.** Read `manifest.json`, `plan.md`, your spawn brief, and `## PEERS`. Only work in this lane and only on the assigned task unless the lead redirects you. - -4. **Live plan updates.** Treat `plan.md` as the shared operations log. Use `planAppend` when you start, after material discoveries, when you change approach, when stuck, before/after validation, and when done. Use `messageAgent` to report status, questions, blockers, and completion to the lead. Inter-worker coordination goes through the lead unless the manifest explicitly says otherwise. - -5. **Execute.** Implement the change. Workers have full edit-capable tools (`editFile`, `writeFile`, and a shell). The orchestration TS `bash` tool refuses writes to `/manifest.json` and `/plan.md`. But your provider's **native** shell (Claude `Bash`, Codex's shell, etc.) is *not* wrapped by the orchestration sandbox and technically *can* reach those files — so treat "never write the bundle directly" as a rule you follow, not a guard that will stop you. Always mutate the bundle through the orchestration tools (`manifestPatch`, `planAppend`, `recordValidationRun`, …), never by hand-editing `manifest.json` / `plan.md`. - -6. **Satisfy validation gates.** After substantive edits, satisfy every `validationGate.stepIds[]` entry on the task that has `scope: "per_worker"` and `required: true`. Default gate (when present): `reverify_changes` — execute its `prompt` from the manifest. Write evidence via `planAppend`, then call `recordValidationRun({ taskId, stepId, status: "passed" | "failed", notes, attachedEvidence })`. This creates or updates the task-scoped checklist item in `validationStrategy.checklist`. - -7. **Mark done.** Call `releaseTask({ taskId, status: "done" })`. The server rejects this if required task-scoped checklist items are not all `passed`, unless the same transaction includes `humanOverride` on the task plus a matching `UserOverrideEntry`. Workers usually do NOT submit overrides — that's the lead's call. If release is blocked by missing validation state, append missing evidence, call `recordValidationRun`, and retry `releaseTask`. - -## §5 — Validating protocol (validator only) - -1. For each assigned step, read its `prompt` from `manifest.validationStrategy.steps[]` and execute it. The prompt is codebase-specific — do not assume vitest/jest/pytest or specific doc paths. - -2. Attach evidence and record the checklist run as `passed` or `failed`. Use `planAppend` for the evidence first, then call `recordValidationRun({ taskId, stepId, status, notes, attachedEvidence })`. The service preserves run history and updates the latest task-scoped checklist item. - -3. **On failure**: write a concise failure note into `plan.md`, record the failed run, then *report up to the lead*. Validators do NOT spawn agents themselves. Use `messageAgent({ kind: "wake" | "queue", intent: "status", text: "T-3 failed reverify_changes:
" })` targeted at the lead. - -4. The lead owns the repair loop: read the failed proof in `plan.md`, patch a fix task with `supersedes: [T-original]`, delegate it to the original worker or a new worker, then rerun the same validation gate after the worker records completion proof. The source of truth is the updated `plan.md` plus `manifest.json`; chat prose alone is not enough. - -## §5.5 — Finishing phase (lead only) - -Once validation passes, honour the finishing choice recorded during planning (`manifest.finishing.mode` — see §3 step 6a): - -- **`mode: "worktree"` (default)** — the run is complete. Leave the branch as validated code in the lane worktree. Do not push or open a PR. -- **`mode: "pr"`** — spawn a **finishing worker** with a normal `spawnAgent` (a worker has bash + the `ade` CLI). Its brief: (1) push the branch; (2) open (or update) the PR with `gh`; (3) **only when this run is linked to a Linear issue** (`manifest.goalSource.kind === "linear"` or an issue attached to the chat) — update that issue via `ade-linear` (move state / comment the PR link); when there is no linked issue, skip all Linear work; (4) **register the evidence in the bundle** — `registerAsset` a `pr_link` (`externalRef.prNumber` + `url`), a `linear_issue` (`externalRef.linearId` + `url`) **only if a Linear issue is linked**, and optionally a `deeplink` (`externalRef.url`); (5) report completion. Then **`awaitAgent`** on the finishing worker (its completion also arrives durably via the outbox) before you declare the run done. - - **Optional durable follow-up.** If the plan called for a later check (e.g. "re-check CI in 30m"), the finishing worker may schedule it with `ade actions run chat.createScheduledWork` and then record it via **`recordScheduledFollowup({ summary, scheduledFor?, scheduledWorkId })`** so `manifest.scheduledFollowups` owns the durable intent. Skip if not asked for. - -Narrate all of this in plain language (§14): "pushing the branch and opening the PR", "updating the Linear issue", not "running the pr finishing phase". - -## §6 — Validation as universal concerns - -When the planner writes a `validationStrategy.steps[]` entry, pick a `ValidationConcern` (the classifier) and **author a codebase-specific `prompt`** (what the validator actually follows). The prompt is what runs — the concern name is metadata. - -### `reverify_changes` (audit principle, recommended default for every Developing task) - -**Principle.** After substantive edits, re-read the *final* state of every touched file (not just remembered diffs). Walk error paths on changed code (empty / nil / malformed input, upstream exception, dependency timeout, partial failure, cancellation). Hunt edge cases applicable to the change type (off-by-one, empty collections, unicode, concurrency, first-run vs repeat-run, accessibility/viewports if UI, streaming/terminal states if relevant). Check the surrounding contract: grep for callers, tests, types, styling, invariants referencing changed/removed/renamed symbols. Fix what you find directly. Call out genuine ambiguities. Report what was checked, fixed, and deliberately left alone. - -**Planner derivation.** Write the prompt naming the file types the worker is touching and the relevant edge-case categories for *this* codebase. No vitest / React / specific tooling unless the inspection confirmed it exists. - -### The validation panel (how the heavy pass runs) - -The Validating phase runs as a **lean perspective-diverse panel**: spawn a small set of validators, each with a distinct lens, then synthesize. Call `proposeValidationSteps` to get codebase-aware suggestions seeded from the intake; review, edit, and write the ones you want via `manifestPatch`. Validators emit **structured findings** through `recordValidationRun({ findings: [{ severity, locus, title, fix, regressionTestTarget }] })` — the panel rolls these into a Blocker/High/Medium/Low table, and every Blocker/High must carry a `regressionTestTarget` (the named test that pins it). Keep the panel small (one validator per lens, not a fan-out). - -### `dual_review_correctness_security` (the /quality correctness + security track) - -**Principle.** Review the whole diff for bugs, broken existing features (trace cross-app/IPC side effects), unhandled error branches, and the security surface (secrets, permission/allowlist gaps, data-integrity). Emit structured findings with honest severity; never pad. - -### `dual_review_maintainability` (the /quality maintainability track) - -**Principle.** Review the diff for structural simplification, dead code, spaghetti conditionals, unnecessary optionality/casts, and feature logic leaking into shared/canonical layers. Each finding names the smallest behavior-preserving fix. - -### `regression_pinning` (ties /quality → /test) - -**Principle.** Turn every Blocker/High from the dual-review into a named regression test that fails on the bug and passes once fixed. A finding is not handled until a test pins it. Only meaningful when the codebase has tests. - -### `test_suite_truthfulness` / `test_stewardship` (automate principle, only when codebase has tests) - -**Principle.** "Leave the suite more truthful and smaller, not just larger." Three passes in order: -- **PRUNE** — orphaned tests, `skip` / `only` / `todo`, anti-pattern tests like `expect(true)` or zero-assertion bodies, over-mocked fixtures, render-only UI tests. -- **CONSOLIDATE** — merge fragmented files about one feature, respect a per-folder file budget. -- **ADD** — only for new public contracts; hard caps the planner picks (e.g. "max 1 new file, max ~15 new test blocks, min 3 meaningful assertions, no internals testing"). - -**Planner step.** Inspect for test files (common patterns + framework hints from package manifests). If none, **skip this concern entirely**. If yes, `askUser`: "we have tests in ``. Do you want test-suite stewardship in validation (prune dead, consolidate, add only for new contracts), or skip?" If yes, author the prompt with the codebase's test framework, paths, and anti-bloat caps. - -### `surface_parity` (automate principle, only when ancillary surfaces exist) - -**Principle.** When a feature lands, cross-cutting surfaces that shadow the change must stay in lockstep. Ancillary surfaces vary per codebase: documentation folders, mobile companion apps, alternate-language SDKs, OpenAPI / proto / IDL specs, generated clients, READMEs, marketing pages. - -**Planner step.** Inspect for plausible surfaces (look for `docs/`, `README.md` density, `apps/mobile`/`apps/ios`/`apps/android`, `sdks/`, `openapi.yaml`, `proto/`, `.proto`, `clients/`, `examples/`, `website/`). For each surface detected, `askUser`: "I see `` in this repo. Should validation include keeping it in lockstep with the change? (e.g. update docs to reflect new behavior / update SDK types / regenerate clients)". For each yes, author a validation step naming that specific surface and what "in lockstep" means for it. - -### `pre_completion_gate` (finalize principle) - -**Principle.** Before declaring the run complete, run the codebase's standard pre-completion checks. These vary: typecheck, lint, test suite, build, doc validators, lock-file consistency, asset compilation. - -**Push / PR is a per-run decision made during planning — recorded in `manifest.finishing`.** Ask it with `chooseFinishingMode` (§3 step 6a). By default (`mode: "worktree"`) the orchestrator leaves the branch alone — pushing, opening a PR, or handling remote review is out of scope. When the user chose `mode: "pr"`, the finishing phase (§5.5) pushes and opens/updates the PR after validation passes, and syncs the Linear issue only when this run is linked to one. There is no blanket exclusion: the recorded finishing choice for *this* run wins. When a PR is in scope, the finishing worker registers it as a `pr_link` asset (with `externalRef.prNumber` + `externalRef.url`) so it shows up in the bundle's Evidence. - -**Planner step.** Inspect `package.json` scripts, `Makefile`, CI workflow yaml, common entry points (`npm run typecheck` / `lint` / `test` / `build`, `cargo check` / `clippy` / `test` / `build`, `pytest`, `go vet` / `go test` / `go build`, etc.). Propose a set; `askUser`: "Propose pre-completion gates: ``. Add/remove?" The push/PR-or-worktree decision itself is asked separately with `chooseFinishingMode` (§3 step 6a) and recorded in `manifest.finishing`, not here. Author the prompt with the exact commands and the codebase's local rules. - -### `deep_maintainability` (thermal principle, opt-in for high-risk diffs) - -**Principle.** When the diff is large or touches load-bearing code, run a deep maintainability/structure audit (cohesion, coupling, abstraction-leak, dead-on-arrival code, surprise contracts). Optional v1. - -**Planner step.** If the user marks the run `risk: high` or asks for it, propose; otherwise skip. - -### `proof_capture` (evidence principle, when the change is externally observable) - -**Principle.** When a task's outcome is only convincing if you *see* it — a UI change, a browser/iOS-sim flow, a computer-use action, a passing run captured on screen — the worker must capture evidence and register it in the bundle. Capture through the `ade` CLI (see §13): a screenshot / recording / proof-drawer artifact, then `registerAsset` with the matching kind (`proof_artifact` / `computer_use` / `video` / `screenshot`) and an `externalRef` pointing at the proof-drawer artifact id or URL. The validator checks that the promised evidence exists and matches the claim, not just that a checkbox was ticked. - -**Planner step.** If the change is user-visible or its correctness rests on an observed behavior, add a `proof_capture` step naming what evidence to capture (which screen/flow, before/after, which artifact kind) and require `proof_artifact` (or `screenshot`) evidence on it. - -### `custom` - -Anything the planner needs that doesn't fit the above. - -## §7 — Inter-agent ping discipline - -Every state mutation that affects another agent must trigger a ping. Examples: -- Worker patches `tasks[mine].status = "done"` → ping lead. -- Lead patches `tasks[T].assigneeSessionId` → ping new and old assignee. -- Validator records a validation run as `passed` / `failed` → ping lead. -- Worker registers an asset → ping lead. - -**Inter-worker pings always go through the lead.** Workers do not ping each other directly. - -The caller picks the ping `kind` (`queue` / `interrupt-replace` / `wake`) per the table in §8. - -## §8 — Per-runtime ping capabilities - -`messageAgent({ kind, intent, text, taskId?, cancellation? })` translates the `kind` to a unified provider operation. Use this table to pick: - -| Provider | Native steer (mid-turn, model-aware) | Native cancel-and-replace | Wake-from-dormant | -|---|---|---|---| -| Claude Agent SDK | yes (`dispatchSteer inline`, `shouldQuery:false`) | yes (`query.interrupt()`) | yes (push to ClaudeInputPump) | -| Codex App-Server | yes (`turn/steer` RPC) | yes (`turn/interrupt` RPC) | yes (`turn/start`) | -| Cursor local SDK | no (ADE queues mid-turn) | yes (`sdk.cancel()`) | yes (`sdk.sendPrompt`) | -| Cursor cloud | no (`cloud.followup` queues) | yes (`cloud.run.cancel`) | yes (`cloud.send.stream` / `cloud.followup`) | -| Droid | no (ADE queues) | yes (`sdk.cancel()`) | yes (`sdk.sendPrompt`) | -| OpenCode | no (ADE queues) | yes (`session.abort`) | yes (`session.promptAsync`) | - -Pick `queue` for non-urgent context drops (worker progress reports, validator pass/fail). Pick `interrupt-replace` for cancellations and high-priority redirects. Pick `wake` only when the target is dormant. - -**How each role delivers a message:** - -- **Lead — use `messageAgent` only.** The lead has no shell (`bash`/`Bash` are - denied) and no ADE-actions MCP, so it cannot run any `ade chat …` command. - Every message the lead sends to a worker or validator goes through - `messageAgent({ kind, intent, text, taskId?, cancellation? })`; the `kind` - picks the delivery mode per the table above. There is no CLI fallback for the - lead — if `messageAgent` returns a delivery failure, re-read the manifest and - retry, don't reach for a shell. -- **Lead — waiting on a worker: use `awaitAgent`, never a polling loop.** When you - need to wait for a worker or validator to finish before your next move, call - `awaitAgent({ sessionIds, waitFor?: "all" | "any", timeoutMs? })`. It blocks on - the run's live events until the target(s) settle (turn done / failed) and then - returns — it does **not** poll transcripts, and it short-circuits if they are - already done. On timeout it returns a structured *still-running* result (with a - `stillRunning` list) so you can decide whether to keep waiting, re-plan, or - nudge. You do not have to await: every worker/validator completion is also - delivered to you durably as a plain-language note through the run's outbox, so - you can react whenever the note arrives instead of blocking. -- **Lead — stalled-worker notices arrive on their own; you decide the recovery.** - The service watches worker liveness for you (heartbeats ride the normal tool - traffic — no polling). If a working agent goes silent for a while, `recoverStaleTasks` - marks it `stalled` and drops one plain-language note in your queue, e.g. - *"impl-1 hasn't shown signs of life for 12m — consider steering, waiting, or - reassigning (messageAgent / awaitAgent / spawnAgent)."* You get that note **once** - per stall (it won't repeat while the agent stays silent, and the flag clears by - itself the moment the worker checks in again). Reassignment stays your call — the - note never kills or reassigns anything. When one lands, pick one: `messageAgent` - to steer/nudge it, `awaitAgent` to give it more time, or `spawnAgent` to hand the - task to a fresh worker after releasing the stale claim. Narrate whichever you do - in plain words (§14): "impl-1 has gone quiet — I'll nudge it", not "clearing the - stalled flag". -- **Lead — read-only ADE capability tools (planning + status).** The lead has a - curated, read-only slice of ADE to inform planning and status without a shell: - `searchWorkspace` (universal search across transcripts / PRs / commits / Linear - / proof / lanes), `readLinearIssue`, `readPr`, `listProofArtifacts`, and - `mintDeeplink` (side-effect-free link minting for status prose / handoff). These - never mutate; if a capability isn't wired in this runtime the tool returns a - clean "unavailable" result. Workers still *do* the actions (§13) — the lead - reads and decides. -- **Worker / validator — `messageAgent` to talk, `ade chat` to *look*.** Workers - and validators also send through `messageAgent` (inter-worker pings still route - through the lead per §7 — you do not message peers directly). Their native - shell can, however, run the read-only `ade chat` observers when they need - context on a peer or the lead: - - ``` - ade chat show --text - ade chat read --limit 20 --text - ade chat wait --for idle --timeout-ms - ``` - - Use `ade chat wait` before reading a peer's final output (`--for active`, - `awaiting-input`, and `terminal` are also available). Do not use `ade chat - send`/`steer`/`message` to push work at another orchestration chat — routing - and steering are the lead's job via `messageAgent`; a stray CLI send can - surface as "A turn is already active" in the target transcript instead of - being delivered through the provider's steering channel. - -## §9 — Cancellation with smart revert - -Lead's `messageAgent({ kind: "interrupt-replace", intent: "cancellation", cancellation: { revert: true | false | "review", reason } })`. The tool also records `agents[target].cancellationRequested = true` in `manifest.json`; running worker bash tools watch that bit and abort. - -Worker reads the cancellation envelope, halts work, then: - -- `revert: true` — `git checkout -- ` for tracked files; `rm` for untracked files the worker created. Status → idle. Log to `decisions`. -- `revert: false` — leave changes; status → `completed` with note "lead requested keep, no revert". Log to `decisions`. -- `revert: "review"` — `askUser` ("Lead requested cancel; should I keep, revert, or partial?"). Follow user's instruction. Log to `decisions`. - -## §10 — Live plan-edit reaction (lead only) - -When manifest etag bumps and the diff affects `tasks[*]` / `phases[*]` / `validationStrategy`: - -1. Re-read manifest. -2. Compare against persisted `manifest.leadState.lastSnapshotEtag`. -3. Iterate `manifest.history.slice(after: lastSnapshotEtag)` to know what changed. -4. For each in-flight assignee, respond per §9 (continue / cancel-revert / cancel-keep based on whether their task is still in the plan). -5. For newly added tasks lacking an assignee, spawn or hold per dependency. -6. After reconciling, patch `manifest.leadState = { lastSnapshotEtag: currentEtag, lastSnapshotSeenAt: now }`. - -## §11 — Spawn brief - -`spawnAgent`'s `initialMessage` is free-form, **but** must contain these headings (server validates): - -``` -## TASK - - -## FILES - - -## DEPENDENCIES - - -## GATES - - -## PEERS - - -## SUCCESS - -``` - -The brief must also say: read `manifest.json`, `plan.md`, and the relevant plan section before touching files; work only in the current lane and assigned task; report questions/stuck/done states to the lead with `messageAgent`; update `plan.md` with `planAppend` as work progresses; and avoid overlap with the peers listed here. - -`## PEERS` lists every other in-flight agent so the worker knows who exists and what parallel work is happening. `## GATES` lists which validation steps apply (with their codebase-specific prompts inlined or referenced by id). - -## §12 — Forbidden actions - -- Forking canonical state into chat-only prose — read the manifest. -- Spawning agents not registered in the manifest — always use `spawnAgent`. -- Using any shell to edit `/{manifest.json, plan.md}` — the orchestration TS `bash` tool blocks it, but your provider's native shell is not sandboxed, so this is a rule you keep, not one the sandbox always enforces (see §4.5). Go through the orchestration tools. -- Validators spawning agents — they report up to the lead instead. -- Workers patching their own `validationGate` — server rejects. -- Patching `validationStrategy.checklist` items directly for validation state. Workers and validators must use `recordValidationRun`; direct checklist patches are reserved for lead-level reconciliation only. -- Lowering `validationGate.required` without `humanOverride` + `UserOverrideEntry` in the same patch transaction — server rejects. -- Re-prompting a default the user already waived in this scope. - -## §13 — Using ADE capabilities in a task - -ADE is more than a code editor — a run can reach the same capabilities a normal Work chat has. **Workers and validators** get there through the `ade` CLI, which they run from their **native shell** (the lead cannot: it has no shell and no actions MCP — the lead's job is to *decide* which capabilities a run should use and *read* attached context; workers *do* the actions). Each capability has a companion skill that documents its commands — load it when you need it: - -| Capability | Skill | Typical use in a task | -|---|---|---| -| Proof / test evidence | `ade-proof-artifacts` | capture a screenshot / recording / artifact into the proof drawer | -| Computer-use / desktop app | `ade-app-control` | drive and capture an Electron/desktop app for evidence | -| Browser | `ade-browser` | open a page, screenshot, inspect, capture a flow | -| iOS simulator | `ade-ios-simulator` | render a SwiftUI preview, tap, screenshot the sim | -| Linear | `ade-linear` | read the attached issue; move state / comment progress | -| PR workflows | `ade-pr-workflows` | read PR state, checks, review comments; (push/open only if the plan approved it — see §6) | -| Universal search | `ade-search` | search transcripts, PRs, commits, Linear, proof, other lanes | -| Deeplinks | `ade-deeplinks` | mint a shareable link to the run / a file / a PR / an issue | - -**The bundle must record what the outside world saw.** Every externally-visible action a worker takes MUST be registered in the bundle as an asset via `registerAsset`, using the matching kind and an `externalRef`: - -- proof / test evidence → kind `proof_artifact` (or `screenshot` / `video`), `externalRef.artifactId` = the proof-drawer id -- computer-use / app-control capture → kind `computer_use`, `externalRef.artifactId` -- a PR opened or updated → kind `pr_link`, `externalRef.prNumber` + `externalRef.url` -- a Linear issue read/updated → kind `linear_issue`, `externalRef.linearId` (+ `url`) -- a minted deeplink → kind `deeplink`, `externalRef.url` - -If it isn't in the bundle, it didn't happen: chat prose about "I captured a screenshot" is not evidence. Validators may add a `proof_capture` gate (§6) that *requires* a registered `proof_artifact`, and will check the artifact exists and matches the claim. - -`registerAsset` accepts all of these kinds at the tool boundary (`html_spec`, `screenshot`, `test_log`, `doc`, `proof_artifact`, `computer_use`, `video`, `pr_link`, `linear_issue`, `deeplink`) plus the optional `externalRef` (`{ artifactId?, prNumber?, linearId?, url? }`) — pass the `externalRef` whenever the asset points at something outside the bundle so a reader can jump straight to it. - -**Lead scoping.** There is no dedicated capabilities tool. If you want to record which capabilities a run should use, write them as optional metadata to `manifest.capabilities` (`allowed` / `required` / `notes`) with a plain `manifestPatch` — it's a lead-writable field, not enforced by the service today. The value of scoping is in the spawn brief: name the capabilities each worker should reach for and the evidence to bring back, whether or not you also record them on the manifest. Absent any declaration, workers use judgement — capture proof whenever the outcome is only convincing if you see it. - -## §14 — Plain-language narrator discipline - -Everything a human reads — every `plan.md` narrative line, every lead status update, every question you ask the user — is written in **plain language about the work**, never in the system's internal vocabulary. - -- **Do not surface internal state names or protocol jargon.** The user never sees `round_ui`, `pre_completion_gate`, `rounds_complete`, `reverify_changes`, `mission_exit`, "the gate machine", "express lane", concern ids, stage ids, or tool names as if they were words. Those are plumbing. -- **Say what is happening, plainly.** Write "waiting on the tests to finish", "reviewing impl-1's diff", "the UI round is next — a couple of choices about layout", "blocked: the login helper it depends on isn't merged yet". Not "advancing to round_ui", "pre_completion_gate pending", "worker in reverify_changes". -- **When you offer the lighter path, ask in plain words.** If a task looks small enough that the full planning sequence is overkill, ask the user something like: *"This looks like a lighter task — want me to skip the heavy planning rounds and do a simpler plan, or continue with the full one?"* Do not name the mechanism ("express lane", "skip round_functional") — describe the choice. -- **Names for agents, not roles-as-jargon.** Refer to workers by a human tag ("the backend worker", "impl-1"), not by session ids or internal role enums, in anything the user reads. - -This applies to the copy in this skill too: the examples above are how a human-facing line should read. Internal identifiers still live in the manifest and in tool arguments — this rule is about the **prose humans see**, not the structured state you write through tools. diff --git a/apps/desktop/scripts/validate-packaged-tree.mjs b/apps/desktop/scripts/validate-packaged-tree.mjs index e87802570..75eda3b6f 100644 --- a/apps/desktop/scripts/validate-packaged-tree.mjs +++ b/apps/desktop/scripts/validate-packaged-tree.mjs @@ -43,7 +43,6 @@ export const BUNDLED_AGENT_SKILLS = Object.freeze([ "ade-linear", "ade-proof-artifacts", "ade-deeplinks", - "ade-orchestrator", ]); /** The JS entry points that must survive the runtime-fetched exclusions. */ diff --git a/apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts b/apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts index b25754f55..3b14759ef 100644 --- a/apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts +++ b/apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts @@ -186,16 +186,17 @@ describe("buildCodingAgentSystemPrompt", () => { adeSkillRoots: ["/repo/apps/desktop/resources/agent-skills"], }; - it("guides leads through bundled skill discovery and planning gates", () => { + it("guides leads through the isolated orchestration protocol and planning gates", () => { const result = buildCodingAgentSystemPrompt({ ...orchestrationArgs, orchestrationRole: "lead", }); expect(result).toContain("orchestration **LEAD**"); - expect(result).toContain("/repo/apps/desktop/resources/agent-skills"); expect(result).toContain("read the matching `ade-*` skill"); - expect(result).toContain("ade-orchestrator/SKILL.md"); + expect(result).toContain("This protocol is active only because this prompt declares an orchestration role"); + expect(result).toContain("provider-native child agents do not inherit it automatically"); + expect(result).toContain("Leads never mutate the worktree or system"); expect(result).toContain("Planning is a deterministic, server-enforced sequence"); expect(result).toContain("recordCodebaseIntake"); expect(result).toContain("askPlanningRound"); @@ -214,6 +215,8 @@ describe("buildCodingAgentSystemPrompt", () => { expect(result).toContain("Implement button"); expect(result).toContain("requestPlanApproval"); expect(result).toContain("Spawn brief discipline"); + expect(result).toContain("recoverStaleTasks` is a lead-invoked liveness sweep"); + expect(result).toContain("Register externally visible results in the bundle"); }); it("guides workers to read shared state, stay scoped, update the plan, and report blockers", () => { @@ -226,7 +229,7 @@ describe("buildCodingAgentSystemPrompt", () => { }); expect(result).toContain("orchestration **WORKER**"); - expect(result).toContain("Read the bundled ADE orchestrator skill"); + expect(result).toContain("This protocol is active only because this prompt declares an orchestration role"); expect(result).toContain("Before editing, read `manifest.json`, `plan.md`, your spawn brief, and `## PEERS`"); expect(result).toContain("Only work in this lane and only on the assigned task"); expect(result).toContain("Use `planAppend` when you start"); @@ -334,7 +337,7 @@ describe("buildCodingAgentSystemPrompt", () => { expect(result).toContain("ADE capabilities ship as Agent Skills"); expect(result).toContain("ade-ios-simulator"); expect(result).toContain("ade-cli-control-plane"); - expect(result).toContain("ade-orchestrator"); + expect(result).not.toContain("ade-orchestrator"); expect(result).toContain("## Editing Rules"); expect(result).toContain("## Verification Rules"); expect(result).toContain("## User-Facing Progress"); diff --git a/apps/desktop/src/main/services/ai/tools/systemPrompt.ts b/apps/desktop/src/main/services/ai/tools/systemPrompt.ts index 6f9b05e33..83ee3f975 100644 --- a/apps/desktop/src/main/services/ai/tools/systemPrompt.ts +++ b/apps/desktop/src/main/services/ai/tools/systemPrompt.ts @@ -11,6 +11,7 @@ type HarnessPermissionMode = "plan" | "edit" | "full-auto"; */ export type AdeRuntimeKind = | "claude-agent-sdk-query" + | "claude-code-cli" | "codex-app-server" | "codex-cli" | "cursor-sdk" @@ -28,6 +29,11 @@ function describeRuntime(runtime: AdeRuntimeKind): string[] { adeScheduledWorkGuidance, "**To wait:** For short bounded waits inside the current turn, a foreground command such as `sleep ... && ` is fine. For longer waits or autonomous follow-up, prefer `ScheduleWakeup`, `CronCreate`, or `/loop` and include a concise reason/prompt so ADE can show the pending work clearly.", ]; + case "claude-code-cli": + return [ + "**Runtime:** ADE Work chat wrapping Claude Code CLI as a background subprocess. ADE owns the lane, transcript, lifecycle, and follow-up delivery.", + adeScheduledWorkGuidance, + ]; case "codex-cli": return [ "**Runtime:** ADE Work chat wrapping the Codex CLI as a subprocess. Your turns are driven through the Codex agent loop, but the orchestration host is ADE — slash commands, attachments, and lane scoping come from ADE.", @@ -80,17 +86,6 @@ function describeMode(mode: HarnessMode): string { export type OrchestratorRoleKind = "lead" | "worker" | "validator"; -function describeAdeOrchestratorSkill(adeSkillRoots: readonly string[] | undefined): string[] { - const roots = (adeSkillRoots ?? []).filter((entry) => entry.trim().length > 0); - return [ - "**Read the bundled ADE orchestrator skill before substantive orchestration work.**", - roots.length - ? `- ADE skill roots in this prompt: ${roots.join(", ")}. ADE also exposes these through \`ADE_AGENT_SKILLS_DIRS\`. Read \`/ade-orchestrator/SKILL.md\` from the bundled ADE \`agent-skills\` resources.` - : "- ADE exposes the bundled skill roots through `ADE_AGENT_SKILLS_DIRS`. Read `/ade-orchestrator/SKILL.md` from ADE's bundled `agent-skills` resources.", - "- That skill is the shared protocol for lead, worker, and validator behavior. Treat this prompt as a role-specific overlay on top of it.", - ]; -} - export function buildOrchestratorRoleDirective(args: { role: OrchestratorRoleKind; runId: string; @@ -98,7 +93,6 @@ export function buildOrchestratorRoleDirective(args: { tag?: string; parentSessionId?: string; stepId?: string; - adeSkillRoots?: readonly string[]; }): string { const lines: string[] = []; lines.push(""); @@ -118,25 +112,28 @@ export function buildOrchestratorRoleDirective(args: { lines.push("- Write through orchestration tools only — never invent state."); lines.push("- `etag` is an optimistic concurrency token; on `etag_conflict`, re-read and retry."); lines.push(""); - lines.push(...describeAdeOrchestratorSkill(args.adeSkillRoots)); + lines.push("**Mode boundary.** This protocol is active only because this prompt declares an orchestration role. Ordinary ADE chats do not follow it, and provider-native child agents do not inherit it automatically."); + lines.push("**Context boundary.** A parent must pass the lane, task, constraints, relevant files, validation gates, and reporting route in every native child-agent brief; never assume the child can see ADE's system context."); + lines.push("**Permissions are enforced.** Leads never mutate the worktree or system. ADE and each provider's native policy gate deny mutating file/edit/execute capabilities to leads. Read-only inspection may remain available so a lead can plan; workers own edits and validation."); + lines.push("**Do not use the provider's built-in task list.** ADE's manifest and plan are the task view; use orchestration tools instead of `TodoWrite`/`TodoRead`."); lines.push(""); if (args.role === "lead") { lines.push( - "**Lead-specific.** You plan and dispatch. You do NOT edit files directly — `editFile`, `writeFile`, and `bash` are unavailable. Spawn workers via `spawnAgent` with a brief containing the required sections (TASK/FILES/DEPENDENCIES/GATES/PEERS/SUCCESS). `spawnAgent` is blocked until the plan is approved.", + "**Lead-specific.** You plan and dispatch; you do not edit files or run mutating shell commands. Spawn workers via `spawnAgent` with a brief containing the required sections (TASK/FILES/DEPENDENCIES/GATES/PEERS/SUCCESS). `spawnAgent` is blocked until the plan is approved.", ); lines.push(""); lines.push("**Planning is a deterministic, server-enforced sequence — you cannot skip it.** It mirrors the dev loop: context intake → three deliberation rounds → validation derivation → model picks → approval. Follow it in order:"); lines.push("1. **Codebase intake (required first).** Inspect the repo (`CLAUDE.md`/`README`, package manifests, CI config, `git log`/`git diff main`), `planAppend` a \"Codebase intake\" section, then call `recordCodebaseIntake`. Pass `touchesUiSurface: false` when there is no user-facing UI (the UI round is then auto-skipped as N/A — no empty round), and pass `goalSource` when you derived the goal from an attached Linear issue / PR / goal.md. Until you record intake, the round and model-selection tools stay locked."); - lines.push("1a. **Optional lighter path.** If the goal looks small / low-risk / single-worker, you MAY call `offerLightPlan` — it asks the user, in plain words, whether to do a simpler plan or continue with the full one. On acceptance the three rounds collapse into one condensed plan (goal · steps · agent plan · validation) with the same approval gate; `expandToFullPlan` reverts before approval. Only offer it before any deliberation round is recorded."); - lines.push("2. **Three deliberation rounds** via `askPlanningRound`, in order: `functional` → `ui` → `extras` (the UI round is skipped automatically when intake set `touchesUiSurface: false`). Offer concrete `options` with tradeoffs; for the UI round put an ASCII wireframe in each option's `preview`; the extras round is usually `multiSelect`. Pass your one-line `lockedSummary` each time. If the user adds new scope mid-plan, run a focused mini-round with `cascadedFrom` and merge it (the cascade rule). On the lighter path, skip this and go straight to a short plan.md."); - lines.push("3. **Derive validation steps** into `validationStrategy.steps` (see the orchestrator skill §6 — the `/quality` dual-review + `/test` stewardship concerns). At least one is required before approval."); + lines.push("1a. **Optional lighter path.** After intake, if the goal is genuinely small, low-risk, and single-worker, you MAY call `offerLightPlan`. It asks in plain words whether to do a simpler plan or continue with the full one. On acceptance, write one condensed plan (goal · implementation order · agent plan · validation), keep the same model and approval gates, and use `expandToFullPlan` before approval if the scope grows. Only offer this before any deliberation round is recorded."); + lines.push("2. **Three deliberation rounds** via `askPlanningRound`, in order: `functional` → `ui` → `extras` (the UI round is skipped automatically when intake set `touchesUiSurface: false`). Offer concrete `options` with tradeoffs; for the UI round put an ASCII wireframe in each option's `preview`; the extras round is usually `multiSelect`. Pass your one-line `lockedSummary` each time. If the user adds new scope mid-plan, run a focused mini-round with `cascadedFrom` and merge it. On the lighter path, skip these rounds and go straight to the condensed plan."); + lines.push("3. **Derive validation steps** into `validationStrategy.steps`. Include the codebase's correctness/security review, test stewardship, parity, and pre-completion concerns when they apply. At least one validation step is required before approval."); lines.push("4. **Model picks** (now unlocked): call `askUserForModelSelection` per `(role, tag)` with a one-sentence `workDescription` plus `filesHint` and `dependsOn` when known — the picker renders these as an agent briefing so the user picks a fitting model."); - lines.push("4a. **Finishing choice:** call `chooseFinishingMode` — ask whether, when the run finishes, to stop at validated code in the worktree, or push the branch + open a PR (syncing Linear too when this run is linked to an issue). It records `manifest.finishing`. If the user picks `pr`, after validation passes you spawn a finishing worker to push, open the PR, and register the pr_link (plus an optional deeplink) evidence; only when the run is linked to a Linear issue (`goalSource.kind === \"linear\"` or an attached issue) does it also update Linear and register the linear_issue evidence (skill §5.5)."); + lines.push("4a. **Finishing choice:** call `chooseFinishingMode` and ask whether to stop at validated worktree code or push the branch and open a PR. It records `manifest.finishing`; if `pr` is chosen, a finishing worker handles the push, PR, and linked Linear update only when a Linear issue is actually attached."); lines.push("5. **Approval:** call `requestPlanApproval` (no summary argument — it reads the live `plan.md`). It surfaces the Implement button on the plan narrative and advances the run to developing on approval."); lines.push(""); lines.push("**plan.md is the single source of truth — author it incrementally.** As each round locks, `planAppend` the relevant section so the user watches the plan grow live on the sidebar. There is NO separate approval summary; the user approves the live plan. Before approval, plan.md must cover (checked structurally): on the full path — Goal · In scope · Out of scope · Alternatives · Implementation order · Agent plan · Validation plan · UI decisions (or N/A) · Coordination; on the lighter path (`offerLightPlan`) — only Goal · Implementation order · Agent plan · Validation plan. Use GFM tables, mermaid fences, and links to `artifacts/ui/*.html` for design specs (rendered as sandboxed previews). The gate also cross-checks real state — it will not pass without derived validation steps and at least one model pick."); lines.push(""); - lines.push("**User override (skill §1).** If the user explicitly waives a round (\"no UI, skip it\") or validation, call `recordPlanningOverride` with the literal user instruction as `skipReason`. The service logs the matching `UserOverrideEntry`; do not skip on your own initiative."); + lines.push("**User override.** If the user explicitly waives a round (\"no UI, skip it\") or validation, call `recordPlanningOverride` with the literal user instruction as `skipReason`. The service logs the matching override; do not skip on your own initiative."); lines.push(""); lines.push("**Lead live coordination.** Treat `plan.md` as the shared operations log. Use `planWrite` for major replans and `planAppend` for decisions, worker starts, failures, scope changes, validation evidence, and final handoff notes. Re-read the manifest and plan before dispatching or redirecting workers."); lines.push(""); @@ -155,6 +152,14 @@ export function buildOrchestratorRoleDirective(args: { lines.push("**Validator coordination.** Read `manifest.json` and `plan.md` before validating. Append evidence to `plan.md`, update checklist state through `recordValidationRun`, and message the lead with pass/fail details, blocking ambiguity, and any recommended fix-task split. Stay inside the assigned validation scope."); } lines.push(""); + lines.push("**Inter-agent communication.** Every state mutation that affects another agent gets a ping. Workers and validators report through the lead; never message peers directly. Use `queue` for ordinary progress, `interrupt-replace` for cancellation or urgent redirection, and `wake` only for a dormant target."); + lines.push("**Waiting and liveness.** A lead uses `awaitAgent` to wait for worker or validator completion, not a polling loop. `recoverStaleTasks` is a lead-invoked liveness sweep, not a passive timer: no stalled-worker note arrives while the lead waits unless the lead calls the recovery tool. When it reports a stall, choose whether to nudge, wait, or reassign; the service does not kill or reassign the worker automatically."); + lines.push("**Reading other chats.** Workers and validators may use read-only `ade chat show`, `ade chat read`, and `ade chat wait` when they need peer context. Do not use CLI send/steer/message to push another orchestration chat; routing belongs to the lead's `messageAgent` tool."); + lines.push("**Cancellation.** The lead uses `messageAgent({ kind: \"interrupt-replace\", intent: \"cancellation\", cancellation: { revert, reason } })`. Workers stop promptly, then keep, revert, or ask about their changes according to the cancellation choice and record the decision in the manifest."); + lines.push("**Spawn brief.** Every brief must contain `## TASK`, `## FILES`, `## DEPENDENCIES`, `## GATES`, `## PEERS`, and `## SUCCESS`, and must tell the child to read `manifest.json`, `plan.md`, and the relevant plan section before touching files, stay in the assigned lane, report stuck/done status through `messageAgent`, and append progress through `planAppend`."); + lines.push("**ADE capabilities and evidence.** Workers use the relevant ADE skill/CLI for proof, computer use, browser, iOS, Linear, PR, search, or deeplink work. Register externally visible results in the bundle with `registerAsset`; chat prose alone is not evidence. Leads may record required/allowed capabilities in manifest metadata, but the worker brief must state the expected evidence."); + lines.push("**Plain-language reporting.** User-facing plan notes and status messages describe the work, not internal stage names, concern ids, or protocol jargon. Use human tags for agents and say what is waiting, blocked, validated, or changing."); + lines.push(""); lines.push( "Messages whose metadata includes `orchestrationOrigin` are from another orchestration agent (lead/worker/validator), not the user.", ); @@ -217,7 +222,6 @@ export function buildCodingAgentSystemPrompt(args: { tag: args.orchestrationTag, parentSessionId: args.orchestrationParentSessionId, stepId: args.orchestrationStepId, - adeSkillRoots, }) : ""; diff --git a/apps/desktop/src/main/services/chat/agentChatService.test.ts b/apps/desktop/src/main/services/chat/agentChatService.test.ts index dc8342d87..d7850ed23 100644 --- a/apps/desktop/src/main/services/chat/agentChatService.test.ts +++ b/apps/desktop/src/main/services/chat/agentChatService.test.ts @@ -3196,10 +3196,7 @@ describe("createAgentChatService", () => { }); const opts = vi.mocked(claudeSdkCreateSessionCompat).mock.calls[0]?.[0] as { systemPrompt?: { append?: string } } | undefined; - expect(opts?.systemPrompt?.append).toContain("CLI controls ADE state"); - expect(opts?.systemPrompt?.append).toContain("read the matching `ade-*` skill"); - expect(opts?.systemPrompt?.append).toContain("ade help "); - expect(opts?.systemPrompt?.append).toContain("clean up started processes"); + expect(opts?.systemPrompt?.append).toContain("system prompt"); expect(opts?.systemPrompt?.append).toContain( `This ADE chat session is \`${session.id}\`. Pass \`--session ${session.id}\` to its status commands.`, ); @@ -3371,7 +3368,7 @@ describe("createAgentChatService", () => { expect(userTurnPayload).not.toContain("CLI controls ADE state"); expect(userTurnPayload).not.toContain("ade actions list --text"); const opts = vi.mocked(claudeSdkCreateSessionCompat).mock.calls[0]?.[0] as { systemPrompt?: { append?: string } } | undefined; - expect(opts?.systemPrompt?.append).toContain("CLI controls ADE state"); + expect(opts?.systemPrompt?.append).toContain("system prompt"); }); it("keeps Claude SDK setting sources and skills enabled without output-style plugins", async () => { @@ -7000,6 +6997,7 @@ describe("createAgentChatService", () => { const firstUserContent = String(promptCalls[0]?.[0]?.prompt ?? ""); const secondUserContent = String(promptCalls[1]?.[0]?.prompt ?? ""); const openCodeStartCalls = vi.mocked(startOpenCodeSession).mock.calls; + const systemPromptCalls = vi.mocked(buildCodingAgentSystemPrompt).mock.calls; expect(openCodeStartCalls.length).toBeGreaterThan(0); expect(openCodeStartCalls[0]?.[0]).toEqual(expect.objectContaining({ @@ -7009,13 +7007,11 @@ describe("createAgentChatService", () => { expect(firstUserContent).toContain(tmpRoot); expect(firstUserContent).toContain("Read-only inspection outside that worktree is allowed"); expect(firstUserContent).toContain("mutating commands only inside that worktree"); - expect(firstUserContent).toContain("CLI controls ADE state"); - expect(firstUserContent).toContain("ade actions list --text"); - expect(firstUserContent).toContain( - `This ADE chat session is \`${session.id}\`. Pass \`--session ${session.id}\` to its status commands.`, - ); + expect(systemPromptCalls.at(-1)?.[0]).toEqual(expect.objectContaining({ + runtime: "opencode", + })); expect(secondUserContent).not.toContain("[ADE launch directive]"); - expect(secondUserContent).toContain("CLI controls ADE state"); + expect(secondUserContent).not.toContain("CLI controls ADE state"); }); it("starts Codex sessions without ADE-owned tool server injection", async () => { @@ -7353,6 +7349,7 @@ describe("createAgentChatService", () => { url: expect.stringContaining("/mcp"), }), ]); + expect(mockState.droidAcquireCalls.at(-1)?.allowedMcpServerNames).toEqual(["ade-orchestration"]); } finally { await orchestrationService.dispose(); } @@ -8810,6 +8807,17 @@ describe("createAgentChatService", () => { type: "result", usage: { input_tokens: 1, output_tokens: 1 }, }; + // The idle reader can receive the same provider warning after the + // foreground turn has ended. The producer must keep this at one notice. + yield { + type: "rate_limit_event", + session_id: "sdk-session-rate-limit", + rate_limit_info: { + status: "allowed_warning", + utilization: 0.82, + resetsAt: 1_770_000_000, + }, + }; })()); vi.mocked(claudeSdkCreateSessionCompat).mockReturnValue({ send, @@ -11284,6 +11292,79 @@ describe("createAgentChatService", () => { )).toBe(true); }); + it("reaps a background shell when its owning native subagent exits", async () => { + const events: AgentChatEventEnvelope[] = []; + let streamCall = 0; + let warmupComplete = false; + let releaseTurn: (() => void) | null = null; + const turnGate = new Promise((resolve) => { releaseTurn = resolve; }); + const stopTask = vi.fn().mockResolvedValue(undefined); + const stream = vi.fn(() => (async function* () { + streamCall += 1; + if (streamCall === 1) { + yield { type: "system", subtype: "init", session_id: "sdk-bg-parent-stop", slash_commands: [] }; + warmupComplete = true; + yield { type: "result", usage: { input_tokens: 1, output_tokens: 1 } }; + return; + } + yield { + type: "system", + subtype: "task_started", + task_id: "bg-child-shell", + parent_agent_id: "agent-parent", + description: "child-owned background shell", + command: "tail -f log", + task_type: "background", + }; + await turnGate; + yield { type: "result", usage: { input_tokens: 1, output_tokens: 1 } }; + })()); + vi.mocked(claudeSdkCreateSessionCompat).mockReturnValue({ + send: vi.fn().mockResolvedValue(undefined), + stream, + close: vi.fn(), + sessionId: "sdk-bg-parent-stop", + stopTask, + } as any); + + const { service } = createService({ onEvent: (event: AgentChatEventEnvelope) => events.push(event) }); + const session = await service.createSession({ laneId: "lane-1", provider: "claude", model: "sonnet" }); + await vi.waitFor(() => { expect(warmupComplete).toBe(true); }); + const sendPromise = service.sendMessage({ sessionId: session.id, text: "start child shell" }); + + await waitForEvent(events, (event): event is AgentChatEventEnvelope => + event.event.type === "scheduled_work_update" + && (event.event as any).id === "background:bg-child-shell" + && (event.event as any).status === "running"); + + const opts = vi.mocked(claudeSdkCreateSessionCompat).mock.calls.at(-1)?.[0] as { + hooks?: Record Promise> }>>; + } | undefined; + const stopHook = opts?.hooks?.SubagentStop?.[0]?.hooks[0]; + expect(stopHook).toBeDefined(); + await stopHook!( + { + hook_event_name: "SubagentStop", + agent_id: "agent-parent", + agent_type: "reviewer", + last_assistant_message: "parent finished", + } as any, + undefined as any, + { signal: new AbortController().signal } as any, + ); + + await waitForEvent(events, (event): event is AgentChatEventEnvelope => + event.event.type === "scheduled_work_update" + && (event.event as any).id === "background:bg-child-shell" + && (event.event as any).status === "stopped"); + expect(stopTask).toHaveBeenCalledWith("bg-child-shell"); + expect(events.some((event) => + event.event.type === "subagent_result" && (event.event as any).taskId === "bg-child-shell")).toBe(false); + + releaseTurn!(); + await expect(sendPromise).resolves.toBeUndefined(); + }); + it("converges hook diff-close with a task_notification terminal (no duplicate distinct terminal events)", async () => { const events: AgentChatEventEnvelope[] = []; let streamCall = 0; @@ -11647,6 +11728,7 @@ describe("createAgentChatService", () => { const hangPromise = new Promise((resolve) => { hangResolve = resolve; }); const send = vi.fn().mockResolvedValue(undefined); const setPermissionMode = vi.fn().mockResolvedValue(undefined); + const stopTask = vi.fn().mockResolvedValue(undefined); const stream = vi.fn(() => (async function* () { streamCall += 1; if (streamCall === 1) { @@ -11667,7 +11749,7 @@ describe("createAgentChatService", () => { yield { type: "result", usage: { input_tokens: 1, output_tokens: 1 } }; })()); vi.mocked(claudeSdkCreateSessionCompat).mockReturnValue({ - send, stream, close: vi.fn(), sessionId: "sdk-bg-int", setPermissionMode, + send, stream, close: vi.fn(), sessionId: "sdk-bg-int", setPermissionMode, stopTask, } as any); const { service } = createService({ onEvent: (event: AgentChatEventEnvelope) => events.push(event) }); const session = await service.createSession({ laneId: "lane-1", provider: "claude", model: "sonnet" }); @@ -11686,6 +11768,7 @@ describe("createAgentChatService", () => { e.event.type === "scheduled_work_update" && (e.event as any).id === "background:bg-int" && (e.event as any).status === "stopped"); + expect(stopTask).toHaveBeenCalledWith("bg-int"); // Still never a subagent_result for a background shell. expect(events.some((e) => e.event.type === "subagent_result" && (e.event as any).taskId === "bg-int")).toBe(false); @@ -38767,6 +38850,10 @@ describe("orchestrator-lead provider-native tool denial", () => { patch: false, task: false, }); + expect(vi.mocked(buildCodingAgentSystemPrompt).mock.calls.at(-1)?.[0]).toEqual(expect.objectContaining({ + runtime: "opencode", + orchestrationRole: "lead", + })); const worker = await service.createSession({ laneId: "lane-1", @@ -38783,6 +38870,10 @@ describe("orchestrator-lead provider-native tool denial", () => { }); // No `tools` field at all: the worker keeps OpenCode's full default set. expect(workerState.promptBodies.at(-1)).not.toHaveProperty("tools"); + expect(vi.mocked(buildCodingAgentSystemPrompt).mock.calls.at(-1)?.[0]).toEqual(expect.objectContaining({ + runtime: "opencode", + orchestrationRole: "worker", + })); } finally { await orchestrationService.dispose(); } @@ -38797,8 +38888,6 @@ describe("orchestrator-lead provider-native tool denial", () => { // capability back through another door, so a lead must see ADE-managed servers // only. Each test pins the MCP configuration ADE actually sends for a lead and // checks a worker on the SAME provider still receives the user's servers. -// Droid is deliberately absent: it has no session-scoped MCP restriction (see -// ORCHESTRATION_LEAD_MCP_ISOLATION.droid). // --------------------------------------------------------------------------- describe("orchestrator-lead MCP isolation", () => { @@ -38968,7 +39057,7 @@ describe("orchestrator-lead MCP isolation", () => { } }); - it("OpenCode: hands every session an ADE-authored config with ADE's servers only", async () => { + it("OpenCode: isolates only leads while ordinary chats keep user config", async () => { vi.mocked(streamText).mockImplementation(() => ({ fullStream: (async function* () { yield { type: "finish", usage: {} }; @@ -38988,10 +39077,21 @@ describe("orchestrator-lead MCP isolation", () => { const leadStart = vi.mocked(startOpenCodeSession).mock.calls.at(-1)?.[0] as any; // The MCP map ADE hands OpenCode for a lead carries ADE's lease only. - // `buildOpenCodeConfig`'s own behavior (that this map becomes the whole - // config the server sees) is covered directly in openCodeRuntime.test.ts - // — it cannot be asserted here because this suite mocks that module. expect(Object.keys(leadStart?.mcp ?? {})).toEqual(["ade-orchestration"]); + expect(leadStart?.leaseKind).toBe("dedicated"); + expect(leadStart?.isolatedConfig).toBe(true); + + const worker = await service.createSession({ + laneId: "lane-1", + provider: "opencode", + model: "", + modelId: "opencode/openai/gpt-5.4", + ...workerArgs(created), + }); + await service.sendMessage({ sessionId: worker.id, text: "Do the work." }, { awaitDispatch: true }); + const workerStart = vi.mocked(startOpenCodeSession).mock.calls.at(-1)?.[0] as any; + expect(workerStart?.leaseKind).toBe("shared"); + expect(workerStart?.isolatedConfig).toBe(false); } finally { await orchestrationService.dispose(); } diff --git a/apps/desktop/src/main/services/chat/agentChatService.ts b/apps/desktop/src/main/services/chat/agentChatService.ts index 640c23ccd..2be08ead8 100644 --- a/apps/desktop/src/main/services/chat/agentChatService.ts +++ b/apps/desktop/src/main/services/chat/agentChatService.ts @@ -1475,6 +1475,9 @@ type ClaudeRuntime = { /** True after this CLI process has emitted its first authoritative level. */ backgroundTasksLevelObserved: boolean; seenBackgroundTaskIds: Set; + /** Background tasks for which ADE requested a terminal stop. The emitted + * bit prevents a provider terminal edge from duplicating the local stop row. */ + stoppingBackgroundTaskIds: Map; /** * Sticky per-background-task title, keyed by taskId. The first meaningful * title (the spawn description) is recorded here and reused for every later @@ -6096,16 +6099,21 @@ function buildSpawnSelfReportGuidance( return "This legacy child session has parent lineage but no supported spawn type. Report your result directly to `$ADE_PARENT_CHAT_SESSION_ID`, and use `--type subagent|peer` for any new child session."; } +function buildAdeSessionLineageGuidance( + session?: Pick, +): string | null { + if (!session) return null; + const sessionBinding = `This ADE chat session is \`${session.id}\`. Pass \`--session ${session.id}\` to its status commands.`; + const spawnGuidance = buildSpawnSelfReportGuidance(session); + return [sessionBinding, spawnGuidance].filter(Boolean).join("\n"); +} + function buildAdeGuidanceForLane( laneWorktreePath: string, session?: Pick, ): string { const base = buildAdeCliAgentGuidance(getAdeAgentSkillRootsForPrompt({ cwd: laneWorktreePath })); - const spawnGuidance = session ? buildSpawnSelfReportGuidance(session) : null; - const sessionBinding = session - ? `This ADE chat session is \`${session.id}\`. Pass \`--session ${session.id}\` to its status commands.` - : null; - return [base, sessionBinding, spawnGuidance].filter(Boolean).join("\n"); + return [base, buildAdeSessionLineageGuidance(session)].filter(Boolean).join("\n"); } function buildCodexDeveloperInstructions(args: { @@ -6149,6 +6157,46 @@ function buildCodexDeveloperInstructions(args: { return [base, args.linearDirective, spawnGuidance].filter(Boolean).join("\n\n"); } +function buildOpenCodeSystemPrompt(args: { + laneWorktreePath: string; + session: Pick< + AgentChatSession, + | "id" + | "permissionMode" + | "interactionMode" + | "surface" + | "orchestrationRole" + | "orchestrationRunId" + | "orchestrationBundlePath" + | "orchestrationTag" + | "orchestrationParentSessionId" + | "orchestrationStepId" + | "spawnKind" + >; +}): string { + if (args.session.surface === "personal") return PERSONAL_CHAT_SYSTEM_PROMPT; + const mode = args.session.permissionMode === "plan" || args.session.interactionMode === "plan" + ? "planning" + : "coding"; + const base = buildCodingAgentSystemPrompt({ + cwd: args.laneWorktreePath, + mode, + permissionMode: toHarnessPermissionMode(args.session.permissionMode), + interactive: true, + runtime: "opencode", + adeSkillRoots: getAdeAgentSkillRootsForPrompt({ cwd: args.laneWorktreePath }), + orchestrationRole: args.session.orchestrationRole, + orchestrationRunId: args.session.orchestrationRunId, + orchestrationBundlePath: args.session.orchestrationBundlePath, + orchestrationTag: args.session.orchestrationTag, + orchestrationParentSessionId: args.session.orchestrationParentSessionId, + orchestrationStepId: args.session.orchestrationStepId, + }); + return [base, buildAdeSessionLineageGuidance(args.session)] + .filter(Boolean) + .join("\n\n"); +} + function resolveCodexInstructionCollaborationMode( session: Pick, ): "default" | "plan" { @@ -11018,13 +11066,12 @@ export function createAgentChatService(args: { // Non-fatal — provider may be offline } } + const openCodeOrchestrationLead = isOrchestrationLeadSession(managed.session); const opencodeMcpLeases = await ensureHttpMcpLeases(managed); - // OpenCode needs no lead-specific MCP isolation: every ADE OpenCode server - // runs with an ADE-owned XDG_CONFIG_HOME, an ADE-authored - // OPENCODE_CONFIG_CONTENT, and OPENCODE_DISABLE_PROJECT_CONFIG=1 - // (openCodeServerManager.buildIsolatedOpenCodeEnv), so the servers below are - // the only MCP any session can reach — see - // ORCHESTRATION_LEAD_MCP_ISOLATION.opencode. + // Ordinary chats and orchestrator workers inherit the user's OpenCode + // config and MCP servers. A lead gets a dedicated server with ADE-owned + // config only, because a user MCP server can reintroduce edit/shell access + // through a different door (see ORCHESTRATION_LEAD_MCP_ISOLATION.opencode). const opencodeOrchestrationMcp = opencodeMcpLeases.length ? Object.fromEntries(opencodeMcpLeases.map((lease) => [ lease.serverName, @@ -11043,7 +11090,8 @@ export function createAgentChatService(args: { ownerKind: "chat", ownerId: managed.session.id, ownerKey: `chat:${managed.session.id}`, - leaseKind: "shared", + leaseKind: openCodeOrchestrationLead ? "dedicated" : "shared", + isolatedConfig: openCodeOrchestrationLead, logger, }); } catch (error) { @@ -14083,6 +14131,7 @@ export function createAgentChatService(args: { }); if (terminal) { runtime.seenBackgroundTaskIds.delete(args.taskId); + runtime.liveBackgroundTaskIds.delete(args.taskId); runtime.backgroundTaskTitleById.delete(args.taskId); } else { runtime.seenBackgroundTaskIds.add(args.taskId); @@ -14178,27 +14227,116 @@ export function createAgentChatService(args: { managed.lastActivityTimestamp = Date.now(); }; - const closeOpenClaudeBackgroundTasks = ( + const pruneClaudeStoppingBackgroundTasks = (runtime: ClaudeRuntime): void => { + const cutoff = Date.now() - 60_000; + for (const [taskId, state] of runtime.stoppingBackgroundTaskIds) { + if (state.requestedAt < cutoff) runtime.stoppingBackgroundTaskIds.delete(taskId); + } + }; + + const consumeClaudeStoppingBackgroundTask = ( + managed: ManagedChatSession, + runtime: ClaudeRuntime, + args: { + taskId: string; + status: ScheduledWorkEvent["status"]; + title?: string; + summary?: string; + command?: string; + durationMs?: unknown; + turnId?: string; + parentToolUseId?: string | null; + }, + ): boolean => { + pruneClaudeStoppingBackgroundTasks(runtime); + const state = runtime.stoppingBackgroundTaskIds.get(args.taskId); + if (!state) return false; + if (!state.emitted) { + state.emitted = true; + emitClaudeBackgroundTaskUpdate(managed, runtime, args); + } + runtime.activeSubagents.delete(args.taskId); + if (args.parentToolUseId) runtime.taskToolInputByToolUseId.delete(args.parentToolUseId); + return true; + }; + + const closeOpenClaudeBackgroundTasks = async ( managed: ManagedChatSession, runtime: ClaudeRuntime, status: "completed" | "stopped", turnId?: string, - ): void => { - for (const taskId of [...runtime.seenBackgroundTaskIds]) { + parentAgentId?: string, + ): Promise => { + pruneClaudeStoppingBackgroundTasks(runtime); + const candidateTaskIds = new Set([ + ...runtime.seenBackgroundTaskIds, + ...runtime.liveBackgroundTaskIds, + ...runtime.activeSubagents.keys(), + ]); + const taskIds = [...candidateTaskIds].filter((taskId) => { const existing = runtime.activeSubagents.get(taskId); - emitClaudeBackgroundTaskUpdate(managed, runtime, { - taskId, - status, - title: existing?.description, - command: existing?.command, - ...(turnId ? { turnId } : {}), - }); + if (!existing || !isBackgroundShellCommand({ + taskType: existing.taskType, + agentType: existing.agentType, + command: existing.command, + description: existing.description, + })) return false; + return parentAgentId === undefined || existing.parentAgentId === parentAgentId; + }); + const control = getClaudeQueryControl(runtime.query); + await Promise.all(taskIds.map(async (taskId) => { + const existing = runtime.activeSubagents.get(taskId); + if (!existing) return; + + let terminalStatus: ScheduledWorkEvent["status"] = status; + let terminalSummary: string | undefined; + const canStopProviderTask = status === "stopped" && runtime.query != null; + const stopState = canStopProviderTask + ? { emitted: false, requestedAt: Date.now() } + : null; + if (stopState) runtime.stoppingBackgroundTaskIds.set(taskId, stopState); + if (canStopProviderTask) { + if (typeof control.stopTask !== "function") { + terminalStatus = "failed"; + terminalSummary = "The Claude query did not expose a task stop control."; + } else { + try { + await awaitClaudeControlCall( + `Stopping Claude background task '${taskId}'`, + CLAUDE_STOP_TASK_TIMEOUT_MS, + () => control.stopTask!(taskId), + ); + } catch (error) { + terminalStatus = "failed"; + terminalSummary = `Failed to stop background task: ${error instanceof Error ? error.message : String(error)}`; + logger.warn("agent_chat.claude_background_stop_task_failed", { + sessionId: managed.session.id, + taskId, + error: error instanceof Error ? error.message : String(error), + }); + } + } + } + const emittedByProvider = stopState?.emitted === true; + if (stopState && !emittedByProvider) { + stopState.emitted = true; + } + if (!emittedByProvider) { + emitClaudeBackgroundTaskUpdate(managed, runtime, { + taskId, + status: terminalStatus, + title: existing.description, + summary: terminalSummary, + command: existing.command, + ...(turnId ? { turnId } : {}), + }); + } runtime.activeSubagents.delete(taskId); if (existing?.parentToolUseId) { runtime.taskToolInputByToolUseId.delete(existing.parentToolUseId); } - } - runtime.seenBackgroundTaskIds.clear(); + })); + for (const taskId of taskIds) runtime.seenBackgroundTaskIds.delete(taskId); }; const emitClaudeTranscriptRetraction = ( @@ -17552,6 +17690,7 @@ export function createAgentChatService(args: { } const taskId = compactString(msg.task_id); if (!taskId) return true; + pruneClaudeStoppingBackgroundTasks(runtime); const notificationAgentId = compactString(msg.agent_id); const taskPatch = subtype === "task_updated" ? asRecord(msg.patch) ?? {} : {}; let existing = resolveClaudeActiveSubagent(runtime, taskId, notificationAgentId); @@ -17573,6 +17712,25 @@ export function createAgentChatService(args: { } return true; } + if (subtype === "task_notification") { + const finalStatus = msg.status === "completed" + ? "completed" + : msg.status === "stopped" + ? "stopped" + : "failed"; + if (consumeClaudeStoppingBackgroundTask(managed, runtime, { + taskId, + status: finalStatus, + title: existing?.description, + summary: compactString(msg.summary), + command: existing?.command, + durationMs: typeof (msg.usage as Record | undefined)?.duration_ms === "number" + ? (msg.usage as Record).duration_ms + : undefined, + ...(state.turnId ? { turnId: state.turnId } : {}), + parentToolUseId: existing?.parentToolUseId, + })) return true; + } if (subtype === "task_started" && msg.skip_transcript === true) { runtime.activeSubagents.set(taskId, { taskId, @@ -17664,6 +17822,15 @@ export function createAgentChatService(args: { return true; } if (status === "completed" || status === "failed" || status === "killed") { + if (consumeClaudeStoppingBackgroundTask(managed, runtime, { + taskId, + status: status === "completed" ? "completed" : status === "killed" ? "stopped" : "failed", + title: existing?.description ?? description, + summary: compactString(patch.error), + command, + ...(turnId ? { turnId } : {}), + parentToolUseId: existing?.parentToolUseId, + })) return true; runtime.activeSubagents.delete(taskId); if (notificationAgentId) runtime.activeSubagents.delete(notificationAgentId); if (parentToolUseId) runtime.taskToolInputByToolUseId.delete(parentToolUseId); @@ -18101,6 +18268,11 @@ export function createAgentChatService(args: { const info = asRecord(record.rate_limit_info) ?? {}; const rawStatus = compactString(info.status) ?? "updated"; if (rawStatus === "allowed") return; + if (rawStatus === "allowed_warning") { + if (managed.claudeRateLimitWarningEmitted) return; + managed.claudeRateLimitWarningEmitted = true; + runtime.rateLimitWarningEmitted = true; + } emitChatEvent(managed, { type: "system_notice", noticeKind: "rate_limit", @@ -19732,6 +19904,17 @@ export function createAgentChatService(args: { const taskType = existing?.taskType; const workflowName = existing?.workflowName; const background = patch.is_backgrounded === true || existing?.background === true; + if (status === "completed" || status === "failed" || status === "killed") { + if (consumeClaudeStoppingBackgroundTask(managed, runtime, { + taskId, + status: status === "completed" ? "completed" : status === "killed" ? "stopped" : "failed", + title: existing?.description ?? description, + summary: compactString(patch.error), + command: existing?.command, + ...(turnId ? { turnId } : {}), + parentToolUseId, + })) continue; + } const classification = classifyClaudeTaskMessage( runtime, taskMsg as Record, @@ -20020,6 +20203,21 @@ export function createAgentChatService(args: { if (notificationAgentId) runtime.activeSubagents.delete(notificationAgentId); continue; } + const stoppingStatus = taskMsg.status === "completed" + ? "completed" + : taskMsg.status === "stopped" + ? "stopped" + : "failed"; + if (consumeClaudeStoppingBackgroundTask(managed, runtime, { + taskId, + status: stoppingStatus, + title: existing?.description, + summary: compactString(taskMsg.summary), + command: existing?.command, + durationMs: typeof taskMsg.usage?.duration_ms === "number" ? taskMsg.usage.duration_ms : undefined, + ...(turnId ? { turnId } : {}), + parentToolUseId: existing?.parentToolUseId, + })) continue; const parentToolUseId = taskParentToolUseId(taskMsg as Record) ?? existing?.parentToolUseId ?? null; const gatedFinalSummary = resolveClaudeNotificationFinalSummary(existing, { taskId, @@ -21087,19 +21285,33 @@ export function createAgentChatService(args: { ] : []; + const harnessPrompt = buildCodingAgentSystemPrompt({ + cwd: managed.laneWorktreePath, + mode: "coding", + permissionMode: toHarnessPermissionMode(managed.session.permissionMode), + interactive: true, + runtime: "claude-code-cli", + adeSkillRoots: getAdeAgentSkillRootsForPrompt({ cwd: managed.laneWorktreePath }), + orchestrationRole: managed.session.orchestrationRole, + orchestrationRunId: managed.session.orchestrationRunId, + orchestrationBundlePath: managed.session.orchestrationBundlePath, + orchestrationTag: managed.session.orchestrationTag, + orchestrationParentSessionId: managed.session.orchestrationParentSessionId, + orchestrationStepId: managed.session.orchestrationStepId, + }); return [ - "## Runtime Environment", - "**Runtime:** ADE Work chat is hosting you through the Claude Code CLI background-session supervisor. The user sees an ADE lane chat, not your terminal.", - "**Lifecycle:** Keep working in the background when useful. ADE will keep the transcript and session pointer, and later user messages may arrive by Agent View reply or by resuming your saved conversation.", + harnessPrompt, + "", + "**Lifecycle:** Keep working in the background when useful. ADE keeps the transcript and session pointer, and later user messages may arrive by Agent View reply or by resuming the saved conversation.", "**UI contract:** Do not ask the user to run `claude attach`, `claude logs`, or a resume command. ADE owns those mechanics.", "", "## ADE Workspace", `ADE launched this session in lane worktree: ${managed.laneWorktreePath}.`, "Read-only inspection outside that worktree is allowed when needed. Edit files and run mutating commands only inside that worktree unless ADE explicitly relaunches you elsewhere.", "", - ...slashCommandsSection, + buildAdeSessionLineageGuidance(managed.session) ?? "", "", - buildAdeGuidanceForLane(managed.laneWorktreePath, managed.session), + ...slashCommandsSection, ].join("\n"); }; @@ -21621,10 +21833,14 @@ export function createAgentChatService(args: { const openCodeAgent = runtime.permissionMode === "config-toml" ? null : mapPermissionModeToOpenCodeAgent(runtime.permissionMode); + const openCodeSystemPrompt = buildOpenCodeSystemPrompt({ + laneWorktreePath: managed.laneWorktreePath, + session: managed.session, + }); const openCodePromptBody = { ...(openCodeAgent ? { agent: openCodeAgent } : {}), model: resolveOpenCodeModelSelection(runtime.modelDescriptor), - ...(isPersonalSession(managed.session) ? { system: PERSONAL_CHAT_SYSTEM_PROMPT } : {}), + ...(openCodeSystemPrompt ? { system: openCodeSystemPrompt } : {}), ...(toolSelection ? { tools: toolSelection } : {}), ...(openCodeVariant ? { variant: openCodeVariant } : {}), parts: buildOpenCodePromptParts({ @@ -23362,7 +23578,7 @@ export function createAgentChatService(args: { // Close still-open background shell commands as stopped (terminal // background_task rows) — they live in activeSubagents too but must never // surface as subagent_result rows. - closeOpenClaudeBackgroundTasks(managed, runtime, "stopped", turnId); + await closeOpenClaudeBackgroundTasks(managed, runtime, "stopped", turnId); const activeSubagents = [...runtime.activeSubagents.values()]; if (activeSubagents.length === 0) return; @@ -27146,6 +27362,16 @@ export function createAgentChatService(args: { runtime.activeSubagents.set(key, { ...entry, finalSummary }); } } + // A native subagent can leave a background shell behind when + // it exits. Reap only shells that carry this subagent's parent + // id; sibling work must continue untouched. + await closeOpenClaudeBackgroundTasks( + managed, + runtime, + "stopped", + runtime.activeTurnId ?? undefined, + agentId, + ); await emitClaudeHookScheduledWorkSnapshots(managed, runtime, input); } return { continue: true }; @@ -27427,14 +27653,25 @@ export function createAgentChatService(args: { ] : []; const linearDirective = resolveSessionLinearDirective(managed.session.id); + const claudeHarnessPrompt = buildCodingAgentSystemPrompt({ + cwd: managed.laneWorktreePath, + mode: managed.session.interactionMode === "plan" ? "planning" : "coding", + permissionMode: toHarnessPermissionMode(managed.session.permissionMode), + interactive: true, + runtime: "claude-agent-sdk-query", + adeSkillRoots: getAdeAgentSkillRootsForPrompt({ cwd: managed.laneWorktreePath }), + orchestrationRole: managed.session.orchestrationRole, + orchestrationRunId: managed.session.orchestrationRunId, + orchestrationBundlePath: managed.session.orchestrationBundlePath, + orchestrationTag: managed.session.orchestrationTag, + orchestrationParentSessionId: managed.session.orchestrationParentSessionId, + orchestrationStepId: managed.session.orchestrationStepId, + }); opts.systemPrompt = { type: "preset", preset: "claude_code", append: [ - "## Runtime Environment", - "**Runtime:** ADE Work chat hosted on the Claude Agent SDK stable `query()` streaming-input API. The `claude_code` preset above is the same system prompt the Claude Code CLI uses, so you may think you're in the CLI — you are NOT. You are inside an ADE-hosted SDK session.", - "**Wake-up semantics:** Native `ScheduleWakeup`, `CronCreate`, and `/loop` are automatically mirrored into ADE's durable scheduler. `durable: true` also persists Claude's provider copy, while ADE's delivery guarantee does not depend on that flag. Jobs survive brain restarts and start a new turn at the next turn boundary even if the chat was busy when they became due. The SDK's own `CronList` view is advisory; ADE state wins. Pause schedules in Chat Info or project-wide in Settings. Recurring jobs expire seven days after creation. `CronCreate` always creates a new job, so replace one with `CronList` + `CronDelete` before creating another.", - "**To wait:** For short bounded waits inside the current turn, a foreground command such as `sleep ... && ` is fine. For longer waits or autonomous follow-up, prefer `ScheduleWakeup`, `CronCreate`, or `/loop` and include a concise reason/prompt so ADE can show the pending work clearly.", + claudeHarnessPrompt, "", "## ADE Workspace", `ADE launched this session in lane worktree: ${managed.laneWorktreePath}.`, @@ -27443,7 +27680,7 @@ export function createAgentChatService(args: { ...(linearDirective ? [linearDirective, ""] : []), ...slashCommandsSection, "", - buildAdeGuidanceForLane(managed.laneWorktreePath, managed.session), + buildAdeSessionLineageGuidance(managed.session) ?? "", ].join("\n"), }; opts.settingSources = ["user", "project", "local"]; @@ -27558,6 +27795,7 @@ export function createAgentChatService(args: { runtime.emittedSubagentStartIds.clear(); resetClaudeProcessBackgroundLevel(runtime); runtime.seenBackgroundTaskIds.clear(); + runtime.stoppingBackgroundTaskIds.clear(); runtime.backgroundTaskTitleById.clear(); runtime.scheduledWorkIdByTaskId.clear(); runtime.scheduledWorkIdByToolUseId.clear(); @@ -28503,6 +28741,7 @@ export function createAgentChatService(args: { liveBackgroundTaskIds: new Set(), backgroundTasksLevelObserved: false, seenBackgroundTaskIds: new Set(), + stoppingBackgroundTaskIds: new Map(), backgroundTaskTitleById: new Map(), scheduledWorkKindById: new Map(), scheduledWorkIdByTaskId: new Map(), @@ -31922,7 +32161,8 @@ export function createAgentChatService(args: { // trusted instruction channel still need the guidance in the user prompt, // including on resumed sessions where `shouldInjectLaneDirective` is false. const providerHasPersistentGuidance = managed.session.provider === "claude" - || managed.session.provider === "codex"; + || managed.session.provider === "codex" + || managed.session.provider === "opencode"; const shouldInjectGuidance = !personalSession && !providerHasPersistentGuidance; const claudeRuntimeSlashCommandNames = managed.runtime?.kind === "claude" ? new Set(managed.runtime.slashCommands.map((command) => slashCommandKey(command.name))) @@ -34636,15 +34876,10 @@ export function createAgentChatService(args: { const auth = await detectAuth(); throwIfDroidSetupInterrupted(); const droidMcpLeases = await ensureHttpMcpLeases(managed); - // KNOWN GAP — orchestrator leads are NOT isolated from the user's MCP - // servers on Droid. `mcpServers` here only *adds* ADE's lease; Droid's - // SDK has no session-scoped way to withhold the servers it loads from the - // user's Factory config. `disabledToolIds` covers the exec tool catalog - // only, and toggleMcpServer/toggleMcpTool persist to the user's global - // settings (settingsLevel is pinned to `User`), which would disable a - // server for every other droid session on the machine. ADE will not - // mutate user config to fake a session gate — see - // ORCHESTRATION_LEAD_MCP_ISOLATION.droid. + // Droid's native MCP config is user/project scoped. The worker keeps + // ADE's inline lease and disables every other live MCP tool through the + // session-scoped toggleMcpTool RPC before each lead turn (see + // ORCHESTRATION_LEAD_MCP_ISOLATION.droid). const droidOrchestrationMcpServers = droidMcpLeases.length ? droidMcpLeases.map((lease) => lease.config) : undefined; @@ -34657,6 +34892,9 @@ export function createAgentChatService(args: { resumeSessionId: persisted?.droidSdkSessionId ?? null, settings: buildDroidSdkSessionSettings(managed, launchModelId), ...(droidOrchestrationMcpServers ? { mcpServers: droidOrchestrationMcpServers } : {}), + ...(isOrchestrationLeadSession(managed.session) + ? { allowedMcpServerNames: droidMcpLeases.map((lease) => lease.serverName) } + : {}), baseEnv: buildAgentRuntimeEnv(managed), logger, }); diff --git a/apps/desktop/src/main/services/chat/droidSdkPool.test.ts b/apps/desktop/src/main/services/chat/droidSdkPool.test.ts index d2d23d5d9..8efafddf6 100644 --- a/apps/desktop/src/main/services/chat/droidSdkPool.test.ts +++ b/apps/desktop/src/main/services/chat/droidSdkPool.test.ts @@ -19,9 +19,11 @@ class FakeSdkChild extends EventEmitter { exitCode: number | null = null; killed = false; disposeCount = 0; + initPayloads: unknown[] = []; - send(message: { type?: string; requestId?: string }): boolean { + send(message: { type?: string; requestId?: string; payload?: unknown }): boolean { if (message.type === "init" && message.requestId) { + this.initPayloads.push(message.payload); queueMicrotask(() => { this.emit("message", { type: "response", @@ -101,6 +103,7 @@ describe("Droid SDK pool", () => { ADE_CHAT_SESSION_ID: "session-1", ADE_DEFAULT_ROLE: "agent", }, + allowedMcpServerNames: [], }; const [first, second] = await Promise.all([ @@ -122,6 +125,7 @@ describe("Droid SDK pool", () => { ); expect(second.pooled).toBe(first.pooled); expect(second.generation).toBe(first.generation); + expect((child.initPayloads[0] as { allowedMcpServerNames?: string[] }).allowedMcpServerNames).toEqual([]); releaseDroidSdkConnection(poolKey, first.generation); expect(child.disposeCount).toBe(0); diff --git a/apps/desktop/src/main/services/chat/droidSdkPool.ts b/apps/desktop/src/main/services/chat/droidSdkPool.ts index 4245fa108..78ecd820d 100644 --- a/apps/desktop/src/main/services/chat/droidSdkPool.ts +++ b/apps/desktop/src/main/services/chat/droidSdkPool.ts @@ -81,6 +81,7 @@ export async function acquireDroidSdkConnection(args: { resumeSessionId?: string | null; settings: DroidSdkSessionSettings; mcpServers?: unknown[]; + allowedMcpServerNames?: readonly string[]; baseEnv?: NodeJS.ProcessEnv; logger?: Logger; }): Promise<{ pooled: DroidSdkPooled; generation: number }> { @@ -321,6 +322,9 @@ async function createDroidSdkConnection(args: Parameters { )).toEqual(["edit_file"]); }); }); + +describe("droidMcpToolsToDisable", () => { + it("disables enabled user MCP tools while retaining ADE's leased server", () => { + expect(droidMcpToolsToDisable([ + { serverName: "ade-orchestration", name: "spawn_agent", isEnabled: true }, + { serverName: "filesystem", name: "write_file", isEnabled: true }, + { serverName: "filesystem", name: "read_file", isEnabled: false }, + { serverName: "linear", name: "search", isEnabled: true }, + ], ["ade-orchestration"])).toEqual([ + { serverName: "filesystem", toolName: "write_file" }, + { serverName: "linear", toolName: "search" }, + ]); + }); + + it("ignores malformed entries and fails closed for unknown MCP state", () => { + expect(droidMcpToolsToDisable([ + { serverName: "", name: "write_file", isEnabled: true }, + { serverName: "filesystem", name: "", isEnabled: true }, + { serverName: "filesystem", name: "write_file", isEnabled: false }, + { serverName: "filesystem", name: "unknown_state" }, + ], [])).toEqual([ + { serverName: "filesystem", toolName: "unknown_state" }, + ]); + }); +}); diff --git a/apps/desktop/src/main/services/chat/droidSdkProtocol.ts b/apps/desktop/src/main/services/chat/droidSdkProtocol.ts index 477201f74..8cc554935 100644 --- a/apps/desktop/src/main/services/chat/droidSdkProtocol.ts +++ b/apps/desktop/src/main/services/chat/droidSdkProtocol.ts @@ -56,6 +56,28 @@ export function droidDisabledToolIdsForCategories( return ids; } +/** + * Selects the live Droid MCP tools a lead must disable. MCP tools are not part + * of `listTools()`'s native exec catalog; Droid exposes their per-session + * enable switch through the low-level `toggleMcpTool` RPC instead. + */ +export function droidMcpToolsToDisable( + tools: ReadonlyArray<{ serverName?: unknown; name?: unknown; isEnabled?: unknown }>, + allowedServerNames: readonly string[], +): Array<{ serverName: string; toolName: string }> { + const allowed = new Set(allowedServerNames.map((name) => name.trim()).filter(Boolean)); + const disabled: Array<{ serverName: string; toolName: string }> = []; + for (const tool of tools) { + const serverName = typeof tool?.serverName === "string" ? tool.serverName.trim() : ""; + const toolName = typeof tool?.name === "string" ? tool.name.trim() : ""; + // Unknown state is unsafe for a lead: disable anything that is not + // explicitly reported as already disabled. + if (!serverName || !toolName || allowed.has(serverName) || tool.isEnabled === false) continue; + disabled.push({ serverName, toolName }); + } + return disabled; +} + export type DroidSdkWorkerInit = { sessionId: string; laneRoot: string; @@ -63,6 +85,8 @@ export type DroidSdkWorkerInit = { resumeSessionId?: string | null; settings: DroidSdkSessionSettings; mcpServers?: unknown[]; + /** MCP server names ADE owns and a lead may retain; all other tools are disabled per session. */ + allowedMcpServerNames?: string[]; }; export type DroidSdkUserImage = { diff --git a/apps/desktop/src/main/services/chat/droidSdkWorker.ts b/apps/desktop/src/main/services/chat/droidSdkWorker.ts index e3a869dc8..6b45e294b 100644 --- a/apps/desktop/src/main/services/chat/droidSdkWorker.ts +++ b/apps/desktop/src/main/services/chat/droidSdkWorker.ts @@ -10,7 +10,7 @@ import type { DroidSdkWorkerRequest, DroidSdkWorkerResponse, } from "./droidSdkProtocol"; -import { droidDisabledToolIdsForCategories } from "./droidSdkProtocol"; +import { droidDisabledToolIdsForCategories, droidMcpToolsToDisable } from "./droidSdkProtocol"; import { loadDroidSdk } from "../ai/droidSdkLoader"; import { summarizeDroidAskUser } from "./droidSdkAskUser"; import { ensureDroidSpawnsAreWindowless } from "./droidSdkWindowsHide"; @@ -264,9 +264,52 @@ async function resolveDisabledToolIds( return droidDisabledToolIdsForCategories(tools, categories); } +/** + * DroidSession exposes MCP enumeration publicly, but @factory/droid-sdk + * 0.2.0 exposes `toggleMcpTool` only on its low-level client. Keep the + * private-field bridge in one place and fail closed if a future SDK removes it. + */ +async function disableUnmanagedMcpToolsForLead(): Promise { + const allowedServerNames = initState?.allowedMcpServerNames; + if (!session || !allowedServerNames) return; + const listed = await session.listMcpTools(); + if (!listed || !Array.isArray(listed.tools)) { + throw new Error("Droid did not return a valid MCP tool list for the orchestrator lead."); + } + const tools = listed.tools; + const toDisable = droidMcpToolsToDisable(tools, allowedServerNames); + if (!toDisable.length) return; + const client = (session as unknown as { + _client?: { + toggleMcpTool?: (params: { + serverName: string; + toolName: string; + enabled: boolean; + }) => Promise; + }; + })._client; + if (!client || typeof client.toggleMcpTool !== "function") { + throw new Error("This Droid SDK build does not expose session-scoped toggleMcpTool."); + } + for (const tool of toDisable) { + await client.toggleMcpTool({ + serverName: tool.serverName, + toolName: tool.toolName, + enabled: false, + }); + } + post({ + type: "log", + level: "debug", + message: "Disabled unmanaged Droid MCP tools for orchestrator lead.", + detail: { disabledCount: toDisable.length }, + }); +} + async function applySettings(settings: DroidSdkSessionSettings): Promise { if (!session) throw new Error("Droid SDK worker is not initialized."); const sdk = await getSdk(); + await disableUnmanagedMcpToolsForLead(); const disabledToolIds = await resolveDisabledToolIds(settings); if (settings.interactionMode === "spec") { await session.enterSpecMode({ @@ -318,6 +361,7 @@ async function initWorker(init: DroidSdkWorkerInit): Promise { const disabledToolIds = await resolveDisabledToolIds(init.settings); if (disabledToolIds?.length) await session.updateSettings({ disabledToolIds }); } + await disableUnmanagedMcpToolsForLead(); const ready = buildReady(); post({ type: "ready", ready }); return ready; diff --git a/apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts b/apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts index b2fa2865d..1b5588a44 100644 --- a/apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts +++ b/apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts @@ -164,6 +164,24 @@ describe("openCodeRuntime", () => { expect(mockState.sharedLease.close).toHaveBeenCalledWith("handle_close"); }); + it("passes lead isolation through to a dedicated OpenCode server", async () => { + await startOpenCodeSession({ + directory: "/repo", + title: "Lead chat", + leaseKind: "dedicated", + isolatedConfig: true, + projectConfig: { ai: {} }, + ownerKind: "chat", + ownerId: "lead-1", + ownerKey: "chat:lead-1", + }); + + expect(acquireDedicatedOpenCodeServer).toHaveBeenCalledWith(expect.objectContaining({ + isolatedConfig: true, + ownerKey: "chat:lead-1", + })); + }); + it("omits the session title when ADE wants OpenCode to auto-name", async () => { await startOpenCodeSession({ directory: "/repo", diff --git a/apps/desktop/src/main/services/opencode/openCodeRuntime.ts b/apps/desktop/src/main/services/opencode/openCodeRuntime.ts index aeb6ae972..ed1cf0a24 100644 --- a/apps/desktop/src/main/services/opencode/openCodeRuntime.ts +++ b/apps/desktop/src/main/services/opencode/openCodeRuntime.ts @@ -144,6 +144,8 @@ type StartOpenCodeSessionArgs = BuildOpenCodeConfigArgs & { ownerId?: string | null; ownerKey?: string | null; leaseKind?: "shared" | "dedicated"; + /** Isolate user/project config for an orchestrator lead only. */ + isolatedConfig?: boolean; logger?: Logger | null; }; @@ -635,6 +637,7 @@ async function startOpenCodeSessionInternal( key: buildSharedOpenCodeServerKey(config), ownerKind, ownerId: args.ownerId, + isolatedConfig: args.isolatedConfig, logger: args.logger, }) : await acquireDedicatedOpenCodeServer({ @@ -642,6 +645,7 @@ async function startOpenCodeSessionInternal( config, ownerKind, ownerId: args.ownerId, + isolatedConfig: args.isolatedConfig, logger: args.logger, }); const client = createOpencodeClient({ diff --git a/apps/desktop/src/main/services/opencode/openCodeServerManager.test.ts b/apps/desktop/src/main/services/opencode/openCodeServerManager.test.ts index a7942b3a9..d5be6d196 100644 --- a/apps/desktop/src/main/services/opencode/openCodeServerManager.test.ts +++ b/apps/desktop/src/main/services/opencode/openCodeServerManager.test.ts @@ -188,6 +188,7 @@ describe("openCodeServerManager", () => { const originalEnv = { PATH: process.env.PATH, HOME: process.env.HOME, + XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, ADE_OPENCODE_XDG_ROOT: process.env.ADE_OPENCODE_XDG_ROOT, OPENCODE_API_KEY: process.env.OPENCODE_API_KEY, OPENCODE_BIN_PATH: process.env.OPENCODE_BIN_PATH, @@ -243,6 +244,7 @@ describe("openCodeServerManager", () => { vi.useRealTimers(); restoreEnv("PATH"); restoreEnv("HOME"); + restoreEnv("XDG_CONFIG_HOME"); restoreEnv("ADE_OPENCODE_XDG_ROOT"); restoreEnv("OPENCODE_API_KEY"); restoreEnv("OPENCODE_BIN_PATH"); @@ -559,6 +561,7 @@ describe("openCodeServerManager", () => { const spec = __buildOpenCodeServeLaunchSpecForTests({ config, port: 4310, + isolatedConfig: true, }); expect(spec.executable).toBe("/Users/admin/.opencode/bin/opencode"); @@ -584,6 +587,49 @@ describe("openCodeServerManager", () => { expect(spec.env.OPENCODE_BIN_PATH).toBeUndefined(); }); + it("keeps the user's OpenCode config layers for ordinary chat servers", () => { + process.env.ADE_OPENCODE_XDG_ROOT = "/tmp/ade-opencode-test-home"; + process.env.XDG_CONFIG_HOME = "/Users/tester/.config"; + process.env.OPENCODE_API_KEY = "ambient-api-key"; + process.env.OPENCODE_CONFIG_DIR = "/Users/tester/.config/opencode"; + process.env.OPENCODE_CONFIG_CONTENT = JSON.stringify({ + mcp: { filesystem: { type: "local", command: ["filesystem"] } }, + theme: "user-theme", + }); + + const spec = __buildOpenCodeServeLaunchSpecForTests({ + config: { + mcp: { "ade-orchestration": { type: "remote", url: "http://ade/mcp" } }, + } as const, + port: 4311, + }); + + expect(spec.env.XDG_CONFIG_HOME).toBe("/Users/tester/.config"); + expect(spec.env.OPENCODE_CONFIG_DIR).toBe("/Users/tester/.config/opencode"); + expect(spec.env.OPENCODE_API_KEY).toBe("ambient-api-key"); + expect(spec.env.OPENCODE_DISABLE_PROJECT_CONFIG).toBeUndefined(); + expect(JSON.parse(spec.env.OPENCODE_CONFIG_CONTENT ?? "{}")).toEqual({ + mcp: { + filesystem: { type: "local", command: ["filesystem"] }, + "ade-orchestration": { type: "remote", url: "http://ade/mcp" }, + }, + theme: "user-theme", + }); + expect(spec.env.ADE_OPENCODE_MANAGED).toBe("1"); + }); + + it("does not replace malformed user OpenCode config content", () => { + process.env.OPENCODE_CONFIG_CONTENT = "{not valid json"; + + const spec = __buildOpenCodeServeLaunchSpecForTests({ + config: { mcp: { "ade-orchestration": { type: "remote", url: "http://ade/mcp" } } } as const, + port: 4312, + }); + + expect(spec.env.OPENCODE_CONFIG_CONTENT).toBe("{not valid json"); + expect(spec.env.ADE_OPENCODE_MANAGED).toBe("1"); + }); + it("quotes the OpenCode executable in Windows cmd launch specs", () => { setProcessPlatform("win32"); process.env.ADE_OPENCODE_XDG_ROOT = "/tmp/ade-opencode-test-home"; diff --git a/apps/desktop/src/main/services/opencode/openCodeServerManager.ts b/apps/desktop/src/main/services/opencode/openCodeServerManager.ts index cbd782605..910a999fb 100644 --- a/apps/desktop/src/main/services/opencode/openCodeServerManager.ts +++ b/apps/desktop/src/main/services/opencode/openCodeServerManager.ts @@ -53,6 +53,8 @@ type OpenCodeServerInstance = { type OpenCodeServerLaunchArgs = { port: number; config: OpenCodeConfig; + /** Keep user/project OpenCode config available when the caller is not a lead. */ + isolatedConfig?: boolean; }; type OpenCodeIsolationPaths = { @@ -107,6 +109,7 @@ type OpenCodeServerEntry = { ownerKind: OpenCodeServerOwnerKind; ownerId: string | null; configFingerprint: string; + isolatedConfig: boolean; server: OpenCodeServerInstance; idleTtlMs: number | null; idleTimer: ReturnType | null; @@ -775,6 +778,57 @@ function buildIsolatedOpenCodeEnv( }; } +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function mergeOpenCodeConfig( + base: Record, + overlay: Record, +): Record { + const merged: Record = { ...base }; + for (const [key, value] of Object.entries(overlay)) { + const existing = merged[key]; + merged[key] = isRecord(existing) && isRecord(value) + ? mergeOpenCodeConfig(existing, value) + : value; + } + return merged; +} + +function buildUserOpenCodeEnv(config: OpenCodeConfig): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { ...process.env }; + const inheritedContent = env.OPENCODE_CONFIG_CONTENT?.trim(); + if (inheritedContent) { + try { + const parsed = JSON.parse(inheritedContent); + if (!isRecord(parsed)) return addUserOpenCodeOwnershipMarkers(env); + env.OPENCODE_CONFIG_CONTENT = JSON.stringify( + mergeOpenCodeConfig(parsed, config as Record), + ); + return addUserOpenCodeOwnershipMarkers(env); + } catch { + // Preserve malformed user content rather than silently replacing a user + // setting. OpenCode will report the parse error to the user, and ADE must + // not turn an invalid setting into a different, valid configuration. + return addUserOpenCodeOwnershipMarkers(env); + } + } + env.OPENCODE_CONFIG_CONTENT = JSON.stringify( + config as Record, + ); + return addUserOpenCodeOwnershipMarkers(env); +} + +function addUserOpenCodeOwnershipMarkers(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { + // Preserve every user-provided OpenCode setting, including XDG paths, API + // keys, config-dir overrides, and an intentional disable-project-config flag. + // Only ADE's ownership markers are added for orphan recovery. + env[ADE_OPENCODE_MANAGED_ENV] = "1"; + env[ADE_OPENCODE_OWNER_PID_ENV] = String(process.pid); + return env; +} + function buildManagedConfigMarkers(): string[] { const markers = new Set(); for (const root of resolveKnownAdeManagedOpenCodeRoots()) { @@ -793,12 +847,13 @@ function isManagedOpenCodeServeCommand(command: string, configMarkers: string[]) && /\bopencode(?:\.cmd|\.bat|\.exe)?\b/i.test(command) && /\bserve\b/i.test(command) ) { - return command.includes("OPENCODE_DISABLE_PROJECT_CONFIG=1"); + return true; } if (!/\bopencode(?:\.cmd|\.bat|\.exe)?\b\s+serve\b/i.test(command)) return false; - if (!command.includes("OPENCODE_DISABLE_PROJECT_CONFIG=1")) return false; if (command.includes(`${ADE_OPENCODE_MANAGED_ENV}=1`)) return true; - return configMarkers.some((marker) => command.includes(marker)); + // Keep recognizing older isolated servers that predate the ownership marker. + return command.includes("OPENCODE_DISABLE_PROJECT_CONFIG=1") + && configMarkers.some((marker) => command.includes(marker)); } /** @@ -1084,8 +1139,11 @@ function buildOpenCodeServeLaunchSpec(args: OpenCodeServerLaunchArgs): OpenCodeS throw new Error("OpenCode executable is not available."); } const xdgPaths = resolveOpenCodeIsolationPaths(); - ensureOpenCodeIsolationDirs(xdgPaths); - const env = buildIsolatedOpenCodeEnv(args.config, xdgPaths); + const isolatedConfig = args.isolatedConfig === true; + if (isolatedConfig) ensureOpenCodeIsolationDirs(xdgPaths); + const env = isolatedConfig + ? buildIsolatedOpenCodeEnv(args.config, xdgPaths) + : buildUserOpenCodeEnv(args.config); // Only shim through cmd.exe when the resolved target actually needs it (a // `.cmd`/`.bat` shim, or an extensionless file), matching // {@link shouldUseWindowsCmdWrapper} — the policy every other ADE CLI launch @@ -1102,11 +1160,12 @@ function buildOpenCodeServeLaunchSpec(args: OpenCodeServerLaunchArgs): OpenCodeS "--hostname=127.0.0.1", `--port=${args.port}`, ].map(quoteWindowsCmdArg).join(" "); - const cmdLine = - `set ${quoteWindowsCmdArg(`${ADE_OPENCODE_MANAGED_ENV}=1`)}` - + `&&set ${quoteWindowsCmdArg("OPENCODE_DISABLE_PROJECT_CONFIG=1")}` - + `&&set ${quoteWindowsCmdArg(`${ADE_OPENCODE_OWNER_PID_ENV}=${process.pid}`)}` - + `&&${serveCmdLine}`; + const assignments = [ + `set ${quoteWindowsCmdArg(`${ADE_OPENCODE_MANAGED_ENV}=1`)}`, + ...(isolatedConfig ? [`set ${quoteWindowsCmdArg("OPENCODE_DISABLE_PROJECT_CONFIG=1")}`] : []), + `set ${quoteWindowsCmdArg(`${ADE_OPENCODE_OWNER_PID_ENV}=${process.pid}`)}`, + ]; + const cmdLine = `${assignments.join("&&")}&&${serveCmdLine}`; const invocation = resolveWindowsCmdLineInvocation(cmdLine, env); return { executable: invocation.command, @@ -1234,6 +1293,7 @@ function parseOpenCodeServerListenUrl(line: string): string | null { async function createOpencodeServerWithRetry( config: OpenCodeConfig, + options: { isolatedConfig: boolean }, ): Promise { const binaryPath = resolveOpenCodeBinaryPath(); let lastError: unknown; @@ -1243,7 +1303,11 @@ async function createOpencodeServerWithRetry( lastPort = port; protectedLaunchPorts.add(port); try { - return await openCodeServerLauncher({ port, config }); + return await openCodeServerLauncher({ + port, + config, + isolatedConfig: options.isolatedConfig, + }); } catch (error) { protectedLaunchPorts.delete(port); lastError = error; @@ -1401,16 +1465,19 @@ async function createEntry(args: { ownerId?: string | null; config: OpenCodeConfig; configFingerprint: string; + isolatedConfig: boolean; idleTtlMs?: number | null; logger?: Logger | null; }): Promise { - const inflightKey = `${args.leaseKind}:${args.key}:${args.configFingerprint}`; + const inflightKey = `${args.leaseKind}:${args.key}:${args.configFingerprint}:${args.isolatedConfig ? "isolated" : "user"}`; const existingPromise = inFlightEntries.get(inflightKey); if (existingPromise) return await existingPromise; const createPromise = (async () => { await recoverManagedOpenCodeOrphans({ logger: args.logger }); - const server = await createOpencodeServerWithRetry(args.config); + const server = await createOpencodeServerWithRetry(args.config, { + isolatedConfig: args.isolatedConfig, + }); const entry: OpenCodeServerEntry = { id: randomUUID(), key: args.key, @@ -1418,6 +1485,7 @@ async function createEntry(args: { ownerKind: args.ownerKind, ownerId: args.ownerId?.trim() || null, configFingerprint: args.configFingerprint, + isolatedConfig: args.isolatedConfig, server, idleTtlMs: args.leaseKind === "shared" ? args.idleTtlMs ?? DEFAULT_SHARED_IDLE_TTL_MS : null, idleTimer: null, @@ -1443,14 +1511,16 @@ export async function acquireSharedOpenCodeServer(args: { ownerKind?: OpenCodeServerOwnerKind; ownerId?: string | null; idleTtlMs?: number | null; + isolatedConfig?: boolean; logger?: Logger | null; }): Promise { const configFingerprint = serializeConfigFingerprint(args.config); + const isolatedConfig = args.isolatedConfig === true; const key = args.key?.trim() || configFingerprint; return await withAcquireLock(`shared:${key}`, async () => { while (true) { const existing = sharedEntries.get(key); - if (existing && existing.configFingerprint === configFingerprint) { + if (existing && existing.configFingerprint === configFingerprint && existing.isolatedConfig === isolatedConfig) { clearIdleTimer(existing); existing.refCount += 1; existing.lastUsedAt = Date.now(); @@ -1477,10 +1547,11 @@ export async function acquireSharedOpenCodeServer(args: { ownerId: args.ownerId, config: args.config, configFingerprint, + isolatedConfig, idleTtlMs: args.idleTtlMs, logger: args.logger, }); - if (entry.configFingerprint !== configFingerprint) { + if (entry.configFingerprint !== configFingerprint || entry.isolatedConfig !== isolatedConfig) { unprotectLaunchPortForUrl(entry.server.url); shutdownEntry(entry, "config_changed", args.logger); continue; @@ -1499,6 +1570,7 @@ export async function acquireDedicatedOpenCodeServer(args: { config: OpenCodeConfig; ownerKind: OpenCodeServerOwnerKind; ownerId?: string | null; + isolatedConfig?: boolean; logger?: Logger | null; }): Promise { const ownerKey = args.ownerKey.trim(); @@ -1506,10 +1578,11 @@ export async function acquireDedicatedOpenCodeServer(args: { throw new Error("ownerKey is required for dedicated OpenCode servers."); } const configFingerprint = serializeConfigFingerprint(args.config); + const isolatedConfig = args.isolatedConfig === true; return await withAcquireLock(`dedicated:${ownerKey}`, async () => { while (true) { const existing = dedicatedEntries.get(ownerKey); - if (existing && existing.configFingerprint === configFingerprint) { + if (existing && existing.configFingerprint === configFingerprint && existing.isolatedConfig === isolatedConfig) { existing.refCount += 1; existing.lastUsedAt = Date.now(); logRuntimeEvent(args.logger, "opencode.server_reused", existing, { refCount: existing.refCount }); @@ -1535,9 +1608,10 @@ export async function acquireDedicatedOpenCodeServer(args: { ownerId: args.ownerId, config: args.config, configFingerprint, + isolatedConfig, logger: args.logger, }); - if (entry.configFingerprint !== configFingerprint) { + if (entry.configFingerprint !== configFingerprint || entry.isolatedConfig !== isolatedConfig) { unprotectLaunchPortForUrl(entry.server.url); shutdownEntry(entry, "config_changed", args.logger); continue; @@ -1630,10 +1704,12 @@ export function __setOpenCodeProcessControllerForTests( export function __buildOpenCodeServeLaunchSpecForTests(args: { config: OpenCodeConfig; port?: number; + isolatedConfig?: boolean; }): OpenCodeServeLaunchSpec { return buildOpenCodeServeLaunchSpec({ port: args.port ?? 4096, config: args.config, + isolatedConfig: args.isolatedConfig, }); } diff --git a/apps/desktop/src/shared/adeCliGuidance.ts b/apps/desktop/src/shared/adeCliGuidance.ts index 17bfad3cc..85825219a 100644 --- a/apps/desktop/src/shared/adeCliGuidance.ts +++ b/apps/desktop/src/shared/adeCliGuidance.ts @@ -9,7 +9,6 @@ export const adeBundledAgentSkills = [ "ade-pr-workflows", "ade-lanes-git", "ade-linear", - "ade-orchestrator", "ade-proof-artifacts", "ade-deeplinks", "ade-search", diff --git a/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts b/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts index 808645a82..d6edb53ab 100644 --- a/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts +++ b/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts @@ -66,15 +66,14 @@ describe("orchestrationRuntimePolicy", () => { it("registers an MCP isolation mechanism for every provider that receives MCP", () => { // A provider added to ADE without an entry here is a compile error; this - // asserts the runtime shape and pins the one provider that has no mechanism - // so the gap cannot quietly become "gated" without someone editing a test. + // asserts the runtime shape so a provider cannot quietly lose its gate + // without someone editing a test. for (const provider of ["claude", "codex", "cursor", "droid", "opencode"] as const) { const isolation = orchestrationLeadMcpIsolation(provider); expect(isolation?.mechanism).toBeTruthy(); expect(isolation?.note.length).toBeGreaterThan(0); } - expect(ORCHESTRATION_LEAD_MCP_ISOLATION.droid.gated).toBe(false); - for (const provider of ["claude", "codex", "cursor", "opencode"] as const) { + for (const provider of ["claude", "codex", "cursor", "droid", "opencode"] as const) { expect(ORCHESTRATION_LEAD_MCP_ISOLATION[provider].gated).toBe(true); } expect(orchestrationLeadMcpIsolation("gemini")).toBeNull(); @@ -111,6 +110,13 @@ describe("orchestrationRuntimePolicy", () => { "mcp_servers = { linear = { command = \"linear\", args = [\"mcp\"] }, \"pg\" = { url = \"http://x\" } }", )).toEqual(["linear", "pg"]); + expect(codexConfiguredMcpServerNames([ + "mcp_servers = {", + " filesystem = { command = \"npx\", args = [\"-y\", \"filesystem\"] }, # inline comment", + " \"my shell\" = { command = \"sh -c '{ echo ok; }'\" },", + "}", + ].join("\n"))).toEqual(["filesystem", "my shell"]); + expect(codexConfiguredMcpServerNames("model = \"gpt-5.4\"")).toEqual([]); }); diff --git a/apps/desktop/src/shared/orchestrationRuntimePolicy.ts b/apps/desktop/src/shared/orchestrationRuntimePolicy.ts index 0caac334d..53317ed80 100644 --- a/apps/desktop/src/shared/orchestrationRuntimePolicy.ts +++ b/apps/desktop/src/shared/orchestrationRuntimePolicy.ts @@ -38,9 +38,8 @@ export const ORCHESTRATION_LOCKED_PERMISSION_MODE = "full-auto" satisfies AgentC // --------------------------------------------------------------------------- // Orchestrator-lead provider-native tool denials // -// The lead plans and delegates; it never edits code or runs shell (see -// resources/agent-skills/ade-orchestrator/SKILL.md). ADE's own orchestration -// toolset already withholds editFile/writeFile/bash from leads, but that +// The lead plans and delegates; it never mutates code or runs mutating shell. +// ADE's own orchestration toolset already withholds editFile/writeFile/bash from leads, but that // toolset is *additive* — it rides alongside each provider's built-in tools. // So every provider needs its own denial expressed in that provider's native // mechanism. All of those definitions live here so the lead's blast radius is @@ -160,22 +159,21 @@ export const ORCHESTRATION_LEAD_MCP_ISOLATION = { + "as risk `unknown`, and are denied by the fail-closed allow-list above.", }, droid: { - mechanism: "none", - gated: false, + mechanism: "session.listMcpTools + session-scoped toggleMcpTool", + gated: true, note: - "Droid's SDK exposes no session-scoped MCP restriction. `disabledToolIds` covers the exec tool " - + "catalog only, and toggleMcpServer/toggleMcpTool write the user's global Factory settings " - + "(ToggleMcpServerRequestParams pins settingsLevel: User), which would disable a server for " - + "every other droid session on the machine. ADE will not mutate user config to fake a " - + "session gate, so a droid lead still sees the user's MCP servers.", + "Droid's `disabledToolIds` covers the exec tool catalog only, so ADE enumerates the live MCP " + + "tool list and uses the low-level session-scoped `toggleMcpTool` RPC to disable every tool " + + "whose server is not an ADE lease. This does not call `toggleMcpServer`, whose SDK request " + + "pins settingsLevel: User and would mutate the user's global config.", }, opencode: { - mechanism: "ADE-authored server config + OPENCODE_DISABLE_PROJECT_CONFIG", + mechanism: "dedicated lead server + ADE-authored config + OPENCODE_DISABLE_PROJECT_CONFIG", gated: true, note: - "Every ADE OpenCode server runs with an ADE-owned XDG_CONFIG_HOME, OPENCODE_CONFIG_CONTENT " - + "built by buildOpenCodeConfig, and OPENCODE_DISABLE_PROJECT_CONFIG=1, so the only MCP servers " - + "any session (lead or worker) can see are ADE's own leases.", + "Only an orchestrator lead gets a dedicated OpenCode server with an ADE-owned XDG_CONFIG_HOME, " + + "OPENCODE_CONFIG_CONTENT built by buildOpenCodeConfig, and OPENCODE_DISABLE_PROJECT_CONFIG=1. " + + "Ordinary chats and workers keep the user's normal OpenCode config and MCP layers.", }, } as const satisfies Record; @@ -200,8 +198,8 @@ export const ORCHESTRATION_LEAD_CURSOR_SETTING_SOURCES = ["user", "team", "mdm"] * `mcp_servers..enabled`, so a lead's isolation is expressed as an * explicit `enabled = false` for every configured server. Handles the three * shapes `config.toml` can use: `[mcp_servers.name]` headers, dotted - * `mcp_servers.name.key = …` assignments, and an inline - * `mcp_servers = { name = { … } }` table. + * `mcp_servers.name.key = …` assignments, and single- or multi-line inline + * `mcp_servers = { name = { … } }` tables. */ export function codexConfiguredMcpServerNames(configText: string): string[] { const names: string[] = []; @@ -221,7 +219,20 @@ export function codexConfiguredMcpServerNames(configText: string): string[] { return segment.length ? segment : null; }; - for (const line of configText.replace(/\r\n?/g, "\n").split("\n")) { + const normalizedConfig = configText.replace(/\r\n?/g, "\n"); + const inlineAssignment = /^\s*mcp_servers\s*=\s*\{/gm; + let inlineMatch: RegExpExecArray | null; + while ((inlineMatch = inlineAssignment.exec(normalizedConfig)) !== null) { + const openBrace = normalizedConfig.indexOf("{", inlineMatch.index); + const closeBrace = findTomlInlineTableEnd(normalizedConfig, openBrace); + if (openBrace < 0 || closeBrace < 0) continue; + for (const key of inlineTableKeys(stripTomlComments( + normalizedConfig.slice(openBrace + 1, closeBrace), + ))) add(key); + inlineAssignment.lastIndex = closeBrace + 1; + } + + for (const line of normalizedConfig.split("\n")) { const withoutComment = line.replace(/^\s*#.*$/, "").trim(); if (!withoutComment.length) continue; @@ -241,12 +252,73 @@ export function codexConfiguredMcpServerNames(configText: string): string[] { const inline = withoutComment.match(/^mcp_servers\s*=\s*\{(.*)\}\s*$/)?.[1]; if (inline !== undefined) { - for (const key of inlineTableKeys(inline)) add(key); + for (const key of inlineTableKeys(stripTomlComments(inline))) add(key); } } return names; } +function stripTomlComments(text: string): string { + let output = ""; + let quote: string | null = null; + for (let index = 0; index < text.length; index += 1) { + const char = text[index]!; + if (quote) { + output += char; + if (quote === '"' && char === "\\") { + const next = text[index + 1]; + if (next !== undefined) output += next; + index += 1; + } else if (char === quote) { + quote = null; + } + continue; + } + if (char === '"' || char === "'") { + quote = char; + output += char; + } else if (char === "#") { + while (index < text.length && text[index] !== "\n") index += 1; + if (index < text.length) output += "\n"; + } else { + output += char; + } + } + return output; +} + +function findTomlInlineTableEnd(text: string, openBrace: number): number { + if (openBrace < 0) return -1; + let depth = 0; + let quote: string | null = null; + for (let index = openBrace; index < text.length; index += 1) { + const char = text[index]!; + if (quote) { + if (quote === '"' && char === "\\") { + index += 1; + } else if (char === quote) { + quote = null; + } + continue; + } + if (char === '"' || char === "'") { + quote = char; + continue; + } + if (char === "#") { + while (index < text.length && text[index] !== "\n") index += 1; + continue; + } + if (char === "{") { + depth += 1; + } else if (char === "}") { + depth -= 1; + if (depth === 0) return index; + } + } + return -1; +} + /** Top-level `key =` names of a single-line TOML inline table body. */ function inlineTableKeys(body: string): string[] { const keys: string[] = []; From 10bb3148f5ed554cc0878263e1529f2dd92139ee Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:01:41 -0400 Subject: [PATCH 5/6] test: align guidance contract and document provider boundaries --- apps/desktop/src/shared/adeCliGuidance.test.ts | 11 +++++++---- docs/features/agents/README.md | 15 +++++++++++++++ docs/features/chat/README.md | 2 +- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/apps/desktop/src/shared/adeCliGuidance.test.ts b/apps/desktop/src/shared/adeCliGuidance.test.ts index 68929d810..39e4eeb33 100644 --- a/apps/desktop/src/shared/adeCliGuidance.test.ts +++ b/apps/desktop/src/shared/adeCliGuidance.test.ts @@ -76,14 +76,17 @@ describe("ADE bootstrap guidance", () => { for (const invariant of [ "ade chat note", "ade chat ask", - "6 words or fewer", - `${MAX_STATUS_NOTE_CHARACTERS} characters`, - `${STATUS_NOTE_GUIDELINE_WORDS} words or fewer`, - "next accepted user message clears the prior hand-raise", "You cannot settle or unsettle a session", ]) { expect(bootstrap.toLowerCase()).toContain(invariant.toLowerCase()); expect(skill.toLowerCase()).toContain(invariant.toLowerCase()); } + expect(bootstrap).toContain("next accepted user message clears the prior hand-raise"); + expect(skill).toContain("next accepted user message clears the hand-raise"); + // The skill deliberately points at the code-owned bounds instead of + // duplicating values that can drift from the runtime contract. + expect(skill).toContain("sessionStatusNote.ts"); + expect(bootstrap).toContain(`${STATUS_NOTE_GUIDELINE_WORDS} words or fewer`); + expect(bootstrap).toContain(`${MAX_STATUS_NOTE_CHARACTERS} characters`); }); }); diff --git a/docs/features/agents/README.md b/docs/features/agents/README.md index e894f4d4f..38f7bfb85 100644 --- a/docs/features/agents/README.md +++ b/docs/features/agents/README.md @@ -255,6 +255,21 @@ The project surfaces use `buildCodingAgentSystemPrompt` with different identity/ repository/project is attached and that explicit filesystem/shell work must remain inside the supplied scratch cwd. +### Orchestration boundary and provider capabilities + +The orchestration protocol is injected into the provider system/developer +prompt only for sessions carrying an orchestration role. Ordinary chats do +not receive or follow that protocol, and `ade-orchestrator` is not a bundled +skill. Orchestrator leads may inspect their lane but their provider-native +mutating tools are denied at each provider's runtime boundary; workers retain +the tools needed to edit and validate. + +Provider capability isolation is role-scoped. In particular, ordinary +OpenCode chats and workers retain the user's OpenCode configuration, project +configuration, and MCP servers. Only an OpenCode orchestration lead receives +ADE's isolated configuration and ADE-owned MCP lease. Other providers apply +their equivalent lead gate without changing ordinary-chat configuration. + ## Smart memory and reconstruction The CTO's durable memory lives in files under `.ade/cto/` (`MEMORY.md`, `thread-state.md`, `daily/.md`) owned by `ctoMemoryService`. A deterministic flush writes the rolling summary before compaction and before model switches; a best-effort LLM upgrade refines it. `refreshReconstructionContext()` re-injects identity plus memory after compaction and switches. Full details in [Identity and Personas](identity-and-personas.md#smart-memory-system). diff --git a/docs/features/chat/README.md b/docs/features/chat/README.md index 0b0636a2b..09ffa53a9 100644 --- a/docs/features/chat/README.md +++ b/docs/features/chat/README.md @@ -22,7 +22,7 @@ for its separate RPC, sync, storage, and UI contracts. | `apps/desktop/src/renderer/components/chat/CrossMachineHandoffModal.tsx`, `crossMachineHandoffPresentation.tsx` | **Send to machine** workflow in the Handoff tab: source Git readiness, eligible connected-machine selection, brief or full-history fork selection, the destination chat's model / reasoning effort / fast mode / permission mode (the shared `PermissionModePicker` and `ReasoningEffortPicker`, each self-hiding when the chosen model can't honor it), optional continuation note, destination project matching or confirmed clone, storage/auth/model/commit/lane checks, a **Fetch & fast-forward there** offer when the destination lane is clean and a strict ancestor of the source commit, transport disclosure, route-pinned final send, and recoverable source-marker completion. Source blockers are `BlockedActionReason` values rendered next to a `BlockedActionButton`, so no blocker can hide behind a disabled control. `crossMachineHandoffPresentation.tsx` holds the pure half — stage/mode types, `SourceCheck`, branch/route/readiness copy, permission tone and icon maps, and `CheckRow` — so the copy and lookups that shipped wrong are directly testable. Cross-machine fork transports provider-native history for Claude, Codex, and OpenCode; Cursor and Droid use brief mode because their histories are not portable between machines yet. A fork that can't be completed always degrades to a one-click brief rather than a dead end: an older destination that omits `forkHandoffSupport`, a history over the transport cap, or an unforkable provider file (e.g. a Codex `.zst` rollout) each surface a plain-language reason and a **send as brief** action that re-runs prepare + preflight in brief mode. The insecure-route consent line is fork-aware — a fork discloses that the full chat history is sent exactly as recorded, while a brief states only the summary is sent, never secrets. See [Cross-machine session handoff](../sync-and-multi-device/cross-machine-session-handoff.md). | | `apps/desktop/src/shared/crossMachineHandoff.ts` and `apps/desktop/src/shared/types/chat.ts` | Renderer-safe Git-origin normalization, portable remote sanitization, untrusted remote-response decoders, and the versioned capsule/preflight/accept DTOs shared across renderer, preload, Electron main, and the ADE runtime. `chat.ts` also owns the fork-handoff contract: `HANDOFF_FORK_PROVIDERS` (`claude`, `codex`, `opencode`, `droid`) + `providerSupportsHandoffFork()`, `AgentChatHandoffArgs.targetLaneId` (brief may retarget any lane in the project; fork must stay in the source lane), the cross-machine capsule's optional `mode: "brief" \| "fork"` with `forkTransport` (provider-native session files) and `transcriptEnvelopes` (gzipped ADE JSONL), and the preflight's optional `forkHandoffSupport` (absent = older destination the source must treat as fork-unsupported, so a fork never silently downgrades to a brief). Cross-machine fork has its own narrower list: `CROSS_MACHINE_HANDOFF_FORK_PROVIDERS` + `providerSupportsCrossMachineHandoffFork()`, derived from `HANDOFF_FORK_PROVIDERS` by filtering Droid out (its session index is machine-local) so the two lists cannot drift. The preflight also carries an optional `laneFastForward` (`laneId`, `laneName`, `behindBy`) — the destination's own assertion that its existing lane is clean and a strict ancestor of the source commit. `decodeCrossMachineDestinationPreflightResult` decodes `forkHandoffSupport` and `laneFastForward` only when present, and rejects a `behindBy` that is not a positive integer because the destination refuses a zero-distance fast-forward. `chat.ts` is also the canonical cross-client contract for context-usage state/sample metadata, Claude result provenance/error/correlation fields, queue-aware interrupt results, the bounded `queue_recovery` lifecycle, and the desktop prompt-stash DTOs plus `MAX_PROMPT_STASHES`. | | `apps/desktop/src/main/services/chat/crossMachineForkTransport.ts` | Node-only fork-transport plumbing shared by the source packaging and destination materialization paths. Owns the uncompressed limits (18 MiB provider main session file, 4 MiB total Claude sidecars, 3 MiB ADE transcript envelopes), the independent base64 bounds that reject oversized input before decoding, and `CROSS_MACHINE_FORK_ENCODED_BUDGET_BYTES` (20 MiB) — a whole-capsule encoded budget kept under the 25 MiB sync-envelope/WebSocket payload caps. `gzipToBase64` / `gunzipFromBase64` (the latter enforces a max output length) do the compression; `enforceCrossMachineForkEncodedBudget` drops the sidecar group first and only throws a "too large, send a brief" error when the main file plus transcript alone blow the budget; `crossMachineForkOversizeError` returns the typed `CROSS_MACHINE_FORK_OVERSIZE` failure; `runCliCapture` buffers `opencode export` / `import` stdout/stderr with a timeout; and `validateForkTransport` re-validates a received capsule's transport (provider match, kind allowlist, base64 shape, path-traversal-safe side-file paths, per-file and total size caps) before any decode. | -| `apps/desktop/src/main/services/chat/agentChatService.ts` | Main service: session lifecycle, external chat import orchestration (`importExternalChatSession` for Claude/Codex sessions discovered by the external-session service), turn dispatch, event emission, provider adapters, steer queue, handoff, auto-title, prompt-derived lane-name suggestions for auto-created / parallel lanes, event-history snapshots, durable chat transcript replay/storage compaction, slash-command discovery/merge (delegates to per-provider discovery modules and `slashCommandPromptExpansion` for unified prompt expansion), and active-workload detection used by project/window close guards. Codex non-retrying app-server failures are deduplicated by turn plus semantic error identity across the early `error` notification and terminal `turn/completed`; retrying notifications (`willRetry: true`) remain provider-health notices while the turn stays active. Lane naming and chat auto-titling both run through the session-intelligence prompt path over the shared candidate chain in `sessionNaming.ts` (configured `titleModelId` → the model the chat was launched with → a model from another provider → a sibling on the leading provider), and only then fall back to a deterministic prompt-derived title/slug; branch uniqueness is handled by the lane id suffix added by lane creation. Tracks Fast Mode with the legacy `codexFastMode: boolean` session field for every provider whose descriptor advertises `serviceTiers: ["fast"]`; Codex forwards it as `serviceTier: "fast" \| null` on every `thread/start` and `turn/start` JSON-RPC call, while Cursor SDK sessions resolve it through discovered model parameters (see [Agent Routing](agent-routing.md#provider-service-tiers-fast-mode)). Codex chat goals are managed through the app-server `thread/goal/get` / `set` / `clear` RPCs, persisted in session summaries, validated to the provider's 4,000-character objective limit, and normalized to ADE's unlimited-budget policy by sending `tokenBudget: null` and clearing provider-reported budgets. `applyCodexEffectiveThreadState` accepts a `requestedCodexPolicy` option and uses `shouldPreserveRequestedCodexPolicy` to keep ADE-controlled picker selections authoritative when the lifecycle response echoes an older thread policy (prevents a manual Plan→Edit switch from snapping back); it also syncs the abstract `permissionMode` via `syncLegacyPermissionMode` after every policy application. Whenever an `updateSession` touches any permission/interaction/mode field, the service also emits a transient `session_meta_updated` chat event carrying the recomputed mode fields (`permissionMode`, `interactionMode`, `claudePermissionMode`, `codexApprovalPolicy`/`codexSandbox`/`codexConfigSource`, `opencodePermissionMode`, `droidPermissionMode`, `cursorModeId`, and the `cursorModeSnapshot`) so any other client viewing the same session — a desktop refreshing a session an iOS device just re-moded, or vice versa — updates its composer controls live. It is a direct state patch, emitted after the Cursor policy sync so `cursorModeSnapshot` reflects the recomputed mode, and is kept off the session-list refresh path. Builds ADE guidance from the active lane worktree so Agent Skill roots are lane-scoped in persistent system/developer prompts and provider fallback injection. `buildAgentRuntimeEnv(managed)` stamps every SDK-backed provider process with `ADE_CHAT_SESSION_ID`, `ADE_DEFAULT_ROLE=agent` (or `orchestrator` for a lead), `ADE_LANE_ID`, `ADE_PROJECT_ROOT`, and `ADE_WORKSPACE_ROOT`; the persistent guidance also names the concrete `--session ` argument for status commands so shared SDK servers do not depend on process-global env inheritance. `dismissPendingInputForSettlement` is the provider-neutral quieting boundary used by **Dismiss & settle**: it interrupts live Claude/Codex/OpenCode/Cursor/Droid turns best-effort, cancels local/provider waiters, removes Codex plan follow-ups, emits pending-input resolution, and persists an idle session before settle is written. When the session has Linear issues attached (`session_linear_issues`), `buildAgentRuntimeEnv` also materializes them into a per-session context file via `writeSessionLinearIssueContextFile` (`//linear-issues.json`, written atomically; stale files cleared when nothing is attached) and sets `ADE_LINEAR_ISSUE_IDS` (comma-joined identifiers) + `ADE_LINEAR_CONTEXT_FILE` so the agent reads its issue context without Linear credentials. Attaching a `linear_issue` context attachment at run time calls `laneService.attachLinearIssueToSession({ chatSessionId, issues, role: "worked", source: "chat_attach", includeInPr: true })` so the link is persisted even for standalone (laneless) chats; when the session has a lane it additionally runs `laneService.linkLinearIssues` for the lane/PR-card semantics. See [Linear integration](../linear-integration/README.md#session-scoped-issue-attachment-and-cli-context-injection). Claude SDK sessions also resolve the executable through `claudeCodeExecutable.ts` and pass `pathToClaudeCodeExecutable` so packaged builds can prefer the bundled native binary before PATH/auth fallbacks; interrupted Claude turns stop active subagents before emitting stopped `subagent_result`s, and every `subagent_result` is gated on a previously emitted `subagent_started` (tracked in `emittedSubagentStartIds`) so an interrupt can never emit a phantom stopped card for a subagent that never announced — terminal events clear both the taskId and agentId aliases. A plain Claude Code task run (`task_type` `other`, no agent metadata — e.g. "Re-run affected test files") is tracked for cleanup but never surfaces subagent rows. Claude resume paths run `claudeThinkingTranscriptRepair` before loading a transcript, and the runtime self-heals the same corruption after the Anthropic thinking-block 400 error. Plan-mode transitions run through `claudePlanMode.ts` and emit a plan-mode notice carrying the resulting access mode, so the renderer composer chip updates from an authoritative value even when the session refresh races with compaction. Cursor SDK setup records interrupts that arrive while the worker is still being acquired, releases the acquired generation if setup loses the race, and suppresses false provider-health failures for user-initiated setup interrupts. Cursor provider slash commands use a dedicated discovery path (`cursorSlashCommandDiscovery`) instead of falling through to the generic filesystem-backed list. Claude query startup is single-flight: concurrent `ensureClaudeQuery` callers latch onto one in-flight `queryStartPromise`, and a per-runtime `queryGeneration` token aborts and reaps a start that a reset or interrupt superseded, so a resumed session never spawns twin subprocesses; both reset and interrupt reap the SDK subprocess through `claudeSubprocessReaper` because a closed `query()` still leaves a live `claude --resume` child. `run_in_background` shell tasks (SDK `task_type` `local_bash`/`background`) survive turn boundaries — the query stays alive across turns and delivers their real completion — so only interrupt, reset/dispose, or a host-restart rebind settle them as stopped; a reset that orphans still-open background tasks emits one `system_notice` that they were stopped without reporting completion, and background-task titles are sticky (the first spawn description is reused through the terminal row). A durable per-`(SDK message id, content index)` emitted-text record keeps a re-delivered assistant snapshot (after a stream-dedup reset from steer, message interleave, or idle handoff) from doubling the transcript. Claude `TaskCreate`/`TaskUpdate` tracking keys creates by tool-use id and remaps the harness's ordinal task id onto the Nth created task; an update for an id it cannot resolve or describe changes nothing rather than fabricating a todo row. `steer()` returns `AgentChatSteerResult` (`{ steerId, queued, reason?: "queue_full" }`); reasoning effort is normalized and applied at steer delivery, and an active Claude `interrupt-replace` uses SDK priority `now` without tearing down the query or its background work. When a spawned child chat ends, `reportChildSpawnEnded` reports its outcome to the spawner according to the child's `spawnKind`; an active Claude parent receives SDK `priority: "next"` delivery, an active Codex parent receives `turn/steer`, and idle or provider-fallback parents receive the normal message path, while scheduled work remains boundary-delivered (see [Spawn types and completion reporting](#spawn-types-and-completion-reporting)). Spawned agents also inherit `ADE_PARENT_CHAT_SESSION_ID` / `ADE_SPAWN_KIND` and a subagent self-report guidance line. Fork/import history seeding (`appendImportedChatEvents`) is chunked with event-loop yields, defers transcript flushes to chunk boundaries, and never publishes seeded historical envelopes to live event subscribers — readers load them via history APIs; live-publishing an entire source chat froze the app during fork handoff (ADE-122). The `chat.handoffSession` / `chat.prepareCrossMachineHandoff` runtime actions carry extended timeouts (120s daemon action, 150s IPC) because a brief handoff spans AI-brief generation plus first-message dispatch — the old 30s default fired a false timeout while the daemon-side handoff completed anyway. For orchestrator-lead sessions it builds the read-only capability services (`buildOrchestrationLeadReadServices` → `searchWorkspace` / `readLinearIssue` / `readPr` / `listProofArtifacts` / `mintDeeplink`), wiring each only when the backing service exists so a null service degrades to an omitted tool rather than a crash. Large service file. | +| `apps/desktop/src/main/services/chat/agentChatService.ts` | Main service: session lifecycle, external chat import orchestration (`importExternalChatSession` for Claude/Codex sessions discovered by the external-session service), turn dispatch, event emission, provider adapters, steer queue, handoff, auto-title, prompt-derived lane-name suggestions for auto-created / parallel lanes, event-history snapshots, durable chat transcript replay/storage compaction, slash-command discovery/merge (delegates to per-provider discovery modules and `slashCommandPromptExpansion` for unified prompt expansion), and active-workload detection used by project/window close guards. Codex non-retrying app-server failures are deduplicated by turn plus semantic error identity across the early `error` notification and terminal `turn/completed`; retrying notifications (`willRetry: true`) remain provider-health notices while the turn stays active. Lane naming and chat auto-titling both run through the session-intelligence prompt path over the shared candidate chain in `sessionNaming.ts` (configured `titleModelId` → the model the chat was launched with → a model from another provider → a sibling on the leading provider), and only then fall back to a deterministic prompt-derived title/slug; branch uniqueness is handled by the lane id suffix added by lane creation. Tracks Fast Mode with the legacy `codexFastMode: boolean` session field for every provider whose descriptor advertises `serviceTiers: ["fast"]`; Codex forwards it as `serviceTier: "fast" \| null` on every `thread/start` and `turn/start` JSON-RPC call, while Cursor SDK sessions resolve it through discovered model parameters (see [Agent Routing](agent-routing.md#provider-service-tiers-fast-mode)). Codex chat goals are managed through the app-server `thread/goal/get` / `set` / `clear` RPCs, persisted in session summaries, validated to the provider's 4,000-character objective limit, and normalized to ADE's unlimited-budget policy by sending `tokenBudget: null` and clearing provider-reported budgets. `applyCodexEffectiveThreadState` accepts a `requestedCodexPolicy` option and uses `shouldPreserveRequestedCodexPolicy` to keep ADE-controlled picker selections authoritative when the lifecycle response echoes an older thread policy (prevents a manual Plan→Edit switch from snapping back); it also syncs the abstract `permissionMode` via `syncLegacyPermissionMode` after every policy application. Whenever an `updateSession` touches any permission/interaction/mode field, the service also emits a transient `session_meta_updated` chat event carrying the recomputed mode fields (`permissionMode`, `interactionMode`, `claudePermissionMode`, `codexApprovalPolicy`/`codexSandbox`/`codexConfigSource`, `opencodePermissionMode`, `droidPermissionMode`, `cursorModeId`, and the `cursorModeSnapshot`) so any other client viewing the same session — a desktop refreshing a session an iOS device just re-moded, or vice versa — updates its composer controls live. It is a direct state patch, emitted after the Cursor policy sync so `cursorModeSnapshot` reflects the recomputed mode, and is kept off the session-list refresh path. Builds ADE guidance from the active lane worktree so Agent Skill roots are lane-scoped in persistent system/developer prompts and provider fallback injection. `buildAgentRuntimeEnv(managed)` stamps every SDK-backed provider process with `ADE_CHAT_SESSION_ID`, `ADE_DEFAULT_ROLE=agent` (or `orchestrator` for a lead), `ADE_LANE_ID`, `ADE_PROJECT_ROOT`, and `ADE_WORKSPACE_ROOT`; the persistent guidance also names the concrete `--session ` argument for status commands so shared SDK servers do not depend on process-global env inheritance. `dismissPendingInputForSettlement` is the provider-neutral quieting boundary used by **Dismiss & settle**: it interrupts live Claude/Codex/OpenCode/Cursor/Droid turns best-effort, cancels local/provider waiters, removes Codex plan follow-ups, emits pending-input resolution, and persists an idle session before settle is written. When the session has Linear issues attached (`session_linear_issues`), `buildAgentRuntimeEnv` also materializes them into a per-session context file via `writeSessionLinearIssueContextFile` (`//linear-issues.json`, written atomically; stale files cleared when nothing is attached) and sets `ADE_LINEAR_ISSUE_IDS` (comma-joined identifiers) + `ADE_LINEAR_CONTEXT_FILE` so the agent reads its issue context without Linear credentials. Attaching a `linear_issue` context attachment at run time calls `laneService.attachLinearIssueToSession({ chatSessionId, issues, role: "worked", source: "chat_attach", includeInPr: true })` so the link is persisted even for standalone (laneless) chats; when the session has a lane it additionally runs `laneService.linkLinearIssues` for the lane/PR-card semantics. See [Linear integration](../linear-integration/README.md#session-scoped-issue-attachment-and-cli-context-injection). Claude SDK sessions also resolve the executable through `claudeCodeExecutable.ts` and pass `pathToClaudeCodeExecutable` so packaged builds can prefer the bundled native binary before PATH/auth fallbacks; interrupted Claude turns stop active subagents before emitting stopped `subagent_result`s, and every `subagent_result` is gated on a previously emitted `subagent_started` (tracked in `emittedSubagentStartIds`) so an interrupt can never emit a phantom stopped card for a subagent that never announced — terminal events clear both the taskId and agentId aliases. A plain Claude Code task run (`task_type` `other`, no agent metadata — e.g. "Re-run affected test files") is tracked for cleanup but never surfaces subagent rows. Claude resume paths run `claudeThinkingTranscriptRepair` before loading a transcript, and the runtime self-heals the same corruption after the Anthropic thinking-block 400 error. Plan-mode transitions run through `claudePlanMode.ts` and emit a plan-mode notice carrying the resulting access mode, so the renderer composer chip updates from an authoritative value even when the session refresh races with compaction. Cursor SDK setup records interrupts that arrive while the worker is still being acquired, releases the acquired generation if setup loses the race, and suppresses false provider-health failures for user-initiated setup interrupts. Cursor provider slash commands use a dedicated discovery path (`cursorSlashCommandDiscovery`) instead of falling through to the generic filesystem-backed list. Claude query startup is single-flight: concurrent `ensureClaudeQuery` callers latch onto one in-flight `queryStartPromise`, and a per-runtime `queryGeneration` token aborts and reaps a start that a reset or interrupt superseded, so a resumed session never spawns twin subprocesses; both reset and interrupt reap the SDK subprocess through `claudeSubprocessReaper` because a closed `query()` still leaves a live `claude --resume` child. `run_in_background` shell tasks (SDK `task_type` `local_bash`/`background`) survive turn boundaries — the query stays alive across turns and delivers their real completion — so interrupt, reset/dispose, a native subagent exit, or a host-restart rebind settle them as stopped; a reset that orphans still-open background tasks emits one `system_notice` that they were stopped without reporting completion, and background-task titles are sticky (the first spawn description is reused through the terminal row). A durable per-`(SDK message id, content index)` emitted-text record keeps a re-delivered assistant snapshot (after a stream-dedup reset from steer, message interleave, or idle handoff) from doubling the transcript. Claude `TaskCreate`/`TaskUpdate` tracking keys creates by tool-use id and remaps the harness's ordinal task id onto the Nth created task; an update for an id it cannot resolve or describe changes nothing rather than fabricating a todo row. `steer()` returns `AgentChatSteerResult` (`{ steerId, queued, reason?: "queue_full" }`); reasoning effort is normalized and applied at steer delivery, and an active Claude `interrupt-replace` uses SDK priority `now` without tearing down the query or its background work. When a spawned child chat ends, `reportChildSpawnEnded` reports its outcome to the spawner according to the child's `spawnKind`; an active Claude parent receives SDK `priority: "next"` delivery, an active Codex parent receives `turn/steer`, and idle or provider-fallback parents receive the normal message path, while scheduled work remains boundary-delivered (see [Spawn types and completion reporting](#spawn-types-and-completion-reporting)). Spawned agents also inherit `ADE_PARENT_CHAT_SESSION_ID` / `ADE_SPAWN_KIND` and a subagent self-report guidance line. Fork/import history seeding (`appendImportedChatEvents`) is chunked with event-loop yields, defers transcript flushes to chunk boundaries, and never publishes seeded historical envelopes to live event subscribers — readers load them via history APIs; live-publishing an entire source chat froze the app during fork handoff (ADE-122). The `chat.handoffSession` / `chat.prepareCrossMachineHandoff` runtime actions carry extended timeouts (120s daemon action, 150s IPC) because a brief handoff spans AI-brief generation plus first-message dispatch — the old 30s default fired a false timeout while the daemon-side handoff completed anyway. For orchestrator-lead sessions it builds the read-only capability services (`buildOrchestrationLeadReadServices` → `searchWorkspace` / `readLinearIssue` / `readPr` / `listProofArtifacts` / `mintDeeplink`), wiring each only when the backing service exists so a null service degrades to an omitted tool rather than a crash. Large service file. | | `apps/desktop/src/main/services/chat/sessionNaming.ts` | Canonical home for everything the three naming callers share — automatic lane identity, chat auto-title, and the legacy lane-name suggestion — because each used to carry its own hand-copied chain that had already drifted. Owns the three system prompts and the lane-identity JSON schema, `MAX_NAMING_WORDS` (six words, handed to the model as a **guideline**: an over-long answer is clamped, never rejected, because a clamped real name beats a slug), `isProviderLevelNamingFailure` (a missing/unusable CLI, auth, quota, or an account that cannot run the model — including the "model is not supported when using X with a Y account" 400; it deliberately excludes "not supported for/on/by", which describes one model lacking a capability and must still retry a sibling), `buildNamingModelCandidates` (preferred ids → a model from a provider none of them belong to → a sibling on the leading provider, so a cross-provider candidate is always reachable), and `runNamingAcrossProviders` (walks the chain up to three attempts; a provider-level failure condemns every remaining model behind that provider, `run` returning null means "answered unusably" and the next candidate still gets a turn, and `shouldStop` abandons the chain when the user renames mid-flight). | | `apps/desktop/src/main/services/chat/chatMentionService.ts` | Composer @-mention service (chats / lanes / terminals), created inside `agentChatService` with injected roster/transcript/PTY deps. Owns the keystroke-rate `chat.listMentionSuggestions` action (daemon-routed, read-only): one shared 1.5 s-TTL roster cache with a single in-flight promise collapses a typing burst into one sessions/lanes/terminals read, per-source failures degrade only their own menu section, and ranking/caps come from `shared/chatMentions.ts`. Also owns send-time expansion: `applyChatMentionExpansion` rewrites send/steer args so the provider receives `` pointer blocks (identity attributes, a ≤1 KB CRLF-normalized neutralized preview, and literal `ade chat read` / `ade lanes show` / `ade terminal read` / `ade search` commands — double-quoted-only so they paste into sh, PowerShell, and cmd) while `displayText` keeps the user's literal chips. Idempotence uses a module-private Symbol marker (structured clone strips it, so nothing over IPC/sync can pre-mark), the single expansion owner on the steer side is `steerWithOptions`, and slash-command prompt rewrites re-attach blocks via `carryChatMentionBlocks`. Lane details never derive git state from `lane.status` (lanes are listed without a status probe and the unprobed default is indistinguishable from clean). Fires the content-free `onMentionsExpanded` analytics hook once per send that actually gained blocks. | | `apps/desktop/src/shared/chatMentions.ts` | Pure, surface-agnostic mention grammar shared by desktop, TUI, web preview mock, and (future) iOS: `@chat:` / `@lane:` / `@term:` token parsing derived from one prefix table (`CHAT_MENTION_KINDS` is the canonical kind order), word-boundary matching so emails never match, `renderChatMentionBlock` (attribute escaping + preview truncation on line boundaries + neutralization of forged `` tags and block headers so another session's transcript text cannot inject fake pointer blocks), `rankChatMentionSuggestions` (exact > prefix > substring > subsequence, recency tie-break, deterministic id tie-break), and per-message caps (8/kind menu rows, 12 expansions, 1024-char previews). Types live in `shared/types/chatMentions.ts`. | From 9d1caf10fbfc294ce1b6897ed8ac5d112b9d3b11 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:09:43 -0400 Subject: [PATCH 6/6] fix(orchestrator): harden Codex MCP isolation parsing --- .../shared/orchestrationRuntimePolicy.test.ts | 13 ++++++- .../src/shared/orchestrationRuntimePolicy.ts | 38 +++++++++++++++---- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts b/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts index d6edb53ab..3fb5fc7ec 100644 --- a/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts +++ b/apps/desktop/src/shared/orchestrationRuntimePolicy.test.ts @@ -77,6 +77,8 @@ describe("orchestrationRuntimePolicy", () => { expect(ORCHESTRATION_LEAD_MCP_ISOLATION[provider].gated).toBe(true); } expect(orchestrationLeadMcpIsolation("gemini")).toBeNull(); + expect(orchestrationLeadMcpIsolation("toString")).toBeNull(); + expect(orchestrationLeadMcpIsolation("__proto__")).toBeNull(); }); it("drops Cursor's MCP-carrying setting layers for a lead but keeps the ADE hook layer", () => { @@ -92,7 +94,7 @@ describe("orchestrationRuntimePolicy", () => { expect(codexConfiguredMcpServerNames([ "model = \"gpt-5.4\"", "", - "[mcp_servers.filesystem]", + "[mcp_servers.filesystem] # trailing comment", "command = \"npx\"", "args = [\"-y\", \"@modelcontextprotocol/server-filesystem\"]", "", @@ -106,6 +108,15 @@ describe("orchestrationRuntimePolicy", () => { "# mcp_servers.commented.command = \"nope\"", ].join("\n"))).toEqual(["filesystem", "my shell", "git"]); + expect(codexConfiguredMcpServerNames([ + "[mcp_servers] # bare table", + "filesystem = { command = \"npx\" } # trailing comment", + "\"my shell\" = { command = \"sh\" } # trailing comment", + "", + "[other]", + "ignored = true", + ].join("\n"))).toEqual(["filesystem", "my shell"]); + expect(codexConfiguredMcpServerNames( "mcp_servers = { linear = { command = \"linear\", args = [\"mcp\"] }, \"pg\" = { url = \"http://x\" } }", )).toEqual(["linear", "pg"]); diff --git a/apps/desktop/src/shared/orchestrationRuntimePolicy.ts b/apps/desktop/src/shared/orchestrationRuntimePolicy.ts index 53317ed80..76239c8c3 100644 --- a/apps/desktop/src/shared/orchestrationRuntimePolicy.ts +++ b/apps/desktop/src/shared/orchestrationRuntimePolicy.ts @@ -180,8 +180,8 @@ export const ORCHESTRATION_LEAD_MCP_ISOLATION = { export function orchestrationLeadMcpIsolation( provider: AgentChatProvider | string, ): OrchestrationLeadMcpIsolation | null { - return (ORCHESTRATION_LEAD_MCP_ISOLATION as Record)[provider] - ?? null; + if (!Object.prototype.hasOwnProperty.call(ORCHESTRATION_LEAD_MCP_ISOLATION, provider)) return null; + return ORCHESTRATION_LEAD_MCP_ISOLATION[provider as McpCapableProvider]; } /** @@ -221,6 +221,7 @@ export function codexConfiguredMcpServerNames(configText: string): string[] { const normalizedConfig = configText.replace(/\r\n?/g, "\n"); const inlineAssignment = /^\s*mcp_servers\s*=\s*\{/gm; + let inBareMcpServersTable = false; let inlineMatch: RegExpExecArray | null; while ((inlineMatch = inlineAssignment.exec(normalizedConfig)) !== null) { const openBrace = normalizedConfig.indexOf("{", inlineMatch.index); @@ -233,16 +234,39 @@ export function codexConfiguredMcpServerNames(configText: string): string[] { } for (const line of normalizedConfig.split("\n")) { - const withoutComment = line.replace(/^\s*#.*$/, "").trim(); + const withoutComment = stripTomlComments(line).trim(); if (!withoutComment.length) continue; - const header = withoutComment.match(/^\[\[?\s*mcp_servers\s*\.\s*(.+?)\s*\]\]?$/)?.[1]; - if (header) { - const segment = firstSegment(header); - if (segment) add(segment); + const tableHeader = withoutComment.match(/^\[\[?\s*(.*?)\s*\]\]?$/)?.[1]?.trim(); + if (tableHeader !== undefined) { + if (tableHeader === "mcp_servers") { + inBareMcpServersTable = true; + continue; + } + + const header = tableHeader.match(/^mcp_servers\s*\.\s*(.+)$/)?.[1]; + if (header) { + const segment = firstSegment(header); + if (segment) add(segment); + inBareMcpServersTable = false; + continue; + } + + inBareMcpServersTable = false; continue; } + if (inBareMcpServersTable) { + const bareAssignment = withoutComment.match( + /^((?:"[^"]+"|'[^']+'|[A-Za-z0-9_-]+)(?:\s*\.\s*(?:"[^"]+"|'[^']+'|[A-Za-z0-9_-]+))*)\s*=/, + )?.[1]; + if (bareAssignment) { + const segment = firstSegment(bareAssignment); + if (segment) add(segment); + continue; + } + } + const dotted = withoutComment.match(/^mcp_servers\s*\.\s*(.+?)\s*=/)?.[1]; if (dotted) { const segment = firstSegment(dotted);