Skip to content

docs(review): tell the agents drive and mock-provider exist, and how a fix's test earns its place #8369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 2 commits into from
Aug 2, 2026
Merged

docs(review): tell the agents drive and mock-provider exist, and how a fix's test earns its place #8369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a43f002
docs(review): tell the agents drive and mock-provider exist, and how …
wenshao Aug 2, 2026
0efc294
docs(review): background the mock, then wait for its report
wenshao Aug 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/cli/src/commands/review/lib/agent-briefs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,22 @@ A finding an A/B settled carries \`Source: [probe]\` like any other run-produced

It writes the \`run:\` script **verbatim** as an executable and reports what the runner would have supplied: the effective \`env:\` with all three levels merged and each key's level named, every \`\${{ … }}\` site listed **unevaluated** — that list is precisely what you have to stub, because the command refuses to invent values for it — the resolved \`shell\` and \`working-directory\`, and the commands the script invokes. Stubbing and input are yours: shim \`gh\`/\`curl\` onto \`PATH\`, export the env, run it, observe. **Combined with \`base-tree\`, a workflow A/B is two invocations** — extract the same step from both trees, feed both the same input, diff what each would have done. That is how the strongest workflow finding in this pipeline's history was produced: the real composer step from both arms, a stubbed \`gh\`, and a byte-for-byte comparison against a comment the workflow had actually posted. Three limits worth knowing before you spend the step: a \`uses:\` step has no \`run:\` and is refused rather than simulated; a step NAME that two steps in the job share is refused as ambiguous rather than resolved to the first, so pass the index (which is what an A/B wants anyway — the two trees must select the same step, and a name that moved between them is exactly how they stop doing that); and the \`invokes\` list is a labelled heuristic — the verbatim script beside it is the authority.

**When the claim is about what the product DOES at runtime, drive it — two commands make that mechanical.** A finding about behaviour ("this hangs when the provider 429s", "the retry never fires", "the daemon answers before it is ready") is settled by running the built product and watching, and the two halves that used to be hand-written every time are now commands.

\`\`\`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

@qwen-code-ci-bot qwen-code-ci-bot Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

@qwen-code-ci-bot qwen-code-ci-bot Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

"\${QWEN_CODE_CLI:-qwen}" review drive --cwd <the worktree> --script <what to run> \\
Comment thread
wenshao marked this conversation as resolved.
--ready <a command polled until it exits 0> --timeout 300 --out <plan dir>/drive.json
\`\`\`

\`mock-provider\` serves \`/v1/chat/completions\` (OpenAI) and \`/v1/messages\` (Anthropic) on an OS-assigned port it reports back, and appends every request to a JSONL log; your responder module exports \`respond(req)\` returning \`{text}\`, \`{tool, args}\` or \`{status, body}\`, and never has to get SSE framing right. **It serves for the whole \`--ttl\` and returns only when that expires** — so background it and wait, as above; run sequentially it is already shut down by the time the next line starts. Its report is written once the port is bound, which is what makes the file's appearance a readiness signal rather than a guess, and the TTL is the only thing that ends it — set it to bound the drive, not to match it. **The log is the A/B evidence** — drive the same script against the PR worktree and the \`base-tree\` path, then diff the two request sequences; a difference is evidence, a reading is not.

\`drive\` owns the three things that used to be guesswork, and its \`outcome\` is what you rule on, never the captured text alone: \`completed\` carries the script's own \`exitCode\` and is the only value that licenses a behavioural claim; \`not-ready\` means the readiness probe never passed, so **nothing was driven and nothing observed is evidence either way**; \`timed-out\` and \`overflowed\` mean the capture is PARTIAL — a partial capture is not evidence that the run produced nothing; \`unavailable\` (no tmux) is an environment gap and explicitly not a finding. Pass \`--ready\` for 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".

For anything that is not one of those two wires — the project's own HTTP service, an MCP server, an OAuth endpoint — stand it up yourself and let \`drive\` own the lifecycle.

**When the claim is about GITHUB's behaviour, neither tree can settle it — only GitHub can.** A claim like "this encoding renders identically and can never ping", "GitHub strips this tag", "this markdown shape closes the fold" is about the comment pipeline's parser, sanitizer allowlist and notification path, none of which exist in this environment — a local markdown library is a model of GitHub, and judging a sanitizer claim against a model of the authority is exactly the parser-divergence failure under review. Measured live: an \`@\` → \`&#64;\` defusal read as sound in every local trace, and GitHub's real renderer registered the mention and fired the notification. So:

- **If the environment variable \`QWEN_REVIEW_SCRATCH_REPO\` is set** (an \`owner/repo\` the user designated for disposable test posts), you may adjudicate on the real renderer: post the payload as an issue comment there — \`gh api repos/$QWEN_REVIEW_SCRATCH_REPO/issues/<n>/comments -f body=@<file>\` against an issue you created there for this purpose — read it back with \`-H "Accept: application/vnd.github.html+json"\`, and rule on the returned HTML (and, for mention claims, the timeline events). The observation is the verdict; quote it. This is the ONLY write destination other than \`submit\`'s that any part of this review may touch, it is user-designated, and nothing about the PR under review, its code, or its authors may appear in what you post there — post the minimal payload shape, not the report.
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/skills/bundled/review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,10 @@ Each entry carries `id` (unique — outcomes and resolved anchors both join on i

Apply each finding to the working tree with the `edit` tool — Criticals and the reuse/simplification/consistency findings alike. **Skip** any finding whose fix would change intended behaviour, would require changes well outside the reviewed diff, or that you judge on a second look to be a false positive. Note the skip; do not argue with it in prose.

**A test you add with a fix earns its place by failing without the fix — so remove the fix and watch it fail.** Not a formality: measured on this pipeline's own PRs, four assertions written to pin a real defect all survived the mutation they were written for. `expect(body).toContain('"index":0')` passed with the tool-call index deleted, because `"index":0` also appears on every `choices` entry. `expect(body).toContain('input_json_delta')` passed with the arguments handed over as a finished object, because the mutation kept the type and changed the field. `expect(wrapScript(s)).toMatch(/set \+e/)` asserted the mechanism rather than the behaviour, and `set +e` has no bearing on the `exit` that broke it. A pure function tested alone passed while the request path called a different one entirely.

The shapes that survive are all the same shape: an assertion that a **string is present** rather than that the **behaviour holds**. Parse and assert structurally, drive the real path rather than its helper, and confirm the removal actually reddens the test you just wrote. A test that cannot fail is a fix nobody can keep.

Then record what happened to **every** finding — one of `fixed`, `skipped`, or `no_change_needed` — as a JSON array of `{id, outcome, note?}`, and merge it back:

```bash
Expand Down
Loading