fix(review): the sandbox default named an image that does not exist - #9972
Conversation
`review.sandbox: auto|required` defaulted to
`ghcr.io/qwenlm/qwen-code/sandbox:latest`, and the comment beside it claimed
that was "the same image `qwen --sandbox` uses". Neither half was true. The
CLI's image comes from its own manifest — `config.sandboxImageUri`,
`ghcr.io/qwenlm/qwen-code:<version>` — a different repository path and a
pinned tag rather than a floating one. And the hardcoded name does not
resolve: an anonymous manifest request answers 403 where the real one answers
200, so every command of an opted-in review failed at image pull. Default is
`off`, so nothing broke for anyone who had not opted in; for anyone who had,
the feature could not work at all.
Read the manifest instead, so the two cannot drift, and honour an operator's
own `QWEN_SANDBOX_IMAGE` rather than ignoring their configuration and pulling
a second image. Through the same file-sourced guard as the other picks: a
repository must not choose the image its own code runs in.
The test that let this through asserted `toContain('sandbox')` — a substring
of the name rather than the property. The broken value contains the word; the
correct one does not, so the assertion passed on the defect and would have
failed on the fix. It now pins the manifest.
Found by doing what nineteen rounds of argv-level review could not: starting
a container from that argv. With the merged default, all five arms of a live
probe fail before anything runs; with this one they pass — the gate admits the
phase, the workload runs in the image, the env is the allowlist and not the
host's, `--network none` holds for test while install keeps the network,
writes land host-owned in the mount, and no container survives the run.
|
Re-run after Template: now literally complete — the body was rewritten to Problem: observed, not theoretical — and re-verified independently in this run, not just last time: an anonymous ghcr.io manifest probe answers 200 for Direction: aligned — repairs a default that an already-merged feature shipped with; no product-direction question. Size: 2 files, +158/−12 — production logic 90 lines ( Approach: still converges with my independent proposal — read Risk: no elevated-revert-path signal (no changed file matches the high-risk patterns). Containment-adjacent code, so the pick-chain semantics were re-checked in the code review below. Moving on to code review. 🔍 中文说明在 模板：现已逐字完整——正文已按 问题：已观测到，不是理论问题——且本轮重新独立核实，不是只引用上次结论：匿名 ghcr.io manifest 探测对 方向：对齐——修复已合并功能交付时即损坏的默认值，无产品方向疑问。 规模：2 个文件，+158/−12——生产逻辑 90 行（ 方案：仍与我的独立方案收敛——从 CLI 自己的 manifest 读取 风险：未命中高回滚风险路径信号。因属容器隔离相关代码，pick 链语义已在下方代码审查中复核。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-reviewed at
One recorded non-blocker, carried over from Test evidenceUnattended CI run — no PR code was built or executed. Evidence is the PR's own CI read through the API, plus static probes I ran myself this run: the anonymous ghcr.io manifest probe answering 200 for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 All PR CI workflow runs on this head completed green — the ubuntu unit suite included, which was still in flight during the last pass. The macOS/Windows test and integration arms are skipped by the repo's own gating for this PR shape; nothing red anywhere. Sandboxed verification would settle the one claim static review cannot, and it is already running: a 中文说明代码审查：已在新 head 一项记录在案的非阻塞项，承 测试证据：无人值守 CI 运行——未构建或执行任何 PR 代码。证据来自 PR 自己的 CI（经 API 读取），加我本轮亲自运行的静态探测：匿名 ghcr.io manifest 请求对 沙箱验证可以落定静态审查看不到的唯一论断，而且已在运行：一次 — Qwen Code · qwen3.8-max Reviewed at |
|
正文已按 @qwen-code /triage |
|
Confidence: 4/5 — clean across every stage on the new head; the residue is one claim only a live container can prove, and a Going back to my independent proposal one more time: the PR converges with it, and this round it went further than the proposal asked. The question I would have left hanging — "how do you test a default whose fallback string-equals its primary value?" — got a real answer: inject the reader, pin both branches by mechanism, pin the ordering, and reject the exact shapes of the original defect. That is the difference between a test that passes on the fix and a test that would have failed on the bug, and this file's history shows why that matters. Every one of the 90 production lines earns its place; both prior nits are fixed without being asked twice; the parity caveat is documented where a future reader will actually stand. What keeps it at 4 rather than 5 is one honest gap, not a doubt about the code: the load-bearing behavioural claim — a container actually boots from this argv — still rests on the author's five-arm table plus my registry probe (200 vs 404, re-run this pass). The unit suite pins resolution, not execution. A In six months' terms: a memoized sync reader with an honest fallback, a precedence chain where every pick is gated, and comments that describe the code instead of its intentions. I would thank whoever shipped this. Approving now, pinned to the reviewed commit — all PR CI on this head is green, nothing pending. 中文说明置信度：4/5 —— 新 head 上每个阶段都干净；剩下的只是一个只有真机容器才能证明的论断，而已有一次 再次回到我的独立方案：PR 与之收敛，且本轮走在了方案要求的前面。我本来会悬而未决的问题——"兜底值与主值字符串相等时，如何测试这个默认值？"——得到了真正的回答：注入 reader，按机制钉住两条分支，钉住顺序，并拒绝原始缺陷的确切形状。这是"在修复上通过的测试"与"会在缺陷上失败的测试"之间的差别，而这个文件的历史恰好说明了为什么这很重要。90 行生产代码每一行都有用途；上轮两处小问题未被追问第二次就已修好；parity 的取舍被写在了未来读者真正会站的位置。 之所以是 4 而不是 5，是一个诚实的缺口，而非对代码的怀疑：承重的行为论断——容器真的由该 argv 启动——目前仍建立在作者的五臂真机表加我本轮重跑的注册表探测（200 对 404）之上。单测锁定的是解析，不是执行。一次 以六个月后的视角看：一个带诚实兜底的记忆化同步读取器，一条每个取值都过闸门的优先级链，以及描述代码而非意图的注释。我会感谢交出这份代码的人。 现在批准，锁定被审提交——该 head 上所有 PR CI 均为绿，无待决项。 — Qwen Code · qwen3.8-max Reviewed at |
Three placement and accuracy fixes on the block this change touched, no behaviour. `DEFAULT_IMAGE` had ended up with two stacked doc blocks that disagreed: the older one still called it "the fallback when neither override names an image", which stopped being true the moment the manifest lookup landed in front of it. Folded into one. `reviewSandboxImage`'s doc block had been detached from its own declaration by the helper and its cache slot landing between them. The helper now sits above the block it is referenced from. And the block's own claim — "the same image `qwen --sandbox` uses" — was written when a hardcoded name made it false. It is true now, which is exactly why it should say what the code does rather than what it was hoping for.
|
两条注释位置问题都修了（
关于你点名的那个有意行为变更：设置了 另外记录一句：五臂活体验证是我自己跑的，你没有独立复跑——这一点你在 stage 2 里已如实披露，我同意那是本 PR 证据链上唯一未被第二方复核的部分。 |
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
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): src/commands/review/lib/sandboxed-exec.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
Test Plan（非阻断）：src/commands/review/lib/sandboxed-exec.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // The operator's own sandbox image, if they configured one for | ||
| // `qwen --sandbox`. Through `pick`, so a repository shipping it in its | ||
| // `.qwen/.env` cannot choose the image its own code runs in. | ||
| pick('QWEN_SANDBOX_IMAGE') || |
There was a problem hiding this comment.
[Suggestion] The file-sourced provenance guard on this line has no test pinning it for the newly honoured key. The sibling key QWEN_REVIEW_SANDBOX_IMAGE has a dedicated rejection test ('ignores a repo-shipped image override — the image IS the code') that spies isFileSourcedEnvKey and asserts the attacker image is ignored, but QWEN_SANDBOX_IMAGE's reject path has no twin. A verified mutant shows the cost: replacing pick('QWEN_SANDBOX_IMAGE') with a plain env['QWEN_SANDBOX_IMAGE']?.trim() fails the guard probe (expected the result not to contain attacker.example, received attacker.example/rogue:1) while the entire existing suite stays green — 36 passed | 1 skipped, identical to baseline. If such a regression ships, a reviewed repository commits QWEN_SANDBOX_IMAGE=attacker.example/rogue:1 in its .qwen/.env and the review container pulls and runs the attacker's image. Mirror the sibling test:
it('ignores a repo-shipped QWEN_SANDBOX_IMAGE too', () => {
vi.stubEnv('QWEN_SANDBOX_IMAGE', 'attacker.example/rogue:1');
const spy = vi
.spyOn(environment, 'isFileSourcedEnvKey')
.mockImplementation((k) => k === 'QWEN_SANDBOX_IMAGE');
try {
expect(reviewSandboxImage()).not.toContain('attacker.example');
} finally {
spy.mockRestore();
vi.unstubAllEnvs();
}
});中文说明
这一行上的文件来源守卫对新引入的键没有任何测试来钉住。兄弟键 QWEN_REVIEW_SANDBOX_IMAGE 有专门的拒绝测试（'ignores a repo-shipped image override — the image IS the code'）：spy isFileSourcedEnvKey 并断言攻击者镜像被忽略；但 QWEN_SANDBOX_IMAGE 的拒绝路径没有孪生测试。验证过的变异体展示了代价：把 pick('QWEN_SANDBOX_IMAGE') 换成普通的 env['QWEN_SANDBOX_IMAGE']?.trim()，守卫探针会失败（期望结果不包含 attacker.example，实际收到 attacker.example/rogue:1），而现有整套测试依旧全绿——36 通过 | 1 跳过，与基线完全一致。如果这样的回归被合入，被审查的仓库就可以在其 .qwen/.env 中提交 QWEN_SANDBOX_IMAGE=attacker.example/rogue:1，审查容器将拉取并运行攻击者的镜像。建议仿照兄弟测试补一条（代码见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ), | ||
| ) as { config?: { sandboxImageUri?: string } }; | ||
| expect(manifest.config?.sandboxImageUri).toBeTruthy(); | ||
| expect(reviewSandboxImage({})).toBe(manifest.config?.sandboxImageUri); |
There was a problem hiding this comment.
[Suggestion] This pin cannot detect the removal or disablement of cliSandboxImage() — the core mechanism this PR adds — because the fallback DEFAULT_IMAGE currently evaluates to the exact same string as the manifest field (ghcr.io/qwenlm/qwen-code:0.22.0; CLI_VERSION is generated from the root package version). Deleting cliSandboxImage() || from the chain, stubbing it to return undefined, or reading a wrong field key all keep the suite green: the result falls through to DEFAULT_IMAGE, which string-equals the manifest. Verified by probe: mocking read-package-up to return manifest-image:test, the assertion passes on clean code and fails on the deletion mutant (expected manifest-image:test, received ghcr.io/qwenlm/qwen-code:0.22.0) with the existing suite still green. The two literals are maintained separately — once they diverge (a version bump landing around a field edit, an image repo rename), review silently runs a different image than qwen --sandbox. Add a mocked variant whose value the fallback can never equal, keeping this integration-style test alongside (mind the module-level manifestImage cache — use vi.resetModules() + dynamic import):
vi.mock('read-package-up', () => ({
readPackageUpSync: () => ({
packageJson: { config: { sandboxImageUri: 'manifest-image:test' } },
}),
}));
// with module isolation:
expect(reviewSandboxImage({})).toBe('manifest-image:test');中文说明
这条钉住语句无法检测 cliSandboxImage()（本 PR 的核心机制）被删除或禁用：兜底值 DEFAULT_IMAGE 目前的求值结果与 manifest 字段的字符串完全相同（ghcr.io/qwenlm/qwen-code:0.22.0；CLI_VERSION 由根包版本生成）。因此，把 cliSandboxImage() || 从链上删掉、让它返回 undefined、或读错字段名，整套测试仍然是绿的：结果会落到 DEFAULT_IMAGE，与 manifest 字符串相等。探针验证：把 read-package-up mock 成返回 manifest-image:test 后，该断言在干净代码上通过，在删除变异体上失败（期望 manifest-image:test，实际收到 ghcr.io/qwenlm/qwen-code:0.22.0），而现有套件保持绿色。这两个字面量是分开维护的——一旦将来不一致（版本升级与字段修改交错落地、镜像仓库改名），审查就会静默运行与 qwen --sandbox 不同的镜像。建议增加一个 mock 变体，其值是兜底字面量永远不可能等于的，同时保留现有这条集成式测试（注意 manifestImage 是模块级缓存，需要 vi.resetModules() + 动态 import 隔离；代码见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * for anyone who needs reproducibility. | ||
| */ | ||
| const DEFAULT_IMAGE = 'ghcr.io/qwenlm/qwen-code/sandbox:latest'; | ||
| const DEFAULT_IMAGE = `ghcr.io/qwenlm/qwen-code:${CLI_VERSION}`; |
There was a problem hiding this comment.
[Suggestion] The last-resort literal this diff changes has zero test coverage: no test makes cliSandboxImage() fail (it reads the real filesystem via readPackageUpSync with no injection seam), and the manifest always resolves in the test layout, so this branch never executes. A verified mutant shows the hole: reverting DEFAULT_IMAGE to the broken ghcr.io/qwenlm/qwen-code/sandbox:latest leaves the entire new suite green — only a probe forcing the fallback (mocking read-package-up to return undefined) catches it (expected ghcr.io/qwenlm/qwen-code:0.22.0, received ghcr.io/qwenlm/qwen-code/sandbox:latest). If a future edit reintroduces an unresolvable name here — the exact bug class this PR fixes — the suite stays green, and on an install where the manifest genuinely cannot be found (the unusual install layout this literal exists for), every command of an opted-in review fails at image pull again. Add a seam and pin the fallback:
// mock read-package-up → undefined (or add a test-only reset of manifestImage), then:
expect(reviewSandboxImage({})).toBe(`ghcr.io/qwenlm/qwen-code:${CLI_VERSION}`);中文说明
这个 diff 修改的兜底字面量没有任何测试覆盖：没有任何测试能让 cliSandboxImage() 失败（它通过 readPackageUpSync 读取真实文件系统，没有注入缝隙），而测试布局中 manifest 总能解析成功，所以兜底分支从不执行。验证过的变异体展示了这个空洞：把 DEFAULT_IMAGE 退回坏值 ghcr.io/qwenlm/qwen-code/sandbox:latest，新套件依旧全绿——只有强制走兜底的探针（mock read-package-up 使其返回 undefined）才能抓住它（期望 ghcr.io/qwenlm/qwen-code:0.22.0，实际收到 ghcr.io/qwenlm/qwen-code/sandbox:latest）。如果未来某次修改在这里重新引入一个无法解析的名字——正是本 PR 修复的那类缺陷——套件会保持绿色，而在 manifest 确实找不到的安装布局（这个字面量为之存在的非常规安装布局）上，所有开启沙箱的审查命令会再次在拉镜像时失败。建议加一个测试缝隙并钉住兜底值（代码见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(reviewSandboxImage({})).toContain('sandbox'); | ||
| // The operator's own sandbox image, if they configured one for | ||
| // `qwen --sandbox`, rather than ignoring it and pulling a second one. | ||
| expect(reviewSandboxImage({ QWEN_SANDBOX_IMAGE: 'theirs:2' })).toBe( |
There was a problem hiding this comment.
[Suggestion] No test sets two override variables at once, so any reordering of the chain survives the whole suite. Verified: with QWEN_REVIEW_SANDBOX_IMAGE=mine:1 and QWEN_SANDBOX_IMAGE=theirs:2 both set, clean code returns mine:1; moving the pick('QWEN_SANDBOX_IMAGE') line above the review-specific overrides returns theirs:2 (expected mine:1, received theirs:2) while every existing test still passes. If such a reorder ships, an operator's QWEN_SANDBOX_IMAGE silently outranks a repository's QWEN_REVIEW_SANDBOX_IMAGE toolchain override, and the review runs in an image missing the toolchain the repository declared it needs, breaking or skewing the reviewed build. Add one assertion with both set (and, if cheap, the QWEN_CODE_CUSTOM_SANDBOX_IMAGE pair):
expect(
reviewSandboxImage({
QWEN_REVIEW_SANDBOX_IMAGE: 'mine:1',
QWEN_SANDBOX_IMAGE: 'theirs:2',
}),
).toBe('mine:1');中文说明
没有任何测试同时设置两个覆盖变量，因此链上的任意重排都能通过整套测试。已验证：同时设置 QWEN_REVIEW_SANDBOX_IMAGE=mine:1 与 QWEN_SANDBOX_IMAGE=theirs:2 时，干净代码返回 mine:1；把 pick('QWEN_SANDBOX_IMAGE') 这一行移到两个审查专用覆盖之上会返回 theirs:2（期望 mine:1，实际收到 theirs:2），而现有测试全部通过。如果这种重排被合入，操作者的 QWEN_SANDBOX_IMAGE 会静默压过仓库声明的 QWEN_REVIEW_SANDBOX_IMAGE 工具链覆盖，审查将运行在缺少仓库所需工具链的镜像里，导致被审查的构建失败或结果偏差。建议补一条同时设置两个变量的断言（代码见上方英文部分；成本允许的话把 QWEN_CODE_CUSTOM_SANDBOX_IMAGE 的配对也加上）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| return ( | ||
| pick('QWEN_REVIEW_SANDBOX_IMAGE') || | ||
| pick(CUSTOM_SANDBOX_IMAGE_ENV_VAR) || | ||
| // The operator's own sandbox image, if they configured one for |
There was a problem hiding this comment.
[Suggestion] The parity this comment promises does not hold when the operator configured their image in a user-level dotenv file — ~/.qwen/.env, ~/.env, or user settings.env — channels qwen --sandbox honours. loadEnvironment marks every applied key as file-sourced with no home-scoped exemption (the tracking sets never carry the homeScoped classification the loader already computes), so pick discards the value and review silently falls back to the manifest default image. Verified by a four-arm probe on unmodified code: in the user-dotenv and user-settings-env arms, qwen --sandbox resolves registry.example/qwen-custom:1 while review resolves ghcr.io/qwenlm/qwen-code:0.22.0; the shell-export arm keeps parity; the repo-dotenv arm is correctly rejected. Not a blocker — the fallback is the CLI's own shipped image, and exporting in the shell is a tested workaround — but the divergence is silent while the comment asserts the opposite. The cheaper fix is narrowing the comment; recording home-scoped provenance separately in environment.ts and exempting it for this key fixes the dotenv arm without weakening the repo guard, though the settings.env arm does not flip until the settings merge carries per-scope provenance:
// The operator's own sandbox image, if they exported it in their shell.
// File-loaded values — including the operator's own `~/.qwen/.env` — are
// dropped by the provenance gate below; export the variable to keep review
// and `qwen --sandbox` on the same image.中文说明
当操作者把镜像配置在用户级 dotenv 文件（~/.qwen/.env、~/.env 或用户 settings.env）里时，这条注释承诺的对齐并不成立——而这些都是 qwen --sandbox 尊重的配置渠道。loadEnvironment 会把每个生效的键都标记为文件来源，且没有家目录作用域的豁免（跟踪集合从不携带加载器已经计算出的 homeScoped 分类），所以 pick 会丢弃该值，审查静默退回 manifest 默认镜像。在未修改代码上做的四臂探针验证：用户级 dotenv 与用户级 settings.env 两臂中，qwen --sandbox 解析出 registry.example/qwen-custom:1，而审查解析出 ghcr.io/qwenlm/qwen-code:0.22.0；shell 导出渠道对齐正常；仓库级 .env 被正确拒绝。这不是阻塞项——兜底是 CLI 自带的镜像，shell 导出是已测试的变通办法——但分歧是静默的，而注释断言的恰恰相反。更便宜的修法是收窄注释；在 environment.ts 中单独记录家目录作用域的来源并对本键豁免，可以在不削弱仓库守卫的前提下修复 dotenv 渠道——不过 settings.env 渠道在设置合并支持按作用域记录来源之前不会翻转（代码见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Three mutants the new suite let through, all found by the review's own probes. The headline pin could not see the mechanism it was named for. `DEFAULT_IMAGE` takes its tag from `CLI_VERSION`, generated from the same manifest version, so the fallback string-equals the manifest field today — delete the manifest lookup and the result falls through to a literal that compares equal, suite green. The reader is now injectable, which is the only way the two can be told apart, and both branches are asserted: a distinct value proves the manifest is consulted, and its absence proves the fallback names something that resolves. That fallback branch had no coverage at all, which is how reverting it to the unpullable name — the exact defect this PR fixes — also shipped green. And no test set two overrides at once, so any reordering of the chain survived. Order is the content here: the reviewed repository's own `QWEN_REVIEW_SANDBOX_IMAGE` has to outrank the operator's general `QWEN_SANDBOX_IMAGE`, or the build runs in an image missing the toolchain the repository declared it needs. Also names the channel where the parity claim does not hold: an operator who set the image in a user-level `~/.qwen/.env` or `settings.env` loses it here, because the loader records what it applied from a file without distinguishing the user's own from the reviewed repository's. Shell export keeps parity. Widening it is a change to the loader, not to this pick.
|
@qwen-code /takeover |
|
四条建议全部处理了， 我的头条测试钉不住我自己新增的机制你是对的，而且理由比「缺一条断言」更难堪： 修法是把 reader 做成可注入——这是唯一能把两者分辨开的办法——然后两条分支都断言：给一个不同的值证明 manifest 确实被查询，给 兜底分支零覆盖，正是本 PR 所修缺陷的同一个位置你的变异证明了这一点：把 顺序无人钉也成立。加了三对同时设置的断言。顺序在这里就是全部内容：仓库自己的 三个此前全部漏网的变异，现在三次全红。 parity 那条机制我认，而且这正是第 8 轮就记录过的已知代价：加载器把它从文件里应用的每个键都记为 file-sourced，不区分是用户自己的还是被审仓库的，而这一侧不能赌自己拿到的是哪一种。我没有改行为——把它写进注释、点名那个不成立的通道（用户级 你复现不了的那条命令
|
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.ts:720 — [review] cliSandboxImage() is a second manifest reader parallel to getPackageJson(); the static import also eager-loads its dependency chain (+226 KB startup closure, measured)
中文说明
已审查——无阻断问题。 建议见行内评论。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // The operator's own sandbox image, if they configured one for | ||
| // `qwen --sandbox`. Through `pick`, so a repository shipping it in its | ||
| // `.qwen/.env` cannot choose the image its own code runs in. | ||
| pick('QWEN_SANDBOX_IMAGE') || |
There was a problem hiding this comment.
[Suggestion] R1-1: The file-sourced rejection of the newly honoured QWEN_SANDBOX_IMAGE channel still has no test pinning it — this round-1 finding still stands (the round-1 response addressed the other four suggestions, not this one). The sibling key QWEN_REVIEW_SANDBOX_IMAGE has a dedicated rejection test ('ignores a repo-shipped image override — the image IS the code') that spies isFileSourcedEnvKey; QWEN_SANDBOX_IMAGE appears in the suite only in honour-path and ordering tests (sandboxed-exec.test.ts lines 925, 936, 942). The guard itself works — the gap is that nothing fails if it goes away: replacing pick('QWEN_SANDBOX_IMAGE') with a plain env['QWEN_SANDBOX_IMAGE']?.trim() ships green (measured this round: 37 passed | 1 skipped, identical to baseline), while an attack probe — stubbing QWEN_SANDBOX_IMAGE=attacker.example/rogue:1 with isFileSourcedEnvKey mocked true for that key — fails under the mutation (expected 'attacker.example/rogue:1' not to contain 'attacker.example') and passes on the restored code. If such a regression ships, a reviewed repository that commits QWEN_SANDBOX_IMAGE=attacker.example/rogue:1 in its .qwen/.env chooses the image its own review code executes in. Mirror the sibling test inside describe('values a repository must not be able to set'):
it('ignores a repo-shipped QWEN_SANDBOX_IMAGE too', () => {
vi.stubEnv('QWEN_SANDBOX_IMAGE', 'attacker.example/rogue:1');
const spy = vi
.spyOn(environment, 'isFileSourcedEnvKey')
.mockImplementation((k) => k === 'QWEN_SANDBOX_IMAGE');
try {
expect(reviewSandboxImage()).not.toContain('attacker.example');
} finally {
spy.mockRestore();
vi.unstubAllEnvs();
}
});中文说明
新引入的 QWEN_SANDBOX_IMAGE 渠道的「文件来源值拒绝」仍然没有任何测试钉住——这是第 1 轮就已提出的发现（R1-1），至今依然成立（第 1 轮的回复处理了其余四条建议，没有处理这一条）。兄弟键 QWEN_REVIEW_SANDBOX_IMAGE 有专门的拒绝测试（'ignores a repo-shipped image override — the image IS the code'）：spy isFileSourcedEnvKey 并断言攻击者镜像被忽略；而 QWEN_SANDBOX_IMAGE 在整套测试里只出现在生效路径与顺序断言中（sandboxed-exec.test.ts 第 925、936、942 行）。守卫本身是正确的——缺口在于：它即使失效也不会有任何测试变红。把 pick('QWEN_SANDBOX_IMAGE') 换成普通的 env['QWEN_SANDBOX_IMAGE']?.trim()，整套测试依旧全绿（本轮实测：37 通过 | 1 跳过，与基线完全一致）；而攻击探针——把 QWEN_SANDBOX_IMAGE stub 成 attacker.example/rogue:1 并让 isFileSourcedEnvKey 对该键返回 true——在变异体上失败（期望不包含 attacker.example，实际收到 attacker.example/rogue:1），在还原后的代码上通过。如果这样的回归被合入，被审查的仓库就可以在其 .qwen/.env 中提交 QWEN_SANDBOX_IMAGE=attacker.example/rogue:1，从而选择审查代码实际运行的镜像。建议在 describe('values a repository must not be able to set') 中仿照兄弟测试补一条（代码见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /takeover stop |
qwen-code-dev-bot
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
@qwen-code /takeover stop |
qwen-code-dev-bot
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 36 passed · 0 failed · 36 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：36 通过 · 0 失败 · 36 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9972 Deep Verification — fix(review): the sandbox default named an image that does not existVerdict: 中文摘要结论：
ScopeCentral claim: with The diff is two files ( A/B: image resolution (central claim)Same 10 cells driven through the real code on both arms — head through the compiled
10/10 assertions pass on each arm. Base produced the unpullable literal in every default-path cell; head resolved the manifest field. Dist-layout sentinel probe. The compiled module lives under
S1 alone cannot discriminate (the two sources string-equal today — exactly the equivalence the PR documents); S2 is the discriminator. The mechanism is live in the shipped layout. A/B: registry resolvabilityThe PR's own Reviewer Test Plan step 1, run verbatim (anonymous token + manifest request). Witness:
Correction to the PR description (not a code change): the description and test plan state the broken name answers 403; from this runner it answers 404 (OCI index Accept). ghcr.io returns either for an absent/unpublished repo depending on request shape; both are "pull denied", so the substantive claim — the base default cannot be pulled and every opted-in review failed before anything ran — holds unchanged. The test plan's "Expected: … 403" line should read "≠ 200" to stay robust. Mutation matrix on the new testsSuite
The PR's mutation claims check out under decomposition: "reverting to the hardcoded default reddens two unit tests" corresponds to broken literal + manifest-lookup removal (M1a's kill plus the defaults-test kill; 2 rows), and "dropping Adjudicating the survivors. M3: unavoidable at unit level — Findings1. (Suggestion) The file-sourced guard on the new 2. (Note) The parity enumeration in the doc comment is missing one channel. The new comment names the channels where parity with Not covered
MethodologyEnvironment: CI verify container ( Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed current head 2b63553. No blocking issues found. The image resolution fix matches the CLI manifest and preserves override precedence and the file-sourced provenance guard. I also verified a clean merge tree against current main, a fresh build and bundle, CLI typecheck, and the focused sandboxed-exec suite (37 passed, 1 skipped). The existing missing regression-test suggestion remains non-blocking and is already captured in its thread.
|
Released in v0.22.2. |
4 participants
What this PR does
Makes
review.sandboxresolve its container image from the CLI's own manifest instead of a hardcoded name, and honours an operator'sQWEN_SANDBOX_IMAGEwhen they have configured one. The last-resort literal is now versioned rather than floating on:latest.Why it's needed
#9723 shipped
ghcr.io/qwenlm/qwen-code/sandbox:latestas the default, with a comment claiming it was "the same imageqwen --sandboxuses". Neither half held. The CLI's image comes from its own manifest fieldconfig.sandboxImageUri—ghcr.io/qwenlm/qwen-code:<version>— a different repository path and a pinned tag. And the hardcoded name does not resolve at all: an anonymous manifest request answers 403 where the real one answers 200. Every command of a review that opted intoautoorrequiredtherefore failed at image pull, before anything ran. The default isoff, so nothing broke for anyone who had not opted in; for anyone who had, the feature could not work at all.The test that let this through asserted
toContain('sandbox')— a substring of the name rather than the property it meant to pin. The broken value contains that word and the correct one does not, so the assertion passed on the defect and would have failed on the fix.Reviewer Test Plan
How to verify
Confirm the two images differ in the way that matters, with no local state involved:
Expected:
0.22.0→ 200,sandbox:latest→ 403.Then, on a machine with a container runtime, that the contained path actually runs. Lay a fixture out the way the pipeline does —
<repo>/.qwen/tmp/review-pr-1/with apackage.json— under a path your runtime shares ($HOMEis safe; see the risk note below), setQWEN_REVIEW_SANDBOX=required, and drivecontainerCommandthrough the real runtime. Unit tests, from the repository root:npm run test --workspace=packages/cli -- src/commands/review/lib/sandboxed-exec.test.ts.Evidence (Before & After)
Five live arms, same fixture and same runtime, differing only in the image the code resolves:
IN-CONTAINERCI=1present--network nonefor test, network for installDNS-FAIL/DNS-OKWith the merged default every arm fails before anything runs, because no container starts.
Mutation evidence: reverting to the hardcoded default reddens two unit tests; dropping
QWEN_SANDBOX_IMAGEsupport reddens one.Tested on
Environment (optional)
Docker Desktop, macOS arm64, image
ghcr.io/qwenlm/qwen-code:0.22.0(226 MB, linux/arm64). Unit tests under vitest, no runtime required.Risk & Scope
config.sandboxImageUrinames at that version rather than a fixed name an operator may have pre-pulled. That is the intended coupling — it is the image the CLI already ships with — but it does mean the first opted-in review on a cold machine pays a pull.$HOME(the normal case) is unaffected, and Linux is unaffected. Not fixed here; recorded because it cost real time to diagnose.review.sandboxdefaults tooff; an operator who had setQWEN_REVIEW_SANDBOX_IMAGEkeeps that value at the same precedence.Linked Issues
Fixes a defect introduced by #9723. Related: #9556.
中文说明
本 PR 做了什么
让
review.sandbox从 CLI 自己的 manifest 解析容器镜像，而不是用一个硬编码的名字；并在操作者配置过QWEN_SANDBOX_IMAGE时尊重它。兜底字面量也从浮动的:latest改成带版本号的形式。为什么需要
#9723 交付的默认值是
ghcr.io/qwenlm/qwen-code/sandbox:latest，旁边的注释写着它是「qwen --sandbox用的同一个镜像」。两半都不成立。CLI 的镜像来自它自己的 manifest 字段config.sandboxImageUri——ghcr.io/qwenlm/qwen-code:<version>—— 不同的仓库路径、版本固定的 tag。而那个硬编码的名字根本无法解析：匿名 manifest 请求返回 403，真实镜像返回 200。因此任何开启了auto或required的审查，每条命令都会在拉镜像时失败，在任何东西运行之前。默认值是off，所以没有开启的人不受影响；但对开启的人来说，这个功能完全无法工作。放过这个缺陷的那条测试断言的是
toContain('sandbox')—— 检查名字的子串，而不是它本想钉住的性质。坏的值含有这个词，正确的值反而不含，所以这条断言在缺陷上通过、会在修复上失败。评审者验证方案
如何验证
先确认两个镜像的关键差异，不涉及任何本地状态：
预期：
0.22.0→ 200，sandbox:latest→ 403。然后在有容器运行时的机器上验证容器化路径确实能跑。按流水线的布局造夹具 ——
<repo>/.qwen/tmp/review-pr-1/内含package.json—— 放在运行时共享的路径下（$HOME是安全的，原因见下方风险说明），设置QWEN_REVIEW_SANDBOX=required，用真实运行时驱动containerCommand。单元测试，在仓库根目录执行：npm run test --workspace=packages/cli -- src/commands/review/lib/sandboxed-exec.test.ts。证据（前后对比）
五条活体验证，同一夹具、同一运行时，唯一差别是代码解析出的镜像：
IN-CONTAINERCI=1在--network none、install 保留网络DNS-FAIL/DNS-OK用已合入的默认值，五臂在任何东西开始运行之前就全部失败，因为容器根本起不来。
变异证据：退回硬编码默认值会让两条单元测试变红；去掉对
QWEN_SANDBOX_IMAGE的支持会让一条变红。测试平台
运行环境（可选）
Docker Desktop、macOS arm64、镜像
ghcr.io/qwenlm/qwen-code:0.22.0（226 MB，linux/arm64）。单元测试在 vitest 下运行，不需要容器运行时。风险与范围
config.sandboxImageUri所指的镜像，而不是操作者可能已预先拉取的某个固定名字。这正是有意建立的耦合 —— 它就是 CLI 本身附带的镜像 —— 但代价是冷机器上第一次开启审查要付一次拉取。$HOME下（常规情形）不受影响，Linux 不受影响。本 PR 不修，记录在此是因为它确实耗费了排查时间。review.sandbox默认off；已设置QWEN_REVIEW_SANDBOX_IMAGE的操作者，该值的优先级不变。关联 Issue
修复 #9723 引入的一个缺陷。相关：#9556。