refactor(agent-core-v2): decouple workspace from session DI via runtime binding - #2961
refactor(agent-core-v2): decouple workspace from session DI via runtime binding#2961sailist wants to merge 1 commit into
Conversation
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 980703b65e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| lifecycle.onWillCreateSession((event) => { | ||
| const servers = event.readSeed(ISessionEphemeralMcpServers); | ||
| if (Object.keys(servers).length === 0) return; | ||
| const overlay = this.sessionOverlay(servers, { |
There was a problem hiding this comment.
When two workspaces have been materialized and a session is created with ephemeral MCP servers, every WorkspaceMcpService subscribes to the App-level ISessionManager.onWillCreateSession event and this callback never checks the session's workspaceId. All workspace instances therefore create overlays and repeatedly provide ISessionMcpHandle; the last listener wins, potentially combining the session overlay with another workspace's baseline MCP manager and runtime. Route the event only to the owning Program/controller, or at minimum reject events whose ISessionContext.workspaceId differs.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
| if (!('type' in server)) { | ||
| out[server.name] = { | ||
| transport: 'stdio', | ||
| command: server.command, | ||
| args: server.args, | ||
| env: namedPairsToRecord(server.env), | ||
| }; | ||
| continue; | ||
| throw new Error(`ACP stdio MCP server ${server.name} does not declare a runtime identity`); | ||
| } |
There was a problem hiding this comment.
When an ACP client supplies a standard stdio MCP server in session/new, load, resume, or fork, the server has no type discriminator or runtime field, so this unconditional throw rejects the entire request. Stdio is the ACP baseline and this server still describes it as supported during initialization; previously these entries were converted successfully. Assign the appropriate local/session runtime internally instead of requiring a wire field the ACP server shape cannot carry.
Useful? React with 👍 / 👎.
| const runtimeId = acpRuntimeProvider.bindSession(context.workspaceId, sessionId, context.cwd); | ||
| sessionWorkspaces.set(sessionId, context.workspaceId); | ||
| const agent = await ensureMainAgent(handle, { runtimeId }); | ||
| agent.accessor.get(IAgentRuntimeBindingService).switch(runtimeId); |
There was a problem hiding this comment.
For ACP clients that do not advertise the optional terminal capability, this callback still binds and switches every main agent to the process-only ACP runtime. That runtime advertises process, so Bash remains available, but AcpProcessService.spawn rejects every invocation with ACP terminal capability is unavailable; before this change such calls fell back to the local process service. Only switch to the ACP runtime when terminal support is enabled, or retain the local fallback.
Useful? React with 👍 / 👎.
| const relative = sw.runtime.path.relative(sw.view.workDir, event.path); | ||
| const path = relative === '' ? '.' : relative.split(sw.runtime.path.separator).join('/'); | ||
| if (!isUnderAny(path, sw.union)) return; | ||
| sw.pending.push({ path, change: event.action, kind: event.kind }); |
There was a problem hiding this comment.
When a client watches . in a repository, this direct runtime event path forwards every .git and gitignored change because the only filter is the requested-path union. The replaced WorkspaceFsWatchService explicitly excluded .git/ and loaded the workspace .gitignore; losing that filtering can turn ordinary Git operations or generated-file churn into noisy or truncated event.fs.changed batches. Apply equivalent ignore matching before adding the event to pending.
Useful? React with 👍 / 👎.
980703b to
9ef2b73
Compare
commit: |
1 participant
Related Issue
No linked issue — the problem is explained below.
Problem
Workspace used to be a DI scope between App and Session: session lifecycle was owned by a Workspace-scoped handler, and every execution resource (fs, process, watch, terminal) was implicitly resolved against the local machine through App-level host services. One workspace could not carry multiple execution environments, and business ownership kept leaking into the DI topology.
What changed
LifecycleScope.Workspace; the scope chain is now App → Session → Agent. An App-level SessionManager owns session create/resume/fork/close, and workspaces are managed as plain business objects (metadata, trust, dirs) instead of scopes.runtime_idthat defaults tolocal, so existing clients keep working. NewGET/POST /api/v1/sessions/{id}/runtimeendpoints expose the binding. The debug surface moves from workspace-scope reflection calls to read-only business snapshots./runtimecommand; klient and acp-server are wired to the same binding.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.