feat(review): post Suggestion findings as inline comments - #6593
Conversation
Suggestion-level findings were routed to a single updatable issue comment (the "suggestion summary") while only Critical findings became inline review comments. That split traded away two things that turned out to matter more than the convergence it bought: - An issue comment has no lifecycle. GitHub folds an inline review thread away as Outdated once the author edits the line it is anchored to, so an addressed finding removes itself from the page. The summary comment just sits in the PR conversation forever; PATCHing it to "all addressed" replaces its content but not the comment. The mechanism meant to prevent clutter was the clutter. - A Markdown table cannot carry a one-click fix. GitHub renders a ```suggestion fence as an applicable change only inside a review comment on a diff line. Suggestion findings are exactly the mechanical, localized cleanups that benefit most from one-click apply, so the split withheld the feature from the findings that needed it most. Both severities now post as inline comments, distinguished by a **[Critical]** or **[Suggestion]** body prefix. The `qwen review post-suggestions` subcommand and its plumbing are removed. Follow-on changes required by the reroute: - pr-context: the "Previous suggestion summary" section is gone. Legacy summary comments are still recognised so they stay out of "Already discussed", but the exclusion is now marker-only rather than author-gated. The author check missed summaries posted by the *other* identity: /review runs as a maintainer locally and as qwen-code-ci-bot in CI, and roughly half of the last 60 PRs carry a bot-authored summary. Those leaked into "Already discussed" and told the review agents not to re-report the findings listed there. The check originally guarded promotion into a trusted rendering section; that section no longer exists, so it only gated exclusion, where a third party embedding the marker merely hides their own comment. - qwen-autofix: the workflow filters "suggestion summaries" out of the autofix bot's actionable queue, but only on the issue-comment channel. With Suggestions now inline, they entered the unfiltered inline channel and the bot would apply non-blocking recommendations and spend a review round on them. The inline channel now applies the same gate, keyed on the **[Suggestion]** prefix plus the /review footer so a human quoting the prefix stays actionable. - Step 7 gains a 422 fallback. Create Review is all-or-nothing, so one Suggestion anchored outside the diff would take the Critical findings down with it — a risk that did not exist when Suggestions travelled on a line-agnostic issue comment. GitHub's 422 does not name the offending entry, so the model rechecks anchors against the diff, relocates failing Criticals into the body, discards failing Suggestions, and degrades to an all-prose review rather than posting nothing. COMMENT reviews now always carry a one-line body: an empty body is only known to be accepted alongside inline comments on REQUEST_CHANGES, and a Suggestion- only review is the common case for a clean PR.
|
Thanks for the PR! Template looks good ✓ — all required sections present (What/Why/Test Plan/Tested On/Risk & Scope). "Evidence (Before & After)" section absent but this is a non-UI behavior change to the review posting pipeline, so N/A is appropriate. Problem: observed and well-articulated. The two costs of the updatable-summary design are concrete: (1) summary comments can never collapse — they stay permanently on the PR page even after all suggestions are addressed; (2) Markdown tables in issue comments don't render Direction: aligned. Routing both severities inline with a severity prefix is a clean simplification. GitHub's Outdated-collapse mechanism handles the convergence concern that originally motivated the summary. The reasoning in DESIGN.md is thorough and the trade-off analysis is honest about the residual edge cases (declined suggestions on unchanged lines, pattern-aggregated findings needing a representative anchor). Size: 10 files changed, +129/−619. Breakdown:
Under 500 production lines. Cross-package (cli + core) but both core files are documentation (DESIGN.md, SKILL.md) — no production code changes in Approach: the scope feels right. Every change traces back to the stated goal:
No drive-by refactors or scope creep. The change is net-negative in code (−490 lines) which is the right direction. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必要部分齐全（What/Why/Test Plan/Tested On/Risk & Scope）。缺少"Evidence (Before & After)"部分，但这是 review 发布管道的非 UI 行为变更，N/A 合理。 问题：已观测且论述充分。updatable-summary 设计的两个代价是具体的：(1) summary 评论永远无法折叠——即使所有建议都已处理，仍永久留在 PR 页面上；(2) issue 评论中的 Markdown 表格无法将 suggestion 块渲染为可一键应用的修改。两者都是可验证的 GitHub 平台行为，不是理论性担忧。 方向：对齐。将两种严重级别都通过 inline 发布并用前缀区分，是一个干净的简化。GitHub 的 Outdated 折叠机制处理了最初促使 summary 设计的收敛问题。DESIGN.md 中的推理充分，权衡分析对残余边缘情况（未修改行上被拒绝的建议、模式聚合发现需要代表性锚点）坦诚说明。 规模：10 个文件变更，+129/−619。明细：
生产行数低于 500。跨包（cli + core）但两个 core 文件都是文档（DESIGN.md、SKILL.md）——packages/core/src/** 无生产代码变更。 方案：范围合理。每项变更都追溯到声明的目标：
无顺手重构或范围蔓延。代码净减少（−490 行），方向正确。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code Review (2a)Independent proposal: I would have done the same thing — move Suggestions to inline comments with a severity prefix, delete the Findings: no critical blockers, no AGENTS.md violations. The implementation is clean and focused:
Reuse check: no new non-trivial logic duplicates existing utilities. The change is net-deletion. Testing (2b)Unit Tests (worktree, PR branch)TypecheckCLI Verification —
|
|
This PR is a clean reversal of a design decision that didn't work out. The original choice to route Suggestion findings to an updatable issue comment was well-intentioned (convergence over noise), but the two costs it paid — permanent comments that can never collapse, and suggestion blocks that can't carry one-click fixes — are worse than the problem they solved. GitHub's Outdated-collapse handles the convergence concern for free, and the severity prefix distinguishes blockers from recommendations without needing a separate channel. The implementation is what I'd want to see from a design-reversal PR: the old machinery is fully removed (post-suggestions subcommand, author-gated summary collection, "Previous suggestion summary" rendering section), the new path is simpler (one The A/B test on PR #6543 confirms the pr-context behavior change — the bot's legacy summary no longer appears in the "Previous suggestion summary" trusted section. The autofix filter correctly gates Suggestion findings against real data from PR #6535. All 9 unit tests pass, typecheck is clean. Net −490 lines of code. Simpler architecture. Honest trade-off documentation. Ship it. 中文说明这个 PR 干净地回退了一个没达到预期效果的设计决策。原始选择将 Suggestion 级别发现路由到可更新的 issue 评论是出于好意（收敛优于噪声），但它付出的两个代价——永远无法折叠的评论、无法携带一键修复的 suggestion 块——比它解决的问题更糟。GitHub 的 Outdated 折叠机制免费处理了收敛问题，严重级别前缀无需单独通道即可区分阻塞项和建议。 实现正是设计回退 PR 应有的样子：旧机制完全移除，新路径更简单，每项后续变更都关闭了真实缺口。 A/B 测试确认了 pr-context 行为变更。Autofix 过滤器在真实数据上正确门控。9 个单元测试全部通过，类型检查干净。 净减少 490 行代码。更简单的架构。诚实的权衡文档。可以合入。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| # actionable. jq's `^` is string-anchored, so tolerate leading whitespace | ||
| # the review model may emit; `**[Critical]**` can never match either way. | ||
| QWEN_SUGGESTION_FILTER='^[[:space:]]*\*\*\[Suggestion\]\*\*' | ||
| N_COMMENTS="$(jq --arg wm "${EFF_WM}" --arg rb "${REVIEW_BOT}" --arg ab "${AUTOFIX_BOT}" \ |
There was a problem hiding this comment.
[Suggestion] This filters the inline review-comment list, but a suggestion-only /review still leaves a COMMENTED review row, and N_REVIEWS above counts every COMMENTED review from the review bot. In that case the scan still sees N_REVIEWS=1 / N_COMMENTS=0, so the autofix job is queued and can burn one of its rounds even though the **[Suggestion]** comments were excluded. Please also exclude the suggestion-only /review body from N_REVIEWS, NEWEST, and the ## Reviews feedback render, or use a marker/body state the workflow can classify as non-actionable.
— GPT-5 via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
.github/workflows/qwen-autofix.yml:~1290 |
DRY violation — suggestion filter regex hardcoded inline instead of reusing QWEN_SUGGESTION_FILTER variable defined at line 1046 |
Replace the inline literal with --arg sf "${QWEN_SUGGESTION_FILTER}" |
scripts/tests/qwen-autofix-workflow.test.js |
No workflow test for the new jq Suggestion filter that excludes **[Suggestion]** comments from autofix |
Add assertions verifying the filter appears in both scan and rendering sections |
packages/cli/src/commands/review/pr-context.ts:84 |
isLegacySuggestionSummary name doesn't convey that it's an unguarded marker check (no author verification) |
Rename to hasLegacySuggestionSummaryMarker or add a @remarks JSDoc |
packages/core/src/skills/bundled/review/SKILL.md:~557 |
422 fallback relies entirely on prompt enforcement — no code-level anchor validation before gh api call |
Consider a pre-submit validation step in code that checks each (path, line) against the diff |
— qwen3.7-max via Qwen Code /review
| # actionable. jq's `^` is string-anchored, so tolerate leading whitespace | ||
| # the review model may emit; `**[Critical]**` can never match either way. | ||
| QWEN_SUGGESTION_FILTER='^[[:space:]]*\*\*\[Suggestion\]\*\*' | ||
| N_COMMENTS="$(jq --arg wm "${EFF_WM}" --arg rb "${REVIEW_BOT}" --arg ab "${AUTOFIX_BOT}" \ |
There was a problem hiding this comment.
One thing I think still needs to be covered here: the new filter removes /review Suggestion inline comments from N_COMMENTS, but a Suggestion-only review still submits a trusted COMMENTED review with the body Reviewed — no blockers. Suggestions are inline.
That means a bot PR with only non-blocking Suggestions still has N_REVIEWS=1, so review-scan will enqueue it for the autofix loop even though the inline Suggestion comments were filtered out. The same gap also affects the later NEWEST calculation / feedback.md rendering path, where COMMENTED reviews are treated as actionable review feedback.
Could we apply the same non-actionable Suggestion-only gate to the review-level channel as well, not just to inline comments?
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM overall. I left one non-blocking suggestion about keeping Suggestion-only review feedback out of the autofix loop; it shouldn't block this PR.
3 participants
What this PR does
/reviewposted Critical findings as inline review comments, while Suggestion findings were collected into a single updatable issue comment (the "suggestion summary", published by aqwen review post-suggestionssubcommand). This PR routes both severities to inline review comments, distinguished by a**[Critical]**/**[Suggestion]**body prefix, and removes the suggestion-summary machinery.Why it's needed
The split (introduced in #5786) optimized for convergence — one refreshable list instead of a growing pile of threads — but paid for it with two costs that dominate:
```suggestionfence as an applicable change only inside a review comment on a diff line; in an issue comment it degrades to a plain code block. Suggestion findings are exactly the mechanical, localized cleanups that benefit most from one-click apply, so the split withheld the feature from the findings that needed it most.The convergence concern is narrower than it looked: GitHub's Outdated-collapse handles every suggestion the author acts on.
DESIGN.mdrecords the reversal and the residual trade-off.Follow-on changes the reroute required
pr-context: legacy-summary exclusion is now marker-only, not author-gated. Old summaries must stay out of the "Already discussed — do NOT re-report" section, or they read as settled discussion and suppress still-open findings. The exclusion was gated oncurrentUser(), but/reviewruns under two identities — a maintainer locally, andqwen-code-ci-botin the review workflow. Scanning the last 60 PRs, ~45 carry a summary, split roughly evenly between the two. So a local/reviewon a bot-reviewed PR never excluded the bot's summary. Verified by driving the realpr-contexthandler against PR #6543 on both arms: onmainthe bot's summary lands under "Already discussed" and the run reports5 issue comments; with this change,4. The author check originally guarded promotion into the trusted "Previous suggestion summary" rendering section (a prompt-injection surface). That section is deleted, so the check only gated exclusion — where a third party embedding the marker merely hides their own comment, and cannot add it to anyone else's.qwen-autofix: the suggestion gate now covers the inline channel. The workflow'sBOT_COMMENT_FILTERtreats suggestion summaries as non-actionable, but applies only to the issue-comment channel. With Suggestions inline they entered the unfiltered inline channel, so the autofix bot would apply non-blocking recommendations and spend one of its 5 rounds on them. The inline count and thefeedback.mdrender now apply the same gate, keyed on the**[Suggestion]**prefix and the/reviewfooter, so a human quoting the prefix stays actionable.SKILL.mdrecords that the prefix and footer are load-bearing.Step 7 gains an HTTP 422 fallback.
Create Reviewis all-or-nothing, so a single Suggestion anchored outside the diff now 422s the whole review and takes the Critical blockers with it — a failure class that could not occur when Suggestions travelled on a line-agnostic issue comment. GitHub's 422 does not identify the offending entry, so the model rechecks each anchor against the diff (valid anywhere inside a hunk, including unchanged context lines), relocates failing Criticals into the body, discards failing Suggestions, and degrades to an all-prose review rather than posting nothing.COMMENTreviews always carry a one-line body. Removing the summary pointer leftevent=COMMENTwithbody: ""— the default outcome for a clean PR with suggestions only. Surveying real reviews on this repo, every batched review posted with an empty body isCHANGES_REQUESTED(e.g. review4653688208, 7 inline comments,bodylen=0); everyCOMMENTEDbatch carries a non-empty body. The empty-bodyCOMMENTEDreviews that do exist each carry exactly one inline comment — implicit reviews from standalone comment POSTs, notCreate Reviewcalls. So the combination is unexercised, and GitHub documentsbodyas required forCOMMENT. The rule now mandates a body rather than gambling on it.Reviewer Test Plan
How to verify
npx tsc --noEmit -p packages/cli/tsconfig.json— clean.npx vitest run --root packages/cli src/commands/review.test.ts src/commands/review/— 9 tests pass.pr-contextA/B on a PR carrying a bot-authored legacy summary (e.g. Stop repeated subagent tool-call loops #6543): onmainthe summary renders under "Already discussed"; on this branch it does not.pr-contextA/B on a PR carrying a maintainer-authored summary (e.g. feat(cli): improve subagent observability — untruncated live commands, transcript path, approval context #6580): onmainthe output contains a## Previous suggestion summaryheading; on this branch it does not.The autofix filter against real data — PR feat(scheduled-tasks): add isolated run mode via create_sub_session tool #6535 has 29
/reviewinline comments, 27**[Critical]**and 2**[Suggestion]**:Tested on
Linux, Node 22,
ghauthenticated. YAML parses (yaml.safe_load),prettier --checkclean on every touched file,shellcheckclean on the new shell/jq pattern.Not verified
Whether GitHub accepts
event=COMMENTwith an emptybodyand non-emptycommentswas not tested live — that requires posting a review to a real PR. The change takes the conservative path (always supply a body), which is correct either way.Risk & Scope
Behavior change to
/review's PR output and toqwen-autofix's actionable-work gate. No runtime API surface changes. Legacy suggestion-summary comments on open PRs remain recognised and filtered; no migration or cleanup is needed.中文版
这个 PR 做了什么
/review原先把 Critical 级发现作为 inline review comment 发布，而 Suggestion 级发现被汇总进一条可更新的 issue comment（"suggestion summary"，由qwen review post-suggestions子命令发布）。本 PR 让两种严重级别都走 inline review comment，通过正文前缀**[Critical]**/**[Suggestion]**区分，并移除 suggestion summary 的整套机制。为什么需要
这个分流（#5786 引入）是为了收敛性而优化的——用一份可刷新的列表取代不断堆积的评论线程——但它付出的两项代价更大：
```suggestion代码块渲染成可一键应用的改动；放进 issue comment 就退化成普通代码块。而 Suggestion 级发现恰恰是那些机械的、局部的清理——最受益于一键应用的一类。这个分流把这个能力从最需要它的发现上拿走了。至于当初担心的收敛性问题，其实比看上去窄得多：作者真正采纳的每一条建议，都会被 GitHub 的 Outdated 折叠机制处理掉。
DESIGN.md里记录了这次反转以及残留的取舍。因这次改动而必须做的连带修改
pr-context：遗留 summary 的排除改为只看 marker，不再校验作者。 旧的 summary 评论必须留在 "Already discussed — do NOT re-report" 之外，否则它会被当作"已经讨论过的事"，从而压制仍然开放的发现。原先这个排除是按currentUser()过滤作者的——但/review会以两种身份运行：本地是维护者，CI 里是qwen-code-ci-bot。扫描最近 60 个 PR，约 45 个带有 summary，两种身份各占大约一半。所以在本地对一个由 bot 评审过的 PR 跑/review，bot 的 summary 从未被排除。我用真实的pr-contexthandler 对 PR #6543 做了双臂验证：在main上，bot 的 summary 落在 "Already discussed" 之下，运行输出5 issue comments；应用本改动后是4。作者校验最初防的是把第三方评论提升进受信任的 "Previous suggestion summary" 渲染区（一个 prompt-injection 面）；那个区块已经删除，所以它现在只控制"排除"——而第三方在自己的评论里嵌入 marker，效果仅仅是把自己的文字藏起来，他无法把 marker 加到别人的评论上。qwen-autofix：建议过滤闸门现在覆盖 inline 通道。 该工作流的BOT_COMMENT_FILTER把 suggestion summary 视为"非可执行"，但它只作用于 issue-comment 通道。Suggestion 改走 inline 之后，它们进入了完全没有过滤的 inline 通道，于是 autofix bot 会去应用这些非阻断的建议，并消耗掉它 5 轮预算中的一轮。现在 inline 的计数与feedback.md的渲染都套用了同一个闸门，判据是**[Suggestion]**前缀加上/reviewfooter，这样人类引用了该前缀的评论仍然是可执行的。SKILL.md中已注明该前缀与 footer 是承重的（load-bearing）。Step 7 增加了 HTTP 422 兜底。
Create Review是原子的（all-or-nothing），因此只要有一条 Suggestion 锚定在 diff 之外，整份评审就会 422，把 Critical 阻断项一起带走——这在 Suggestion 走无行锚的 issue comment 时是不可能发生的一类失败。GitHub 的 422 不会指明是哪一条出错，所以模型需要自己拿每个锚点去比对 diff（只要落在某个 hunk 内即有效，包含未改动的上下文行），把失败的 Critical 移入 body，丢弃失败的 Suggestion，最终降级为纯散文评审，而不是什么都不发。COMMENT类评审现在恒带一行 body。 删掉那句指向 summary 的 pointer 之后，event=COMMENT就会配上body: ""——而这正是"干净 PR 只有建议"这一最常见情形的默认结果。我调查了本仓库真实发布过的评审：所有批量提交且 body 为空的评审都是CHANGES_REQUESTED（例如 review4653688208，7 条 inline 评论，bodylen=0）；而COMMENTED的批量评审 body 都非空。确实存在的那些空 body 的COMMENTED评审，每一条都只挂着 1 条 inline 评论——那是单条评论 POST 产生的隐式评审，不是Create Review调用。也就是说这个组合从未被实际跑过，而 GitHub 文档写明COMMENT的body是必填。因此规则改为强制带 body，而不是去赌它。Reviewer 验证方案
如何验证
npx tsc --noEmit -p packages/cli/tsconfig.json—— 干净。npx vitest run --root packages/cli src/commands/review.test.ts src/commands/review/—— 9 个测试通过。在一个带有 bot 发布的遗留 summary 的 PR（如 Stop repeated subagent tool-call loops #6543）上做
pr-contextA/B：main上该 summary 会渲染进 "Already discussed"；本分支不会。在一个带有 维护者发布的 summary 的 PR（如 feat(cli): improve subagent observability — untruncated live commands, transcript path, approval context #6580）上做
pr-contextA/B：main的输出含## Previous suggestion summary标题；本分支不含。用真实数据验证 autofix 过滤器 —— PR feat(scheduled-tasks): add isolated run mode via create_sub_session tool #6535 有 29 条
/reviewinline 评论，其中 27 条**[Critical]**、2 条**[Suggestion]**：测试环境
Linux，Node 22，
gh已认证。YAML 可解析（yaml.safe_load），所有改动文件prettier --check干净，新增的 shell/jq 片段shellcheck干净。未验证的部分
event=COMMENT搭配空body与非空comments究竟会不会被 GitHub 接受，没有实测——那需要往一个真实 PR 上发布评审。本改动选择了保守路径（恒带 body），无论真相如何都是正确的。风险与影响范围
改变了
/review在 PR 上的输出行为，以及qwen-autofix判定"可执行工作"的闸门。没有改动任何运行时 API 表面。开放 PR 上的遗留 suggestion summary 评论仍会被识别并过滤，无需迁移或清理。