Skip to content

fix(ci): reset stale PR review context after history changes - #301

Merged
Sun-sunshine06 merged 1 commit into
OpenCoworkAI:mainfrom
Sun-sunshine06:fix/bot-review-rebase-context
Jul 14, 2026
Merged

fix(ci): reset stale PR review context after history changes#301
Sun-sunshine06 merged 1 commit into
OpenCoworkAI:mainfrom
Sun-sunshine06:fix/bot-review-rebase-context

Conversation

@Sun-sunshine06

Copy link
Copy Markdown
Collaborator

Summary

  • Reuse prior bot-review context only when the reviewed head is a verified ancestor on a complete, merge-free linear extension.
  • Discard prior review IDs and run a fresh full review after rebase, force-push, rollback, merge commits, incomplete comparisons, or ancestry-check failures.
  • Treat the current Files Changed list and current PR diff as authoritative in both Codex and DeepSeek review paths, with paginated PR file loading.
  • Prevent an older matching review from suppressing a new run in an A -> B -> A history rollback.
  • Clarify that bot reviews are advisory; a green workflow run only reflects automation completion.

Why

PR #298 exposed the failure mode: the bot compared diverged heads (04bcc817...e91a849, 13 ahead / 7 behind) and inherited findings for files that were not in the PR's current seven-file diff. The workflow previously treated any different reviewed SHA as a valid follow-up.

The new checks fail closed: old review text and old-head compare data are available only for a verified safe linear update. All other cases reset review context and review the current PR from scratch.

Validation

  • npx vitest run tests/deepseek-common.test.ts tests/codex-pr-review-context.test.ts (11 tests passed)
  • npm run typecheck
  • npm run lint (0 errors; 8 existing warnings)
  • node --check for both changed DeepSeek scripts
  • Workflow YAML parse, Prettier check for tracked formatted file types, and git diff --check
  • Live GitHub compare replay for PR Fix/wsl sandbox bash tool wiring #298: diverged range rejected; linear range accepted

The review event remains COMMENT; this PR does not turn advisory findings into a merge gate.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review mode: initial

Findings

  • [MAJOR] listPullRequestFiles will return an empty array for any PR with files due to incorrect flatMap usage with --paginate --slurp – The gh api --paginate --slurp flag returns a single flattened JSON array of all items, not an array of per-page arrays. The added flatMap with Array.isArray check discards every item because each element is an object, not an array.

    Evidence: .github/scripts/deepseek-common.mjs:420-424

    const pages = JSON.parse(raw);
    return pages.flatMap((page) => (Array.isArray(page) ? page : []));

    This breaks the DeepSeek PR review path whenever the PR has at least one file — the file list will be [], so no file excerpts or patches are included.

    Suggested fix: Remove the flatMap and return JSON.parse(raw) directly:

    - const pages = JSON.parse(raw);
    - return pages.flatMap((page) => (Array.isArray(page) ? page : []));
    + const pages = JSON.parse(raw);
    + return Array.isArray(pages) ? pages : [];
  • [MAJOR] Test mock for listPullRequestFiles does not reflect actual gh api --paginate --slurp output – The test mocks the command to return '[[{"filename":"first.ts"}],[{"filename":"second.ts"}]]' (an array of arrays), but the real output is '[{"filename":"first.ts"},{"filename":"second.ts"}]' (a flat array). The test passes only because the implementation accidentally flattens array-of-arrays, which is the opposite of real behavior.

    Evidence: tests/deepseek-common.test.ts:116-122
    Mock returns a nested array, but actual gh api --paginate --slurp returns a single array.

    Suggested fix: Update the mock to return a flat array:

    - return '[[{"filename":"first.ts"}],[{"filename":"second.ts"}]]';
    + return '[{"filename":"first.ts"},{"filename":"second.ts"}]';

    And adjust the expectation or the implementation accordingly (the implementation should not use flatMap — see above).

  • [MINOR] gh pr diff --name-only result is loaded but never used – In .github/prompts/codex-pr-review.md line 55, the command gh pr diff "$pr_number" -R "$repo" --name-only is run, but the output is not stored in a variable nor referenced later. It wastes an API call.

    Suggested fix: Remove the line unless it is needed for another purpose.

  • [NIT] In isLinearReviewUpdate, has_merge_commit field could be undefined for empty commit arrays – If comparison is provided but commits is empty (e.g., ahead_by=0), the function accesses comparison?.has_merge_commit which may be undefined. It will still compare === false, so it works, but explicit handling would be clearer.

    Suggested fix: No change needed, but consider adding a guard for clarity.

Summary

  • Review mode: initial
  • Review policy: advisory — the check reflects automation health/completion only; it does not approve the PR or resolve findings.
  • Major issues: The listPullRequestFiles pagination implementation is incorrect and will break the DeepSeek review path. The accompanying test is also wrong. These must be fixed before merging.
  • Minor: Unused command in prompt.
  • Other logic: Ancestry check and context-reset logic appear sound and correctly handle failures by discarding context.

Testing

  • New tests for isLinearReviewUpdate and PR file pagination were added, but the pagination test is invalid due to a mismatched mock. The existing tests for linear update correctness are good.
  • Suggested tests: Once the pagination bug is fixed, verify that listPullRequestFiles works with real gh output (e.g., by running against a PR with >100 files). Also add a test for the prior_context_discarded and is_follow_up_review output logic in the workflow.

Open Cowork Bot

@Sun-sunshine06

Copy link
Copy Markdown
Collaborator Author

I verified the pagination findings against the real GitHub CLI/API before changing the implementation, and they are false positives.

Using this PR with per_page=2 to force three pages:

{"outerIsArray":true,"outerLength":3,"firstIsArray":true,"pageLengths":[2,2,2],"firstPath":".github/prompts/codex-pr-review.md"}

gh help api also states that --slurp wraps all pages of JSON arrays or objects in an outer JSON array. Running the committed helper directly against PR #301 returns all six changed paths, so the nested mock and flatMap are intentional and match real output.

The gh pr diff --name-only command is also intentional: this is an agent prompt, so its stdout is supplied directly to the reviewing model as the authoritative current Files Changed list; it does not need a shell variable to be consumed.

No code change is needed for these findings. The live commands used were:

gh api --paginate --slurp "repos/OpenCoworkAI/open-cowork/pulls/301/files?per_page=2"
node --input-type=module -e "import {listPullRequestFiles} from './.github/scripts/deepseek-common.mjs'; console.log(listPullRequestFiles('OpenCoworkAI/open-cowork', '301'))"

@Sun-sunshine06
Sun-sunshine06 merged commit 6f0c047 into OpenCoworkAI:main Jul 14, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant