diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 738268aa964..6b7b7b60c89 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -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 \\ + --log /mock.jsonl --ttl 600 --out /mock.json & +until [ -s /mock.json ]; do sleep 0.1; done # its port is in that report +"\${QWEN_CODE_CLI:-qwen}" review drive --cwd --script \\ + --ready --timeout 300 --out /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 \`@\` → \`@\` 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//comments -f body=@\` 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. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index b74ad0f03ea..aaafe7e8343 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -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