fix(review): address post-merge P2 review feedback on the review skill#7708
Conversation
Follow-up to #7690 and #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.
|
Thanks for the follow-up! Template: headings differ from the repo template ( Problem: observed, not theoretical. These are six concrete P2 review comments from @yiliang114 on #7690 and #7691, both already merged. The degraded-path Direction: aligned. This hardens the review skill's failure edges and clarifies its documentation. No new features, no scope creep — each change maps 1:1 to a specific review comment. Size: 97 additions / 20 deletions across 5 files. Production logic (comment-status.ts + submit.ts): ~50 lines. Tests: ~55 lines. SKILL.md docs: ~9 lines. The only core-path touch is Approach: minimal and focused. Every edit is needed for the stated goal. The Moving on to code review. 🔍 中文说明感谢后续跟进! 模板:标题与仓库模板不同( 问题:已观测到的,非理论性的。这是 @yiliang114 在 #7690 和 #7691(均已合并)上留下的六条具体 P2 审查意见。降级路径的 方向:对齐。加固 review skill 的失败边界并澄清文档。无新功能、无范围蔓延——每项改动与具体审查意见一一对应。 规模:97 行新增 / 20 行删除,共 5 个文件。生产逻辑(comment-status.ts + submit.ts):约 50 行。测试:约 55 行。SKILL.md 文档:约 9 行。唯一触及核心路径的是 方案:最小且聚焦。每处编辑都是目标所必需的。 进入代码审查。🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal (from title + description, before reading the diff): add Comparison: the diff matches this proposal exactly. No simpler path missed. Walkthrough of each change:
No critical blockers. No AGENTS.md violations. Testing evidenceCI on
Windows/macOS tests and integration tests were skipped (not caused by this PR — standard matrix selection for this CI run). Real-scenario testing: N/A — all changes are internal to the review skill's command handlers and documentation. No user-visible TUI behavior changes. 中文说明代码审查独立方案(仅根据标题和描述,未看 diff):在接口上添加 对比: diff 与此方案完全一致。没有遗漏更简路径。 逐项审查:
无关键阻塞项。无 AGENTS.md 违规。 测试证据
真实场景测试: N/A——所有改动都在 review skill 的命令处理器和文档内部。无用户可见的 TUI 行为变化。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, focused follow-up; every change maps to a specific P2 review comment and the implementation is minimal and correct. Stepping back: the PR does exactly what it says — addresses six P2 review comments from two merged PRs in a single follow-up. The independent proposal matched the diff 1:1; no simpler path was missed. The The only nit: the PR body uses non-standard headings relative to the repo template. Content is all there, so this is cosmetic — noted in Stage 1, not blocking. CI is fully green on the reviewed commit. No user-visible behavior changes, so no real-scenario testing needed. Ready to ship. 中文说明信心:4/5 — 干净、聚焦的后续 PR;每项改动都对应一条具体的 P2 审查意见,实现最小且正确。 回顾:PR 完全如其所述——在一个后续 PR 中处理两个已合并 PR 的六条 P2 审查意见。独立方案与 diff 完全一致;没有遗漏更简路径。 唯一的小问题:PR 正文使用了与仓库模板不同的标题。内容都在,所以是格式问题——已在 Stage 1 中指出,不阻塞。 CI 在审查提交上全绿。无用户可见行为变化,无需真实场景测试。可以合并。 — Qwen Code · qwen3.8-max-preview Reviewed at |
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. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
|
@qwen-code /takeover |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下: Review feedback addressed[rc:3649965240]
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.7-max via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下: No changes made this round. The only feedback is an informational note from the automated reviewer The PR's changed behavior (comment-status race-detection fallback, degraded 中文说明本轮未做任何更改。 唯一的反馈是自动审查机器人(@qwen-code-ci-bot)的一条信息性说明,指出 本 PR 变更的行为(comment-status 竞态检测回退、降级时 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
yiliang114
left a comment
There was a problem hiding this comment.
Approve ✅
Clean follow-up addressing all 6 P2s from #7690 and #7691:
host?: stringon CommentStatusArgs — type-level contract visibility ✅- Race detection fallback to
liveHeadBefore— second head sample failure no longer discards comments ✅ prAuthor: nullin degraded report — distinguishable from legitimately absent author ✅atomicWriteFileSyncfor submit receipt — TOCTOU closed ✅- Lightweight mode guard in SKILL.md — explicit "skip in lightweight mode" ✅
- One-sentence write-ban summary leading the paragraph — survives context compression ✅
Every change directly maps to a P2 finding. Good test coverage for the race detection fallback and atomic write.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Summary
Follow-up to #7690 and #7691. Both merged with six P2 review comments from @yiliang114 still open — each valid but non-blocking, so they land here as a single follow-up rather than reopening the merged PRs. No behaviour changes on the happy path; every item hardens a failure edge or removes an ambiguity.
What changed
comment-statushost?: stringonCommentStatusArgs. The handler already readshostand callssetGhHostbeforerunCommentStatus; the interface now states that contract, so a future cleanup that moves thesetGhHostcall insiderunCommentStatusbecomes a type error instead of a silent host-drop.prAuthor: nullinstead of''.''is what the success path emits for a legitimately absent author (a deleted account), so a total index failure was indistinguishable from a real empty author.nullmatches whatworktreeHeadShaalready does in the same report.gh pr viewhead sample (race detection) now has its owntry/catchand falls back toliveHeadBefore. Previously a transient failure on that second call — after the comments were already fetched — threw the whole result away and wrote a fully degraded report, losing data already in hand. With the fallback,headMovedDuringFetchreadsfalse(the safe "head did not move" default) and the comments plus anchor facts survive.submit.tmpandrenameSyncd over the target.renameSyncis atomic on every filesystem Node supports, so a crash or interleaved read mid-write can never leave a truncated receipt — whichparseReceiptIdstolerates by returning[], silently dropping every accumulated review id and makingcleanupflag an earlier sanctioned review as a bypass.SKILL.mdcomment-statusguard is now keyed on worktree presence, not "the context file reports inline comments".pr-contextruns in lightweight mode too and reports existing inline comments there, so the old wording would send a lightweight run at a command that needs a worktree.ghcommand that writes to the pull request —qwen review submitis the only write path…"), with the enumeration kept as support beneath it, so the core rule survives even aggressive context compression.Testing
comment-status.handler.test.ts: added a case that fails the second head sample and asserts the comments survive (noerror,headMovedDuringFetch: false); the degraded case now also assertsprAuthor: null.submit.test.ts: added a case asserting the receipt is written atomically with no.tmpsibling left behind.vitest(review suites) +bundled-skills.integration.test.tsgreen;eslint --max-warnings 0,prettier, andtscclean forpackages/cliandpackages/core.中文说明
概述
#7690 与 #7691 的后续。这两个 PR 合并时,@yiliang114 留下的六条 P2 review 意见仍未处理——每一条都成立但非阻塞,因此归拢到这一个后续 PR,而不是重开已合并的 PR。正常路径行为不变;每一项都是加固某个失败边界或消除歧义。
改动内容
comment-statusCommentStatusArgs上声明host?: string。handler 本就会读取host并在runCommentStatus之前调用setGhHost;接口现在把这个契约写明,这样将来若把setGhHost调用挪进runCommentStatus,会变成类型错误,而不是悄悄丢掉 host。prAuthor: null而非''。成功路径在作者确实缺失(账号被删)时才输出'',因此原先整个索引失败与真实空作者无法区分。null与同一报告中worktreeHeadSha的既有做法一致。gh pr view采样(竞态检测)现在有自己的try/catch,失败时回退到liveHeadBefore。此前若这第二次调用在评论已取到之后发生瞬时失败,会把整个结果丢弃、写出完全降级的报告,丢掉手上已有的数据。加了回退后,headMovedDuringFetch读作false(安全的"head 未移动"默认值),评论与锚点事实得以保留。submit.tmp再renameSync覆盖目标。renameSync在 Node 支持的所有文件系统上都是原子的,因此写到一半崩溃或读写交错都不会留下截断的回执——而parseReceiptIds对截断 JSON 的容错是返回[],会悄悄丢掉所有已累积的 review id,导致cleanup把更早的合规 review 误判为绕过。SKILL.mdcomment-status守卫现在以是否存在 worktree 为判据,而非"上下文文件报告了 inline 评论"。pr-context在轻量模式下也会运行并报告已有 inline 评论,原措辞会让轻量模式的运行去调用一个需要 worktree 的命令。gh命令——qwen review submit是唯一的写入路径……"),枚举作为其下方的支撑,这样核心规则即使在激进的上下文压缩下也能存活。测试
comment-status.handler.test.ts:新增一条用例,让第二次 head 采样失败并断言评论得以保留(无error、headMovedDuringFetch: false);降级用例现在还断言prAuthor: null。submit.test.ts:新增一条用例,断言回执被原子写入且不留.tmp残留。vitest(review 套件)+bundled-skills.integration.test.ts全绿;packages/cli与packages/core的eslint --max-warnings 0、prettier、tsc均干净。