refactor(review): split SKILL.md into a core body plus verdict-gated reference files - #9804
Conversation
…ferences (#9787) The bundled review skill's SKILL.md (381,984 bytes, ~95k tokens) was injected whole on every run, although large stretches are conditional territory a given run never touches. Split it into a core body plus reference files the orchestrator reads on demand, gated on the parse-args verdict it already holds: - references/posting.md — whole Step 7 (authorisation gate, presubmit, anchors, submit, 422/head-drift recovery, publish-assets). Loaded only when posting is live (comment.effective or a same-session post request; PR + high only). Its compose-state field list relocates verbatim to Step 6's Verdict section, because a report-only run still writes that state for compose-review without ever loading posting.md. - references/persistence.md — whole Step 8 (tail batching, report, artifact registration, incremental cache). Loaded before Step 8 on every run except cross-repo lightweight mode. - references/aone.md — the self-contained Aone blocks of Step 1 (clone and two-host rules, a1-backed surface, the five submit failure shapes, dedup shape notes). Loaded before match-remote when the host/meta says Aone. The split moves whole steps; incident-backed rules stay with the step they guard. The write prohibition and the posting gates remain in the injected core so they bind runs that never load a file. No enterprise.md: the GHE host notes are sentences woven into universal paragraphs, and extracting them would strip rules from steps that remain in core. Injected prompt: 381,125 -> 304,427 body bytes. Typical non-posting runs (local/file/PR, any effort) save ~58 KB (~15%); lightweight runs ~77 KB (~20%); posting runs load posting.md back and save only the Aone block. The issue's "roughly a third" estimate is unreachable under its own whole-step guardrail — Steps 1 and 6 dominate the core and interleaving forbids fragmenting them; Step 5 / Step 3C effort-gated splits are the natural follow-up. Drive-by, verified against #9627's revert-guard test and the a1 implementation: three stale sentences still claiming comment-status "has no Aone backing" are aligned with the a1-backed behavior that landed in #9627. Tests: SKILL.test.ts revert guards now govern the full corpus (SKILL.md + references), with new pins for the gates, the core-retained invariants and the no-duplication invariant; run-skill-parity reads the corpus oracle; bundled-skills integration pins the shipped reference files. Verified by build + bundle, all review-skill suites, and a real-model E2E run of the split skill (verdict-gated reads observed: persistence.md loaded before Step 8, posting.md and aone.md correctly skipped).
github-actions
Bot
added
the
review/self-reported
E2E verification reportStatus: VERIFIED_FIXED — real-model run against the fresh bundle ( Headless run from the split files (scratch repo,
|
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed and measured, not theoretical — issue #9787 documents the bundled review skill injecting 381,984 bytes (~95k tokens) whole on every run, ~19× the next-largest bundled skill, with large stretches (posting protocol, persistence, Aone paths) being conditional territory a given run never touches. Direction: aligned. Progressive disclosure via Size: core paths touched ( Approach: the scope feels right — whole-step moves gated by the verdict the run already holds, matching the guardrail the issue itself sets ("move whole steps, never strip rules from steps that stay in core"). The deviation from the issue's "~one third" estimate is honest: actual injection drops ≈15–20% per run because Steps 1 and 6 dominate the core, and fragmenting them would strip rules from steps that remain. Two small carry-alongs, both justified: the compose-state field list relocates from the posting step to the verdict section so report-only runs don't owe the posting file a read, and one verified drive-by aligns three stale Aone comment-status sentences with the a1-backed behavior that already landed in #9627. Risk: no changed file matches the high-risk paths from the revert-history analysis. No elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测且有实测数据，不是理论性问题——issue #9787 记录了内置 review 技能每次运行全量注入 381,984 字节（约 9.5 万 token），约为第二大内置技能的 19 倍，其中大段内容（发布协议、持久化、Aone 路径）是特定运行根本不会触及的条件性内容。 方向： 对齐。 规模： 触及核心路径（ 方案： 范围合理——按运行已持有的 verdict 整步搬迁，与 issue 自设的护栏一致（"只搬整步、不把规则从留在核心的步骤中剥离"）。对 issue 中"约三分之一"估计的偏离是诚实的：由于 Step 1 与 Step 6 占据核心主体，拆散它们会把规则从留在核心的步骤中剥离，实际每次运行注入量下降约 15–20%。两处小的附带改动均有理由：compose 状态字段表从发布步骤搬到裁决小节，使仅出报告的运行无需读取发布文件；以及一处已核实的顺带修复，把三处过时的 Aone comment-status 表述与 #9627 已落地的 a1 支持行为对齐。 风险： 无变更文件命中 revert 历史分析得出的高风险路径。无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe approach is the one I'd have proposed from the issue alone — whole-step moves gated by the verdict the run already holds, gates living in the injected core — and the execution is faithful. Three things I checked mechanically rather than by eye: The move is verbatim. A line-level set comparison of the diff: all 393 removed lines reappear byte-identical in the three reference files, except exactly seven documented rewordings — the three stale Aone/comment-status sentences (below), the Cross-references still resolve. The step headings stay in the core (pinned by the new tests), so every "Step 7"/"Step 8" reference in the corpus lands on the pointer that forwards. The old "the skip list in the Aone section above" phrasing appears only on the removed side — no dangling reference survives. The compose-state field list moved from Step 7 into Step 6's verdict section with a single-copy pin ( The build needs no change, and got none. The asset copier is recursive (skipping only test files and DESIGN.md), and both sides of the stale-bundle digest walk the whole review-skill directory with an The drive-by holds up. The three removed sentences claimed The revert guards now govern the full corpus via TestingCI evidence via the API at the reviewed commit — nothing was built or run locally (unattended run). The macOS/Windows/integration skips are by design: those jobs are gated to Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified here: live behaviour — that a real run actually fires the verdict-gated reads (persistence loaded before Step 8, posting/Aone skipped when not applicable). The PR description reports a headless E2E observing exactly this; that is the author's claim, not independently re-run in this pass. The gating prose is pinned by tests, but prose effect is not prose presence, so:
中文说明代码审查方案与仅从 issue 出发我会提出的方案一致——按运行已持有的 verdict 整步搬迁、门控留在注入的核心——且执行忠实。三点是机械化核验而非目测： 搬迁逐字保真。 对 diff 做行级集合比对：全部 393 行删除内容都在三个参考文件中逐字节重现，仅有七处已声明的改写——三处过时的 Aone/comment-status 句子（见下）、 交叉引用仍可解析。 步骤标题保留在核心（新测试钉住），语料中所有「Step 7」「Step 8」引用都落在转发指针上。旧措辞「the skip list in the Aone section above」只出现在删除侧——没有悬空引用残留。compose 状态字段表从 Step 7 搬入 Step 6 裁决小节，并有单副本钉住（ 构建无需改动，也确实没改。 资产复制器是递归的（仅跳过测试文件与 DESIGN.md），过期检查两侧的摘要都对整个 review 技能目录做 顺带修复成立。 被删的三句话声称 防回归守卫现在通过 测试经由 API 获取的被审提交 CI 证据——无人值守运行，本地未构建未执行任何 PR 代码。macOS/Windows/集成测试的跳过是设计使然：这些任务在 未在此验证：真实运行行为——即一次真实运行是否按 verdict 触发按需读取（Step 8 前加载 persistence、不适用时跳过 posting/Aone）。PR 描述报告了一次 headless E2E 观察到了这些；这是作者的声明，本环节未独立复跑。门控措辞有测试钉住，但措辞的效果不等于措辞的存在，因此：沙箱验证可以定论—— — Qwen Code · qwen3.8-max Reviewed at |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/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 冲突，直到移除标签或达到轮次上限。移除 |
|
Confidence: 4/5 — clean, disciplined split whose claims check out mechanically; the one gap no CI run can close is live-model compliance with the new gates, and CI itself is still in flight. Stepping back: this is the shape the issue itself proposed, and the PR hews to it — including the places where it honestly under-delivers against the issue's estimate. "~one third" was not reachable under the issue's own whole-step guardrail (Steps 1 and 6 dominate the core), so the PR takes the measured 15–20% and documents why instead of fragmenting steps to chase a number; likewise no GHE reference file, because those notes are sentences woven into universal paragraphs and extracting them would strip rules from steps that stay in core. I'd make the same calls. What earns the confidence: the move is byte-verbatim except seven named rewordings, every one of them justified and pinned by a test; the revert guards were extended to the corpus rather than quietly narrowed, and new pins hold the gates, the retained invariants, and the no-duplication property; and the split needed zero build-side changes because the copier and both digest walks were already recursive — the sign of a split that goes with the grain of the system. The one behavioral edit (three stale Aone/comment-status sentences) aligns the skill with code that already merged in #9627, which I verified at the code level, not just the PR's word. Reservations, named: whether a live run actually follows the verdict-gated reads is not provable from tests — prose presence is not prose effect — and the headless E2E that observed the gates firing is the author's claim; the 中文说明置信度：4/5 —— 一次干净、有纪律的拆分，各项声明均通过机械化核验；CI 无法弥补的唯一缺口是真实模型对新门控的遵从度，且 CI 本身仍在运行。 退一步看：这正是 issue 自身提出的形态，PR 严格遵循——包括它诚实「未达标」之处。「约三分之一」在 issue 自设的整步护栏下不可达（Step 1 与 Step 6 占据核心主体），所以 PR 拿走实测的 15–20% 并如实记录原因，而不是为了追数字拆散步骤；同理没有 GHE 参考文件——那些说明是织在通用段落里的句子，抽出来会把规则从留在核心的步骤中剥离。我也会做同样的选择。 置信度的依据：除七处点名的改写外搬迁逐字保真，每一处改写都有理由且有测试钉住；防回归守卫被扩展到完整语料而非悄悄收窄，新增钉住覆盖门控、保留的不变量与无重复性质；拆分在构建侧零改动，因为复制器与两侧摘要遍历本就是递归的——这是顺着系统纹理拆分而不是逆着来。唯一的行为性编辑（三处过时的 Aone/comment-status 句子）是把技能与 #9627 已合并的代码对齐，我在代码层面核验过，而非仅凭 PR 的说法。 保留意见，点名：真实运行是否遵循按需读取的门控，测试无法证明——措辞的存在不等于措辞的效果——观察到门控正确触发的 headless E2E 是作者的声明；若维护者希望在合并前得到这层保障，上文点名的 — Qwen Code · qwen3.8-max 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
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
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
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.8-max via Qwen Code /review (v0.22.0)
| { | ||
| "path": "src/file.ts", | ||
| "line": 42, | ||
| "body": "**[Critical]** issue description as plain sentences carrying the concrete trigger and the wrong outcome\n\n```suggestion\nfix code\n```\n\n_— YOUR_MODEL_ID via Qwen Code /review (v{{cliVersion}})_", |
There was a problem hiding this comment.
[Critical] R1-1: The split moves the template tokens out of the only text BundledSkillLoader interpolates. {{cliVersion}} now appears only in this reference file (here and at lines 219, 248) and {{model}} only in references/persistence.md:85 — both loaded raw via read_file, for which no substitution pass exists, and nothing else in the injected context supplies the CLI version. A posting run therefore drafts the mandated comment footer with a literal (v{{cliVersion}}): REVIEW_FOOTER_RE's version span [A-Za-z0-9._+-] excludes {/}, so stripReviewFooter cannot match it, and normalizeInlineComments appends the canonical footer after it — every posted inline comment carries a visibly broken (v{{cliVersion}}) footer above the canonical one, on the default attribution-on leg. If the model invents a version instead, the footer names a version the run never ran — the deterministic guarantee the loader provided pre-split is gone either way. The {{model}} instance, copied literally into .qwen/review-cache/pr-<n>.json, also fails the next round's same-model anchor gate (cross-model-anchor) and silently degrades incremental reviews to full-diff re-reviews. Witness: a probe ran the real stripReviewFooter/reviewFooter on the drafted body _— qwen3-coder via Qwen Code /review (v{{cliVersion}})_ — attribution-on strip removed the literal-token footer: false, posted body carries the marker twice ((v{{cliVersion}}) above (v0.21.2)); flip side (same draft with the token interpolated as pre-PR): strips true, one canonical footer. The attribution-off leg is NOT affected (its forged-footer regex admits braces — verified).
Fix — do not keep a template variable in a file no substitution pass reads; submit already strips the drafted footer and re-appends the canonical one itself, so the draft can name the slot without template syntax:
_— YOUR_MODEL_ID via Qwen Code /review_(in all three footer templates), and restate the cache template's lastModelId in terms of the already-declared YOUR_MODEL_ID value. Alternatively extend the loader to interpolate reference files, or make normalizeInlineComments fail closed on residual {{…}} tokens.
中文说明
此次拆分把模板变量移出了 BundledSkillLoader 唯一会做插值的文本。{{cliVersion}} 现在只出现在本参考文件中（此处及第 219、248 行），{{model}} 只出现在 references/persistence.md:85 —— 两者都是模型用 read_file 原样读取的文件，不存在任何替换通道，注入上下文的其他地方也不提供 CLI 版本号。因此发布运行会把强制要求的评论落款起草为字面量 (v{{cliVersion}})：REVIEW_FOOTER_RE 的版本段 [A-Za-z0-9._+-] 不含 {/}，stripReviewFooter 无法匹配，normalizeInlineComments 会在其后追加规范落款 —— 默认开启署名时，每条已发布的行内评论都会在规范落款上方多出一个显眼的坏落款 (v{{cliVersion}})。若模型改为杜撰一个版本号，落款将写上该运行从未使用的版本 —— 拆分前由加载器提供的确定性保证两种情况下都已丧失。{{model}} 若被逐字抄进 .qwen/review-cache/pr-<n>.json，还会使下一轮的同模型锚点门控拒绝（cross-model-anchor），把增量评审静默降级为全量重审。证据：探针用真实的 stripReviewFooter/reviewFooter 处理起草正文 _— qwen3-coder via Qwen Code /review (v{{cliVersion}})_ —— 开启署名时字面量落款被剥掉：false，发布正文出现两次落款；翻转侧（按拆分前方式完成插值的同一段草稿）：剥离 true，只剩一个规范落款。关闭署名的分支不受影响（其伪造落款正则允许花括号 —— 已验证）。修复建议：不要在无替换通道的文件里保留模板变量 —— submit 本就会剥掉草稿落款并重新追加规范落款，草稿可以不含模板语法地指明该槽位（三个落款模板均改为 _— YOUR_MODEL_ID via Qwen Code /review_），并把缓存模板的 lastModelId 改为引用已声明的 YOUR_MODEL_ID 值；或者扩展加载器对参考文件做插值，或者让 normalizeInlineComments 对残留的 {{…}} 失败即拒绝。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| **Apply the report:** | ||
|
|
||
| - `blockOnExistingComments=true` → **an overlap is a duplicate; the disposal is deterministic — do not ask the user.** Drop each finding whose `(path, line)` appears in `existingComments.overlap` from your `comments` array — **except a finding whose `id` appears in `matchedIds` of an `existingComments.repost` entry at the same location**: that is a Step 6 ledger re-post, and re-posting under the original id is exactly how the id survives into the next round's marker — GitHub stacks it in the original thread, which is where it belongs. The inline counts follow automatically, because `submit` counts the comments you actually attach, so a dropped Critical is simply no longer there to count (and a dropped Critical that was already on the PR does not belong in `state.bodyCriticals` either). List each dropped finding in the terminal summary as "already reported at <path>:<line> — comment <id> (by <user>): <excerpt>", taking `<id>`, `<user>` (omit the `(by <user>)` slot when the entry carries no `user`), and the 80-char `<excerpt>` from the overlapping comment (`existingComments.overlap` entries carry all three), and submit the remainder without pausing. Naming the author is what makes an authorship-refused re-post exemption self-explanatory: the drop line then shows a DIFFERENT author next to the matching id. Name the comment on EVERY drop — that is what makes a same-line false positive visible to the operator instead of a bare location. This decision point has been improvised as an interactive question, which stalls a headless run forever (measured; DESIGN.md — The interactive overlap question); the Exclusion Criteria already forbid re-reporting discussed issues, so there is nothing to ask. (If dropping overlaps leaves zero findings, that is still not a question: submit with an empty `comments` array like any other run — `submit` composes the body from `state`, and a run with nothing to add posts whatever that computes. A recap like "all already reported, N resolved by `<sha>`, two still standing" goes in the **terminal summary**, not the PR: `compose-review` has no free-text body field to carry it (see Step 7 — you do not author PR-facing prose), and it is never a `gh pr comment` — a hand-posted issue comment bypasses the authorisation gate, the downgrade semantics, and the `posted` contract all at once.) | ||
| - `downgradeApprove` / `downgradeRequestChanges` / `downgradeReasons` → **do not apply these by hand.** Copy them into the `presubmit` field of the `compose-review` input (below); the subcommand owns the semantics its tests pin — a downgrade fires only when the verdict it names is the one on the table (a Suggestion-only review is already Comment, so nothing is downgraded and no "Downgraded" sentence is emitted), the downgrade sentence carries the reasons, and a downgraded Request changes keeps its body Criticals after the sentence so the self-PR downgrade never erases the only copy of a blocker. |
There was a problem hiding this comment.
[Suggestion] R1-2: Stale "(below)" pointer created by the split. The compose-state field list this bullet points at moved from Step 7 into the core SKILL.md (Step 6 Verdict section), so nothing below line 147 of this file documents the fields anymore. The sibling deictic at line 223 (// the compose-review state below in the review-JSON example) is stale in the same sense, though the adjacent "the field list there is authoritative for both consumers" sentence redirects the reader; line 147 has no such redirect. The failure is real at the consumer: compose-review.ts reads only input.presubmit.{downgradeApprove,downgradeRequestChanges,downgradeReasons}; an executor that scans this file for the promised spec, finds none, and improvises a top-level shape would silently lose the downgrades — a self-PR or drifted run would keep the verdict the gate meant to cap. Mitigating fact: the relocated list sits in the always-injected core, so the spec is still reachable — which keeps this a wrong-pointer fix rather than a dropped contract.
Copy them into the `presubmit` field of the `compose-review` input (listed with the state fields in Step 6's Verdict section)and fix the same deictic in the JSON comment at line 223.
中文说明
拆分产生的过期「(below)」指向。本条所指的 compose 状态字段表已从 Step 7 移入核心 SKILL.md（Step 6 裁决小节），因此本文件第 147 行之下再无任何字段说明。第 223 行的同类指示语（review-JSON 示例中的 // the compose-review state below）同样过期，但其紧邻的「the field list there is authoritative for both consumers」一句为读者指了路；第 147 行没有这样的改道。对消费端的失败是真实的：compose-review.ts 只读 input.presubmit.{downgradeApprove,downgradeRequestChanges,downgradeReasons}；执行者若按承诺在文件内寻找字段规范而不得、转而即兴发挥成顶层字段，降级会被静默丢弃 —— 自身 PR 或 head 漂移的运行将保留门控本应封顶的裁决。缓解事实：搬迁后的字段表位于始终注入的核心正文，规范仍然可达 —— 因此这是「指错位置」的修复，而非合同丢失。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| @@ -183,7 +181,7 @@ Based on the parsed `target.type`: | |||
|
|
|||
| - **`{"resumed": false, "resumeRefused": "<reason>"}`** — the same command has already fallen through to a fresh fetch; proceed exactly as a normal run (the report at `--out` is new) and tell the user why the resume was refused. A refusal with reason `head-moved` IS this review's one head-movement restart — `fetch-pr` records it on disk, and Step 7's restart bound reads as already spent. | |||
|
|
|||
| - **The setup calls that do not feed each other go out in ONE response — as separate tool calls, never joined with `&&`/`;` into one Shell command** (high and medium effort — at low, Step 2's rules load is skipped and nothing consumes the comment index, so the batch is whatever calls remain). A joined chain changes the failure semantics — a `pr-context` failure must warn-and-continue, not skip the other two — and merges the `warning:` size lines the paging decisions below read. Once `fetch-pr` has returned (and the incremental check, which reads its report, is decided — except on the side-file anchor path, where the decision deliberately waits for `pr-context`'s side file), the next three commands are mutually independent — `pr-context` (below), `comment-status` (below), and Step 2's rules load — every one a read with no side effect the others observe (on an Aone target, `comment-status` drops out of the batch — it has no Aone backing; the skip list in the Aone section above names why — leaving a two-call batch). Issue the whole batch in a single response, exactly as Step 3 already requires for the agent fan-out, then read their outputs (paging where a file exceeds one read, and those reads can share a response too). The rules load takes `<remote>/<baseRefName>` — the ref `fetch-pr` just updated; no local-existence probe — **except when the fetch report recorded `baseFetchFailed: true`: drop it from the batch and `git fetch <remote> <baseRefName>` first** (on an unresolvable ref `load-rules` reports "no rules found", indistinguishable from a repo that has none, and the review silently enforces nothing). Measured on a real small-PR run: the stretch from `parse-args` to the first agent launch took **7 minutes of wall clock**, one round-trip at a time, on calls that never needed an order. The only orderings that matter: `fetch-pr` before all of them (it creates the worktree and the plan), **any side-file `fetch-pr --since` re-run before `repo-context`** (the re-run rewrites the fetch report from scratch, and `repo-context` enriches that same file in place — an enrichment written first is silently discarded, and the roster then builds without the manifest's required agents), `repo-context` before `agent-prompt --roster` (the roster and every brief bake the manifest's required agents and context blocks, so building them first silently drops the context), and `agent-prompt --roster` after the rules load (the roster bakes the rules into every brief). | |||
| - **The setup calls that do not feed each other go out in ONE response — as separate tool calls, never joined with `&&`/`;` into one Shell command** (high and medium effort — at low, Step 2's rules load is skipped and nothing consumes the comment index, so the batch is whatever calls remain). A joined chain changes the failure semantics — a `pr-context` failure must warn-and-continue, not skip the other two — and merges the `warning:` size lines the paging decisions below read. Once `fetch-pr` has returned (and the incremental check, which reads its report, is decided — except on the side-file anchor path, where the decision deliberately waits for `pr-context`'s side file), the next three commands are mutually independent — `pr-context` (below), `comment-status` (below), and Step 2's rules load — every one a read with no side effect the others observe. Issue the whole batch in a single response, exactly as Step 3 already requires for the agent fan-out, then read their outputs (paging where a file exceeds one read, and those reads can share a response too). The rules load takes `<remote>/<baseRefName>` — the ref `fetch-pr` just updated; no local-existence probe — **except when the fetch report recorded `baseFetchFailed: true`: drop it from the batch and `git fetch <remote> <baseRefName>` first** (on an unresolvable ref `load-rules` reports "no rules found", indistinguishable from a repo that has none, and the review silently enforces nothing). Measured on a real small-PR run: the stretch from `parse-args` to the first agent launch took **7 minutes of wall clock**, one round-trip at a time, on calls that never needed an order. The only orderings that matter: `fetch-pr` before all of them (it creates the worktree and the plan), **any side-file `fetch-pr --since` re-run before `repo-context`** (the re-run rewrites the fetch report from scratch, and `repo-context` enriches that same file in place — an enrichment written first is silently discarded, and the roster then builds without the manifest's required agents), `repo-context` before `agent-prompt --roster` (the roster and every brief bake the manifest's required agents and context blocks, so building them first silently drops the context), and `agent-prompt --roster` after the rules load (the roster bakes the rules into every brief). | |||
There was a problem hiding this comment.
[Suggestion] R1-3: This diff removes the last three Aone comment-status skip residues (the "drops out of the batch — it has no Aone backing … leaving a two-call batch" parenthetical here, the "and on Aone targets, where the command has no backing" guard clause in the comment-status section, and the Step 6 no-report clause) — a deliberate, code-backed alignment with the a1 behavior that already landed — but no revert-guard test pins the removal. The existing guard 'runs comment-status and presubmit on Aone targets — backed, not skipped' (SKILL.test.ts) negates older caveat wordings only; none of its assertions mention the phrases this diff deleted or the replacement contract. A future merge resolution or partial revert re-adding the skip ships green — every existing assertion stays satisfied — while Aone runs would again skip comment-status, contradicting references/aone.md's a1-backing statement and degrading Step 6's blocker re-check to lightweight-mode routing. This file has a documented merge-residue incident already — the sibling 'not merge residue' test exists because of it.
Extend that guard with the removal and its replacement, e.g. in SKILL.test.ts:
expect(body).not.toContain('drops out of the batch');
expect(body).not.toContain('leaving a two-call batch');
expect(body).toContain('on an Aone target it runs a1-backed');中文说明
本 diff 删除了最后三处 Aone comment-status 跳过残留（此处的「drops out of the batch — it has no Aone backing … leaving a two-call batch」括注、comment-status 小节的「and on Aone targets, where the command has no backing」守卫从句、以及 Step 6 的「无报告」从句）—— 这是与已落地的 a1 行为对齐的、有代码支撑的有意修复 —— 但没有任何防回归测试钉住这次删除。现有守卫『runs comment-status and presubmit on Aone targets — backed, not skipped』（SKILL.test.ts）只否定了更早期的告诫措辞；它的断言既不提及本 diff 删除的这些短语，也不钉住替代后的合同。未来某次合并解决或部分回退若重新加回跳过措辞，将一路绿灯地合入 —— 所有现有断言仍然满足 —— 而 Aone 运行将再次跳过 comment-status，与 references/aone.md 的 a1 支持声明矛盾，并把 Step 6 的 blocker 复核降级为轻量模式路由。本文件已有合并残留事故的先例 —— 姊妹测试『not merge residue』正是因此存在。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| @@ -829,7 +827,7 @@ The posture binds the posting path; low and medium never post, so for them it ch | |||
|
|
|||
| ### 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 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, AND a platform the command is backed on — an Aone run skips the command with the Step 1 batch, so it gets no report, exactly like a lightweight-mode run), 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. A run with no report because one was never written (lightweight mode, Aone) has no per-thread status routing at all and no hand-derived substitute: each blocker is ruled from the code at the reviewed commit (the diff itself, in lightweight mode), and a ruling that would rest on facts only the report could supply is `cannot tell`, never a guess. A run where the command RAN and FAILED keeps its Step 1 fallback — statuses become "re-derive if needed", exactly as the comment-status section above prescribes. 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 — run …)_` note naming the exact, already-filled-in `review comment-body` command for the rest** — a candidate blocker whose snippet was cut is ruled on only after running that command; ruling on the visible prefix alone is the fail-closed violation. Run it **with `--out` writing to a file, never bare 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): add `--out .qwen/tmp/qwen-review-{target}-body-<id>.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 — on an Aone target it runs a1-backed, with the thread-shape notes in `references/aone.md`), 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. A run with no report because one was never written (lightweight mode) has no per-thread status routing at all and no hand-derived substitute: each blocker is ruled from the code at the reviewed commit (the diff itself, in lightweight mode), and a ruling that would rest on facts only the report could supply is `cannot tell`, never a guess. A run where the command RAN and FAILED keeps its Step 1 fallback — statuses become "re-derive if needed", exactly as the comment-status section above prescribes. 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 — run …)_` note naming the exact, already-filled-in `review comment-body` command for the rest** — a candidate blocker whose snippet was cut is ruled on only after running that command; ruling on the visible prefix alone is the fail-closed violation. Run it **with `--out` writing to a file, never bare 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): add `--out .qwen/tmp/qwen-review-{target}-body-<id>.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: | |||
There was a problem hiding this comment.
[Suggestion] R1-7: The hunk folds Aone into the comment-status report's existence ("on an Aone target it runs a1-backed"), but the same sentence's second scope limit — "it indexes inline threads only — an issue-level or review-level blocker (the #6486 shape) has no entry there" — is true only of the GitHub report. On Aone, listMrComments returns ALL MR comments, and the shared thread builder keeps another account's pathless (MR-level) blocker comments in the index — a probe driving the real aoneCommentToStatusComment/buildThreadStatuses with such a comment observed the entry: path "", isBlocker: true, file-level anchor with line null, all code facts unknown, never outdated (own-account pathless roots are dropped from blocker promotion; other accounts' are kept). The references/aone.md thread-shape note this sentence points at covers code-fact degradation and outdated/resolved bucketing but says nothing about pathless entries. Consequence: on an Aone MR where another account posted an issue-level blocker, Step 6's re-checker — told by this sentence that such blockers "have no entry" in the report — either distrusts the report it is told to route the status half through, or treats the pathless entry as an inline thread with vanished anchored code instead of taking the context-file walk prescribed here. The fail-closed cannot tell default prevents a wrong verdict; the cost is misrouting in the step whose purpose is fail-closed blocker re-checking.
Qualify the limit for the newly covered platform — e.g. "(GitHub shapes the index as inline threads only — the Aone index also carries pathless MR-level threads)" — or extend the references/aone.md thread-shape note with the pathless-entry shape.
中文说明
该 hunk 把 Aone 纳入了 comment-status 报告的存在条件（「on an Aone target it runs a1-backed」），但同句的第二个范围限定 —— 「it indexes inline threads only — an issue-level or review-level blocker (the #6486 shape) has no entry there」—— 只对 GitHub 报告成立。在 Aone 上，listMrComments 返回全部 MR 评论，共享的线程构造器会把其他账号的无路径（MR 级）blocker 评论保留进索引 —— 探针用这样一条评论驱动真实的 aoneCommentToStatusComment/buildThreadStatuses，观察到条目：path 为 ""、isBlocker: true、文件级锚点且 line 为 null、所有代码事实为 unknown、永不过期（本账号的无路径根评论会被取消 blocker 提升，其他账号的保留）。本句指向的 references/aone.md 线程形态注释只覆盖代码事实降级与 outdated/resolved 分桶，完全未提及无路径条目。后果：当某个 Aone MR 上有其他账号发布了 issue 级 blocker 时，Step 6 的复核者被本句告知此类 blocker 在报告里「没有条目」，于是要么不信任它被告知要用来路由状态信息的报告，要么把无路径条目当成锚定代码已消失的行内线程，而不是走此处规定的 context 文件路径。失败即保守的 cannot tell 默认防止错误裁决；代价是在这个以「失败即保守复核 blocker」为目的的步骤里发生误路由。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // A run must learn from the injected core alone WHICH file to read and | ||
| // when; a gate that moved into the file it gates would be unreadable. | ||
| const core = coreBody(); | ||
| expect(core).toContain('**Reference files, gated by this verdict.**'); |
There was a problem hiding this comment.
[Suggestion] R1-8: This test claims in its name and comment to pin "WHICH file to read and when", but its assertions stop at the file-enumeration prefixes — none of the three load-condition clauses in the core gate list (SKILL.md:78-80) is asserted by any test in the repo (grepped: zero hits for "posting is live", "cross-repo lightweight", "GitHub runs never", etc.). A probe confirmed the gap: rewriting posting.md's bullet to Load it before Step 7 on every run. with the enumeration prefix intact passes 35/35 — the mutant survives; adding toContain pins for the condition clauses fails the mutant and passes on the pristine corpus. The trigger is not hypothetical for this file — merge residue demonstrably happens here (the sibling 'not merge residue' test exists because of one), and the gate conditions are the mechanism this PR introduces.
| expect(core).toContain('**Reference files, gated by this verdict.**'); | |
| expect(core).toContain('**Reference files, gated by this verdict.**'); | |
| expect(core).toContain('Load it when, and only when, posting is live'); | |
| expect(core).toContain('Load it before Step 8 on every run except cross-repo lightweight mode'); | |
| expect(core).toContain('Load it before `match-remote` when the target is Aone'); |
中文说明
该测试的名称与注释声称要钉住「读哪个文件、何时读」，但其断言止步于文件枚举报头 —— 核心门控表（SKILL.md:78-80）中三个加载条件从句，整个仓库没有任何测试断言（grep 证实："posting is live"、"cross-repo lightweight"、"GitHub runs never" 等均零命中）。探针确认了缺口：把 posting.md 的条件从句改写为 Load it before Step 7 on every run.（保留枚举报头）仍通过 35/35 —— 变异体存活；为条件从句补上 toContain 钉住后，变异体失败、原始语料通过。对本文件而言这并非假想触发 —— 合并残留在这里有实锤先例（姊妹测试『not merge residue』正是因此存在），而门控条件正是本 PR 引入的机制。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const core = coreBody(); | ||
| // Distinctive openings of the moved sections: present in exactly one file. | ||
| expect(core).not.toContain( | ||
| '**Use the "Create Review" API to submit verdict + inline comments', |
There was a problem hiding this comment.
[Suggestion] R1-5: This test's name and comment promise the moved sections are "present in exactly one file", but the three section-opening pins assert absence from the CORE only — duplication between the three reference files passes. Only the separate modelId pin enforces corpus-wide uniqueness (toHaveLength(1)). A probe confirmed the gap: appending the "Create Review" opening to aone.md (so it exists in both aone.md and posting.md) passes 35/35 as shipped; corpus-wide toHaveLength(1) pins fail the mutant and pass on the pristine corpus. The harm path is real: an Aone --comment run loads both posting.md and aone.md (core gate block, SKILL.md:76-81), so a duplicated step body governs one run in two potentially divergent copies — and this PR's own head merge committed exactly this residue shape once (the sibling 'keeps the corrected Aone --comment contract, not merge residue' test exists because of it).
Promote the three pins to the modelId shape over skillBody():
const corpus = skillBody();
expect(corpus.match(/\*\*Use the "Create Review" API/g)).toHaveLength(1);
expect(corpus.match(/### Report persistence/g)).toHaveLength(1);
expect(corpus.match(/run `\/review` \*\*from inside a clone of that repo\*\*/g)).toHaveLength(1);(optionally keeping the not.toContain(core) halves).
中文说明
该测试的名称与注释承诺被搬迁的小节「恰好存在于一个文件中」，但三个小节开头钉住只断言了核心正文中的缺席 —— 三个参考文件之间的重复可以通过。只有独立的 modelId 钉住通过 toHaveLength(1) 强制全语料唯一。探针确认了缺口：把 "Create Review" 开头追加到 aone.md（使其同时存在于 aone.md 与 posting.md）按现状仍通过 35/35；改为全语料 toHaveLength(1) 钉住后变异体失败、原始语料通过。危害路径真实存在：Aone --comment 运行会同时加载 posting.md 与 aone.md（核心门控块，SKILL.md:76-81），重复的步骤正文会让同一次运行受两份可能分叉的副本约束 —— 而本 PR 自己的头部合并就曾提交过完全相同的残留形态（姊妹测试『keeps the corrected Aone --comment contract, not merge residue』正是因此存在）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| } | ||
| } | ||
| } | ||
| return parts.join('\n').replace(/\r\n/g, '\n'); |
There was a problem hiding this comment.
[Suggestion] R1-6: The widened corpus oracle accepts each pinned template from ANY corpus file — including verdict-gated reference files many runs never load — weaker than both the pre-PR oracle (SKILL.md only = injected on every run) and the placement invariant the comment above states ("the composed-name template lives in the core SKILL.md, the report stems in references/persistence.md"). Today's placement is correct (composed-name template only at SKILL.md:921, stems only at persistence.md:14-16), so nothing is currently broken — the guard is just wider than the runtime visibility it is meant to pin. A probe confirmed both directions: moving the --out template into posting.md, or the stems into posting.md/aone.md, keeps both oracles green (2/2 passing) while a report-only PR run (never loads posting.md) or a GitHub run (never loads aone.md) would never see the template and would improvise artifact names — composedNameFor/reportPatternFor drifting from what the skill writes is the exact live defect class this test's header says the pinning exists to end.
Match each oracle to the files the owning step is guaranteed to see: extract the composed-name template from SKILL.md only, and the report stems from SKILL.md + references/persistence.md only (the one reference loaded before Step 8 on every run that has a Step 8), instead of the whole corpus for both.
中文说明
扩展后的语料 oracle 接受来自任意语料文件的模板匹配 —— 包括许多运行从不加载的、按 verdict 门控的参考文件 —— 这比拆分前的 oracle（只有 SKILL.md = 每次运行都注入）更弱，也弱于上方注释自述的摆放不变量（「composed-name 模板在核心 SKILL.md，report 词干在 references/persistence.md」）。当前的摆放是正确的（composed-name 模板仅在 SKILL.md:921，词干仅在 persistence.md:14-16），所以现在没有东西坏掉 —— 只是守卫比它想要钉住的运行时可见性更宽。探针双向确认：把 --out 模板移入 posting.md、或把词干移入 posting.md/aone.md，两个 oracle 都仍然绿灯（2/2 通过），而仅出报告的 PR 运行（从不加载 posting.md）或 GitHub 运行（从不加载 aone.md）将永远看不到该模板、只能即兴发挥产物文件名 —— composedNameFor/reportPatternFor 与技能实际写出的内容漂移，正是本测试头部注释声称钉住机制要终结的那类线上缺陷。修复：让每个 oracle 只匹配其所属步骤必然可见的文件 —— composed-name 模板只从 SKILL.md 提取，report 词干只从 SKILL.md + references/persistence.md 提取（唯一在所有含 Step 8 运行中于 Step 8 前加载的参考文件），而不是两者都读整个语料。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9804 (review round 2)All seven inline findings (1 Critical, 6 Suggestions) were verified against the Findings and dispositions[rc:3838826115] R1-1 — Critical: template tokens moved out of the loader's substitution pass — FIXEDReproduced before changing anything: Fix (the finding's first option — root-cause, no new machinery): [rc:3838826118] R1-2 — stale "(below)" pointers in posting.md — FIXEDThe compose-state field list moved into the core SKILL.md's Step 6 [rc:3838826121] R1-3 — no revert guard pinned the Aone skip-residue removal — FIXEDVerified all three removed residues against [rc:3838826124] R1-7 — "indexes inline threads only" is GitHub-only — FIXEDReproduced with the real functions: driving [rc:3838826127] R1-8 — load-condition clauses unpinned — FIXEDVerified the gap: the gating test asserted only the enumeration prefixes. [rc:3838826139] R1-5 — section pins allowed cross-reference duplication — FIXEDPromoted the three section-opening pins to corpus-wide [rc:3838826141] R1-6 — parity oracle wider than runtime visibility — FIXEDVerified current placement (composed-name template only at SKILL.md:921, [rv:5002681043] CHANGES_REQUESTED — disclosed review gap (integration tests)The review body discloses "build-and-test — Integration Tests (CLI, No [ic:5386073438] E2E verification report — notes
VerificationCommands actually run this round (repo root unless noted):
Changed files: 中文说明轮次总结 — PR #9804（评审第 2 轮）全部七条行内发现（1 条 Critical、6 条 Suggestion）均已对照代码核实，并在一次提交中解决： 发现与处置[rc:3838826115] R1-1 — Critical：模板变量被移出了加载器的替换通道 — 已修复修改前先行复现： 修复（采用发现给出的第一方案 —— 根因修复，不引入新机制）： [rc:3838826118] R1-2 — posting.md 中过期的「(below)」指向 — 已修复compose 状态字段表已移入核心 SKILL.md 的 Step 6 [rc:3838826121] R1-3 — Aone 跳过残留的删除没有防回归守卫 — 已修复已对照 [rc:3838826124] R1-7 — 「indexes inline threads only」只对 GitHub 成立 — 已修复用真实函数复现：以其他账号的无路径（MR 级）blocker 评论驱动 [rc:3838826127] R1-8 — 加载条件从句未被钉住 — 已修复缺口核实：门控测试此前只断言枚举报头。按发现建议补上三条从句钉住（ [rc:3838826139] R1-5 — 小节钉住允许参考文件之间互相重复 — 已修复把三个小节开头钉住提升为对 [rc:3838826141] R1-6 — parity oracle 比运行时可见性更宽 — 已修复核实当前摆放（composed-name 模板仅在 SKILL.md:921，report 词干仅在 persistence.md:14-16），并把每个 oracle 收窄到其所属步骤必然可见的文件：composed-name 模板现只从核心 SKILL.md 提取，report 词干只从 SKILL.md + [rv:5002681043] CHANGES_REQUESTED — 已披露的评审缺口（集成测试）评审正文披露「build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally」。 [ic:5386073438] E2E 验证报告 — 备注
验证本轮实际执行的命令（未注明者均在仓库根目录）：
变更文件： Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 1: executing npx vitest run src/commands/review/run-skill-parity.test.ts in packages/cli (no node_modules/dist in the review worktree); "agent reverse-audit (round 1)": locating the exact home of the "literal \n from -f body= " and body-vs-empty-comments refusals posting.md attributes to submit — structuralProblems now refus….
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/skills/bundled/review/SKILL.test.ts:37 — [probe] corpus-wide revert guards lost placement guarantees when skillBody() widened (visibility-timing guards pin corpus-only)packages/core/src/skills/bundled/review/SKILL.test.ts:27 — [probe] REFERENCE_FILES constant unreconciled with references/ directory — forward-drift hole in the guard suite
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 1：executing npx vitest run src/commands/review/run-skill-parity.test.ts in packages/cli (no node_modules/dist in the review worktree)；"agent reverse-audit (round 1)"：locating the exact home of the "literal \n from -f body= " and body-vs-empty-comments refusals posting.md attributes to submit — structuralProblems now refus…。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // covered by run.test.ts's own cases. Failing here would report a checkout | ||
| // shape as a contract drift. | ||
| const itWithSkill = skill === null ? it.skip : it; | ||
| const itWithSkill = coreSkill === null ? it.skip : it; |
There was a problem hiding this comment.
[Suggestion] R2-1: The sparse-checkout skip guard keys only on the existence of SKILL.md, but the Step 8 report-stems oracle now also reads references/persistence.md. A sparse/partial checkout that has SKILL.md but not that file runs the stems test instead of skipping it, and the test then fails as if the skill's templates had drifted: step8Corpus falls back to the core body, the stems regex finds zero stems (all three live only in persistence.md), and the arrayContaining assertion reports a checkout shape as a contract drift — the exact misattribution the guard's own comment above exists to prevent. Pre-split the same checkout passed because the stems lived in SKILL.md itself, so the split widens the set of checkout shapes that fail spuriously. A probe at the reviewed commit reproduced it: removing only persistence.md fails reportPatternFor accepts Step 8's report stems with expected [] to deeply equal ArrayContaining ["local", "pr-<number>", "<filename>"], and splitting the guard flips that to 1 passed | 1 skipped. Split the guard so the stems oracle skips unless persistence.md exists too, and use it for the stems test:
| const itWithSkill = coreSkill === null ? it.skip : it; | |
| const itWithSkill = coreSkill === null ? it.skip : it; | |
| const itWithStep8 = | |
| coreSkill !== null && | |
| existsSync(join(SKILL_DIR, 'references', 'persistence.md')) | |
| ? it | |
| : it.skip; |
中文说明
sparse-checkout 跳过守卫只以 SKILL.md 是否存在为条件，但 Step 8 的报告文件名干（report stems）oracle 现在还会读取 references/persistence.md。当稀疏/部分检出包含 SKILL.md 但缺少该文件时，stems 测试会照常运行而不是被跳过，然后以「技能模板发生漂移」的姿态失败：step8Corpus 回退到核心正文，stems 正则一个也匹配不到（三个名干全部只存在于 persistence.md），arrayContaining 断言会把检出形态误报为合同漂移 —— 这正是上方守卫注释本要防止的误归因。拆分前同样的检出是绿的（名干当时就在 SKILL.md 里），因此本次拆分扩大了会被误报失败的检出形态。探针在受审提交上复现：仅删除 persistence.md 时 reportPatternFor accepts Step 8's report stems 以 expected [] to deeply equal ArrayContaining ["local", "pr-<number>", "<filename>"] 失败；拆分守卫后同一形态变为 1 passed | 1 skipped。建议把守卫拆开，让 stems oracle 在 persistence.md 不存在时也跳过，并在 stems 测试中改用新守卫。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(core).toContain('Load it when, and only when, posting is live'); | ||
| expect(core).toContain( | ||
| 'Load it before Step 8 on every run except cross-repo lightweight mode', | ||
| ); | ||
| expect(core).toContain( | ||
| 'Load it before `match-remote` when the target is Aone', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R2-2: The three load-condition clauses are pinned with position-free toContain checks that never tie a clause to its bullet, so swapping two clauses between bullets ships green. A merge resolution or paraphrase of the **Reference files, gated by this verdict.** block that reattaches Load it when, and only when, posting is live to the persistence.md bullet and the Step 8 clause to posting.md keeps all six assertions passing (each prefix present, each clause present, all in core), while a report-only run then never loads references/persistence.md — the review report is not saved and the incremental cache never written, silently degrading every later round to a full review. A probe at the reviewed commit confirmed it: with the two clauses swapped the suite passes 36/36, while pinning each prefix contiguously with its clause fails on that same mutation and stays green on the real corpus. (The un-pinned Step 8 forwarder at SKILL.md:1014 restates the correct gate, so a run hitting a swapped block sees conflicting instructions rather than one clean wrong gate — but that forwarder is not pinned either.) Pin each prefix together with its clause as one contiguous substring:
| expect(core).toContain('Load it when, and only when, posting is live'); | |
| expect(core).toContain( | |
| 'Load it before Step 8 on every run except cross-repo lightweight mode', | |
| ); | |
| expect(core).toContain( | |
| 'Load it before `match-remote` when the target is Aone', | |
| ); | |
| expect(core).toContain( | |
| '`references/posting.md` — Step 7 (authorisation, anchors, presubmit, `submit`, the 422/head-drift recovery, `publish-assets`). Load it when, and only when, posting is live', | |
| ); | |
| expect(core).toContain( | |
| '`references/persistence.md` — Step 8 (report, artifact registration, incremental cache). Load it before Step 8 on every run except cross-repo lightweight mode', | |
| ); | |
| expect(core).toContain( | |
| '`references/aone.md` — the Aone paths (see the Aone note below). Load it before `match-remote` when the target is Aone', | |
| ); |
中文说明
三个加载条件从句由不定位的 toContain 检查钉住，但从未把从句与其所属条目绑定，因此把两个从句在条目之间互换仍能一路绿灯地合入。若对 **Reference files, gated by this verdict.** 块的一次合并解决或改写把 Load it when, and only when, posting is live 重新挂到 persistence.md 条目、把 Step 8 从句挂到 posting.md 条目，六个断言依旧全部通过（每个前缀都在、每个从句都在、且都在核心正文中），而仅出报告的运行将不再加载 references/persistence.md —— 评审报告不会保存、增量缓存不会写入，之后每一轮都会被静默降级为全量评审。探针在受审提交上确认：互换两个从句后套件 36/36 通过；把每个前缀与其从句作为连续子串钉住后，同一变异失败、真实语料保持绿。（SKILL.md:1014 处未钉住的 Step 8 转发句复述了正确的门控，撞上互换块的运行会看到相互矛盾的指示而非单一错误门控 —— 但该转发句同样未被钉住。）建议把每个前缀与其从句合并为一个连续子串钉住。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed — round summary (PR #9804)Both inline Suggestions from the automated reviewer were reproduced on the reviewed commit and fixed in one test-only commit: Feedback points and dispositions[rc:3839303895] R2-1 — sparse-checkout skip guard misses
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Not explored to full depth (tool budget reached): chunk 9: Could not verify whether the presubmit command is being re-implemented elsewhere in this PR, or whether the posting.md reference is documenting a legacy com….
— qwen3.8-max via Qwen Code /review (v0.21.10)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Head drift detected. The review was started against
daabf20dc52e49575b0a51b1f540fc9b59c527a0, but the PR head is now84703e79ca8107377f6f2f33cf7ba46d7b537eb3. Inline comments are therefore omitted; only the aggregate summary is kept below.
Round 1 review summary (deepseek-v4-flash)
Verdict: ISSUES_FOUND — no Critical issues, but the reverse audit surfaced 5 Suggestion-level findings.
The 12 review agents (10 chunks + test-coverage matrix + removal-behavior audit) and 10 reverse-audit agents did not identify any Critical problems.
Reverse-audit findings (pre-existing, not introduced by this PR)
posting.md— thepresubmitcommand example is missing the--hostflag.- Lightweight-mode 422 recovery description is inconsistent with the
fetch-diffoutput format. publish-assets--findings-outlacks failure-behavior documentation.startLine !== linecondition does not handlenull/undefined.bodyCriticalsdescription omits deferred Criticals as a source.
Key invariants verified
- Core
SKILL.mdretains run-binding invariants (write prohibition, posting gates, step headings). - Reference files (
posting.md,persistence.md,aone.md) are correctly verdict-gated. - Deleted content was faithfully migrated to reference files (byte-identical except for one necessary restatement).
- Test coverage is reinforced with new revert guards.
- Injected prompt size reduced from ~381 KB to ~304 KB for non-posting runs (~58 KB / ~15% saving).
Posted by qwen-review-bot after PR head drift.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): "agent 1b": none — all checks above ran to completion..
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/skills/bundled/review/SKILL.test.ts:27 — [probe] REFERENCE_FILES enumeration unreconciled with references/ directory — a fourth file escapes every corpus guard including the token banpackages/core/src/skills/bundled/review/SKILL.test.ts:1043 — [probe] moved-section openings not pinned to their owning reference file — relocation between reference files ships greenpackages/core/src/skills/bundled/review/SKILL.test.ts:37 — [probe] skillBody() corpus re-scoping silently re-pins ~40 every-run assertions to corpus-wide scope
中文说明
无阻断问题。LGTM！✅
未探索到全部深度（达到工具调用预算）："agent 1b"：none — all checks above ran to completion.。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #9804This round triaged the five Suggestion-level reverse-audit findings from review [rv:5003294860] (posted after head drift, so the findings arrived as the aggregate summary with no inline threads). Three findings were fixed, two were declined with recorded evidence. No Critical findings were reported; no failed checks; no conflict resolution was needed ( Fixed
Declined (recorded evidence)
Mutation probesEach new revert guard was proven to witness its fix before committing:
Files changed
VerificationCommands actually run this round (all after the fixes were in place):
中文说明Autofix 轮次总结 — PR #9804本轮分诊了评审 [rv:5003294860] 中的 5 条 Suggestion 级反向审计发现（该评审因 head 漂移而以聚合摘要形式发布，没有行内评论线程）。其中 3 条已修复，2 条附证据拒绝。本轮没有 Critical 发现；没有失败的检查项；无需冲突解决（ 已修复
已拒绝（附证据记录）
变异探针每个新增防回退测试在提交前都已证明能守护其对应修复：
变更文件
验证本轮实际执行的命令（均在修复就位之后运行）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
ec89eb8
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): chunk 6: execute the vitest suite itself — the review worktree has no node_modules and no packages/core/dist , and npm install + build exceeds this review's tool bu….
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/core/src/skills/bundled/review/SKILL.md:934 — [probe] SKILL.md:934 states the never-in-body rationale…packages/core/src/skills/bundled/review/SKILL.test.ts:1067 — [probe] Revert guard pins the adjudicated-false mechanism clause…packages/core/src/skills/bundled/review/references/persistence.md:4 — [review] persistence.md's preamble claims Steps 1–7 are in context,…packages/core/src/skills/bundled/review/references/posting.md:21 — [review] Payload-refusal sentence misdescribes two checks that do…packages/core/src/skills/bundled/review/references/posting.md:173 — [review] posting.md:173 fabricates a **[Suggestion]** workflow…packages/core/src/skills/bundled/review/references/posting.md:184 — [probe] The actionable publish-assets block omits…packages/core/src/skills/bundled/review/references/posting.md:188 — [probe] publish-assets --host annotation is GHE-only while every…packages/core/src/skills/bundled/review/references/posting.md:250 — [review] posting.md:250 autofix rationale names a failure mode the…packages/core/src/skills/bundled/review/references/posting.md:259 — [review] The actionable submit block omits [--user-authorized] ,…
中文说明
无阻断问题。LGTM！✅
未探索到全部深度（达到工具调用预算）：chunk 6：execute the vitest suite itself — the review worktree has no node_modules and no packages/core/dist , and npm install + build exceeds this review's tool bu…。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 9 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Verified at head ec89eb8:
- The sole Critical (R1-1) is closed: no
{{...}}template token survives in any reference file — the uninterpolated{{cliVersion}}/{{model}}tokens were dropped rather than moved, the mandated footer now keys off the model name declared in the prompt, and nothing raw-loaded via read_file carries a token no substitution pass would fill. - The verdict-gated loading is pinned precisely: posting.md reads only when posting is live (comment.effective or an in-session post request, PR-only/high-only), persistence.md before Step 8 on every run except cross-repo lightweight, aone.md before match-remote on Aone targets — each gate clause pinned to its exact bullet text, and the no-duplication invariant (one rule, one file — an Aone --comment run loads posting.md and aone.md without divergent copies) is documented and pinned.
- The compose-state field list relocation is sound: the fields the composer reads now live in the core verdict section, so a report-only run writes state without owing posting.md a read, and the one restated rule (no Suggestion text in the review body) carries its substance in core with a pointer to the full rule.
- The test-side guards the later rounds asked for are in: the report-stems oracle reads only what Step 8 is guaranteed to see (core + persistence.md) with its own sparse-checkout skip keyed on persistence.md, and the corpus oracle refuses templates from verdict-gated files a run might never load.
CI at approval time: 14 checks passing, none failing. The remaining unresolved threads are earlier-round Suggestions on pin granularity, superseded by the later commits.
|
Released in v0.22.2. |
5 participants
What this PR does
Splits the bundled review skill's manifest into a core body plus reference files loaded on demand. The injected core keeps target resolution, fan-out, verification and presentation — and every invariant that binds all runs: the write prohibition, the posting gates, and the step headings all cross-references resolve against. The conditional territory moves into three reference files beside it, which the orchestrator reads right after argument parsing, gated by the verdict it already holds: the posting protocol (Step 7) loads only when posting is live — an effective comment flag or a same-session request to post, on a PR target at high effort; the persistence step (report, artifact registration, incremental cache) loads before Step 8 on every run except cross-repo lightweight mode; the Aone-specific paths load before remote matching when the host or platform says Aone. The compose-state field list relocates verbatim from the posting step to the verdict section where the composer is invoked, so a report-only run writes that state without owing the posting file a read. All moved content is byte-verbatim with one restatement: a relocated field entry referenced the "no Suggestion text in the review body" rule after that rule moved, so the entry now restates the rule's substance in the core (pinned by a new revert guard). One verified drive-by: three stale sentences still claiming comment-status had no Aone backing are aligned with the a1-backed behavior that already landed (#9627) and that its revert-guard test already mandates.
Why it's needed
The skill's manifest was injected whole on every run — 381,984 bytes, ~95k tokens, 19× the next-largest bundled skill — although large stretches are conditional territory a given run never touches: the posting protocol on runs that never post, the persistence step on cross-repo lightweight runs, the Aone paths on GitHub runs. The parse-args verdict already decides which of these apply, and progressive disclosure via
references/is an established bundled-skill pattern (dataviz ships one). Measured effect on the injected prompt: 381,125 → 304,427 body bytes. A typical non-posting run (local/file/PR, any effort) saves ~58 KB (≈15%, ≈14.5k tokens); a cross-repo lightweight run saves ~77 KB (≈20%); a posting run loads the posting file back and saves only the Aone block. The issue's "roughly a third" estimate is not reachable under its own whole-step guardrail — Steps 1 and 6 dominate the core, and fragmenting them would strip rules from steps that remain in core; effort-gated splits of the reverse audit and the low-tier inline pass are the natural follow-up and are noted in the issue artifact.Reviewer Test Plan
How to verify
npm run build && npm run bundle, then: | node dist/cli.js review parse-args --stdin— expect a clean verdict JSON and no "NOT built from the review sources" staleness warning (the build digests the new reference files on both sides of the staleness check).dist/bundled/review/ships the manifest plus the three reference files byte-identical to the sources, with no DESIGN.md or test files.cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts src/skills/bundled-skills.integration.test.ts— every revert guard now governs the full corpus (core + references); new pins cover the gates, the core-retained invariants and the no-duplication invariant.cd packages/cli && npx vitest run src/commands/review/run-skill-parity.test.ts— the filename-template oracle reads the corpus..qwen/issues/issue-9787.md.Evidence (Before & After)
N/A (prompt-structure refactor; evidence is the measured injection size and the E2E logs referenced above).
Tested on
Environment (optional)
npm run build && npm run bundle+node dist/cli.js; headless E2E with real model calls.Risk & Scope
Linked Issues
Fixes #9787
中文说明
本 PR 做了什么
将内置 review 技能的清单文件拆分为核心正文 + 按需加载的参考文件。注入的核心保留目标解析、扇出、验证与呈现，以及所有对任何运行都有约束力的不变量：写入禁令、发布门控、以及所有交叉引用所指向的步骤标题。条件性内容移入旁边的三个参考文件，由编排器在参数解析后按 verdict 已有的判定按需读取：发布协议（Step 7）仅在发布生效时加载（
--comment生效或用户在本会话中要求发布，且为 high 档的 PR 目标）；持久化步骤（报告、工件注册、增量缓存）在除跨仓轻量模式外的所有运行中于 Step 8 前加载；Aone 专属路径在 host 或 platform 表明是 Aone 时于 match-remote 之前加载。compose 状态字段表从发布步骤逐字搬迁到调用 compose-review 的裁决小节，使仅出报告的非发布运行无需加载发布文件即可写该状态。所有搬迁内容均为逐字节原样，仅有一处重述：一个搬迁后的字段条目原指向「Suggestion 文本不得进入 review 正文」规则，该规则已随发布文件移出，因此该条目现在核心中重述规则实质（并有新的防回归测试钉住）。另有一处已核实的顺带修复：三处仍声称 comment-status 无 Aone 支持的过时句子，与 #9627 已落地的 a1 支持行为对齐（该 PR 的防回归测试本就强制要求如此）。为什么需要
技能清单此前在每次运行时全量注入 —— 381,984 字节、约 9.5 万 token，是第二大内置技能的 19 倍 —— 尽管其中大段是某次运行根本不会触及的条件性内容：从不发布的运行用不到发布协议，跨仓轻量运行用不到持久化步骤，GitHub 运行用不到 Aone 路径。parse-args 的 verdict 本就已判定哪些内容适用，且
references/渐进式披露是既有的内置技能模式（dataviz 已在用）。对注入量的实测效果：381,125 → 304,427 正文字节。典型非发布运行（local/file/PR、任意档位）节省约 58 KB（约 15%，约 1.45 万 token）；跨仓轻量运行节省约 77 KB（约 20%）；发布运行会重新加载发布文件，仅节省 Aone 块。Issue 中「约三分之一」的估计在其自身的「整步搬迁」护栏下不可达 —— Step 1 与 Step 6 占据核心主体，拆散它们会把规则从留在核心的步骤中剥离；按 effort 门控拆分反向审计与低档内联通道是自然的后续项，已记录在 issue 工件中。评审者测试计划
如何验证
npm run build && npm run bundle，然后: | node dist/cli.js review parse-args --stdin—— 应得到干净的 verdict JSON，且无「NOT built from the review sources」过期警告（构建在过期检查的两侧都会摘要新的参考文件）。dist/bundled/review/携带清单与三个参考文件、与源码逐字节一致，且不含 DESIGN.md 或测试文件。cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts src/skills/bundled-skills.integration.test.ts—— 所有防回归守卫现在覆盖完整语料（核心 + 参考文件）；新增钉住门控表、核心保留的不变量与无重复不变量。cd packages/cli && npx vitest run src/commands/review/run-skill-parity.test.ts—— 文件名模板 oracle 改为读取整个语料。.qwen/issues/issue-9787.md。前后对比证据
N/A（提示词结构重构；证据为上述注入量实测与 E2E 日志）。
测试环境
环境（可选）
npm run build && npm run bundle+node dist/cli.js；headless E2E 使用真实模型调用。风险与范围
关联 Issue
Fixes #9787