diff --git a/services/api/src/builder-plan-policy.ts b/services/api/src/builder-plan-policy.ts index 22291bad..68a4dc5a 100644 --- a/services/api/src/builder-plan-policy.ts +++ b/services/api/src/builder-plan-policy.ts @@ -22,6 +22,8 @@ export type BuilderPlanDenialCode = | "builder_plan_context_invalid" | "builder_plan_expired" | "builder_plan_rejected" + | "builder_plan_superseded" + | "builder_plan_ambiguous" | "builder_plan_already_consumed" | "builder_plan_stale" | "builder_plan_freshness_unavailable"; @@ -31,6 +33,8 @@ const BUILDER_PLAN_DENIAL_CODES = new Set([ "builder_plan_context_invalid", "builder_plan_expired", "builder_plan_rejected", + "builder_plan_superseded", + "builder_plan_ambiguous", "builder_plan_already_consumed", "builder_plan_stale", "builder_plan_freshness_unavailable", @@ -428,6 +432,9 @@ async function validatePlanAcceptance( if (proposal.state === "rejected") { return invalid("builder_plan_rejected", "proposal_rejected"); } + if (proposal.state === "cancelled") { + return invalid("builder_plan_superseded", "proposal_superseded"); + } if ( (proposal.state === "open" && proposal.expiresAt.getTime() <= Date.now()) || (proposal.decidedAt && proposal.decidedAt.getTime() > proposal.expiresAt.getTime()) diff --git a/services/api/src/github/plan-acceptance.ts b/services/api/src/github/plan-acceptance.ts new file mode 100644 index 00000000..670d56a4 --- /dev/null +++ b/services/api/src/github/plan-acceptance.ts @@ -0,0 +1,270 @@ +import { actionTypes, type FacilityDb, proposalEvents, proposals, runs } from "@facility/db"; +import { and, desc, eq, gt, ne, sql } from "drizzle-orm"; + +export const PLAN_ACCEPTANCE_PROPOSAL_ID_RE = /^prop_[0-9a-z_]+$/i; + +export type GithubPlanAcceptanceIssueScope = { + orgId: string; + projectId: string; + owner: string; + repo: string; + issueNumber: number; +}; + +export type GithubPlanAcceptanceRow = { + proposal: typeof proposals.$inferSelect; + architectRun: typeof runs.$inferSelect; +}; + +export type GithubPlanAcceptanceResolution = + | ({ status: "resolved" } & GithubPlanAcceptanceRow) + | { status: "ambiguous"; liveProposalIds: string[] }; + +export type GithubPlanAcceptanceCreateInput = { + orgId: string; + projectId: string; + repoId: string; + owner: string; + repo: string; + issueNumber: number; + architectRunId: string; + actionTypeId: string; + proposalId: string; + payload: Record; + contextMd: string; + expiresAt: Date; +}; + +function githubIssueScopeWhere(input: GithubPlanAcceptanceIssueScope) { + return and( + eq(proposals.orgId, input.orgId), + eq(proposals.projectId, input.projectId), + eq(actionTypes.name, "plan_acceptance"), + eq(runs.status, "succeeded"), + sql`${runs.gh} ->> 'owner' = ${input.owner}`, + sql`${runs.gh} ->> 'repo' = ${input.repo}`, + sql`(${runs.gh} ->> 'issueNumber')::int = ${input.issueNumber}`, + ); +} + +function liveOpenPlanAcceptanceWhere(now = new Date()) { + return and(eq(proposals.state, "open"), gt(proposals.expiresAt, now)); +} + +/** Issue-scoped lock so concurrent Architect completions serialize Gate 1 creation. */ +export function githubPlanAcceptanceIssueLockKey(input: { + orgId: string; + repoId: string; + issueNumber: number; +}) { + return `architect-plan-issue:${input.orgId}:${input.repoId}:${input.issueNumber}`; +} + +export async function listLiveGithubPlanAcceptances( + db: FacilityDb, + input: GithubPlanAcceptanceIssueScope, + now = new Date(), +): Promise { + return db + .select({ proposal: proposals, architectRun: runs }) + .from(proposals) + .innerJoin(actionTypes, eq(actionTypes.id, proposals.actionTypeId)) + .innerJoin(runs, eq(runs.id, proposals.runId)) + .where(and(githubIssueScopeWhere(input), liveOpenPlanAcceptanceWhere(now))) + .orderBy(desc(proposals.createdAt)); +} + +async function latestGithubPlanAcceptance( + db: FacilityDb, + input: GithubPlanAcceptanceIssueScope, +): Promise { + const latest = ( + await db + .select({ proposal: proposals, architectRun: runs }) + .from(proposals) + .innerJoin(actionTypes, eq(actionTypes.id, proposals.actionTypeId)) + .innerJoin(runs, eq(runs.id, proposals.runId)) + .where(githubIssueScopeWhere(input)) + .orderBy(desc(proposals.createdAt)) + .limit(1) + )[0]; + return latest ?? null; +} + +async function githubPlanAcceptanceById( + db: FacilityDb, + input: GithubPlanAcceptanceIssueScope & { proposalId: string }, +): Promise { + const match = ( + await db + .select({ proposal: proposals, architectRun: runs }) + .from(proposals) + .innerJoin(actionTypes, eq(actionTypes.id, proposals.actionTypeId)) + .innerJoin(runs, eq(runs.id, proposals.runId)) + .where(and(githubIssueScopeWhere(input), eq(proposals.id, input.proposalId))) + .limit(1) + )[0]; + return match ?? null; +} + +export async function resolveGithubPlanAcceptance( + db: FacilityDb, + input: GithubPlanAcceptanceIssueScope & { proposalId?: string }, + now = new Date(), +): Promise { + if (input.proposalId) { + const explicit = await githubPlanAcceptanceById(db, { + ...input, + proposalId: input.proposalId, + }); + return explicit ? { status: "resolved", ...explicit } : null; + } + + const live = await listLiveGithubPlanAcceptances(db, input, now); + if (live.length > 1) { + return { status: "ambiguous", liveProposalIds: live.map((row) => row.proposal.id) }; + } + if (live.length === 1) { + const [only] = live; + if (!only) return null; + return { status: "resolved", ...only }; + } + + const latest = await latestGithubPlanAcceptance(db, input); + return latest ? { status: "resolved", ...latest } : null; +} + +/** + * Cancel other live Gate 1 proposals on the same issue. + * Caller must already hold the issue advisory lock inside a transaction. + * Operates on the caller's connection — no nested transactions — so create + + * supersede can commit or roll back together. + */ +export async function supersedeOpenGithubPlanAcceptances( + db: FacilityDb, + input: { + orgId: string; + projectId: string; + repoId: string; + issueNumber: number; + architectRunId: string; + keepProposalId: string; + }, + now = new Date(), +) { + const stale = await db + .select({ proposal: proposals }) + .from(proposals) + .innerJoin(actionTypes, eq(actionTypes.id, proposals.actionTypeId)) + .where( + and( + eq(proposals.orgId, input.orgId), + eq(proposals.projectId, input.projectId), + eq(actionTypes.name, "plan_acceptance"), + eq(proposals.state, "open"), + gt(proposals.expiresAt, now), + sql`(${proposals.payload} ->> 'issueNumber')::int = ${input.issueNumber}`, + sql`${proposals.payload} ->> 'repoId' = ${input.repoId}`, + ne(proposals.id, input.keepProposalId), + ), + ); + let superseded = 0; + for (const row of stale) { + const updated = ( + await db + .update(proposals) + .set({ state: "cancelled", updatedAt: now }) + .where( + and( + eq(proposals.orgId, row.proposal.orgId), + eq(proposals.id, row.proposal.id), + eq(proposals.state, "open"), + gt(proposals.expiresAt, now), + ), + ) + .returning() + )[0]; + if (!updated) continue; + const latest = ( + await db + .select({ seq: proposalEvents.seq }) + .from(proposalEvents) + .where( + and(eq(proposalEvents.orgId, updated.orgId), eq(proposalEvents.proposalId, updated.id)), + ) + .orderBy(desc(proposalEvents.seq)) + .limit(1) + )[0]; + await db.insert(proposalEvents).values({ + orgId: updated.orgId, + proposalId: updated.id, + seq: (latest?.seq ?? 0) + 1, + type: "cancelled", + actor: { type: "system", name: "architect_plan_supersede" }, + data: { + reason: "superseded_by_architect_run", + architectRunId: input.architectRunId, + keptProposalId: input.keepProposalId, + }, + }); + superseded += 1; + } + return superseded; +} + +/** + * Atomically open a Gate 1 proposal and cancel older live plans on the same + * issue. Insert happens first so a create failure never leaves the issue + * without a live plan. Concurrent Architect completions serialize on the + * issue advisory lock held for the caller's transaction. + */ +export async function insertGithubPlanAcceptanceReplacingSiblings( + db: FacilityDb, + input: GithubPlanAcceptanceCreateInput, + now = new Date(), +) { + await db.execute( + sql`select pg_advisory_xact_lock(hashtextextended(${githubPlanAcceptanceIssueLockKey({ + orgId: input.orgId, + repoId: input.repoId, + issueNumber: input.issueNumber, + })}, 0))`, + ); + const created = ( + await db + .insert(proposals) + .values({ + id: input.proposalId, + orgId: input.orgId, + projectId: input.projectId, + runId: input.architectRunId, + actionTypeId: input.actionTypeId, + payload: input.payload, + contextMd: input.contextMd, + expiresAt: input.expiresAt, + }) + .returning() + )[0]; + if (!created) throw new Error("plan_acceptance_create_failed"); + await db.insert(proposalEvents).values({ + orgId: input.orgId, + proposalId: created.id, + seq: 1, + type: "open", + actor: { type: "agent", id: input.architectRunId }, + data: { source: "architect_run" }, + }); + await supersedeOpenGithubPlanAcceptances( + db, + { + orgId: input.orgId, + projectId: input.projectId, + repoId: input.repoId, + issueNumber: input.issueNumber, + architectRunId: input.architectRunId, + keepProposalId: created.id, + }, + now, + ); + return created; +} diff --git a/services/api/src/github/router.ts b/services/api/src/github/router.ts index 3095d73f..f45b782c 100644 --- a/services/api/src/github/router.ts +++ b/services/api/src/github/router.ts @@ -1,6 +1,5 @@ import { newId } from "@facility/core"; import { - actionTypes, type FacilityDb, insertAuditEvent, proposalEvents, @@ -24,6 +23,7 @@ import { appendRunEvents } from "../sandbox/state.js"; import { findAgentDef, laneFor } from "./agent-routing.js"; import { type FacilityGithubClient, GithubIssueContextTooLargeError } from "./client.js"; import { syncRepoFacilityConfig } from "./kickstart.js"; +import { resolveGithubPlanAcceptance } from "./plan-acceptance.js"; import { renderGithubRunProgress } from "./run-progress.js"; export { findAgentDef, laneFor } from "./agent-routing.js"; @@ -58,18 +58,34 @@ export type GithubIssueCommentContext = { export const ISSUE_CONTEXT_MAX_CHARS = 512 * 1024; const COMMAND_RE = - /(?:^|\n)\s*\/(builder|architect|codex-builder|codex-architect)(?=$|[\s,.:;!?)])/g; + /(?:^|\n)\s*\/(builder|architect|codex-builder|codex-architect)(?:\s+(prop_[0-9a-z_]+))?(?=$|[\s,.:;!?)])/gi; export function resolveSlashCommand(body: string): { command?: string; agentCommand?: string; + proposalId?: string; ambiguous: boolean; } { - const commands = [...body.matchAll(COMMAND_RE)].map((match) => match[1]).filter(Boolean); + const pattern = new RegExp(COMMAND_RE.source, COMMAND_RE.flags); + const matches = [...body.matchAll(pattern)]; + const commands = matches.map((match) => match[1]).filter(Boolean); const unique = [...new Set(commands)]; if (unique.length !== 1) return { ambiguous: unique.length > 1 }; const raw = unique[0] ?? ""; - return { command: raw.replace(/^codex-/, ""), agentCommand: raw, ambiguous: false }; + const proposalIds = matches + .filter((match) => match[1] === raw) + .map((match) => match[2]) + .filter((value): value is string => Boolean(value)); + const uniqueProposalIds = [...new Set(proposalIds)]; + if (uniqueProposalIds.length > 1) return { ambiguous: true }; + const proposalId = + raw.includes("builder") && uniqueProposalIds.length === 1 ? uniqueProposalIds[0] : undefined; + return { + command: raw.replace(/^codex-/, ""), + agentCommand: raw, + proposalId, + ambiguous: false, + }; } export function githubTriggerRequiresClient(payload: TriggerPayload) { @@ -151,15 +167,24 @@ export async function routeTrigger( const request = githubRequestContext(payload, issueComments); assertGithubRequestContextSize(request); const governedBuilder = builderIdentity(command, agent.name); - let accepted = governedBuilder + const acceptance = governedBuilder ? await githubPlanAcceptance(db, { orgId: repo.orgId, projectId: repo.projectId, owner, repo: name, issueNumber, + proposalId: resolved.proposalId, }) : null; + let accepted = + acceptance?.status === "resolved" + ? { + proposal: acceptance.proposal, + architectRun: acceptance.architectRun, + blockedRunId: acceptance.blockedRunId, + } + : null; // `optional` is the backwards-compatible default. Before the policy seam, // only a non-expired open/approved/executing/executed proposal participated // in /builder routing; every other lifecycle state fell through to an @@ -193,6 +218,27 @@ export async function routeTrigger( ...(payload.issue?.node_id ? { issueNodeId: payload.issue.node_id } : {}), }; const githubActor = { type: "user" as const, id: `github:${sender}`, name: sender }; + if (acceptance?.status === "ambiguous") { + await recordBuilderPlanDenial( + db, + { + orgId: repo.orgId, + projectId: repo.projectId, + mode: agent.name, + agentDefId: agent.id, + trigger: { + type: "github_comment", + ambiguousProposalIds: acceptance.liveProposalIds, + }, + gh: runGh, + actor: githubActor, + source: "github_builder_ambiguous", + }, + "builder_plan_ambiguous", + `multiple_live_plans:${acceptance.liveProposalIds.join(",")}`, + ); + return { routed: false, reason: "builder_plan_ambiguous" }; + } let run: typeof runs.$inferSelect | undefined; let approvedByThisInvocation = false; if (governedBuilder && accepted?.proposal) { @@ -646,31 +692,19 @@ async function githubPlanAcceptance( owner: string; repo: string; issueNumber: number; + proposalId?: string; }, ) { - const latest = ( - await db - .select({ proposal: proposals, architectRun: runs }) - .from(proposals) - .innerJoin(actionTypes, eq(actionTypes.id, proposals.actionTypeId)) - .innerJoin(runs, eq(runs.id, proposals.runId)) - .where( - and( - eq(proposals.orgId, input.orgId), - eq(proposals.projectId, input.projectId), - eq(actionTypes.name, "plan_acceptance"), - eq(runs.status, "succeeded"), - sql`${runs.gh} ->> 'owner' = ${input.owner}`, - sql`${runs.gh} ->> 'repo' = ${input.repo}`, - sql`(${runs.gh} ->> 'issueNumber')::int = ${input.issueNumber}`, - ), - ) - .orderBy(desc(proposals.createdAt)) - .limit(1) - )[0]; - if (!latest) return null; - const existing = await loadPlanBuilderRun(db, latest.proposal); - return { ...latest, blockedRunId: existing?.id ?? null }; + const resolved = await resolveGithubPlanAcceptance(db, input); + if (!resolved) return null; + if (resolved.status === "ambiguous") return resolved; + const existing = await loadPlanBuilderRun(db, resolved.proposal); + return { + status: "resolved" as const, + proposal: resolved.proposal, + architectRun: resolved.architectRun, + blockedRunId: existing?.id ?? null, + }; } async function githubProposalDenialCode( @@ -678,6 +712,7 @@ async function githubProposalDenialCode( proposal: typeof proposals.$inferSelect, ): Promise { if (proposal.state === "rejected") return "builder_plan_rejected"; + if (proposal.state === "cancelled") return "builder_plan_superseded"; if (proposal.state === "expired") return "builder_plan_expired"; if (["approved", "executing", "executed"].includes(proposal.state)) { return "builder_plan_already_consumed"; diff --git a/services/api/src/github/run-progress.ts b/services/api/src/github/run-progress.ts index 549bf770..238aacc5 100644 --- a/services/api/src/github/run-progress.ts +++ b/services/api/src/github/run-progress.ts @@ -88,7 +88,7 @@ export function renderGithubRunProgress(input: GithubRunProgressInput) { "", "## Continue from GitHub", "", - `- Approve this plan and start implementation by commenting \`/${builderCommand(input.command, input.mode)}\`.`, + `- Approve this plan and start implementation by commenting \`/${builderCommand(input.command, input.mode)} ${input.proposalId}\`, or \`/${builderCommand(input.command, input.mode)}\` when it is the only open plan on the issue.`, `- Request another planning pass by commenting \`/${architectCommand(input.command, input.mode)} \`.`, `- Proposal audit ID: \`${input.proposalId}\`. The architect cannot approve its own proposal.`, ); @@ -120,6 +120,10 @@ export function renderBuilderPlanDenial(code: string) { "The Architect approval expired. Run `/architect` again to create a fresh plan.", builder_plan_rejected: "The Architect plan was rejected. Run `/architect` again after updating the request.", + builder_plan_superseded: + "A newer Architect plan replaced this one. Review the latest plan comment, then approve with `/builder `.", + builder_plan_ambiguous: + "More than one open Architect plan exists on this issue. Approve the plan you reviewed with `/builder ` (shown in the plan comment).", builder_plan_already_consumed: "That Architect approval has already been consumed. Inspect its Builder run or create a new plan with `/architect`.", builder_plan_stale: diff --git a/services/api/src/sandbox/orchestrator.ts b/services/api/src/sandbox/orchestrator.ts index 167a2c35..b1753c3f 100644 --- a/services/api/src/sandbox/orchestrator.ts +++ b/services/api/src/sandbox/orchestrator.ts @@ -70,6 +70,11 @@ import { } from "../github/client.js"; import { pullRequestBodyForIssue } from "../github/closing-issues.js"; import { githubIssueRevisionSha256 } from "../github/issue-revision.js"; +import { + githubPlanAcceptanceIssueLockKey, + insertGithubPlanAcceptanceReplacingSiblings, + supersedeOpenGithubPlanAcceptances, +} from "../github/plan-acceptance.js"; import { type GithubRunProgressPhase, progressCommentId, @@ -1090,7 +1095,11 @@ async function ensureArchitectPlanAcceptance( ...(issueRevisionSha256 ? { issueRevisionSha256 } : {}), }; await db.execute( - sql`select pg_advisory_xact_lock(hashtextextended(${`architect-plan:${run.id}`}, 0))`, + sql`select pg_advisory_xact_lock(hashtextextended(${githubPlanAcceptanceIssueLockKey({ + orgId: run.orgId, + repoId: repo.id, + issueNumber, + })}, 0))`, ); const candidates = await db .select() @@ -1159,33 +1168,31 @@ async function ensureArchitectPlanAcceptance( }) .onConflictDoNothing(); } + // Same-run retry: keep this proposal and cancel any other live Gate 1 rows. + await supersedeOpenGithubPlanAcceptances(db, { + orgId: run.orgId, + projectId: run.projectId, + repoId: repo.id, + issueNumber, + architectRunId: run.id, + keepProposalId: existing.id, + }); return existing; } - const created = ( - await db - .insert(proposals) - .values({ - id: newId("prop"), - orgId: run.orgId, - projectId: run.projectId, - runId: run.id, - actionTypeId: actionType.id, - payload: canonicalPayload, - contextMd: plan, - expiresAt: new Date(Date.now() + actionType.defaultTtlHours * 3_600_000), - }) - .returning() - )[0]; - if (!created) throw new Error("plan_acceptance_create_failed"); - await db.insert(proposalEvents).values({ + return insertGithubPlanAcceptanceReplacingSiblings(db, { orgId: run.orgId, - proposalId: created.id, - seq: 1, - type: "open", - actor: { type: "agent", id: run.id }, - data: { source: "architect_run" }, + projectId: run.projectId, + repoId: repo.id, + owner: repo.owner, + repo: repo.name, + issueNumber, + architectRunId: run.id, + actionTypeId: actionType.id, + proposalId: newId("prop"), + payload: canonicalPayload, + contextMd: plan, + expiresAt: new Date(Date.now() + actionType.defaultTtlHours * 3_600_000), }); - return created; } export async function reconcileArchitectPlanPublications( diff --git a/services/api/test/builder-plan-policy.integration.test.ts b/services/api/test/builder-plan-policy.integration.test.ts index 8ce6d091..738d95fd 100644 --- a/services/api/test/builder-plan-policy.integration.test.ts +++ b/services/api/test/builder-plan-policy.integration.test.ts @@ -35,6 +35,10 @@ import { import { ApiError } from "../src/errors.js"; import { githubIssueRevisionSha256 } from "../src/github/issue-revision.js"; import { syncRepoFacilityConfig } from "../src/github/kickstart.js"; +import { + insertGithubPlanAcceptanceReplacingSiblings, + supersedeOpenGithubPlanAcceptances, +} from "../src/github/plan-acceptance.js"; import { routeTrigger, type TriggerPayload } from "../src/github/router.js"; import { createGovernedBuilderRetry, @@ -484,6 +488,268 @@ describe("builder plan policy integration", async () => { expect(decisions).toHaveLength(1); }); + it("refuses bare /builder when multiple live plan_acceptance proposals exist on the issue", async () => { + const fixture = await githubRouteFixture("open"); + await db + .update(projects) + .set({ builderPlanPolicy: "required" }) + .where(eq(projects.id, fixture.projectId)); + await db + .update(repos) + .set({ + fingerprint: { files: [] }, + fingerprintStatus: "ok", + fingerprintVerifiedAt: new Date(), + }) + .where(eq(repos.projectId, fixture.projectId)); + await insertSiblingOpenPlanAcceptance(fixture); + const before = await projectRuns(fixture.orgId, fixture.projectId); + const result = await routeTrigger(db, fixture.orgId, fixture.client, fixture.payload); + expect(result).toMatchObject({ routed: false, reason: "builder_plan_ambiguous" }); + expect(await projectRuns(fixture.orgId, fixture.projectId)).toHaveLength(before.length); + await expect(lastDenialCode(fixture.orgId)).resolves.toBe("builder_plan_ambiguous"); + }); + + it("binds GitHub /builder to the explicit proposal id when several live plans exist", async () => { + const fixture = await githubRouteFixture("open"); + await db + .update(projects) + .set({ builderPlanPolicy: "required" }) + .where(eq(projects.id, fixture.projectId)); + await db + .update(repos) + .set({ + fingerprint: { files: [] }, + fingerprintStatus: "ok", + fingerprintVerifiedAt: new Date(), + }) + .where(eq(repos.projectId, fixture.projectId)); + const sibling = await insertSiblingOpenPlanAcceptance(fixture); + const enqueued: Array<{ queue: string; data: Record }> = []; + const result = await routeTrigger( + db, + fixture.orgId, + fixture.client, + { + ...fixture.payload, + comment: { id: 205, body: `/builder ${fixture.proposalId}` }, + }, + async (queue, data) => { + enqueued.push({ queue, data }); + return null; + }, + `delivery_${crypto.randomUUID()}`, + ); + expect(result.routed).toBe(true); + const run = ( + await db + .select() + .from(runs) + .where(eq(runs.id, result.runId ?? "")) + .limit(1) + )[0]; + expect(run?.trigger).toMatchObject({ + source: "plan_acceptance", + proposalId: fixture.proposalId, + architectRunId: fixture.architectRunId, + }); + expect(run?.trigger).not.toMatchObject({ proposalId: sibling.proposalId }); + expect(enqueued).toHaveLength(1); + const siblingProposal = ( + await db.select().from(proposals).where(eq(proposals.id, sibling.proposalId)).limit(1) + )[0]; + expect(siblingProposal?.state).toBe("open"); + }); + + it("supersedes older open proposals when a newer architect plan opens on the same issue", async () => { + const fixture = await githubRouteFixture("open"); + const repo = ( + await db.select().from(repos).where(eq(repos.projectId, fixture.projectId)).limit(1) + )[0]; + if (!repo) throw new Error("supersede repo fixture missing"); + const sibling = await insertSiblingOpenPlanAcceptance(fixture); + const superseded = await db.transaction(async (transaction) => { + const tx = transaction as unknown as FacilityDb; + return supersedeOpenGithubPlanAcceptances(tx, { + orgId: fixture.orgId, + projectId: fixture.projectId, + issueNumber: 204, + repoId: repo.id, + architectRunId: sibling.architectRunId, + keepProposalId: sibling.proposalId, + }); + }); + expect(superseded).toBe(1); + const original = ( + await db.select().from(proposals).where(eq(proposals.id, fixture.proposalId)).limit(1) + )[0]; + expect(original?.state).toBe("cancelled"); + const enqueued: Array<{ queue: string; data: Record }> = []; + const result = await routeTrigger( + db, + fixture.orgId, + fixture.client, + fixture.payload, + async (queue, data) => { + enqueued.push({ queue, data }); + return null; + }, + `delivery_${crypto.randomUUID()}`, + ); + expect(result.routed).toBe(true); + expect(enqueued).toHaveLength(1); + const run = ( + await db + .select() + .from(runs) + .where(eq(runs.id, result.runId ?? "")) + .limit(1) + )[0]; + expect(run?.trigger).toMatchObject({ + source: "plan_acceptance", + proposalId: sibling.proposalId, + }); + }); + + it("keeps the previous live plan open when creating the replacement proposal fails", async () => { + const fixture = await githubRouteFixture("open"); + const repo = ( + await db.select().from(repos).where(eq(repos.projectId, fixture.projectId)).limit(1) + )[0]; + if (!repo) throw new Error("create-failure repo fixture missing"); + const original = ( + await db.select().from(proposals).where(eq(proposals.id, fixture.proposalId)).limit(1) + )[0]; + if (!original) throw new Error("create-failure proposal fixture missing"); + const architectRunId = newId("run"); + await db.insert(runs).values({ + id: architectRunId, + orgId: fixture.orgId, + projectId: fixture.projectId, + mode: "architect", + engine: "codex", + status: "succeeded", + trigger: { type: "github_comment" }, + gh: fixture.dispatch.gh, + createdBy: { type: "user", id: "architect-create-fail" }, + }); + + await expect( + db.transaction(async (transaction) => { + const tx = transaction as unknown as FacilityDb; + await insertGithubPlanAcceptanceReplacingSiblings(tx, { + orgId: fixture.orgId, + projectId: fixture.projectId, + repoId: repo.id, + owner: repo.owner, + repo: repo.name, + issueNumber: 204, + architectRunId, + // Invalid action type forces the insert to fail before supersede. + actionTypeId: "act_missing_for_atomicity_regression", + proposalId: newId("prop"), + payload: { + ...(original.payload as Record), + architectRunId, + }, + contextMd: "This plan must never replace the live Gate 1 proposal.", + expiresAt: new Date(Date.now() + 3_600_000), + }); + }), + ).rejects.toThrow(); + + const stillOpen = ( + await db.select().from(proposals).where(eq(proposals.id, fixture.proposalId)).limit(1) + )[0]; + expect(stillOpen?.state).toBe("open"); + const live = await db + .select({ id: proposals.id, state: proposals.state }) + .from(proposals) + .innerJoin(actionTypes, eq(actionTypes.id, proposals.actionTypeId)) + .where( + and( + eq(proposals.orgId, fixture.orgId), + eq(proposals.projectId, fixture.projectId), + eq(actionTypes.name, "plan_acceptance"), + eq(proposals.state, "open"), + ), + ); + expect(live.map((row) => row.id)).toEqual([fixture.proposalId]); + }); + + it("creates the new plan first, then cancels older live plans in one transaction", async () => { + const fixture = await githubRouteFixture("open"); + const repo = ( + await db.select().from(repos).where(eq(repos.projectId, fixture.projectId)).limit(1) + )[0]; + if (!repo) throw new Error("atomic-create repo fixture missing"); + const original = ( + await db.select().from(proposals).where(eq(proposals.id, fixture.proposalId)).limit(1) + )[0]; + if (!original) throw new Error("atomic-create proposal fixture missing"); + const actionTypeId = original.actionTypeId; + const architectRunId = newId("run"); + const proposalId = newId("prop"); + await db.insert(runs).values({ + id: architectRunId, + orgId: fixture.orgId, + projectId: fixture.projectId, + mode: "architect", + engine: "codex", + status: "succeeded", + trigger: { type: "github_comment" }, + gh: fixture.dispatch.gh, + createdBy: { type: "user", id: "architect-atomic" }, + }); + + const created = await db.transaction(async (transaction) => { + const tx = transaction as unknown as FacilityDb; + return insertGithubPlanAcceptanceReplacingSiblings(tx, { + orgId: fixture.orgId, + projectId: fixture.projectId, + repoId: repo.id, + owner: repo.owner, + repo: repo.name, + issueNumber: 204, + architectRunId, + actionTypeId, + proposalId, + payload: { + ...(original.payload as Record), + architectRunId, + planSha256: createHash("sha256").update("Atomic replacement plan").digest("hex"), + }, + contextMd: "Atomic replacement plan", + expiresAt: new Date(Date.now() + 3_600_000), + }); + }); + + expect(created.id).toBe(proposalId); + expect( + (await db.select().from(proposals).where(eq(proposals.id, fixture.proposalId)).limit(1))[0] + ?.state, + ).toBe("cancelled"); + expect( + (await db.select().from(proposals).where(eq(proposals.id, proposalId)).limit(1))[0]?.state, + ).toBe("open"); + const result = await routeTrigger(db, fixture.orgId, fixture.client, { + ...fixture.payload, + comment: { id: 301, body: "/builder" }, + }); + expect(result).toMatchObject({ routed: true }); + const run = ( + await db + .select() + .from(runs) + .where(eq(runs.id, result.runId ?? "")) + .limit(1) + )[0]; + expect(run?.trigger).toMatchObject({ + source: "plan_acceptance", + proposalId, + }); + }); + it.each([ { name: "default branch", @@ -2224,6 +2490,56 @@ describe("builder plan policy integration", async () => { }; } + async function insertSiblingOpenPlanAcceptance( + fixture: Awaited>, + ) { + const original = ( + await db.select().from(proposals).where(eq(proposals.id, fixture.proposalId)).limit(1) + )[0]; + if (!original) throw new Error("sibling plan fixture missing"); + const architectRunId = newId("run"); + const proposalId = newId("prop"); + await db.insert(runs).values({ + id: architectRunId, + orgId: fixture.orgId, + projectId: fixture.projectId, + mode: "architect", + engine: "codex", + status: "succeeded", + trigger: { type: "github_comment" }, + gh: fixture.dispatch.gh, + createdBy: { type: "user", id: "architect-2" }, + }); + await db.insert(proposals).values({ + ...original, + id: proposalId, + runId: architectRunId, + contextMd: "Second architect plan with a different scope.", + payload: { + ...(original.payload as Record), + architectRunId, + planSha256: createHash("sha256") + .update("Second architect plan with a different scope.") + .digest("hex"), + }, + state: "open", + decidedBy: null, + decidedAt: null, + createdAt: new Date(Date.now() + 1_000), + updatedAt: new Date(Date.now() + 1_000), + expiresAt: new Date(Date.now() + 3_600_000), + }); + await db.insert(proposalEvents).values({ + orgId: fixture.orgId, + proposalId, + seq: 1, + type: "open", + actor: { type: "agent", id: architectRunId }, + data: { source: "architect_run" }, + }); + return { architectRunId, proposalId }; + } + async function projectRuns(orgId: string, projectId: string) { return db .select({ id: runs.id }) diff --git a/services/api/test/builder-plan-policy.test.ts b/services/api/test/builder-plan-policy.test.ts index c7f39992..0fb36feb 100644 --- a/services/api/test/builder-plan-policy.test.ts +++ b/services/api/test/builder-plan-policy.test.ts @@ -74,6 +74,8 @@ describe("builder plan policy", () => { it.each([ "builder_plan_expired", "builder_plan_rejected", + "builder_plan_superseded", + "builder_plan_ambiguous", "builder_plan_already_consumed", "builder_plan_stale", "builder_plan_freshness_unavailable", diff --git a/services/api/test/github-run-progress.test.ts b/services/api/test/github-run-progress.test.ts index 1f4fdd61..2db7e937 100644 --- a/services/api/test/github-run-progress.test.ts +++ b/services/api/test/github-run-progress.test.ts @@ -43,7 +43,8 @@ describe("GitHub run progress", () => { expect(body).toContain("1. Add the behavior."); expect(body).toContain("## Agent progress"); expect(body).toContain("- [x] Inspect the code"); - expect(body).toContain("commenting `/codex-builder`"); + expect(body).toContain("commenting `/codex-builder prop_validation`"); + expect(body).toContain("when it is the only open plan on the issue"); expect(body).toContain("commenting `/codex-architect `"); expect(body).toContain("`prop_validation`"); }); diff --git a/services/api/test/github.test.ts b/services/api/test/github.test.ts index 1a07bade..0cbad0b4 100644 --- a/services/api/test/github.test.ts +++ b/services/api/test/github.test.ts @@ -819,6 +819,24 @@ describe("github integration", async () => { agentCommand: "codex-builder", ambiguous: false, }); + expect(resolveSlashCommand("/builder prop_plan_75389e0c0f8f42e0a2c33ae410a7cb9f")).toEqual({ + command: "builder", + agentCommand: "builder", + proposalId: "prop_plan_75389e0c0f8f42e0a2c33ae410a7cb9f", + ambiguous: false, + }); + expect(resolveSlashCommand("/architect")).toEqual({ + command: "architect", + agentCommand: "architect", + ambiguous: false, + }); + expect(resolveSlashCommand("/builder prop_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")).toEqual({ + command: "builder", + agentCommand: "builder", + proposalId: "prop_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + ambiguous: false, + }); + expect(resolveSlashCommand("/builder prop_a\n/builder prop_b")).toEqual({ ambiguous: true }); }); it("routes only creation actions and denies replay-prone GitHub updates", async () => { diff --git a/services/api/test/plan-acceptance.test.ts b/services/api/test/plan-acceptance.test.ts new file mode 100644 index 00000000..72632d79 --- /dev/null +++ b/services/api/test/plan-acceptance.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from "vitest"; +import { + githubPlanAcceptanceIssueLockKey, + PLAN_ACCEPTANCE_PROPOSAL_ID_RE, +} from "../src/github/plan-acceptance.js"; +import { resolveSlashCommand } from "../src/github/router.js"; + +describe("github plan acceptance command binding", () => { + it("accepts proposal ids in builder slash commands", () => { + expect(PLAN_ACCEPTANCE_PROPOSAL_ID_RE.test("prop_0194abcd0194abcd0194abcd0194abcd")).toBe(true); + expect(resolveSlashCommand("/codex-builder prop_0194abcd0194abcd0194abcd0194abcd: go")).toEqual( + { + command: "builder", + agentCommand: "codex-builder", + proposalId: "prop_0194abcd0194abcd0194abcd0194abcd", + ambiguous: false, + }, + ); + }); + + it("ignores proposal ids on architect commands", () => { + expect(resolveSlashCommand("/architect prop_0194abcd0194abcd0194abcd0194abcd")).toEqual({ + command: "architect", + agentCommand: "architect", + ambiguous: false, + }); + }); + + it("scopes the Gate 1 advisory lock to org, repo, and issue", () => { + expect( + githubPlanAcceptanceIssueLockKey({ + orgId: "org_1", + repoId: "repo_1", + issueNumber: 42, + }), + ).toBe("architect-plan-issue:org_1:repo_1:42"); + }); +});