CTO tools wiring + iOS attention -> Primary - #987
Conversation
…reach iOS Three follow-ups to #986, all rooted in the same fact: the CTO session is pinned to the project's PRIMARY lane. It does not have a lane of its own. Operator tools now execute. createCtoOperatorTools was only ever reached by previewSessionToolNames and the prompt manifest, so the CTO was told it had a tool surface it could not call. createCtoRuntimeToolMap now feeds all five provider transports: an `ade-cto` SDK MCP server for Claude (deliberately without the orchestration lead's allowManagedMcpServersOnly lockdown, which would strip the user's own MCP servers from a daily-driver chat), the `ade_cto` namespace inside Codex's single dynamic-tool refresher, and a second HTTP MCP lease for Cursor/Droid/OpenCode. buildCtoOperatorToolDeps is shared by the preview and runtime paths so the advertised and callable surfaces cannot drift. Mutating git tools no longer default their lane. resolveLaneId defaulted to the CTO's session lane, so an omitted laneId on gitCommit/gitPush/gitPull — or on gitUndo/gitRedo, which run `git reset --hard` — meant writing to the primary worktree, the exact thing lanes exist to prevent. Reads keep the default; mutations must name a lane. Enforced in code rather than trusting the model to have read the manifest rule. Attention reaches iOS. The dot was desktop-only because the phone talks to the CTO over sync commands, a third transport. cto.getAttention delegates to the same canonical getCtoAttention() and is registered as an OPTIONAL mobile capability — requiring it would flip every shipped brain to limited mode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe change makes CTO Git mutations lane-explicit, adds separate CTO MCP runtime wiring, and introduces the ChangesCTO workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/desktop/src/main/services/chat/agentChatService.ts (2)
15561-15565: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
closeOrchestrationHttpMcpServerto match the new behavior.The function now closes the orchestration lease and the CTO lease. The current name states only the orchestration lease. A future teardown path can read the name and assume the CTO lease is still open.
♻️ Proposed rename
- /** Closes both leases. Every teardown path must drop the CTO one too. */ - const closeOrchestrationHttpMcpServer = (managed: ManagedChatSession): void => { + /** Closes both leases. Every teardown path must drop the CTO one too. */ + const closeSessionHttpMcpServers = (managed: ManagedChatSession): void => { closeHttpMcpServer(managed, "orchestration"); closeHttpMcpServer(managed, "cto"); };Update every call site if you apply the rename.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 15561 - 15565, Rename closeOrchestrationHttpMcpServer to reflect that it closes both orchestration and CTO leases, and update every reference and call site to use the new name consistently.
8304-8315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
ascast so the deps object stays type-checked.The declared return type is
Parameters<typeof createCtoOperatorTools>[0], so the& { previewSessionToolNames }part of the cast is erased at the call boundary and adds nothing. Theascast also suppresses type errors inside the returned object literal, including thedefaultModelIdanddefaultReasoningEffortfields this change introduced. IfpreviewSessionToolNamesis part of the real dependency contract, declare it in the return type instead of casting.♻️ Proposed refactor
- }): Parameters<typeof createCtoOperatorTools>[0] => { + }): Parameters<typeof createCtoOperatorTools>[0] & { + previewSessionToolNames: typeof previewSessionToolNames; + } => {previewSessionToolNames, - } as Parameters<typeof createCtoOperatorTools>[0] & { - previewSessionToolNames: typeof previewSessionToolNames; - }; + }; };Also applies to: 8354-8357
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 8304 - 8315, Remove the `as` cast from the object returned by `buildCtoOperatorToolDeps` so it is checked against `Parameters<typeof createCtoOperatorTools>[0]`. If `previewSessionToolNames` is required by the actual dependency contract, add it to that contract’s type instead of asserting it, while preserving validation of `defaultModelId` and `defaultReasoningEffort`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts`:
- Around line 1446-1455: Update the input schemas for gitUndoLastHeadChange,
gitRedoLastHeadChange, gitRebaseContinue, gitRebaseAbort, and gitMergeAbort to
require a trimmed laneId string instead of an optional one. Keep
requireMutationLaneId() in each execute path as the runtime guard.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 18896-18930: Update refreshActiveSessionsAndSnapshot so
refreshCtoAttentionIfNeeded() runs before the early return taken when the roster
signature is unchanged, ensuring CTO attention refreshes without roster changes.
Add a regression test covering a CTO attention transition while the roster
signature remains unchanged.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 15561-15565: Rename closeOrchestrationHttpMcpServer to reflect
that it closes both orchestration and CTO leases, and update every reference and
call site to use the new name consistently.
- Around line 8304-8315: Remove the `as` cast from the object returned by
`buildCtoOperatorToolDeps` so it is checked against `Parameters<typeof
createCtoOperatorTools>[0]`. If `previewSessionToolNames` is required by the
actual dependency contract, add it to that contract’s type instead of asserting
it, while preserving validation of `defaultModelId` and
`defaultReasoningEffort`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7df43984-561a-4578-99f3-f93b2fc8663f
⛔ Files ignored due to path filters (8)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/tool-registration.mdis excluded by!docs/**docs/features/chat/agent-routing.mdis excluded by!docs/**docs/features/chat/tool-system.mdis excluded by!docs/**docs/features/cto/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**
📒 Files selected for processing (12)
apps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/shared/syncMobileCompatibility.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/App/ContentView.swiftapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swift
…ease layer Findings from /quality and /test on this lane. The iOS attention probe could not fire for the case it exists for. refreshCtoAttentionIfNeeded() sat below a guard keyed on the roster signature, and the CTO is excluded from that roster by design — so a turn where only the CTO changed left the signature identical and the probe never ran. Once lit by an unrelated change, nothing cleared it either. Hoisted above the guard, and the previously-dead `force` parameter now drives the first probe after a (re)connect, when the host's command descriptors finally arrive. Four operator tools were still dead. steerChat, cancelSteer, listSubagents and approveToolUse were wired to `undefined`, so they were advertised in the prompt manifest and registered on every transport but could only answer "not available" — the exact defect this lane set out to remove, fixed for ~75 tools and left for these. They are now required deps, so a future caller cannot ship them unwired. handoffChat is deleted rather than wired: it targeted "a different agent identity", a subsystem that no longer exists. The headless agentChatService stub was missing three of them, which would have thrown a raw TypeError through `ade actions run chat.*` and the matching sync commands. Same bug class this lane already hit once with getCtoAttention. Structural: the HttpMcpToolSet union spent itself on four string-switch helpers and a duplicated session field. Replaced with a descriptor table and a single keyed lease record, which also collapsed the three transport call sites and the codex namespace fallback. closeOrchestrationHttpMcpServer closed both leases despite its name; it is closeHttpMcpServers now. Also: five mutating git tools still advertised laneId as optional while throwing at runtime, and two error messages named tools that do not exist; createTerminal relied on an invisible pty clamp instead of passing dimensions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Follow-up to #986. Three fixes, all rooted in one fact: the CTO session is pinned to the project's primary lane. It does not have a lane of its own — earlier phrasing that said "the CTO's own lane" was misleading and is corrected throughout the docs.
1. The operator tools actually execute now
createCtoOperatorToolswas only ever reached bypreviewSessionToolNamesand the prompt manifest, so the CTO was told it had a tool surface it could not call.createCtoRuntimeToolMap(gated onidentityKey === "cto") now feeds all five provider transports:ade-ctoSDK MCP server injected intoopts.mcpServersade_ctonamespace inside the singlerefreshCodexDynamicToolsmanaged.ctoHttpMcpServerTwo invariants worth knowing:
allowManagedMcpServersOnlylockdown. The CTO is a daily-driver chat; that lockdown would strip the user's own MCP servers.buildCtoOperatorToolDepsis shared by the preview and runtime paths so the advertised surface and the callable surface cannot drift apart again. Both leases are closed on teardown.2. Mutating git tools no longer default their lane
resolveLaneIddefaulted to the CTO's session lane — the primary lane. SogitCommit/gitPush/gitPullwithlaneIdomitted meant write to the primary worktree, andgitUndoLastHeadChange/gitRedoLastHeadChangemeantgit reset --hardon it.Split into
resolveReadLaneId(7 read-only tools keep the default — inspecting primary is normal supervision) andrequireMutationLaneId(11 mutating tools must name a lane). Enforced in code rather than trusting the model to have read the manifest rule, and the zod schemas now marklaneIdrequired so the generated manifest stops implying a safe default.3. Attention reaches iOS
The dot was desktop-only because the phone talks to the CTO over sync commands — a third transport, separate from IPC and the action domain. New
cto.getAttentiondelegates to the same canonicalgetCtoAttention().Registered in
MOBILE_SYNC_OPTIONAL_REMOTE_COMMAND_ACTIONS, not the required list: requiring it would flip every already-shipped brain intolimitedmode. iOS gates onsupportsRemoteActionand simply never lights the dot against an older brain.Verification
swiftc -parseclean on all three changed Swift filespasses discovered local Claude plugins,resubscribes chat listeners after reconnect) reproduce on a clean stashed tree — both pre-existing and environment-dependentKnown limitation
Registering ~80 CTO tools as always-load MCP tools adds their schemas to context on top of the prompt manifest that documents them, and CTO sessions run with
ENABLE_TOOL_SEARCH=0. Not measured here; worth a look if CTO context pressure shows up.🤖 Generated with Claude Code
Summary by CodeRabbit