PR review never converges: each pass reports a few findings and every push starts a new pass

[SaulMoro]

The automated PR review reports a few findings per pass, and every push starts a new pass. A PR with many latent findings needs one push per handful, so it never converges.

PR #698 measured (passes that completed; cancel-in-progress: true drops the ones a new push interrupted):

Review passes	12
Findings per pass	2, 5, 3, 4, 3, 3, 2, 2, 2, 4, 3, 4
Total findings	37
Most in any one pass	5

flowchart LR
    A[Author pushes] --> B[Review fires on synchronize]
    B --> C[Reports a few of what it found]
    C --> D[Author fixes those]
    D --> A

Loading

The placement collision bug was visible in the round 1 diff. The review raised it in round 6.

Cause 1: nothing tells the review to report everything, and it cannot see earlier passes

.github/workflows/codex-review-on-assign.yml

- Flag bugs, risks, and rule violations. Be concise and specific.
+ Flag bugs, risks, and rule violations. Keep each finding short and never
+ cap how many you report: a finding present in this diff that you leave
+ for a later pass costs the author another round trip. The severity label
+ ranks them; the list does not need to.

Codex runs read-only on the base checkout with no GitHub token, so it cannot read the PR's earlier review comments. A rule in AGENTS.md asking it to would have no effect. The workflow has to feed them in:

   - name: Fetch PR head objects (data only, never executed)
+  - name: Collect earlier review comments
+    id: prior
+    uses: actions/github-script@v7
+    # issues.listComments, keep the github-actions[bot] ones, join as text
   - name: Run Codex review
       prompt: |
+        Earlier passes of this review already raised the findings below.
+        A finding already fixed in the current diff is resolved: say so,
+        do not raise it again.
+        ----
+        ${{ steps.prior.outputs.comments }}

Cause 2: the review reads the PR body from the moment the push landed

${{ github.event.pull_request.body }} is captured when the workflow fires. A body edited after the push is invisible to that pass.

sequenceDiagram
    participant Author
    participant Workflow
    Author->>Workflow: push
    Workflow->>Workflow: snapshot body and head
    Author->>Author: update PR body with the new test record
    Workflow-->>Author: "the test plan names an older commit"

Loading

This produced a false blocking finding in one pass, and two already fixed findings in another. Two changes:

.github/workflows/codex-review-on-assign.yml: re-read the body right before the Codex step instead of using the event snapshot. It is still a snapshot, but taken after the author's usual post-push edit.

+  - name: Read current PR body
+    id: pr
+    uses: actions/github-script@v7
+    # pulls.get -> outputs.body
   - name: Run Codex review
       prompt: |
-        ${{ github.event.pull_request.body }}
+        ${{ steps.pr.outputs.body }}

AGENTS.md

  - The PR description must document sufficient testing...
+   A test record naming an older commit than the head is a note, not a
+   blocking finding. The body may have been edited after this pass started.

Cause 3: test files hand-list the exports of config.js

One mechanical error cost four separate debugging detours in PR #698.

52  files write  vi.mock('../config.js', () => ({ ... }))
 6  files spread importOriginal() and override a few keys
16  exports in src/config.ts

Production code that calls an export a given test did not list fails that test with No "X" export is defined on the mock. No linter catches a missing key in a vi.mock factory. A shared helper does:

// src/__tests__/helpers/mock-config.ts
export const mockConfig = (overrides: Partial<typeof import('../../config.js')> = {}) =>
  vi.mock('../config.js', async (importOriginal) => ({ ...(await importOriginal()), ...overrides }));

Migrating the 52 literal factories to it removes the class of failure.

Cause 4: fork PRs get no e2e in CI, and the docs do not name the e2e command

AGENTS.md requires a real CLI verification record. CI does run the full e2e surface (E2E (GitHub provider, full surface) runs vitest run --config vitest.e2e.config.ts), but that job is gated on vars.TEAMAI_TEST_REPO_URL != '', so a PR from a fork skips it and the rule holds only because a reviewer reads prose about it.

CLAUDE.md and AGENTS.md list npx vitest run in their command line and not npm run test:e2e, so an agent has to find the split on its own.

- Commands: `npm run build`, `npx tsc --noEmit`, `npx vitest run`.
+ Commands: `npm run build`, `npx tsc --noEmit`, `npx vitest run`, `npm run test:e2e`.

What is not the review's fault

Some passes found real defects, and some of those defects came from the previous pass's fix. Adding a mechanism to fix a mechanism makes the chain longer. A self review of the whole branch diff before pushing caught half of one pass's findings before the review saw them, which suggests the same habit written down would shorten the chain.

  ## PR 前测试
+ ## Self review before push
+ Review the whole branch diff, not only the last change. For every piece of
+ shared state, list every reader and every writer.

Suggested split

Causes 1 and 2 are one workflow PR. Cause 3 is a test-only PR. Cause 4's doc line is trivial; the fork gap is a CI decision on its own.

[jeff-r2026]

Thanks for the report. We will fix this later

[ydflow]

I'd like to take this one.

dvd233's #734 was closed because the description reported vitest / test:e2e failures and had no real CLI run, and it explicitly left #731 open for a compliant revision. I'd pick it up from there rather than start over: same two causes from this report, but with every Test Plan item actually passing and a real-CLI record in the description, as AGENTS.md requires.

Two things I want to flag before I open a PR, since they shape the design:

1. Feeding earlier passes back in is the part with real risk. The workflow runs on pull_request_target and its whole security model rests on treating PR data as passive. Adding a github-script step that reads the PR's comment history means the review prompt now contains attacker-controlled text. That is not automatically unsafe — it is data, not instructions, and the checkout stays on the base commit with permission-profile: :read-only — but it does widen what reaches Codex, so I'd like to keep the step strictly read-only, cap what it pulls in, and call the tradeoff out in the PR description rather than smuggle it in silently.

2. The uncapped-report change is the cheaper half. Telling the review to report every finding it sees in one pass needs no new permissions and no new data flow, so I'd like to land it on its own if you would rather review that separately.

So, before I spend the effort: is this still free, or have you already scheduled it on your side? And would you prefer one PR carrying both halves, or the prompt change first and the history-replay change as a follow-up?

[jeff-r2026]

Thanks for offering to take this — we already landed a fix in #739, so no need to open another PR. Appreciate the security notes; the review workflow now reads prior comments as untrusted data with a size cap.