Skip to content

fix(integration-tests): ack the daemon tool guard in the mock ACP child (#9160) #9162

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.

Sign up for GitHub

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

Merged
wenshao merged 3 commits into from
Aug 14, 2026
Merged

fix(integration-tests): ack the daemon tool guard in the mock ACP child (#9160) #9162

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d05a09c
fix(integration-tests): ack the daemon tool guard in the mock ACP chi…
Aug 14, 2026
ac2fa92
fix(integration-tests): scrub the guard provider marker in the mock A…
Aug 14, 2026
4b86538
fix(integration-tests): attach mock ACP child initialize _meta once (…
Aug 14, 2026
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
29 changes: 29 additions & 0 deletions integration-tests/fixtures/mock-acp-child/agent.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ import {
PROTOCOL_VERSION,
RequestError,
} from '@agentclientprotocol/sdk';
import {
EXTERNAL_TOOL_GUARD_READY_META_KEY,
EXTERNAL_TOOL_GUARD_REQUIRED_VALUE,
PRIVATE_EXTERNAL_TOOL_GUARD_ENV,
PRIVATE_EXTERNAL_TOOL_GUARD_PROVIDER_ENV,
} from '@qwen-code/acp-bridge/externalToolGuard';
Comment on lines +27 to +32

@qwen-code-ci-bot qwen-code-ci-bot Aug 14, 2026

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] R1-1: A racing duplicate PR (9161) edits these same fixture lines for sibling issue 9159 — both autofix PRs opened about 4 minutes apart, and issue 9160's triage instructed that the repair PR reference both issues. PR 9161 makes the identical handshake change off the same base blob, plus one extra delete process.env[PRIVATE_EXTERNAL_TOOL_GUARD_PROVIDER_ENV] (matching the real child in gemini.tsx/acpAgent.ts). — Failure scenario: whichever of PR 9161 / PR 9162 merges second hits a guaranteed text conflict on agent.mjs; if this PR merges alone, issue 9160 closes but issue 9159 stays open until PR 9161 merges separately. Suggested fix: add Fixes #9159 to this PR's description and close PR 9161 (optionally adopting its extra provider-env delete so the retained variant fully mirrors the real child), or defer to PR 9161 and close this PR. No code change is needed for the issue 9160 fix itself.

中文说明

[建议] 竞争中的重复 PR（9161）正在为姊妹 issue 9159 修改本夹具的同一段代码——两个 autofix PR 相隔约 4 分钟提交，且 issue 9160 的分诊评论要求修复 PR 同时引用两个 issue。PR 9161 基于相同的基础版本对同一文件做了完全相同的握手修改，只是额外多了一行 delete process.env[PRIVATE_EXTERNAL_TOOL_GUARD_PROVIDER_ENV]（与真实子进程 gemini.tsx/acpAgent.ts 的行为一致）。— 失败场景：PR 9161 / PR 9162 中后合并的一方必然在 agent.mjs 上产生文本冲突；若仅本 PR 合并，issue 9160 会关闭，但 issue 9159 要等 PR 9161 另行合并后才会关闭。建议处理：在本 PR 描述中添加 Fixes #9159 并关闭 PR 9161（可选地采纳其多出的 provider 环境变量删除行，使保留的变体完全镜像真实子进程），或者改用 PR 9161 并关闭本 PR。就 issue 9160 的修复本身而言无需任何代码改动。

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

@qwen-code-dev-bot qwen-code-dev-bot Aug 14, 2026

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.

Code part adopted; PR-management part needs a maintainer.

This round adopted PR 9161's extra line: the mock child now also does delete process.env[PRIVATE_EXTERNAL_TOOL_GUARD_PROVIDER_ENV] (commit ac2fa92abc). Verified against the real children first — acpAgent.ts and gemini.tsx both consume and delete both guard markers, and qwen serve passes exactly these two envs via childEnvOverrides (the token/capability envs are scrubbed daemon-side and never reach the child, so the fixture does not delete them). This PR's fixture change is now a strict superset of PR 9161's.

Not actionable from this flow (no GitHub credentials): adding Fixes #9159 to the PR description and closing PR 9161, or the inverse. Recommendation: keep this PR, add Fixes #9159 to its description, and close PR 9161, since it now carries the complete fixture change. Leaving this thread open for the maintainer's call.

Verification this round: npm run build, npm run bundle, npm run typecheck, npm run lint all pass; both journal-recovery integration suites (the fixture's consumers) pass 3/3 against the bundled CLI with QWEN_SANDBOX=false.

中文说明

代码部分已采纳；PR 管理部分需要维护者处理。

本轮已采纳 PR 9161 多出的那一行：mock 子进程现在同样执行 delete process.env[PRIVATE_EXTERNAL_TOOL_GUARD_PROVIDER_ENV]（提交 ac2fa92abc）。实现前已对照真实子进程核实——acpAgent.ts 与 gemini.tsx 都会消费并删除这两个守卫标记，而 qwen serve 通过 childEnvOverrides 传递给子进程的恰好就是这两个环境变量（token/capability 环境变量在守护进程一侧即被清除、不会到达子进程，因此夹具不删除它们）。本 PR 的夹具改动现在是 PR 9161 的严格超集。

本流程无法执行（没有 GitHub 凭据）：在 PR 描述中添加 Fixes #9159 并关闭 PR 9161，或反向操作。建议：保留本 PR、在其描述中添加 Fixes #9159、关闭 PR 9161，因为本 PR 已包含完整的夹具改动。此线程保持打开，等待维护者决定。

本轮验证：npm run build、npm run bundle、npm run typecheck、npm run lint 全部通过；两个 journal-recovery 集成测试套件（该夹具的消费方）在 QWEN_SANDBOX=false 下针对打包后的 CLI 运行，3/3 全部通过。

import { Writable, Readable } from 'node:stream';

// Protect the stdout NDJSON pipe — any console method that writes to
Expand All @@ -40,14 +46,37 @@ const delayMs = parseInt(process.env.MOCK_ACP_PROMPT_DELAY_MS || '100', 10);
const emitChunks = parseInt(process.env.MOCK_ACP_EMIT_CHUNKS || '3', 10);
let sessionCounter = 0;

// Mirror the real child (acpAgent.ts): `qwen serve` requires the guard ack
// in the initialize response, and the markers are consumed + deleted before
// anything else can inherit them.
const externalToolGuardMarker = process.env[PRIVATE_EXTERNAL_TOOL_GUARD_ENV];
delete process.env[PRIVATE_EXTERNAL_TOOL_GUARD_ENV];
Comment on lines +52 to +53

@qwen-code-ci-bot qwen-code-ci-bot Aug 14, 2026

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] R1-2: No PR-stage CI gate exercises this fixture change. The PR-stage integration gate (test:integration:no-ak:sandbox:none) is a hardcoded file list containing none of the fixture's three consumers; test:ci covers no workspace this file belongs to; full coverage lives only in the merge-queue job (integration_cli) and post-merge/nightly e2e.yml. This PR's own CI run skipped Integration Tests (CLI, No Sandbox), so CI verified nothing about this change, and the review's scoped build/test collected zero suites for the file. — Failure scenario: if this fixture edit were wrong (ack _meta key/value mismatch, or the new @qwen-code/acp-bridge/externalToolGuard import failing to resolve at child-spawn time), nothing running on pull_request would catch it — the PR would merge and only then fail the merge queue or post-merge E2E, i.e. the very CI failures this PR fixes would persist or be reintroduced, discovered late. Suggested fix: run the consuming integration tests before merge (the Test Plan reports this was already done for two of the three files):

npm run build && npm run bundle
cd integration-tests && QWEN_SANDBOX=false npx vitest run \
  cli/qwen-serve-live-journal-recovery.test.ts \
  cli/qwen-serve-webui-live-journal-recovery.test.ts
中文说明

[建议] 本夹具改动没有任何 PR 阶段的 CI 门禁覆盖。PR 阶段的集成门禁（test:integration:no-ak:sandbox:none）是一个硬编码的文件列表，其中不包含该夹具的三个使用者测试；test:ci 也不覆盖该文件所属的任何工作区；完整覆盖只存在于合并队列任务（integration_cli）和合并后/每夜的 e2e.yml。本 PR 自己的 CI 运行中 Integration Tests (CLI, No Sandbox) 被跳过，因此 CI 未对本改动做任何验证，且本次评审中按范围运行的构建/测试对该文件也未收集到任何测试套件。— 失败场景：如果该夹具修改有误（确认用的 _meta 键/值不匹配，或新增的 @qwen-code/acp-bridge/externalToolGuard 导入在子进程启动时解析失败），pull_request 阶段运行的任何检查都无法发现——PR 会先合并，随后才在合并队列或合并后 E2E 中失败，也就是说本 PR 要修复的 CI 失败会持续存在或被重新引入，且发现时机滞后。建议处理：在合并前运行消费该夹具的集成测试（测试计划显示作者已对其中两个文件执行过）：

npm run build && npm run bundle
cd integration-tests && QWEN_SANDBOX=false npx vitest run \
  cli/qwen-serve-live-journal-recovery.test.ts \
  cli/qwen-serve-webui-live-journal-recovery.test.ts

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

delete process.env[PRIVATE_EXTERNAL_TOOL_GUARD_PROVIDER_ENV];
const externalToolGuardRequired =
externalToolGuardMarker === EXTERNAL_TOOL_GUARD_REQUIRED_VALUE;

new AgentSideConnection(
(connection) => ({
async initialize() {
// Build ONE meta record and attach `_meta` once, exactly like the
// real child (acpAgent.ts), so a future conditional meta source
// merges instead of clobbering the guard ack via duplicate keys.
const responseMeta = {
...(externalToolGuardRequired
? {
[EXTERNAL_TOOL_GUARD_READY_META_KEY]:
EXTERNAL_TOOL_GUARD_REQUIRED_VALUE,
}
: {}),
};
return {
protocolVersion: PROTOCOL_VERSION,
agentInfo: { name: 'mock-acp', version: '0.0.1' },
authMethods: [],
agentCapabilities: {},
...(Object.keys(responseMeta).length > 0
? { _meta: responseMeta }
: {}),
};
},

Expand Down
Loading