test(review): pin drive's capped-stream case to its invariant, not to bash 5.2 - #8366
Conversation
… bash 5.2
The SIGPIPE fabricated-exit-code test asserted the one wrong answer CI's
bash 5.2 produces (the EXIT trap firing with rc=0), and failed on every
other shell. Measured, per version, same script, same pipe:
- bash 5.2 (CI ubuntu): rc=0 — fabricated clean pass
- bash 5.3 (homebrew macOS): empty — sentinel file created, write lost
- bash 3.2 (stock macOS): rc=1 — the echo's EPIPE error recorded,
plus a stray padding line leaked
into the sentinel file
Three shells, three different wrong answers. The assertion now pins the
one invariant they share — the script's real `exit 5` never survives the
cap — which is the design point the test exists to defend, and holds on
every bash instead of one. (A first draft enumerated the wrong answers as
an allowed set and was immediately falsified by the third shell; the
enumeration is a moving target, the invariant is not.)
Also reads the sentinel through existsSync like the suite's own realExit
helper, so a shell that never creates the file reports null instead of
throwing ENOENT.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug, well-documented. The SIGPIPE test pins bash 5.2's specific wrong answer ( Direction: aligned. Test-only fix that makes the suite portable across shell versions. No product-direction concerns. Size: not applicable — single test file in Approach: the scope is exactly right. Weakening the assertion from Risk: no elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,记录详尽。SIGPIPE 测试钉死了 bash 5.2 的特定错误答案( 方向:对齐。仅测试修复,使套件在不同 shell 版本间可移植。无产品方向顾虑。 规模:不适用——单个测试文件位于 方案:范围恰好。将断言从 风险:无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal first: given a test that pins one shell's wrong answer, I would (1) weaken the assertion to the shared invariant — the true exit code never survives the pipe — (2) guard the sentinel read with The diff does exactly this, and nothing else. The CI test evidenceFinal CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The ubuntu unit suite is still running; macOS and Windows tests were skipped by CI config. No failures so far. The finalize workflow will update this table once CI settles. Not verified: the macOS-specific fix itself — CI skips macOS tests for this run, and the author's local results (24/24 on three consecutive runs with bash 5.3 and 3.2) are their own claim, not independently re-run here. The assertion change is straightforward enough that static review covers it: 中文说明代码审查先独立提出方案:给定一个钉死某个 shell 错误答案的测试,我会(1)将断言弱化为共享不变量——真实退出码永远不会在管道中存活;(2)用 diff 恰好做了这些,且没有多余改动。 CI 测试证据ubuntu 单元测试仍在运行;macOS 和 Windows 测试被 CI 配置跳过。目前无失败。finalize 工作流会在 CI 结束后更新此表。 未验证:macOS 特定修复本身——CI 本次跳过了 macOS 测试,作者本地结果(bash 5.3 和 3.2 下连续三次 24/24)是其自身声明,未在此独立重跑。断言变更足够简单,静态审查可覆盖: — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. This is exactly the kind of test fix that earns its place: a real, reproduced failure (three consecutive red runs on macOS), a minimal change that pins the invariant instead of one shell's behavior, and a comment that records the measured matrix so nobody re-derives it. The My independent proposal matched the diff almost exactly — the only thing I might have done differently is nothing; the PR already covers every angle I would have. Approval deferred until CI lands green on 中文说明置信度:5/5 —— 每个阶段都干净;毫不犹豫地合并。 这正是那种值得存在的测试修复:真实的、可复现的失败(macOS 上连续三次红),最小的改动钉住不变量而非某个 shell 的行为,注释记录了实测矩阵以免后人重新推导。 我的独立方案与 diff 几乎完全一致——唯一可能不同的是什么都不做;PR 已经覆盖了我能想到的每个角度。 审批推迟至 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
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-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its macOS-bash path did not run locally (Agent 7 ran Linux bash 5.2 only); this PR targets macOS bash specifically.
中文说明
已审查。 未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its macOS-bash path did not run locally (Agent 7 ran Linux bash 5.2 only); this PR targets macOS bash specifically。
— qwen3.8-max-preview via Qwen Code /review
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — correct fix. The old assertion pinned bash 5.2's flavor of wrong (rc=0); the new one pins the invariant all shells share (reported !== 5). The existsSync guard handles the bash 5.3 empty-file case cleanly. Test-only, no production code, well-documented measured matrix.
Conflict: the subcommand demand message again — main adds mock-provider, this branch adds publish-assets; union, in registration order. Absorbs #8366's drive.test.ts invariant fix, un-redding the macOS suite.
|
Released in v0.21.4. |
What this PR does
Fixes the one
drive.test.tscase that fails on macOS: the SIGPIPE fabricated-exit-code test pinned the specific wrong answer CI's bash 5.2 produces (rc=0), and every other shell produces a different wrong answer. The assertion now pins the invariant all shells share — the script's realexit 5never survives piping the drive throughhead -c— which is the design point the test exists to defend. The sentinel read also goes throughexistsSynclike the suite's ownrealExithelper, so a shell that never creates the file reportsnullinstead of throwing.Why it's needed
Measured, per version, same script, same pipe:
rc=0rc=1+ a stray padding line leaked into the fileThree shells, three different wrong answers — so
expect(...).toBe(0)is a pin on bash 5.2's behavior, not on the design rule ("bound the log by watching its size, never by capping the stream"). Anyone running the suite on macOS hits a stable red that has nothing to do with their change (found while resolving a conflict on #8351: three consecutive local runs failed identically). A first draft of this fix enumerated the wrong answers as an allowed set and was immediately falsified by the third shell — the enumeration is a moving target, the invariant (reported !== 5) is not, and the test's own comment now records the measured matrix so the next reader doesn't re-derive it.Reviewer Test Plan
How to verify
Expected: 24/24 on both Linux and macOS (observed locally on macOS with homebrew bash 5.3 first in PATH, three consecutive runs; the same probe script run explicitly under
/bin/bash3.2 confirms the rc=1 row of the table). On main, the same suite fails this one case on macOS withexpected null to be +0.Evidence (Before & After)
Before (main, macOS):
AssertionError: expected null to be +0— three consecutive runs. After: 24/24, three consecutive runs. Non-UI change, no screenshots.Tested on
Environment (optional)
Unit tests only, macOS, Node v24; bash 5.3 (homebrew) and /bin/bash 3.2 both probed.
Risk & Scope
not.toBe(5)instead oftoBe(0)) — it no longer notices if bash changes which wrong answer it gives, only that the true code never survives. That is exactly the property the design depends on; the per-version matrix lives in the comment for archaeology.drive.tsitself.Linked Issues
None. Surfaced while resolving a merge conflict on #8351; introduced by #8349.
中文说明
这个 PR 做了什么
修复
drive.test.ts在 macOS 上必红的一个用例:SIGPIPE 伪造退出码测试钉死了 CI bash 5.2 的特定错误答案(rc=0),而其他 shell 各给出不同的错误答案。断言改为钉住所有 shell 共享的不变量——把 drive 管道进head -c后,脚本真实的exit 5永远不会存活——这正是该测试要守卫的设计点。哨兵读取同时改走existsSync(与套件自己的realExit辅助函数一致),不创建文件的 shell 报null而非抛 ENOENT。为什么需要
同一脚本、同一管道,逐版本实测:
rc=0rc=1+ 一行 padding 泄漏进文件三个 shell 三种错法——
expect(...).toBe(0)钉的是 bash 5.2 的行为,不是设计规则("用监视文件大小来限制日志,绝不 cap 流")。任何在 macOS 上跑套件的人都会遇到与自己改动无关的稳定红(在解决 #8351 冲突时发现:本地连跑三次失败形态一致)。本修复的第一稿把错误答案枚举成允许集合,立刻被第三个 shell 证伪——枚举是移动靶,不变量(reported !== 5)不是;实测矩阵已写进测试注释,后来者无需重新推导。Reviewer 测试计划
如何验证
预期:Linux 与 macOS 均 24/24(本地 macOS、homebrew bash 5.3 在 PATH 首位,连跑三次;同一探针脚本显式用
/bin/bash3.2 跑,证实表中 rc=1 一行)。在 main 上,同套件在 macOS 红这一个用例:expected null to be +0。证据(Before & After)
Before(main,macOS):
AssertionError: expected null to be +0,连续三次。After:24/24,连续三次。非 UI 改动,无截图。测试平台
环境(可选)
仅单元测试,macOS,Node v24;homebrew bash 5.3 与 /bin/bash 3.2 均已探测。
风险与范围
not.toBe(5)而非toBe(0))——不再关心 bash 给出哪种错误答案,只关心真实退出码永不存活。这正是设计所依赖的性质;逐版本矩阵留在注释里供考古。drive.ts本身行为无改动。关联 Issue
无。解决 #8351 合并冲突时发现;由 #8349 引入。