fix(cli): require exact attribution marker values - #8712
Merged
Conversation
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the quick follow-up!
Moving on to code review. 🔍 中文说明感谢这个及时的 follow-up！
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewIndependent baseline first: for "truthy marker check accepts disabled values like Verified against the tree:
No blockers, no convention violations. A focused 2-file change with nothing extra, so the diagram and files-table enrichments are skipped. Testing — the PR's own CI, via API (PR code never executed locally)The Linux unit suite is still running on the reviewed commit; macOS/Windows, integration, tmux, and verify checks were skipped by this PR's precheck gates. The new parametrized cases are load-bearing: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified: live runtime attribution — covered by the pending unit suite; nothing user-visible on a TUI surface, so no real-scenario run on this CI path. The author's claim of 332 passing tests on macOS arm64 is their own report, not independently re-run here. 中文说明代码审查先给出独立基线：对"truthy 判断误接受 已在代码树中核实：
无阻塞项、无规范违规。两文件聚焦改动、无多余内容，故省略时序图与文件表。 测试——引自 PR 自身 CI（未在本地执行 PR 代码）Linux 单元测试仍在运行；macOS/Windows、集成、tmux 与 verify 检查被本 PR 的预检门控跳过。新增参数化用例是"承重"的： 未验证：运行时实际归因——由等待中的单元测试覆盖；无用户可见 TUI 变化，CI 路径不做真实场景实测。作者所称 macOS arm64 上 332 个测试通过为其自述，未在此独立复跑。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage: demonstrated problem, minimal diff matching the documented launcher contract, load-bearing regression tests. Stepping back: this is exactly what a follow-up fix should look like. #8670 shipped the attribution markers with a truthy read; this tightens the read to the exact value the launchers actually write, and does nothing else. The approach matches my independent proposal, every affected consumer is nameable (one call site in The one open item is CI, not the code: the Linux unit suite is still running on the reviewed commit, so approval is deferred until CI lands green on 中文说明置信度：5/5 —— 各阶段全部干净：问题已证实，改动最小且与文档化的启动器契约一致，回归测试具备"承重"作用。 整体看：这正是 follow-up 修复该有的样子。#8670 引入归因标记时使用了 truthy 判断；本 PR 将判断收紧为启动器实际写入的精确值，且仅此而已。方案与我的独立提案一致；所有受影响的消费方均可点名（ 唯一未决项是 CI 而非代码：Linux 单元测试仍在运行，因此批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these changes
Aug 8, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
yiliang114
enabled auto-merge
August 8, 2026 16:16
doudouOUC
approved these changes
Aug 9, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Approving — reviewed at 6df400d367. C=0 (no review threads on this PR at all).
Two production lines, so the only question worth asking is the one the change turns on: does every producer of these markers write exactly '1'? A strict === '1' is a regression the moment one writer uses true, yes, or anything else. I enumerated all of them rather than assuming.
Every write site verified
QWEN_CODE_SERVE and QWEN_CODE_DESKTOP are set in exactly three places across the repo, and all three write the literal '1':
| Producer | Value |
|---|---|
packages/acp-bridge/src/spawnChannel.ts:152 |
childEnv['QWEN_CODE_SERVE'] = '1' |
packages/cli/src/serve/channel-worker-supervisor.ts:382 |
env['QWEN_CODE_SERVE'] = '1' |
packages/desktop-shell/src-tauri/src/runtime.rs:57 |
.env("QWEN_CODE_DESKTOP", "1") |
The sandbox path forwards rather than re-derives — env[envVar] === undefined ? [] : ['--env', \${envVar}=${env[envVar]}`]insandbox.ts— so the value reaches a container verbatim and stays'1'. The design doc is consistent with the same semantics throughout (QWEN_CODE_DESKTOP=1, QWEN_CODE_SERVE=1, and the attribution table keyed on =1`). No producer, doc, or pass-through disagrees with the new check.
What it fixes
Under truthiness, QWEN_CODE_SERVE=0 and =false are both non-empty strings and therefore attributed as daemon — the opposite of what an operator setting 0 intends. The test change captures exactly that, widening ignores empty marker values into it.each(['', '0', 'false']), which is the right generalisation: the old test only pinned the empty case, so '0' and 'false' were the untested half of the same predicate.
Worth noting the layering here is sound: #8663 separately keeps these two keys out of project-scoped .env/settings.env so a workspace cannot forge attribution at all, and this PR makes the reader strict about what it accepts. Belt and braces on the same surface, from opposite directions.
The blast radius is telemetry attribution only, and the failure mode of a mis-set marker is now a fall back to ACP — the conservative direction.
Tests
6/6 pass locally at this commit. CI is fully green on this head, including Test (ubuntu-latest, Node 22.x), both Desktop Shell legs and the web-shell smoke.
Nothing further from me.
中文说明
批准 —— 审查提交 6df400d367，C=0（本 PR 没有任何评审线程）。
只有两行生产代码，因此唯一值得追问的就是这次改动所依赖的那件事：这两个标记的每一个写入方是否都恰好写 '1'？ 一旦有任何写入方用 true、yes 或别的值，严格的 === '1' 立刻变成回归。我把它们全部枚举了，而非假定。
全部写入点已核实：QWEN_CODE_SERVE 与 QWEN_CODE_DESKTOP 在整个仓库中恰好有三处设置，且三处都写字面量 '1'——packages/acp-bridge/src/spawnChannel.ts:152、packages/cli/src/serve/channel-worker-supervisor.ts:382、以及 Rust 侧 packages/desktop-shell/src-tauri/src/runtime.rs:57 的 .env("QWEN_CODE_DESKTOP", "1")。
sandbox 路径是透传而非重新推导（sandbox.ts 中 env[envVar] === undefined ? [] : ['--env', \${envVar}=${env[envVar]}`]），因此值原样进入容器、仍为 '1'。设计文档通篇也是同一语义（QWEN_CODE_DESKTOP=1、QWEN_CODE_SERVE=1，归因表以 =1` 为键）。没有任何写入方、文档或透传路径与新的判断相冲突。
它修的是什么：按真值判断，QWEN_CODE_SERVE=0 与 =false 都是非空字符串、因而被归因为 daemon，与运维设置 0 的本意正好相反。测试改动准确地覆盖了这一点，把 ignores empty marker values 扩展为 it.each(['', '0', 'false'])——这是正确的泛化方向：旧测试只钉住了空串，'0' 与 'false' 恰是同一谓词中未被测试的那一半。
值得一提的是这里的分层是合理的：#8663 另外把这两个键排除在项目级 .env/settings.env 之外，使工作区根本无法伪造归因；本 PR 则让读取侧对所接受的值变严格。同一面上从两个相反方向各加一道。
影响面仅限遥测归因，且标记设置错误时的失效方向现在是回退到 ACP——保守方向。
测试：本提交下本地 6/6 通过；CI 在本 head 全绿，含 Test (ubuntu-latest, Node 22.x)、两个 Desktop Shell 腿与 web-shell smoke。
我没有其他意见。
Collaborator
|
Released in v0.21.9. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR treats the daemon and desktop attribution markers as enabled only when their value is exactly
1. Values such as0andfalsenow fall back toACPinstead of reportingdaemonordesktop.Why it's needed
#8670 introduced
QWEN_CODE_SERVE=1andQWEN_CODE_DESKTOP=1as internal launcher markers, but the fallback checked them by JavaScript truthiness. A non-empty disabled value such asQWEN_CODE_DESKTOP=0therefore reported the session asdesktop. The trusted launchers already write the exact value1, so matching that contract fixes the misattribution without changing any supported launch path.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/config/acp-channel-fallback.test.ts src/config/config.test.ts; expect 332 tests to pass.QWEN_CODE_SERVE=1still resolves todaemon,QWEN_CODE_DESKTOP=1still resolves todesktop, and explicit--channelbehavior is unchanged.0, andfalsemarker values resolve toACP.Evidence (Before & After)
Before: the new regression cases failed because
0andfalseresolved todesktop. After: all six fallback cases and the full neighboring config suite pass.Tested on
Environment (optional)
Unit tests on macOS arm64. CLI typecheck, targeted ESLint and Prettier checks, and the CLI package build also pass.
Risk & Scope
1marker values no longer opt into attribution; repository launchers already use1exclusively.Linked Issues
Follow-up to #8670 and #8660.
中文说明
本 PR 做了什么
本 PR 仅在 daemon 与 desktop 归因标记的值精确等于
1时将其视为启用。0、false等值现在会回退为ACP，而不会误报为daemon或desktop。为什么需要
#8670 引入了
QWEN_CODE_SERVE=1和QWEN_CODE_DESKTOP=1作为内部启动器标记，但回退逻辑使用 JavaScript truthy 判断。因此，QWEN_CODE_DESKTOP=0这类非空禁用值仍会把会话上报为desktop。可信启动器本来就只写入精确值1，因此按该契约匹配可以修复误归因，同时不改变任何受支持的启动路径。评审测试计划
如何验证
cd packages/cli && npx vitest run src/config/acp-channel-fallback.test.ts src/config/config.test.ts，预期 332 个测试全部通过。QWEN_CODE_SERVE=1仍解析为daemon，QWEN_CODE_DESKTOP=1仍解析为desktop，显式--channel行为不变。0和false均解析为ACP。前后对比证据
修改前：新增回归用例失败，因为
0与false被解析为desktop。修改后：六个 fallback 用例及完整的相邻 config 测试均通过。测试环境
环境（可选）
在 macOS arm64 上运行单元测试。CLI typecheck、针对改动文件的 ESLint 与 Prettier 检查，以及 CLI package build 也均通过。
风险与范围
1marker 不再启用归因；仓库内的启动器始终只使用1。关联 Issue
#8670 与 #8660 的 follow-up。