docs(review): tell the agents drive and mock-provider exist, and how a fix's test earns its place - #8369
Merged
Merged
Conversation
wenshao
requested review from
LaZzyMan,
tanzhenxin and
yiliang114
as code owners
August 2, 2026 07:48
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ — the "Why it's needed" and "Linked Issues" headings are folded into the body rather than standing alone, but the content is all there and then some. Problem: observed, not theoretical. The reference-count table speaks for itself — Direction: squarely aligned. This is documentation for the review pipeline's own tooling — making commands discoverable and teaching agents to write tests that actually pin defects. No CHANGELOG signal needed for internal tooling docs. Size: 20 production lines (all additions, zero deletions), touching Approach: minimal and focused. Two changes, both directly addressing the stated problems. The "Deliberately not added" section is a good sign — the author actively resisted duplicating guidance the briefs already carry. Nothing to cut. Risk: no elevated risk signals. Neither file matches the high-risk path patterns. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— "Why it's needed" 和 "Linked Issues" 标题融入了正文，但内容齐全且详尽。 问题：已观测到的，非理论性的。引用计数表说明一切 —— 方向：完全对齐。这是 review 流水线自身工具的文档 —— 让命令可被发现，并教会 agent 写出真正能钉住缺陷的测试。内部工具文档无需 CHANGELOG 信号。 规模：20 行生产代码（全部为新增，零删除），触及 方案：最小且聚焦。两处变更均直接针对所述问题。"刻意没有加的"一节是好信号 —— 作者主动克制，没有重复 briefs 已有的指导。无可删减。 风险：无升级风险信号。两个文件均未匹配高风险路径模式。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewIndependent proposal: given "drive and mock-provider have zero references in the agent briefs", I would add both commands to the verify brief in What the PR does: exactly that, and the execution is strong. The The SKILL.md addition (4 lines) lands in Step 6B right where a fixer applies findings, which is where the guidance is actionable. The four examples are concrete and varied — string containment matching a different occurrence, mutation preserving the type but changing the field, asserting the mechanism instead of the behaviour, testing a helper the real path doesn't call. The distillation ("asserting a string is present rather than the behaviour holds") is the right lesson. No correctness issues, no convention violations, no scope creep. The writing matches the existing dense, instructional style of both files. TestingThis is a docs-only PR — no code paths change, no user-visible behaviour. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Ubuntu test suite is still running. No failures so far. The finalize workflow will update this table when CI settles. Real-scenario testing: N/A — docs-only, nothing user-visible. 中文说明代码审查独立方案： 给定"drive 和 mock-provider 在 agent briefs 中零引用"，我会在 PR 实际做法： 完全一致，且执行质量高。
SKILL.md 的新增（4 行）落在 Step 6B 修复器应用 findings 的位置，正是指导可操作的地方。四个示例具体且各不相同。提炼（"断言字符串存在而非行为成立"）是正确的教训。 无正确性问题，无规范违反，无范围蔓延。写作风格与两个文件已有的密集教学风格一致。 测试纯文档 PR —— 无代码路径变化，无用户可见行为。 Ubuntu 测试套件仍在运行中。目前无失败。finalize 工作流会在 CI 完成后更新此表。 真实场景测试：N/A —— 纯文档，无用户可见内容。 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. This is exactly the kind of PR the review pipeline needs: the author dogfooded the tooling, found that two commands were invisible to the agents they were built for, measured the gap, and closed it with minimal, well-placed documentation. The Step 6B addition is the standout — four concrete mutation survivals from the pipeline's own PRs, distilled into one actionable rule. That's not theoretical best-practice padding; it's a lesson learned and written down where it will actually be read. The diff is 20 lines of additions across two files, both documentation, both in the right place, both matching the existing style. Nothing to cut, nothing missing, nothing that shouldn't be there. The "Deliberately not added" section shows the author thinking about what not to do, which is rarer than it should be. Approval deferred until CI lands green on 中文说明置信度：5/5 —— 每个阶段都干净，毫不犹豫可以合入。 这正是 review 流水线需要的 PR：作者实际使用了工具，发现两个命令对它们所服务的 agent 不可见，测量了差距，并用最小、位置恰当的文档弥合了它。Step 6B 的新增是亮点 —— 来自流水线自身 PR 的四条具体突变存活，提炼为一条可操作的规则。这不是理论性的最佳实践填充，而是学到的教训并写在了真正会被读到的地方。 diff 是两个文件共 20 行新增，均为文档，位置正确，风格一致。无可删减，无遗漏，无不当之处。"刻意没有加的"一节展示了作者在思考什么不该做，这比应有的更少见。 审批推迟至 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
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
previously approved these changes
Aug 2, 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 — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
wenshao
force-pushed
the
feat/review-lenses-from-dogfooding
branch
from
August 2, 2026 08:42
2fb9a2f to
a19c9b0
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）为单个提交。 |
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 33 passed · 0 failed · 33 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：33 通过 · 0 失败 · 33 总计 Verification reportPR 8369 — deep verificationVerdict: 中文摘要
Central claim + A/BThis is a documentation-only PR (2 files, +20/−0). Its central claim is that the verify agent brief now makes
The premise diff is captured in What the harness pins, against the real binary, grouped by the brief's own wording:
FindingsNone blocking. The documentation is field-accurate against the implementation: every flag the brief shows ( CorrectionsNone required — the PR description's mechanism claims hold up under measurement:
Not covered
MethodologyEnvironment: CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
…a fix's test earns its place
Two commands shipped and merged with zero references in SKILL.md or the
briefs. An agent reading its brief had no way to know they were there, so
the verification they exist for went on being hand-written — which is the
state they were built to end.
The verify brief now carries both: what they serve, and — for `drive` —
that the ruling is on `outcome`, never on the captured text alone.
`not-ready` means nothing was driven, so nothing observed is evidence
either way; `timed-out` and `overflowed` mean the capture is partial, and
a partial capture is not evidence that the run produced nothing;
`unavailable` is an environment gap and not a finding. It says to pass
`--ready` for anything that binds a port, because without it an empty
capture reads as "the feature does not work" when it means "the daemon
had not finished starting". And it names what the request log is FOR: the
same drive against both trees, then a diff of the two request sequences.
Step 6B gains the one lesson from this stack of PRs that nothing already
covered. A test added with a fix has to fail without the fix, and
measured on this pipeline's own PRs, four assertions written to pin a
real defect all survived the mutation they were written for:
`toContain('"index":0')` passed with the tool-call index deleted because
that string is also on every `choices` entry; `toContain('input_json_delta')`
passed with the arguments handed over whole because the mutation kept the
type and changed the field; a `set +e` assertion pinned the mechanism
while `exit` was what broke it; a pure function tested alone passed while
the request path called a different one. All the same shape — asserting a
string is present rather than that the behaviour holds.
Deliberately NOT added: a lens on rules applied to one path and not its
sibling. That mistake was made three times across these PRs, and the
briefs already carry it as "the highest-value check in your slice, do it
first and do it exhaustively" — better stated than I would have restated
it. A second copy would be the drift this skill keeps fixing.
The paired mock-provider/drive example ran both commands sequentially in one block. mock-provider serves for the whole --ttl and returns only when it expires -- measured: with --ttl 3 the handler returns at 3.0s -- so an agent copying the block verbatim gets `drive` starting 600 seconds later, against a mock that has already shut down. That is a not-ready outcome or an empty capture: the exact false negative the paragraph below it warns about. Background the mock and wait for its report before driving. The report is written once the port is bound, so the file's appearance is a readiness signal rather than a sleep -- which is the same discipline drive's own --ready enforces, and the example should not have been guessing where the command it documents polls.
wenshao
force-pushed
the
feat/review-lenses-from-dogfooding
branch
from
August 2, 2026 09:14
a19c9b0 to
0efc294
Compare
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
approved these changes
Aug 2, 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 — CI landed green after the review. ✅
wenshao
enabled auto-merge
August 2, 2026 09:55
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| \`\`\`bash | ||
| "\${QWEN_CODE_CLI:-qwen}" review mock-provider --responder <a module you write> \\ | ||
| --log <plan dir>/mock.jsonl --ttl 600 --out <plan dir>/mock.json & | ||
| until [ -s <plan dir>/mock.json ]; do sleep 0.1; done # its port is in that report |
Collaborator
There was a problem hiding this comment.
[Suggestion] The documented mock-provider readiness wait has no failure escape hatch. If the responder module fails to load, or forgets to export respond, startMockProvider throws before server.listen (mock-provider.ts:492, 497-500); the handler catches, writes one stderr line, sets exit 1, and never writes --out (mock-provider.ts:718-734). The backgrounded job dies in milliseconds, but until [ -s mock.json ] can never succeed, so the agent's shell step hangs until its tool timeout — with the only diagnostic (the stderr line) invisible, because the agent is polling a file, not watching the job. The adjacent claim "the TTL is the only thing that ends it" is also false on this path. — Failure scenario: a responder with a syntax error → startMockProvider throws → no --out written → the until loop spins to the tool timeout with no visible error.
A bounded, liveness-checked wait fails loud instead of hanging:
MOCK_PID=$!
for _ in $(seq 1 100); do
[ -s <plan dir>/mock.json ] && break
kill -0 "$MOCK_PID" || { echo "mock-provider died at startup" >&2; exit 1; }
sleep 1
done中文说明
文档里 mock-provider 的就绪等待没有失败退出路径。如果 responder 模块加载失败，或忘记导出 respond，startMockProvider 会在 server.listen 之前 抛出异常（mock-provider.ts:492、497-500）；handler 捕获后只写一行 stderr、置 exit 1，并且永远不会写 --out（mock-provider.ts:718-734）。后台进程几毫秒内就死了，但 until [ -s mock.json ] 永远无法成功，于是 agent 的 shell 步骤会一直挂到工具超时 —— 而唯一的诊断（那行 stderr）不可见，因为 agent 在轮询文件而非监视进程。旁边那句“TTL 是唯一能结束它的东西”在此路径下也不成立。— 失败场景：responder 有语法错误 → startMockProvider 抛出 → 不写 --out → until 循环一直转到工具超时，且看不到任何错误。带边界、带存活检查的等待会大声失败而非挂起（见上方代码块）。
— qwen3.8-max-preview via Qwen Code /review
| \`\`\`bash | ||
| "\${QWEN_CODE_CLI:-qwen}" review mock-provider --responder <a module you write> \\ | ||
| --log <plan dir>/mock.jsonl --ttl 600 --out <plan dir>/mock.json & | ||
| until [ -s <plan dir>/mock.json ]; do sleep 0.1; done # its port is in that report |
Collaborator
There was a problem hiding this comment.
[Suggestion] The readiness wait uses fractional sleep 0.1 — the exact pattern drive.ts identifies as a portability hazard and deliberately works around with Atomics.wait. POSIX specifies an integer operand for sleep; fractional is a GNU/BSD extension, so on a system without it sleep 0.1 fails and returns instantly, turning the until loop into a tight filesystem-polling loop. drive.ts:173-178 measured 8.2 million readiness probes in one second in this exact scenario, and drive.ts:195 switched to Atomics.wait to avoid it — but this new template handed to agents reintroduces the pattern. — Failure scenario: agent runs the snippet on a POSIX-strict shell → sleep 0.1 fails instantly → tight loop hammering the filesystem at millions of probes/sec.
Suggested change
| until [ -s <plan dir>/mock.json ]; do sleep 0.1; done # its port is in that report | |
| until [ -s <plan dir>/mock.json ]; do sleep 1; done # its port is in that report |
中文说明
就绪等待使用了小数 sleep 0.1 —— 正是 drive.ts 明确指出为可移植性隐患、并用 Atomics.wait 刻意规避的那种写法。POSIX 规定 sleep 的操作数为整数；小数是 GNU/BSD 扩展，因此在不支持的系统上 sleep 0.1 会失败并立即返回，把 until 循环变成一个紧密的文件系统轮询循环。drive.ts:173-178 在完全相同的场景下测到一秒 820 万次就绪探测，drive.ts:195 改用 Atomics.wait 以规避 —— 但这份交给 agent 的新模板又把该写法引了回来。— 失败场景：agent 在严格 POSIX 的 shell 上运行该片段 → sleep 0.1 立即失败 → 紧密循环以每秒数百万次探测猛砸文件系统。
— qwen3.8-max-preview via Qwen Code /review
yiliang114
approved these changes
Aug 2, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — both additions are well-grounded. The drive/mock-provider section follows the same shape as the existing base-tree entry (command, when to reach for it, how to read each outcome), and the outcome semantics are correctly stated (not-ready = no evidence, timed-out/overflowed = partial, unavailable = not a finding). The test-must-fail rule is measured, not preached — four real surviving assertions named with their exact failure mode. The 'string present vs behaviour holds' distillation is the actionable core.
Collaborator
|
Released in v0.21.4. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Two things, from working through #8345 / #8349 / #8355 and the /review run on the last of them.
1.
driveandmock-providerhad zero references anywhereBoth shipped and merged (#8349, #8355) with no mention in
SKILL.mdor the agent briefs. An agent reading its brief had no way to know they existed, so the verification they were built for went on being hand-written — the exact state they exist to end.base-treetest-deltaextract-stepdrivemock-providerThe verify brief now carries both, in the shape
base-treealready uses — the command, when to reach for it, and how to read each outcome:drive'soutcome, never the captured text alone.not-readymeans nothing was driven, so nothing observed is evidence either way;timed-outandoverflowedmean the capture is partial, and a partial capture is not evidence that the run produced nothing;unavailableis an environment gap and explicitly not a finding.--readyfor anything that binds a port. Without it the drive starts immediately, and an empty capture reads as "the feature does not work" when it means "the daemon had not finished starting".base-treepath, then a diff of the two request sequences.2. Step 6B: a test added with a fix has to fail without the fix
Measured on this pipeline's own PRs — four assertions written to pin a real defect all survived the mutation they were written for:
toContain('"index":0')choicesentrytoContain('input_json_delta')toMatch(/set \+e/)exitwas what broke itAll the same shape: asserting a string is present rather than the behaviour holds. So: parse and assert structurally, drive the real path rather than its helper, and confirm the removal actually reddens the test you just wrote.
Deliberately not added
A lens on rules applied to one path and not its sibling. That mistake was made three times across these PRs — the mutant guard the hunk loop had, the request counter's skip paths, the serialisation guard on one reply branch — but the briefs already carry it as "the highest-value check in your slice; do it first and do it exhaustively", better stated than I would have restated it. A second copy is the drift this skill keeps fixing.
Why it's needed
Two commands shipped this week and no agent could know they exist.
driveandmock-providerlanded in #8349 and #8355 with tests and docs of their own, but nothing in the briefs or the skill pointed at them — so a finding about runtime behaviour still gets settled the way it was before they existed, by reading the code and hoping. A command an agent is never told about is a command that does not run.The Step 6B line comes from the same place. A test added alongside a fix is worth what it would have caught, and the only way to know that is to remove the fix and watch the test fail. That check was practised in this pipeline's own work but never written down, so it held only when someone remembered it.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review src/commands/review.test.ts— 49 files / 1496 tests green.grep -c "review drive\|review mock-provider" packages/cli/src/commands/review/lib/agent-briefs.ts— 2, was 0.Evidence (Before & After)
Prose only — no behaviour change, so the evidence is the reference counts above and the four measured mutation survivals that motivated Step 6B's addition. Each of those four is now pinned by a structural assertion in the PR that introduced it. Non-UI: N/A.
Tested on
Risk & Scope
Linked Issues
None. Both changes come from dogfooding
/reviewon this repository; they reference #8349 and #8355 without closing anything.中文说明
本 PR 做了什么
两件事，来自 #8345 / #8349 / #8355 的实作，以及最后那个 PR 上的 /review 运行。
一、
drive与mock-provider在任何地方都没有被引用两个命令都已合入（#8349、#8355），却在
SKILL.md和 agent briefs 里零引用。读 brief 的 agent 无从知道它们存在，于是它们要终结的那种手工验证仍在继续 —— 正是它们被造出来要消灭的状态。verify brief 现在按
base-tree已有的格式收录了两者：命令、何时取用、每个 outcome 怎么读。drive的outcome，绝不是单看抓取到的文本。not-ready表示什么都没驱动，因此观察到的一切都不构成任一方向的证据；timed-out与overflowed表示 capture 是部分的，而部分 capture 不等于"这次运行什么也没产生"；unavailable是环境缺口，明确不是 finding。--ready。不传的话驱动会立即开始，而一个空的 capture 会被读成"这个功能不工作"，其实是"daemon 还没启动完"。base-tree的路径，然后 diff 两侧的请求序列。二、Step 6B：随修复添加的测试，必须在没有该修复时变红
在本流水线自己的 PR 上实测 —— 四条为钉住真实缺陷而写的断言，全都在它们所针对的突变下存活：
toContain('"index":0')（该字符串也出现在每个choices条目上）、toContain('input_json_delta')（突变保留了 delta 类型只换了字段）、toMatch(/set \+e/)（钉的是机制，而破坏它的是exit）、只测纯函数（请求路径调用的是另一个）。形态完全相同：断言某个字符串存在，而不是行为成立。所以：解析后按结构断言、驱动真实路径而非其辅助函数、并确认撤掉修复确实会让你刚写的测试变红。
刻意没有加的
关于"规则只施加于一条路径而不施加于其兄弟"的 lens。这个错误在这几个 PR 里我犯了三次，但 briefs 已经把它写成"你这一片里价值最高的检查，优先做且要穷尽"，比我重述得更好。再写一份就是这个 skill 一直在修的漂移。
为什么需要
本周合入了两个命令，而没有任何 agent 能知道它们的存在。
drive与mock-provider随 #8349、#8355 落地，各自带着测试与文档，但 brief 和 skill 里没有一处指向它们 —— 于是关于运行时行为的结论，仍然按它们不存在时的方式得出：读代码，然后指望自己没读错。一个从未被告知给 agent 的命令，就是一个不会被执行的命令。Step 6B 那一行出自同一处。随修复添加的测试，其价值等于它本可以捕获的东西，而唯一能确认这一点的办法，是把修复移除、看着测试变红。这个检查在本流水线自己的工作中一直在做，却从未被写下来，所以它只在有人记得时才成立。
评审者测试计划
如何验证
cd packages/cli && npx vitest run src/commands/review src/commands/review.test.ts—— 49 个文件 / 1496 条测试通过。grep -c "review drive\|review mock-provider" packages/cli/src/commands/review/lib/agent-briefs.ts—— 结果为 2，此前为 0。证据（前后对比）
仅文字改动 —— 没有行为变化，因此证据就是上面的引用计数，以及促成 Step 6B 的那四次实测到的变异存活。这四次现在都已被引入它们的那个 PR 中的结构化断言所锁定。非 UI 改动：N/A。
测试环境
仅在 Linux 上验证（见上表）。
风险与影响范围
关联 Issue
无。两处改动均来自在本仓库对
/review的 dogfooding；它们引用 #8349 与 #8355，但不关闭任何 issue。