diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 93844b7b549..373d48f60d4 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -7342,6 +7342,18 @@ describe('composeReview — fixedFindings', () => { // An inline link's DESTINATION is raw characters, so a backtick in it // is not a delimiter either. ['see [x](a`b)
` here', 'see [x](a`b) <details>` here'], + // A reference link's LABEL is not: MEASURED against cmark-gfm, a + // backtick inside `[a`b][c]`, `[x][a`b]`, `[a`b]` or a link + // reference definition's own label is an ordinary code-span + // delimiter, and the tag it pairs over is code. Ordinary text is + // what this pass already reads them as, so nothing special is + // needed — the rows are here because two audits reasoned the other + // way and neither could build a counterexample (#9940 review, + // rounds 11 and 12). + ['see [a`b][c] and
` here', 'see [a`b][c] and
` here'], + ['see [x][a`b]
` here', 'see [x][a`b]
` here'], + ['see [a`b]
` here', 'see [a`b]
` here'], + ['[a`b]: /u
` here', '[a`b]: /u
` here'], // …and the `_` rule reaches the LAST label too, not only the one // before it: `http://a.b_c` does not link, so the backticks pair and // the tag between them is live. @@ -7413,6 +7425,183 @@ describe('composeReview — fixedFindings', () => { ); }); + it('every model-written channel reaches the escape as ONE line — the invariant the per-line escape rests on (#9940 review, round 12 reverse audit)', () => { + // `escapeTagOpeners` models CommonMark INLINE structure only. That is + // correct exactly while every channel folds its text to ONE LINE before + // posting it — and the fold is not one primitive. `collapseEntry` and + // `collapseToLine` carry the entry, disclosure and duplicate-drop + // legs; the downgrade reasons have their own `\s+` pass; the deferral + // leg is held TWICE, by a `collapse` pass and by `mdField`'s + // line-ending strip (lib/md-field.ts), and either alone holds it — + // MEASURED: break only one and the row is still folded; the bare CR + // reaches the POSTED LINE only with both broken. 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`. Breaking any two of the three still leaves it folded. + // + // So THREE rows cannot redden on a SINGLE-site regression — a property + // of the code, not a gap here — and the other five do redden on theirs + // (all eight measured). Do not read a double-held row as a sentinel + // for its own folds; it is here for the channel, not for them. + // + // The two rows asserted inert as a CODE SPAN get there differently. + // The deferral LIST line is `- ${mdField(entry)}` and never reaches + // `escapeTagOpeners` at all; the relocation line does reach it, and + // comes back unchanged because the escape reads `mdField`'s backtick + // span correctly. The fold is what all eight share, and it is what + // this case is about. + // + // Three producers push into `bodyCriticals` AFTER `ingestEntryList` + // has run, and that is how a channel gets missed: `scriptLintGate`'s + // push, which for a while did not fold; a Critical deferral's + // RELOCATION exit, which a `Suggestion` row never reaches (armed + // below); and the non-convergence Critical, which is this module's own + // fixed prose and carries no line break to fold. The gate's push is + // the one this case does not arm — it needs a report fixture beside + // the plan — and it is pinned by `folds its own entry`. A producer + // outside these still owes a case. + const multiline = [ + 'the retry helper\n
and `x\nnever clears it', + 'the retry helper\r\n
and `x\r\nnever clears it', + 'the retry helper\r
and `x\rnever clears it', + ]; + const carries = (body: string, where: string, escapes = true) => { + // Split on `\n` ONLY, so a surviving bare `\r` stays INSIDE the + // matched line where the next assertion can see it. Splitting on the + // folds' own set here would consume the very break under test, and + // counting lines proves nothing on its own: the filter keeps the + // entry's first physical line whatever happened to the rest. + const lines = body + .split('\n') + .filter( + (l) => + l.includes('retry helper') && !l.includes('qwen-review-ledger'), + ); + expect([where, lines.length]).toEqual([where, 1]); + // The head and the TAIL of the entry on the SAME line, and no break + // left inside it. That is the fold, stated as the reader sees it: an + // LF that survives puts the tail on another line, a bare CR that + // survives stays in this one — and the per-line escape then decides + // each half on its own and can post a tag the renderer holds live + // (compose-review.ts records that happening). `&lt;` was asserted + // here before and could not fail: the escape only ever writes + // `<`, so the only body that trips it is one a model wrote with + // those characters already in it. + expect([where, lines[0]!.includes('never clears it')]).toEqual([ + where, + true, + ]); + expect([where, /[\r\n]/.test(lines[0]!)]).toEqual([where, false]); + // Inert by ESCAPE on the prose channels; the deferral line is a code + // span, where the renderer already holds the tag and an escape would + // show the reader a literal `<`. + expect([ + where, + escapes + ? lines[0]!.includes('<div class="w">') + : /^[^`]*`[^`]*
[^`]*`[^`]*$/.test(lines[0]!), + ]).toEqual([where, true]); + }; + for (const text of multiline) { + carries( + composeReview(base({ bodyCriticals: [text, 'blocker two'] })).body, + 'bodyCriticals', + ); + carries( + composeReview(base({ cannotTellCriticals: [text] })).body, + 'cannotTellCriticals', + ); + carries( + composeReview( + base({ + suggestionsInline: 1, + suggestionsDroppedAsDuplicates: [text], + }), + ).body, + 'suggestionsDroppedAsDuplicates', + ); + carries( + composeReview(base({ uncoverableChunks: [text] })).body, + 'uncoverableChunks', + ); + carries( + composeReview(base({ unreviewedDimensions: [text] })).body, + 'unreviewedDimensions', + ); + carries( + composeReview( + base({ + criticalsInline: 1, + presubmit: { + downgradeRequestChanges: true, + downgradeReasons: [text], + }, + }), + ).body, + 'downgradeReasons', + ); + carries( + composeReview( + base({ + suggestionsInline: 1, + deferredSuggestions: [ + { + file: 'a.ts', + line: 1, + source: 'review', + severity: 'Suggestion', + title: text, + }, + ], + }), + ).body, + 'deferredSuggestions', + false, + ); + // The deferral channel has a SECOND exit: a Critical relocates into + // `bodyCriticals` — another producer that joins after + // `ingestEntryList` has run, and the one this case would otherwise + // miss, since a `Suggestion` never reaches it. + carries( + composeReview( + base({ + suggestionsInline: 1, + deferredSuggestions: [ + { + file: 'a.ts', + line: 1, + source: 'review', + severity: 'Critical', + title: text, + }, + ], + }), + ).body, + 'deferredSuggestions (relocated)', + false, + ); + // A ruling note's `by` does not fold — it REFUSES, because the note + // is one posted line and a folded `by` would misquote the author. + expect(() => + composeReview(base({ fixedFindings: [{ id: 'R1-2', by: text }] })), + ).toThrow(/must be ONE non-empty line/); + } + // A quoted FENCE is refused rather than folded on the two Critical + // channels: folding it would join the delimiters onto one line and + // silently change what the author quoted. + const fenced = + 'the retry helper\n```\n
\n```\nnever clears it'; + for (const input of [ + { bodyCriticals: [fenced, 'blocker two'] }, + { cannotTellCriticals: [fenced] }, + ]) { + expect(() => composeReview(base(input))).toThrow( + /quotes a code fence its one-line render cannot carry/, + ); + } + }); + it('a raw ` …
… ]]>` does not fold the rest of the body away (#9940 review, round 10 reverse audit)', () => { // The unit rows pin the escape; this pins what the escape is FOR. The // HTML parser closes the bogus comment at the first `>`, so under the diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 213d497265b..0a8b42748f0 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -3673,11 +3673,20 @@ export function escapeTagOpeners(text: string): string { // each closing one hole and opening the next; a line is the unit the // rule below can decide exactly (#9940 review, round 31 reverse audit). // - // The channels are folded at four sites, not one: `ingestEntryList` for - // the entry channels, the `\s+` normalisation for downgrade reasons, - // `collapseEntry` for the `Not reviewed:` disclosures, and - // `scriptLintGate`'s own push — that last one joins `bodyCriticals` - // after `ingestEntryList` has run, so the shared fold never sees it. + // The channels are folded at several sites, not one: `collapseEntry` for + // the entry channels and the `Not reviewed:` disclosures (the `\r\n?` + // normalisation `ingestEntryList` does first folds nothing on its own), + // the `\s+` pass for downgrade reasons, `collapseToLine` for the + // duplicate-drop leg and — at `toDeferredEntries` and again through + // `boundDeferredLine` — for a Critical deferral's relocation exit, and + // `scriptLintGate`'s own push. The + // relocation exit and the gate push join `bodyCriticals` AFTER + // `ingestEntryList` has run, so the shared fold never sees them. (The + // deferral LIST line is folded by `mdField` and posts without coming + // here at all.) `every model-written channel reaches the escape as ONE + // line` in the tests drives every one of these EXCEPT the gate push, + // which needs a report fixture and is pinned by `folds its own entry`; + // it also says which legs are held more than once. // Per-line is NOT a conservative fallback — it is the model that the // fold makes correct. Handed a multi-line string anyway it differs from // the renderer in BOTH directions: it pairs backticks the renderer keeps diff --git a/packages/cli/src/commands/review/lib/review-footer.test.ts b/packages/cli/src/commands/review/lib/review-footer.test.ts index f84981a57c6..cfa45d93059 100644 --- a/packages/cli/src/commands/review/lib/review-footer.test.ts +++ b/packages/cli/src/commands/review/lib/review-footer.test.ts @@ -1078,9 +1078,26 @@ describe('the review footer and the regex that strips it', () => { 'q