refactor(agent-core-v2): decouple workspace from session DI via runtime binding - #2961
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: |
9ef2b73 to
cb198c0
Compare
cb198c0 to
92079c3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb198c0c1b
ℹ️ 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".
| ); | ||
| class AcpSessionRuntime implements Runtime { | ||
| readonly identity; | ||
| readonly capabilities = new Set(['process'] as const); |
There was a problem hiding this comment.
When an ACP session is activated, the main agent is switched to this runtime, but advertising only process causes runtime-aware tool activation to unregister every tool requiring fs (Read, Write, Edit, Grep, Glob, and ReadMediaFile). This happens even when the client advertised fs.readTextFile/fs.writeTextFile and AcpHostFileSystem is available, so ACP users lose the file tools entirely; expose the ACP filesystem through this runtime and include the corresponding capability.
Useful? React with 👍 / 👎.
| fork: (input) => spawn('fork', input), | ||
| createChild: (input) => spawn('createChild', input), | ||
| createChild: (input) => spawn('fork', input), |
There was a problem hiding this comment.
Every session(id).createChild() call now invokes sessionManager.fork, so the resulting session lacks the parent_session_id and session_kind: child metadata that SessionLifecycleService.createChild() adds. Callers therefore receive an ordinary fork that will not participate in child-session discovery or cleanup; retain createChild in the session-manager contract and dispatch this facade method to it.
AGENTS.md reference: packages/klient/AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
| try { | ||
| base = await prepareSystemPromptContext( | ||
| { fs: lease.runtime.fs!, homeDir: env.homeDir }, | ||
| this.sessionContext.cwd, |
There was a problem hiding this comment.
For any runtime whose workspace.mapRoots() translates the host workspace into a container or remote path, this calls the runtime filesystem with the original local sessionContext.cwd and local additional directories. The initial directory listing is consequently missing or read from the wrong location, and line 963 also tells the model the unmapped cwd even though all file tools operate on mapped roots; construct a RuntimeWorkspaceView and use its mapped roots throughout the prompt context.
Useful? React with 👍 / 👎.
| runner: this.processRunner, | ||
| log: this.log, | ||
| }); | ||
| const lease = this.runtimeResolver.acquire({ workspaceId: this.sessionContext.workspaceId, runtimeId: 'local' }, ['process']); |
There was a problem hiding this comment.
When a caller is bound to a non-local runtime, AgentSwarm hardcodes the profile-prefix process to local, and the preceding lifecycle.create() supplies no runtimeId, so every newly spawned swarm agent also defaults to local. Those children can inspect or modify the host workspace instead of the caller's selected environment; resolve the caller agent's current binding and pass the same runtime snapshot to both child creation and prompt-prefix execution.
Useful? React with 👍 / 👎.
| osArch: 'acp', | ||
| osVersion: 'acp', | ||
| shellName: 'bash' as const, | ||
| shellPath: '/bin/bash', |
There was a problem hiding this comment.
On Windows ACP hosts, every Bash call is now sent to the client as /bin/bash while the runtime also reports POSIX path semantics, even though the existing host-environment probe selects the available Windows/Git-Bash shell and Windows paths can be supplied as the session cwd. Such clients cannot launch the requested executable and paths like C:\... are resolved as POSIX-relative; build the ACP runtime environment/path adapter from the detected host environment rather than forcing Linux.
Useful? React with 👍 / 👎.
…eMcp seeds - externalHooksService: inject App-level ISessionManager instead of the unregistered ISessionLifecycleService so SessionStart/SessionEnd hooks actually activate in production; keep sessionId matching and tolerate absent lifecycle events - workspaceMcpService: ignore onWillCreateSession events whose session belongs to another workspace, preventing cross-workspace ISessionMcpHandle seed overrides - update externalHooks integration tests, agent harness, and workspaceMcp tests; add reloadSources coverage in skillCatalog tests
92079c3 to
e846c74
Compare
…spawn, and ACP sessions - map system-prompt cwd, directory listing, and additional dirs through RuntimeWorkspaceView, and skip the listing when the bound runtime has no fs capability - pass the caller agent's runtime binding to AgentSwarm child creation and prompt-prefix execution instead of hardcoding local - expose the ACP client filesystem through the ACP session runtime and build its shell/path environment from the probed host instead of hardcoded Linux - dispatch klient facade createChild to sessionManager.createChild so child sessions keep their parent markers
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adc2d1b350
ℹ️ 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".
| fs: new WorkspaceFsService( | ||
| workspace, | ||
| dirs, | ||
| lease.runtime.fs!, |
There was a problem hiding this comment.
When kap-server and the selected runtime use different path classes—for example, a Linux server driving a Windows runtime—this manually constructed WorkspaceFsService still normalizes mapped.workDir and request paths with its node:path imports. A mapped root such as C:\repo therefore becomes a server-local path such as /workspace/kimi-code/C:\repo, causing every REST read/list/stat operation to target the wrong location. Move this adapter into a runtime-aware filesystem service or consistently use lease.runtime.path instead of instantiating the Workspace-scoped implementation at the route edge.
AGENTS.md reference: packages/kap-server/AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
| return extendWorkspaceWithSkillRoots( | ||
| { | ||
| workspaceDir: this.workspaceCtx.workDir, | ||
| additionalDirs: this.workspaceCtx.additionalDirs, | ||
| workspaceDir: view.workDir, | ||
| additionalDirs: view.additionalDirs, | ||
| }, | ||
| this.skillCatalog?.catalog.getSkillRoots() ?? [], | ||
| this.env.pathClass, | ||
| this.skillCatalog.catalog.getSkillRoots(), | ||
| env.pathClass, |
There was a problem hiding this comment.
When a non-local runtime's mapRoots() rewrites paths, this maps only the session workspace and then appends the catalog's host-side skill roots unchanged. The system prompt exposes those skill paths, so a subsequent Read sends a local absolute path to the remote filesystem or rejects it against the remote path class; Write, Glob, and Grep repeat the same pattern. Include skill roots in the RuntimeWorkspaceView input before mapping, as EditTool already does.
Useful? React with 👍 / 👎.
| private async wireSession(sessionId: string): Promise<AcpSession> { | ||
| const session = this.klient.session(sessionId); | ||
| await this.bindDefaultModel(session.agent('main')); | ||
| await this.bindSessionRuntime?.(sessionId); |
There was a problem hiding this comment.
For every ACP session wired here, bindSessionRuntime registers a session-specific runtime and records it in sessionWorkspaces, but closeSession() only closes the klient session and never invokes the new unbind callback; only permanent deletion does. A long-lived ACP server that opens and closes sessions therefore retains one runtime registration and map entry per closed session until process shutdown. Invoke unbindSessionRuntime from the close path as well so reopening can create a fresh binding without accumulating stale runtimes.
Useful? React with 👍 / 👎.
| name: 'runtime', | ||
| aliases: [], | ||
| description: 'Show or switch execution runtime', |
There was a problem hiding this comment.
This commit adds the user-visible /runtime slash command and public SDK runtime methods, but the reviewed diff contains no .changeset/ entry. Without one, the released CLI/SDK will not receive the required version and changelog update for this new surface; add the changeset prescribed by the repository workflow.
AGENTS.md reference: AGENTS.md:L84-L84
Useful? React with 👍 / 👎.
…h semantics - WorkspaceFsService resolves via the bound runtime's RuntimePath (extended with basename/dirname) instead of node:path, so mapped roots such as C:\\repo stay runtime-local. - Read/Write/Glob/Grep pass skill roots through mapRoots via RuntimeWorkspaceView input, matching Edit. - acp-server unbinds session runtimes on session/close, not only on delete. - apps/kimi-code drops the /runtime slash command; SDK runtime methods stay.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 934f794424
ℹ️ 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".
| subscriptions.add(controller.onDidCloseSession((event) => { | ||
| this.sessions.delete(event.sessionId); | ||
| this.owners.delete(event.sessionId); | ||
| this.didCloseEmitter.fire(event); |
There was a problem hiding this comment.
When a workspace's local runtime generation is replaced after a controller has been materialized, the controller keeps the retired ProgramGeneration lease alive, but closing or archiving its last session here only removes the session maps. The controller remains in controllerEntries until app shutdown, so runtime replacement waits the five-second drain timeout and then force-disposes the generation while its services and listeners remain retained. Dispose an obsolete controller and its subscriptions once it owns no live sessions.
Useful? React with 👍 / 👎.
| track: <T extends RuntimeResource>(resource: T): T => { | ||
| if (!active || generation.draining) throw new RuntimeError('runtime.unavailable', `runtime ${binding.runtimeId} is draining`); | ||
| generation.resources.push(resource); | ||
| return resource; |
There was a problem hiding this comment.
For every short-lived tracked resource, such as each Bash process and completed terminal/download watch, this array retains the resource object for the lifetime of the runtime generation even after the caller disposes it and releases the lease. A long-lived local runtime therefore accumulates every completed process and its streams/listeners without bound, and drains all of them again during shutdown. Tracking needs an unregister path tied to resource disposal rather than retaining every historical resource until generation replacement.
Useful? React with 👍 / 👎.
| private async resolveSession(sessionId: string, runtimeId: string): Promise<SessionWatch | undefined> { | ||
| const key = sessionRuntimeKey(sessionId, runtimeId); | ||
| const existing = this.bySession.get(key); | ||
| if (existing !== undefined) return existing; |
There was a problem hiding this comment.
When a provider replaces a runtime generation under the same runtimeId, the registry drain disposes this watch's tracked handle, but the bridge keeps the SessionWatch cached by only session/runtime ID. Subsequent watch_fs_add calls return that cached entry instead of acquiring the new generation, so the connection silently stops receiving filesystem events after a runtime reconnect or update. Include generation identity in the cache or subscribe to registry changes and rebuild the watch.
Useful? React with 👍 / 👎.
| const lease = this.runtime.acquire(); | ||
| const pathClass = lease.runtime.environment.pathClass; | ||
| lease.dispose(); |
There was a problem hiding this comment.
For a runtime whose mapRoots() rewrites the workspace path, Write/Edit tool accesses contain mapped runtime paths, but this policy only reads the runtime path class and later compares them against the unmapped this.workspace.workDir and additional directories. In manual mode an in-repository remote write consequently fails isWithinWorkspace and falls through to the approval prompt instead of receiving the intended git-cwd approval. Build a RuntimeWorkspaceView and use its mapped roots for containment and git discovery.
Useful? React with 👍 / 👎.
| if (command === undefined) return { dirs: [], selfKnown }; | ||
| const cwdArg = stringArg(args, 'cwd'); | ||
| const base = hostPath(this.sessionContext.cwd, this.env.pathClass); | ||
| const base = hostPath(this.sessionContext.cwd, env.pathClass); |
There was a problem hiding this comment.
When mapRoots() translates the session workspace, Bash actually executes from the mapped runtime workdir, but the AGENTS reminder still derives its base from the host-side sessionContext.cwd and probes that path through the runtime filesystem. Bash calls without an explicit cwd—and relative cwd/target paths—therefore fail to discover applicable AGENTS.md files in remote or container runtimes, so required instructions are not injected after those accesses. Derive the base from RuntimeWorkspaceView.workDir before extracting target directories.
Useful? React with 👍 / 👎.
… runtime resources, and rebuild fs watches on generation replace
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f4ae86ed9
ℹ️ 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".
| const { conns, seq } = sw; | ||
| this.teardownSession(sw); | ||
| return this.createSessionWatch(sw.id, sw.runtimeId, { conns, seq }); |
There was a problem hiding this comment.
When a provider publishes a replacement generation in connecting or otherwise unavailable state, this tears down and deletes the existing SessionWatch before createSessionWatch() tries to acquire the replacement. That acquisition throws and refreshIfStale() logs the failure, but subsequent ready-status events iterate bySession, where this watch no longer exists, so all connected clients silently stop receiving events. Fresh evidence beyond the previous generation-rebuild report is this newly added failure path; preserve the old/retry record until the replacement watch is successfully created.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| const stream = Readable.from(chunks()); | ||
| runtimeFs.lease.track({ dispose: () => { stream.destroy(); } }); |
There was a problem hiding this comment.
For every successful download, the tracked disposable returned here is discarded, while the stream's end/close handler only releases the lease. The registry's new untracking logic runs only when the tracked object's dispose() is invoked, so a long-lived local generation retains each completed stream through generation.resources until shutdown. Fresh evidence beyond the prior registry-level resource report is that this call site never triggers the new unregister path; retain the tracked handle and dispose it from release().
Useful? React with 👍 / 👎.
| let proc: IHostProcess; | ||
| try { | ||
| proc = await this.spawn(effectiveCwd, command); | ||
| proc = lease.track(await this.spawn(lease.runtime.process!, env, effectiveCwd, command)); |
There was a problem hiding this comment.
If the runtime generation begins draining while the asynchronous spawn() is pending, the process can be created successfully and then lease.track() throws because the generation is draining. Since assignment to proc never completes, the catch block only releases the lease and leaves the new command running without task ownership or cancellation. Split spawning from tracking and explicitly dispose the spawned process when tracking fails.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
kimi-code/packages/agent-core-v2/src/app/git/gitService.ts
Lines 167 to 168 in 1dca391
Release the runtime lease when process spawning fails
When an executable cannot be spawned—most commonly the optional gh command invoked by every uncached Git status request—this return bypasses the only lease.dispose() call in the later finally. The generation's lease count can therefore never reach zero, causing runtime replacement or workspace shutdown to wait the full drain timeout and then force-dispose the generation; wrap spawning in the cleanup scope or release the lease before returning.
ℹ️ 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".
| const workspace = this.workspaces.findByRoot(cwd); | ||
| if (workspace === undefined) { | ||
| throw new Error(`workspace for root ${cwd} is not materialized`); |
There was a problem hiding this comment.
When /api/v2/sessions?include=git lists cold sessions after a fresh server start, GitDomainResolver calls IGitService.status(cwd) without materializing their workspaces, so findByRoot returns undefined here and the route degrades every valid repository to { branch: null, pull_request: null }. Git metadata remains unavailable until an unrelated operation resumes the session or otherwise materializes that workspace; resolve the catalog workspace through getOrCreate before acquiring its local runtime.
Useful? React with 👍 / 👎.
| private retireEntryIfIdle(workspaceId: string, entry: SessionControllerEntry): void { | ||
| if (entry.sessionCount !== 0 || !this.controllerEntries.has(entry)) return; |
There was a problem hiding this comment.
When a runtime generation is replaced while the first create, cold resume, or cold fork on this controller is still awaiting initialization, another lifecycle request installs the new controller while sessionCount is still zero and retires this one here. Fresh evidence beyond the prior idle-controller issue is that the count increments only after onDidCreateSession, so the pending operation can finish after its subscriptions were disposed and return a handle that SessionManager never records; later close/archive calls miss it and a second resume can materialize the same session again. Track pending lifecycle operations as references before treating an entry as idle.
Useful? React with 👍 / 👎.
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.