-
Notifications
You must be signed in to change notification settings - Fork 3k
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 all commits
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,95 @@ 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 cdPromise = entry.promptQueue.then(async () => { | ||
| if (entry.promptActive) { | ||
| throw new CdWhilePromptActiveError(sessionId); | ||
| } | ||
|
|
||
| const ci = await ensureChannel(); | ||
| const raw = await ci.connection.extMethod( | ||
| SERVE_CONTROL_EXT_METHODS.sessionCd, | ||
| { | ||
| sessionId, | ||
| path: req.path, | ||
| }, | ||
| ); | ||
| const extResult = raw as { | ||
| previousCwd: string; | ||
| newCwd: string; | ||
| warnings: string[]; | ||
| }; | ||
| if ( | ||
| typeof extResult?.previousCwd !== 'string' || | ||
| typeof extResult?.newCwd !== 'string' || | ||
| !Array.isArray(extResult?.warnings) | ||
| ) { | ||
| throw new Error( | ||
| `changeSessionCwd: unexpected response shape from agent: ${JSON.stringify(raw)}`, | ||
| ); | ||
| } | ||
|
|
||
| // State update inside the queue lambda — always executes when | ||
| // the extMethod settles, regardless of caller timeout. | ||
| if (extResult.previousCwd !== extResult.newCwd) { | ||
| entry.events.publish({ | ||
| type: 'session_cwd_changed', | ||
| data: { | ||
| sessionId, | ||
| previousCwd: extResult.previousCwd, | ||
| newCwd: extResult.newCwd, | ||
| }, | ||
| ...(originatorClientId ? { originatorClientId } : {}), | ||
| }); | ||
| } | ||
|
|
||
| return extResult; | ||
| }); | ||
|
|
||
| // Queue tail tied to the raw extMethod settlement — subsequent | ||
| // operations wait for the actual cd to finish, not the timeout. | ||
| entry.promptQueue = cdPromise.then( | ||
| () => undefined, | ||
| () => undefined, | ||
| ); | ||
|
|
||
| // Timeout is caller-facing only: surfaces a deadline exceeded error | ||
| // to the HTTP client without advancing the queue prematurely. | ||
| const result = await withTimeout( | ||
| cdPromise, | ||
| Math.max(initTimeoutMs, 30_000), | ||
| 'changeSessionCwd', | ||
| ); | ||
|
|
||
| writeStderrLine( | ||
| `qwen serve: session ${sessionId} cwd changed: ` + | ||
| `${result.previousCwd} -> ${result.newCwd}` + | ||
| (result.warnings.length > 0 | ||
| ? ` (warnings: ${result.warnings.join('; ')})` | ||
| : ''), | ||
| ); | ||
|
|
||
| return { sessionId, ...result }; | ||
| }, | ||
|
|
||
|
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. [Suggestion] The PR adds 277 lines across 9 files (bridge method, agent handler, HTTP route, error class, config option) without any test coverage. The existing The PR description notes "unit tests for bridge/handler (follow-up PR)". Key paths that should be tested before shipping:
— qwen3.7-max 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] 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,109 @@ 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
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. [Suggestion] The interactive if (input.includes('\0') || baseDir.includes('\0')) {
throw new Error('Path contains null bytes.');
}This ACP handler accepts any absolute string without this check. Null bytes in paths can cause truncation at the C/syscall boundary, potentially bypassing the directory-existence check or trust validation. Add the same null byte guard here for defense in depth.
Suggested change
— qwen3.7-max 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 |
||||||||||||||||
| typeof targetPath !== 'string' || | ||||||||||||||||
| targetPath.length === 0 || | ||||||||||||||||
| !path.isAbsolute(targetPath) || | ||||||||||||||||
| targetPath.includes('\0') | ||||||||||||||||
| ) { | ||||||||||||||||
| 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 and artifact | ||||||||||||||||
| // migration for ACP — storage stays at the bound workspace so | ||||||||||||||||
| // branch/load/lifecycle paths remain consistent). | ||||||||||||||||
| 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, skipArtifactMigration: true }, | ||||||||||||||||
|
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. [Suggestion] With The interactive Consider auditing direct — qwen3.7-max 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] Acknowledged — this is a valid known limitation. With This is by design for now: the ACP |
||||||||||||||||
| ); | ||||||||||||||||
| 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']; | ||||||||||||||||
|
|
||||||||||||||||
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.
[Suggestion]
changeSessionCwdhas zero logging — no entry log, no success log, no warning whenwarnings[]is non-empty. For comparison,sendPromptlogs forward failures and mid-turn queue drops viawriteStderrLine, and wraps the dispatch in atelemetry.withSpan. A directory-change mutation is operator-significant: when this breaks at 3 AM, there will be no trail to determine whether the/cdwas attempted, succeeded, or left the session in a degraded state (memory/model-context refresh failures silently returned inwarnings[]).Consider adding at minimum:
debugLogger.info('changeSessionCwd', { sessionId, path: req.path })at entrydebugLogger.warn('changeSessionCwd warnings', { sessionId, warnings })whenwarnings.length > 0— qwen3.7-max via Qwen Code /review
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.