fix(core): deflake the shell-registry fixtures, and share the display-strip helper - #8795
Conversation
The retention-cap tests time out at 15s on shared CI runners while passing everywhere else, and it is not load. Every settle writes a status sidecar next to the entry's outputPath, and the fixture's default pointed all of them at one fixed path: `/tmp/s1.output`, so `/tmp/s1.status` on every machine that ever ran this suite. On a sticky-bit /tmp the file can already belong to another user, and then the atomic write's rename answers EPERM and retries with a blocking exponential backoff — 50, 100, 200ms — before giving up. That is ~350ms per settle, measured at 362ms; the cap tests settle 34 entries each, which is 12.3s of a 15s budget before the assertions even run. A/B on one machine with that exact shape (an unrenameable file planted at the shared path): the old fixture reports `Test timed out in 15000ms` three times over, the new one passes 57/57 in 778ms. The fix is the fixture, not the retry policy: the backoff protects real writes against transient EPERM and should stay. Entries now get output paths under a per-test temp directory keyed by shellId, which the existing afterEach already cleans, so no two entries — and no two jobs — share a sidecar. The two tests that hand-wrote `/tmp` paths to exercise `&` and control characters in a basename keep exactly that subject and move their directory too. Seen on feat/daemon-git-worktree-guard and feat/review-capture-tui, four tests apiece, same describe block, with the file taking 120s.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @wenshao!
The description doesn't follow the PR template — all of the required sections are missing:
## What this PR does## Why it's needed## Reviewer Test Plan— with### How to verify,### Evidence (Before & After), and### Tested on## Risk & Scope## Linked Issues
(The Chinese translation inside <details><summary>中文说明</summary> is already there ✓.)
Could you update the PR body to fill in the template? Everything you wrote maps cleanly onto it: "The flake" + "Root cause" → Why it's needed, "The fix" → What this PR does, and the A/B table is exactly what Evidence (Before & After) asks for. One section matters especially here: Tested on — the flake's root cause is a shared /tmp sidecar owned by another account, which is a self-hosted-runner condition, so saying which environment the 57/57 green run happened in (and whether it included a runner where the collision can actually occur) is the part a reviewer can't reconstruct from the diff.
中文说明
感谢提交 PR，@wenshao！
PR 描述没有使用 PR 模板，所有必填章节都缺失：
## What this PR does## Why it's needed## Reviewer Test Plan——包含### How to verify、### Evidence (Before & After)、### Tested on## Risk & Scope## Linked Issues
（<details><summary>中文说明</summary> 里的中文翻译已经有了 ✓）
请更新 PR 描述，填写模板。你已经写好的内容可以直接对应填进去："The flake" + "Root cause" → Why it's needed，"The fix" → What this PR does，A/B 对比表正是 Evidence (Before & After) 要求的内容。其中 Tested on 部分尤其重要：这个 flake 的根因是共享 /tmp 上属于其他账户的状态边车文件，这是自托管 runner 才会出现的条件——所以请说明 57/57 全绿的运行是在哪个环境完成的（是否包含真正会发生冲突的 runner），这部分是 reviewer 无法从 diff 中还原的。
— Qwen Code · qwen3.8-max
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 27 passed · 0 failed · 27 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：27 通过 · 0 失败 · 27 总计 Verification reportPR 8795 — test(core): give shell-registry fixtures their own output pathsVerdict: 中文摘要
Central claim and A/BCentral claim: the retention-cap timeouts on shared CI runners are caused by every fixture entry sharing This container cannot create the genuine trigger (uid 1000,
Witnesses: Mechanism measurement (
Corrections
Findings (all pre-existing, non-blocking)
Mutation matrix (test-only PR coverage equivalence)Six single-point mutants of the unchanged production file, each run against the old (base) and new (head) test file; unmutated controls are the base-clean / head-clean cells (57/57 both).
No mutant regressed killed→survived: the fixture rewrite loses no mutation-detectable coverage. M5's killer being the relocated escaping test proves the moved basename tests still pin their subject after the path move. Reviewer Test Plan walkthrough
Not covered
MethodologyEnvironment: Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Re-run — the scope changed substantially since the last pass (which reviewed Template looks good ✓ Problem: observed, not theoretical. The original flake (two unrelated branches timing out on the shared Direction: aligned — Size: core paths touched ( Approach: minimal. One behaviour-preserving extraction ( Risk: no elevated risk signals — none of the changed production files matches a high-risk path pattern. Moving on to code review. 🔍 中文说明重新运行 —— 自上一轮（审查 模板完整 ✓ 问题：已观测到，不是理论问题。最初的 flake（两个互不相关的分支在共享的 方向：对齐 —— 规模：触及核心路径（ 方案：最小化。一处行为保持的抽取（ 风险：无升级风险信号 —— 改动的生产文件均不匹配高风险路径模式。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe branch moved since the last pass ( Independent proposal first (before reading the new diff): with the deflake already in main, what this PR's history says still needs doing is (a) give the shell-registry tail path the same bidi stripping the display helper applies — without losing the tail's newline preservation, so a shared predicate, not a shared function — and (b) close the one render site the probe work surfaced, the monitor's streaming What I verified against the diff and
No blockers found. Test evidenceThe PR's own CI on the reviewed commit
The macOS/Windows/Integration skips are by design — The behavioural claim here — bidi stripped from Real-scenario testing: N/A — model-envelope sanitizers with no user-visible terminal surface to drive. 中文说明代码审查自上一轮（ 独立方案（读新 diff 之前）：deflake 既然已在 main，这个 PR 的历史中尚需完成的是：(a) 让 shell-registry 的 tail 路径获得与展示 helper 相同的 bidi 剥离 —— 但不能丢掉 tail 的换行保留，所以用共享谓词而非共享函数；(b) 关闭探针工作暴露的那一处渲染点 —— monitor 的流式 对照 diff 与
未发现阻塞问题。 测试证据被审 commit macOS/Windows/Integration 的 skip 是设计如此 —— 此处的行为声明 —— bidi 从 真实场景测试：N/A —— 模型信封 sanitizer，没有可在终端驱动的用户可见界面。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean review across every stage; the only nit is cosmetic: the title and body still describe the pre-extraction scope (the fixture deflake merged to main as #8813), so both should be updated at squash-merge to match what the remaining diff actually does. Stepping back: this PR is better for having been narrowed mid-flight. What remains is 27 production lines of envelope hardening with a clear invariant behind it — Approving, pinned to the reviewed commit. ✅ 中文说明置信度：4/5 —— 每个阶段都干净；唯一的意见是外观层面的：标题与正文仍在描述拆分前的范围（fixture deflake 已经 #8813 合入 main），squash 合并时应同步更新二者，使其与剩余 diff 的实际内容一致。 退一步看：这个 PR 因中途收窄而变得更好。剩下的是 27 行生产层面的信封加固，背后有一条清晰的不变量 —— 批准，锚定在被审 commit。✅ — Qwen Code · qwen3.8-max 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. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the modified test file's suite ran locally on Linux only. Test Plan (not a blocker): 57 passed — this review observed 19546, 1124, 18603, 1470, 481, 2941, 454 passed.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the modified test file's suite ran locally on Linux only。 Test Plan（非阻断）：57 passed — this review observed 19546, 1124, 18603, 1470, 481, 2941, 454 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
The rebuilt <output-file> expectations computed their value by hand — a single-occurrence String.replace, and none at all in the control-character test — while production emits escapeXml(stripDisplayControlChars(path)), which escapes all five XML metacharacters globally. So the expectations were only correct while os.tmpdir() itself contained none of & < > " '. That is the same "passes here, fails there" dependence this PR exists to remove, in a new form and introduced by its own first commit. Probe: under TMPDIR=/tmp/qcprobe/o&brien and .../o'brien, three tests fail with production behaving correctly. Routing every expectation through escapeXml makes all three environments agree — 57/57 under a plain TMPDIR and under both poisoned ones. Includes the pre-existing assertion in 'emits one task-notification when a shell completes', which carried the same latent dependence and now goes through the same pipeline.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): 57 passed — this review observed 19546, 1124, 18603, 1470, 481, 2941, 454 passed.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。 未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。 Test Plan（非阻断）：57 passed — this review observed 19546, 1124, 18603, 1470, 481, 2941, 454 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
Third time on the same class, so this fixes the class rather than the case. Production renders <output-file> as escapeXml(stripDisplayControlChars(p)). My first pass hand-wrote the escape (single-occurrence, so only the first `&`), the second added escapeXml but not the strip, and each was correct only while os.tmpdir() happened to hold nothing the pipeline transforms — a TMPDIR under a control character, a C1 byte, or a second \x03 failed the assertion with production behaving correctly. The expectations now call the real functions in the real order through one helper, so they cannot drift from production at all. Verified 57/57 under a clean TMPDIR, one containing \x01, and one containing `&`; dropping either half of the composition turns three tests red under the matching hostile TMPDIR. (Noted while here, not changed: the registry keeps a private copy of stripDisplayControlChars byte-identical to the exported one in terminalSafe.ts. Out of scope for a deflake, but worth a look.)
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued in workflow run. |
The registry kept a private stripDisplayControlChars that predates #4358: that change added the bidi override/isolate ranges (U+202A-U+202E, U+2066-U+2069) to the shared terminalSafe version and migrated monitorRegistry onto it, but left this copy behind. The copies have diverged ever since, even though terminalSafe's contract says both background notification surfaces must apply the same defense. The divergence bit this PR: the deflake's rebuilt <output-file> expectations compose escapeXml(stripDisplayControlChars(...)) from the shared utils — "exactly what production composes" was true for every input except bidi codepoints, one more pass-here-fail-there dependence in the class this PR exists to remove (and the earlier "byte-identical copies" note was wrong: the shared one strips two more ranges). Import the shared function and delete the stale copy. Notification rendering (cwd, command label, result, output-file) now strips bidi override/isolate characters — the documented Trojan-Source defense the shared function exists for — and the test's composition can no longer drift from production on any input. Net -20 lines.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： This round: verified the two inline suggestions were already fixed at HEAD, closed the last gap of the same class found while verifying them, and prepared the template-conformant PR description the triage review asked for. 1. PR description does not follow the template (CHANGES_REQUESTED, triage review) — addressed as far as this flow can: a fully template-conformant body was prepared ( 2. [Suggestion] 3. [Suggestion] expectations apply 4. Prior-round observations left as declared out of scope — the VerificationCommands actually run this round (Linux runner):
Conflict: none ( 中文说明本轮工作：核实两条行内建议已在 HEAD 修复；核实过程中发现同类问题的最后一处缺口并已 closing；同时按 triage 评审的要求准备好了符合模板的 PR 描述。 1. PR 描述未遵循模板（CHANGES_REQUESTED，triage 评审） —— 在本流程能力范围内已处理：完整的模板化描述已准备好（见运行产物中的 2. [建议] 3. [建议] 期望值应用了 4. 前几轮已声明超出范围的观察项 —— 验证本轮实际执行的命令（Linux runner）：
冲突：无（ Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. Test Plan (not a blocker): 57 passed — this review observed 1124, 18603, 1470, 481, 2941, 454 passed.
中文说明
已审查——无阻断问题。 建议见行内评论。 Test Plan（非阻断）：57 passed — this review observed 1124, 18603, 1470, 481, 2941, 454 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
The composed oracle this PR introduced cannot discriminate a mutation in
escapeXml — it computes the expectation with the same function under test,
so both sides move together by construction. The literal case in
xml.test.ts is where that property has to live, and it carried exactly one
`&`: the other four metacharacters appeared twice and were pinned globally,
`&` was not.
Measured: mutating `.replace(/&/g, '&')` to `.replace('&', '&')`
ships green across the whole package — 19,546 tests passed. A real path
with two of them (a TMPDIR under `o&brien` with a basename like
`out&err.log`, the very case this PR's own history is about) would then
render a raw `&` into a model-facing XML envelope with nothing failing.
The case now carries two `&`, and the mutant turns it red.
|
@qwen-code /review |
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8795Feedback triage[Suggestion] R6-2 (inline Change (test-only, one file):
Mutation-killed: with the strengthened fixture, the same C0/C1-only mutant turns exactly this test red (1 failed | 58 passed); the restored correct code is green (59/59). Review-body note — skipped macOS/Windows CI test runs: noted, no code action. Informational; the touched suites ran on Linux. Review-body note — Test Plan count mismatch ( Conflict
DispositionOne commit: Verification
中文说明Autofix 审查轮次 — PR #8795反馈分诊[建议] R6-2（行内 改动（纯测试，单文件）：
变异已击杀：使用加固后的 fixture，同一个 C0/C1-only 变异体恰好使该测试变红（1 failed | 58 passed）；恢复正确代码后重新运行全绿（59/59）。 审查正文说明 — macOS/Windows CI 测试被跳过：已知悉，无需代码动作。 信息性说明；受影响套件已在 Linux 上运行。 审查正文说明 — Test Plan 计数不一致（ 冲突
处置单个提交： 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): 57 passed — this review observed 19548, 1124, 18604, 1470, 481, 2941, 454 passed.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan（非阻断）：57 passed — this review observed 19548, 1124, 18604, 1470, 481, 2941, 454 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
…ar deflake #8813 landed the same shared /tmp/s1.status deflake this branch started, plus the shared display sanitizer reuse. Keep main's per-entry fixture mechanism and expectedOutputFileElement helper; preserve this branch's unique coverage on top: bidi stripping in the output tail (all nine codepoints), bidi pins at the command/cwd/output-file render sites, and the <result> render-site pin in the failure test.
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8795Feedback triageThe only new feedback since the last evaluation is an automated-reviewer COMMENTED review (round 7, zero findings). It is informational, not actionable:
No inline comments, no issue-level comments, no failed checks, and no still-red checks were reported. There were no findings to implement or decline this round. Work done: base-conflict resolution (
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI; the touched suites ran locally on Linux only. Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the touched suites ran locally on Linux only. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): 57 passed — this review observed 19559, 1172, 18740, 1501, 481, 2983, 459 passed.
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI; the touched suites ran locally on Linux only。 未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the touched suites ran locally on Linux only。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan（非阻断）：57 passed — this review observed 19559, 1172, 18740, 1501, 481, 2983, 459 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Review feedback round — PR #8795One Suggestion-level inline finding from the automated reviewer (round 8). No failed checks, no conflict ( Feedback points and decisions
Changes
Conflict notesNone — no conflicts reported and no merge performed. Verification
Commit: 中文说明审查反馈轮次 — PR #8795来自自动审查者（第 8 轮）的 1 条建议级（Suggestion）行内发现。无失败检查，无冲突（ 反馈点与决定
改动内容
冲突说明无——未报告冲突，也未执行任何合并。 验证
提交： Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): PR #8795 deflakes the shell-registry test fixtures by giv...: (none — all planned checks completed).. Test Plan (not a blocker): 57 passed — this review observed 19560, 1172, 18740, 1501, 481, 2983, 459 passed.
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：PR #8795 deflakes the shell-registry test fixtures by giv...：(none — all planned checks completed).。 Test Plan（非阻断）：57 passed — this review observed 19560, 1172, 18740, 1501, 481, 2983, 459 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action takenThis round found no actionable feedback for PR #8795:
Deferred non-Critical feedback (intentionally untouched)Critical-only mode is active after five change-producing rounds. The workflow excluded the following non-Critical items from this round's actionable scope; they remain open for human follow-up. Per the audit-record rule, no code changes were made, no threads were resolved, and no comment replies were written for them:
VerificationNo code was changed this round, so no build/typecheck/lint/test commands were run. The working tree is clean and the branch head remains at 中文说明Autofix 评审轮次 — 未采取任何行动本轮未发现 PR #8795 有任何可处理的反馈：
延后的非 Critical 反馈（有意不处理）在完成 5 个产生改动的轮次后，已进入仅处理 Critical 的模式。工作流已将以下非 Critical 条目排除在本轮可处理范围之外，它们保持开放状态留待人工跟进。根据审计记录规则，本轮未对它们做任何代码改动、未解决任何讨论串、也未撰写任何评论回复：
验证（Verification）本轮未修改任何代码，因此未运行 build / typecheck / lint / 测试命令。工作树干净，分支 HEAD 仍为 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 72 passed · 0 failed · 72 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：72 通过 · 0 失败 · 72 总计 Verification reportSandboxed verification: ✅ passed — merge-ready (agent verdict) — follow-up round 3 at head Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, mutation matrix, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 72 passed · 0 failed · 72 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）本轮为第 3 轮跟进验证：上轮验证 head
Previous-round finding status (follow-up round)All carried measurements re-executed at the new head
Central claim and A/BCentral claim (this round): the monitor's streaming
A/B totals: 43 scripted assertions (23 leak-control + 9 head + 9 dist + 2 identity diffs), all passed. Secondary claim A — the production changes are exactly the documented bidi hardening:
Secondary claim B — the new tests pin what they say they pin: see the mutation matrix below. Mutation matrix and vacuityWitness:
M3–M6 are exactly the one-character bound typos the commit history says once shipped green under one-codepoint-per-range fixtures; the all-nine fixtures (plus the pre-existing terminalSafe boundary tests from #4358) kill all four. Positive controls: M1–M8 each turn the suites red with the quoted assertions; an unmutated run is green. Every mutant was hash-verified applied and restored (tree clean after each). Methodology note (disclosed): mid-round, a restore-path bug in the matrix driver left M1 applied for one unintended baseline run — which correctly went red on exactly the new monitor test, then was restored and re-run green. The accidental run is an extra positive control, not a source of the numbers above. FindingsF1 — low: PR title/body no longer describe the effective diff. The snapshot title is still Carried nits (re-measured, unchanged, pre-existing):
Observations (informational, out of scope): ~14 inline copies of the bidi ranges remain elsewhere in the repo ( Targeted gates
Not covered
MethodologyEnvironment: Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the exact head ab245bbae8. No blocking findings; approving.
I verified the full current diff and its downstream consumers. The output-tail path now removes the same nine bidi controls as the single-line display sanitizer while still preserving TAB/CR/LF; the shared predicate preserves the existing stripDisplayControlChars range semantics; and the monitor streaming <result> now uses the already-sanitized event line before XML escaping. The only in-repo production caller of MonitorRegistry.emitEvent is the monitor tool, and the resulting envelopes flow unchanged through the interactive, non-interactive, ACP, and agent-owned notification queues. No registry lifecycle, persistence, retry, truncation, or task-state behavior changes.
I also rechecked the unresolved review threads against this head rather than relying on their GitHub state: the output-tail test now covers all nine codepoints; command, cwd, output-file, and failed-result sites are pinned; and the monitor streaming leak is fixed. The remaining open item (discussion_r3747129026) is a Suggestion-level test-coverage hardening for direct C0/C1 calls. The sole production caller already strips C0/C1, and the repository is in Critical-only mode after five change-producing rounds, so I am explicitly deferring that non-blocking suggestion rather than widening this PR again.
Verification on ab245bbae8: 220 relevant core tests passed (background shell registry, monitor registry/tool, terminal safety, XML); core typecheck passed; core build passed; Prettier passed. GitHub's required checks are green, with no failures.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： AutoFix review — no changes neededThis round reviewed the latest feedback on PR #8795 and made no changes. No new commits were created; the branch head remains at Why nothing was changed:
Since no code was modified, no build/typecheck/lint/test verification commands were required this round. 中文说明AutoFix 评审 —— 无需改动本轮审查了 PR #8795 的最新反馈，未做任何改动。没有创建新的提交；分支头仍为 未改动的原因：
由于本轮未修改任何代码，不需要运行 build/typecheck/lint/测试等验证命令。 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approving at head ab245bb. The merge with #8813 (8430a5f) is a verified clean union against both parents: #8813's per-entry makeTempDir fixture, tmpFiles cleanup, expectedOutputFileElement helper, and uniqueness test survive byte-for-byte, the branch's bidi coverage is fully preserved with no leftover entryDir/renderedPath/s1.status code and no duplicated helper, and the control-char fixture was extended rather than weakened (out\x03\u202e.log asserting both strip). The new work since my last check is sound: 5e441d2 pins the fourth render site (failed shell already rendered entry.error through the shared strip helper at 42e1665), and ab245bb closes the monitor streaming- bidi hole at its riskiest path with escapeXml(safeEventLine) plus a nine-codepoint pin on both model and display text. The single-home bidi range set holds (terminalSafe.ts, all nine codepoints pinned at the render sites and range edges). CI green, 43/43 with zero failures.
One non-blocking note: the ci-bot's 06:30 suggestion postdates head — the new monitor streaming- test pins only the bidi half, and model-facing C0/C1 removal on that path is unpinned (production is masked because sanitizeMonitorLine pre-strips C0/C1, but emitEvent is public). Worth a small follow-up test. Nothing blocks merge.
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. ✅
|
Released in v0.21.9. |
|
Consolidated response to the nine open threads (rounds 3–6). Eight are closed by commits already on this branch; one was genuinely open and is fixed in Closed by Closed by Over-stripping is pinned too: the tail assertions name the text after each stripped run, so a Open until now, fixed in 中文说明九条线程的合并答复（第 3–6 轮）：八条已由本分支上的提交关闭，一条确实未解决，已在
此前真正未解决的一条已在 |
A verifier ruling on "the panel clips at 80 columns" reads the layout code and imagines a terminal; measured on this repo, the imagining is where rendering verdicts go wrong. `qwen review capture-tui` makes the terminal real and the evidence a file: it drives a command in a throwaway tmux and hands back what actually rendered — `.ans` pane text always, a `.png` when freeze is available, and a manifest saying which rung it reached and why. Isolation is structural, not disciplined. Every tmux call is scoped to a per-run private server (`-L`), so a capture cannot see — let alone resize or kill — the user's own sessions; the measured failure mode of desktop-automation verification was exactly "drives the user's own windows". `kill-server` reaps everything the capture started, a signal handler reaps before re-raising, and the pane holder carries its own watchdog so an unreaped server still self-terminates. Evidence is never invented. The manifest records the rung and the reason it stopped there, so a verdict can say what it stands on. A png is credited only when this run's render demonstrably produced it; artifact paths are stamped by identity, so nothing this run did not write is ever deleted or claimed; and a path already occupied by a foreign file is refused up front rather than overwritten. The trailing-space fidelity that column claims depend on follows the tmux version — `-N` where it means real spaces, `-T` where padding must be trimmed, neither where `-N` would fabricate them. Refusals are a contract: exit 3, a reason on stderr and the same reason as JSON on stdout, and no stale evidence left beside it. An environment that cannot produce evidence says so instead of producing something weaker and calling it the same thing. Recreated from #8388 to collapse 373 commits and rebase onto current main. The CI tooling step and its workflow pin are #8792; the shell-registry deflake is #8795.
5 participants
What this PR does
Gives every shell-registry test fixture its own output path under a per-test temporary directory, keyed by
shellId, instead of the one fixed/tmp/s1.outputthat all of them shared. The two tests that hand-wrote/tmppaths to exercise&and control characters in a basename keep exactly that subject and move their directory too.It also carries one production change, which is not behaviour-preserving and is described here because an earlier version of this body wrongly said nothing in production changed: the registry now renders notifications through the shared
stripDisplayControlCharsinterminalSafe.tsinstead of its own private copy. The shared helper strips two ranges the private one did not — U+202A–202E and U+2066–2069 — so bidi overrides are removed from<cwd>,<result>,<output-file>and the two command-display paths.<output-tail>renders through a second helper — it must keep newlines, which the display helper strips — and that one now removes the same two ranges, because the tail is the largest attacker-controllable field in the envelope (up to 8 KiB of a background shell's own output). Measured:/tmp/a<RLO>evil<PDI>/out.logrenders unchanged through the old copy and as/tmp/aevil/out.logthrough the shared one. Those characters reorder how a path displays without changing its bytes, so leaving them in a model-facing envelope is a spoofing surface — the stronger behaviour is the one worth keeping, and a registry-level test now pins it.The atomic write's retry policy is untouched: it is the safety mechanism this bug merely exposed.
Why it's needed
backgroundShellRegistry.test.ts→terminal-entry retention captimes out on shared CI runners while passing everywhere else — four tests,Test timed out in 15000mseach, with the whole file taking ~120s. It showed up on two unrelated branches on the same day:feat/daemon-git-worktree-guard(job 93206694115) andfeat/review-capture-tui(job 93208639663).The tests read as pure in-memory loops over a cap of 32, so "the runner was starved" is the natural first guess. It is wrong. Every settle (
complete/fail/cancel) writes a status sidecar next to the entry'soutputPath, and the fixture's default pointed all of them at/tmp/s1.output— so/tmp/s1.status, the same absolute path on every machine that has ever run this suite./tmpis sticky-bit: when that file already belongs to another user (another job, another account, an earlier run on a self-hosted runner), the atomic write'srenameanswersEPERM, andrenameWithRetrySyncretries with a blocking exponential backoff — 50 + 100 + 200ms — before giving up.writeStatusFilethen swallows the error and logs it, so nothing looks wrong except the clock.Measured against the real
atomicWriteFileSyncwith the exact options the registry passes: 362ms per settle. The cap tests settle 34 entries each, which is 12.3s of a 15s budget before a single assertion runs. That accounts for every observed symptom — why only that describe block (only it loops 34 times), why the file inflates to 120s, why it never reproduces on a developer machine, and why it strikes branches that have nothing to do with shells.Reviewer Test Plan
How to verify
Run the suite:
npx vitest run --root packages/core src/services/backgroundShellRegistry.test.ts→ 57 passed in ~0.7s.To see the flake itself, recreate the shared-runner condition on one machine — a file at the shared path that this process cannot rename over, which is what "owned by another user under a sticky-bit /tmp" means to
rename(2):On
mainthat reportsTest timed out in 15000ms; on this branch it passes, because no fixture writes to that path any more. A reviewer can also confirm the cost directly by callingatomicWriteFileSync(target, …, { flush: false, mode: 0o600, forceMode: true, noFollow: true })34 times against such a target and timing it.Evidence (Before & After)
Same machine, same poisoned path, only the fixture differs:
/tmp/s1.outputTest timed out in 15000ms×3Supporting measurement: 34 status writes against an unrenameable target took 12304ms, i.e. 362ms each, against a 15000ms test budget.
Tested on
To be precise about what that means, since it is the part a reviewer cannot reconstruct from the diff: the 57/57 green run and the A/B above were done on macOS, with the collision simulated (
chflags uchgmakesrenameanswerEPERMexactly as a foreign-owned file under a sticky-bit/tmpdoes). I did not have a self-hosted runner with a genuinely foreign-owned/tmp/s1.statusto run against — the CI evidence for the real condition is the two failing jobs linked above, and the fix removes the shared path entirely rather than trying to survive it, so the failing condition can no longer be reached from this suite on any platform.Environment (optional)
Unit tests only.
Risk & Scope
renameWithRetrySyncprotects real writes against transientEPERMand is left alone deliberately; this PR removes the test's dependence on a shared path rather than weakening that protection. Whether other suites in the repo write to fixed/tmppaths was not audited here.Linked Issues
None — found while investigating repeated CI failures on #8388.
中文说明
这个 PR 做了什么
让 shell registry 的每个测试 fixture 使用各自的输出路径——放在 per-test 临时目录下、按
shellId命名——取代此前所有 entry 共用的固定路径/tmp/s1.output。两个手写/tmp路径以考察 basename 中&与控制字符的用例，保留原有考察点，只把目录一并挪走。本 PR 还包含一处生产改动，它并非行为等价——之所以在此说明，是因为本描述的早期版本错误地写成「生产代码不变」：registry 现在改用
terminalSafe.ts中共享的stripDisplayControlChars，取代其自有的私有副本。共享版比私有版多剥离两个区段——U+202A–202E 与 U+2066–2069——因此 bidi 覆盖符会从<cwd>、<result>、<output-file>以及两条命令展示路径中被移除。<output-tail>走的是另一个 helper——它必须保留换行，而展示用 helper 会剥掉换行——现在那个 helper 也移除同样两个区段，因为 tail 是信封中最大的攻击者可控字段（最多 8 KiB 的后台 shell 自身输出）。实测：/tmp/a<RLO>evil<PDI>/out.log经旧副本原样输出，经共享版输出为/tmp/aevil/out.log。这些字符能在不改变字节的前提下改变路径的显示顺序，把它们留在面向模型的信封里就是一个伪装面——更强的行为才是值得保留的，并且现在有 registry 层的测试钉住它。原子写的重试策略未作改动：它是这个 bug 暴露出来的安全机制。
为什么需要
backgroundShellRegistry.test.ts的terminal-entry retention cap在共享 CI runner 上超时，其他地方全绿——四个用例各自Test timed out in 15000ms，整个文件约 120 秒。同一天在两个互不相关的分支上出现：feat/daemon-git-worktree-guard（job 93206694115）与feat/review-capture-tui（job 93208639663）。这些用例看上去是上限为 32 的纯内存循环，所以"runner 被压垮"是最自然的第一猜测——但它是错的。每次 settle（
complete/fail/cancel）都会在 entry 的outputPath旁写一个状态边车，而 fixture 的默认值把它们全部指向/tmp/s1.output，也就是所有运行过该套件的机器上同一个绝对路径/tmp/s1.status。/tmp带 sticky 位：当该文件已属于另一个用户（另一个 job、另一个账号、自托管 runner 上更早的运行），原子写的rename返回EPERM，renameWithRetrySync便以阻塞式指数退避重试——50 + 100 + 200ms——然后放弃；而writeStatusFile会吞掉错误只记日志，于是除了时钟，什么都不像出错。以 registry 实际传入的选项对真实
atomicWriteFileSync实测：每次 settle 362ms。上限用例每个要 settle 34 次，即在断言开始前就消耗掉 15 秒预算中的 12.3 秒。这解释了全部现象：为何只有那个 describe 块（只有它循环 34 次）、为何整文件涨到 120 秒、为何开发机上永远复现不了、以及为何会打到与 shell 毫无关系的分支。复核测试计划
如何验证
运行套件：
npx vitest run --root packages/core src/services/backgroundShellRegistry.test.ts→ 57 通过，约 0.7 秒。要看到 flake 本身，在同一台机器上重建共享 runner 的条件——在共享路径放一个本进程无法 rename 覆盖的文件，这正是 sticky-bit
/tmp下"属于其他用户"对rename(2)的含义：在
main上会报Test timed out in 15000ms；在本分支上通过，因为已没有任何 fixture 写入该路径。复核者也可以直接以同样选项对这种目标调用atomicWriteFileSync34 次并计时来确认代价。证据（前后对比）
同一台机器、同一个被污染的路径，唯一差别是 fixture：
/tmp/s1.outputTest timed out in 15000ms×3佐证测量：对无法 rename 覆盖的目标做 34 次状态写入耗时 12304ms，即每次 362ms，而测试预算是 15000ms。
测试环境
需要说明清楚（这是复核者无法从 diff 还原的部分）：57/57 的绿色运行与上面的 A/B 都在 macOS 上完成，冲突是模拟出来的（
chflags uchg让rename返回EPERM，与 sticky-bit/tmp下他人属主文件的表现一致）。我没有一台带真实他人属主/tmp/s1.status的自托管 runner 可供实跑——真实条件的证据是上面链接的两个失败 job；而本修复是彻底移除共享路径，而不是设法在其上存活，因此该失败条件在任何平台上都无法再从这个套件触及。运行环境（可选）
仅单元测试。
风险与范围
renameWithRetrySync中的阻塞退避是保护真实写入应对瞬时EPERM的机制，此处刻意不动；本 PR 消除的是测试对共享路径的依赖，而非削弱该保护。仓库中是否还有其他套件写入固定/tmp路径，本次未做审计。关联 Issue
无——在排查 #8388 上反复出现的 CI 失败时发现。