feat(review): add comment-status helper for existing-thread triage#7690
Merged
Conversation
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.
wenshao
requested review from
LaZzyMan,
tanzhenxin and
yiliang114
as code owners
July 24, 2026 17:57
Collaborator
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
|
Thanks for the PR! Template: the body covers everything the template asks for in substance — motivation with measured evidence, design rationale, and validation — though under different headings ("Why" / "Validation" instead of "Why it's needed" / "Reviewer Test Plan"). Not blocking, but worth aligning next time. Problem: this is a Direction: aligned. The review pipeline has been systematically moving deterministic classification out of the model loop and into tested subcommands ( Size: the core-path changes ( Approach: the scope feels right. One new subcommand with a pure classification core ( Moving on to code review. 🔍 中文说明感谢贡献！ 模板：PR 正文在实质上覆盖了模板要求的所有内容——有实测数据支撑的动机、设计理由、验证——只是标题不同（"Why" / "Validation" 而非 "Why it's needed" / "Reviewer Test Plan"）。不阻塞，但下次建议对齐。 问题：这是一个 方向：对齐。review 管线一直在系统性地将确定性分类从模型循环中移出，放入经过测试的子命令（ 规模：核心路径变更（ 方案：范围合理。一个新子命令，纯分类核心（ 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Independent proposal: I'd do exactly what this PR does — a new Code review findings: no blockers. The implementation is clean and well-structured. A few things worth calling out as done well:
The Testing: this is a CI run (
Ubuntu unit tests pass. macOS/Windows and integration tests are skipped (standard for this repo's CI configuration). No failures. The PR carries 11 unit tests (pure core), 7 integration tests (real git probe — including the subdirectory CWD regression, the cap, the memo, the missing-commit gate, the non-ancestor gate, and the literal-pathspec guard), and 9 handler tests (drift, warnings, Not verified: TUI-level behavior — this is a CLI subcommand consumed by the 中文说明独立方案： 我会做和这个 PR 完全一样的事——在 代码审查发现： 无阻塞项。 实现干净、结构良好。几个做得好的地方：
测试：PR 自带 11 个单测、7 个集成测试、9 个 handler 测试。CI 绿（Ubuntu 单测通过）。 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 4/5 — clean implementation that does exactly what it sets out to do; the only nit is the template heading mismatch, which is non-blocking. This PR moves factual, deterministic comment-status classification out of the model loop and into a single subcommand call — the same trade the pipeline has been making everywhere else ( The architecture is right: pure classification core with an injected probe, git/gh glue only in the handler, and shared functions ( The 4 commits since the last review added graceful degradation — the handler now catches any runtime failure (auth, network, gh) and writes a same-shape empty report with safe neutral defaults, fulfilling SKILL.md's promise that a failed CI is green on the reviewed commit. No pending workflow runs. Approving. ✅ 中文说明置信度：4/5 —— 干净的实现，完全达成了目标；唯一的小问题是模板标题不匹配，不阻塞。 这个 PR 将事实性的、确定性的评论状态分类从模型循环中移出，放入单个子命令调用——与管线其他地方一直在做的权衡一致。动机有具体实测数据（20+ 次逐条拉取 → 1 次调用），不是推测。 架构正确：纯分类核心 + 注入式 probe，git/gh 胶水仅在 handler 中，共享函数从 上次审查后的 4 个提交增加了优雅降级——handler 现在捕获任何运行时失败（认证、网络、gh），输出同形空报告（安全中性默认值），兑现 SKILL.md 的承诺。降级报告设计仔细： CI 在审查提交上全绿。无待处理的工作流运行。 批准。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
requested changes
Jul 24, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
One fix needed: add 'comment-status' to the expected subcommand list in packages/cli/src/commands/review.test.ts:41 — the registry test asserts the exact list and CI is red without it. Everything else looks great, see notes above. 🙏
Collaborator
Author
|
Fixed in d098e4f — added |
Collaborator
Author
Review —
|
| report size | 53 153 chars |
| threads visible in the first 25 000 chars | 35 of 71 |
| threads past the cut | 36 |
of those, isBlocker: true |
24 |
threads is sorted by path, so the loss is not random — every thread in the alphabetically-later files disappears wholesale (.../mention.ts, channel-registry.ts, scripts/build.js all vanish entirely). And because it is JSON, the truncated read is not merely incomplete but syntactically invalid, cut mid-object.
This is the same failure pr-context already fixed for itself. It emits (pr-context.ts:885):
if (md.length > DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD) {
writeStdoutLine(`warning: ${out} is ${md.length} chars; read_file returns the first …`);…and SKILL.md:129 — the paragraph immediately above the new block — is a bolded instruction to page it. The new command has neither. The whole rationale for that warning ("a reader that stops at the threshold loses the tail in silence") applies here with more force, since the index exists to make sure no blocker goes unrouted.
Suggested fix — mirror pr-context exactly:
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. Page the rest ` +
`with offset/limit, or query it with jq, before reasoning about it.`,
);
}plus a sentence in the SKILL.md block. Note that dropping the pretty-print only gets to 36 726 chars — still over — so a warning (and ideally a jq-first instruction, since this file is machine-shaped in a way the Markdown context file is not) is the real fix, not slimming.
🟠 Medium — --host is implemented but never threaded through, so GitHub Enterprise runs silently query github.com
The command correctly accepts --host and calls setGhHost (comment-status.ts:595). But:
- The SKILL.md invocation at
SKILL.md:134-135does not pass--host <host>. SKILL.md:78still enumerates onlyfetch-pr,pr-context, andpresubmitas the subcommands that must receive it. So doesdocs/users/features/code-review.md:276.
setGhHost is per-process module state and each subcommand is a separate qwen review … invocation, so nothing else can supply it. A GHE PR reaching worktree mode (matching remote → yes, it does) therefore runs comment-status against github.com. If github.com happens to host a same-named owner/repo with that PR number, the review gets a status index for an entirely different pull request, with no error — which is verbatim the failure SKILL.md:78 exists to prevent ("a dropped host makes a call read from and post to the wrong site's owner/repo"). If it does not, the call 404s and the skill's own "warn and continue" rule swallows it.
Fix: add --host <host> to the command block (guarded like the others), and add comment-status to the list at SKILL.md:78 and in docs/users/features/code-review.md:276.
🟡 Minor — Step 6's new sentence is unconditional, but the report is conditional and narrower than the re-check's scope
Two mismatches in the SKILL.md:623 addition:
- It can be absent. The Step 1 block is gated on worktree mode, and its own failure rule is "warn and continue". Step 6 nevertheless says, without qualification, to "read Step 1's
comment-statusreport instead of fetching per-comment metadata". A lightweight-mode run still performs the Step 6 re-check (it is not the context-unavailable state —pr-contextsucceeds cross-repo) and will look for a file that was never written. Worth a clause like "when Step 1 produced it; otherwise re-derive as before". - It covers only inline comments. The report is built solely from
pulls/{n}/comments. Step 6's re-check scope is explicitly broader — "Open inline comments", "Blockers to re-check", "Review summaries", and "Already discussed (both its inline threads and its issue-level comments)". An issue-level blocker — the feat(cli): Add model toggle hotkey (Ctrl+F) #6486 case the surrounding prose is built around — has no entry here at all, and neither does a review-level summary. The sentence's "the anchor" hints at inline-only, but an agent walking the full blocker set will find nothing for those and the text does not say why. One clause scoping it to inline threads would close this.
🟡 Minor — touchedBy truncation is silent
TOUCHED_BY_CAP = 10 slices without recording that it did. On #7632, 31 of 71 threads sit at exactly 10 — I checked the underlying git log for the two distinct (path, sinceSha) pairs behind them and the real counts are also exactly 10, so nothing was actually lost there, but the ceiling is being touched on the very first real PR. Since git log is newest-first the retained slice is the right half, and SKILL.md calls this list "the candidate 'fixed by' commits" — a reader who cannot tell the list was cut may conclude the fix is not among them. A touchedByTruncated: boolean (or a touchedByTotal count) costs one line and makes the cut visible.
🔵 Nits
sideis declared and never read (comment-status.ts:41). Either drop it or use it — an unused field in an interface that documents the API surface invites the reader to think it matters.- Ghost-author false positive.
prAuthorfalls back to''(comment-status.ts:518) andauthorRepliedcompares(r.user?.login ?? '').toLowerCase() === authorLc. If the PR author is a deleted account (author: null) and a reply is from a deleted account (user: null),'' === ''reportsauthorReplied: true.participantsuses'unknown'for the same missing value — worth making the two fallbacks agree, and short-circuiting onauthorLc === ''. - Force-push degradation is documented more optimistically than it behaves. The doc comment says a force-pushed-away commit yields
'unknown', and that holds when the object is gone. But a worktree shares the main clone's object database, so on a re-review the previous head is often still present —cat-file -esucceeds, andA..HEADthen spans the whole re-pushed branch rather than the intended range. That is the fail-safe direction (over-reportschanged: true; it can never hide a change, since under-reporting would require the fix to be reachable from the comment's own commit), so it is a comment-accuracy nit, not a defect.
Verdict: the mechanism is sound and I could not break the classification core. The High finding is the one I would fix before merge — the index silently losing 24 of its blocker-flagged threads to a read_file cut defeats the purpose it was built for, and the repo already has the exact fix pattern one function over.
中文说明
在 d098e4feb 上完成审阅（审阅期间 head 从 324a3c1fd 前移；后续提交已修复当时唯一的 CI 红灯）。
实测：独立 worktree 中 review/ 全量 32 文件 804 测试绿、review.test.ts 5/5 绿、eslint 干净；在 #7632 真实 worktree 端到端复现作者数据（74→71 线程、40 outdated、61 changed、2 unknown），抽查评论 3643747580 结果一致；另用 GraphQL isOutdated 独立验证 line === null ⇒ outdated 前提，71/71 完全一致（顺带确认 position 在这 42 条上均非 null，用废弃字段会判错——本 PR 的选择是对的）；失败路径与零评论 PR 均正常。
结论：机制可靠，分类核心未能被证伪。
- ✅ 已在审阅期间修复：
review.test.ts:41的子命令清单断言未更新，324a3c1fd上 CI 红（唯一一个失败）。PR 描述里"src/commands/review/全绿"没错，但断言在上一层目录src/commands/。d098e4feb已修。 - 🔴 High：报告体积超出
read_file截断阈值且无警告。 在本功能对标的那个 PR 上，报告为 53 153 字符，是 25 000 阈值的两倍多。实测：一次read_file只能看到 71 个线程中的 35 个，36 个被截断，其中 24 个isBlocker: true；threads按 path 排序，靠后的文件整片消失，且 JSON 被拦腰截断后不可解析。pr-context.ts:885已有完全对应的 size 警告，SKILL.md:129（就在新增块正上方）还有整段分页指示——新命令两样都没有。紧凑输出也仍有 36 726 字符，所以正解是补警告 + 建议用jq查询，而非压缩体积。 - 🟠 Medium：
--host实现了却没接上，GHE 场景静默打到 github.com。 命令本身支持--host，但SKILL.md:134-135的调用没传，SKILL.md:78与docs/users/features/code-review.md:276的名单也没加comment-status。setGhHost是进程内状态、每个子命令是独立进程，无从继承。GHE PR 走 worktree 模式会命中——若 github.com 上恰有同名owner/repo且 PR 号存在，就会拿到另一个 PR 的状态索引且无任何报错，正是SKILL.md:78明写要防的失效模式。 - 🟡 Minor：Step 6 新增句子是无条件的，而报告是有条件且范围更窄的。 其一，Step 1 只在 worktree 模式下跑、失败时"警告并继续"，但 Step 6 无条件要求"读 Step 1 的报告"——lightweight 模式仍会执行 re-check，却根本没有这个文件。其二，报告只取
pulls/{n}/comments，而 Step 6 的 re-check 范围明确包含 issue 级评论与 review 级正文（feat(cli): Add model toggle hotkey (Ctrl+F) #6486 那个案例正是 issue 级），这些在报告里完全没有条目，文中未说明。 - 🟡 Minor：
touchedBy截断无提示。TOUCHED_BY_CAP = 10静默切片。feat(channels): GitHub polling adapter with notification-as-wakeup architecture #7632 上 71 个线程里有 31 个正好卡在 10（我核对了背后两组(path, sinceSha)的真实git log，恰好也是 10，本次未真丢），但天花板已被触到。加一个touchedByTruncated或touchedByTotal即可。 - 🔵 Nits：
side声明后从未使用；PR 作者与回复者同为已注销账号时'' === ''会误报authorReplied: true（且与participants的'unknown'兜底不一致）；force-push 后旧对象常仍在共享对象库中，此时不是文档所说的'unknown'而是范围放大（属 fail-safe 方向，仅注释与实际行为的偏差）。
建议合并前先处理 High 那条——一个为"不漏 blocker"而生的索引，却在真实 PR 上静默丢掉 24 个 blocker 线程；而修复范式就在隔壁函数里。
…wiring, scope clauses Addresses the review at d098e4f: - 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).
Collaborator
Author
|
All addressed in 7c899d1 — thanks for the depth here, especially the GraphQL cross-validation of the
One deliberate non-change: kept pretty-printed JSON. Compact still exceeds the threshold (your 36 726 measurement), so the warning is the real fix and readability wins for the paging case. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
wenshao
commented
Jul 24, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
中文说明
— qwen3.8-max-preview via Qwen Code /review
Collaborator
Author
Review — LGTM, no blockersThe design is right: anchor validity, code drift, reply participation and blocker signal are all facts, and pulling them out of the orchestrator's turn budget into one deterministic pass is exactly the trade this pipeline has been making everywhere else. The pure-core + injected-probe split keeps the tests honest (no Verification I ranFresh worktree at
Suggestions — none blocking1. This is the one I'd fix before merge, because its failure mode is silent and points the wrong way. Reproduced in a scratch repo: run the same call from a subdirectory of the worktree and
Every thread then reports 2. The report still emits 3. Character-for-character identical, including the cycle guard. The PR argues — correctly — that reusing 4. Second full fetch of the same endpoint
5. No REST has no thread-resolution state, so the index can't carry the single strongest "a human closed this out" signal — 6. The pure core has 12; the git glue — memo key, the 7. Minor
What I liked
中文说明评审 — LGTM，无阻塞项设计方向是对的：锚点有效性、代码漂移、回复参与、blocker 判定，这些都是事实，把它们从编排模型的轮次预算里抽出来变成一次确定性扫描，正是这条流水线一直在做的取舍。纯核心 + 注入式 probe 的切分让测试保持诚实（不 spawn 我做的验证在
建议 — 均非阻塞1. 这一条我会在合并前修掉，因为它的失效模式既静默、方向又不利。已在临时仓库复现：从 worktree 的子目录里跑同样的调用，
于是每个线程都报 2. 报告仍以完全置信度输出 3. 逐字符一致，连防环守卫都一样。本 PR 的论证是对的：复用 4. 同一端点被完整拉取两次
5. 缺 REST 没有线程解决状态，所以索引无法携带"人已把它关掉"这个最强信号—— 6. 纯核心有 12 个测试；git 胶水——memo key、 7. 细节
我欣赏的部分
🤖 Generated with Claude Code — Claude Opus 5 (1M context) |
…ec, shared walk, stale flag
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).
Collaborator
Author
|
Second round addressed in e65860d. Suggestion 1 was the catch of the day — thanks for actually reproducing the subdirectory case:
|
wenshao
commented
Jul 24, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex GPT-5 via Qwen Code /review
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 <worktreePath>`, 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)<path>` 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).
Collaborator
Author
|
Acknowledging the latest review — no blockers, thanks. The accompanying "not reviewed / no agent launched with the built prompt" lines are the reviewing run reporting on its own harness (its agents ran from hand-built prompts rather than the CLI-composed ones), not findings on this diff, so there's nothing to change here. All earlier inline suggestions on this PR were fixed and replied to individually. |
…ILL.md contract
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.
… 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.
doudouOUC
reviewed
Jul 25, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
…ot a stripped one
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.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
…rktreeHeadSha: null 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.
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 25, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
approved these changes
Jul 25, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
yiliang114
reviewed
Jul 25, 2026
| }; | ||
| } | ||
|
|
||
| interface CommentStatusArgs { |
Collaborator
There was a problem hiding this comment.
P2 — CommentStatusArgs interface is missing the host field
The yargs builder below declares .option('host', …) and the handler reads it via (argv as { host?: string }).host, but the CommentStatusArgs interface only lists pr_number, owner_repo, and out. The handler then casts argv as unknown as CommentStatusArgs, which silently drops the host property from the type's perspective.
This works at runtime because setGhHost is called before runCommentStatus, but the type erasure means a future refactor that moves the setGhHost call inside runCommentStatus (a reasonable cleanup) would silently lose the host — TypeScript would not flag the missing field.
Suggestion: add host?: string to the interface so the full contract is visible at the type level:
interface CommentStatusArgs {
pr_number: string;
owner_repo: string;
out: string;
host?: string;
}
Collaborator
Author
There was a problem hiding this comment.
Good call on making the contract visible at the type level — done in #7708: CommentStatusArgs now declares host?: string, so a later cleanup that moves the setGhHost call into runCommentStatus would be a type error rather than a silent host-drop.
yiliang114
reviewed
Jul 25, 2026
| prNumber, | ||
| ownerRepo, | ||
| error: msg, | ||
| prAuthor: '', |
Collaborator
There was a problem hiding this comment.
P2 — Degraded report's prAuthor: '' is indistinguishable from a legitimately empty author
When the entire command fails (auth, network), the degraded report writes prAuthor: ''. The success path also produces prAuthor: '' when the PR author account is deleted (meta.author?.login ?? ''). A downstream consumer that reads prAuthor without checking error first cannot tell whether the PR author is unknown or the index itself is broken.
The error field exists and the comments correctly note consumers should check it, but the design goal is that "a consumer that reads a fact gets a neutral value, never a misleading one." An empty string for prAuthor violates that for any consumer displaying the author name.
Consider using null (which is what worktreeHeadSha already does in this same degraded report) or omitting the field entirely, so the absence is structurally visible:
prAuthor: null, // or omit — signals "unknown due to failure"
Collaborator
Author
There was a problem hiding this comment.
Agreed. #7708 changes the degraded report to prAuthor: null (matching what worktreeHeadSha already does in the same report), so a total index failure is no longer indistinguishable from a legitimately absent/deleted author.
yiliang114
reviewed
Jul 25, 2026
| const prAuthor = meta.author?.login ?? ''; | ||
| const liveHeadBefore = meta.headRefOid ?? ''; | ||
|
|
||
| const comments = ghApiAll( |
Collaborator
There was a problem hiding this comment.
P2 — Second gh pr view failure discards already-fetched comments
The second gh pr view call (for liveHeadAfter) is inside the same try/catch as the comments fetch. If this call fails transiently (rate limit, network blip) after the comments were successfully fetched, the entire result is thrown away and the catch block writes a fully degraded empty report — losing the comments data that was already in hand.
The liveHeadAfter is a race-detection enhancement, not a core requirement. A more resilient approach would be to wrap just this second sample in its own try/catch, falling back to liveHeadBefore (which still gives a usable drift comparison):
let liveHeadAfter = liveHeadBefore;
try {
liveHeadAfter = (JSON.parse(gh('pr', 'view', …)) as …).headRefOid ?? liveHeadBefore;
} catch {
// Race detection degraded; use the pre-fetch sample. headMovedDuringFetch
// will read false, which is the safe default (assumes the head did not move).
}This keeps the comments + anchor facts available even when the race-detection sample fails.
Collaborator
Author
There was a problem hiding this comment.
Fixed in #7708 — the second head sample now sits in its own try/catch and falls back to liveHeadBefore. A transient failure after the comments were already fetched keeps them (and the anchor facts) instead of discarding everything into a degraded report; headMovedDuringFetch then reads false, the safe "head did not move" default.
yiliang114
reviewed
Jul 25, 2026
|
|
||
| **`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 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.) |
Collaborator
There was a problem hiding this comment.
P2 — Lightweight-mode model may attempt comment-status based on this instruction
The parenthetical "(worktree mode, whenever the context file reports existing inline comments)" is the only guard telling the model not to run comment-status in lightweight mode. However, pr-context does run in lightweight mode (it's pure GitHub API) and does report existing inline comments — so a literal model following this instruction would see "context file reports existing inline comments" and attempt to run comment-status, which needs a worktree.
The Step 6 re-check section already handles this correctly ("the report exists only when Step 1 wrote it"), but Step 1's instruction should be explicit to prevent a wasted invocation. Consider a more direct guard:
Fetch the comment STATUS index (worktree mode only — skip in lightweight mode where no worktree exists). Run whenever the context file reports existing inline comments.
Collaborator
Author
There was a problem hiding this comment.
Right — the guard was ambiguous precisely because pr-context reports existing inline comments in lightweight mode too. #7708 re-keys the Step 1 guard on worktree presence and says explicitly to skip comment-status in lightweight mode, so a literal reading no longer sends a lightweight run at a command that needs a worktree.
yiliang114
reviewed
Jul 25, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
Well-structured addition — the pure-core / integration / handler test split is clean, the findRootId generalization to share thread-grouping between pr-context and comment-status is the right call, and the git probe's ancestry gate + :(top,literal) pathspec hardening against injection are both solid. The degraded-report shape-preserving design (same schema + error field) is thoughtful.
Findings (all P2, none blocking)
| # | File | Issue |
|---|---|---|
| 1 | comment-status.ts:306 |
CommentStatusArgs interface missing host field — type erasure risk for future refactors |
| 2 | comment-status.ts:481 |
Degraded report's prAuthor: '' is indistinguishable from a legitimately deleted-account author — should be null |
| 3 | comment-status.ts:346 |
Second gh pr view (race detection) is in the same try/catch as the core fetch — a transient failure there discards successfully-fetched comments |
| 4 | SKILL.md:131 |
Lightweight-mode model may attempt comment-status since pr-context (which runs in lightweight mode) reports inline comments — needs an explicit "skip in lightweight mode" guard |
What looks good
- Test coverage is thorough: 230 lines of pure-core tests (thread grouping, anchor classification, blocker signal, negation, ghost-author guard), 140 lines of real-git integration tests (CWD scoping, pathspec magic, ancestry, cap), and 267 lines of handler wiring tests (drift, worktree, auth degradation, size warning).
- The git probe is well-designed: memoization by
(path, sha), separate ancestry memo,git -Cscoping, and the:(top,literal)prefix against pathspec injection. - Two-sample head race detection is a clever idea, well-documented with the rationale for why it exists.
- The
findRootIdgeneralization fromRawCommentto a generic{ id; in_reply_to_id }is clean and avoids code duplication betweenpr-contextandcomment-status.
No P0 or P1 issues. LGTM after the P2 items are addressed (or acknowledged as acceptable trade-offs).
yiliang114
approved these changes
Jul 25, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Approve ✅
Clean read-only helper that eliminates 20+ redundant gh api calls per review run. Strong test coverage (637 lines, 3 test files), well-designed git probe and two-sample head race detection. Four non-blocking P2s posted inline (host field typing, degraded report null vs empty, race detection fallback, lightweight mode guard).
pull Bot
pushed a commit
to bit-cook/qwen-code
that referenced
this pull request
Jul 25, 2026
QwenLM#7708) * fix(review): address post-merge P2 review feedback on the review skill Follow-up to QwenLM#7690 and QwenLM#7691, picking up six P2 review comments that landed just before those PRs merged: - comment-status: declare `host` on CommentStatusArgs so a future refactor that drops it becomes a type error, not a silent runtime regression. - comment-status: the degraded report now emits `prAuthor: null` (matching `worktreeHeadSha`) instead of '', so a total index failure is distinguishable from a legitimately absent (deleted) PR author. - comment-status: wrap the second head sample in its own try/catch and fall back to liveHeadBefore, so a transient failure after the comments are already fetched no longer discards them into a degraded report. - submit: write the audit receipt to a sibling tmp then renameSync over the target, so a crash mid-write can never leave a truncated receipt that parseReceiptIds reads as [] and drops every accumulated review id. - SKILL.md: the Step 1 comment-status guard is now worktree presence, not "the context file reports inline comments" — pr-context reports those in lightweight mode too, where no worktree exists. - SKILL.md: lead the Step 7 write-ban with a single compression-proof sentence; the enumeration stays as support beneath it. * fix(review): use atomicWriteFileSync for submit receipt (QwenLM#7708) --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds a
qwen review comment-statussubcommand: one deterministic pass over a PR's existing inline comments that emits a per-thread status index, and wires it into the/reviewskill (Step 1 fetch + Step 6 re-check routing).Why
The questions this report answers used to be re-derived by the orchestrating model one
gh apicall at a time. Measured on a heavily discussed PR (#7632, 72+ inline comments), a single/reviewrun made 20+ individualgh api repos/…/pulls/comments/<id>fetches — each one a whole model turn — asking only factual questions: is this comment outdated? did the code move since it was filed? did anyone answer it? Those are facts, not judgments, and a subcommand can answer all of them in one call.What the report carries (per thread)
line: nullon a line-scoped comment means GitHub can no longer map it to the latest diff (outdated); file-level comments are exempt.code.touchedBy, capped at 10 — the candidate "fixed by" list the Step 6 re-check needs).pr-context), participants, and whether the PR author replied (seen ≠ fixed; the field is documented as such).carriesBlockerSignalfrompr-context, so the two surfaces agree by construction.Design notes
buildThreadStatuses) with an injectedCodeChangeProbe; the git/gh glue lives only in the command handler. Unit tests exercise the core directly — noghspawns.(path, sinceSha)and degrades to'unknown'when the question is unanswerable (no worktree, the comment's commit force-pushed away).pr-contextfile under its untrusted-data preamble, with per-comment fetch refs only for bodies the renderer truncated.comment-statuswarns and continues, and never sets the context-unavailable state.Validation
src/commands/review/suite green (32 files, 804 tests);eslint --max-warnings 0andtsc --noEmitclean.57a9273on line 80) reports re-anchoredline: 96, not outdated,touchedBy: [08ede56, 9edc953, c0a7e95]— exactly the commits pushed after it, including the one that fixed it.中文说明
本 PR 做了什么
新增
qwen review comment-status子命令：对 PR 的存量 inline 评论做一次确定性扫描，输出按线程组织的状态索引，并接入/reviewskill（Step 1 获取 + Step 6 re-check 路由）。动机
这份报告回答的问题，过去由编排模型逐条
gh api调用重复推导。实测一个讨论密集的 PR（#7632，72+ 条 inline 评论），单次/review运行发起了 20+ 次gh api repos/…/pulls/comments/<id>单条拉取——每次都是一个完整的模型轮次——问的只是事实性问题：这条评论 outdated 了吗？评论之后代码动过吗？有人回复过吗？这些是事实而非判断，一个子命令一次就能全部答完。报告内容（每线程）
line: null即无法映射到最新 diff（outdated）；file 级评论豁免。code.touchedBy，上限 10 条——正是 Step 6 re-check 需要的"候选修复提交"清单）。pr-context一致）、参与者、以及 PR 作者是否回复过（"已看到"≠"已修复"，字段注释如此说明）。pr-context的carriesBlockerSignal，两个表面天然一致。设计说明
buildThreadStatuses）+ 注入式CodeChangeProbe；git/gh 胶水只在命令 handler 里。单测直接测核心，不 spawngh。(path, sinceSha)记忆化；无法回答时（无 worktree、评论所在提交被 force-push 冲掉）降级为'unknown'。pr-context文件里（带不可信数据前言），只有被截断的正文才按其 fetch ref 单独获取。comment-status失败只警告并继续，不触发 context-unavailable 状态。验证
src/commands/review/全量绿（32 文件、804 测试）；eslint --max-warnings 0与tsc --noEmit干净。57a9273提交在第 80 行）报告重锚为line: 96、非 outdated、touchedBy: [08ede56, 9edc953, c0a7e95]——恰为其后推送的全部提交，含真正修复它的那个。