fix(cli): drop duplicate fake properties breaking the main build - #10022
Merged
Conversation
Collaborator
|
⏳ Approval still deferred — 1 PR CI workflow run(s) still in progress for ⏳ 审批仍在延迟中 —— |
yiliang114
enabled auto-merge
August 25, 2026 13:25
Collaborator
|
Thanks for the PR! Template: the essentials are covered in substance — motivation ( Problem: observed, not theoretical. Main is red right now — the Direction: fully aligned — this unblocks the main build. No CHANGELOG signal needed for a build repair. Size: not applicable — test file only, 2 deletions, 0 production logic lines. Approach: the scope is exactly right and cannot be more minimal. For each duplicate pair the kept declaration is behaviorally equivalent to today's runtime winner: the later property wins in JS object literals, so for Risk: no elevated risk signals — test-file-only change, none of the revert-correlated high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献！ 模板：核心内容已实质覆盖——动机（ 问题：已观测到的真实问题，不是理论性加固。main 当前是红的—— 方向：完全对齐——解除 main 构建阻塞。构建修复无需 CHANGELOG 信号。 规模：不适用——纯测试文件，删除 2 行，0 行生产代码。 方案：范围恰好，已是最小改动。每对重复声明中保留的一份与今天运行时生效者行为等价：JS 对象字面量中后声明者生效，因此 风险：无升级风险信号——仅测试文件改动，未命中任何与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewMy independent proposal for "main fails to compile on duplicate object properties in the session-swap telemetry fakes" is exactly what this diff does: delete one declaration from each duplicated pair in
Testing evidence — the PR's own CI via API (this unattended run executes no PR code)CI on the reviewed commit was still settling at fetch time — the That is the runner's disk filling up while extracting dependencies — a two-line deletion in a test file cannot cause it, and the job never reached compilation. CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Pending at fetch time: the build + unit-suite result for this branch — those jobs had not reached execution behind the ENOSPC attempt, and per policy this review does not poll. The build itself ( 中文说明代码审查针对"session-swap telemetry fake 中重复对象属性导致 main 编译失败"，我的独立方案与本 diff 完全一致：删除
测试证据——PR 自身 CI（经 API 读取；本 unattended 运行不执行 PR 代码）抓取时被审提交的 CI 尚未跑完—— 抓取时尚未决出：本分支的构建 + 单测结果——相关作业排在 ENOSPC 尝试之后尚未执行，按规约本审查不轮询。构建本身（ — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — a provably correct two-line repair of a red main; the only nit is that the PR body's headings deviate from the template. Stepping back: this is a maintainer repairing their own regression from this morning (#9998). The failure is observed and pinpointed — main's The one reservation is process, not code: the body uses CI on the reviewed commit is still settling — the 中文说明置信度：4/5 —— 对红色 main 的一次可证明正确的两行修复；唯一的小问题是 PR 正文标题与模板不一致。 退一步看：这是维护者修复自己今晨引入的回归（#9998）。故障已被观测且精确定位——main 的 唯一保留意见在流程而非代码：正文用 被审提交上的 CI 仍在进行——抓取时 — Qwen Code · qwen3.8-max Reviewed at |
wenshao
added a commit
that referenced
this pull request
Aug 25, 2026
The merge stacked two duplicate keys in this file and the previous commit removed only getCurrentCustomTitle; the sessionService fake also carries getSessionDisplayName twice. Identical expressions, and a JS literal keeps the last one — behaviour unchanged. Matches #10022's fix of main line for line, so neither merge order conflicts.
#9998 re-added getSessionDisplayName and getCurrentCustomTitle to the session-swap telemetry fakes, which already declared both. TS1117 fails tsc --build, breaking every workflow that runs npm run build on main.
yiliang114
force-pushed
the
fix/main-ts1117-duplicate-props
branch
from
August 25, 2026 13:53
d83802c to
dbc958e
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
chiga0
approved these changes
Aug 25, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Checked and approved — no blockers.
Diagnosis confirmed. On main, session-swap-telemetry.test.ts contains two properties declared twice in the same object literal:
getSessionDisplayNameat lines 186 and 197 (bothvi.fn().mockResolvedValue(undefined))getCurrentCustomTitleat lines 211 and 215 (() => undefinedthenvi.fn().mockReturnValue(undefined))
TypeScript TS1117 rejects both; the CLI package cannot compile.
Fix is correct. This PR removes the second getSessionDisplayName (the redundant identical copy) and the first getCurrentCustomTitle (the plain-function () => undefined added by #9998). The surviving getCurrentCustomTitle: vi.fn().mockReturnValue(undefined) is the original mock — a strictly better form for testing since it is spyable. Net behavioral change: none.
CI
Test (ubuntu-latest, Node 22.x): 🔄 in progress at review timeTest (macos-latest, Node 22.x),Test (windows-latest, Node 22.x): ⏭ SKIPPED (path filter; change is test-only)Desktop Shell (ubuntu/windows): ✅ passDependency CVE audit+Secret scan: ✅ pass
Reviewed with AI assistance.
qqqys
approved these changes
Aug 25, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Verified locally against upstream/main (526809d) in a clean worktree — this is the complete fix for the broken main build, and it is behavior-preserving.
Scope of the breakage. A fresh npm ci on main fails in prepare → tsc --build with exactly two errors and nothing else:
src/ui/hooks/session-swap-telemetry.test.ts(197,5): error TS1117: An object literal cannot have multiple properties with the same name.
src/ui/hooks/session-swap-telemetry.test.ts(215,7): error TS1117: An object literal cannot have multiple properties with the same name.
How it landed. #9998 (merged 08:32Z) and #9994 (merged 12:32Z) each added getSessionDisplayName and getCurrentCustomTitle to the same fakes in makeFakeEnv(), at different offsets. git blame on main confirms the split: line 186 / 215 from 2d42f4244 (#9998), line 197 / 211 from 50c553550 (#9994). The two hunks do not overlap textually, so the merge was clean and neither PR's own CI could see it — ci.yml checks out refs/pull/{N}/head, so a PR never builds the merged result.
Why this diff is right. Duplicate keys resolve last-wins, so before the fix line 197 and line 215 were the live ones. This PR deletes 197 (the survivor at 186 is the identical vi.fn().mockResolvedValue(undefined)) and 211 (the survivor at 215 already won). Runtime behavior is unchanged; only the type error goes away.
Evidence after applying this diff:
npm run build --workspace=packages/cli→ exit 0, no TS errors.npx vitest run src/ui/hooks/session-swap-telemetry.test.ts --root packages/cli→ 10 passed (1 file).
LGTM — worth landing quickly, since every PR that merges main after 50c553550 now reds at the install step on a file outside its own diff.
中文说明
已在干净 worktree 中针对 upstream/main（526809dd91）本地验证：这就是 main 构建被打断的完整修复，且不改变运行时行为。
破坏范围。 在 main 上全新 npm ci 会在 prepare → tsc --build 阶段失败，报错只有以下两条，没有其他问题：
src/ui/hooks/session-swap-telemetry.test.ts(197,5): error TS1117: An object literal cannot have multiple properties with the same name.
src/ui/hooks/session-swap-telemetry.test.ts(215,7): error TS1117: An object literal cannot have multiple properties with the same name.
是怎么合进来的。 #9998（08:32Z 合入）和 #9994（12:32Z 合入）各自在 makeFakeEnv() 的同一批 fake 上添加了 getSessionDisplayName 和 getCurrentCustomTitle，只是插入位置不同。main 上的 git blame 印证了这一点：186 / 215 行来自 2d42f4244（#9998），197 / 211 行来自 50c553550（#9994）。两处改动在文本上不重叠，因此合并没有冲突，而任何一个 PR 自己的 CI 都看不到对方——ci.yml 检出的是 refs/pull/{N}/head，PR 从不构建合并后的结果。
为什么这个 diff 是对的。 重复键遵循后者生效，所以修复前真正生效的是第 197 行和第 215 行。本 PR 删除第 197 行（保留的 186 行是完全相同的 vi.fn().mockResolvedValue(undefined)）和第 211 行（保留的 215 行本来就是生效的那个）。运行时行为不变，只是消除了类型错误。
应用此 diff 后的验证结果：
npm run build --workspace=packages/cli→ 退出码 0，无 TS 报错。npx vitest run src/ui/hooks/session-swap-telemetry.test.ts --root packages/cli→ 10 个测试通过（1 个文件）。
LGTM——建议尽快合入，因为在 50c553550 之后合过 main 的 PR，都会在 install 阶段因为一个不属于自己 diff 的文件而变红。
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
This was referenced Aug 25, 2026
wenshao
added a commit
that referenced
this pull request
Aug 26, 2026
…he split key, harden the oracle's edges R17-1: the duplicate-key cleanups each removed a DIFFERENT copy of getSessionDisplayName (#10022 on main, its twin here) and the clean merge resolved to zero — the /branch hook TypeError'd on the absent method and three telemetry tests went red. One copy restored, with a comment pinning why exactly one. R17-2: the scope-emptied split keyed on file PRESENCE, which cannot see a discarded change (file present, cited bytes gone), and no channel named the affected paths at all. The capture now publishes incremental.scope.supersededPaths — cached paths whose recorded change is gone from this capture — and the bullet routes the SUPERSEDED split through membership, both directions. R17-3: check-attr answers diff=unspecified byte-identically for the no-rule state and an explicit diff=unspecified value, so with diff.unspecified.binary configured a rendering flip crossed the anchor unseen. The config's presence now takes the whole answered dimension to UNHASHABLE; without it — every ordinary repo — nothing changes. R17-4: the candidate path is stable per target with no lease, so a concurrent same-target run overwrites it mid-round undetectably. The plan now publishes cacheCandidateStateId beside the path and Step 8 compares before promoting; a mismatch is a withheld candidate. R17-5: sparse-checkout S-tags every out-of-cone path by design, so the oracle read non-empty on every sample in every sparse repo — permanent non-convergence on a clean materialized tree. Under core.sparseCheckout an S path counts only while it exists on disk (nothing to hide an edit in); the assume-unchanged family is never exempted, and outside sparse an absent S path stays a hidden deletion. R17-6 entrance 2 (created by this diff): the 64-char cap's `-` joiner made every exactly-55-char uncapped stem a strict sweep-prefix of every capped stem sharing its head, so cleanup deleted the capped twin's live artifacts. The joiner is now `~`, outside the token alphabet, making capped stems prefix-free against ALL stems. Entrance 1 (organic dash-prefix tokens) predates this PR and is tracked in #10057. Every fix mutation-checked: dropped sample-0 exemption, reverted sparse filter, removed unspecified special-case, dropped supersededPaths, and a `-` joiner each turn exactly their new tests red.
Collaborator
|
Released in v0.22.2. |
pull Bot
pushed a commit
to bhardwajRahul/qwen-code
that referenced
this pull request
Aug 27, 2026
…w-fix loop (QwenLM#9659) * feat(review): content-anchored incremental rounds for the local review-fix loop A local or file-path review at high effort can now skip what it already reviewed, anchored on CONTENT rather than on a commit. The reviewed state is a dirty working tree: it has no commit to anchor on, and `local-diff.ts`'s standing constraint — nothing on the capture path writes to the index, the worktree, or any ref — rules out snapshot commits and stashes. So the anchor is the hashed per-file state of exactly what the plan covered, plus the HEAD the diff was measured against. `git hash-object` without `-w` computes the blob id git WOULD store and writes nothing. The identity is `<mode>:<blob>`, not the blob alone: an exec-bit flip or a file/symlink typechange is its own diff lines, so identical bytes under a different mode are not an identical change. Symlinks hash their link text at 120000, exactly what `git diff` renders. Whatever cannot be captured faithfully is UNHASHABLE — which never equals itself, and is therefore re-reviewed every round rather than silently certified. Rendering attributes ride in the identity too, from `git check-attr` rather than a hand-derivation: `.gitattributes` at any level, `.git/info/attributes`, the commondir's copy in a linked worktree, `core.attributesFile`, and the config-side `diff.<driver>.binary` that flips a section between readable hunks and "Binary files differ" with the bytes standing still. The scope is the same slicing the PR flow uses — the round's own files plus one import hop — and every refusal falls back to the full capture with its reason on stderr. Hardening from review, each mutation-checked: - The same-model gate is ruled over the provider-qualified identity the runtime publishes, recorded by the capture itself. `{{model}}` interpolates the bare model id, so two provider configurations exposing one model name passed each other's gate. - The cache is keyed by the SOURCE path, not the flattened token. `safeTarget` is not injective — `src/foo.ts` and `src_foo.ts` collide — and two different files were accepting each other's anchor and erasing each other's findings. - The target stem is capped with a digest suffix. Unbounded, a legal deep path made every write for that target throw ENAMETOOLONG. - Either side of a rename keeps its section, so a staged `git mv` no longer publishes an empty slice and spins the loop until HEAD moves. - The review's own plumbing is excluded by path segment at any depth, and on the TRACKED half too, so a round cannot review its own cache and never converge. - `check-attr`'s NUL protocol is read raw; the trimming wrapper stole the first record from any path beginning with whitespace, and failed open on exactly the driver lookup the identity exists for. - The mid-capture tree guard samples three interleaved states, so a phase-aligned write can no longer certify bytes no round reviewed. - The decided stops carry a machine-readable `nothingToReview`, so `qwen review run` stops reporting "Review did not complete" over a round that decided something. Replaces #9190, which cannot be merged or repaired: its base is the head of the closed #9188, GitHub counts it as a stack, and stack validation fails on the closed root while base changes are refused for stack members. The review is there — 20 reviews, 166 inline comments. * fix(review): close nine Criticals, most of them my own last round's Round 1 on this PR found nine, and the majority are regressions the previous round's fixes introduced. Taken in that order: **The PR cache lost its only writer.** The last round removed `lastModelId: "{{model}}"` from Step 8's template because the LOCAL capture had started recording the identity itself — but the PR flow has no such writer in this PR (that producer is the follow-up's), so every PR cache written after it carried no certifier, Step 1 omitted `--since-model`, and `fetch-pr` refused every anchor as `cross-model-anchor` for ever. The template line is back; the "do not hand-carry a model" instruction stays where it belongs, on the local bullet whose command does derive one. **Two stops contradicted each other.** The `clean-tree` decided stop was written without consulting `treeHeldStill`, so a capture whose own guard had just proved the tree moved mid-capture still ended the round as "nothing to review": stderr printed both lines back to back and the just-written change went unreviewed while the run recorded clean. It is gated now, like the skipped-content case beside it. **A third decided shape had no stop at all.** A cached path that vanished is a change by design, so the unchanged-since stop cannot fire, and the clean-tree stop is gated on `!incremental` — leaving `chunks: []` with an `incremental` block and no field: `agent-prompt --roster` threw and the parent reported "Review did not complete". That shape is `scope-emptied`, and only when no more specific stop already fired — the first cut of this overwrote `unchanged-since-last-round`, which the existing test caught. **The attribute probe was buffer-bound.** `gitWithInputRaw` inherited `execFileSync`'s 1 MB default while `check-attr --stdin -z` emits ~3 records per path — ~1.16 MB at this repo's file count. Past it the call threw, the blanket catch answered an empty map, every identity became UNHASHABLE, and since UNHASHABLE never equals itself the whole target was silently re-reviewed every round with a stable stateId and no refusal. Same 512 MiB ceiling `gitRaw` takes. **The parent polled a name the skill lets the model choose.** The stop was published inside the plan, whose `--out` is the orchestrator's to pick — necessarily, since the target token does not exist at Step 1 — so every file review's decided stop was invisible to `qwen review run`. It is a sidecar now, named from the same target the parent derives. **A decided stop passed `--fail-on request-changes`.** Both stop branches open by rendering the cache's still-open findings, and the common shape is a user who committed without fixing a Critical. Reported with no verdict, the gate returned 0 over a blocker the round itself called standing — passing the moment the author stopped touching the tree. The sidecar carries the ledger's open-blocker count and the parent maps a non-zero one to `REQUEST_CHANGES`. **The ledger reads named a token that does not exist yet.** Step 1's reads and Step 8's write all spelled `<target>.json` before the command that derives `target` had run, and `safeTarget` is not hand-reproducible past 64 characters or through a symlink. The capture publishes the resolved `cachePath` and every reader takes it from there. **Step 8's local paragraph pointed at a rule that cannot apply.** It extended "the same fail-closed rule as the PR cache" to local rounds, but that rule keys on a `sha` inside a posted review's marker, and a local round posts nothing — read literally it skipped the cache write on every round. The local conditions are stated in full instead. **The cache key did not discriminate the subject.** The anchor gate's `source` check is the second layer, not the first: it can only refuse a cache the round already opened, leaving the ledger — read and written by the orchestrator — sharing one file. `src/foo.ts` and `src_foo.ts` erased each other's findings, a root file named `local` produced the whole-tree key byte for byte, and one named `pr-<n>` produced PR <n>'s. File reviews get their own namespace and a digest of the source path. Safe to respell because nothing predicts the name any more. Every fix is mutation-checked, and the ordering bug in the third one is the reason: the fix that closes a finding is exactly as capable of opening one. * fix(review): close round 2 — the cache rename's fallout, and the stop's gaps Nine Criticals, and the shape is the same as last round: the fixes were what opened them. **The cache rename was split-brain (R2-1).** `cachePathFor` namespaces a file review's cache by source path, and the directory resolver still probed the old `<dir>/<target>.json` — so a file review reported "the cache is missing or unreadable" over a cache sitting right there. The resolver goes through the same speller now. (The follow-up PR already carried this fix; it belonged here, with the rename. Second time a fix and its dependency landed in different halves of this stack.) Step 6's prose named the old spelling too. **The stderr clean-tree warning was not gated (R2-2).** The field-level stop got `treeHeldStill` last round; the sentence beside it did not, so the round still printed "the working tree changed while the capture was being hashed" and "the working tree is clean" back to back — and the orchestrator branches on prose. It now says the tree is NOT clean and to re-run. **Three stops, two branches (R2-3, R2-12).** `scope-emptied` appeared nowhere in SKILL.md, so the reason existed with nothing to act on it; and the skipped-only shape — no chunks, non-empty `skippedFiles`, field deliberately withheld — had no executable continuation at all. Both have branches now, the second saying explicitly that it is NOT a stop. **The stop sidecar was collidable (R2-13).** It decides `completed` and can carry a REQUEST_CHANGES event, while its name is the flattened target token, which is not injective — and the epoch fence separates earlier runs, not concurrent ones. `run` stamps its child with a nonce and accepts only a sidecar stamped back. **A file review of an unmodified file read as a clean stop (R2-15).** An empty diff is not decided for a file target — SKILL.md's no-diff branch owes it a whole-file review — and marking it decided turned "Review did not complete" into a passing gate over a file nobody read. **One fixed plan name for every file review (R2-8).** File reviews take no lease and the plan is re-read all round, so two concurrent ones overwrote each other's central artifact mid-run: the second reviewed the first's file and merged its findings into the wrong ledger. The name must be unique to the run; it does not have to match anything. **The file-path ledger was write-only (R2-20).** Every ledger, incremental and stop bullet sat under the `local` branch; the `file` branch had none, so round 2 of a file review presented zero blockers over a Critical round 1 had recorded open. Mutation-checked: reverting the file-review exclusion, the sidecar stamp, or the stderr gate each turns exactly one test red. * fix(review): close round 3 — the cache records the gate's identity, and the naming prose follows the writer Two Criticals and six Suggestions. The PR cache wrote `lastModelId` from `{{model}}`, which interpolates the BARE model id — but the same-model gate inside `fetch-pr` compares whole-string against the provider-qualified identity it samples from the runtime. A bare-id cache is refused as `cross-model-anchor` on every later round and never heals (each clean round rewrites it bare again), so cache-path incremental scoping and the `upToDate` stop were dead for every PR, and a run that does not post lost the anchor entirely. Step 8 now copies the fetch report's `reviewModelId` — the CLI-published qualified identity, the gate's kind of string — verbatim, omitting the field when the runtime published none (the gate then fails closed to a full review, the designed state for an unrecorded identity). Pinned in SKILL.test.ts. The round-2 commit renamed the file-review cache to `file-<target>-<digest>.json` but left three texts spelling the old `<target>.json`: the Step-6 ledger-source parenthetical (the finding's anchor), the Step-1 incremental bullet, and `capture-local`'s own docblock/help. All four now point at the plan's published `cachePath`, and DESIGN.md's identity-channel section names what Step 8 actually types. The six Suggestions: the garbled comment restored to English; the model-refusal message's cached-side fallback aligned to `||` so a legitimately-empty recorded identity prints "an unrecorded model" instead of a blank name (regression test added); the source-path gate's stderr sentence pinned plus a hostile-source escaping variant; the malformed-cache refusal added as the third leg its test title always promised; the driver-binary fold matched segment-exactly so a driver whose name is a prefix of another (`md` / `mdbook`) no longer folds its config into the other's paths (two-driver fixture added); and the shared `IncrementalScope.fullDiffPath` declared optional, matching the PR producer that never emits it. Every guard mutation-probed: each one removed makes its test fail, each one restored makes the suite green (packages/cli review suites 4363 passed, packages/core SKILL.test.ts 24 passed; build, typecheck, lint clean). * fix(review): close round 4 — the named-path flow's six dead ends Six Criticals, all in the capture's named-path handling, all probed against the round-3 commit before fixing. **The driver fold re-parsed its own serialization (R3-1).** A driver NAME may contain a comma — `*.bin diff=a,b` is a legal gitattributes line — and the fold matched by splitting the comma-joined attribute string, which can never equal such a value. Its `binary` flag silently left the identity, so flipping the config changed how `git diff` rendered the same bytes while the identity stood still: the next round certified the previous verdict against a different rendering. The fold now matches a structured path→driver map recorded during parsing. **Directory-shaped plumbing targets reviewed nothing (R1-21, escalated).** `.qwen/reviews/` is the shape the named-path exemption was designed for, but the exemption matched `p === pathspec` — a child path never equals its parent directory — so every child was filtered with no skipped record. The exemption now keys on one named-plumbing predicate that covers the path and its children. **The same flow was dead under ignore rules (R3-2).** `.qwen/*` in `.gitignore` is the common configuration, and `--exclude-standard` applies to explicitly named pathspecs too, so the named file never reached the filter at all. A deliberately named plumbing path now wins over ignore rules; the new integration tests carry `.qwen/*` in `.gitignore`. **A directory target kept its tracked plumbing (R3-4).** The tracked drop skipped for ANY pathspec, but only a FILE-shaped pathspec is "what the user asked for": a `sub/` target kept every tracked plumbing section beneath it while the untracked half dropped plumbing descendants. The drop now runs unless the pathspec itself is plumbing, so both halves of the capture agree. **The cap gate ran after the parse it exists to skip (R1-20, escalated).** An oversized tracked diff was decoded and section-parsed before the 10 MB cap rejected it — and near `gitRaw`'s 512 MiB ceiling an all-ASCII diff decodes past Node's maximum string length, so the decode threw instead of producing the graceful skip record. The gate now runs before any decode; an over-cap diff is rejected whole. **A file review of an unchanged file was told the tree is clean (R3-3).** The field-level stop excludes `--file` captures; the stderr sentence beside it did not, so the orchestrator — which reads prose too — stopped on "the working tree is clean" over a pathspec-scoped capture that says nothing about the tree. The prose now carries the same exclusion and points at the no-diff branch's whole-file review. Every guard mutation-probed: removing it turns its witness red, restoring it turns the suite green (packages/cli review suites 4365 passed, 4 skipped; build, typecheck, lint clean). * fix(review): close round 5 — including a gate of mine that could never clear Five Criticals. Three are the same shape as the last three rounds: a rule applied to one branch and not its sibling. **The blocker gate could not be cleared (worst of the five, and mine).** Round 1's fix mapped the cache's open-Critical count to REQUEST_CHANGES so a stop round would not silently pass `--fail-on`. But the ledger is rewritten only by a round that WRITES the cache, and a stop round does not — so once the user fixes the blocker and commits, the ordinary workflow, every later round reads the same stale `open` entry and fails the gate over code that no longer contains the defect, with nothing the user can do to clear it. The CLI cannot tell that case from "committed without fixing": both leave a clean tree and a moved HEAD. A false failure no action clears is worse than a false pass sitting beside a rendered blocker list, so the synthesised event is gone. The count stays in the sidecar as reporting. The gate question's real answer is a composed verdict on the stop path — a verdict the model produces after re-ruling the ledger, not one this process invents from a file it cannot date against the code. **`scope-emptied` lacked the file-review exclusion** both sibling stops carry, so a file review whose anchored change was discarded completed as decided — while the identical tree WITHOUT a cache routed to the whole-file review SKILL.md owes a file target. Same tree, two answers. **The fail-closed list said "in full" and was not.** It omitted the two anchor-withholding classes the PR paragraph beside it names — a finding still `— [unverified]`, and an undecided blocker whose verifier never returned. Neither enters `findings[]` and neither reads as "unreviewed scope", so a local round promoted the anchor over a Critical nobody ruled on. Two that predate this PR's changes: **A decoded path is not a name.** Every invalid byte folds to U+FFFD, and beside a file literally named with one, two plan paths fold to a single key: `lstat` succeeds on the real file, the sibling inherits its identity, is never hashed, and compares unchanged for ever. The `lstat` guard cannot see it because the stat succeeds. Such paths are UNHASHABLE now — over-review, the affordable direction. **The repository root was classified as an escape.** `classifyRunTarget` accepts a directory target, so the root is reachable, and calling it an escape split the parent's pin from the child's derivation: the poll never matched and a review that had run reported no verdict, while `--file <root>` threw "resolves to <root>, which is outside the repository at <root>". Mutation-checked: each of the three code fixes turns exactly one test red when reverted. * fix(review): close round 6 — the five Criticals the re-read of HEAD still found * fix(review): close round 7 — and stop enumerating the fail-closed list Five Criticals, all judged against real git before changing anything. **The empty diff-driver name.** `*.dat diff=` is a legal attributes line, `check-attr --stdin -z` answers it with an empty value, and `git config diff..binary true` flips that section between readable hunks and "Binary files differ" with the mode and the blob standing still — verified against git 2.47.3 rather than taken on the report's word. The `value !== ''` clause dropped exactly that spelling, which was the last entrance of the family whose `set`/`unset`/`unspecified` siblings the previous round closed, under a comment claiming every answer was covered. **A driver name that did not survive the decode.** The record stream is utf8-decoded, so an invalid byte in a driver NAME folds to U+FFFD and the config probe asks for a key git never matches — nothing folds, and the flip moves the rendering with every identity component still. Such paths are UNHASHABLE, the same discipline this module already applies to a decoded PATH. That fix took two more passes before it was right, both caught here rather than in review: writing UNHASHABLE mid-loop let the path's later `binary`/`text` records append onto it, and leaving the path in the driver map let the config fold append onto it again. It is recorded in a set and applied once, after the stream. **A round that sees LESS cannot certify one that saw more.** With `--no-untracked` the untracked block never runs and records no `skipped` entries, so the skipped-content gate sees zero while a cached untracked path reads as vanished rather than out of scope: the slice keeps nothing and the round stops decided over bytes it never captured — and the stop does not advance the cache, so every later narrow round repeats it. The candidate records the capture's scope and the anchor is refused when this round's is narrower. **The plan's `--out` was still unbounded.** "Keep it short" is not a bound: a basename may be 255 bytes and the decoration adds 34. The guidance names a number now, and the guard pins the number. **The fail-closed list is gone, replaced by the rule.** This is the third round that "completed" it — cannot-tell was the shape that walked through the last completion. Write the cache only when every Critical carries a confirmed disposition; everything else withholds. The reason is one reason, which is why it is stated as one: the anchor's whole claim is "the next round may skip this", and a claim nobody ruled on is exactly what must not be skipped. * fix(review): close round 8 — the four shapes the last fixes left open Four Criticals, each reproduced with a failing test before the fix and mutation-probed after: each witness fails when its guard is removed. **An ignore rule is visibility, not deletion.** The round-6 fix closed the `--no-untracked` flag entrance, but an ignore rule added between rounds narrows the capture identically while no flag moves: the cached untracked path stops being enumerated, reads as vanished, and the scope-emptied stop fires over bytes no round captured — every round, because a stop never advances the cache. The anchor gate now probes the cached paths this capture lost: one still on disk that the base HEAD does not certify refuses the anchor, reason said out loud. A tracked file reverted to HEAD is certified by HEAD itself and keeps the designed discarded-change stop; a genuine deletion still reads as a change. **An undecodable driver name unhashes the WHOLE identity.** The round-6 fix set the attribute slot to UNHASHABLE, but the composition appended it — `100644:<blob>:unhashable` equals itself across rounds, so a rendering flip still moved nothing. What cannot be named faithfully cannot be certified: the whole identity takes it now. Pinned against real git with a raw 0xFF driver name. **The stop sidecar is snapshotted in-run.** It is the shared per-target name written with plain writeFileSync, read once after the child exits: a concurrent same-target run overwrites it (foreign stamp turns a decided round into "did not complete") and a same-stem cleanup sweep can unlink it, over a window spanning the whole child session. The capture poll snapshots it exactly like the composed verdict; the post-close read stays as the fallback for a child that exits inside one tick. **A trailing backslash is a filename character on POSIX.** The parent's pin stripped it while the child's derivation never does, so a file literally named `notes\` polled `notes` while every child artifact carried `notes_` — the review ran and posted while the parent reported no verdict, every run. The pin strips forward slashes only; the two-sides agreement is pinned against real git. * fix(review): close round 9 — date a stop's blockers; key the local cache write to the marker * fix(review): close round 10 — date each stop blocker against its own file, not the whole cached state * fix(review): close round 11 — date blockers on bytes; certify vanished paths by bytes Six Criticals from the round-10 re-read, each reproduced with an executed probe before the fix, each fix pinned by a test that fails on the pre-round branch and witnessed by a mutation probe. Blocker dating now compares mode-plus-blob, never the rendering-qualified identity: an .gitattributes normalisation commit, an info/attributes edit, or a repo-local diff.<driver>.binary flip changes no bytes and appears in no git diff, yet it unseated a standing blocker and passed --fail-on while the stop still rendered the Critical as open. And a blocker whose recorded identity is UNHASHABLE is undatable — no user action ever changes an UNHASHABLE<->UNHASHABLE comparison — so it no longer stands for ever over a fix nothing clears; it leans the documented false-pass way beside the rendered blocker list. The vanished-on-disk guard certifies by bytes, not by name: under assume-unchanged git hides the edited tracked file from git diff HEAD while ls-tree HEAD still names it, so the name check certified a divergence no round ever read and the scope-emptied stop fired DECIDED over it. The added-file veto keys on the population the cached round could actually enumerate: a scoped capture (--file, --no-untracked) skips it, and the listing excludes the review's own plumbing, so pre-existing untracked files and the cache file itself no longer permanently disarm every blocker. hashWorktreeFiles dedups at the module boundary — check-attr answers per input occurrence, so a path listed twice forged a double-suffix identity that never matched — and revisionIdentities pins LITERAL_PATHSPECS like every sibling, so one :(magic ledger path no longer fatals the whole batch and clears the gate. Also closes the two carried confirmations: the ledger key space is digest-namespaced (verified end to end for the local/pr-<n> reserved names and the non-injective token pair), and Step 9 now names the file review's duty to remove its own chosen plan name — the one artifact cleanup cannot glob without racing a concurrent run. * fix(review): close round 12 — attribute landed fixes, date by regime, stop on the published ledger (#9659) * fix(review): close round 13 — pay the isolation test's import cost at collect, not in its timeout (#9659) * refactor(review): drop the stop-round blocker-dating chain Remove blockerStateStillMatchesTree, filesAddedSince, byteIdentity and openCriticalsInCache from capture-local, the openBlockers/blockersStand fields they fed through the stop sidecar, and the standingBlockers half of exitCodeFor. A decided stop now completes with event null and exits 0 under --fail-on, unconditionally. The chain existed to let a stop round block CI while an open Critical still stood, without the failure-no-action-clears trap a raw ledger count carries. Three rounds of review showed the dating itself is the trap: every comparison regime it grew (dates, then bytes, then per-file regime bits) spawned a successor defect in the next round -- R10-2 begat R11-4/R11-6 begat R12-1/R12-2 -- because "did the user fix it" is a judgement about a mechanism, not a property of file bytes. Seven of the last sixteen review Criticals sat on these functions. What remains is what was already sound: the stop sidecar still decides completedness (reason + runId nonce), the skill still renders the cache ledger's open findings on every stop branch, and vanishedStillOnDisk stays -- it feeds the anchor gate, not this chain. The gate that can block a stop round correctly is a composed verdict on the stop path, where the model re-rules the ledger; that is a separate change. * fix(review): close round 14 — certify what git diff cannot see, split the emptied ledger (#9659) - renderingAttributes: fold a `diff` answer of `set`/`unset` to UNHASHABLE instead of the identity — check-attr answers the attribute STATE and a driver NAME that spells a state byte-identically while git renders them differently (`-diff` binary vs `diff=unset` readable, probed on git 2.39.5), so the pair cannot be certified on this stream - capture-local: hash a FILE review's subject into the anchor even with no diff section; refuse a cache whose files map is empty; condition all three decided stops (and the clean-tree prose) on `git ls-files -v` finding no assume-unchanged/skip-worktree bit, since `git diff` is blind to any edit on a marked path - SKILL.md: split the scope-emptied ledger by cited path (SUPERSEDED only for gone bytes — the gate cannot tell removed from byte-identical); name the visibility-bit shape among the withheld-stop shapes; move the file-review plan family to `file-review-…` so no cleanup sweep prefix (`qwen-review-<target>-*`) can ever match it — a target named `file` or `file-<X>` deleted concurrent file reviews' live plans mid-round - persistence.md: end the file-review report stem at `<target>` when it already ends in `.md` — the report pin never doubles the extension * fix(review): close round 15 — withhold the candidate under visibility bits, name the record dir's removal (#9659) - capture-local: condition the cache-candidate write on the same visibility-bit oracle the three decided stops already consume — `hash-object` reads the worktree bytes THROUGH a set assume-unchanged/skip-worktree bit while `git diff` cannot see them, so a candidate written under a bit recorded the identity of bytes the round's diff never showed; promoted, clearing the bit between rounds keeping the bytes left every comparison unchanged and every gate clean, and the unchanged-since stop certified them. Withheld with the reason said out loud (hoist the memoized oracle above the write; the enumeration-failed arm withholds too) - SKILL.md: Step 9's manual removal now covers the plan-derived `-prompts` record directory beside the plan JSON — the `file-review-` rename moved it out of every cleanup sweep prefix and retention scan, and nothing else removes it; drop the reverse-audit transcripts from the token-bearing inventory, because they ride the plan's free stem via that record directory - persistence.md: name the visibility-bit shapes among the capture-owned candidate withholdings Step 8 can observe as absence * fix(review): close round 15 review — bracket the visibility oracle, gate clean-tree on untracked scope R15-1: the visibility-bit oracle was sampled exactly once, after the three-pass capture loop, while the candidate write and all three decided stops conditioned on that single reading. A bit set through every diff pass and cleared before the one query read clean while the captured diffs were blind to the edit it hid — the candidate then certified the identity of bytes no pass ever showed, and the next round's comparison sliced them out of scope for ever. The oracle now rides the same both-endpoints discipline as the diffs and hashes: one sample before the first capture, one inside each re-capture pass, one after the loop. A bit visible at ANY sample withholds (the union is reported), and a single failed enumeration withholds everything. R15-2: the clean-tree stop claims "nothing staged, nothing unstaged, nothing untracked", and under --no-untracked the third clause was checked by nobody — the untracked enumeration never runs and records no skipped entries, so a tracked-clean tree with pending untracked work stopped decided and `review run` exited 0 over files no round enumerated. SKILL.md's own recovery from an oversized-untracked skip re-runs with exactly this flag. The stop and its prose twin now carry the exclusion the anchor gate has had since the candidate recorded `untracked`, said out loud; the field-less shapes bullet gains the fourth shape, with do-not-re-run guidance (the flag is the cause). Deliberately NOT a synthetic skipped entry: Step 8 withholds the cache write on non-empty skippedFiles, and a --no-untracked round anchors legitimately through the candidate's recorded `untracked` scope — a synthetic entry would have killed incremental review for that population wholesale. Both fixes mutation-checked: post-only sampling and a dropped untracked conjunct each turn their new tests red. * fix(review): close round 16 — withhold the incremental stops under --no-untracked (#9659) R16-1: the unchanged-since-last-round and scope-emptied stops lacked the args.untracked !== false exclusion their sibling clean-tree stop carries. The anchor gate's untracked clause only refuses a narrower round than the cache, so two --no-untracked rounds pass it and either stop decided "nothing to review" over untracked content neither round enumerated — a brand-new file stopped the loop while never seen. Both stops now follow the sibling: withheld, out loud, with the SKILL.md field-less bullet routing the shape to the same do-not-re-run branch. * fix(review): gate decided stops on a composed re-rule verdict (#9941) * fix(review): gate decided stops on a composed re-rule verdict (#9908) A decided stop round whose cache ledger still holds open findings used to complete with event: null, so `qwen review run --fail-on request-changes` exited 0 while its own prose said Request changes — a user who committed without fixing a Critical left a permanently clean tree and CI passed. The stop branches now re-rule the open ledger against the current tree and compose a real verdict: `compose-review` gains a `stopReRule` state, honoured only when the plan carries the capture's own `nothingToReview` decision and — under a `review run` parent — the runId-fenced stop sidecar agrees, so it exempts the round from agent-transcript floors that are unsatisfiable when no agents ran. `review run` gates the composed verdict exactly like a full round: a still-standing Critical exits 3, a re-rule that judged every blocker fixed or superseded exits 0. * fix(review): address round-1 review of the stop-round gate (#9908) - Strip stopReRule at the submit boundary: a posted verdict is a PR round's, and no PR round is ever a decided stop, so the floor exemption is illegitimate on every path that posts — closing the laundering path the round-1 review opened (R1-1). - Exempt the findingsPath floor as well: the reverse-audit delivery check is unsatisfiable on a round that ran no reverse audit, and a faithful stop state can only carry a previous round's file (R1-2). - Require the capture's stop sidecar for the exemption even when no run id is published (interactive rounds write one too), and disclose on stderr which read refused a claimed stopReRule (R1-1, R1-5). Document the fence's honest residual instead of overclaiming it. - Share the sidecar parse between run.ts and compose-review.ts in lib/stop-sidecar.ts so the stamp contract has one reader-side spelling (R1-3). - Report the decided-stop reason as stopReason in the run result so a gate can separate a clean stop from a stop that owed a verdict (R1-6). - Tests: hermetic cwd + QWEN_REVIEW_RUN_ID isolation for the suite (R1-4, R1-7); coverage for the new fence arms and stopReason. * fix(review): pin the stop-round gate against its round-2 mutations (#9908) - Handler pass-through: add a CLI-glue test asserting the stopReRule exemption survives the compose-review handler's {...parsed} spread — a plausible `delete parsed.stopReRule` (the submit idiom) left every direct composeReview() test green while the exemption died end-to-end (R2-1). - findingsPath exemption: regression-test both arms — a previous round's file with surviving unverified tags, and a since-cleaned-up path — dropping `!stopReRule &&` now turns red (R2-3). - Shared sidecar contract: STOP_SIDECAR_SUFFIX / STOP_SIDECAR_NAME / stopSidecarNameFor / isValidStopReason in lib/stop-sidecar.ts are now the single spelling the writer (capture-local) and both readers (run.ts, compose-review) derive the name family and reason check from (R2-2). - stopReason docstring: state the true contract — it separates a decided stop from a verdict round, NOT a clean stop from a lost re-rule (both are stopReason-set + composedPath-null; that shape needs the cache ledger, which the result deliberately does not carry) (R2-4). - submit strip seam test: a stop-shaped state + sidecar must still post a floored COMMENT (capped by criticals-unverified); deleting the strip now turns red (R2-5). All three code-arm mutations verified red under the new tests and green at this commit; 787 tests across the six affected suites pass. * Revert "fix(review): gate decided stops on a composed re-rule verdict (#9941)" This reverts commit af521c5d58a26acb843440a730458143733daccb. #9941 was merged into this PR branch by accident: it is the #9908 successor (composed re-rule verdicts on the stop path), deliberately kept out of this PR by the recorded R13-3 ruling — sixteen review rounds have converged on the branch as scoped, and the merge landed ~750 new lines of unreviewed mechanism (plus files outside the review surface: agent-core.ts, geminiChat.ts) at the finish line. Reverted to restore the reviewed scope; the change returns as its own PR against main once this one lands, exactly as the ruling recorded. * fix(cli): drop duplicate getCurrentCustomTitle key that breaks npm ci Carried in from main by the last merge: #9998 added the vi.fn variant of getCurrentCustomTitle to the getChatRecordingService fake while #9994's Live Host restore re-added the plain arrow variant in the same literal — each PR built green alone, the clean merge stacked both keys, and `tsc --build` fails the cli package with TS1117 inside `npm ci`'s prepare step, so this PR's Install dependencies step dies on fresh runners. Keep the vi.fn variant: a JS literal keeps the LAST key, so it is the one every run actually used — behaviour unchanged, suite 10/10. Same one-line fix as #10023 proposes for main; landing it here unblocks this PR's CI without waiting, and the two are byte-identical so neither merge order conflicts. * fix(cli): drop the second duplicate key too (getSessionDisplayName) The merge stacked two duplicate keys in this file and the previous commit removed only getCurrentCustomTitle; the sessionService fake also carries getSessionDisplayName twice. Identical expressions, and a JS literal keeps the last one — behaviour unchanged. Matches #10022's fix of main line for line, so neither merge order conflicts. * test(review): pin the visibility oracle's sample count The sandboxed verifier's mutation matrix caught the one unpinned guard (M6): the visibility test's scripted mock consumes by CALL ORDER, so deleting sample 0 in production merely shifts the script — the dirty sample still lands on a later query and the test stays green. Pin the count: four samples exactly, 0 before the first capture, 1-2 in the loop, 3 after. Replanted M6 locally — the pin turns exactly this test red ("expected 3 to be 4"), matching the verifier's proof fixture. * fix(review): close round 17 — restore the merged-away fake, publish the split key, harden the oracle's edges R17-1: the duplicate-key cleanups each removed a DIFFERENT copy of getSessionDisplayName (#10022 on main, its twin here) and the clean merge resolved to zero — the /branch hook TypeError'd on the absent method and three telemetry tests went red. One copy restored, with a comment pinning why exactly one. R17-2: the scope-emptied split keyed on file PRESENCE, which cannot see a discarded change (file present, cited bytes gone), and no channel named the affected paths at all. The capture now publishes incremental.scope.supersededPaths — cached paths whose recorded change is gone from this capture — and the bullet routes the SUPERSEDED split through membership, both directions. R17-3: check-attr answers diff=unspecified byte-identically for the no-rule state and an explicit diff=unspecified value, so with diff.unspecified.binary configured a rendering flip crossed the anchor unseen. The config's presence now takes the whole answered dimension to UNHASHABLE; without it — every ordinary repo — nothing changes. R17-4: the candidate path is stable per target with no lease, so a concurrent same-target run overwrites it mid-round undetectably. The plan now publishes cacheCandidateStateId beside the path and Step 8 compares before promoting; a mismatch is a withheld candidate. R17-5: sparse-checkout S-tags every out-of-cone path by design, so the oracle read non-empty on every sample in every sparse repo — permanent non-convergence on a clean materialized tree. Under core.sparseCheckout an S path counts only while it exists on disk (nothing to hide an edit in); the assume-unchanged family is never exempted, and outside sparse an absent S path stays a hidden deletion. R17-6 entrance 2 (created by this diff): the 64-char cap's `-` joiner made every exactly-55-char uncapped stem a strict sweep-prefix of every capped stem sharing its head, so cleanup deleted the capped twin's live artifacts. The joiner is now `~`, outside the token alphabet, making capped stems prefix-free against ALL stems. Entrance 1 (organic dash-prefix tokens) predates this PR and is tracked in #10057. Every fix mutation-checked: dropped sample-0 exemption, reverted sparse filter, removed unspecified special-case, dropped supersededPaths, and a `-` joiner each turn exactly their new tests red. * fix(review): close round 18 — ask git for the cone, make the cap idempotent, keep the stop evidence R18-1: tmpFile/tmpPrefix apply safeTarget a second time to tokens other callers already derived with it, and the second flatten rewrote the capped `~` joiner to `_` while run's pins interpolated the single-flatten spelling raw — every deep-path round wrote one name and polled another, resurrecting the never-matching-poll regression this PR exists to kill. safeTarget is idempotent over its own outputs now (the capped shape is recognized before the flatten); the pass-through that permits is no wider than the flatten's own collisions. R18-2/3/4: the sparse exemption re-derived git state by hand and got all three edges wrong — a raw --get missed legacy truthy spellings and read an inherited global; "absent + S" is not "out of cone" on gits that retain a manual bit inside the cone (2.43 does, 2.47 re-clears); and the combined-bit lowercase `s` spelling missed the S-only match. The oracle now asks git at every step: the flag reads --worktree --type=bool (sparse-checkout writes config.worktree via extensions.worktreeConfig), cone membership comes from `git sparse-checkout check-rules`, and the rule is uniform over every tag — exempt exactly the paths that are absent AND outside the rules; a failed check-rules exempts nothing. The version-dependent arms real git 2.47 cannot stage (it re-clears the manual bit and canonicalizes the bool even on read) are pinned by a scripted-git unit suite beside the real-git integration arms. R18-5: Step 9's file-review `-prompts` removal was unconditional, and it is that family's only remover — every unconverged file review destroyed its own budget-stop evidence on the way out, voiding the #9206 retention cleanup implements for swept families. The removal now keeps the plan and the record directory when the budget-stop marker is present, with cleanup's Kept-line wording. Mutation-checked where constructible: the exempt-all-absent mutant is killed by the scripted in-rules arm (proven live-red before restore), the joiner and idempotence mutants by paths.test, the flag-read mutant by the 2.43-class CI git via the raw-spelling arm. * fix(review): close round 19 — only ENOENT is absence, undecodable is never exempt Three foldings of failure semantics onto the safe side, all instances of the discipline hashWorktreeFiles and revisionIdentities already carry: R19-1: the sparse exemption ran both its halves on a name the decode had already mangled — lstat missed the PRESENT file through the U+FFFD spelling and check-rules was fed bytes git never knew — so a bit-carrying present file was silently exempted. An undecodable name is never exempt. R19-2: every lstat failure folded into "absent", so a PRESENT flagged path under an unmeasurable ancestor (EACCES/ENOTDIR/ELOOP) was exempted while git diff stayed blind to its bytes. Only ENOENT proves absence; anything else stays flagged. R19-3: vanishedStillOnDisk folded every lstat failure into "genuinely gone", ending the round at a decided scope-emptied stop over bytes no round captured. Only ENOENT proves deletion; an unmeasurable cached path stays in the on-disk set, whose downstream is an anchor refusal at the cost of a full round. Deterministically staged as root via ENOTDIR (a regular file where the ancestor directory was) — EACCES needs a non-root runner. All three foldings mutation-checked red. * fix(review): close round 20 — bool-typed fileMode, no directory in the anchor, certify gitlinks R20-1: `core.fileMode` was read with a raw `--get`, which echoes the stored spelling, so `off`/`no`/`0`/`FALSE` all failed the `!== 'false'` test and silently disabled the exec fold — the same misreading R18-2 fixed for `core.sparseCheckout`, at the call site that fix did not touch. Read `--type=bool`, and fold only on an EXPLICIT false: the knob defaults to true and an unset one must not erase a real divergence. R20-2: a FILE review's subject entered the hashed population unconditionally, so `qwen review <dir>` recorded the directory itself as UNHASHABLE in every candidate. UNHASHABLE never equals itself, so `changedSince` reported the directory every round and the unchanged-since stop was unreachable for that target for ever — the very non-convergence `movedSince` was added to close. A confirmed directory is skipped now; its files carry the bytes, and an unmeasurable subject keeps the pre-existing coverage. R20-3: a submodule gitlink is UNHASHABLE on both sides by design, so the both-unhashable refusal wedged the loop for ever once a round had touched one. Git measures submodules itself and the pinned flags keep them in the capture, so a gitlink's absence from the diff is git's own answer that the pointer did not move. Which paths are gitlinks is asked of `ls-tree`, not inferred from the placeholder they share with undecodable names — those still refuse. R20-5, disclosed rather than folded: `core.symlinks=false` erases the file↔symlink type the same way, and the comment here claimed the type survives every fileMode — true of fileMode, false of this knob. A mode fold does NOT close it: the two spellings also differ in whether they carry rendering attributes at all, and equalizing those would drop the rendering dimension for that path. The bounded over-review stands, now pinned by a test so it cannot become a silent certification. R20-4 is pre-existing and NOT introduced here — before this PR the file token was the basename, so a `local` at ANY depth collided; the repo-relative derivation narrowed that to the repo root. The general class is tracked in #10057. Step 9 gains the cheap guard meanwhile: a file review whose token derives to a reserved name must not run `cleanup` at all, and removes only what it wrote. Every code fix mutation-checked red. * fix(review): close round 21 — measure the pointer, ride the skipped lists, one path spelling R20-3 follow-up: the gitlink exemption took the diff's ABSENCE as git's answer that the pointer did not move — but with the submodule's gitdir gone that absence is git's silence. The pointer is now read directly (`rev-parse HEAD` inside the submodule) and compared to HEAD's recorded oid; a submodule whose HEAD cannot be read is unmeasurable, and unmeasurable is uncertifiable. R20-4 follow-up: the reserved list was under-enumerated — a repo-root file literally named `pr` derives the bare token `pr`, whose sweep prefix engulfs EVERY PR family while the lease guard lives inside the `pr-<n>` branch a bare `pr` never enters. `cleanup` refuses the token outright now (no target legitimately owns that prefix), and the Step 9 reserved list names it. R21-1: `repoRelativeOf` returned node's platform-separated `relative()` output verbatim, which flows into git pathspecs, the candidate's recorded `source`, and `cachePathFor`'s digest — on win32 one file got two cache filenames across platforms and the Windows lane failed every posix-spelled assertion. `rel` is normalized to forward slashes (git's own spelling on every platform), after the escape check computes against the platform separator. R21-2: the sampling loop kept only `.diff` from re-captures 1 and 2 — a file entering the window that lands in a skip class is in no capture's diff BYTES, so the byte comparison read "held still" while two of the three captures explicitly skipped content, and every stop gate reads only capture 0's list. Skip-set movement is tree movement: the skipped path-sets ride the treeHeldStill comparison now. All but the win32 normalization mutation-checked red locally (that branch is a no-op where sep is '/'; the Windows lane is its enforcement). * fix(review): close round 22 — a gitlink's identity is real, on both sides R22-1: a moved or in-diff submodule pointer entered the hashed population and could only record UNHASHABLE — which never equals itself — so changedSince reported it every round and the unchanged-since stop was unreachable for the lifetime of any change set holding a dirty pointer, with a misdescribing diagnosis. Yet the module already measures exactly this identity (the R20-3 fix did it inside a special case). Make the identity real instead: hashWorktreeFiles answers 160000:<oid> for a readable, content-CLEAN submodule (the pointer oid says nothing about internal edits — git renders those as `-dirty` — so cleanliness is part of measurability and a dirty submodule stays UNHASHABLE), and revisionIdentities answers the same shape from ls-tree's recorded oid. With both sides real, vanishedStillOnDisk's R20-3 special case became redundant and is deleted along with its helper: a restored pointer certifies through the ordinary equality, an unreadable or dirty one refuses through the ordinary UNHASHABLE clause — net-negative plumbing. The submodule fixture now pins all three arms: the dirty pointer CONVERGES (unchanged-since reachable), internal dirt never certifies (no decided stop over `-dirty` bytes), and the odb-removed pointer still refuses. Both mutants — UNHASHABLE-again and dirt-invisible — turn it red. * fix(review): close round 23 — withhold over the dropped-out set, exclude file targets, keep every byte Six findings, all confirmed against the tree: The candidate write now gates on the dropped-out-while-on-disk set: a refused-anchor round used to write a candidate that silently OMITTED the dropped path, Step 8 promoted the omission, and two rounds later a scope-emptied stop certified bytes no round read. The cache is read before the write now, the withholding is voiced with its own sentence, and the scoping branch reuses the early read. The unchanged-since-last-round stop gains the file-review exclusion BOTH sibling stops carry — a cached round-2 file review of an unmodified subject stopped decided while the identical tree without a cache routed to the whole-file review. The excluded shape gets its own honest stderr line instead of falling into the unhashable-paths diagnosis, and the directory-subject test now pins convergence as the ABSENCE of the wedge. gitlinkIdentity asks the submodule's OWN visibility bits: status --porcelain honours an assume-unchanged bit set inside the submodule, so cleanliness judged by status alone held the identity still over interior bytes no round can see — the fix-induced half of R22-1, closed with the same oracle one level down. canonicalise strips only the platform's separators on the ancestor walk: `\` is a legal POSIX filename byte this PR's own fixtures insist on, and the two-class strip corrupted a dangling `\link` into `link`. SKILL.md: the Step 1 file bullet's --out template carries the 24-char truncation (the full-basename spelling died with ENAMETOOLONG past ~226-byte basenames, measured), and both PR stops (up-to-date, empty diff) now write the stop sidecar with the run's nonce before cleanup — the reader predicted the name but nothing in the PR flow ever wrote it, so every decided PR stop exited 1 "Review did not complete". All four code fixes mutation-checked red; guards pin both prose fixes. * fix(review): keep this run's stop sidecar across its own cleanup Human review on #9659: the round-23 PR stop protocol writes the sidecar and runs `cleanup pr-<n>` in the same breath, but cleanup swept the same prefix — and the parent's first in-run poll is up to 250 ms away, so a write+cleanup finishing before the first tick left NOTHING for either the snapshot or the post-close fallback to read: an already-decided up-to-date/empty-diff round still exited 1 "Review did not complete". The local flow never hit this only because its stop (Step 1) and its cleanup (Step 9) are minutes apart. cleanup now spares `<prefix>stop.json` exactly when its runId matches the environment the parent stamped — a foreign or unstamped sidecar is residue and sweeps as before, and the NEXT run's cleanup (different nonce) collects this one. SKILL.md says so beside the write instruction, so the orchestrator does not "tidy" the file by hand. Pinned from both sides of the window: cleanup.test spares the matching sidecar and sweeps a foreign one (mutation-checked red); run.test gains the zero-timer-advance arm — child writes the sidecar and closes on a microtask, before ANY poll tick, and the post-close fallback still reads the decision (the existing race arms all advanced 1000 ms first, which is exactly how the window went unpinned). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Main is currently red: every workflow that installs dependencies fails during the prepare build, e.g. the SDK Java daemon E2E run and the npm cache producer run from the latest push to main.
The root cause is a test-fake regression from #9998: the session-swap telemetry fakes already declared a session display name lookup and a custom title lookup, and that PR added both properties a second time in the same object literals. TypeScript rejects duplicate object properties (TS1117), so the CLI package no longer compiles.
Changes
Removes the two duplicate declarations, keeping the versions added by #9998 (the later property wins at runtime today, so behavior is unchanged). Two lines deleted, nothing else touched.
Reviewer Test Plan
How to verify: check out this branch, run
npm install(its prepare step performs the full build that fails on main), and confirm it completes. Running the session-swap telemetry test file should show all 10 tests passing. Verified locally: full build succeeds and the suite passes.