Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 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
4 changes: 2 additions & 2 deletions packages/channels/base/src/AcpBridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,12 @@ export class AcpBridge extends EventEmitter {

async loadSession(sessionId: string, cwd: string): Promise<string> {
const conn = this.ensureConnection();
const response = await conn.loadSession({
await conn.loadSession({
sessionId,
cwd,
mcpServers: [],
});

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.

[Nit] 行为契约从「返回 agent 报告的 session ID」改成了「直接回显调用者传入的 sessionId,信任 ACP agent 用同样的 ID 加载」。这是正确的——LoadSessionResponse 规范里就没这个字段——但建议加一行注释,避免未来 reader 觉得「这看起来像 bug」又把它改回去:

// LoadSessionResponse has no sessionId field — the ACP agent loads the
// session under the caller-supplied ID, so we just echo it back.
return sessionId;

return response.sessionId;
return sessionId;
}

async prompt(
Expand Down
201 changes: 201 additions & 0 deletions packages/channels/base/src/SessionRouter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,67 @@ describe('SessionRouter', () => {
expect(router.hasSession('ch', 'alice', 'chat1')).toBe(false);
expect(router.getAll()).toEqual([]);
});

it('preserves persist file for restoration on next start', async () => {
const { mkdirSync, writeFileSync, rmSync, readFileSync, existsSync } =
await import('node:fs');
const { join } = await import('node:path');
const tmpDir = join('/tmp', `test-cleara11-persist-${Date.now()}`);

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.

[Nit] 4 个新测试都用 join('/tmp', `test-...-${Date.now()}`) 拼临时目录名。Vitest 文件级并行下,两个测试在同一毫秒启动会撞同名目录;/tmp 也不是 Windows 友好。改用 fs.mkdtempSync 既保证唯一性又跨平台:

import { mkdtempSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
const tmpDir = mkdtempSync(join(tmpdir(), 'test-restore-'));

顺便:本行的目录前缀写成了 test-cleara11-persist-,应是 test-clearAll-persist- 的笔误。

mkdirSync(tmpDir, { recursive: true });

const persistFile = join(tmpDir, 'sessions.json');
const originalEntries = {
'telegram:alice:chat1': {
sessionId: 'session-xyz',
target: {
channelName: 'telegram',
senderId: 'alice',
chatId: 'chat1',
},
cwd: '/workspace',
},
};

try {
// Create a router, write a session, then clearAll
const router = new SessionRouter(bridge, '/tmp', 'user', persistFile);
await router.resolve('telegram', 'alice', 'chat1');

// Populate the persist file with known data
writeFileSync(persistFile, JSON.stringify(originalEntries, null, 2));

router.clearAll();

// Memory should be cleared
expect(router.hasSession('telegram', 'alice', 'chat1')).toBe(false);
expect(router.getAll()).toEqual([]);

// Persist file should still exist with original content
expect(existsSync(persistFile)).toBe(true);
const onDisk = JSON.parse(readFileSync(persistFile, 'utf-8'));
expect(onDisk).toEqual(originalEntries);

// A fresh router should be able to restore from it
const freshRouter = new SessionRouter(
bridge,
'/tmp',
'user',
persistFile,
);
const result = await freshRouter.restoreSessions();

expect(result.restored).toBe(1);
expect(result.failed).toBe(0);
expect(freshRouter.hasSession('telegram', 'alice', 'chat1')).toBe(true);
expect(freshRouter.getTarget('session-xyz')).toEqual({
channelName: 'telegram',
senderId: 'alice',
chatId: 'chat1',
});
} finally {
rmSync(tmpDir, { recursive: true });
}
});
});

describe('setBridge', () => {
Expand All @@ -229,4 +290,144 @@ describe('SessionRouter', () => {
expect(bridge.newSession).not.toHaveBeenCalled();
});
});

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.

[Suggestion] describe('restoreSessions') 下的 3 个新测试直接调用 rmSync 清理临时目录,没有 try/finally 包裹。若测试中途失败(断言异常等),/tmp/test-restore-* 目录会残留。建议与同文件中 preserves persist file 测试保持一致,使用 try { ... } finally { rmSync(tmpDir, { recursive: true }) } 包裹测试体。

— deepseek-v4-pro via Qwen Code /review

describe('restoreSessions', () => {
it('restores sessions from persist file', async () => {
const { mkdirSync, writeFileSync, rmSync } = await import('node:fs');
const { join } = await import('node:path');
const tmpDir = join('/tmp', `test-restore-${Date.now()}`);
mkdirSync(tmpDir, { recursive: true });

try {
const persistFile = join(tmpDir, 'sessions.json');
const entries = {
'telegram:alice:chat1': {
sessionId: 'old-session-1',
target: {
channelName: 'telegram',
senderId: 'alice',
chatId: 'chat1',
},
cwd: '/workspace',
},
'telegram:bob:chat2': {
sessionId: 'old-session-2',
target: {
channelName: 'telegram',
senderId: 'bob',
chatId: 'chat2',
},
cwd: '/workspace',
},
};
writeFileSync(persistFile, JSON.stringify(entries, null, 2));

const router = new SessionRouter(bridge, '/tmp', 'user', persistFile);
const result = await router.restoreSessions();

expect(result.restored).toBe(2);
expect(result.failed).toBe(0);
expect(bridge.loadSession).toHaveBeenCalledTimes(2);
expect(router.hasSession('telegram', 'alice', 'chat1')).toBe(true);
expect(router.hasSession('telegram', 'bob', 'chat2')).toBe(true);

const target = router.getTarget('old-session-1');
expect(target).toEqual({
channelName: 'telegram',
senderId: 'alice',
chatId: 'chat1',
});
} finally {
rmSync(tmpDir, { recursive: true });
}
});

it('skips entries whose loadSession throws', async () => {
const { mkdirSync, writeFileSync, rmSync } = await import('node:fs');
const { join } = await import('node:path');
const tmpDir = join('/tmp', `test-restore-fail-${Date.now()}`);
mkdirSync(tmpDir, { recursive: true });

try {
const persistFile = join(tmpDir, 'sessions.json');
const entries = {
'telegram:alice:chat1': {
sessionId: 'good-session',
target: {
channelName: 'telegram',
senderId: 'alice',
chatId: 'chat1',
},
cwd: '/workspace',
},
'telegram:bob:chat2': {
sessionId: 'stale-session',
target: {
channelName: 'telegram',
senderId: 'bob',
chatId: 'chat2',
},
cwd: '/workspace',
},
};
writeFileSync(persistFile, JSON.stringify(entries, null, 2));

// Make loadSession throw for the stale session
(bridge.loadSession as ReturnType<typeof vi.fn>).mockImplementation(
(id: string) => {
if (id === 'stale-session') {
throw new Error('Session not found');
}
return id;
},
);

const router = new SessionRouter(bridge, '/tmp', 'user', persistFile);
const result = await router.restoreSessions();

expect(result.restored).toBe(1);
expect(result.failed).toBe(1);
expect(router.hasSession('telegram', 'alice', 'chat1')).toBe(true);
expect(router.hasSession('telegram', 'bob', 'chat2')).toBe(false);

// Persist file should be updated to remove the failed entry
const { readFileSync } = await import('node:fs');
const updated = JSON.parse(readFileSync(persistFile, 'utf-8'));
expect(Object.keys(updated)).toEqual(['telegram:alice:chat1']);
} finally {
rmSync(tmpDir, { recursive: true });
}
});

it('returns zeros when no persist file exists', async () => {
const router = new SessionRouter(
bridge,
'/tmp',
'user',
'/nonexistent/sessions.json',
);
const result = await router.restoreSessions();
expect(result).toEqual({ restored: 0, failed: 0 });
});

it('returns zeros when persist file is empty', async () => {
const { mkdirSync, writeFileSync, rmSync } = await import('node:fs');
const { join } = await import('node:path');
const tmpDir = join('/tmp', `test-restore-empty-${Date.now()}`);
mkdirSync(tmpDir, { recursive: true });

try {
const persistFile = join(tmpDir, 'sessions.json');
writeFileSync(persistFile, '{}');

const router = new SessionRouter(bridge, '/tmp', 'user', persistFile);
const result = await router.restoreSessions();

expect(result).toEqual({ restored: 0, failed: 0 });
expect(bridge.loadSession).not.toHaveBeenCalled();
} finally {
rmSync(tmpDir, { recursive: true });
}
});
});
});
12 changes: 3 additions & 9 deletions packages/channels/base/src/SessionRouter.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { existsSync, readFileSync, writeFileSync, unlinkSync } from 'node:fs';
import { existsSync, readFileSync, writeFileSync } from 'node:fs';
import type { SessionScope, SessionTarget } from './types.js';
import type { AcpBridge } from './AcpBridge.js';

Expand Down Expand Up @@ -196,18 +196,12 @@ export class SessionRouter {
return { restored, failed };
}

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.

[Suggestion] 新加的 && restored > 0 守护合理(避免一次性失败清空文件),但反向意味着:当 failed > 0 && restored === 0 时,持久文件永远不会被修剪。如果底层 qwen-code session 存储被清掉、或所有 sessionId 都真的失效,每次启动都会重试同一批死 ID 并写 stderr 失败日志;同时文件还会随后续 resolve() 单调增长。

可考虑:

  • 在「至少加载到 1 条」之外,再加一个「连续 N 次全失败则强制修剪」的逃生口;或
  • 区分「桥未就绪/瞬时错误」与「agent 明确拒绝(如 sessionExists=false)」,只对后者修剪。

若不想现在改,建议至少留 TODO 标注,避免后续遗忘。

/** Clear in-memory state and delete persist file. Used on clean shutdown. */
/** Clear in-memory state. Persist file is left intact for the next start

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.

[Suggestion] clearAll() now intentionally preserves the persist file, but the test coverage only verifies that the in-memory maps are cleared. This leaves the new shutdown persistence contract unprotected: a future change could re-delete or truncate sessions.json while the current tests would still pass.

Please add a regression test that writes a session through a router with a real persistPath, calls clearAll(), and then asserts that the file still exists and can be restored by a fresh router.

— gpt-5.5 via Qwen Code /review

* via {@link restoreSessions}. */
clearAll(): void {
this.toSession.clear();
this.toTarget.clear();
this.toCwd.clear();
if (this.persistPath && existsSync(this.persistPath)) {
try {
unlinkSync(this.persistPath);
} catch {
// best-effort
}
}
}

private persist(): void {
Expand Down
60 changes: 59 additions & 1 deletion packages/cli/src/acp-integration/acpAgent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ vi.mock('@qwen-code/qwen-code-core', () => ({
_args: args,
})),
SessionService: vi.fn(),
Storage: {
runWithRuntimeBaseDir: vi.fn(
(_dir: unknown, _cwd: unknown, fn: () => unknown) => fn(),
),
},
tokenLimit: vi.fn(),
SessionStartSource: {
Startup: 'startup',
Expand Down Expand Up @@ -126,7 +131,11 @@ import {
import type { Config } from '@qwen-code/qwen-code-core';
import type { LoadedSettings } from '../config/settings.js';
import type { CliArgs } from '../config/config.js';
import { SessionEndReason, MCPServerConfig } from '@qwen-code/qwen-code-core';
import {
SessionEndReason,
MCPServerConfig,
SessionService,
} from '@qwen-code/qwen-code-core';
import type { McpServer } from '@agentclientprotocol/sdk';
import { AgentSideConnection } from '@agentclientprotocol/sdk';
import { loadSettings } from '../config/settings.js';
Expand Down Expand Up @@ -631,6 +640,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => {
type AgentLike = {
initialize: (args: Record<string, unknown>) => Promise<unknown>;
newSession: (args: Record<string, unknown>) => Promise<unknown>;
loadSession: (args: Record<string, unknown>) => Promise<unknown>;
};

let mockConfig: Config;
Expand Down Expand Up @@ -893,4 +903,52 @@ describe('QwenAgent MCP SSE/HTTP support', () => {
mockConnectionState.resolve();
await agentPromise;
});

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.

[Suggestion] loadSession rejects when sessionExists returns false 测试通过 vi.mocked(SessionService).mockImplementation(...) 全局覆盖 mock,但测试结束时未恢复默认实现。虽然当前恰好是该 describe 块的最后一个测试,但若未来有人在该块末尾新增测试,残留的 mock 状态可能造成污染。建议在测试中手动保存/恢复原始 mock,或使用 beforeEach 钩子统一重置。

— deepseek-v4-pro via Qwen Code /review

it('loadSession rejects when sessionExists returns false, skipping config load', async () => {
// Save the default SessionService mock and override for this test
const savedImpl = vi.mocked(SessionService).getMockImplementation();
const sessionExistsStub = vi.fn().mockResolvedValue(false);
vi.mocked(SessionService).mockImplementation(
() =>
({
sessionExists: sessionExistsStub,
}) as unknown as InstanceType<typeof SessionService>,
);

try {
await setupSessionMocks('session-nonexistent');

const agentPromise = runAcpAgent(
mockConfig,
makeSessionSettings(),
mockArgv,
);
await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined());

const agent = capturedAgentFactory!({
get closed() {
return mockConnectionState.promise;
},
}) as AgentLike;

await expect(
agent.loadSession({
cwd: '/tmp',
sessionId: 'nonexistent',
}),
).rejects.toThrow('Session nonexistent does not exist at /tmp');

// Verify sessionExists was queried for the correct session
expect(sessionExistsStub).toHaveBeenCalledWith('nonexistent');

mockConnectionState.resolve();
await agentPromise;
} finally {
// Restore the default mock to avoid polluting subsequent tests
vi.mocked(SessionService).mockImplementation(
savedImpl as typeof vi.mocked<typeof SessionService>,

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] Type error TS2345/TS2352 — savedImpl as typeof vi.mocked<typeof SessionService> cast is incompatible. getMockImplementation() returns NormalizedProcedure<...> | undefined which cannot be cast to vitest's overloaded MockedObject type. This breaks npm run build.

Suggested change
savedImpl as typeof vi.mocked<typeof SessionService>,
vi.mocked(SessionService).mockImplementation(
(savedImpl ?? vi.fn()) as (cwd: string) => SessionService,
);

— deepseek-v4-pro via Qwen Code /review

);
// Also let beforeEach clearAllMocks handle the sessionExists stub
}
});
});
6 changes: 6 additions & 0 deletions packages/cli/src/acp-integration/acpAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,12 @@ class QwenAgent implements Agent {
},
);

if (!exists) {

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.

[Suggestion] The new sessionExists === false branch is the behavior that lets SessionRouter.restoreSessions() prune stale persisted sessions, but it is not directly covered by ACP agent tests. The router tests mock loadSession, so they would not catch a regression where the real agent accidentally resumes or creates a session for a nonexistent ID again.

Please add an ACP agent test that mocks SessionService.sessionExists(params.sessionId) to return false, expects agent.loadSession(...) to reject with this error, and verifies that session creation/config loading is not attempted.

— gpt-5.5 via Qwen Code /review

throw new Error(
`Session ${params.sessionId} does not exist at ${params.cwd}`,
);
}

const config = await this.newSessionConfig(
params.cwd,
params.mcpServers,
Expand Down
16 changes: 16 additions & 0 deletions packages/cli/src/commands/channel/start.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,16 @@ function sessionsPath(): string {
return path.join(os.homedir(), '.qwen', 'channels', 'sessions.json');
}

async function restoreAndLogSessions(router: SessionRouter): Promise<void> {

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.

[Suggestion] restoreAndLogSessions swallows the result — callers can't distinguish partial vs total failure

The helper returns void, discarding { restored, failed }. In the crash-recovery path, the error message says "Bridge restarted but session restore failed" regardless of whether 1/50 or 50/50 sessions failed. Returning the result object would let callers provide better diagnostics.

Suggested change
async function restoreAndLogSessions(router: SessionRouter): Promise<void> {
async function restoreAndLogSessions(router: SessionRouter): Promise<{ restored: number; failed: number }> {
const result = await router.restoreSessions();
if (result.restored > 0 || result.failed > 0) {
writeStdoutLine(
`[Channel] Sessions restored: ${result.restored}` +
(result.failed > 0 ? `, failed: ${result.failed}` : ''),
);
}
return result;
}

— DeepSeek/deepseek-v4-pro via Qwen Code /review

const result = await router.restoreSessions();
if (result.restored > 0 || result.failed > 0) {
writeStdoutLine(
`[Channel] Sessions restored: ${result.restored}` +
(result.failed > 0 ? `, failed: ${result.failed}` : ''),
);
}
}

function loadChannelsConfig(): Record<string, unknown> {
const settings = loadSettings(process.cwd());
const channels = (
Expand Down Expand Up @@ -215,6 +225,9 @@ async function startSingle(name: string, proxy?: string): Promise<void> {
channels.set(name, channel);
registerToolCallDispatch(bridge, router, channels);

// Restore sessions from previous run

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.

[Suggestion] startSinglestartAll 中插入了完全相同的 restoreSessions + 日志代码块(6 行)。建议提取为私有辅助函数以避免未来维护时两处产生分歧:

Suggested change
// Restore sessions from previous run
async function restoreAndLogSessions(router: SessionRouter): Promise<void> {
const result = await router.restoreSessions();
if (result.restored > 0 || result.failed > 0) {
writeStdoutLine(
`[Channel] Sessions restored: ${result.restored}` +
(result.failed > 0 ? `, failed: ${result.failed}` : ''),
);
}
}

— deepseek-v4-pro via Qwen Code /review

await restoreAndLogSessions(router);

try {
await channel.connect();
} catch (err) {
Expand Down Expand Up @@ -365,6 +378,9 @@ async function startAll(proxy?: string): Promise<void> {
}
registerToolCallDispatch(bridge, router, channels);

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.

[Suggestion] startAll 初始启动路径(line 376)中 await restoreAndLogSessions(router)try/catch 保护,而崩溃恢复路径(line 442)有。虽然 restoreSessions 内部有错误处理,但同步异常(如 EACCES)仍可能传播为未处理 rejection,导致 channel 服务崩溃。

Suggested change
try {
await restoreAndLogSessions(router);
} catch (err) {
writeStderrLine(
`[Channel] Failed to restore sessions: ${err instanceof Error ? err.message : String(err)}`,
);
}

— deepseek-v4-pro via Qwen Code /review

// Restore sessions from previous run
await restoreAndLogSessions(router);

// Connect all channels
let connectedCount = 0;
for (const [name, channel] of channels) {
Expand Down
Loading