Repository navigation
feat(acp): support /cd command in ACP sessions #5903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
261b6bd
f56f14b
5e83ea7
0300b35
6bfc526
4fa0456
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ import { | |
| } from './status.js'; | ||
| import { | ||
| BranchWhilePromptActiveError, | ||
| CdWhilePromptActiveError, | ||
| SessionNotFoundError, | ||
| RestoreInProgressError, | ||
| InvalidSessionScopeError, | ||
|
|
@@ -81,6 +82,8 @@ import type { | |
| AcpSessionBridge, | ||
| MidTurnQueueEntry, | ||
| BridgeDaemonStatusSnapshot, | ||
| ChangeSessionCwdRequest, | ||
| ChangeSessionCwdResult, | ||
| } from './bridgeTypes.js'; | ||
| import type { BridgeOptions, BridgeTelemetry } from './bridgeOptions.js'; | ||
| import { MCP_RESTART_SERVER_DEADLINE_MS } from './mcpTimeouts.js'; | ||
|
|
@@ -3474,6 +3477,65 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { | |
| return branchResult; | ||
| }, | ||
|
|
||
| async changeSessionCwd( | ||
| sessionId: string, | ||
| req: ChangeSessionCwdRequest, | ||
| context?: BridgeClientRequestContext, | ||
| ): Promise<ChangeSessionCwdResult> { | ||
| if (shuttingDown) throw new Error('AcpSessionBridge is shutting down'); | ||
|
|
||
| const entry = byId.get(sessionId); | ||
| if (!entry) throw new SessionNotFoundError(sessionId); | ||
|
|
||
| const originatorClientId = resolveTrustedClientId( | ||
| entry, | ||
| context?.clientId, | ||
| ); | ||
|
|
||
| // Chain onto promptQueue and update tail — ensures: | ||
| // 1. cd waits for any in-flight prompt to complete | ||
| // 2. Subsequent prompts wait for cd to complete (prevents stale config.cwd) | ||
| const cdResult = entry.promptQueue.then(async () => { | ||
| if (entry.promptActive) { | ||
| throw new CdWhilePromptActiveError(sessionId); | ||
| } | ||
|
|
||
| const ci = await ensureChannel(); | ||
| return (await withTimeout( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] — GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [qwen] Fixed in 6bfc526. Restructured
This ensures subsequent prompts cannot run against unknown cwd state: even if the caller times out, the queue stays blocked until the real cd finishes (or fails). |
||
| ci.connection.extMethod(SERVE_CONTROL_EXT_METHODS.sessionCd, { | ||
| sessionId, | ||
| path: req.path, | ||
| }), | ||
| initTimeoutMs, | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [qwen] Fixed in f56f14b. Changed to |
||
| 'changeSessionCwd', | ||
| )) as { previousCwd: string; newCwd: string; warnings: string[] }; | ||
| }); | ||
|
|
||
| // Update queue tail (same pattern as sendPrompt L3090) | ||
| entry.promptQueue = cdResult.then( | ||
| () => undefined, | ||
| () => undefined, | ||
| ); | ||
|
|
||
| const result = await cdResult; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [qwen] Fixed in 6bfc526. Added response shape validation after the |
||
|
|
||
| // Only update state and broadcast if directory actually changed | ||
| if (result.previousCwd !== result.newCwd) { | ||
| entry.workspaceCwd = result.newCwd; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [qwen] Fixed in 6bfc526. Removed
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [qwen] Fixed in 6bfc526. Same fix as above — |
||
| entry.events.publish({ | ||
| type: 'session_cwd_changed', | ||
| data: { | ||
| sessionId, | ||
| previousCwd: result.previousCwd, | ||
| newCwd: result.newCwd, | ||
| }, | ||
| ...(originatorClientId ? { originatorClientId } : {}), | ||
| }); | ||
| } | ||
|
|
||
| return { sessionId, ...result }; | ||
| }, | ||
|
|
||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [qwen] Acknowledged — test coverage will be added in a follow-up commit. The |
||
| async closeSession(sessionId, context, closeOpts) { | ||
| return closeSessionImpl(sessionId, context, closeOpts); | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -172,7 +172,11 @@ import { | |
| getCurrentLanguage, | ||
| SUPPORTED_LANGUAGES, | ||
| } from '../i18n/index.js'; | ||
| import { isWorkspaceTrusted } from '../config/trustedFolders.js'; | ||
| import { | ||
| isWorkspaceTrusted, | ||
| isFolderTrustEnabled, | ||
| loadTrustedFolders, | ||
| } from '../config/trustedFolders.js'; | ||
| import { | ||
| ACP_PREFLIGHT_KINDS, | ||
| STATUS_SCHEMA_VERSION, | ||
|
|
@@ -5511,6 +5515,106 @@ class QwenAgent implements Agent { | |
| await this.closeStoredSession(sessionId); | ||
| return { sessionId, closed: true }; | ||
| } | ||
| case SERVE_CONTROL_EXT_METHODS.sessionCd: { | ||
| const sessionId = params['sessionId']; | ||
| const targetPath = params['path']; | ||
| if (typeof sessionId !== 'string' || sessionId.length === 0) { | ||
| throw RequestError.invalidParams( | ||
| undefined, | ||
| 'Invalid or missing sessionId', | ||
| ); | ||
| } | ||
| if ( | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [qwen] Fixed in 6bfc526. Added |
||
| typeof targetPath !== 'string' || | ||
| targetPath.length === 0 || | ||
| !path.isAbsolute(targetPath) | ||
| ) { | ||
| throw RequestError.invalidParams( | ||
| undefined, | ||
| 'Invalid or missing path (must be an absolute path)', | ||
| ); | ||
| } | ||
|
|
||
| const session = this.sessionOrThrow(sessionId); | ||
| const config = session.getConfig(); | ||
|
|
||
| // Restrictive sandbox check | ||
| if (config.isRestrictiveSandbox()) { | ||
| throw new RequestError(-32003, 'Restrictive sandbox mode active', { | ||
| errorKind: 'restrictive_sandbox', | ||
| }); | ||
| } | ||
|
|
||
| // Verify directory exists | ||
| let stats; | ||
| try { | ||
| stats = await fs.stat(targetPath); | ||
| } catch { | ||
| throw new RequestError(-32002, `Directory not found: ${targetPath}`, { | ||
| errorKind: 'directory_not_found', | ||
| path: targetPath, | ||
| }); | ||
| } | ||
| if (!stats.isDirectory()) { | ||
| throw new RequestError(-32002, `Not a directory: ${targetPath}`, { | ||
| errorKind: 'directory_not_found', | ||
| path: targetPath, | ||
| }); | ||
| } | ||
|
|
||
| // Canonicalize path | ||
| const canonicalPath = await fs.realpath(targetPath); | ||
|
|
||
| // Noop check | ||
| const previousCwd = config.getTargetDir(); | ||
| if (canonicalPath === previousCwd) { | ||
| return { previousCwd, newCwd: canonicalPath, warnings: [] }; | ||
| } | ||
|
|
||
| // Trust check | ||
| if (isFolderTrustEnabled(this.settings.merged)) { | ||
| const trustedFolders = loadTrustedFolders(); | ||
| if (trustedFolders.isPathTrusted(canonicalPath) !== true) { | ||
| throw new RequestError( | ||
| -32001, | ||
| `Directory not trusted: ${canonicalPath}`, | ||
| { errorKind: 'directory_not_trusted', path: canonicalPath }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Relocate working directory (skip process.chdir for ACP) | ||
| const warnings: string[] = []; | ||
| const relocation = await config.relocateWorkingDirectory( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] — GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [qwen] Fixed in 6bfc526. Added This means branch/load/lifecycle operations continue to find artifacts at the daemon-bound workspace's storage root, eliminating the state divergence. |
||
| canonicalPath, | ||
| canonicalPath, | ||
| { skipProcessChdir: true }, | ||
| ); | ||
| if (relocation.memoryRefreshError) { | ||
| warnings.push( | ||
| `Memory refresh failed: ${ | ||
| relocation.memoryRefreshError instanceof Error | ||
| ? relocation.memoryRefreshError.message | ||
| : String(relocation.memoryRefreshError) | ||
| }`, | ||
| ); | ||
| } | ||
|
|
||
| // Update model context | ||
| try { | ||
| await config | ||
| .getGeminiClient() | ||
| ?.addWorkingDirectoryChangedContext(previousCwd, canonicalPath); | ||
| } catch (error) { | ||
| warnings.push( | ||
| `Model context refresh failed: ${ | ||
| error instanceof Error ? error.message : String(error) | ||
| }`, | ||
| ); | ||
| } | ||
|
|
||
| return { previousCwd, newCwd: canonicalPath, warnings }; | ||
| } | ||
| case SERVE_CONTROL_EXT_METHODS.sessionApprovalMode: { | ||
| const sessionId = params['sessionId']; | ||
| const mode = params['mode']; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,6 +80,7 @@ import { | |
| canonicalizeWorkspace, | ||
| CancelSentinelCollisionError, | ||
| BranchWhilePromptActiveError, | ||
| CdWhilePromptActiveError, | ||
| createAcpSessionBridge, | ||
| InvalidClientIdError, | ||
| InvalidPermissionOptionError, | ||
|
|
@@ -3324,6 +3325,74 @@ export function createServeApp( | |
| } | ||
| }); | ||
|
|
||
| app.post('/session/:id/cd', mutate(), async (req, res) => { | ||
| const sessionId = requireSessionId(req, res); | ||
| if (sessionId === null) return; | ||
| const body = safeBody(req); | ||
| const targetPath = body['path']; | ||
| if ( | ||
| typeof targetPath !== 'string' || | ||
| targetPath.length === 0 || | ||
| !path.isAbsolute(targetPath) | ||
| ) { | ||
| res.status(400).json({ | ||
| error: '`path` is required and must be an absolute path', | ||
| code: 'invalid_path', | ||
| }); | ||
| return; | ||
| } | ||
| const clientId = parseClientIdHeader(req, res); | ||
| if (clientId === null) return; | ||
| try { | ||
| const result = await bridge.changeSessionCwd( | ||
| sessionId, | ||
| { path: targetPath }, | ||
| clientId !== undefined ? { clientId } : undefined, | ||
| ); | ||
| res.status(200).json(result); | ||
| } catch (err) { | ||
| // Map ACP child errorKind codes to HTTP responses | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [qwen] Fixed in f56f14b. Moved all three errorKind cases ( |
||
| if (err && typeof err === 'object') { | ||
| const data = (err as { data?: unknown }).data; | ||
| if (data && typeof data === 'object') { | ||
| const kind = (data as { errorKind?: unknown }).errorKind; | ||
| if (kind === 'restrictive_sandbox') { | ||
| res.status(403).json({ | ||
| error: | ||
| (err as { message?: string }).message ?? | ||
| 'Restrictive sandbox mode active', | ||
| code: 'restrictive_sandbox', | ||
| }); | ||
| return; | ||
| } | ||
| if (kind === 'directory_not_found') { | ||
| res.status(400).json({ | ||
| error: | ||
| (err as { message?: string }).message ?? 'Directory not found', | ||
| code: 'directory_not_found', | ||
| path: (data as { path?: string }).path, | ||
| }); | ||
| return; | ||
| } | ||
| if (kind === 'directory_not_trusted') { | ||
| res.status(403).json({ | ||
| error: | ||
| (err as { message?: string }).message ?? | ||
| 'Directory not trusted', | ||
| code: 'directory_not_trusted', | ||
| path: (data as { path?: string }).path, | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| sendBridgeError(res, err, { | ||
| route: 'POST /session/:id/cd', | ||
| sessionId, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| app.get('/session/:id/status', (req, res) => { | ||
| const sessionId = requireSessionId(req, res); | ||
| if (sessionId === null) return; | ||
|
|
@@ -5766,6 +5835,14 @@ function sendBridgeErrorImpl( | |
| }); | ||
| return; | ||
| } | ||
| if (err instanceof CdWhilePromptActiveError) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [qwen] Fixed in 6bfc526. Renamed to |
||
| res.status(409).json({ | ||
| error: err.message, | ||
| code: 'session_busy', | ||
| sessionId: err.sessionId, | ||
| }); | ||
| return; | ||
| } | ||
| if (err instanceof TrustGateError) { | ||
| // Trust-folder rejection. 403 because the workspace's trust posture | ||
| // forbids the privileged mode. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[qwen] Fixed in 6bfc526.
Added
writeStderrLinelogging after successful cwd change: logs session ID, old → new path, and any warnings. This provides the operator trail for debugging ACP/cdmutations at 3 AM.