From e11f7f4febf0c49569400b4f1ca64d9dea4faf43 Mon Sep 17 00:00:00 2001 From: ydflow <314143294+ydflow@users.noreply.github.com> Date: Thu, 22 Oct 2026 20:34:40 +0800 Subject: [PATCH] fix(import): paginate GitHub PR commits in REST fallback Signed-off-by: ydflow <314143294+ydflow@users.noreply.github.com> --- docs/guide/knowledge.md | 2 + docs/guide/zh-CN/knowledge.md | 2 + .../e2e/import-mr-commit-pagination.test.ts | 136 ++++++++++++++++++ src/__tests__/github-mr-fetch.test.ts | 40 +++++- src/providers/github/mr-fetch.ts | 4 +- 5 files changed, 177 insertions(+), 7 deletions(-) create mode 100644 src/__tests__/e2e/import-mr-commit-pagination.test.ts diff --git a/docs/guide/knowledge.md b/docs/guide/knowledge.md index c1191ee61..78c503f49 100644 --- a/docs/guide/knowledge.md +++ b/docs/guide/knowledge.md @@ -130,6 +130,8 @@ teamai import --from-repo https://github.com/org/repo --skip-enrich `--from-mr` takes several MR URLs. Each draft is still confirmed on its own (or none with `--all`), the accepted learnings are published once at the end, and each repository's teamwiki is updated once. An MR that fails does not stop the rest; the run ends with a summary and exits non-zero if any failed. +When GitHub import falls back from `gh` to REST, it reads commit pages of 100, up to the [GitHub endpoint's 250-commit limit](https://docs.github.com/en/rest/pulls/pulls#list-commits-on-a-pull-request). A failed commit page fails that PR before AI extraction. + Each GitLab MR API request has a 30-second deadline, including reading its response body. A metadata timeout fails that MR; a commits, diff or review-threads timeout leaves out that optional data and continues. Each page of review threads gets its own deadline. The extraction reads the MR's review threads along with its description, commits and diff: GitLab discussions, GitHub review comments, reviews and conversation comments, TGit notes. Each thread goes into the prompt with the file and line it is anchored to, whether it is resolved, and its notes marked as the MR author's or a reviewer's; a decision in a resolved thread outweighs the diff. teamai's own CI comments are skipped. `--no-discussions` leaves the threads out, for `teamai ci extract-mr` too. With `language: en` in `teamai.yaml` the learning is written in English; otherwise in Chinese. diff --git a/docs/guide/zh-CN/knowledge.md b/docs/guide/zh-CN/knowledge.md index f39b349b0..fa12c23aa 100644 --- a/docs/guide/zh-CN/knowledge.md +++ b/docs/guide/zh-CN/knowledge.md @@ -130,6 +130,8 @@ teamai import --from-repo https://github.com/org/repo --skip-enrich `--from-mr` 可以一次传多个 MR URL：每个草稿照旧单独确认（`--all` 则都不问），接受的 learning 在最后一次性发布，每个仓库的 teamwiki 只更新一次。其中一个 MR 失败不影响其余，运行结束时给出汇总，有失败则退出码非零。 +GitHub 导入从 `gh` 回退到 REST 时，会按每页 100 个提交继续读取，最多返回 [GitHub 此接口限制的 250 个提交](https://docs.github.com/en/rest/pulls/pulls#list-commits-on-a-pull-request)。任一提交页失败都会使该 PR 在 AI 提炼前失败。 + 每个 GitLab MR API 请求都有 30 秒期限，包含读取响应正文。元信息超时会使该 MR 失败；commits、diff 或评审 threads 超时则省略相应可选数据并继续处理。评审 threads 的每一页都有独立期限。 提炼时除了 MR 的描述、提交和 diff，还会读取评审 threads：GitLab 的 discussions，GitHub 的 review 评论、review 结论和会话评论，TGit 的 notes。每个 thread 带上它锚定的文件和行、是否已 resolved，以及按顺序标为 MR 作者或评审者的各条 note 进入提示词；已 resolved 的 thread 里的决定比 diff 更重要。teamai 自己的 CI 评论会被跳过。`--no-discussions` 不读取 threads，`teamai ci extract-mr` 同样适用。`teamai.yaml` 设了 `language: en` 时 learning 用英文写，否则用中文。 diff --git a/src/__tests__/e2e/import-mr-commit-pagination.test.ts b/src/__tests__/e2e/import-mr-commit-pagination.test.ts new file mode 100644 index 000000000..c05c2b310 --- /dev/null +++ b/src/__tests__/e2e/import-mr-commit-pagination.test.ts @@ -0,0 +1,136 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { spawn } from 'node:child_process'; +import fs from 'node:fs'; +import http from 'node:http'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../../..'); +const CLI = path.join(ROOT, 'dist/index.js'); +const PR = '/repos/team/repo/pulls/42'; +const COMMITS = Array.from({ length: 101 }, (_, i) => ({ + sha: `commit-${i + 1}`, commit: { message: `Change ${i + 1}\n\nDetails` }, +})); + +describe('import --from-mr with paginated GitHub commits', () => { + let sandbox: string; + let preload: string; + + beforeEach(() => { + sandbox = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-mr-commits-')); + const repo = path.join(sandbox, 'repo'); + fs.mkdirSync(repo); + fs.writeFileSync(path.join(repo, 'teamai.yaml'), 'team: pagination\nrepo: https://example.com/team.git\nprovider: git\nlanguage: en\n'); + fs.mkdirSync(path.join(sandbox, '.teamai')); + fs.writeFileSync(path.join(sandbox, '.teamai/config.yaml'), [ + 'repo:', ` localPath: ${JSON.stringify(repo)}`, ' remote: https://example.com/team.git', + 'username: test-user', 'scope: user', '', + ].join('\n')); + const ai = path.join(sandbox, 'ai.cjs'); + fs.writeFileSync(ai, `process.stdout.write(${JSON.stringify('---\ntitle: Preserved history\n---\nFixture learning.\n')});\n`); + preload = path.join(sandbox, 'github-fixture.mjs'); + // External boundaries only: actual HTTP responses and the packaged CLI's + // extraction prompt, with a deterministic subprocess replacing the AI CLI. + fs.writeFileSync(preload, ` +import childProcess from 'node:child_process'; +import fs from 'node:fs'; +import http from 'node:http'; +import https from 'node:https'; +import { syncBuiltinESMExports } from 'node:module'; +const execSync = childProcess.execSync; +childProcess.execSync = (command, ...args) => { + if (command.startsWith('gh pr ')) throw new Error('gh unavailable in fixture'); + return execSync(command, ...args); +}; +const execFileSync = childProcess.execFileSync; +childProcess.execFileSync = (command, args, ...rest) => { + if ((command === 'where' || command === 'which') && args[0] === 'claude' + || (command === 'bash' || command === 'zsh') && args[1] === 'command -v claude') return process.execPath; + return execFileSync(command, args, ...rest); +}; +const spawn = childProcess.spawn; +childProcess.spawn = (command, args, options) => { + if (command === process.execPath && args[0] === '-p') { + fs.writeFileSync(process.env.PROMPT_FILE, args[1]); + return spawn(process.execPath, [${JSON.stringify(ai)}], options); + } + return spawn(command, args, options); +}; +const request = https.request; +https.request = (options, callback) => options.hostname === 'api.github.com' + ? http.request({ ...options, protocol: 'http:', hostname: '127.0.0.1', port: process.env.FIXTURE_PORT }, callback) + : request(options, callback); +syncBuiltinESMExports(); +`); + }); + + afterEach(() => { if (sandbox) fs.rmSync(sandbox, { recursive: true, force: true }); }); + + it.each([false, true])('reads later commits before extraction (second page fails: %s)', async (failSecondPage) => { + const pages: number[] = []; + const server = http.createServer((req, res) => { + const url = new URL(req.url!, 'http://fixture'); + if (url.pathname === `${PR}/commits`) { + const page = Number(url.searchParams.get('page') ?? 1); + const size = Number(url.searchParams.get('per_page')); + pages.push(page); + if (failSecondPage && page === 2) { + res.writeHead(429); + res.end(JSON.stringify({ message: 'Too many requests' })); + } else { + res.end(JSON.stringify(COMMITS.slice((page - 1) * size, page * size))); + } + } else if (req.headers.accept?.includes('.diff')) { + res.end('diff --git a/a b/a\n+fixed\n'); + } else if (url.pathname === PR) { + res.end(JSON.stringify({ title: 'Preserved history', body: '', user: { login: 'dev' }, merged_at: null })); + } else { + res.writeHead(404); + res.end(); + } + }); + await new Promise((resolve) => { server.listen(0, '127.0.0.1', resolve); }); + try { + const address = server.address(); + if (!address || typeof address === 'string') throw new Error('Expected a TCP address'); + const promptFile = path.join(sandbox, 'prompt.txt'); + const outputDir = path.join(sandbox, 'output'); + const result = await new Promise<{ code: number | null; output: string }>((resolve, reject) => { + const child = spawn(process.execPath, [ + '--import', pathToFileURL(preload).href, CLI, 'import', '--from-mr', 'https://github.com/team/repo/pull/42', + '--all', '--no-discussions', '--output', outputDir, + ], { + cwd: sandbox, + env: { + ...process.env, HOME: sandbox, USERPROFILE: sandbox, FORCE_COLOR: '0', + GITHUB_TOKEN: '', GH_TOKEN: '', FIXTURE_PORT: String(address.port), PROMPT_FILE: promptFile, + }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let output = ''; + child.stdout.on('data', (chunk: Buffer) => { output += chunk.toString(); }); + child.stderr.on('data', (chunk: Buffer) => { output += chunk.toString(); }); + const timer = setTimeout(() => { child.kill(); reject(new Error(`CLI timed out: ${output}`)); }, 20_000); + child.on('error', (error) => { clearTimeout(timer); reject(error); }); + child.on('close', (code) => { clearTimeout(timer); resolve({ code, output }); }); + }); + expect(result.code, result.output).toBe(failSecondPage ? 1 : 0); + expect(pages).toEqual([1, 2]); + if (failSecondPage) { + expect(result.output).toContain('GitHub API error 429'); + expect(fs.existsSync(promptFile)).toBe(false); + expect(fs.existsSync(outputDir)).toBe(false); + } else { + const prompt = fs.readFileSync(promptFile, 'utf8'); + expect(prompt.match(/^- [^:]+: Change \d+$/gm)).toHaveLength(101); + expect(prompt).toContain('Change 51'); + expect(prompt).toContain('Change 101'); + expect(fs.readFileSync(path.join(outputDir, 'learning.md'), 'utf8')).toContain('Fixture learning.'); + } + } finally { + server.closeAllConnections(); + await new Promise((resolve, reject) => { server.close((error) => error ? reject(error) : resolve()); }); + } + }); +}); diff --git a/src/__tests__/github-mr-fetch.test.ts b/src/__tests__/github-mr-fetch.test.ts index bb5bdd012..51c3051bf 100644 --- a/src/__tests__/github-mr-fetch.test.ts +++ b/src/__tests__/github-mr-fetch.test.ts @@ -33,7 +33,13 @@ const CONVERSATION = [ { body: 'Merging after the split.', user: { login: 'dev' } }, ]; -function mockResponses(failedPath?: string, status = 200, body = '', responseError?: Error): string[] { +function mockResponses( + failedPath?: string, + status = 200, + body = '', + responseError?: Error, + commits = [{ sha: 'abc123', commit: { message: 'Fix retries\n\nDetails' } }], +): string[] { const seen: string[] = []; const request = (options: https.RequestOptions, callback: (res: IncomingMessage) => void) => { const url = String(options.path); @@ -41,12 +47,15 @@ function mockResponses(failedPath?: string, status = 200, body = '', responseErr const diff = options.headers && (options.headers as Record).Accept.includes('.diff'); const key = diff ? `${url}.diff` : url; const failed = key === failedPath; + const params = new URL(url, 'https://api.github.com').searchParams; + const pageSize = Number(params.get('per_page') ?? 100); + const start = (Number(params.get('page') ?? 1) - 1) * pageSize; const response = failed ? body : diff ? 'diff --git a/a b/a\n+fixed\n' : url === PR ? JSON.stringify(METADATA) : url.startsWith(`${PR}/comments`) ? JSON.stringify(REVIEW_COMMENTS) : url.startsWith(`${PR}/reviews`) ? JSON.stringify(REVIEWS) : url.startsWith('/repos/team/repo/issues/42/comments') ? JSON.stringify(CONVERSATION) - : JSON.stringify([{ sha: 'abc123', commit: { message: 'Fix retries\n\nDetails' } }]); + : JSON.stringify(commits.slice(start, start + pageSize)); const req = Object.assign(new EventEmitter(), { setTimeout: vi.fn(), destroy: vi.fn(), @@ -75,7 +84,7 @@ function mockResponses(failedPath?: string, status = 200, body = '', responseErr afterEach(() => { vi.restoreAllMocks(); }); describe('GitHub PR REST fallback', () => { - it.each([PR, `${PR}/commits?per_page=50`, `${PR}.diff`])( + it.each([PR, `${PR}/commits?per_page=100&page=1`, `${PR}.diff`])( 'rejects when the response stream fails at %s', async (endpoint) => { const error = new Error('aborted'); const seen = mockResponses(endpoint, 200, 'partial response', error); @@ -98,7 +107,7 @@ describe('GitHub PR REST fallback', () => { }); it('reports a commits endpoint error instead of calling map on an error object', async () => { - mockResponses(`${PR}/commits?per_page=50`, 429, '{"message":"Too many requests"}'); + mockResponses(`${PR}/commits?per_page=100&page=1`, 429, '{"message":"Too many requests"}'); await expect(fetchGitHubPR('https://github.com/team/repo/pull/42')) .rejects.toThrow('GitHub API error 429: {"message":"Too many requests"}'); }); @@ -117,6 +126,29 @@ describe('GitHub PR REST fallback', () => { diff: 'diff --git a/a b/a\n+fixed\n', }); }); + + it.each([0, 1, 50, 51, 100, 101, 250])('paginates %i commits without dropping or reordering them', async (count) => { + const commits = Array.from({ length: count }, (_, i) => ({ + sha: `commit-${i + 1}`, commit: { message: `Change ${i + 1}\n\nDetails` }, + })); + const seen = mockResponses(undefined, 200, '', undefined, commits); + const mr = await fetchGitHubPR('https://github.com/team/repo/pull/42', { discussions: false }); + expect(mr.commits).toEqual(commits.map((c) => ({ hash: c.sha, message: c.commit.message.split('\n')[0] }))); + expect(seen.filter((u) => u.includes('/commits?'))).toEqual( + Array.from({ length: Math.floor(count / 100) + 1 }, (_, i) => `${PR}/commits?per_page=100&page=${i + 1}`), + ); + }); + + it.each([ + [429, '{"message":"Too many requests"}', undefined, 'GitHub API error 429'], + [200, '{"message":"Not a commit list"}', undefined, 'GitHub API returned no list'], + [200, 'partial response', new Error('aborted'), 'aborted'], + ] as const)('rejects a failed later commit page (%i, %s) instead of returning partial history', async (status, body, error, message) => { + const commits = Array.from({ length: 100 }, (_, i) => ({ sha: `commit-${i}`, commit: { message: `Change ${i}` } })); + const seen = mockResponses(`${PR}/commits?per_page=100&page=2`, status, body, error, commits); + await expect(fetchGitHubPR('https://github.com/team/repo/pull/42', { discussions: false })).rejects.toThrow(message); + expect(seen.at(-1)).toBe(`${PR}/commits?per_page=100&page=2`); + }); }); describe('GitHub PR review threads (#1187)', () => { diff --git a/src/providers/github/mr-fetch.ts b/src/providers/github/mr-fetch.ts index 903f24bd6..0842d4690 100644 --- a/src/providers/github/mr-fetch.ts +++ b/src/providers/github/mr-fetch.ts @@ -199,9 +199,7 @@ async function fetchGitHubPRViaApi(owner: string, repo: string, number: string): const pr = await githubApiGet(`/repos/${owner}/${repo}/pulls/${number}`) as GitHubApiPR; // ── 2. 提交列表 ────────────────────────────────────────── - const commitsRaw = await githubApiGet( - `/repos/${owner}/${repo}/pulls/${number}/commits?per_page=50`, - ) as GitHubApiCommit[]; + const commitsRaw = await readPages(githubApiGet, `/repos/${owner}/${repo}/pulls/${number}/commits`); const commits = commitsRaw.map((c) => ({ hash: c.sha, message: c.commit.message.split('\n')[0],