feat(git): reopen a task when CI fails on the commit its merge produced

Closes 151 — the last loop in the lifecycle. A task reaches `merged`, CI then
fails on the merge commit, and the same task comes back as `reopened` instead of
the breakage going unnoticed because the board already said done.

Two properties make this safe rather than a liability.

**It fails closed.** The endpoint is unauthenticated by nature — a forge calls
it — so it accepts nothing unless the provider has a configured webhook signing
secret AND the HMAC verifies. No secret, no effect. The signature is computed
over the RAW body: re-serializing a parsed object changes the bytes, which is
the classic way this check silently stops working, and there is a test for it.
A 404 for an unknown provider is deliberately vague, because confirming which
provider names exist is free reconnaissance on a public endpoint.

**It matches precisely.** Accept now records `merge_commit_sha` — the commit the
merge actually produced, which is what CI runs against — so a failing status
finds exactly the task that caused it. The alternative, reopening "whatever
merged most recently", would reopen innocent work. A failure on a commit mcpctl
did not merge is somebody else's business and is ignored, as is a task that has
already moved on from `merged`.

Capturing the merge commit is best-effort by design: the merge already
succeeded, so failing to re-read the sha must not fail the accept — it only
costs the webhook its precise match later.

The route is skipped by the RBAC hook, since the forge has no mcpctl identity.
That is only safe because of the fail-closed rule above, and both the skip and
the reason are asserted in tests.

Tests: 12 covering every refusal path (no secret, unsigned, wrong key, signature
over different bytes), the vague 404, the RBAC skip, and the reopen matrix.

Local: build clean, workspace 2592 passed (+12), db 128 passed (same 4
pre-existing), 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 23:10:49 +01:00
parent ac9423e9c8
commit f0ba6e8096
12 changed files with 364 additions and 17 deletions

View File

@@ -0,0 +1,6 @@
-- AlterTable
ALTER TABLE "Task" ADD COLUMN "mergeCommit" TEXT;
-- AlterTable
ALTER TABLE "GitProvider" ADD COLUMN "webhookSecretKey" TEXT;

View File

@@ -1047,6 +1047,10 @@ model Task {
headCommit String?
prUrl String?
prNumber Int?
// The commit the merge produced. CI runs against this, so a failing status
// webhook can find exactly the task that caused it instead of guessing at
// "whatever merged most recently".
mergeCommit String?
// Free-form reviewer comment attached at the accept gate.
reviewNote String? @db.Text
ownerId String
@@ -1256,21 +1260,26 @@ enum SessionState {
// is a non-migration change (same reasoning as ResourceProposal.resourceType).
model GitProvider {
id String @id @default(cuid())
name String @unique
description String @default("")
id String @id @default(cuid())
name String @unique
description String @default("")
// Driver key: gitea | (future) github | gitlab
type String @default("gitea")
type String @default("gitea")
// Instance base URL, e.g. https://mysources.co.uk — never hardcoded anywhere.
url String
url String
// API credential, by reference. Same shape as Llm.apiKeySecret*; the value is
// resolved through SecretService and never stored or returned here.
credSecretId String?
credSecretKey String?
ownerId String
version Int @default(1)
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
credSecretId String?
credSecretKey String?
// Key (in the same Secret) holding this forge's webhook signing secret.
// Required before the webhook endpoint accepts anything from this provider —
// an unauthenticated webhook that can reopen work is an attack surface, so
// the route fails closed when this is unset.
webhookSecretKey String?
ownerId String
version Int @default(1)
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
cred Secret? @relation(fields: [credSecretId], references: [id])
owner User @relation(fields: [ownerId], references: [id], onDelete: Cascade)

View File

@@ -114,6 +114,7 @@ import { registerEpicTaskRoutes } from './routes/epictasks.js';
import { registerTaskRoutes } from './routes/tasks.js';
import { registerCodeRepoRoutes } from './routes/repos.js';
import { registerGitProviderRoutes } from './routes/gitproviders.js';
import { registerGitWebhookRoutes } from './routes/git-webhooks.js';
import { registerSkillRoutes } from './routes/skills.js';
import { registerGitBackupRoutes } from './routes/git-backup.js';
import { PromptService } from './services/prompt.service.js';
@@ -643,6 +644,13 @@ async function main(): Promise<void> {
registerTaskRoutes(app, workTaskService, epicTaskRepo);
registerCodeRepoRoutes(app, codeRepoService, projectRepo);
registerGitProviderRoutes(app, gitProviderService, projectRepo);
registerGitWebhookRoutes(app, {
providers: gitProviderRepo,
secretRepo,
secrets: secretService,
tasks: workTaskRepo,
taskService: workTaskService,
});
// ── Git-based backup ──
const gitBackup = new GitBackupService(prisma);

View File

@@ -36,6 +36,11 @@ export function mapUrlToPermission(method: string, url: string): PermissionCheck
return { kind: 'operation', operation: SET_OWN_PASSWORD_OPERATION };
}
// Git webhooks authenticate by HMAC signature, not by a user token — the
// forge has no mcpctl identity. Skipping RBAC here is safe only because the
// route itself fails closed without a configured signing secret.
if (url.startsWith('/api/v1/webhooks/git/')) return { kind: 'skip' };
// Operations (non-resource endpoints)
if (segment === 'backup') return { kind: 'operation', operation: 'backup' };
if (segment === 'restore') return { kind: 'operation', operation: 'restore' };

View File

@@ -31,6 +31,7 @@ export interface TaskUpdateInput {
headCommit?: string;
prUrl?: string;
prNumber?: number;
mergeCommit?: string;
reviewNote?: string;
}
@@ -61,6 +62,8 @@ export interface EventCreateInput {
export interface ITaskRepository {
findAll(filters?: TaskFilters): Promise<Task[]>;
/** Find the task a merge produced this commit for — the webhook's entry point. */
findByMergeCommit(sha: string): Promise<Task | null>;
findById(id: string): Promise<Task | null>;
findByName(name: string): Promise<Task | null>;
findByNameAndEpicTask(name: string, epicTaskId: string): Promise<Task | null>;
@@ -98,6 +101,10 @@ export class TaskRepository implements ITaskRepository {
});
}
async findByMergeCommit(sha: string): Promise<Task | null> {
return this.prisma.task.findFirst({ where: { mergeCommit: sha }, include: TASK_INCLUDE });
}
async findById(id: string): Promise<Task | null> {
return this.prisma.task.findUnique({ where: { id }, include: TASK_INCLUDE });
}

View File

@@ -0,0 +1,142 @@
import { createHmac, timingSafeEqual } from 'node:crypto';
import type { FastifyInstance } from 'fastify';
import type { IGitProviderRepository } from '../repositories/git-provider.repository.js';
import type { ISecretRepository } from '../repositories/interfaces.js';
import type { ITaskRepository } from '../repositories/task.repository.js';
import type { SecretService } from '../services/secret.service.js';
import type { TaskService } from '../services/task.service.js';
/**
* Post-merge failure → reopen the task that caused it.
*
* The design's last loop: a task reaches `merged`, CI then fails on the merge
* commit, and the same task comes back as `reopened` rather than the breakage
* going unnoticed because the board already said done.
*
* Two things make this safe rather than a liability:
*
* 1. **It fails closed.** The endpoint is unauthenticated by nature — a forge
* calls it — so it accepts nothing unless the provider has a configured
* webhook signing secret and the signature verifies. No secret, no effect.
* 2. **It matches precisely.** A failing status carries the commit sha, and the
* accept path recorded exactly which commit each merge produced. No guessing
* at "whatever merged most recently", which would reopen innocent work.
*/
interface GiteaStatusEvent {
sha?: string;
state?: string;
context?: string;
description?: string;
target_url?: string;
repository?: { full_name?: string };
}
/** Failing states across forges. Anything else is ignored. */
const FAILED = new Set(['failure', 'error']);
export interface GitWebhookDeps {
providers: IGitProviderRepository;
secretRepo: ISecretRepository;
secrets: SecretService;
tasks: ITaskRepository;
taskService: TaskService;
}
export function registerGitWebhookRoutes(app: FastifyInstance, deps: GitWebhookDeps): void {
// Raw body is required: a signature is over the exact bytes the forge sent,
// and re-serializing a parsed object changes them.
app.addContentTypeParser(
'application/json',
{ parseAs: 'string' },
(req, body, done) => {
(req as unknown as { rawBody?: string }).rawBody = body as string;
try {
done(null, JSON.parse(body as string));
} catch (err) {
done(err as Error, undefined);
}
},
);
app.post<{ Params: { provider: string } }>(
'/api/v1/webhooks/git/:provider',
async (request, reply) => {
const provider = await deps.providers.findByName(request.params.provider);
if (provider === null) {
// Deliberately vague: this endpoint is public, and confirming which
// provider names exist is free reconnaissance.
reply.code(404);
return { error: 'unknown webhook' };
}
if (provider.webhookSecretKey === null || provider.credSecretId === null) {
reply.code(403);
return {
error: 'webhook_not_configured',
message: `GitProvider '${provider.name}' has no webhook signing secret, so its webhooks are refused.`,
};
}
const secret = await deps.secretRepo.findById(provider.credSecretId);
if (secret === null) {
reply.code(403);
return { error: 'webhook_not_configured', message: 'Signing secret no longer resolvable.' };
}
const signingKey = await deps.secrets.resolve(secret.name, provider.webhookSecretKey);
const raw = (request as unknown as { rawBody?: string }).rawBody ?? '';
const sent = String(request.headers['x-gitea-signature'] ?? request.headers['x-hub-signature-256'] ?? '')
.replace(/^sha256=/, '');
if (!verify(raw, signingKey, sent)) {
reply.code(401);
return { error: 'bad_signature' };
}
const body = request.body as GiteaStatusEvent;
const state = String(body.state ?? '').toLowerCase();
if (!FAILED.has(state)) {
// Success and pending are the common case — acknowledge and do nothing.
return { ok: true, action: 'ignored', reason: `state '${state}' is not a failure` };
}
const sha = body.sha;
if (sha === undefined || sha === '') {
return { ok: true, action: 'ignored', reason: 'no commit sha in payload' };
}
const task = await deps.tasks.findByMergeCommit(sha);
if (task === null) {
// A failing build on a commit mcpctl did not merge is somebody else's
// business — not an error, and not something to guess about.
return { ok: true, action: 'ignored', reason: `no task merged commit ${sha.slice(0, 8)}` };
}
if (task.state !== 'merged') {
return { ok: true, action: 'ignored', reason: `task '${task.name}' is '${task.state}', not 'merged'` };
}
const why = [
`CI failed on the merge commit ${sha.slice(0, 8)}`,
body.context !== undefined ? ` (${body.context})` : '',
body.description !== undefined ? `: ${body.description}` : '',
].join('');
await deps.taskService.reopen(task.id, why, { type: 'system' });
return { ok: true, action: 'reopened', task: task.name, reason: why };
},
);
}
/** Constant-time compare of the forge's HMAC against ours. */
function verify(rawBody: string, key: string, sent: string): boolean {
if (sent === '') return false;
const expected = createHmac('sha256', key).update(rawBody).digest('hex');
const a = Buffer.from(expected, 'utf8');
const b = Buffer.from(sent, 'utf8');
// timingSafeEqual throws on length mismatch, which would itself leak length.
if (a.length !== b.length) return false;
return timingSafeEqual(a, b);
}

View File

@@ -41,6 +41,7 @@ interface GiteaPull {
state?: string;
merged?: boolean;
mergeable?: boolean;
merge_commit_sha?: string;
head?: { sha?: string; ref?: string };
base?: { ref?: string };
}
@@ -234,6 +235,7 @@ function toPullRequest(p: GiteaPull): PullRequest {
head: p.head?.ref ?? '',
base: p.base?.ref ?? '',
mergeable: p.mergeable ?? null,
mergeCommitSha: p.merge_commit_sha ?? null,
};
}

View File

@@ -69,6 +69,8 @@ export interface PullRequest {
base: string;
/** Forge's own verdict on whether it can merge cleanly. Null when unknown. */
mergeable: boolean | null;
/** Commit the merge produced. Null until merged — CI runs against this. */
mergeCommitSha: string | null;
}
export interface MergeOptions {

View File

@@ -22,7 +22,7 @@ export class GitTaskMerger implements TaskMerger {
private readonly mergeMethod: 'merge' | 'squash' | 'rebase' = 'merge',
) {}
async mergeForTask(task: Task, expectHeadSha: string | null): Promise<{ merged: boolean; detail: string }> {
async mergeForTask(task: Task, expectHeadSha: string | null): Promise<{ merged: boolean; detail: string; mergeCommit?: string }> {
if (task.prNumber === null) {
return { merged: true, detail: 'no pull request attached — approval recorded only' };
}
@@ -58,9 +58,21 @@ export class GitTaskMerger implements TaskMerger {
...(expectHeadSha !== null ? { expectHeadSha } : {}),
},
);
// Re-read to capture what the merge produced. Best-effort: the merge
// already succeeded, so failing to read the sha must not fail the accept —
// it only costs the webhook its precise match later.
let mergeCommit: string | undefined;
try {
const after = await driver.getPullRequest(ctx, { owner: slug.owner, repo: slug.repo, number: task.prNumber });
mergeCommit = after.mergeCommitSha ?? undefined;
} catch {
mergeCommit = undefined;
}
return {
merged: true,
detail: `merged ${slug.owner}/${slug.repo}#${String(task.prNumber)} via ${provider.name}`,
...(mergeCommit !== undefined ? { mergeCommit } : {}),
};
} catch (err: unknown) {
if (err instanceof MergeStaleError) {

View File

@@ -78,7 +78,7 @@ 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 }>;
mergeForTask(task: Task, expectHeadSha: string | null): Promise<{ merged: boolean; detail: string; mergeCommit?: string }>;
}
export class TaskService {
@@ -301,6 +301,11 @@ export class TaskService {
throw Object.assign(new Error(result.detail), { statusCode: 409 });
}
mergeDetail = result.detail;
// Record what the merge produced: a failing CI webhook arrives with this
// sha, and it is the only precise way back to the task that caused it.
if (result.mergeCommit !== undefined) {
await this.repo.update(id, { mergeCommit: result.mergeCommit });
}
}
return this.transition(

View File

@@ -0,0 +1,149 @@
import { createHmac } from 'node:crypto';
import type { GitProvider, Secret, Task } from '@prisma/client';
import Fastify, { type FastifyInstance } from 'fastify';
import { describe, it, expect, vi, afterEach } from 'vitest';
import { errorHandler } from '../src/middleware/error-handler.js';
import { mapUrlToPermission } from '../src/middleware/url-permission.js';
import type { IGitProviderRepository } from '../src/repositories/git-provider.repository.js';
import type { ISecretRepository } from '../src/repositories/interfaces.js';
import type { ITaskRepository } from '../src/repositories/task.repository.js';
import { registerGitWebhookRoutes } from '../src/routes/git-webhooks.js';
import type { SecretService } from '../src/services/secret.service.js';
import type { TaskService } from '../src/services/task.service.js';
const SECRET = 'hunter2';
const SHA = 'deadbeefcafe1234';
let app: FastifyInstance;
afterEach(async () => { if (app) await app.close(); });
function build(opts: {
provider?: Partial<GitProvider> | null;
task?: Partial<Task> | null;
} = {}) {
const reopen = vi.fn(async () => ({}));
const providerRow = opts.provider === null ? null : {
id: 'gp1', name: 'mysources', type: 'gitea',
credSecretId: 'sec1', credSecretKey: 'TOKEN', webhookSecretKey: 'WEBHOOK',
...opts.provider,
} as GitProvider;
const taskRow = opts.task === null ? null
: ({ id: 't1', name: 'wire-cli', state: 'merged', mergeCommit: SHA, ...opts.task } as Task);
const instance = Fastify();
instance.setErrorHandler(errorHandler);
registerGitWebhookRoutes(instance, {
providers: { findByName: vi.fn(async () => providerRow) } as unknown as IGitProviderRepository,
secretRepo: { findById: vi.fn(async () => ({ id: 'sec1', name: 'gitea-token' } as Secret)) } as unknown as ISecretRepository,
secrets: { resolve: vi.fn(async () => SECRET) } as unknown as SecretService,
tasks: { findByMergeCommit: vi.fn(async () => taskRow) } as unknown as ITaskRepository,
taskService: { reopen } as unknown as TaskService,
});
return { instance, reopen };
}
const sign = (body: string, key = SECRET) => createHmac('sha256', key).update(body).digest('hex');
async function post(instance: FastifyInstance, payload: unknown, sig?: string) {
const body = JSON.stringify(payload);
return instance.inject({
method: 'POST',
url: '/api/v1/webhooks/git/mysources',
headers: { 'content-type': 'application/json', 'x-gitea-signature': sig ?? sign(body) },
payload: body,
});
}
const failure = { sha: SHA, state: 'failure', context: 'ci/build', description: 'tests failed' };
describe('git webhook — fails closed', () => {
it('refuses when the provider has no signing secret configured', async () => {
const b = build({ provider: { webhookSecretKey: null } }); app = b.instance;
const res = await post(b.instance, failure);
expect(res.statusCode).toBe(403);
expect(b.reopen).not.toHaveBeenCalled();
});
it('refuses an unsigned request', async () => {
const b = build(); app = b.instance;
const res = await post(b.instance, failure, '');
expect(res.statusCode).toBe(401);
expect(b.reopen).not.toHaveBeenCalled();
});
it('refuses a wrong signature', async () => {
const b = build(); app = b.instance;
const res = await post(b.instance, failure, sign(JSON.stringify(failure), 'wrong-key'));
expect(res.statusCode).toBe(401);
expect(b.reopen).not.toHaveBeenCalled();
});
it('refuses a signature over different bytes than were sent', async () => {
// Signing a re-serialized object rather than the raw body is the classic
// way this check silently stops working.
const b = build(); app = b.instance;
const res = await post(b.instance, failure, sign('{"sha":"other"}'));
expect(res.statusCode).toBe(401);
});
it('does not confirm which provider names exist', async () => {
const b = build({ provider: null }); app = b.instance;
const res = await post(b.instance, failure);
expect(res.statusCode).toBe(404);
expect(JSON.stringify(res.json())).not.toContain('mysources');
});
it('is skipped by the RBAC hook — it authenticates by signature', () => {
expect(mapUrlToPermission('POST', '/api/v1/webhooks/git/mysources')).toEqual({ kind: 'skip' });
});
});
describe('git webhook — reopening', () => {
it('reopens the task whose merge produced the failing commit', async () => {
const b = build(); app = b.instance;
const res = await post(b.instance, failure);
expect(res.statusCode).toBe(200);
expect(res.json()).toMatchObject({ action: 'reopened', task: 'wire-cli' });
expect(b.reopen).toHaveBeenCalledWith('t1', expect.stringContaining('deadbeef'), { type: 'system' });
expect(b.reopen.mock.calls[0]?.[1]).toContain('ci/build');
});
it('ignores a passing status', async () => {
const b = build(); app = b.instance;
const res = await post(b.instance, { ...failure, state: 'success' });
expect(res.json()).toMatchObject({ action: 'ignored' });
expect(b.reopen).not.toHaveBeenCalled();
});
it('ignores a failure on a commit mcpctl did not merge', async () => {
// Somebody else's build breaking is not mcpctl's business, and guessing at
// a task would reopen innocent work.
const b = build({ task: null }); app = b.instance;
const res = await post(b.instance, failure);
expect(res.json().reason).toMatch(/no task merged commit deadbeef/);
expect(b.reopen).not.toHaveBeenCalled();
});
it('ignores a task that has already moved on from merged', async () => {
const b = build({ task: { state: 'reopened' } }); app = b.instance;
const res = await post(b.instance, failure);
expect(res.json().reason).toMatch(/is 'reopened', not 'merged'/);
expect(b.reopen).not.toHaveBeenCalled();
});
it('ignores a payload with no sha', async () => {
const b = build(); app = b.instance;
const res = await post(b.instance, { state: 'failure' });
expect(res.json().reason).toMatch(/no commit sha/);
});
it('treats error the same as failure', async () => {
const b = build(); app = b.instance;
await post(b.instance, { ...failure, state: 'error' });
expect(b.reopen).toHaveBeenCalled();
});
});