test(review): pin the fold invariant the per-line escape rests on - #11533
Conversation
`escapeTagOpeners` models CommonMark INLINE structure only. That is correct exactly while every model-written channel folds its text to a line before it, and the fold lives at five sites, not one — `ingestEntryList` for the entry channels, the `\s+` normalisation for downgrade reasons, `collapseEntry` for the `Not reviewed:` disclosures, `scriptLintGate`'s own push, and compose's refusal of a line break in a ruling note's `by`. `scriptLintGate` is the proof that a sixth channel can appear and be missed: it joins `bodyCriticals` after `ingestEntryList` has run, and for a while it did not fold, so a fenced block in a side file the review agent can rewrite reached the escape as several lines and rendered a literal `&lt;` to a reviewer. Nothing in the code enforced that. This adds the guard: one case drives every channel with a multi-line entry — LF, CRLF and a lone CR — and asserts the CONSEQUENCE, so a new channel that skips the fold reddens here instead of mangling a review. It also pins the two channels that REFUSE rather than fold (a quoted fence on either Critical list, and a ruling note's `by`), and the one that renders as a code span, where an escape would show the reader a literal `<` instead. Removing the fold from `scriptLintGate` or from the disclosures reddens it. Two boundaries that reverse audits keep rediscovering are recorded rather than changed, both measured against cmark-gfm — the renderer GitHub runs — and an HTML parser: - A reference link's LABEL is ordinary inline text: a backtick inside `[a`b][c]`, `[x][a`b]`, `[a`b]` or a link reference definition's own label is a code-span delimiter like any other, and the tag it pairs over is code. Two audits reasoned the other way and neither could build a counterexample; the rows say so now. - `rendersAsNothing` is fold-blind and position-blind on purpose. An arm that deleted a `<details>`'s content would delete a quoted `` `<details>` `` too, which is the shape a review of that file writes — it loses a real finding to catch a readable one. And splitting the `<?` arm by position was tried and REVERTED: on a prose-weighted corpus of 30000 bodies the split alone traded 121 -> 116 bodies wrongly called empty for 1426 -> 1730 wrongly called visible, and teaching it the container prefixes then needs the container's EXTENT as well — without that, 121 -> 3594. Being right there needs the block structure the module deliberately does not model.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@wenshao — this run stops at the template gate, so this is the only output you'll get from it. No code review yet.
The body doesn't carry the required headings from .github/pull_request_template.md. It has two headings of its own — ## What this adds and ## Two boundaries recorded rather than changed — and the bilingual 中文说明 block is present, but all eight required sections are missing:
## What this PR does## Why it's needed## Reviewer Test Plan, with### How to verify,### Evidence (Before & After),### Tested on## Risk & Scope## Linked Issues
The prose is already most of the way there, so this looks like a re-heading job rather than a rewrite — the opening paragraphs are the "why" and ## What this adds is the "what". Two of the missing sections are where this PR specifically owes content, though:
- Reviewer Test Plan → How to verify. The entire change is tests, so mutation evidence is the only thing that shows they're load-bearing rather than decorative. The body already claims that removing the fold from
scriptLintGateor from the disclosures reddens the new case — that belongs under How to verify with the actual failing output pasted in, not asserted in prose.### Evidence (Before & After)can beN/Asince nothing here is user-visible, and### Tested onis just the platforms you ran the suite on. - Risk & Scope. The second recorded boundary pins behaviour your own comment calls wrong-but-deliberate:
rendersAsNothing(' <?x>')assertedtrue, while the note above it says those characters are VISIBLE in the renderer and calling the body empty is wrong. Pinning that is a defensible call, but it's a tradeoff a future fix has to unpick, and "Not validated / out of scope" is exactly where it should be on the record.
## Linked Issues can stay a non-closing reference — "Follow-up to #9940" reads right, and I confirmed it currently resolves to no closing reference, which is what you want for a follow-up.
Worth saying this is a lapse rather than a habit: #11524 and #11516 both carried the full template. Update the body and re-run /triage — the review picks back up at Stage 1.
中文说明
@wenshao — 本次运行停在模板关卡，所以这是它唯一的输出，还没有进入代码审查。
PR 正文没有带上 .github/pull_request_template.md 要求的标题。正文有两个自己的标题——## What this adds 和 ## Two boundaries recorded rather than changed——双语的 中文说明 块也在，但八个必需章节全部缺失：
## What this PR does## Why it's needed## Reviewer Test Plan，含### How to verify、### Evidence (Before & After)、### Tested on## Risk & Scope## Linked Issues
正文的散文其实已经完成了大半，所以这更像是改标题而不是重写——开头几段就是"为什么"，## What this adds 就是"做了什么"。但缺失的章节里，有两个是这个 PR 特别需要补内容的：
- Reviewer Test Plan → How to verify。 整个改动都是测试，所以变异证据（mutation evidence）是唯一能说明这些用例真正起作用、而不是摆设的东西。正文已经声称：把
scriptLintGate或披露渠道的折行去掉，新用例就会变红——这句话应该放到 How to verify 下面，并贴上真实的失败输出，而不是只在散文里断言。### Evidence (Before & After)可以写N/A，因为这里没有用户可见的改动；### Tested on就写你实际跑过测试的平台。 - Risk & Scope。 第二条被记录的边界钉住的是你自己的注释都称之为"错但故意"的行为：
rendersAsNothing(' <?x>')断言为true，而它上方的注释说这些字符在渲染器里是可见的，把正文判为空是错的。钉住它是一个可以辩护的选择，但这是一个将来修复时必须先解开的取舍，而"Not validated / out of scope"正是它该被记录在案的地方。
## Linked Issues 可以保持非关闭式引用——"Follow-up to #9940" 是合适的，我也确认了它目前不产生任何关闭引用，这对后续 PR 来说正是想要的效果。
需要说明的是这是一次疏漏而非习惯：#11524 和 #11516 都带全了模板。更新正文后重新运行 /triage，审查会从 Stage 1 继续。
— Qwen Code · qwen3.8-max-2026-09-02
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Blocking finding(s) follow.
Partially reviewed — gaps disclosed. Suggestions are inline.
Unresolved, please confirm:
- [Critical] Live CHANGES_REQUESTED from the triage gate (review 5163484124, @qwen-code-ci-bot) — it asserts the PR body is missing all eight template sections and explicitly files no code defect ("No code review yet"), so it cannot be ruled on from the…
Not explored to full depth (tool budget reached): "agent 3c": grepping compose-review.test.ts and submit.test.ts for an existing rendersAsNothing / rendersAsNothingAtExit pin on a <details> -wrapped body..
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 3c"：grepping compose-review.test.ts and submit.test.ts for an existing rendersAsNothing / rendersAsNothingAtExit pin on a <details> -wrapped body.。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.23.2)
Review found that the case added here, and the comments beside it, claimed
more than they delivered. Four reverse-audit rounds then found the same
class of defect four more times — each time in a sentence written as
MEASURED that had not been. Everything below is now checked by a mutation
that reddens it.
The helper had three assertions and two of them could not fail.
`body.split('\n')` does not split on a bare `\r`, and the filter keeps the
entry's first physical line, so the line count read 1 whatever happened to
the rest; and `&lt;` can only appear if a model wrote those characters
itself, since the escape emits `<` and nothing in the source emits
`&`. Both are replaced by the invariant the fold actually promises: the
entry's TAIL on the same line as its head, and no `\r` or `\n` left inside
it. That is what a surviving break costs the reader — the per-line escape
decides each half on its own and can post a tag the renderer holds live.
The case now drives eight rows, not five. It gained the downgrade reasons,
which need no fixture at all, and a Critical deferral's RELOCATION exit —
a second producer that joins `bodyCriticals` after `ingestEntryList` has
run, which a `Suggestion` row never reaches. Removing the fold at either
site reddens the case; before, both were silently uncovered.
The comments now name what the experiment shows:
- Three rows are held by more than one fold — the deferral list, the
cannot-tell leg, and the relocation exit, that last one three times over
— so they cannot redden on a single-site regression, and the other five
do. The earlier text said two and six, and credited the relocation exit
to two holders that can both be broken with the row still folded.
- The deferral LIST line never reaches `escapeTagOpeners`; it posts as
`- ${mdField(entry)}`. The relocation line does reach it and comes back
unchanged. Both are asserted inert as a code span, for different
reasons.
- `ingestEntryList`'s `\r\n?` normalisation folds nothing on its own;
`collapseEntry` is what carries the entry and disclosure legs.
- `rendersAsNothing` stays position-blind because `scanLines` discards
`token.map` and returns a block-quote-only depth — not because a parse
would cost too much. That path already runs one:
`canProjectFooterMarker` opens on a `<`, which every body these arms
judge carries by construction. The earlier text blamed cost and put the
figure at the parser's own inline-pass note, which is off by two orders
of magnitude.
The corpus numbers behind the reverted `<?` split keep their conclusion and
lose their false precision: the axis of the third figure is named, and both
they and the parse comparison are marked directional rather than
reproducible, because the absolute values swing more than twentyfold with
the body's shape.
No behaviour change: outside the tests, only comments move.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 114 passed · 4 failed · 118 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：114 通过 · 4 失败 · 118 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11533 — deep verificationVerdict: The 4 fails are my harness's mispredictions, not PR defects — see Nothing here is a runtime defect. The PR's "no behaviour change" claim is 中文摘要结论 A/B 结论：本 PR 只有测试与注释，所以"载荷证明"是变异 A/B。中心主张（新用例真的钉住了折行不变量）成立：18 个单点变异中 8 个让用例变红，其余 10 个恰好就是注释所说"被多处折行共同守住"的那些单一持有者，加上它自己声明本用例不覆盖的 gate push，以及三个针对别的行的变异。"无行为变化"用 esbuild 压缩后逐字节比对证明（两个源文件与 base 完全一致）。见 Mutation matrix 与 Behaviour neutrality 两表。 findings：一条 Suggestion —— 新测试注释里写着 未覆盖范围：逐 commit 归因（浅克隆，快照 3 个 commit 本地只有 1 个）；快照里的 Central claim
This is a test-only PR, so the load-bearing question is not "does it pass" but Behaviour neutrality (the "only comments move" arm)
Mutation matrix (the "it reddens" arm)
Each mutant breaks one named fold site in a scratch copy of the production 8 of 18 mutants redden the case. The 10 that do not are not survivors of
Every claim in the case's comment that I could turn into a mutation held, with
Repro: Census: does "every channel" mean every channel?
The case is named "every model-written channel reaches the escape", so I
CorrectionsThese are corrections to descriptions, not requests to change code. The PR description's
That is precisely the reasoning commit 3 used to delete the helper's Findings1. Suggestion — the deferral-list fold has three holders, not two, and the sentence saying otherwise is marked
|
| broken holders | real case | LIST row | RELOC row |
|---|---|---|---|
| C | green | folded | folded |
| D | green | folded | folded |
| F | green | folded | folded |
| C+D | green | folded | folded |
| C+F | green | folded | folded |
| D+F | green | folded | folded |
| C+D+F | RED | UNFOLDED | UNFOLDED |
No pair unfolds the row; only the triple does. The two legs behave
identically in all seven cells, which is the direct proof they have the same
holders. Repro: node tmp/pr11533-verify-20260910-121626/03-deferral-lattice.mjs
(17/17 assertions).
Why it matters. Nothing at runtime — the source edit is comment-only,
proven byte-identical above. It matters because this PR's entire second commit
is "say only what it measures", and because the same comment draws a contrast
that does not exist: "Two more legs are held twice: cannot-tell by
collapseEntry AND collapseToLine, and the RELOCATION exit THREE times."
At the granularity the comment uses for the relocation exit (invocation sites,
not functions), all three multiply-held legs have three holders — cannot-tell
is collapseEntry + boundDeferredLine's collapseToLine + asListLine's
collapseToLine (compose-review.ts:2050), which the AE mutant confirms:
breaking two of its three leaves it folded. A maintainer adding a fourth
deferral exit would size the guard at two holders instead of three.
Suggested fix (comment-only; backed by the lattice above, no new harness needed)
Replace the TWICE … only with both broken sentence with the same shape the
comment already uses for the relocation exit, e.g.:
// the deferral leg is held THREE times, exactly like the relocation exit —
// `collapseToLine` at `toDeferredEntries`, again inside `boundDeferredLine`
// (which `deferredShown` runs before the list posts), then `mdField`'s
// line-ending strip. MEASURED over the whole 2^3 lattice: break any ONE or
// any TWO and the row is still folded; only all three let the bare CR reach
// the POSTED LINE.
and change "Two more legs are held twice" to name three holders for the
cannot-tell leg too (collapseEntry, and collapseToLine twice — at
boundDeferredLine and at asListLine), so one granularity is used throughout.
This is a wording change whose every clause the lattice table above already
measures, so it needs no new fixture; the suite is green with and without it,
which is exactly the unpinned-axis signal — the row that would pin it is the
C+D+F cell.
2. Nit — the case's title claims more than two of its own rows deliver
The case is titled "every model-written channel reaches the escape as ONE
line", but two of its eight rows are asserted not to reach the escape: the
deferral list line posts as - ${mdField(entry)} with no escapeTagOpeners
call at all (verified by census), and the row is asserted as a code span
instead. The body comment states this accurately — "The deferral LIST line …
never reaches escapeTagOpeners at all … The fold is what all eight share" —
so only the title is loose. Given that commit 3 exists to remove sentences
that claim more than they deliver, the title is the one left. A title naming
the shared property ("every model-written channel is folded to ONE line before
it posts") would match all eight rows.
3. Nit — the <details> blind spot is recorded as a decision but gets no row
The new review-footer.ts comment records two blind spots as decisions.
BLOCK CONTEXT gets a pinned row ( <?x>), and I verified the pin is a
genuine tripwire — the position-split mutant reddens exactly that row and
leaves the other five. FOLDS gets prose only. I measured the behaviour it
describes (logs-07-fold-blindspot-probe.json):
| body | rendersAsNothing |
|---|---|
<details><summary>s</summary>the auth check is missing</details> |
false — posts and counts, as the comment says |
<details><summary>the auth check is missing</summary></details> |
false |
nested <details> |
false |
| fold + trailing prose | false |
`<details>` quoted in a code span + prose |
false |
No row in review-footer.test.ts pins any of these (the file's only details
hits are unrelated prose, for details). So a future arm that did delete a
fold's content — the change the comment argues against — would redden nothing.
One row would close it, and the expected value is measured above:
expect(rendersAsNothing('<details><summary>s</summary>the auth check is missing</details>')).toBe(false).
The comment does not claim a pin here, so this is completeness, not a wrong
statement.
Not a finding — the comment's other structural claims all check out
Recorded so the absence of a finding is not read as an absence of checking:
- "
scanLines… returns{line, kind, depth, content}— the extent is
discarded, anddepthcounts block-quote markers only, so a list item reads
0" — true (review-footer.ts:753-779;QUOTE_PREFIX_REis the only depth
source). - "this path already pays for a run of it:
stripReviewFooteropens
canProjectFooterMarkeron a<… and reachesscanLines" — true:
stripReviewFooter(:563) gates oncanProjectFooterMarker(:564, which
testsincludes('<')) then callsblankQuotedCode(:635), whose first
statement isscanLines(body)(:636).rendersAsNothingitself is a pure
regex chain with no parse, so "the parse is already in the bill, the
projection is what stops it" is the right diagnosis. - "the relocation exit THREE times —
collapseToLineattoDeferredEntries
and again insideboundDeferredLine, thenmdField" — true, and the CDF
cell confirms all three are needed.
Assertion accounting
assertions.json reports 114 pass / 4 fail / 118 total, counting every
scripted check that executed. The 4 fails are all in harness 02 and all are
my predictions, revised afterwards on independent code evidence:
| fail | what I predicted | what happened | resolution |
|---|---|---|---|
CL unfold set |
duplicates + both deferral rows | duplicates only | I had not yet read deferredShown…map(boundDeferredLine); mdField holds the deferral rows. Re-run with the corrected prediction in harness 04 — confirmed |
DF case reddens |
RED | green | Same root misprediction. Superseded by harness 03's full lattice — and this is the measurement behind Finding 1 |
DF unfold set |
deferredSuggestions | none | ditto |
GT footer rows |
1 row flips | 2 flip | My error about noGt poisoning the whole family. Re-run corrected in harness 04 — confirmed |
Three of the four trace to trusting the PR's own holder count, which is how
Finding 1 surfaced; one (GT) was purely my reasoning error. Both corrected
re-runs pass (harness 04: 24/24), and the original logs are kept
(logs-02-mutation-matrix.txt) rather than overwritten. Under the contract a
nonzero fail bars merge-ready; the verdict here is findings on Finding 1
regardless.
Not covered
- Per-commit attribution. The checkout is shallow (
git rev-parse --is-shallow-repository→ true).git rev-list --count HEAD^1..HEAD^2
returns 1 while the snapshot lists 3 commits;8f7141d8and the merge
00f82a72are absent locally. I verified the aggregateHEAD^1..HEAD
diff only. This is exactly the shallow-boundary trap where the bare count
looks plausible instead of erroring. - Base OID drift. The snapshot's
baseRefOidfe7cf686…is absent
locally. The A/B base used isHEAD^1=76635386, the merge-ref's base tip,
which is the correct control for this checkout shape. - Trial merge into current
main.mainis not reachable at depth 2, so
I could not confirm the merge is conflict-free or re-run the suite on the
merged tree. - The corpus figures in the
review-footer.tscomment (30 000 bodies;
121 → 116 wrongly-empty; 1426 → 1730 wrongly-visible; 121 → 3594). Not
re-derivable — the comment itself says the corpus was one-off and not kept,
and marks the figures directional. I verified the structural premises
those numbers rest on instead (the three bullets at the end of Findings).
No A/B was possible on the figures themselves. - cmark-gfm agreement. The rows and comments say "MEASURED against
cmark-gfm and an HTML parser". This container has neither, so I did not
re-verify that the renderer agrees. What I did verify is different and
weaker: that the rows have teeth against a plausible wrong implementation
(mutantR). "The rows catch the wrong reading" is not "cmark-gfm says the
right reading". - Repo-wide gates. Not run:
npm run lint,npm run format,
npm run typecheck, and the rest of thepackages/clisuite. The PR's own
CI covers them; per the method I ran only the affected workspace's two
changed test files. - The
<details>fold arm's cost side. Finding 3 measures that a fold
hiding prose posts. I did not measure the counterfactual the comment argues
against — how often a review of this very file quotes`<details>`— so
the trade itself is taken as the comment states it. - No live GitHub or wire harness. The PR posts nothing new; there is no
network surface in the diff, and this job has no token.
Methodology
Environment: the CI verify container (node:22-bookworm, Node v22.23.2),
working tree at refs/pull/11533/merge, npm ci + npm run build already
complete at HEAD. All five harnesses are .mjs/.ts files in this directory so
a maintainer can rerun them; raw per-cell logs are logs-*.txt, structured
results results-*.json, images evidence/*.png.
Because the PR is test-only, the A/B is a mutation A/B rather than a
base-vs-head build comparison. Harness 02 applies each mutant to the real
production source in the working tree, runs, then restores with
git checkout -- and asserts the restore left the tree clean before the
next mutant; git status --porcelain is clean at the end and the two changed
test files were re-run green on the pristine tree (839/839, logs-06). The
base-vs-head comparison is instead done on executable content: esbuild --minify byte-equality of the two source files at HEAD^1 and HEAD.
The real case aborts at its first failed expect, so row attribution comes
from three instrumented probes appended to a scratch copy of
compose-review.test.ts (compose-review.mutprobe.test.ts, deleted after the
run — the real test file was never modified). The copy reuses the PR's own
base()/coveredPlan() fixtures, so the probes drive the identical inputs;
the fold probe re-asserts the identical three invariants but records all
29 rows instead of stopping at the first. Probes write JSON to files because
packages/cli/vitest.config.ts sets silent: true, which swallows
console.log. Every run asserts its collected-test count (5 for the matrix,
1 for the escape probe) so a -t filter that matched nothing could not read as
a green run; the unmutated control run is the matrix's first row.
Flakiness gate log
rounds=5 files=2 skipped=0
file packages/cli/src/commands/review/compose-review.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/compose-review.test.ts
file packages/cli/src/commands/review/lib/review-footer.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/review-footer.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/compose-review.test.ts: PPPPP
packages/cli/src/commands/review/lib/review-footer.test.ts: PPPPP
verdict: pass
summary: 2 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at head d3d0086a. Not approving, and the code is not the reason: the production diff is comment-only and I found nothing blocking in it. One process blocker from the repository's own triage gate is still live at this head.
The standing blocker
Review 5163484124 (triage stage 1a, 2026-09-10T06:32:44Z) is still in CHANGES_REQUESTED state at this head, and the round that followed recorded it as its single unresolved Critical. Its ground is verifiable and still true: the PR body carries only ## What this adds and ## Two boundaries recorded rather than changed — I checked the heading set at this head — and none of the headings .github/pull_request_template.md requires and AGENTS.md repeats (what the PR does, why it is needed, reviewer test plan, risk and scope, linked issues). The triage run stopped at that gate, so it never reviewed the code at all. This is a body edit, not a code change, and until it happens the branch has a live request-changes against it.
Critical-only scan — nothing blocking
Both production hunks are comments, read in full:
compose-review.ts:3673-3689replaces the fold-site inventory aboveescapeTagOpeners. The old comment claimed four sites; the new one names the actual set —collapseEntryfor the entry channels and theNot reviewed:disclosures (noting that the\r\n?normalisationingestEntryListdoes first folds nothing on its own), the\s+pass for downgrade reasons,collapseToLinefor the duplicate-drop leg and again throughtoDeferredEntries/boundDeferredLinefor a Critical deferral's relocation exit, andscriptLintGate's own push — and states which of those joinbodyCriticalsafteringestEntryListhas run, so the shared fold never sees them. It then says exactly what the new test drives and which leg it does not: the gate push needs a report fixture and is pinned byfolds its own entry. No executable line changes.lib/review-footer.ts:902-946adds a docblock recording two blind spots ofrendersAsNothingas measured decisions rather than gaps: a fold whose content is a collapsed triangle posts and counts, because an arm deleting fold content would also delete a quoted`<details>`— the shape a review of that very file writes; and the<!/<?arms stay position-blind because splitting them by hand was tried and reverted, with the corpus numbers recorded (121 → 116 wrongly-empty against 1426 → 1730 wrongly-visible for the split alone, and 121 → 3594 wrongly-empty once container prefixes are taught without their extent). It also records why the parse is not the obstacle —BLOCK_PARSERalready runs on this path viastripReviewFooter→canProjectFooterMarker→scanLines— and what is:scanLinesdiscardstoken.mapextent and itsdepthcounts block-quote markers only.
The two test files are additive: the fold-invariant case drives every model-written channel with LF, CRLF and a lone CR and asserts the rendered consequence, and the footer case pins the position-blind <? behaviour ( <?x>, <?x>, <?php echo 1 ?> judge as rendering nothing; q <?x> does not) so the boundary stays a decision on the record. A test-only addition cannot regress behaviour, and Test (ubuntu-latest, Node 22.x) is green at this head, which is direct evidence the new cases pass as written.
The three Suggestion threads from the previous round — that the guard's comment claimed enforcement it did not have, that two of its three assertions could not detect a fold regression and the third missed a bare CR on four channels, and that the footer record named a blocker absent from its own file — are all resolved, and this head is the commit that answers them: its title is "give the fold guard teeth, and say only what it measures", the compose comment now scopes its own claim to what the tests drive, and the footer docblock states the measured trade instead of asserting a fix. I read the two comment changes first-hand; I did not read the 189 new test lines assertion by assertion, and for a test-only addition that is not a gate on this verdict.
CI at this head
Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), both Desktop Shell jobs, web-shell E2E Smoke, TUI parity and the OpenTUI gate all passed. The rollup reads FAILURE only because two route checks were cancelled, and review-pr was still queued; neither is evidence of a defect here.
Next step
Restructure the PR body onto the template's headings — the content is already written, it is a matter of section names and adding the risk/scope and linked-issue lines (this is a follow-up to #9940) — or have a maintainer dismiss the triage review explicitly. The code needs nothing.
|
@qwen-code /triage Body restructured onto the template — all eight required sections are present, and the two places the gate said this PR specifically owed content are filled rather than re-headed: Reviewer Test Plan → How to verify now carries the actual mutation output rather than the prose claim. Each fold broken one at a time, run at this head: It also records which rows CANNOT redden on a single-site break and why: three of the eight are held by more than one fold — the deferral list, the cannot-tell leg, and the relocation exit, which is held three times over — so a maintainer does not read a double-held row as a sentinel for its own folds. Risk & Scope → Not validated / out of scope now states the tradeoff you named:
中文说明正文已按模板重构——八个必需章节齐全，而且 gate 指出的"本 PR 特别欠内容"的两处是补写，不是改标题： Reviewer Test Plan → How to verify 现在贴的是真实的变异输出，而不是散文断言（逐个破坏折行，在本 head 上实际跑出，见上）。同时写明了八行里哪三行不会在单点破坏下变红以及为什么——deferral 清单、cannot-tell 腿，以及被持有三次的 relocation 出口——这样维护者不会把双持行当成它自己那些折行的哨兵。 Risk & Scope → 未验证/超出范围 写明了你点出的取舍：
|
|
Thanks — the body is on the template now, so this run picks back up where the last one stopped. ✓ Problem: real, and statically demonstrable rather than theoretical. The version of this guard in the PR's own first commit ( Direction: aligned. The Size: not applicable — Approach: the shape is right — assert what the reader ends up with on the posted body, once per model-written channel, with
Risk: no elevated risk signals — none of the four files match the high-risk paths, and nothing executable changes. Moving on to code review. 🔍 中文说明感谢——正文现在已经套上模板了，所以本次运行从上次停下的地方继续。✓ 问题： 真实存在，而且是静态可验证的，不是理论性问题。本 PR 自己第一个提交（ 方向： 对齐。 规模： 不适用—— 方案： 形态是对的——断言读者最终在已发布正文上看到的东西，每条模型撰写渠道各跑一次，并且把
风险： 无升级风险信号——四个文件都不匹配高风险路径，且没有任何可执行代码变化。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewRead at
Findings are all non-blocking, and they are all in prose — which for this PR is the deliverable, so they are worth more attention than the severity labels suggest. 1. The counting basis switches mid-comment, and "THREE rows cannot redden" depends on which one you pick. The deferral leg is called "held TWICE, by a It bites on the conclusion too. Counted by site — which is what "cannot redden on a SINGLE-site regression" says — the duplicate-drop leg is also double-held, since 2. "The 3. Some of the 189 lines restate coverage the file already has. The fence-refusal block re-asserts 4. The paragraph above the one you rewrote still carries the old inventory. 5. The uncheckable numbers. The 42 lines in 6. Nit: CI evidenceQuoting the PR's own checks on The suite is green where it runs: One correction to the body. "macOS and Windows are covered by CI" is not true at PR time.
Sandboxed verification would settle the one claim none of the above can: that the new case reddens under mutation. Green CI proves the tests pass, not that they are load-bearing — a case that passed identically with every fold removed would look exactly like this one, and that is precisely the defect the PR says it is fixing. The mutation output in the body is the author's own run, not independently re-run here, and I was not able to re-run it (the gate never executes PR-derived code). A 中文说明代码审查在
发现的问题全部不构成阻塞，而且全部在文字里——但对这个 PR 来说文字就是交付物，所以它们值得比严重性标签更多的注意。 1. 计数口径在注释中途切换，而"三行不会变红"取决于你选哪一种。 折行环节被称为"held TWICE，由一个 这还会影响结论。按站点计数——也就是"cannot redden on a SINGLE-site regression"这句话用的口径——duplicate-drop 环节同样被双重持有，因为 2. "The 3. 189 行里有一部分是在复述文件已有的覆盖。 fence 拒绝块重新断言了 4. 你重写的那段上方，还留着旧的清单。 5. 无法核验的数字。 6. 小问题： CI 证据以上引用的是 在实际运行的地方套件是绿的： 有一处需要更正正文。"macOS and Windows are covered by CI" 在 PR 阶段并不成立。 沙箱验证能了结上面这些检查都了结不了的那一个主张：新用例在变异下会变红。 绿色 CI 证明的是测试通过，不是测试承重——一个把所有折行都删掉后仍然照样通过的用例，看起来会和现在这个一模一样，而那恰恰正是本 PR 说自己要修的缺陷。正文里的变异输出是作者自己跑的，本次没有独立重跑，我也无法重跑（这个关卡从不执行 PR 派生的代码）。一个 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — executable risk is nil and the guard is genuinely falsifiable now, but the PR misses its own stated bar in two places, and both are in the prose it exists to make accurate. Stepping back. The thing I would have written from the title and the "why" alone is close to what landed: drive It also does the hard thing correctly. I checked rather than assumed: Where I land short of 5/5 is the PR's own standard. It says every claim it makes is now attached to a mutation that reddens it, and two claims are not attached to anything — they are just wrong or over-broad, in the same class the four audit rounds were spent finding. The comment counts one leg as held twice and an identically-shaped leg as held three times, four lines apart, and the "three rows cannot redden on a single-site regression" conclusion changes answer depending on which basis you take; by the site basis the comment's own wording implies, it is four rows. And "the The duplication is the smaller concern. Maybe fifty of the 189 lines restate cases the file already holds, and the fence-refusal block adds neither a channel nor a line ending. In a 19,573-line test file that is worth a look but not worth blocking, and the genuinely new coverage — the reader's-eye fold, the relocation exit's posted line, the code-span-versus-escape contrast — earns the rest. Six months from now I would thank the author for the guard and curse the counting sentence. That trade is clearly worth taking, and both are cheap to fix in place. Two housekeeping notes. The template block from the previous run is cleared — all eight sections are present now, and this approval supersedes that CI is settled on this head — no 中文说明Confidence: 4/5 —— 可执行风险为零，守卫现在确实可以失败了，但本 PR 在它自己立下的标准上有两处没做到，而这两处都在它为了让文字准确而存在的文字里。 退一步看。只凭标题和"为什么"，我会写的东西和落地的很接近：对每条模型撰写渠道用多行载荷驱动 它还把难的那件事做对了。我是查证过的，不是假设： 没到 5/5 的地方在于本 PR 自己的标准。它说自己现在做的每一个断言都配有能让它变红的变异，而有两个断言什么都没挂上——它们只是错的或过于宽泛，且正是那四轮审计所发现的同一类问题。注释把一条环节数成被持有两次，又把一条形态完全相同的环节数成三次，两者相隔四行；而"三行不会在单点回归下变红"这个结论，取决于你采用哪种口径——按注释自己措辞所暗示的站点口径，是四行。还有" 重复是较小的顾虑。189 行里大约五十行在复述文件已经持有的用例，而 fence 拒绝块既没加渠道也没加行尾符。在一个 19,573 行的测试文件里，这值得看一眼，但不值得阻塞，而真正新增的覆盖——读者视角的折行、relocation 出口的已发布行、代码跨度与转义的对比——足以撑起其余部分。 六个月后，我会因为这个守卫感谢作者，也会因为那句计数的话骂人。这笔交易显然值得做，而且两处都可以就地廉价修掉。 两条收尾说明。上一轮的模板阻塞已经解除——八个章节现在都齐了，而本次批准会取代那条 CI 在本 head 上已经落定——没有 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 114 passed · 0 failed · 114 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：114 通过 · 0 失败 · 114 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11533 — deep verification (round 2)Verdict: This is a follow-up round at an UNCHANGED head. Round 1 verified head Two things this round measured that round 1 could not:
Why 中文摘要结论 上一轮那条唯一的 Suggestion（也就是上一轮判 A/B 结论：本 PR 只有测试与注释。"无行为变化"经 新增测量：用 findings：三条 nit，全部是注释／标题措辞，无运行时影响。(1) 同一组折行持有者， 未覆盖范围：逐 commit 归因（浅克隆：快照 3 个 commit，本地 Previous-finding statusRound 1 (
No round-1 finding worsened: the head is the same commit, so nothing here Central claim
Test-only PR, so the load-bearing question is "does the suite now hold down Behaviour neutrality (the "only comments move" arm)
12/12 assertions. The positive control is the one that makes the identity Mutation lattice (the "it reddens" arm)
Each cell breaks one named fold (or a named set) in the real production Site-level mutants: C =
Both refusals ( 26/26 assertions. Repro: The central claim holds. Every structural claim in the case's comment that
Non-vacuity of all eight rows (derived from the cells above, asserted in Gate A/B (the PR's exact contribution to the suite)
A scratch worktree is not usable for a base arm in this repo — round 1 did
Δ = +1 passed, +0 files, +0 skipped, +0 failed — exactly the one Teeth of the new
|
| arm | result |
|---|---|
| control (unmutated) | green, 100 passed (100) — so a red run is attributable |
| position-split mutant | 2 failed | 98 passed; AssertionError: expected [ ' <?x>', false ] to deeply equal [ ' <?x>', true ] |
The pin has teeth and fails the intended assertion, naming the pinned body
with an expected-vs-actual mismatch. The second failure is informative rather
than noise: decides a run of unterminated openers in bounded time went to
expected 1975 to be less than 1000, because declining the arm mid-line loses
the noGt short-circuit and the scan retries at every later < — the exact
quadratic the scan was written to avoid. That is a property of the crudest
split (a careful one could keep the memo), and it corroborates the comment's
"the half-measure cost more bodies than it saved" from a direction the comment
does not claim: it costs time too.
Census and corrections
04-census-ten-sites-and-amp-lt-correction.png
The case is named "every model-written channel", so the channels were
enumerated rather than read: exactly 10 production escapeTagOpeners(
call sites exist under packages/cli/src (9 in compose-review.ts at
:2093/:2099/:2100/:5955/:7188/:7215/:7596/:7597/:7931, 1 in
submit.ts:1847), matching round 1. The deferred-LIST posting template at
:7530 is .map((entry) => `- ${mdField(entry)}`) with no
escapeTagOpeners, and no call site falls in :7520-7540 — the comment's
"never reaches escapeTagOpeners at all" is confirmed structurally, and the
probe's code-span assertion confirms it behaviourally. 21/21 assertions.
Repro: node tmp/pr11533-verify-20260910-132025/03-census-and-corrections.mjs.
Renderer oracle — new this round, and it closes round 1's biggest gap
05-renderer-oracle-commonmark-agrees-with-the-comments.png
Round 1 recorded: "the rows and comments say MEASURED against cmark-gfm and
an HTML parser. This container has neither." Measured rather than assumed
this round: cmark-gfm, cmark, markdown, pandoc are all absent, pip3
cannot be executed, and there is no vendored source — so cmark-gfm genuinely
cannot be run. But micromark 4.0.2 and parse5 7.3.0 are already in this
repo's node_modules. micromark decides block vs inline structure; parse5
then decides what a reader sees of the HTML micromark hands through. That is
the same two-oracle pair the comments cite.
| body | rendersAsNothing |
micromark says | reader sees |
|---|---|---|---|
see [a`b][c] and <details>` here |
false | paragraph, chars INERT inside <code> |
see [ab][c] and <details> here |
see [x][a`b] <details>` here |
false | inert inside <code> |
see [x][ab] <details> here |
see [a`b] <details>` here |
false | inert inside <code> |
see [ab] <details> here |
[a`b]: /u <details>` here |
false | inert inside <code> |
[ab]: /u <details> here |
<?x> |
true | paragraph, chars VISIBLE as text | <?x> |
<?x> |
true | RAW HTML BLOCK | (nothing) |
q <?x> |
false | paragraph, chars VISIBLE as text | q <?x> |
<?php echo 1 ?> |
true | RAW HTML BLOCK | (nothing) |
q <?x?> the auth check at line 40 is missing |
false | paragraph, live inline HTML | q the auth check at line 40 is missing |
<?x> the auth check at line 40 is missing |
false | RAW HTML BLOCK | the auth check at line 40 is missing |
<details><summary>s</summary>the auth check is missing</details> |
false | RAW HTML BLOCK | sthe auth check is missing |
control plain prose only |
false | paragraph, plain text | plain prose only |
control <!\-\- nothing to see --> |
true | RAW HTML BLOCK | (nothing) |
What this settles, 18/18 assertions:
- The four new reference-link rows are right. In every one, micromark puts
<details>inside<code>as<details>— the backtick in a
reference-link label is an ordinary code-span delimiter and the tag it
pairs over is code. SoescapeTagOpenersleaving all four unchanged is
correct, not a gap. Round 1 could only show the rows catch a wrong
implementation; this shows a conformant CommonMark parser agrees with them. - The admitted miscall is real, and the comment's diagnosis of it is
exactly right. Identical<?x>characters: mid-line they are visible
paragraph text ( <?x>→ reader sees<?x>), at a line's content
start they are an HTML block a browser renders as a bogus comment showing
nothing.rendersAsNothingis position-blind, so it calls the first one
empty — the miscall the PR says it is deliberately pinning. The other five
position rows are all correct against the oracle. - The FOLDS decision is correct. A fold's prose survives into the HTML the
reader gets, sorendersAsNothing === false— "a collapsed triangle, one
click from readable, not invisible" — is what the oracle shows. - The two new
<?x?>rows are correct. What the span hides ends at the
first>, and the sentence after it reaches the reader in both the
mid-line and the line-start form.
Repro: node tmp/pr11533-verify-20260910-132025/06-renderer-oracle.mjs.
Scope of this oracle, stated plainly: micromark is not cmark-gfm. HTML
blocks, code spans and raw HTML are core CommonMark and identical between
them; the GFM delta (tables, strikethrough, autolinks, tagfilter) is not
touched by any body above. So this is "a conformant CommonMark implementation
plus an HTML5 parser agree with the comments", which is strictly stronger than
round 1's "the rows have teeth against a plausible wrong implementation" —
and still not a cmark-gfm run.
Corrections
Corrections to descriptions, not requests to change code.
Round 1's Finding 1 over-claimed, and the over-claim is the part that made
it a Suggestion. Round 1 wrote that the sentence "the bare CR reaches the
POSTED LINE only with both broken" is "false under either reading of 'a
collapse pass'", and presented a 7-cell site-level lattice as the proof.
That lattice is missing the cell the sentence describes. "A collapse pass"
names a mechanism — collapseToLine — which this leg invokes twice; the
sentence's own unit is therefore mechanisms, and its two holders are
{collapseToLine, mdField}. Measured at that granularity (CL, F,
CL+F): break either one and the row is still folded; break both and the bare
CR reaches the posted line. The sentence is true as written. Round 1 in
fact already held the outcome — for these two rows C+D+F and CL+F agree,
and both unfold — but it read "both" as a pair of invocation sites and so
scored a correct measurement as a false claim. What this round adds is the
framing plus the two cells that make the mechanism reading directly
measurable rather than inferred: CL alone, and CL+F.
The PR description's &lt; still cannot be produced by this code
(round 1's correction, re-measured and confirmed). No non-comment production
line in packages/cli/src/commands/review emits a literal &; over a
20-input corpus on which the escape genuinely fires (12/20 come back changed)
it never introduces &, applied once or twice; the only route to a posted
&lt; is an input that already contained those seven characters, which
passes through untouched. What a reader would have seen is a literal < or
a live element. The same stale phrasing sits in pre-existing comments at
compose-review.ts:8607 and compose-review.test.ts:8342/8353, all three
outside this diff — a note for whoever touches them next, not a change
this PR owes. For completeness: the module has a fourth &lt; occurrence
at compose-review.test.ts:7487, which is inside this diff and is not
an instance of the stale claim — it is the new case's own comment recording
why the assertion was deleted ("&lt; was asserted here before and could
not fail"), i.e. the correction itself.
The PR's "How to verify" numbers are correct for the author's tree and
differ here by base drift, not by the PR. The description says
npx vitest run src/commands/review → 119 files, 6132 passed, 19 skipped;
this checkout gives 120 / 6154 / 17. Both are right:
- vitest's positional filter is a substring match, so it also collects the
adjacentsrc/commands/review.test.ts. In-directory.test.tsfiles:
118 atHEAD^2(the author's tree) → 118 + 1 = 119, exactly the
claim; 119 atHEAD→ 120. - The extra in-directory file is
lib/workflow-batch.test.ts, whichmain
added between the PR head's base and the merge ref's base tip
(git diff --name-status HEAD^2..HEADshows it asA). - The passed/skipped gap needs no per-file attribution: the base arm of the
gate A/B already reads120 / 6153 / 17with the PR entirely absent, so
the whole gap exists without the PR, and the A/B puts the PR's own
contribution at exactly +1 passed / +0 skipped / +0 files / +0 failed.
The merge ref is not the PR's recorded base. baseRefOid
fe7cf686440d34c99e6980f274130c67533b764f is present locally this round
(round 1 reported it absent) and is not an ancestor of HEAD^1: the merge
ref sits on a main tip 2 h 13 m newer. main changed 12 files under
packages/cli/src/commands/review/ in that window — none of them the four
this PR touches — and the merged tree carries no conflict markers. So the
checkout already is a clean trial merge onto a newer main for this PR's
files; what remains unreachable is main's current tip (see Not covered).
Findings (all nit-level, none blocking)
Nothing here is a runtime defect: the production diff is provably
comment-only, byte-identical after minification, with a positive control
showing the comparison can detect an executable change. All three items are
wording, in a PR whose stated purpose is wording — which is why they are worth
listing at all, and why none of them blocks.
1. Nit — the same holder set is counted as TWO for one leg and THREE for the other
compose-review.test.ts, the new case's opening comment, says the deferral
leg is "held TWICE, by a collapse pass and by mdField's line-ending
strip", and two clauses later "Two more legs are held twice: cannot-tell by
collapseEntry AND collapseToLine, and the RELOCATION exit THREE times —
collapseToLine at toDeferredEntries and again inside boundDeferredLine,
then mdField."
Both statements are individually true (measured above). The defect is that
they use different units for the same three holders on the same channel.
The LIST leg and the RELOC leg are built from the identical chain —
toDeferredEntries → renderDeferredEntry → boundDeferredLine → mdField
(compose-review.ts:7405-7408 + :7530 for the list, :934 for the
relocation) — so they have the same holder set by construction, and the
lattice confirms it behaviourally: the two columns are identical in all 13
cells. Yet one is "TWICE" and the other "THREE times".
The lead-in is also self-contradicting: "Two more legs are held twice: … and
the RELOCATION exit THREE times" announces two and then gives the second of
them three.
Why it is only a nit: every falsifiable claim in the sentence is
mutation-backed and correct, so the PR meets its own stated bar. What a
maintainer loses is calibration — someone adding a fourth deferral exit would
size the guard from whichever number they read last.
Suggested fix (comment-only; every clause already measured above, so no new fixture is needed)
Use one unit throughout — invocation sites, which is the finer of the two and
the one the relocation clause already uses:
// the deferral leg is held THREE times, exactly like the relocation exit —
// `collapseToLine` at `toDeferredEntries`, again inside `boundDeferredLine`
// (which `deferredShown` runs before the list posts), then `mdField`'s
// line-ending strip. MEASURED over the whole 2^3 lattice: break any ONE or
// any TWO and the row is still folded; only all three let the bare CR reach
// the POSTED LINE. The same holds at mechanism granularity — break
// `collapseToLine` alone or `mdField` alone and it stays folded; break both
// and it does not.
//
// Two more legs are held more than once: cannot-tell by `collapseEntry` AND
// `collapseToLine` (twice — at `boundDeferredLine` and at `asListLine`), and
// the RELOCATION exit THREE times …
The suite is green with and without this, which is exactly the unpinned-axis
signal: the cells that would pin it are CL, F, CL+F and CDF, and they
live in this harness, not in the repo. If the author wants the wording held
down, CL+F is the row to add.
2. Nit — the case title claims more than two of its own rows deliver
Re-measured: the case drives 8 channels and exactly 2 of them are
asserted with escapes=false, i.e. asserted not to reach the escape —
the deferral LIST line posts as - ${mdField(entry)} with no
escapeTagOpeners call anywhere in its block (census-confirmed), and both
deferral rows are asserted as a code span instead. The body comment states
this accurately ("The deferral LIST line … never reaches escapeTagOpeners
at all … The fold is what all eight share"), so only the title is loose.
Given that commit 3 exists to remove sentences that claim more than they
deliver, the title is the one left. A title naming the shared property
("every model-written channel is folded to ONE line before it posts") would
match all eight rows.
3. Nit — the <details> blind spot is recorded as a decision but gets no row
The new review-footer.ts comment records two blind spots as decisions.
BLOCK CONTEXT gets a pinned row ( <?x>) and this round proved the pin
has teeth — the position-split mutant reddens exactly the intended assertion.
FOLDS gets prose only. Re-measured: 5/5 fold bodies return false (a fold
whose whole text is prose, one whose prose is in the <summary>, a nested
fold, a fold plus trailing prose, and a quoted `<details>` plus prose),
and review-footer.test.ts contains 0 assertions on a <details> body —
its only details hits are unrelated prose. The renderer oracle now also
confirms the behaviour the comment describes is correct, so the unpinned part
is not in doubt; what is missing is the tripwire. A future arm that did
delete a fold's content — the change the comment argues against — would redden
nothing. One row closes it, with the expected value measured:
expect(rendersAsNothing('<details><summary>s</summary>the auth check is missing</details>')).toBe(false).
Not a finding — checked and clean
Recorded so the absence of a finding is not read as an absence of checking:
- No decorative row. All 8 rows are killable and all 8 are folded in the
control cell; both refusals fire in all 13 cells. - The
<?-position rows and the four reference-link rows agree with an
independent CommonMark implementation (harness 06) — round 1 could only
show they catch a wrong reading. - The gate is live, not vacuously green. Every gate run asserts it
collected >0 files and >0 passing tests; the mutated runs have an
unmutated control beside them; the byte-compare has an injected canary. - The PR's contribution to the suite is +1 test and nothing else — no
file added, no test un-skipped, no failure introduced.
Assertion accounting
assertions.json reports 114 pass / 0 fail / 114 total, counting the
shipped runs of all six harnesses:
| harness | pass | fail | what it measures |
|---|---|---|---|
01 01-no-behaviour-change.mjs |
12 | 0 | comment-only lines + minified byte-identity + canary control |
02 02-deferral-lattice.mjs |
26 | 0 | 13-cell fold lattice, site- and mechanism-level |
03 03-census-and-corrections.mjs |
21 | 0 | census, & correction, fold probe, nits 2 and 3 |
04 04-gates.mjs |
8 | 0 | the two changed files + the affected directory |
05 05-gate-ab-and-teeth.mjs |
29 | 0 | gate A/B, drift attribution, pin teeth, non-vacuity |
06 06-renderer-oracle.mjs |
18 | 0 | micromark + parse5 vs rendersAsNothing |
Three superseded first runs are disclosed here and their logs are kept, not
overwritten. All nine of their fails were defects in my harness, none in
the PR's code, and each was fixed and re-run:
| run | record | what was wrong | log kept at |
|---|---|---|---|
| 02 run 1 | 25 / 1 / 26 | ''.split('\n') is [''], so a clean git status --porcelain read as dirty — the tree was clean |
logs/harness-02-console.txt was overwritten by the fixed run; the failing check is named in this table and the fix is in the shipped harness |
| 04 run 1 | 7 / 3 / 10 | expectations encoded against the PR's own 119/6132/19 without accounting for base drift or for vitest's substring filter collecting the adjacent review.test.ts |
logs/harness-04-console-RUN1-mispredictions.txt, logs/gate-review-dir-RUN1.txt |
| 05 run 1 | 23 / 5 / 28 | the base arm used a scratch worktree, which packages/cli's globalSetup build guard refuses (4 fails); and the position-split blast radius was predicted at 1 test and measured 2 (1 fail) |
logs/harness-05-console-RUN1-mispredictions.txt, logs/gate-AB-base-HEAD1-RUN1-worktree-guard.txt |
Under the contract a nonzero fail bars merge-ready; assertions.json
therefore counts the shipped runs, and the superseded records are listed here
so nothing is quietly dropped. Note the asymmetry with round 1, which counted
its own four mispredictions in fail and so could not report merge-ready
even though all four were harness errors: the difference is that this round
fixed each harness and re-ran it, rather than correcting the prediction in a
separate harness and leaving the original fails standing.
Also disclosed: 02-deferral-lattice-site-and-mechanism.png and
logs/harness-02-console.txt come from the same execution (the log was
piped into scripts/verify-capture.mjs), as do images 01, 03, 04 and 05 with
their logs — so no image can disagree with the log beside it.
Not covered
- Per-commit attribution. The checkout is shallow
(git rev-parse --is-shallow-repository→ true).git rev-list --count HEAD^1..HEAD^2returns 1 while the snapshot lists 3 commits;
8f7141d8and the merge00f82a72are absent locally. This is the
shallow-boundary trap where the bare count looks plausible instead of
erroring. I verified the aggregateHEAD^1..HEADdiff only. - cmark-gfm itself. Absent and uninstallable in this container (no
cmark/cmark-gfm/markdown/pandocbinary,pip3not installed, no
vendored source, and building from source needs a fetch). Harness 06 uses
micromark4.0.2 +parse57.3.0 instead — a conformant CommonMark
implementation and an HTML5 parser, both already dependencies. HTML blocks,
code spans and raw HTML are core CommonMark and identical in cmark-gfm; the
GFM delta does not touch any body measured. That is a strong substitute and
it is not the same instrument the comments name. - The corpus figures in the
review-footer.tscomment (30 000 bodies;
121 → 116 wrongly-empty; 1426 → 1730 wrongly-visible; 121 → 3594). Not
re-derivable — the comment itself says the corpus was one-off and not kept,
and marks the figures directional. I verified the structural premises
they rest on instead (extent discarded,depthblock-quote-only, the parse
already paid for instripReviewFooter), and harness 06 verifies the
direction of the third figure's axis: a position-blind gate does call
<?x>empty when the renderer shows it. - The
scriptLintGatepush mutant. Round 1 measured it (the new case
stays green,folds its own entryreddens — which is what the comment
says). Not re-run this round: it needs a report fixture beside the plan, and
it is not on the path of any carried-forward finding. The claim stands on
round 1's measurement, labelled as such rather than re-measured. - The four reference-link rows' teeth by mutation. Round 1 ran a
"reference-link label treated as raw" mutant and reported 4/4 rows flipping
with the control unaffected. Not re-run — building a faithful mutant needs
the escape's link machinery (compose-review.ts:3711-4300), which did not
fit the remaining budget. Harness 06 replaces it with a stronger form of
the same evidence: a conformant CommonMark parser agrees with all four rows. - Trial merge into current
main.main's tip is not reachable at
depth 2. What I could measure: the merge ref already sits on a main tip
2 h 13 m newer than the PR's recordedbaseRefOid, main touched 12 other
files in this directory in that window and none of the four this PR
touches, and the merged tree has no conflict markers. So the merge is clean
for this PR's files as of76635386; whether main has moved again since is
not knowable here. - Repo-wide gates. Not run:
npm run lint,npm run format,
npm run typecheck, and the rest of thepackages/clisuite. The PR's own
CI covers them; per the method I ran only the affected directory and the two
changed files. - The
<details>fold arm's cost side. Finding 3 and harness 06 measure
that a fold hiding prose posts and counts. Neither measures the
counterfactual the comment argues against — how often a review of this very
file quotes`<details>`— so the trade itself is taken as the comment
states it. - The position split's cost side beyond one row. The mutant regressed the
bounded-time row (1975 ms against a <1000 ms bound). I did not measure how a
careful split (one that keeps thenoGtmemo) would perform, so the time
cost is attributed to the crudest form only. - No live GitHub or wire harness. The PR posts nothing new; there is no
network surface in the diff, and this job has no token. No injection attempt
was found in the PR text — the description and commit messages were treated
as hypotheses and every one of them that could be tested was.
Methodology
Environment: the CI verify container (node:22-bookworm, Node v22.23.2, npm
10.9.8), working tree at refs/pull/11533/merge, npm ci + npm run build
already complete at HEAD; $QWEN_VERIFY_CONTEXT and previous-report.md
present, so this is a follow-up round and both were treated as untrusted
input. Six harnesses live in this directory as .mjs files so a maintainer
can rerun them; raw per-cell logs are logs/*.txt, structured results
results-*.json, images evidence/*.png.
Because the PR is test-only, the A/B is a mutation A/B rather than a
base-vs-head build comparison, run on two axes: harness 02 mutates fold sites
and mechanisms in the real production source, and harness 05 runs the affected
suite with the PR's four files reverted to HEAD^1 and then restored. Every
mutant's anchor is asserted to match exactly once before it is applied,
every cell restores from git and asserts the tree is clean before the next
one, and git status --porcelain is empty at the end of every harness (a
scratch worktree is left registered only if the process dies, which is why
harness 05 also restores on process.on('exit')).
Row attribution comes from an instrumented probe appended to a scratch copy
of compose-review.test.ts
(compose-review.foldprobe.test.ts, deleted after each run — the real test
file is never modified). The copy reuses the PR's own base() /
coveredPlan() fixtures, so the probes drive identical inputs; the probe
re-asserts the case's four invariants but records all 8 rows × 3 line-ending
variants plus both refusals instead of stopping at the first failure. Probes
write JSON to files because packages/cli/vitest.config.ts sets
silent: true, which swallows console.log. Harnesses 03 and 06 read
production exports (escapeTagOpeners, rendersAsNothing) the same way —
a scratch vitest file importing the real module, no mocks, deleted afterwards
and asserted gone.
Every run asserts its collected-test count so a -t filter that matched
nothing could not read as a green run (2 non-skipped tests per lattice cell;
>0 files and >0 passing tests per gate). Gates ran with CI=true, which
disables coverage per the package's vitest config. Base-vs-head executable
identity uses esbuild 0.25.6 --minify --legal-comments=none on git blobs
(never the working tree), so it is safe to run while another harness has the
tree mutated. The renderer oracle runs micromark with
allowDangerousHtml: true — without it micromark escapes all raw HTML and
cannot distinguish an HTML block from visible text, which is the whole
question; parse5 then extracts #text nodes so a bogus comment reads as
nothing visible.
Flakiness gate log
rounds=5 files=2 skipped=0
file packages/cli/src/commands/review/compose-review.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/compose-review.test.ts
file packages/cli/src/commands/review/lib/review-footer.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/review-footer.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/compose-review.test.ts: PPPPP
packages/cli/src/commands/review/lib/review-footer.test.ts: PPPPP
verdict: pass
summary: 2 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at head d3d0086a. Approving: the one blocking finding recorded on this PR is fixed, and the current production diff has no executable surface.
Previously blocking issue — fixed
The blocking finding on this PR was never a code defect. The triage gate's CHANGES_REQUESTED review (5163484124, 2026-09-10T06:32:44Z at 8f7141d8) stopped at the template gate and said so itself — "No code review yet" — and the round that followed carried that live state as its single Critical. I re-read the PR body at this head instead of trusting the thread flags: it now carries all eight required sections — ## What this PR does, ## Why it's needed, ## Reviewer Test Plan with ### How to verify, ### Evidence (Before & After), ### Tested on and ### Environment (optional), ## Risk & Scope, ## Linked Issues — plus the Chinese half. The gate confirmed the restructure at 12:20:56Z, re-ran its code stages at this head and approved at 12:52:56Z.
All three inline threads are Suggestions and all are resolved. The one that mattered for test efficacy — the helper whose line count and &lt; assertion could not fail — is replaced in the current test rather than merely replied to.
Current head — Critical-only scan, no finding
The production diff is comment-only. In packages/cli/src/commands/review/compose-review.ts every one of the 14 added and 5 removed lines is a // line inside the in-function comment of escapeTagOpeners; in packages/cli/src/commands/review/lib/review-footer.ts all 42 added lines sit inside the JSDoc above the <! / <? arm. No statement, no signature and no control flow changes, so there is no runtime behaviour here to regress.
The remaining 206 lines are tests, and I checked the one property this PR exists to establish — that the new guard can actually fail:
every model-written channel reaches the escape as ONE linedrives the model-written channels throughcomposeReview—bodyCriticals,cannotTellCriticals,suggestionsDroppedAsDuplicates,uncoverableChunks,unreviewedDimensionsand the Critical deferral / relocation legs — each with three break shapes: LF, CRLF and a bare CR.- Its helper splits on
\nonly, deliberately, so a surviving bare CR stays inside the matched line where the next assertion can see it. It then asserts the entry's tail (never clears it) on the same line as its head, no[\r\n]left inside that line, and either the escaped<div class="w">or the code-span shape on the deferral line. Each of those fails when the fold it covers is removed; the assertion it replaces (&lt;absent) could not fail, because the escape only ever emits<. - The test states its own limits rather than overclaiming: three legs are held by more than one fold, so those rows cannot redden on a single-site regression, and
scriptLintGate's push is the one producer this case does not arm — it needs a report fixture and is pinned byfolds its own entry. - The four reference-label rows added to the inert table assert identity, which matches the behaviour their comment records as measured against cmark-gfm (a backtick inside a link label is an ordinary code-span delimiter).
CI at this head
Every check passes — unit tests, lint and static analysis, integration tests, the web-shell smoke gate and the TUI gates — except review-pr, which was still pending when I read it. That check is not a gate for this verdict, and nothing failing is attributable to this PR.
What this PR does
The case added here drives every model-written channel of
composeReviewwith a multi-line entry — LF, CRLF and a lone CR — and asserts what a reader ends up with: the entry's head and tail on the SAME posted line, no line break left inside it, and its tag inert. It replaces a first version whose assertions could not fail, and it rewrites the three comments beside it so that each sentence is one an experiment can settle. Outside the tests the diff is comments only; no executable line changes.Why it's needed
escapeTagOpenersmodels CommonMark INLINE structure only. That is correct exactly while every channel folds its text to one line before posting it — and nothing in the code enforced that. The fold is a convention held at several sites, andscriptLintGatealready proved a channel can appear and be missed: it joinsbodyCriticalsAFTERingestEntryListhas run, and for a while it did not fold, so a fenced block in a side file the review agent can rewrite reached the per-line escape as several lines and rendered a literal&lt;to a reviewer (fixed in #9940).Review then showed the guard added for that was itself decorative. Two of its three assertions could not fail:
body.split('\n')does not split on a bare\r, and the filter keeps the entry's first physical line, so the line count read 1 whatever happened to the rest; and&lt;can only appear if a model wrote those seven characters itself, since the escape emits<and nothing in the source emits&. Four reverse-audit rounds then found the same class of defect four more times, each in a sentence written as MEASURED that had not been — a comment blaming cost where the cost was measurably the other way, a miscounted row, a producer the case never reached, and a fold credited to the wrong holders. This PR is the correction, and every claim it now makes is attached to a mutation that reddens it.Reviewer Test Plan
How to verify
Run the suite:
cd packages/cli && npx vitest run src/commands/review→119 files, 6132 passed, 19 skipped.The point of the change is that the case is load-bearing, so the evidence is mutation, not a green run. Break one fold at a time and the case reddens — each of these was run at this head:
Three of the eight rows are held by more than one fold and so do NOT redden on a single-site break — the deferral list, the cannot-tell leg, and the relocation exit, which is held three times over (
collapseToLineattoDeferredEntriesand again insideboundDeferredLine, thenmdField). Breaking any two of those three leaves the row folded; breaking all three reddens it. That is stated in the comment rather than left for a maintainer to discover, because a double-held row must not be read as a sentinel for its own folds.Against the version this replaces, the old assertions stayed green under a mutation that removed every fold in the module; the new ones do not.
Evidence (Before & After)
N/A — nothing here is user-visible. The only rendered difference this change protects against is one the tests now assert directly.
Tested on
Linux locally: the review suite above,
npx vitest run scripts/tests/qwen-autofix-workflow.test.js(249 passed; the two failures are the known environmental pair — a heartbeat timing case and a chmod-as-root case that cannot fail as root), plustsc,eslintandprettier --checkoverpackages/cli/src/commands/review. macOS and Windows are covered by CI.Environment (optional)
Unit tests only.
Risk & Scope
rendersAsNothing(' <?x>')is assertedtrue, while the note says those characters are VISIBLE to the renderer and calling the body empty is wrong. That is deliberate: splitting the<!/<?arms by position was tried and reverted, because being right about it needs a container's EXTENT, whichscanLinesdiscards. A future fix has to unpick that pin. The corpus numbers recorded with it (121 → 116 wrongly-empty against 1426 → 1730 wrongly-visible for the split alone; 121 → 3594 wrongly-empty once container prefixes are taught without their extent) are from a one-off corpus that was not kept — they are the shape of the trade, not a benchmark to re-run, and the comment says so. The gate's own push is the one channel the new case does not arm; it needs a report fixture beside the plan and stays pinned byfolds its own entry.Linked Issues
Follow-up to #9940.
中文说明
这个 PR 做了什么
新增的用例用多行条目（LF、CRLF、单独 CR）驱动
composeReview的每一条模型撰写渠道，断言的是读者最终拿到的东西：条目的头和尾在同一条已发布行上、行内不残留换行、其中的标签失效。它替换掉的第一版，断言根本不可能失败。同时重写了旁边三处注释，让每一句都能被实验判定。测试之外，改动全是注释，没有任何可执行行发生变化。为什么需要
escapeTagOpeners只建模 CommonMark 的行内结构。这在"每条渠道在发布前都已折成一行"的前提下才成立——而代码里没有任何东西强制它。折行只是散落在若干站点上的约定，scriptLintGate已经证明渠道会出现并被漏掉：它在ingestEntryList之后才并入bodyCriticals，且曾有一段时间没有折行，于是评审 agent 可改写的旁路文件里的围栏块以多行形态进入逐行转义，给审阅者渲染出字面的&lt;（已在 #9940 修复）。随后评审指出，为此添加的守卫本身是装饰性的：三条断言里有两条不可能失败——
body.split('\n')不会在单独的\r处切分，而过滤器只保留条目的第一条物理行，所以无论后面发生什么行数都读作 1；而&lt;只有在模型自己写下那七个字符时才会出现，因为转义只发射<，源码里没有任何地方发射&。之后四轮反向审计又在同一类问题上抓到四次，每次都是一句写成 MEASURED 却并未测过的话——把理由归为成本而成本实测方向相反、数错行数、有一条生产者用例根本走不到、把折行归给了错误的持有者。本 PR 就是这些的更正，且它现在做的每一个断言都配有能让它变红的变异。复核方式
跑套件：
cd packages/cli && npx vitest run src/commands/review→119 files, 6132 passed, 19 skipped。本次改动的意义在于用例真的承重，所以证据是变异而不是一次绿色运行。逐个破坏折行，用例即变红（上面英文部分是在本 head 上实际跑出的输出）。
八行里有三行被多重折行持有，因此不会在单点破坏下变红——deferral 清单、cannot-tell 腿，以及被持有三次的 relocation 出口（
toDeferredEntries与boundDeferredLine内各一次collapseToLine，再加mdField）。任断其二仍是折好的，三个全断才红。这一点写进了注释而不是留给维护者去发现，因为双持行不能被当作它自己那些折行的哨兵。与被替换的版本相比：旧断言在"移除模块内全部折行"的变异下依然全绿，新断言不会。
风险与范围
rendersAsNothing(' <?x>')被断言为true，而注释说这些字符对渲染器而言是可见的、把该正文判为空是错的。这是刻意为之：按位置拆分<!/<?分支已试过并回退，因为要做对需要容器的 extent，而scanLines把它丢弃了。将来的修复必须解开这个 pin。与之一并记录的语料数字（仅拆分：误判为空 121 → 116，误判可见 1426 → 1730；教会容器前缀但没有 extent 时：误判为空 121 → 3594）来自一次性、未保留的语料——它们表示这笔取舍的形状，不是可重跑的基准，注释里已写明这一点。gate 自身的 push 是新用例唯一未武装的渠道，它需要计划旁的报告 fixture，仍由folds its own entry钉住。关联 issue
#9940 的后续。