diff --git a/server/src/__tests__/change-consent-gate.test.ts b/server/src/__tests__/change-consent-gate.test.ts index 17b54031e54..65a173f0dd1 100644 --- a/server/src/__tests__/change-consent-gate.test.ts +++ b/server/src/__tests__/change-consent-gate.test.ts @@ -1,5 +1,6 @@ import { randomUUID } from "node:crypto"; import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { eq } from "drizzle-orm"; import { agents, companies, @@ -133,6 +134,50 @@ describeEmbeddedPostgres("changeConsentGateService", () => { }); it("allows a previous-run accepted interaction with a displayed diff for the bound target", async () => { + const { companyId, coachId, sourceRunId, proposalIssueId, targetKey } = await seedGateFixture(); + const interactionId = randomUUID(); + await db.insert(issueThreadInteractions).values({ + id: interactionId, + companyId, + issueId: proposalIssueId, + kind: "request_confirmation", + status: "accepted", + continuationPolicy: "wake_assignee_on_accept", + sourceRunId, + createdByAgentId: coachId, + payload: { + version: 1, + prompt: "Apply this Reflection Coach skill diff?", + detailsMarkdown: "```diff\n+Tighten the workflow.\n```", + target: { type: "custom", key: targetKey, revisionId: "proposal-v1" }, + }, + result: { version: 1, outcome: "accepted" }, + resolvedByUserId: "board-user", + resolvedAt: new Date(), + }); + const actorRunId = randomUUID(); + + await expect(changeConsentGateService(db).assertConsented({ + companyId, + actorAgentId: coachId, + actorRunId, + targetKeys: [targetKey], + })).resolves.toBe(true); + + const [stored] = await db + .select({ result: issueThreadInteractions.result }) + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, interactionId)); + + expect(stored?.result).toMatchObject({ + consumedByRunId: actorRunId, + outcome: "accepted", + version: 1, + }); + expect((stored?.result as { consumedAt?: unknown } | undefined)?.consumedAt).toEqual(expect.any(String)); + }); + + it("rejects reusing an accepted interaction after it is consumed by a mutation", async () => { const { companyId, coachId, sourceRunId, proposalIssueId, targetKey } = await seedGateFixture(); await db.insert(issueThreadInteractions).values({ id: randomUUID(), @@ -160,6 +205,16 @@ describeEmbeddedPostgres("changeConsentGateService", () => { actorRunId: randomUUID(), targetKeys: [targetKey], })).resolves.toBe(true); + + await expect(changeConsentGateService(db).assertConsented({ + companyId, + actorAgentId: coachId, + actorRunId: randomUUID(), + targetKeys: [targetKey], + })).rejects.toMatchObject({ + status: 403, + details: { code: "reflection_coach_mutation_gate_required" }, + }); }); it("allows legacy Reflection Coach target keys for durable accepted interactions", async () => { diff --git a/server/src/services/change-consent-gate.ts b/server/src/services/change-consent-gate.ts index dc5eab1217d..8523f04bcad 100644 --- a/server/src/services/change-consent-gate.ts +++ b/server/src/services/change-consent-gate.ts @@ -6,6 +6,11 @@ import { forbidden } from "../errors.js"; export const AGENT_PROFILE_CHANGE_CONSENT_FIELDS = ["name", "role", "title", "capabilities"] as const; +type ConsumedRequestConfirmationResult = RequestConfirmationResult & { + consumedAt?: string | null; + consumedByRunId?: string | null; +}; + export function agentInstructionsChangeTargetKey(agentId: string) { return `agent:${agentId}:instructions`; } @@ -47,6 +52,23 @@ function payloadHasDisplayedDiff(payload: RequestConfirmationPayload) { return /(^|\n)[+-][^\n]+/.test(details); } +function requestConfirmationResultConsumed(result: RequestConfirmationResult | null) { + const consumed = result as ConsumedRequestConfirmationResult | null; + return Boolean(readNonEmptyString(consumed?.consumedByRunId) || readNonEmptyString(consumed?.consumedAt)); +} + +function markRequestConfirmationResultConsumed( + result: RequestConfirmationResult, + actorRunId: string, + consumedAt: Date, +): ConsumedRequestConfirmationResult { + return { + ...result, + consumedAt: consumedAt.toISOString(), + consumedByRunId: actorRunId, + }; +} + function legacyTargetKeysFor(targetKey: string) { if (targetKey.startsWith("agent:") && targetKey.endsWith(":instructions")) { const agentId = targetKey.slice("agent:".length, -":instructions".length); @@ -146,6 +168,7 @@ export function changeConsentGateService(db: Db) { return payload.target?.type === "custom" && queryTargetKeys.includes(payload.target.key) && result?.outcome === "accepted" + && !requestConfirmationResultConsumed(result) && payloadHasDisplayedDiff(payload) && Boolean(row.sourceRunId) && row.sourceRunId !== actorRunId; @@ -154,7 +177,48 @@ export function changeConsentGateService(db: Db) { if (!accepted) { throw forbidden( "Reflection Coach mutations require an accepted request_confirmation with a displayed diff for this target, " - + "created in a previous run.", + + "created in a previous run and not already consumed.", + { + code: "reflection_coach_mutation_gate_required", + targetKeys, + }, + ); + } + + const acceptedResult = accepted.result as RequestConfirmationResult | null; + if (!acceptedResult) { + throw forbidden( + "Reflection Coach mutations require an accepted request_confirmation with a displayed diff for this target, " + + "created in a previous run and not already consumed.", + { + code: "reflection_coach_mutation_gate_required", + targetKeys, + }, + ); + } + + const now = new Date(); + const [consumed] = await db + .update(issueThreadInteractions) + .set({ + result: markRequestConfirmationResultConsumed(acceptedResult, actorRunId, now), + updatedAt: now, + }) + .where(and( + eq(issueThreadInteractions.id, accepted.id), + eq(issueThreadInteractions.companyId, input.companyId), + eq(issueThreadInteractions.createdByAgentId, actorAgentId), + eq(issueThreadInteractions.kind, "request_confirmation"), + eq(issueThreadInteractions.status, "accepted"), + sql`${issueThreadInteractions.result}->>'outcome' = 'accepted'`, + sql`coalesce(${issueThreadInteractions.result}->>'consumedByRunId', ${issueThreadInteractions.result}->>'consumedAt') is null`, + )) + .returning({ id: issueThreadInteractions.id }); + + if (!consumed) { + throw forbidden( + "Reflection Coach mutations require an accepted request_confirmation with a displayed diff for this target, " + + "created in a previous run and not already consumed.", { code: "reflection_coach_mutation_gate_required", targetKeys,