refactor(review): define each certification-bar atom exactly once - #9257
Closed
wenshao wants to merge 1 commit into
Closed
refactor(review): define each certification-bar atom exactly once#9257wenshao wants to merge 1 commit into
wenshao wants to merge 1 commit into
Conversation
This was referenced Aug 16, 2026
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Five review rounds on this stack each found a re-implemented copy of one of these certification atoms that was weaker than the original — each a real defect fixed in an earlier PR of the stack, each leaving the duplicated predicate behind. Deduplicating the atoms addresses a demonstrated drift class, not a hypothetical one. Direction: aligned. This is internal machinery of the review pipeline in a maintainer-driven stack; each atom having exactly one definition is the obvious end state, and it fulfills the commitment made in the #9092 review thread. No product-surface impact, so no external direction signal needed. Size: not a core-module path (all changes are under Approach: the scope feels right. Each atom is extracted once and consumers are rewired to it; compositions intentionally stay per-consumer, which is the correct split. No drive-by changes or unrelated edits in the file list. The one thing worth a careful look in code review is the classic extraction hazard — a silent semantic shift while moving (regex shape, JSON-quoting, null guards at call sites). The PR body's mutation matrix claims to pin exactly those; I'll check the diff itself next. Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. One structural note for later stages: because the base is the stacked branch Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到，而非理论性的。这个堆叠的五轮 review 中，每一轮都发现这些认证原子（certification atoms）的某个重新实现副本比原始版本更弱——每一次都是堆叠中更早的 PR 修复的真实缺陷，但重复的谓词一直留着。去重这些原子针对的是已被证实的漂移类别，而不是假设。 方向：对齐。这是维护者主导的堆叠中 review 流水线的内部机制；每个原子只有一个定义是显而易见的最终形态，也兑现了 #9092 review 线程中的承诺。无产品面影响，因此不需要外部方向信号。 规模：不属于核心模块路径（所有改动都在 方案：范围合理。每个原子只抽取一次，消费方改接到它；组合理应留在各消费方，这个切分是对的。文件列表中没有顺手改动或无关变更。代码审查时值得仔细看的是经典抽取风险——搬运过程中无声的语义变化（正则形状、JSON 引号、调用点的 null 守卫）。PR 正文的变异矩阵声称恰好钉住了这些；接下来我会核对 diff 本身。 风险：无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。 一个后续阶段的结构性说明：由于基分支是堆叠分支 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewI read the diff against my own baseline for this problem (extract each predicate into one shared module, rewire consumers, keep compositions local) — the PR does exactly that, and I verified the part that matters for an extraction: semantic equivalence at every rewired call site. What I checked, site by site:
A grep across the whole review tree at the PR head confirms the stated goal: no production file outside No blockers found, and no convention violations — house style, license headers, ESM Test evidenceBeing plain about what this section carries: no unit-test CI ran on this commit, and none will —
Sandboxed verification would settle the remaining gap: 中文说明代码审查：我对照自己对该问题的基线方案（把每个谓词抽到一个共享模块、消费方改接、组合逻辑留在本地）读了 diff——PR 正是这么做的，并且我逐一核对了抽取真正关键的部分：每个改接调用点的语义等价性。 测试证据：直说——该提交没有也不会运行单元测试 CI： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, faithful extraction verified site by site; the one reservation is structural, not in the code: no test CI runs on this stacked base, so the suite evidence is the author's. Stepping back: this is the fifth round of a stack where every prior round found a re-implemented copy of one of these atoms that had quietly gone weaker than the original. The right fix for a drift class found five times is to remove the room for a sixth, and that is exactly what this does — one definition per atom, compositions left where they belong, and the why each shape matters carried into the new module's comments instead of being scattered across three files. My independent proposal for this problem was indistinguishable from the PR's approach; there isn't a meaningfully simpler version of it. What earns the 4 rather than the 5: I verified behavior preservation statically — every rewired call site checked against its inline original, the dead no-op branch confirmed dead, the pointer-null collapse proven equivalent — but on this commit the suite itself has not run, because CI only triggers for Approving. 中文说明回顾整体：这是堆叠的第五轮，此前每一轮都发现这些原子中某个重新实现的副本悄悄弱于原版。对一个被发现五次的漂移类别，正确的修法是消除第六次出现的空间——本 PR 正是如此：每个原子一个定义，组合逻辑留在原处，每个形状"为何重要"的注释从三个文件收拢进新模块。我对该问题的独立方案与 PR 的做法没有实质区别——这个问题不存在更有意义的更简方案。 给 4 分而不是 5 分的原因：行为保持是我静态验证的——逐个核对改接调用点与内联原版、确认空分支确为死代码、证明 pointer-null 折叠等价——但该提交上套件没有运行过，因为 CI 只对基于 批准。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
approved these changes
Aug 16, 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
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
wenshao
requested review from
LaZzyMan,
doudouOUC,
tanzhenxin and
yiliang114
as code owners
August 16, 2026 08:41
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 16, 2026 08:41
e88f92d to
bf0bc04
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 16, 2026 08:53
bf0bc04 to
6cbc7e0
Compare
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 16, 2026 09:30
6cbc7e0 to
a2eb777
Compare
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.
Test Plan (not a blocker): lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Comment on lines
+70
to
+74
| import { | ||
| declaresOwnUncoverable, | ||
| openedBrief, | ||
| readFindingsPointer, | ||
| } from './certification.js'; |
Collaborator
There was a problem hiding this comment.
[Suggestion] R2-3 (2 of 6 locations): coverage.ts rewirings are ungated by any test (pattern details at the :785 comment). This location: reverting this certification import block on its own leaves every affected test green — the rewiring to the shared atoms ships with nothing gating it. — Failure scenario: a future change that re-inlines local copies of these atoms in coverage.ts (the precise failure mode this module's header documents — "five review rounds found five places where a re-implemented copy drifted") stays green, because no test observes which definition the walk calls; a drifted copy would then misrule coverage silently.
Witness: test-efficacy probe, harnessValidated: true — single-hunk revert of the import block: all tests green.
Suggested fix: see the :785 comment — a walk-level test through coverageFromTranscripts that would change outcome if the shared atom were swapped for a divergent local copy.
中文说明
[建议] R2-3（6 处之 2）：coverage.ts 的改接无任何测试把关（模式详情见 :785 评论）。本处：单独还原此 certification import 块后所有受影响测试仍全绿——改接共享原子上线时没有任何测试把关。— 失败场景：未来某次修改在 coverage.ts 重新内联这些原子的本地副本（正是本模块头部记载的失败模式——"五轮审查找出五处再实现副本漂移"）将全绿通过，因为没有测试观察走查调用的是哪份定义；漂移的副本会静默地错判覆盖。
证据：测试效力探针，harnessValidated: true——import 块单 hunk 还原：所有测试全绿。
建议修复：见 :785 评论——通过 coverageFromTranscripts 的走查级测试，使共享原子被换成分歧本地副本时结果改变。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Collaborator
Author
There was a problem hiding this comment.
Declining as stated, because the mutant class cannot be discriminated in principle: reverting the import block alone means re-inlining IDENTICAL copies of the atoms, and no behavioral test can (or should) distinguish a delegation from a byte-equivalent inline body. The property that matters — a DIVERGENT copy is caught — is what the PR's mutation matrix demonstrates: weakening any atom in certification.ts reddens the consumer suites (bare-substring needle → check-coverage's .bak trap; raw regex → recover-findings' quotation pins; etc.), which means the consumers demonstrably route through the shared definitions. 6e64caf additionally gates the two call sites the probes proved genuinely unreached (keySatisfied, gapsSuperseded).
|
|
||
| const UNCOVERABLE_RE = /^\s*Uncoverable:\s*chunk\s+(\d+)\b/im; | ||
|
|
||
| /** The exact rebuild flags for one required agent — operator-facing (stderr). */ |
Collaborator
There was a problem hiding this comment.
[Suggestion] R2-3 (3 of 6 locations): coverage.ts rewirings are ungated by any test (pattern details at the :785 comment). This location: reverting the removal of the module-local UNCOVERABLE_RE (just above this line) leaves every affected test green. — Failure scenario: if the local regex constant were restored and a call site quietly switched back to it with a modified pattern, no test would catch it; Uncoverable: vetoes could then match quotations or miss declarations, flipping chunk verdicts.
Witness: test-efficacy probe, harnessValidated: true — single-hunk revert restoring the local regex: all tests green.
Suggested fix: see the :785 comment — a walk-level test asserting veto behavior end-to-end would gate the relocation.
中文说明
[建议] R2-3（6 处之 3）：coverage.ts 的改接无任何测试把关（模式详情见 :785 评论）。本处：还原对模块本地 UNCOVERABLE_RE（本行正上方）的删除后，所有受影响测试仍全绿。— 失败场景：若该本地正则常量被恢复、且某调用点带着改动过的模式悄然切回，没有任何测试能捕获；Uncoverable: 否决就可能匹配引用、漏掉声明，翻转 chunk 裁决。
证据：测试效力探针，harnessValidated: true——恢复本地正则的单 hunk 还原：所有测试全绿。
建议修复：见 :785 评论——端到端断言否决行为的走查级测试可为该搬迁把关。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Collaborator
Author
There was a problem hiding this comment.
Declining as stated: restoring a dead local const is unobservable by any behavioral test — the meaningful regression is a call site switching to a MODIFIED pattern, and that is pinned from both ends: the atom's own flag/scope pins (/m, /i, quotation, chunk-scope — extended in 6e64caf) and the walk-level veto gates (forcing the veto true fails 26 consumer tests; see the :785 reply).
Comment on lines
+450
to
+451
| const openedBriefOf = (rec: AgentRecord, key: string): boolean => | ||
| openedBrief(rec, planPath, key); |
Collaborator
There was a problem hiding this comment.
[Suggestion] R2-3 (4 of 6 locations): coverage.ts rewirings are ungated by any test (pattern details at the :785 comment). This location: reverting this openedBriefOf delegation (restoring the inline implementation) leaves every affected test green. — Failure scenario: the verbatim-drift rescue and roster-seeding paths that call openedBriefOf are not behaviorally distinguished from the inline version by any test, so a future divergence between the delegation and the atom (an extra condition added to one side only) ships green and re-introduces the brief-opening drift this PR removed.
Witness: test-efficacy probe, harnessValidated: true — single-hunk revert restoring the inline implementation: all tests green.
Suggested fix: see the :785 comment — a walk-level case where brief-opening is the deciding factor for the drift rescue, so a change to either side of the delegation alters an asserted outcome.
中文说明
[建议] R2-3（6 处之 4）：coverage.ts 的改接无任何测试把关（模式详情见 :785 评论）。本处：还原此 openedBriefOf 委托（恢复内联实现）后所有受影响测试仍全绿。— 失败场景：调用 openedBriefOf 的逐字漂移救援与 roster 播种路径在行为上不被任何测试区别于内联版本，因此委托与原子之间的未来分歧（只在某一侧添加额外条件）将全绿通过，重新引入本 PR 已移除的 brief 打开漂移。
证据：测试效力探针，harnessValidated: true——恢复内联实现的单 hunk 还原：所有测试全绿。
建议修复：见 :785 评论——增加 brief 打开成为漂移救援决定因素的走查级用例，使委托任一侧的改动都会翻转某个被断言的结果。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Collaborator
Author
There was a problem hiding this comment.
Declining as stated, same class as the import-block thread: the delegation and an identical inline body are behaviorally indistinguishable by design. Divergence between openedBriefOf call sites and the atom is caught at the atom: the bare-substring mutant reddens check-coverage's .bak fixture, which exercises the walk's openedBriefOf path. The genuinely unreached call sites the probes found (keySatisfied, gapsSuperseded) are now gated by walk-level tests in 6e64caf.
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 16, 2026 12:20
6e64caf to
03ef84d
Compare
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 16, 2026 13:23
03ef84d to
ec859f8
Compare
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 16, 2026 13:26
ec859f8 to
9520e14
Compare
qwen-code-ci-bot
requested changes
Aug 16, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Test Plan (not a blocker): lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory.
中文说明
Test Plan（非阻断）：lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Comment on lines
+90
to
+91
| const needle = JSON.stringify(briefPath(planPath, key)); | ||
| return rec.successfulReadFileArgs.some((a) => a.includes(needle)); |
Collaborator
There was a problem hiding this comment.
[Suggestion] R3-2: The needle-build-and-search idiom is hand-written three times inside the very module created to define each atom exactly once — Concrete cost: openedBrief (lines 75-76), readBrief (lines 90-91), and readFindingsPointer (lines 107-108) each write const needle = JSON.stringify(<path>) + <args>.some((a) => a.includes(needle)). The whole-JSON-value needle is the documented defense against the ${brief}.bak trap, and certification.test.ts pins it separately for all three atoms (lines 126-129, 136-139, 162-165) — the matching rule is load-bearing, replicated three times, against this module's own charter ("nothing left to drift"). Any change to the matching rule — path normalization, escaping, switching to exact equality — must be applied to three 2-line bodies in lockstep; missing one silently splits the three atoms, which is exactly the atom-drift class this module exists to eliminate.
One private helper composed by all three atoms (the fix spans three sites, so not a one-click suggestion):
function argsMention(args: string[], path: string): boolean {
const needle = JSON.stringify(path);
return args.some((a) => a.includes(needle));
}
// openedBrief: argsMention(rec.successfulCallArgs, briefPath(planPath, key))
// readBrief: argsMention(rec.successfulReadFileArgs, briefPath(planPath, key))
// readFindingsPointer: pointer === null ? true : argsMention(rec.successfulReadFileArgs, pointer)中文说明
[Suggestion] “构建 needle 再搜索”的惯用法在“每个原子只定义一次”的模块里被手写了三份 —— 具体代价：openedBrief（75-76 行）、readBrief（90-91 行）、readFindingsPointer（107-108 行）各自写一遍 const needle = JSON.stringify(<path>) + <args>.some((a) => a.includes(needle))。整串 JSON 值的 needle 是针对 ${brief}.bak 陷阱的既定防线，certification.test.ts 也为三个原子分别钉住了它（126-129、136-139、162-165 行）—— 因此该匹配规则是承重结构，却被复制了三份，与本模块自己的章程（“无处可漂移”）相悖。将来任何匹配规则的变更（路径规范化、转义、改为精确相等）都必须同步应用到三处两行函数体；漏掉一处就会悄悄撕裂三个原子 —— 这正是本模块要消灭的原子漂移类别。
修法：收进一个私有 helper，由三个原子组合调用（改三处，故不用一键 suggestion，示例代码见英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Comment on lines
+819
to
+820
| transcript('g2', prompt, { calls: 3 }); | ||
| expect(coverageFromTranscripts(p, ENV).budgetGaps).toEqual([]); |
Collaborator
There was a problem hiding this comment.
[Suggestion] R3-3: No test supplies a NON-RETURNED superseding record to gapsSuperseded's whole-diff branch (coverage.ts:631) or keySatisfied (coverage.ts:557) — Failure scenario: the r.returned conjunct exists for a documented reason (coverage.ts:612-615: an empty return has no gaps BECAUSE it has nothing at all, and reading that as a gap-free repair silences the disclosure it never addressed). If a future refactor drops the conjunct — the same conjunct-drop class round-2 findings R2-7/R2-8 shipped and fixed at :557/:632 — a relaunch that opened its brief and died mid-flight silences the dead attempt's honest Budget gap: disclosure with the whole suite green. Probe-confirmed: deleting the conjunct from either branch leaves the full suite 107/107 green while dedicated dead-relaunch probes fail; the original code passes both probes. The identical hazard IS pinned for the sibling chunkSatisfied ('an honest Uncoverable declaration survives an unreturned relaunch', line 2749) — its two whole-diff siblings have none.
Witness (probe):
mutant-1 (drop r.returned from gapsSuperseded whole-diff branch): suite 107/107 green; probe × expected [] to have a length of 1
mutant-2 (drop it from keySatisfied): suite 107/107 green; probe × expected +0 to be 1
original: both probes pass (2/2)
Negative case to pin the conjunct (e.g. in this test):
// A gap-free relaunch that DIED mid-flight must NOT silence the disclosure:
transcript('g2', prompt, { calls: 3, text: '' });
expect(coverageFromTranscripts(p, ENV).budgetGaps).toHaveLength(1);
// and/or in the recovery test: transcript('w2', prompt, { calls: 3, text: '' })
// expecting recoveredAgents to stay 1.中文说明
[Suggestion] 没有任何测试向 gapsSuperseded 的 whole-diff 分支（coverage.ts:631）或 keySatisfied（coverage.ts:557）提供“未正常返回（NON-RETURNED）”的取代记录 —— 失败场景：r.returned 联言项的存在有文档明示的理由（coverage.ts:612-615：空返回没有 gap 是因为它什么都没有，把它读成“无 gap 修复”会 silence 一个从未处理过披露的进程本应留下的披露）。若未来重构删掉该联言项 —— 与第二轮 R2-7/R2-8 实际发生过并在 :557/:632 修复过的删联言项类别相同 —— 一个开过 brief 但中途死掉的 relaunch 会 silence 掉死去尝试诚实上报的 Budget gap: 披露，而整套测试保持绿色。探针验证：从任一分支删除该联言项，全套 107/107 仍绿，而专门的“死亡 relaunch”探针失败（expected [] to have a length of 1 / expected +0 to be 1）；原代码两个探针均通过（2/2）。同样的危险在兄弟谓词 chunkSatisfied 上有钉住（2749 行 “an honest Uncoverable declaration survives an unreturned relaunch”），这两处却没有。
修法：补一个负例钉住该联言项（示例代码见英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 16, 2026 17:50
9520e14 to
54d6a1a
Compare
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 17, 2026 18:32
dd214e2 to
d8e2108
Compare
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 17, 2026 18:40
d8e2108 to
7c0618b
Compare
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 17, 2026 18:45
7c0618b to
4ce76e0
Compare
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. |
qwen-code-ci-bot
requested changes
Aug 17, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Test Plan (not a blocker): lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory.
中文说明
Test Plan（非阻断）：lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Comment on lines
+800
to
+808
| it('a whole-diff disclosure is silenced only by a compliant gap-free relaunch', () => { | ||
| // `gapsSuperseded`'s whole-diff branch: the superseding record must have | ||
| // OPENED the key's brief and be gap-free itself. Neither conjunct was | ||
| // reached by any test — a revert of the branch shipped green. | ||
| const p = plan3a(); | ||
| const d = promptRecordDir(p); | ||
| mkdirSync(d, { recursive: true }); | ||
| const brief = briefPath(p, 'audit-w'); | ||
| writeFileSync(brief, 'The audit-w brief.'); |
Collaborator
There was a problem hiding this comment.
[Suggestion] R4-1: The identical ~9-10-line whole-diff auditor seeding block (mkdir the record dir, write the brief via briefPath(p, 'audit-w'), compose the launch prompt carrying the brief read_file line, write the audit-w.txt record) is pasted verbatim into all four tests this PR adds — lines 804-813, 823-832, 842-851, and a near-identical fourth copy at 2707-2716 in the recovery test. — Concrete cost: this block is the fixture that exercises the very atoms this PR centralizes; if the recorded-brief shape changes, four copies must be edited in lockstep — updating three leaves the fourth silently testing the stale shape, and these tests exist specifically so a reverted atom branch cannot ship green. The file's own fixture layer (plan, plan3a, wholeDiff, transcript, satisfyRoster) is built from extracted helpers, so a helper fits the existing style:
function briefedWholeDiffLaunch(p: string): string {
const d = promptRecordDir(p);
mkdirSync(d, { recursive: true });
const brief = briefPath(p, 'audit-w');
writeFileSync(brief, 'The audit-w brief.');
const prompt =
'You are review agent `audit-w`.\n' +
`read_file(file_path="${brief}")\n` +
wholeDiff();
writeFileSync(join(d, 'audit-w.txt'), prompt);
return prompt;
}(the recovery test prepends its ledger(p, 'S0', 'S1') call before invoking it)
中文说明
[Suggestion] 本 PR 新增的四个测试逐字粘贴了同一段约 9-10 行的 whole-diff 审计员播种块（创建记录目录、经 briefPath(p, 'audit-w') 写 brief、组装携带 brief read_file 行的启动 prompt、写入 audit-w.txt 记录）——第 804-813、823-832、842-851 行，以及恢复测试中几乎相同的第四份副本（2707-2716 行）。具体代价：该块正是演练本 PR 集中化的各原子谓词的夹具；一旦记录的 brief 形态变化，四份副本必须同步修改——改三漏一就会让第四份悄悄测试过期形态，而这些测试的存在正是为了让被还原的原子分支无法绿灯通过。本文件的夹具层（plan、plan3a、wholeDiff、transcript、satisfyRoster）本就由抽取的助手函数构成，加一个助手符合既有风格（见上方代码）；恢复测试在调用前先执行自己的 ledger(p, 'S0', 'S1')。
— qwen3.8-max via Qwen Code /review (v0.21.13)
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 18, 2026 02:45
4ce76e0 to
20cf101
Compare
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.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- Agent 3a readBrief/readFindingsPointer body duplication (certification.ts:90-91) — already reported as R3-2 (comment 3792139002)
- Agent 7 seven test-efficacy revert-mutant survivors in coverage.ts (:70-74, :353, :453-454, :551, :560, :634, :788) — already reported and settled as the R2-3 family, rounds 2-3 (comments 3791544834-41)
Test Plan (not a blocker): lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/check-coverage.test.ts:2395 — [probe] mtime-stamp fixture block duplicated across three describe blocks with a hidden call-order couplingpackages/cli/src/commands/review/lib/certification.ts:20 — [review] module header overclaims: chunk assignment is defined by two atoms across two modulespackages/cli/src/commands/review/check-coverage.test.ts:808 — [review] R4-1 still stands: identical whole-diff auditor seeding block pasted into the new tests
中文说明
已审查——无阻断问题。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
doudouOUC
reviewed
Aug 18, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1 needle-build-and-search idiom triplicated inside certification.ts (openedBrief/readBrief/readFindingsPointer) — already reported as R3-2 (comment 3792139002)
Test Plan (not a blocker): lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory.
中文说明
已审查——无阻断问题。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.13)
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 18, 2026 12:46
20cf101 to
eeb7b5a
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- Agent 7 mutant-survived at coverage.ts:788 (walk-veto guard forced true) — already reported and settled as the R2-3 family, rounds 2-3 (comment 3791544834)
- Agent 7 six hunk-survived rewirings in coverage.ts (:70, :353, :451, :549, :560, :634) — already reported and settled as the R2-3 family, rounds 2-3 (comments 3791544834-41)
Not explored to full depth (tool budget reached): "agent 6a": none — no check was cut short..
Test Plan (not a blocker): lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory.
中文说明
已审查。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 6a"：none — no check was cut short.。
Test Plan（非阻断）：lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.13)
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 18, 2026 23:43
eeb7b5a to
77ae0e5
Compare
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 18, 2026 23:52
77ae0e5 to
f70c357
Compare
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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R3-3 unpinned
r.returnedconjunct in both whole-diff supersession branches — already reported (comment 3792139003)
Test Plan (not a blocker): lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/check-coverage.test.ts:819 — [probe] wasDeliveredVerbatim conjunct unpinned in both whole-diff supersession branchespackages/cli/src/commands/review/check-coverage.test.ts:2733 — [probe] only(r) session-filter conjunct unpinned in keySatisfied's whole-diff branch
中文说明
已审查——无阻断问题。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
The two-author certification bar — a CLI-built prompt delivered verbatim, plus the agent demonstrably opening its brief, the diff, or the findings list its prompt named — was re-implemented in four places: the coverage walk, the layer-audit gate, retirement, and the resume recovery command. Re-implementing a bar means drifting from it, and each copy had drifted in its own direction. Extract the atoms into `lib/certification.ts` — `chunkOfKey`, `declaresOwnUncoverable`, `openedBrief`, `readBrief`, `readFindingsPointer` — and have coverage, layer-audit-gate, retirement and `recover-findings` compose the same functions. No behavior change: the atoms are the exact predicates the live pipeline already ran, now spelled once so a future edit to the bar reaches every consumer.
wenshao
force-pushed
the
review-resume/5-shared-bar
branch
from
August 19, 2026 05:18
f70c357 to
29e2a00
Compare
doudouOUC
reviewed
Aug 19, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.
— qwen3.8-max via Qwen Code /review (v0.21.13)
qwen-code-ci-bot
approved these changes
Aug 19, 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! ✅
Test Plan (not a blocker): lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory.
中文说明
未发现问题。LGTM！✅
Test Plan（非阻断）：lib/certification.test.ts — no such file or directory; lib/certification.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.14)
doudouOUC
reviewed
Aug 19, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Two-stage review summary
- Round 1 (
deepseek-v4-flash): no code defects found. - Round 2 (
qwen3.8-max): confirmed the extraction is faithful, but surfaced two minor items related to PR framing rather than code.
Findings
[Suggestion] Stale stack description in PR body
The PR body states this change is "Stacked on #9163 (fifth of five)" and that "the stack merges bottom-up, this last." Current state checked at review time: #9163 is closed (unmerged), the base branch has been reset to origin/main, and the merge base is the already-merged #9153. This PR therefore merges standalone; the stack ordering text may mislead a maintainer into waiting for a merge sequence that no longer exists. A one-line body update before merge is worth considering.
[Nit] "the entire existing review suite passes untouched" is overstated
check-coverage.test.ts was modified in this PR: utimesSync mtime pinning was added to existing helpers, and 4 new tests were introduced. The pinning is benign (it deterministically pins re-homed transcripts inside the session-attribution window), and the new tests are sound, but the wording "untouched" does not match the diff. The behavior-preservation argument still stands; only the "unmodified" claim is imprecise.
[Context] Pre-existing Windows test failures
Seven tests fail on Windows identically at PR head and at base/main (POSIX-vs-backslash path assertions, EBUSY rmdir on temp cleanup, and path-sensitive mock plumbing). Their failure is disjoint from the changed import graph, so this PR does not cause them.
Verdict
No code defects; the extraction is clean. Only PR body/test-claim wording suggestions remain.
Collaborator
Author
|
Superseded by #9473. After the stack base (#9092/#9153) merged and #9163 was dropped, this PR's base was locked by GitHub's native-stack feature to the now-deleted |
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
3 participants
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
Defines each atom of the review pipeline's certification bar exactly once, in a new
lib/certification.ts, and routes every consumer through it. The atoms:openedBrief(any successful tool arg naming the exact brief path, as a whole JSON string value),readBrief(a successfulread_fileonly — the layer-audit gate's stricter bar),readFindingsPointer(the findings-list floor),declaresOwnUncoverable(the chunk-scoped veto),chunkOfKey(barechunk-Nand the--chunk-Nsegment of per-chunk audit keys), and theUNCOVERABLE_REthey share.Before this, those predicates lived as copies: the brief-opened needle existed six times across
coverage.ts,recover-findings.tsandlayer-audit-gate.ts; the findings-pointer read three times; the uncoverable veto three times; the regex itself twice. The rewired consumers are the coverage walk, its two supersession predicates, the recovered-work counter,verificationGaps' delivery floor, the layer-audit gate, retirement's pairing, andrecover-findings' recovery bar. Compositions still differ per consumer — that is their job — but each atom now has one definition, so there is nothing left to drift.This is a pure extraction: zero behavior change intended, and the entire existing review suite passes untouched.
Why it's needed
Five review rounds on this stack found five places where a re-implemented copy of one of these atoms was weaker than the original:
recover-findings' bar accepted a chunk agent that never opened the diff, then a verifier that never opened its brief, then one that skipped the findings-list read, then mis-parsed per-chunk audit keys, then missed the returned requirement. Each fix was correct and each left the underlying cause — the same question answered by two bodies of code — in place. Deduplicating the atoms removes the class rather than the fifth instance.Stacked on #9163 (fifth of five, after #9091 → #9092 → #9153 → #9163). Merge order unchanged: the stack merges bottom-up, this last.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review— 2979 pass (the pre-existingstale-bundle.test.tslocal-only failure is unrelated; it compares against the installed bundle). The newlib/certification.test.tspins each atom directly: both chunk-key forms and the non-forms, declaration vs quotation vs different-chunk, the${brief}.bakprefix trap, mention-vs-read for both the brief and the findings pointer.Evidence (Before & After)
Behavior preservation: the full review suite — 2964 tests written against the OLD inlined copies — passes unmodified on the rewired code.
Necessity of each atom's exact shape, and proof the consumers actually route through the shared module (a leftover private copy would shrug these off) — each mutation applied to
lib/certification.tsalone, suites re-run, then reverted:openedBrief: bare-substring needle (no JSON quoting).baktrap incertification.test.tsand incheck-coverage.test.tsdeclaresOwnUncoverable: raw regex (quotation = declaration)recover-findings.test.ts/certification.test.tsreadFindingsPointer: any tool arg counts (mention = open)certification.test.tsandcheck-coverage.test.ts's findings-unread pinreadBrief:successfulCallArgs(a grep supplies a receipt)layer-audit-gate.test.tsandcertification.test.tschunkOfKey: bare form only (per-chunk audit keys go chunk-less)recover-findings.test.ts/certification.test.tsTested on
Environment (optional)
vitest only; no platform-specific fixtures.
Risk & Scope
chunk !== null && declaresOwnUncoverable(rec, chunk)(the null guard was implicit in the old inline condition).recover-findings' composition itself (which branches on chunk vs findings roles) stays local to it by design.Linked Issues
Stacked on #9163 (which stacks on #9153 → #9092 → #9091). Follow-up promised in the round-8 review threads on #9092: the shared-bar refactor that retires the meetsBar divergence class.
中文说明
本 PR 做了什么
把 review 流水线认证 bar 的每个原子谓词收进新模块
lib/certification.ts，全部消费方改走同一份定义。原子包括：openedBrief（任一成功工具调用以完整 JSON 字符串值命名 brief 精确路径）、readBrief（仅认成功的read_file——layer-audit 门的更严格 bar）、readFindingsPointer（findings 清单地板）、declaresOwnUncoverable（按 chunk 限域的否决）、chunkOfKey（裸chunk-N与 per-chunk 审计键的--chunk-N段），以及它们共享的UNCOVERABLE_RE。此前这些谓词以副本形式散布：brief-opened 判据在
coverage.ts、recover-findings.ts、layer-audit-gate.ts里有六份；findings-pointer 读取三份；uncoverable 否决三份；正则本体两份。改接的消费方：coverage 主遍历、两个 supersession 谓词、recovered-work 计数、verificationGaps的交付地板、layer-audit 门、retirement 配对、recover-findings的恢复 bar。各消费方的组合逻辑仍各自不同——那本来就是它们的职责——但每个原子只剩一个定义，无处可漂移。纯抽取：意图零行为变化，既有 review 全量套件原样通过。
为什么需要
本栈五轮审查找出了五处"重新实现的副本弱于原版"：
recover-findings的 bar 先后放过了没开 diff 的 chunk agent、没开 brief 的 verifier、跳过 findings 清单读取的 verifier、误解析 per-chunk 审计键、遗漏 returned 要求。每次修复都正确，但都留下了病根——同一个问题由两处代码各自作答。原子去重删除的是这个类别，而不是第五个实例。堆叠于 #9163（五之五，前有 #9091 → #9092 → #9153 → #9163）。合入顺序不变：自底向上，本 PR 最后。
审阅者验证方案
如何验证
cd packages/cli && npx vitest run src/commands/review——2979 通过（stale-bundle.test.ts是与本 PR 无关的本机既有失败，它比对的是已安装 bundle）。新增lib/certification.test.ts直接钉住每个原子：两种 chunk 键形态与非形态、声明/引用/他人 chunk 之别、${brief}.bak前缀陷阱、brief 与 findings pointer 的"提及 ≠ 读取"。证据（前后对比）
行为保持：全量 review 套件——2964 个针对旧内联副本写的测试——在改接后的代码上原样通过。
各原子精确形状的必要性，以及消费方确实经由共享模块（若有残留私有副本，这些变异将无感）——每个变异只施加于
lib/certification.ts，跑完即还原：openedBrief：裸子串判据（去掉 JSON 引号）certification.test.ts与check-coverage.test.ts的.bak陷阱declaresOwnUncoverable：原始正则（引用即声明）recover-findings.test.ts/certification.test.tsreadFindingsPointer：任意工具参数皆可（提及即打开）certification.test.ts与check-coverage.test.ts的 findings-unread 钉readBrief：改用successfulCallArgs（grep 也能交收据）layer-audit-gate.test.ts与certification.test.tschunkOfKey：只认裸形态（per-chunk 审计键失去 chunk）recover-findings.test.ts/certification.test.ts已测试平台
环境（可选）
仅 vitest；无平台相关夹具。
风险与范围
chunk !== null && declaresOwnUncoverable(rec, chunk)（旧内联条件里该 null 守卫是隐式的）。recover-findings自身的组合逻辑（按 chunk 与 findings 角色分支）按设计留在原地。关联 Issue
堆叠于 #9163（其堆叠于 #9153 → #9092 → #9091）。兑现 #9092 第 8 轮审查线程中的承诺：以共享 bar 重构终结 meetsBar 发散类别。