Skip to content

ci(autofix): add cross-package contract verification - #7642

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
qqqys:agent/autofix-contract-verification
Jul 24, 2026
Merged

ci(autofix): add cross-package contract verification#7642
wenshao merged 1 commit into
QwenLM:mainfrom
qqqys:agent/autofix-contract-verification

Conversation

@qqqys

@qqqys qqqys commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds a trusted cross-package contract gate to both autofix verification paths. Every candidate now runs the repository i18n check before it can be published, and candidates that change the canonical tool-name contract also run the Web Shell tool-display drift test.

The verifier is staged from the trusted base checkout before the candidate branch is selected, matching the existing security boundary for verification helpers. Failures produce an explicit failed outcome, including on the review no-op path.

Why it's needed

The existing affected-workspace tests only exercise the workspace that owns a changed file. A core tool-name change therefore does not select the independent Web Shell workspace, so a stale display-name mapping can pass takeover verification and fail later in full CI. The autofix gate also omitted the i18n check that full CI already treats as required.

Reviewer Test Plan

How to verify

  1. Run the workflow regression test and confirm all 102 tests pass.
  2. Feed a non-contract path to the helper and confirm it runs only the i18n check.
  3. Feed the canonical tool-name path to the helper and confirm it additionally runs the Web Shell drift test.
  4. Force either command to fail and confirm the helper exits non-zero and emits outcome=failed.
  5. Confirm both autofix verification paths invoke only the helper copy staged from the trusted base, and that review verification runs it before accepting a no-op.

Evidence (Before & After)

N/A — workflow-only change with behavioral regression coverage.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Local Node.js 22 workspace. Verified with the focused workflow suite (102/102), the real i18n command, the targeted Web Shell drift test (2/2), full build, typecheck, ESLint, and targeted Prettier check.

Risk & Scope

  • Main risk or tradeoff: Every autofix candidate now pays the cost of the i18n check; the Web Shell test remains conditional on the canonical tool-name contract changing.
  • Not validated / out of scope: Hosted-runner end-to-end execution and unrelated cross-package contracts; GitHub CI covers the hosted workflow environment.
  • Breaking changes / migration notes: None.

Linked Issues

Refs #7638

中文说明

本 PR 做了什么

为两条 autofix 验证路径增加 trusted cross-package contract gate。现在每个候选修改在发布前都会执行仓库 i18n 检查;当候选修改 canonical tool-name contract 时,还会执行 Web Shell 的 tool-display drift 测试。

验证脚本会在切换到候选分支前从 trusted base checkout 暂存,与现有验证 helper 的安全边界一致。失败会产生明确的 failed outcome,包括 review no-op 路径。

为什么需要

现有 affected-workspace 测试只会执行 changed file 所属 workspace 的测试。因此 core tool-name 变化不会选中独立的 Web Shell workspace,过期的 display-name mapping 可能通过 takeover 验证,之后才在完整 CI 失败。autofix gate 也遗漏了完整 CI 已要求的 i18n 检查。

Reviewer Test Plan

如何验证

  1. 运行 workflow 回归测试,确认 102 个测试全部通过。
  2. 向 helper 输入非 contract 路径,确认仅执行 i18n 检查。
  3. 向 helper 输入 canonical tool-name 路径,确认额外执行 Web Shell drift 测试。
  4. 强制任一命令失败,确认 helper 非零退出并输出 outcome=failed
  5. 确认两条 autofix 验证路径都只调用从 trusted base 暂存的 helper 副本,并且 review verification 会在接受 no-op 之前执行它。

证据(Before & After)

不适用——这是仅修改 workflow 的变更,并有行为回归测试覆盖。

测试平台

OS 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

环境(可选)

本地 Node.js 22 workspace。已验证 focused workflow suite(102/102)、真实 i18n 命令、定向 Web Shell drift 测试(2/2)、完整 build、typecheck、ESLint 和定向 Prettier 检查。

风险与范围

  • 主要风险或取舍:每个 autofix 候选现在都会承担 i18n 检查开销;Web Shell 测试仍只在 canonical tool-name contract 变化时执行。
  • 未验证 / 不在范围内:hosted-runner 端到端执行和其他 cross-package contract;GitHub CI 会覆盖 hosted workflow 环境。
  • Breaking changes / 迁移说明:无。

关联 Issue

Refs #7638

@qqqys
qqqys marked this pull request as ready for review July 24, 2026 03:55
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual body complete.

Problem: Real and independently verified, not theoretical. Full CI runs npm run check-i18n (ci.yml:316) but the autofix verification gate has zero references to it on main — so a candidate that breaks i18n can pass takeover verification and only fail later in full CI. The cross-package gap is equally concrete: toolFormatting.drift.test.ts reads core/src/tools/tool-names.ts via the filesystem (web-shell deliberately does not depend on @qwen-code/qwen-code-core), so the autofix's import-graph / owning-package test selection (--changed origin/main) never selects the web-shell workspace when tool-names.ts changes. A stale TOOL_DISPLAY_NAMES map sails through takeover and dies in full CI. Both gaps confirmed by reading the code.

Direction: Aligned. This is phase 1 of #7638 and it strictly tightens the autofix safety boundary — it rejects candidates that would fail full CI rather than letting them through. It doesn't touch auth/sandbox/model/telemetry/release/public contract. CHANGELOG: no direct reference, but the autofix/CI verification area is clearly in scope for a ci enhancement.

Size: Not a core-path change (.github/ + scripts/tests/ only). Production logic is ~30 lines (24 shell + 6 workflow YAML); the remaining 95 are regression tests. Well under any threshold.

Approach: Scope feels right and matches the issue's phase 1 exactly. Good reuse — it reuses the existing check-settings-schema.sh "stage from trusted base into ${RUNNER_TEMP}" security pattern rather than inventing a new mechanism, and reuses the existing check-i18n script and drift test rather than adding parallel logic. Running i18n on every candidate but gating the web-shell test behind the tool-names.ts change is a sensible cost tradeoff (acknowledged in Risk & Scope). No drive-by changes; everything is on-topic. One thing I'll confirm in code review: the deliberate set -uo pipefail (no -e) relies on explicit if ! ... checks — I'll verify nothing unchecked can fail silently.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必填段落齐全,双语正文完整。

问题: 真实存在,且我已独立核实,并非理论性问题。完整 CI 会执行 npm run check-i18nci.yml:316),但 main 上的 autofix 验证 gate 对它零引用——因此破坏 i18n 的候选可以通过 takeover 验证,直到完整 CI 才失败。跨包 gap 同样具体:toolFormatting.drift.test.ts 通过文件系统读取 core/src/tools/tool-names.ts(web-shell 刻意不依赖 @qwen-code/qwen-code-core),所以 autofix 基于 import graph / owning-package 的测试选择(--changed origin/main)在 tool-names.ts 变化时永远不会选中 web-shell workspace。过期的 TOOL_DISPLAY_NAMES 映射会通过 takeover,然后在完整 CI 挂掉。两个 gap 均已通过读代码确认。

方向: 对齐。这是 #7638 的第一阶段,且严格来说收紧了 autofix 安全边界——它会拒绝将在完整 CI 失败的候选,而不是放行。不触及 auth/sandbox/model/telemetry/release/public contract。CHANGELOG:无直接引用,但 autofix/CI 验证领域显然属于 ci 增强范围。

规模: 非核心路径变更(仅 .github/ + scripts/tests/)。生产逻辑约 30 行(24 行 shell + 6 行 workflow YAML),其余 95 行为回归测试。远低于任何阈值。

方案: 范围合理,与 issue 第一阶段完全吻合。复用良好——复用现有 check-settings-schema.sh 的"从 trusted base 暂存到 ${RUNNER_TEMP}"安全模式,而非新造机制;复用现有 check-i18n 脚本和 drift 测试,而非新增并行逻辑。对每个候选都跑 i18n、但把 web-shell 测试门控在 tool-names.ts 变化之后,是合理的成本取舍(已在 Risk & Scope 说明)。无顺手改动,全部切题。代码审查时我会确认一点:刻意的 set -uo pipefail(无 -e)依赖显式 if ! ... 检查——我会核实没有未被检查的命令会静默失败。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at a5c39e2f10e8e18ceb0380a9551c9979db018149 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

My independent take before reading the diff: close the i18n gap by running npm run check-i18n in both autofix verification paths, and close the cross-package gap by running the Web Shell drift test only when packages/core/src/tools/tool-names.ts changes — staging the gate from the trusted base so a candidate branch can't tamper with it. The PR does essentially this, and I didn't find a simpler path it missed.

No critical blockers and no convention violations. Notes from the read:

  • Security model is correct. The script is cp'd from the base checkout into ${RUNNER_TEMP} before the candidate branch is selected (the same pattern as check-settings-schema.sh, staged in exactly 2 places — asserted by the test), and the gate is invoked from ${RUNNER_TEMP}, never from the working tree. The only branch-controlled input is the changed-file list, consumed solely by a fixed-string whole-line grep -Fxq (no eval, no injection). A malicious branch can only influence whether the drift test runs, and including tool-names.ts just runs more verification.
  • set -uo pipefail without -e is deliberate and safe. I traced every fallible command: both npm calls are wrapped in explicit if ! …; then fail; fi, the grep lives in an if condition, and a no-match if returns 0 — so there's no unchecked command that could fail silently and cause a false pass. ${GITHUB_OUTPUT:-} guards the -u when the var is unset.
  • Wiring & ordering verified. Both invocation sites run the gate right after the schema gate; in the review path it runs before the no-op acceptance (git diff --quiet …), so even a no-op review response must pass the contract gate — matching the PR's stated intent. BRANCH is already in scope at both sites (the adjacent no-op check uses it).
  • Reuse is good — reuses the existing staging pattern, the existing check-i18n script, and the existing drift test; no parallel utility. The conditional drift test (only on tool-names.ts) vs. always-on i18n is a sensible cost split.
  • The behavioral test spawns the real script with a stub npm and covers all four branches (non-contract, contract, i18n-fail, drift-fail) including the outcome=failed write.

Real-scenario testing

Checked out the reviewed commit a5c39e2 into an isolated worktree, ran npm ci (2012 packages), and drove the real scripts in tmux.

1. Workflow regression suite (vitest run … qwen-autofix-workflow.test.js) — 102/102:

 RUN  v3.2.4 …/worktrees/triage

 ✓ scripts/tests/qwen-autofix-workflow.test.js (102 tests) 5188ms

 Test Files  1 passed (1)
      Tests  102 passed (102)
   Duration  5.53s

2. New gate, non-contract path, real npm run check-i18n (expect: i18n only, exit 0):

$ echo 'packages/core/src/config/config.ts' | bash .github/scripts/check-autofix-contracts.sh
> @qwen-code/qwen-code@0.20.1 check-i18n
> npm run check-i18n --workspace=packages/cli
  … [i18n locale-key report, ~200 lines] …
JSON report not written. Re-run with --write-unused-locale-keys … to update it.

✅ All checks passed!

EXIT_CODE=0

3. New gate, contract path tool-names.ts, real npm (expect: i18n and Web Shell drift test, exit 0):

$ echo 'packages/core/src/tools/tool-names.ts' | bash .github/scripts/check-autofix-contracts.sh
✅ All checks passed!          # ← i18n

> @qwen-code/web-shell@0.20.1 test
> vitest run --config vitest.config.ts client/components/messages/toolFormatting.drift.test.ts

 ✓ components/messages/toolFormatting.drift.test.ts (2 tests) 3ms

 Test Files  1 passed (1)
      Tests  2 passed (2)

EXIT_CODE=0

4. Failure path (stub npm exiting 1, expect: exit 1 + outcome=failed):

$ echo 'packages/core/src/tools/tool-names.ts' | PATH="$STUB:$PATH" GITHUB_OUTPUT=$OUT bash .github/scripts/check-autofix-contracts.sh
❌ i18n verification failed.
EXIT_CODE=1
GITHUB_OUTPUT:
outcome=failed

The gate runs the real i18n check on every candidate, additionally runs the Web Shell drift test exactly when the tool-name contract changes, and rejects with an explicit outcome=failed on failure. Behaves as advertised.

中文说明

代码审查

读 diff 前我的独立方案:在两条 autofix 验证路径中运行 npm run check-i18n 关闭 i18n gap;仅当 packages/core/src/tools/tool-names.ts 变化时运行 Web Shell drift 测试关闭跨包 gap;并把 gate 从 trusted base 暂存,使候选分支无法篡改。PR 基本就是这么做的,我没找到它遗漏的更简路径。

无关键阻塞项,无规范违规。阅读要点:

  • 安全模型正确。 脚本在切换到候选分支之前从 base checkout cp${RUNNER_TEMP}(与 check-settings-schema.sh 同一模式,恰好 2 处暂存——由测试断言),gate 从 ${RUNNER_TEMP} 调用,绝不来自工作树。唯一的分支可控输入是 changed-file 列表,仅被固定字符串整行 grep -Fxq 消费(无 eval、无注入)。恶意分支只能影响 drift 测试是否运行,而包含 tool-names.ts 只会触发更多验证。
  • set -uo pipefail 不带 -e 是刻意且安全的。 我追踪了每个可能失败的命令:两个 npm 调用都包在显式 if ! …; then fail; fi 中,grep 位于 if 条件内,无匹配的 if 返回 0——因此没有未被检查的命令会静默失败导致误放行。${GITHUB_OUTPUT:-} 在该变量未设置时保护 -u
  • 接线与顺序已核实。 两处调用点都在 schema gate 之后运行该 gate;review 路径中它在接受 no-op(git diff --quiet …之前运行,因此即便是 no-op 的 review 响应也必须通过 contract gate——与 PR 所述意图一致。BRANCH 在两处都已在作用域内(相邻的 no-op 检查就用到了它)。
  • 复用良好——复用现有暂存模式、现有 check-i18n 脚本和现有 drift 测试;无并行工具。条件式 drift 测试(仅 tool-names.ts)对比始终开启的 i18n,是合理的成本拆分。
  • 行为测试用 stub npm 启动真实脚本,覆盖全部四个分支(非 contract、contract、i18n 失败、drift 失败),包括 outcome=failed 的写入。

真实场景测试

将受审 commit a5c39e2 检出到隔离 worktree,运行 npm ci(2012 个包),并在 tmux 中驱动真实脚本。

  1. workflow 回归套件 102/102 通过。
  2. 新 gate 非 contract 路径 + 真实 npm run check-i18n:仅 i18n,✅ All checks passed!,退出码 0。
  3. 新 gate contract 路径 tool-names.ts + 真实 npm:i18n 加 Web Shell drift 测试(2/2),退出码 0。
  4. 失败路径(stub npm 退出 1):退出码 1,GITHUB_OUTPUT 写入 outcome=failed

gate 对每个候选都运行真实 i18n 检查,仅在 tool-name contract 变化时额外运行 Web Shell drift 测试,失败时以明确的 outcome=failed 拒绝。行为与描述一致。

Qwen Code · qwen3.8-max-preview

Reviewed at a5c39e2f10e8e18ceb0380a9551c9979db018149 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — clean across every stage; the gap is real and independently verified, the fix is minimal and safe, and I confirmed it works end-to-end.

Stepping back: this is exactly the kind of PR the gate should wave through. I verified both gaps against the code rather than taking the description at face value — check-i18n is required by full CI but absent from the autofix gate on main, and the Web Shell drift test reads tool-names.ts over the filesystem (web-shell deliberately doesn't depend on core), so the import-graph/owning-package test selection genuinely can't see it. The diagnosis is precise, not hand-waved.

My independent proposal matched the PR; I didn't find a simpler path it missed. The implementation is a 24-line gate that reuses the existing check-settings-schema.sh "stage from trusted base" pattern, adds 6 lines of wiring, and backs it with thorough regression coverage (wiring assertions plus a four-branch behavioral test). Every line is on-topic — no drive-by changes, no scope creep. It's phase 1 of #7638, scoped exactly as the issue describes ("three independently reviewable phases").

The risk profile is asymmetric in the safe direction: this only ever makes the autofix gate stricter, so it can't let a broken commit through — the worst case is a false-positive rejection, which is visible and reversible. After running it for real (102/102 suite, both happy paths with actual npm, and a forced failure writing outcome=failed), the behavior matches the promise. In six months I'd thank the author, not curse them — it follows the established security model and the test guards against drift.

No reservations to name. Shipping it. ✅

中文说明

置信度:5/5 —— 每个阶段都干净;gap 真实且已独立核实,修复最小且安全,并已端到端确认可用。

退一步看:这正是 gate 应当放行的 PR。我没有照单全收描述,而是对照代码核实了两个 gap——check-i18n 被完整 CI 要求,但 main 上的 autofix gate 缺失它;Web Shell drift 测试通过文件系统读取 tool-names.ts(web-shell 刻意不依赖 core),所以基于 import graph / owning-package 的测试选择确实看不到它。诊断精确,而非泛泛而谈。

我的独立方案与 PR 一致;没找到它遗漏的更简路径。实现是一个 24 行的 gate,复用现有 check-settings-schema.sh 的"从 trusted base 暂存"模式,新增 6 行接线,并以充分的回归覆盖支撑(接线断言加四分支行为测试)。每一行都切题——无顺手改动,无范围蔓延。这是 #7638 的第一阶段,范围与 issue 描述完全一致("三个可独立审查的阶段")。

风险偏向安全方向:它只会让 autofix gate 更严格,因此不会放行损坏的 commit——最坏情况是误拒,可见且可逆。真实运行后(102/102 套件、用真实 npm 跑通两条 happy path、以及强制失败写入 outcome=failed),行为与承诺一致。六个月后我会感谢作者,而非埋怨——它遵循既有安全模型,且测试防止漂移。

没有需要指出的保留意见。发布。✅

Qwen Code · qwen3.8-max-preview

Reviewed at a5c39e2f10e8e18ceb0380a9551c9979db018149 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@wenshao
wenshao added this pull request to the merge queue Jul 24, 2026
Merged via the queue into QwenLM:main with commit cb98102 Jul 24, 2026
76 checks passed
@github-actions github-actions Bot added the skip-changelog-auto Automatically exclude internal CI changes from release notes label Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog-auto Automatically exclude internal CI changes from release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants