Skip to content

chore(core): wrap two over-width cancellation assertions - #11232

Merged
wenshao merged 1 commit into
mainfrom
chore/format-core-tool-scheduler-test
Sep 7, 2026
Merged

wenshao merged 1 commit into
mainfrom
chore/format-core-tool-scheduler-test

Conversation

@wenshao

@wenshao wenshao commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Wraps two assertions in the tool-cancellation tests whose string arguments outgrew the 80-column print width, putting each argument on its own line. Whitespace and two trailing commas only — no token changes, no behaviour change. This is Prettier's own output for the file, nothing hand-written.

Why it's needed

main is red. The push run for 62588d2892 (run 34070970087) died in Lint & Static at the Prettier step:

Checking formatting...
[warn] packages/core/src/core/coreToolScheduler.test.ts
[warn] Code style issues found in 1 file. Run Prettier with --write to fix.

Every step behind that gate in the job was skipped, and every job that needs it was skipped too, so two long lines turned the whole run red. Test (ubuntu-latest) passed in the same run, so this is the only thing standing between main and green.

How it landed is a race rather than carelessness. The cancellation-message rewrite lengthened two short assertion strings into long ones and reached main without a format pass — but the green check that authorized the merge was produced seventeen hours earlier, under a Prettier lane that ran prettier --write . and exited 0 no matter what it rewrote. Four minutes before the merge, that lane flipped to prettier --check ., and nothing re-ran CI in between. ESLint cannot cover the gap: the shared config loads eslint-config-prettier, which disables formatting rules instead of enforcing them, so the lane that just became strict was the only enforcement point outside the pre-commit hook.

Reviewer Test Plan

How to verify

  • Run the exact command the failing step runs: node scripts/lint.js --prettier. Before this PR it names the one file above and exits 1; after, it prints All matched files use Prettier code style! and exits 0.
  • cd packages/core && npx vitest run src/core/coreToolScheduler.test.ts → 406 passed, 0 skipped.
  • To confirm the two rewritten assertions still execute rather than being quietly filtered out: cd packages/core && npx vitest run src/core/coreToolScheduler.test.ts -t 'tells the model a' → 2 passed, 404 skipped.
  • The change is provably formatting-only. Stripping all whitespace, and all commas that directly precede a closing paren, from both revisions of the file yields byte-identical strings.
  • Local Prettier is 3.6.1, the exact version pinned in the lockfile, so the formatting here is what CI will compute.

Evidence (Before & After)

N/A — non-user-visible formatting change. The gate output above is the before/after.

Tested on

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

Environment (optional)

Node 22.22.2, npm run build --workspace=packages/core then vitest, no sandbox involved.

Risk & Scope

  • Main risk or tradeoff: none material. The diff is whitespace and two trailing commas in a test file, and it is the formatter's own output rather than a human judgment about style.
  • Not validated / out of scope: three of the nine steps that were skipped behind the gate could not be reproduced in this checkout, and I am not claiming they are green from local runs. Check serve fast-path bundle closure and Check VS Code companion notices are up-to-date fail locally because @tanstack/react-table is absent from node_modules — the same reason a local full build stops at the web-shell package, and unrelated to this diff. Run .github/scripts helper tests fails 42 subtests locally on macOS because the flakiness gate's integrity checks call GNU stat -c with no BSD stat -f fallback, while CI runs them on Ubuntu. All three are pre-existing local artifacts, not regressions: the helper tests fail identically at 7567824d4c, the last main commit whose Lint & Static run was green, and no triage or flake file changed between there and this branch. The other six steps behind the gate pass locally — sensitive keyword linter, i18n check, settings schema generation, settings schema freshness, notices generation, and core subpath exports — as do ESLint on the touched file and tsc --noEmit on the touched package, neither of which sits behind the gate.
  • Breaking changes / migration notes: none.
  • Worth a follow-up, deliberately not bundled here: the merge path accepted a required check whose result was computed before a base-branch workflow change made that check stricter. Any gate that tightens will re-run this scenario against whatever PRs are in flight, and the first red appears on main rather than on the PR.

Linked Issues

Refs #11109, the issue behind the Prettier gate that made this failure visible. Does not close it.

中文说明

这个 PR 做了什么

把工具取消相关测试里两个字符串实参超出 80 列打印宽度的断言换行,让每个实参各占一行。只涉及空白字符和两个尾逗号——没有 token 变化,没有行为变化。这就是 Prettier 对该文件自己的输出,没有任何手写内容。

为什么需要

main 现在是红的。62588d2892 的 push run(run 34070970087)在 Lint & Static 的 Prettier 步骤挂掉:

Checking formatting...
[warn] packages/core/src/core/coreToolScheduler.test.ts
[warn] Code style issues found in 1 file. Run Prettier with --write to fix.

该 job 里 gate 之后的所有步骤被跳过,所有 needs 它的 job 也被跳过,于是两行超长代码把整个 run 染红。同一次 run 里 Test (ubuntu-latest) 是成功的,所以这是 main 转绿之前唯一的障碍。

它是怎么进 main 的:这是一次时序竞态,而不是谁不小心。取消提示语的改写把两个短断言字符串变长了,进 main 前没有跑格式化——但授权那次合并的绿灯是十七小时前产出的,当时 Prettier lane 跑的还是 prettier --write .,无论重写了什么都退出 0。合并前四分钟,这条 lane 翻成了 prettier --check .,而中间没有任何东西重跑 CI。ESLint 补不上这个缺口:共享配置加载了 eslint-config-prettier,它是关闭格式规则而不是强制执行,所以刚刚变严格的这条 lane 是 pre-commit hook 之外唯一的强制点。

Reviewer 测试计划

如何验证

  • 跑失败步骤的原样命令:node scripts/lint.js --prettier。本 PR 之前它会点名上面那个文件并退出 1;之后输出 All matched files use Prettier code style! 并退出 0。
  • cd packages/core && npx vitest run src/core/coreToolScheduler.test.ts → 406 passed,0 skipped。
  • 确认两处被改写的断言确实还在执行、没有被悄悄过滤掉:cd packages/core && npx vitest run src/core/coreToolScheduler.test.ts -t 'tells the model a' → 2 passed,404 skipped。
  • 这次改动可证明是纯格式的。把文件两个版本里的所有空白字符、以及所有紧邻右括号前的逗号都剥掉之后,得到的字符串逐字节相同。
  • 本地 Prettier 是 3.6.1,与 lockfile 里钉住的版本完全一致,所以这里的格式就是 CI 会算出来的格式。

证据(Before & After)

N/A——非用户可见的格式改动。上面的 gate 输出就是 before/after。

测试环境

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

运行环境(可选)

Node 22.22.2,npm run build --workspace=packages/core 之后跑 vitest,不涉及 sandbox。

风险与范围

  • 主要风险或取舍:无实质风险。diff 是一个测试文件里的空白字符和两个尾逗号,而且是格式化器自己的输出,不是人对风格的判断。
  • 未验证 / 范围之外:gate 之后被跳过的九个步骤里有三个在我这份工作副本中无法复现,我不声称它们在本地是绿的。Check serve fast-path bundle closure 和 Check VS Code companion notices are up-to-date 在本地失败,原因是 node_modules 里缺 @tanstack/react-table——这也是本地全量构建停在 web-shell 包的原因,与本 diff 无关。Run .github/scripts helper tests 在 macOS 本地失败 42 个子测试,因为 flakiness gate 的完整性校验调用 GNU stat -c 且没有 BSD stat -f 回退,而 CI 是在 Ubuntu 上跑它们的。这三个都是既有的本地环境产物,不是回归:helper tests 在 7567824d4c(Lint & Static 上一次全绿的 main commit)上失败情况完全相同,且从那以后到这个分支之间没有任何 triage 或 flake 文件被改动。gate 之后其余六个步骤本地全部通过——敏感词 linter、i18n 检查、settings schema 生成、settings schema 新鲜度、notices 生成、core subpath exports;被改动文件的 ESLint 与被改动包的 tsc --noEmit 也通过,这两者都不在 gate 之后。
  • 破坏性变更 / 迁移说明:无。
  • 值得后续跟进、这里刻意不捆绑:合并路径接受了一个「其结果是在 base 分支 workflow 变更让该检查变严格之前算出来的」必需检查。任何收紧的 gate 都会对当时在飞的 PR 重演这个场景,而第一个红叉出现在 main 上,不是 PR 上。

关联 Issue

Refs #11109,即让这次失败显现出来的 Prettier gate 背后的 issue。不关闭它。

#10180 lengthened two assertion strings in the cancellation-message tests
past the 80-column print width and reached main without a format pass. The
Prettier lane had flipped from `--write` to `--check` four minutes earlier in
#11117, and that PR's own green lint run predated the flip by seventeen hours,
so nothing caught it until the post-merge push run failed and skipped every
step downstream of the gate.

Whitespace and trailing commas only: the file's normalized token stream is
byte-identical to its parent, and both enclosing tests still execute and pass.

@qqqys qqqys 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.

APPROVE (verified at head cb47b95)

Pure formatting: the two over-width cancellation assertions from #10180 are wrapped onto continuation lines with the string literals byte-identical ('User intentionally cancelled this tool call.' and the negative '… Stop' discriminator unchanged), so the assertions' semantics are provably untouched by reading the diff itself. No review history exists on this PR yet, and CI shows 7 green with zero failures (the remaining lanes are still queued/running from the fresh push — non-gating; the changed file belongs to the packages/core suite that those lanes execute).

@doudouOUC doudouOUC 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✅

@wenshao
wenshao added this pull request to the merge queue Sep 7, 2026

@yiliang114 yiliang114 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

Merged via the queue into main with commit 421393d Sep 7, 2026
119 of 121 checks passed
@wenshao

wenshao commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Answering all three stage comments in one place — issue comments here are flat, and three separate replies would put three notifications on an already-merged PR. Nothing in the diff changes; this is about the record.

Stage 1 — the correction is right, and my sentence was wrong as written

I enumerated the jobs of the Qwen Code CI run for 62588d2892 and stopped there. E2E Tests is a separate workflow that ran on the same SHA and I never opened it, so "the only thing standing between main and green" was scoped to one workflow while reading as scoped to the branch. Verified independently rather than taken on trust: run 34070970091 is failure, its only failing job is E2E Test - macOS - shard 2/2, and the named test exists at integration-tests/cli/acp-integration.test.ts:962.

Where that red stands now, since it bears on whether anyone should still expect it: it did not persist past the merge, and it has not come back. Counting every E2E Tests run on main from the merge commit onward as of this writing rather than sampling — 18 runs, 10 success, 5 cancelled, 1 still in progress — there are exactly 2 failures, and both are a different job, E2E Test (Linux) - sandbox:none - shard 1/3: ac01395240 (scheduled run 34082757271, 04:20) and fb12a6e7fe (push run 34083672277, 04:35). E2E Tests on the merge commit 421393d51d itself (push run 34076271660) is success. So the macOS shard you named is not among the failures — but I am not holding it up as stable either, because that Linux shard plainly is not. The narrow claim is that the specific red you identified had cleared by the merge commit and has not reappeared since.

Stage 2 — closing the item you explicitly left unverified

You recorded that you could not verify "whether Run Prettier passes on this commit" because the check was still in flight. It passed. Lint & Static (ubuntu-latest, Node 22.x) on cb47b95122 is job 101597096093, success; step 25 Run Prettier is success, and all nine steps behind the gate that were skipped on main also completed success — 26 sensitive keyword linter, 27 i18n, 28/29 settings schema generation and freshness, 30/31 notices generation and freshness, 32 serve fast-path bundle closure, 33 core subpath exports, 34 helper tests.

Confirmed from the branch side as well: step 25 is success in the push run for the merge commit (34076271669), and the whole job is success in about 36 minutes in the next one (ac01395240, run 34081391249). Stage 3 below counts every run since, so I will not restate the tally here.

One thing your table could not show, because it happened after you read it: Test (ubuntu-latest, Node 22.x) on cb47b95122 never went green. It was cancelled at the ceiling — The job has exceeded the maximum execution time of 2h0m0s — on host ecs-qwen-hk3-2, where Install dependencies alone took 23 minutes against 5 minutes on a healthy host 46 minutes earlier. So "no check on this commit has failed" was true when you wrote it and stopped being true afterwards, though not because of anything in the diff.

Stage 3 — the deferred approval never resolved, and the reason is worth recording

No commit-pinned approval landed on cb47b95122; the review list is three human approvals and nothing from the bot. Your condition was "every check on that SHA completes green", and Test (ubuntu-latest, Node 22.x) was cancelled rather than green, so it was never satisfied. The PR merged at 02:26:00Z while that job was still running; it was cancelled at 03:49:28Z.

The gate waited for a result that infrastructure was never going to produce, and the shape of that failure matters for whoever reads it next: the Test (ubuntu) failure in run 34076271669 on ecs-qwen-hk5-31 had zero failing tests — cli 28857 passed, core 23655 passed, exit 1 from Error: [vitest-worker]: Timeout calling "onTaskUpdate", under a load average that peaked at 194.49 across 400 samples with disk at 87–88%. The annotation it leaves behind says only Process completed with exit code 1., which points whoever reads it at the tests rather than at the host.

Where the gate stands since, counted rather than sampled — again as of this writing, main moves fast: of the 17 Qwen Code CI push runs on main from this merge commit onward, 9 are success, 5 were cancelled before starting any job at all (superseded by a newer push, so they say nothing), and 3 did not finish clean. In those 3 the job that failed or hit a ceiling is Test (ubuntu-latest, Node 22.x) every time, and Lint & Static failed in none of them:

  • 421393d51d (run 34076271669) — Test (ubuntu) failure, as above; Lint & Static cancelled at its own ceiling (The job has exceeded the maximum execution time of 1h30m0s), but only at step 33, after step 25 Run Prettier had already returned success along with steps 26–32.
  • ac01395240 (run 34081391249) — Lint & Static success in about 36 minutes; Test (ubuntu) cancelled at the 120-minute ceiling.
  • 36f2c14f0e (run 34088422677) — Lint & Static success on hk4-23; Test (ubuntu) failure on hk5-21.

Run Prettier is success in all three and in the three most recent completed runs (34108897396, 34112356776, 34114492637). So the gate this PR restored has held on every commit since it landed; what is still costing main a clean board is the unit-test job on the shared ECS pool, which is a separate problem from the one this PR was opened for.

On the follow-up this PR declined to bundle — a required check whose green result was computed before a base-branch workflow change made that check stricter — I agree with your reading that it is a merge-path property and does not belong in an eight-line gate fix. It is still unaddressed.

The four hours after this PR opened surfaced a second gap, and I would rather name the difference between them than fold them together. The one this PR raised is a stale green being trusted. Yours is a deferral that never reached either of its two outcomes: you wrote that the approval would follow automatically once every check completed green, and that it would be "withheld and flagged here" if anything landed red. Test (ubuntu-latest, Node 22.x) was cancelled, which is neither. So the condition could never become true, the approval never came, and nothing was posted here to record that it had failed rather than merely not yet succeeded — all four of your comments predate that cancellation by over an hour and a half. The PR merged on three human approvals with the deferred condition silently unresolved. A deferral that cannot distinguish "not green yet" from "will never be green" needs a terminal branch, and a cancelled check is exactly the input that finds the missing one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants