Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions packages/acp-bridge/src/bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import {
} from './status.js';
import {
BranchWhilePromptActiveError,
CdWhilePromptActiveError,
SessionNotFoundError,
RestoreInProgressError,
InvalidSessionScopeError,
Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -3474,6 +3477,65 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge {
return branchResult;
},

async changeSessionCwd(

Copy link
Copy Markdown
Collaborator Author

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 writeStderrLine logging after successful cwd change: logs session ID, old → new path, and any warnings. This provides the operator trail for debugging ACP /cd mutations at 3 AM.

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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] withTimeout only races the ACP sessionCd request; it does not cancel the underlying ext-method. Because entry.promptQueue is advanced from this timed promise, a 30s timeout releases subsequent prompts while the ACP child may still finish relocating config/storage later, leaving bridge state/events at the old cwd while the agent has moved. Please keep the queue tail tied to the raw ext-method until it settles, or make timeout fatal/reconciled so later prompts cannot run against unknown cwd state.

— GPT-5 via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[qwen] Fixed in 6bfc526.

Restructured changeSessionCwd so that:

  1. promptQueue tail is tied to the raw extMethod settlement (not the timeout-wrapped version)
  2. State update (event broadcast) happens inside the queue lambda — executes when the extMethod actually settles, regardless of whether the caller timed out
  3. withTimeout is purely caller-facing — surfaces a deadline-exceeded error to the HTTP client without advancing the queue

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,
}),
Math.max(initTimeoutMs, 30_000),
'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;

Copy link
Copy Markdown
Collaborator Author

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 response shape validation after the extMethod call — if previousCwd, newCwd, or warnings are missing/wrong type, throws a descriptive error with the raw response stringified for debugging.


// Only update state and broadcast if directory actually changed
if (result.previousCwd !== result.newCwd) {
entry.workspaceCwd = result.newCwd;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[qwen] Fixed in 6bfc526.

Removed entry.workspaceCwd = result.newCwd entirely. The session stays listed under its original daemon-bound workspace for listWorkspaceSessions and lifecycle operations (branchSession etc.). The session's logical cwd lives only inside the agent's Config object — the event bus still broadcasts session_cwd_changed so clients know the current directory.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[qwen] Fixed in 6bfc526.

Same fix as above — entry.workspaceCwd is no longer mutated. Session remains visible in listWorkspaceSessions(boundWorkspace) after /cd, and branchSession continues to use boundWorkspace as the child's cwd (which is correct since storage also stays at boundWorkspace now).

entry.events.publish({
type: 'session_cwd_changed',
data: {
sessionId,
previousCwd: result.previousCwd,
newCwd: result.newCwd,
},
...(originatorClientId ? { originatorClientId } : {}),
});
}

return { sessionId, ...result };
},

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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 changeSessionCwd method now has identical queue chaining semantics to branchSession, so the test patterns for queue-FIFO, prompt-active-guard, and event broadcast from bridge.test.ts apply directly.

async closeSession(sessionId, context, closeOpts) {
return closeSessionImpl(sessionId, context, closeOpts);
},
Expand Down
11 changes: 11 additions & 0 deletions packages/acp-bridge/src/bridgeErrors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -501,3 +501,14 @@ export class BranchWhilePromptActiveError extends Error {
this.sessionId = sessionId;
}
}

export class CdWhilePromptActiveError extends Error {
readonly sessionId: string;
constructor(sessionId: string) {
super(
`Cannot change directory for session ${sessionId}: a prompt is currently active`,
);
this.name = 'CdWhilePromptActiveError';
this.sessionId = sessionId;
}
}
26 changes: 26 additions & 0 deletions packages/acp-bridge/src/bridgeTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,17 @@ export interface BridgeForkAgentResult {
launched: boolean;
}

export interface ChangeSessionCwdRequest {
path: string;
}

export interface ChangeSessionCwdResult {
sessionId: string;
previousCwd: string;
newCwd: string;
warnings: string[];
}

/** Sparse summary used by `GET /workspace/:id/sessions`. */
export interface BridgeSessionSummary {
sessionId: string;
Expand Down Expand Up @@ -301,6 +312,21 @@ export interface AcpSessionBridge {
context?: BridgeClientRequestContext,
): Promise<BridgeBranchedSession>;

/**
* Change the working directory of a live session. The session must be
* idle (no active prompt). Chains onto `entry.promptQueue` and updates
* the tail to prevent concurrent mutations.
*
* Throws `CdWhilePromptActiveError` when a prompt is running,
* `SessionNotFoundError` for unknown ids, and `InvalidClientIdError`
* when the caller's client id is not bound to the session.
*/
changeSessionCwd(
sessionId: string,
req: ChangeSessionCwdRequest,
context?: BridgeClientRequestContext,
): Promise<ChangeSessionCwdResult>;

/**
* Forward a prompt to the agent. Concurrent prompts against the same
* session FIFO-serialize through a per-session queue.
Expand Down
1 change: 1 addition & 0 deletions packages/acp-bridge/src/status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ export const SERVE_CONTROL_EXT_METHODS = {
workspaceMcpRuntimeRemove: 'qwen/control/workspace/mcp/runtime-remove',
workspaceReload: 'qwen/control/workspace/reload',
workspaceExtensionsRefresh: 'qwen/control/workspace/extensions/refresh',
sessionCd: 'qwen/control/session/cd',
} as const;

export type ServeStatus =
Expand Down
106 changes: 105 additions & 1 deletion packages/cli/src/acp-integration/acpAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 (

Copy link
Copy Markdown
Collaborator Author

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 targetPath.includes('\0') to the validation predicate in the ACP sessionCd handler, matching the same null-byte check pattern used by the interactive /cd command.

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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] relocateWorkingDirectory moves the live session artifacts into Storage(canonicalPath), but bridge lifecycle paths still operate against the daemon-bound workspace. For example, sessionBranch sends cwd: boundWorkspace and the agent forks via new SessionService(cwd), so after /cd the source transcript can have been moved away from the storage root that branch/load later inspect. Please either avoid moving transcript storage for ACP logical cwd changes, or route branch/load/lifecycle operations through the session's current storage root.

— GPT-5 via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

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 skipArtifactMigration: true to the ACP /cd call to relocateWorkingDirectory. When this option is set, the method skips prepareSessionArtifactMigration and does NOT change this.storage — artifacts (transcripts, recordings) remain in the original bound-workspace storage location. Only the logical cwd (this.cwd, this.targetDir) and runtime context (workspace roots, memory, file caches) are updated.

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'];
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/serve/acp-session-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export type {

export {
BranchWhilePromptActiveError,
CdWhilePromptActiveError,
SessionNotFoundError,
RestoreInProgressError,
InvalidSessionScopeError,
Expand Down
33 changes: 33 additions & 0 deletions packages/cli/src/serve/routes/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,39 @@ export function registerSessionRoutes(
}
});

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) {
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;
Expand Down
9 changes: 9 additions & 0 deletions packages/cli/src/serve/server/error-response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { writeStderrLine } from '../../utils/stdioHelpers.js';
import {
BranchWhilePromptActiveError,
CancelSentinelCollisionError,
CdWhilePromptActiveError,
InvalidClientIdError,
InvalidPermissionOptionError,
InvalidRewindTargetError,
Expand Down Expand Up @@ -218,6 +219,14 @@ export function sendBridgeError(
});
return;
}
if (err instanceof CdWhilePromptActiveError) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] The ACP child now throws RequestErrors with data.errorKind values like restrictive_sandbox, directory_not_found, and directory_not_trusted, but this route-side mapper does not handle those kinds. They fall through to the generic 500 path instead of the documented 403/400/403 responses, so ACP clients cannot rely on stable policy/error handling for /cd failures. Please add these cases to the data.errorKind block in sendBridgeError and preserve the returned path where present.

— GPT-5 via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

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 restrictive_sandbox, directory_not_found, and directory_not_trusted errorKind cases to sendBridgeErrorImpl (now in the refactored server/error-response.ts). The /cd route's catch block is now a single sendBridgeError(res, err, {...}) call — matching the /branch and /fork pattern. ACP clients now receive stable 403/400/403 responses for these error kinds.

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.
Expand Down
Loading
Loading