fix(review): harden capture-tui against a same-uid racer, within what Node allows - #9274
fix(review): harden capture-tui against a same-uid racer, within what Node allows#9274wenshao wants to merge 5 commits into
Conversation
… Node allows The base command states its file guarantee against ordinary conditions and explicitly declines the adversarial one, because Node exposes no *at() syscalls and every path resolves by name. This narrows that gap as far as the runtime does allow — from "an actor has the whole capture window (up to an hour) to redirect the evidence" down to "an actor has the interval between a check and the syscall on its heels". Every item below was reproduced against the base command before it was fixed, each with a witness: - The collision gate runs BEFORE the window; the window then runs for up to --timeout-ms. A captured command writing its own `<out>.json` had that file silently replaced and the run reported success. Occupancy is re-decided at write time against the same pre-window stamp. - A symlink planted at `<out>.ans` during the window redirected this run's bytes out of the --out base. The opens carry O_NOFOLLOW, and create atomically (O_CREAT|O_EXCL) rather than truncating whatever is there. - The png is the one path an external binary writes: its pre-window stamp said "absent" for anything planted since, so freeze wrote THROUGH a planted symlink and the run attested `evidence: 'png'` for bytes outside the base, while a failed render deleted whatever the captured command had put there. It is re-stamped after the window, and only a regular file counts as a rendering. - The manifest's signature authenticates the MANIFEST and said nothing about the files beside it, so a genuine previous manifest authorized unlinking whatever had since taken the .ans name. The manifest records the identity of the artifacts it wrote, and the clear phase requires a match. - Every path resolves by name against a tree the captured command can mutate: `mv dir dir.stolen && ln -s /victim dir` mid-window sent all three artifacts out of the base while the manifest attested the original paths. The directory is pinned by dev+ino and re-checked before every artifact operation. - A refusal must not run its own cleanup on the occupant it refused to replace: on a collision a path is removed only while its current identity still proves it is this run's. What these tests do and do not show: they pin the branches and the outcomes, and each fix reddens its test when reverted. They do not reproduce the races themselves — a single-threaded test cannot occupy the interval a racer occupies, so the atomic-open and re-check guarantees are argued from the syscalls, not proven by the suite.
|
Gate passes — moving on to code review. 🔍
中文说明准入通过，进入代码审查。🔍
— Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline first: for this problem I would have proposed exactly what this PR does — re-decide occupancy at write time with What reads well:
Findings — one suggestion, one informational, no blockers:
Also noted: Test evidenceThis branch has no PR CI, and that is the load-bearing fact of this review.
(The finalize job rewrites this region only once Sandboxed verification would settle this: Real-scenario testingN/A — the happy path is byte-identical by construction (the guards change behavior only under an adversarial same-uid race, which the unit reproductions pin), and per triage rules PR-derived code is not checked out or executed. The command also does not exist on 中文说明代码审查先说独立基线：只看问题描述，我会提出的方案与本 PR 完全一致——写时用 做得好的地方：处处 fail-closed（open 遇 EEXIST → 碰撞；缺失或畸形的 发现——一条建议、一条说明，均非阻塞：
另注： 测试证据本分支没有 PR CI——这是本次审查的关键事实。 沙箱验证可以补齐： 真实场景测试N/A——正常路径按构造字节级不变（守卫只改变同 uid 对抗竞速下的行为，已由单测复现钉住），且按 triage 规则不检出、不执行 PR 代码。该命令在 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean code review with one non-blocking suggestion, but nothing has run on this branch and its base PR hasn't merged; that is not something the diff alone can attest to. Stepping back: this is the kind of PR the gate should be glad to see. The threat model is real for this feature (the captured command is untrusted code by definition, and evidence integrity is the product), the scope is stated honestly — closes the window, not the race — and the implementation and my independent proposal agree one-to-one. Every guard is fail-closed, each has a pinning test, and the header contract now says exactly what the command does and does not defend. In six months this file reads better for it. Why not approve, then? Two things I cannot resolve statically:
⏸️ Deferring to @wenshao — once #9273 merges and this is re-targeted at No approve-on-green marker: there is no 中文说明回头看：这是门禁应当乐见的那类 PR。威胁模型对该功能是真实的（被采集的命令按定义不可信，证据完整性就是产品本身），范围声明诚实——关窗口、不关竞态——实现与我的独立方案一一对应。每个守卫都 fail-closed、各有一个钉住它的测试，文件头契约现在精确说明该命令防什么、不防什么。六个月后读这个文件只会更顺畅。 那为什么不批？两件静态无法确认的事：
⏸️ 转交 @wenshao——#9273 合入、本 PR 重新以 不发 approve-on-green 标记：这个基线上没有 — 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.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): capdir/cap.ans — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：capdir/cap.ans — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| ans: ansWritten as ArtifactId, | ||
| png: png ? idOf(stampOf(pngPath)) : null, |
There was a problem hiding this comment.
[Critical] R1-1: The manifest records the png identity from a fresh by-name stat (idOf(stampOf(pngPath))) instead of the credit-time pngWritten, and no png identity re-check runs anywhere between the freeze write and the manifest write — while the .ans gets both (write-time identity + the sameFile(ansWritten, ansPath) re-check below this block). This contradicts the adjacent comment "From the write, not from a fresh stat of the name". — Failure scenario: a same-uid racer replaces <out>.png anywhere between the freeze write and the manifest stat → the run exits 0 attesting evidence: 'png' for bytes it never produced and records the replacer's identity, which a later run's clear phase (sameFile(ids?.png, pngPath)) then authorizes to unlink; the recorded identity also diverges from pngWritten, this run's own cleanup authority.
Witness (executed):
probe sweep under load: 30/30 attestation wins
run 1 (delay=0): exit=undefined evidence=png manifestPngSize=7777 attestedForgery=true
SUMMARY: attestedForgerySeen=true
The suggestion below records the credit-time identity; additionally add if (png && !sameFile(pngWritten, pngPath)) throw new ArtifactCollision(...) beside the .ans re-check below. Note this closes the post-credit shape; the pre-credit interval additionally needs the credit itself to be identity-pinned (see the render/credit findings).
| ans: ansWritten as ArtifactId, | |
| png: png ? idOf(stampOf(pngPath)) : null, | |
| ans: ansWritten as ArtifactId, | |
| png: pngWritten ?? null, |
中文说明
manifest 记录 png 身份时用的是对路径的重新 stat（idOf(stampOf(pngPath))），而不是授信时刻已捕获的 pngWritten；并且从 freeze 写入到 manifest 写入之间完全没有 png 身份复核——而 .ans 两者都有（写入时身份 + 下方 sameFile(ansWritten, ansPath) 复核）。这与紧邻的注释 "From the write, not from a fresh stat of the name" 自相矛盾。
故障场景：同 uid 竞速者在 freeze 写入与 manifest stat 之间替换 <out>.png → 运行以 exit 0 结束，为本非本次运行产生的字节作证 evidence: 'png'，并记录下替换者的身份；后续运行的 clear 阶段（sameFile(ids?.png, pngPath)）会据此获得删除授权。记录的身份还会与 pngWritten（本次运行清理逻辑的授权依据）发生分歧。
验证证据（已执行）：负载下探测扫描 30/30 全部成功伪证（attestedForgery=true），未设防窗口约 50-250ms。
修复：下方 suggestion 改为记录授信时刻的身份；并在 .ans 复核旁补上 if (png && !sameFile(pngWritten, pngPath)) throw new ArtifactCollision(...)。注意这只能关闭授信后的窗口；授信前区间还需要授信本身基于身份固定（见 render/credit 相关发现）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // Its identity at credit time: the cleanup above asks whether the | ||
| // file still IS this one before removing anything. | ||
| pngWritten = idOf(stampOf(pngPath)); |
There was a problem hiding this comment.
[Critical] R1-2: assertSameOutDir() has exactly one call site (inside writeArtifact): the freeze render spawn, the credit stats, and the failure-cleanup rmSync calls never re-check directory identity — contradicting the header's "re-checked before every artifact operation". — Failure scenario: mid-render directory swap (this PR's own threat model): freeze writes the png into the substitute directory through the redirected name; the run refuses exit 3 at the .ans identity re-check, but the collision cleanup's rmSync(pngPath) resolves the redirected name and — pngWritten having been credited through the same swap — sameFile(pngWritten, pngPath) matches the victim's file, so the cleanup deletes a file in a foreign directory. Net: foreign file destroyed + render bytes land outside --out.
Witness (executed):
5/5 live runs:
{"exitCode":3,"swapped":true,"victimPngNow":"ABSENT","victimBeforeOverwrite":"VICTIM-FILE"}
with a dir-identity guard added to the collision cleanup: 5/5 victimPngNow ABSENT -> FREEZE-RENDERED-BYTES
Fix: call assertSameOutDir() before the freeze spawn, before the credit stats, and before the cleanup rmSync calls — routing the throw through the refusal path (the render block is inside no try).
中文说明
assertSameOutDir() 只有一个调用点（writeArtifact 内部）：freeze 渲染 spawn、授信统计、失败清理的 rmSync 调用都从不复核目录身份——与文件头 "re-checked before every artifact operation" 的声明矛盾。
故障场景：渲染期间目录被掉包（本 PR 自己的威胁模型）：freeze 通过被重定向的路径把 png 写进替代目录；运行随后在 .ans 身份复核处以 exit 3 拒绝，但碰撞清理的 rmSync(pngPath) 沿重定向后的路径解析，且 pngWritten 也是通过同一次掉包授信的，于是 sameFile(pngWritten, pngPath) 匹配到受害者目录里的文件，清理把它删掉。净效果：外部文件被销毁 + 渲染字节落在 --out 之外。
验证证据（已执行）：5/5 复现（受害者文件被删）；给碰撞清理加上目录身份守卫后 5/5 翻转为不再删除。
修复：在 freeze spawn 前、授信统计前、清理 rmSync 前调用 assertSameOutDir()——并把抛错接入拒绝路径（render 块不在任何 try 内）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (!sameFile(ansWritten, ansPath)) { | ||
| throw new ArtifactCollision( |
There was a problem hiding this comment.
[Critical] R1-3: freeze consumes .ans BY NAME during the render (freezePlan passes the path as argv), but this new identity re-check runs only AFTER the render — a swap-then-restore around the render defeats it, because rename preserves ino/size/mtime exactly. — Failure scenario: a daemonized descendant (the class this file documents it does not reap) renames cap.ans aside before freeze opens it and puts attacker content at the name → freeze renders attacker bytes into cap.png, the credit gate passes, and the original is renamed back before this check — sameFile(ansWritten, ansPath) passes. The run exits 0 with evidence: 'png', the manifest coupling the png to the genuine ansPath while the png renders attacker content. Swap window = freeze probe + spawn (seconds); restore window = the entire render — no syscall-interval race needed, contrary to the stated residue.
Witness (executed, real tmux + fake freeze):
PR: exitCode undefined, png = "PNG-RENDERED-FROM:ATTACKER-ANSI", .ans genuine+restored
control (no restore): exit 3 "cap.ans was replaced after this capture wrote it"
fd-pinned-copy fix: png = "PNG-RENDERED-FROM:HELLO-RED-WORLD"
Fix: render from an fd-pinned private copy: after the .ans write, open ansPath O_NOFOLLOW, fstat-verify against ansWritten, copy through the descriptor to an unpredictable path, and pass that to freezePlan. A pre-render check alone does not close it (a swap can land between the check and freeze's open, then be restored before this post-render check).
中文说明
freeze 在渲染期间按名字读取 .ans（freezePlan 把路径作为 argv 传入），而这条新增的身份复核只在渲染之后运行——围绕渲染做"换走再换回"即可绕过，因为 rename 精确保留 ino/size/mtime。
故障场景：守护化的后代进程（本文件明确声明不会收割的一类）在 freeze 打开前把 cap.ans 改名挪走、放入攻击者内容 → freeze 把攻击者字节渲染进 cap.png，授信门通过，原文件在此复核前被改名换回——sameFile(ansWritten, ansPath) 通过。运行以 exit 0 结束并作证 evidence: 'png'：manifest 把 png 关联到真实的 ansPath，而 png 渲染的是攻击者内容。换走窗口 = freeze 探测 + spawn（秒级）；换回窗口 = 整个渲染期——无需任何系统调用级竞态，与声明的残留窗口不符。
验证证据（真实 tmux + fake freeze 执行）：见英文部分；对照臂（不换回）被本检查正确拦下；fd 固定副本修复后探测翻转。
修复：从 fd 固定的私有副本渲染：.ans 写入后以 O_NOFOLLOW 打开 ansPath，用 fstat 对照 ansWritten 验证，通过描述符复制到一个不可预测路径，再把它传给 freezePlan。只在渲染前检查无法关闭此窗口（交换仍可落在检查与 freeze 打开之间，并在渲染后复核前换回）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| st.existed && | ||
| st.ino === r.ino && | ||
| st.size === r.size && | ||
| st.mtimeMs === r.mtimeMs |
There was a problem hiding this comment.
[Critical] R1-4: The identity triple carries no content binding and the artifacts are owner-writable (a same-uid actor IS the owner): an in-place overwrite + mtime restore passes every identity gate this PR adds, without winning any syscall-interval race. — Failure scenario: during the render window the actor overwrites cap.ans in place (ino preserved) with doctored content padded to the recorded size, then restores the original mtime via utimensat (ns-exact round-trip) → sameFile passes on all three components and the manifest attests content this run never produced, exit 0. The header's residue says what remains is "a racer fast enough to land inside that interval" — this shape needs no interval at all, so it contradicts the stated residue rather than being covered by it.
Witness (executed):
E2E: exitCode undefined, .ans = "FORGED\0..." (padded to recorded size),
png = "PNG-RENDERED-FROM:FORGED", artifacts.ans matches doctored identity
digest-binding fix: exit 3 "content changed after this capture wrote it"
micro-probe: mtime round-trip 1786896873655.4246 === 1786896873655.4246
Fix: bind content, not just metadata: compute a content digest THROUGH the write descriptor (before closeSync(fd) in writeArtifact) and record it in artifacts, comparing before crediting or clearing. If content binding is declined for this PR, state the in-place-overwrite shape explicitly in the residue comments instead of claiming only syscall-interval racers remain.
中文说明
身份三元组不绑定内容，且工件对属主可写（同 uid 竞速者就是属主）：原地覆写 + mtime 还原可以通过本 PR 新增的所有身份门，完全不需要赢得任何系统调用级竞态。
故障场景：渲染窗口内，攻击者原地覆写 cap.ans（ino 不变），内容填充到记录大小，再用 utimensat 还原原始 mtime（纳秒级精确往返）→ sameFile 三项全部通过，manifest 为本非本次运行产生的内容作证，exit 0。文件头的残留声明说剩下的只是"快到能落进那个间隙的竞速者"——这种形态根本不需要任何间隙，因此与声明的残留相矛盾而非被其覆盖。
验证证据（已执行）：E2E 复现成功（伪证内容被渲染并被 manifest 记录）；内容摘要绑定修复后翻转为 exit 3 拒绝。
修复：绑定内容而非仅元数据：在 writeArtifact 的 closeSync(fd) 之前通过写描述符计算内容摘要并记入 artifacts，在授信/清理前比对。如果本 PR 不做内容绑定，请在残留注释中明确写出"原地覆写"这一形态，而不是声称只剩系统调用间隙竞速。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // against a closed window with only the render able to have changed it. | ||
| pngStamp = stampOf(pngPath); |
There was a problem hiding this comment.
[Critical] R1-5: The png's write has no write-time occupancy gate: this re-stamp is the LAST check on <out>.png, and it is separated from freeze's by-name open of --output by two process spawns (the freeze probe, then the render spawn). The header's "occupancy is re-decided at write time, the artifact opens are atomic and do not follow symlinks" is false for the one artifact this command does not open itself. — Failure scenario: the re-stamp finds <out>.png absent; between that stamp and freeze's open — signal-triggerable via the probe process in /proc, no microsecond timing — a racer runs ln -s /victim/file cap.png → freeze follows the link and truncates the victim's file, writing render bytes through it. The credit-side guard records ans-only (correct attestation), but the victim's file is already destroyed. The hardlink variant (ln victim cap.png) is even credited as evidence: 'png', laundering the destroyed file's identity into artifacts.png.
Witness (executed; freeze modeled through the repo's fake-freeze seam with freeze's documented open semantics):
ARM-B (daemonized racer, planted on FIRST poll): render-ran=true, victim-content="RENDER-BYTES", evidence=ans-only
ARM-C (hardlink): victim-content="RENDER-BYTES", evidence=png
spawn-boundary re-check fix: render-ran=false, victim-content="VICTIM-ORIGINAL-CONTENT"
Fix: re-decide occupancy at the spawn boundary: move probes.freeze() ahead of this re-stamp and re-lstatSync(pngPath) as the last statement before spawnSync, degrading if occupied (narrows check-to-open to the spawn itself). Also scope the header's atomic-open claim to the opens this command performs, stating the png is written by freeze under credit-refusal-only protection.
中文说明
png 的写入没有写入时刻占用门：这次重新取样是对 <out>.png 的最后一次检查，而它与 freeze 按名字打开 --output 之间隔着两次进程 spawn（freeze 探测、渲染 spawn）。文件头 "occupancy is re-decided at write time, the artifact opens are atomic and do not follow symlinks" 的声明，对唯一不由本命令自己打开的工件不成立。
故障场景：重新取样发现 <out>.png 不存在；在该取样与 freeze 打开之间——可通过 /proc 中的探测进程发信号触发、无需微秒级时机——竞速者执行 ln -s /victim/file cap.png → freeze 跟随链接截断受害者文件并把渲染字节写穿过去。授信侧守卫记录 ans-only（作证正确），但受害者文件已被销毁。硬链接变体（ln victim cap.png）甚至被授信为 evidence: 'png'，把被销毁文件的身份洗进 artifacts.png。
验证证据（已执行）：见英文部分三个探测臂（守护化竞速者首次轮询即命中）；spawn 边界重检修复后翻转。
修复：在 spawn 边界重新裁定占用——把 probes.freeze() 移到这次重新取样之前，并把 lstatSync(pngPath) 作为 spawnSync 前最后一条语句，占用则降级（把检查-打开间隙收窄到 spawn 本身）。同时把文件头的原子打开声明限定到本命令自己执行的打开，并注明 png 由 freeze 写入、只有授信拒绝一层保护。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| ? sameFile(ansWritten, ansPath) | ||
| : changed(ansPath, ansStamp) |
There was a problem hiding this comment.
[Suggestion] R1-8: The collision arm of this .ans-write catch cleanup is provably always false: ansWritten is only assigned when writeArtifact returns successfully, and every ArtifactCollision throw inside writeArtifact precedes the write. The ternary was transplanted from the manifest-failure cleanup below, where a collision CAN coexist with successful writes — here it expresses an impossible state (collision AND a successful write) and contradicts the comment directly above ("...and NEVER on a collision"). — Concrete cost: no behavioral bug today, but the next maintainer of this safety-critical cleanup logic is misled into believing a collision here can follow a successful write — or must re-trace writeArtifact to prove the branch dead.
Fix: restore the flat form the comment describes:
if (!(e instanceof ArtifactCollision) && changed(ansPath, ansStamp))中文说明
.ans 写入 catch 清理中的碰撞分支可以证明恒为假：ansWritten 只在 writeArtifact 成功返回时赋值，而 writeArtifact 内部所有 ArtifactCollision 抛出都发生在写入之前。这个三元表达式是从下方 manifest 失败清理移植来的——那里碰撞确实可能与成功写入共存——但在这里它表达一个不可能的状态（碰撞且写入成功），并与紧邻上方的注释（"...and NEVER on a collision"）矛盾。
具体代价：今天没有行为 bug，但下一个维护这段安全关键清理逻辑的人会被误导，以为这里的碰撞可能跟随成功写入——或者必须重新追踪 writeArtifact 才能证明该分支是死的。
修复：恢复注释所描述的扁平形式（见英文代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // same-uid actor racing it during the capture window (which runs up to | ||
| // --timeout-ms, an hour at the cap): occupancy is re-decided at write |
There was a problem hiding this comment.
[Suggestion] R1-9: Comments added by this diff disagree on the max capture-window length: the header says "runs up to --timeout-ms, an hour at the cap"; the dir-pin comment (:452) says "up to 70 minutes"; the png re-stamp comment (:1578) says "(70 minutes at the --timeout-ms cap)". The bounds table caps --timeout-ms at 3_600_000 (60 min) and --settle-ms at 600_000 (10 min), and a --ready-only run can spend the full timeout and then additionally sleep settleMs — the true max is 70 minutes. — Failure scenario: a reader sizing the residual race window — the exact question this prose exists to answer — gets two different bounds depending on which comment they read, and the 70-minute figure is attributed to a flag whose cap is 60.
Fix: state the window runs up to --timeout-ms + --settle-ms (70 minutes at the caps) in all three places.
中文说明
本 diff 新增的注释对采集窗口最大时长说法不一：文件头说 "runs up to --timeout-ms, an hour at the cap"；目录钉住注释（约 :452）说 "up to 70 minutes"；png 重新取样注释（约 :1578）说 "(70 minutes at the --timeout-ms cap)"。边界表把 --timeout-ms 上限设为 60 分钟、--settle-ms 上限设为 10 分钟，而 --ready 单独运行可以耗满超时后再额外 sleep settleMs——真正的最大值是 70 分钟。
故障场景：想要估算残留竞态窗口的读者（正是这些注释存在的目的）会从不同注释读到两个不同的上限，而且 70 分钟被归因于上限只有 60 分钟的参数。
修复：三处统一改为"窗口最长为 --timeout-ms + --settle-ms（上限合计 70 分钟）"。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const ids = (m as { artifacts?: unknown }).artifacts as | ||
| | { ans?: unknown; png?: unknown } | ||
| | undefined; | ||
| if (shaped && !sameFile(ids?.ans, ansPath)) shaped = false; |
There was a problem hiding this comment.
[Suggestion] R1-10: The deliberate behavior change that manifests without an artifacts rung — i.e. every manifest this same command wrote before this PR — lose clear authority is pinned by no test: this diff's fixture updates added artifacts to every fixture, so the legacy shape left the test suite entirely. — Failure scenario (mutant verified): skipping both identity checks when the manifest carries no artifacts rung keeps the full suite green (138 passed | 1 skipped) while a probe flips — a fully signed manifest minus only the artifacts rung clears LEGACY-ANS/LEGACY-PNG and the run proceeds, where this PR's behavior is exit 3 + refusal + all three files surviving. That mutant re-opens the exact window this PR closes.
Witness (executed):
mutant suite: 138 passed | 1 skipped (green)
probe flip: AssertionError: expected undefined to be 3
Fix: add one fixture: a fully signed manifest (evidence/ansPath/settledBy all valid, matching files) minus only the artifacts rung, asserting exit 3, refusal, and all three files surviving.
中文说明
"没有 artifacts 层的 manifest（即本 PR 之前该命令写出的所有 manifest）失去清理授权"是刻意的设计变更，但没有任何测试钉住它：本 diff 的 fixture 更新给每个 fixture 都加上了 artifacts，遗留形态彻底退出了测试套件。
故障场景（突变体验证）：当 manifest 没有 artifacts 层时跳过两项身份检查，整个套件仍为绿色（138 通过 | 1 跳过），而探测翻转——只缺 artifacts 层的完整签名 manifest 会清掉 LEGACY-ANS/LEGACY-PNG 并继续运行；本 PR 的行为应是 exit 3 + 拒绝 + 三个文件全部幸存。该突变体重新打开了本 PR 要关闭的窗口。
验证证据（已执行）：突变体套件全绿；探测翻转 expected undefined to be 3。
修复：补一个 fixture：只缺 artifacts 层的完整签名 manifest，断言 exit 3、拒绝、三个文件全部幸存。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (manifestHadPng && !sameFile(ids?.png, pngPath)) { | ||
| manifestHadPng = false; |
There was a problem hiding this comment.
[Suggestion] R1-11: This new png-side clear-phase identity check is tested only on its happy path: the GENUINE-REPLACED test replaces the .ans and never the .png, and no other fixture reaches this branch with a mismatched png identity. — Failure scenario (mutant verified): deleting this three-line block keeps the full suite green while a probe flips — with a seeded genuine manifest whose recorded png identity matches nothing, the user's file at <out>.png is deleted instead of being spared. That is the same data-loss shape this PR exists to close, arriving through the png name.
Witness (executed):
mutant suite: 138 passed | 1 skipped (green)
probe flip: Error: ENOENT: ... open '/tmp/probe-f14-sU3Nx8/cap.png' (user png deleted)
Fix: add the symmetric test: seed cap.ans/cap.png, write staleManifest(...), replace only cap.png with a user file, assert refusal + the user's png surviving byte for byte.
中文说明
这个新增的 png 侧 clear 阶段身份检查只有快乐路径被测试到：GENUINE-REPLACED 测试替换的是 .ans 而从不替换 .png，也没有其他 fixture 以不匹配的 png 身份到达此分支。
故障场景（突变体验证）：删除这三行后整个套件仍为绿色，而探测翻转——播种一个记录 png 身份与任何文件都不匹配的真实 manifest 时，用户在 <out>.png 的文件会被删除而不是被保留。这正是本 PR 要关闭的数据丢失形态，从 png 这个名字进来。
验证证据（已执行）：突变体套件全绿；探测翻转（用户 png 被删，ENOENT）。
修复：补对称测试：播种 cap.ans/cap.png，写入 staleManifest(...)，只替换 cap.png 为用户文件，断言拒绝 + 用户 png 逐字节幸存。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| try { | ||
| pngIsFile = lstatSync(pngPath).isFile(); |
There was a problem hiding this comment.
[Suggestion] R1-12: The new pngIsFile credit guard has no test that reaches it: it only bites when the post-window re-stamp says absent and a symlink appears between re-stamp and credit — but both new png tests plant occupants mid-window, where pngStamp.existed fires the degradation ladder before freeze is ever spawned. — Failure scenario (mutant verified): dropping pngIsFile && from the credit condition keeps the full suite green while a probe flips — a fake freeze that runs ln -s outside "$5" during the render and exits 0 is credited as evidence: 'png' where this PR's behavior is 'ans-only' with the link target untouched.
Witness (executed):
mutant suite: 138 passed | 1 skipped (green)
probe flip: AssertionError: expected 'png' to be 'ans-only'
Fix: add a fake-freeze test that plants ln -s <target> "$5" during the render and exits 0, asserting evidence: 'ans-only', pngPath: null, and the link/target untouched.
中文说明
新增的 pngIsFile 授信守卫没有任何测试能到达：它只在窗口后重新取样为"不存在"、且符号链接出现在重新取样与授信之间时才生效——但两个新的 png 测试都在窗口中埋设占用者，那里 pngStamp.existed 会在 freeze 被 spawn 之前就触发降级阶梯。
故障场景（突变体验证）：从授信条件中删掉 pngIsFile &&，整个套件仍为绿色，而探测翻转——fake freeze 在渲染期间执行 ln -s outside "$5" 并以 0 退出时，会被授信为 evidence: 'png'；本 PR 的行为应是 'ans-only' 且链接目标不受影响。
验证证据（已执行）：突变体套件全绿；探测翻转 expected 'png' to be 'ans-only'。
修复：补一个 fake-freeze 测试：渲染期间 ln -s <target> "$5" 并以 0 退出，断言 evidence: 'ans-only'、pngPath: null、链接与目标均未被动。
— qwen3.8-max via Qwen Code /review (v0.21.12)
# Conflicts: # packages/cli/src/commands/review.ts # packages/cli/src/commands/review/agent-prompt.test.ts # packages/cli/src/commands/review/cleanup.test.ts # packages/cli/src/commands/review/cleanup.ts # packages/cli/src/commands/review/lib/agent-briefs.ts
…to HEAD # Conflicts: # packages/cli/src/commands/review/capture-tui.ts # packages/cli/src/commands/review/cleanup.test.ts # packages/cli/src/commands/review/cleanup.ts
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent test-matrix": could not execute packages/cli vitest to confirm the predicted failures — the review worktree has no node_modules /built dist , and install+build exceeds th…; "agent test-matrix": did not walk the real-tmux describe body between lines 4060-5330 (signal/holder E2E region) beyond sampling 3996-4056 and grepping its wording pins; additional …; chunk 5: run capture-tui.test.ts under vitest to observe the wall-of-stderr test failing end-to-end (review worktree has no node_modules/dist; substituted the standalone….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): capdir/cap.ans — no such file or directory.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/capture-tui.ts:103 — [review] dead verdict-base exports (verdictExaminedBase/isSocketPathAbsent/isSocketDirNeverCreated) after the reap rewrite — deferred (code-age: unchanged since round 1)packages/cli/src/commands/review/capture-tui.ts:829 — [review] unconditional post-mkdir re-baseline adopts a swapped outDir identity — deferred (code-age: unchanged since round 1)packages/cli/src/commands/review/capture-tui.ts:769 — [review] clear gate conflates an ABSENT recorded artifact with a replaced one, wedging same---out reuse — deferred (code-age: unchanged since round 1)packages/cli/src/commands/review/capture-tui.ts:829 — [review] R1-6 still stands — unconditional re-baseline leaves the up-front outDirId sample deadpackages/cli/src/commands/review/capture-tui.ts:1564 — [review] R1-8 still stands — .ans-write catch collision arm provably always falsepackages/cli/src/commands/review/capture-tui.ts:45 — [review] R1-9 still stands — comments added by this diff disagree on the max capture-window lengthpackages/cli/src/commands/review/capture-tui.ts:771 — [review] R1-11 still stands — png-side clear-phase identity check tested only on its happy pathpackages/cli/src/commands/review/capture-tui.ts:1720 — [review] R1-12 still stands — pngIsFile credit guard has no test that reaches it
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："agent test-matrix"：could not execute packages/cli vitest to confirm the predicted failures — the review worktree has no node_modules /built dist , and install+build exceeds th…；"agent test-matrix"：did not walk the real-tmux describe body between lines 4060-5330 (signal/holder E2E region) beyond sampling 3996-4056 and grepping its wording pins; additional …；chunk 5：run capture-tui.test.ts under vitest to observe the wall-of-stderr test failing end-to-end (review worktree has no node_modules/dist; substituted the standalone…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：capdir/cap.ans — no such file or directory。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| isSocketDirUnusable, | ||
| isSocketPathAbsent, | ||
| verdictExaminedBase, | ||
| validGeometry, | ||
| type ArtifactId, |
There was a problem hiding this comment.
[Critical] R2-1: The merge resolution deleted the base branch's probe-verified defenses while keeping the base branch's tests that pin them — the committed suite is red at HEAD: 25 failures in capture-tui.test.ts + 3 in cleanup.test.ts, while merge base e8ab9092 is 229/229 green. Every failure maps 1:1 onto a behavior this diff removes whose deleted comment recorded the harm as probe-reproduced. Reverted clusters (failing test counts): (1) nonce-staged render + rename output landing — freeze never reads the .ans by name, symlink-before-staging refused via link() clone, stage-replaced-by-dir safe, no render THROUGH a planted symlink, torn/0-byte png left in place (7); (2) manifest size gate in UTF-8 bytes of the pretty-printed writer shape, not dense UTF-16 code units (2); (3) the errTail .slice(0, 2048) cap — a wall of freeze stderr cannot push the manifest past the reader cap (1, observed 3,145,792 bytes against the 8,192 assertion); (4) per-base env-pinned kills + base-scoped verdict crediting + stamped socket identity in the reap (6); (5) canonical realpathSync socket-base length measure (2); (6) hostStateFor ENFILE/EIO/ESTALE arms (3); (7) TMPDIR gate directoryness check + sentinel-rmSync attribution belt (1); (8) --no-keys negation refusal wording (1); (9) -T capture-trims caveat degradation (1); (10) the orphan-server sweep hoisted above the early returns in cleanup.ts + reaps kept off removedAny (3). Note for cluster (10): the sweep must be restored ABOVE ALL early returns (its base position as the first statement) — not only above the lease gate — because both symlink-redirect refusal paths also skip it now, and the sweep touches only tmux socket dirs, never REVIEW_TMP_DIR, so the redirect gates' hazard does not apply to it. Also reverted without a pinning test: the probeOutput ENOBUFS arm (a probe binary that ran and out-spewed maxBuffer now classifies as spawned:false — a false "could not spawn" claim). The legacy-manifest fallback failure is tracked separately as R1-10.
Witness (measured on both trees):
HEAD: capture-tui.test.ts 25 failed | 139 passed | 1 skipped; cleanup.test.ts 3 failed | 68 passed
merge base e8ab9092: 229 passed, 0 failed
Fix: redo the merge of feat/review-capture-tui-core preserving the base side's staged render, byte-based pretty-shape size gate, errTail cap, stamped multi-base reap verdicts, canonical socket measure, hostStateFor arms, TMPDIR gate + belt, keys wording, -T caveat, and the hoisted sweep — or justify each deletion and delete/update the corresponding committed tests in this PR.
中文说明
合并取舍删掉了 base 分支上经过探测验证的防御，却保留了钉住这些行为的 base 分支测试——HEAD 上已提交的测试套件是红的：capture-tui.test.ts 25 个失败 + cleanup.test.ts 3 个失败，而合并基 e8ab9092 是 229/229 全绿。每个失败都与本 diff 删除的某个行为一一对应，且被删注释都记录过该危害是探测复现过的。被还原的簇（失败测试数）：(1) nonce 暂存渲染 + rename 落盘——freeze 永不按名读 .ans、暂存前符号链接被 link() 克隆拒绝、暂存被换成目录安全、渲染不会写穿植入的符号链接、撕裂/0 字节 png 原地保留（7 个）；(2) manifest 大小门按美化输出形态的 UTF-8 字节计量，而非稠密 UTF-16 码元（2 个）；(3) errTail 的 .slice(0, 2048) 上限——freeze stderr 墙不能把 manifest 推过读取上限（1 个，实测 3,145,792 字节对 8,192 断言）；(4) reap 中按 base 固定环境的 kill + 按 base 裁定的判决授信 + 带取样的 socket 身份（6 个）；(5) 规范化 realpathSync socket base 长度计量（2 个）；(6) hostStateFor 的 ENFILE/EIO/ESTALE 分支（3 个）；(7) TMPDIR 门的目录性检查 + 哨兵 rmSync 归因保护带（1 个）；(8) --no-keys 否定形式的拒绝措辞（1 个）；(9) -T 捕获裁剪告警降级（1 个）；(10) cleanup.ts 中把孤儿服务器清扫提到早退之前 + reap 不计入 removedAny（3 个）。簇 (10) 注意：清扫必须恢复到所有早退之前（base 位置，函数第一句），而不仅是 lease 门之前——两个符号链接重定向拒绝路径现在也会跳过它，而清扫只碰 tmux socket 目录、从不碰 REVIEW_TMP_DIR，重定向门的危险不适用于它。另有一处无测试钉住的还原：probeOutput 的 ENOBUFS 分支（探测二进制运行了但输出超过 maxBuffer 现在被归类为 spawned:false——对一个确实启动了的二进制做出"无法启动"的虚假声明）。legacy manifest 回退缺失的失败单独追踪为 R1-10。
修复：重做 feat/review-capture-tui-core 的合并，保留 base 侧的分阶段渲染、按字节的美化形态大小门、errTail 上限、按 base 取样的 reap 判决、规范化 socket 计量、hostStateFor 分支、TMPDIR 门 + 保护带、keys 措辞、-T 告警、以及前置清扫——或者逐条论证每个删除并删除/更新对应的已提交测试。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| ans: ansWritten as ArtifactId, | ||
| png: png ? idOf(stampOf(pngPath)) : null, |
There was a problem hiding this comment.
[Critical] R1-1: Still stands — no fix commits since round 1 (only merges). The manifest records the png identity from a fresh by-name stat instead of the credit-time pngWritten, and no png identity re-check runs anywhere between the freeze write and the manifest write — while the .ans side gets both. The adjacent comment "From the write, not from a fresh stat of the name" contradicts the png line; pngWritten is read only by the collision cleanup.
Failure scenario: a same-uid racer replaces <out>.png anywhere between the freeze write and the manifest stat → the run exits 0 attesting evidence: 'png' for bytes it never produced and records the replacer's identity, which a later run's clear phase (sameFile(ids?.png, pngPath)) then authorizes to unlink.
Witness (round 2, executed at HEAD):
probe: fake freeze writes genuine render, then replaces cap.png
PR code: exit 0, evidence='png', attested content = 'FORGED-BYTES-ATTESTED'
control: identical .ans replacement refuses exit 3
flip: png identity re-check + rename landing → 5/5 attest 'GENUINE-RENDER'
| ans: ansWritten as ArtifactId, | |
| png: png ? idOf(stampOf(pngPath)) : null, | |
| ans: ansWritten as ArtifactId, | |
| png: png ? (pngWritten as ArtifactId) : null, |
plus a sameFile(pngWritten, pngPath) re-check beside the .ans one below.
中文说明
R1-1 仍然存在——第 1 轮之后没有修复提交（只有合并）。manifest 记录 png 身份时用的是对路径的重新 stat，而不是授信时刻已捕获的 pngWritten；并且从 freeze 写入到 manifest 写入之间完全没有 png 身份复核——而 .ans 两者都有。紧邻注释 "From the write, not from a fresh stat of the name" 与 png 这一行自相矛盾；pngWritten 只被碰撞清理读取。
故障场景：同 uid 竞速者在 freeze 写入与 manifest stat 之间替换 <out>.png → 运行以 exit 0 结束，为本非本次运行产生的字节作证 evidence: 'png'，并记录下替换者的身份；后续运行的 clear 阶段（sameFile(ids?.png, pngPath)）据此获得删除授权。
验证证据（第 2 轮，在 HEAD 上执行）：fake freeze 先写真渲染再替换 cap.png——PR 代码 exit 0、作证伪证内容；对照臂（替换 .ans）正确拒绝 exit 3；补上 png 身份复核 + rename 落盘后 5/5 翻转为真渲染。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| path === ansPath | ||
| ? sameFile(ansWritten, ansPath) | ||
| : path === pngPath | ||
| ? sameFile(pngWritten, pngPath) |
There was a problem hiding this comment.
[Critical] R1-2: Still stands. assertSameOutDir() has exactly one call site (inside writeArtifact): the freeze render spawn, the credit stats, the manifest stat, and these failure-cleanup rmSync calls never re-check directory identity — contradicting the file header's "re-checked before every artifact operation".
Failure scenario: mid-render directory swap (this PR's own threat model): freeze writes the png into the substitute directory through the redirected name; pngWritten is credited through the same swap; the .ans re-check throws exit 3 (correct); this cleanup arm then resolves pngPath by name into the substitute directory, sameFile(pngWritten, pngPath) matches the victim's file, and the cleanup deletes a file in a foreign directory. The PR's own dir-swap test passes only because its swap lands before the .ans write, so no png is ever credited — it does not exercise this shape.
Witness (round 2, executed at HEAD):
fake freeze: mv dir dir.stolen; ln -s victim dir; write png through redirect
PR code: exitCode=3, victimPngSurvives=false, realAnsSurvives=true
flip: adding dirIdOf() === outDirId to the cleanup's mine → victim intact
Fix: call assertSameOutDir() before the freeze spawn, before the credit stats, and before these cleanup rmSync calls — routing the throw through the refusal path.
中文说明
R1-2 仍然存在。assertSameOutDir() 只有一个调用点（writeArtifact 内部）：freeze 渲染 spawn、授信统计、manifest stat、以及这些失败清理的 rmSync 调用都从不复核目录身份——与文件头 "re-checked before every artifact operation" 的声明矛盾。
故障场景：渲染期间目录被掉包（本 PR 自己的威胁模型）：freeze 通过被重定向的路径把 png 写进替代目录；pngWritten 也是通过同一次掉包授信的；.ans 复核以 exit 3 拒绝（正确）；随后这条清理分支沿重定向后的路径解析 pngPath，sameFile(pngWritten, pngPath) 匹配到受害者目录里的文件，清理把它删掉。本 PR 自己的目录掉包测试之所以通过，只是因为掉包发生在 .ans 写入之前，没有任何 png 被授信——它没有覆盖这种形态。
修复：在 freeze spawn 前、授信统计前、这些清理 rmSync 前调用 assertSameOutDir()——并把抛错接入拒绝路径。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| if (!sameFile(ansWritten, ansPath)) { | ||
| throw new ArtifactCollision( |
There was a problem hiding this comment.
[Critical] R1-3: Still stands, and worsened relative to the base branch. freeze consumes .ans BY NAME during the render (freezePlan(ansPath, pngPath)), but this identity re-check runs only AFTER the render — a swap-then-restore around the render defeats it, because rename preserves ino/size/mtime exactly. The base branch had added a nonce-staged render (commit d2a57afb82, linkSync input stage) as the fix for this class; the PR's merge resolution deleted it — grep at HEAD finds zero staging machinery.
Failure scenario: a daemonized descendant (the survivor class this file documents it does not reap) renames cap.ans aside before freeze opens it and puts attacker content at the name → freeze renders attacker bytes into cap.png, the credit gate passes, and the original is renamed back before this check — sameFile(ansWritten, ansPath) passes. The run exits 0 with evidence: 'png'. Swap window = freeze probe + spawn (seconds); restore window = the entire render — no syscall-interval race needed, contrary to the header's residue claim.
Witness (round 2, executed at HEAD):
fake freeze: swap .ans aside, render attacker content, restore original
PR code: exit 0, evidence='png', png = 'PNG-RENDERED-FROM: ATTACKER-ANSI-CONTENT'
flip: staged input (linkSync) renders only the genuine bytes
committed base test 'stages the render INPUT under a nonce' fails at HEAD
Fix: render from an fd-pinned private copy: after the .ans write, open ansPath O_NOFOLLOW, fstat-verify against ansWritten, copy through the descriptor to an unpredictable path, and pass that to freezePlan (or restore the base's staging and harden it). A pre-render check alone does not close it.
中文说明
R1-3 仍然存在，且相对 base 分支恶化。freeze 在渲染期间按名字读取 .ans（freezePlan(ansPath, pngPath)），而这条身份复核只在渲染之后运行——围绕渲染做"换走再换回"即可绕过，因为 rename 精确保留 ino/size/mtime。base 分支曾添加 nonce 分阶段渲染（提交 d2a57afb82，linkSync 输入暂存）作为这一类的修复；本 PR 的合并取舍把它删掉了——HEAD 上 grep 不到任何暂存机制。
故障场景：守护化的后代进程（本文件明确声明不会收割的一类）在 freeze 打开前把 cap.ans 改名挪走、放入攻击者内容 → freeze 把攻击者字节渲染进 cap.png，授信门通过，原文件在此复核前被改名换回——sameFile(ansWritten, ansPath) 通过。运行以 exit 0 结束并作证 evidence: 'png'。换走窗口 = freeze 探测 + spawn（秒级）；换回窗口 = 整个渲染期——无需任何系统调用级竞态，与文件头声明的残留不符。
修复：从 fd 固定的私有副本渲染：.ans 写入后以 O_NOFOLLOW 打开 ansPath，用 fstat 对照 ansWritten 验证，通过描述符复制到一个不可预测路径，再把它传给 freezePlan（或恢复 base 的暂存并加固）。只在渲染前检查无法关闭此窗口。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // fast enough to land inside that interval — not one with the whole | ||
| // capture window to work in, which is what the guards above took away. |
There was a problem hiding this comment.
[Critical] R1-4: Still stands, in both offered forms. There is still no content binding — grep for digest|createHash|sha256 in the file at HEAD returns nothing; ownership is the {ino, size, mtimeMs} triple alone, and the artifacts are owner-writable: a same-uid actor IS the owner. An in-place overwrite + ns-exact mtime restore passes every identity gate this PR adds, without winning any syscall-interval race. This residue paragraph still does not name that shape — it claims what remains is "a racer fast enough to land inside that interval", but the in-place-overwrite shape needs no interval at all, so it contradicts the stated residue rather than being covered by it.
Failure scenario: during the render window the actor overwrites cap.ans in place (ino preserved) with doctored content padded to the recorded size, then restores the original mtime via utimensat (ns-exact round-trip) → sameFile passes on all three components and the manifest attests content this run never produced, exit 0.
Witness (round 2, executed at HEAD):
fake freeze: overwrite in place, pad to recorded size, restore mtime
PR code: exit 0, evidence='png', .ans begins 'DOCTORED'
flip: content-binding re-check against ansText refuses exit 3
round 1: mtime round-trip 1786896873655.4246 === 1786896873655.4246
Fix: bind content, not just metadata: compute a content digest THROUGH the write descriptor (before closeSync(fd) in writeArtifact) and record it in artifacts, comparing before crediting or clearing. If content binding is declined for this PR, state the in-place-overwrite shape explicitly in the residue instead of claiming only interval racers remain.
中文说明
R1-4 仍然存在，两种修复形式都未落实。仍然没有任何内容绑定——HEAD 上在文件内 grep digest|createHash|sha256 一无所获；所有权只是 {ino, size, mtimeMs} 三元组，而工件对属主可写：同 uid 竞速者就是属主。原地覆写 + 纳秒级精确 mtime 还原可以通过本 PR 新增的所有身份门，完全不需要赢得任何系统调用级竞态。这段残留声明仍然没有点名这一形态——它声称剩下的只是"快到能落进那个间隙的竞速者"，但原地覆写形态根本不需要任何间隙，因此与声明的残留相矛盾而非被其覆盖。
修复：绑定内容而非仅元数据：在 writeArtifact 的 closeSync(fd) 之前通过写描述符计算内容摘要并记入 artifacts，在授信/清理前比对。如果本 PR 不做内容绑定，请在残留声明中明确写出"原地覆写"这一形态，而不是声称只剩间隙竞速。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // asks this stamp instead, so "did THIS run produce the image" is decided | ||
| // against a closed window with only the render able to have changed it. | ||
| pngStamp = stampOf(pngPath); |
There was a problem hiding this comment.
[Critical] R1-5: Still stands. This re-stamp is still the LAST occupancy decision on <out>.png before the write: the freeze probe spawn and the render spawn both run after it, with no lstatSync(pngPath) between them and freeze's by-name open of the output. The header's "occupancy is re-decided at write time, the artifact opens are atomic and do not follow symlinks" remains unscoped for the one artifact this command does not open itself. The base's staged-output rename landing — which replaced planted links instead of following them — was deleted by this merge.
Failure scenario: the re-stamp finds <out>.png absent; between that stamp and freeze's open — signal-triggerable via the probe process in /proc, no microsecond timing — a racer runs ln -s /victim/file cap.png → freeze follows the link and truncates the victim's file, writing render bytes through it. The credit-side guard records ans-only (correct attestation), but the victim's file is already destroyed. The hardlink variant (ln victim cap.png) is even credited as evidence: 'png', laundering the destroyed file's identity into artifacts.png.
Witness (round 2, executed at HEAD with real freeze v0.2.2, planted through the probes.freeze seam):
hardlink arm: victim afterwards holds PNG bytes [137,80,78,71]
run exits 0, evidence='png', manifest.artifacts.png.ino === victim's ino
flip: staged output + rename landing leaves the victim untouched
Fix: re-decide occupancy at the spawn boundary: move probes.freeze() ahead of this re-stamp and re-lstatSync(pngPath) as the last statement before spawnSync, degrading if occupied. Also scope the header's atomic-open claim to the opens this command performs, stating the png is written by freeze under credit-refusal-only protection.
中文说明
R1-5 仍然存在。这次重新取样仍然是对 <out>.png 写入前的最后一次占用裁定：freeze 探测 spawn 与渲染 spawn 都在它之后运行，两者与 freeze 按名打开输出之间没有任何 lstatSync(pngPath)。文件头 "occupancy is re-decided at write time, the artifact opens are atomic and do not follow symlinks" 的声明，对唯一不由本命令自己打开的工件仍未加限定。base 的输出暂存 + rename 落盘（替换而非跟随植入的链接）已被本次合并删除。
故障场景：重新取样发现 <out>.png 不存在；在该取样与 freeze 打开之间——可通过 /proc 中的探测进程发信号触发、无需微秒级时机——竞速者执行 ln -s /victim/file cap.png → freeze 跟随链接截断受害者文件并把渲染字节写穿过去。授信侧守卫记录 ans-only（作证正确），但受害者文件已被销毁。硬链接变体甚至被授信为 evidence: 'png'，把被销毁文件的身份洗进 artifacts.png。
修复：在 spawn 边界重新裁定占用——把 probes.freeze() 移到这次重新取样之前，并把 lstatSync(pngPath) 作为 spawnSync 前最后一条语句，占用则降级。同时把文件头的原子打开声明限定到本命令自己执行的打开，并注明 png 由 freeze 写入、只有授信拒绝一层保护。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const dirIdOf = (): string => { | ||
| try { | ||
| const st = lstatSync(outDir); |
There was a problem hiding this comment.
[Critical] R1-7: Still stands — escalated from Suggestion this round on new executed end-to-end evidence. dirIdOf pins the directory by lstatSync(outDir), which does not follow the final component: when the last component of the --out parent is a symlink (the common latest/current pattern, symlinked homes/workspaces), the pin tracks the LINK's identity, not the resolved directory. resolve() is lexical. Swapping the symlink's TARGET evades every assertSameOutDir() for the entire capture window — up to an hour — with no micro-race at all.
Failure scenario: with --out /home/u/proj/report where proj -> /data/proj, a same-uid racer (the captured command itself) runs mv /data/proj /data/proj.bak && mkdir /data/proj as its first act — lstatSync(outDir) still reports the unchanged link, every assertSameOutDir() passes, all three artifacts are redirected into the racer's fresh directory, and the manifest attests the original paths over content the racer controls: the exact wrong-evidence outcome this command exists to prevent.
Witness (round 2, executed end-to-end with real tmux):
ARM A (direct mid-window swap, the PR's own shape): refused exit 3 as designed
ARM B (symlink-target swap): exitCode undefined — FULL SUCCESS
link identity before/after: 66307:52757 66307:52757 (unchanged)
racer's NEW dir holds cap.ans/cap.json/cap.png; ORIGINAL dir empty
manifest ansPath attests the original path
flip: statSync (re-resolves the full path) → ARM B refused; adjacent dir tests green 2/2
| const dirIdOf = (): string => { | |
| try { | |
| const st = lstatSync(outDir); | |
| const dirIdOf = (): string => { | |
| try { | |
| const st = statSync(outDir); |
中文说明
R1-7 仍然存在——本轮依据新执行的端到端证据从 Suggestion 升级为 Critical。dirIdOf 用 lstatSync(outDir) 固定目录身份，它不跟随最后一个路径组件：当 --out 父路径的最后一个组件是符号链接（常见的 latest/current 模式、符号链接化的 home/工作区）时，固定的是链接本身的身份，而不是路径解析到的目录。resolve() 是词法解析。掉包符号链接的目标即可在整个采集窗口内（最长一小时）绕过所有 assertSameOutDir()，完全不需要任何微竞态。
故障场景：--out /home/u/proj/report 且 proj -> /data/proj 时，同 uid 竞速者（即被采集命令本身）第一步执行 mv /data/proj /data/proj.bak && mkdir /data/proj——lstatSync(outDir) 仍报告未改变的链接，所有 assertSameOutDir() 通过，三个工件全部被重定向进竞速者的新目录，而 manifest 为竞速者控制的内容作证原始路径：正是本命令本要防止的伪证结局。
修复：lstatSync → statSync（每次调用都重新解析完整路径，固定解析后的目录）；翻转验证后本 PR 相邻的目录测试仍为绿色。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const ids = (m as { artifacts?: unknown }).artifacts as | ||
| | { ans?: unknown; png?: unknown } | ||
| | undefined; | ||
| if (shaped && !sameFile(ids?.ans, ansPath)) shaped = false; |
There was a problem hiding this comment.
[Critical] R1-10: Still stands — escalated from Suggestion this round on new test evidence. The new artifacts identity rung is mandatory for clearing: a manifest without it gets shaped = false (sameFile(undefined, ansPath) is false), nothing is cleared, and the collision gate refuses. There is no signature-only fallback for the entire existing population of manifests this same command wrote before this PR — even though a re-used --out is the documented ordinary condition (the first one listed in the header guarantee).
Failure scenario: any --out written by a previous build carries signature + ansPath + settledBy but no artifacts field. The first post-upgrade run against it clears nothing and refuses --out collides with a file this capture did not write … A previous capture's own artifacts are cleared automatically. Pick another --out — for exactly the artifacts it is NOT clearing. Every re-run refuses until the user deletes them by hand. The diff's own fixture comment shows the author knew a missing rung makes the clear refuse, and updated the fixtures — but gave the production population no migration.
Witness (round 2, executed at HEAD):
committed test 'a manifest whose pngPath names ANOTHER file cannot authorize
clearing <out>.png' (fixture carries all signature rungs, no artifacts rung):
FAIL — received 'capture-tui: refused — --out collides with a file this capture
did not write: …/cap.ans. A previous capture's own artifacts are cleared
automatically; this one is not ours to replace. Pick another --out.'
Fix: accept the legacy signature shape when artifacts is absent (enforce sameFile only when the rung exists), while continuing to write the rung.
中文说明
R1-10 仍然存在——本轮依据新的测试证据从 Suggestion 升级为 Critical。新增的 artifacts 身份层级是清理的强制条件：没有它的 manifest 会得到 shaped = false（sameFile(undefined, ansPath) 为 false），什么都不清理，碰撞门直接拒绝。对于本命令在本 PR 之前写下的全部现存 manifest，没有任何仅凭签名的回退——尽管复用 --out 是文件头担保中列出的第一种普通情况。
故障场景：任何由旧版本写出的 --out 都带签名 + ansPath + settledBy 但没有 artifacts 字段。升级后第一次对它的运行什么都不清理，并以 "--out collides with a file this capture did not write … Pick another --out" 拒绝——拒绝的恰恰是它本应清理的工件。在用户手动删除之前，每次重跑都拒绝。diff 自己的 fixture 注释表明作者知道缺少该层级会让清理拒绝，于是更新了 fixture——但没有给生产存量任何迁移。
修复：artifacts 缺失时接受旧的签名形态（只在该层级存在时强制 sameFile），同时继续写入该层级。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| } catch { | ||
| // Same. | ||
| } | ||
| if (changed(pngPath, pngStamp)) rmSync(pngPath, { force: true }); |
There was a problem hiding this comment.
[Critical] R2-2: The failed-render cleanup unlinks whatever occupies pngPath by name. The render branch of the ladder is only reachable when pngStamp.existed === false, so changed(pngPath, pngStamp) takes its absent-stamp arm and degenerates to occupied(path) — it answers "something is there now", never "freeze wrote this". The base code spared occupants here (degradation entry "left in place") and only ever removed its own nonce-named stage; this rewrite removed that. The added comment's claim "Only when the clear phase left nothing there" is not what the gate implements.
Failure scenario: the re-stamp sees no <out>.png; during the blocking freeze probe (10 s budget) or render spawn (30 s timeout) a concurrent actor on the same --out — the exact shape this block's own comment acknowledges ("a concurrent actor on the same --out, the very shape the clear phase and collision gate exist to handle") — creates <out>.png; freeze then fails without opening the output (EMFILE at spawn, belt kill, non-zero exit). changed() → true and rmSync deletes the actor's file; nothing records the deletion — the manifest says only "freeze failed … no image rendered" and exits 0, against the header guarantee "never deletes a file it cannot show a previous run of THIS command wrote".
Witness (round 2, executed at HEAD):
probe: planter placed 'FOREIGN-PNG' at cap.png 0.3s into a 3s fake-freeze window;
freeze exited 9 → capPngExistsAfter: false; degradedBecause names only the freeze failure
flip: occupant-sparing shape (degradation entry, no rmSync) preserves 'FOREIGN-PNG' byte-for-byte
committed test 'leaves a TORN png in place when the render fails' FAILS at HEAD:
ENOENT reading cap.png — 'expect(readFileSync(join(dir, 'cap.png')))…'
Fix: do not rmSync an unattributable occupant: restore the base's spare-and-degrade behavior here (degradation entry, no rmSync), or restore the nonce-staged render so removable bytes carry a run-private name.
中文说明
渲染失败的清理会按名字删除 pngPath 上的任何占用者。渲染分支只有在 pngStamp.existed === false 时可达，因此 changed(pngPath, pngStamp) 走"不存在"分支退化为 occupied(path)——它回答的是"现在那里有东西"，而不是"freeze 写了它"。base 代码在这里保留占用者（降级记录 "left in place"），只删除自己 nonce 命名的暂存；本次重写删掉了这一点。新增注释声称 "Only when the clear phase left nothing there"，但这个门实现的并非此事。
故障场景：重新取样看到 <out>.png 不存在；在阻塞式的 freeze 探测（10 秒预算）或渲染 spawn（30 秒超时）期间，同一 --out 上的并发者——正是本代码块注释自己承认的形态——创建了 <out>.png；随后 freeze 未打开输出即失败（spawn 时 EMFILE、belt 杀掉、非零退出）。changed() → true，rmSync 删掉对方的文件；删除没有任何记录——manifest 只说 "freeze failed … no image rendered" 并 exit 0，违背文件头 "never deletes a file it cannot show a previous run of THIS command wrote" 的担保。
修复：不要 rmSync 无法归属的占用者：恢复 base 在此处的"保留并降级"行为（降级记录、不 rmSync），或恢复 nonce 分阶段渲染，让可删除的字节带上仅本运行可知的名字。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // pointing at the user's untouched file, no degradation recorded). | ||
| changed(pngPath, pngStamp) |
There was a problem hiding this comment.
[Critical] R2-3: The credit gate's changed(pngPath, pngStamp) is the same existence-only check: with the absent post-window stamp it reduces to occupied(), so the condition this comment describes — "THIS run is what put those bytes there" — is not what is decided. And the scenario the comment claims to fix (a file the clear phase spared + freeze exiting 0 without writing) cannot even reach this branch, because pngStamp.existed stops the ladder earlier; the only occupants present at credit time appeared DURING the render window. The base's private nonce stage made credit attribution sound; the merged public-name gate cannot attribute.
Failure scenario: a concurrent same-uid actor (or a survivor of the captured command) places a regular file at <out>.png during the freeze window while freeze exits 0 without writing — the shape this comment itself says was "measured end to end". All four gates pass (r.status === 0, lstatSync().isFile(), pngSize > 0, changed ≡ occupied) → evidence: 'png' is credited, pngWritten records the planted file's identity, and the manifest attests foreign bytes as this run's rendering — the wrong-evidence outcome this command exists to prevent. The recorded identity then propagates clear authority for the file to the next run.
Witness (round 2, executed at HEAD):
probe: fake freeze 'sleep 2; exit 0' (writes nothing); planter placed
'PLANTED-BYTES' (13 bytes) at cap.png 0.3s into the window
PR code: stdout {"captured":true,"evidence":"png"}, no degradedBecause,
manifest.artifacts.png = { ino: 55323, size: 13 } — the planted file's identity certified
flip: refuse the rung when the occupant cannot be bound → evidence 'ans-only', artifacts.png null
Fix: credit only what this run can prove it wrote: restore the nonce-staged render / rename landing (credit against the run-private stage), or refuse the png rung whenever the occupant cannot be bound to this run's render.
中文说明
授信门的 changed(pngPath, pngStamp) 是同样的"只看存在性"检查：在窗口后取样为"不存在"的前提下它退化为 occupied()，因此注释所描述的条件——"THIS run is what put those bytes there"——并不是实际裁定的内容。而且注释声称要修复的场景（clear 阶段保留的文件 + freeze exit 0 未写入）根本到不了这个分支，因为 pngStamp.existed 会更早地停下梯子；授信时刻存在的占用者只能是在渲染窗口期间出现的。base 的私有 nonce 暂存让授信归属可靠；合并后的按名公开门无法归属。
故障场景：同 uid 并发者（或被采集命令的幸存者）在 freeze 窗口内把一个普通文件放到 <out>.png，而 freeze exit 0 且未写入——正是注释自己说"端到端实测过"的形态。四道门全部通过 → 授信 evidence: 'png'，pngWritten 记录植入文件的身份，manifest 把外来字节作证为本运行的渲染——正是本命令本要防止的伪证结局。记录的身份还会把该文件的清理授权传递给下一次运行。
修复：只授信本运行能证明是自己写的东西：恢复 nonce 分阶段渲染 / rename 落盘（对运行私有暂存授信），或在占用者无法绑定到本次渲染时拒绝 png 层级。
— qwen3.8-max via Qwen Code /review (v0.21.15)
# Conflicts: # packages/cli/src/commands/review/cleanup.test.ts # packages/cli/src/commands/review/cleanup.ts
…to HEAD # Conflicts: # packages/cli/src/commands/review/capture-tui.ts # packages/cli/src/commands/review/cleanup.test.ts # packages/cli/src/commands/review/cleanup.ts
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): capdir/cap.ans — no such file or directory.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/capture-tui.ts:1333 — [probe] both-bases socket unlink shares one try/catch — EACCES on the first base skips the /tmp unlink (code-age: unchanged since round 2)packages/cli/src/commands/review/capture-tui.ts:45 — [review] header caps the race window at an hour; body comments and yargs help say 70 minutes (timeout-ms + settle-ms) (code-age: unchanged since round 2)packages/cli/src/commands/review/capture-tui.test.ts:2638 — [review] added comment claims the REPLACED test sits in the real-tmux describe; it sits in the tmux-free describe (code-age: unchanged since round 2)packages/cli/src/commands/review/capture-tui.ts:459 — [probe] dirIdOf collapses every lstatSync failure into 'gone' — a transient errno becomes a false 'directory replaced' refusal (code-age: unchanged since round 2)packages/cli/src/commands/review/capture-tui.test.ts:2532 — [probe] oversize-manifest cap fixture missed by the artifacts-rung treatment — the reader-cap pin no longer discriminates cap removal (code-age: unchanged since round 2)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：capdir/cap.ans — no such file or directory。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| isNothingToKill, | ||
| isSocketDirNeverCreated, | ||
| isSocketDirUnusable, | ||
| isSocketPathAbsent, | ||
| verdictExaminedBase, | ||
| validGeometry, | ||
| type ArtifactId, |
There was a problem hiding this comment.
[Critical] R2-1: Still stands — re-measured at this round's HEAD. The committed suite is red: capture-tui.test.ts 25 failed | 139 passed | 1 skipped; cleanup.test.ts 3 failed | 92 passed. The merge resolution deleted the base branch's probe-verified defenses while keeping the base branch's tests that pin them — the removed defenses exist intact in merge parent HEAD^2. Every failure maps 1:1 onto a removed behavior whose deleted comment recorded the harm as probe-reproduced: nonce-staged render + rename landing (7 failing tests), UTF-8-byte pretty-shape manifest size gate (2), the errTail .slice(0, 2048) cap (1, observed 3,145,792 bytes against the 8,192 assertion), per-base env-pinned kills + base-scoped verdict crediting + stamped socket identity in the reap (6), canonical realpathSync socket measure (2), hostStateFor ENFILE/EIO/ESTALE arms (3), TMPDIR directoryness check + sentinel-rmSync belt (1), --no-keys refusal wording (1), -T capture-trims caveat (1), the cleanup.ts orphan-sweep hoist + reaps kept off removedAny (3). Also reverted without a pinning test: the probeOutput ENOBUFS arm (a spawned probe that out-spews maxBuffer now classifies as spawned:false) and the start-threw-before-bind socket-absent credit (false orphan WARNING). Note: the redirect-refusal strand of the sweep relocation is pinned by NO test — cleanup.test.ts:1478 passes with zero reap assertions — so the "justify each deletion" branch must address it explicitly. Fix: redo the merge preserving the base side's defenses (the sweep restored above ALL early returns, including the redirectedAncestor refusals), or justify each deletion and delete/update the corresponding committed tests in this PR.
中文说明
R2-1：仍然存在——本轮在 HEAD 重新实测。已提交的测试套件是红的：capture-tui.test.ts 25 失败 | 139 通过 | 1 跳过；cleanup.test.ts 3 失败 | 92 通过。合并取舍删掉了 base 分支上经过探测验证的防御，却保留了 base 分支钉住这些行为的测试——被删除的防御在合并父 HEAD^2 中完整存在。每个失败都与本 diff 删除的某个行为一一对应，且被删注释记录过该危害是探测复现过的：nonce 暂存渲染 + rename 落盘（7 个失败测试）、按美化输出形态 UTF-8 字节计量的 manifest 大小门（2 个）、errTail 的 .slice(0, 2048) 上限（1 个，实测 3,145,792 字节对 8,192 断言）、reap 中按 base 固定环境的 kill + 按 base 授信的判决 + 带取样的 socket 身份（6 个）、规范化 realpathSync socket 计量（2 个）、hostStateFor 的 ENFILE/EIO/ESTALE 分支（3 个）、TMPDIR 门目录性检查 + 哨兵 rmSync 保护带（1 个）、--no-keys 拒绝措辞（1 个）、-T 捕获裁剪告警（1 个）、cleanup.ts 孤儿清扫前置 + reap 不计入 removedAny（3 个）。另有两处无测试钉住的还原：probeOutput 的 ENOBUFS 分支（探测二进制运行了但输出超过 maxBuffer 现在被归类为 spawned:false）与绑定前启动失败的 socket 缺失授信（虚假孤儿 WARNING）。注意：清扫重定位的符号链接重定向拒绝分支没有任何测试钉住——cleanup.test.ts:1478 通过且不含任何 reap 断言——因此若选择"逐条论证删除"的解决路径必须显式处理该分支。修复：重做合并并保留 base 侧的防御（清扫恢复到所有早退之前，包括 redirectedAncestor 拒绝路径），或者逐条论证每个删除并删除/更新对应的已提交测试。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| ans: ansWritten as ArtifactId, | ||
| png: png ? idOf(stampOf(pngPath)) : null, |
There was a problem hiding this comment.
[Critical] R1-1: Still stands — the manifest records the png identity from a fresh by-name stat (png: png ? idOf(stampOf(pngPath)) : null) instead of the credit-time pngWritten (:1744), and no png identity re-check runs anywhere between the freeze write and the manifest write — while the .ans side gets both (write-time identity + the sameFile(ansWritten, ansPath) re-check at :1839). The adjacent comment "From the write, not from a fresh stat of the name" contradicts the png line. Failure scenario: freeze writes the png, the credit checks pass, a same-uid racer replaces <out>.png between freeze's write and the manifest write → the manifest records the replacement's identity with evidence: 'png', and the next run's clear phase matches it and deletes the racer's file — the manifest's authority extended to an artifact it never produced. Witness (executed scratch-tree probe): racer swap during the freeze window lands in the manifest (artifacts.png ino == racer file ino, content 'RACER-CONTROLLED-BYTES', exit success); the next run deleted the racer file. Fix: png: png ? pngWritten : null plus a sameFile(pngWritten, pngPath) re-check before the manifest write, as the .ans does. (This narrows but does not close the wider freeze-write→credit window; full closure is the nonce-staged landing from R2-1.)
中文说明
R1-1：仍然存在——manifest 用一次按名的新鲜 stat（png: png ? idOf(stampOf(pngPath)) : null）记录 png 身份，而不是授信时刻的 pngWritten（:1744），并且在 freeze 写入与 manifest 写入之间没有任何 png 身份复查——而 .ans 两者都有（写入时身份 + :1839 的 sameFile(ansWritten, ansPath) 复查）。紧邻的注释"来自写入，而非按名的新鲜 stat"与 png 这行相矛盾。故障场景：freeze 写出 png、授信检查通过后，同 uid 竞速者在 freeze 写入与 manifest 写入之间掉包 <out>.png → manifest 以 evidence: 'png' 记录掉包文件的身份，下一次运行的清理阶段匹配该身份并删除竞速者的文件——manifest 的授权被扩展到它从未产生的工件。证据（已执行的暂存树探测）：渲染窗口内的掉包落入 manifest（artifacts.png 的 ino == 竞速者文件的 ino，内容 'RACER-CONTROLLED-BYTES'，退出成功）；下一次运行删除了竞速者的文件。修复：改为 png: png ? pngWritten : null，并在 manifest 写入前增加 sameFile(pngWritten, pngPath) 复查，与 .ans 一致。（这收窄但不能完全关闭更宽的 freeze 写入→授信窗口；彻底关闭需要 R2-1 的 nonce 暂存落盘。）
— qwen3.8-max via Qwen Code /review (v0.21.15)
| path === ansPath | ||
| ? sameFile(ansWritten, ansPath) | ||
| : path === pngPath | ||
| ? sameFile(pngWritten, pngPath) |
There was a problem hiding this comment.
[Critical] R1-2: Still stands — assertSameOutDir() has exactly one call site (inside writeArtifact, :564): the freeze render spawn, the credit stats, the manifest stat, and these failure-cleanup rmSync calls never re-check directory identity — contradicting the file header's "re-checked before every artifact operation". Failure scenario: a directory swap bracketing the render — after the last assertSameOutDir and before the credit/manifest checks — plants attacker bytes that the manifest then credits, while every check passes. Witness (executed probe): directory swap after the .ans write + attacker-bytes plant in the stolen directory, restored before credit → the manifest credits evidence: 'png' for 'ATTACKER-EVIDENCE' bytes with exit success; grep at HEAD confirms the single call site. Fix: re-check assertSameOutDir() before the freeze render spawn, the credit stats, the manifest write, and the failure-cleanup unlinks — or state the actual coverage in the header instead of "before every artifact operation".
中文说明
R1-2：仍然存在——assertSameOutDir() 只有一个调用点（writeArtifact 内部 :564）：freeze 渲染启动、授信 stat、manifest stat、以及这些故障清理 rmSync 调用都不复查目录身份——与文件头"每次工件操作前都复查"矛盾。故障场景：包围渲染的目录掉包——在最后一次 assertSameOutDir 之后、授信/manifest 检查之前——植入攻击者字节，manifest 随后为其作证，而所有检查全部通过。证据（已执行探测）：.ans 写入后的目录掉包 + 在被偷走的目录中植入攻击者字节、授信前还原 → manifest 以 evidence: 'png' 为 'ATTACKER-EVIDENCE' 字节作证，退出成功；HEAD 上 grep 确认唯一调用点。修复：在 freeze 渲染启动、授信 stat、manifest 写入、故障清理 unlink 之前都复查 assertSameOutDir()——或者在文件头如实写明实际覆盖范围，而不是"每次工件操作前"。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| if (!sameFile(ansWritten, ansPath)) { | ||
| throw new ArtifactCollision( |
There was a problem hiding this comment.
[Critical] R1-3: Still stands, worsened relative to the base branch — freeze consumes .ans BY NAME during the render (freezePlan(ansPath, pngPath) at :1694), but this identity re-check runs only AFTER the render; a swap-then-restore around the render defeats it, because rename preserves ino/size/mtime exactly. The base branch's nonce-staged render (linkSync input stage pinned by identity + nonce output + renameSync landing) was removed by this diff. Witness: all four pinning committed tests fail at HEAD — 'stages the render INPUT under a nonce — freeze never reads the .ans by name' (expected 'ans-only' to be 'png'), 'refuses a symlink swapped in for the .ans before staging' (expected 3 to be undefined), 'does not land a render THROUGH a symlink planted during the render window' (expected 'PNG-BYTES' to be 'VICTIM-CONTENT'), 'a stage replaced by a directory during the render does not mask the capture result' (expected 3 to be undefined). Fix: restore the nonce staging — hard-link the input under a per-run name pinned by identity, render to a nonce output, land by rename (which replaces a planted link instead of following it).
中文说明
R1-3：仍然存在，且相对 base 分支恶化——freeze 在渲染期间按名消费 .ans（:1694 的 freezePlan(ansPath, pngPath)），而该身份复查只在渲染之后运行；包围渲染的掉包再还原即可绕过，因为 rename 完整保留 ino/size/mtime。base 分支的 nonce 暂存渲染（按身份钉住的 linkSync 输入暂存 + nonce 输出 + renameSync 落盘）被本 diff 删除。证据：四个钉住测试在 HEAD 全部失败——'stages the render INPUT under a nonce'（期望 'png' 得到 'ans-only'）、'refuses a symlink swapped in for the .ans before staging'（期望 3 得到 undefined）、'does not land a render THROUGH a symlink planted during the render window'（期望 'VICTIM-CONTENT' 得到 'PNG-BYTES'）、'a stage replaced by a directory during the render'（期望 3 得到 undefined）。修复：恢复 nonce 暂存——把输入按运行内唯一名字硬链接并按身份钉住，渲染到 nonce 输出，用 rename 落盘（替换植入的链接而不是跟随它）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // on its heels cannot be closed from here. What that leaves is a racer | ||
| // fast enough to land inside that interval — not one with the whole | ||
| // capture window to work in, which is what the guards above took away. |
There was a problem hiding this comment.
[Critical] R1-4: Still stands — there is still no content binding (grep for digest|createHash|sha256 at HEAD: zero matches), and the artifacts are owner-writable: a same-uid actor IS the owner. An in-place overwrite of cap.ans (ino preserved) padded to the recorded size, plus an ns-exact mtime restore via utimensat, passes every identity gate this PR adds — without winning any syscall-interval race. This residue paragraph claims only interval racers remain; the in-place-overwrite shape needs no interval at all, so it contradicts the stated residue. Witness (executed flip probe at HEAD): BASE — exit success, .ans content doctored ('FAKE-' where the capture wrote 'MARK'), manifest credited; FIXED (sha256 digest bound through the write descriptor, scratch tree only) — exit 3 'cap.ans was replaced after this capture wrote it'; the identity triple round-trips bit-identical after overwrite+restore. Fix: bind content, not just metadata — compute a content digest THROUGH the write descriptor (before closeSync(fd) in writeArtifact) and record it in artifacts, comparing before crediting or clearing. If content binding is declined for this PR, state the in-place-overwrite shape explicitly in the residue.
中文说明
R1-4：仍然存在——仍然没有任何内容绑定（HEAD 上 grep digest|createHash|sha256：零匹配），且工件对属主可写：同 uid 竞速者就是属主。原地覆写 cap.ans（ino 不变）、内容填充到记录大小，再用 utimensat 做纳秒级精确 mtime 还原，即可通过本 PR 新增的所有身份门——完全不需要赢得任何系统调用间隙竞态。这段残留声明只剩间隙竞速者；原地覆写形态根本不需要任何间隙，因此与声明的残留相矛盾。证据（HEAD 上已执行的翻转探测）：BASE——退出成功，.ans 内容被篡改（采集写入 'MARK' 变为 'FAKE-'），manifest 授信；修复（通过写描述符绑定 sha256 摘要，仅在暂存树）——退出 3 '该文件在本采集写入后被替换'；身份三元组在覆写+还原后逐位相同。修复：绑定内容而非仅元数据——在 writeArtifact 的 closeSync(fd) 之前通过写描述符计算内容摘要并记入 artifacts，在授信/清理前比对。如果本 PR 不做内容绑定，请在残留声明中明确写出原地覆写形态。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| expect(p).toContain('src/caller.ts (imports src/changed.ts)'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R3-4 (new this round, 1 of 5 locations): the merge commit adds a trailing blank line at EOF of this file — it now fails npx prettier --check (executed at HEAD: exit 1) while the base-branch version passes the same check. Failure scenario: npm run format / npm run preflight run prettier --write . and silently rewrite this committed file, leaving a dirty diff against HEAD for anyone who runs them. The same shape exists in four more files: lib/agent-briefs.ts (EOF), cleanup.test.ts (two blank lines at EOF), cleanup.ts (three blank lines at EOF), and capture-tui.ts (EOF); both merge parents end cleanly, so the blank lines were introduced by the reviewed merge itself. Fix: delete the added trailing blank line(s) so each file ends with the single newline after its last code line.
中文说明
R3-4（本轮新发现，共 5 处之 1）：合并提交在本文件 EOF 处添加了多余空行——现在 npx prettier --check 失败（HEAD 上已执行：退出 1），而 base 分支版本通过同样的检查。故障场景：npm run format / npm run preflight 运行 prettier --write . 时会静默重写这个已提交文件，给任何运行它们的人留下相对 HEAD 的脏 diff。同样的形态还出现在另外四个文件：lib/agent-briefs.ts（EOF）、cleanup.test.ts（EOF 两个空行）、cleanup.ts（EOF 三个空行）、capture-tui.ts（EOF）；两个合并父都以干净的单换行结尾，因此空行是被审查的合并本身引入的。修复：删除添加的末尾空行，使每个文件以最后一个代码行后的单个换行结尾。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| export function readsDiff(role: RoleId): boolean { | ||
| return BRIEFS[role].readsDiff; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R3-4 (new this round, 2 of 5 locations): the merge commit adds a trailing blank line at EOF of this file — it now fails npx prettier --check (executed at HEAD: exit 1) while the base-branch version passes the same check. Failure scenario: npm run format / npm run preflight run prettier --write . and silently rewrite this committed file, leaving a dirty diff against HEAD for anyone who runs them. Sibling locations: agent-prompt.test.ts, cleanup.test.ts, cleanup.ts, capture-tui.ts (all EOF). Fix: delete the added trailing blank line so the file ends with the single newline after its last code line.
中文说明
R3-4（本轮新发现，共 5 处之 2）：合并提交在本文件 EOF 处添加了多余空行——现在 npx prettier --check 失败（HEAD 上已执行：退出 1），而 base 分支版本通过同样的检查。故障场景：npm run format / npm run preflight 运行 prettier --write . 时会静默重写这个已提交文件，给任何运行它们的人留下相对 HEAD 的脏 diff。同批位置：agent-prompt.test.ts、cleanup.test.ts、cleanup.ts、capture-tui.ts（均为 EOF）。修复：删除添加的末尾空行，使文件以最后一个代码行后的单个换行结尾。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| }); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R3-4 (new this round, 3 of 5 locations): the merge commit adds TWO trailing blank lines at EOF of this file — it now fails npx prettier --check (executed at HEAD: exit 1) while the base-branch version passes the same check. Failure scenario: npm run format / npm run preflight run prettier --write . and silently rewrite this committed file, leaving a dirty diff against HEAD for anyone who runs them. Sibling locations: agent-prompt.test.ts, lib/agent-briefs.ts, cleanup.ts, capture-tui.ts (all EOF). Fix: delete the added trailing blank lines so the file ends with the single newline after its last code line.
中文说明
R3-4（本轮新发现，共 5 处之 3）：合并提交在本文件 EOF 处添加了两个多余空行——现在 npx prettier --check 失败（HEAD 上已执行：退出 1），而 base 分支版本通过同样的检查。故障场景：npm run format / npm run preflight 运行 prettier --write . 时会静默重写这个已提交文件，给任何运行它们的人留下相对 HEAD 的脏 diff。同批位置：agent-prompt.test.ts、lib/agent-briefs.ts、cleanup.ts、capture-tui.ts（均为 EOF）。修复：删除添加的末尾空行，使文件以最后一个代码行后的单个换行结尾。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| }, | ||
| }; |
There was a problem hiding this comment.
[Suggestion] R3-4 (new this round, 4 of 5 locations): the merge commit adds THREE trailing blank lines at EOF of this file — it now fails npx prettier --check (executed at HEAD: exit 1) while the base-branch version passes the same check. Failure scenario: npm run format / npm run preflight run prettier --write . and silently rewrite this committed file, leaving a dirty diff against HEAD for anyone who runs them. Sibling locations: agent-prompt.test.ts, lib/agent-briefs.ts, cleanup.test.ts, capture-tui.ts (all EOF). Fix: delete the added trailing blank lines so the file ends with the single newline after its last code line.
中文说明
R3-4（本轮新发现，共 5 处之 4）：合并提交在本文件 EOF 处添加了三个多余空行——现在 npx prettier --check 失败（HEAD 上已执行：退出 1），而 base 分支版本通过同样的检查。故障场景：npm run format / npm run preflight 运行 prettier --write . 时会静默重写这个已提交文件，给任何运行它们的人留下相对 HEAD 的脏 diff。同批位置：agent-prompt.test.ts、lib/agent-briefs.ts、cleanup.test.ts、capture-tui.ts（均为 EOF）。修复：删除添加的末尾空行，使文件以最后一个代码行后的单个换行结尾。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| timeoutMs: argv['timeout-ms'] as number, | ||
| }), | ||
| }; |
There was a problem hiding this comment.
[Suggestion] R3-4 (new this round, 5 of 5 locations): the merge commit adds a trailing blank line at EOF of this file — it now fails npx prettier --check (executed at HEAD: exit 1) while both merge parents end cleanly, so the blank line was introduced by the reviewed merge itself. Failure scenario: npm run format / npm run preflight run prettier --write . and silently rewrite this committed file, leaving a dirty diff against HEAD for anyone who runs them. Sibling locations: agent-prompt.test.ts, lib/agent-briefs.ts, cleanup.test.ts, cleanup.ts (all EOF). Fix: delete the added trailing blank line so the file ends with the single newline after the closing };.
中文说明
R3-4（本轮新发现，共 5 处之 5）：合并提交在本文件 EOF 处添加了多余空行——现在 npx prettier --check 失败（HEAD 上已执行：退出 1），而两个合并父都以干净的单换行结尾，因此空行是被审查的合并本身引入的。故障场景：npm run format / npm run preflight 运行 prettier --write . 时会静默重写这个已提交文件，给任何运行它们的人留下相对 HEAD 的脏 diff。同批位置：agent-prompt.test.ts、lib/agent-briefs.ts、cleanup.test.ts、cleanup.ts（均为 EOF）。修复：删除添加的末尾空行，使文件以结尾 }; 后的单个换行结尾。
— qwen3.8-max via Qwen Code /review (v0.21.15)
…ed reap can promise R18-2, re-filed each round: a same-uid process renames a live foreign socket into a capture-shaped name before the sweep scans; it is a plain socket with one link and a chosen-dead pid, so it passes all three entry checks and the pinned kill-server destroys the victim under a "Reaped" line. The finding is correct that the code overclaimed, and correct that it is real — and it is exactly the active-same-uid boundary capture-tui's own header already states as a non-goal (Node exposes no *at() syscalls, so every path resolves by name and a name can be redirected). The sweep's local comments did not carry that boundary: the entry-guard comment implied it forecloses redirection "including the user's own tmux server," and the post-kill re-check claimed to make "a won swap loud" — neither is true for a rename that is already in place at scan time (a plain socket indistinguishable from a real orphan; the re-check then compares the entry against itself and is silent). Decision, after investigating whether it is closable: it is not, from here. Every identity signal a sweep could add is within the same adversary's reach — an on-disk server-pid record is same-uid writable, and the answering server's own shape is same-uid craftable — so recording a pid (the suggested fix) or probing the server's shape raises the bar without closing the model, while manufacturing the false security this PR's contract is written to avoid. The sweep is also an optimization: the orphan self-terminates within the holder's three-hour bound regardless. So the honest change is to make the comments state the boundary the header already draws, not to grow the diff with machinery that does not hold. Real hardening is #9274's, as the header says. Comment-only; no behavior change. `npm run build` clean, prettier and eslint clean, `vitest run cleanup.test.ts` 97 passed / 0 failed.
2 participants
What this PR does
Narrows
capture-tui's file guarantee from "protected against ordinary conditions" to "also defended against a same-uid actor racing the capture", as far as this runtime allows. Stacks on #9273, which states the base guarantee and explicitly declines this one.Six holes, each reproduced against #9273's code before it was fixed:
--timeout-ms. A captured command writing its own<out>.jsonhad that file silently replaced while the run reported success. Occupancy is re-decided at write time.<out>.ansduring the window redirected this run's bytes out of the--outbase. The artifact opens carryO_NOFOLLOWand create atomically (O_CREAT|O_EXCL) rather than truncating whatever is there.freezewrote through a planted symlink and the run attestedevidence: "png"for bytes outside the base — and a failed render then deleted whatever the captured command had put there. Re-stamped after the window; only a regular file counts as a rendering..ansname. The manifest now records the identity of the artifacts it wrote, and the clear phase requires a match.mv dir dir.stolen && ln -s /victim dirmid-window sent all three artifacts out of the base while the manifest attested the original paths. The directory is pinned by dev+ino and re-checked before every artifact operation.Why it's needed
The base command's contract is honest about what it does not defend, which is the right default for a feature PR — but the window it leaves open is large: up to an hour of
--timeout-msduring which the captured command, running as the same uid, can redirect or destroy the evidence while the manifest goes on attesting the original paths. That is worth closing on its own terms, with its own tests, rather than inside a feature review.What this does not claim. Node exposes no
*at()syscalls (openat/unlinkatagainst a directory descriptor), so every path still resolves by name, and the interval between a check and the syscall on its heels cannot be closed from here. What the guards remove is the window, not the race: an actor now needs to land inside that interval rather than anywhere in the capture. The header says so.Reviewer Test Plan
How to verify
Six tests carry the reproductions, most usefully:
refuses when the --out DIRECTORY is swapped mid-window— performs themv+ln -sand asserts the victim directory stays empty.never credits a png written THROUGH a mid-window symlink— assertsevidence: "ans-only"and that the link's target is untouched.does not delete a FOREIGN file that replaced <out>.ans mid-render— the replacement is done by a fakefreezefrom inside the render window.Each fix reddens its own test when reverted; I checked each one individually.
Evidence (Before & After)
Before (on #9273's code), a captured command running
cd ..; mv capdir capdir.stolen; ln -s victim capdircompleted withexitCode 0,{"captured":true,…}, the artifacts written undervictim/, andmanifest.ansPathnamingcapdir/cap.ans. After: exit 3,the directory holding --out was replaced during the capture: …, andvictim/empty.Tested on
Environment (optional)
macOS 15, tmux 3.7b, freeze 0.2.2.
O_NOFOLLOWis POSIX-only; on Windows the occupancy checks are the whole guard, which the header states.中文说明
把
capture-tui的文件担保从"防普通情况"收紧到"同时防同 uid 竞速者"——在本运行时允许的范围内。基于 #9273，那个 PR 写明了基础担保并明确声明不覆盖本 PR 这一类。六个漏洞，每一个都在 #9273 的代码上复现过：碰撞门在窗口之前、窗口最长跑满
--timeout-ms；窗口期埋符号链接可以把字节写到--out之外；png 是唯一由外部程序写的路径，窗口前的取样对新埋的东西一律报"不存在"；manifest 签名只认证 manifest 本身、不认证旁边的文件；所有路径按名解析，父目录可被整体掉包；以及拒绝路径反手删掉了它本要保护的占用者。不声称的部分：Node 没有
*at()系统调用，路径仍按名解析，检查与随后系统调用之间那一瞬关不掉。这些守卫消除的是窗口，不是竞态——对手现在必须落进那一瞬，而不是整个采集期。文件头写明了这一点。