diff --git a/CHANGELOG.md b/CHANGELOG.md index 330f8f25a..f0c5870f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ All notable changes to this project will be documented in this file. See [standa ### ✨ Features +- `teamai import --from-mr` and `teamai ci extract-mr` read the MR's review threads into the extraction: GitLab discussions, GitHub review comments, reviews and conversation comments, and TGit notes, without system notes or teamai's own CI comments. The threads get a section of the prompt with its own size budget, each 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; threads with a reply come first, a long thread is shortened to its first and last notes, and the model is told that a decision in a resolved thread outweighs a restatement of the diff. A failure to read the threads warns and extracts without them. `--no-discussions` keeps the previous behaviour. With `language: en` in `teamai.yaml` the learning is written in English, with English headings; otherwise it stays Chinese (for [#1187](https://github.com/Tencent/teamai-cli/issues/1187)). - A tool without the `teamai-recall` subagent (Pi, Hermes, OpenClaw) now gets a recall block too, which tells the agent to run `teamai recall ""` itself before code, debugging or design work, with the same skip conditions as the subagent block. It shares the subagent block's markers, so `recall disable`, uninstall and doctor treat both alike. Pi's README row now shows ✓ for learnings, codebase and teamwiki (for [#945](https://github.com/Tencent/teamai-cli/issues/945)). - A YAML agent can set `model: strong`, `model: fast`, or an alias the team defines in an optional `models/aliases.yaml`, which maps each alias per tool to that tool's own model value and an optional effort. `teamai pull` writes the mapped model into every tool's agent file, and the effort into that tool's own field: `effort` for Claude, claude-internal, tclaude, CodeBuddy, Qoder and Qoder CN, `model_reasoning_effort` for Codex, codex-internal and tcodex, `variant` for OpenCode. Cursor receives its model string as written, so its bracket form (`claude-opus-5[effort=high]`) carries the effort; Copilot receives the first entry as one model string. Copilot, Cursor, Kiro, WorkBuddy, JoyCode, ZCode and OMP get no effort field: an effort mapped for them is dropped, and pull warns once, naming the alias and the tool. The `-internal` and `t` variants use the `claude` or `codex` entry and Qoder CN the `qoder` entry unless the alias has their own key; no other tool inherits one. A tool the alias does not map, or any tool when the team has no aliases file, gets no `model` field and runs on its default, never a literal `strong`. `tool_extras..model` skips the alias for that tool, and an extras effort alone overrides the alias's effort. A `model` that is not a string now makes the agent fail to parse; a legacy `.md` agent whose `model` is an alias is copied as is with a warning. While `models/aliases.yaml` cannot be read, pull holds every agent that sets a `model`, since the file may define any name, and push skips them. Pull records the model each agent copy received, so an ordinary pull applies a changed resolution even when the team repo has not moved, which fixes an agent an older CLI wrote with `model: strong` as is, and delivers a copy that is missing; a copy the member edited is kept, and that pull names it with how to take the new model. Pull warns when an alias an agent resolved through is removed, also where the alias gave a tool no model field (for [#830](https://github.com/Tencent/teamai-cli/issues/830)). - A member overrides a team model alias on their own machine in `~/.teamai/models/aliases.yaml`, in the same `aliases:` shape. An entry replaces the team's whole entry for that tool, effort included, and `~` or `default` gives the tool no model field and no effort; only `tool_extras..model` wins over it. Keys are `strong`, `fast` or an alias the team defines, so a member can map `strong` before the team has an aliases file, and any other name has no effect. The file applies in every scope and to every team using that alias name, and an ordinary pull applies an edit to it. While it cannot be read, pull holds the agents whose `model` is an alias, naming the file, and push skips them; since the file can make no name an alias, agents with a concrete model are delivered and pushed as usual. In the team file, `default` is written as a model value and `~` is an error. When pull keeps an edited copy whose deployed version changed, it now says `the version teamai would deploy there ... has changed since` instead of blaming the team, since the change may be the member's override (for [#830](https://github.com/Tencent/teamai-cli/issues/830)). diff --git a/docs/guide/knowledge.md b/docs/guide/knowledge.md index e1d85dc34..0547824b9 100644 --- a/docs/guide/knowledge.md +++ b/docs/guide/knowledge.md @@ -126,6 +126,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. +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. + `--incremental` re-extracts only the files changed since the last import; each codebase keeps that baseline in its own `evidence/code//`. A remote repo's slug is `____`, shared by `--from-repo`, `--from-mr` and `teamai ci extract-mr`; `--dir` uses the repo name. `--max-files` raises the cap on source files collected per repository (default 200). Files are collected key-file first; a source with more files is truncated, and the run warns with both counts instead of dropping the rest silently. diff --git a/docs/guide/zh-CN/knowledge.md b/docs/guide/zh-CN/knowledge.md index a069d2ebb..d14120e50 100644 --- a/docs/guide/zh-CN/knowledge.md +++ b/docs/guide/zh-CN/knowledge.md @@ -126,6 +126,8 @@ teamai import --from-repo https://github.com/org/repo --skip-enrich `--from-mr` 可以一次传多个 MR URL：每个草稿照旧单独确认（`--all` 则都不问），接受的 learning 在最后一次性发布，每个仓库的 teamwiki 只更新一次。其中一个 MR 失败不影响其余，运行结束时给出汇总，有失败则退出码非零。 +提炼时除了 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 用英文写，否则用中文。 + `--incremental` 只重新提取上次导入后改过的文件，基线随每个代码库存在它自己的 `evidence/code//` 里。远端仓库的 slug 是 `____`，`--from-repo`、`--from-mr` 和 `teamai ci extract-mr` 用的都是它；`--dir` 用仓库名。 `--max-files` 调整单个仓库采集源码文件的上限（默认 200）。采集按关键文件优先；文件数超出上限时会截断，并输出同时带两个计数的警告，不再静默丢弃。 diff --git a/skill-data/core/references/commands.md b/skill-data/core/references/commands.md index 4bc051658..f040d97f4 100644 --- a/skill-data/core/references/commands.md +++ b/skill-data/core/references/commands.md @@ -392,6 +392,7 @@ Generated: do not edit by hand. Regenerate with - `--dir ` — Extract code knowledge from a local directory (same as --from-repo but no clone) - `--from-claude` (hidden) — Scan Claude/Cursor rule directories (the Claude root's rules/ — ~/.claude or the recorded toolRoots.claude — and ~/.cursor/rules) - `--from-mr ` — Extract learning from one or more merged MRs/PRs and trigger incremental teamwiki update + - `--no-discussions` — Leave the MR's review threads out of the extraction (with --from-mr) - `--from-iwiki ` — Import documents from iWiki Space ID or page URL (requires TAI_PAT_TOKEN) - `--resume` (hidden) — Resume an interrupted import session - `--all` — Accept all suggestions without interactive confirmation @@ -454,3 +455,4 @@ Generated: do not edit by hand. Regenerate with - `--write-mode ` — Write strategy: direct | pending-review - `--output ` — Write artifacts to directory - `--individual-comments` — Post each suggestion as separate comment with reaction/resolve support + - `--no-discussions` — Leave the MR's review threads out of the extraction diff --git a/src/__tests__/ci-extract-mr.test.ts b/src/__tests__/ci-extract-mr.test.ts index 3911373c2..7709a71ba 100644 --- a/src/__tests__/ci-extract-mr.test.ts +++ b/src/__tests__/ci-extract-mr.test.ts @@ -89,6 +89,18 @@ describe('ciExtractMr', () => { ); }); + it('hands its --comment-marker to the extraction so its own comments stay out of the review threads (#1187)', async () => { + await ciExtractMr({ + url: 'https://github.com/org/repo/pull/1', + mode: 'comment', + commentMarker: '', + }); + + expect(mockImportFromMR).toHaveBeenCalledWith(expect.objectContaining({ + commentMarker: '', + })); + }); + it('write 模式需要 --team-repo 参数', async () => { await expect(ciExtractMr({ url: 'https://github.com/org/repo/pull/1', diff --git a/src/__tests__/github-mr-fetch.test.ts b/src/__tests__/github-mr-fetch.test.ts index de5cdb27f..bb5bdd012 100644 --- a/src/__tests__/github-mr-fetch.test.ts +++ b/src/__tests__/github-mr-fetch.test.ts @@ -9,10 +9,30 @@ vi.mock('node:child_process', async (importOriginal) => ({ ...await importOriginal(), execSync: vi.fn(() => { throw new Error('gh unavailable'); }), })); +vi.mock('../utils/logger.js', () => ({ + log: { info: vi.fn(), success: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn(), dim: vi.fn() }, +})); +import { log } from '../utils/logger.js'; const PR = '/repos/team/repo/pulls/42'; const METADATA = { title: 'Fix retries', body: null, user: { login: 'dev' }, merged_at: null }; +/** The review threads (#1187): diff comments, review summaries, conversation. */ +const REVIEW_COMMENTS = [ + { id: 1, body: 'Three retries hide a dead bucket.', path: 'src/up.ts', line: 42, user: { login: 'bob' } }, + { id: 2, body: 'Unrelated nit', path: 'src/x.ts', line: null, original_line: 7, user: { login: 'carol' } }, + { id: 3, body: 'Agreed, fail fast on 403.', path: 'src/up.ts', line: 42, in_reply_to_id: 1, user: { login: 'dev' } }, +]; +const REVIEWS = [ + { body: '', state: 'APPROVED', user: { login: 'bob' } }, + { body: 'Please split the retry policy out.', state: 'CHANGES_REQUESTED', user: { login: 'carol' } }, + { body: 'draft', state: 'PENDING', user: { login: 'erin' } }, +]; +const CONVERSATION = [ + { body: '\n## TeamAI 知识提炼', user: { login: 'ci-bot' } }, + { body: 'Merging after the split.', user: { login: 'dev' } }, +]; + function mockResponses(failedPath?: string, status = 200, body = '', responseError?: Error): string[] { const seen: string[] = []; const request = (options: https.RequestOptions, callback: (res: IncomingMessage) => void) => { @@ -23,6 +43,9 @@ function mockResponses(failedPath?: string, status = 200, body = '', responseErr const failed = key === failedPath; 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' } }]); const req = Object.assign(new EventEmitter(), { setTimeout: vi.fn(), @@ -95,3 +118,34 @@ describe('GitHub PR REST fallback', () => { }); }); }); + +describe('GitHub PR review threads (#1187)', () => { + it('groups diff comments into threads and adds review verdicts and conversation, without teamai comments', async () => { + mockResponses(); + const mr = await fetchGitHubPR('https://github.com/team/repo/pull/42'); + expect(mr.discussions).toEqual([ + { file: 'src/up.ts', line: 42, notes: [{ author: 'bob', body: 'Three retries hide a dead bucket.' }, { author: 'dev', body: 'Agreed, fail fast on 403.' }] }, + { file: 'src/x.ts', line: 7, notes: [{ author: 'carol', body: 'Unrelated nit' }] }, + { notes: [{ author: 'carol', body: 'changes requested: Please split the retry policy out.' }] }, + { notes: [{ author: 'dev', body: 'Merging after the split.' }] }, + ]); + }); + + it('requests none of the list endpoints with discussions: false', async () => { + const seen = mockResponses(); + const mr = await fetchGitHubPR('https://github.com/team/repo/pull/42', { discussions: false }); + expect(mr).not.toHaveProperty('discussions'); + expect(seen.filter((u) => /comments|reviews/.test(u))).toEqual([]); + }); + + it('keeps the extraction going, with a warning, when a list endpoint fails', async () => { + vi.mocked(log.warn).mockClear(); + mockResponses(`${PR}/reviews?per_page=100&page=1`, 403, '{"message":"Forbidden"}'); + const mr = await fetchGitHubPR('https://github.com/team/repo/pull/42'); + expect(mr.title).toBe('Fix retries'); + expect(mr).not.toHaveProperty('discussions'); + expect(vi.mocked(log.warn).mock.calls.map(([m]) => String(m))).toEqual([ + expect.stringMatching(/Review threads not read \(GitHub API error 403/), + ]); + }); +}); diff --git a/src/__tests__/gitlab-mr-discussions.test.ts b/src/__tests__/gitlab-mr-discussions.test.ts new file mode 100644 index 000000000..3b76bc877 --- /dev/null +++ b/src/__tests__/gitlab-mr-discussions.test.ts @@ -0,0 +1,111 @@ +/** + * `fetchGitLabMR` reads the MR's review threads from `/discussions` (#1187): + * every page, without system notes or teamai's own CI comments, with the file + * and line a diff thread is anchored to and whether it is resolved. An API + * error there keeps the extraction going without threads, and + * `discussions: false` does not call the endpoint at all. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../utils/logger.js', () => ({ + log: { info: vi.fn(), success: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn(), dim: vi.fn() }, +})); + +import { fetchGitLabMR } from '../providers/gitlab/mr-fetch.js'; +import { log } from '../utils/logger.js'; + +const URL = 'https://gitlab.com/group/repo/-/merge_requests/7'; +const API = 'https://gitlab.com/api/v4/projects/group%2Frepo/merge_requests/7'; + +const MR = { title: 'Retry uploads', description: 'd', author: { username: 'alice' }, merged_at: '2026-09-20T10:00:00Z' }; + +const note = (author: string, body: string, extra: Record = {}) => ({ body, author: { username: author }, ...extra }); + +const PAGE_1 = [ + { notes: [note('gitlab', 'changed the description', { system: true })] }, + { + notes: [ + note('bob', 'Fail fast on 403.', { resolvable: true, resolved: true, position: { new_path: 'src/up.ts', old_path: 'src/up.ts', new_line: 42, old_line: null } }), + note('alice', 'Done.', { resolvable: true, resolved: true }), + ], + }, + { notes: [note('ci', '\n## TeamAI 知识提炼')] }, +]; +const PAGE_2 = [ + { notes: [note('carol', 'Deleted line was load-bearing.', { resolvable: true, resolved: false, position: { new_path: null, old_path: 'src/old.ts', new_line: null, old_line: 3 } })] }, + { notes: [note('dave', 'LGTM')] }, +]; + +type Handler = (url: string) => { status?: number; body: unknown; nextPage?: string }; + +function stubFetch(handler: Handler): string[] { + const seen: string[] = []; + vi.stubGlobal('fetch', vi.fn(async (input: string) => { + seen.push(input); + const { status = 200, body, nextPage } = handler(input); + return { + ok: status >= 200 && status < 300, + status, + headers: new Headers(nextPage ? { 'x-next-page': nextPage } : {}), + json: async () => body, + text: async () => (typeof body === 'string' ? body : JSON.stringify(body)), + } as unknown as Response; + })); + return seen; +} + +const base: Handler = (url) => { + if (url === `${API}`) return { body: MR }; + if (url.startsWith(`${API}/commits`)) return { body: [{ id: 'abc', title: 'retry' }] }; + if (url.startsWith(`${API}/changes`)) return { body: { changes: [{ diff: '+retry' }] } }; + if (url.endsWith('/discussions?per_page=100&page=1')) return { body: PAGE_1, nextPage: '2' }; + if (url.endsWith('/discussions?per_page=100&page=2')) return { body: PAGE_2 }; + throw new Error(`unexpected ${url}`); +}; + +beforeEach(() => { + vi.stubEnv('GITLAB_TOKEN', 'secret'); + vi.mocked(log.warn).mockClear(); +}); +afterEach(() => { + vi.unstubAllGlobals(); + vi.unstubAllEnvs(); +}); + +describe('fetchGitLabMR review threads (#1187)', () => { + it('reads every page of discussions, mapping anchors and resolution, without system or teamai notes', async () => { + const seen = stubFetch(base); + const mr = await fetchGitLabMR(URL); + expect(seen.filter((u) => u.includes('/discussions'))).toHaveLength(2); + expect(mr.discussions).toEqual([ + { file: 'src/up.ts', line: 42, resolved: true, notes: [{ author: 'bob', body: 'Fail fast on 403.' }, { author: 'alice', body: 'Done.' }] }, + { file: 'src/old.ts', line: 3, resolved: false, notes: [{ author: 'carol', body: 'Deleted line was load-bearing.' }] }, + { file: undefined, line: undefined, resolved: undefined, notes: [{ author: 'dave', body: 'LGTM' }] }, + ]); + expect(mr.title).toBe('Retry uploads'); + expect(mr.diff).toBe('+retry'); + }); + + it('does not call the endpoint with discussions: false', async () => { + const seen = stubFetch(base); + const mr = await fetchGitLabMR(URL, { discussions: false }); + expect(seen.some((u) => u.includes('/discussions'))).toBe(false); + expect(mr).not.toHaveProperty('discussions'); + }); + + it('keeps the extraction going, with a warning, when the discussions call fails', async () => { + stubFetch((url) => (url.includes('/discussions') ? { status: 403, body: { message: 'Forbidden' } } : base(url))); + const mr = await fetchGitLabMR(URL); + expect(mr).not.toHaveProperty('discussions'); + expect(mr.title).toBe('Retry uploads'); + expect(vi.mocked(log.warn).mock.calls.map(([m]) => String(m))).toEqual([ + expect.stringMatching(/Review threads not read \(GitLab API error 403\)/), + ]); + }); + + it('returns an empty list for an MR without review', async () => { + stubFetch((url) => (url.includes('/discussions') ? { body: [] } : base(url))); + const mr = await fetchGitLabMR(URL); + expect(mr.discussions).toEqual([]); + }); +}); diff --git a/src/__tests__/gitlab-provider.test.ts b/src/__tests__/gitlab-provider.test.ts index 6a4d06b6c..588357eb6 100644 --- a/src/__tests__/gitlab-provider.test.ts +++ b/src/__tests__/gitlab-provider.test.ts @@ -652,12 +652,16 @@ describe('fetchGitLabMR', () => { if (String(url) === `${base}/changes`) { return new Response(JSON.stringify({ changes: [{ diff: '+fixed' }] }), { status: 200 }); } + if (String(url).startsWith(`${base}/discussions`)) { + return new Response(JSON.stringify([]), { status: 200 }); + } return new Response(JSON.stringify({ title: 'T', description: null, author: { username: 'a' } }), { status: 200 }); }) as never; const mr = await fetchGitLabMR('https://gitlab.com/gitlab/g/sub/repo/-/merge_requests/7'); - expect(seen).toEqual([base, `${base}/commits?per_page=50`, `${base}/changes`]); + // The review threads (#1187) come from the same instance root. + expect(seen).toEqual([base, `${base}/commits?per_page=50`, `${base}/changes`, `${base}/discussions?per_page=100&page=1`]); expect(mr.commits).toEqual([{ hash: 'abc123', message: 'Fix' }]); expect(mr.diff).toBe('+fixed'); }); diff --git a/src/__tests__/import-mr-threads.test.ts b/src/__tests__/import-mr-threads.test.ts new file mode 100644 index 000000000..a300187b1 --- /dev/null +++ b/src/__tests__/import-mr-threads.test.ts @@ -0,0 +1,127 @@ +/** + * `teamai import --from-mr` reads the MR's review threads (#1187): the prompt + * gets them in a section of their own, with the MR author and reviewers + * marked, replied threads first, a long thread shortened rather than the + * section cut off, and the model told that a resolved thread's decision + * outweighs the diff. With `language: en` the learning is written in English. + */ +import { describe, expect, it } from 'vitest'; + +import { extractMRLearningPrompt, formatMrThreads } from '../import-mr.js'; +import { pruneThreads } from '../providers/mr-threads.js'; +import type { MRData, MRThread } from '../types.js'; + +const mr: MRData = { + title: 'Retry uploads', + description: 'Retries S3 uploads.', + author: 'alice', + commits: [{ hash: 'abc12345', message: 'retry upload' }], + diff: 'diff --git a/up.ts b/up.ts\n+retry(3)\n', + url: 'https://gitlab.example.com/g/r/-/merge_requests/7', +}; + +const replied: MRThread = { + file: 'src/up.ts', + line: 42, + resolved: true, + notes: [ + { author: 'bob', body: 'Three retries hide a dead bucket. Fail fast on 403.' }, + { author: 'alice', body: 'Agreed, 403 now raises at once; retries only on 5xx.' }, + ], +}; +const remark: MRThread = { notes: [{ author: 'carol', body: 'LGTM' }] }; + +describe('pruneThreads', () => { + it('drops teamai CI comments and blank notes, and a thread left empty', () => { + const threads = pruneThreads([ + { notes: [{ author: 'ci', body: '\n## TeamAI 知识提炼' }] }, + { notes: [{ author: 'ci', body: '' }, { author: 'bob', body: ' ' }] }, + remark, + ]); + expect(threads).toEqual([remark]); + }); + + it('drops comments posted with a custom --comment-marker as well', () => { + const threads = pruneThreads([ + { notes: [{ author: 'ci', body: '\n## TeamAI 知识提炼' }] }, + { notes: [{ author: 'ci', body: '' }] }, + remark, + ], ''); + expect(threads).toEqual([remark]); + // Without the marker the custom comment is an ordinary note. + expect(pruneThreads([{ notes: [{ author: 'ci', body: '' }] }])).toHaveLength(1); + }); +}); + +describe('formatMrThreads', () => { + it('puts replied threads before single remarks and marks the MR author', () => { + const text = formatMrThreads([remark, replied], 'alice'); + expect(text.indexOf('src/up.ts:42')).toBeLessThan(text.indexOf('LGTM')); + expect(text).toContain('src/up.ts:42 (resolved)'); + expect(text).toContain('[reviewer bob] Three retries hide a dead bucket.'); + expect(text).toContain('[author alice] Agreed, 403 now raises at once;'); + expect(text).toContain('[reviewer carol] LGTM'); + }); + + it('labels an unresolved thread and one without a resolution state', () => { + const text = formatMrThreads([{ ...replied, resolved: false }, { ...replied, resolved: undefined, file: undefined }], 'alice'); + expect(text).toContain('src/up.ts:42 (unresolved)'); + expect(text).toMatch(/^### Thread 2\n/m); + }); + + it('shortens a long thread to its first and last notes instead of cutting the section', () => { + const long: MRThread = { + notes: Array.from({ length: 12 }, (_, i) => ({ author: i % 2 ? 'alice' : 'bob', body: `note ${i} ${'x'.repeat(300)}` })), + }; + const text = formatMrThreads([long], 'alice', 6000); + expect(text).toContain('note 0'); + expect(text).toContain('note 11'); + expect(text).not.toContain('note 5'); + expect(text).toContain('(10 notes omitted)'); + expect(text.length).toBeLessThan(2000); + }); + + it('stops at the budget and says how many threads it left out', () => { + const many = Array.from({ length: 8 }, (_, i) => ({ ...replied, line: i })); + const text = formatMrThreads(many, 'alice', 500); + expect(text).toContain('### Thread 1'); + expect(text).not.toContain('### Thread 8'); + expect(text).toMatch(/\d+ more threads? omitted/); + expect(text.length).toBeLessThanOrEqual(600); + }); +}); + +describe('extractMRLearningPrompt', () => { + it('has no thread section when the MR has no threads', () => { + const prompt = extractMRLearningPrompt({ ...mr, discussions: [] }); + expect(prompt).not.toContain('Review threads'); + expect(prompt).toContain('用中文撰写'); + }); + + it('adds the threads with the instruction that a resolved decision outweighs the diff', () => { + const prompt = extractMRLearningPrompt({ ...mr, discussions: [remark, replied] }); + expect(prompt).toContain('Review threads'); + expect(prompt).toContain('[author alice]'); + expect(prompt).toMatch(/resolved 的 thread 里的决定比复述 diff 更重要/); + // The threads do not push the diff out. + expect(prompt).toContain('+retry(3)'); + expect(extractMRLearningPrompt({ ...mr, discussions: [replied] }, 'en')) + .toMatch(/decision reached in a resolved thread outweighs a restatement of the diff/); + }); + + it('writes the learning in English when the team language is en, with English headings', () => { + const prompt = extractMRLearningPrompt({ ...mr, discussions: [replied] }, 'en'); + expect(prompt).toContain('Write in English'); + expect(prompt).toContain('## Background'); + expect(prompt).not.toContain('用中文撰写'); + expect(prompt).toContain(`source_mr: "${mr.url}"`); + }); + + it('keeps the Chinese prompt for zh and when the language is not set', () => { + for (const language of ['zh', undefined] as const) { + const prompt = extractMRLearningPrompt(mr, language); + expect(prompt).toContain('用中文撰写'); + expect(prompt).toContain('## 背景'); + } + }); +}); diff --git a/src/__tests__/import-mr.test.ts b/src/__tests__/import-mr.test.ts index ee32da35a..ffed3cae6 100644 --- a/src/__tests__/import-mr.test.ts +++ b/src/__tests__/import-mr.test.ts @@ -18,6 +18,8 @@ const DRAFT_BODY = 'gateway retry budget exhausted under burst traffic backoff j /** The team repo `teamai import` runs against, set per test. */ let teamRepo = ''; +/** The team's teamai.yaml `language`, set per test (#1187). */ +let teamLanguage: 'en' | 'zh' | undefined; vi.mock('../config.js', async (importOriginal) => ({ ...(await importOriginal()), autoDetectInit: vi.fn(async () => ({ @@ -28,7 +30,7 @@ vi.mock('../config.js', async (importOriginal) => ({ additionalRoles: [], projects: ['alpha'], }, - teamConfig: { team: 't', repo: 'r' }, + teamConfig: { team: 't', repo: 'r', ...(teamLanguage ? { language: teamLanguage } : {}) }, })), })); @@ -249,6 +251,32 @@ describe('teamai import --from-mr (#1104)', () => { expect(infos()).toContainEqual(`[2/2] ${urls[1]}`); }); + it('reads the review threads by default and leaves them out with --no-discussions (#1187)', async () => { + const write = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + try { + await importCmd({ fromMr: [urls[0]], dryRun: true, all: true }); + await importCmd({ fromMr: [urls[0]], dryRun: true, all: true, discussions: false }); + } finally { + write.mockRestore(); + } + + expect(vi.mocked(fetchGitHubPR).mock.calls.map(([, opts]) => opts)) + .toEqual([{ discussions: true }, { discussions: false }]); + }); + + it('writes the learning in English when teamai.yaml sets language: en (#1187)', async () => { + const write = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + teamLanguage = 'en'; + try { + await importCmd({ fromMr: [urls[0]], dryRun: true, all: true }); + } finally { + teamLanguage = undefined; + write.mockRestore(); + } + + expect(vi.mocked(callClaude).mock.calls[0][0]).toContain('Write in English'); + }); + it('keeps going when one MR fails, reports it and exits non-zero', async () => { vi.mocked(fetchGitHubPR).mockRejectedValueOnce(new Error('MR not found')); const write = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); diff --git a/src/__tests__/tgit-mr-discussions.test.ts b/src/__tests__/tgit-mr-discussions.test.ts new file mode 100644 index 000000000..a3f18604c --- /dev/null +++ b/src/__tests__/tgit-mr-discussions.test.ts @@ -0,0 +1,86 @@ +/** + * `fetchTGitMR` reads the MR's notes through the endpoint `teamai ci` uses and + * groups them into threads by `discussion_id` (#1187), without system notes + * or teamai's own CI comments. A failure there keeps the extraction going. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../providers/tgit/gf-cli.js', () => ({ gfGetOAuthToken: vi.fn(() => null) })); +vi.mock('../utils/logger.js', () => ({ + log: { info: vi.fn(), success: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn(), dim: vi.fn() }, +})); + +import { fetchTGitMR } from '../providers/tgit/mr-fetch.js'; +import { log } from '../utils/logger.js'; + +const URL = 'https://git.woa.com/group/repo/merge_requests/5'; +const API = 'https://git.woa.com/api/v3/projects/group%2Frepo'; + +const NOTES = [ + { body: 'approved this merge request', author: { username: 'bot' }, system: true }, + { body: 'Fail fast on 403.', author: { username: 'bob' }, discussion_id: 'd1' }, + { body: 'LGTM', author: { username: 'dave' } }, + { body: 'Done.', author: { username: 'alice' }, discussion_id: 'd1' }, + { body: '\n## TeamAI 知识提炼', author: { username: 'ci' } }, +]; + +type Handler = (url: string) => { status?: number; body: unknown }; + +function stubFetch(handler: Handler): string[] { + const seen: string[] = []; + vi.stubGlobal('fetch', vi.fn(async (input: string) => { + seen.push(input); + const { status = 200, body } = handler(input); + return { + ok: status >= 200 && status < 300, + status, + json: async () => body, + text: async () => JSON.stringify(body), + } as unknown as Response; + })); + return seen; +} + +const base: Handler = (url) => { + if (url === `${API}/merge_requests?iid=5`) return { body: [{ id: 99, iid: 5, title: 'Retry uploads', description: 'd', author: { username: 'alice' } }] }; + if (url === `${API}/merge_request/99/changes`) return { body: { files: [{ diff: '+retry' }] } }; + if (url === `${API}/merge_requests/99/notes?per_page=100&page=1`) return { body: NOTES }; + throw new Error(`unexpected ${url}`); +}; + +beforeEach(() => { + vi.stubEnv('TGIT_TOKEN', 'secret'); + vi.mocked(log.warn).mockClear(); +}); +afterEach(() => { + vi.unstubAllGlobals(); + vi.unstubAllEnvs(); +}); + +describe('fetchTGitMR review threads (#1187)', () => { + it('groups the notes by discussion, without system or teamai notes', async () => { + stubFetch(base); + const mr = await fetchTGitMR(URL); + expect(mr.discussions).toEqual([ + { notes: [{ author: 'bob', body: 'Fail fast on 403.' }, { author: 'alice', body: 'Done.' }] }, + { notes: [{ author: 'dave', body: 'LGTM' }] }, + ]); + expect(mr.diff).toBe('+retry'); + }); + + it('does not read the notes with discussions: false', async () => { + const seen = stubFetch(base); + const mr = await fetchTGitMR(URL, { discussions: false }); + expect(seen.some((u) => u.includes('/notes'))).toBe(false); + expect(mr).not.toHaveProperty('discussions'); + }); + + it('keeps the extraction going, with a warning, when the notes call fails', async () => { + stubFetch((url) => (url.includes('/notes') ? { status: 500, body: {} } : base(url))); + const mr = await fetchTGitMR(URL); + expect(mr).not.toHaveProperty('discussions'); + expect(vi.mocked(log.warn).mock.calls.map(([m]) => String(m))).toEqual([ + expect.stringMatching(/Review threads not read \(TGit API error 500\)/), + ]); + }); +}); diff --git a/src/ci/extract-mr.ts b/src/ci/extract-mr.ts index 260690d4d..c4906257c 100644 --- a/src/ci/extract-mr.ts +++ b/src/ci/extract-mr.ts @@ -32,6 +32,8 @@ export interface CiExtractMrOptions { output?: string; dryRun?: boolean; individualComments?: boolean; + /** Read the MR's review threads into the extraction; `--no-discussions` sets false (#1187) */ + discussions?: boolean; } // ─── Git User 配置 ─────────────────────────────────────── @@ -239,12 +241,20 @@ export async function ciExtractMr(opts: CiExtractMrOptions): Promise { // AI 提炼（复用 importFromMR） let learning: LearningDraft | undefined; + // The team repo's `language: en` makes the learning English (#1187). + const language = opts.teamRepo + ? (await import('../config.js').then(({ loadTeamConfig }) => loadTeamConfig(opts.teamRepo!)))?.language + : undefined; const result = await importFromMR({ url: opts.url, all: true, // learnings-root ok: CI runs against a bare checkout, with no LocalConfig learningsDirs: opts.teamRepo ? [path.join(opts.teamRepo, 'learnings')] : undefined, dryRun: true, // 不让 importFromMR 自己写文件，我们自己控制写入 + discussions: opts.discussions, + // A custom marker's comments are teamai's own too: keep them out of the threads. + commentMarker: opts.commentMarker, + language, }); learning = result.learning; diff --git a/src/import-mr.ts b/src/import-mr.ts index 3d726df5b..ba9b75c4d 100644 --- a/src/import-mr.ts +++ b/src/import-mr.ts @@ -7,7 +7,7 @@ import matter from 'gray-matter'; import { fetchGitHubPR } from './providers/github/mr-fetch.js'; import { fetchGitLabMR } from './providers/gitlab/mr-fetch.js'; import { fetchTGitMR } from './providers/tgit/mr-fetch.js'; -import type { MRData, LearningDraft } from './types.js'; +import type { MRData, MRFetchOptions, MRThread, LearningDraft } from './types.js'; import { callClaude } from './utils/ai-client.js'; import { extractKeywords, findOverlappingLearnings } from './utils/dedup.js'; import { log, spinner } from './utils/logger.js'; @@ -19,51 +19,155 @@ const DEFAULT_LEARNINGS_DIR = path.join(getUserHome(), '.teamai', 'learnings'); /** * Auto-detects the provider from the URL and fetches MR data. * - * @param url Full URL of the MR / PR - * @returns Normalized MRData object - * @throws Error when the URL does not belong to a known provider + * @param url Full URL of the MR / PR + * @param opts What to read besides the MR, its commits and its diff + * @returns Normalized MRData object + * @throws Error when the URL does not belong to a known provider */ -async function fetchMR(url: string): Promise { +async function fetchMR(url: string, opts: MRFetchOptions): Promise { if (url.includes('github.com')) { - return fetchGitHubPR(url); + return fetchGitHubPR(url, opts); } if (url.includes('git.woa.com')) { - return fetchTGitMR(url); + return fetchTGitMR(url, opts); } // GitLab (incl. self-hosted): the `/-/merge_requests/` route is unique to // GitLab, so it identifies the platform on any host. if (/\/-\/merge_requests\/\d+/.test(url)) { - return fetchGitLabMR(url); + return fetchGitLabMR(url, opts); } throw new Error(`Unsupported MR URL: ${url}. Only GitHub, TGit and GitLab are supported`); } +/** Characters of the prompt the review threads may take (#1187). */ +const THREADS_BUDGET = 6000; +/** Characters one thread may take before it is shortened to its first and last notes. */ +const THREAD_MAX = 1500; +/** Characters one note may take. */ +const NOTE_MAX = 500; + +function clip(text: string, max: number): string { + const trimmed = text.trim(); + return trimmed.length <= max ? trimmed : `${trimmed.slice(0, max - 1)}…`; +} + +/** + * Renders the review threads for the extraction prompt (#1187). + * + * Threads with a reply come first and single unanswered remarks last, so a + * budget cut drops the remarks. Each thread names the file and line it is + * anchored to and whether it is resolved, then its notes in order, each marked + * as the MR author's or a reviewer's. A thread over {@link THREAD_MAX} keeps + * its first and last notes; once the budget is reached the rest of the + * threads are counted, not cut off mid-thread. + * + * @param threads The MR's review threads + * @param mrAuthor The MR author's username, to mark their notes + * @param budget Characters the section may take + */ +export function formatMrThreads(threads: MRThread[], mrAuthor: string | undefined, budget = THREADS_BUDGET): string { + const ordered = [...threads.filter((t) => t.notes.length > 1), ...threads.filter((t) => t.notes.length <= 1)]; + const sections: string[] = []; + let used = 0; + for (const [index, thread] of ordered.entries()) { + const anchor = thread.file ? ` — ${thread.file}${thread.line !== undefined ? `:${thread.line}` : ''}` : ''; + const state = thread.resolved === undefined ? '' : thread.resolved ? ' (resolved)' : ' (unresolved)'; + const header = `### Thread ${index + 1}${anchor}${state}`; + const line = (note: MRThread['notes'][number]): string => { + const role = note.author !== undefined && note.author === mrAuthor ? 'author' : 'reviewer'; + const who = note.author ? `${role} ${note.author}` : role; + return `- [${who}] ${clip(note.body, NOTE_MAX).replace(/\n+/g, '\n ')}`; + }; + let lines = thread.notes.map(line); + if (lines.join('\n').length > THREAD_MAX && thread.notes.length > 2) { + lines = [lines[0], `- (${thread.notes.length - 2} notes omitted)`, lines[lines.length - 1]]; + } + const section = `${header}\n${lines.join('\n')}`; + if (used + section.length + 2 > budget) { + const left = ordered.length - index; + sections.push(`(${left} more thread${left === 1 ? '' : 's'} omitted)`); + break; + } + sections.push(section); + used += section.length + 2; + } + return sections.join('\n\n'); +} + /** * Builds the learning extraction prompt. * - * @param mr MR data object - * @returns Full prompt string for callClaude + * The MR's review threads, when the fetch read any, get a section of their + * own after the diff, and the model is told that a decision reached in a + * resolved thread outweighs a restatement of the diff (#1187). `language` + * `en` asks for an English learning with English headings; `zh` or unset + * keeps the Chinese one. + * + * @param mr MR data object + * @param language The team's `language` from teamai.yaml + * @returns Full prompt string for callClaude */ -function extractMRLearningPrompt(mr: MRData): string { +export function extractMRLearningPrompt(mr: MRData, language?: 'en' | 'zh'): string { const commitsFormatted = mr.commits .map((c) => `- ${c.hash.slice(0, 8)}: ${c.message}`) .join('\n'); const diff3000 = mr.diff.slice(0, 3000); const author = mr.author ?? 'unknown'; const date = mr.mergedAt ? mr.mergedAt.slice(0, 10) : new Date().toISOString().slice(0, 10); - - return `你是团队知识库管理员。从以下 MR 信息提炼一条有价值的团队 learning。 -【必须】用中文撰写，输出完整 Markdown 文档（含 YAML frontmatter）。 - -frontmatter 字段（严格按此格式，不要加其他字段）： ---- -title: "<简短标题，描述核心问题或发现，<60字符>" + const threads = mr.discussions && mr.discussions.length > 0 ? formatMrThreads(mr.discussions, mr.author) : ''; + const frontmatter = (titleHint: string): string => `--- +title: "${titleHint}" author: ${author} date: ${date} tags: [tag1, tag2, tag3] confidence: 0.85 source_mr: "${mr.url}" +---`; + + if (language === 'en') { + return `You are the team's knowledge base curator. Distill one valuable team learning from the MR below. +Write in English. Output a complete Markdown document with YAML frontmatter. +${threads ? '\nThe review threads carry the decisions: a decision reached in a resolved thread outweighs a restatement of the diff. Lead with what the review decided and why.\n' : ''} +Frontmatter fields (exactly this format, no other fields): +${frontmatter('')} + +Body structure (every section is required): +## Background +What was being done? What problem came up? + +## Solution +How was it solved? What were the key steps? + +## Lessons +- lesson 1 +- lesson 2 + +## Related Skills +- skill-name (or "none") + +Pick 2-5 tags from these groups: +stack: python, typescript, go, k8s, docker, sglang, cuda +problem: troubleshooting, performance, deployment, config, api +pattern: workflow, pattern, tool-usage, best-practice +scene: debugging, testing, monitoring, security + --- +MR title: ${mr.title} +MR description: +${mr.description} + +Commits: +${commitsFormatted} + +Key diff (first 3000 characters): +${diff3000}${threads ? `\n\nReview threads (${mr.discussions!.length}):\n${threads}` : ''}`; + } + + return `你是团队知识库管理员。从以下 MR 信息提炼一条有价值的团队 learning。 +【必须】用中文撰写，输出完整 Markdown 文档（含 YAML frontmatter）。 +${threads ? '\nReview threads 一节记录了评审达成的决定：已 resolved 的 thread 里的决定比复述 diff 更重要，优先写评审决定了什么、为什么。\n' : ''} +frontmatter 字段（严格按此格式，不要加其他字段）： +${frontmatter('<简短标题，描述核心问题或发现，<60字符>')} body 结构（以下各节必须包含）： ## 背景 @@ -94,7 +198,7 @@ ${mr.description} ${commitsFormatted} 关键 diff（前 3000 字）： -${diff3000}`; +${diff3000}${threads ? `\n\nReview threads（${mr.discussions!.length}）：\n${threads}` : ''}`; } /** @@ -190,6 +294,12 @@ export async function importFromMR(opts: { outputFileName?: string; queueLearning?: (filename: string, content: string) => Promise; dryRun?: boolean; + /** Read the MR's review threads into the extraction (default true; `--no-discussions` turns it off). */ + discussions?: boolean; + /** The marker `ci extract-mr --comment-marker` posts with, so its own comments stay out of the threads. */ + commentMarker?: string; + /** The team's `language` from teamai.yaml: `en` writes the learning in English. */ + language?: 'en' | 'zh'; }): Promise<{ learning?: LearningDraft; repoUrl: string; learningFile?: string }> { const learningsDirs = opts.learningsDirs ?? [DEFAULT_LEARNINGS_DIR]; @@ -199,8 +309,10 @@ export async function importFromMR(opts: { let mr: MRData; try { - mr = await fetchMR(opts.url); - fetchSpinner.succeed('MR data fetched'); + mr = await fetchMR(opts.url, { discussions: opts.discussions ?? true, commentMarker: opts.commentMarker }); + fetchSpinner.succeed(mr.discussions?.length + ? `MR data fetched (${mr.discussions.length} review thread${mr.discussions.length === 1 ? '' : 's'})` + : 'MR data fetched'); } catch (err: unknown) { fetchSpinner.fail('MR data fetch failed'); throw err; @@ -212,7 +324,7 @@ export async function importFromMR(opts: { let learningContent: string; try { - learningContent = await callClaude(extractMRLearningPrompt(mr)); + learningContent = await callClaude(extractMRLearningPrompt(mr, opts.language)); aiSpinner.succeed('AI analysis complete'); } catch (err: unknown) { aiSpinner.fail('AI analysis failed'); diff --git a/src/import.ts b/src/import.ts index b07ed4c62..8dc217931 100644 --- a/src/import.ts +++ b/src/import.ts @@ -26,6 +26,8 @@ interface ImportOptions extends GlobalOptions { fromClaude?: boolean; /** Extract knowledge from one or more merged MR/PR URLs */ fromMr?: string | string[]; + /** Read the MR's review threads into the extraction; `--no-discussions` sets false (#1187) */ + discussions?: boolean; /** iWiki Space ID or page URL for bulk importing iWiki documents */ fromIwiki?: string; /** Whether to resume an interrupted import session */ @@ -265,6 +267,9 @@ export async function importCmd(opts: ImportOptions): Promise { return queued.path; }, dryRun: opts.dryRun, + discussions: opts.discussions, + // `language: en` in teamai.yaml makes the learning English (#1187). + language: teamConfig?.language, }); extractedAll.push({ url, ...extracted }); } catch (e: unknown) { diff --git a/src/index.ts b/src/index.ts index 679802134..932045d1e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1235,6 +1235,7 @@ program .option('--dir ', 'Extract code knowledge from a local directory (same as --from-repo but no clone)') .addOption(new Option('--from-claude', 'Scan Claude/Cursor rule directories (the Claude root\'s rules/ — ~/.claude or the recorded toolRoots.claude — and ~/.cursor/rules)').hideHelp()) .option('--from-mr ', 'Extract learning from one or more merged MRs/PRs and trigger incremental teamwiki update') + .option('--no-discussions', 'Leave the MR\'s review threads out of the extraction (with --from-mr)') .option('--from-iwiki ', 'Import documents from iWiki Space ID or page URL (requires TAI_PAT_TOKEN)') .addOption(new Option('--resume', 'Resume an interrupted import session').hideHelp()) .option('--all', 'Accept all suggestions without interactive confirmation') @@ -1350,6 +1351,7 @@ ciCmd .option('--write-mode ', 'Write strategy: direct | pending-review', 'direct') .option('--output ', 'Write artifacts to directory') .option('--individual-comments', 'Post each suggestion as separate comment with reaction/resolve support') + .option('--no-discussions', 'Leave the MR\'s review threads out of the extraction') .action(async (cmdOpts) => { const globalOpts = program.opts() as GlobalOptions; const { ciExtractMr } = await import('./ci/extract-mr.js'); diff --git a/src/providers/github/mr-fetch.ts b/src/providers/github/mr-fetch.ts index bfb7363aa..903f24bd6 100644 --- a/src/providers/github/mr-fetch.ts +++ b/src/providers/github/mr-fetch.ts @@ -1,8 +1,9 @@ -import { execSync } from 'node:child_process'; +import { execFileSync, execSync } from 'node:child_process'; import https from 'node:https'; -import { type MRData } from '../../types.js'; +import { type MRData, type MRFetchOptions, type MRThread } from '../../types.js'; import { log } from '../../utils/logger.js'; +import { pruneThreads } from '../mr-threads.js'; /** GitHub PR URL 解析结果 */ interface ParsedGitHubPR { @@ -94,6 +95,99 @@ async function githubApiGet(path: string): Promise { }); } +/** Reads one API path as JSON: through `gh api` or the REST client. */ +type ApiGet = (path: string) => Promise; + +/** `gh api `: the same paths as the REST client, with gh's credentials. */ +async function ghApiGet(path: string): Promise { + const out = execFileSync('gh', ['api', path.replace(/^\//, '')], { maxBuffer: 10 * 1024 * 1024, encoding: 'utf8' }); + return JSON.parse(out); +} + +/** A `/pulls/:n/comments` entry (the fields read). */ +interface GitHubReviewComment { + id: number; + body?: string | null; + path?: string; + line?: number | null; + original_line?: number | null; + /** The thread's first comment, on a reply. */ + in_reply_to_id?: number; + user?: { login: string }; +} + +/** A `/pulls/:n/reviews` entry (the fields read). */ +interface GitHubReview { + body?: string | null; + state?: string; + user?: { login: string }; +} + +/** A `/issues/:n/comments` entry (the fields read). */ +interface GitHubIssueComment { + body?: string | null; + user?: { login: string }; +} + +/** Pages of 100 read at most per list endpoint. */ +const MAX_LIST_PAGES = 10; + +/** Reads every page of a list endpoint. */ +async function readPages(get: ApiGet, path: string): Promise { + const all: T[] = []; + for (let page = 1; page <= MAX_LIST_PAGES; page++) { + const items = await get(`${path}?per_page=100&page=${page}`); + if (!Array.isArray(items)) { + throw new Error(`GitHub API returned no list for ${path}`); + } + all.push(...(items as T[])); + if (items.length < 100) break; + } + return all; +} + +/** + * Reads the PR's review threads (#1187): the diff comments grouped into their + * threads, each review's summary, and the conversation comments, without + * teamai's own CI comments. The REST API does not say whether a thread is + * resolved, so `resolved` stays unset. + * + * @throws Error on an API error; the caller keeps the extraction going without threads + */ +async function fetchGitHubThreads( + get: ApiGet, + owner: string, + repo: string, + number: string, + commentMarker?: string, +): Promise { + const base = `/repos/${owner}/${repo}`; + const comments = await readPages(get, `${base}/pulls/${number}/comments`); + const reviews = await readPages(get, `${base}/pulls/${number}/reviews`); + const conversation = await readPages(get, `${base}/issues/${number}/comments`); + + const byRoot = new Map(); + for (const c of comments) { + const note = { author: c.user?.login, body: c.body ?? '' }; + const thread = byRoot.get(c.in_reply_to_id ?? c.id); + if (thread) { + thread.notes.push(note); + } else { + byRoot.set(c.id, { file: c.path, line: c.line ?? c.original_line ?? undefined, notes: [note] }); + } + } + const threads = [...byRoot.values()]; + for (const r of reviews) { + if (!r.body?.trim() || r.state === 'PENDING') continue; + const verdict = r.state ? `${r.state.toLowerCase().replace(/_/g, ' ')}: ` : ''; + threads.push({ notes: [{ author: r.user?.login, body: `${verdict}${r.body}` }] }); + } + for (const c of conversation) { + threads.push({ notes: [{ author: c.user?.login, body: c.body ?? '' }] }); + } + return pruneThreads(threads, commentMarker); +} + /** * 通过 GitHub REST API 获取 PR 数据（gh CLI 不可用时的回退路径）。 */ @@ -159,16 +253,22 @@ async function fetchGitHubPRViaApi(owner: string, repo: string, number: string): * * gh CLI 不可用时自动回退到 GitHub REST API（支持公开仓库，无需 token）。 * + * 评审 threads（diff 评论、review 结论、会话评论）随后通过同一条路径读取， + * 失败非致命（#1187）。 + * * @param url - GitHub PR 完整 web URL，例如 https://github.com/owner/repo/pull/123 + * @param opts - 是否读取评审 threads（默认读取） * @returns 包含标题、描述、提交列表、diff 的 MRData 对象 * @throws Error 当 URL 格式不合法或两种方式均失败时 */ -export async function fetchGitHubPR(url: string): Promise { +export async function fetchGitHubPR(url: string, opts: MRFetchOptions = {}): Promise { const { owner, repo, number } = parseGitHubPRUrl(url); const repoArg = `${owner}/${repo}`; log.debug(`fetchGitHubPR: ${repoArg}#${number}`); + let mr: MRData; + let get: ApiGet; // ── 优先尝试 gh CLI ────────────────────────────────────── try { const viewOutput = execSync( @@ -182,7 +282,7 @@ export async function fetchGitHubPR(url: string): Promise { { maxBuffer: 50 * 1024 * 1024, encoding: 'utf8' }, ); - return { + mr = { title: prView.title, description: prView.body ?? '', author: prView.author?.login, @@ -194,13 +294,24 @@ export async function fetchGitHubPR(url: string): Promise { diff: rawDiff.slice(0, 50000), url, }; + get = ghApiGet; } catch { // gh CLI 不可用或失败，回退到 REST API log.debug('gh CLI unavailable, falling back to GitHub REST API'); + // ── 回退：GitHub REST API ──────────────────────────────── + mr = await fetchGitHubPRViaApi(owner, repo, number); + get = githubApiGet; } - // ── 回退：GitHub REST API ──────────────────────────────── - return fetchGitHubPRViaApi(owner, repo, number); + // ── 评审 threads（失败非致命） ─────────────────────────── + if (opts.discussions !== false) { + try { + mr.discussions = await fetchGitHubThreads(get, owner, repo, number, opts.commentMarker); + } catch (err) { + log.warn(`Review threads not read (${(err as Error).message}); the learning is extracted without them`); + } + } + return mr; } diff --git a/src/providers/gitlab/mr-fetch.ts b/src/providers/gitlab/mr-fetch.ts index 37ff19af9..4b84c30d1 100644 --- a/src/providers/gitlab/mr-fetch.ts +++ b/src/providers/gitlab/mr-fetch.ts @@ -1,5 +1,6 @@ -import { type MRData } from '../../types.js'; +import { type MRData, type MRFetchOptions, type MRThread } from '../../types.js'; import { log } from '../../utils/logger.js'; +import { pruneThreads } from '../mr-threads.js'; import { getGitLabToken, gitlabBaseUrl } from './gitlab-api.js'; import { GITLAB_HOST } from './repo-url.js'; @@ -97,6 +98,73 @@ function authHeaders(token: string): Record { return { 'PRIVATE-TOKEN': token, 'Accept': 'application/json' }; } +/** A note of a `/discussions` entry (the fields read). */ +interface GitLabDiscussionNote { + body: string; + author?: { username: string }; + /** A system note ("changed the description", "approved"), not a review remark. */ + system?: boolean; + resolvable?: boolean; + resolved?: boolean; + /** Set for a diff comment: the file and line the thread is anchored to. */ + position?: { + new_path?: string | null; + old_path?: string | null; + new_line?: number | null; + old_line?: number | null; + }; +} + +/** One entry of `GET /projects/:id/merge_requests/:iid/discussions`. */ +interface GitLabDiscussion { + notes?: GitLabDiscussionNote[]; +} + +/** Pages of 100 discussions read at most; an MR with more review is unusual. */ +const MAX_DISCUSSION_PAGES = 20; + +/** Maps a GitLab discussion to a thread; system notes are left out. */ +function toThread(discussion: GitLabDiscussion): MRThread { + const notes = (discussion.notes ?? []).filter((n) => !n.system); + const position = notes[0]?.position; + const resolvable = notes.find((n) => n.resolvable); + return { + file: position?.new_path ?? position?.old_path ?? undefined, + line: position?.new_line ?? position?.old_line ?? undefined, + resolved: resolvable ? resolvable.resolved === true : undefined, + notes: notes.map((n) => ({ author: n.author?.username, body: n.body })), + }; +} + +/** + * Reads the MR's review threads, page by page (`x-next-page`), without + * system notes or teamai's own CI comments (#1187). + * + * @throws Error on an API error; the caller keeps the extraction going without threads + */ +async function fetchGitLabDiscussions( + apiBase: string, + enc: string, + mrIid: string, + headers: Record, + commentMarker?: string, +): Promise { + const threads: MRThread[] = []; + for (let page = 1; page <= MAX_DISCUSSION_PAGES; page++) { + const resp = await fetch( + `${apiBase}/projects/${enc}/merge_requests/${mrIid}/discussions?per_page=100&page=${page}`, + { headers, redirect: 'manual' }, + ); + if (!resp.ok) { + throw new Error(`GitLab API error ${resp.status}`); + } + const raw = (await resp.json()) as GitLabDiscussion[]; + threads.push(...raw.map(toThread)); + if (!resp.headers.get('x-next-page')) break; + } + return pruneThreads(threads, commentMarker); +} + /** * 通过 GitLab REST API（/api/v4）获取 MR 的完整数据。 * @@ -104,13 +172,16 @@ function authHeaders(token: string): Record { * 2. GET /projects/{enc}/merge_requests/{iid}/commits 获取提交列表 * 3. GET /projects/{enc}/merge_requests/{iid}/changes 获取 diff * （截断至 50KB，失败非致命） + * 4. GET /projects/{enc}/merge_requests/{iid}/discussions 获取评审 threads + * （opts.discussions !== false 时；失败非致命，#1187） * * @param url - GitLab MR 完整 web URL，例如 * https://gitlab.com/group/repo/-/merge_requests/456 + * @param opts - 是否读取评审 threads（默认读取） * @returns 包含标题、描述、提交列表、diff 的 MRData 对象 * @throws Error 当 URL 格式不合法、未配置 token 或 API 调用失败时 */ -export async function fetchGitLabMR(url: string): Promise { +export async function fetchGitLabMR(url: string, opts: MRFetchOptions = {}): Promise { const { apiBase, projectPath, mrIid } = parseGitLabMRUrl(url); const token = getGitLabToken(); if (!token) { @@ -163,6 +234,16 @@ export async function fetchGitLabMR(url: string): Promise { log.debug(`GitLab MR diff 获取异常，diff 将为空：${(err as Error).message}`); } + // ── 4. 获取评审 threads（失败非致命） ───────────────────── + let discussions: MRThread[] | undefined; + if (opts.discussions !== false) { + try { + discussions = await fetchGitLabDiscussions(apiBase, enc, mrIid, headers, opts.commentMarker); + } catch (err) { + log.warn(`Review threads not read (${(err as Error).message}); the learning is extracted without them`); + } + } + return { title: mr.title, description: mr.description ?? '', @@ -171,5 +252,6 @@ export async function fetchGitLabMR(url: string): Promise { commits, diff, url, + ...(discussions ? { discussions } : {}), }; } diff --git a/src/providers/mr-threads.ts b/src/providers/mr-threads.ts new file mode 100644 index 000000000..70a2a6795 --- /dev/null +++ b/src/providers/mr-threads.ts @@ -0,0 +1,32 @@ +import type { MRThread } from '../types.js'; + +/** + * Every comment `teamai ci extract-mr` posts carries an HTML marker with this + * prefix (``, ``, the + * codebase-graph marker). A fetch of the review threads leaves them out so the + * CI extraction does not feed its own output back in (#1187). + */ +const TEAMAI_COMMENT_PREFIX = '