review-pr

Review PR

Most past regressions in this repository were not logic errors, but rather "the change was correct relative to its own intent, but quietly changed a behavior that some existing group depends on": narrowed dialect branches, flipped defaults, deleted system prompt sentences, lost functionality of old paths during refactoring or porting, and startup timing changes introduced by fixes. A general review would exclude this kind of "intentional feature change" from being a bug, so here it is called out as a separate round.

There is only one review criterion:Any input that worked before the change (config keys, environment variables, command-line arguments, provider response shapes, session data written by older versions, client requests, hook payloads) must behave unchanged after the change, unless the PR explicitly declares this change and provides a fallback.

Workflow

Pull PR metadata and description:

gh pr view <number> --json title,body,url,files,additions,deletions,baseRefName,headRefName

Read the full diff (for large PRs, read it in batches by file):
```
gh pr diff <number>
```

Read enough surrounding code to verify every assertion in the description. Looking only at the diff, you cannot tell "who reached this branch before the change".
First pass: characterization. Determine the type of change to decide the depth of the third pass; see "First pass: characterization".
Second pass: check section by section against the template. See "Section-by-section template check".
Third pass: regression and user impact. Run it with an independent sub-agent if possible: give it only the PR number, base and head, a two- or three-sentence summary of the change, and the path of surfaces.md , without giving it the author's conclusions about the scope of impact; have it first independently list items from the diff and code, and after it returns, compare with the table filled in by the author. If sub-agent capability is unavailable, do it yourself in the same order: first list the items, then read the author's table. See "Third pass: regression and user impact".
Write the review summary in the language the user is currently using; for the format, see "Output".

First pass: characterization

Type of change	Depth of third pass
Purely internal refactoring, tests, documentation, test CI	Only provide evidence of "no observable change": which branches, defaults, and contract files were not touched
Release and packaging: release, native builds, VS Code release workflow, packaging scripts, web artifact sync	Do all of it, with groups split by installation method and platform: npm package, native binaries for each platform, VS Code Marketplace, bundled web artifacts
New feature that does not touch existing paths	Only check whether the new branch has taken over existing inputs: whether the new condition is placed before the old condition
Behavior changes, flipped defaults, tightened or loosened validation or permissions	Do all of it
Prompt text changes: system prompt, tool descriptions, reminders, overlay, built-in skills	Do all of it, and put each sentence change on its own line
Porting, replacing old paths, major refactoring: v1 to v2, runtime binding, protocol replacement	Do all of it, and require the author to provide a list of old-path functionality
Changes to protocols, disk formats, SDK exports, hook payloads, command-line contracts	Do all of it, and name the consumers outside the repository
Fix-type changes that alter startup order, lifecycle, or asynchronous timing	Do all of it, and focus on who depends on the original order

A PR can belong to multiple categories at the same time; handle it according to the heaviest category.

Third pass: regressions and user impact

1. Behavior change list

List every observable behavior changed by the diff, including those the author says are "unchanged" and those the author considers "internal". Internal packages get shipped in the CLI release package; "internal" does not mean users won't notice.

Record each row: behavior, before the change, after the change, change type (added / modified / removed / default flipped / tightened / loosened), evidence (one file:linebefore and after the change; when one side does not exist, give a reachable path).

How to read:

For every deleted or narrowed branch, condition, default value, or sentence, ask "who reached this before the change, and where did they go after the change".
For every added branch or condition, ask "which existing inputs will now hit the new branch first". #3492's regression arose exactly this way: when an array field exists, the string field is no longer read, while OpenRouter-style gateways send both at the same time.
In tests, changes that "turn old assertions into new behavior" are direct evidence of behavior changes; map each one to the list.
First check whether the diff file list hits surfaces.md 's "trigger clues". Any hit surface must have a corresponding row in the list; otherwise it is a missed item.

2. Affected populations

Each row corresponds to a specific population in surfaces.md ; write the population name and the evidence that "this population exists": documentation pages, issues, configuration examples, provider payload shapes. If no population can be found, it does not go into findings; it goes into "pending author confirmation".

3. Impact level

The level is determined only by the facts of the change, not by remediation states such as changesets, documentation, or tests. Three-step determination:

Data loss, relaxation of a security boundary, old data becoming unreadable, or a population's functionality becoming entirely unavailable: directly L3.
Otherwise, perceptible and with a one-key fallback (config, env, flag): L1; perceptible and with no fallback, old behavior unrecoverable: L2; no observable change: L0.
Silent upgrade: user-provided input (configuration, environment variables, data, request fields) is ignored or discarded, with no error signal at all, and users cannot discover it on their own: raise one level, up to L3. If the behavior changes publicly and users can see it in the output, it does not count as silent.

Three breadth tiers; the population name must be written: all users / a certain configuration or platform population / edge environment.

Each row is graded separately; the PR's level is the highest. Reason for silent upgrade: with no signal there is no report, and the regression will survive until someone happens to discover it.

4. Remediation check

Four fixed questions; answer each with yes / no / not applicable, and give evidence:

Fallback switch: whether there is a config, env, or flag that can restore the old behavior.
changeset: whether it points out what userslost, rather than just what the new default is; and when there is no fallback, whether it states explicitly that the old behavior is unrecoverable.
Old data and old clients: whether sessions written by old versions, migrated data, or clients released in the previous version (desktop, web, VS Code, ACP) are still usable; and whether rolling back to the previous version is safe.
guard test: whether there is a test that pins downthe unchanged population's old behavior.

Add one more question:docs/en and docs/zh whether the commitment to this surface is updated in sync.

Handling requirements by level

L3: recommend blocking. A guard test must be added, and a fallback, flag gating, or migration must be provided; none may be omitted.
L2: the maintainer must explicitly make the call in the PR; the changeset must state the lost behavior; docs must be updated in sync.
L1: just check the changeset and docs.
L0: explain in one sentence why it is L0, and give evidence.

The line between a finding and "pending author confirmation"

A finding must have both: one file:line piece of evidence each before and after the change, and a named group of people plus evidence of their existence. If either is missing, downgrade it to "pending author confirmation". When an entire file is added or a whole implementation is deleted, one side is naturally empty; in that case, add to the side that exists the file:line of the affected reachable path.
"Pending author confirmation" items: at most 5, sorted by possible impact level; for any extra, just report the count.
Do not report: issues that already existed before the PR (for serious ones, a single separate sentence as "incidental finding"), issues that lint and type checking would catch, or style issues.
Do not treat these as evidence of "no impact": the author saying "unchanged", no tests referencing the deleted sentence, or the change being only in an internal package.

Section-by-section template check

When the output is in Chinese, use the translated section names in the table below; for other languages, keep the section names from the original template:

English	Chinese
Requirement or Bug	Requirement or Bug
Bug Reproduction Steps	Bug reproduction steps
Root Cause	Root cause
Code Changes	Code changes
Behavior Changes and Affected Users	Behavior changes and affected groups
Checklist	Checklist

If an old PR still uses the Impact Scope section, check it against the "Behavior changes and affected groups" standard.

Requirement or Bug

Is the linked issue valid and relevant?
If there is no issue, is the requirement explained clearly in one or two sentences?

Bug reproduction steps

For bug-type PRs: are the steps clear and reproducible?
Following the steps, can the bug be confirmed on the base branch?

Root cause

For bug-type PRs: is the root cause explanation convincing?
Is the described root cause consistent with the diff?
Is it a root fix or a workaround, and is that made clear?

Code changes

Is the description consistent with the actual diff?
Are the visual outlines (diff blocks, call trees, file trees) accurate and helpful?
Is the approach reasonable? Is there a simpler alternative?
Are there edge cases the author missed?

Behavior changes and affected groups

Compare the list you independently produced in the third pass with the table filled in by the author line by line: behaviors the author omitted are findings; those the author wrote but you could not find evidence for go into "pending author confirmation".
Are the groups named, or is it just "some users"?
For those that wrote None , does the evidence hold?
Are the affected modules and test coverage consistent with the diff file list? Are there risky untested paths?

Checklist

Are all applicable items checked?
Do you agree with the items marked "not needed"?

Output

Use the language the user is currently using, in the order below; the example is in Chinese:

## 结论
一句话：可合 / 需修改 / 建议阻塞。附 PR 影响等级与改动类型。

## 回归与用户影响
- 改动类型：…
- 影响等级：Lx（广度：…；是否静默：…）

| 行为 | 变化前 | 变化后 | 受影响人群 | 证据 | 等级 | 退路 |
|---|---|---|---|---|---|---|

补救核对：
- 退路开关：…
- changeset 是否点明失去的行为：…
- 老数据与老客户端：…
- guard 测试：…
- 文档：…

待作者确认（最多 5 条）：
1. …

## 模板逐节核对
逐节说明是否填写充分，指出缺失、不准确、与 diff 不一致之处。

## 需要修改的项
可执行的列表；没有则写"无"。

Retrospective backfill

After each online regression retrospective, add the newly discovered audience segments or trigger clues to surfaces.md, and add a line in its "Historical Cases." If the checklist is not updated, it will become outdated.