diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 4461c3522ee..aefe9bdb530 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -100,6 +100,10 @@ function plan( /** Override the fixture's 5000 — the low-signal floor reads this. */ srcDiffLines?: number; repositoryContext?: unknown; + /** The PR identity fetch-pr records — anchors and bilingual recovery. */ + ownerRepo?: string; + prNumber?: string | number; + host?: string; } = {}, ): string { const p = join(dir, 'plan.json'); @@ -116,6 +120,9 @@ function plan( ...(opts.repositoryContext === undefined ? {} : { repositoryContext: opts.repositoryContext }), + ...(opts.ownerRepo === undefined ? {} : { ownerRepo: opts.ownerRepo }), + ...(opts.prNumber === undefined ? {} : { prNumber: opts.prNumber }), + ...(opts.host === undefined ? {} : { host: opts.host }), srcDiffLines: opts.srcDiffLines ?? 5000, diffLines: 5000, files: [{ path: 'a.ts', kind: 'source', removedLines: 0, heavy: false }], @@ -352,6 +359,9 @@ function coveredPlan( effort?: 'low' | 'medium' | 'high'; srcDiffLines?: number; repositoryContext?: unknown; + ownerRepo?: string; + prNumber?: string | number; + host?: string; } = {}, ): string { transcript('a1', goodPrompt(1), { toolCalls: 3 }); @@ -2913,7 +2923,7 @@ describe('bilingual body — the PR author writes Chinese (prDescriptionHasHan)' expect(r.body).toContain('没有记录表明它的 brief 到达过任何 agent'); }); - it('quotes untranslatable caller text as-is in both halves', () => { + it('keeps the untranslatable unresolved list in the English half; the Chinese half points at it', () => { const r = composeReview({ suggestionsInline: 1, cannotTellCriticals: ['old blocker at a.ts:1 — still reachable?'], @@ -2922,11 +2932,16 @@ describe('bilingual body — the PR author writes Chinese (prDescriptionHasHan)' modelId: MODEL, }); expect(r.body).toContain('Unresolved, please confirm:'); - expect(r.body).toContain('未决,请确认:'); - // The caller's text, once per half. + // The caller's text once, above the fold — the fold carries a count and + // a pointer, not a duplicate of the English list (#8388's fold doubled + // the body copying 31 untranslated entries verbatim). expect( r.body.match(/old blocker at a\.ts:1 — still reachable\?/g) ?? [], - ).toHaveLength(2); + ).toHaveLength(1); + expect(r.body).toContain('未决,请确认:共 1 条'); + expect(r.body.indexOf('old blocker at a.ts:1')).toBeLessThan( + r.body.indexOf('
'), + ); }); }); @@ -4038,3 +4053,457 @@ describe('composeReview — the findings file tag check', () => { ).toThrow(/findingsPath must be a non-empty string/); }); }); + +/** + * #8388's posted body ran 31 unresolved existing Criticals and seven + * disclosures together in one space-joined paragraph, each entry restating + * the same reason, every comment id a bare number, and the Chinese fold + * duplicating the whole untranslated wall. These pin the readable shape: + * paragraphs, a Markdown list, one reason per group, anchored ids. + */ +describe('composeReview — unresolved-Critical rendering (#8388 readability)', () => { + // The github.com anchor assertions ride the effective-host chain's + // default; an exported GH_HOST must not leak in — save/delete/restore + // it, as every sibling suite whose assertions read the host does. + let savedGhHost: string | undefined; + beforeEach(() => { + savedGhHost = process.env['GH_HOST']; + delete process.env['GH_HOST']; + }); + afterEach(() => { + if (savedGhHost !== undefined) { + process.env['GH_HOST'] = savedGhHost; + } else delete process.env['GH_HOST']; + }); + + it('renders the cannot-tell entries as a Markdown list in its own paragraph', () => { + const r = composeReview( + base({ + suggestionsInline: 1, + cannotTellCriticals: [ + 'a.ts:1 — full text unfetchable', + 'b.ts:2 — quarantined by the harness', + ], + }), + ); + expect(r.event).toBe('COMMENT'); + // Opener sentences stay one paragraph; the block opens its own. + expect(r.body).toContain( + 'Reviewed. Suggestions are inline.\n\nUnresolved, please confirm:\n\n', + ); + expect(r.body).toContain( + '\n- **[Critical]** a.ts:1 — full text unfetchable', + ); + expect(r.body).toContain( + '\n- **[Critical]** b.ts:2 — quarantined by the harness', + ); + }); + + it('collapses entries sharing the exact reason into one group that says it once', () => { + const r = composeReview( + base({ + cannotTellCriticals: [ + 'comment one (a.ts) — body truncated; status undetermined', + 'unique.ts:9 — full text unfetchable', + 'comment two (b.ts) — body truncated; status undetermined', + ], + }), + ); + expect(r.body).toContain( + '- **[Critical]** 2 entries — body truncated; status undetermined:\n' + + ' - comment one (a.ts)\n' + + ' - comment two (b.ts)', + ); + // The shared reason renders once, not per entry … + expect(r.body.match(/body truncated; status undetermined/g)).toHaveLength( + 1, + ); + // … and the odd one out keeps its own full line, nothing dropped. + expect(r.body).toContain( + '- **[Critical]** unique.ts:9 — full text unfetchable', + ); + }); + + it('links bare comment ids to their GitHub anchors when the plan names the PR', () => { + const r = composeReview({ + cannotTellCriticals: [ + 'comment 3733696855 (capture-tui.test.ts, R10-1) — body truncated', + 'issue-level comment 5199834809 (author review) — body truncated', + ], + planPath: coveredPlan(undefined, { + ownerRepo: 'QwenLM/qwen-code', + prNumber: '8388', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '[comment 3733696855](https://github.com/QwenLM/qwen-code/pull/8388#discussion_r3733696855)', + ); + expect(r.body).toContain( + '[issue-level comment 5199834809](https://github.com/QwenLM/qwen-code/pull/8388#issuecomment-5199834809)', + ); + }); + + it('leaves comment ids bare when the plan names no PR', () => { + const r = composeReview( + base({ + cannotTellCriticals: ['comment 3733696855 (a.ts) — body truncated'], + }), + ); + expect(r.body).toContain( + '- **[Critical]** comment 3733696855 (a.ts) — body truncated', + ); + expect(r.body).not.toContain('discussion_r'); + }); + + it('a budget gap that says "(none …)" is completion, not a gap — dropped', () => { + // #8388's body: `Not explored to full depth …: chunk 2: (none — all + // planned checks completed)` — the agent reported finishing, and the + // disclosure contradicted it. + transcript('a1', goodPrompt(1), { + toolCalls: 3, + range: [0, 100], + text: + 'No issues found — walked chunk 1 fully.\n' + + 'Budget gap: (none — all planned checks completed)', + }); + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + recordStep45(p, ['verify', 'reverse-audit']); + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).not.toContain('Not explored to full depth'); + expect(r.event).toBe('APPROVE'); + expect(r.body).toContain('No issues found. LGTM! ✅'); + }); + + it('leaves an already-linked entry untouched — never nests a second link', () => { + const r = composeReview({ + cannotTellCriticals: [ + '[comment 3733696855](https://github.com/QwenLM/qwen-code/pull/8388#discussion_r3733696855) — body truncated', + ], + planPath: coveredPlan(undefined, { + ownerRepo: 'QwenLM/qwen-code', + prNumber: '8388', + }), + env: ENV, + modelId: MODEL, + }); + // Byte-identical passthrough: the model linked it itself. + expect(r.body).toContain( + '[comment 3733696855](https://github.com/QwenLM/qwen-code/pull/8388#discussion_r3733696855) — body truncated', + ); + expect(r.body).not.toContain('[[comment'); + }); + + it('renders reasonless entries as their own bullets — no collapse, no dangling dash', () => { + const r = composeReview( + base({ + cannotTellCriticals: ['old blocker', 'second blocker'], + }), + ); + expect(r.body).toContain('\n- **[Critical]** old blocker\n'); + expect(r.body).toContain('\n- **[Critical]** second blocker\n'); + expect(r.body).not.toContain('entries —'); + }); + + it('reads a dangling " — " as reasonless, not an empty group key', () => { + const r = composeReview( + base({ + cannotTellCriticals: ['a.ts:1 — ', 'b.ts:2 — '], + }), + ); + expect(r.body).toContain('\n- **[Critical]** a.ts:1\n'); + expect(r.body).toContain('\n- **[Critical]** b.ts:2\n'); + expect(r.body).not.toContain('entries —'); + }); + + it('collapses embedded newlines so a multi-line entry stays one list item', () => { + const r = composeReview( + base({ + cannotTellCriticals: [ + 'comment 3733696855 (a.ts) — body truncated\nsee also b.ts', + ], + }), + ); + expect(r.body).toContain( + '- **[Critical]** comment 3733696855 (a.ts) — body truncated see also b.ts', + ); + }); + + it('counts entries, not groups, in the Chinese fold', () => { + // Three entries collapsing into two groups — the fold must carry 3. + const r = composeReview({ + cannotTellCriticals: [ + 'one (a.ts) — body truncated', + 'two (b.ts) — body truncated', + 'three (c.ts) — quarantined by the harness', + ], + planPath: coveredPlan(undefined, { han: true }), + env: ENV, + modelId: MODEL, + }); + // … the count AND the pointer — the fold's whole payload besides the + // list it points at. + expect(r.body).toContain( + '未决,请确认:共 3 条(原文未翻译,列表见上方英文部分)。', + ); + }); + + it("anchors comment ids at the plan's GHE host, short ids included", () => { + const r = composeReview({ + cannotTellCriticals: ['comment 12345 (a.ts) — body truncated'], + planPath: coveredPlan(undefined, { + ownerRepo: 'corp/widgets', + prNumber: '12', + host: 'ghe.example.com', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '[comment 12345](https://ghe.example.com/corp/widgets/pull/12#discussion_r12345)', + ); + }); + + it('leaves short ids bare on github.com — ordinals are not anchors', () => { + const r = composeReview({ + cannotTellCriticals: ['comment 12345 (a.ts) — body truncated'], + planPath: coveredPlan(undefined, { + ownerRepo: 'QwenLM/qwen-code', + prNumber: '8388', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '- **[Critical]** comment 12345 (a.ts) — body truncated', + ); + expect(r.body).not.toContain('discussion_r12345'); + }); + + it('reads a cased or :443-suffixed github.com as the default host', () => { + // GH_HOST reaches the anchor builder through resolveGhHost; a cased + // variant of the default host must not dodge the short-id floor. + process.env['GH_HOST'] = 'GitHub.com:443'; + const r = composeReview({ + cannotTellCriticals: ['comment 12345 (a.ts) — body truncated'], + planPath: coveredPlan(undefined, { + ownerRepo: 'QwenLM/qwen-code', + prNumber: '8388', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '- **[Critical]** comment 12345 (a.ts) — body truncated', + ); + expect(r.body).not.toContain('discussion_r12345'); + }); + + it('anchors an Issue-level mention at #issuecomment whatever its casing', () => { + // pr-context renders `**Issue-level comment**` capitalized; an entry + // echoing that casing must still anchor under #issuecomment, not + // #discussion_r — an anchor GitHub cannot resolve. The link text keeps + // the entry's own casing: the linkifier navigates, it does not rewrite. + const r = composeReview({ + cannotTellCriticals: [ + 'Issue-level comment 5199834809 (author review) — body truncated', + ], + planPath: coveredPlan(undefined, { + ownerRepo: 'QwenLM/qwen-code', + prNumber: '8388', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '[Issue-level comment 5199834809](https://github.com/QwenLM/qwen-code/pull/8388#issuecomment-5199834809)', + ); + }); + + it('falls back to github.com when the recorded host is not a hostname', () => { + const r = composeReview({ + cannotTellCriticals: ['comment 3733696855 (a.ts) — body truncated'], + planPath: coveredPlan(undefined, { + ownerRepo: 'QwenLM/qwen-code', + prNumber: '8388', + host: 'ghe.example.com/evil', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '[comment 3733696855](https://github.com/QwenLM/qwen-code/pull/8388#discussion_r3733696855)', + ); + expect(r.body).not.toContain('ghe.example.com/evil'); + }); + + it('leaves ids bare when the recorded ownerRepo is misshapen', () => { + // `../repo` rides the character class but is a dot segment — it must + // not reach the anchor URL's path. + const r = composeReview({ + cannotTellCriticals: ['comment 3733696855 (a.ts) — body truncated'], + planPath: coveredPlan(undefined, { + ownerRepo: '../repo', + prNumber: '8388', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '- **[Critical]** comment 3733696855 (a.ts) — body truncated', + ); + expect(r.body).not.toContain('discussion_r3733696855'); + }); + + it('anchors at the run-routed host when the plan recorded none', () => { + setGhHost('ghe.example.com'); + try { + const r = composeReview({ + cannotTellCriticals: ['comment 12345 (a.ts) — body truncated'], + planPath: coveredPlan(undefined, { + ownerRepo: 'corp/widgets', + prNumber: '12', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '[comment 12345](https://ghe.example.com/corp/widgets/pull/12#discussion_r12345)', + ); + } finally { + setGhHost(undefined); + } + }); + + it('strips a copied **[Critical]** prefix from a cannot-tell entry', () => { + // The orchestrator copies blocker lines as the context file renders + // them — marker included; the bullet renders it exactly once. + const r = composeReview( + base({ + cannotTellCriticals: [ + '**[Critical]** old blocker (a.ts) — body truncated', + ], + }), + ); + expect(r.body).toContain( + '- **[Critical]** old blocker (a.ts) — body truncated', + ); + expect(r.body).not.toContain('**[Critical]** **[Critical]**'); + }); + + it('reads www./trailing-dot/zero-padded-port github.com variants as the default host', () => { + // Each is the same default instance; a variant must not dodge the + // short-id floor and link an ordinal into a dead anchor. + for (const variant of [ + 'www.github.com', + 'github.com.', + 'github.com:0443', + ]) { + process.env['GH_HOST'] = variant; + const r = composeReview({ + cannotTellCriticals: ['comment 12345 (a.ts) — body truncated'], + planPath: coveredPlan(undefined, { + ownerRepo: 'QwenLM/qwen-code', + prNumber: '8388', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '- **[Critical]** comment 12345 (a.ts) — body truncated', + ); + expect(r.body).not.toContain('discussion_r12345'); + } + // And a long id under the www variant anchors at the apex host. + process.env['GH_HOST'] = 'www.github.com'; + const r = composeReview({ + cannotTellCriticals: ['comment 3733696855 (a.ts) — body truncated'], + planPath: coveredPlan(undefined, { + ownerRepo: 'QwenLM/qwen-code', + prNumber: '8388', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '[comment 3733696855](https://github.com/QwenLM/qwen-code/pull/8388#discussion_r3733696855)', + ); + }); + + it("routes an issue-level entry's bare id to #issuecomment — the anchor family is per entry", () => { + // pr-context's own header shape carries the id apart from the phrase: + // `**Issue-level comment** — by @alice (comment 5199834809)`. Issue- + // comment ids and review-comment ids are separate id spaces, so + // routing that id by adjacency alone mints a #discussion_r anchor + // that can never resolve. + const r = composeReview({ + cannotTellCriticals: [ + '**Issue-level comment** — by @alice (comment 5199834809) — full text unfetchable', + ], + planPath: coveredPlan(undefined, { + ownerRepo: 'QwenLM/qwen-code', + prNumber: '8388', + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain('#issuecomment-5199834809'); + expect(r.body).not.toContain('discussion_r5199834809'); + }); + + it('degrades to bare ids on a corrupt plan file — never throws', () => { + // The orchestrator killed mid-write leaves plan.json truncated; the + // anchors degrade, the composition survives. + const planPath = join(dir, 'corrupt-plan.json'); + writeFileSync(planPath, '{ not json'); + const r = composeReview({ + cannotTellCriticals: ['comment 3733696855 (a.ts) — body truncated'], + planPath, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '- **[Critical]** comment 3733696855 (a.ts) — body truncated', + ); + expect(r.body).not.toContain('discussion_r'); + }); + + it('accepts a numeric prNumber — plans record both JSON forms', () => { + const r = composeReview({ + cannotTellCriticals: ['comment 3733696855 (a.ts) — body truncated'], + planPath: coveredPlan(undefined, { + ownerRepo: 'QwenLM/qwen-code', + prNumber: 8388, + }), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '[comment 3733696855](https://github.com/QwenLM/qwen-code/pull/8388#discussion_r3733696855)', + ); + }); + + it('stays linear on a cannot-tell entry with a long whitespace run', () => { + // The newline collapse must not reintroduce a quadratic scan: a + // model-written entry has no length cap, and `/\s*\n+\s*/g` was + // measured at seconds on an 80k whitespace run with no newline in it. + const flat = `comment 101 (a.ts) — body${' '.repeat(80_000)}truncated`; + const wrapped = `comment 102 (b.ts) — body\n${' '.repeat(80_000)}truncated`; + const t0 = performance.now(); + const r = composeReview(base({ cannotTellCriticals: [flat, wrapped] })); + expect(performance.now() - t0).toBeLessThan(2000); + // The multi-line entry still collapses to one list item. + expect(r.body).toContain('comment 102 (b.ts) — body truncated'); + }); +}); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 3b3b25b5f55..c6ab2216d76 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -36,7 +36,14 @@ import { readBudgetStop, } from './lib/deadline.js'; import { shellQuotePath } from './lib/shell-quote.js'; -import { gh, setGhHost } from './lib/gh.js'; +import { + HOSTNAME_RE, + gh, + getGhHost, + isOwnerRepo, + resolveGhHost, + setGhHost, +} from './lib/gh.js'; import { isPositivePrNumber, hasExecutableScript, @@ -61,6 +68,7 @@ import { type DraftedComment, } from './lib/inline-counts.js'; import { + FOOTER_MARKER, REVIEW_FOOTER_RE, footerVersion, isFooterSafeModelId, @@ -239,6 +247,182 @@ function withMarker(line: string): string { return line.startsWith(CRITICAL_PREFIX) ? line : `${CRITICAL_PREFIX} ${line}`; } +/** The plan's PR identity, when it names one — the base for comment anchors. */ +interface PrIdentity { + ownerRepo: string; + prNumber: string; + /** The host fetch-pr recorded for a non-default instance, else null. */ + host: string | null; +} + +/** + * The one rule for "this parsed plan names a PR" — the bilingual recovery + * and the comment anchors both read it, so a hardening of plan-identity + * validation lands once, not twice in this file. + */ +function planPrIdentity(plan: unknown): PrIdentity | null { + if (typeof plan !== 'object' || plan === null) return null; + const p = plan as { + ownerRepo?: unknown; + prNumber?: unknown; + host?: unknown; + }; + const ownerRepo = + typeof p.ownerRepo === 'string' && isOwnerRepo(p.ownerRepo) + ? p.ownerRepo + : null; + const prNumber = isPositivePrNumber(p.prNumber) ? String(p.prNumber) : null; + // The plan is a file on disk; hold a recorded host to the same standard + // the rest of this surface applies (HOSTNAME_RE in setGhHost) before it + // rides into a posted anchor URL. + const host = + typeof p.host === 'string' && HOSTNAME_RE.test(p.host) ? p.host : null; + return ownerRepo && prNumber ? { ownerRepo, prNumber, host } : null; +} + +function prIdentityFromPlan(planPath: string | undefined): PrIdentity | null { + if (!planPath) return null; + try { + return planPrIdentity(JSON.parse(readFileSync(planPath, 'utf8'))); + } catch { + return null; + } +} + +/** + * `comment 3733696855` in a model-written unresolved entry is a bare number + * the PR page cannot navigate; with the plan's PR identity it becomes the + * anchor GitHub already serves — review-thread comments under + * `#discussion_r`, issue-level ones under `#issuecomment`. An entry that + * already carries a markdown link is left alone: the model linked it itself, + * and rewriting inside its link text would corrupt it. + */ +function linkifyCommentRefs(text: string, pr: PrIdentity | null): string { + if (!pr || text.includes('](')) return text; + // The anchor must point at the instance the PR lives on: the host the + // plan recorded, else this run's routed host, else an operator-exported + // GH_HOST — the same effective-host resolution `submit` posts through. + // Defaulting to github.com 404s a GHE review's anchors, or lands them on + // a same-named public repo's different PR. + // Normalized before the github.com comparison below: hostnames are + // case-insensitive, :443 is the implicit port (leading zeros included), + // a trailing dot is the same DNS name, and www. fronts the same default + // instance — every one of these variants must land on the floor, or a + // `GH_HOST=www.github.com` run links an ordinal `comment 5` into a dead + // anchor. + const host = (resolveGhHost(pr.host ?? getGhHost()) ?? 'github.com') + .toLowerCase() + .replace(/:0*443$/, '') + .replace(/\.$/, '') + .replace(/^www\.github\.com$/, 'github.com'); + const base = `https://${host}/${pr.ownerRepo}/pull/${pr.prNumber}`; + // github.com's comment ids run long, so a short number after "comment" + // reads likelier as an ordinal; a GHE instance's id space is its own and + // often short, and the floor would leave the feature inert there. + // Case-insensitive: the pipeline's own label is capitalized + // (`**Issue-level comment**` in pr-context), and an entry echoing that + // casing must still anchor under #issuecomment, not #discussion_r. + const commentRef = + host === 'github.com' + ? /\b(issue-level )?comment (\d{6,})\b/gi + : /\b(issue-level )?comment (\d+)\b/gi; + // The anchor family is decided per ENTRY, not per match: issue-comment + // ids and review-comment ids are separate id spaces, and an issue-level + // entry that echoes pr-context's own header shape (`**Issue-level + // comment** — … (comment 5199834809)`) carries its id apart from the + // phrase — routed by adjacency alone, that id anchors under + // #discussion_r, a link that can never resolve. + const issueLevelEntry = /\bissue(?:-level)?\s+comment\b/i.test(text); + return text.replace( + commentRef, + (_m, issueLevel: string | undefined, id: string) => + issueLevel || issueLevelEntry + ? `[${issueLevel ?? ''}comment ${id}](${base}#issuecomment-${id})` + : `[comment ${id}](${base}#discussion_r${id})`, + ); +} + +/** + * The unresolved-existing-Critical block, as a Markdown list instead of a + * space-joined paragraph: #8388's posted body ran 31 of these together in + * one unreadable wall. Entries sharing the exact reason after their first + * ` — ` collapse into one marked group that states the reason once and + * lists the subjects — the same repetition-killing move the not-reviewed + * sentences already make. Nothing is dropped: every subject and every + * distinct reason still renders, because erasing one is how a review + * approves the very thing it is asking about. The Chinese half carries a + * count and a pointer instead of duplicating the untranslatable English + * list — on #8388 that duplication alone doubled the body. + */ +function formatCannotTell(cannotTell: string[], pr: PrIdentity | null): Bi { + const parsed = cannotTell.map((raw) => { + // Entries render as one-line list items: an unindented newline ends a + // list item (CommonMark), so a model-written entry spanning lines would + // leak its continuation out of the list. Collapsed by split/join, not + // by a `/\s*\n+\s*/g` replace: that regex backtracks quadratically on + // a long whitespace run with no newline in it, and these entries are + // model-written with no length cap — one such entry stalled a measured + // probe for seconds at 80k characters. + const unmarked = raw.startsWith(CRITICAL_PREFIX) + ? raw.slice(CRITICAL_PREFIX.length).trim() + : raw; + const line = linkifyCommentRefs( + unmarked.includes('\n') + ? unmarked + .split('\n') + .map((seg) => seg.trim()) + .filter((seg) => seg !== '') + .join(' ') + : unmarked, + pr, + ); + const idx = line.indexOf(' — '); + // `|| null`: a dangling ` — ` with nothing after it is reasonless — an + // empty-string reason would become a group key and render `2 entries — :`. + return idx === -1 + ? { head: line, reason: null } + : { + head: line.slice(0, idx), + reason: line.slice(idx + 3).trim() || null, + }; + }); + // Grouped on the exact reason text, in first-appearance order. A reasonless + // entry stays its own item — there is nothing to share. + interface Group { + reason: string | null; + heads: string[]; + } + const groups: Group[] = []; + const byReason = new Map(); + for (const p of parsed) { + const existing = p.reason === null ? undefined : byReason.get(p.reason); + if (existing) { + existing.heads.push(p.head); + continue; + } + const group: Group = { reason: p.reason, heads: [p.head] }; + groups.push(group); + if (p.reason !== null) byReason.set(p.reason, group); + } + const lines: string[] = []; + for (const { reason, heads } of groups) { + if (heads.length === 1) { + lines.push( + `- ${CRITICAL_PREFIX} ${heads[0]}${reason === null ? '' : ` — ${reason}`}`, + ); + } else { + lines.push( + `- ${CRITICAL_PREFIX} ${heads.length} entries — ${reason}:`, + ...heads.map((head) => ` - ${head}`), + ); + } + } + return { + en: `Unresolved, please confirm:\n\n${lines.join('\n')}`, + zh: `未决,请确认:共 ${cannotTell.length} 条(原文未翻译,列表见上方英文部分)。`, + }; +} + // The input arrives as JSON a model wrote, and the skill tells it to omit // fields that do not apply — so absence is normal and means zero/empty. What // must never pass is a PRESENT field of the wrong shape: `undefined + 1` is @@ -268,7 +452,14 @@ function toStringList(value: unknown, field: string): string[] { } function stripReviewFooter(entry: string): string { - return entry.replace(REVIEW_FOOTER_RE, ''); + // Guarded on the marker: the strip regex opens `\s*` under an unanchored + // search, which scans quadratically on a long whitespace run in an entry + // that carries no footer at all — and these entries are model-written + // with no length cap (measured ~20 s at 80k characters). An entry + // without the marker has nothing to strip. + return entry.includes(FOOTER_MARKER) + ? entry.replace(REVIEW_FOOTER_RE, '') + : entry; } // Booleans get the same boundary treatment as the counts: the JSON is @@ -1195,14 +1386,7 @@ function composeReviewBody( const cannotTellBlock: Bi[] = cannotTell.length === 0 ? [] - : [ - { - en: `Unresolved, please confirm: ${cannotTell - .map((l) => withMarker(l)) - .join(' ')}`, - zh: `未决,请确认:${cannotTell.map((l) => withMarker(l)).join(' ')}`, - }, - ]; + : [formatCannotTell(cannotTell, prIdentityFromPlan(input.planPath))]; // Model-written blockers: quoted as-is in both halves. const bodyCriticalBlock: Bi[] = bodyCriticals @@ -1428,6 +1612,13 @@ function composeReviewBody( }); } + // Clauses 1–4 are the verdict: short sentences that read as one opener + // paragraph. Everything after — unresolved Criticals, disclosures, body + // blockers — gets a paragraph of its own: #8388's posted body joined all + // of it with spaces, 31 unresolved entries and seven disclosures in a + // single unreadable wall. + const openerCount = clauses.length; + // 5. Unresolved existing Criticals. clauses.push(...cannotTellBlock); @@ -1458,9 +1649,21 @@ function composeReviewBody( clauses.push(...bodyCriticalBlock); } + const openerParts = clauses.slice(0, openerCount); + const paragraphs: Bi[] = [ + ...(openerParts.length > 0 + ? [ + { + en: openerParts.map((c) => c.en).join(' '), + zh: openerParts.map((c) => c.zh).join(' '), + }, + ] + : []), + ...clauses.slice(openerCount), + ]; return { event, - body: render(clauses, ' '), + body: render(paragraphs, '\n\n'), baseEvent, cappedBy, downgraded, @@ -1848,29 +2051,23 @@ function bilingualFromPlan( fetchPrBody: PrBodyFetcher = fetchPrBodyViaGh, ): boolean { if (!planPath) return false; - let plan: { - prDescriptionHasHan?: unknown; - ownerRepo?: unknown; - prNumber?: unknown; - }; + let plan: unknown; try { plan = JSON.parse(readFileSync(planPath, 'utf8')); } catch { return false; } - if (typeof plan?.prDescriptionHasHan === 'boolean') { - return plan.prDescriptionHasHan; + const han = (plan as { prDescriptionHasHan?: unknown })?.prDescriptionHasHan; + if (typeof han === 'boolean') { + return han; } - const ownerRepo = - typeof plan?.ownerRepo === 'string' && plan.ownerRepo - ? plan.ownerRepo - : undefined; - const prNumber = isPositivePrNumber(plan?.prNumber) - ? String(plan.prNumber) - : undefined; - if (!ownerRepo || !prNumber) return false; + // The identity rule is shared with the comment anchors (planPrIdentity). + // The stricter ownerRepo shape changes no outcome: a misshapen one failed + // the gh fetch and fell back to English anyway. + const pr = planPrIdentity(plan); + if (!pr) return false; try { - return /\p{Script=Han}/u.test(fetchPrBody(ownerRepo, prNumber)); + return /\p{Script=Han}/u.test(fetchPrBody(pr.ownerRepo, pr.prNumber)); } catch { return false; } diff --git a/packages/cli/src/commands/review/lib/budget.test.ts b/packages/cli/src/commands/review/lib/budget.test.ts index eea0f756fb0..91056fd3a44 100644 --- a/packages/cli/src/commands/review/lib/budget.test.ts +++ b/packages/cli/src/commands/review/lib/budget.test.ts @@ -291,15 +291,114 @@ describe('budgetGapDisclosures — the one parser of the disclosure format', () 'Budget gap: none', 'Budget gap: None.', 'Budget gap: None (all checks completed)', + 'Budget gap: (none — all planned checks completed)', + 'Budget gap: (None.)', 'Budget gap: N/A - stayed under budget', + 'Budget gap: (N/A - stayed under budget)', + 'Budget gap: none — all planned checks completed', 'Budget gap: nothing skipped', 'Budget gap: no gaps', + // The rest of the drop vocabulary, pinned — this regex is a live + // edit site, and a narrowing that turns `no checks` into a phantom + // gap must not ship green. + 'Budget gap: no checks', + 'Budget gap: nothing', + 'Budget gap: n/a', + 'Budget gap: none — planned checks completed', + 'Budget gap: none — every check covered', + 'Budget gap: none — everything completed', + // The found / to-report non-answers, and inner paren padding. + 'Budget gap: none found', + 'Budget gap: nothing to report', + 'Budget gap: no gaps found', + 'Budget gap: none ( all checks completed)', 'Budget gap:', ]) { expect(budgetGapDisclosures(line)).toEqual([]); } }); + it('keeps a REAL gap in parentheses — the paren strip fires only for placeholders', () => { + // The strip exists for `(none — all planned checks completed)`; a + // genuine parenthesized disclosure must survive it … + expect(budgetGapDisclosures('Budget gap: (chunk 2 unfetchable)')).toEqual([ + '(chunk 2 unfetchable)', + ]); + // … including the ones that merely START with a placeholder token: the + // greedy leading-token class swallows them otherwise, certifying work + // that never happened. + for (const gap of [ + '(none of the chunk-2 checks ran — the runner died)', + '(N/A — the Windows runner was unavailable)', + '(no checks ran on Windows — runner unavailable)', + ]) { + expect(budgetGapDisclosures(`Budget gap: ${gap}`)).toEqual([gap]); + } + // A completion HEAD is not a completion: these merely continue with + // an "all done" word. Dropping them certifies work that never + // happened — the exact failure the paren strip exists to kill. + for (const gap of [ + '(no checks — all deferred to follow-up)', + '(nothing — every check crashed)', + '(none — all 5 Windows checks failed to start)', + '(none — all planned checks completed except the Windows matrix)', + ]) { + expect(budgetGapDisclosures(`Budget gap: ${gap}`)).toEqual([gap]); + } + // … and inner text merely STARTING with the template/dash shapes is + // not a template or a dash run — the classifier is anchored, never + // prefix-matching. + for (const gap of [ + '( runner unavailable)', + '(- second-order callers untested)', + '(* flaky reruns pending)', + ]) { + expect(budgetGapDisclosures(`Budget gap: ${gap}`)).toEqual([gap]); + } + }); + + it('keeps a REAL gap bare too — one strict judgment for both forms', () => { + // The identical gaps without parentheses are the brief's canonical + // form; they must survive the same strict shapes, not fall to a + // greedier bare-path class. + for (const gap of [ + 'none of the chunk-2 checks ran — the runner died', + 'N/A — the Windows runner was unavailable', + 'no checks ran on Windows — runner unavailable', + 'no checks — all deferred to follow-up', + 'nothing — every check crashed', + 'none — all 5 Windows checks failed to start', + 'none — all planned checks completed except the Windows matrix', + ' runner unavailable', + ]) { + expect(budgetGapDisclosures(`Budget gap: ${gap}`)).toEqual([gap]); + } + }); + + it('keeps the stayed / negated-completion / exception shapes — real gaps that brush the idioms', () => { + for (const gap of [ + // The stayed idiom is end-anchored: text continuing past `budget` + // discloses skipped work, and `stayed` heading somewhere else + // entirely is no completion at all. + 'N/A - stayed under budget, but the Windows matrix never ran', + 'no checks — stayed queued behind the runner outage', + 'none — stayed under budget but skipped the Windows matrix', + // A completion word that is NEGATED is a failure report ending in + // "completed", not completion. + 'none — all checks crashed, none completed', + 'no checks — all deferred, nothing finished', + // An exception quantifier between head and completion word restricts + // the claim — `all but X completed` names the X that was not. + 'none — all but the Windows checks completed', + 'none — all but one check completed', + ]) { + expect(budgetGapDisclosures(`Budget gap: ${gap}`)).toEqual([gap]); + expect(budgetGapDisclosures(`Budget gap: (${gap})`)).toEqual([ + `(${gap})`, + ]); + } + }); + it('folds duplicate disclosures into one gap', () => { // An agent commonly states its gap mid-return and restates it in the // closing summary — one gap, not two, and duplicates must not consume @@ -311,6 +410,20 @@ describe('budgetGapDisclosures — the one parser of the disclosure format', () 'Budget gap: Second-order callers', ), ).toEqual(['second-order callers']); + // … whether or not the restatement wraps the gap in parentheses … + expect( + budgetGapDisclosures( + 'Budget gap: auth flow untested\n' + 'Budget gap: (auth flow untested)', + ), + ).toEqual(['auth flow untested']); + // … and the fold survives sentence punctuation INSIDE the parens — + // a parenthesized sentence naturally ends in a period. + expect( + budgetGapDisclosures( + 'Budget gap: (auth flow untested.)\n' + + 'Budget gap: auth flow untested', + ), + ).toEqual(['(auth flow untested.)']); }); it('sanitizes and caps what will reach a terminal and the posted body', () => { @@ -344,10 +457,32 @@ describe('budgetGapDisclosures — the one parser of the disclosure format', () // The previous single multiline regex was measured at 5.8 s on 98 KB // of newlines — quadratic backtracking from every line start. The // line-based scan has no cross-line class to backtrack over. - const pathological = '-\n'.repeat(49_000) + ' \n> - '.repeat(20_000); + const pathological = + '-\n'.repeat(49_000) + ' \n> - '.repeat(20_000) + ' '.repeat(40_000); const t0 = performance.now(); expect(budgetGapDisclosures(pathological)).toEqual([]); expect(performance.now() - t0).toBeLessThan(1000); + // The placeholder classifier's own hazard shape — a token followed by + // a long whitespace run — must stay linear too; it was measured + // quadratic (seconds at 40k spaces) when its quantifiers overlapped. + const spaced = `Budget gap: (none${' '.repeat(160_000)}x)`; + const t1 = performance.now(); + expect(budgetGapDisclosures(spaced)).toHaveLength(1); + expect(performance.now() - t1).toBeLessThan(1000); + // The line matcher's own hazard shape — a long indentation run on a + // line that is NOT a disclosure. The pre-rewrite matcher's overlapping + // `[ \t]*` pair backtracked quadratically here (seconds at 40k tabs); + // the disclosure on the line above pins that a real gap still parses + // out of the same text. + const indented = `Budget gap: ok\n${'\t'.repeat(40_000)}not a gap line`; + const t2 = performance.now(); + expect(budgetGapDisclosures(indented)).toEqual(['ok']); + expect(performance.now() - t2).toBeLessThan(1000); + // And a deep-indented bullet disclosure still matches — the leading + // whitespace lives inside the optional bullet group, not beside it. + expect( + budgetGapDisclosures(`${'\t'.repeat(4000)}- Budget gap: the check`), + ).toEqual(['the check']); }); }); diff --git a/packages/cli/src/commands/review/lib/budget.ts b/packages/cli/src/commands/review/lib/budget.ts index fdd16fa9688..0e2b3a49ca3 100644 --- a/packages/cli/src/commands/review/lib/budget.ts +++ b/packages/cli/src/commands/review/lib/budget.ts @@ -254,8 +254,13 @@ export function launchToolBudget( * disclosures, and a disclosure lost to a bullet is unobservable — * nothing downstream can tell "no gaps" from "gaps we failed to parse". */ +// Linear-by-construction, for the same reason the scan is line-based: the +// bullet's leading whitespace rides INSIDE the optional group (no +// overlapping `[ \t]*` pair), and the gap capture is greedy to the end of +// a pre-trimmed line (no lazy-dot vs trailing-whitespace pair) — gap lines +// carrying long whitespace runs must not stall the parse. const BUDGET_GAP_LINE_RE = - /^[ \t]*(?:[-*+]|\d+[.)])?[ \t]*(`?)[*_~]{0,3}(?:budget gap|预算(?:缺口|不足|用尽))[*_~]{0,3}[ \t]*[::][*_~]{0,3}[ \t]*(.+?)[ \t]*$/i; + /^(?:[ \t]*(?:[-*+]|\d+[.)]))?[ \t]*(`?)[*_~]{0,3}(?:budget gap|预算(?:缺口|不足|用尽))[*_~]{0,3}[ \t]*[::][*_~]{0,3}[ \t]*(.+)$/i; /** A cheap pre-filter so the line walk skips returns with nothing to find. */ const GAP_HINT_RE = /budget gap|预算(?:缺口|不足|用尽)/i; @@ -273,15 +278,49 @@ export const INLINE_BUDGET_GAP_RE = /(?:budget gap|预算(?:缺口|不足|用尽))[*_~`]{0,3}[ \t]*[::]/i; /** - * Templates and non-answers that must not become gaps someone rules on. - * Tested against the gap with trailing punctuation stripped, and matched on - * the LEADING token — `none.`, `None (all checks completed)` and - * `N/A - stayed under budget` are all the agent saying it has nothing to - * disclose, and a phantom gap costs real rounds downstream (a chunk that - * never retires, a body that discloses "None." on an Approve). + * Templates and non-answers that must not become gaps someone rules on — + * the agent saying it has nothing to disclose. A phantom gap costs real + * rounds downstream (a chunk that never retires, a body that discloses + * "None." on an Approve), so these shapes are dropped. + * + * One classifier judges the paren-stripped text, bare and wrapped alike: + * #8388's posted body disclosed `(none — all planned checks completed)` + * because a leading `(` defeated the match, and a bare-vs-wrapped split + * judgment let the two forms diverge on identical content. The vocabulary + * lives in this one regex for the same reason. + * + * The shapes are deliberately NARROW, because the two errors are not + * symmetric: dropping a REAL gap certifies work that never happened (the + * failure #8388's body shipped), while keeping a placeholder only + * over-discloses. Anything outside them survives as a gap: + * + * - the brief's own `` template, and dash-only text — both + * end-anchored, so inner text merely STARTING with them keeps; + * - a bare placeholder token in any trailing punctuation (`none`, + * `None.`, `no gaps`), and the non-answer idioms `nothing skipped`, + * `none found`, `nothing to report`; + * - the stayed-under-budget idiom, end-anchored like its siblings + * (`N/A - stayed under budget`); text continuing past `budget` keeps + * (`N/A - stayed under budget, but the Windows matrix never ran`); + * - the completion idiom — token, dash, an "all done" head, then a + * completion word the text ENDS with (`none — all planned checks + * completed`). The head alone is not completion (`none — all 5 + * Windows checks failed to start` keeps), the completion word must be + * AFFIRMED (`none — all checks crashed, none completed` keeps), and + * the span must not cross an exception (`none — all but the Windows + * checks completed` keeps); + * - a token followed by a parenthesized completion clause (`None (all + * checks completed)`), inner padding tolerated — under the same + * negation and exception guards. + * + * No two quantifiers overlap on whitespace: a placeholder token followed + * by a long whitespace run must stay linear (the module header's hazard + * note applies — this parse runs on every agent return). The completion + * spans are tempered (a per-character exception lookahead), which keeps + * them linear too. */ const PLACEHOLDER_GAP_RE = - /^(?:<[^>]*>|none\b.*|n\/a\b.*|nothing\b.*|no (?:gaps?|checks?)\b.*|[-—*_~`]+)$/i; + /^(?:<[^>]*>$|[-—*_~`]+$|(?:none|n\/a|nothing|no (?:gaps?|checks?))\b(?:[.!…,;:\s]*$|\s+(?:skipped|found|to report)\b[.!…,;:\s]*$|\s*[-—–]\s*(?:stayed\s+(?:under|within|below)\s+budget\b[.!…,;:\s]*$|(?:all|every(?:thing)?|planned|further|no further)\b(?:(?!\b(?:but|except|excepting|excluding)\b).)*(? 0 && TRAILING_GAP_CHAR_RE.test(s.charAt(end - 1))) end--; + return s.slice(0, end); +} + /** Truncate on code points — a slice through a surrogate pair is mojibake. */ function truncateGap(s: string): string { const points = [...s]; @@ -361,8 +412,11 @@ export function budgetGapDisclosures(finalText: string): string[] { if (/^[ \t]*>/.test(line)) continue; // Sanitized BEFORE matching: U+2028/29 are line terminators to the // regex dot, and a gap carrying one would otherwise fail the match and - // vanish — silent loss in a channel whose promise is delivery. - const m = BUDGET_GAP_LINE_RE.exec(line.replace(DANGEROUS_CHARS_RE, ' ')); + // vanish — silent loss in a channel whose promise is delivery. The + // pre-trim keeps the greedy end-anchored capture's code-span + // `endsWith` semantics on lines with trailing whitespace. + const sanitized = line.replace(DANGEROUS_CHARS_RE, ' ').trimEnd(); + const m = BUDGET_GAP_LINE_RE.exec(sanitized); if (!m) continue; // A line written as a code span (`Budget gap: …`) is only taken when // the backtick closes — and then unwrapped with its partner, so a @@ -373,11 +427,20 @@ export function budgetGapDisclosures(finalText: string): string[] { raw = raw.slice(0, -1); } raw = stripWrappers(raw.trim()).trim(); - const normalized = raw.replace(/[.!…,;:\s]+$/, '').trim(); - if (normalized.length === 0 || PLACEHOLDER_GAP_RE.test(normalized)) { + const normalized = stripTrailingGapChars(raw).trim(); + // Judged on the paren-stripped text, bare and wrapped alike, by the + // one strict classifier — its doc names why the shapes are narrow. + const unparenthesized = + normalized.startsWith('(') && normalized.endsWith(')') + ? normalized.slice(1, -1).trim() + : normalized; + if (normalized.length === 0 || PLACEHOLDER_GAP_RE.test(unparenthesized)) { continue; } - const key = normalized.toLowerCase(); + // Folded on the paren-stripped text with its OWN trailing punctuation + // gone, so one gap restated with and without parentheses — `(auth + // flow untested.)` and `auth flow untested` — discloses once. + const key = stripTrailingGapChars(unparenthesized).toLowerCase(); if (seen.has(key)) continue; seen.add(key); gaps.push(truncateGap(raw)); diff --git a/packages/cli/src/commands/review/lib/gh.ts b/packages/cli/src/commands/review/lib/gh.ts index 80558628ce8..6100f1e3ad3 100644 --- a/packages/cli/src/commands/review/lib/gh.ts +++ b/packages/cli/src/commands/review/lib/gh.ts @@ -67,7 +67,27 @@ function execGhWithRetry(args: string[], options: { input?: string }): string { let ghHost: string | undefined; -const HOSTNAME_RE = /^[A-Za-z0-9.-]+(?::\d+)?$/; +export const HOSTNAME_RE = /^[A-Za-z0-9.-]+(?::\d+)?$/; + +const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; + +/** + * `owner/repo` — and neither half may be a dot segment. + * + * The character class alone admits `../repo`, `owner/..` and `./repo`: `.` + * and `..` are made of legal characters and mean something else entirely + * once they reach a URL path. One home for the rule — submit's --repo + * check and compose-review's plan identity both build API/anchor URLs + * from it, and a hardening that lands in only one of them leaves the + * other URL-building site on the stale rule. + */ +export function isOwnerRepo(repo: string): boolean { + const parts = repo.split('/'); + return ( + parts.length === 2 && + parts.every((p) => REPO_SEGMENT.test(p) && p !== '.' && p !== '..') + ); +} /** * Route every subsequent `gh` invocation in this process at a GitHub host diff --git a/packages/cli/src/commands/review/lib/review-footer.ts b/packages/cli/src/commands/review/lib/review-footer.ts index 7f4f49d9581..cf42b9ddd8b 100644 --- a/packages/cli/src/commands/review/lib/review-footer.ts +++ b/packages/cli/src/commands/review/lib/review-footer.ts @@ -17,7 +17,7 @@ // this directory already shares between the same two commands. /** The attribution marker the strip regex anchors on. */ -const FOOTER_MARKER = 'via Qwen Code /review'; +export const FOOTER_MARKER = 'via Qwen Code /review'; /** The footer naming the reviewing model and the CLI version it ran under. */ export function reviewFooter(modelId: string, cliVersion: string): string { diff --git a/packages/cli/src/commands/review/submit.ts b/packages/cli/src/commands/review/submit.ts index 866a58a18d5..0eb2f17ebd8 100644 --- a/packages/cli/src/commands/review/submit.ts +++ b/packages/cli/src/commands/review/submit.ts @@ -48,7 +48,12 @@ import { atomicWriteFileSync } from '@qwen-code/qwen-code-core'; import { mkdirSync, readFileSync } from 'node:fs'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { getCliVersion } from '../../utils/version.js'; -import { ghWithInput, resolveGhHost, setGhHost } from './lib/gh.js'; +import { + ghWithInput, + isOwnerRepo, + resolveGhHost, + setGhHost, +} from './lib/gh.js'; import { REVIEW_TMP_DIR, tmpFile } from './lib/paths.js'; import { parseReceiptIds } from './lib/receipt.js'; import { composeReview, type ComposeReviewInput } from './compose-review.js'; @@ -368,28 +373,12 @@ function inconsistencies(payload: ReviewPayload, event: string): string[] { return problems; } -/** - * `owner/repo` — and neither half may be a dot segment. - * - * The character class alone admits `../repo`, `owner/..` and `./repo`: `.` and - * `..` are made of legal characters and mean something else entirely once they - * reach a URL path. - */ -const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; -function isRepo(repo: string): boolean { - const parts = repo.split('/'); - return ( - parts.length === 2 && - parts.every((p) => REPO_SEGMENT.test(p) && p !== '.' && p !== '..') - ); -} - export function runSubmit(args: SubmitArgs, cliVersion = 'unknown'): void { setGhHost(args.host); // The repo goes straight into the API path. A malformed value does not fail // safely — it fails as a confusing 404 from a URL nobody meant to build. - if (!isRepo(args.repo)) { + if (!isOwnerRepo(args.repo)) { throw new Error( `--repo ${JSON.stringify(args.repo)} is not /.`, );