Skip to content

feat(review): route suggestion-level findings to an updatable PR comment#5786

Open
pomelo-nwu wants to merge 1 commit into
QwenLM:mainfrom
pomelo-nwu:worktree-review-suggestion-summary
Open

feat(review): route suggestion-level findings to an updatable PR comment#5786
pomelo-nwu wants to merge 1 commit into
QwenLM:mainfrom
pomelo-nwu:worktree-review-suggestion-summary

Conversation

@pomelo-nwu

Copy link
Copy Markdown
Collaborator

/review used to publish both Critical and Suggestion-level findings as per-line inline review comments. Each run re-emitted a fresh batch with no notion of "this suggestion was already posted and is still open," so a PR's Files changed view accumulated unresolved Suggestion threads every round. For agentic authors iterating on a PR, every thread felt mandatory to resolve — the issues never converged.

This change routes Suggestion findings to a single, updatable issue comment that is PATCHed in place across runs, while Critical findings stay inline (pinned to the exact code line, real blockers).

Behavior change

  • Critical findings → inline review comments (unchanged). Pinned to the code, the strongest "fix before merge" signal.
  • Suggestion findings → one issue comment on the PR thread, refreshed each /review run. Each row keeps a file:line so it stays actionable despite not being inline.
  • The new deterministic qwen review post-suggestions subcommand locates the existing summary by author + an embedded HTML marker and PATCHes it, or creates it on first run — the LLM never reposts a duplicate.
  • Nice-to-have and low-confidence findings remain terminal-only (unchanged).

Why not "Critical-only inline + Suggestion in the review body"

The review body is a frozen artifact of one review submission. A new /review run appends a new review with its own body, so Suggestion lists would still accumulate across runs (one body per review) and never converge. One updatable issue comment is the only primitive that actually refreshes rather than grows.

How to verify

  • /review <pr> --comment on a PR with both Critical and Suggestion findings → Critical appears inline; Suggestion appears as one issue comment.
  • Run /review <pr> --comment again on the same PR → the Suggestion comment is updated in place (same id), not duplicated. The terminal logs "Suggestion summary updated as comment <id>".
  • /review <pr> --comment on a PR with only Suggestion findings → no inline comments, just the summary comment plus a COMMENT review pointing to it.

Tests

  • New packages/cli/src/commands/review/post-suggestions.test.ts covers the pure findExistingSummary locator (author match, marker match, latest-wins, case-insensitivity, empty/no-body).
  • typecheck, build, lint all green.
中文说明

/review 之前会把 Critical 和 Suggestion 级别的发现都作为逐行 inline review comment 发布。每次运行都重新发一批,没有"这条 suggestion 之前已经提过且仍 open"的概念,导致 PR 的 Files changed 视图每轮都堆积未解决的 Suggestion 线程。对于在 PR 上反复迭代的 agentic author,每条线程都像是必须解决——问题始终无法收敛。

本次改动把 Suggestion 发现路由到一条可原地更新的 issue comment(每次 /review 运行 PATCH 覆盖),Critical 发现保持 inline(钉在具体代码行,是真正的阻塞项)。

行为变化:

  • Critical → inline review comment(不变)
  • Suggestion → PR 主线程上的单条 issue comment,每次运行刷新;每行保留 file:line 保证可操作性
  • 新增确定性子命令 qwen review post-suggestions:按 author + 嵌入的 HTML marker 定位已有 summary 并 PATCH,或首次创建,LLM 不会重复发布
  • Nice-to-have / 低置信 发现仍只在终端输出(不变)

为什么不是"Critical inline + Suggestion 进 review body":review body 是单次提交的冻结产物,新一轮 /review 会追加一个新 review(各自带 body),Suggestion 列表仍会跨运行累积,无法收敛。只有"一条可更新的 issue comment"才是真正会刷新而非增长的原语。

Suggestion-level /review findings now go to a single issue comment that is PATCHed in place across runs, instead of becoming per-line inline comments. Critical findings stay inline.

Why: every /review run re-emitted a fresh batch of inline comments with no notion of "this suggestion was already posted and is still open", so the PR Files-changed view grew noisier each round and issues never converged — worst for agentic authors who feel forced to resolve each thread one-by-one. One updatable comment keeps the suggestion list a single refreshable view; the locate-and-PATCH lives in a new deterministic `qwen review post-suggestions` subcommand so the LLM never reposts a duplicate.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @pomelo-nwu, thanks for this PR — the description and motivation are actually quite thorough, so this is purely a structural thing.

The PR body is missing the required headings from the PR template. Specifically, these sections are absent:

  • ## What this PR does
  • ## Why it's needed
  • ## Reviewer Test Plan (the ### How to verify heading is there, but the parent section is missing)
  • ## Risk & Scope
  • ## Linked Issues

Could you restructure the body to use those headings? The content is all there — it just needs to be slotted into the right sections. Once that's done, I'll continue the review.

中文说明

@pomelo-nwu 你好,感谢这个 PR——描述和动机其实写得相当充分,这纯粹是结构上的问题。

PR body 缺少 PR 模板 中要求的标题,具体缺少:

  • ## What this PR does
  • ## Why it's needed
  • ## Reviewer Test Plan### How to verify 有,但父级 section 缺失)
  • ## Risk & Scope
  • ## Linked Issues

能否把内容按模板标题重新组织一下?内容本身都在——只需要放到对应的 section 里。改完后我会继续审查。

Qwen Code · qwen3.7-max


ensureAuthenticated();

const bodyContent = readFileSync(bodyFile, 'utf8');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] No validation that bodyContent contains SUMMARY_MARKER. The --body-file option description says "must begin with the summary marker", but this is never enforced at runtime. If the caller omits the marker (LLM prompt drift, direct CLI invocation), the first run POSTs a comment without the marker. On every subsequent run, findExistingSummary fails to match (no marker in the body), so a new duplicate comment is created each time — the exact "growing pile of threads" problem this subcommand was designed to prevent.

Suggested change
const bodyContent = readFileSync(bodyFile, 'utf8');
const bodyContent = readFileSync(bodyFile, 'utf8');
if (!bodyContent.includes(SUMMARY_MARKER)) {
throw new Error(
`body-file must contain the summary marker (${SUMMARY_MARKER}) so the comment can be located and updated on subsequent runs`,
);
}
const payload = JSON.stringify({ body: bodyContent });

— qwen3.7-max via Qwen Code /review

'--input',
payloadPath,
);
commentId = (JSON.parse(raw) as { id: number }).id;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] unlinkSync(payloadPath) in the finally block is not wrapped in try/catch. If unlinkSync throws (e.g., file already removed by concurrent cleanup, permission error), it replaces the original error from the try block — the operator sees a filesystem error instead of the actual GitHub API failure. The sibling cleanup.ts wraps every unlinkSync in try/catch for exactly this reason.

Also, writeFileSync(payloadPath, ...) at line 93 is outside the try block. If preceding code (ensureAuthenticated, currentUser, ghApiAll) throws before try is reached, finally still runs and unlinkSync may throw ENOENT if the file was never written.

Suggested change
commentId = (JSON.parse(raw) as { id: number }).id;
} finally {
try {
unlinkSync(payloadPath);
} catch {
// best-effort cleanup — also swept by `qwen review cleanup`
}
}

— qwen3.7-max via Qwen Code /review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants