From ac9423e9c8ae8eb854923cb100b41d6915ddf088 Mon Sep 17 00:00:00 2001 From: Michal Date: Sat, 15 Aug 2026 00:05:31 +0100 Subject: [PATCH] feat(git): merge the PR when a task is accepted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes 148/149 — the accept gate now lands code instead of just moving a row. Driver gains createPullRequest / getPullRequest / mergePullRequest, verified against the real Gitea API shapes rather than guessed: MergePullRequestOption requires a capitalised `Do`, and supports `head_commit_id`. That `head_commit_id` is the point. A human approved a SPECIFIC commit; if someone pushed after that, merging anyway lands code nobody reviewed. So the reviewed sha travels with the request and is checked twice — once by us for a clear MergeStaleError naming both commits, and again by the forge, which closes the race between our read and the merge. When it trips, the message says what happened and what to do: re-review the current head, then accept again. Accept merges BEFORE it transitions. If the forge refuses — conflicts, failing checks, a moved branch — the task stays in awaiting_review and no event is written. Reporting `merged` while main does not have the code is the single worst thing this gate could do, and there is a test asserting the transition never happens on a failed merge. Failure modes are answers, not stack traces: no PR attached is approval-only and succeeds; a repo with no forge connection explains that mcpctl cannot merge it; an already-merged PR is treated as success so a retry after a timeout is safe. owner/repo comes from `sourceFullName` when discovery recorded it, falling back to parsing https, ssh:// and scp-style clone URLs, and returning null rather than guessing when it cannot tell. The merger is injected and optional, so an mcpd without a git provider still runs and accept degrades to approval-only — which is also what the help text now says, restored after the earlier commit that removed a claim we could not back up. Tests: 7 driver PR tests (Do field, head pinning, stale refusal, idempotent re-merge, 405 → refusal), 10 merger tests incl. URL parsing, 7 accept-gate tests. Local: build clean, workspace 2580 passed (+23), lint unchanged at 794 errors. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01N4wNHWf7xSwnZCWpJcyv9p --- .taskmaster/tasks/tasks.json | 12 +- completions/mcpctl.fish | 2 +- src/cli/src/commands/tasks.ts | 2 +- src/mcpd/src/main.ts | 10 +- src/mcpd/src/services/git/gitea.driver.ts | 124 ++++++++++++++++++- src/mcpd/src/services/git/types.ts | 66 ++++++++++ src/mcpd/src/services/task-merger.ts | 108 ++++++++++++++++ src/mcpd/src/services/task.service.ts | 43 +++++-- src/mcpd/tests/services/git-provider.test.ts | 103 ++++++++++++++- src/mcpd/tests/services/task-merger.test.ts | 105 ++++++++++++++++ src/mcpd/tests/services/task-service.test.ts | 68 ++++++++++ 11 files changed, 623 insertions(+), 20 deletions(-) create mode 100644 src/mcpd/src/services/task-merger.ts create mode 100644 src/mcpd/tests/services/task-merger.test.ts diff --git a/.taskmaster/tasks/tasks.json b/.taskmaster/tasks/tasks.json index 6fd7448..bc4452c 100644 --- a/.taskmaster/tasks/tasks.json +++ b/.taskmaster/tasks/tasks.json @@ -3031,9 +3031,9 @@ "dependencies": [ "132" ], - "status": "pending", + "status": "done", "subtasks": [], - "updatedAt": "2026-08-08T08:55:58.627Z" + "updatedAt": "2026-08-14T23:05:31.782Z" }, { "id": "149", @@ -3046,9 +3046,9 @@ "130", "148" ], - "status": "pending", + "status": "done", "subtasks": [], - "updatedAt": "2026-08-08T08:55:58.627Z" + "updatedAt": "2026-08-14T23:05:31.792Z" }, { "id": "150", @@ -3461,9 +3461,9 @@ ], "metadata": { "version": "1.0.0", - "lastModified": "2026-08-10T11:11:23.912Z", + "lastModified": "2026-08-14T23:05:31.792Z", "taskCount": 177, - "completedCount": 134, + "completedCount": 136, "tags": [ "master" ] diff --git a/completions/mcpctl.fish b/completions/mcpctl.fish index 9cac1cb..30a055e 100644 --- a/completions/mcpctl.fish +++ b/completions/mcpctl.fish @@ -371,7 +371,7 @@ complete -c mcpctl -n "__mcpctl_subcmd_active repos import" -l ssh -d 'Use ssh c set -l tasks_cmds advance verdict accept reject history complete -c mcpctl -n "__fish_seen_subcommand_from tasks; and not __fish_seen_subcommand_from $tasks_cmds" -a advance -d 'Move a task to the next state' complete -c mcpctl -n "__fish_seen_subcommand_from tasks; and not __fish_seen_subcommand_from $tasks_cmds" -a verdict -d 'Record a QA / Security / SRE verdict, pinned to the commit it judged' -complete -c mcpctl -n "__fish_seen_subcommand_from tasks; and not __fish_seen_subcommand_from $tasks_cmds" -a accept -d 'Accept a task awaiting review (records the decision; does NOT merge the PR yet)' +complete -c mcpctl -n "__fish_seen_subcommand_from tasks; and not __fish_seen_subcommand_from $tasks_cmds" -a accept -d 'Accept a task awaiting review — merges its PR if it has one' complete -c mcpctl -n "__fish_seen_subcommand_from tasks; and not __fish_seen_subcommand_from $tasks_cmds" -a reject -d 'Send a task back with direction (the comment is required)' complete -c mcpctl -n "__fish_seen_subcommand_from tasks; and not __fish_seen_subcommand_from $tasks_cmds" -a history -d 'Show the transition log and recorded verdicts' diff --git a/src/cli/src/commands/tasks.ts b/src/cli/src/commands/tasks.ts index 469a646..8a5114e 100644 --- a/src/cli/src/commands/tasks.ts +++ b/src/cli/src/commands/tasks.ts @@ -85,7 +85,7 @@ export function createTasksCommand(deps: TasksCommandDeps): Command { }); cmd.command('accept') - .description('Accept a task awaiting review (records the decision; does NOT merge the PR yet)') + .description('Accept a task awaiting review — merges its PR if it has one') .argument('', 'Task name or id') .option('--note ', 'Comment stored with the decision') .action(async (task: string, opts) => { diff --git a/src/mcpd/src/main.ts b/src/mcpd/src/main.ts index a9aa8d7..bb62d48 100644 --- a/src/mcpd/src/main.ts +++ b/src/mcpd/src/main.ts @@ -131,6 +131,7 @@ import { EpicService } from './services/epic.service.js'; import { EpicTaskService } from './services/epic-task.service.js'; import { TaskService } from './services/task.service.js'; import { CodeRepoService } from './services/code-repo.service.js'; +import { GitTaskMerger } from './services/task-merger.js'; import { GitProviderService } from './services/git-provider.service.js'; import { GitProviderRegistry } from './services/git/registry.js'; import { McpctlToolProvider } from './services/mcpctl-tools.js'; @@ -402,13 +403,20 @@ async function main(): Promise { const epicService = new EpicService(epicRepo, projectRepo, epicTaskRepo, workTaskRepo); epicService.setRevisionService(resourceRevisionService); const epicTaskService = new EpicTaskService(epicTaskRepo, epicRepo); - const workTaskService = new TaskService(workTaskRepo, epicTaskRepo); const codeRepoService = new CodeRepoService(codeRepoRepo, projectRepo, secretRepo, secretService, gitProviderRepo); // Forge drivers are resolved by type — nothing above this layer knows what a // Gitea is, and the instance URL always comes from the GitProvider row. const gitProviderService = new GitProviderService( gitProviderRepo, codeRepoRepo, projectRepo, new GitProviderRegistry(), secretRepo, secretService, ); + // Accepting a task lands its PR. Constructed after the provider service + // because it needs it; without a merger, accept records approval only. + const gitDrivers = new GitProviderRegistry(); + const workTaskService = new TaskService( + workTaskRepo, + epicTaskRepo, + new GitTaskMerger(codeRepoRepo, gitProviderService, gitDrivers), + ); const personalityRepo = new PersonalityRepository(prisma); const personalityService = new PersonalityService(personalityRepo, agentRepo, promptRepo); // Agentic teams: an agent is a person who joins projects. The junction repo diff --git a/src/mcpd/src/services/git/gitea.driver.ts b/src/mcpd/src/services/git/gitea.driver.ts index 6cd5557..58c6c50 100644 --- a/src/mcpd/src/services/git/gitea.driver.ts +++ b/src/mcpd/src/services/git/gitea.driver.ts @@ -1,10 +1,16 @@ import { GitProviderAuthError, GitProviderError, + MergeConflictError, + MergeStaleError, + type CreatePullRequestInput, type DiscoverOptions, type DiscoveredRepo, type GitProviderContext, type GitProviderDriver, + type MergeOptions, + type PullRequest, + type PullRequestRef, } from './types.js'; /** Gitea's search endpoint wraps results; /version does not. */ @@ -27,6 +33,18 @@ interface GiteaRepo { owner?: { login?: string }; } +interface GiteaPull { + number?: number; + html_url?: string; + url?: string; + title?: string; + state?: string; + merged?: boolean; + mergeable?: boolean; + head?: { sha?: string; ref?: string }; + base?: { ref?: string }; +} + const PAGE_SIZE = 50; const DEFAULT_LIMIT = 200; const TIMEOUT_MS = 15_000; @@ -34,15 +52,25 @@ const TIMEOUT_MS = 15_000; export class GiteaDriver implements GitProviderDriver { readonly type = 'gitea'; - private async call(ctx: GitProviderContext, path: string): Promise { + private async call( + ctx: GitProviderContext, + path: string, + init: { method?: string; body?: unknown } = {}, + ): Promise { const base = ctx.url.replace(/\/+$/, ''); const headers: Record = { Accept: 'application/json' }; // Gitea's scheme is `token `, not `Bearer`. if (ctx.token !== null) headers['Authorization'] = `token ${ctx.token}`; + if (init.body !== undefined) headers['Content-Type'] = 'application/json'; let res: Response; try { - res = await fetch(`${base}${path}`, { headers, signal: AbortSignal.timeout(TIMEOUT_MS) }); + res = await fetch(`${base}${path}`, { + method: init.method ?? 'GET', + headers, + ...(init.body !== undefined ? { body: JSON.stringify(init.body) } : {}), + signal: AbortSignal.timeout(TIMEOUT_MS), + }); } catch (err: unknown) { const detail = err instanceof Error ? err.message : String(err); throw new GitProviderError(`Cannot reach Gitea at ${base}: ${detail}`); @@ -115,6 +143,98 @@ export class GiteaDriver implements GitProviderDriver { return out.slice(0, limit); } + + // ── Pull requests ── + + async createPullRequest(ctx: GitProviderContext, input: CreatePullRequestInput): Promise { + const pull = await this.call( + ctx, + `/api/v1/repos/${enc(input.owner)}/${enc(input.repo)}/pulls`, + { + method: 'POST', + body: { + title: input.title, + base: input.base, + head: input.head, + ...(input.body !== undefined ? { body: input.body } : {}), + }, + }, + ); + return toPullRequest(pull); + } + + async getPullRequest(ctx: GitProviderContext, ref: PullRequestRef): Promise { + const pull = await this.call( + ctx, + `/api/v1/repos/${enc(ref.owner)}/${enc(ref.repo)}/pulls/${String(ref.number)}`, + ); + return toPullRequest(pull); + } + + /** + * Merge, refusing rather than guessing. + * + * `expectHeadSha` is checked twice on purpose: once here so the caller gets a + * clear MergeStaleError naming both commits, and again by the forge via + * `head_commit_id`, which closes the race between our read and the merge. + */ + async mergePullRequest( + ctx: GitProviderContext, + ref: PullRequestRef, + opts: MergeOptions = {}, + ): Promise { + if (opts.expectHeadSha !== undefined) { + const current = await this.getPullRequest(ctx, ref); + if (current.merged) return; // already landed — merging is idempotent + if (current.headSha !== opts.expectHeadSha) { + throw new MergeStaleError(opts.expectHeadSha, current.headSha); + } + } + + try { + await this.call( + ctx, + `/api/v1/repos/${enc(ref.owner)}/${enc(ref.repo)}/pulls/${String(ref.number)}/merge`, + { + method: 'POST', + body: { + // Gitea capitalises this field. + Do: opts.method ?? 'merge', + ...(opts.expectHeadSha !== undefined ? { head_commit_id: opts.expectHeadSha } : {}), + ...(opts.deleteBranchAfterMerge === true ? { delete_branch_after_merge: true } : {}), + }, + }, + ); + } catch (err: unknown) { + // Gitea answers 405 when the PR is not in a mergeable state — conflicts, + // failing required checks, or a closed PR. That is a refusal, not a fault. + const msg = err instanceof Error ? err.message : String(err); + if (msg.includes('HTTP 405') || /conflict/i.test(msg)) { + throw new MergeConflictError( + `Forge refused to merge PR #${String(ref.number)}: it is not in a mergeable state (conflicts, or required checks not passing).`, + ); + } + throw err; + } + } +} + +function enc(v: string): string { + return encodeURIComponent(v); +} + +function toPullRequest(p: GiteaPull): PullRequest { + return { + number: p.number ?? 0, + url: p.html_url ?? p.url ?? '', + title: p.title ?? '', + state: p.state ?? 'unknown', + merged: p.merged ?? false, + headSha: p.head?.sha ?? '', + head: p.head?.ref ?? '', + base: p.base?.ref ?? '', + mergeable: p.mergeable ?? null, + }; } function toDiscovered(r: GiteaRepo): DiscoveredRepo { diff --git a/src/mcpd/src/services/git/types.ts b/src/mcpd/src/services/git/types.ts index 2a02bb3..f5111ba 100644 --- a/src/mcpd/src/services/git/types.ts +++ b/src/mcpd/src/services/git/types.ts @@ -39,12 +39,78 @@ export interface GitProviderContext { token: string | null; } +/** Owner-qualified location of a pull request on a forge. */ +export interface PullRequestRef { + owner: string; + repo: string; + number: number; +} + +export interface CreatePullRequestInput { + owner: string; + repo: string; + title: string; + body?: string; + /** Source branch. */ + head: string; + /** Target branch. */ + base: string; +} + +export interface PullRequest { + number: number; + url: string; + title: string; + state: string; + merged: boolean; + /** Tip commit of the source branch — what a reviewer actually looked at. */ + headSha: string; + head: string; + base: string; + /** Forge's own verdict on whether it can merge cleanly. Null when unknown. */ + mergeable: boolean | null; +} + +export interface MergeOptions { + method?: 'merge' | 'squash' | 'rebase'; + /** + * Merge only if the PR head is still this commit. + * + * The human accepted a specific commit. If someone pushed after that, merging + * anyway lands code nobody approved — so the expected sha travels with the + * request and the forge refuses on mismatch. + */ + expectHeadSha?: string; + deleteBranchAfterMerge?: boolean; +} + export interface GitProviderDriver { readonly type: string; /** List repositories visible to the configured credential. */ listRepositories(ctx: GitProviderContext, opts?: DiscoverOptions): Promise; /** Cheap credential/URL check used at registration time. */ probe(ctx: GitProviderContext): Promise<{ ok: boolean; detail: string }>; + + createPullRequest(ctx: GitProviderContext, input: CreatePullRequestInput): Promise; + getPullRequest(ctx: GitProviderContext, ref: PullRequestRef): Promise; + /** Merge it. Throws MergeConflictError / MergeStaleError rather than lying. */ + mergePullRequest(ctx: GitProviderContext, ref: PullRequestRef, opts?: MergeOptions): Promise; +} + +/** The forge refused because the PR cannot merge cleanly. */ +export class MergeConflictError extends Error { + constructor(message: string) { super(message); this.name = 'MergeConflictError'; } +} + +/** + * The PR moved since it was reviewed. Distinct from a conflict: nothing is + * broken, the approval is simply no longer about the current code. + */ +export class MergeStaleError extends Error { + constructor(readonly expected: string, readonly actual: string) { + super(`Pull request moved since it was reviewed: approved ${expected.slice(0, 8)}, head is now ${actual.slice(0, 8)}`); + this.name = 'MergeStaleError'; + } } /** Raised when a forge rejects the credential or its scopes are insufficient. */ diff --git a/src/mcpd/src/services/task-merger.ts b/src/mcpd/src/services/task-merger.ts new file mode 100644 index 0000000..f966b44 --- /dev/null +++ b/src/mcpd/src/services/task-merger.ts @@ -0,0 +1,108 @@ +import type { Task } from '@prisma/client'; + +import type { ICodeRepoRepository } from '../repositories/code-repo.repository.js'; + +import type { GitProviderService } from './git-provider.service.js'; +import type { GitProviderRegistry } from './git/registry.js'; +import { MergeConflictError, MergeStaleError } from './git/types.js'; +import type { TaskMerger } from './task.service.js'; + +/** + * Lands the pull request behind a task when the human accepts it. + * + * The accept gate only means something if accepting actually lands the code. + * Everything here is written so that a failure leaves the task un-merged and + * says why, rather than reporting success the board can't back up. + */ +export class GitTaskMerger implements TaskMerger { + constructor( + private readonly codeRepos: ICodeRepoRepository, + private readonly providers: GitProviderService, + private readonly drivers: GitProviderRegistry, + private readonly mergeMethod: 'merge' | 'squash' | 'rebase' = 'merge', + ) {} + + async mergeForTask(task: Task, expectHeadSha: string | null): Promise<{ merged: boolean; detail: string }> { + if (task.prNumber === null) { + return { merged: true, detail: 'no pull request attached — approval recorded only' }; + } + if (task.repoId === null) { + return { merged: false, detail: `Task '${task.name}' has PR #${String(task.prNumber)} but no repo, so there is nothing to merge it into. Attach a repo first.` }; + } + + const repo = await this.codeRepos.findById(task.repoId); + if (repo === null) { + return { merged: false, detail: `Task '${task.name}' points at a repo that no longer exists.` }; + } + if (repo.gitProviderId === null) { + return { merged: false, detail: `Repo '${repo.name}' is not connected to a git provider, so mcpctl cannot merge its pull requests. Import it from a gitprovider, or merge in the forge.` }; + } + + const slug = ownerRepoOf(repo.sourceFullName, repo.url); + if (slug === null) { + return { merged: false, detail: `Cannot work out owner/repo for '${repo.name}' from '${repo.url}'.` }; + } + + const provider = await this.providers.get(repo.gitProviderId); + const driver = this.drivers.build(provider.type); + const ctx = await this.providers.contextFor(provider); + + try { + await driver.mergePullRequest( + ctx, + { owner: slug.owner, repo: slug.repo, number: task.prNumber }, + { + method: this.mergeMethod, + // Null headCommit means we never recorded what was reviewed, so there + // is nothing to pin to — merge unpinned rather than refusing outright. + ...(expectHeadSha !== null ? { expectHeadSha } : {}), + }, + ); + return { + merged: true, + detail: `merged ${slug.owner}/${slug.repo}#${String(task.prNumber)} via ${provider.name}`, + }; + } catch (err: unknown) { + if (err instanceof MergeStaleError) { + return { + merged: false, + detail: + `Not merging: ${err.message}. Someone pushed after this was reviewed, so accepting now would land code nobody approved. ` + + 'Re-review the current head, then accept again.', + }; + } + if (err instanceof MergeConflictError) { + return { merged: false, detail: err.message }; + } + return { + merged: false, + detail: `Merge failed: ${err instanceof Error ? err.message : String(err)}`, + }; + } + } +} + +/** + * Owner/repo for the forge API. + * + * Prefers `sourceFullName`, which discovery recorded verbatim. Falls back to + * parsing the clone URL for hand-added repos, handling https and scp-style ssh. + */ +export function ownerRepoOf( + sourceFullName: string | null, + url: string, +): { owner: string; repo: string } | null { + if (sourceFullName !== null && sourceFullName.includes('/')) { + const [owner, ...rest] = sourceFullName.split('/'); + if (owner !== undefined && rest.length > 0) { + return { owner, repo: rest.join('/').replace(/\.git$/, '') }; + } + } + + // git@host:owner/repo.git | https://host/owner/repo.git | ssh://git@host/owner/repo + const scp = url.match(/^[\w.-]+@[\w.-]+:(.+)$/); + const path = scp?.[1] ?? url.replace(/^\w+:\/\/[^/]+\//, ''); + const parts = path.replace(/\.git$/, '').split('/').filter((p) => p !== ''); + if (parts.length < 2) return null; + return { owner: parts[parts.length - 2] as string, repo: parts[parts.length - 1] as string }; +} diff --git a/src/mcpd/src/services/task.service.ts b/src/mcpd/src/services/task.service.ts index 7131d75..b72f29e 100644 --- a/src/mcpd/src/services/task.service.ts +++ b/src/mcpd/src/services/task.service.ts @@ -31,10 +31,8 @@ const TRANSITIONS: Record = { security_review: ['sre_review', 'done', 'in_progress', 'cancelled'], sre_review: ['done', 'in_progress', 'cancelled'], done: ['awaiting_review', 'in_progress', 'cancelled'], - // The human gate: accept records the decision, reject sends it back with a - // comment. NOTE: accept does not yet merge anything — the git-provider merge - // call is task 148/149. Until then `merged` means "a human approved it", not - // "it is in main". + // The human gate: accept lands the PR and records the decision, reject sends + // it back with a comment. awaiting_review: ['merged', 'in_progress', 'cancelled'], // Post-merge failure reopens; the same agent picks it back up. merged: ['reopened'], @@ -74,10 +72,20 @@ export interface TransitionResult { event: TaskEvent; } +/** Lands the PR behind a task. Optional: without it, accept records approval only. */ +export interface TaskMerger { + /** + * @param expectHeadSha merge only if the PR head is still this commit — + * the human approved that commit, not whatever landed after. + */ + mergeForTask(task: Task, expectHeadSha: string | null): Promise<{ merged: boolean; detail: string }>; +} + export class TaskService { constructor( private readonly repo: ITaskRepository, private readonly epicTaskRepo: IEpicTaskRepository, + private readonly merger?: TaskMerger, ) {} static isLegal(from: TaskState, to: TaskState): boolean { @@ -266,9 +274,9 @@ export class TaskService { * Accept: awaiting_review → merged. The caller's comment is stored on the * task, so the decision and its reason live together. * - * Does NOT merge a pull request. No git-provider client is wired yet (148), - * so this records human approval and nothing else. Anything that reads - * `merged` as "landed in main" is currently wrong. + * Merges the task's pull request when it has one and a merger is wired. A + * task with no PR is still acceptable — approval-only is legitimate for work + * that never produced one. * * This is a task-level yes/no on a resolved outcome, NOT a per-tool approval * — the agent has already acted autonomously up to this point. @@ -280,7 +288,26 @@ export class TaskService { throw badRequest(`Only a task in 'awaiting_review' can be accepted (this one is '${task.state}')`); } if (note !== undefined) await this.repo.update(id, { reviewNote: note }); - return this.transition(id, { toState: 'merged', note: note ?? 'accepted' }, actor); + + // Merge FIRST, then transition. If the forge refuses — conflicts, failing + // checks, or the branch moved since review — the task must stay in + // awaiting_review. Marking it merged when nothing merged is the single + // worst thing this gate could do: the board would say landed while main + // does not have the code. + let mergeDetail = 'no pull request attached — approval recorded only'; + if (this.merger !== undefined && task.prNumber !== null) { + const result = await this.merger.mergeForTask(task, task.headCommit); + if (!result.merged) { + throw Object.assign(new Error(result.detail), { statusCode: 409 }); + } + mergeDetail = result.detail; + } + + return this.transition( + id, + { toState: 'merged', note: note !== undefined ? `${note} — ${mergeDetail}` : mergeDetail }, + actor, + ); } /** Reject: back to in_progress. The comment is mandatory — that is the direction. */ diff --git a/src/mcpd/tests/services/git-provider.test.ts b/src/mcpd/tests/services/git-provider.test.ts index 60efa38..f7cc410 100644 --- a/src/mcpd/tests/services/git-provider.test.ts +++ b/src/mcpd/tests/services/git-provider.test.ts @@ -7,7 +7,7 @@ import type { ISecretRepository } from '../../src/repositories/interfaces.js'; import type { IProjectRepository } from '../../src/repositories/project.repository.js'; import { GiteaDriver } from '../../src/services/git/gitea.driver.js'; import { GitProviderRegistry, UnsupportedGitProviderError } from '../../src/services/git/registry.js'; -import { GitProviderAuthError, GitProviderError } from '../../src/services/git/types.js'; +import { GitProviderAuthError, GitProviderError, MergeConflictError, MergeStaleError } from '../../src/services/git/types.js'; import { GitProviderService, repoNameFrom } from '../../src/services/git-provider.service.js'; import type { SecretService } from '../../src/services/secret.service.js'; @@ -340,3 +340,104 @@ describe('GitProviderService.importRepos', () => { ).rejects.toThrow(); }); }); + +// ── Pull requests ───────────────────────────────────────────────────────── + +describe('GiteaDriver — pull requests', () => { + const original = globalThis.fetch; + afterEach(() => { globalThis.fetch = original; }); + + const CTX = { url: 'https://mysources.co.uk', token: 'tok123' }; + const REF = { owner: 'michal', repo: 'mcpctl', number: 7 }; + const pull = (o: Record = {}) => ({ + number: 7, html_url: 'https://mysources.co.uk/michal/mcpctl/pulls/7', + title: 'Wire the CLI', state: 'open', merged: false, mergeable: true, + head: { sha: 'abc1234def', ref: 'feat/wheels' }, base: { ref: 'main' }, ...o, + }); + + it('creates a PR and maps the forge payload', async () => { + let sent: Record = {}; + globalThis.fetch = mockFetch((_u, init) => { + sent = JSON.parse(String(init.body ?? '{}')) as Record; + return { status: 201, body: pull() }; + }) as unknown as typeof fetch; + + const pr = await new GiteaDriver().createPullRequest(CTX, { + owner: 'michal', repo: 'mcpctl', title: 'Wire the CLI', head: 'feat/wheels', base: 'main', + }); + + expect(sent).toEqual({ title: 'Wire the CLI', base: 'main', head: 'feat/wheels' }); + expect(pr).toMatchObject({ number: 7, headSha: 'abc1234def', base: 'main', merged: false }); + }); + + it('merges with the capitalised Do field the forge actually requires', async () => { + let sent: Record = {}; + globalThis.fetch = mockFetch((url, init) => { + if (init.method === 'POST') { + sent = JSON.parse(String(init.body ?? '{}')) as Record; + return { status: 200, body: {} }; + } + return { status: 200, body: pull() }; + }) as unknown as typeof fetch; + + await new GiteaDriver().mergePullRequest(CTX, REF, { method: 'squash' }); + expect(sent).toMatchObject({ Do: 'squash' }); + }); + + it('refuses to merge a PR that moved since it was reviewed', async () => { + // The human approved a specific commit. Merging anyway lands code nobody + // looked at — the whole point of the accept gate. + globalThis.fetch = mockFetch(() => ({ status: 200, body: pull({ head: { sha: 'newsha999', ref: 'f' } }) })) as unknown as typeof fetch; + + await expect( + new GiteaDriver().mergePullRequest(CTX, REF, { expectHeadSha: 'abc1234def' }), + ).rejects.toThrow(MergeStaleError); + await expect( + new GiteaDriver().mergePullRequest(CTX, REF, { expectHeadSha: 'abc1234def' }), + ).rejects.toThrow(/approved abc1234d, head is now newsha99/); + }); + + it('sends head_commit_id so the forge closes the read-then-merge race', async () => { + let sent: Record = {}; + globalThis.fetch = mockFetch((_u, init) => { + if (init.method === 'POST') { + sent = JSON.parse(String(init.body ?? '{}')) as Record; + return { status: 200, body: {} }; + } + return { status: 200, body: pull() }; + }) as unknown as typeof fetch; + + await new GiteaDriver().mergePullRequest(CTX, REF, { expectHeadSha: 'abc1234def' }); + expect(sent).toMatchObject({ head_commit_id: 'abc1234def' }); + }); + + it('treats an already-merged PR as success, not an error', async () => { + // Merging is idempotent: a retry after a timeout must not fail. + let posts = 0; + globalThis.fetch = mockFetch((_u, init) => { + if (init.method === 'POST') { posts++; return { status: 200, body: {} }; } + return { status: 200, body: pull({ merged: true }) }; + }) as unknown as typeof fetch; + + await expect( + new GiteaDriver().mergePullRequest(CTX, REF, { expectHeadSha: 'anything' }), + ).resolves.toBeUndefined(); + expect(posts).toBe(0); + }); + + it('maps the forge 405 to a refusal, not a crash', async () => { + globalThis.fetch = mockFetch((_u, init) => + init.method === 'POST' + ? { status: 405, body: { message: 'Please try again later' } } + : { status: 200, body: pull() }, + ) as unknown as typeof fetch; + + await expect(new GiteaDriver().mergePullRequest(CTX, REF)).rejects.toThrow(MergeConflictError); + await expect(new GiteaDriver().mergePullRequest(CTX, REF)).rejects.toThrow(/not in a mergeable state/); + }); + + it('still reports a bad credential as an auth error on the PR path', async () => { + globalThis.fetch = mockFetch(() => ({ status: 403, body: { message: 'token scope' } })) as unknown as typeof fetch; + await expect(new GiteaDriver().getPullRequest(CTX, REF)).rejects.toThrow(GitProviderAuthError); + }); +}); diff --git a/src/mcpd/tests/services/task-merger.test.ts b/src/mcpd/tests/services/task-merger.test.ts new file mode 100644 index 0000000..65da86a --- /dev/null +++ b/src/mcpd/tests/services/task-merger.test.ts @@ -0,0 +1,105 @@ +import type { CodeRepo, GitProvider, Task } from '@prisma/client'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +import type { ICodeRepoRepository } from '../../src/repositories/code-repo.repository.js'; +import type { GitProviderRegistry } from '../../src/services/git/registry.js'; +import { MergeConflictError, MergeStaleError } from '../../src/services/git/types.js'; +import type { GitProviderService } from '../../src/services/git-provider.service.js'; +import { GitTaskMerger, ownerRepoOf } from '../../src/services/task-merger.js'; + +const task = (o: Partial = {}): Task => + ({ id: 't1', name: 'wire-cli', prNumber: 7, repoId: 'r1', headCommit: 'abc1234def', ...o } as Task); + +const repo = (o: Partial = {}): CodeRepo => + ({ id: 'r1', name: 'mcpctl', url: 'https://mysources.co.uk/michal/mcpctl.git', + gitProviderId: 'gp1', sourceFullName: 'michal/mcpctl', ...o } as CodeRepo); + +function harness(opts: { repoRow?: CodeRepo | null; mergeErr?: unknown } = {}) { + const merge = vi.fn(async () => { if (opts.mergeErr !== undefined) throw opts.mergeErr; }); + const codeRepos = { + findById: vi.fn(async () => (opts.repoRow === undefined ? repo() : opts.repoRow)), + } as unknown as ICodeRepoRepository; + const providers = { + get: vi.fn(async () => ({ id: 'gp1', name: 'mysources', type: 'gitea' } as GitProvider)), + contextFor: vi.fn(async () => ({ url: 'https://mysources.co.uk', token: 'tok' })), + } as unknown as GitProviderService; + const drivers = { build: vi.fn(() => ({ mergePullRequest: merge })) } as unknown as GitProviderRegistry; + return { merge, merger: new GitTaskMerger(codeRepos, providers, drivers) }; +} + +describe('GitTaskMerger', () => { + beforeEach(() => { vi.clearAllMocks(); }); + + it('merges the PR and pins to the reviewed commit', async () => { + const { merger, merge } = harness(); + const r = await merger.mergeForTask(task(), 'abc1234def'); + + expect(r.merged).toBe(true); + expect(r.detail).toMatch(/merged michal\/mcpctl#7 via mysources/); + expect(merge).toHaveBeenCalledWith( + expect.anything(), + { owner: 'michal', repo: 'mcpctl', number: 7 }, + expect.objectContaining({ expectHeadSha: 'abc1234def' }), + ); + }); + + it('does not pin when no reviewed commit was ever recorded', async () => { + const { merger, merge } = harness(); + await merger.mergeForTask(task({ headCommit: null }), null); + expect(merge.mock.calls[0]?.[2]).not.toHaveProperty('expectHeadSha'); + }); + + it('refuses, with direction, when the branch moved after review', async () => { + const { merger } = harness({ mergeErr: new MergeStaleError('abc1234def', 'newsha999') }); + const r = await merger.mergeForTask(task(), 'abc1234def'); + + expect(r.merged).toBe(false); + expect(r.detail).toMatch(/would land code nobody approved/); + expect(r.detail).toMatch(/Re-review the current head/); + }); + + it('reports a forge refusal without dressing it up as success', async () => { + const { merger } = harness({ mergeErr: new MergeConflictError('not in a mergeable state') }); + const r = await merger.mergeForTask(task(), null); + expect(r.merged).toBe(false); + expect(r.detail).toMatch(/not in a mergeable state/); + }); + + it('treats a task with no PR as approval-only, not a failure', async () => { + const { merger, merge } = harness(); + const r = await merger.mergeForTask(task({ prNumber: null }), null); + expect(r.merged).toBe(true); + expect(r.detail).toMatch(/no pull request attached/); + expect(merge).not.toHaveBeenCalled(); + }); + + it('explains rather than merges when the repo has no forge connection', async () => { + const { merger } = harness({ repoRow: repo({ gitProviderId: null }) }); + const r = await merger.mergeForTask(task(), null); + expect(r.merged).toBe(false); + expect(r.detail).toMatch(/not connected to a git provider/); + }); + + it('explains when the repo is gone', async () => { + const { merger } = harness({ repoRow: null }); + const r = await merger.mergeForTask(task(), null); + expect(r.merged).toBe(false); + expect(r.detail).toMatch(/no longer exists/); + }); +}); + +describe('ownerRepoOf', () => { + it('prefers the name discovery recorded verbatim', () => { + expect(ownerRepoOf('michal/mcpctl', 'https://x/y/z.git')).toEqual({ owner: 'michal', repo: 'mcpctl' }); + }); + + it('parses https, ssh and scp-style clone urls for hand-added repos', () => { + expect(ownerRepoOf(null, 'https://mysources.co.uk/michal/mcpctl.git')).toEqual({ owner: 'michal', repo: 'mcpctl' }); + expect(ownerRepoOf(null, 'git@mysources.co.uk:michal/mcpctl.git')).toEqual({ owner: 'michal', repo: 'mcpctl' }); + expect(ownerRepoOf(null, 'ssh://git@mysources.co.uk/michal/mcpctl')).toEqual({ owner: 'michal', repo: 'mcpctl' }); + }); + + it('returns null rather than guessing at an unparseable url', () => { + expect(ownerRepoOf(null, 'https://mysources.co.uk/')).toBeNull(); + }); +}); diff --git a/src/mcpd/tests/services/task-service.test.ts b/src/mcpd/tests/services/task-service.test.ts index 419c0b8..aac6b54 100644 --- a/src/mcpd/tests/services/task-service.test.ts +++ b/src/mcpd/tests/services/task-service.test.ts @@ -288,3 +288,71 @@ describe('TaskService verdicts and creation', () => { ).rejects.toThrow(/already exists in epictask 'chassis'/); }); }); + +describe('TaskService.accept — the merge must actually happen', () => { + beforeEach(() => { vi.clearAllMocks(); }); + + function withMerger(result: { merged: boolean; detail: string }, taskOverrides = {}) { + const mergeForTask = vi.fn(async () => result); + const m = mocks(makeTask({ state: 'awaiting_review', prNumber: 7, ...taskOverrides })); + return { + ...m, + mergeForTask, + service: new TaskService(m.repo, m.epicTaskRepo, { mergeForTask }), + }; + } + + it('lands the PR before moving the task', async () => { + const { service, mergeForTask } = withMerger({ merged: true, detail: 'merged michal/mcpctl#7' }); + const { task } = await service.accept('task-1', {}, ACTOR); + + expect(mergeForTask).toHaveBeenCalled(); + expect(task.state).toBe('merged'); + }); + + it('leaves the task in awaiting_review when the forge refuses', async () => { + // The single worst thing this gate could do is report landed while main + // does not have the code. + const { service, repo } = withMerger({ merged: false, detail: 'conflicts on main' }); + + await expect(service.accept('task-1', {}, ACTOR)).rejects.toThrow(/conflicts on main/); + expect(repo.transition).not.toHaveBeenCalled(); + expect(repo.appendEvent).not.toHaveBeenCalled(); + }); + + it('records what actually merged in the transition note', async () => { + const { service, repo } = withMerger({ merged: true, detail: 'merged michal/mcpctl#7 via mysources' }); + await service.accept('task-1', { note: 'ship it' }, ACTOR); + + expect(repo.appendEvent).toHaveBeenCalledWith( + expect.objectContaining({ note: expect.stringContaining('merged michal/mcpctl#7 via mysources') }), + ); + }); + + it('passes the reviewed commit so a moved branch cannot slip through', async () => { + const { service, mergeForTask } = withMerger( + { merged: true, detail: 'ok' }, { headCommit: 'abc1234def' }, + ); + await service.accept('task-1', {}, ACTOR); + expect(mergeForTask).toHaveBeenCalledWith(expect.objectContaining({ headCommit: 'abc1234def' }), 'abc1234def'); + }); + + it('still accepts a task with no PR — approval-only is legitimate', async () => { + const mergeForTask = vi.fn(async () => ({ merged: true, detail: 'x' })); + const m = mocks(makeTask({ state: 'awaiting_review', prNumber: null })); + const service = new TaskService(m.repo, m.epicTaskRepo, { mergeForTask }); + + const { task } = await service.accept('task-1', {}, ACTOR); + expect(task.state).toBe('merged'); + expect(mergeForTask).not.toHaveBeenCalled(); + }); + + it('without a merger wired, accept records approval and says so', async () => { + const { service, repo } = mocks(makeTask({ state: 'awaiting_review', prNumber: 7 })); + const { task } = await service.accept('task-1', {}, ACTOR); + expect(task.state).toBe('merged'); + expect(repo.appendEvent).toHaveBeenCalledWith( + expect.objectContaining({ note: expect.stringContaining('approval recorded only') }), + ); + }); +});