fix(cli): require exact daemon marker before env scrub - #8811
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): This PR fixes a startup-gate mismatch in the Qwen Code CL...: could not run the test file — the worktree has no usable node_modules ( packages/cli/node_modules exists but is empty, no vitest binary, no root install), an…; This PR fixes a startup-gate mismatch in the Qwen Code CL...: live execution of gemini.test.tsx (blocked by missing worktree dependencies).; This PR fixes a startup-gate mismatch in the Qwen Code CL...: could not execute the test (worktree has no node_modules; parent install stale — vitest fails at transform on ajv/dist/2020.js ).I've completed my review of th….
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：This PR fixes a startup-gate mismatch in the Qwen Code CL...：could not run the test file — the worktree has no usable node_modules ( packages/cli/node_modules exists but is empty, no vitest binary, no root install), an…；This PR fixes a startup-gate mismatch in the Qwen Code CL...：live execution of gemini.test.tsx (blocked by missing worktree dependencies).；This PR fixes a startup-gate mismatch in the Qwen Code CL...：could not execute the test (worktree has no node_modules; parent install stale — vitest fails at transform on ajv/dist/2020.js ).I've completed my review of th…。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
|
Review closeout for
中文摘要
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) / Test (windows-latest, Node 22.x) / Integration Tests (CLI, No Sandbox) were skipped in CI at this commit and the full packages/cli suite did not complete locally (300s timeout); only the targeted gemini.test.tsx rows ran green. Not explored to full depth (tool budget reached): Incremental re-review of PR #8811 (QwenLM/qwen-code): the...: runtime test execution and tsc typecheck of the changed test file (both blocked by the dependency-resolution failure above); PR review comments/thread history….
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Test (macos-latest, Node 22.x) / Test (windows-latest, Node 22.x) / Integration Tests (CLI, No Sandbox) were skipped in CI at this commit and the full packages/cli suite did not complete locally (300s timeout); only the targeted gemini.test.tsx rows ran green。 未探索到全部深度（达到工具调用预算）：Incremental re-review of PR #8811 (QwenLM/qwen-code): the...：runtime test execution and tsc typecheck of the changed test file (both blocked by the dependency-resolution failure above); PR review comments/thread history…。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
|
This pass reviewed the three newly completed suggestions against the exact head and PR scope. No code or PR-body change was needed. Intentionally not changed: an unreachable test guard cleanup, mutation-only coverage for removing the ACP conjunct, and duplicated producer-site documentation. None demonstrates a current-head behavior defect; the exact marker regression and contract are already pinned by the existing cases and PR description. Pending: maintainer review. The current exact-head checks are complete. 中文摘要本轮对照 exact head 和 PR 范围处理了 3 条新 Suggestion，无需修改代码或 PR 描述。未纳入测试死守卫清理、仅用于 mutation 的非 ACP 组合覆盖，以及重复 producer-site 文档；它们都未证明当前 head 存在行为缺陷，现有用例和 PR 描述已钉住精确 marker 回归与约定。当前等待 maintainer review，exact-head checks 已完成。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Not explored to full depth (tool budget reached): PR #8811 (QwenLM/qwen-code) requires QWEN_CODE_SERVE to...: full-file run of gemini.test.tsx (all 76 tests) not performed — only the filtered 5-row table was run.; PR #8811 (QwenLM/qwen-code) requires QWEN_CODE_SERVE to...: npm run typecheck / npm run lint not run for the touched files; the type-inference check of the it.each callback parameters was done by analysis, not comp….
中文说明
未发现问题。LGTM！✅
未探索到全部深度（达到工具调用预算）：PR #8811 (QwenLM/qwen-code) requires QWEN_CODE_SERVE to...：full-file run of gemini.test.tsx (all 76 tests) not performed — only the filtered 5-row table was run.；PR #8811 (QwenLM/qwen-code) requires QWEN_CODE_SERVE to...：npm run typecheck / npm run lint not run for the touched files; the type-inference check of the it.each callback parameters was done by analysis, not comp…。
— qwen3.8-max via Qwen Code /review (v0.21.8)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: This PR narrows the daemon-marker check from a truthy tes... — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
中文说明
已审查。 未审查：This PR narrows the daemon-marker check from a truthy tes...——它被指向 diff 的行却从未打开：有工具调用，但没有一次读取 diff。
— glm-5.2 via Qwen Code /review (v0.21.8)
Local verification: PASSVerified exact head
The contract is consistent at the surrounding boundaries: ACP channel fallback also accepts only the literal No UI screenshot is attached because this change affects child-process environment state, not rendered TUI behavior; a terminal screenshot would not prove the fix. |
|
@qwen-code-review-bot pls review this pr |
|
|
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #8811 fix(cli): require exact daemon marker before env scrub
Type: Bug fix
Change size: +18/-6 across 2 files
HEAD: 026c6f3b8f3fe482c171bbfd5067684048314960
Findings Summary
- Critical/Major: 0
- Minor: 0
- Nit: 0 unique (confirmed existing suggestions from another reviewer)
Key Observations
This is a tightly scoped bugfix: it aligns the ACP loader-env scrub gate in packages/cli/src/gemini.tsx with the exact QWEN_CODE_SERVE=1 contract already used by resolveAcpChannelFallback. The one-line production change is correct, and the regression table in gemini.test.tsx now explicitly covers 0 and false marker values. I independently verified that every in-repository daemon producer sets the marker to '1' explicitly, so normal daemon behavior is unchanged.
Cross-Validation
| Finding | Other Reviewer | My Assessment |
|---|---|---|
Keep rows should explicitly stub QWEN_CODE_SERVE to avoid ambient env dependence |
wenshao (R1-1) | Confirmed — addressed in commit 026c6f3; keep rows now use '' |
daemonMarker !== undefined guard is dead code |
wenshao (R2-1) | Confirmed — all table rows pass explicit strings; Nit-level cleanup |
non-ACP row should use marker '1' to pin isAcpMode && conjunct |
wenshao (R2-2) | Confirmed observation, but agree with author it is beyond the current defect scope |
Gate comment should document exact '1' contract and stamp sites |
wenshao (R2-3) | Confirmed suggestion, but the contract is already documented in acp-channel-fallback.ts |
| No issues found | qwen-code-ci-bot | Confirmed — no blocking defects identified independently |
Additional Audit Coverage
Areas I independently checked that go beyond existing findings:
- Daemon producers: verified
packages/acp-bridge/src/spawnChannel.tsandpackages/cli/src/serve/channel-worker-supervisor.tsboth setQWEN_CODE_SERVE = '1'explicitly. - Consumer consistency: verified
resolveAcpChannelFallbackalready uses the exact=== '1'check, so the two consumers now match. - Straggler truthy checks: searched the codebase for other truthy checks of
QWEN_CODE_SERVE/QWEN_CODE_SERVE_ENV; none remain outside the now-fixed gate. - Project conventions: ESM, strict TypeScript, collocated tests, and no cross-package relative imports are all satisfied.
- Sibling consistency: the exact-match pattern is already established in
acp-channel-fallback.ts.
Final Verdict
APPROVE / Recommend Merge
The change correctly fixes the root cause (truthy vs. exact marker check), includes focused regression tests, and does not introduce new risks. Existing reviewer suggestions are valid Nits/cleanups that the author has already addressed or scoped out; none are blocking.
This review was generated by QoderWork AI
|
Released in v0.21.9. |
…ctions (#8857) * fix(cli): word review gap disclosures so they cannot read as contradictions A posted review body on #8811 opened "Reviewed. Not reviewed: This PR narrows the daemon-marker check from a truthy tes..." and readers could not tell whether the PR had been reviewed at all. The opener certified a part of the diff while the disclosure named an agent that never opened it — both true, but the pair read as a self-contradiction, and the disclosure subject (a launch-prompt fragment truncated mid-word) looked like a claim about the whole PR. - When "Not reviewed:" clauses follow, the opener now says "Reviewed, with gaps disclosed below." The certifying and zero-certified openers keep their exact wording. - Agent-derived subjects (the truncated prompt line used when an agent is tied to no chunk) render quoted — they are names, not sentences. - That truncation stops at a word boundary instead of mid-word. Verdicts, caps, dedup and the certification check are untouched: the internal subject stays the unquoted label and every structural check keys on it. * fix(cli): open gap-carrying review bodies as partial, and de-translate one reason Feedback on the first wording: "Reviewed, with gaps disclosed below." still starts with the word the "Not reviewed:" clauses take back, and readers had to reconcile the pair anyway. The opener now says the review is PARTIAL instead — "Partially reviewed — gaps disclosed below." — so opener and disclosures read in one direction (Chinese half: 仅完成部分审查，未覆盖部分见下方披露。). Also replaces the translationese Chinese rendering of the unopened-agent reason ("它被指向 diff 的行却从未打开") with natural phrasing. * fix(cli): keep review gap disclosures unambiguous * fix(review): preserve distinct agent labels * fix(review): normalize agent label spacing * fix(review): keep incomplete review openers honest
5 participants
What this PR does
This PR requires
QWEN_CODE_SERVEto equal the literal value1before applying daemon-only loader environment cleanup. It also extends the existing ACP startup regression table so0andfalseare verified as ordinary non-daemon values.Why it's needed
The channel attribution fallback already accepts only
QWEN_CODE_SERVE=1, but the loader environment cleanup still used a truthy check. As a result, a direct ACP launch withQWEN_CODE_SERVE=0orQWEN_CODE_SERVE=falsereportedchannel=ACPwhile unexpectedly removingNODE_OPTIONSandNODE_PATH. This aligns both consumers on the same exact marker contract. Normal daemon launches are unchanged because every in-repository daemon producer writes1explicitly.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/gemini.test.tsxand confirm all startup tests pass, including the0andfalsemarker cases.cd packages/cli && npx vitest run src/config/acp-channel-fallback.test.tsand confirm attribution still rejects non-1marker values.QWEN_CODE_SERVE=0orfalsepreserves them.Evidence (Before & After)
Before: the direct-ACP regression test failed with
expected undefined to be '--import file:///other-checkout/register.mjs'when the ambient marker was0orfalse.After: the complete startup test file passes with explicit coverage for both values.
Tested on
Environment (optional)
Node.js 22, local unit tests and repository build.
Risk & Scope
1.Linked Issues
Follow-up to #8712 and #8663. Related to #8660.
中文说明
这个 PR 做了什么
本 PR 要求
QWEN_CODE_SERVE必须严格等于字面量1，才执行 daemon 专属的 loader 环境清理。同时扩展现有 ACP 启动回归测试，确认0和false都按普通非 daemon 值处理。为什么需要
channel 归因回退已经只接受
QWEN_CODE_SERVE=1，但 loader 环境清理仍使用 truthy 判断。因此，直接 ACP 启动如果设置了QWEN_CODE_SERVE=0或QWEN_CODE_SERVE=false，会上报channel=ACP，却意外删除NODE_OPTIONS和NODE_PATH。本次修复让两个 consumer 遵循同一个精确 marker 约定。正常 daemon 启动不受影响，因为仓库内所有 daemon 生产者都会显式写入1。Reviewer Test Plan
如何验证
cd packages/cli && npx vitest run src/gemini.test.tsx，确认全部启动测试通过，包括0和falsemarker 用例。cd packages/cli && npx vitest run src/config/acp-channel-fallback.test.ts，确认归因逻辑仍拒绝非1marker。QWEN_CODE_SERVE=0或false的直接 ACP 启动会保留这些变量。前后对比证据
修复前：ambient marker 为
0或false时，直接 ACP 回归测试失败，错误为expected undefined to be '--import file:///other-checkout/register.mjs'。修复后：完整启动测试文件通过，并显式覆盖这两个值。
测试环境
环境（可选）
Node.js 22，本地单元测试与仓库构建。
风险与范围
1。关联 Issue
#8712 和 #8663 的后续修复，与 #8660 相关。