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') }), + ); + }); +});