fix(web-shell): parse 256-color and truecolor SGR sequences in parseAnsi - #7620
Conversation
parseAnsi read every ';'-separated SGR parameter as a standalone code, but the arguments of 38/48/58 (extended foreground/background/underline color) are not codes. `38;5;<n>` and `38;2;<r>;<g>;<b>` were fed back into the code loop, so: - `38;5;2` set dim (from color index 2) and produced no color; - any truecolor value with a zero channel hit the `code === 0` branch and wiped the color, bold and dim already set on the line; - a background such as `48;5;22` fed 22 to the reset-intensity branch, silently un-bolding the text. Consume the 38/48/58 arguments instead of reading them as codes, and resolve the foreground to hex: 0-15 map onto the existing themed palette, 16-231 onto the 6x6x6 cube, 232-255 onto the grayscale ramp. Background and underline color are parsed but not rendered (Segment has no such field) so their arguments still cannot leak into the code stream. parseAnsi feeds shell tool-output rendering in ToolGroup, so this affects any 256-color CLI. Adds ansi.test.ts (none existed); the four new cases fail on the previous parser.
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after✅ No screenshot changes against the PR base. Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Thanks for the PR! Template looks good ✓ Problem: this is a real, observed bug — not theoretical. The existing Direction: aligned. The web shell renders shell tool output via Size: not applicable — Approach: the scope is tight — two files in one module, no unrelated changes. Consuming the extended-color arguments in the code loop is the standard fix for this class of bug. Foreground resolves to hex; background and underline are parsed but not rendered (Segment has no field for them), which is the right call — surfacing them is a separate feature. Nothing to cut. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个真实的、已观测到的 bug，而非理论性加固。现有的 方向：对齐。Web shell 通过 规模：不适用—— 方案：范围紧凑——同一模块的两个文件，无无关改动。在 code 循环中消费扩展颜色参数是此类 bug 的标准修复方式。前景色解析为 hex；背景和下划线颜色被解析但不渲染（Segment 没有相应字段），这是正确的做法——呈现它们是另一项功能。没有需要砍掉的部分。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal: I would change the Comparison: the PR matches this proposal exactly. No simpler path was missed. The implementation is clean:
No correctness bugs, no security concerns, no convention violations. Comments explain why (the corruption mechanism), not what. The test file covers the five new behaviors plus four pre-existing ones, and the assertions are specific enough to catch regressions (whole-segment equality, not just individual fields). Nothing to flag. TestingCI test evidence (fetched via API for
All checks completed — no failures. macOS/Windows tests and CLI integration tests were skipped (expected for a web-shell-only change). The ubuntu unit suite and web-shell E2E smoke both passed. Real-scenario (tmux): N/A — the change is in the web-shell browser component ( 中文说明代码审查独立方案： 我会将 对比： PR 与此方案完全一致，没有遗漏更简路径。 实现干净： 无正确性 bug、无安全隐患、无规范违反。 测试CI 证据（通过 API 获取）：ubuntu 单元测试 ✅、web-shell E2E Smoke ✅、web-shell 视觉捕获 ✅、precheck ✅。macOS/Windows 测试和 CLI 集成测试按预期跳过。无失败。 实际场景（tmux）：不适用——改动在 web-shell 浏览器组件中，非终端 TUI。单元测试和 CI web-shell E2E smoke 是合适的验证面。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean, focused bug fix with a real reproduction, standard approach, and thorough tests. This is exactly the kind of PR that's easy to say yes to. The bug is concrete and well-demonstrated — three distinct failures from one root cause (extended-color arguments read as SGR codes), each with a specific sequence and expected-vs-actual output. The fix is the standard approach: consume the arguments instead of re-reading them. The implementation is minimal (two files, one module), the xterm mapping is correct (hand-verified against the spec), and the test file pins both the new behavior and the pre-existing behavior so nothing regresses silently. The scope discipline is good: background and underline color are parsed but not rendered, with a clear note that surfacing them is a separate feature. No drive-by changes, no over-abstraction. If I had to maintain this in six months, the comments and test names would tell me exactly what each branch does and why. CI is green across the board on 中文说明置信度：5/5 — 干净、聚焦的 bug 修复，有真实复现、标准方案和充分测试。 这是一个容易说"是"的 PR。Bug 具体且展示充分——同一根因（扩展颜色参数被当作 SGR code 读取）导致三种不同失效，每种都有具体序列和预期/实际输出对比。修复是标准方案：消费参数而非重新读取。实现最小化（两个文件、一个模块），xterm 映射正确（已手工核对规范），测试文件同时固定了新行为和既有行为，防止静默回归。 范围控制良好：背景和下划线颜色被解析但不渲染，并明确说明呈现它们是另一项功能。无顺手改动、无过度抽象。 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| } | ||
| // Only the foreground maps onto a Segment. Background and underline | ||
| // color are still parsed so their arguments cannot leak into the loop. | ||
| if (code === 38) color = value; |
There was a problem hiding this comment.
[Critical] A malformed extended-color sequence clears a previously valid foreground color — Failure scenario: \x1b[31m\x1b[38;5;300mtext sets red via code 31, then xterm256(300) returns undefined (index > 255), and color = value unconditionally overwrites — the text renders in the default color instead of red. The same occurs for truncated truecolor (\x1b[31m\x1b[38;2;1;2mtext). The existing malformed-sequence test only starts from color: undefined (bold but no prior color), so this regression is invisible to the test suite.
| if (code === 38) color = value; | |
| if (code === 38 && value !== undefined) color = value; |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Confirmed and fixed in 32684dd — this one was a regression I introduced, not just a gap.
Before this PR, \x1b[31m\x1b[38;5;300m left the red intact: 38, 5 and 300 all fell through the code branches untouched. My version consumed the arguments correctly but then assigned the result unconditionally, so a sequence that failed to resolve actively cleared a color that had been set correctly — strictly worse than the behavior I was replacing, and the opposite of what the test named "without corrupting state" was supposed to be guarding.
Taken as suggested:
if (code === 38 && value !== undefined) color = value;An unreadable sequence is now ignored rather than treated as a reset, which is also what a real terminal does with an out-of-range parameter.
Your read on the test was exactly right — it started from color: undefined, so a cleared color was invisible to it. Added the case it could not catch, covering both malformed forms you named plus the rest of the malformed set:
it('leaves an already-set color alone when the sequence is malformed', () => {
for (const seq of ['38;5;300', '38;5', '38;2;1;2', '38;7;1', '38']) {
expect(parseAnsi(`${ESC}31m${ESC}${seq}mtext`)).toEqual([
{ text: 'text', color: '#fc8181', bold: false, dim: false },
]);
}
// A well-formed sequence still replaces it.
expect(parseAnsi(`${ESC}31m${ESC}38;5;21mtext`)[0]!.color).toBe('#0000ff');
});The trailing assertion pins the other side, so "ignore malformed" can't silently degrade into "ignore everything". Fail-before/pass-after: reverting only ansi.ts fails exactly this new case, 1 failed | 8 passed; with the fix, 9/9.
| const mode = codes[i + 1]; | ||
| let value: string | undefined; | ||
| if (mode === 5) { | ||
| value = xterm256(codes[i + 2]!); |
There was a problem hiding this comment.
[Suggestion] Non-null assertions (!) on array accesses that can be undefined at runtime lie to the type system — toHex and xterm256 declare number parameters but receive undefined for truncated sequences like \x1b[38;5m. Today the Number.isInteger() guards catch it, but a future refactor that trusts the declared type (e.g. Math.round(index) before validation) would silently produce wrong output. The ! hides the type mismatch from the compiler.
| value = xterm256(codes[i + 2]!); | |
| value = xterm256(codes[i + 2]); |
Widen the parameter types to match:
function toHex(r: number | undefined, g: number | undefined, b: number | undefined): string | undefined {
function xterm256(index: number | undefined): string | undefined {— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Agreed — taken in 32684dd. toHex and xterm256 now declare number | undefined, which is what a truncated sequence like \x1b[38;5m actually hands them, and the ! assertions at the call sites are gone.
I went one step further than the suggestion inside toHex. Widening the parameters alone still left ! in the body, since Number.isInteger() is not a type predicate and so doesn't narrow number | undefined — the assertion would have just moved rather than gone away. Restructuring the validation narrows properly instead:
function toHex(
r: number | undefined,
g: number | undefined,
b: number | undefined,
): string | undefined {
let hex = '#';
for (const v of [r, g, b]) {
if (v === undefined || !Number.isInteger(v) || v < 0 || v > 255) {
return undefined;
}
hex += v.toString(16).padStart(2, '0');
}
return hex;
}xterm256 takes the same shape (index === undefined || !Number.isInteger(index) first), which also lets the CUBE_LEVELS[...] lookups drop their assertions, since number | undefined is now the honest parameter type. There are no non-null assertions left in the module, so the refactor hazard you describe — a future change trusting the declared type — is now a compile error rather than silently wrong output.
tsc --noEmit reports zero errors in ansi.ts and ansi.test.ts; eslint clean.
…e is malformed An unreadable 38/48/58 sequence was assigning its undefined result straight to the color, so `\x1b[31m\x1b[38;5;300m` dropped the red that code 31 had already set. Ignore the sequence instead and leave the color as-is; a well-formed one still replaces it. Also widen toHex/xterm256 to accept `number | undefined` so the truncated arguments they are actually handed match their declared types, and drop the non-null assertions that were hiding that mismatch from the compiler. Add the regression case the existing malformed-sequence test could not catch: it started from no color, so a cleared color was invisible to it.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Local verification report — merge referenceVerified as maintainer on a real local build, not by reading the diff. Two isolated worktrees: PR head Verdict: the fix is correct, matches a real terminal emulator byte for byte, and regresses nothing. Recommend merge. One documentation nit and three non-blocking follow-ups below. 1. The PR's own tests, and its A/B claim
2. Mutation testing — do those tests actually hold the fix down?13 mutants of
The three survivors are test-coverage gaps, not defects — the shipped code behaves correctly in all three cases:
Tightening the 3. Independent oracles — is the mapping actually right?The reference table is not retyped by hand: it is the
4. Real render — the actual
|
| test files | tests | |
|---|---|---|
base 32c491fc6 |
8 failed | 117 passed (125) | 9 failed | 1727 passed (1736) |
PR 32684dd6b |
8 failed | 118 passed (126) | 9 failed | 1736 passed (1745) |
The delta is exactly +9 passing tests — the new file — with an identical failure profile. All 9 failing tests live in build-artifact.test.ts, which needs a built dist; unrelated and unchanged.
eslint and prettier --check are clean on both changed files. tsc --noEmit reports 60 errors in this local checkout, but the set is byte-identical on base and PR and none are in ansi.ts / ansi.test.ts — they come from workspace type deps that aren't built locally. CI builds first, and Test (ubuntu-latest, Node 22.x) is green.
6. Follow-ups — none block this PR
- Colon-separated extended colour is not handled at all —
\e[38:5:208mand\e[38:2::255:140:0m(the ISO 8613-6 form emitted by tmux, delta and somelsbuilds). The CSI regex is[0-9;]*, so those sequences never match and the raw escape is rendered as literal text. Identical on base and PR — pre-existing, out of scope, worth a separate issue. - Non-SGR escapes leak the same way —
\e[2K, bare\rprogress rewrites, cursor movement. Also pre-existing. - Out-of-range values diverge from the emulator by choice — xterm masks (
38;2;999;0;0→#e70000,38;5;256→ palette 0); this PR drops to no colour. Defensible for malformed input, and strictly better than base; just noting the difference.
中文版本
本地验证报告 —— 合并参考
以维护者身份在本地真实构建验证，而非仅阅读 diff。使用两个隔离 worktree：PR head 32684dd6b 与 base 32c491fc6（与 main 的 merge-base）。macOS 15.6 · Node v22.23.1 · vitest 3.2.4。
结论：修复正确，与真实终端模拟器逐字节一致，且无回归。建议合并。 下面有 1 处文档问题与 3 项不阻塞的后续项。
1. PR 自带测试与其 A/B 声明
utils/ansi.test.ts 在 PR head 上 9/9 通过。仅将 ansi.ts 还原到 base、保留新测试文件时，恰好是描述中列出的那 5 个用例失败——另外 4 个（基础色 / bold / dim / reset / 分段）仍然通过，说明新测试确实锚定在这次修复上，而非附带行为。
⚠️ PR 描述里的验证命令跑不起来。
npx vitest run --root packages/web-shell --config vitest.config.ts client/utils/ansi.test.ts会报
Cannot find module .../packages/web-shell/test/setup.ts。配置里已经设了root: 'client'，命令行--root覆盖了它，于是setupFiles: ['./test/setup.ts']解析到了错误目录。可用形式：cd packages/web-shell && npx vitest run --config vitest.config.ts utils/ansi.test.ts仅影响描述，代码本身没问题。
2. 变异测试 —— 这些测试是否真的锁住了修复？
对 ansi.ts 构造 13 个变异体，逐个针对 PR 原样的测试文件运行：10 个被抓出，3 个存活。色立方档位、灰阶偏移、通道顺序、两处参数步进计数、未知形式的 break，以及「畸形值不得重置颜色」这条规则，都被真正锁住了。
3 个存活项是测试覆盖缺口，而非缺陷——这三种情况下发布的代码行为都是正确的：
| 变异体 | 存活原因 | 实际行为 |
|---|---|---|
M1 从三元组中去掉 58 |
唯一的 58 断言只检查 bold；去掉该分支破坏的是 dim |
正确——\e[1m\e[58;5;2m → dim:false（与模拟器一致）；base 给出 dim:true |
M10 把索引上界抬到 255 以上 |
toHex 的通道守卫已经拒绝了 38;5;300 产出的值，只有索引 256 能区分 |
正确 |
M11 去掉 toHex 的 0–255 守卫 |
没有测试使用越界的真彩通道值 | 正确——38;2;999;0;0 不返回颜色 |
把 58;5;2 那条断言从 [0]!.bold).toBe(true) 收紧为完整的 toEqual({ text: 'text', color: undefined, bold: true, dim: false }) 即可覆盖 M1。可选。
3. 独立 oracle —— 映射到底对不对？
参考表并非手工誊抄：它是从已发布的 @xterm/xterm 产物包中原样提取的 DEFAULT_ANSI_COLORS IIFE，再以桩函数求值得到。差分运行则把同样的字节同时喂给 @xterm/headless（真正的 xterm.js VT 解析器）与 parseAnsi，逐字符比较 (前景色, bold, dim)。
- 调色板：色立方 + 灰阶（16–255）共 240/240 与 xterm 自带表完全一致。全部 16 个标准索引上
38;5;<i>与基础 SGR code 相等，说明刻意复用主题调色板的做法成立。真彩 7/7 精确。 - 差分：跨 6 组语料共 1112 个字符单元——完整的
38;5;<idx>扫描（纯色、bold 前缀、以及作为背景）、真彩网格、交错序列，以及本机git diff --color=always与tput setaf 208真实输出的字节。PR 与模拟器的分歧为 0 个单元；base 为 844 个。 - base 在
38;5;<idx>全部 256 个索引上的量化结果：240 个完全没有颜色、16 个渲染出错误颜色（索引与某个 SGR 颜色码撞车），另有 1 个错误地设置了dim。
4. 真实渲染 —— 真正的 <ToolGroup>，不是 mock
在两个检出上分别用 jsdom 挂载真实的 <ToolGroup>，输入是真实的 git diff --color=always 字节流外加一行 256 色提示符，然后在仓库自带的 ToolChrome.module.css 与 shadcn token 下对产出的 DOM 截图。只重写了 CSS module 的哈希后缀以便源样式生效；图中每一处内联样式都出自组件本身。
注意在 main 上丢失 bold 的那两行——BOLD TRUECOLOUR（零通道命中 code === 0 重置）与 bold on a 256-colour background（48;5;22 把 22 送进了重置粗细分支）。这正是本 bug 中会破坏「原本正确」输出的部分，也是最有力的合并理由。
5. 回归检查
在两棵树上以相同命令跑完整的 packages/web-shell 套件：
| 测试文件 | 用例 | |
|---|---|---|
base 32c491fc6 |
8 失败 | 117 通过（125） | 9 失败 | 1727 通过（1736） |
PR 32684dd6b |
8 失败 | 118 通过（126） | 9 失败 | 1736 通过（1745） |
差异恰好是 +9 个通过用例——即新增文件——失败画像完全一致。9 个失败用例全在 build-artifact.test.ts，它需要已构建的 dist；与本 PR 无关且未被改动。
eslint 与 prettier --check 在两个改动文件上均干净。tsc --noEmit 在本地检出报 60 个错误，但base 与 PR 上的错误集合逐字节一致，且没有一个出自 ansi.ts / ansi.test.ts——它们来自本地未构建的工作区类型依赖。CI 会先构建，Test (ubuntu-latest, Node 22.x) 为绿。
6. 后续项 —— 均不阻塞本 PR
- 冒号分隔的扩展色完全未被处理 ——
\e[38:5:208m与\e[38:2::255:140:0m（ISO 8613-6 形式，tmux、delta 及部分ls版本会发出）。CSI 正则是[0-9;]*，这些序列根本匹配不上，于是原始转义序列被当作纯文本渲染出来。base 与 PR 表现一致——属既有问题、范围之外，值得单开 issue。 - 非 SGR 转义同样会泄漏 ——
\e[2K、裸\r进度重写、光标移动等。同为既有问题。 - 越界值与模拟器的差异属于取舍 —— xterm 会做掩码（
38;2;999;0;0→#e70000，38;5;256→ palette 0），本 PR 则退化为无颜色。对畸形输入而言这是合理的，且严格优于 base；此处仅作记录。
Review + verification report (Linux, real tmux-captured ANSI, before/after harness)Verdict: correct, spec-conformant, and demonstrably fixes two real state-corruption bugs — not just missing colors. The tests genuinely pin the fix (5/9 fail on base). Code review reasoning
Verification evidence (commit 32684dd)Before/after harness (base parser from merge-base vs PR parser, same inputs):
Spec conformance: programmatic sweep of all indices 16–255 against the xterm reference formula (6×6×6 cube levels Real terminal data: captured a live tmux pane ( Tests: 9/9 pass on the PR; running the same tests against the base parser fails 5/9 — the suite pins the new behavior rather than restating existing behavior. No concerns. Nice, tightly-scoped fix with honest comments. |
The 58;5;2 case asserted only bold, so dropping 58 from the extended-color trio left the test green: the leaked 2 argument turns on dim, not bold. Assert the whole segment instead. No case used an out-of-range truecolor channel either, so toHex's 0-255 guard was unpinned. Add 38;2;999;0;0 to both malformed-sequence loops. Both gaps found by wenshao's mutation run in review.
|
Thank you for this — the differential run against The broken verify command is my error — corrected in the description. I reproduced it before editing: the published form dies with Took the M1 tightening too, plus M11 — pushed as f1bd14f (test-only). You were right that the assertion was the weak part, and I checked the mutants rather than assuming the change closed them:
M1 is exactly your diagnosis: On the three follow-ups — agreed, all out of scope here, and (1) is the one I'd want to fix next. Colon-separated On (3), out-of-range values: noted, and I'll keep dropping to no colour rather than masking. Masking is what a terminal does because it must render something; here rendering nothing is the honest signal that the sequence was malformed, and it can't invent a colour that misleads. Strictly better than base either way, as you say. Full suite on this branch matches your numbers — 9 failing tests, all in |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.7-max via Qwen Code /review
|
@qwen-code /review CI has since landed green on |
Re-verification at
|
| prior # | previous finding | severity | status now |
|---|---|---|---|
M1 → M1 |
58;5;2 case asserted only bold, so dropping 58 from the extended-color trio left the suite green |
coverage gap | ✅ fixed by f1bd14f0f — assertion widened to the whole segment; the mutant now dies |
M11 → M12 |
no test used an out-of-range truecolor channel, so toHex's 0–255 guard was unpinned |
coverage gap | ✅ fixed by f1bd14f0f — 38;2;999;0;0 added to both malformed loops; the mutant now dies |
M10 → M11 |
raising the palette index bound past 255 survives (only index 256+ discriminates) | coverage gap, code correct | ⚪ still open, still not a defect — see §3 |
| Doc | the verify command in the PR description does not run | documentation | ✅ fixed — the description now carries the working form; I re-ran it, 9/9 (§5) |
| F1–F3 | colon-separated SGR, non-SGR escape leakage, out-of-range divergence from xterm | pre-existing / out of scope | ⚪ unchanged, still pre-existing on main |
The production file did not change. packages/web-shell/client/utils/ansi.ts is byte-identical between the head I reviewed and this one:
sha256 048ba69d48df9530e2972139e8d8a0c9294c1b4a4e3452da7cccd268f7a324f4 @ 32684dd6b
sha256 048ba69d48df9530e2972139e8d8a0c9294c1b4a4e3452da7cccd268f7a324f4 @ f1bd14f0f
So all correctness evidence from the previous round carries over unmodified, and every difference measured below is attributable to the test change alone.
2. Does the new test actually hold the fix down?
13 single-point mutants of the unmodified ansi.ts, each run against two test files: the one at 32684dd6b and the one at f1bd14f0f. Same harness, same mutants, only the test file swapped.
8/13 → 10/13 killed. No mutant regressed from caught to survived.
The two that flipped are exactly the two the commit targets, and — importantly — the killing assertion in each case is the one the commit message names, not some unrelated test that happened to go red:
M1(drop58from the trio) is killed byit('keeps background and underline color out of the code stream')— the test whose58;5;2assertion was widened.M12(droptoHex's channel guard) is killed byit('drops malformed extended-color sequences…')andit('leaves an already-set color alone…')— the two loops that gained38;2;999;0;0.
M7 (truecolor argument advance) also got strictly better: the number of tests that catch it rose from 2 to 4. The unmutated control is 9/9 green, so no kill above is noise.
3. The 3 remaining survivors are coverage gaps, not defects
I ran a deeper mutation sweep this round, which surfaced two survivors I had not probed before (M2, M13) alongside the known M11. All three are cases the unit suite does not assert — in all three the shipped code is correct, and I verified that independently rather than by inspection:
The reference palette is not hand-written: it is xterm.js's own DEFAULT_ANSI_COLORS table lifted verbatim out of the shipped @xterm/xterm bundle. The behavioural oracle is @xterm/headless — the real xterm.js VT parser — fed identical bytes and compared per character.
- Palette: all 240 cube + grayscale indices (16–255) identical to xterm's table; all 16 themed indices render the same as their plain SGR code.
38;5;7→#e0e6f0= SGR37(settlesM13); every index 256–1000 → no color (settlesM11); cube level 215 is reachable at indices 20/26/32 and correct there (settlesM2). - Differential: 4575 character cells across seven corpora — full
38;5;<idx>sweeps (plain, bold-prefixed, and as a background), a truecolor grid including zero channels, an interleaved stateful sequence, and real bytes captured on this machine fromgit diff --color=always(which emits genuine1;38;5;208,38;5;245and38;2;255;0;136) andtput setaf 208.
PR disagrees with the emulator on 0 cells. Base disagrees on 3764.
None of these three is new to this PR, and none was worsened by the new commit. They are assertions the unit suite does not make; the 256-index sweep above is what covers that ground.
4. Regression, merge risk, and gates
- base vs PR: +9 passing, +0 failing. Failing test files and failing test names are byte-identical on both sides — the 10 pre-existing failures are 9 ×
ENOENTon an unbuiltdist/inbuild-artifact.test.tsplus 1 composer-icon URL expectation, all present onmaintoo. - Merged into current
main(ecd86421c, 145 commits ahead): trial merge is clean, 0 conflict markers,mainhas never touched either file since the merge-base. Ansi suite 9/9 on the merged tree; full suite +9 passing, +0 failing vsmainalone. So the stale base is not hiding anything. eslintexit 0 with no output, and I confirmed it was actually running by planting an unused variable — which it correctly reported.prettier --checkclean on both files.
5. The documentation nit from last round is fixed
The PR description now carries the working form, and I re-ran it verbatim at this head:
cd packages/web-shell && npx vitest run --config vitest.config.ts utils/ansi.test.ts
→ Test Files 1 passed (1) · Tests 9 passed (9)For the record, the form that was in the description last round still fails, which is why the correction mattered — the config already sets root: 'client', so a CLI --root overrides it and setupFiles: ['./test/setup.ts'] resolves to the wrong directory:
npx vitest run --root packages/web-shell --config vitest.config.ts client/utils/ansi.test.ts
→ Cannot find module .../packages/web-shell/test/setup.ts
Nothing outstanding here.
6. What the fix looks like in the real component
Reproduced from the previous round and still exactly valid — ansi.ts is byte-identical (§1) and the base is the same merge-base, so these renders are unchanged by definition. The real <ToolGroup> mounted in jsdom on both checkouts with a Shell tool whose output is the genuine git diff --color=always byte stream, screenshotted under the repo's own ToolChrome.module.css and shadcn tokens. Every inline style in the image is the component's own.
Note the two rows that lose bold on main — BOLD TRUECOLOUR (a zero channel hits the code === 0 reset) and bold on a 256-colour background (48;5;22 feeds 22 to reset-intensity). That is the part of this bug that damages output which was previously correct, and it remains the strongest argument for merging.
Recommendation: merge. The follow-up commit is a clean, honest response to review — it fixes the two assertions that were not pinning what they appeared to pin, and my re-run confirms it rather than taking the commit message's word for it.
中文版本
在 f1bd14f0f 上的复验 —— 合并参考
这是对我上一轮本地验证（head 32684dd6b）的跟进。以维护者身份在真实本地构建上重跑，使用五个隔离的 worktree —— base、PR、变异测试、当前 main，以及一次试探性合并。macOS 15.6 · Node v22.23.1 · vitest 3.2.7。
结论：可以合并，0 个阻塞项。 新增的这一个提交确实做到了它声称的事：补上了我上一轮报告的两个缺口，没有削弱任何已有覆盖，并且生产代码文件完全没有改动。
1. 上一轮的问题 → 在 f1bd14f0f 上的状态
本轮我扩大了变异矩阵，因此变异体编号有所变动，下表同时给出两轮的编号：上一轮 → 本轮。
| 上一轮编号 | 上一轮的问题 | 严重度 | 当前状态 |
|---|---|---|---|
M1 → M1 |
58;5;2 用例只断言了 bold，因此把 58 从扩展颜色三元组里删掉时测试仍然全绿 |
覆盖缺口 | ✅ 已修复 —— 断言扩大到整个 segment，该变异体现在会被杀死 |
M11 → M12 |
没有任何用例使用越界的 truecolor 通道值，toHex 的 0–255 守卫没有被钉住 |
覆盖缺口 | ✅ 已修复 —— 两个畸形序列循环都加入了 38;2;999;0;0，该变异体现在会被杀死 |
M10 → M11 |
把调色板索引上界从 255 抬高后变异体存活（只有索引 256+ 才有区分度） | 覆盖缺口，代码本身正确 | ⚪ 仍存在，但仍不是缺陷 —— 见 §3 |
| Doc | PR 描述里给出的验证命令跑不起来 | 文档问题 | ✅ 已修复 —— 描述里现在是可用的写法，我重跑确认 9/9（见 §5） |
| F1–F3 | 冒号分隔的 SGR、非 SGR 转义序列泄漏、越界值与 xterm 的行为差异 | 既有问题 / 超出范围 | ⚪ 无变化，在 main 上同样存在 |
生产代码文件没有任何改动。 packages/web-shell/client/utils/ansi.ts 在我上轮评审的 head 与本次 head 之间逐字节相同：
sha256 048ba69d48df9530e2972139e8d8a0c9294c1b4a4e3452da7cccd268f7a324f4 @ 32684dd6b
sha256 048ba69d48df9530e2972139e8d8a0c9294c1b4a4e3452da7cccd268f7a324f4 @ f1bd14f0f
因此上一轮的全部正确性证据原样继续成立，而下面测得的每一处差异都只能归因于测试文件的改动。
2. 新增的测试是否真的钉住了这个修复？
对未修改的 ansi.ts 构造 13 个单点变异体，每个都分别针对两份测试文件运行：32684dd6b 的那份和 f1bd14f0f 的那份。同一套 harness、同一批变异体，只替换测试文件。
杀死率 8/13 → 10/13，没有任何变异体从"被杀死"退化为"存活"。
翻转的两个正是这个提交所针对的两个，并且更重要的是——每个变异体的致死断言都正是提交信息所指名的那一个，而不是碰巧变红的无关测试：
M1（从三元组中删除58）由it('keeps background and underline color out of the code stream')杀死 —— 正是58;5;2断言被扩大的那个用例。M12（删除toHex的通道守卫）由it('drops malformed extended-color sequences…')和it('leaves an already-set color alone…')共同杀死 —— 正是新增了38;2;999;0;0的那两个循环。
M7（truecolor 参数步进）也严格变强了：能捕获它的测试数量从 2 个增加到 4 个。未变异的对照组是 9/9 全绿，所以上述"杀死"都不是噪声。
3. 剩下 3 个存活的变异体是覆盖缺口，不是缺陷
本轮我做了更深的变异扫描，因此额外浮现出两个之前没有探测过的存活变异体（M2、M13），加上已知的 M11。这三个都是单元测试没有断言到的情形——三种情形下已发布的代码行为都是正确的，而且我是独立验证的，不是靠读代码下结论：
参考调色板不是手工抄写的：它是从已发布的 @xterm/xterm bundle 中逐字提取出来的 xterm.js 自己的 DEFAULT_ANSI_COLORS 表。行为层面的对照物是 @xterm/headless——真正的 xterm.js VT 解析器——喂入完全相同的字节，逐字符比对。
- 调色板：全部 240 个色立方 + 灰阶索引（16–255）与 xterm 的表完全一致；全部 16 个主题化索引的渲染结果与其对应的普通 SGR 码一致。
38;5;7→#e0e6f0等于 SGR37（解决M13）；索引 256–1000 全部返回无颜色（解决M11）；色阶 215 在索引 20/26/32 处可达且正确（解决M2）。 - 差分比对：七组语料共 4575 个字符单元——完整的
38;5;<idx>扫描（普通、加粗前缀、以及作为背景色）、包含零通道的 truecolor 网格、交错的有状态序列，以及在本机真实抓取的字节：来自git diff --color=always（会产生真实的1;38;5;208、38;5;245和38;2;255;0;136）和tput setaf 208。
PR 与模拟器在 0 个单元上存在分歧；base 上有 3764 个。
这三个都不是本 PR 引入的，也没有被这个新提交恶化。它们是单元测试没有做出的断言；上面这个 256 索引全覆盖的扫描才是覆盖这块地面的手段。
4. 回归、合并风险与仓库门禁
- base vs PR：+9 通过，+0 失败。两侧失败的测试文件和失败的测试名称逐字节相同——那 10 个既有失败是
build-artifact.test.ts中 9 个因未构建dist/导致的ENOENT，外加 1 个 composer 图标 URL 的期望，在main上同样存在。 - 合并进当前
main（ecd86421c，领先 145 个提交）：试探性合并干净，0 个冲突标记，自 merge-base 以来main从未触碰过这两个文件。合并后的树上 ansi 套件 9/9 通过；相对main本身，完整套件 +9 通过，+0 失败。因此陈旧的 base 并没有掩盖任何问题。 eslint退出码 0 且无输出；我通过植入一个未使用变量确认它确实在运行——它正确地报出了该问题。prettier --check在两个文件上均干净。
5. 上一轮的文档问题已修复
PR 描述里现在给出的是可用的写法，我在本次 head 上原样重跑确认：
cd packages/web-shell && npx vitest run --config vitest.config.ts utils/ansi.test.ts
→ Test Files 1 passed (1) · Tests 9 passed (9)作为记录：上一轮描述里的那种写法现在仍然会失败，这也正是这处修正有意义的原因——配置里已经设置了 root: 'client'，命令行的 --root 会覆盖它，导致 setupFiles: ['./test/setup.ts'] 解析到错误的目录：
npx vitest run --root packages/web-shell --config vitest.config.ts client/utils/ansi.test.ts
→ Cannot find module .../packages/web-shell/test/setup.ts
此项已无遗留。
6. 这个修复在真实组件里的效果
沿用上一轮的结果，并且依然完全有效——ansi.ts 逐字节相同（§1），base 也是同一个 merge-base，所以这两张渲染图按定义不会发生变化。在两个 checkout 上分别用 jsdom 挂载真实的 <ToolGroup>，其 Shell 工具的输出是真实的 git diff --color=always 字节流，然后在仓库自己的 ToolChrome.module.css 与 shadcn tokens 下截图。图中每一处内联样式都来自组件本身。
请注意在 main 上丢失加粗的那两行——BOLD TRUECOLOUR（零通道命中了 code === 0 的重置分支）和 bold on a 256-colour background（48;5;22 把 22 喂给了重置强度）。这正是此 bug 会破坏"本来正确的输出"的部分，也依然是支持合并的最有力理由。
建议：合并。 这个跟进提交是对评审的一次干净、诚实的回应——它修好了那两条看起来在钉住行为、实际却没有钉住的断言，而我的复跑是对它的验证，而不是采信提交信息的说法。
|
@qwen-code /triage |
…te liveness Fold techniques from the round-2 verification on #7620 (an ANSI parser PR) that the skill had no equivalent for: - test-only PRs get their own method: a mutation A/B across TEST FILES (same mutants of the unmodified production file, only the test file swapped), reporting killed/total on both sides, requiring that no mutant regressed from killed to survived, checking that the killing assertion is the one the commit claims to have strengthened, and adjudicating every survivor as coverage gap or defect with independent evidence rather than by inspection - when the code emulates a known implementation, that implementation is the oracle: feed identical input to both and report disagreement counts per side, lift reference tables verbatim out of the shipped dependency, and build the corpus from bytes captured off a real producer alongside synthesized sweeps - prove a gate is live before citing it: plant a violation the linter must catch, confirm it is reported, remove it — a linter that matched no files exits 0 exactly like one that passed - attribute pre-existing failures by byte-identical failing file AND test names on both sides, with deltas, not just totals - when the base is far behind, verify the merge: trial-merge into current main, confirm it is conflict-free, and re-run the affected suite on the merged tree - round continuity gains its one legitimate shortcut: a production file proven byte-identical (sha256 quoted at both heads) carries prior evidence forward by construction
|
@qwen-code /triage |
|
Triage re-run completed without a new review. The stage comments above were updated with the latest result. View workflow run. |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
LGTM. Correct fix for SGR extended-color parsing: 38/48/58 arguments are now consumed instead of leaking into the code loop. xterm 256-color mapping verified by hand (cube, grayscale, standard range). Defensive handling for truncated/malformed/out-of-range sequences is solid. 9 tests cover both the fix and pre-existing behavior. 0 findings (manual audit).
…7737) A maintainer approved PR #7620 three minutes before re-triggering `@qwen-code /triage`. The run reviewed the PR, scored it 5/5, and then reported "✅ Approved (5/5) — existing approval from prior run still valid, head SHA unchanged" without ever calling the approve API. The approval it read was the maintainer's, on the same head commit; the bot's own latest review there was a `/review` downgrade to COMMENTED, and its earlier approval had been dismissed by a push. The PR sat at 1 of the 2 required approvals with nothing in the run log marked wrong. The skill documents an "already exists, skip re-submitting" rule for CHANGES_REQUESTED only, and that snippet filters on the bot's login. Nothing covered approvals, so the rule was generalized to them with the author filter dropped. Since the maintainer's habit is to approve and then ask triage for the second vote, this reproduces on every re-run. Spell the approval check out instead of leaving it to inference: the skip applies only to the bot's own APPROVED review pinned to the exact reviewed commit — another account's approval is a different vote, a DISMISSED review is not an approval, and an approval on an earlier commit was already voided by the push. Keep the skip itself, so three re-runs still don't stack three approvals. Back it with a workflow check, since the failure is silent by nature. "Notify silent triage re-run" already detected that no review was added; it just said so in wording that read as a normal ending. It now reports whether the bot has a review of its own on the head commit, and warns when it does not. Also paginate the CHANGES_REQUESTED probe. An unpaginated read sees only the first page, and re-runs happen on exactly the heavily-reviewed PRs where the gating review has scrolled past it. Co-authored-by: verify <verify@local>
* feat(triage): add sandboxed /verify deep-verification lane @qwen-code /verify on a PR now runs a local-verification-style evidence round in the isolated /tmux sandbox contract (container, token-free agent env, loopback model proxy, author-write gate) and publishes the report via a separate PR-code-free job: - new verify job: merge-ref checkout at depth 2 (base tip + PR head for A/B), skills pinned from base so the tree under test can never rewrite its own verifier, PR-planted tmp/*-verify-* artifacts dropped, git exec-vector sweep for the persistent workspace, agent verdict allowlisted before it reaches workflow outputs - new publish-verify job: upserts one marker comment (running status -> final report), HTML-escapes the untrusted report, reports skip/na/ prepare-fail/infra outcomes explicitly since /verify is always an explicit request - new verify-pr skill: A/B load-bearing proof, vacuity check on new tests, mock-free wire-oracle harnesses, targeted gates, fixed report/ verdict/assertions artifact contract, counts-are-sacred rules - triage skill Stage 2c now names /verify (not just /tmux) as the trigger to recommend when a PR's central claim needs behavioral evidence The verify check-runs ride the issue_comment event, which the finalize workflow's event == "pull_request" universe structurally excludes, so they cannot pollute the CI table or the deferred-approval gate. * feat(triage): teach /verify round continuity and artifact-matched methods Fold two more hand-verification patterns into the verify lane: - round continuity: the resolve step snapshots the previous verify report (if any) into the agent context before the status upsert overwrites it, and the skill re-checks each prior finding at the new head (fixed/stands/superseded), scoping new probes to the delta - harness quality: prefer configuration seams over module interception, encode the upstream's real semantics in the fake peer, add decoy targets - artifact-matched methods: per-commit load-bearing tables for multi-commit PRs; workflow/CI PRs get embedded-script replay against real data, repo lint gates, and day-one trigger cost math from real event history; every new config knob must trace to an observable effect, and default-path dispatch combinations get probed - findings quality: blockers enumerate blast radius, demonstrate the sharpest consequence end-to-end when budget allows, and carry a collapsed minimal suggested fix preserving the original commit's intent * feat(triage): host /verify evidence images and encode quantified-A/B rules Borrow the image-evidence and quantified-verification patterns from hand-run rounds (QwenLM#7265, QwenLM#7471, QwenLM#7686 r2 and the pr-assets convention): - publish-verify now hosts agent-produced evidence/*.png on the pr-assets branch (verify/pr<N>-<run>-<attempt>/) and appends them below the escaped report. Untrusted-payload discipline: strict filename allowlist, 8-image / 2 MB caps enforced in the find predicates, racing-push retry, and every failure degrades to a text-only comment. VERIFY_ASSETS_REMOTE is a test seam; the block was dry-run against a local bare remote covering hosting, hostile filenames, oversize files, dotfiles, missing branch, and no-image runs - skill: evidence images are named as kebab-case captions binding image to claim, before/after pairs over lone after-shots; follow-up rounds lead with a previous-finding status table (fixed/stands/superseded/declined, with adjudication) and re-measure instead of diffing the old report; size/perf claims get measured-metric Δ tables with residual deltas accounted for; unreachable branches get the configuration that reaches them constructed; defensive guards get their accept path checked against real production artifacts, not just mocked rejects * fix(triage): address /review suggestions on the verify lane - skill: local invocation resolves --repo and passes it to every gh call - skill: call out the dependency confound when the base A/B side reuses the PR-installed node_modules and the PR touches package.json/lockfile - workflow: document the pin step's bootstrap logic — issue_comment jobs run the default branch's YAML, so base always carries the verify-pr skill by the time this job exists * fix(triage): harden /verify gate, comment budget, and evidence hosting per review Address review round 5078770575 items 1-3 plus the cheap follow-ups: - authorize: /verify now requires write from BOTH the PR author (whose code runs) and the commenter (who spends a scarce runner slot + model budget) — a drive-by account can no longer burn 45 minutes of ecs-qwen on someone else's PR; duplicates check once; /tmux and /triage gates unchanged. Replayed 8 principal scenarios against a stubbed gh - authorize acks /verify with the eyes reaction from the always-hosted job, so a queued/saturated sandbox pool no longer means total silence - publish: emit_block escapes FIRST and caps the escaped size (45 KB for the report) — a raw-side cap let dense <>& content inflate past GitHub's 65,536-char comment limit, 422 the post, and strand the running status with no report at all; iconv -c keeps a UTF-8 sequence split by the byte cut (likely, given the mandated 中文 summary) from shipping broken; replayed: 50 KB dense report -> 45,873-byte body - publish: image cap is byte-exact (-size -2097153c; find's -2M rounds sizes UP to MiB, silently making the documented 2 MB cap 1 MiB), bytes must carry the PNG magic (extension is attacker-choosable), duplicate sanitized names dedupe instead of overwriting + double-rendering, and dropped images are reported in the comment instead of vanishing - publish: weak terminal notices (cancelled/infra/skipped/n-a) only replace this run's own running status; a previous round's real report survives as the marker comment and the notice posts fresh - publish: report.md/assertions.json lookups pin the artifact-dir shape and sort (bare find -name order is filesystem-dependent); the verify job's verdict.txt lookup sorts likewise - verify: global npm install runs from RUNNER_TEMP (the persistent workspace still holds the PREVIOUS run's tree, whose .npmrc would apply to a root install); both cleanup passes remove leftover tmp/ worktrees (git worktree prune alone only drops metadata); the run step no longer re-chowns 50k node_modules files; pr-assets clone sets its committer identity once so the racing-push rebase retry can commit - skill: worktree guidance now tells the agent to remove its base tree itself, with the workflow sweep as backstop only * fix(triage): close runtime-plant and stale-RUNNER_TEMP channels in /verify Address review round 2 (comment 5079157987) and the CHANGES_REQUESTED round on the verify lane: - run step re-sweeps tmp/*-verify-* AFTER npm ci/build and before the agent starts: the pin step's sweep runs before PR lifecycle scripts (postinstall etc.), which could re-plant a fake artifact dir whose zeroed timestamp deterministically wins the sorted collector. From the sweep on, only the agent writes those dirs; a steered agent forging its own artifacts remains the documented advisory-report residual - RUNNER_TEMP verify-results/verify-context are rm'd before mkdir: the pool is persistent and runner temp hygiene is runner-managed — a stale report or previous-report.md from ANOTHER PR must never ride along - symlinks are stripped from verify-results before upload: actions/upload-artifact dereferences them, so a node-planted link would exfiltrate whatever it points at into the artifact - a trusted commenter invoking /verify on a PR whose author lacks write now gets an explanation comment from the hosted authorize job instead of total silence (the commenter is checked first; drive-by accounts and API errors still get nothing); job timeout 45->60 so a slow install can never let the JOB limit kill the agent past its own graceful 25m budget - stale tmp/base-tree (skill's canonical scratch worktree) is removed by name at job start — a plain dir isn't git-registered, so the worktree sweep alone misses it and the next worktree add would fail - scripts/tests/qwen-triage-workflow.test.js gains a verify-lane describe block: an 8-arm stub-gh replay of the dual principal gate (drive-by deny, author-without-write deny + explain flag, self-comment dedupe, 404 fail-closed, /tmux and /triage unchanged) plus guards for the post-prepare sweep placement, the symlink strip, and the RUNNER_TEMP resets — the replay found this commit's sweep edit had silently not applied, which is exactly the regression class it exists to catch * fix(triage): close proxy-hijack, gate-bypass, and false-verdict paths in /verify Address the Codex /review round (19 findings) and the bot's follow-up. Each fix was replayed locally; the proxy fix has a decisive A/B. Gate and routing: - the shell command match is case-insensitive: GitHub Actions expression comparisons ignore case, so `@QWEN-CODE /VERIFY` reached the step and fell through to the commenter-only branch — running the PR author's code with the author never checked - the verify ack and denial notice require github.event.issue.pull_request: /verify on a plain issue was acknowledged but could never report - publish-verify joins the verify job's per-PR concurrency group, and a failed PATCH falls back to posting fresh instead of going silent Untrusted-input paths: - the model proxy binds an EPHEMERAL port, reports it through a root-owned file, and its health check must echo a per-run nonce with the recorded PID alive. A/B with a squatter on 8787: the old code's proxy dies EADDRINUSE yet still reports enabled and points qwen at the squatter; the new code comes up unaffected on an ephemeral port - worktree-scoped git config is deleted before hooksPath is resolved: `extensions.worktreeConfig` is allowlisted and .git/config.worktree is invisible to `git config --local`, so a prior run could set core.hooksPath=/ and make the hook sweep's recursive delete walk / as root (verified locally). The sweep now also refuses any hooks path outside the repository's git dir - marker-comment lookups accept only bot-owned comments that START with the marker: any user can paste the marker and divert the bot into PATCHing a stranger's comment - the upload staging dir is re-flushed after npm lifecycle scripts Honest verdicts: - the docs-only classifier no longer uses a pipeline (grep -q made the writer take SIGPIPE, so under pipefail a long file list with an early code file classified a code PR as docs-only and skipped verification), and executable markdown/YAML (.qwen, .github/workflows, scripts) is classified as behavioral before the extension rule - tee's status is checked alongside qwen's: a full results volume made a truncated evidence stream publish as pass - 137 is split by elapsed budget into watchdog timeout vs crash/OOM - the agent's verdict is honored only for VERDICT=pass with a report and zero failed assertions; otherwise the process outcome headlines and the scope paragraph says the run did not complete - verdict.txt is read through a bounded prefix (SIGPIPE under pipefail) Skill contract corrections: per-commit tables only when the commits are reachable at depth 2 (else aggregate + Not covered); internal workspace symlinks must have their realpaths asserted before a base control is trusted; repo lint gates and event-history cost math are qualified to what the token-free container can actually run; --repo is never inferred from `origin` (a fork holds a different PR under the same number). Tests: 9 new guards, all mutation-verified (reverting each fix turns one red), including an executable escaping/size-cap/UTF-8 test for the publisher's own emit_block and a fix to the earlier command-file test, which matched the tmux job's identically named step. * fix(triage): re-establish the /verify trust boundary after PR code runs Third review round (31 findings). The unifying defect: everything the lane pinned or swept happened BEFORE npm ci/build executed PR-authored lifecycle scripts as node, so each control could be undone afterwards. Trust boundary, rebuilt in order before the agent starts: - kill every process owned by the build user and fail the step if any survives — a detached postinstall child could otherwise wait out each one-shot sweep and re-plant afterwards - re-pin .qwen from the base commit again, now root-owned and read-only: the prepare step chowns the workspace to node, so a lifecycle script could rewrite the very skill that defines /verify-pr - give the agent a fresh HOME/QWEN_HOME: qwen loads user-scope file commands from $HOME/.qwen, and /home/node belongs to the build user, so a planted commands/verify-pr.toml could shadow the pinned skill - the model proxy now requires a per-run bearer token, closing the blind-localhost-scan path to an unauthenticated signer for the real model credential (a command the agent itself launches still inherits it — documented residual, not closed) Authorization and lifecycle: - re-verify the PR author's write permission at execution time and pin the authorized head OID; refuse if the checked-out HEAD^2 differs, so a push during the runner wait cannot smuggle in unreviewed code - validate each principal separately: an empty author vanished in word splitting and left only the commenter checked - honor MAINTAINER_ECS_RUNNER_DISABLED with an explicit notice instead of queueing forever against a disabled pool - status comments carry a machine state marker; inferring 'running' from prose let a report quoting that sentence be overwritten - previous-report.md snapshots the newest substantive report, never a weak/cancelled notice, so prior findings survive into the next round - bot-identity lookup failures fail closed instead of widening the ownership filter to every user's comments - publish-verify uses a per-run concurrency group: a per-PR group holds only one pending job, so a second /verify could cancel a completed run's pending publisher Correctness: - install/build failures are classified: signals, ENOSPC, registry and network errors are infra-error, not a PR verdict - watchdog classification measures the child's own elapsed time, not shell-global $SECONDS which includes proxy setup - assertions.json must be three non-negative integers with a positive total and total == pass + fail before it counts as evidence - the proxy keeps its upstream deadline armed until the body ends and aborts upstream when the client disconnects - cleanups remove .qwen/tmp itself: PR code can make it a symlink, and globbing below it deleted the target's contents as root (verified) - emit_block materializes the escaped text and truncates on a character boundary via node — iconv -c passes an incomplete trailing sequence through on BSD (measured), which the new test caught Skill: local mode requires the same isolation CI provides and must not assume HEAD^1/HEAD^2 on a plain head checkout; shallow boundaries make rev-list counts unreliable for per-commit claims; never run scripts/lint.js with no arguments (it runs prettier --write and rewrites the tree under the harnesses); a vacuity check must fail the intended assertion, not the import. pr-workflow.md now says both sandboxed lanes need the author to have write, so triage stops recommending a guaranteed denial on external PRs. Tests: 9 more guards, all mutation-verified, including executable replays of the docs-only classifier (SIGPIPE + executable-markdown cases), the uppercase-command gate, the empty-principal deny, and the untrusted-image hosting path against a bare pr-assets remote. * test(triage): pass the classifier fixture through a file, not argv The new docs-only classifier replay passed on macOS and failed on CI with `Cannot read properties of undefined (reading 'trim')`: its 60,001-entry fixture is ~889 KB and was passed as a single argv element. Linux caps one argument at MAX_ARG_STRLEN (128 KB), so the spawn failed with E2BIG and stdout was undefined; macOS has no per-argument limit and only a ~1 MB total, so the same call succeeded locally (verified both). Write the list to a temp file and pass the path. The harness now also asserts the spawn succeeded, so a future spawn failure reports itself instead of surfacing as a TypeError on undefined output. * fix(triage): make the /verify report match what the run actually produced Three publisher findings, all introduced by my own previous round: - an artifact download failure (the step is continue-on-error) let the full-report path run with no results: the headline read 'completed' and the scope paragraph claimed the A/B, the harnesses and the gates had run when nothing had been delivered. The download outcome is now an input, and its failure gets its own body saying the results could not be retrieved - the prepare-failure branch ignored the verdict the prepare step had just computed, so an install killed by a registry outage or OOM (classified infra-error) still told the author 'this is treated as a PR failure verdict rather than an infrastructure failure' — the exact opposite. It now branches on the verdict, and an infra-classified prepare failure is a weak body that cannot overwrite a real report - weak notices were being snapshotted as the follow-up round's previous-report.md: they lack the running marker, so 'newest non-running comment' selected them. Bodies that carry findings now mark themselves (qwen-triage:verify-substantive) and the snapshot selects on that marker. A/B on the real jq: report A then cancelled B now snapshots A (101), the old filter picked B (102) Tests: 4 more guards, all mutation-verified — the publisher is rendered for each outcome with a stubbed gh and the assertions read the body it would post, and the snapshot test runs the workflow's own jq program verbatim against a paginate-shaped fixture. * fix(triage): stop PR build output from masquerading as an infra failure Two review findings plus a test-helper hazard: - classify_failure grepped the prepare log for bare words like ENOSPC and ETIMEDOUT, but that log is written by PR-controlled code: a genuine build failure that merely prints 'expected ETIMEDOUT to equal ok' would be published as an infrastructure incident, telling the author to re-run something that fails identically. The patterns are now anchored to lines only npm's reporter or the kernel emits ('npm ERR! code E…', 'npm ERR! network …', kernel OOM, bare 'Killed'); a signal exit still needs no log evidence. Replayed 10 cells: four PR-authored logs quoting infra words stay 'fail', five real diagnostics and one signal exit are 'infra-error' - the two execution-time controls added last round — re-verifying the author's permission after the runner wait, and refusing a head that moved since authorization — had no tests. Both are now executed: the re-auth snippet against a stubbed permission API (write proceeds and pins head_oid; read skips with a publishable reason), and the pin step against a real git repo with a real merge commit (matching head proceeds, moved head exits non-zero) - add a stepIn(job, step) test helper. Several step names exist in both the tmux and verify jobs, and the unscoped step() returns the first match, so a verify-lane assertion silently tests the tmux copy — that has now bitten this suite three times, including in this commit. * docs(triage): teach verify-pr test-only PRs, differential oracles, gate liveness Fold techniques from the round-2 verification on QwenLM#7620 (an ANSI parser PR) that the skill had no equivalent for: - test-only PRs get their own method: a mutation A/B across TEST FILES (same mutants of the unmodified production file, only the test file swapped), reporting killed/total on both sides, requiring that no mutant regressed from killed to survived, checking that the killing assertion is the one the commit claims to have strengthened, and adjudicating every survivor as coverage gap or defect with independent evidence rather than by inspection - when the code emulates a known implementation, that implementation is the oracle: feed identical input to both and report disagreement counts per side, lift reference tables verbatim out of the shipped dependency, and build the corpus from bytes captured off a real producer alongside synthesized sweeps - prove a gate is live before citing it: plant a violation the linter must catch, confirm it is reported, remove it — a linter that matched no files exits 0 exactly like one that passed - attribute pre-existing failures by byte-identical failing file AND test names on both sides, with deltas, not just totals - when the base is far behind, verify the merge: trial-merge into current main, confirm it is conflict-free, and re-run the affected suite on the merged tree - round continuity gains its one legitimate shortcut: a production file proven byte-identical (sha256 quoted at both heads) carries prior evidence forward by construction * style(triage): reflow verify-pr skill to prettier's markdown wrapping The previous commit's added paragraphs were hand-wrapped and prettier --check flagged the file; the repo runs prettier over all of it. * test(triage): cover the disabled-runner-pool notice The kill-switch path had no test: a refactor could drop the notice and leave a /verify request acknowledged with 👀 but permanently unanswered, since the verify job refuses to start and publish-verify skips with it. Fold the step into the existing PR-guard loop (now scoped through stepIn, so it cannot match a same-named step in another job) and assert the parts that make the answer useful — the kill-switch and permission conditions, both languages, the alternative it points at, and the verify job's own exclusion of the disabled pool. All three mutations turn it red: removing the step, dropping its PR guard, or letting the verify job queue against the disabled pool. * fix(triage): repair a step-killing PIPESTATUS read and six forgeable controls Sixth review round, 12 findings. Several are regressions from my own two previous rounds; the first would have broken every single run. - `AGENT_STATUS=${PIPESTATUS[0]}` is itself a command and resets PIPESTATUS, so the next line's ${PIPESTATUS[1]} was unset and `set -u` aborted the step immediately after the agent finished — before artifact collection, the verdict, or anything else. Verified by replaying the exact structure: 'PIPESTATUS[1]: unbound variable'. Both elements are now snapshotted in one command - concurrency predicates were broader than the job conditions they guard, and GitHub evaluates concurrency BEFORE the job `if`: a /verify comment entered the triage job's shared per-PR group (where it could displace a pending /triage and then skip), and a /verify queued while the runner kill switch was on did the same to a real verification. Both predicates now match their job's runnable set exactly - an outward-resolving .git/hooks entry was only warned about and left in place, so the next root-owned git command would run it. It is now unlinked without traversing its target, a root-owned hooks directory is restored, and core.hooksPath is unset - the second .qwen pin re-derived HEAD^1 from git metadata after the workspace, including .git, had been handed to the build user. The base OID is now recorded while .git is still root-owned and the re-pin archives that content-addressed OID - classify_failure took both of its inputs from PR-controlled sources: a lifecycle script can exit with a signal status and can print any line the log patterns matched, turning its own deterministic breakage into 'infrastructure, please re-run' — which hid the failure and preserved a stale report. No infra verdict is derivable there, so the prepare step reports `fail` and lets the embedded log speak for itself - cleanups descended through PR-writable parents: `.qwen` itself can be a symlink, and the worktree sweep trusted git metadata with only a lexical prefix check. Symlinks are unlinked without traversal and worktree paths must canonicalize inside the workspace. Replayed all three escapes - skipped and docs-only outcomes upload no artifact, so the new download-failure branch pre-empted them and made their real reason unreachable; they are answered first now - a run that crashed before writing report.md still claimed the substantive marker, letting a headline overwrite the previous round's evidence. The marker now requires a report Skill: the byte-identical shortcut needs the whole input closure, not one file hash; the credential-free local path cannot call `gh` at all (fetch the metadata outside and mount it read-only); and the A/B base is `baseRefOid` in local mode, not `HEAD^1`. Tests: 7 new guards plus 4 updated to the new shapes, all mutation-verified (50/50). * fix(triage): answer dropped /verify requests and prove the proxy rejects Maintainer review (yiliang114), 7 items: - a third /verify while two runs are in flight is dropped by the concurrency group with no job and therefore no comment. The hosted authorize job now counts this workflow's other in-flight runs and says so; an API hiccup leaves the request alone rather than denying it - the proxy's bearer check had no executable test. It now starts the real proxy against a real upstream and issues real requests: no header and a wrong token are 401, this run's token is 200, and a route other than /chat/completions is 403 — with the health endpoint echoing the nonce - the 502 path forwarded the raw upstream error, which can name resolved hosts and TLS detail to PR code. It logs server-side and returns a generic failure - publish-verify inherited the 360-minute default; it downloads one artifact and posts one comment, so it is bounded at 10 - removing the log classifier last round left the comment block it replaced, which still said failures are classified from the exit status and the log. Deleted - that removal also left every install failure reported as the PR's fault, including a registry outage. There is exactly one signal here PR code cannot write — asking the registry ourselves, as root, with the container's resolver — so an install failure is downgraded to infra-error only when that probe fails. It proves reachability now rather than at failure time, so it can only ever downgrade, never confirm; a build failure has no equivalent and stays the tree's problem - the skill's local-invocation warning ran into the preceding sentence, which GFM renders as one paragraph Tests: 5 new guards, all mutation-verified (55/55). * fix(triage): resolve hooks hermetically and mirror symlink guards at job end Maintainer review round (doudouOUC), 6 findings. Two were Critical and both reproduced: - the hooks sweep resolved its path with the ambient git config in play. With a global core.hooksPath set — which the reviewer has and I do not, which is why my earlier replay showed a false pass — `git rev-parse --git-path hooks` returns that global path, the in-git-dir guard reads 'outside', and a planted `.git/hooks` symlink survives untouched. A/B: old code leaves the symlink under a global hooksPath, new code removes it in both environments and never touches the link target. Resolution now runs with GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM pointed at /dev/null - the END-of-job cleanup still used the bare `rm -rf .qwen/tmp` that the start-of-job cleaner was hardened against two rounds ago. The agent executes PR code between the two, so the end is no safer than the start: it now unlinks symlinks without descending and canonicalizes worktree paths inside the workspace before deleting Plus four suggestions, all valid: - the saturation notice counted this workflow's in-flight runs across every PR while the concurrency group is per-PR, so a run on another PR would trigger a warning about a queue that does not exist. It now matches on the PR title (the only per-PR handle an issue_comment run record carries) and stays silent when that cannot be resolved - the skill recommended `require.resolve` for the workspace-realpath check; these packages are ESM-only with import-only exports, so it throws ERR_PACKAGE_PATH_NOT_EXPORTED and reads like a missing module. Verified, and replaced with `readlink -f node_modules/@qwen-code/...` - the symlink-escape test inherited the developer's git config, which is what hid the first finding. It now runs with global/system config neutralized AND repeats the case with a global core.hooksPath planted - the publisher's build-phase arm was never rendered by any test (every case used 'install'), so a typo in that command name would have shipped. Now covered, along with an unrecognized phase Mutation-verified 4/4. The hooks guard needed a discriminating assertion: git's own `*.sample` files must survive the sweep, because the outward-path fallback removes the whole directory and would otherwise satisfy a bare 'planted hook is gone' check. * fix(triage): count only /verify runs for saturation, and test the PATCH arm Bot review round, 2 suggestions, both valid: - the saturation notice matched runs by PR title, which narrowed to this PR but not to /verify. /triage and /tmux live in their own concurrency groups, so two of those in flight would warn about a verify queue that is actually empty. It now also requires the run to have a job named 'verify' — the run record carries no command, but its job list does. Replayed: two non-verify runs stay silent, two verify runs warn - every publish fixture returned an empty comments listing, so the PATCH arm was never executed: a broken PATCH would have stranded the running status comment and posted a duplicate below it, with the suite green. The publisher now runs against a stubbed listing and the test asserts which verb went to which comment id — bot-owned live status is PATCHed in place, an absent comment posts fresh, and a marker comment owned by someone else is left alone and posted around Mutation-verified 3/3: counting every command, never PATCHing, and accepting foreign-owned markers each turn one test red. Two stub bugs found while writing these, both mine and both silent: ${*#pattern} applies per positional parameter rather than to the joined string (yielding a wrong run id), and the paginate fixture needs one array per page, not an array of pages. * fix(triage): fix the real silent drop and drop the step built on a wrong premise Review round 4. The blocker was mine twice over: the saturation notice I added last round had GitHub's concurrency semantics backwards, and the silent drop it claimed to cover was somewhere else entirely. - GitHub cancels the OLDER pending run in a group and admits the new one (confirmed against the workflow-syntax reference). My step told the person who had just typed /verify that their request might be dropped, when theirs is the one that runs — and said nothing to the person whose queued run actually died. This PR already had it right in publish-verify's own comment, so the file contradicted itself and the user-facing copy followed the wrong half. The step is removed rather than reworded: with the fix below there is nothing left for it to warn about, and it cost 2+N API calls on every /verify. - the actual drop: a verify job cancelled while still PENDING never reaches a runner, so its outputs block — where the "|| github.event.issue.number" fallback lived — is never evaluated. publish-verify then read an empty PR_NUMBER, hit its own guard and exited 0, making the cancelled branch unreachable in exactly the scenario that produces cancellations. The fallback now lives where the value is read. Reproduced both arms by executing the real step: with a number the cancelled notice posts, with an empty one it only warns. - same one-line class in publish-tmux, fixed alongside. Two copy defects from the classifier removal, both mis-attribution pointed the other way: - the infra-error body still named a signal/OOM kill and a full disk, none of which the current prepare step can produce — infra-error now requires npm ci to fail AND the registry probe to fail. It names that condition only, and offers a re-run instead of asserting it is the fix. - the code comment above it still described the deleted classifier. Also fixes the indentation break an earlier scripted edit left in the publish body builder, and replaces the saturation test with one that executes the cancelled path. Mutation-verified 2/2; the copy needed its own guard, since reverting the wording alone left every test green. * docs(triage): teach verify-pr survivor accounting and observability regressions Fold techniques from the re-verification on QwenLM#7709 that the skill had no equivalent for: - the mutation matrix must report the mutations that changed NOTHING, not only the ones that failed. Each survivor gets classified as an ordinary coverage gap or as dead code — a guard whose deletion leaves every test green is one of those two, and the difference is what the author needs. Survivors mirroring a pre-existing gap are labelled as such, and the set is framed as completeness reporting rather than merge conditions - the sharper case that report demonstrates: a test that passes for the WRONG REASON. If deleting the new guard leaves its own new test green, that test is pinned by an earlier early-return, not by the change, and asserts nothing about it. Name what actually pins it - and do not generalize from one dead guard to its siblings: the same report shows a clause that is unreachable on one path while being the only protection on another. Check each, report the contrast - observability regressions: when a change suppresses output, follow the value before calling the suppression correct. A bare catch on the path plus a field with no readers anywhere in the repo means the cause is now unobservable even in devtools — a real loss that no behavioural assertion can see - report structure gains a Corrections section: when an earlier round or bot comment described the code inaccurately, state the correct fact with evidence and label it as a correction to the description, not a request to change code. A wrong description left standing costs the next reader more than the original finding did --------- Co-authored-by: wenshao <wenshao@example.com>
…LM#7753) * feat(triage): add sandboxed /verify deep-verification lane @qwen-code /verify on a PR now runs a local-verification-style evidence round in the isolated /tmux sandbox contract (container, token-free agent env, loopback model proxy, author-write gate) and publishes the report via a separate PR-code-free job: - new verify job: merge-ref checkout at depth 2 (base tip + PR head for A/B), skills pinned from base so the tree under test can never rewrite its own verifier, PR-planted tmp/*-verify-* artifacts dropped, git exec-vector sweep for the persistent workspace, agent verdict allowlisted before it reaches workflow outputs - new publish-verify job: upserts one marker comment (running status -> final report), HTML-escapes the untrusted report, reports skip/na/ prepare-fail/infra outcomes explicitly since /verify is always an explicit request - new verify-pr skill: A/B load-bearing proof, vacuity check on new tests, mock-free wire-oracle harnesses, targeted gates, fixed report/ verdict/assertions artifact contract, counts-are-sacred rules - triage skill Stage 2c now names /verify (not just /tmux) as the trigger to recommend when a PR's central claim needs behavioral evidence The verify check-runs ride the issue_comment event, which the finalize workflow's event == "pull_request" universe structurally excludes, so they cannot pollute the CI table or the deferred-approval gate. * feat(triage): teach /verify round continuity and artifact-matched methods Fold two more hand-verification patterns into the verify lane: - round continuity: the resolve step snapshots the previous verify report (if any) into the agent context before the status upsert overwrites it, and the skill re-checks each prior finding at the new head (fixed/stands/superseded), scoping new probes to the delta - harness quality: prefer configuration seams over module interception, encode the upstream's real semantics in the fake peer, add decoy targets - artifact-matched methods: per-commit load-bearing tables for multi-commit PRs; workflow/CI PRs get embedded-script replay against real data, repo lint gates, and day-one trigger cost math from real event history; every new config knob must trace to an observable effect, and default-path dispatch combinations get probed - findings quality: blockers enumerate blast radius, demonstrate the sharpest consequence end-to-end when budget allows, and carry a collapsed minimal suggested fix preserving the original commit's intent * feat(triage): host /verify evidence images and encode quantified-A/B rules Borrow the image-evidence and quantified-verification patterns from hand-run rounds (QwenLM#7265, QwenLM#7471, QwenLM#7686 r2 and the pr-assets convention): - publish-verify now hosts agent-produced evidence/*.png on the pr-assets branch (verify/pr<N>-<run>-<attempt>/) and appends them below the escaped report. Untrusted-payload discipline: strict filename allowlist, 8-image / 2 MB caps enforced in the find predicates, racing-push retry, and every failure degrades to a text-only comment. VERIFY_ASSETS_REMOTE is a test seam; the block was dry-run against a local bare remote covering hosting, hostile filenames, oversize files, dotfiles, missing branch, and no-image runs - skill: evidence images are named as kebab-case captions binding image to claim, before/after pairs over lone after-shots; follow-up rounds lead with a previous-finding status table (fixed/stands/superseded/declined, with adjudication) and re-measure instead of diffing the old report; size/perf claims get measured-metric Δ tables with residual deltas accounted for; unreachable branches get the configuration that reaches them constructed; defensive guards get their accept path checked against real production artifacts, not just mocked rejects * fix(triage): address /review suggestions on the verify lane - skill: local invocation resolves --repo and passes it to every gh call - skill: call out the dependency confound when the base A/B side reuses the PR-installed node_modules and the PR touches package.json/lockfile - workflow: document the pin step's bootstrap logic — issue_comment jobs run the default branch's YAML, so base always carries the verify-pr skill by the time this job exists * fix(triage): harden /verify gate, comment budget, and evidence hosting per review Address review round 5078770575 items 1-3 plus the cheap follow-ups: - authorize: /verify now requires write from BOTH the PR author (whose code runs) and the commenter (who spends a scarce runner slot + model budget) — a drive-by account can no longer burn 45 minutes of ecs-qwen on someone else's PR; duplicates check once; /tmux and /triage gates unchanged. Replayed 8 principal scenarios against a stubbed gh - authorize acks /verify with the eyes reaction from the always-hosted job, so a queued/saturated sandbox pool no longer means total silence - publish: emit_block escapes FIRST and caps the escaped size (45 KB for the report) — a raw-side cap let dense <>& content inflate past GitHub's 65,536-char comment limit, 422 the post, and strand the running status with no report at all; iconv -c keeps a UTF-8 sequence split by the byte cut (likely, given the mandated 中文 summary) from shipping broken; replayed: 50 KB dense report -> 45,873-byte body - publish: image cap is byte-exact (-size -2097153c; find's -2M rounds sizes UP to MiB, silently making the documented 2 MB cap 1 MiB), bytes must carry the PNG magic (extension is attacker-choosable), duplicate sanitized names dedupe instead of overwriting + double-rendering, and dropped images are reported in the comment instead of vanishing - publish: weak terminal notices (cancelled/infra/skipped/n-a) only replace this run's own running status; a previous round's real report survives as the marker comment and the notice posts fresh - publish: report.md/assertions.json lookups pin the artifact-dir shape and sort (bare find -name order is filesystem-dependent); the verify job's verdict.txt lookup sorts likewise - verify: global npm install runs from RUNNER_TEMP (the persistent workspace still holds the PREVIOUS run's tree, whose .npmrc would apply to a root install); both cleanup passes remove leftover tmp/ worktrees (git worktree prune alone only drops metadata); the run step no longer re-chowns 50k node_modules files; pr-assets clone sets its committer identity once so the racing-push rebase retry can commit - skill: worktree guidance now tells the agent to remove its base tree itself, with the workflow sweep as backstop only * fix(triage): close runtime-plant and stale-RUNNER_TEMP channels in /verify Address review round 2 (comment 5079157987) and the CHANGES_REQUESTED round on the verify lane: - run step re-sweeps tmp/*-verify-* AFTER npm ci/build and before the agent starts: the pin step's sweep runs before PR lifecycle scripts (postinstall etc.), which could re-plant a fake artifact dir whose zeroed timestamp deterministically wins the sorted collector. From the sweep on, only the agent writes those dirs; a steered agent forging its own artifacts remains the documented advisory-report residual - RUNNER_TEMP verify-results/verify-context are rm'd before mkdir: the pool is persistent and runner temp hygiene is runner-managed — a stale report or previous-report.md from ANOTHER PR must never ride along - symlinks are stripped from verify-results before upload: actions/upload-artifact dereferences them, so a node-planted link would exfiltrate whatever it points at into the artifact - a trusted commenter invoking /verify on a PR whose author lacks write now gets an explanation comment from the hosted authorize job instead of total silence (the commenter is checked first; drive-by accounts and API errors still get nothing); job timeout 45->60 so a slow install can never let the JOB limit kill the agent past its own graceful 25m budget - stale tmp/base-tree (skill's canonical scratch worktree) is removed by name at job start — a plain dir isn't git-registered, so the worktree sweep alone misses it and the next worktree add would fail - scripts/tests/qwen-triage-workflow.test.js gains a verify-lane describe block: an 8-arm stub-gh replay of the dual principal gate (drive-by deny, author-without-write deny + explain flag, self-comment dedupe, 404 fail-closed, /tmux and /triage unchanged) plus guards for the post-prepare sweep placement, the symlink strip, and the RUNNER_TEMP resets — the replay found this commit's sweep edit had silently not applied, which is exactly the regression class it exists to catch * fix(triage): close proxy-hijack, gate-bypass, and false-verdict paths in /verify Address the Codex /review round (19 findings) and the bot's follow-up. Each fix was replayed locally; the proxy fix has a decisive A/B. Gate and routing: - the shell command match is case-insensitive: GitHub Actions expression comparisons ignore case, so `@QWEN-CODE /VERIFY` reached the step and fell through to the commenter-only branch — running the PR author's code with the author never checked - the verify ack and denial notice require github.event.issue.pull_request: /verify on a plain issue was acknowledged but could never report - publish-verify joins the verify job's per-PR concurrency group, and a failed PATCH falls back to posting fresh instead of going silent Untrusted-input paths: - the model proxy binds an EPHEMERAL port, reports it through a root-owned file, and its health check must echo a per-run nonce with the recorded PID alive. A/B with a squatter on 8787: the old code's proxy dies EADDRINUSE yet still reports enabled and points qwen at the squatter; the new code comes up unaffected on an ephemeral port - worktree-scoped git config is deleted before hooksPath is resolved: `extensions.worktreeConfig` is allowlisted and .git/config.worktree is invisible to `git config --local`, so a prior run could set core.hooksPath=/ and make the hook sweep's recursive delete walk / as root (verified locally). The sweep now also refuses any hooks path outside the repository's git dir - marker-comment lookups accept only bot-owned comments that START with the marker: any user can paste the marker and divert the bot into PATCHing a stranger's comment - the upload staging dir is re-flushed after npm lifecycle scripts Honest verdicts: - the docs-only classifier no longer uses a pipeline (grep -q made the writer take SIGPIPE, so under pipefail a long file list with an early code file classified a code PR as docs-only and skipped verification), and executable markdown/YAML (.qwen, .github/workflows, scripts) is classified as behavioral before the extension rule - tee's status is checked alongside qwen's: a full results volume made a truncated evidence stream publish as pass - 137 is split by elapsed budget into watchdog timeout vs crash/OOM - the agent's verdict is honored only for VERDICT=pass with a report and zero failed assertions; otherwise the process outcome headlines and the scope paragraph says the run did not complete - verdict.txt is read through a bounded prefix (SIGPIPE under pipefail) Skill contract corrections: per-commit tables only when the commits are reachable at depth 2 (else aggregate + Not covered); internal workspace symlinks must have their realpaths asserted before a base control is trusted; repo lint gates and event-history cost math are qualified to what the token-free container can actually run; --repo is never inferred from `origin` (a fork holds a different PR under the same number). Tests: 9 new guards, all mutation-verified (reverting each fix turns one red), including an executable escaping/size-cap/UTF-8 test for the publisher's own emit_block and a fix to the earlier command-file test, which matched the tmux job's identically named step. * fix(triage): re-establish the /verify trust boundary after PR code runs Third review round (31 findings). The unifying defect: everything the lane pinned or swept happened BEFORE npm ci/build executed PR-authored lifecycle scripts as node, so each control could be undone afterwards. Trust boundary, rebuilt in order before the agent starts: - kill every process owned by the build user and fail the step if any survives — a detached postinstall child could otherwise wait out each one-shot sweep and re-plant afterwards - re-pin .qwen from the base commit again, now root-owned and read-only: the prepare step chowns the workspace to node, so a lifecycle script could rewrite the very skill that defines /verify-pr - give the agent a fresh HOME/QWEN_HOME: qwen loads user-scope file commands from $HOME/.qwen, and /home/node belongs to the build user, so a planted commands/verify-pr.toml could shadow the pinned skill - the model proxy now requires a per-run bearer token, closing the blind-localhost-scan path to an unauthenticated signer for the real model credential (a command the agent itself launches still inherits it — documented residual, not closed) Authorization and lifecycle: - re-verify the PR author's write permission at execution time and pin the authorized head OID; refuse if the checked-out HEAD^2 differs, so a push during the runner wait cannot smuggle in unreviewed code - validate each principal separately: an empty author vanished in word splitting and left only the commenter checked - honor MAINTAINER_ECS_RUNNER_DISABLED with an explicit notice instead of queueing forever against a disabled pool - status comments carry a machine state marker; inferring 'running' from prose let a report quoting that sentence be overwritten - previous-report.md snapshots the newest substantive report, never a weak/cancelled notice, so prior findings survive into the next round - bot-identity lookup failures fail closed instead of widening the ownership filter to every user's comments - publish-verify uses a per-run concurrency group: a per-PR group holds only one pending job, so a second /verify could cancel a completed run's pending publisher Correctness: - install/build failures are classified: signals, ENOSPC, registry and network errors are infra-error, not a PR verdict - watchdog classification measures the child's own elapsed time, not shell-global $SECONDS which includes proxy setup - assertions.json must be three non-negative integers with a positive total and total == pass + fail before it counts as evidence - the proxy keeps its upstream deadline armed until the body ends and aborts upstream when the client disconnects - cleanups remove .qwen/tmp itself: PR code can make it a symlink, and globbing below it deleted the target's contents as root (verified) - emit_block materializes the escaped text and truncates on a character boundary via node — iconv -c passes an incomplete trailing sequence through on BSD (measured), which the new test caught Skill: local mode requires the same isolation CI provides and must not assume HEAD^1/HEAD^2 on a plain head checkout; shallow boundaries make rev-list counts unreliable for per-commit claims; never run scripts/lint.js with no arguments (it runs prettier --write and rewrites the tree under the harnesses); a vacuity check must fail the intended assertion, not the import. pr-workflow.md now says both sandboxed lanes need the author to have write, so triage stops recommending a guaranteed denial on external PRs. Tests: 9 more guards, all mutation-verified, including executable replays of the docs-only classifier (SIGPIPE + executable-markdown cases), the uppercase-command gate, the empty-principal deny, and the untrusted-image hosting path against a bare pr-assets remote. * test(triage): pass the classifier fixture through a file, not argv The new docs-only classifier replay passed on macOS and failed on CI with `Cannot read properties of undefined (reading 'trim')`: its 60,001-entry fixture is ~889 KB and was passed as a single argv element. Linux caps one argument at MAX_ARG_STRLEN (128 KB), so the spawn failed with E2BIG and stdout was undefined; macOS has no per-argument limit and only a ~1 MB total, so the same call succeeded locally (verified both). Write the list to a temp file and pass the path. The harness now also asserts the spawn succeeded, so a future spawn failure reports itself instead of surfacing as a TypeError on undefined output. * fix(triage): make the /verify report match what the run actually produced Three publisher findings, all introduced by my own previous round: - an artifact download failure (the step is continue-on-error) let the full-report path run with no results: the headline read 'completed' and the scope paragraph claimed the A/B, the harnesses and the gates had run when nothing had been delivered. The download outcome is now an input, and its failure gets its own body saying the results could not be retrieved - the prepare-failure branch ignored the verdict the prepare step had just computed, so an install killed by a registry outage or OOM (classified infra-error) still told the author 'this is treated as a PR failure verdict rather than an infrastructure failure' — the exact opposite. It now branches on the verdict, and an infra-classified prepare failure is a weak body that cannot overwrite a real report - weak notices were being snapshotted as the follow-up round's previous-report.md: they lack the running marker, so 'newest non-running comment' selected them. Bodies that carry findings now mark themselves (qwen-triage:verify-substantive) and the snapshot selects on that marker. A/B on the real jq: report A then cancelled B now snapshots A (101), the old filter picked B (102) Tests: 4 more guards, all mutation-verified — the publisher is rendered for each outcome with a stubbed gh and the assertions read the body it would post, and the snapshot test runs the workflow's own jq program verbatim against a paginate-shaped fixture. * fix(triage): stop PR build output from masquerading as an infra failure Two review findings plus a test-helper hazard: - classify_failure grepped the prepare log for bare words like ENOSPC and ETIMEDOUT, but that log is written by PR-controlled code: a genuine build failure that merely prints 'expected ETIMEDOUT to equal ok' would be published as an infrastructure incident, telling the author to re-run something that fails identically. The patterns are now anchored to lines only npm's reporter or the kernel emits ('npm ERR! code E…', 'npm ERR! network …', kernel OOM, bare 'Killed'); a signal exit still needs no log evidence. Replayed 10 cells: four PR-authored logs quoting infra words stay 'fail', five real diagnostics and one signal exit are 'infra-error' - the two execution-time controls added last round — re-verifying the author's permission after the runner wait, and refusing a head that moved since authorization — had no tests. Both are now executed: the re-auth snippet against a stubbed permission API (write proceeds and pins head_oid; read skips with a publishable reason), and the pin step against a real git repo with a real merge commit (matching head proceeds, moved head exits non-zero) - add a stepIn(job, step) test helper. Several step names exist in both the tmux and verify jobs, and the unscoped step() returns the first match, so a verify-lane assertion silently tests the tmux copy — that has now bitten this suite three times, including in this commit. * docs(triage): teach verify-pr test-only PRs, differential oracles, gate liveness Fold techniques from the round-2 verification on QwenLM#7620 (an ANSI parser PR) that the skill had no equivalent for: - test-only PRs get their own method: a mutation A/B across TEST FILES (same mutants of the unmodified production file, only the test file swapped), reporting killed/total on both sides, requiring that no mutant regressed from killed to survived, checking that the killing assertion is the one the commit claims to have strengthened, and adjudicating every survivor as coverage gap or defect with independent evidence rather than by inspection - when the code emulates a known implementation, that implementation is the oracle: feed identical input to both and report disagreement counts per side, lift reference tables verbatim out of the shipped dependency, and build the corpus from bytes captured off a real producer alongside synthesized sweeps - prove a gate is live before citing it: plant a violation the linter must catch, confirm it is reported, remove it — a linter that matched no files exits 0 exactly like one that passed - attribute pre-existing failures by byte-identical failing file AND test names on both sides, with deltas, not just totals - when the base is far behind, verify the merge: trial-merge into current main, confirm it is conflict-free, and re-run the affected suite on the merged tree - round continuity gains its one legitimate shortcut: a production file proven byte-identical (sha256 quoted at both heads) carries prior evidence forward by construction * style(triage): reflow verify-pr skill to prettier's markdown wrapping The previous commit's added paragraphs were hand-wrapped and prettier --check flagged the file; the repo runs prettier over all of it. * test(triage): cover the disabled-runner-pool notice The kill-switch path had no test: a refactor could drop the notice and leave a /verify request acknowledged with 👀 but permanently unanswered, since the verify job refuses to start and publish-verify skips with it. Fold the step into the existing PR-guard loop (now scoped through stepIn, so it cannot match a same-named step in another job) and assert the parts that make the answer useful — the kill-switch and permission conditions, both languages, the alternative it points at, and the verify job's own exclusion of the disabled pool. All three mutations turn it red: removing the step, dropping its PR guard, or letting the verify job queue against the disabled pool. * fix(triage): repair a step-killing PIPESTATUS read and six forgeable controls Sixth review round, 12 findings. Several are regressions from my own two previous rounds; the first would have broken every single run. - `AGENT_STATUS=${PIPESTATUS[0]}` is itself a command and resets PIPESTATUS, so the next line's ${PIPESTATUS[1]} was unset and `set -u` aborted the step immediately after the agent finished — before artifact collection, the verdict, or anything else. Verified by replaying the exact structure: 'PIPESTATUS[1]: unbound variable'. Both elements are now snapshotted in one command - concurrency predicates were broader than the job conditions they guard, and GitHub evaluates concurrency BEFORE the job `if`: a /verify comment entered the triage job's shared per-PR group (where it could displace a pending /triage and then skip), and a /verify queued while the runner kill switch was on did the same to a real verification. Both predicates now match their job's runnable set exactly - an outward-resolving .git/hooks entry was only warned about and left in place, so the next root-owned git command would run it. It is now unlinked without traversing its target, a root-owned hooks directory is restored, and core.hooksPath is unset - the second .qwen pin re-derived HEAD^1 from git metadata after the workspace, including .git, had been handed to the build user. The base OID is now recorded while .git is still root-owned and the re-pin archives that content-addressed OID - classify_failure took both of its inputs from PR-controlled sources: a lifecycle script can exit with a signal status and can print any line the log patterns matched, turning its own deterministic breakage into 'infrastructure, please re-run' — which hid the failure and preserved a stale report. No infra verdict is derivable there, so the prepare step reports `fail` and lets the embedded log speak for itself - cleanups descended through PR-writable parents: `.qwen` itself can be a symlink, and the worktree sweep trusted git metadata with only a lexical prefix check. Symlinks are unlinked without traversal and worktree paths must canonicalize inside the workspace. Replayed all three escapes - skipped and docs-only outcomes upload no artifact, so the new download-failure branch pre-empted them and made their real reason unreachable; they are answered first now - a run that crashed before writing report.md still claimed the substantive marker, letting a headline overwrite the previous round's evidence. The marker now requires a report Skill: the byte-identical shortcut needs the whole input closure, not one file hash; the credential-free local path cannot call `gh` at all (fetch the metadata outside and mount it read-only); and the A/B base is `baseRefOid` in local mode, not `HEAD^1`. Tests: 7 new guards plus 4 updated to the new shapes, all mutation-verified (50/50). * fix(triage): answer dropped /verify requests and prove the proxy rejects Maintainer review (yiliang114), 7 items: - a third /verify while two runs are in flight is dropped by the concurrency group with no job and therefore no comment. The hosted authorize job now counts this workflow's other in-flight runs and says so; an API hiccup leaves the request alone rather than denying it - the proxy's bearer check had no executable test. It now starts the real proxy against a real upstream and issues real requests: no header and a wrong token are 401, this run's token is 200, and a route other than /chat/completions is 403 — with the health endpoint echoing the nonce - the 502 path forwarded the raw upstream error, which can name resolved hosts and TLS detail to PR code. It logs server-side and returns a generic failure - publish-verify inherited the 360-minute default; it downloads one artifact and posts one comment, so it is bounded at 10 - removing the log classifier last round left the comment block it replaced, which still said failures are classified from the exit status and the log. Deleted - that removal also left every install failure reported as the PR's fault, including a registry outage. There is exactly one signal here PR code cannot write — asking the registry ourselves, as root, with the container's resolver — so an install failure is downgraded to infra-error only when that probe fails. It proves reachability now rather than at failure time, so it can only ever downgrade, never confirm; a build failure has no equivalent and stays the tree's problem - the skill's local-invocation warning ran into the preceding sentence, which GFM renders as one paragraph Tests: 5 new guards, all mutation-verified (55/55). * fix(triage): resolve hooks hermetically and mirror symlink guards at job end Maintainer review round (doudouOUC), 6 findings. Two were Critical and both reproduced: - the hooks sweep resolved its path with the ambient git config in play. With a global core.hooksPath set — which the reviewer has and I do not, which is why my earlier replay showed a false pass — `git rev-parse --git-path hooks` returns that global path, the in-git-dir guard reads 'outside', and a planted `.git/hooks` symlink survives untouched. A/B: old code leaves the symlink under a global hooksPath, new code removes it in both environments and never touches the link target. Resolution now runs with GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM pointed at /dev/null - the END-of-job cleanup still used the bare `rm -rf .qwen/tmp` that the start-of-job cleaner was hardened against two rounds ago. The agent executes PR code between the two, so the end is no safer than the start: it now unlinks symlinks without descending and canonicalizes worktree paths inside the workspace before deleting Plus four suggestions, all valid: - the saturation notice counted this workflow's in-flight runs across every PR while the concurrency group is per-PR, so a run on another PR would trigger a warning about a queue that does not exist. It now matches on the PR title (the only per-PR handle an issue_comment run record carries) and stays silent when that cannot be resolved - the skill recommended `require.resolve` for the workspace-realpath check; these packages are ESM-only with import-only exports, so it throws ERR_PACKAGE_PATH_NOT_EXPORTED and reads like a missing module. Verified, and replaced with `readlink -f node_modules/@qwen-code/...` - the symlink-escape test inherited the developer's git config, which is what hid the first finding. It now runs with global/system config neutralized AND repeats the case with a global core.hooksPath planted - the publisher's build-phase arm was never rendered by any test (every case used 'install'), so a typo in that command name would have shipped. Now covered, along with an unrecognized phase Mutation-verified 4/4. The hooks guard needed a discriminating assertion: git's own `*.sample` files must survive the sweep, because the outward-path fallback removes the whole directory and would otherwise satisfy a bare 'planted hook is gone' check. * fix(triage): count only /verify runs for saturation, and test the PATCH arm Bot review round, 2 suggestions, both valid: - the saturation notice matched runs by PR title, which narrowed to this PR but not to /verify. /triage and /tmux live in their own concurrency groups, so two of those in flight would warn about a verify queue that is actually empty. It now also requires the run to have a job named 'verify' — the run record carries no command, but its job list does. Replayed: two non-verify runs stay silent, two verify runs warn - every publish fixture returned an empty comments listing, so the PATCH arm was never executed: a broken PATCH would have stranded the running status comment and posted a duplicate below it, with the suite green. The publisher now runs against a stubbed listing and the test asserts which verb went to which comment id — bot-owned live status is PATCHed in place, an absent comment posts fresh, and a marker comment owned by someone else is left alone and posted around Mutation-verified 3/3: counting every command, never PATCHing, and accepting foreign-owned markers each turn one test red. Two stub bugs found while writing these, both mine and both silent: ${*#pattern} applies per positional parameter rather than to the joined string (yielding a wrong run id), and the paginate fixture needs one array per page, not an array of pages. * fix(triage): fix the real silent drop and drop the step built on a wrong premise Review round 4. The blocker was mine twice over: the saturation notice I added last round had GitHub's concurrency semantics backwards, and the silent drop it claimed to cover was somewhere else entirely. - GitHub cancels the OLDER pending run in a group and admits the new one (confirmed against the workflow-syntax reference). My step told the person who had just typed /verify that their request might be dropped, when theirs is the one that runs — and said nothing to the person whose queued run actually died. This PR already had it right in publish-verify's own comment, so the file contradicted itself and the user-facing copy followed the wrong half. The step is removed rather than reworded: with the fix below there is nothing left for it to warn about, and it cost 2+N API calls on every /verify. - the actual drop: a verify job cancelled while still PENDING never reaches a runner, so its outputs block — where the "|| github.event.issue.number" fallback lived — is never evaluated. publish-verify then read an empty PR_NUMBER, hit its own guard and exited 0, making the cancelled branch unreachable in exactly the scenario that produces cancellations. The fallback now lives where the value is read. Reproduced both arms by executing the real step: with a number the cancelled notice posts, with an empty one it only warns. - same one-line class in publish-tmux, fixed alongside. Two copy defects from the classifier removal, both mis-attribution pointed the other way: - the infra-error body still named a signal/OOM kill and a full disk, none of which the current prepare step can produce — infra-error now requires npm ci to fail AND the registry probe to fail. It names that condition only, and offers a re-run instead of asserting it is the fix. - the code comment above it still described the deleted classifier. Also fixes the indentation break an earlier scripted edit left in the publish body builder, and replaces the saturation test with one that executes the cancelled path. Mutation-verified 2/2; the copy needed its own guard, since reverting the wording alone left every test green. * docs(triage): teach verify-pr survivor accounting and observability regressions Fold techniques from the re-verification on QwenLM#7709 that the skill had no equivalent for: - the mutation matrix must report the mutations that changed NOTHING, not only the ones that failed. Each survivor gets classified as an ordinary coverage gap or as dead code — a guard whose deletion leaves every test green is one of those two, and the difference is what the author needs. Survivors mirroring a pre-existing gap are labelled as such, and the set is framed as completeness reporting rather than merge conditions - the sharper case that report demonstrates: a test that passes for the WRONG REASON. If deleting the new guard leaves its own new test green, that test is pinned by an earlier early-return, not by the change, and asserts nothing about it. Name what actually pins it - and do not generalize from one dead guard to its siblings: the same report shows a clause that is unreachable on one path while being the only protection on another. Check each, report the contrast - observability regressions: when a change suppresses output, follow the value before calling the suppression correct. A bare catch on the path plus a field with no readers anywhere in the repo means the cause is now unobservable even in devtools — a real loss that no behavioural assertion can see - report structure gains a Corrections section: when an earlier round or bot comment described the code inaccurately, state the correct fact with evidence and label it as a correction to the description, not a request to change code. A wrong description left standing costs the next reader more than the original finding did * fix(triage): carry the /verify lane's hardening across to /tmux The /tmux job executes the same untrusted PR code, as the same user, on the same persistent self-hosted pool as the /verify lane that QwenLM#7710 hardened. Five of those controls had no equivalent here. Each was found on the verify side by reproducing an attack or a failure, not by reading the code, so the same evidence applies unchanged. - the model proxy bound a FIXED port (8787). PR lifecycle scripts run before it, so a detached child can squat that port: the real proxy then dies with EADDRINUSE while the health probe succeeds against the squatter, and the agent takes its chat completions. Now an ephemeral port published through a root-owned file, a per-run nonce the health endpoint must echo, and a liveness check on the PID we started. Replayed with 8787 occupied: the proxy comes up on an OS-chosen port and answers with the nonce. - nothing swept planted artifact directories. npm ci/build run the PR's lifecycle scripts, which can create tmp/<name>-tmux-<ts>/ holding a report.md and a transcript; the collector globs *-tmux-* and the publisher takes the first match, so a planted directory could supply the comment's contents. Swept after the last PR-controlled process and before the agent. - the global npm install ran with the workspace as cwd, where the PREVIOUS run's checked-out tree still sits. npm reads a cwd .npmrc, and a --registry flag does not override script-shell or hooks, so that config reached a root-privileged install. It now runs from RUNNER_TEMP. - the end-of-job cleanup globbed below .qwen/tmp. PR code ran in this workspace, so either .qwen or .qwen/tmp can be a symlink out of the tree — verified on the verify lane, where the glob deleted the link target's contents as root. Symlinks are unlinked without descending. - emit_block capped the raw log then escaped it. Escaping inflates every & < > by 4-5 bytes, so dense content can push the assembled body past GitHub's 65,536-character comment limit, 422 the post, and leave no comment at all. It now escapes first, caps the escaped bytes, and truncates on a character boundary via node — BSD iconv -c passes an incomplete trailing UTF-8 sequence through unchanged. Tests: a tmux-lane-parity suite, all six mutations verified (restoring the fixed port, dropping the sweep, moving the install back, dropping the symlink guard, reverting to a raw-side cap, and dropping the character-boundary truncation each turn one test red; a no-op control correctly changes nothing). One pre-existing assertion updated: it pinned emit_block's old inline-capture shape, and the guarantee it protects — a render failure is caught — is asserted in the new form. Also adds the regression guard for the publish-tmux PR_NUMBER fallback that landed in QwenLM#7710 without one: a job cancelled while pending never evaluates its outputs, so without the fallback the result comment silently does not post. * fix(triage): address review — symlink guard, artifact strip, bearer auth (QwenLM#7753) * fix(triage): address R2 review — proxy parity, bearer wire tests, process kill (QwenLM#7753) * fix(triage): address R3 review — publisher parity, dedup ownership, cap budget tests (QwenLM#7753) * fix(triage): address R4 review — drop redundant tmux-lane .mjs guards (QwenLM#7753) * fix(triage): address R5 review — tmp symlink sweep guard, proxy timer clear (QwenLM#7753) * fix(triage): address R6 review — hoist proxy timer out of try, dead-upstream 502 tests (QwenLM#7753) * fix(triage): address R7 review — make proxy watchdog idle, end stalled response (QwenLM#7753) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
|
Released in v0.21.1. |
5 participants
What this PR does
parseAnsirenders shell tool output in the web shell. It split each SGR escape on;and treated every parameter as a standalone code — but the arguments of38/48/58(extended foreground/background/underline color) are not codes. This PR consumes those arguments instead of re-reading them, and resolves the foreground to a hex color.Why it's needed
Feeding the color arguments back into the code loop corrupts the output — and not only for the color itself, but for styling that was already correct:
\e[38;5;2m(256-color green)dim: true, no color#48bb78\e[1m\e[38;2;0;128;255m(bold + truecolor)bold: false, no colorbold: true,#0080ff\e[1m\e[48;5;22m(bold + 256-color bg)bold: falsebold: trueThree distinct failures, all from the same root cause:
38;5;2→ the index2is read as SGR2, setting dim and producing no color.0hits thecode === 0branch and resets color, bold and dim mid-line.48;5;22feeds22to the reset-intensity branch, so setting a background silently un-bolds the text.parseAnsifeedsToolGroup.tsx, which renders the output of shell tools, so this affects any 256-color CLI — which is most of them.The fix
When a parameter is
38,48or58, read its argument form (5;<index>or2;<r>;<g>;<b>) and advance past it. The foreground (38) resolves to hex:ANSI_COLORSpalette, so38;5;2and32render as the same green.0, 95, 135, 175, 215, 255).Background (
48) and underline (58) color are parsed but not rendered —Segmenthas no field for them — so their arguments still cannot leak into the code stream. Malformed sequences (out-of-range index, truncated argument list, unknown form) yield no color rather than a bogus one and never fall through to the plain-code branches.Reviewer Test Plan
How to verify
cd packages/web-shell && npx vitest run --config vitest.config.ts utils/ansi.test.ts→ 9/9.ansi.test.tsis new — there was no test for this module. Five cases assert the fixed behavior and pass only with this change; four more pin the pre-existing basic-color / bold / dim / reset behavior so the change can't regress it.ansi.tsfails exactly the five new cases:does not read 256-color arguments as SGR codesdoes not let truecolor channels reset the stylekeeps background and underline color out of the code streamdrops malformed extended-color sequences without corrupting stateleaves an already-set color alone when the sequence is malformed208 → (5,2,0) → #ff8700, corners16 → #000000and231 → #ffffff, grayscale232 → #080808and255 → #eeeeee.Evidence (Before & After)
\e[38;5;208m➜\e[0m) renders the glyph uncolored and, if it was inside a bold run, un-bolded.#ff8700) with bold preserved.Tested on
macOS:
ansi.test.ts9/9. In the fullpackages/web-shellrun (1160 tests) this branch is 1151 passed / 9 failed — byte-identical failures to a clean checkout of the same tree, which is 1150 passed / 9 failed; the only delta is the one test this change adds. Those 9 pre-existing failures are allbuild artifact — package boundarycases that need a built dist, and the 47 test files that fail to load do so on unresolved workspace modules (@qwen-code/sdk/daemon,@qwen-code/webui/daemon-react-sdk) — neither is touched here.tsc --noEmitreports zero errors inansi.ts/ansi.test.ts; eslint and prettier clean. Pure string parsing with no platform-dependent behavior, so no manual QA is required; CI covers Windows/Linux.Environment (optional)
Node v24;
@qwen-code/web-shellworkspace; vitest 3.2.Risk & Scope
32or38;5;2.Segmenthas no field for them. This PR only stops their arguments from corrupting the code stream; surfacing them is a separate feature. Cursor-movement and other non-SGR escapes are likewise untouched.Linked Issues
None — found while reading
parseAnsiagainst the SGR extended-color grammar.中文说明
本 PR 的作用
parseAnsi负责在 web shell 中渲染 shell 工具输出。它按;拆分每个 SGR 转义序列并把每个参数都当作独立的 code——但38/48/58（扩展前景/背景/下划线颜色）后面的参数并不是 code。本 PR 改为消费这些参数，并将前景色解析为 hex。为什么需要
把颜色参数重新塞回 code 循环会破坏输出——而且不仅是颜色本身，连原本正确的样式也会被破坏：
\e[38;5;2m（256 色绿）dim: true，无颜色#48bb78\e[1m\e[38;2;0;128;255m（粗体 + 真彩）bold: false，无颜色bold: true，#0080ff\e[1m\e[48;5;22m（粗体 + 256 色背景）bold: falsebold: true三种不同的失效，根因相同：
38;5;2→ 索引2被当作 SGR2，从而设置 dim 且无颜色。0命中code === 0分支，在行中途重置颜色、bold 与 dim。48;5;22这样的背景会把22送入「重置粗细」分支，于是设置背景会悄悄取消粗体。parseAnsi供给ToolGroup.tsx，后者渲染 shell 工具的输出，因此凡是 256 色 CLI（也就是绝大多数）都会受影响。修复方式
当参数为
38、48或58时，读取其参数形式（5;<索引>或2;<r>;<g>;<b>）并跳过。前景（38）解析为 hex：ANSI_COLORS调色板，因此38;5;2与32渲染为同一种绿。0, 95, 135, 175, 215, 255）。背景（
48）与下划线（58）颜色被解析但不渲染——Segment没有相应字段——因此它们的参数仍不会泄漏进 code 流。畸形序列（越界索引、参数截断、未知形式）返回无颜色而非错误颜色，且绝不会落到普通 code 分支。复核测试计划
如何验证
cd packages/web-shell && npx vitest run --config vitest.config.ts utils/ansi.test.ts→ 9/9 通过。ansi.test.ts为新增——此前该模块没有测试。五个用例断言修复后的行为，仅在本改动下通过；另有四个固定既有的基础色 / bold / dim / reset 行为，防止回归。ansi.ts时，恰好这五个新用例失败：does not read 256-color arguments as SGR codesdoes not let truecolor channels reset the stylekeeps background and underline color out of the code streamdrops malformed extended-color sequences without corrupting stateleaves an already-set color alone when the sequence is malformed208 → (5,2,0) → #ff8700，角点16 → #000000、231 → #ffffff，灰阶232 → #080808、255 → #eeeeee。证据（修复前后对比）
\e[38;5;208m➜\e[0m）的工具，其字形无颜色，且若处于粗体段内还会被取消粗体。#ff8700）并保留粗体。测试环境
macOS：
ansi.test.ts9/9。在packages/web-shell完整运行（1160 个测试）中，本分支为 1151 通过 / 9 失败——与同一棵树的干净检出（1150 通过 / 9 失败）失败项完全一致，唯一差异是本改动新增的那个测试。这 9 个既有失败全部是需要已构建产物的build artifact — package boundary用例；另有 47 个测试文件因无法解析工作区模块（@qwen-code/sdk/daemon、@qwen-code/webui/daemon-react-sdk）而加载失败——两者本 PR 均未触及。tsc --noEmit对ansi.ts/ansi.test.ts零报错；eslint 与 prettier 干净。纯字符串解析、无平台相关行为，故无需人工 QA；Windows/Linux 由 CI 覆盖。运行环境（可选）
Node v24；
@qwen-code/web-shell工作区；vitest 3.2。风险与影响范围
32还是38;5;2都保持不变。Segment没有相应字段。本 PR 只是阻止其参数破坏 code 流；将其呈现出来是另一项功能。光标移动等非 SGR 转义同样未触及。关联 Issue
无——在对照 SGR 扩展色文法阅读
parseAnsi时发现。