test(review): close confirmed pin gaps from #9194 (batch 1) - #9225
test(review): close confirmed pin gaps from #9194 (batch 1)#9225yiliang114 wants to merge 3 commits into
Conversation
Pin the auth<gh ordering half (issue-context/comment-body/fetch-diff), the issue-context write-target path, the full 'NOT in the closing set' extras header wording, and the Number.isInteger guard halves for fractional pr_number/id/--pr (mutation-verified). Audited the rest of the checklist: whitespace-only --host rejection, fetch-diff non-integer case, comment-body write path, and pr-context host-routing/omitted-host pins are already covered on main. Remaining #9194 items deferred to follow-up batches.
|
Thanks for the PR! Template looks good ✓ — all required headings present (the optional Chinese Problem: observed and documented, not theoretical. The linked tracking issue #9194 records the mutation-verified pin gaps from the PR #9096 review rounds — each item is a contract a production mutation can strip while the suite stays green, and the PR names the mutation evidence per item. Direction: aligned — test-pin completeness for the Size: not applicable — no core paths touched; 0 production lines vs. 66 test lines (+65/−1 across three Approach: scope feels right. Each addition is the missing half of an existing assertion, completed in place — no new fixtures, no restructure, no drive-by changes. The remaining #9194 items are explicitly listed as follow-up batches, which keeps this one minimal. The "already covered" audit claims check out against the base commit (fetch-diff's fractional Risk: no elevated risk signals — all changed files are Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓——所有必需标题齐全(可选的中文 问题:已观测且有记录,不是理论性问题。关联的跟踪 issue #9194 记录了 PR #9096 审查轮次中经变异验证的 pin 缺口——每个条目都是一个可以在测试套件保持绿色的情况下被生产代码变异剥离的契约,PR 中逐条列出了变异证据。 方向:对齐—— 规模:不适用——未触及核心路径;0 行生产代码,66 行测试代码(3 个 方案:范围合理。每处新增都是对既有断言缺失的一半,就地补全——没有新 fixture、没有重构、没有顺手改动。#9194 的剩余条目被明确列为后续批次,保持了本 PR 的最小化。"已覆盖"的审计声明经 base 提交核实属实(fetch-diff 的小数 风险:无升级风险信号——所有改动文件均为 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline first: from the title and the #9194 items alone, I would have closed each gap by completing the existing assertion in place — pin Each claim verified against the reviewed base commit:
No blockers, no convention issues. The new test comments explain the why per house style, and the diff carries no drive-by changes. Testing evidenceUnattended CI run — the PR's own checks are the evidence (per policy, no local execution of PR code). Orchestration/gate checks (authorize, label, Classify PR, precheck/resolve/publish steps) completed or skipped as expected; the meaningful signal: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The ubuntu unit suite — the one that runs these three vitest files — is still in flight; the table updates in place when CI settles. macOS/Windows tests and the tmux/verify jobs are skipped by the precheck gates, consistent with a test-only diff. Not verified: runtime behavior — N/A by construction, this PR changes no production code. 中文说明代码审查先给出独立基线:仅从标题和 #9194 条目出发,我会通过就地补全既有断言来关闭每个缺口——在三个已记录调用顺序的 ordering 测试中固定 各项声明均已基于被审 base 提交核实:
无阻塞项,无规范问题。新测试注释按项目风格解释了"为什么",diff 无顺手改动。 测试证据无人值守 CI 运行——证据来自 PR 自身的 CI 检查(按策略不在本地执行 PR 代码)。ubuntu 单测套件(运行这三个 vitest 文件的作业)仍在进行中,表格将在 CI 结束后就地更新。macOS/Windows 测试及 tmux/verify 作业被 precheck 门控跳过,与纯测试 diff 一致。未验证:运行时行为——按构造不适用,本 PR 不改动任何生产代码。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal test-pin work verified assertion-by-assertion against the production code it pins; the only nit is the missing Chinese This is exactly what a pin-completeness batch should look like. The independent baseline I wrote before reading the diff matches what the PR does — and there is no simpler path, because each addition is the missing half of an existing assertion. Every new assertion was checked against the base commit: the ordering invariant holds against the real call sequence and is non-vacuous, the write target matches what production actually passes to The one reservation, deliberately minor: the PR body omits the template's Chinese translation section. All required headings are present, so this isn't a gate issue — noting it for completeness. Verdict: approve. The ubuntu unit suite that runs these files is still in flight, so approval is deferred until CI lands green on 中文说明回顾:这正是 pin 完整性批次应有的样子。读 diff 前写下的独立基线与 PR 做法一致——也不存在更简路径,因为每处新增本身就是既有断言缺失的一半。每条新断言都已对照 base 提交核实:顺序不变量与真实调用序列一致且非空转;写入目标与生产代码实际传给 唯一的保留意见(刻意保持轻微):PR 正文缺少模板的中文翻译部分。所有必需标题齐全,不构成门禁问题——仅作完整性备注。 结论:批准。运行这些文件的 ubuntu 单测套件仍在进行中,批准将延迟到 CI 在被审提交上全绿后执行。 — 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
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 3b": none — nothing was cut short..
Test Plan (not a blocker): src/commands/review/issue-context.test.ts — no such file or directory; src/commands/review/comment-body.test.ts — no such file or directory; src/commands/review/fetch-diff.test.ts — no such file or directory.
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Closeout: added the missing meta auth-before-gh ordering assertion. Verified focused meta vitest, ESLint, Prettier, and diff check. |
|
Closeout for this pass: Changed:
Recovered:
Pending:
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6a": executing the four touched vitest files (npm ci extraction corrupted in the review worktree; pins verified statically against production source instead); "agent 6a": executing the four touched vitest files — npm ci in the review worktree produced corrupt extractions (directories created, package contents absent, tar "Canno…; "agent 6b": could not run the 4 test files — npm ci repeatedly failed with ENOTEMPTY in the worktree (broken/partial node_modules).; "agent 6b": could not execute the four test files — npm ci failed repeatedly with ENOTEMPTY in the worktree (pre-existing broken/partial node_modules ), so the green-run…; "agent 5": running the four touched test files ( comment-body , fetch-diff , issue-context , meta ) — blocked by environment: repeated npm ci attempts in the review w…, and 1 more.
Test Plan (not a blocker): src/commands/review/issue-context.test.ts — no such file or directory; src/commands/review/comment-body.test.ts — no such file or directory; src/commands/review/fetch-diff.test.ts — no such file or directory.
— qwen3.8-max via Qwen Code /review (v0.21.12)
What this PR does
First batch of the #9194 pin-completeness checklist — the gaps confirmed still open on current main, closed test-side only (no production behavior changes):
threads --host …tests pinnedhost < min(auth, gh)but notauth < gh; a data fetch that beatgh auth statuswould have shipped green. Now pinned in issue-context, comment-body, and fetch-diff.calls[0][1]) but never the target path; a redirected write reporting the right body shipped green.calls[0][0]is now asserted against the resolved--outpath.Number.isIntegerguard halves — the non-positive cases were tested but not the integrality half: fractionalpr_number(issue-context) and fractionalid/--pr(comment-body) now exit 2 before any gh/auth call. Mutation-verified: strippingNumber.isIntegerfrom comment-body's guard turns the new test red; restoring it goes green.Also audited and confirmed already covered (no change needed): fetch-diff's non-integer
pr_numbercase,setGhHostwhitespace-only/flag-shaped rejection inlib/gh.test.ts, comment-body's write path viatoHaveBeenCalledWith, and pr-context's host-routing + omitted-host (setGhHost(undefined)) pins.Why it's needed
Each item is a test that under-pins its stated contract — a mutation in the production code ships the suite green (the reviewer mutation-verified the class on PR #9096 rounds 5–6). The gaps are non-blocking for behavior but defeat the purpose of the pins.
Reviewer Test Plan
How to verify
Mutation check used for item 4 (then reverted): remove
Number.isIntegerfrom comment-body's usage guard — the fractional test fails; restore — green.Evidence (Before & After)
N/A — test-only change; the new assertions pass against current production behavior and fail when the pinned invariant is mutated (demonstrated for the guard half).
Tested on
Environment (optional)
Unit tests only (mocked gh/fs boundary).
Risk & Scope
not.toContain('gh issue view')and 422headRefOidnegative assertions; stderr error-message pins; pr-context write-target/mkdir ordering pins; the meta HOSTNAME_RE exit-1 classification + GH_HOST env attribution; all code-hygiene items; the round-9 additions.Linked Issues
Part of #9194