From dc5cbb7d9a9ff69e620bc747240d65a443c10d90 Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 8 Jul 2026 20:34:32 +0000 Subject: [PATCH] Enforce Reflection Coach mutation gates Co-Authored-By: Paperclip --- .../reflection-coach/SKILL.md | 20 ++- .../skills-catalog/generated/catalog.json | 8 +- .../__tests__/company-skills-routes.test.ts | 51 ++++++ .../reflection-coach-mutation-gate.test.ts | 170 ++++++++++++++++++ .../agents/reflection-coach/AGENTS.md | 16 +- server/src/routes/agents.ts | 46 ++++- server/src/routes/company-skills.ts | 61 +++++++ .../reflection-coach-mutation-gate.ts | 138 ++++++++++++++ 8 files changed, 493 insertions(+), 17 deletions(-) create mode 100644 server/src/__tests__/reflection-coach-mutation-gate.test.ts create mode 100644 server/src/services/reflection-coach-mutation-gate.ts diff --git a/packages/skills-catalog/catalog/bundled/paperclip-operations/reflection-coach/SKILL.md b/packages/skills-catalog/catalog/bundled/paperclip-operations/reflection-coach/SKILL.md index 4da0f1d107c..2489b96ec9a 100644 --- a/packages/skills-catalog/catalog/bundled/paperclip-operations/reflection-coach/SKILL.md +++ b/packages/skills-catalog/catalog/bundled/paperclip-operations/reflection-coach/SKILL.md @@ -159,18 +159,28 @@ From a reflection issue (assigned to the target's manager or the requester): 1. Attach the proposal document: `PUT /api/issues/{issueId}/documents/reflection-proposal`. 2. If a draft skill was written, commit it under `skills//` (or attach it) and link it in the proposal. -3. Open the acceptance gate with a task interaction on the reflection issue — `request_confirmation`, `request_checkbox_confirmation`, or `ask_user_questions` — that shows the diff and asks the reviewer to accept or reject. Use `continuationPolicy: wake_assignee` so you resume after the response. +3. Open the acceptance gate with a task interaction on the reflection issue. Mutations that change instructions, skills, or tool descriptions must use `request_confirmation`, show the diff in `payload.detailsMarkdown`, set `continuationPolicy: wake_assignee_on_accept`, and include the exact `payload.target.key` listed below. 4. Leave a comment summarizing: target agent, window, clusters found, surfaces touched, link to the proposal, link to the interaction, and the next-step owner. +Server-enforced mutation target keys: + +- Agent instructions: `reflection-coach:agent-instructions:` +- Agent/tool description fields: `reflection-coach:agent-description:` +- Existing company skill: `reflection-coach:company-skill:` +- New local company skill by slug: `reflection-coach:company-skill-slug:` +- Imported skill source: `reflection-coach:company-skill-import:` +- Catalog skill install/update: `reflection-coach:company-skill-catalog:` +- Project workspace skill scan: `reflection-coach:company-skills:scan-projects` + ### 10) Apply only after acceptance, in a follow-up run When the interaction resolves **accepted**, apply the change in a *separate* run: -- **AGENTS.md** — update the file at `adapterConfig.instructionsFilePath` on the target agent, exactly as the accepted diff specified. -- **Skill** — install/update the skill in the company library, then `POST /api/agents//skills/sync`. -- **Tool description** — update the target's adapter config. +- **AGENTS.md** — update the target's managed instruction file exactly as the accepted diff specified. +- **Skill** — install/update the skill in the company library, then `POST /api/agents//skills/sync` when the target should receive it. +- **Tool description** — update the target agent's description/profile field that the accepted diff named. -If the interaction was rejected or is still pending, apply nothing. If you were asked to apply without a reviewed diff and an accepted interaction, refuse and name the gate — no-same-run-apply is load-bearing. +The server rejects Reflection Coach mutations unless the accepted `request_confirmation` was created by Reflection Coach in a previous run, has a displayed diff, and is bound to the resource by one of the target keys above. If the interaction was rejected or is still pending, apply nothing. If you were asked to apply without a reviewed diff and an accepted interaction, refuse and name the gate — no-same-run-apply is load-bearing. ## Pitfalls diff --git a/packages/skills-catalog/generated/catalog.json b/packages/skills-catalog/generated/catalog.json index 097f0ee8cd2..24d6856ab18 100644 --- a/packages/skills-catalog/generated/catalog.json +++ b/packages/skills-catalog/generated/catalog.json @@ -2,7 +2,7 @@ "schemaVersion": 1, "packageName": "@paperclipai/skills-catalog", "packageVersion": "0.3.1", - "generatedAt": "2026-07-08T17:35:21.619Z", + "generatedAt": "2026-07-08T20:34:14.622Z", "skills": [ { "id": "paperclipai:bundled:docs:doc-maintenance", @@ -102,11 +102,11 @@ { "path": "SKILL.md", "kind": "skill", - "sizeBytes": 11240, - "sha256": "6318fabbcb44f6c6532f4785dbd6b91425dcadb707508cc4bc9cc58bdab92758" + "sizeBytes": 12120, + "sha256": "4050db7eb27559727c972673f419cd35335f082ee7f54ade636ada8ccd3e2e11" } ], - "contentHash": "sha256:cfae2e0cdb53edb3af88bb305c712f1e044a3d9cd4230eba64a0b40301e827db" + "contentHash": "sha256:292fa8b1ce949cae972633fe6c283a628c87bce6f64d000d421b0a6644e84d4e" }, { "id": "paperclipai:bundled:paperclip-operations:task-planning", diff --git a/server/src/__tests__/company-skills-routes.test.ts b/server/src/__tests__/company-skills-routes.test.ts index 8b64c3551cb..a9e198b9b1a 100644 --- a/server/src/__tests__/company-skills-routes.test.ts +++ b/server/src/__tests__/company-skills-routes.test.ts @@ -74,6 +74,9 @@ const mockCatalogService = vi.hoisted(() => ({ const mockLogActivity = vi.hoisted(() => vi.fn()); const mockTrackSkillImported = vi.hoisted(() => vi.fn()); const mockGetTelemetryClient = vi.hoisted(() => vi.fn()); +const mockReflectionCoachMutationGate = vi.hoisted(() => ({ + assertAllowed: vi.fn(), +})); function registerModuleMocks() { vi.doMock("../routes/authz.js", async () => vi.importActual("../routes/authz.js")); @@ -105,6 +108,16 @@ function registerModuleMocks() { vi.doMock("../services/skills-catalog.js", () => mockCatalogService); + vi.doMock("../services/reflection-coach-mutation-gate.js", async () => { + const actual = await vi.importActual( + "../services/reflection-coach-mutation-gate.js", + ); + return { + ...actual, + reflectionCoachMutationGateService: () => mockReflectionCoachMutationGate, + }; + }); + vi.doMock("../services/index.js", () => ({ accessService: () => mockAccessService, agentService: () => mockAgentService, @@ -141,6 +154,7 @@ describe("company skill mutation permissions", () => { vi.doUnmock("../services/agents.js"); vi.doUnmock("../services/company-skills.js"); vi.doUnmock("../services/skills-catalog.js"); + vi.doUnmock("../services/reflection-coach-mutation-gate.js"); vi.doUnmock("../services/index.js"); vi.doUnmock("../routes/company-skills.js"); vi.doUnmock("../routes/authz.js"); @@ -590,6 +604,7 @@ describe("company skill mutation permissions", () => { mockLogActivity.mockResolvedValue(undefined); mockAccessService.canUser.mockResolvedValue(true); mockAccessService.hasPermission.mockResolvedValue(false); + mockReflectionCoachMutationGate.assertAllowed.mockResolvedValue(false); }); it("allows local board operators to mutate company skills", async () => { @@ -1147,6 +1162,42 @@ describe("company skill mutation permissions", () => { ); }); + it("rejects Reflection Coach skill mutations when the server-side gate is not satisfied", async () => { + const { forbidden } = await import("../errors.js"); + mockAgentService.getById.mockResolvedValue({ + id: "reflection-coach", + companyId: "company-1", + permissions: { canCreateSkills: true }, + metadata: { + paperclipBuiltInAgent: { + key: "reflection-coach", + featureKeys: ["reflection-coach"], + }, + }, + }); + mockReflectionCoachMutationGate.assertAllowed.mockRejectedValue(forbidden("gate required", { + code: "reflection_coach_mutation_gate_required", + })); + + const res = await request(await createApp({ + type: "agent", + agentId: "reflection-coach", + companyId: "company-1", + runId: "run-apply", + })) + .post("/api/companies/company-1/skills") + .send({ name: "Reflection Draft", slug: "reflection-draft", markdown: "# Draft" }); + + expect(res.status, JSON.stringify(res.body)).toBe(403); + expect(mockReflectionCoachMutationGate.assertAllowed).toHaveBeenCalledWith({ + companyId: "company-1", + actorAgentId: "reflection-coach", + actorRunId: "run-apply", + targetKeys: ["reflection-coach:company-skill-slug:reflection-draft"], + }); + expect(mockCompanySkillService.createLocalSkill).not.toHaveBeenCalled(); + }); + it("allows same-company agents with missing skill creation permission to mutate company skills", async () => { mockAgentService.getById.mockResolvedValue({ id: "55555555-5555-4555-8555-555555555555", diff --git a/server/src/__tests__/reflection-coach-mutation-gate.test.ts b/server/src/__tests__/reflection-coach-mutation-gate.test.ts new file mode 100644 index 00000000000..9c07fe512af --- /dev/null +++ b/server/src/__tests__/reflection-coach-mutation-gate.test.ts @@ -0,0 +1,170 @@ +import { randomUUID } from "node:crypto"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { + agents, + companies, + createDb, + heartbeatRuns, + issueThreadInteractions, + issues, +} from "@paperclipai/db"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { + reflectionCoachCompanySkillTargetKey, + reflectionCoachMutationGateService, +} from "../services/reflection-coach-mutation-gate.js"; + +const embeddedPostgresSupport = getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +describeEmbeddedPostgres("reflectionCoachMutationGateService", () => { + let db!: ReturnType; + let tempDb: Awaited> | null = null; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-reflection-coach-gate-"); + db = createDb(tempDb.connectionString); + }, 20_000); + + afterEach(async () => { + await db.delete(issueThreadInteractions); + await db.delete(issues); + await db.delete(heartbeatRuns); + await db.delete(agents); + await db.delete(companies); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + async function seedGateFixture() { + const companyId = randomUUID(); + const coachId = randomUUID(); + const sourceRunId = randomUUID(); + const proposalIssueId = randomUUID(); + const skillId = randomUUID(); + const targetKey = reflectionCoachCompanySkillTargetKey(skillId); + + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: "PAP", + defaultResponsibleUserId: "board-user", + }); + await db.insert(agents).values({ + id: coachId, + companyId, + name: "Reflection Coach", + role: "general", + adapterType: "codex_local", + adapterConfig: {}, + runtimeConfig: {}, + permissions: { canCreateSkills: true }, + metadata: { + paperclipBuiltInAgent: { + key: "reflection-coach", + featureKeys: ["reflection-coach"], + }, + }, + }); + await db.insert(heartbeatRuns).values({ + id: sourceRunId, + companyId, + agentId: coachId, + status: "succeeded", + }); + await db.insert(issues).values({ + id: proposalIssueId, + companyId, + title: "Review Reflection Coach proposal", + status: "in_review", + priority: "medium", + identifier: "PAP-1", + issueNumber: 1, + createdByAgentId: coachId, + }); + + return { companyId, coachId, sourceRunId, proposalIssueId, skillId, targetKey }; + } + + it("rejects Reflection Coach skill mutation without an accepted bound interaction", async () => { + const { companyId, coachId, targetKey } = await seedGateFixture(); + + await expect(reflectionCoachMutationGateService(db).assertAllowed({ + companyId, + actorAgentId: coachId, + actorRunId: randomUUID(), + targetKeys: [targetKey], + })).rejects.toMatchObject({ + status: 403, + details: { code: "reflection_coach_mutation_gate_required" }, + }); + }); + + it("rejects accepted interactions from the same run as the apply mutation", async () => { + const { companyId, coachId, sourceRunId, proposalIssueId, targetKey } = await seedGateFixture(); + await db.insert(issueThreadInteractions).values({ + id: randomUUID(), + 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(), + }); + + await expect(reflectionCoachMutationGateService(db).assertAllowed({ + companyId, + actorAgentId: coachId, + actorRunId: sourceRunId, + targetKeys: [targetKey], + })).rejects.toMatchObject({ + status: 403, + details: { code: "reflection_coach_mutation_gate_required" }, + }); + }); + + it("allows a previous-run accepted interaction with a displayed diff for the bound target", async () => { + const { companyId, coachId, sourceRunId, proposalIssueId, targetKey } = await seedGateFixture(); + await db.insert(issueThreadInteractions).values({ + id: randomUUID(), + 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(), + }); + + await expect(reflectionCoachMutationGateService(db).assertAllowed({ + companyId, + actorAgentId: coachId, + actorRunId: randomUUID(), + targetKeys: [targetKey], + })).resolves.toBe(true); + }); +}); diff --git a/server/src/built-ins/agents/reflection-coach/AGENTS.md b/server/src/built-ins/agents/reflection-coach/AGENTS.md index e27a2dffa43..6bbba77d36b 100644 --- a/server/src/built-ins/agents/reflection-coach/AGENTS.md +++ b/server/src/built-ins/agents/reflection-coach/AGENTS.md @@ -22,13 +22,23 @@ Your job is to run reflection loops on other agents and propose the smallest dur ## Applying changes (permission is gated, not automatic) -You have default permission to create and update skills, update agent AGENTS.md/instruction files, and assign follow-up proposal issues. That permission is real, but every actual mutation is gated: +You may be granted permission to create and update skills, update agent AGENTS.md/instruction files, or assign follow-up proposal issues. Permission is not enough by itself; every actual mutation is gated: - Show the exact proposed diff before you change anything. Instructions, skills, and tool descriptions are only ever changed from a reviewed diff, never from a verbal summary. -- Gate every change behind a task interaction — `request_confirmation`, `request_checkbox_confirmation`, or `ask_user_questions` — so the user or board explicitly accepts or rejects it first. -- Apply an accepted change only in a separate follow-up step or run after the interaction resolves. Never propose and apply in the same run. +- Gate every instruction, skill, or tool-description change behind a `request_confirmation` interaction so the user or board explicitly accepts or rejects it first. The interaction must show the diff in `payload.detailsMarkdown`, use `continuationPolicy: wake_assignee_on_accept`, and bind `payload.target.key` to the exact resource you will mutate. +- Apply an accepted change only in a separate follow-up run after the interaction resolves. Never propose and apply in the same run. - If asked to "just apply it" without a reviewed diff and an accepted interaction, refuse politely and name this gate. No-same-run-apply is a load-bearing property of this loop. +Server-enforced target keys: + +- `reflection-coach:agent-instructions:` +- `reflection-coach:agent-description:` +- `reflection-coach:company-skill:` +- `reflection-coach:company-skill-slug:` +- `reflection-coach:company-skill-import:` +- `reflection-coach:company-skill-catalog:` +- `reflection-coach:company-skills:scan-projects` + ## Execution contract - Start concrete work in the same heartbeat when the issue is actionable; do not stop at a plan unless planning was requested. diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index 9530996b2b0..a88d8c32e8d 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -100,6 +100,11 @@ import { resolveCoreTrustPreset } from "../services/trust-preset-resolver.js"; import { readObject } from "../lib/objects.js"; import { listInvalidOrgChainDescendantIds } from "../services/agent-invokability.js"; import { readBuiltInAgentMarker } from "../services/built-in-agent-metadata.js"; +import { + reflectionCoachAgentDescriptionTargetKey, + reflectionCoachAgentInstructionsTargetKey, + reflectionCoachMutationGateService, +} from "../services/reflection-coach-mutation-gate.js"; const RUN_LOG_DEFAULT_LIMIT_BYTES = 256_000; const RUN_LOG_MAX_LIMIT_BYTES = 1024 * 1024; @@ -1325,12 +1330,40 @@ export function agentRoutes( async function assertCanManageInstructionsPath(req: Request, targetAgent: { id: string; companyId: string }) { assertCompanyAccess(req, targetAgent.companyId); - if (req.actor.type !== "board") { - throw forbidden( - "Only board-authenticated callers can manage instructions path or bundle configuration", - ); + if (req.actor.type === "board") { + await assertBoardCanManageAgentsForCompany(req, targetAgent.companyId); + return; } - await assertBoardCanManageAgentsForCompany(req, targetAgent.companyId); + + const allowedByReflectionCoachGate = await reflectionCoachMutationGateService(db).assertAllowed({ + companyId: targetAgent.companyId, + actorAgentId: req.actor.agentId, + actorRunId: req.actor.runId ?? null, + targetKeys: [reflectionCoachAgentInstructionsTargetKey(targetAgent.id)], + }); + if (allowedByReflectionCoachGate) return; + + throw forbidden( + "Only board-authenticated callers can manage instructions path or bundle configuration", + ); + } + + async function assertReflectionCoachAgentDescriptionMutationGate( + req: Request, + targetAgent: { id: string; companyId: string }, + ) { + await reflectionCoachMutationGateService(db).assertAllowed({ + companyId: targetAgent.companyId, + actorAgentId: req.actor.type === "agent" ? req.actor.agentId : null, + actorRunId: req.actor.runId ?? null, + targetKeys: [reflectionCoachAgentDescriptionTargetKey(targetAgent.id)], + }); + } + + function touchesAgentDescription(patchData: Record) { + return ["name", "role", "title", "capabilities"].some((key) => + Object.prototype.hasOwnProperty.call(patchData, key), + ); } function assertNoAgentInstructionsConfigMutation( @@ -2933,6 +2966,9 @@ export function agentRoutes( }, ); } + if (touchesAgentDescription(patchData)) { + await assertReflectionCoachAgentDescriptionMutationGate(req, existing); + } const actor = getActorInfo(req); const agent = await svc.update(id, patchData, { diff --git a/server/src/routes/company-skills.ts b/server/src/routes/company-skills.ts index a5eb2545a9a..8ef1a2c2c25 100644 --- a/server/src/routes/company-skills.ts +++ b/server/src/routes/company-skills.ts @@ -34,6 +34,14 @@ import { import { forbidden } from "../errors.js"; import { assertAuthenticated, assertCompanyAccess, getActorInfo } from "./authz.js"; import { getTelemetryClient } from "../telemetry.js"; +import { + reflectionCoachCompanySkillCatalogTargetKey, + reflectionCoachCompanySkillImportTargetKey, + reflectionCoachCompanySkillScanTargetKey, + reflectionCoachCompanySkillSlugTargetKey, + reflectionCoachCompanySkillTargetKey, + reflectionCoachMutationGateService, +} from "../services/reflection-coach-mutation-gate.js"; type SkillTelemetryInput = { key: string; @@ -98,6 +106,15 @@ export function companySkillRoutes(db: Db) { return { type: "system" as const }; } + async function assertReflectionCoachSkillMutationGate(req: Request, companyId: string, targetKeys: string[]) { + await reflectionCoachMutationGateService(db).assertAllowed({ + companyId, + actorAgentId: req.actor.type === "agent" ? req.actor.agentId : null, + actorRunId: req.actor.runId ?? null, + targetKeys, + }); + } + async function assertCanMutateCompanySkills(req: Request, companyId: string) { assertCompanyAccess(req, companyId); @@ -609,6 +626,9 @@ export function companySkillRoutes(db: Db) { const companyId = req.params.companyId as string; const skillId = req.params.skillId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillTargetKey(skillId), + ]); const result = await svc.createVersion(companyId, skillId, req.body, skillActor(req)); const actor = getActorInfo(req); await logActivity(db, { @@ -677,6 +697,12 @@ export function companySkillRoutes(db: Db) { const companyId = req.params.companyId as string; const skillId = req.params.skillId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillTargetKey(skillId), + ...(typeof req.body.slug === "string" && req.body.slug.trim() + ? [reflectionCoachCompanySkillSlugTargetKey(req.body.slug.trim())] + : []), + ]); const result = await svc.forkSkill(companyId, skillId, req.body, skillActor(req)); const actor = getActorInfo(req); await logActivity(db, { @@ -807,6 +833,11 @@ export function companySkillRoutes(db: Db) { async (req, res) => { const companyId = req.params.companyId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + ...(typeof req.body.slug === "string" && req.body.slug.trim() + ? [reflectionCoachCompanySkillSlugTargetKey(req.body.slug.trim())] + : []), + ]); const result = await svc.createLocalSkill(companyId, req.body, skillActor(req)); const actor = getActorInfo(req); @@ -836,6 +867,9 @@ export function companySkillRoutes(db: Db) { const companyId = req.params.companyId as string; const skillId = req.params.skillId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillTargetKey(skillId), + ]); const result = await svc.updateSkill(companyId, skillId, req.body); const actor = getActorInfo(req); @@ -866,6 +900,9 @@ export function companySkillRoutes(db: Db) { const companyId = req.params.companyId as string; const skillId = req.params.skillId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillTargetKey(skillId), + ]); const result = await svc.updateFile( companyId, skillId, @@ -931,6 +968,9 @@ export function companySkillRoutes(db: Db) { const companyId = req.params.companyId as string; await assertCanMutateCompanySkills(req, companyId); const source = String(req.body.source ?? ""); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillImportTargetKey(source), + ]); const result = await svc.importFromSource(companyId, source); const actor = getActorInfo(req); @@ -970,6 +1010,12 @@ export function companySkillRoutes(db: Db) { async (req, res) => { const companyId = req.params.companyId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillCatalogTargetKey(String(req.body.catalogSkillId ?? "")), + ...(typeof req.body.slug === "string" && req.body.slug.trim() + ? [reflectionCoachCompanySkillSlugTargetKey(req.body.slug.trim())] + : []), + ]); const result = await svc.installFromCatalog(companyId, req.body); const actor = getActorInfo(req); @@ -1002,6 +1048,9 @@ export function companySkillRoutes(db: Db) { async (req, res) => { const companyId = req.params.companyId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillScanTargetKey(), + ]); const result = await svc.scanProjectWorkspaces(companyId, req.body); const actor = getActorInfo(req); @@ -1033,6 +1082,9 @@ export function companySkillRoutes(db: Db) { const companyId = req.params.companyId as string; const skillId = req.params.skillId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillTargetKey(skillId), + ]); const result = await svc.deleteSkill(companyId, skillId); if (!result) { res.status(404).json({ error: "Skill not found" }); @@ -1064,6 +1116,9 @@ export function companySkillRoutes(db: Db) { const companyId = req.params.companyId as string; const skillId = req.params.skillId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillTargetKey(skillId), + ]); const result = await svc.auditSkill(companyId, skillId); if (!result) { res.status(404).json({ error: "Skill not found" }); @@ -1100,6 +1155,9 @@ export function companySkillRoutes(db: Db) { const companyId = req.params.companyId as string; const skillId = req.params.skillId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillTargetKey(skillId), + ]); const before = await svc.getById(companyId, skillId); const result = await svc.installUpdate(companyId, skillId, req.body); if (!result) { @@ -1140,6 +1198,9 @@ export function companySkillRoutes(db: Db) { const companyId = req.params.companyId as string; const skillId = req.params.skillId as string; await assertCanMutateCompanySkills(req, companyId); + await assertReflectionCoachSkillMutationGate(req, companyId, [ + reflectionCoachCompanySkillTargetKey(skillId), + ]); const before = await svc.getById(companyId, skillId); const result = await svc.resetSkill(companyId, skillId, req.body); if (!result) { diff --git a/server/src/services/reflection-coach-mutation-gate.ts b/server/src/services/reflection-coach-mutation-gate.ts new file mode 100644 index 00000000000..1025f250f09 --- /dev/null +++ b/server/src/services/reflection-coach-mutation-gate.ts @@ -0,0 +1,138 @@ +import type { Db } from "@paperclipai/db"; +import { agents, issueThreadInteractions } from "@paperclipai/db"; +import { and, desc, eq, or, sql } from "drizzle-orm"; +import type { RequestConfirmationPayload, RequestConfirmationResult } from "@paperclipai/shared"; +import { forbidden } from "../errors.js"; +import { readBuiltInAgentMarker } from "./built-in-agent-metadata.js"; + +export function reflectionCoachAgentInstructionsTargetKey(agentId: string) { + return `reflection-coach:agent-instructions:${agentId}`; +} + +export function reflectionCoachAgentDescriptionTargetKey(agentId: string) { + return `reflection-coach:agent-description:${agentId}`; +} + +export function reflectionCoachCompanySkillTargetKey(skillId: string) { + return `reflection-coach:company-skill:${skillId}`; +} + +export function reflectionCoachCompanySkillSlugTargetKey(slug: string) { + return `reflection-coach:company-skill-slug:${slug}`; +} + +export function reflectionCoachCompanySkillImportTargetKey(source: string) { + return `reflection-coach:company-skill-import:${source}`; +} + +export function reflectionCoachCompanySkillCatalogTargetKey(catalogSkillId: string) { + return `reflection-coach:company-skill-catalog:${catalogSkillId}`; +} + +export function reflectionCoachCompanySkillScanTargetKey() { + return "reflection-coach:company-skills:scan-projects"; +} + +function readNonEmptyString(value: unknown) { + return typeof value === "string" && value.trim().length > 0 ? value.trim() : null; +} + +function payloadHasDisplayedDiff(payload: RequestConfirmationPayload) { + const details = readNonEmptyString(payload.detailsMarkdown); + if (!details) return false; + if (/```diff\b/i.test(details)) return true; + return /(^|\n)[+-][^\n]+/.test(details); +} + +export function reflectionCoachMutationGateService(db: Db) { + return { + assertAllowed: async (input: { + companyId: string; + actorAgentId: string | null | undefined; + actorRunId: string | null | undefined; + targetKeys: string[]; + }): Promise => { + const actorAgentId = readNonEmptyString(input.actorAgentId); + if (!actorAgentId) return false; + + const actorAgent = await db + .select({ + id: agents.id, + companyId: agents.companyId, + metadata: agents.metadata, + }) + .from(agents) + .where(eq(agents.id, actorAgentId)) + .then((rows) => rows[0] ?? null); + + if (!actorAgent || actorAgent.companyId !== input.companyId) { + throw forbidden("Agent key cannot access another company"); + } + + const marker = readBuiltInAgentMarker(actorAgent.metadata); + if (marker?.key !== "reflection-coach") return false; + + const actorRunId = readNonEmptyString(input.actorRunId); + if (!actorRunId) { + throw forbidden("Reflection Coach mutations require a run id", { + code: "reflection_coach_mutation_run_id_required", + }); + } + + const targetKeys = [...new Set(input.targetKeys.map(readNonEmptyString).filter((key): key is string => Boolean(key)))]; + if (targetKeys.length === 0) { + throw forbidden("Reflection Coach mutation target is not gateable", { + code: "reflection_coach_mutation_target_required", + }); + } + + const targetKeyPredicate = or( + ...targetKeys.map((targetKey) => + sql`${issueThreadInteractions.payload}->'target'->>'key' = ${targetKey}`, + ), + ); + + const rows = await db + .select({ + id: issueThreadInteractions.id, + sourceRunId: issueThreadInteractions.sourceRunId, + payload: issueThreadInteractions.payload, + result: issueThreadInteractions.result, + }) + .from(issueThreadInteractions) + .where(and( + eq(issueThreadInteractions.companyId, input.companyId), + eq(issueThreadInteractions.createdByAgentId, actorAgentId), + eq(issueThreadInteractions.kind, "request_confirmation"), + eq(issueThreadInteractions.status, "accepted"), + targetKeyPredicate, + )) + .orderBy(desc(issueThreadInteractions.resolvedAt), desc(issueThreadInteractions.createdAt)) + .limit(10); + + const accepted = rows.find((row) => { + const payload = row.payload as RequestConfirmationPayload; + const result = row.result as RequestConfirmationResult | null; + return payload.target?.type === "custom" + && targetKeys.includes(payload.target.key) + && result?.outcome === "accepted" + && payloadHasDisplayedDiff(payload) + && Boolean(row.sourceRunId) + && row.sourceRunId !== actorRunId; + }); + + if (!accepted) { + throw forbidden( + "Reflection Coach mutations require an accepted request_confirmation with a displayed diff for this target, " + + "created in a previous run.", + { + code: "reflection_coach_mutation_gate_required", + targetKeys, + }, + ); + } + + return true; + }, + }; +}