From 4a372bb3901ad638e215164b61e566e4c88e2a29 Mon Sep 17 00:00:00 2001 From: tomsen-ai <230283659+tomsen-ai@users.noreply.github.com> Date: Fri, 4 Sep 2026 11:14:18 +0800 Subject: [PATCH 1/5] refactor(cli): route every per-request runtime-root pin through runWithPinnedRuntimeBaseDir Follow-up to #10095 (review item D2-1 + maintainer nit). QwenAgent already had a private runWithPinnedRuntimeBaseDir helper, but five handlers composed the loadSettingsCached -> runWithAcpRuntimeOutputDir routing by hand instead: unstable_listSessions, deleteSession, renameSession (the three #10095 fixed) plus the older qwen/status/session/transcript and sessionTurnStatus readers. Hand-composed routing is exactly the shape that picked up the stale this.settings cache in #10095, so make the helper the single choke point and document why it exists. Pure delegation swap: the helper's body is runWithAcpRuntimeOutputDir(settings, cwd, operation), so no runtime behavior changes. The two wide sites name their inline callbacks (readTranscriptPage / readSettledTurnResult) so the reroute does not re-indent 60-line bodies. Because the two spellings reach the same function, no behavioral test can distinguish them; a source-level test pins the invariant instead (the only remaining runWithAcpRuntimeOutputDir call is the helper's own delegation), following the cli.test.ts source-pin precedent. Mutation check, one site at a time: re-adding a direct call is killed only by the new pin; routing this.settings through the helper at deleteSession is killed only by the #10095 deleteSession test; making the helper itself use this.settings is killed by exactly the three #10095 tests. Also rename the describe block that holds those three tests -- it was named "extMethod renameSession routing" but covers rename / delete / list / branch / close, so a listSessions failure reported under "renameSession routing" (wenshao's nit on #10095). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TGftREDUQDei396TrMLQHA --- .../cli/src/acp-integration/acpAgent.test.ts | 22 ++++++++++++- packages/cli/src/acp-integration/acpAgent.ts | 33 +++++++++++++++---- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 67c81b85f06..1a225fc005a 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -14,6 +14,7 @@ import { afterAll, type MockInstance, } from 'vitest'; +import { readFileSync } from 'node:fs'; import type { Stats } from 'node:fs'; import * as fs from 'node:fs/promises'; import * as os from 'node:os'; @@ -20203,7 +20204,7 @@ describe('QwenAgent sessionIdContext binding', () => { }); }); -describe('QwenAgent extMethod renameSession routing', () => { +describe('QwenAgent session-management routing (rename / delete / list / branch / close)', () => { type AgentSideConnectionLike = { closed: Promise }; type AgentLike = { initialize: (args: Record) => Promise; @@ -29916,3 +29917,22 @@ describe('createManagedExternalToolGuard', () => { expect(extMethod).toHaveBeenCalledTimes(1); }); }); + +describe('QwenAgent runtime-root pinning choke point', () => { + it('routes every per-request runtime-root pin through runWithPinnedRuntimeBaseDir', () => { + // #10095 fixed handlers that composed `loadSettingsCached` → + // `runWithAcpRuntimeOutputDir` by hand and picked up the stale + // `this.settings` cache. The private helper is the one place that + // decision is made, so a handler calling `runWithAcpRuntimeOutputDir` + // directly is exactly the shape that regressed. No behavioral test can + // see the difference (both spellings reach the same function), so pin + // the source: the only call left is the helper's own delegation. + const source = readFileSync( + new URL('./acpAgent.ts', import.meta.url), + 'utf8', + ); + const directCalls = source.match(/\brunWithAcpRuntimeOutputDir\(/g) ?? []; + expect(directCalls).toHaveLength(1); + expect(source).toContain('private runWithPinnedRuntimeBaseDir('); + }); +}); diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index e948d1a1786..9cc434fe087 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -4555,6 +4555,15 @@ class QwenAgent implements Agent { } } + /** + * Single choke point for pinning the runtime root of a per-request + * operation to the settings loaded for THAT request's cwd. Every handler + * that resolves session storage for a caller-supplied cwd goes through + * here rather than calling `runWithAcpRuntimeOutputDir` directly, so the + * "which settings pin this operation" decision lives in one place — the + * bug class fixed in #10095 was handlers composing the routing by hand + * with the stale `this.settings` cache. + */ private runWithPinnedRuntimeBaseDir( settings: LoadedSettings, cwd: string, @@ -5774,7 +5783,7 @@ class QwenAgent implements Agent { // advanced.runtimeOutputDir and this listing would scan the wrong runtime // root (returning an empty/foreign list for this cwd). const settings = loadSettingsCached(cwd); - const result = await runWithAcpRuntimeOutputDir(settings, cwd, () => { + const result = await this.runWithPinnedRuntimeBaseDir(settings, cwd, () => { const sessionService = new SessionService(cwd); return sessionService.listSessions({ cursor: numericCursor, @@ -8861,7 +8870,7 @@ class QwenAgent implements Agent { try { const settings = loadSettingsCached(cwd); - return await runWithAcpRuntimeOutputDir(settings, cwd, async () => { + const readTranscriptPage = async () => { if (rawDirection === 'backward') { await this.sessions .get(sessionId) @@ -8913,7 +8922,12 @@ class QwenAgent implements Agent { ? { partial: true, replayError: replay.replayError } : {}), } as Record; - }); + }; + return await this.runWithPinnedRuntimeBaseDir( + settings, + cwd, + readTranscriptPage, + ); } catch (error) { if ( error instanceof InvalidSessionTranscriptCursorError || @@ -11786,7 +11800,7 @@ class QwenAgent implements Agent { } const session = this.sessionOrThrow(sessionId); const settings = loadSettingsCached(cwd); - return await runWithAcpRuntimeOutputDir(settings, cwd, async () => { + const readSettledTurnResult = async () => { try { await session.getConfig().getChatRecordingService()?.flush(); } catch { @@ -11859,7 +11873,12 @@ class QwenAgent implements Agent { } throw error; } - }); + }; + return await this.runWithPinnedRuntimeBaseDir( + settings, + cwd, + readSettledTurnResult, + ); } case SERVE_CONTROL_EXT_METHODS.sessionContinue: { const sessionId = params['sessionId']; @@ -12077,7 +12096,7 @@ class QwenAgent implements Agent { // destructive lookup at the wrong runtime root — silently returning // success:false for a session that exists, or deleting a stale // same-id copy under the wrong root. - const success = await runWithAcpRuntimeOutputDir( + const success = await this.runWithPinnedRuntimeBaseDir( loadSettingsCached(cwd), cwd, async () => { @@ -12128,7 +12147,7 @@ class QwenAgent implements Agent { return { success: ok }; } // Per-request settings for the same reason as deleteSession above. - const success = await runWithAcpRuntimeOutputDir( + const success = await this.runWithPinnedRuntimeBaseDir( loadSettingsCached(cwd), cwd, async () => { From 51b54ca72163fa91c907e27715bd28e9a59e215b Mon Sep 17 00:00:00 2001 From: tomsen-ai <230283659+tomsen-ai@users.noreply.github.com> Date: Fri, 4 Sep 2026 11:49:02 +0800 Subject: [PATCH 2/5] test(cli): read the runtime-root pin's source the house way and locate its failures (round-1) Triage round 1 on #10988 asked for two things on the new source pin: - Read the source with the spelling every other source-text assertion in packages/cli uses (readFileSync('src/...', 'utf8') against vitest's package-root cwd) instead of new URL(..., import.meta.url), which cli.test.ts warns Vite may rewrite to a non-file URL under vitest. - Make the failure self-locating. The pin now scans line by line, skips comment lines, and puts ": " for every direct call into the assertion message, so re-adding a direct call reports e.g. "12099: const success = await runWithAcpRuntimeOutputDir(" next to the helper's own delegation line. A doc comment that spells the call with a paren no longer trips it. The surviving call is asserted to be the helper's delegation body, which subsumes the previous "helper still exists" check. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TGftREDUQDei396TrMLQHA --- .../cli/src/acp-integration/acpAgent.test.ts | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 1a225fc005a..3d018ea93fe 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -29927,12 +29927,25 @@ describe('QwenAgent runtime-root pinning choke point', () => { // directly is exactly the shape that regressed. No behavioral test can // see the difference (both spellings reach the same function), so pin // the source: the only call left is the helper's own delegation. - const source = readFileSync( - new URL('./acpAgent.ts', import.meta.url), - 'utf8', - ); - const directCalls = source.match(/\brunWithAcpRuntimeOutputDir\(/g) ?? []; - expect(directCalls).toHaveLength(1); - expect(source).toContain('private runWithPinnedRuntimeBaseDir('); + const source = readFileSync('src/acp-integration/acpAgent.ts', 'utf8'); + const directCalls = source + .split('\n') + .map((line, index) => ({ line: index + 1, text: line.trim() })) + .filter( + ({ text }) => + /\brunWithAcpRuntimeOutputDir\(/.test(text) && + !text.startsWith('//') && + !text.startsWith('*') && + !text.startsWith('/*'), + ); + const located = directCalls.map(({ line, text }) => `${line}: ${text}`); + expect( + located, + `acpAgent.ts must not call runWithAcpRuntimeOutputDir directly; route the operation through this.runWithPinnedRuntimeBaseDir (see #10095). Direct calls at:\n${located.join('\n')}`, + ).toEqual([ + expect.stringMatching( + /^\d+: return runWithAcpRuntimeOutputDir\(settings, cwd, operation\);$/, + ), + ]); }); }); From 81df6e5b64b893b0526328a3d95803c8eed23d20 Mon Sep 17 00:00:00 2001 From: tomsen-ai <230283659+tomsen-ai@users.noreply.github.com> Date: Fri, 4 Sep 2026 13:29:24 +0800 Subject: [PATCH 3/5] refactor(cli): resolve per-request settings inside the runtime-root pin (round-1 R1-1/R1-2/R1-3) review-pr round 1 on #10988 showed the shared helper still let a handler hand it `this.settings`, so the doc comment's "decision lives in one place" was not true and the source pin only checked call shape. - R1-1: add runWithPinnedRuntimeBaseDirForRequest(cwd, operation): it loads the settings for the request's cwd itself and hands them to the operation, so a per-request handler has no settings parameter to get wrong. All five per-request sites (unstable_listSessions, deleteSession, renameSession, sessionTranscript, sessionTurnStatus) now use it; the three-argument helper stays for callers that hold deliberately scoped settings (workspace MCP discovery, live-session scope checks, session creation). Both doc comments now claim only what each helper guarantees. - R1-2: add behavioral pins for the two handlers that had none: sessionTurnStatus and qwen/status/session/transcript each assert the settings/cwd reaching the mocked context function are the request's (the transcript test also checks the replay config was seeded from the request's settings, distinguished by outputLanguage). - R1-3: the source pin matches identifier occurrences, not only same-line calls, and skips import lines, so an alias (`const pin = runWithAcpRuntimeOutputDir;`) is flagged with its line. Mutation matrix, one mutant at a time, whole file each run (604 tests): deleteSession -> this.settings via 3-arg helper: 1 failed (its pin) sessionTranscript -> this.settings: 3 failed (new pin + 2 replay-config tests) sessionTurnStatus -> this.settings: 1 failed (new pin) alias probe in the helper: 1 failed (source pin, names the alias line) ForRequest helper itself uses this.settings: 7 failed (all five handlers' pins + 2) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TGftREDUQDei396TrMLQHA --- .../cli/src/acp-integration/acpAgent.test.ts | 143 +++++++++++++++++- packages/cli/src/acp-integration/acpAgent.ts | 55 ++++--- 2 files changed, 170 insertions(+), 28 deletions(-) diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 3d018ea93fe..ed9f385ec7b 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -15071,6 +15071,68 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agentPromise; }); + it('resolves sessionTurnStatus settings per request, not from the this.settings cache', async () => { + const sessionId = '11111111-1111-1111-1111-111111111111'; + await setupSessionMocks(sessionId); + const readPage = vi.fn().mockResolvedValue({ + sessionId, + records: [], + hasMore: false, + gaps: [], + startTime: 'start', + lastUpdated: 'end', + }); + vi.mocked(SessionTranscriptReader).mockImplementation( + () => + ({ + readPage, + }) as unknown as InstanceType, + ); + + const agentPromise = runAcpAgent( + mockConfig, + makeSessionSettings(), + mockArgv, + ); + await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined()); + const agent = capturedAgentFactory!({ + get closed() { + return mockConnectionState.promise; + }, + }) as AgentLike; + await agent.newSession({ cwd: '/tmp', mcpServers: [] }); + + // Multi-workspace daemon shape: the live session and `this.settings` + // belong to the boot workspace; the status request names another cwd + // whose own settings (and therefore advanced.runtimeOutputDir) must pin + // the transcript read. Routing through the stale cache would scan the + // wrong runtime root. + const perRequestSettings = makeSessionSettings(); + vi.mocked(loadSettings).mockClear(); + vi.mocked(loadSettings).mockReturnValue(perRequestSettings); + vi.mocked(runWithAcpRuntimeOutputDir).mockClear(); + + await expect( + agent.extMethod(SERVE_CONTROL_EXT_METHODS.sessionTurnStatus, { + cwd: '/tmp/workspace-a', + sessionId, + promptId: 'prompt-1', + }), + ).resolves.toEqual({ v: 1, sessionId, turnResult: null }); + + expect(loadSettings).toHaveBeenCalledWith('/tmp/workspace-a'); + expect(runWithAcpRuntimeOutputDir).toHaveBeenCalledTimes(1); + expect(vi.mocked(runWithAcpRuntimeOutputDir).mock.calls[0]![0]).toBe( + perRequestSettings, + ); + expect(vi.mocked(runWithAcpRuntimeOutputDir).mock.calls[0]![1]).toBe( + '/tmp/workspace-a', + ); + + mockConnectionState.resolve(); + await agentPromise; + }); + it('still scans the transcript when the pre-read flush fails', async () => { const sessionId = '11111111-1111-1111-1111-111111111111'; const innerConfig = await setupSessionMocks(sessionId); @@ -16642,6 +16704,66 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agentPromise; }); + it('resolves qwen/status/session/transcript settings per request, not from the this.settings cache', async () => { + const settings = makeCoreSettings(); + mockRunExitCleanup.mockResolvedValue(undefined); + const transcriptConfig = { + ...makeInnerConfig(), + enableFileCheckpointing: vi.fn(), + }; + vi.mocked(loadCliConfig).mockResolvedValue( + transcriptConfig as unknown as Config, + ); + const readPage = vi.fn().mockResolvedValue({ + sessionId: VALID_SESSION_ID, + records: [], + hasMore: false, + gaps: [], + startTime: 'start', + lastUpdated: 'end', + }); + vi.mocked(SessionTranscriptReader).mockImplementation( + () => + ({ + readPage, + }) as unknown as InstanceType, + ); + mockHistoryReplayPage.mockResolvedValue({ pendingToolCalls: [] }); + const { agent, agentPromise } = await bootCoreSettingsAgent(settings); + + // Multi-workspace daemon shape: `this.settings` holds the boot + // workspace's settings; the transcript request names another cwd whose + // own settings must pin the read (and seed the replay config). + // A different outputLanguage makes the request's settings distinguishable + // from the boot settings by content, not just by identity. + const perRequestSettings = makeCoreSettings('French'); + vi.mocked(loadSettings).mockClear(); + vi.mocked(loadSettings).mockReturnValue(perRequestSettings); + vi.mocked(runWithAcpRuntimeOutputDir).mockClear(); + + await agent.extMethod(SERVE_STATUS_EXT_METHODS.sessionTranscript, { + cwd: '/tmp/workspace-a', + sessionId: VALID_SESSION_ID, + }); + + expect(loadSettings).toHaveBeenCalledWith('/tmp/workspace-a'); + // The outer pin is the handler's; the replay-config build inside it may + // pin again with the same settings. Every pin must carry the request's. + const pins = vi.mocked(runWithAcpRuntimeOutputDir).mock.calls; + expect(pins.length).toBeGreaterThanOrEqual(1); + expect(pins[0]![0]).toBe(perRequestSettings); + expect(pins[0]![1]).toBe('/tmp/workspace-a'); + expect(pins.every(([s]) => s === perRequestSettings)).toBe(true); + // The replay config built inside the pinned scope was seeded from the + // request's settings (the operation receives them), not this.settings. + expect(vi.mocked(loadCliConfig).mock.calls.at(-1)?.[0]).toMatchObject({ + general: { outputLanguage: 'French' }, + }); + + mockConnectionState.resolve(); + await agentPromise; + }); + it('flushes the live recording before reading the latest persisted page', async () => { const innerConfig = await setupSessionMocks(VALID_SESSION_ID); const recording = innerConfig.getChatRecordingService(); @@ -29920,20 +30042,25 @@ describe('createManagedExternalToolGuard', () => { describe('QwenAgent runtime-root pinning choke point', () => { it('routes every per-request runtime-root pin through runWithPinnedRuntimeBaseDir', () => { - // #10095 fixed handlers that composed `loadSettingsCached` → - // `runWithAcpRuntimeOutputDir` by hand and picked up the stale - // `this.settings` cache. The private helper is the one place that - // decision is made, so a handler calling `runWithAcpRuntimeOutputDir` - // directly is exactly the shape that regressed. No behavioral test can - // see the difference (both spellings reach the same function), so pin - // the source: the only call left is the helper's own delegation. + // #10095 fixed handlers that composed the runtime-root routing by hand + // and picked up the stale `this.settings` cache. Per-request handlers now + // go through `runWithPinnedRuntimeBaseDirForRequest`, which resolves the + // settings from the request cwd itself, and everything else through the + // shared helper. A handler naming `runWithAcpRuntimeOutputDir` directly — + // as a call or via an alias — is exactly the shape that regressed, and + // no behavioral test can see the difference (both spellings reach the + // same function), so pin the source: outside imports and comments the + // only mention left is the shared helper's own delegation. The per-request + // handlers themselves are pinned behaviorally (settings/cwd reaching the + // mock) by the routing tests above. const source = readFileSync('src/acp-integration/acpAgent.ts', 'utf8'); const directCalls = source .split('\n') .map((line, index) => ({ line: index + 1, text: line.trim() })) .filter( ({ text }) => - /\brunWithAcpRuntimeOutputDir\(/.test(text) && + /\brunWithAcpRuntimeOutputDir\b/.test(text) && + !text.startsWith('import ') && !text.startsWith('//') && !text.startsWith('*') && !text.startsWith('/*'), diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 9cc434fe087..d433dc4f0c6 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -4556,13 +4556,15 @@ class QwenAgent implements Agent { } /** - * Single choke point for pinning the runtime root of a per-request - * operation to the settings loaded for THAT request's cwd. Every handler - * that resolves session storage for a caller-supplied cwd goes through - * here rather than calling `runWithAcpRuntimeOutputDir` directly, so the - * "which settings pin this operation" decision lives in one place — the - * bug class fixed in #10095 was handlers composing the routing by hand - * with the stale `this.settings` cache. + * Single choke point for pinning the runtime root of an operation to a + * settings object and a cwd. Every caller in this class goes through here + * rather than calling `runWithAcpRuntimeOutputDir` directly, so the routing + * is composed in exactly one place. Which settings to pin with is still the + * caller's decision at this level: callers that already hold deliberately + * scoped settings (workspace MCP discovery, live-session scope checks, + * session creation) pass them in. Handlers that serve a caller-supplied cwd + * must not make that decision themselves — they use + * `runWithPinnedRuntimeBaseDirForRequest` below. */ private runWithPinnedRuntimeBaseDir( settings: LoadedSettings, @@ -4572,6 +4574,26 @@ class QwenAgent implements Agent { return runWithAcpRuntimeOutputDir(settings, cwd, operation); } + /** + * Per-request form of `runWithPinnedRuntimeBaseDir` for handlers that act + * on a caller-supplied cwd. It resolves the settings for THAT cwd itself, + * so the "which settings pin this operation" decision is made here, once, + * and a handler cannot reach the pin with the process-wide `this.settings` + * cache — the bug class fixed in #10095 (three handlers composed the + * routing by hand and pinned another workspace's runtime root). The + * operation receives the resolved settings for handlers that also need + * them inside the pinned scope. + */ + private runWithPinnedRuntimeBaseDirForRequest( + cwd: string, + operation: (settings: LoadedSettings) => T, + ): T { + const settings = loadSettingsCached(cwd); + return this.runWithPinnedRuntimeBaseDir(settings, cwd, () => + operation(settings), + ); + } + /** * Whether an ungated restore replay (qwen/session/loadUpdates) may * finalize dangling tool calls. A session with an active turn — a client @@ -5782,8 +5804,7 @@ class QwenAgent implements Agent { // a multi-workspace daemon it may hold another workspace's // advanced.runtimeOutputDir and this listing would scan the wrong runtime // root (returning an empty/foreign list for this cwd). - const settings = loadSettingsCached(cwd); - const result = await this.runWithPinnedRuntimeBaseDir(settings, cwd, () => { + const result = await this.runWithPinnedRuntimeBaseDirForRequest(cwd, () => { const sessionService = new SessionService(cwd); return sessionService.listSessions({ cursor: numericCursor, @@ -8869,8 +8890,7 @@ class QwenAgent implements Agent { } try { - const settings = loadSettingsCached(cwd); - const readTranscriptPage = async () => { + const readTranscriptPage = async (settings: LoadedSettings) => { if (rawDirection === 'backward') { await this.sessions .get(sessionId) @@ -8923,8 +8943,7 @@ class QwenAgent implements Agent { : {}), } as Record; }; - return await this.runWithPinnedRuntimeBaseDir( - settings, + return await this.runWithPinnedRuntimeBaseDirForRequest( cwd, readTranscriptPage, ); @@ -11799,7 +11818,6 @@ class QwenAgent implements Agent { ); } const session = this.sessionOrThrow(sessionId); - const settings = loadSettingsCached(cwd); const readSettledTurnResult = async () => { try { await session.getConfig().getChatRecordingService()?.flush(); @@ -11874,8 +11892,7 @@ class QwenAgent implements Agent { throw error; } }; - return await this.runWithPinnedRuntimeBaseDir( - settings, + return await this.runWithPinnedRuntimeBaseDirForRequest( cwd, readSettledTurnResult, ); @@ -12096,8 +12113,7 @@ class QwenAgent implements Agent { // destructive lookup at the wrong runtime root — silently returning // success:false for a session that exists, or deleting a stale // same-id copy under the wrong root. - const success = await this.runWithPinnedRuntimeBaseDir( - loadSettingsCached(cwd), + const success = await this.runWithPinnedRuntimeBaseDirForRequest( cwd, async () => { const sessionService = new SessionService(cwd); @@ -12147,8 +12163,7 @@ class QwenAgent implements Agent { return { success: ok }; } // Per-request settings for the same reason as deleteSession above. - const success = await this.runWithPinnedRuntimeBaseDir( - loadSettingsCached(cwd), + const success = await this.runWithPinnedRuntimeBaseDirForRequest( cwd, async () => { const sessionService = new SessionService(cwd); From e95b83c28993d701b0f70170248b34449cc65ecc Mon Sep 17 00:00:00 2001 From: tomsen-ai <230283659+tomsen-ai@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:08:10 +0800 Subject: [PATCH 4/5] refactor(cli): fold loadUpdates' non-live pin into the per-request helper (round-2 R1-1/R2-1/R2-2) review-pr round 2 on #10988: - R1-1 (fix-induced): the shared helper's comment claimed every caller-supplied-cwd handler already used the per-request variant, but qwen/session/loadUpdates' non-live branch still composed loadSettingsCached + the three-argument helper by hand, and session load/resume resolve settings at the call site on purpose. Migrate the loadUpdates branch (settings are not needed outside the pin), add a behavioral pin for it, and make the comment say exactly which handlers use the variant and why load/resume do not (profiler-instrumented resolution whose settings are adopted for the session afterwards). - R2-1: the source pin skipped every import line, so an aliased import (`import { runWithAcpRuntimeOutputDir as pinDirect }`) was invisible. It now skips only the canonical import line. - R2-2: the pin's failure message pointed fixers at the three-argument helper; it now names runWithPinnedRuntimeBaseDirForRequest for caller-supplied-cwd handlers. Mutation matrix, one mutant at a time, whole file (605 tests): loadUpdates non-live -> this.settings: 1 failed (its new pin) aliased import + helper pins this.settings: 7 failed (source pin names line 321 + six handler pins) deleteSession -> this.settings: 1 failed (its pin), source pin green, message now names the per-request helper Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TGftREDUQDei396TrMLQHA --- .../cli/src/acp-integration/acpAgent.test.ts | 51 +++++++++++++++++-- packages/cli/src/acp-integration/acpAgent.ts | 14 ++--- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index ed9f385ec7b..bbd6aa63e5a 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -20895,6 +20895,46 @@ describe('QwenAgent session-management routing (rename / delete / list / branch await agentPromise; }); + it('resolves non-live qwen/session/loadUpdates settings per request, not from the this.settings cache', async () => { + const innerConfig = makeLiveSessionInnerConfig(null); + const { agent, agentPromise } = await bootAgent(innerConfig); + + // No newSession: the target is not live in this process, so loadUpdates + // takes the disk-only SessionService branch. `this.settings` holds the + // boot workspace's settings; the request names another cwd whose own + // settings must pin the read. + const perRequestSettings = makeAcpSettings(); + vi.mocked(loadSettings).mockReturnValue(perRequestSettings); + const loadSession = vi.fn().mockResolvedValue(null); + vi.mocked(SessionService).mockImplementation( + () => ({ loadSession }) as unknown as InstanceType, + ); + vi.mocked(runWithAcpRuntimeOutputDir).mockClear(); + + await expect( + agent.extMethod('qwen/session/loadUpdates', { + cwd: '/tmp/workspace-a', + sessionId: '6ba7b810-9dad-11d1-80b4-00c04fd430c8', + }), + ).resolves.toEqual({ updates: [] }); + + expect(SessionService).toHaveBeenCalledWith('/tmp/workspace-a'); + expect(loadSession).toHaveBeenCalledWith( + '6ba7b810-9dad-11d1-80b4-00c04fd430c8', + ); + expect(loadSettings).toHaveBeenCalledWith('/tmp/workspace-a'); + expect(runWithAcpRuntimeOutputDir).toHaveBeenCalledTimes(1); + expect(vi.mocked(runWithAcpRuntimeOutputDir).mock.calls[0]![0]).toBe( + perRequestSettings, + ); + expect(vi.mocked(runWithAcpRuntimeOutputDir).mock.calls[0]![1]).toBe( + '/tmp/workspace-a', + ); + + mockConnectionState.resolve(); + await agentPromise; + }); + it('returns success=false when the live ChatRecordingService rejects the title (I/O error)', async () => { const recording = makeRecordingService(); recording.recordCustomTitle.mockResolvedValue(false); @@ -30049,8 +30089,9 @@ describe('QwenAgent runtime-root pinning choke point', () => { // shared helper. A handler naming `runWithAcpRuntimeOutputDir` directly — // as a call or via an alias — is exactly the shape that regressed, and // no behavioral test can see the difference (both spellings reach the - // same function), so pin the source: outside imports and comments the - // only mention left is the shared helper's own delegation. The per-request + // same function), so pin the source: outside the canonical import line + // and comments, the only mention left is the shared helper's own + // delegation (an aliased import would be a second mention). The per-request // handlers themselves are pinned behaviorally (settings/cwd reaching the // mock) by the routing tests above. const source = readFileSync('src/acp-integration/acpAgent.ts', 'utf8'); @@ -30060,7 +30101,9 @@ describe('QwenAgent runtime-root pinning choke point', () => { .filter( ({ text }) => /\brunWithAcpRuntimeOutputDir\b/.test(text) && - !text.startsWith('import ') && + !text.startsWith( + "import { runWithAcpRuntimeOutputDir } from './runtimeOutputDirContext.js';", + ) && !text.startsWith('//') && !text.startsWith('*') && !text.startsWith('/*'), @@ -30068,7 +30111,7 @@ describe('QwenAgent runtime-root pinning choke point', () => { const located = directCalls.map(({ line, text }) => `${line}: ${text}`); expect( located, - `acpAgent.ts must not call runWithAcpRuntimeOutputDir directly; route the operation through this.runWithPinnedRuntimeBaseDir (see #10095). Direct calls at:\n${located.join('\n')}`, + `acpAgent.ts must not name runWithAcpRuntimeOutputDir directly. Handlers serving a caller-supplied cwd route through this.runWithPinnedRuntimeBaseDirForRequest; only callers holding deliberately scoped settings may use this.runWithPinnedRuntimeBaseDir (see #10095). Direct mentions at:\n${located.join('\n')}`, ).toEqual([ expect.stringMatching( /^\d+: return runWithAcpRuntimeOutputDir\(settings, cwd, operation\);$/, diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index d433dc4f0c6..c7b1b05ff4b 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -4562,9 +4562,13 @@ class QwenAgent implements Agent { * is composed in exactly one place. Which settings to pin with is still the * caller's decision at this level: callers that already hold deliberately * scoped settings (workspace MCP discovery, live-session scope checks, - * session creation) pass them in. Handlers that serve a caller-supplied cwd - * must not make that decision themselves — they use - * `runWithPinnedRuntimeBaseDirForRequest` below. + * session creation) pass them in. Per-request session-management handlers + * (list, delete, rename, transcript page, settled turn status, and the + * non-live branch of loadUpdates) must not make that decision themselves — + * they use `runWithPinnedRuntimeBaseDirForRequest` below. Session load and + * resume resolve the request's settings at the call site deliberately, + * under profiler instrumentation, because they adopt those settings for + * the session afterwards. */ private runWithPinnedRuntimeBaseDir( settings: LoadedSettings, @@ -12394,9 +12398,7 @@ class QwenAgent implements Agent { : await loadAuthoritative(); replayConfig = config; } else { - const settings = loadSettingsCached(cwd); - sessionData = await this.runWithPinnedRuntimeBaseDir( - settings, + sessionData = await this.runWithPinnedRuntimeBaseDirForRequest( cwd, async () => { const sessionService = new SessionService(cwd); From 2c4d313eca40f27964eb362f1a221a91d6fa5765 Mon Sep 17 00:00:00 2001 From: tomsen-ai <230283659+tomsen-ai@users.noreply.github.com> Date: Fri, 4 Sep 2026 17:35:41 +0800 Subject: [PATCH 5/5] test(cli): make the runtime-root source pin walk the AST (round-3 R2-1 root cause) review-pr rounds 2 and 3 kept finding false positives in the line-based source pin (exact-match import exemption trips on a Prettier reformat; whole-line comment exclusion misses trailing comments and string literals). The root cause is scanning text lines, so scan the AST instead, following the fast-path.test.ts precedent: every Identifier named runWithAcpRuntimeOutputDir is a mention unless it is the name of an un-aliased import specifier. Comments, string literals and import formatting cannot false-positive; an aliased import keeps the identifier under propertyName and is reported with its line. Probe matrix on the pin alone (restored between runs): benign multi-line import reformat green trailing comment naming the function green string literal naming the function green aliased import used by the helper red, names the import line direct call at deleteSession red, names the call line const pin = runWithAcpRuntimeOutputDir red, names the alias line Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TGftREDUQDei396TrMLQHA --- .../cli/src/acp-integration/acpAgent.test.ts | 65 ++++++++++++------- 1 file changed, 41 insertions(+), 24 deletions(-) diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index bbd6aa63e5a..0bceec28a97 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -16,6 +16,7 @@ import { } from 'vitest'; import { readFileSync } from 'node:fs'; import type { Stats } from 'node:fs'; +import * as ts from 'typescript'; import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; @@ -30087,31 +30088,47 @@ describe('QwenAgent runtime-root pinning choke point', () => { // go through `runWithPinnedRuntimeBaseDirForRequest`, which resolves the // settings from the request cwd itself, and everything else through the // shared helper. A handler naming `runWithAcpRuntimeOutputDir` directly — - // as a call or via an alias — is exactly the shape that regressed, and - // no behavioral test can see the difference (both spellings reach the - // same function), so pin the source: outside the canonical import line - // and comments, the only mention left is the shared helper's own - // delegation (an aliased import would be a second mention). The per-request - // handlers themselves are pinned behaviorally (settings/cwd reaching the - // mock) by the routing tests above. - const source = readFileSync('src/acp-integration/acpAgent.ts', 'utf8'); - const directCalls = source - .split('\n') - .map((line, index) => ({ line: index + 1, text: line.trim() })) - .filter( - ({ text }) => - /\brunWithAcpRuntimeOutputDir\b/.test(text) && - !text.startsWith( - "import { runWithAcpRuntimeOutputDir } from './runtimeOutputDirContext.js';", - ) && - !text.startsWith('//') && - !text.startsWith('*') && - !text.startsWith('/*'), - ); - const located = directCalls.map(({ line, text }) => `${line}: ${text}`); + // as a call, via an alias, or via an aliased import — is exactly the + // shape that regressed, and no behavioral test can see the difference + // (both spellings reach the same function), so pin the source. Walk the + // AST rather than lines: comments, string literals and import formatting + // cannot false-positive, the canonical un-aliased import specifier is the + // one exempt mention, and the only other mention must be the shared + // helper's own delegation. The per-request handlers themselves are pinned + // behaviorally (settings/cwd reaching the mock) by the routing tests above. + const sourcePath = 'src/acp-integration/acpAgent.ts'; + const source = readFileSync(sourcePath, 'utf8'); + const lines = source.split('\n'); + const sourceFile = ts.createSourceFile( + sourcePath, + source, + ts.ScriptTarget.Latest, + true, + ts.ScriptKind.TS, + ); + const mentions: string[] = []; + const visit = (node: ts.Node): void => { + if (ts.isIdentifier(node) && node.text === 'runWithAcpRuntimeOutputDir') { + // `import { runWithAcpRuntimeOutputDir } from …` binds the name + // without an alias (`propertyName` is unset). An aliased specifier + // (`runWithAcpRuntimeOutputDir as pin`) keeps this identifier under + // `propertyName` and is reported like any other mention. + const isCanonicalImport = + ts.isImportSpecifier(node.parent) && + node.parent.propertyName === undefined; + if (!isCanonicalImport) { + const { line } = sourceFile.getLineAndCharacterOfPosition( + node.getStart(sourceFile), + ); + mentions.push(`${line + 1}: ${lines[line]!.trim()}`); + } + } + ts.forEachChild(node, visit); + }; + visit(sourceFile); expect( - located, - `acpAgent.ts must not name runWithAcpRuntimeOutputDir directly. Handlers serving a caller-supplied cwd route through this.runWithPinnedRuntimeBaseDirForRequest; only callers holding deliberately scoped settings may use this.runWithPinnedRuntimeBaseDir (see #10095). Direct mentions at:\n${located.join('\n')}`, + mentions, + `acpAgent.ts must not name runWithAcpRuntimeOutputDir directly. Handlers serving a caller-supplied cwd route through this.runWithPinnedRuntimeBaseDirForRequest; only callers holding deliberately scoped settings may use this.runWithPinnedRuntimeBaseDir (see #10095). Direct mentions at:\n${mentions.join('\n')}`, ).toEqual([ expect.stringMatching( /^\d+: return runWithAcpRuntimeOutputDir\(settings, cwd, operation\);$/,