Skip to content

test(cli): keep the redaction ReDoS guard asserting on the shared pool - #10912

Merged
wenshao merged 3 commits into
mainfrom
test/latency-budgets-remaining-workspaces
Sep 3, 2026
Merged

test(cli): keep the redaction ReDoS guard asserting on the shared pool#10912
wenshao merged 3 commits into
mainfrom
test/latency-budgets-remaining-workspaces

Conversation

@yiliang114

@yiliang114 yiliang114 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Restores the missing pool bound on the one migrated ReDoS-shaped guard that was left asserting nothing on the shared ECS pool, and corrects the criterion recorded in the latency-budget helper's own test comment.

Why it's needed

channel-worker-supervisor.test.ts pushes 66 KB of a. through the credential-redaction path and bounds it at 1000 ms. #10870 quarantined that bound with no poolMultiplier, so on the shared pool the case runs and asserts nothing about the property it exists to defend. A backtracking regression in the redaction regex shows up as time, while the sibling assertions in the same case check truncation, which a slow-but-correct regex still satisfies. On the pool lane the case was therefore green against exactly the regression it guards.

The original sweep missed it because it matched on test namesstays linear on pathological inputs, does not catastrophically backtrack … (ReDoS), runs in bounded time on pathological input. This one is named for its behaviour, handles long non-url worker log lines while applying credential redaction, and the adversarial signal is 'a.'.repeat(33_000) in the body rather than in the name. Name-matching was the wrong instrument, and this is the site it missed.

Separately, latency-budget.test.ts still described the selection rule as "Three of the quarantined cases have no other expect()". The count is stale, and more importantly that reading is the one the review showed to be too narrow: it is what left complexity guards asserting only things a quadratic regression still satisfies. The comment should record the criterion actually applied — whether the duration is the property under test.

Reviewer Test Plan

How to verify

The change adds poolMultiplier: 20 to a single call, consistent with the other 1000 ms guards. A reviewer should confirm the multiplier is sound in three respects: 20x is well clear of the ~5x contention measured on that fleet; it stays far under a quadratic blowup, so the guard still catches what it exists for; and the enclosing case carries no per-test timeout, so it takes the package's 60s pool ceiling and the assertion — not vitest — still decides the outcome. That makes 19 of the 34 quarantined sites relaxed rather than skipped.

The helper's own suites are self-contained and were run in isolation. The full package suites need npm run build for workspace dist/ output, which was not run here; the supervisor change adds an options object to a call whose signature already accepts it, in the same shape as the 18 sites CI has already run green.

Evidence (Before & After)

N/A for user-visible behaviour — this is a test-only change.

Before, on the pool lane the helper returns early when no multiplier is supplied, so the 1000 ms bound was not evaluated at all. After, the pool lane asserts against 20 × 1000 ms = 20 s, which sits under the package's 60 s pool testTimeout.

Both copies of the helper suite pass, and remain byte-identical after the comment change:

✓ packages/core/src/test-utils/latency-budget.test.ts (6 tests)
✓ packages/cli/src/test-utils/latency-budget.test.ts  (6 tests)
Test Files  2 passed (2)
     Tests  12 passed (12)

Site census backing the corrected comment — 19 poolMultiplier sites out of 34 total calls, at three values (13 × 20, 4 × 10, 2 × 5). prettier --check clean on all three changed files.

Tested on

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

Environment (optional)

Unit tests only (Node 22, vitest 3.2.4); the two helper suites run standalone without a workspace build.

Risk & Scope

  • Main risk or tradeoff: test-only. The change can only make an assertion stricter on the pool lane, never looser, and does not touch the local or GitHub-hosted lanes at all.
  • Not validated / out of scope: the full packages/cli suite was not run here because it needs a workspace build first, so the supervisor file's green run comes from CI. Also deliberately out of scope are the seven raw wall-clock budgets in four workspaces outside packages/{cli,core} — see below.
  • Breaking changes / migration notes: none.

What this PR deliberately does not do, and why

@wenshao's post-merge verification also found seven raw wall-clock budgets outside packages/{cli,core}sdk-typescript ×4, qwen-live, channels/base, channels/dingtalk ×2 — all on the same pool hosts with no code path for the flag. I confirmed all seven, and that all four packages do run in the npm run test:ci --workspaces lane. They are not fixed here because it is not the mechanical helper-copy it looks like.

First, there is no shared home: only sdk-typescript depends on core, channels/base has no qwen dependencies at all, and there is no root-level shared test-utils precedent in this repo. Adding a channels/base → core edge for a test-only helper would be real coupling.

Second, and decisively, for the two channels packages quarantining the budget first would have bought almost nothing. Neither package set testTimeout, so both ran on vitest's 5000 ms default against 1000 ms budgets — a largest usable multiplier of about 4x, while dingtalk's own comment measures the quadratic cost it guards at ~4.1 s, and the fleet's contention factor is itself ~5x. There was no room. #10915 is the prerequisite that creates it, by giving those packages a 60 s pool ceiling; once it lands, these budgets can take a poolMultiplier in a follow-up. sdk-typescript's four are unrelated and easy — all four are scaffolding upper bounds ("generous bound — just confirms the timer fired") that want a plain skip.

Linked Issues

Follow-up to #10870, where this site was raised by @doudouOUC (S3) and confirmed post-merge by @wenshao. Prerequisite for the deferred half: #10915. Underlying flakiness umbrella: #10490.

中文说明

这个 PR 做了什么

为唯一一处迁移后在共享 ECS 池上不再断言任何东西的 ReDoS 型守卫补回池上的上界,并修正 latency-budget helper 自身测试注释中记录的判据。

为什么需要

channel-worker-supervisor.test.ts 把 66 KB 的 a. 推过凭据脱敏路径,并以 1000 ms 为界。#10870 在隔离该上界时没有带 poolMultiplier,因此在共享池上这个用例会运行,却对它本要守护的属性不作任何断言。脱敏正则的回溯回归表现为时间,而同一用例中的其它断言检查的是截断,而一个慢但正确的正则仍然满足截断。于是在池通道上,这个用例恰恰对它所守护的那种回归是绿的。

最初的排查漏掉了它,因为那次是按测试名字匹配的 —— stays linear on pathological inputsdoes not catastrophically backtrack … (ReDoS)runs in bounded time on pathological input。而这一处是按行为命名的:handles long non-url worker log lines while applying credential redaction,对抗性信号是函数体里的 'a.'.repeat(33_000),不在名字里。按名字匹配这个方法本身就不对,这就是它漏掉的那处。

另外,latency-budget.test.ts 仍把选取规则描述为「三处被隔离的用例没有别的 expect()」。计数已过期,更重要的是这个判读正是评审证明过于狭窄的那一个:它导致复杂度守卫只保留了二次回归依然能满足的断言。注释应当记录实际采用的判据 —— 时长是否就是被测属性。

评审验证方案

如何验证

改动为单个调用添加 poolMultiplier: 20,与其它 1000 ms 守卫一致。评审者应从三方面确认该倍数是合理的:20 倍远高于该机群实测的约 5 倍争抢;它仍远低于二次爆炸的量级,因此守卫仍能抓住它存在的意义所在;以及外层用例没有 per-test 超时,会采用该包 60 秒的池上限,因此仍由断言而非 vitest 决定结果。这使得 34 处被隔离站点中有 19 处是放宽而非跳过。

helper 自身的两个套件是自包含的,已独立运行。完整的包级套件需要先 npm run build 生成工作区 dist/ 产物,此处未运行;supervisor 的改动只是为一个签名本就接受该参数的调用增加了选项对象,形状与 CI 已经跑绿的 18 处一致。

证据(前后对比)

用户可见行为方面为 N/A —— 这是一处纯测试改动。

修改前:在池通道上,未提供倍数时 helper 会提前返回,因此 1000 ms 的上界根本没有被求值。修改后:池通道对 20 × 1000 ms = 20 秒作断言,该值位于该包 60 秒池 testTimeout 之下。

helper 的两份套件均通过,且在注释改动后仍逐字节一致:

✓ packages/core/src/test-utils/latency-budget.test.ts (6 tests)
✓ packages/cli/src/test-utils/latency-budget.test.ts  (6 tests)
Test Files  2 passed (2)
     Tests  12 passed (12)

支撑修正后注释的站点普查 —— 34 处调用中有 19 处带 poolMultiplier,取三种值(13 处 ×20、4 处 ×10、2 处 ×5)。三个改动文件的 prettier --check 干净。

测试平台

系统 状态
🍏 macOS ⚠️
🪟 Windows ⚠️
🐧 Linux

运行环境(可选)

仅单元测试(Node 22、vitest 3.2.4);两个 helper 套件无需工作区构建即可独立运行。

风险与范围

  • 主要风险或权衡:纯测试改动。该改动只可能让池通道上的断言更严格,绝不会更松,且完全不影响本地与 GitHub 托管通道。
  • 未验证 / 范围之外:完整的 packages/cli 套件此处未运行,因为需要先做工作区构建,因此 supervisor 文件的绿色结果来自 CI。另外刻意排除在外的还有 packages/{cli,core} 之外四个工作区中的七处裸挂钟预算 —— 见下。
  • 破坏性变更 / 迁移说明:无。

本 PR 刻意不做什么,以及为什么

@wenshao 的合并后验证还发现了 packages/{cli,core} 之外的七处裸挂钟预算 —— sdk-typescript 4 处、qwen-livechannels/basechannels/dingtalk 2 处 —— 全都跑在同样的池主机上,且没有该开关的任何代码路径。我确认了这七处全部存在,也确认这四个包确实都在 npm run test:ci --workspaces 通道内。此处不修,是因为它并不是看上去那样机械地复制一份 helper 就行。

其一,没有共同的归属地:只有 sdk-typescript 依赖 corechannels/base 完全没有 qwen 依赖,而本仓库也不存在根级共享 test-utils 的先例。为一个纯测试 helper 新增 channels/base → core 的依赖边是真实的耦合。

其二,也是决定性的:对那两个 channels 包而言,先隔离预算几乎不会带来任何收益。这两个包都没有设置 testTimeout,因此跑在 vitest 的 5000 ms 默认值上、对着 1000 ms 的预算 —— 可用倍数最大约为 4 倍,而 dingtalk 自己的注释实测它所守护的二次代价为约 4.1 秒,机群自身的争抢因子又是约 5 倍。根本没有余量。#10915 正是创造余量的前置条件,它为这些包提供 60 秒的池上限;待其合入后,这些预算即可在后续 PR 中带上 poolMultipliersdk-typescript 的四处与此无关且简单 —— 四处都是脚手架性质的上界(注释写着「generous bound —— 只是确认定时器触发了」),直接跳过即可。

关联 Issue

#10870 的后续,该站点由 @doudouOUC(S3)提出并由 @wenshao 在合并后确认。被推迟部分的前置条件:#10915。底层 flakiness 总 issue:#10490

`channel-worker-supervisor.test.ts` pushes 66 KB of `a.` through the
credential-redaction path and bounds it at 1000 ms. #10870 quarantined that
bound without a `poolMultiplier`, so on the shared pool the case runs and
asserts nothing about the property it exists to defend: a backtracking
regression in the redaction regex shows up as time, and the sibling assertions
in the case check truncation, which a slow-but-correct regex still satisfies.

It is the same family as the 13 complexity guards that already carry a
multiplier — it was missed because the sweep that found those matched on test
names (`stays linear`, `ReDoS`, `runs in bounded time`) and this one is named
for its behaviour, `handles long non-url worker log lines while applying
credential redaction`. The adversarial signal is in the body, not the name.

Kept at 20x, consistent with the other 1000 ms guards: well clear of the ~5x
contention on that fleet, far under a quadratic blowup, and the enclosing case
takes the package's 60s pool timeout, so the assertion still decides the
outcome. That makes 19 of the 34 sites relaxed rather than skipped.

Also corrects the criterion recorded in `latency-budget.test.ts` (both copies,
kept byte-identical). It still said "three of the quarantined cases have no
other expect()" — the count is stale, and more importantly that reading was the
one the review showed to be too narrow: it left complexity guards asserting only
what a quadratic regression still passes. The comment now states the criterion
actually applied, that the duration *is* the property, and why the multiplier
varies with the lane's timeout.

Follow-up to #10870, raised there by doudouOUC (S3) and confirmed post-merge by
wenshao.
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Deferred approval not posted — the PR head moved (or the PR closed) after the review of c4e30ef; approving now would attest to unreviewed code. Re-run @qwen-code /triage on the new head. finalize run

⚠️ 延迟审批未提交 —— 审查 c4e30ef 之后 PR head 已变更(或 PR 已关闭),此时审批会为未审查的代码背书。请在新 head 上重新运行 @qwen-code /triage查看 finalize 运行

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

@yiliang114 the description is genuinely thorough — but it doesn't follow the PR template, so the gate stops here on shape before content.

What's missing against the template: Why it's needed (the material is there, under "The guard" and "The stale criterion"), the whole Reviewer Test Plan block — How to verify / Evidence (Before & After) / Tested on — currently a free-form "Verification" section, Risk & Scope (currently just "Risk"), Linked Issues, and the bilingual Chinese <details> translation. Every recently merged PR follows the template — including your own #10870 — so this one stands out.

On the merits, nothing looks wrong: the supervisor case really did lose its bound on the pool lane (the helper returns without asserting when poolMultiplier is absent), the 20x choice is consistent with the sibling guards, and the "19 of 34" comment refresh matches the actual site count. Please restructure the body into the template headings and this can go back through review — @qwen-code /triage re-runs it.

中文说明

@yiliang114 描述写得很充分——但没有遵循 PR 模板,所以门禁在此先卡在格式上,再看内容。

相对模板缺失的部分:Why it's needed(内容实际在 "The guard" 和 "The stale criterion" 里)、整个 Reviewer Test Plan 块(How to verify / Evidence (Before & After) / Tested on,目前是一个自由格式的 "Verification" 小节)、Risk & Scope(目前只有 "Risk")、Linked Issues,以及双语的中文 <details> 翻译。最近合并的 PR 都遵循模板——包括你自己的 #10870——这一篇因此显得突兀。

就内容本身而言没有看出问题:supervisor 用例在池通道上确实丢了上限(缺少 poolMultiplier 时 helper 直接返回、不做任何断言),20 倍的选择与同类守卫一致,注释里 "19 of 34" 的更新也与实际站点数吻合。请把正文重排进模板标题,即可重新进入审查——回复 @qwen-code /triage 重新运行。

Qwen Code · qwen3.8-max

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Prerequisite is up: #10915.

It turned out to be a wider gap than the four packages named in the review. A census of all 22 vitest projects that npm run test:ci --workspaces runs found fifteen still on vitest's 5000 ms default, not two — every channels package, audio-capture, chrome-extension, webui and both external-context integrations. 99 test files, ~3170 cases. #10915 gives them the same ? 60_000 : undefined pool ceiling acp-bridge and web-shell already use, and extends the parity sweep in unit-vitest-configs.test.ts to all 22 so a new workspace cannot join the lane on the 5s default.

Once that lands, the seven budgets deferred here have room to take a poolMultiplier — which they did not under a 5s ceiling, since dingtalk's guard measures its own quadratic regression at ~4.1 s against a 1000 ms bound.

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head dcbf4a1, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

No response changes against the PR base across 12 scenario(s).

Qwen Code · serve A/B

@yiliang114

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

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: 37 passed · 0 failed · 37 total

Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:37 通过 · 0 失败 · 37 总计

抖动门:✅ 3 changed test file(s) x 5 identical rounds, no divergence

Verification report

PR #10912 verification — test(cli): keep the redaction ReDoS guard asserting on the shared pool

Verdict: merge-ready — 37/37 scripted assertions passed, 0 unexpected failures.
Verified head: c4e30ef7ece9165c07059d592f478db96990d571 (base tip 80b5dab3).
One non-blocking Suggestion (a description correction, measured, below); no blocking findings.

中文摘要
  • 判定merge-ready。37/37 脚本化断言通过,0 个意外失败。
  • A/B 结论(见 "Central claim" 表与 01-ab-flip-base-green-head-red.png02-mutation-matrix.png):向真实生产模块注入一个"慢但正确"的二次回溯回归(24s 档),池通道上 base 用例绿(对时长不作任何断言,截断断言照常通过——正是本 PR 要堵的洞),head 用例于新加的 20s 上界(expected 23646 to be less than 20000,失败点正是 poolMultiplier 分支,且 23.7s < 60s 通道上限,由断言而非 vitest 超时判定)。本地通道两臂皆红于 1s 上界,证明本 PR 不影响本地/GitHub 通道。
  • 普查03-census-19-of-34-sites.png):34 处隔离站点、19 处带 poolMultiplier(13×20、4×10、2×5)、新增站点位于 channel-worker-supervisor.test.ts:3138,与 PR 描述完全一致;刻意推迟的 7 处裸挂钟预算(sdk-typescript×4、qwen-live、channels/base、channels/dingtalk×2)均确认存在,channels 两包确无 testTimeout
  • Findings(不阻塞):新注释与描述中"far too tight for a quadratic regression"在本站点被实测削弱——典型二次回归(去掉 {0,31} 上限,恰是该注释声称要防的回归)在 66KB 输入上仅 1.4–1.5s,远低于 20s 池上界;池守卫实际只抓 ≥20s 的回归,典型形态由本地 1s 上界兜住(C5 红)。改动本身仍是严格改进(base 在池上什么都不断言)。另有一处可议站点 fzfWorkerHandle.test.ts:87,见 Findings。
  • 未覆盖:完整包级套件、机群争抢系数实测、>60s 档回归的判定机制、Windows/macOS。

Central claim + A/B

Central claim: on the shared ECS pool lane (QWEN_SKIP_LATENCY_BUDGETS=1, RUNNER_NAME=ecs-qwen-* → vitest testTimeout 60 s), the redaction test case previously ran and asserted nothing about duration; the PR's { poolMultiplier: 20 } makes it assert elapsed < 20 000 there, catching slow-but-correct redaction regressions that the truncation assertions still pass.

Secondary claims: (a) the census is 19-of-34 with distribution 13×20/4×10/2×5 and the seven deferred raw budgets are as described; (b) the change cannot loosen any lane and leaves local/GitHub lanes untouched.

How the A/B drives the real path: the cli vitest config aliases @qwen-code/acp-bridge/logRedaction to live TS source, so mutants were injected into packages/acp-bridge/src/logRedaction.ts (snapshot → mutate → run → restore, sha256-verified) with no rebuild; the test arm was switched with git show HEAD^1|HEAD of the test file. The pool lane was replicated with RUNNER_NAME=ecs-qwen-verify QWEN_SKIP_LATENCY_BUDGETS=1 (the exact two env-dependent config knobs, ci.yml:723 + vitest.config.ts). Mutants: M1 = canonical quadratic regression, uncap the URL-scheme {0,31} charset (the exact cap whose comment exists to prevent quadratic backtracking) — 1.5 s on 66 KB of a., output byte-identical on the adversarial input; M2 = M1 registered 16× (a constant-scaled variant, placed inside the (20 s, 60 s) band at 24.2 s so the assertion decides, per the PR's own claim).

cell test arm redaction lane expected actual
C1 head wild type pool green green
C7 base wild type pool green green
C6 head wild type local green green
C9 base wild type local green green
C4 head M1 (1.4 s) pool green — gap measurement green (1.4 s < 20 s)
C5 head M1 local red red — expected 1445 to be less than 1000
C3 base M2 (24 s) pool green — the hole green
C2 head M2 (24 s) pool red on the new bound red — expected 23646 to be less than 20000 at latency-budget.ts:52 (the multiplier branch), test line 3138, 23.7 s < 60 s lane timeout
C8 base M2 local red red — expected 24060 to be less than 1000

The load-bearing flip is C3→C2: identical mutant, identical environment; the only difference is the hunk this PR adds. C5/C8 are the positive control (both arms still fail on the local lane — the PR does not touch it), and C4 is the tradeoff measurement (below). Witness captures: 01-ab-flip-base-green-head-red.png, 02-mutation-matrix.png, 04-mutant-timing-probes.png, 05-scripted-assertions-37-pass.png.

Corrections

The added comment's "far too tight for a quadratic regression" overstates the pool guard's discrimination at this site (correction to the description, not a request to change code). The new comment in both latency-budget.test.ts copies and the Reviewer Test Plan both say the relaxed bound sits "far too tight for a quadratic regression". Measured through the real module on the real 66 KB input (04-mutant-timing-probes.png): wild type redacts in 4.2 ms (×238 margin vs the 1 s local budget), while the canonical quadratic regression — removing the {0,31} scheme-cap, the precise cap whose comment exists to prevent quadratic backtracking — takes 1.5 s: ~360× the wild type, yet ~13× under the relaxed 20 s pool bound (≈7 s even at the fleet's cited ~5× contention). So on a pool-only run the pool guard catches regressions ≥20 s, and the canonical quadratic is caught by the local/GitHub lane's 1 s budget (C5 red at 1445 ms), not by the pool bound. The change remains a strict improvement — base asserted nothing on the pool — and the case runs on every lane, so the regression class is still pinned somewhere; but the comment's generalization is empirically too strong for this site's input shape, and a reader who trusts it will overestimate what the pool lane alone can catch.

Findings (non-blocking)

  1. Suggestion — fzfWorkerHandle.test.ts:87 is the one debatable site among the 15 non-multiplier guards. The sibling sweep of all 15 pool-skipped sites found: two FIFO hang guards (customBanner.test.ts:257, DualOutputBridge.test.ts:390) where a bound can never fire (a blocking open() stalls the event loop forever; only the lane timeout catches it — multiplier buys nothing); git-ignore.test.ts:406 whose property is still pinned on the pool by a raw >= 4000 lower bound; and the rest carrying sibling correctness assertions. Only fzfWorkerHandle's 50 ms budget discriminates a property (worker spawn ≥10 ms vs in-thread µs) that a ×20 multiplier (1 s) could still discriminate. It is defensible either way — spawn latency under 5× contention is unpredictable, so a pool bound there risks false positives — and it predates this PR; noted for the ci: give every workspace the shared-pool test timeout, not just five #10915 follow-up rather than as a defect here.

Not covered

  • Full packages/cli / packages/core suites were not run; the PR's own CI covers them, and this round ran the changed case (9 cells) plus both helper suites (12/12).
  • The fleet contention factor (~5×) is taken from the helper's own comment, not measured here (needs pool-host history; this container is not an ecs-qwen-* runner — RUNNER_NAME is empty). The 13× discriminability figure therefore inherits that uncertainty.
  • Regressions slower than the 60 s lane timeout were not exercised (M2 was placed in-band by design); for those, vitest's worker kill decides, on both arms.
  • M1 is a no-op on the adversarial input but not on all inputs (corpus: 1/10 diff — schemes longer than 32 chars get redacted where WT leaves them). It models "slow but correct" for this test's input, which is what the guard sees; the corpus diff is documented in the probe log.
  • Windows/macOS lanes not exercised (linux container).
  • The seven deferred raw budgets were verified to exist and to match the PR's rationale (channels packages have no testTimeout, dingtalk's comment documents the ~4.1 s quadratic), but not executed.

Methodology

Environment: the CI verify container at the merge commit (HEAD = merge, HEAD^1 = base tip, HEAD^2 = PR head; 1 reachable commit, matching the metadata snapshot). Harnesses live in tmp/pr10912-verify-20260903-122705/src/ (census.mjs, mutant-probe.ts, apply-mutant.mjs, run-cell.sh, verify-assertions.mjs, print-*.mjs); raw per-cell logs in .../logs/. Cells ran the real packages/cli vitest against the real production module (the cli vitest config aliases logRedaction to source; mutants were applied in place and restored, sha256-verified byte-identical, working tree left clean). All 37 assertions are re-checkable by rerunning node src/verify-assertions.mjs, which parses only saved logs.

Flakiness gate log

rounds=5 files=3 skipped=0
file packages/cli/src/serve/channel-worker-supervisor.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/channel-worker-supervisor.test.ts
file packages/cli/src/test-utils/latency-budget.test.ts: (cd packages/cli) npx --no-install vitest run ./src/test-utils/latency-budget.test.ts
file packages/core/src/test-utils/latency-budget.test.ts: (cd packages/core) npx --no-install vitest run ./src/test-utils/latency-budget.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/cli/src/serve/channel-worker-supervisor.test.ts: PPPPP
  packages/cli/src/test-utils/latency-budget.test.ts: PPPPP
  packages/core/src/test-utils/latency-budget.test.ts: PPPPP

verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/serve/channel-worker-supervisor.test.ts: P (exit 0)
round 1 · packages/cli/src/test-utils/latency-budget.test.ts: P (exit 0)
round 1 · packages/core/src/test-utils/latency-budget.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/channel-worker-supervisor.test.ts: P (exit 0)
round 2 · packages/cli/src/test-utils/latency-budget.test.ts: P (exit 0)
round 2 · packages/core/src/test-utils/latency-budget.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/channel-worker-supervisor.test.ts: P (exit 0)
round 3 · packages/cli/src/test-utils/latency-budget.test.ts: P (exit 0)
round 3 · packages/core/src/test-utils/latency-budget.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/channel-worker-supervisor.test.ts: P (exit 0)
round 4 · packages/cli/src/test-utils/latency-budget.test.ts: P (exit 0)
round 4 · packages/core/src/test-utils/latency-budget.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/channel-worker-supervisor.test.ts: P (exit 0)
round 5 · packages/cli/src/test-utils/latency-budget.test.ts: P (exit 0)
round 5 · packages/core/src/test-utils/latency-budget.test.ts: P (exit 0)

Evidence images

01-ab-flip-base-green-head-red

02-mutation-matrix

03-census-19-of-34-sites

04-mutant-timing-probes

05-scripted-assertions-37-pass

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Welcome back, and thanks for restructuring this — the template is now fully in place, so the shape gate that stopped the last run is cleared and this can be judged on content.

Template: ✓ — all required headings present (What this PR does, Why it's needed, Reviewer Test Plan + subsections, Risk & Scope, Linked Issues), with a full bilingual <details> translation.

Problem: Observed, not theoretical. The credential-redaction ReDoS guard in channel-worker-supervisor.test.ts pushes 66 KB of a. through the regex and bounds it at 1000 ms, but #10870 quarantined that bound with no poolMultiplier — and I confirmed against the helper that on the pool lane expectWithinLatencyBudget returns early and asserts nothing when the multiplier is absent. So on the shared pool this case ran green against exactly the backtracking regression it exists to catch. Real gap, with a named history (raised in #10870, confirmed post-merge).

Direction: Aligned. This is test-reliability work squarely inside the #10490 flakiness effort — restoring assertion coverage a quarantine sweep accidentally dropped, not adding new surface.

Size: Test-only. 0 production lines (all three files are *.test.ts); the one packages/core/src/** path is a test file, so no core-module size trigger.

Approach: Minimal and correct in scope — one call gets poolMultiplier: 20 plus a why-comment, and a stale criterion comment in the helper's own test is corrected. The author explicitly defers the seven raw wall-clock budgets outside packages/{cli,core} to the #10915 prerequisite with clear reasoning, which is the right call rather than scope creep.

Risk: No elevated signals — every changed file is a .test.ts, none on a high-risk path.

Moving on to code review. 🔍

中文说明

感谢重新整理 —— 模板现已完整,上一次卡住的格式门禁已解除,可以按内容来评审了。

模板: ✓ —— 所有必需标题齐全(What this PR doesWhy it's neededReviewer Test Plan 及子小节、Risk & ScopeLinked Issues),并附完整的双语 <details> 翻译。

问题: 已观测到,而非理论性的。channel-worker-supervisor.test.ts 中的凭据脱敏 ReDoS 守卫把 66 KB 的 a. 推过正则并以 1000 ms 为界,但 #10870 隔离该上界时没有带 poolMultiplier —— 我对照 helper 确认:池通道上缺少倍数时 expectWithinLatencyBudget 会提前返回、不作任何断言。因此在共享池上,这个用例恰恰对它要守护的那种回溯回归是绿的。这是真实的缺口,且有据可查(在 #10870 中提出、合并后确认)。

方向: 对齐。这属于 #10490 flakiness 工作正中央的测试可靠性工作 —— 恢复一次隔离排查中意外丢掉的断言覆盖,而非新增功能面。

规模: 纯测试改动。生产代码 0 行(三个文件均为 *.test.ts);唯一一处 packages/core/src/** 路径是测试文件,不触发核心模块规模门禁。

方案: 范围上最小且正确 —— 一处调用补上 poolMultiplier: 20 并附说明原因的注释,同时修正 helper 自身测试里一处过期的判据注释。作者明确将 packages/{cli,core} 之外的七处裸挂钟预算推迟到前置 #10915,并给出清晰理由,这是正确的取舍而非范围扩张。

风险: 无升级信号 —— 所有改动文件都是 .test.ts,均不在高风险路径上。

进入代码审查。🔍

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Code review. I read the helper first, then the diff, and the change does exactly what a reviewer would ask for. Independent read of the problem: the supervisor case needs the same treatment as its sibling guards — pass a poolMultiplier so the pool lane asserts something — and that is precisely what landed.

Verified against the actual implementation, not the PR's framing:

  • expectWithinLatencyBudget (in latency-budget.ts, identical in cli and core) returns early and asserts nothing on the skip lane when poolMultiplier is absent. So before this PR the 1000 ms ReDoS bound genuinely wasn't evaluated on the pool — the gap is real.
  • Adding { poolMultiplier: 20 } makes the pool lane assert elapsed < 1000 × 20 = 20s. That is consistent with the 12 existing ×20 sites, well clear of the ~5x measured contention, far below any quadratic blowup, and — checked against packages/cli/vitest.config.ts — under the pool's 60s testTimeout, so the assertion (not vitest) still decides the outcome. Off the pool the full 1000 ms bound is untouched.
  • The "19 of 34" census in the corrected comment checks out exactly: before this PR there are 18 poolMultiplier sites out of 34 calls (12×20, 4×10, 2×5); this adds one ×20 → 19 (13×20, 4×10, 2×5).
  • Both latency-budget.test.ts copies and both latency-budget.ts helpers are byte-identical across packages/cli and packages/core, and the diff keeps them in lockstep. The comment correction is accurate and says why (a slow-but-correct regex still satisfies the truncation asserts).

No correctness blockers, no convention violations — test-only, uses the existing helper, no duplication. The added why-comment on the supervisor call is the kind of comment that earns its place.

Testing. This is the CI path, so I'm quoting the PR's own checks on the reviewed commit rather than running anything. The check that matters here is Test (ubuntu-latest, Node 22.x) — it directly executes the changed supervisor test, and because the author is a collaborator it runs on the self-hosted ecs-qwen pool with QWEN_SKIP_LATENCY_BUDGETS=1 set (ci.yml), so it genuinely exercises the pool lane this PR fixes. That run is still in flight; everything statically checkable is green. The macOS/Windows test legs and the CLI-sandbox integration leg are skipped (not failures), and the SDK Java run was cancelled — neither touches this test-only change, but both are noted below for transparency. Not independently verified here: a local full-suite run (the author notes it needs a workspace npm run build first), which is what the in-flight CI run covers.

Final CI results for c4e30ef (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Real daemon E2E / Java 11 🚫 cancelled
Test (ubuntu-latest, Node 22.x) 🚫 cancelled
web-shell E2E Smoke (ubuntu-latest, Node 22.x) 🚫 cancelled
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
macos-latest / Java 21 ✅ success
OpenTUI no-flicker gate ✅ success
Secret scan (TruffleHog) ✅ success
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
TUI parity snapshots (ink vs opentui) ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
windows-latest / Java 21 ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

No user-visible behavior changes (test-only), so there is no live TUI/behavioral claim for a sandboxed lane to settle — the in-flight unit suite is the right oracle here.

中文说明

代码审查。 我先读了 helper 实现,再看 diff,这个改动正是评审者会要求的样子。我对问题的独立判断是:supervisor 用例需要与同类守卫相同的处理 —— 传入 poolMultiplier,让池通道也能断言到东西 —— 而落地的正是如此。

以下是对照真实实现核实过的,而不是照搬 PR 的说法:

  • expectWithinLatencyBudgetlatency-budget.ts,cli 与 core 两份完全一致)在 skip 通道上若缺少 poolMultiplier 会提前返回、不作任何断言。因此本 PR 之前,1000 ms 的 ReDoS 上界在池上确实根本不被求值 —— 缺口是真实的。
  • 加上 { poolMultiplier: 20 } 后,池通道断言 耗时 < 1000 × 20 = 20 秒。这与现有 12 处 ×20 站点一致,远高于实测约 5 倍的争抢、远低于任何二次爆炸量级,并且 —— 已对照 packages/cli/vitest.config.ts 核实 —— 低于该池 60 秒的 testTimeout,因此仍由断言(而非 vitest)决定结果。池外通道 1000 ms 的完整上界不受影响。
  • 修正后注释里 "19 of 34" 的普查逐条吻合:本 PR 之前 34 处调用中有 18 处带 poolMultiplier(12×20、4×10、2×5);本次新增一处 ×20 → 19(13×20、4×10、2×5)。
  • packages/clipackages/core 的两份 latency-budget.test.ts 与两份 latency-budget.ts 逐字节一致,diff 让它们保持同步。注释修正是准确的,并说明了原因(一个慢但正确的正则仍能满足截断断言)。

无正确性阻塞项、无规范违规 —— 纯测试、复用现有 helper、无重复。supervisor 调用上新增的 why 注释是那种"值得存在"的注释。

测试。 这是 CI 通道,因此我引用该提交自身的 CI 检查结果,而非自行运行。这里最关键的检查是 Test (ubuntu-latest, Node 22.x) —— 它会真实执行被改动的 supervisor 测试;且因作者是协作者,它跑在自托管 ecs-qwen 池上并设置了 QWEN_SKIP_LATENCY_BUDGETS=1(ci.yml),因此确实施展到本 PR 所修复的池通道。该运行仍在进行中;所有可静态检查的项均已通过。macOS/Windows 测试分支与 CLI 沙箱集成分支为跳过(并非失败),SDK Java 运行被取消 —— 两者都与这一纯测试改动无关,但为透明起见一并列于下表。此处未独立验证:本地完整套件运行(作者说明需先 npm run build 生成工作区产物),而这正是在途 CI 运行所覆盖的内容。

(CI 表格见上方标记区域;用户可见行为无变化,属纯测试改动,故无需沙箱通道去验证任何 TUI / 行为断言 —— 在途的单元测试套件就是此处正确的裁判。)

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — clean across every stage; this is a small, verified-correct test fix that restores a guard the quarantine sweep silently disabled.

This one earns its merge. The prior run stopped purely on the template, and with that addressed the substance is what I'd want to see: a real, named gap (the redaction ReDoS bound was asserting nothing on the shared pool), a fix that matches the pattern the siblings already use, and scope discipline — the seven sibling budgets outside cli/core are deliberately deferred to #10915 with sound reasoning instead of being bolted on.

Going back to my independent read, I'd have proposed exactly this: pass a poolMultiplier so the pool lane asserts something, matching the sibling ×20 guards. The PR does that and nothing more. I confirmed the load-bearing facts against the code rather than the description — the helper's early-return when the multiplier is absent, that 20× stays under the 60s pool testTimeout, the exact 19-of-34 census, and that both cli/core copies stay byte-identical. Every change in the diff is necessary; there is no drive-by churn. In six months this reads as a careful, well-commented reliability fix, not something to curse.

The one thing I cannot yet attest is a green full-suite run — the author didn't run it locally (it needs a workspace build first), and the Test (ubuntu-latest, Node 22.x) suite that exercises this very test on the pool lane is still in flight. So approval is deferred until CI lands green on the reviewed commit; the finalize pass will post it automatically, and will withhold it if anything lands red or the head moves.

中文说明

置信度:5/5 —— 每个阶段都干净;这是一处小而正确、经过验证的测试修复,恢复了一个被隔离排查悄悄禁用的守卫。

这个 PR 值得合入。上一次运行纯粹卡在模板上;模板补齐后,其实质内容正是我希望看到的:一个真实且被点名指出的缺口(脱敏 ReDoS 上界在共享池上不作任何断言)、一个与同类守卫已有做法一致的修复,以及范围上的克制 —— cli/core 之外的七处同类预算被明确推迟到 #10915,并给出了合理理由,而非顺手塞进来。

回到我的独立判断,我会提出的也正是这个方案:传入 poolMultiplier,让池通道也能断言到东西,并与同类 ×20 守卫保持一致。这个 PR 做的正是这件事,且仅此而已。我是对照代码、而非照搬描述来确认那些关键事实的 —— helper 在缺少倍数时的提前返回、20 倍仍低于 60 秒池 testTimeout、精确的 19/34 普查,以及 cli/core 两份保持逐字节一致。diff 中每一处改动都是必要的,没有顺手夹带的杂项。六个月后回看,这读起来是一处细致、注释得当的可靠性修复,而不是让人恼火的东西。

我唯一尚不能背书的,是一次绿色的完整套件运行 —— 作者未在本地运行(需先做工作区构建),而那个会在池通道上真实执行此测试的 Test (ubuntu-latest, Node 22.x) 套件仍在进行中。因此批准将推迟到 CI 在被审提交上转绿;finalize 流程会自动发布该批准,若有检查转红或提交头移动则会保留不发。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

The bot already has a review of its own on c4e30ef7ece9165c07059d592f478db96990d571, which still stands.

机器人在 c4e30ef7ece9165c07059d592f478db96990d571 上已有自己的评审,且仍然有效。

The stage comments above were updated with the latest result. View workflow run.

上方各阶段评论已更新为最新结果。查看工作流运行

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

Reviewed. Suggestions are inline.

Test Plan (not a blocker): Tests 12 passed — this review observed 28107, 23147, 1919, 298, 1769, 504, 5732, 94 passed.

— qwen3.8-max via Qwen Code /review (v0.22.3)

Comment thread packages/cli/src/test-utils/latency-budget.test.ts Outdated

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

Reviewed. Suggestions are inline.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R2-1 criterion-comment sub-20x rationale — already reported as R1-1 (comment 3924537783)

Test Plan (not a blocker): Tests 12 passed — this review observed 23144, 1919, 298, 1772, 504 passed.

中文说明

已审查。 建议见行内评论。

本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

Test Plan(非阻断):Tests 12 passed — this review observed 23144, 1919, 298, 1772, 504 passed

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/test-utils/latency-budget.test.ts Outdated
The criterion comment's closing clause attributed every sub-20x
poolMultiplier to a shorter per-test timeout, but only 2 of the 6
sub-20x sites fit that reading: cleanup.test.ts keeps 5x because the
bound must sit under the ~5s no-cap drain (a larger multiple would
clear the regression it exists to catch), and review-footer /
compose-review keep 10x as a margin landing near 20s with no per-test
timeout at all. Following the clause as a selection rule would pick
20x whenever budget x 20 fits the lane, reproducing the dead-guard
hole this family closes. Reword the clause: 20x where the lane allows
it and the bound still sits under the regression's own cost; a lower
multiple where a shorter per-test timeout or the regression's own
cost decides the outcome first. Both byte-identical copies updated in
lockstep.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtlz4ubdgq

@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 blocking issues. LGTM! ✅

Not explored to full depth (tool budget reached): "agent 4": none — no check was cut short..

Test Plan (not a blocker): Tests 12 passed — this review observed 28112, 23151, 1919, 298, 1772, 504, 5822, 94 passed.

Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:

  • packages/core/src/test-utils/latency-budget.test.ts:45 — [probe] D3-1 hardcoded "19 of the 34" site census in the criterion comment drifts with routine site churn
中文说明

无阻断问题。LGTM!✅

未探索到全部深度(达到工具调用预算):"agent 4"none — no check was cut short.

Test Plan(非阻断):Tests 12 passed — this review observed 28112, 23151, 1919, 298, 1772, 504, 5822, 94 passed

收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

@wenshao
wenshao added this pull request to the merge queue Sep 3, 2026
Merged via the queue into main with commit b559525 Sep 3, 2026
130 of 131 checks passed
@qqqys

qqqys commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

E2E verification report (qqqys patrol, head dcbf4a1360)

Gate: ci-bot APPROVED 21:42Z + wenshao APPROVED 22:11Z, both at head. CI at head: 28 success / 112 skipped / 0 failure — Test, Lint & Static, web-shell E2E Smoke, Desktop ×2, Integration Tests (no-AK), Serve A/B, Dependency CVE audit all green; the single cancelled lane is a superseded route run (3 other route runs at this head succeeded, including the latest 22:11:52Z).

What the diff does (verified against head, not just the description)

  • channel-worker-supervisor.test.ts:3138 — the 66 KB credential-redaction ReDoS guard now passes { poolMultiplier: 20 }, so on the shared ECS pool (QWEN_SKIP_LATENCY_BUDGETS truthy) it asserts < 20 000 ms instead of asserting nothing. Off-pool behavior is unchanged (strict 1 000 ms).
  • The two latency-budget.test.ts copies (cli + core) get a comment-only correction of the skip-vs-bound criterion; the helper implementations are untouched.

Load-bearing A/B (head tarball + symlinked deps, no build; helper read from source)

Ran a probe alongside the real suites simulating the pool (QWEN_SKIP_LATENCY_BUDGETS=1):

  • base call shape expectWithinLatencyBudget(999_999, 1_000)passes (vacuous — the guard asserted nothing on the pool before this PR);
  • head call shape with { poolMultiplier: 20 }: 15 000 ms → passes (tolerates ~5x pool contention), 25 000 ms → fails (assertion is active; a quadratic backtracking regression on 66 KB overruns any such bound);
  • off-pool, 1 500 ms vs the strict 1 000 ms budget → fails (strict bound intact).

Real suites at head:

  • packages/cli pool-simulated: latency-budget.test.ts 6/6 + probe 4/4;
  • packages/core pool-simulated: latency-budget.test.ts 6/6;
  • the actual redaction case (handles long non-url worker log lines while applying credential redaction): passes pool-simulated (20x bound) and with the strict 1 000 ms bound, no pool env.

Conclusion: no Critical issues found; the change does what it claims and nothing else. Mergeable from this reviewer's perspective.

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.

5 participants