Skip to content

fix(import): reject interrupted GitHub PR responses - #1189

Merged
jeff-r2026 merged 1 commit into
Tencent:mainfrom
ydflow:fix/github-import-response-errors
Oct 17, 2026
Merged

jeff-r2026 merged 1 commit into
Tencent:mainfrom
ydflow:fix/github-import-response-errors

Conversation

@ydflow

@ydflow ydflow commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

When the GitHub REST fallback receives headers and part of a PR response before the connection closes, it listens for errors on the request but not the response. The fetch promise never settles. With the built CLI, import --from-mr <url1> <url2> exits 0 during the first PR, without reporting its failure or processing the second PR.

Listen for response errors in both the JSON and diff readers so the existing per-MR catch reports the failure, continues to the next PR, and exits nonzero. This covers metadata, commit-list, and diff reads. It complements #1150's HTTP status handling; no retries, new options, or state changes are introduced.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Test Plan

  • npm run build
  • npm run typecheck
  • npm run lint
  • npx vitest run src/__tests__/github-mr-fetch.test.ts src/__tests__/import-mr.test.ts src/__tests__/gitlab-provider.test.ts --maxWorkers=2 — 85 passed.
  • npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/import-mr-response-errors.test.ts --retry=0 — 3 passed with the built CLI on Windows / Node 24.
  • Added unit and real-CLI regressions for interrupted metadata, commits, and diff responses.

Red/green evidence: all three new unit cases fail on unchanged main. All three real-CLI cases fail on the unchanged build because it exits 0 before the second PR; after the fix they report both failed PRs and exit 1, without reaching AI extraction or writing a learning. The E2E uses a local HTTP server that sends a partial response and destroys the socket. A preload routes the GitHub request to that server and makes gh pr unavailable; the CLI and response stream are real. No external API or model is required.

The full Windows unit attempt was stopped after 29 failing files had been reported; it did not complete or pass. Nine failing files were re-run on a separate unmodified fe3b906 worktree and reproduced the same 109 failures (873 passed, 71 skipped), including unavailable POSIX shell execution and symlink EPERM. Other Windows failures have not been individually triaged. The attempt and baseline logs are retained; the repository's Ubuntu/macOS CI checks the supported runner behavior. The focused import tests and real-CLI regressions above passed locally.

Related Issues

Found during source review. Related, distinct fix: #1150. No duplicate open PR found during submission checks.

Notes for Reviewers

The existing English/Chinese knowledge guide already promises that a failed MR does not stop the remaining imports and that any failure produces a nonzero exit. This restores that contract, with no CLI/help or skill workflow changes. Runtime changes are just the two response error listeners; no shared persisted state is added.

@github-actions

Copy link
Copy Markdown

No findings in the scoped diff.

The two response-error listeners reject interrupted reads through the existing promise error path. Regression tests cover metadata, commits, and diff responses, including continuation to the second PR.

The PR description satisfies the testing requirement: it records three passing real-CLI E2E cases using the built CLI and documents baseline failures separately.

Review was read-only; I did not run, build, or install anything.

@jeff-r2026
jeff-r2026 merged commit a3ac3b4 into Tencent:main Oct 17, 2026
14 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.

2 participants