refactor(cli): reuse trusted workspace runtime resolver - #9958
Merged
Conversation
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: Real and verifiable. The same resolve-then-trust composition was copy-pasted across five workspace route files — I confirmed identical blocks in Direction: Aligned. Keeping the workspace trust guard in one place makes the "workspace routes stay inside the resolved runtime" invariant easier to hold. Behavior is preserved: the shared helper forwards a Size: 107 production lines + 39 test lines, all under Approach: The extraction is the minimal version of this change, and the diff carries nothing unrelated. One observation: Risk: No elevated risk signals (no high-risk paths matched). The helper is security-sensitive, so the code review will verify behavior preservation closely; the new tests cover both the trusted and the untrusted → 403 paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 真实存在且可验证。同样的"解析运行时 + 信任校验"组合被复制粘贴在五个路由文件中——我在基线提交上确认了 方向： 对齐。把工作区信任校验集中到一处，能让"工作区路由保持在已解析运行时之内"这一不变量更容易维持。行为保持一致：共享辅助函数透传的 规模： 107 行生产代码 + 39 行测试，全部位于 方案： 这次提取是该改动的最小形态，diff 里也没有夹带无关内容。一个观察： 风险： 无升级风险信号（未命中高风险路径）。该辅助函数涉及安全边界，代码审查会仔细核对行为一致性；新增测试同时覆盖了可信与不可信 → 403 两条路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
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. |
Collaborator
Code reviewMy independent take before reading the diff: five identical copies of a security-sensitive resolve-then-trust sequence belong in What I verified against the base commit:
Two non-blocking notes:
Testing evidenceThis is an unattended CI run, so no local execution was performed — the evidence below is the PR's own CI on the reviewed commit, fetched via the API. All four
No sandboxed lane ( 中文说明代码审查读 diff 之前我的独立想法：五份完全相同的"解析运行时 + 信任校验"安全敏感序列，应该放进 对照基线提交核实的内容：
两条不阻塞的提醒：
测试证据本次为无人值守的 CI 运行，未做任何本地执行——以上证据是本 PR 在被审提交上的 CI 结果，通过 API 获取。四个 未在此点名沙箱验证通道（ — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, minimal, behavior-preserving dedup of a security-sensitive guard, verified against base and backed by fully green CI; the two named nits are follow-up material, not merge blockers. Stepping back: this PR does one thing, and the thing it does is real. I confirmed on the base commit that five route files carried byte-identical copies of the resolve-then-trust sequence — that's an observed problem, not a hypothetical one, and consolidating a workspace trust guard into the module that owns both halves is exactly where it belongs. The implementation matches the approach I'd have proposed independently: extract next to the composed functions, keep the call sites untouched, pin both outcomes with a test. Nothing in the diff is trying too hard; if anything the only speculative element is the passthrough The security-sensitive nature of this code is why I checked behavior preservation at the level of defaults and response bodies rather than trusting the "no behavior change" claim: same composition order, same In six months, the maintainer touching a sixth workspace route will find one guard to reuse instead of five to copy from. That's the thank-you case. Approving. 中文说明置信度：4/5 —— 干净、最小化、行为保持一致的安全敏感守卫去重，已对照基线核实并得到全绿 CI 的支持；两处点名的提醒属于后续工作，不构成合并阻塞。 退一步看：这个 PR 只做一件事，而且做的事是真实的。我在基线提交上确认了五个路由文件中存在逐字节相同的"解析 + 信任校验"序列——这是已观察到的问题，而不是假设性的问题；把工作区信任守卫合并到同时拥有其两个组成部分的模块中，正是它该在的位置。实现与我独立会提出的方案一致：在紧邻被组合函数处提取、保持调用点不变、用测试钉住两种结果。diff 里没有任何过度设计；唯一称得上投机的是那个还没有人传值的 这段代码的安全敏感性，正是我为何选择在默认值与响应体层面核实行为一致性、而不是轻信"行为无变化"声明的原因：组合顺序相同、 六个月后，维护者在编写第六个工作区路由时，会找到一个可复用的守卫，而不是五份可供复制的代码。这就是值得感谢的情形。批准合并。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
approved these changes
Aug 24, 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. ✅
qwen-code-ci-bot
approved these changes
Aug 24, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.8-max via Qwen Code /review (v0.22.0)
wenshao
approved these changes
Aug 25, 2026
DragonnZhang
added a commit
to DragonnZhang/qwen-code
that referenced
this pull request
Aug 26, 2026
…dRuntime block Review R5-1: the 'resolveTrustedRuntime (route selection)' block added by this branch duplicates the resolveTrustedRuntime block that arrived via main (QwenLM#9958) -- identical inputs and assertions once makeRuntime is parameterized. Fold the one genuinely new assertion (expect(response.status).not.toHaveBeenCalled()) into the existing 'returns an active trusted runtime' test, drop the duplicated block, and restore the unparameterized makeRuntime helper now that makeRuntime(false) has no remaining callers.
Collaborator
|
Released in v0.22.2. |
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
Workspace-scoped Git and status routes now reuse one trusted-runtime resolution path. The shared path performs runtime lookup and workspace trust validation together, preserving the existing route results and failure responses.
Why it's needed
Five routes independently repeated the same security-sensitive lookup and trust guard. Keeping the check in one place makes the workspace boundary explicit and prevents future routes from accidentally implementing only part of the guard.
Reviewer Test Plan
How to verify
Exercise the workspace Git branches, diff, log, repository status, and workspace status routes against a trusted runtime and confirm their existing responses are unchanged. Repeat against an untrusted runtime and confirm every route rejects the request with HTTP 403 rather than reaching a fallback runtime.
The focused route suites pass: 26 tests.
Evidence (Before & After)
N/A — internal route boundary refactor with direct trusted and untrusted runtime coverage.
Tested on
Environment (optional)
macOS arm64, Node.js v25.6.1; 26 focused Vitest tests, CLI build, and ESLint completed successfully.
Risk & Scope
Linked Issues
None.
中文说明
本 PR 做了什么
工作区范围的 Git 和状态路由现在复用同一条可信运行时解析路径。共享逻辑把运行时查找和工作区信任校验放在一起，同时保持原有路由结果和失败响应不变。
为什么需要
此前五个路由分别重复同一套安全敏感的查找和信任保护。将检查集中到一处可以明确工作区边界，也能避免未来路由只实现其中一部分保护逻辑。
Reviewer 测试计划
如何验证
对可信运行时分别调用工作区 Git 分支、diff、日志、仓库状态和工作区状态路由，确认原有响应不变。再对不可信运行时重复调用，确认所有路由都返回 HTTP 403，而不会落到后备运行时。
聚焦路由测试全部通过：26 个测试。
证据（修改前与修改后）
不适用——这是带可信和不可信运行时直接覆盖的内部路由边界重构。
测试平台
环境（可选）
macOS arm64、Node.js v25.6.1；26 个聚焦 Vitest 测试、CLI 构建和 ESLint 均成功完成。
风险与范围
关联 Issue
无。