fix(deps): clear critical runtime audit findings - #6065
Conversation
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary (automated, high-confidence only)
The dependency audit fix itself (commit 56584333d) is well-scoped and correct: it bumps simple-git, shell-quote, tar, and undici in both packages/cli and packages/core, adds a audit:runtime:critical npm script, and wires it into CI. The lockfile is internally consistent.
However, the PR as a whole has a significant scope issue — see inline comment below.
| "lint:ci": "eslint . --ext .ts,.tsx --max-warnings 0 && eslint integration-tests --max-warnings 0", | ||
| "lint:sdk:python": "python3 -m ruff check --config packages/sdk-python/pyproject.toml packages/sdk-python", | ||
| "lint:all": "node scripts/lint.js", | ||
| "audit:runtime:critical": "npm audit --omit=dev --audit-level=critical", |
There was a problem hiding this comment.
PR scope mismatch — bundles 4 unrelated commits from 3 authors
The origin/main...HEAD diff shows 43 files changed (+3,753/-325), but only 6 files belong to the dependency audit fix described in the PR title. The other 37 files come from 3 additional commits by other authors:
e1ddcdc2afeat(loop): add autonomous mode for a bare /loop (feat(loop): add autonomous mode for a bare /loop #5991) — author: @qqqys — addsautonomous-loop.ts+ loop skill changesf37c64c7ffix(cli): Keep serve health responsive before runtime load (fix(cli): Keep serve health responsive before runtime load #6013) — author: @jinye — modifies serve fast-path, session, headless safetyea536d361fix(cli): Handle ACP read_file for managed local paths (fix(cli): Handle ACP read_file for managed local paths #6021) — author: @jinye — modifiesacpAgent.ts,filesystem.ts,Session.ts,read-file.ts,write-file.ts,errors.ts,fileUtils.ts
None of these commits exist on origin/main. When this PR merges, all 43 files of changes will land on main under a PR titled "fix(deps): clear critical runtime audit findings".
Why this matters: Reviewers looking at the PR title and description will focus on the 6 dependency files and may miss the ~3,500 lines of code changes in ACP integration, serve health, autonomous loop, error handling, and file access control.
Suggestion: Rebase this branch onto origin/main and drop the 3 commits that belong to other PRs. The dependency fix (commit 56584333d) should be the only commit in this PR. The other work should land via its own PRs (or may already have landed via squash merges on main — worth verifying).
487d200 to
c15e8dd
Compare
|
@DragonnZhang thanks for catching this. I rebased the branch onto the current main and force-pushed so the PR now contains only the dependency-audit fix commit. GitHub now shows 1 commit, 6 changed files, and the branch is mergeable. Verified locally after the rebase:
|
c15e8dd to
d10c54d
Compare
d10c54d to
39205fb
Compare
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Well-scoped dependency bump that clears critical runtime audit findings. Lockfile changes are consistent, CI audit step is correctly placed, and the rebase cleaned up the prior scope concern.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
|
Thanks for the PR! (Re-run after follow-up commit for Template looks good ✓ On direction: clearing critical npm audit findings in runtime dependencies is a clear security win — Qwen Code executes shell, git, archive, and network code, so critical advisories in those paths deserve fast fixes. The CI guard ( On approach: scope remains tight — four runtime dep bumps, one CI step, lockfile resolution, NOTICES.txt update, and now a minimal source-code adaptation for Moving on to code review. 🔍 中文说明感谢贡献!(针对 模板完整 ✓ 方向:清理运行时依赖中的 critical 级别 npm audit 告警是明确的安全改进——Qwen Code 会执行 shell、git、归档解压和网络相关代码,这些路径中的 critical 级别告警值得优先修复。CI 守卫( 方案:范围仍然紧凑——四个运行时依赖升级、一个 CI 步骤、lockfile 解析、NOTICES.txt 更新,以及现在针对 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code Review(Re-run covering the follow-up commit The original commit is a clean dependency-bump PR — no source code changes, only The follow-up commit adds Lockfile changes are consistent with No correctness bugs, security concerns, or structural violations found. Real-Scenario Testing(Run on Linux, Ubuntu CI runner.) Before (main branch)After (this PR)Additional verification on PR branch
The 2 criticals on main are exactly cleared by the bumps:
Bonus: 中文说明代码审查(涵盖后续提交 原始提交是一个干净的依赖升级 PR——没有源代码改动,只有 后续提交在 Lockfile 改动与 没有发现正确性 bug、安全问题或结构性违规。 真实场景测试(在 Linux Ubuntu CI runner 上运行。) Before(main 分支)After(此 PR)PR 分支额外验证
main 上的 2 个 critical 正好被这些升级清除:
附带收益: — Qwen Code · qwen3.7-max |
|
(Re-run after follow-up commit.) This PR does exactly what it says — clears 2 critical runtime audit findings ( The CI guard ( The follow-up commit was a necessary response to Ubuntu CI catching All three stages clean. Straightforward ship. 中文说明(后续提交后的重新审查。) 此 PR 完全如其描述——用最小范围的定向依赖升级清除了 2 个 critical 级别运行时 audit 告警( CI 守卫( 后续提交是对 Ubuntu CI 发现 三个阶段全部通过。可以合入。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Local verification report — PR #6065Verified locally on macOS with real Environment: Node v22.22.2 · npm 10.9.7 · Darwin 25.5.0 · merge-base 1. Core claim — critical runtime audit (independently reproduced)Ran the real audit off each tree's own lockfile:
The 2 criticals on main are exactly cleared by the bumps:
Bonus: 2.
|
| Check (on merged tree) | Result |
|---|---|
npm ci (hard-errors on lockfile drift) |
✅ exit 0 — 1842 packages, build ran |
npm run check:lockfile |
✅ Lockfile check passed |
npm run audit:runtime:critical |
✅ exit 0 — 0 critical |
npm run typecheck (all workspaces) |
✅ exit 0 |
packages/cli → @qwen-code/sdk edge in lockfile |
✅ present |
4. Regression tests on the upgraded deps
Confirmed the worktree installed the upgraded versions (shell-quote 1.9.0, simple-git 3.36.0, tar 7.5.19, undici 6.27.0), then ran the Reviewer Test Plan:
packages/core: 479 passed (shell-utils, shell, gitWorktreeService, github)packages/cli: 70 passed (sandbox, standalone-update, gitUtils)
Import surface is clean: all four deps are imported only via their main entry (no deep imports), so simple-git@3.36.0's new exports map can't break anything — confirmed by runtime load.
Non-blocking notes (for the merge decision, not blockers)
- The new CI gate is non-hermetic.
npm auditqueries the live advisory DB, soAudit critical runtime dependenciescan turn red with zero code change when a new critical advisory is later published against a currently-clean runtime dep (or flake if the registry is slow). It fails-closed (red, never a silent pass). Scoping tocritical+--omit=devkeeps the noise low — reasonable, just worth knowing. - Scope is critical-only / runtime-only. 4 high + 23 moderate runtime advisories remain (axios via
@larksuiteoapi/node-sdk, OpenTelemetry chain) — explicitly deferred by the PR. The gate won't catch high/moderate or dev-dependency criticals. - Desktop is out of coverage.
packages/desktopis excluded from the root workspace (!packages/desktop), andpackages/desktop/packages/sharedstill declaresshell-quote@^1.8.3(a critical-range version), plustar@^7.5.2/undici@^6.22.0. The root gate does not see it. If the desktop app ships, it needs its own audit — out of this PR's scope, but flagging it. - Platform: the gate is a pure
npmcommand with no shell/path specifics, so it behaves identically on Windows/Linux despite only macOS being tested. - Stale branch: merge is verified clean & consistent today; the lockfile lands via git's line-merge. If
mainmoves substantially before merge, an "Update branch" / rebase would regenerate the lockfile via npm for extra safety.
Recommendation: ✅ Safe to merge as-is. The @qwen-code/sdk diff line needs no action. Consider follow-up issues for the deferred high/moderate advisories and the desktop workspace.
🇨🇳 中文版(完整对应)
✅ 本地验证报告 —— PR #6065
在 macOS 上用真实 npm ci 安装 + tmux,基于 origin/main(不是陈旧的本地 base)验证。结论:PR 的核心主张成立,合并是安全的。 diff 里那行吓人的 -"@qwen-code/sdk" 是陈旧分支造成的假象,并非真实删除——详见下文。文末列了几条不阻塞合并的说明。
环境: Node v22.22.2 · npm 10.9.7 · Darwin 25.5.0 · merge-base f23357d6a
1. 核心主张 —— critical 运行时 audit(独立复现)
基于每棵树各自的 lockfile 跑真实 audit:
| 树 | 命令 | 退出码 | 漏洞 |
|---|---|---|---|
origin/main |
npm audit --omit=dev --audit-level=critical |
1(失败) | 共 48 —— 2 个 critical,15 high,30 mod,1 low |
| PR head | npm run audit:runtime:critical |
0(通过) | 共 28 —— 0 个 critical,4 high,23 mod,1 low |
main 上的 2 个 critical 正好被这些升级清除:
- shell-quote
1.8.3 → 1.9.0—— GHSA-w7jw-789q-3m8p(quote()换行转义绕过) - simple-git
3.28.0 → 3.36.0—— GHSA-jcxm-m3jx-f287 等 3 条(RCE / 选项解析绕过)
附带收益:tar 7.5.2→7.5.19、undici 6.22.0→6.27.0(都是 high,不触发这个 gate)也被清除;加上传递依赖重新解析(fast-uri、path-to-regexp、hono、markdown-it、linkify-it,即 NOTICES.txt 的改动)把 high 数量从 15 降到 4。
2. ⚠️ →✅ diff 里的 -"@qwen-code/sdk" 是假象(对 reviewer 最重要)
2-way 的 git diff origin/main..PR 显示 packages/cli/package.json 删除了 "@qwen-code/sdk": "file:../sdk-typescript",而 daemon-worker.ts 仍在 await import('@qwen-code/sdk/daemon') 用它。看起来像回归——但其实不是:
- merge-base
f23357d6a本来就没有这一行。它是在 PR 拉分支之后才被cf6323bfb(feat(cli): Add daemon-managed channel worker for serve --channel #6031,daemon channel worker)加到main上的。 - 所以 PR 根本没碰这一行;只因为分支陈旧了约 1 天,2-way diff 才看起来像删除。
用真实的 3-way 合并(GitHub 各种合并策略底层都是这个)证明:
git merge-tree --write-tree origin/main <PR> → 干净(exit 0,无冲突)
git merge --no-ff <PR>(合入 origin/main) → 干净(ort 策略)
合并后的 packages/cli/package.json 保留了 @qwen-code/sdk,同时应用了版本升级;合并后的 lockfile 自动恢复了 packages/cli → @qwen-code/sdk 这条依赖边。
3. 合并后的树可安装且一致
真实合入 origin/main 后跑干净的 npm ci:
| 检查(在合并后的树上) | 结果 |
|---|---|
npm ci(lockfile 不一致会直接报错) |
✅ exit 0 —— 1842 个包,build 已执行 |
npm run check:lockfile |
✅ Lockfile check passed |
npm run audit:runtime:critical |
✅ exit 0 —— 0 critical |
npm run typecheck(全部 workspace) |
✅ exit 0 |
lockfile 中 packages/cli → @qwen-code/sdk 边 |
✅ 存在 |
4. 针对升级后依赖的回归测试
先确认 worktree 装的是升级版本(shell-quote 1.9.0、simple-git 3.36.0、tar 7.5.19、undici 6.27.0),再跑 Reviewer Test Plan:
packages/core:479 通过(shell-utils、shell、gitWorktreeService、github)packages/cli:70 通过(sandbox、standalone-update、gitUtils)
导入面干净:四个依赖都只从主入口导入(没有深层子路径导入),所以 simple-git@3.36.0 新增的 exports 映射不会破坏任何东西——已用运行时加载验证。
不阻塞合并的说明(供合并决策参考,非阻断项)
- 新增的 CI gate 非确定性(non-hermetic)。
npm audit查的是实时公告库,所以Audit critical runtime dependencies这一步可能在代码零改动的情况下变红——当以后某个当前干净的运行时依赖被爆出新的 critical 公告时(或 registry 慢时抖动)。它是 fail-closed(变红,绝不会静默通过)。限定在critical+--omit=dev已把噪声压到很低——合理,只是需要知道这个性质。 - 范围仅限 critical / 仅运行时。 仍有 4 个 high + 23 个 moderate 运行时公告(axios 经
@larksuiteoapi/node-sdk、OpenTelemetry 链)——PR 明确留作后续。这个 gate 不会抓 high/moderate,也不抓 dev 依赖里的 critical。 - desktop 不在覆盖范围内。
packages/desktop被排除在根 workspace 之外(!packages/desktop),而packages/desktop/packages/shared仍声明shell-quote@^1.8.3(critical 区间),外加tar@^7.5.2/undici@^6.22.0。根 gate 看不到它。如果 desktop 应用会发布,它需要单独 audit——不在本 PR 范围内,但提一下。 - 平台: 这个 gate 是纯
npm命令,没有 shell/路径相关写法,所以尽管只在 macOS 测过,在 Windows/Linux 上行为一致。 - 陈旧分支: 当下已验证合并干净且一致;lockfile 是通过 git 的按行合并落地的。如果合并前
main又大幅前进,用 "Update branch" / rebase 让 npm 重新生成 lockfile 会更保险。
建议: ✅ 可以按现状合并。@qwen-code/sdk 那行无需处理。建议为留下的 high/moderate 公告和 desktop workspace 建后续 issue。
|
@wenshao @doudouOUC thanks again for the reviews and verification. This looks ready from my side, with the earlier scope issue fixed. I don’t seem to have merge-queue access here, so please feel free to queue it when convenient. If you want one more rebase first, I’m happy to do that. |
Head branch was pushed to by a user without write access
8aa0874
|
Ubuntu CI exposed a I pushed a small follow-up that enables that only for the existing intentional worktree hook-path setup. Verified locally with the failing focused test, lockfile check, critical runtime audit, typecheck, and lint. |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
This PR updates the runtime dependency versions that were responsible for critical npm audit findings in the CLI and core dependency tree. It also adds a small CI check that runs a production-only audit at the critical threshold, so this class of issue is caught before future changes move forward.
Why it's needed
Qwen Code runs local shell, git, archive, and network workflows, so critical advisories in runtime dependencies deserve a fast and focused fix. The old versions of packages such as
simple-gitandshell-quotecould remain in the lockfile while normal build and type checks still passed, because CI installs dependencies with--no-audit.This PR keeps the scope intentionally narrow. It clears the critical runtime audit path and leaves the larger high/moderate follow-up work, such as
diff@9and OpenTelemetry, for separate PRs because those upgrades require broader compatibility work.Reviewer Test Plan
How to verify
Run
npm run audit:runtime:criticaland confirm it exits successfully. The command may still print lower-severity audit entries, but it should not fail while there are no critical runtime advisories.Run the focused shell, git, archive, and update tests to confirm the updated runtime packages still work in the code paths that use them.
Evidence (Before & After)
Before:
npm audit --omit=dev --audit-level=criticalfailed because critical runtime advisories were present.After:
npm run audit:runtime:criticalexits successfully. The dependency tree now resolvessimple-git@3.36.0,shell-quote@1.9.0,tar@7.5.19, andundici@6.27.0for the relevant runtime packages.Local validation completed:
Results: build completed through
npm install/prepare; lockfile check passed; typecheck passed; critical runtime audit exited successfully; 479 focused core tests passed; 70 focused CLI tests passed; lint passed.Tested on
Environment (optional)
Node/npm workspace install on macOS. No UI or TUI behavior changed.
Risk & Scope
diff@9changes exported types and needs a separate compatibility PR.Linked Issues
Fixes #6063
Refs #6062
中文说明
这个 PR 做了什么
这个 PR 更新了 CLI 和 core 运行时依赖树中导致 critical 级别 npm audit 报告的依赖版本。同时增加了一个很小的 CI 检查,只审计生产依赖里的 critical 级别问题,这样以后同类问题可以在合入前被发现。
为什么需要这个改动
Qwen Code 会执行本地 shell、git、归档解压和网络相关流程,所以运行时依赖里的 critical 级别安全问题需要优先处理。旧版本的
simple-git、shell-quote等依赖即使存在安全告警,也可能在正常 build 和 typecheck 通过的情况下继续留在 lockfile 里,因为 CI 安装依赖时使用了--no-audit。这个 PR 有意保持很小的范围。它清理 critical 级别的运行时 audit 路径,并把更大的 high/moderate 后续工作留给单独 PR,例如
diff@9和 OpenTelemetry,因为这些升级需要更多兼容性处理。Reviewer Test Plan
如何验证
运行
npm run audit:runtime:critical,确认命令成功退出。这个命令可能仍然打印较低级别的 audit 信息,但在没有 critical 运行时告警时不应该失败。运行聚焦 shell、git、归档和更新逻辑的测试,确认更新后的运行时依赖仍然能正常工作。
Evidence (Before & After)
Before:
npm audit --omit=dev --audit-level=critical会失败,因为存在 critical 级别运行时告警。After:
npm run audit:runtime:critical可以成功退出。相关运行时依赖树现在解析到simple-git@3.36.0、shell-quote@1.9.0、tar@7.5.19和undici@6.27.0。本地验证已完成:
结果:
npm install/prepare完成 build;lockfile 检查通过;typecheck 通过;critical runtime audit 成功退出;479 个 core 聚焦测试通过;70 个 CLI 聚焦测试通过;lint 通过。Tested on
Environment (optional)
macOS 上的 Node/npm workspace 安装环境。没有 UI 或 TUI 行为变化。
Risk & Scope
diff@9改变了导出的类型,需要单独的兼容性 PR。Linked Issues
Fixes #6063
Refs #6062