feat(git): merge the PR when a task is accepted

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N4wNHWf7xSwnZCWpJcyv9p
This commit is contained in:
Michal
2026-08-15 00:05:31 +01:00
parent 821c45b2ec
commit ac9423e9c8
11 changed files with 623 additions and 20 deletions

View File

@@ -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>', 'Task name or id')
.option('--note <text>', 'Comment stored with the decision')
.action(async (task: string, opts) => {

View File

@@ -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<void> {
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

View File

@@ -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<T>(ctx: GitProviderContext, path: string): Promise<T> {
private async call<T>(
ctx: GitProviderContext,
path: string,
init: { method?: string; body?: unknown } = {},
): Promise<T> {
const base = ctx.url.replace(/\/+$/, '');
const headers: Record<string, string> = { Accept: 'application/json' };
// Gitea's scheme is `token <value>`, 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<PullRequest> {
const pull = await this.call<GiteaPull>(
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<PullRequest> {
const pull = await this.call<GiteaPull>(
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<void> {
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<unknown>(
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 {

View File

@@ -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<DiscoveredRepo[]>;
/** Cheap credential/URL check used at registration time. */
probe(ctx: GitProviderContext): Promise<{ ok: boolean; detail: string }>;
createPullRequest(ctx: GitProviderContext, input: CreatePullRequestInput): Promise<PullRequest>;
getPullRequest(ctx: GitProviderContext, ref: PullRequestRef): Promise<PullRequest>;
/** Merge it. Throws MergeConflictError / MergeStaleError rather than lying. */
mergePullRequest(ctx: GitProviderContext, ref: PullRequestRef, opts?: MergeOptions): Promise<void>;
}
/** 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. */

View File

@@ -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 };
}

View File

@@ -31,10 +31,8 @@ const TRANSITIONS: Record<TaskState, readonly TaskState[]> = {
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. */

View File

@@ -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<string, unknown> = {}) => ({
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<string, unknown> = {};
globalThis.fetch = mockFetch((_u, init) => {
sent = JSON.parse(String(init.body ?? '{}')) as Record<string, unknown>;
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<string, unknown> = {};
globalThis.fetch = mockFetch((url, init) => {
if (init.method === 'POST') {
sent = JSON.parse(String(init.body ?? '{}')) as Record<string, unknown>;
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<string, unknown> = {};
globalThis.fetch = mockFetch((_u, init) => {
if (init.method === 'POST') {
sent = JSON.parse(String(init.body ?? '{}')) as Record<string, unknown>;
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);
});
});

View File

@@ -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> = {}): Task =>
({ id: 't1', name: 'wire-cli', prNumber: 7, repoId: 'r1', headCommit: 'abc1234def', ...o } as Task);
const repo = (o: Partial<CodeRepo> = {}): 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();
});
});

View File

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