Conversation
|
Thanks for the PR — this one is easy to get behind. It fixes a fail-open that a maintainer filed with measurements attached, and it follows the shape that issue asked for almost line for line. Template looks good ✓ — every required heading is present, the OS table is filled in honestly (Windows ✅, macOS/Linux Problem: observed, not theoretical. #11848 was filed from review finding R1-26 on #11787 and carries real output from two independent Windows self-hosted arms ( Direction: aligned. Two identity comparators degrade to a weaker check on volumes with 64-bit file ids, and the weaker check is wrong in the dangerous direction — an alias admitted, a journal swap undetected. Asking for Size: not applicable. No core paths are touched — Approach: the scope feels right, and I checked the two constraints #11848 set rather than taking the description's word for them. One thing worth your attention before the code review, raised there rather than here as a blocker: the same Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths. Worth naming for the reviewer anyway: this is a security-boundary comparator (the deletion journal's swap detection), so the interesting question is not "does it still work" but "did any identity check get looser". I read the diff for exactly that and could not find one — the Moving on to code review. 🔍 中文说明感谢贡献 —— 这个 PR 很容易支持。它修复的是一个由维护者带着实测数据提交的 fail-open 缺陷，并且几乎逐行遵循了该 issue 给出的方案形态。 模板完整 ✓ —— 所有必需标题齐全，操作系统表格如实填写（Windows ✅，macOS/Linux 问题： 已观测，非理论性。#11848 源自 #11787 的 review finding R1-26，附带两台独立 Windows 自托管 CI 机器的真实输出（ 方向： 对齐。两个身份比较器在文件 ID 为 64 位的卷上退化为更弱的检查，而退化方向恰恰是危险的那一侧 —— 别名被放行、日志换包未被发现。改用 规模： 不适用。未触及核心路径 —— 方案： 范围合理。#11848 提出的两条约束我逐项核对过，而非仅采信 PR 描述： 有一点请在代码审查前留意 —— 我把它放在审查意见里提出，而不是当作阻塞项：你在所触及的两个测试文件中收窄的 风险： 无升级风险信号 —— 改动文件均未命中与 revert 相关的高风险路径。但仍值得向审查者点明：这是一个安全边界比较器（删除日志的换包检测），因此关键问题不是「是否仍能工作」，而是「是否有身份检查被放宽」。我正是按这个角度读的 diff，没有找到 —— 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI read this against the question that actually matters for a security-boundary comparator: did any identity check get looser? I could not find one, and the containment is better than the description claims. The whole bigint surface is module-private. Also verified: all three Nothing below blocks the merge. Two are cleanup this PR creates, one is a follow-up. 1. Two consumer test files still skip on the platform you just fixed, and their comments now say something false. You narrowed the
Both comments assert " 2. The doc comment on the predicate you stopped importing goes stale. 3. Follow-up, explicitly not a request to widen this PR. The same number-backed predicate still guards two sibling comparators: Testing evidenceNo CI evidence exists for this commit — CI has not run. All five So the compile-level claims I verified above are verified by reading, not by a green The Windows before/after numbers in the PR description are the author's claim on a single host, not evidence I re-ran — I did not execute any PR-derived code, and on an unattended run I do not. They are plausible and internally consistent (the 2 remaining
Sandboxed verification would settle this: 中文说明代码审查我是按「对一个安全边界比较器真正重要的问题」来读的：是否有任何身份检查被放宽？ 没有找到，而且改动的封闭性比 PR 描述所说的还要好。 整个 bigint 影响面是模块私有的。 同时核实：三处 以下均不阻塞合并。前两条是本 PR 造成的清理项，第三条是后续工作。 1. 两个消费方测试文件仍在你刚修好的平台上跳过，而且它们的注释现在陈述了错误的事实。 你在所触及的两个测试文件里收窄了
两处注释都断言「 2. 你不再导入的那个谓词，其文档注释变陈旧了。 3. 后续工作，明确不是要求扩大本 PR。 同一个 number 形态谓词仍在守卫两个同类比较器： 测试证据该提交没有任何 CI 证据 —— CI 尚未运行。 五个 因此我上面核实的编译层面结论是靠阅读得出的，不是靠绿色的 PR 描述里的 Windows before/after 数字是作者在单台主机上的自述，不是我复跑的证据 —— 我没有执行任何 PR 派生代码，在无人值守的 CI 运行中也不会执行。这些数字可信且内部自洽（ 上方表格中的 CI 结论由 finalize 任务在 CI 落定后就地更新。 沙箱验证可以定这件事： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the fix itself is sound and I would merge it on the strength of the review, but not one line of CI has executed on this commit, and the entire PR is a claim about runtime behaviour on a platform no check has exercised. ⏸️ Deferring to a maintainer — not approving, not requesting changes. Before reading the diff I wrote down what I would do with #11848 myself: convert The one thing my proposal had that the PR missed is the finding that matters: asking "which tests does this fix un-skip?" rather than "which files did it touch?" surfaces What stops me approving is not that finding. It is that all five To be explicit about what I did and did not establish. By reading, I am confident the change is type-consistent, that no bigint can reach Two smaller reasons I am comfortable with the change on the merits, for whoever picks this up. The pattern is precedented: the reference agent shipped "using 64-bit precision for inode values" for exactly this class of false-identity bug, and separately fixed agent loading on filesystems reporting zero inodes — which is the If I were maintaining this in six months I would thank the author. The header comments explain why bigint rather than narrating what the code does, the new fixtures pin their own premises ( What a maintainer needs to decide, in order: authorize the five CI runs and confirm One process note so this escalation is not silently lost: I could not resolve an owner to @mention. 中文说明信心度：3/5 —— 修复本身是可靠的，仅凭审查我就愿意合并它；但这个提交上没有执行过任何一行 CI，而整个 PR 主张的恰恰是一个没有任何检查覆盖到的平台上的运行时行为。 ⏸️ 转交维护者 —— 不批准，也不要求修改。 在读 diff 之前，我先写下了自己会怎么修 #11848：把 我的方案里有、而本 PR 漏掉的那一点，正是关键发现：问「这个修复摘除了哪些测试的跳过」而不是「它改了哪些文件」，就会看到 阻止我批准的并不是这条发现，而是：五个 明确区分我确认了什么、没确认什么。通过阅读，我有信心：改动类型一致、bigint 不可能抵达 另外两点让我对改动本身放心，供接手者参考。其一，这个做法有先例：参考 agent 就为同一类错误身份缺陷发布过「using 64-bit precision for inode values」，也单独修复过在报告零 inode 的文件系统上加载 agent 的问题 —— 后者正是本 PR 有意保留、而没有顺手优化掉的 如果六个月后由我维护这段代码，我会感谢作者。头部注释解释的是为什么用 bigint，而不是复述代码在做什么；新增 fixture 钉住了自身前提（ 维护者需要按顺序决定：先授权那五个 CI 运行，确认 一点流程说明，以免这次升级被无声丢掉：我无法解析出应当 @ 的负责人。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
…efresh the predicate doc comment
|
Thank you — the "did any identity check get looser" framing made this a very easy review to answer. All three points addressed: 1. The two consumer test gates — fixed in 2. The stale predicate doc comment — fixed in the same commit. 3. Follow-up — filed as #11877. I verified both sites before filing, and you're right that they're the same class. Two wrinkles worth flagging there that make it genuinely a separate piece of work: On the CI gap: understood — the five |
|
Small self-correction on my point 2: the review already said "plus the internal uses in |
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. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 4": none — no check was cut short by the tool ceiling..
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): src/commands/review/lib/same-file.test.ts — no such file or directory; src/serve/conversations/standalone-deletion-journal.test.ts — no such file or directory; Tests 22 passed — this review observed 31272 passed; 6 passed — this review observed 31272 passed; 8 passed — this review observed 31272 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 4"：none — no check was cut short by the tool ceiling.。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：src/commands/review/lib/same-file.test.ts — no such file or directory; src/serve/conversations/standalone-deletion-journal.test.ts — no such file or directory; Tests 22 passed — this review observed 31272 passed; 6 passed — this review observed 31272 passed; 8 passed — this review observed 31272 passed。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const rightStat = tryStat(right); | ||
| if (leftStat !== undefined && rightStat !== undefined) { | ||
| if (hasVerifiableInode(leftStat.ino) && hasVerifiableInode(rightStat.ino)) { | ||
| if (leftStat.ino !== 0n && rightStat.ino !== 0n) { |
There was a problem hiding this comment.
[Suggestion] R1-1: Both gates this PR adds — this one and const inodeVerifiable = stat.ino !== 0n; in directoryIdentityOf (standalone-deletion-journal.ts:102) — restate a predicate core already exports in bigint-ready form: hasVerifiableInode(ino: number | bigint) at packages/core/src/utils/file-identity.ts:19, whose body is return Number(ino) !== 0;. The new header comment gives the reason for not using it as "core's canonical one … stays looser", but that holds only for number inputs — for every bigint the two predicates agree, so at exactly the two call sites this diff creates the stated reason does not distinguish them. The cost is that the "is this inode proof of identity" rule now has six independent expressions across the repo (file-identity.ts:19, conversation-directory-identity.ts:40, managed-scratch-workspace.ts:93, acp-bridge/src/sessionAttachments.ts:31, plus these two), and the next change to that rule has to be found and applied in each — with the two new copies sitting behind a comment telling the next reader the shared helper does not fit.
Core's signature is already (ino: number | bigint), so reusing it needs no widening and does not engage the issue's "keep the signature" constraint:
import { hasVerifiableInode } from '@qwen-code/qwen-code-core/utils/file-identity.js';
// …
if (hasVerifiableInode(leftStat.ino) && hasVerifiableInode(rightStat.ino)) {with const inodeVerifiable = hasVerifiableInode(stat.ino); in the journal. If you would rather keep the local gates, the smaller change is to correct the rationale: the real reason to leave core's predicate alone is the issue's "conversions local to their comparators" constraint plus the bundle-closure concern managed-scratch-workspace.ts:88-92 documents — not looseness.
Witness:
divergences over 200015 bigint inputs: 0
edges: 0n, -0n, ±1n, 2n**53n, 2n**60n+1n/+2n, 2n**64n, 2n**128n,
2n**2000n -> Number(...) = Infinity, -(2n**2000n), 29273397578164384n
plus a 200k random sweep to 2^2100
with the reuse fix applied:
✓ src/commands/review/lib/same-file.test.ts (10 tests)
✓ src/serve/conversations/standalone-deletion-journal.test.ts (25 tests)
Tests 35 passed (35)
The import has to be the deep …/utils/file-identity.js path rather than the package root: AGENTS.md § Code Conventions requires production code in packages/cli/src to import core values from the module that defines them, and the barrel allowlist is closed ("drop an entry when you move its file off the root, never add one"). If you take the reuse fix, the tests that pin it are equates hard-linked names through an exact inode above the safe-integer range and decides by canonical spelling when inodes are unverifiable in same-file.test.ts, plus rejects a complete private replacement %s tree on every operation on the journal side — please confirm each goes red when the substituted predicate is removed.
中文说明
本 PR 新增的两处判定——此处这一处，以及 directoryIdentityOf 中的 const inodeVerifiable = stat.ino !== 0n;（standalone-deletion-journal.ts:102）——重述了 core 已经以支持 bigint 的形式导出的谓词：packages/core/src/utils/file-identity.ts:19 的 hasVerifiableInode(ino: number | bigint)，其函数体为 return Number(ino) !== 0;。新增的头部注释把不复用它的理由写为「core 的那个更宽松」，但这只对 number 入参成立——对任何 bigint，两个谓词的结论完全一致，因此在本 diff 新建的这两个调用点上，所述理由并不能区分二者。代价是：「这个 inode 能否作为身份证明」这条规则现在在仓库中有六处独立表述（file-identity.ts:19、conversation-directory-identity.ts:40、managed-scratch-workspace.ts:93、acp-bridge/src/sessionAttachments.ts:31，加上本 PR 新增的两处），将来任何一次对该规则的修改都必须逐处找到并同步；而新增的这两处还位于一段告诉后续读者「共享 helper 不适用」的注释之后。
core 的签名本就是 (ino: number | bigint)，所以复用它不需要放宽签名，也不触及 issue 中「保持签名不变」的约束：
import { hasVerifiableInode } from '@qwen-code/qwen-code-core/utils/file-identity.js';
// …
if (hasVerifiableInode(leftStat.ino) && hasVerifiableInode(rightStat.ino)) {日志侧对应改为 const inodeVerifiable = hasVerifiableInode(stat.ino);。如果更希望保留本地判定，那么更小的改动是修正理由：不动 core 谓词的真正原因是 issue 要求的「转换局限于各自的比较器内部」，以及 managed-scratch-workspace.ts:88-92 记录的打包闭包顾虑——而不是「更宽松」。
证据：
divergences over 200015 bigint inputs: 0
edges: 0n, -0n, ±1n, 2n**53n, 2n**60n+1n/+2n, 2n**64n, 2n**128n,
2n**2000n -> Number(...) = Infinity, -(2n**2000n), 29273397578164384n
plus a 200k random sweep to 2^2100
with the reuse fix applied:
✓ src/commands/review/lib/same-file.test.ts (10 tests)
✓ src/serve/conversations/standalone-deletion-journal.test.ts (25 tests)
Tests 35 passed (35)
该 import 必须使用深层路径 …/utils/file-identity.js 而非包根：AGENTS.md 的 Code Conventions 要求 packages/cli/src 中的生产代码从定义该值的模块导入 core 内容，且 barrel 白名单是封闭的（「把文件从根导入迁走时就删掉对应条目，绝不要新增」）。若采用复用方案，钉住它的测试是 same-file.test.ts 中的 equates hard-linked names through an exact inode above the safe-integer range 与 decides by canonical spelling when inodes are unverifiable，以及日志侧的 rejects a complete private replacement %s tree on every operation——请确认移除所替换的谓词后这些用例都会变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // exact and the alias guard under test is live on volumes whose ids | ||
| // exceed 2^53 (#11848). The only unverifiable case left is a volume | ||
| // reporting no inode numbers at all (FAT/exFAT/SMB). | ||
| if (statSync(out, { bigint: true }).ino === 0n) { |
There was a problem hiding this comment.
[Suggestion] R1-2: This is one of four copies of the same volume-verifiability skip gate that this PR has to edit in lockstep to make one semantic change — the others are repo-context.test.ts:1021, lib/same-file.test.ts:112 and the lstat variant at standalone-deletion-journal.test.ts:453 — and packages/cli/src/commands/review/lib/test-utils.ts already exists as this directory's shared-oracle module, imported by 27 suites here including repo-context.test.ts:37. The gate's meaning ("skip only where the volume reports no inode numbers at all") is therefore maintained in four places, and a future change to it that misses one copy fails silently by construction: ctx.skip() reports as skipped, never as failed, so that suite stops exercising the alias guard on exactly the platform #11848 exists to fix and nothing anywhere goes red.
One helper in test-utils.ts, carrying the #11848 rationale once:
export function inodesVerifiable(
stat: (p: string, o: { bigint: true }) => { ino: bigint },
path: string,
): boolean {
return stat(path, { bigint: true }).ino !== 0n;
}called as if (!inodesVerifiable(statSync, out)) { ctx.skip(); return; } in the three review suites. The stat binding has to be a parameter, and the journal suite's lstat variant lives outside commands/review so it may keep its local form — which is why this is worth weighing rather than taking automatically: the consolidatable part is one line per site across three sites, against AGENTS.md's "Minimum code that solves the problem".
Witness:
BASE 9efdd898 — pattern 'isSafeInteger\((inode|journalStats\.ino)|ino <= 0'
findings.test.ts:1467 same-file.test.ts:102 repo-context.test.ts:1023
standalone-deletion-journal.test.ts:450 -> 4 copies
HEAD 15b8b696 — pattern 'ino === 0n' (gate sites only)
findings.test.ts:1463 lib/same-file.test.ts:112 repo-context.test.ts:1021
standalone-deletion-journal.test.ts:453 -> 4 copies
all four HEAD sites are in files this diff edits
test-utils.ts: 27 importing suites in this directory; plantRepository doc (:197)
"The oracle every host-execution witness shares, so a fixture that quietly
stops being an attack fails in one place instead of in each suite."
The helper must take the caller's stat binding rather than importing statSync itself: test-utils.ts:117-124 records that "Callers hand over their own bindings: the parse-args suite mocks node:fs for the whole file, so bindings this module imported itself would write into the mock instead of the tree the check under test reads" — and same-file.test.ts mocks node:fs file-wide too, so a self-imported binding would read the mock or the tree unpredictably.
中文说明
这是同一个「卷可验证性」跳过门的四份副本之一，本 PR 为了完成一次语义修改必须同步改动全部四份——其余三处在 repo-context.test.ts:1021、lib/same-file.test.ts:112，以及 standalone-deletion-journal.test.ts:453 的 lstat 变体——而 packages/cli/src/commands/review/lib/test-utils.ts 本就是该目录的共享 oracle 模块，被这里 27 个测试文件导入，其中就包括 repo-context.test.ts:37。因此这条门的含义（「仅在卷完全不报告 inode 号时跳过」）被分散维护在四处，而将来某次修改只要漏掉一份，其失败在结构上就是静默的：ctx.skip() 只会上报为 skipped，永远不会上报为 failed，于是那个测试文件恰好在 #11848 要修复的平台上不再真正执行别名守卫，而任何地方都不会变红。
在 test-utils.ts 中提供一个 helper，把 #11848 的理由只写一遍：
export function inodesVerifiable(
stat: (p: string, o: { bigint: true }) => { ino: bigint },
path: string,
): boolean {
return stat(path, { bigint: true }).ino !== 0n;
}在三个 review 测试文件中以 if (!inodesVerifiable(statSync, out)) { ctx.skip(); return; } 调用。stat 绑定必须作为参数传入，而日志套件的 lstat 变体位于 commands/review 之外，可以保留其本地形式——这也是为什么这一条值得权衡而非直接照做：可整合的部分只是三个站点各一行，而 AGENTS.md 要求「用解决问题的最少代码」。
证据：
BASE 9efdd898 — pattern 'isSafeInteger\((inode|journalStats\.ino)|ino <= 0'
findings.test.ts:1467 same-file.test.ts:102 repo-context.test.ts:1023
standalone-deletion-journal.test.ts:450 -> 4 copies
HEAD 15b8b696 — pattern 'ino === 0n' (gate sites only)
findings.test.ts:1463 lib/same-file.test.ts:112 repo-context.test.ts:1021
standalone-deletion-journal.test.ts:453 -> 4 copies
all four HEAD sites are in files this diff edits
test-utils.ts: 27 importing suites in this directory; plantRepository doc (:197)
"The oracle every host-execution witness shares, so a fixture that quietly
stops being an attack fails in one place instead of in each suite."
该 helper 必须接收调用方传入的 stat 绑定，而不能自己导入 statSync：test-utils.ts:117-124 记录了「调用方交出它们自己的绑定：parse-args 套件对整个文件 mock 了 node:fs，因此本模块自行导入的绑定会写进 mock，而不是写进被测检查所读取的目录树」——same-file.test.ts 同样对整个文件 mock 了 node:fs，所以自行导入的绑定会不可预测地读到 mock 或真实目录树。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // which rounds a 64-bit NTFS file index. Tracked in #11848 — converting that | ||
| // call to `{ bigint: true }` is what lets this gate come off. | ||
| it.skipIf(process.platform === 'win32').each(['base', 'state'] as const)( | ||
| // Regression cover for #11848: this swap detection was inert on NTFS |
There was a problem hiding this comment.
[Suggestion] R1-3: The journal's exact-inode comparison has no platform-independent test. same-file.test.ts pins the identical property on every platform by mocking node:fs to report one exact id under a bigint stat and its rounded double under a number stat; the journal has no equivalent, so its only discriminating witness is the case this diff un-skips for Windows — and that runs on ci.yml:1652-1662, whose test_windows.if: is merge_group || schedule || workflow_dispatch, with no pull_request. On Linux, where inode numbers sit far below 2^53, a mutant restoring the number predicate is behaviourally identical to the original, so nothing a PR triggers can tell the two apart. Concretely: replacing stat.ino !== 0n at standalone-deletion-journal.ts:102 with hasVerifiableInode(Number(stat.ino)) — the shared number predicate this diff's own comment tells maintainers to edit in lockstep — typechecks cleanly, keeps every { bigint: true } call site intact so the mixed-shape canary stays green, and restores exactly the #11848 fail-open: both sides come back inodeVerifiable: false, sameDirectoryIdentity collapses to a device-only compare, and hasRecord resolves false over a live prepared record instead of rejecting reason: 'compromised'.
Give the journal the same volume pose same-file.test.ts uses. This file already mocks node:fs/promises (currently overriding only open), so add an lstat override keyed by path — the journal directory reports 2n ** 60n + 1n when first recorded and 2n ** 60n + 2n after a rename-and-recreate — then assert every operation rejects.toMatchObject({ reason: 'compromised' }). Include the fixture guard the same-file test uses, expect(Number(beforeIno)).toBe(Number(afterIno)), so the case cannot silently stop exercising one rounding bucket.
Witness:
baseline (pristine) Tests 25 passed (25)
mutant A :102 stat.ino !== 0n
-> Number.isSafeInteger(Number(stat.ino)) && Number(stat.ino) > 0
Tests 25 passed (25)
#11848 fail-open restored; mixed-shape canary stays green
mutant B :141 left.inode === right.inode
-> Number(left.inode) === Number(right.inode)
Tests 25 passed (25)
ci.yml:1652-1662 test_windows.if: merge_group || schedule || workflow_dispatch
directoryIdentityOf must keep returning inode: 0n when inodeVerifiable is false — parseIdentity rejects a persisted record whose fields disagree, via (value['inodeVerifiable'] ? value['inode'] === 0 : value['inode'] !== 0) at standalone-deletion-journal.ts:172. The mutations that must go red once the new case lands are the two above: Number(left.inode) === Number(right.inode) in sameDirectoryIdentity (:133-141), and hasVerifiableInode(Number(stat.ino)) in directoryIdentityOf — both measured green today, so please confirm the new test reds against each.
中文说明
日志的精确 inode 比较没有任何与平台无关的测试。same-file.test.ts 通过 mock node:fs——在 bigint stat 下报告一个精确 id、在 number stat 下报告其舍入后的 double——在所有平台上钉住了同一性质；日志侧没有等价物，因此它唯一能区分正反实现的见证就是本 diff 为 Windows 摘除跳过门的那个用例——而它运行在 ci.yml:1652-1662，其 test_windows.if: 为 merge_group || schedule || workflow_dispatch，并不包含 pull_request。在 Linux 上 inode 号远低于 2^53，恢复 number 谓词的变异体与原实现在行为上完全一致，所以 PR 能触发的任何检查都无法区分二者。具体而言：把 standalone-deletion-journal.ts:102 的 stat.ino !== 0n 换成 hasVerifiableInode(Number(stat.ino))——也就是本 diff 自己的注释要求维护者「同步修改」的那个共享 number 谓词——类型检查通过，所有 { bigint: true } 调用点保持不变因而混合形态的哨兵用例依然为绿，却恰好恢复了 #11848 的「失败即放行」：两侧都返回 inodeVerifiable: false，sameDirectoryIdentity 退化为只比较 device，于是 hasRecord 面对一份仍然存在的 prepared 记录回答 false，而不是以 reason: 'compromised' 拒绝。
给日志套件加上与 same-file.test.ts 相同的卷姿态。该文件已经 mock 了 node:fs/promises（目前只覆盖 open），因此按路径追加一个 lstat 覆盖——日志目录在首次记录时报告 2n ** 60n + 1n，在 rename 并重建之后报告 2n ** 60n + 2n——然后断言每个操作都 rejects.toMatchObject({ reason: 'compromised' })。同时加上 same-file 测试所用的 fixture 守卫 expect(Number(beforeIno)).toBe(Number(afterIno))，以免该用例在无声中不再覆盖同一个舍入桶。
证据：
baseline (pristine) Tests 25 passed (25)
mutant A :102 stat.ino !== 0n
-> Number.isSafeInteger(Number(stat.ino)) && Number(stat.ino) > 0
Tests 25 passed (25)
#11848 fail-open restored; mixed-shape canary stays green
mutant B :141 left.inode === right.inode
-> Number(left.inode) === Number(right.inode)
Tests 25 passed (25)
ci.yml:1652-1662 test_windows.if: merge_group || schedule || workflow_dispatch
directoryIdentityOf 必须在 inodeVerifiable 为 false 时继续返回 inode: 0n——parseIdentity 会拒绝字段自相矛盾的持久化记录，其判定为 standalone-deletion-journal.ts:172 的 (value['inodeVerifiable'] ? value['inode'] === 0 : value['inode'] !== 0)。新用例落地后必须变红的变异就是上面两个：sameDirectoryIdentity（:133-141）中的 Number(left.inode) === Number(right.inode)，以及 directoryIdentityOf 中的 hasVerifiableInode(Number(stat.ino))——两者今天实测均为绿，因此请确认新测试对每一个都会变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
… journal's >2^53 volume in tests
|
Round 1 findings addressed in R1-1 (predicate reuse) — done as specified. Both comparators now import core's
R1-2 (gate consolidation) — done for the three review suites. R1-3 (platform-independent journal test) — done. The suite's
Full local state after the round: same-file 8/10 (2 symlink-EPERM baseline), journal 23/23 + 3 intentional win32 skips, findings 127/130 and repo-context 48/51 (failures all the pre-existing symlink-EPERM class, identical on unmodified On the Test Plan note: the two "no such file or directory" observations were the commands run from the repo root — they are package-relative paths, meant to be run from |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
@qwen-code /review |
2 participants
What this PR does
Fixes the two file-identity comparators that #11848 reported as failing open on volumes whose file ids exceed 2^53 (in practice: NTFS).
review/lib/same-file.ts(isSameFile) and the standalone deletion journal's directory-identity check both statted without{ bigint: true }, so a 64-bit NTFS file index arrived rounded at the JS boundary, the strict safe-integer predicate withheld verifiability, and both comparators degraded —isSameFileto canonical spellings that can never see through a hard link,sameDirectoryIdentityto comparing equal for a complete private replacement of the journal tree. Both call sites now stat with{ bigint: true }(plus the twohandle.stat()sites that share the journal'sDirectoryIdentity), and verifiability is gated on the exact id being non-zero, keeping the canonical-spelling fallback only for genuineino === 0nvolumes (FAT/exFAT/SMB). Per the issue's constraints, the shared strict predicatehasVerifiableInode(ino: number)keeps its signature — both conversions are local to the comparators, matching the repo's existing{ bigint: true }convention (no-follow-open.ts,session-writer-lease.ts,sessionArtifacts.ts, ...). The fix validates all threeisSameFilealias guards (findings.ts,repo-context.ts,save-artifact.ts), which are plain consumers needing no changes of their own.On the test side, per the triage additions on #11848: the shared
node:fsmock insame-file.test.tsnow forwards stat options and reports an exact bigint id under{ bigint: true }versus its rounded double under a number stat; the two tests whose premise disappeared (refuses inode identity above the safe-integer range,still finds one file through an unsafe inode via canonical spelling) are rewritten around the exact-id regime, and the issue's acceptance test is added — two hard-linked names with an exactinoabove 2^53 must compare as one file, and it goes red against the number-stat implementation. The Windows gate on the journal'srejects a complete private replacement %s tree on every operationcomes off (the comment #11853 left there said converting to bigint stats is exactly what lets it come off), and the inode-verifiability skip gates in both touched test files narrow to the genuineino === 0case.Why it's needed
On an NTFS volume whose file ids exceed 2^53, an
--outhard-linked to--to-anchorswas not recognised as an alias and silently overwrote the previous artifact, and the deletion journal's swap detection went inert —hasRecordansweredfalseover an attacker-created empty tree instead of rejecting withreason: 'compromised'. Both were measured red on two Windows self-hosted CI arms at the base of #11787, and the machine this PR was written on reports real temp-dir file ids above 2^53 (fs.statSync(f).ino === 29273397578164384,Number.isSafeInteger(...) === false), so the defect is reproduced against a live NTFS volume below, not only through mocks.Reviewer Test Plan
How to verify
npx vitest run src/commands/review/lib/same-file.test.ts src/serve/conversations/standalone-deletion-journal.test.tsfrompackages/cli— the three new/rewritten exact-inode cases and the two un-gated journal replacement tests should pass.same-file.tsreverted tomain, the new caseequates hard-linked names through an exact inode above the safe-integer rangefails (the mock reports the rounded double, the strict predicate refuses it, the canonical-spelling fallback answers false for the two hard-link names) — the issue's acceptance criterion.fs.statSync(anyFile).inofor a value > 2^53), the pre-existing testtreats two hard links to one file as the same filefails onmainand passes with this branch, using no mocks at all.Evidence (Before & After)
Measured on this Windows host (Node v24.21.0, real NTFS temp volume with ino
29273397578164384> 2^53),packages/cli:Before (branch's tests against
main'ssame-file.ts):After (this branch):
Journal suite on the same host — the two tests gated off on Windows since #11787 now run and pass against a real >2^53 NTFS volume:
Also green:
npx eslint <4 changed files> --max-warnings 0,npx prettier --experimental-cli --check <4 changed files>, andtsc --noEmitreports no errors in the changed files (the pre-existingsrc/ui/selection/*ink-typing errors on a fresh Windows checkout are unrelated and reproduce identically without this change).Tested on
Environment (optional)
Windows 11, Node v24.21.0,
npx vitest runinpackages/cli; the host's NTFS temp volume reports file ids above 2^53, which is what made the no-mock before/after reproduction possible.Risk & Scope
DirectoryIdentityis now bigint-shaped end to end (all three construction sites converted together, so no number/bigint mixing); the mode/uid privacy checks on the same stat objects use bigint-safe arithmetic (0o777n,BigInt(process.getuid())). Persisted JSON record schemas are untouched — records carry caller-supplied number identities and never heldDirectoryIdentity.readPhase(lstat+handle.stat()on the record file) stays number-backed; both sides round consistently so it cannot fail open in the reported sense, and the issue scopes the conversion to the two directory comparators. The pre-existing symlink-EPERM test failures on Windows hosts without developer mode are unchanged.Linked Issues
Fixes #11848
中文说明
本 PR 做了什么
修复 #11848 报告的两个在文件 ID 超过 2^53 的卷（实践中即 NTFS）上「失败即放行」的文件身份比较器。
review/lib/same-file.ts（isSameFile）与独立删除日志的目录身份检查此前都未以{ bigint: true }进行 stat，64 位 NTFS 文件索引在 JS 边界被舍入，严格的安全整数判定因此拒绝承认其可验证性，两个比较器随之退化——isSameFile退化为永远无法看穿硬链接的规范拼写比较，sameDirectoryIdentity则在日志目录树被完整私有替换时仍判定相等。现在两处调用点（以及共享日志DirectoryIdentity的两个handle.stat()调用点）都以{ bigint: true}进行 stat，可验证性以精确 ID 非零为门槛，规范拼写回退仅保留给真正ino === 0n的卷（FAT/exFAT/SMB）。按 issue 的约束，共享的严格谓词hasVerifiableInode(ino: number)签名不变——两处转换都局限在各自的比较器内部，与仓库已有的{ bigint: true }惯例一致（no-follow-open.ts、session-writer-lease.ts、sessionArtifacts.ts等）。本修复覆盖全部三个isSameFile别名守卫（findings.ts、repo-context.ts、save-artifact.ts），它们都是纯消费方，自身无需改动。测试侧按 #11848 的 triage 补充意见处理：
same-file.test.ts的共享node:fsmock 现在转发 stat options，在{ bigint: true }下报告精确 bigint ID、在 number stat 下报告其舍入 double；两个前提已消失的测试（refuses inode identity above the safe-integer range、still finds one file through an unsafe inode via canonical spelling）围绕精确 ID 机制重写，并加入了 issue 的验收测试——两个硬链接名在精确ino超过 2^53 时必须判定为同一文件，该用例在 number-stat 实现下会变红。日志的rejects a complete private replacement %s tree on every operation的 Windows 跳过门被移除（#11853 留下的注释正说明转换为 bigint stats 就是摘掉它的条件），两个被触及测试文件中的 inode 可验证性跳过门都收窄到真正的ino === 0场景。为什么需要
在文件 ID 超过 2^53 的 NTFS 卷上，与
--to-anchors建立硬链接的--out不会被识别为别名，会静默覆盖已有产物；删除日志的换包检测也会失效——面对攻击者创建的空目录树，hasRecord回答false而不是以reason: 'compromised'拒绝。两者都在 #11787 基线的两台 Windows 自托管 CI 机器上实测变红，而本 PR 编写所用的机器真实临时目录文件 ID 即超过 2^53（fs.statSync(f).ino === 29273397578164384，Number.isSafeInteger(...) === false），因此下文的缺陷复现是在真实 NTFS 卷上完成的，而非仅靠 mock。审查者测试计划
如何验证
packages/cli下运行npx vitest run src/commands/review/lib/same-file.test.ts src/serve/conversations/standalone-deletion-journal.test.ts——三个新增/重写的精确 inode 用例和两个摘除跳过门的日志替换测试应全部通过。same-file.ts回退到main，新用例equates hard-linked names through an exact inode above the safe-integer range即失败（mock 报告舍入后的 double，严格谓词拒绝它，规范拼写回退对两个硬链接名回答 false）——即 issue 的验收标准。fs.statSync(任意文件).ino是否大于 2^53），既有测试treats two hard links to one file as the same file在main上失败、在本分支上通过，全程不使用任何 mock。证据（Before & After）
在本 Windows 主机（Node v24.21.0，真实 NTFS 临时卷 ino
29273397578164384> 2^53）上的packages/cli实测：Before（本分支的测试 +
main的same-file.ts）：After（本分支）：
同一主机上的日志套件——自 #11787 起在 Windows 上被跳过的两个测试如今在真实 >2^53 NTFS 卷上运行并通过：
同时通过：
npx eslint <4 个改动文件> --max-warnings 0、npx prettier --experimental-cli --check <4 个改动文件>，且tsc --noEmit在改动文件上无错误（全新 Windows 检出中src/ui/selection/*的 ink 类型错误为既有问题，不含本改动时可同样复现）。测试平台
环境（可选）
Windows 11，Node v24.21.0，在
packages/cli中运行npx vitest run；本机 NTFS 临时卷报告超过 2^53 的文件 ID，使得不依赖 mock 的 before/after 复现成为可能。风险与范围
DirectoryIdentity现已全链路为 bigint 形态（三个构造点一并转换，不存在 number/bigint 混用）；同一 stat 对象上的 mode/uid 隐私检查使用 bigint 安全运算（0o777n、BigInt(process.getuid())）。持久化 JSON 记录结构未动——记录携带调用方提供的 number 身份，从不保存DirectoryIdentity。readPhase中记录文件的竞争检查（对记录文件的lstat+handle.stat()）保持 number——两侧一致舍入，不会出现本 issue 所述意义上的 fail-open，且 issue 将转换范围限定为两个目录比较器。无开发者模式的 Windows 主机上既有的 symlink EPERM 测试失败保持不变。关联 Issue
Fixes #11848