Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,15 @@ describe('qwen serve live journal recovery', () => {
expect(replayText).toContain('chunk-19');

await prompt;

// Gate the fixture's session-close ack: the only other consumer of
// this path is the opt-in loadtest, so without this call a drifted
// close method name or missing close handler would surface only in
// the post-merge E2E workflow (ack-shape drift is absorbed by the
// daemon's cancel fallback and is not observable from the client).
await expect(
activeDaemon.client.closeSession(created.sessionId),
).resolves.toBeUndefined();
Comment on lines +90 to +92

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 gate is proven effective (removing the fixture's close handler fails this assertion with DaemonHttpError: DELETE /session/:id: "Method not found": qwen/control/session/close), but no PR-gated CI job collects this test file, so it only executes in the post-merge/nightly E2E Tests workflow. Verified at this commit: e2e.yml has no pull_request trigger (its own comment says E2E "is NOT in the merge queue"; it runs post-merge on main, nightly, and on dispatch); the PR-gated no-AK integration gate (test:integration:no-ak:sandbox:none in the root package.json) is a fixed file list that omits this file; and ci.yml's integration_cli job — whose test:integration:cli:sandbox:none would collect it — is gated to merge_group and never triggers while no merge queue is configured on main (the active main ruleset has no merge_queue rule). The comment above claims this gates the ack "in default CI". Concrete cost: a future change that drifts SESSION_CLOSE_EXT_METHOD or removes the fixture's close handler leaves every PR check green, merges, and surfaces only at the next post-merge main E2E or nightly run — the silent-rot window this call is meant to close, displaced to after merge. (Drift still fails loudly there — that is how the original breakage this PR fixes was caught — so this is a placement improvement, not an open hole.) Suggested fix: add ./cli/qwen-serve-live-journal-recovery.test.ts to the test:integration:no-ak:sandbox:none file list — the test needs no API key (it drives the mock ACP child), which is the class that gate exists to run at PR time; alternatively narrow the comment to name the post-merge E2E workflow.

中文说明

该门控已被证实有效(移除夹具的 close 处理器会使此断言以 DaemonHttpError: DELETE /session/:id: "Method not found": qwen/control/session/close 失败),但没有任何 PR 门控的 CI 任务收集该测试文件,因此它只会在合并后/每晚的 E2E Tests 工作流中执行。已在本提交上核实:e2e.yml 没有 pull_request 触发器(其自身注释写明 E2E "不在合并队列中";仅在 main 合并后、每晚定时和手动触发时运行);PR 门控的 no-AK 集成门(根 package.json 中的 test:integration:no-ak:sandbox:none)是一份固定文件列表,其中不含该文件;ci.yml 中的 integration_cli 任务——其 test:integration:cli:sandbox:none 本会收集它——仅在 merge_group 事件下运行,而 main 当前生效的 ruleset 没有 merge_queue 规则,故该任务从不触发。上方注释声称此调用在"默认 CI"中门控该 ack。具体代价:未来若 SESSION_CLOSE_EXT_METHOD 漂移或夹具的 close 处理器被移除,所有 PR 检查仍将保持绿色并照常合并,问题只会在下一次 main 合并后 E2E 或每晚运行中浮现——这正是此调用意图关闭的静默腐化窗口,只是被推迟到了合并之后。(漂移在那里仍会响亮地失败——本 PR 所修复的最初破损正是这样被捕获的——因此这是门控位置的改进,而非敞开的漏洞。)建议修复:将 ./cli/qwen-serve-live-journal-recovery.test.ts 加入 test:integration:no-ak:sandbox:none 的文件列表——该测试不需要 API key(它驱动 mock ACP 子进程),正是该门存在以便在 PR 阶段运行的类别;或者将注释收窄,改为指明合并后 E2E 工作流。

— qwen3.8-max via Qwen Code /review (v0.21.11)

} finally {
await activeDaemon?.dispose();
activeDaemon = undefined;
Expand Down
17 changes: 17 additions & 0 deletions integration-tests/fixtures/mock-acp-child/agent.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ delete process.env[PRIVATE_EXTERNAL_TOOL_GUARD_PROVIDER_ENV];
const externalToolGuardRequired =
externalToolGuardMarker === EXTERNAL_TOOL_GUARD_REQUIRED_VALUE;

// SERVE_CONTROL_EXT_METHODS.sessionClose from @qwen-code/acp-bridge/status,
// hardcoded because that module runtime-imports @qwen-code/qwen-code-core
// and would pull core's whole barrel into this lightweight fixture. Drift
// fails loudly: the daemon's session close errors when this stops matching.
const SESSION_CLOSE_EXT_METHOD = 'qwen/control/session/close';

new AgentSideConnection(
(connection) => ({
async initialize() {
Expand Down Expand Up @@ -119,6 +125,17 @@ new AgentSideConnection(
},

async cancel() {},

async extMethod(method, params) {
if (method === SESSION_CLOSE_EXT_METHOD) {
// The daemon's DELETE /session/:id forwards this ext method down the
// ACP channel; ack with the production success shape so teardown
// completes. The mock keeps no per-session state to drain.
const { sessionId } = params;
return { sessionId, closed: true };
}
throw RequestError.methodNotFound(method);
},
}),
ndJsonStream(Writable.toWeb(process.stdout), Readable.toWeb(process.stdin)),
);
Expand Down
Loading