From 324a3c1fd0f9c27ed563dd1cc3ac5979b4201e5c Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 01:57:02 +0800 Subject: [PATCH 01/12] feat(review): add comment-status helper for existing-thread triage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One deterministic pass over a PR's existing inline comments, replacing the per-comment `gh api` fetches the orchestrating model used to make during /review: anchor validity at the live head (outdated detection, with a file-level exemption), whether the anchored file changed in the reviewed worktree since each comment's commit and which commits touched it (the re-check's candidate "fixed by" list), reply participation and PR-author response, the blocker signal (same carriesBlockerSignal as pr-context, so the two surfaces agree by construction), and worktree-vs-live head drift. Measured on a heavily discussed PR (72+ inline comments), a single review run burned 20+ model turns re-deriving exactly these fields one comment id at a time. SKILL.md now runs the subcommand in Step 1 and routes the Step 6 re-check's status questions at the report; comment bodies stay in the pr-context file under its untrusted-data preamble, and a comment-status failure only warns — it is an index, not the evidence, so it never sets the context-unavailable state. --- packages/cli/src/commands/review.ts | 4 +- .../commands/review/comment-status.test.ts | 200 ++++++++++ .../cli/src/commands/review/comment-status.ts | 357 ++++++++++++++++++ .../core/src/skills/bundled/review/SKILL.md | 11 +- 4 files changed, 570 insertions(+), 2 deletions(-) create mode 100644 packages/cli/src/commands/review/comment-status.test.ts create mode 100644 packages/cli/src/commands/review/comment-status.ts diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index 1b8447b0f2f..a42257b772f 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -15,6 +15,7 @@ import { fetchPrCommand } from './review/fetch-pr.js'; import { captureLocalCommand } from './review/capture-local.js'; import { planDiffCommand } from './review/plan-diff.js'; import { prContextCommand } from './review/pr-context.js'; +import { commentStatusCommand } from './review/comment-status.js'; import { loadRulesCommand } from './review/load-rules.js'; import { presubmitCommand } from './review/presubmit.js'; import { resolveAnchorsCommand } from './review/resolve-anchors.js'; @@ -36,6 +37,7 @@ export const reviewCommand: CommandModule = { .command(captureLocalCommand) .command(planDiffCommand) .command(prContextCommand) + .command(commentStatusCommand) .command(loadRulesCommand) .command(agentPromptCommand) .command(buildTestCommand) @@ -48,7 +50,7 @@ export const reviewCommand: CommandModule = { .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: parse-args, fetch-pr, capture-local, plan-diff, pr-context, load-rules, agent-prompt, build-test, resolve-anchors, check-coverage, presubmit, test-efficacy, compose-review, submit, or cleanup.', + 'Specify a subcommand: parse-args, fetch-pr, capture-local, plan-diff, pr-context, comment-status, load-rules, agent-prompt, build-test, resolve-anchors, check-coverage, presubmit, test-efficacy, compose-review, submit, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/comment-status.test.ts b/packages/cli/src/commands/review/comment-status.test.ts new file mode 100644 index 00000000000..eef5f63f34a --- /dev/null +++ b/packages/cli/src/commands/review/comment-status.test.ts @@ -0,0 +1,200 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { + buildThreadStatuses, + summarizeThreads, + type CodeChangeProbe, + type RawStatusComment, +} from './comment-status.js'; + +const noChange: CodeChangeProbe = () => ({ changed: false, touchedBy: [] }); + +const comment = (over: Partial & { id: number }) => + ({ + user: { login: 'reviewer' }, + body: 'looks odd', + path: 'src/a.ts', + line: 10, + original_line: 10, + commit_id: 'headsha', + original_commit_id: 'oldsha', + created_at: '2026-07-24T10:00:00Z', + subject_type: 'line', + ...over, + }) as RawStatusComment; + +describe('buildThreadStatuses — thread grouping', () => { + it('groups nested reply chains under the root and sorts replies by id', () => { + const threads = buildThreadStatuses( + [ + comment({ id: 1 }), + comment({ id: 3, in_reply_to_id: 2, user: { login: 'author' } }), + comment({ id: 2, in_reply_to_id: 1 }), + ], + 'author', + noChange, + ); + expect(threads).toHaveLength(1); + expect(threads[0].rootId).toBe(1); + expect(threads[0].replies.map((r) => r.id)).toEqual([2, 3]); + expect(threads[0].participants).toEqual(['reviewer', 'author']); + }); + + it('drops replies whose root was deleted, same as pr-context renders', () => { + const threads = buildThreadStatuses( + [comment({ id: 5, in_reply_to_id: 999 })], + 'author', + noChange, + ); + expect(threads).toHaveLength(0); + }); + + it('survives a reply cycle without hanging', () => { + const threads = buildThreadStatuses( + [ + comment({ id: 1, in_reply_to_id: 2 }), + comment({ id: 2, in_reply_to_id: 1 }), + ], + 'author', + noChange, + ); + // Both point at each other; neither is a root. Nothing to report, but + // the walk must terminate. + expect(threads).toHaveLength(0); + }); + + it('sorts threads by path, then original line, then id', () => { + const threads = buildThreadStatuses( + [ + comment({ id: 3, path: 'src/b.ts', original_line: 1 }), + comment({ id: 2, path: 'src/a.ts', original_line: 20 }), + comment({ id: 1, path: 'src/a.ts', original_line: 5 }), + ], + 'author', + noChange, + ); + expect(threads.map((t) => t.rootId)).toEqual([1, 2, 3]); + }); +}); + +describe('buildThreadStatuses — anchor status', () => { + it('marks a line comment with null line as outdated', () => { + const [t] = buildThreadStatuses( + [comment({ id: 1, line: null })], + 'author', + noChange, + ); + expect(t.anchor.outdated).toBe(true); + expect(t.anchor.originalLine).toBe(10); + }); + + it('never marks a file-level comment as outdated', () => { + const [t] = buildThreadStatuses( + [ + comment({ + id: 1, + line: null, + original_line: null, + subject_type: 'file', + }), + ], + 'author', + noChange, + ); + expect(t.anchor.outdated).toBe(false); + expect(t.anchor.isFileLevel).toBe(true); + }); + + it('anchors code drift to original_commit_id, falling back to commit_id', () => { + const seen: Array = []; + const probe: CodeChangeProbe = (_path, since) => { + seen.push(since); + return { changed: 'unknown', touchedBy: [] }; + }; + buildThreadStatuses( + [ + comment({ id: 1 }), + comment({ id: 2, original_commit_id: undefined }), + comment({ id: 3, original_commit_id: undefined, commit_id: undefined }), + ], + 'author', + probe, + ); + expect(seen).toEqual(['oldsha', 'headsha', undefined]); + }); +}); + +describe('buildThreadStatuses — signals', () => { + it('flags a blocker body and threads the probe result through', () => { + const probe: CodeChangeProbe = () => ({ + changed: true, + touchedBy: ['abc1234', 'def5678'], + }); + const [t] = buildThreadStatuses( + [comment({ id: 1, body: '🔴 Finding 1 — poll loop wedges (blocker)' })], + 'author', + probe, + ); + expect(t.isBlocker).toBe(true); + expect(t.code).toEqual({ + changedSinceComment: true, + touchedBy: ['abc1234', 'def5678'], + }); + }); + + it('does not flag a negated blocker mention', () => { + const [t] = buildThreadStatuses( + [comment({ id: 1, body: 'No blockers here, just a nit.' })], + 'author', + noChange, + ); + expect(t.isBlocker).toBe(false); + }); + + it('detects a PR-author reply case-insensitively', () => { + const threads = buildThreadStatuses( + [ + comment({ id: 1 }), + comment({ id: 2, in_reply_to_id: 1, user: { login: 'OrbitZore' } }), + comment({ id: 3 }), + ], + 'orbitzore', + noChange, + ); + const byRoot = new Map(threads.map((t) => [t.rootId, t])); + expect(byRoot.get(1)!.authorReplied).toBe(true); + expect(byRoot.get(3)!.authorReplied).toBe(false); + }); +}); + +describe('summarizeThreads', () => { + it('counts each status dimension once per thread', () => { + const probe: CodeChangeProbe = (path) => + path === 'src/changed.ts' + ? { changed: true, touchedBy: ['abc1234'] } + : { changed: 'unknown', touchedBy: [] }; + const threads = buildThreadStatuses( + [ + comment({ id: 1, path: 'src/changed.ts', body: 'this is a blocker' }), + comment({ id: 2, in_reply_to_id: 1, user: { login: 'author' } }), + comment({ id: 3, path: 'src/other.ts', line: null }), + ], + 'author', + probe, + ); + expect(summarizeThreads(threads)).toEqual({ + threads: 2, + outdated: 1, + blockers: 1, + changedSinceComment: 1, + changeUnknown: 1, + withReplies: 1, + authorReplied: 1, + }); + }); +}); diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts new file mode 100644 index 00000000000..3d0a4d2c88c --- /dev/null +++ b/packages/cli/src/commands/review/comment-status.ts @@ -0,0 +1,357 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review comment-status`: one deterministic pass over a PR's existing +// inline comments, emitting a per-thread status index — anchor validity at the +// live head, whether the anchored file changed in the reviewed worktree since +// the comment was filed (and which commits touched it), reply participation, +// and the blocker signal. +// +// This exists because the questions it answers used to be re-derived by the +// orchestrating model one `gh api` call at a time. Measured on a real +// heavily-discussed PR (#7632, 72 inline comments), a single review run made +// 20+ individual `gh api repos/…/pulls/comments/` fetches — each a whole +// model turn — asking only "is this comment outdated?", "did the code move +// since?", "did anyone answer it?". Those are facts, not judgments; a +// subcommand answers all of them in one call. Bodies are NOT here: they stay +// in the pr-context file, which renders them under its untrusted-data +// preamble with per-comment fetch refs for anything it truncated. + +import type { CommandModule } from 'yargs'; +import { mkdirSync, writeFileSync } from 'node:fs'; +import { dirname } from 'node:path'; +import { writeStdoutLine } from '../../utils/stdioHelpers.js'; +import { ensureAuthenticated, gh, ghApiAll, setGhHost } from './lib/gh.js'; +import { gitOpt } from './lib/git.js'; +import { carriesBlockerSignal } from './pr-context.js'; + +/** Inline review comment, as listed by `GET /pulls/{n}/comments`. */ +export interface RawStatusComment { + id: number; + user?: { login: string } | null; + body?: string; + path?: string; + /** Line in the PR's LATEST diff; null when GitHub cannot map the comment + * to it (the anchor is outdated). File-level comments are always null. */ + line?: number | null; + original_line?: number | null; + side?: string | null; + /** SHA GitHub currently anchors the comment to. */ + commit_id?: string; + /** SHA the comment was filed against. */ + original_commit_id?: string; + in_reply_to_id?: number | null; + created_at?: string; + /** 'line' for ordinary comments, 'file' for file-level ones. */ + subject_type?: string; +} + +/** + * Answers "did `path` change between `sinceSha` and the worktree HEAD, and + * which commits touched it?" — injected so the classification core stays + * pure. `'unknown'` when the question cannot be answered (no worktree, the + * comment's commit was force-pushed away, path outside the repo). + */ +export type CodeChangeProbe = ( + path: string, + sinceSha: string | undefined, +) => { changed: boolean | 'unknown'; touchedBy: string[] }; + +export interface ThreadStatus { + rootId: number; + path: string; + author: string; + createdAt: string; + /** The root body asserts a blocking defect (same semantic test pr-context + * uses to build its "Blockers to re-check" section). */ + isBlocker: boolean; + anchor: { + /** Current-diff line at the LIVE head; null = not mappable. */ + line: number | null; + originalLine: number | null; + /** line === null on a line-scoped comment: the anchor no longer maps to + * the latest diff. File-level comments are never outdated. */ + outdated: boolean; + isFileLevel: boolean; + /** SHA the comment was filed against (falls back to the current anchor + * SHA when the API omits original_commit_id). */ + commitId: string; + }; + code: { + /** Did the anchored file change between the comment's commit and the + * WORKTREE head (the code this review is ruling on)? */ + changedSinceComment: boolean | 'unknown'; + /** Short SHAs of commits in that range touching the file, newest first, + * capped — the candidate "fixed by" commits for the re-check. */ + touchedBy: string[]; + }; + replies: Array<{ id: number; author: string; createdAt: string }>; + /** The PR author replied somewhere in this thread — the strongest cheap + * signal that the concern has been seen (NOT that it is fixed). */ + authorReplied: boolean; + participants: string[]; +} + +const TOUCHED_BY_CAP = 10; + +/** Cycle-safe walk to a reply chain's root (mirrors pr-context's walk). */ +function findRootId( + startId: number, + byId: Map, +): number { + const seen = new Set(); + let cur = startId; + while (true) { + if (seen.has(cur)) return cur; + seen.add(cur); + const c = byId.get(cur); + if (!c || c.in_reply_to_id === undefined || c.in_reply_to_id === null) { + return cur; + } + cur = c.in_reply_to_id; + } +} + +/** + * Pure classification core: group the flat comment list into threads and + * compute each thread's status. Replies whose root was deleted (the id no + * longer appears in the list) are dropped, matching pr-context's rendering + * of the same data. + */ +export function buildThreadStatuses( + comments: RawStatusComment[], + prAuthor: string, + probe: CodeChangeProbe, +): ThreadStatus[] { + const byId = new Map(); + for (const c of comments) byId.set(c.id, c); + + const repliesByRoot = new Map(); + for (const c of comments) { + if (c.in_reply_to_id === undefined || c.in_reply_to_id === null) continue; + const rootId = findRootId(c.in_reply_to_id, byId); + if (rootId === c.id) continue; + if (!repliesByRoot.has(rootId)) repliesByRoot.set(rootId, []); + repliesByRoot.get(rootId)!.push(c); + } + for (const replies of repliesByRoot.values()) { + replies.sort((a, b) => a.id - b.id); + } + + const authorLc = prAuthor.toLowerCase(); + const threads: ThreadStatus[] = []; + for (const root of comments) { + if (root.in_reply_to_id !== undefined && root.in_reply_to_id !== null) { + continue; + } + const replies = repliesByRoot.get(root.id) ?? []; + const isFileLevel = root.subject_type === 'file'; + const anchorSha = root.original_commit_id || root.commit_id || ''; + const probed = probe(root.path ?? '', anchorSha || undefined); + const participants = [ + ...new Set([root, ...replies].map((c) => c.user?.login ?? 'unknown')), + ]; + threads.push({ + rootId: root.id, + path: root.path ?? '', + author: root.user?.login ?? 'unknown', + createdAt: root.created_at ?? '', + isBlocker: carriesBlockerSignal(root.body), + anchor: { + line: root.line ?? null, + originalLine: root.original_line ?? null, + outdated: + !isFileLevel && (root.line === null || root.line === undefined), + isFileLevel, + commitId: anchorSha, + }, + code: { + changedSinceComment: probed.changed, + touchedBy: probed.touchedBy, + }, + replies: replies.map((r) => ({ + id: r.id, + author: r.user?.login ?? 'unknown', + createdAt: r.created_at ?? '', + })), + authorReplied: replies.some( + (r) => (r.user?.login ?? '').toLowerCase() === authorLc, + ), + participants, + }); + } + + threads.sort((a, b) => { + const p = a.path.localeCompare(b.path); + if (p !== 0) return p; + const l = (a.anchor.originalLine ?? 0) - (b.anchor.originalLine ?? 0); + if (l !== 0) return l; + return a.rootId - b.rootId; + }); + return threads; +} + +export function summarizeThreads(threads: ThreadStatus[]) { + return { + threads: threads.length, + outdated: threads.filter((t) => t.anchor.outdated).length, + blockers: threads.filter((t) => t.isBlocker).length, + changedSinceComment: threads.filter( + (t) => t.code.changedSinceComment === true, + ).length, + changeUnknown: threads.filter( + (t) => t.code.changedSinceComment === 'unknown', + ).length, + withReplies: threads.filter((t) => t.replies.length > 0).length, + authorReplied: threads.filter((t) => t.authorReplied).length, + }; +} + +/** + * Git-backed probe against the current working directory (the skill runs + * this inside the PR worktree). Memoized per (path, sinceSha): several + * threads routinely anchor to the same file at the same commit. + */ +function makeGitProbe(): CodeChangeProbe { + const inRepo = gitOpt('rev-parse', '--is-inside-work-tree') === 'true'; + const memo = new Map>(); + return (path, sinceSha) => { + if (!inRepo || !sinceSha || !path) { + return { changed: 'unknown', touchedBy: [] }; + } + const key = `${sinceSha}\0${path}`; + const hit = memo.get(key); + if (hit) return hit; + let result: ReturnType; + // The comment's commit can be absent locally (force-pushed away and gone + // from the fetched history) — then the range is unanswerable. + if (gitOpt('cat-file', '-e', `${sinceSha}^{commit}`) === null) { + result = { changed: 'unknown', touchedBy: [] }; + } else { + const log = gitOpt('log', '--format=%h', `${sinceSha}..HEAD`, '--', path); + result = + log === null + ? { changed: 'unknown', touchedBy: [] } + : { + changed: log.length > 0, + touchedBy: log + .split('\n') + .filter(Boolean) + .slice(0, TOUCHED_BY_CAP), + }; + } + memo.set(key, result); + return result; + }; +} + +interface CommentStatusArgs { + pr_number: string; + owner_repo: string; + out: string; +} + +async function runCommentStatus(args: CommentStatusArgs): Promise { + const { pr_number: prNumber, owner_repo: ownerRepo, out } = args; + if (ownerRepo.indexOf('/') < 0) { + throw new Error('owner_repo must look like "owner/repo"'); + } + const [owner, repo] = ownerRepo.split('/'); + + ensureAuthenticated(); + + const meta = JSON.parse( + gh( + 'pr', + 'view', + prNumber, + '--repo', + ownerRepo, + '--json', + 'author,headRefOid', + ), + ) as { author?: { login?: string } | null; headRefOid?: string }; + const prAuthor = meta.author?.login ?? ''; + const liveHeadSha = meta.headRefOid ?? ''; + + const comments = ghApiAll( + `repos/${owner}/${repo}/pulls/${prNumber}/comments`, + ) as RawStatusComment[]; + + const worktreeHeadSha = gitOpt('rev-parse', 'HEAD'); + // Anchor facts (`line`, outdated) describe the LIVE head — GitHub maps + // comments against the latest diff it serves. Code facts (`touchedBy`, + // changedSinceComment) describe the WORKTREE head — the code this review + // rules on. When the two differ the PR advanced mid-review; surface it + // rather than letting the two halves silently describe different commits. + const headDrift = + worktreeHeadSha !== null && + liveHeadSha !== '' && + worktreeHeadSha !== liveHeadSha; + + const threads = buildThreadStatuses(comments, prAuthor, makeGitProbe()); + const summary = summarizeThreads(threads); + + const report = { + prNumber, + ownerRepo, + prAuthor, + liveHeadSha, + worktreeHeadSha, + headDrift, + inlineComments: comments.length, + summary, + threads, + }; + + mkdirSync(dirname(out), { recursive: true }); + writeFileSync(out, JSON.stringify(report, null, 2) + '\n', 'utf8'); + writeStdoutLine( + `Wrote comment-status report to ${out} (${comments.length} inline comments in ${summary.threads} threads: ` + + `${summary.outdated} outdated, ${summary.blockers} blocker(s), ` + + `${summary.changedSinceComment} on files changed since their comment, ` + + `${summary.withReplies} with replies, ${summary.authorReplied} answered by the PR author)`, + ); + if (headDrift) { + writeStdoutLine( + `warning: worktree HEAD ${worktreeHeadSha!.slice(0, 8)} != live PR head ${liveHeadSha.slice(0, 8)} — ` + + `the PR advanced since fetch-pr. Anchor facts describe the live head; ` + + `code facts describe the worktree.`, + ); + } +} + +export const commentStatusCommand: CommandModule = { + command: 'comment-status ', + describe: + "Emit a per-thread status index for a PR's existing inline comments (anchor validity, code drift since each comment, replies, blocker signal)", + builder: (yargs) => + yargs + .positional('pr_number', { + type: 'string', + demandOption: true, + describe: 'PR number', + }) + .positional('owner_repo', { + type: 'string', + demandOption: true, + describe: 'GitHub "owner/repo"', + }) + .option('out', { + type: 'string', + demandOption: true, + describe: 'Output JSON path (will be overwritten)', + }) + .option('host', { + type: 'string', + describe: + 'GitHub host for this PR (GitHub Enterprise). Routes every gh call in this command via GH_HOST; omit for github.com.', + }), + handler: async (argv) => { + setGhHost((argv as { host?: string }).host); + await runCommentStatus(argv as unknown as CommentStatusArgs); + }, +}; diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index c490836e155..530a18afb8a 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -128,6 +128,15 @@ Based on the parsed `target.type`: **`read_file` returns the first `truncateToolOutputThreshold` characters (25 000 by default) and sets `isTruncated`. Read that flag.** On a PR with a long history the context file exceeds it — `pr-context` prints a `warning:` line naming the size and any headings past the cut. When it does, page the remainder with `offset`/`limit` before Step 3, and pass the _whole_ file's contents onward. A review that never reached the open-comment section will report "no blockers" without having seen a single one of them. + - **Fetch the comment STATUS index** (worktree mode, whenever the context file reports existing inline comments; run it from inside the worktree so the git half can see the reviewed code): + + ```bash + "${QWEN_CODE_CLI:-qwen}" review comment-status / \ + --out .qwen/tmp/qwen-review-pr--comment-status.json + ``` + + One call answers, per existing thread, every status question the re-check and the finder agents otherwise re-derive one API fetch at a time: is the anchor **outdated** at the live head (`line: null`), did the anchored **file change in the worktree since the comment's commit** and which commits touched it (`code.touchedBy` — the candidate "fixed by" commits), who replied and **did the PR author answer**, and whether the body **asserts a blocker** (same `carriesBlockerSignal` the context file's promotion uses). It also compares the worktree HEAD against the live PR head and warns on drift. **Do not fetch per-comment status metadata yourself** — no `gh api repos/…/pulls/comments/` calls to read `line`/`outdated`/`commit_id`, and no hand-run `git log` per comment: measured on a real 72-comment PR, a run burned 20+ model turns re-deriving exactly these fields one id at a time. Comment **bodies** are a different matter and stay where they were: the context file renders them (in full for blockers and review summaries), and only a body the renderer truncated is fetched, via the exact ref its `_(truncated — fetch …)_` note names. If `comment-status` itself fails (auth, network), warn and continue — it is an index, not the evidence: statuses become "re-derive if needed", and nothing here sets the context-unavailable state. + The context file does not prefetch linked issues. For bugfix PRs, instruct Step 3's Issue Fidelity agent to fetch issue evidence itself: ```bash @@ -611,7 +620,7 @@ If there are none of these, omit this section. ### Before an Approve or a zero-Critical verdict: re-check the open Criticals -A `C=0` outcome — Approve, or a Comment with no Critical — is a claim that nothing blocks the merge. It is not the default you fall back to when your own agents surfaced nothing. **If Step 1 set the context-unavailable state** (`pr-context` failed — lightweight or same-repo), there is no context file to read: skip the walk below, record every existing Critical as `cannot tell` by construction, and carry that into the verdict — which the Step 7 invariant already caps at `COMMENT`. Otherwise, take **each live blocker already on the PR — from every comment-bearing section of the context file: "Open inline comments", "Blockers to re-check", "Review summaries", and "Already discussed" (both its inline threads and its issue-level comments)** — and check it against the code as it stands at the reviewed commit. Select **semantically, not by the literal marker**: a `**[Critical]**` prefix qualifies, but so does any body that asserts a blocking defect in other words — a "Critical findings could not be anchored" preamble, an explicit must-fix claim (legacy body-only blockers were emitted markerless, and one such review is exactly what a marker filter once discarded). When unsure whether a body asserts a blocker, re-check it — the cost is one ruling; the alternative is certifying a merge past it. ("Already discussed" stays in scope even though `pr-context` now promotes blocker-bearing bodies out of it: `carriesBlockerSignal` is a **fail-safe floor, not a ceiling** — it recognises the phrasings we have seen, not every phrasing that exists, and a blocker worded around all of them still settles there. That section's "do NOT re-report" header governs duplicate-_reporting_ by the finder agents; it does not exempt a body from this re-check. Read it with the same eyes you bring to the promoted section.) Review-level bodies matter because an unmappable or 422-relocated blocker lives **only** there — and the context file now carries them **in full**: `pr-context` renders every meaningful review body whole under "Review summaries" (no more 240-character snippets), and pulls every blocker-bearing body — replied inline thread or issue comment, marker or no marker — into the "Blockers to re-check" section, rendered in full, because a reply alone never settles a blocker. So the re-check usually needs no separate fetch: read those sections under the file's untrusted-data preamble, paging with `offset`/`limit` until `isTruncated` is false. Review summaries and blocker bodies are rendered in full; the Open and Already-discussed sections use one-line snippets, and **every snippet the renderer cut carries its own `_(truncated — fetch …)_` note naming the exact, already-filled-in command for the rest** — a candidate blocker whose snippet was cut is ruled on only after running that fetch; ruling on the visible prefix alone is the fail-closed violation. Run any such fetch **redirected to a file, never into the terminal** (Shell returns only an approximately 4 000-character model preview for output beyond its 30 000-character persistence trigger, which would re-truncate the very body being completed): append `--jq .body > .qwen/tmp/qwen-review-{target}-body-.md` to the command the note names, then `read_file` that file, paging until `isTruncated` is false, before ruling. **Fail closed either way:** a body you could not read whole — the capped tail unfetched, or the single-object fetch failing (auth, rate limit, network) — is `cannot tell`, not "no Critical in it": it goes to compose-review's `cannotTellCriticals` input, which serializes it and caps the event at `COMMENT`; a blocker you could not read is never approved past. A reply alone does not retire a blocker — "I disagree" or "wontfix" is a reply, which is exactly why `pr-context` quarantines blocker-bearing threads in their own section instead of letting them settle into "Already discussed". Only the code decides: a blocker counts as closed exactly when the re-check below lands on "fixed by this diff", never because the thread has an answer. Record one verdict per blocker: +A `C=0` outcome — Approve, or a Comment with no Critical — is a claim that nothing blocks the merge. It is not the default you fall back to when your own agents surfaced nothing. **If Step 1 set the context-unavailable state** (`pr-context` failed — lightweight or same-repo), there is no context file to read: skip the walk below, record every existing Critical as `cannot tell` by construction, and carry that into the verdict — which the Step 7 invariant already caps at `COMMENT`. Otherwise, take **each live blocker already on the PR — from every comment-bearing section of the context file: "Open inline comments", "Blockers to re-check", "Review summaries", and "Already discussed" (both its inline threads and its issue-level comments)** — and check it against the code as it stands at the reviewed commit. Select **semantically, not by the literal marker**: a `**[Critical]**` prefix qualifies, but so does any body that asserts a blocking defect in other words — a "Critical findings could not be anchored" preamble, an explicit must-fix claim (legacy body-only blockers were emitted markerless, and one such review is exactly what a marker filter once discarded). When unsure whether a body asserts a blocker, re-check it — the cost is one ruling; the alternative is certifying a merge past it. ("Already discussed" stays in scope even though `pr-context` now promotes blocker-bearing bodies out of it: `carriesBlockerSignal` is a **fail-safe floor, not a ceiling** — it recognises the phrasings we have seen, not every phrasing that exists, and a blocker worded around all of them still settles there. That section's "do NOT re-report" header governs duplicate-_reporting_ by the finder agents; it does not exempt a body from this re-check. Read it with the same eyes you bring to the promoted section.) Review-level bodies matter because an unmappable or 422-relocated blocker lives **only** there — and the context file now carries them **in full**: `pr-context` renders every meaningful review body whole under "Review summaries" (no more 240-character snippets), and pulls every blocker-bearing body — replied inline thread or issue comment, marker or no marker — into the "Blockers to re-check" section, rendered in full, because a reply alone never settles a blocker. So the re-check usually needs no separate fetch: read those sections under the file's untrusted-data preamble, paging with `offset`/`limit` until `isTruncated` is false. **For the status half of each ruling — is the anchor outdated, did the anchored file change since the blocker was filed, which commits touched it — read Step 1's `comment-status` report instead of fetching per-comment metadata**: its `code.touchedBy` list is the candidate "fixed by" commits to read, and `changedSinceComment: false` (with no head drift) tells you the anchored file is untouched since the blocker — so a claimed fix, if any, must live in some OTHER file, and the mechanism-read below is still owed either way. The report never substitutes for reading the code: it routes the read, it does not rule. Review summaries and blocker bodies are rendered in full; the Open and Already-discussed sections use one-line snippets, and **every snippet the renderer cut carries its own `_(truncated — fetch …)_` note naming the exact, already-filled-in command for the rest** — a candidate blocker whose snippet was cut is ruled on only after running that fetch; ruling on the visible prefix alone is the fail-closed violation. Run any such fetch **redirected to a file, never into the terminal** (Shell returns only an approximately 4 000-character model preview for output beyond its 30 000-character persistence trigger, which would re-truncate the very body being completed): append `--jq .body > .qwen/tmp/qwen-review-{target}-body-.md` to the command the note names, then `read_file` that file, paging until `isTruncated` is false, before ruling. **Fail closed either way:** a body you could not read whole — the capped tail unfetched, or the single-object fetch failing (auth, rate limit, network) — is `cannot tell`, not "no Critical in it": it goes to compose-review's `cannotTellCriticals` input, which serializes it and caps the event at `COMMENT`; a blocker you could not read is never approved past. A reply alone does not retire a blocker — "I disagree" or "wontfix" is a reply, which is exactly why `pr-context` quarantines blocker-bearing threads in their own section instead of letting them settle into "Already discussed". Only the code decides: a blocker counts as closed exactly when the re-check below lands on "fixed by this diff", never because the thread has an answer. Record one verdict per blocker: - **still stands** — the defect is present in the code you just read. It blocks: the event is `REQUEST_CHANGES`, and the finding goes inline (or into the body if it cannot be anchored). - **fixed by this diff** — you traced the blocker's **mechanism** through the code as it now stands and it can no longer fire. Say nothing; do not re-report it. A GitHub thread can read `isResolved: false, isOutdated: false` for a bug a later commit fixed on an adjacent line — the flag tracks the anchored line, not the fix, so the flag is not evidence either way. Only the code is. From d098e4feb6b91ab16dc8e7dd259d816cbdd33076 Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 02:23:11 +0800 Subject: [PATCH 02/12] test(review): add comment-status to the subcommand registry expectations --- packages/cli/src/commands/review.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index 7458737df32..f65e0ae25db 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -44,6 +44,7 @@ describe('reviewCommand', () => { 'capture-local', 'plan-diff', 'pr-context', + 'comment-status', 'load-rules', 'agent-prompt', 'build-test', From 7c899d12492b90ed8c2c25dfbf1024fed7adb2d0 Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 02:44:30 +0800 Subject: [PATCH 03/12] =?UTF-8?q?fix(review):=20comment-status=20review=20?= =?UTF-8?q?follow-ups=20=E2=80=94=20size=20warning,=20--host=20wiring,=20s?= =?UTF-8?q?cope=20clauses?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review at d098e4feb: - High: warn when the report exceeds read_file's truncation threshold, mirroring pr-context — measured 53k chars on the benchmark PR, where a single read lost 36 of 71 threads (24 blocker-flagged) and the cut JSON did not parse. The warning points at jq first: the file is machine-shaped in a way the Markdown context file is not. - Medium: thread --host through — the SKILL.md command block now says to pass it (each subcommand is its own process, so a host set elsewhere cannot carry over), and comment-status joins the host-required lists in SKILL.md and the code-review docs. - Minor: Step 6's routing sentence now states both scope limits — the report exists only when Step 1 wrote it (worktree mode), and it indexes inline threads only; issue-/review-level blockers keep the context-file walk. - Minor: touchedByTotal exposes the real commit count behind the capped touchedBy list, so a cut list is visible instead of reading as "the fix is not among them". - Nits: drop the never-read `side` field; guard authorReplied against a deleted-author/deleted-replier '' === '' match; correct the force-push doc comment (a shared object database usually retains the old commit, so the range widens — fail-safe — rather than going unknown). --- docs/users/features/code-review.md | 2 +- .../commands/review/comment-status.test.ts | 25 ++++++- .../cli/src/commands/review/comment-status.ts | 73 +++++++++++++------ .../core/src/skills/bundled/review/SKILL.md | 8 +- 4 files changed, 79 insertions(+), 29 deletions(-) diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index cdbf6940a0a..fd69536aca4 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -273,7 +273,7 @@ Reports include: timestamp, diff stats, build/test results, all findings with ve The deterministic halves of the pipeline — argument parsing (`qwen review parse-args`) and the event/body decision (`qwen review compose-review`) — are tested subcommands rather than prompt text, so `--effort` grammar, `--comment` forcing, verdict caps, and downgrade behavior are pinned by unit tests and cannot drift with the model. -**GitHub Enterprise:** reviewing a PR URL on a non-`github.com` host routes every GitHub call at that host — the review subcommands (`fetch-pr`, `pr-context`, `presubmit`) accept `--host` and set it in code, so a forgotten host cannot silently retarget the review at `github.com`. +**GitHub Enterprise:** reviewing a PR URL on a non-`github.com` host routes every GitHub call at that host — the review subcommands (`fetch-pr`, `pr-context`, `comment-status`, `presubmit`) accept `--host` and set it in code, so a forgotten host cannot silently retarget the review at `github.com`. Every run ends with one machine-readable line (`Review complete: — `), so scripts and CI wrappers can detect completion and outcome with a single `^Review complete: ` match. diff --git a/packages/cli/src/commands/review/comment-status.test.ts b/packages/cli/src/commands/review/comment-status.test.ts index eef5f63f34a..ad09f3deb34 100644 --- a/packages/cli/src/commands/review/comment-status.test.ts +++ b/packages/cli/src/commands/review/comment-status.test.ts @@ -12,7 +12,11 @@ import { type RawStatusComment, } from './comment-status.js'; -const noChange: CodeChangeProbe = () => ({ changed: false, touchedBy: [] }); +const noChange: CodeChangeProbe = () => ({ + changed: false, + touchedBy: [], + touchedByTotal: 0, +}); const comment = (over: Partial & { id: number }) => ({ @@ -114,7 +118,7 @@ describe('buildThreadStatuses — anchor status', () => { const seen: Array = []; const probe: CodeChangeProbe = (_path, since) => { seen.push(since); - return { changed: 'unknown', touchedBy: [] }; + return { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; }; buildThreadStatuses( [ @@ -134,6 +138,7 @@ describe('buildThreadStatuses — signals', () => { const probe: CodeChangeProbe = () => ({ changed: true, touchedBy: ['abc1234', 'def5678'], + touchedByTotal: 12, }); const [t] = buildThreadStatuses( [comment({ id: 1, body: '🔴 Finding 1 — poll loop wedges (blocker)' })], @@ -144,6 +149,7 @@ describe('buildThreadStatuses — signals', () => { expect(t.code).toEqual({ changedSinceComment: true, touchedBy: ['abc1234', 'def5678'], + touchedByTotal: 12, }); }); @@ -156,6 +162,17 @@ describe('buildThreadStatuses — signals', () => { expect(t.isBlocker).toBe(false); }); + it('never reports authorReplied for a ghost author matching a ghost replier', () => { + // A deleted PR-author account and a deleted reply account both fall back + // to '' — without the guard they match each other. + const [t] = buildThreadStatuses( + [comment({ id: 1 }), comment({ id: 2, in_reply_to_id: 1, user: null })], + '', + noChange, + ); + expect(t.authorReplied).toBe(false); + }); + it('detects a PR-author reply case-insensitively', () => { const threads = buildThreadStatuses( [ @@ -176,8 +193,8 @@ describe('summarizeThreads', () => { it('counts each status dimension once per thread', () => { const probe: CodeChangeProbe = (path) => path === 'src/changed.ts' - ? { changed: true, touchedBy: ['abc1234'] } - : { changed: 'unknown', touchedBy: [] }; + ? { changed: true, touchedBy: ['abc1234'], touchedByTotal: 1 } + : { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; const threads = buildThreadStatuses( [ comment({ id: 1, path: 'src/changed.ts', body: 'this is a blocker' }), diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index 3d0a4d2c88c..31903038771 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -23,6 +23,7 @@ import type { CommandModule } from 'yargs'; import { mkdirSync, writeFileSync } from 'node:fs'; import { dirname } from 'node:path'; +import { DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD } from '@qwen-code/qwen-code-core'; import { writeStdoutLine } from '../../utils/stdioHelpers.js'; import { ensureAuthenticated, gh, ghApiAll, setGhHost } from './lib/gh.js'; import { gitOpt } from './lib/git.js'; @@ -38,7 +39,6 @@ export interface RawStatusComment { * to it (the anchor is outdated). File-level comments are always null. */ line?: number | null; original_line?: number | null; - side?: string | null; /** SHA GitHub currently anchors the comment to. */ commit_id?: string; /** SHA the comment was filed against. */ @@ -53,12 +53,22 @@ export interface RawStatusComment { * Answers "did `path` change between `sinceSha` and the worktree HEAD, and * which commits touched it?" — injected so the classification core stays * pure. `'unknown'` when the question cannot be answered (no worktree, the - * comment's commit was force-pushed away, path outside the repo). + * comment's commit absent from the object store, path outside the repo). + * + * Force-push caveat: the worktree shares the main clone's object database, + * so a force-pushed-away commit is often still PRESENT — the range then + * spans the whole re-pushed branch rather than the intended window. That + * errs fail-safe (it can over-report `changed: true`, never hide a change), + * so it is accepted rather than special-cased. */ export type CodeChangeProbe = ( path: string, sinceSha: string | undefined, -) => { changed: boolean | 'unknown'; touchedBy: string[] }; +) => { + changed: boolean | 'unknown'; + touchedBy: string[]; + touchedByTotal: number; +}; export interface ThreadStatus { rootId: number; @@ -87,6 +97,9 @@ export interface ThreadStatus { /** Short SHAs of commits in that range touching the file, newest first, * capped — the candidate "fixed by" commits for the re-check. */ touchedBy: string[]; + /** Real count before the cap — when it exceeds `touchedBy.length`, the + * list was cut and a fix commit may be among the ones not shown. */ + touchedByTotal: number; }; replies: Array<{ id: number; author: string; createdAt: string }>; /** The PR author replied somewhere in this thread — the strongest cheap @@ -171,15 +184,18 @@ export function buildThreadStatuses( code: { changedSinceComment: probed.changed, touchedBy: probed.touchedBy, + touchedByTotal: probed.touchedByTotal, }, replies: replies.map((r) => ({ id: r.id, author: r.user?.login ?? 'unknown', createdAt: r.created_at ?? '', })), - authorReplied: replies.some( - (r) => (r.user?.login ?? '').toLowerCase() === authorLc, - ), + // Guard the empty-author case: a deleted PR-author account and a + // deleted reply account would otherwise match as '' === ''. + authorReplied: + authorLc !== '' && + replies.some((r) => (r.user?.login ?? '').toLowerCase() === authorLc), participants, }); } @@ -220,28 +236,30 @@ function makeGitProbe(): CodeChangeProbe { const memo = new Map>(); return (path, sinceSha) => { if (!inRepo || !sinceSha || !path) { - return { changed: 'unknown', touchedBy: [] }; + return { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; } const key = `${sinceSha}\0${path}`; const hit = memo.get(key); if (hit) return hit; let result: ReturnType; - // The comment's commit can be absent locally (force-pushed away and gone - // from the fetched history) — then the range is unanswerable. + // The comment's commit can be absent from the object store — then the + // range is unanswerable. (When a force-pushed-away commit is still + // present via the shared object database, the range widens instead; + // see the CodeChangeProbe doc for why that direction is accepted.) if (gitOpt('cat-file', '-e', `${sinceSha}^{commit}`) === null) { - result = { changed: 'unknown', touchedBy: [] }; + result = { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; } else { const log = gitOpt('log', '--format=%h', `${sinceSha}..HEAD`, '--', path); - result = - log === null - ? { changed: 'unknown', touchedBy: [] } - : { - changed: log.length > 0, - touchedBy: log - .split('\n') - .filter(Boolean) - .slice(0, TOUCHED_BY_CAP), - }; + if (log === null) { + result = { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; + } else { + const shas = log.split('\n').filter(Boolean); + result = { + changed: shas.length > 0, + touchedBy: shas.slice(0, TOUCHED_BY_CAP), + touchedByTotal: shas.length, + }; + } } memo.set(key, result); return result; @@ -308,7 +326,8 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { }; mkdirSync(dirname(out), { recursive: true }); - writeFileSync(out, JSON.stringify(report, null, 2) + '\n', 'utf8'); + const json = JSON.stringify(report, null, 2) + '\n'; + writeFileSync(out, json, 'utf8'); writeStdoutLine( `Wrote comment-status report to ${out} (${comments.length} inline comments in ${summary.threads} threads: ` + `${summary.outdated} outdated, ${summary.blockers} blocker(s), ` + @@ -322,6 +341,18 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { `code facts describe the worktree.`, ); } + // Same silent-tail hazard pr-context already warns about, with sharper + // teeth here: `threads` is sorted by path, so a truncated read drops the + // alphabetically-later files WHOLESALE (measured on a 71-thread PR: one + // read showed 35 threads and lost 24 blocker-flagged ones), and cut JSON + // is not merely incomplete but unparseable. + if (json.length > DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD) { + writeStdoutLine( + `warning: ${out} is ${json.length} chars; read_file returns the first ` + + `${DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD} and sets isTruncated — cut JSON does not parse. ` + + `Query it with jq (it is machine-shaped), or page with offset/limit until isTruncated is false.`, + ); + } } export const commentStatusCommand: CommandModule = { diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 530a18afb8a..863ccf299b0 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -75,7 +75,7 @@ The parser already classified the target, so there is nothing to disambiguate by 1. Check if any git remote matches the URL's **host and owner/repo — by exact segment equality, never substring**: run `git remote -v` and parse each remote URL structurally (`git@:/.git` and `https:////(.git)` are the two shapes). A remote matches only when its host equals the verdict's `host` AND its `/` (with any `.git` suffix stripped) equals the verdict's `owner/repo`, both compared case-insensitively as whole segments — `shao/qwen-code` does NOT match a `wenshao/qwen-code` remote, and a `github.com` PR does not match a same-named repo on another host. Substring "contains" matching once allowed exactly those, which is reviewing one repository and posting to another. This still handles forks — a local clone of `wenshao/jdk` with an `upstream` remote pointing to `openjdk/jdk` still matches `openjdk/jdk` PRs exactly. 2. If a matching remote is found, proceed with the **normal worktree flow** — use that remote name (instead of hardcoded `origin`) for `git fetch pull//head:qwen-review/pr-`. In Step 7, use the owner/repo from the URL for posting comments. -For a `pr-url` whose `host` is not `github.com` (GitHub Enterprise), **pass `--host ` to every review subcommand that talks to GitHub — `fetch-pr`, `pr-context`, and `presubmit`** — which routes all of their `gh` calls via GH_HOST in code; a forgotten host cannot silently retarget them at github.com. The `gh` commands you run directly are still yours to route: prefix Agent 0's `gh pr view`/`gh issue view`, Step 6's residual body fetch, and the Step 7 submission with `GH_HOST= ` (e.g. `GH_HOST=github.example.com gh api ...`). `gh` defaults to `github.com`, so a dropped host makes a call read from and post to the wrong site's `owner/repo`. +For a `pr-url` whose `host` is not `github.com` (GitHub Enterprise), **pass `--host ` to every review subcommand that talks to GitHub — `fetch-pr`, `pr-context`, `comment-status`, and `presubmit`** — which routes all of their `gh` calls via GH_HOST in code; a forgotten host cannot silently retarget them at github.com. The `gh` commands you run directly are still yours to route: prefix Agent 0's `gh pr view`/`gh issue view`, Step 6's residual body fetch, and the Step 7 submission with `GH_HOST= ` (e.g. `GH_HOST=github.example.com gh api ...`). `gh` defaults to `github.com`, so a dropped host makes a call read from and post to the wrong site's `owner/repo`. 3. If **no remote matches**, use **lightweight mode**: run `gh pr diff ` to get the diff directly. Skip Step 2 (no local rules) and Step 8 (no local reports or cache). In Step 9, skip worktree removal (none was created) but still clean up temp files (`.qwen/tmp/qwen-review-{target}-*`). Also run `"${QWEN_CODE_CLI:-qwen}" review pr-context / --out .qwen/tmp/qwen-review-pr--context.md` — it is pure GitHub API and works cross-repo. Agent 0 and Step 6's open-Critical re-check depend on it: a `Refs #123`-style target issue is only discoverable from the PR body, and open Critical threads only from the context file, so skipping it lets a wrong-root fix sail through blocker-free. If `pr-context` fails here (auth, network), warn and continue with the diff alone — but skip Agent 0 (it has nothing to work from) and treat every open-Critical re-check verdict as "cannot tell", which forbids an Approve. Carry this forward as the **context-unavailable** state: Step 7's invariant caps **every** `C=0` outcome of such a run at `COMMENT` with a diff-only body (both the would-be APPROVE and the Suggestion-only "no blockers" sentence), so a run that could not see the PR's existing discussion can post findings but never certify the absence of blockers. In Step 7, use the owner/repo from the URL. Inform the user: "Cross-repo review: running in lightweight mode (no build/test)." @@ -133,9 +133,11 @@ Based on the parsed `target.type`: ```bash "${QWEN_CODE_CLI:-qwen}" review comment-status / \ --out .qwen/tmp/qwen-review-pr--comment-status.json + # GitHub Enterprise: add --host , same as fetch-pr/pr-context/presubmit — + # each subcommand is its own process, so a host set elsewhere does not carry over. ``` - One call answers, per existing thread, every status question the re-check and the finder agents otherwise re-derive one API fetch at a time: is the anchor **outdated** at the live head (`line: null`), did the anchored **file change in the worktree since the comment's commit** and which commits touched it (`code.touchedBy` — the candidate "fixed by" commits), who replied and **did the PR author answer**, and whether the body **asserts a blocker** (same `carriesBlockerSignal` the context file's promotion uses). It also compares the worktree HEAD against the live PR head and warns on drift. **Do not fetch per-comment status metadata yourself** — no `gh api repos/…/pulls/comments/` calls to read `line`/`outdated`/`commit_id`, and no hand-run `git log` per comment: measured on a real 72-comment PR, a run burned 20+ model turns re-deriving exactly these fields one id at a time. Comment **bodies** are a different matter and stay where they were: the context file renders them (in full for blockers and review summaries), and only a body the renderer truncated is fetched, via the exact ref its `_(truncated — fetch …)_` note names. If `comment-status` itself fails (auth, network), warn and continue — it is an index, not the evidence: statuses become "re-derive if needed", and nothing here sets the context-unavailable state. + One call answers, per existing thread, every status question the re-check and the finder agents otherwise re-derive one API fetch at a time: is the anchor **outdated** at the live head (`line: null`), did the anchored **file change in the worktree since the comment's commit** and which commits touched it (`code.touchedBy` — the candidate "fixed by" commits), who replied and **did the PR author answer**, and whether the body **asserts a blocker** (same `carriesBlockerSignal` the context file's promotion uses). It also compares the worktree HEAD against the live PR head and warns on drift. **The report can exceed one `read_file`** — on a 71-thread PR it measured over twice the 25 000-character threshold, and because `threads` is path-sorted a truncated read drops the alphabetically-later files wholesale (24 blocker-flagged threads, in that measurement) while the cut JSON does not even parse. The command prints a `warning:` line naming the size when this happens; when it does, query the file with `jq` (it is machine-shaped) or page with `offset`/`limit` until `isTruncated` is false — same rule as the context file above. **Do not fetch per-comment status metadata yourself** — no `gh api repos/…/pulls/comments/` calls to read `line`/`outdated`/`commit_id`, and no hand-run `git log` per comment: measured on a real 72-comment PR, a run burned 20+ model turns re-deriving exactly these fields one id at a time. Comment **bodies** are a different matter and stay where they were: the context file renders them (in full for blockers and review summaries), and only a body the renderer truncated is fetched, via the exact ref its `_(truncated — fetch …)_` note names. If `comment-status` itself fails (auth, network), warn and continue — it is an index, not the evidence: statuses become "re-derive if needed", and nothing here sets the context-unavailable state. The context file does not prefetch linked issues. For bugfix PRs, instruct Step 3's Issue Fidelity agent to fetch issue evidence itself: @@ -620,7 +622,7 @@ If there are none of these, omit this section. ### Before an Approve or a zero-Critical verdict: re-check the open Criticals -A `C=0` outcome — Approve, or a Comment with no Critical — is a claim that nothing blocks the merge. It is not the default you fall back to when your own agents surfaced nothing. **If Step 1 set the context-unavailable state** (`pr-context` failed — lightweight or same-repo), there is no context file to read: skip the walk below, record every existing Critical as `cannot tell` by construction, and carry that into the verdict — which the Step 7 invariant already caps at `COMMENT`. Otherwise, take **each live blocker already on the PR — from every comment-bearing section of the context file: "Open inline comments", "Blockers to re-check", "Review summaries", and "Already discussed" (both its inline threads and its issue-level comments)** — and check it against the code as it stands at the reviewed commit. Select **semantically, not by the literal marker**: a `**[Critical]**` prefix qualifies, but so does any body that asserts a blocking defect in other words — a "Critical findings could not be anchored" preamble, an explicit must-fix claim (legacy body-only blockers were emitted markerless, and one such review is exactly what a marker filter once discarded). When unsure whether a body asserts a blocker, re-check it — the cost is one ruling; the alternative is certifying a merge past it. ("Already discussed" stays in scope even though `pr-context` now promotes blocker-bearing bodies out of it: `carriesBlockerSignal` is a **fail-safe floor, not a ceiling** — it recognises the phrasings we have seen, not every phrasing that exists, and a blocker worded around all of them still settles there. That section's "do NOT re-report" header governs duplicate-_reporting_ by the finder agents; it does not exempt a body from this re-check. Read it with the same eyes you bring to the promoted section.) Review-level bodies matter because an unmappable or 422-relocated blocker lives **only** there — and the context file now carries them **in full**: `pr-context` renders every meaningful review body whole under "Review summaries" (no more 240-character snippets), and pulls every blocker-bearing body — replied inline thread or issue comment, marker or no marker — into the "Blockers to re-check" section, rendered in full, because a reply alone never settles a blocker. So the re-check usually needs no separate fetch: read those sections under the file's untrusted-data preamble, paging with `offset`/`limit` until `isTruncated` is false. **For the status half of each ruling — is the anchor outdated, did the anchored file change since the blocker was filed, which commits touched it — read Step 1's `comment-status` report instead of fetching per-comment metadata**: its `code.touchedBy` list is the candidate "fixed by" commits to read, and `changedSinceComment: false` (with no head drift) tells you the anchored file is untouched since the blocker — so a claimed fix, if any, must live in some OTHER file, and the mechanism-read below is still owed either way. The report never substitutes for reading the code: it routes the read, it does not rule. Review summaries and blocker bodies are rendered in full; the Open and Already-discussed sections use one-line snippets, and **every snippet the renderer cut carries its own `_(truncated — fetch …)_` note naming the exact, already-filled-in command for the rest** — a candidate blocker whose snippet was cut is ruled on only after running that fetch; ruling on the visible prefix alone is the fail-closed violation. Run any such fetch **redirected to a file, never into the terminal** (Shell returns only an approximately 4 000-character model preview for output beyond its 30 000-character persistence trigger, which would re-truncate the very body being completed): append `--jq .body > .qwen/tmp/qwen-review-{target}-body-.md` to the command the note names, then `read_file` that file, paging until `isTruncated` is false, before ruling. **Fail closed either way:** a body you could not read whole — the capped tail unfetched, or the single-object fetch failing (auth, rate limit, network) — is `cannot tell`, not "no Critical in it": it goes to compose-review's `cannotTellCriticals` input, which serializes it and caps the event at `COMMENT`; a blocker you could not read is never approved past. A reply alone does not retire a blocker — "I disagree" or "wontfix" is a reply, which is exactly why `pr-context` quarantines blocker-bearing threads in their own section instead of letting them settle into "Already discussed". Only the code decides: a blocker counts as closed exactly when the re-check below lands on "fixed by this diff", never because the thread has an answer. Record one verdict per blocker: +A `C=0` outcome — Approve, or a Comment with no Critical — is a claim that nothing blocks the merge. It is not the default you fall back to when your own agents surfaced nothing. **If Step 1 set the context-unavailable state** (`pr-context` failed — lightweight or same-repo), there is no context file to read: skip the walk below, record every existing Critical as `cannot tell` by construction, and carry that into the verdict — which the Step 7 invariant already caps at `COMMENT`. Otherwise, take **each live blocker already on the PR — from every comment-bearing section of the context file: "Open inline comments", "Blockers to re-check", "Review summaries", and "Already discussed" (both its inline threads and its issue-level comments)** — and check it against the code as it stands at the reviewed commit. Select **semantically, not by the literal marker**: a `**[Critical]**` prefix qualifies, but so does any body that asserts a blocking defect in other words — a "Critical findings could not be anchored" preamble, an explicit must-fix claim (legacy body-only blockers were emitted markerless, and one such review is exactly what a marker filter once discarded). When unsure whether a body asserts a blocker, re-check it — the cost is one ruling; the alternative is certifying a merge past it. ("Already discussed" stays in scope even though `pr-context` now promotes blocker-bearing bodies out of it: `carriesBlockerSignal` is a **fail-safe floor, not a ceiling** — it recognises the phrasings we have seen, not every phrasing that exists, and a blocker worded around all of them still settles there. That section's "do NOT re-report" header governs duplicate-_reporting_ by the finder agents; it does not exempt a body from this re-check. Read it with the same eyes you bring to the promoted section.) Review-level bodies matter because an unmappable or 422-relocated blocker lives **only** there — and the context file now carries them **in full**: `pr-context` renders every meaningful review body whole under "Review summaries" (no more 240-character snippets), and pulls every blocker-bearing body — replied inline thread or issue comment, marker or no marker — into the "Blockers to re-check" section, rendered in full, because a reply alone never settles a blocker. So the re-check usually needs no separate fetch: read those sections under the file's untrusted-data preamble, paging with `offset`/`limit` until `isTruncated` is false. **For the status half of each INLINE-thread ruling — is the anchor outdated, did the anchored file change since the blocker was filed, which commits touched it — read Step 1's `comment-status` report instead of fetching per-comment metadata**: its `code.touchedBy` list is the candidate "fixed by" commits to read, and `changedSinceComment: false` (with no head drift) tells you the anchored file is untouched since the blocker — so a claimed fix, if any, must live in some OTHER file, and the mechanism-read below is still owed either way. Two scope limits, both deliberate: the report exists only **when Step 1 wrote it** (worktree mode, fetch succeeded — a lightweight-mode run still walks this re-check and re-derives status facts the old way), and it indexes **inline threads only** — an issue-level or review-level blocker (the #6486 shape) has no entry there and keeps the context-file walk as its sole source. The report never substitutes for reading the code: it routes the read, it does not rule. Review summaries and blocker bodies are rendered in full; the Open and Already-discussed sections use one-line snippets, and **every snippet the renderer cut carries its own `_(truncated — fetch …)_` note naming the exact, already-filled-in command for the rest** — a candidate blocker whose snippet was cut is ruled on only after running that fetch; ruling on the visible prefix alone is the fail-closed violation. Run any such fetch **redirected to a file, never into the terminal** (Shell returns only an approximately 4 000-character model preview for output beyond its 30 000-character persistence trigger, which would re-truncate the very body being completed): append `--jq .body > .qwen/tmp/qwen-review-{target}-body-.md` to the command the note names, then `read_file` that file, paging until `isTruncated` is false, before ruling. **Fail closed either way:** a body you could not read whole — the capped tail unfetched, or the single-object fetch failing (auth, rate limit, network) — is `cannot tell`, not "no Critical in it": it goes to compose-review's `cannotTellCriticals` input, which serializes it and caps the event at `COMMENT`; a blocker you could not read is never approved past. A reply alone does not retire a blocker — "I disagree" or "wontfix" is a reply, which is exactly why `pr-context` quarantines blocker-bearing threads in their own section instead of letting them settle into "Already discussed". Only the code decides: a blocker counts as closed exactly when the re-check below lands on "fixed by this diff", never because the thread has an answer. Record one verdict per blocker: - **still stands** — the defect is present in the code you just read. It blocks: the event is `REQUEST_CHANGES`, and the finding goes inline (or into the body if it cannot be anchored). - **fixed by this diff** — you traced the blocker's **mechanism** through the code as it now stands and it can no longer fire. Say nothing; do not re-report it. A GitHub thread can read `isResolved: false, isOutdated: false` for a bug a later commit fixed on an adjacent line — the flag tracks the anchored line, not the fix, so the flag is not evidence either way. Only the code is. From e65860dc43224cb04bf0e50d40c6a6bf73a00fd3 Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 03:56:38 +0800 Subject: [PATCH 04/12] =?UTF-8?q?fix(review):=20comment-status=20second-ro?= =?UTF-8?q?und=20follow-ups=20=E2=80=94=20CWD-safe=20pathspec,=20shared=20?= =?UTF-8?q?walk,=20stale=20flag?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the LGTM-with-suggestions round: - The git probe's pathspec is anchored with `:(top)`: run from a subdirectory of the worktree, the old CWD-relative form returned empty output with exit 0 and every thread read as "untouched since the comment" — silent, and pointing the one direction this index must not fail in. A real-git integration test now drives the probe from the repo root AND a subdirectory (plus the cap, the memo, and the missing-commit gate — none of which the injected-probe unit tests could see). - findRootId is imported from pr-context (made generic and exported) instead of duplicated — the thread walk now agrees by construction, like the blocker signal already did. - Head drift is denormalized onto every thread as code.staleWorktree, so a jq consumer of threads[] cannot skip the top-level flag by construction. - Commit existence is memoized per SHA (it never depended on the path), halving git spawns on thread-heavy PRs; summarizeThreads gets a named return interface like every other exported shape here. - DESIGN.md gains the missing section: why comment-status is a separate subcommand and why the second fetch of pulls/{n}/comments is deliberate (process boundary — pr-context must stay pure-API for lightweight mode; this one exists to join API facts with worktree git). --- .../review/comment-status.integration.test.ts | 113 ++++++++++++++++++ .../commands/review/comment-status.test.ts | 1 + .../cli/src/commands/review/comment-status.ts | 71 +++++++---- .../cli/src/commands/review/pr-context.ts | 9 +- .../core/src/skills/bundled/review/DESIGN.md | 19 +++ 5 files changed, 189 insertions(+), 24 deletions(-) create mode 100644 packages/cli/src/commands/review/comment-status.integration.test.ts diff --git a/packages/cli/src/commands/review/comment-status.integration.test.ts b/packages/cli/src/commands/review/comment-status.integration.test.ts new file mode 100644 index 00000000000..9085e769a6c --- /dev/null +++ b/packages/cli/src/commands/review/comment-status.integration.test.ts @@ -0,0 +1,113 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Real `git`. The pure core is covered by comment-status.test.ts with an +// injected probe; this file covers the probe itself — the memo, the +// missing-commit gate, the cap, and above all the CWD assumption: the probe +// once resolved its pathspec against the CURRENT directory, so running from a +// subdirectory of the worktree returned empty output with exit 0 and every +// thread read as "untouched since the comment". A mocked child_process would +// have passed that bug happily. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { makeGitProbe } from './comment-status.js'; + +let repo: string; +let savedCwd: string; + +function git(...args: string[]): string { + return execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); +} + +function commitFile(path: string, content: string, message: string): string { + writeFileSync(join(repo, path), content); + git('add', path); + git( + '-c', + 'user.email=t@example.com', + '-c', + 'user.name=T', + 'commit', + '-qm', + message, + ); + return git('rev-parse', 'HEAD'); +} + +beforeEach(() => { + repo = mkdtempSync(join(tmpdir(), 'comment-status-probe-')); + savedCwd = process.cwd(); + execFileSync('git', ['init', '-q', repo]); + mkdirSync(join(repo, 'pkg', 'src'), { recursive: true }); +}); + +afterEach(() => { + process.chdir(savedCwd); + rmSync(repo, { recursive: true, force: true }); +}); + +describe('makeGitProbe (real git)', () => { + it('reports the touching commits from the repo root', () => { + const base = commitFile('pkg/src/a.ts', 'v1\n', 'base'); + const fix = commitFile('pkg/src/a.ts', 'v2\n', 'fix'); + + process.chdir(repo); + const got = makeGitProbe()('pkg/src/a.ts', base); + expect(got.changed).toBe(true); + expect(got.touchedByTotal).toBe(1); + expect(fix.startsWith(got.touchedBy[0])).toBe(true); + }); + + it('resolves the repo-relative path even when run from a subdirectory', () => { + // The regression: without `:(top)` the pathspec resolves against the + // subdirectory, git log returns nothing with exit 0, and the report + // claims the file is untouched. + const base = commitFile('pkg/src/a.ts', 'v1\n', 'base'); + commitFile('pkg/src/a.ts', 'v2\n', 'fix'); + + process.chdir(join(repo, 'pkg', 'src')); + const got = makeGitProbe()('pkg/src/a.ts', base); + expect(got.changed).toBe(true); + expect(got.touchedByTotal).toBe(1); + }); + + it('caps touchedBy at 10 while reporting the real total', () => { + const base = commitFile('pkg/src/a.ts', 'v0\n', 'base'); + for (let i = 1; i <= 12; i++) { + commitFile('pkg/src/a.ts', `v${i}\n`, `edit ${i}`); + } + + process.chdir(repo); + const got = makeGitProbe()('pkg/src/a.ts', base); + expect(got.touchedBy).toHaveLength(10); + expect(got.touchedByTotal).toBe(12); + }); + + it('degrades to unknown for a commit absent from the object store', () => { + commitFile('pkg/src/a.ts', 'v1\n', 'base'); + process.chdir(repo); + const got = makeGitProbe()( + 'pkg/src/a.ts', + 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef', + ); + expect(got.changed).toBe('unknown'); + expect(got.touchedByTotal).toBe(0); + }); + + it('reports unchanged for a file the range never touched', () => { + const base = commitFile('pkg/src/a.ts', 'v1\n', 'base'); + commitFile('pkg/src/b.ts', 'other\n', 'unrelated'); + + process.chdir(repo); + const got = makeGitProbe()('pkg/src/a.ts', base); + expect(got.changed).toBe(false); + expect(got.touchedBy).toEqual([]); + }); +}); diff --git a/packages/cli/src/commands/review/comment-status.test.ts b/packages/cli/src/commands/review/comment-status.test.ts index ad09f3deb34..507bdf7b619 100644 --- a/packages/cli/src/commands/review/comment-status.test.ts +++ b/packages/cli/src/commands/review/comment-status.test.ts @@ -150,6 +150,7 @@ describe('buildThreadStatuses — signals', () => { changedSinceComment: true, touchedBy: ['abc1234', 'def5678'], touchedByTotal: 12, + staleWorktree: false, }); }); diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index 31903038771..6e002646ab6 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -27,7 +27,7 @@ import { DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD } from '@qwen-code/qwen-code-cor import { writeStdoutLine } from '../../utils/stdioHelpers.js'; import { ensureAuthenticated, gh, ghApiAll, setGhHost } from './lib/gh.js'; import { gitOpt } from './lib/git.js'; -import { carriesBlockerSignal } from './pr-context.js'; +import { carriesBlockerSignal, findRootId } from './pr-context.js'; /** Inline review comment, as listed by `GET /pulls/{n}/comments`. */ export interface RawStatusComment { @@ -100,6 +100,11 @@ export interface ThreadStatus { /** Real count before the cap — when it exceeds `touchedBy.length`, the * list was cut and a fix commit may be among the ones not shown. */ touchedByTotal: number; + /** True when the worktree HEAD no longer matches the live PR head: the + * code facts above describe a superseded checkout. Denormalized onto + * every thread so a `jq` filter over `threads[]` cannot skip the + * top-level drift flag by construction. */ + staleWorktree: boolean; }; replies: Array<{ id: number; author: string; createdAt: string }>; /** The PR author replied somewhere in this thread — the strongest cheap @@ -110,24 +115,6 @@ export interface ThreadStatus { const TOUCHED_BY_CAP = 10; -/** Cycle-safe walk to a reply chain's root (mirrors pr-context's walk). */ -function findRootId( - startId: number, - byId: Map, -): number { - const seen = new Set(); - let cur = startId; - while (true) { - if (seen.has(cur)) return cur; - seen.add(cur); - const c = byId.get(cur); - if (!c || c.in_reply_to_id === undefined || c.in_reply_to_id === null) { - return cur; - } - cur = c.in_reply_to_id; - } -} - /** * Pure classification core: group the flat comment list into threads and * compute each thread's status. Replies whose root was deleted (the id no @@ -185,6 +172,7 @@ export function buildThreadStatuses( changedSinceComment: probed.changed, touchedBy: probed.touchedBy, touchedByTotal: probed.touchedByTotal, + staleWorktree: false, }, replies: replies.map((r) => ({ id: r.id, @@ -210,7 +198,17 @@ export function buildThreadStatuses( return threads; } -export function summarizeThreads(threads: ThreadStatus[]) { +export interface ThreadSummary { + threads: number; + outdated: number; + blockers: number; + changedSinceComment: number; + changeUnknown: number; + withReplies: number; + authorReplied: number; +} + +export function summarizeThreads(threads: ThreadStatus[]): ThreadSummary { return { threads: threads.length, outdated: threads.filter((t) => t.anchor.outdated).length, @@ -231,9 +229,13 @@ export function summarizeThreads(threads: ThreadStatus[]) { * this inside the PR worktree). Memoized per (path, sinceSha): several * threads routinely anchor to the same file at the same commit. */ -function makeGitProbe(): CodeChangeProbe { +export function makeGitProbe(): CodeChangeProbe { const inRepo = gitOpt('rev-parse', '--is-inside-work-tree') === 'true'; const memo = new Map>(); + // Commit existence depends on the SHA alone; memoizing it separately from + // the (path, sha) results roughly halves the git spawns on a thread-heavy + // PR where many threads share one anchor commit. + const commitExists = new Map(); return (path, sinceSha) => { if (!inRepo || !sinceSha || !path) { return { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; @@ -246,10 +248,27 @@ function makeGitProbe(): CodeChangeProbe { // range is unanswerable. (When a force-pushed-away commit is still // present via the shared object database, the range widens instead; // see the CodeChangeProbe doc for why that direction is accepted.) - if (gitOpt('cat-file', '-e', `${sinceSha}^{commit}`) === null) { + let exists = commitExists.get(sinceSha); + if (exists === undefined) { + exists = gitOpt('cat-file', '-e', `${sinceSha}^{commit}`) !== null; + commitExists.set(sinceSha, exists); + } + if (!exists) { result = { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; } else { - const log = gitOpt('log', '--format=%h', `${sinceSha}..HEAD`, '--', path); + // `:(top)` anchors the pathspec at the repository root regardless of + // the CWD inside the worktree. Without it, running from a subdirectory + // silently resolves the repo-relative comment path against the + // subdirectory — empty output, exit 0, and every thread reads as + // "untouched since the comment": the one direction this index must + // not fail in. + const log = gitOpt( + 'log', + '--format=%h', + `${sinceSha}..HEAD`, + '--', + `:(top)${path}`, + ); if (log === null) { result = { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; } else { @@ -311,6 +330,12 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { worktreeHeadSha !== liveHeadSha; const threads = buildThreadStatuses(comments, prAuthor, makeGitProbe()); + if (headDrift) { + // Denormalize the drift onto every thread: the code facts describe a + // superseded checkout, and a jq consumer of threads[] must not need to + // remember a top-level flag to see that. + for (const t of threads) t.code.staleWorktree = true; + } const summary = summarizeThreads(threads); const report = { diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index 5fcccd5c796..42a7a798e06 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -388,8 +388,15 @@ function quoteBlock(s: string): string { /** * Walk a comment's `in_reply_to_id` chain up to the root. Defends against * cycles (which shouldn't happen on GitHub but cheap to handle). + * + * Exported and generic: `comment-status` groups the same flat comment list + * into the same threads, and a shared walk is what keeps the two surfaces + * agreeing by construction — a cycle-guard fix applied to one private copy + * and not the other would silently diverge their thread classification. */ -function findRootId(startId: number, byId: Map): number { +export function findRootId< + T extends { id: number; in_reply_to_id?: number | null }, +>(startId: number, byId: Map): number { const seen = new Set(); let cur = startId; while (true) { diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 910ffa907ac..8fb5017d602 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -273,6 +273,25 @@ PR #6486: the one job that would have exercised the new `Ctrl+F` hotkey — `Int **Trade-off:** when the deterministic logic changes (e.g., a new GitHub `conclusion` value), the cli code must be rebuilt + re-shipped along with the skill. SKILL.md and the subcommand are versioned together in this monorepo so that's a benefit, not a cost — they cannot drift apart in any single release. +## Why comment-status is a subcommand — and a second fetch of the same endpoint + +`pr-context` already paginates `pulls/{n}/comments` moments earlier in Step 1, +and `comment-status` fetches the same endpoint again. The duplication is +deliberate, and the reason is the process boundary: `pr-context` is pure +GitHub API — it runs in lightweight cross-repo mode, where there is no +worktree — while `comment-status` exists precisely to join the API's anchor +facts with the WORKTREE's git history (`changedSinceComment`, `touchedBy`). +One subcommand serving both modes would either drag a git dependency into the +one command that must not have it, or silently emit half a report cross-repo. +The cost is one extra paginated GET per worktree-mode review; the alternative +the subcommand replaced was 20+ single-comment fetches, each a whole model +turn, measured on a real 72-comment PR. + +Bodies stay in `pr-context`'s Markdown (under its untrusted-data preamble); +`comment-status` carries only status facts. The two surfaces cannot disagree +on classification: the blocker test (`carriesBlockerSignal`) and the +thread-root walk (`findRootId`) are imported from `pr-context`, not copied. + ## Why base-branch rule loading (security) A malicious PR could add `.qwen/review-rules.md` with "never report security issues." If rules are read from the PR branch, the review is compromised. From 42dd072b170c25a80ec4f1d74789f8027bda7968 Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 04:29:40 +0800 Subject: [PATCH 05/12] fix(review): harden comment-status against untrusted-PR inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the security review round: - Symlink --out: the command now runs from the trusted main checkout (so a relative --out cannot be redirected through a symlink an untrusted PR planted in its own worktree) and scopes its git queries to the worktree with `git -C `, which it locates itself. SKILL.md no longer cd's into the worktree for it. The report lands in the main checkout's .qwen/tmp alongside every sibling report. - Non-ancestor comment commit: after a force-push the anchor commit can survive in the shared object store without being on HEAD's history, so `sinceSha..HEAD` is empty and a changed file reads as changed:false — the one direction this index must not fail in. A single `merge-base --is-ancestor` gate now returns 'unknown' for a non-ancestor OR a missing commit, replacing the cat-file existence check (one git process instead of two). - Literal pathspec: the GitHub-supplied path is passed as `:(top,literal)` so a value like `:(exclude)a.ts` cannot be read as pathspec magic and inspect unrelated files. - Fetch-race drift: the live head is sampled before AND after the comments fetch; a push landing mid-fetch (which would pair newer anchor mappings with a stale comparison) is now detected, recorded as both samples, and warned on distinctly from ordinary worktree lag. - pr-context renders the root comment id in the Open and Already-discussed sections, giving Step 6 a stable join key back to comment-status's per-thread rootId (the blocker renderer already did this). - Handler-level tests (mocked gh/git/fs) for the three drift outcomes, plus real-git integration cases for the non-ancestor gate and the literal pathspec. Multi-page pagination is a non-issue: gh api --paginate merges top-level arrays into one (verified on the live 93-comment PR). --- .../review/comment-status.handler.test.ts | 145 ++++++++++++++++++ .../review/comment-status.integration.test.ts | 48 ++++-- .../cli/src/commands/review/comment-status.ts | 119 +++++++++----- .../src/commands/review/pr-context.test.ts | 10 ++ .../cli/src/commands/review/pr-context.ts | 4 +- .../core/src/skills/bundled/review/SKILL.md | 2 +- 6 files changed, 275 insertions(+), 53 deletions(-) create mode 100644 packages/cli/src/commands/review/comment-status.handler.test.ts diff --git a/packages/cli/src/commands/review/comment-status.handler.test.ts b/packages/cli/src/commands/review/comment-status.handler.test.ts new file mode 100644 index 00000000000..3ba4b71c308 --- /dev/null +++ b/packages/cli/src/commands/review/comment-status.handler.test.ts @@ -0,0 +1,145 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Command-level tests: the pure core (comment-status.test.ts) and the real +// git probe (comment-status.integration.test.ts) are covered elsewhere; this +// file pins the handler wiring — the two head samples, the drift field and +// its warning, and that the report is written — with gh/git/fs mocked. A +// regression that reversed the drift comparison or dropped the warning would +// otherwise leave every other test green while live anchors were silently +// paired with stale worktree facts. + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + gh: vi.fn(), + ghApiAll: vi.fn((_p: string): unknown[] => []), + ensureAuthenticated: vi.fn(), + setGhHost: vi.fn(), + gitOpt: vi.fn((..._a: string[]): string | null => null), + writeFileSync: vi.fn(), + mkdirSync: vi.fn(), + writeStdoutLine: vi.fn(), +})); + +vi.mock('./lib/gh.js', () => ({ + gh: mocks.gh, + ghApiAll: mocks.ghApiAll, + ensureAuthenticated: mocks.ensureAuthenticated, + setGhHost: mocks.setGhHost, +})); + +vi.mock('./lib/git.js', () => ({ + gitOpt: mocks.gitOpt, +})); + +vi.mock('./lib/paths.js', () => ({ + worktreePath: (n: string | number) => `/repo/.qwen/tmp/review-pr-${n}`, +})); + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + default: { + ...actual, + writeFileSync: mocks.writeFileSync, + mkdirSync: mocks.mkdirSync, + }, + writeFileSync: mocks.writeFileSync, + mkdirSync: mocks.mkdirSync, + }; +}); + +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mocks.writeStdoutLine, +})); + +const { commentStatusCommand } = await import('./comment-status.js'); + +async function run() { + const handler = commentStatusCommand.handler; + if (!handler) throw new Error('handler missing'); + await handler({ + _: [], + $0: 'qwen', + pr_number: '7632', + owner_repo: 'o/r', + out: '/repo/.qwen/tmp/qwen-review-pr-7632-comment-status.json', + } as unknown as Parameters[0]); +} + +function reportWritten() { + const call = mocks.writeFileSync.mock.calls.find(([p]) => + String(p).endsWith('comment-status.json'), + ); + if (!call) throw new Error('report not written'); + return JSON.parse(String(call[1])); +} + +function warnings() { + return mocks.writeStdoutLine.mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('warning:')); +} + +// gh('pr','view',…) is called for author+head, then again for the second +// head sample. This drives both from one queue. +function queueHeads(before: string, after: string, author = 'octocat') { + let n = 0; + mocks.gh.mockImplementation((..._args: string[]) => { + n += 1; + return n === 1 + ? JSON.stringify({ author: { login: author }, headRefOid: before }) + : JSON.stringify({ headRefOid: after }); + }); +} + +describe('comment-status handler', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.ghApiAll.mockReturnValue([]); + // Worktree HEAD matches the (stable) live head by default: no drift. + mocks.gitOpt.mockImplementation((...args: string[]) => + args.includes('rev-parse') ? 'headA' : null, + ); + }); + + it('reports no drift and writes the report when heads all agree', async () => { + queueHeads('headA', 'headA'); + await run(); + const report = reportWritten(); + expect(report.headDrift).toBe(false); + expect(report.headMovedDuringFetch).toBe(false); + expect(report.liveHeadSha).toBe('headA'); + expect(warnings()).toEqual([]); + }); + + it('flags drift and warns when the worktree lags the live head', async () => { + queueHeads('headB', 'headB'); // live head B, worktree still at headA + await run(); + const report = reportWritten(); + expect(report.headDrift).toBe(true); + expect(report.headMovedDuringFetch).toBe(false); + expect(warnings().join('\n')).toContain('worktree HEAD'); + }); + + it('flags a push that raced the comments fetch, recording both samples', async () => { + // Worktree happens to match the post-fetch head, but the head MOVED + // during the fetch — anchor facts may be mixed across commits. + mocks.gitOpt.mockImplementation((...args: string[]) => + args.includes('rev-parse') ? 'headB' : null, + ); + queueHeads('headA', 'headB'); + await run(); + const report = reportWritten(); + expect(report.headMovedDuringFetch).toBe(true); + expect(report.headDrift).toBe(true); + expect(report.liveHeadBefore).toBe('headA'); + expect(report.liveHeadSha).toBe('headB'); + expect(warnings().join('\n')).toContain('moved during the comments fetch'); + }); +}); diff --git a/packages/cli/src/commands/review/comment-status.integration.test.ts b/packages/cli/src/commands/review/comment-status.integration.test.ts index 9085e769a6c..8bef1afb55e 100644 --- a/packages/cli/src/commands/review/comment-status.integration.test.ts +++ b/packages/cli/src/commands/review/comment-status.integration.test.ts @@ -58,22 +58,22 @@ describe('makeGitProbe (real git)', () => { const base = commitFile('pkg/src/a.ts', 'v1\n', 'base'); const fix = commitFile('pkg/src/a.ts', 'v2\n', 'fix'); - process.chdir(repo); - const got = makeGitProbe()('pkg/src/a.ts', base); + const got = makeGitProbe(repo)('pkg/src/a.ts', base); expect(got.changed).toBe(true); expect(got.touchedByTotal).toBe(1); expect(fix.startsWith(got.touchedBy[0])).toBe(true); }); - it('resolves the repo-relative path even when run from a subdirectory', () => { - // The regression: without `:(top)` the pathspec resolves against the - // subdirectory, git log returns nothing with exit 0, and the report - // claims the file is untouched. + it('scopes git to the worktree via -C regardless of the process CWD', () => { + // The command runs from the trusted main checkout; the probe must still + // read the worktree. Driving from an unrelated CWD proves it is `-C + // `, not the ambient directory, that git sees. (Also the + // regression guard for the old CWD-relative pathspec.) const base = commitFile('pkg/src/a.ts', 'v1\n', 'base'); commitFile('pkg/src/a.ts', 'v2\n', 'fix'); - process.chdir(join(repo, 'pkg', 'src')); - const got = makeGitProbe()('pkg/src/a.ts', base); + process.chdir(tmpdir()); + const got = makeGitProbe(repo)('pkg/src/a.ts', base); expect(got.changed).toBe(true); expect(got.touchedByTotal).toBe(1); }); @@ -84,16 +84,14 @@ describe('makeGitProbe (real git)', () => { commitFile('pkg/src/a.ts', `v${i}\n`, `edit ${i}`); } - process.chdir(repo); - const got = makeGitProbe()('pkg/src/a.ts', base); + const got = makeGitProbe(repo)('pkg/src/a.ts', base); expect(got.touchedBy).toHaveLength(10); expect(got.touchedByTotal).toBe(12); }); it('degrades to unknown for a commit absent from the object store', () => { commitFile('pkg/src/a.ts', 'v1\n', 'base'); - process.chdir(repo); - const got = makeGitProbe()( + const got = makeGitProbe(repo)( 'pkg/src/a.ts', 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef', ); @@ -101,12 +99,34 @@ describe('makeGitProbe (real git)', () => { expect(got.touchedByTotal).toBe(0); }); + it('degrades to unknown for a non-ancestor (force-pushed-away) commit that still resolves', () => { + // A change lives on an orphaned branch that is a real commit but not an + // ancestor of HEAD. `sinceSha..HEAD` would be empty and wrongly read as + // "unchanged"; the ancestry gate must return unknown instead. + commitFile('pkg/src/a.ts', 'v1\n', 'base'); + git('checkout', '-q', '-b', 'orphan'); + const orphan = commitFile('pkg/src/a.ts', 'ORPHAN CHANGE\n', 'orphan edit'); + git('checkout', '-q', '-'); + commitFile('pkg/src/b.ts', 'unrelated\n', 'move head forward'); + + const got = makeGitProbe(repo)('pkg/src/a.ts', orphan); + expect(got.changed).toBe('unknown'); + }); + + it('takes the comment path literally, not as pathspec magic', () => { + const base = commitFile('pkg/src/a.ts', 'v1\n', 'base'); + commitFile('pkg/src/a.ts', 'v2\n', 'fix'); + // `:(exclude)…` would flip the meaning if magic were honored; literal + // means "a file actually named this", which does not exist → unchanged. + const got = makeGitProbe(repo)(':(exclude)pkg/src/a.ts', base); + expect(got.changed).toBe(false); + }); + it('reports unchanged for a file the range never touched', () => { const base = commitFile('pkg/src/a.ts', 'v1\n', 'base'); commitFile('pkg/src/b.ts', 'other\n', 'unrelated'); - process.chdir(repo); - const got = makeGitProbe()('pkg/src/a.ts', base); + const got = makeGitProbe(repo)('pkg/src/a.ts', base); expect(got.changed).toBe(false); expect(got.touchedBy).toEqual([]); }); diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index 6e002646ab6..2a7637a643a 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -27,6 +27,7 @@ import { DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD } from '@qwen-code/qwen-code-cor import { writeStdoutLine } from '../../utils/stdioHelpers.js'; import { ensureAuthenticated, gh, ghApiAll, setGhHost } from './lib/gh.js'; import { gitOpt } from './lib/git.js'; +import { worktreePath } from './lib/paths.js'; import { carriesBlockerSignal, findRootId } from './pr-context.js'; /** Inline review comment, as listed by `GET /pulls/{n}/comments`. */ @@ -225,17 +226,21 @@ export function summarizeThreads(threads: ThreadStatus[]): ThreadSummary { } /** - * Git-backed probe against the current working directory (the skill runs - * this inside the PR worktree). Memoized per (path, sinceSha): several - * threads routinely anchor to the same file at the same commit. + * Git-backed probe. Every call is scoped to `worktree` with `git -C`, NOT to + * the process CWD: the command runs from the trusted main checkout (so its + * `--out` cannot be redirected through a symlink an untrusted PR planted in + * its own tree), while the code facts must come from the PR's checked-out + * worktree. Memoized per (path, sinceSha): several threads routinely anchor + * to the same file at the same commit. */ -export function makeGitProbe(): CodeChangeProbe { - const inRepo = gitOpt('rev-parse', '--is-inside-work-tree') === 'true'; +export function makeGitProbe(worktree: string): CodeChangeProbe { + const inRepo = + gitOpt('-C', worktree, 'rev-parse', '--is-inside-work-tree') === 'true'; const memo = new Map>(); - // Commit existence depends on the SHA alone; memoizing it separately from - // the (path, sha) results roughly halves the git spawns on a thread-heavy - // PR where many threads share one anchor commit. - const commitExists = new Map(); + // Ancestry depends on the SHA alone (HEAD is fixed for the run); memoizing + // it apart from the (path, sha) results roughly halves the git spawns on a + // thread-heavy PR where many threads share one anchor commit. + const isAncestor = new Map(); return (path, sinceSha) => { if (!inRepo || !sinceSha || !path) { return { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; @@ -244,30 +249,43 @@ export function makeGitProbe(): CodeChangeProbe { const hit = memo.get(key); if (hit) return hit; let result: ReturnType; - // The comment's commit can be absent from the object store — then the - // range is unanswerable. (When a force-pushed-away commit is still - // present via the shared object database, the range widens instead; - // see the CodeChangeProbe doc for why that direction is accepted.) - let exists = commitExists.get(sinceSha); - if (exists === undefined) { - exists = gitOpt('cat-file', '-e', `${sinceSha}^{commit}`) !== null; - commitExists.set(sinceSha, exists); + // `sinceSha..HEAD` is only meaningful when sinceSha is an ANCESTOR of the + // worktree HEAD. After a force-push the comment's commit can survive in + // the shared object database without being on the current history: the + // range is then empty even though the trees differ, which would report a + // changed file as `changed: false` — the one direction this index must + // never fail in. `merge-base --is-ancestor` is exit 0 for an ancestor, + // non-zero for a non-ancestor OR a missing commit (both → `null` here), + // so this one call replaces the old existence check and closes that hole. + let ancestor = isAncestor.get(sinceSha); + if (ancestor === undefined) { + ancestor = + gitOpt( + '-C', + worktree, + 'merge-base', + '--is-ancestor', + sinceSha, + 'HEAD', + ) !== null; + isAncestor.set(sinceSha, ancestor); } - if (!exists) { + if (!ancestor) { result = { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; } else { - // `:(top)` anchors the pathspec at the repository root regardless of - // the CWD inside the worktree. Without it, running from a subdirectory - // silently resolves the repo-relative comment path against the - // subdirectory — empty output, exit 0, and every thread reads as - // "untouched since the comment": the one direction this index must - // not fail in. + // `:(top,literal)` anchors the pathspec at the repo root (so a subdir + // CWD cannot silently mis-scope it) AND takes `path` literally — a + // GitHub-supplied path is untrusted, and a value like `:(exclude)a.ts` + // or `:(glob)**` would otherwise be read as pathspec magic and inspect + // unrelated files. const log = gitOpt( + '-C', + worktree, 'log', '--format=%h', `${sinceSha}..HEAD`, '--', - `:(top)${path}`, + `:(top,literal)${path}`, ); if (log === null) { result = { changed: 'unknown', touchedBy: [], touchedByTotal: 0 }; @@ -300,6 +318,11 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { ensureAuthenticated(); + // Sample the live head BEFORE the comments fetch and AGAIN after: GitHub + // maps comment lines against whatever head is current when the list is + // served, so a push landing mid-fetch would pair anchor facts from a newer + // head with a stale drift comparison. If the two samples disagree the PR is + // moving right now — treat it as drift. const meta = JSON.parse( gh( 'pr', @@ -312,24 +335,41 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { ), ) as { author?: { login?: string } | null; headRefOid?: string }; const prAuthor = meta.author?.login ?? ''; - const liveHeadSha = meta.headRefOid ?? ''; + const liveHeadBefore = meta.headRefOid ?? ''; const comments = ghApiAll( `repos/${owner}/${repo}/pulls/${prNumber}/comments`, ) as RawStatusComment[]; - const worktreeHeadSha = gitOpt('rev-parse', 'HEAD'); + const liveHeadAfter = + ( + JSON.parse( + gh('pr', 'view', prNumber, '--repo', ownerRepo, '--json', 'headRefOid'), + ) as { headRefOid?: string } + ).headRefOid ?? ''; + + const worktree = worktreePath(prNumber); + const worktreeHeadSha = gitOpt('-C', worktree, 'rev-parse', 'HEAD'); // Anchor facts (`line`, outdated) describe the LIVE head — GitHub maps // comments against the latest diff it serves. Code facts (`touchedBy`, // changedSinceComment) describe the WORKTREE head — the code this review - // rules on. When the two differ the PR advanced mid-review; surface it - // rather than letting the two halves silently describe different commits. - const headDrift = + // rules on. Drift when the worktree lags the live head, OR when the head + // moved between the two samples (a push raced the fetch). + const headMovedDuringFetch = + liveHeadBefore !== '' && + liveHeadAfter !== '' && + liveHeadBefore !== liveHeadAfter; + const worktreeStale = worktreeHeadSha !== null && - liveHeadSha !== '' && - worktreeHeadSha !== liveHeadSha; + liveHeadAfter !== '' && + worktreeHeadSha !== liveHeadAfter; + const headDrift = headMovedDuringFetch || worktreeStale; - const threads = buildThreadStatuses(comments, prAuthor, makeGitProbe()); + const threads = buildThreadStatuses( + comments, + prAuthor, + makeGitProbe(worktree), + ); if (headDrift) { // Denormalize the drift onto every thread: the code facts describe a // superseded checkout, and a jq consumer of threads[] must not need to @@ -342,9 +382,11 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { prNumber, ownerRepo, prAuthor, - liveHeadSha, + liveHeadSha: liveHeadAfter, + liveHeadBefore, worktreeHeadSha, headDrift, + headMovedDuringFetch, inlineComments: comments.length, summary, threads, @@ -359,9 +401,14 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { `${summary.changedSinceComment} on files changed since their comment, ` + `${summary.withReplies} with replies, ${summary.authorReplied} answered by the PR author)`, ); - if (headDrift) { + if (headMovedDuringFetch) { + writeStdoutLine( + `warning: PR head moved during the comments fetch (${liveHeadBefore.slice(0, 8)} → ${liveHeadAfter.slice(0, 8)}) — ` + + `anchor facts may be mixed across commits. Re-run after the push settles.`, + ); + } else if (worktreeStale) { writeStdoutLine( - `warning: worktree HEAD ${worktreeHeadSha!.slice(0, 8)} != live PR head ${liveHeadSha.slice(0, 8)} — ` + + `warning: worktree HEAD ${(worktreeHeadSha ?? '').slice(0, 8)} != live PR head ${liveHeadAfter.slice(0, 8)} — ` + `the PR advanced since fetch-pr. Anchor facts describe the live head; ` + `code facts describe the worktree.`, ); diff --git a/packages/cli/src/commands/review/pr-context.test.ts b/packages/cli/src/commands/review/pr-context.test.ts index 2393a5ef170..a60ff76bd91 100644 --- a/packages/cli/src/commands/review/pr-context.test.ts +++ b/packages/cli/src/commands/review/pr-context.test.ts @@ -145,6 +145,16 @@ describe('buildMarkdown section order', () => { expect(md).not.toContain('## Open inline comments'); expect(md).toContain('## Already discussed'); }); + + it('renders the root comment id in both open and already-discussed entries', () => { + // The id is the stable join key back to comment-status's per-thread + // rootId; two short roots at the same path:line by the same author are + // otherwise indistinguishable. `root` heads a discussed thread, `open` + // is an un-replied root. + const md = buildMarkdown('1', 'o/r', meta, [root, reply, open], [], []); + expect(md).toContain(`(comment ${open.id})`); + expect(md).toContain(`(comment ${root.id})`); + }); }); describe('fullBody', () => { diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index 42a7a798e06..51dff600739 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -748,7 +748,7 @@ export function buildMarkdown( parts.push(''); for (const c of openRoots) { parts.push( - `- \`${c.path ?? '?'}\`:${c.line ?? '?'} by @${c.user?.login ?? '?'}: ${snippetWithRef(c.body, 240, pullCommentRef(c.id, ctx))}`, + `- \`${c.path ?? '?'}\`:${c.line ?? '?'} by @${c.user?.login ?? '?'} (comment ${c.id}): ${snippetWithRef(c.body, 240, pullCommentRef(c.id, ctx))}`, ); } parts.push(''); @@ -776,7 +776,7 @@ export function buildMarkdown( for (const root of sortedRoots) { const replies = repliesByRoot.get(root.id) ?? []; parts.push( - `**\`${root.path ?? '?'}\`:${root.line ?? '?'}** — initiated by @${root.user?.login ?? '?'}`, + `**\`${root.path ?? '?'}\`:${root.line ?? '?'}** — initiated by @${root.user?.login ?? '?'} (comment ${root.id})`, ); parts.push(''); parts.push( diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 863ccf299b0..4c7e60dc63c 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -128,7 +128,7 @@ Based on the parsed `target.type`: **`read_file` returns the first `truncateToolOutputThreshold` characters (25 000 by default) and sets `isTruncated`. Read that flag.** On a PR with a long history the context file exceeds it — `pr-context` prints a `warning:` line naming the size and any headings past the cut. When it does, page the remainder with `offset`/`limit` before Step 3, and pass the _whole_ file's contents onward. A review that never reached the open-comment section will report "no blockers" without having seen a single one of them. - - **Fetch the comment STATUS index** (worktree mode, whenever the context file reports existing inline comments; run it from inside the worktree so the git half can see the reviewed code): + - **Fetch the comment STATUS index** (worktree mode, whenever the context file reports existing inline comments). Run it **from the main checkout, exactly like the other subcommands** — do NOT `cd` into the worktree for it: it locates the PR worktree itself and scopes its git queries there with `git -C`, while writing its `--out` report into the trusted main-checkout `.qwen/tmp` alongside the others. (Running it from inside the untrusted worktree would let a PR redirect that relative `--out` through a planted symlink.) ```bash "${QWEN_CODE_CLI:-qwen}" review comment-status / \ From 64aced94b7324ec0290731144f78cd2d6c410a4d Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 05:32:35 +0800 Subject: [PATCH 06/12] =?UTF-8?q?fix(review):=20comment-status=20round=203?= =?UTF-8?q?=20=E2=80=94=20precise=20staleWorktree,=20worktree-missing=20wa?= =?UTF-8?q?rning,=20discriminating=20pathspec=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses three Suggestions: - staleWorktree is now keyed on worktreeStale alone, not the headDrift union. A head that merely moved between the two samples while the worktree already matches the final head is NOT a superseded checkout, so its threads no longer carry staleWorktree:true against the field's documented meaning. headMovedDuringFetch stays a separate top-level flag + warning. - A missing worktree (comment-status run before fetch-pr or after cleanup) now sets worktreeMissing on the report and prints a warning — previously every thread degraded to code:'unknown' silently, readable as "nothing changed". - The literal-pathspec test now uses a discriminating pathspec (`:(glob)pkg/**`): magic would match the changed file (true), literal is a nonexistent filename (false), so asserting false actually fails if the `:(top,literal)` prefix is dropped — the old `:(exclude)…` read false under both interpretations. A plain-path control proves the probe is live. --- .../review/comment-status.handler.test.ts | 45 +++++++++++++++++++ .../review/comment-status.integration.test.ts | 15 +++++-- .../cli/src/commands/review/comment-status.ts | 33 +++++++++++--- 3 files changed, 82 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/commands/review/comment-status.handler.test.ts b/packages/cli/src/commands/review/comment-status.handler.test.ts index 3ba4b71c308..1bf9fe4db6a 100644 --- a/packages/cli/src/commands/review/comment-status.handler.test.ts +++ b/packages/cli/src/commands/review/comment-status.handler.test.ts @@ -142,4 +142,49 @@ describe('comment-status handler', () => { expect(report.liveHeadSha).toBe('headB'); expect(warnings().join('\n')).toContain('moved during the comments fetch'); }); + + it('does not mark threads staleWorktree when the head raced but the worktree matches the final head', async () => { + // Worktree is at headB (the final live head); the head merely moved + // mid-fetch. The checkout is NOT superseded, so staleWorktree must stay + // false even though headDrift/headMovedDuringFetch are true. + mocks.gitOpt.mockImplementation((...args: string[]) => + args.includes('rev-parse') ? 'headB' : null, + ); + // One thread so we can inspect its code facts. + mocks.ghApiAll.mockReturnValue([ + { + id: 1, + user: { login: 'r' }, + path: 'a.ts', + line: 1, + original_commit_id: 's', + }, + ]); + queueHeads('headA', 'headB'); + await run(); + const report = reportWritten(); + expect(report.headMovedDuringFetch).toBe(true); + expect(report.threads[0].code.staleWorktree).toBe(false); + }); + + it('warns and flags worktreeMissing when the worktree is absent', async () => { + // gitOpt returns null for everything (no worktree): every thread's code + // facts are unknown, which must not pass silently. + mocks.gitOpt.mockReturnValue(null); + mocks.ghApiAll.mockReturnValue([ + { + id: 1, + user: { login: 'r' }, + path: 'a.ts', + line: 1, + original_commit_id: 's', + }, + ]); + queueHeads('headA', 'headA'); + await run(); + const report = reportWritten(); + expect(report.worktreeMissing).toBe(true); + expect(report.threads[0].code.changedSinceComment).toBe('unknown'); + expect(warnings().join('\n')).toContain('no worktree at'); + }); }); diff --git a/packages/cli/src/commands/review/comment-status.integration.test.ts b/packages/cli/src/commands/review/comment-status.integration.test.ts index 8bef1afb55e..32f753c7b1a 100644 --- a/packages/cli/src/commands/review/comment-status.integration.test.ts +++ b/packages/cli/src/commands/review/comment-status.integration.test.ts @@ -116,10 +116,17 @@ describe('makeGitProbe (real git)', () => { it('takes the comment path literally, not as pathspec magic', () => { const base = commitFile('pkg/src/a.ts', 'v1\n', 'base'); commitFile('pkg/src/a.ts', 'v2\n', 'fix'); - // `:(exclude)…` would flip the meaning if magic were honored; literal - // means "a file actually named this", which does not exist → unchanged. - const got = makeGitProbe(repo)(':(exclude)pkg/src/a.ts', base); - expect(got.changed).toBe(false); + // A DISCRIMINATING pathspec: `:(glob)pkg/**` under magic matches the + // changed a.ts → changed:true; taken literally it is a file named + // `:(glob)pkg/**`, which does not exist → changed:false. Asserting false + // therefore fails if the literal prefix is ever dropped (magic would + // return true) — unlike `:(exclude)…`, which reads false either way. + const magic = makeGitProbe(repo)(':(glob)pkg/**', base); + expect(magic.changed).toBe(false); + // Control: the same file under its plain path IS seen as changed, proving + // the false above is the literal-vs-magic distinction, not a dead probe. + const plain = makeGitProbe(repo)('pkg/src/a.ts', base); + expect(plain.changed).toBe(true); }); it('reports unchanged for a file the range never touched', () => { diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index 2a7637a643a..c8555e8d21e 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -350,17 +350,26 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { const worktree = worktreePath(prNumber); const worktreeHeadSha = gitOpt('-C', worktree, 'rev-parse', 'HEAD'); + // A null HEAD means the worktree is absent (comment-status run before + // fetch-pr, or after cleanup) — every thread's code facts then degrade to + // 'unknown', which must not pass silently as if the files were unchanged. + const worktreeMissing = worktreeHeadSha === null; // Anchor facts (`line`, outdated) describe the LIVE head — GitHub maps // comments against the latest diff it serves. Code facts (`touchedBy`, // changedSinceComment) describe the WORKTREE head — the code this review - // rules on. Drift when the worktree lags the live head, OR when the head - // moved between the two samples (a push raced the fetch). + // rules on. Two distinct conditions: + // - worktreeStale: the checked-out code lags the live head, so the code + // facts describe a SUPERSEDED checkout (this is what staleWorktree means + // per-thread — NOT the union below). + // - headMovedDuringFetch: the head moved between the two samples, so the + // anchor facts may be mixed across commits even if the worktree happens + // to match the final head; that is a separate warning, not staleness. const headMovedDuringFetch = liveHeadBefore !== '' && liveHeadAfter !== '' && liveHeadBefore !== liveHeadAfter; const worktreeStale = - worktreeHeadSha !== null && + !worktreeMissing && liveHeadAfter !== '' && worktreeHeadSha !== liveHeadAfter; const headDrift = headMovedDuringFetch || worktreeStale; @@ -370,10 +379,12 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { prAuthor, makeGitProbe(worktree), ); - if (headDrift) { - // Denormalize the drift onto every thread: the code facts describe a - // superseded checkout, and a jq consumer of threads[] must not need to - // remember a top-level flag to see that. + if (worktreeStale) { + // Denormalize onto every thread: the code facts describe a superseded + // checkout, and a jq consumer of threads[] must not need to remember a + // top-level flag to see that. Keyed on worktreeStale specifically — a + // head that merely moved mid-fetch while the worktree matches the final + // head is NOT a superseded checkout. for (const t of threads) t.code.staleWorktree = true; } const summary = summarizeThreads(threads); @@ -385,6 +396,7 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { liveHeadSha: liveHeadAfter, liveHeadBefore, worktreeHeadSha, + worktreeMissing, headDrift, headMovedDuringFetch, inlineComments: comments.length, @@ -401,6 +413,13 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { `${summary.changedSinceComment} on files changed since their comment, ` + `${summary.withReplies} with replies, ${summary.authorReplied} answered by the PR author)`, ); + if (worktreeMissing) { + writeStdoutLine( + `warning: no worktree at ${worktree} — run \`qwen review fetch-pr\` first. ` + + `Every thread's code facts (changedSinceComment, touchedBy) are \`unknown\`; ` + + `only the anchor and reply facts are usable.`, + ); + } if (headMovedDuringFetch) { writeStdoutLine( `warning: PR head moved during the comments fetch (${liveHeadBefore.slice(0, 8)} → ${liveHeadAfter.slice(0, 8)}) — ` + From b6ba1288ec97979813181110679a8f700f077771 Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 06:43:26 +0800 Subject: [PATCH 07/12] test(review): make the comment-status negation test actually exercise negation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The body 'No blockers here' matched no BLOCKER_PATTERN (the bare plural never triggers /\bblocking\b/ etc.), so isBlocker returned false before the negation branch ran — false coverage. It now uses 'No blocking issues', which matches the signal and must be suppressed by the leading 'No', plus an un-negated control that asserts true. --- .../src/commands/review/comment-status.test.ts | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/review/comment-status.test.ts b/packages/cli/src/commands/review/comment-status.test.ts index 507bdf7b619..31ea1dda20a 100644 --- a/packages/cli/src/commands/review/comment-status.test.ts +++ b/packages/cli/src/commands/review/comment-status.test.ts @@ -155,12 +155,24 @@ describe('buildThreadStatuses — signals', () => { }); it('does not flag a negated blocker mention', () => { - const [t] = buildThreadStatuses( - [comment({ id: 1, body: 'No blockers here, just a nit.' })], + // The body must contain a phrase carriesBlockerSignal actually MATCHES so + // the negation branch is exercised: "blocking" hits /\bblocking\b/, and + // the leading "No " must suppress it. (The bare plural "blockers" matches + // no pattern at all, which would test nothing.) + const [negated] = buildThreadStatuses( + [comment({ id: 1, body: 'No blocking issues here, just a nit.' })], + 'author', + noChange, + ); + expect(negated.isBlocker).toBe(false); + // Control: the same signal un-negated IS flagged, proving the false above + // is the negation logic and not a pattern that never fired. + const [asserted] = buildThreadStatuses( + [comment({ id: 2, body: 'This is a blocking defect.' })], 'author', noChange, ); - expect(t.isBlocker).toBe(false); + expect(asserted.isBlocker).toBe(true); }); it('never reports authorReplied for a ghost author matching a ghost replier', () => { From 87f37878e6cf6c9a8a5d5f2285a37ddad7ea1340 Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 07:50:40 +0800 Subject: [PATCH 08/12] test(review): assert per-thread staleWorktree and --host wiring; document ghApiAll merge contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two test gaps + one recurring-review clarification: - The worktree-lag drift test now includes a thread and asserts staleWorktree:true is denormalized onto its code object — the old positive case had zero threads, so the denormalization loop iterated over nothing and would pass even if the block were deleted. - A --host test now asserts setGhHost is called with the argv host, matching the presubmit analog; without it a dropped setGhHost would silently target github.com for a GHE review. - ghApiAll's doc now explains why one JSON.parse is correct on multi-page output: gh --paginate MERGES top-level arrays into one (it does not emit one array per page); the per-page-concat failure only affects key-nested arrays, which is exactly why ghApiAllNested exists. Verified on a 4-page 97-comment response. --- .../review/comment-status.handler.test.ts | 30 ++++++++++++++++++- packages/cli/src/commands/review/lib/gh.ts | 15 ++++++++-- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/commands/review/comment-status.handler.test.ts b/packages/cli/src/commands/review/comment-status.handler.test.ts index 1bf9fe4db6a..1c1f35c138a 100644 --- a/packages/cli/src/commands/review/comment-status.handler.test.ts +++ b/packages/cli/src/commands/review/comment-status.handler.test.ts @@ -118,15 +118,43 @@ describe('comment-status handler', () => { expect(warnings()).toEqual([]); }); - it('flags drift and warns when the worktree lags the live head', async () => { + it('flags drift, warns, and denormalizes staleWorktree onto each thread when the worktree lags', async () => { + // A thread must be present so the denormalization loop is actually + // exercised: with zero threads it iterates over nothing and would pass + // even if the block were removed. + mocks.ghApiAll.mockReturnValue([ + { + id: 1, + user: { login: 'r' }, + path: 'a.ts', + line: 1, + original_commit_id: 's', + }, + ]); queueHeads('headB', 'headB'); // live head B, worktree still at headA await run(); const report = reportWritten(); expect(report.headDrift).toBe(true); expect(report.headMovedDuringFetch).toBe(false); + expect(report.threads[0].code.staleWorktree).toBe(true); expect(warnings().join('\n')).toContain('worktree HEAD'); }); + it('threads --host to setGhHost so a GHE review targets the right host', async () => { + queueHeads('headA', 'headA'); + const handler = commentStatusCommand.handler; + if (!handler) throw new Error('handler missing'); + await handler({ + _: [], + $0: 'qwen', + pr_number: '7632', + owner_repo: 'o/r', + out: '/repo/.qwen/tmp/qwen-review-pr-7632-comment-status.json', + host: 'github.example.com', + } as unknown as Parameters[0]); + expect(mocks.setGhHost).toHaveBeenCalledWith('github.example.com'); + }); + it('flags a push that raced the comments fetch, recording both samples', async () => { // Worktree happens to match the post-fetch head, but the head MOVED // during the fetch — anchor facts may be mixed across commits. diff --git a/packages/cli/src/commands/review/lib/gh.ts b/packages/cli/src/commands/review/lib/gh.ts index f68e629ac2d..d1fb87e9b7e 100644 --- a/packages/cli/src/commands/review/lib/gh.ts +++ b/packages/cli/src/commands/review/lib/gh.ts @@ -160,9 +160,18 @@ export function ghApi(path: string, jq?: string): unknown { * * Use this for endpoints that return arrays and may have more than 30 * (the default `per_page`) entries — PR `/comments`, `/issues/{n}/comments`, - * `/reviews`, etc. `gh --paginate` walks every `next` link and concatenates - * each page's array into a single top-level array, so a single - * `JSON.parse` recovers the full set. + * `/reviews`, etc. + * + * **Why a single `JSON.parse` is correct on multi-page output (a recurring + * review question):** for a TOP-LEVEL JSON array `gh --paginate` MERGES the + * pages into one array — it does NOT emit one array per page. So the output + * is a single well-formed array and `JSON.parse` recovers the full set. The + * per-page-concatenation failure mode (`}{` / `][` between pages that would + * throw) only happens for endpoints whose array is NESTED under a key (e.g. + * `check-runs`), and those go through {@link ghApiAllNested} with + * `--jq '.[]'` + NDJSON parsing precisely because `--paginate` can't + * merge them. Verified empirically on a 4-page (`per_page=30`, 97-comment) + * `pulls/{n}/comments` response: zero `][` markers, one array, clean parse. * * Returns `[]` for empty responses or non-array payloads (defensive — the * endpoint may legitimately return an object on a 4xx-style 200, e.g. an From e2d82cd9aeff52221e06c82e7da8f2323bd430b5 Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 11:23:00 +0800 Subject: [PATCH 09/12] fix(review): comment-status degrades gracefully on failure per its SKILL.md contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SKILL.md promises this command is an index, not evidence — "if it fails (auth, network), warn and continue" — but runCommentStatus had no try/catch, so an ensureAuthenticated() or gh throw propagated as an unhandled rejection and killed the whole review. The runtime body is now wrapped: any throw writes a minimal empty report ({prNumber, ownerRepo, error, threads: []}) so downstream jq still parses, prints a "comment-status failed" warning, and exits 0. Handler test pins the auth-failure path (no throw, empty report, warning). Reported by @yiliang114. --- .../review/comment-status.handler.test.ts | 15 + .../cli/src/commands/review/comment-status.ts | 261 ++++++++++-------- 2 files changed, 161 insertions(+), 115 deletions(-) diff --git a/packages/cli/src/commands/review/comment-status.handler.test.ts b/packages/cli/src/commands/review/comment-status.handler.test.ts index 1c1f35c138a..9894c9f6fe1 100644 --- a/packages/cli/src/commands/review/comment-status.handler.test.ts +++ b/packages/cli/src/commands/review/comment-status.handler.test.ts @@ -215,4 +215,19 @@ describe('comment-status handler', () => { expect(report.threads[0].code.changedSinceComment).toBe('unknown'); expect(warnings().join('\n')).toContain('no worktree at'); }); + + it('degrades gracefully when gh auth fails: no throw, empty report, warning', async () => { + // SKILL.md promises this command cannot kill the pipeline on an auth or + // network failure. A throw must become a minimal empty report + warning, + // not an unhandled rejection. + mocks.ensureAuthenticated.mockImplementation(() => { + throw new Error('gh CLI is not authenticated. Run `gh auth login`.'); + }); + + await expect(run()).resolves.toBeUndefined(); + const report = reportWritten(); + expect(report.threads).toEqual([]); + expect(report.error).toContain('not authenticated'); + expect(warnings().join('\n')).toContain('comment-status failed'); + }); }); diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index c8555e8d21e..7f20ce73079 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -316,132 +316,163 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { } const [owner, repo] = ownerRepo.split('/'); - ensureAuthenticated(); + // Graceful degradation per SKILL.md: this command is an index, not the + // evidence — a runtime failure (auth, network, gh) must not kill the + // review. On any throw, write a minimal empty report so downstream jq + // still parses, warn, and exit 0; Step 6 then re-derives statuses per + // comment. + try { + ensureAuthenticated(); - // Sample the live head BEFORE the comments fetch and AGAIN after: GitHub - // maps comment lines against whatever head is current when the list is - // served, so a push landing mid-fetch would pair anchor facts from a newer - // head with a stale drift comparison. If the two samples disagree the PR is - // moving right now — treat it as drift. - const meta = JSON.parse( - gh( - 'pr', - 'view', - prNumber, - '--repo', - ownerRepo, - '--json', - 'author,headRefOid', - ), - ) as { author?: { login?: string } | null; headRefOid?: string }; - const prAuthor = meta.author?.login ?? ''; - const liveHeadBefore = meta.headRefOid ?? ''; + // Sample the live head BEFORE the comments fetch and AGAIN after: GitHub + // maps comment lines against whatever head is current when the list is + // served, so a push landing mid-fetch would pair anchor facts from a newer + // head with a stale drift comparison. If the two samples disagree the PR is + // moving right now — treat it as drift. + const meta = JSON.parse( + gh( + 'pr', + 'view', + prNumber, + '--repo', + ownerRepo, + '--json', + 'author,headRefOid', + ), + ) as { author?: { login?: string } | null; headRefOid?: string }; + const prAuthor = meta.author?.login ?? ''; + const liveHeadBefore = meta.headRefOid ?? ''; - const comments = ghApiAll( - `repos/${owner}/${repo}/pulls/${prNumber}/comments`, - ) as RawStatusComment[]; + const comments = ghApiAll( + `repos/${owner}/${repo}/pulls/${prNumber}/comments`, + ) as RawStatusComment[]; - const liveHeadAfter = - ( - JSON.parse( - gh('pr', 'view', prNumber, '--repo', ownerRepo, '--json', 'headRefOid'), - ) as { headRefOid?: string } - ).headRefOid ?? ''; + const liveHeadAfter = + ( + JSON.parse( + gh( + 'pr', + 'view', + prNumber, + '--repo', + ownerRepo, + '--json', + 'headRefOid', + ), + ) as { headRefOid?: string } + ).headRefOid ?? ''; - const worktree = worktreePath(prNumber); - const worktreeHeadSha = gitOpt('-C', worktree, 'rev-parse', 'HEAD'); - // A null HEAD means the worktree is absent (comment-status run before - // fetch-pr, or after cleanup) — every thread's code facts then degrade to - // 'unknown', which must not pass silently as if the files were unchanged. - const worktreeMissing = worktreeHeadSha === null; - // Anchor facts (`line`, outdated) describe the LIVE head — GitHub maps - // comments against the latest diff it serves. Code facts (`touchedBy`, - // changedSinceComment) describe the WORKTREE head — the code this review - // rules on. Two distinct conditions: - // - worktreeStale: the checked-out code lags the live head, so the code - // facts describe a SUPERSEDED checkout (this is what staleWorktree means - // per-thread — NOT the union below). - // - headMovedDuringFetch: the head moved between the two samples, so the - // anchor facts may be mixed across commits even if the worktree happens - // to match the final head; that is a separate warning, not staleness. - const headMovedDuringFetch = - liveHeadBefore !== '' && - liveHeadAfter !== '' && - liveHeadBefore !== liveHeadAfter; - const worktreeStale = - !worktreeMissing && - liveHeadAfter !== '' && - worktreeHeadSha !== liveHeadAfter; - const headDrift = headMovedDuringFetch || worktreeStale; + const worktree = worktreePath(prNumber); + const worktreeHeadSha = gitOpt('-C', worktree, 'rev-parse', 'HEAD'); + // A null HEAD means the worktree is absent (comment-status run before + // fetch-pr, or after cleanup) — every thread's code facts then degrade to + // 'unknown', which must not pass silently as if the files were unchanged. + const worktreeMissing = worktreeHeadSha === null; + // Anchor facts (`line`, outdated) describe the LIVE head — GitHub maps + // comments against the latest diff it serves. Code facts (`touchedBy`, + // changedSinceComment) describe the WORKTREE head — the code this review + // rules on. Two distinct conditions: + // - worktreeStale: the checked-out code lags the live head, so the code + // facts describe a SUPERSEDED checkout (this is what staleWorktree means + // per-thread — NOT the union below). + // - headMovedDuringFetch: the head moved between the two samples, so the + // anchor facts may be mixed across commits even if the worktree happens + // to match the final head; that is a separate warning, not staleness. + const headMovedDuringFetch = + liveHeadBefore !== '' && + liveHeadAfter !== '' && + liveHeadBefore !== liveHeadAfter; + const worktreeStale = + !worktreeMissing && + liveHeadAfter !== '' && + worktreeHeadSha !== liveHeadAfter; + const headDrift = headMovedDuringFetch || worktreeStale; - const threads = buildThreadStatuses( - comments, - prAuthor, - makeGitProbe(worktree), - ); - if (worktreeStale) { - // Denormalize onto every thread: the code facts describe a superseded - // checkout, and a jq consumer of threads[] must not need to remember a - // top-level flag to see that. Keyed on worktreeStale specifically — a - // head that merely moved mid-fetch while the worktree matches the final - // head is NOT a superseded checkout. - for (const t of threads) t.code.staleWorktree = true; - } - const summary = summarizeThreads(threads); + const threads = buildThreadStatuses( + comments, + prAuthor, + makeGitProbe(worktree), + ); + if (worktreeStale) { + // Denormalize onto every thread: the code facts describe a superseded + // checkout, and a jq consumer of threads[] must not need to remember a + // top-level flag to see that. Keyed on worktreeStale specifically — a + // head that merely moved mid-fetch while the worktree matches the final + // head is NOT a superseded checkout. + for (const t of threads) t.code.staleWorktree = true; + } + const summary = summarizeThreads(threads); - const report = { - prNumber, - ownerRepo, - prAuthor, - liveHeadSha: liveHeadAfter, - liveHeadBefore, - worktreeHeadSha, - worktreeMissing, - headDrift, - headMovedDuringFetch, - inlineComments: comments.length, - summary, - threads, - }; + const report = { + prNumber, + ownerRepo, + prAuthor, + liveHeadSha: liveHeadAfter, + liveHeadBefore, + worktreeHeadSha, + worktreeMissing, + headDrift, + headMovedDuringFetch, + inlineComments: comments.length, + summary, + threads, + }; - mkdirSync(dirname(out), { recursive: true }); - const json = JSON.stringify(report, null, 2) + '\n'; - writeFileSync(out, json, 'utf8'); - writeStdoutLine( - `Wrote comment-status report to ${out} (${comments.length} inline comments in ${summary.threads} threads: ` + - `${summary.outdated} outdated, ${summary.blockers} blocker(s), ` + - `${summary.changedSinceComment} on files changed since their comment, ` + - `${summary.withReplies} with replies, ${summary.authorReplied} answered by the PR author)`, - ); - if (worktreeMissing) { - writeStdoutLine( - `warning: no worktree at ${worktree} — run \`qwen review fetch-pr\` first. ` + - `Every thread's code facts (changedSinceComment, touchedBy) are \`unknown\`; ` + - `only the anchor and reply facts are usable.`, - ); - } - if (headMovedDuringFetch) { + mkdirSync(dirname(out), { recursive: true }); + const json = JSON.stringify(report, null, 2) + '\n'; + writeFileSync(out, json, 'utf8'); writeStdoutLine( - `warning: PR head moved during the comments fetch (${liveHeadBefore.slice(0, 8)} → ${liveHeadAfter.slice(0, 8)}) — ` + - `anchor facts may be mixed across commits. Re-run after the push settles.`, + `Wrote comment-status report to ${out} (${comments.length} inline comments in ${summary.threads} threads: ` + + `${summary.outdated} outdated, ${summary.blockers} blocker(s), ` + + `${summary.changedSinceComment} on files changed since their comment, ` + + `${summary.withReplies} with replies, ${summary.authorReplied} answered by the PR author)`, ); - } else if (worktreeStale) { + if (worktreeMissing) { + writeStdoutLine( + `warning: no worktree at ${worktree} — run \`qwen review fetch-pr\` first. ` + + `Every thread's code facts (changedSinceComment, touchedBy) are \`unknown\`; ` + + `only the anchor and reply facts are usable.`, + ); + } + if (headMovedDuringFetch) { + writeStdoutLine( + `warning: PR head moved during the comments fetch (${liveHeadBefore.slice(0, 8)} → ${liveHeadAfter.slice(0, 8)}) — ` + + `anchor facts may be mixed across commits. Re-run after the push settles.`, + ); + } else if (worktreeStale) { + writeStdoutLine( + `warning: worktree HEAD ${(worktreeHeadSha ?? '').slice(0, 8)} != live PR head ${liveHeadAfter.slice(0, 8)} — ` + + `the PR advanced since fetch-pr. Anchor facts describe the live head; ` + + `code facts describe the worktree.`, + ); + } + // Same silent-tail hazard pr-context already warns about, with sharper + // teeth here: `threads` is sorted by path, so a truncated read drops the + // alphabetically-later files WHOLESALE (measured on a 71-thread PR: one + // read showed 35 threads and lost 24 blocker-flagged ones), and cut JSON + // is not merely incomplete but unparseable. + if (json.length > DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD) { + writeStdoutLine( + `warning: ${out} is ${json.length} chars; read_file returns the first ` + + `${DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD} and sets isTruncated — cut JSON does not parse. ` + + `Query it with jq (it is machine-shaped), or page with offset/limit until isTruncated is false.`, + ); + } + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); writeStdoutLine( - `warning: worktree HEAD ${(worktreeHeadSha ?? '').slice(0, 8)} != live PR head ${liveHeadAfter.slice(0, 8)} — ` + - `the PR advanced since fetch-pr. Anchor facts describe the live head; ` + - `code facts describe the worktree.`, + `warning: comment-status failed: ${msg}. It is an index, not the ` + + `evidence — re-derive thread statuses per-comment if needed.`, ); - } - // Same silent-tail hazard pr-context already warns about, with sharper - // teeth here: `threads` is sorted by path, so a truncated read drops the - // alphabetically-later files WHOLESALE (measured on a 71-thread PR: one - // read showed 35 threads and lost 24 blocker-flagged ones), and cut JSON - // is not merely incomplete but unparseable. - if (json.length > DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD) { - writeStdoutLine( - `warning: ${out} is ${json.length} chars; read_file returns the first ` + - `${DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD} and sets isTruncated — cut JSON does not parse. ` + - `Query it with jq (it is machine-shaped), or page with offset/limit until isTruncated is false.`, + mkdirSync(dirname(out), { recursive: true }); + writeFileSync( + out, + JSON.stringify( + { prNumber, ownerRepo, error: msg, threads: [] }, + null, + 2, + ) + '\n', + 'utf8', ); } } From 2939e647aeff2bd660a12e67e685ab8a2074870b Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 11:34:42 +0800 Subject: [PATCH 10/12] test(review): pin comment-status owner_repo guard and truncation-size warning Two untested paths flagged in review: the owner_repo-without-slash guard (a caller error that must still throw, distinct from the runtime graceful-degradation path) and the report-size warning that fires when the JSON crosses read_file's truncation threshold. --- .../review/comment-status.handler.test.ts | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/review/comment-status.handler.test.ts b/packages/cli/src/commands/review/comment-status.handler.test.ts index 9894c9f6fe1..3348d741110 100644 --- a/packages/cli/src/commands/review/comment-status.handler.test.ts +++ b/packages/cli/src/commands/review/comment-status.handler.test.ts @@ -60,14 +60,14 @@ vi.mock('../../utils/stdioHelpers.js', () => ({ const { commentStatusCommand } = await import('./comment-status.js'); -async function run() { +async function run(ownerRepo = 'o/r') { const handler = commentStatusCommand.handler; if (!handler) throw new Error('handler missing'); await handler({ _: [], $0: 'qwen', pr_number: '7632', - owner_repo: 'o/r', + owner_repo: ownerRepo, out: '/repo/.qwen/tmp/qwen-review-pr-7632-comment-status.json', } as unknown as Parameters[0]); } @@ -216,6 +216,31 @@ describe('comment-status handler', () => { expect(warnings().join('\n')).toContain('no worktree at'); }); + it('rejects an owner_repo with no slash (a caller error, not runtime degradation)', async () => { + await expect(run('ownerrepo')).rejects.toThrow(/owner\/repo/); + }); + + it('warns when the report exceeds the read_file truncation threshold', async () => { + // A big PR pushes the JSON past ~25k chars; the size warning is the only + // signal a consumer gets before a silent isTruncated on cut, unparseable + // JSON. Generate enough threads to cross the threshold. + mocks.ghApiAll.mockReturnValue( + Array.from({ length: 200 }, (_, i) => ({ + id: i + 1, + user: { login: `reviewer-with-a-longish-name-${i}` }, + path: `packages/cli/src/commands/review/some/deep/path/file-${i}.ts`, + line: i + 1, + original_line: i + 1, + original_commit_id: `commit-sha-placeholder-${i}`, + subject_type: 'line', + body: `A finding body number ${i} with enough text to add weight.`, + })), + ); + queueHeads('headA', 'headA'); + await run(); + expect(warnings().join('\n')).toMatch(/chars; read_file returns the first/); + }); + it('degrades gracefully when gh auth fails: no throw, empty report, warning', async () => { // SKILL.md promises this command cannot kill the pipeline on an auth or // network failure. A throw must become a minimal empty report + warning, From 32587712ecc16ece3a272e68c092c7572fc3fb30 Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 14:12:13 +0800 Subject: [PATCH 11/12] fix(review): comment-status degraded report carries the full shape, not a stripped one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The graceful-degradation path wrote { prNumber, ownerRepo, error, threads: [] }, omitting headDrift/summary/headMovedDuringFetch/etc. A consumer reading report.headDrift then got undefined (falsy = "no drift"), silently mistaking a total index failure for a clean "nothing moved" — and the review orchestrator keys code-fact warnings on exactly that field. The degraded report now emits the same shape as the success report with safe defaults plus `error`, so a consumer that checks `error` sees the failure and one that reads a fact gets a neutral value, never a misleading one. Reported by @doudouOUC. --- .../review/comment-status.handler.test.ts | 6 +++++ .../cli/src/commands/review/comment-status.ts | 22 ++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/review/comment-status.handler.test.ts b/packages/cli/src/commands/review/comment-status.handler.test.ts index 3348d741110..2ceb7e8ee1e 100644 --- a/packages/cli/src/commands/review/comment-status.handler.test.ts +++ b/packages/cli/src/commands/review/comment-status.handler.test.ts @@ -254,5 +254,11 @@ describe('comment-status handler', () => { expect(report.threads).toEqual([]); expect(report.error).toContain('not authenticated'); expect(warnings().join('\n')).toContain('comment-status failed'); + // The degraded report carries the SAME shape as success so a consumer + // reading headDrift/summary gets a neutral value, not a misleading + // `undefined` (which reads as "no drift"). + expect(report.headDrift).toBe(false); + expect(report.summary).toMatchObject({ threads: 0, blockers: 0 }); + expect(report.inlineComments).toBe(0); }); }); diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index 7f20ce73079..2c5bc19165c 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -465,10 +465,30 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { `evidence — re-derive thread statuses per-comment if needed.`, ); mkdirSync(dirname(out), { recursive: true }); + // Emit the SAME shape as the success report (with an added `error`), not a + // stripped one: a consumer reading `report.headDrift` on a stripped report + // gets `undefined` (falsy = "no drift"), silently mistaking a total index + // failure for a clean "nothing moved". Safe defaults + `error` let a + // consumer that checks `error` see the failure and one that reads a fact + // get a neutral value, never a misleading one. writeFileSync( out, JSON.stringify( - { prNumber, ownerRepo, error: msg, threads: [] }, + { + prNumber, + ownerRepo, + error: msg, + prAuthor: '', + liveHeadSha: '', + liveHeadBefore: '', + worktreeHeadSha: null, + worktreeMissing: false, + headDrift: false, + headMovedDuringFetch: false, + inlineComments: 0, + summary: summarizeThreads([]), + threads: [], + }, null, 2, ) + '\n', From 819d9d770122ce77056244152ec26e43af00d5e7 Mon Sep 17 00:00:00 2001 From: verify Date: Sat, 25 Jul 2026 15:20:23 +0800 Subject: [PATCH 12/12] fix(review): degraded report's worktreeMissing must not contradict worktreeHeadSha: null MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The catch-block report hardcoded worktreeMissing: false beside worktreeHeadSha: null — a positive "worktree present" assertion the success path (worktreeMissing = worktreeHeadSha === null) would never make for a null head. A consumer reading worktreeMissing without gating on error would conclude the worktree exists on a run where nothing is known. Now true, matching the null head and the fail-safe reading (code facts unavailable). Reported by qwen-code-ci-bot. --- .../cli/src/commands/review/comment-status.handler.test.ts | 3 +++ packages/cli/src/commands/review/comment-status.ts | 6 +++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/review/comment-status.handler.test.ts b/packages/cli/src/commands/review/comment-status.handler.test.ts index 2ceb7e8ee1e..3c4ffd22e3b 100644 --- a/packages/cli/src/commands/review/comment-status.handler.test.ts +++ b/packages/cli/src/commands/review/comment-status.handler.test.ts @@ -260,5 +260,8 @@ describe('comment-status handler', () => { expect(report.headDrift).toBe(false); expect(report.summary).toMatchObject({ threads: 0, blockers: 0 }); expect(report.inlineComments).toBe(0); + // worktreeMissing must not contradict worktreeHeadSha: null. + expect(report.worktreeHeadSha).toBeNull(); + expect(report.worktreeMissing).toBe(true); }); }); diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index 2c5bc19165c..edc4eaf3dad 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -482,7 +482,11 @@ async function runCommentStatus(args: CommentStatusArgs): Promise { liveHeadSha: '', liveHeadBefore: '', worktreeHeadSha: null, - worktreeMissing: false, + // Consistent with `worktreeHeadSha: null` (the success path derives + // worktreeMissing from exactly that) and fail-safe: a degraded run + // has no usable worktree, so `true` reads code facts as unavailable + // rather than falsely asserting the worktree is present. + worktreeMissing: true, headDrift: false, headMovedDuringFetch: false, inlineComments: 0,