From 77eec1915a50f09e78efaa7bb582b31f5a7f3004 Mon Sep 17 00:00:00 2001 From: Oliver Gibbs Date: Wed, 16 Sep 2026 09:14:56 +0000 Subject: [PATCH 1/3] fix(fabricator): require the caller organisation on every fabrication request Fabrication requests from the GraphQL resolver and from the intake agent now carry the organisation resolved server-side, and neither producer enqueues when it cannot be resolved. The Fabricator refuses queue messages that lack an organisation instead of creating untenanted agents or tools, so producer and consumer enforcement land together. Adds a dispatch-gate-enumeration guard for the request resolver. Any authenticated member of an organisation may still request fabrication; there is intentionally no platform-role gate. --- .../test_deadline_guard_process_event.py | 4 + .../__tests__/test_design_assessment_gate.py | 4 + .../__tests__/test_fabrication_status.py | 4 + .../__tests__/test_intake_progress_scaling.py | 3 + .../test_process_event_org_tenancy_gate.py | 162 ++++++++++++++ arbiter/fabricator/index.py | 22 +- ...resolver-dispatch-gate-enumeration.test.ts | 199 ++++++++++++++++++ ...or-request-resolver-source-project.test.ts | 157 +++++++------- .../fabricator-request-resolver.test.ts | 158 ++++++++++---- .../src/lambda/fabricator-request-resolver.ts | 60 ++++-- .../tests/test_fabricate_org_tenancy.py | 108 ++++++++++ .../agent_intake_single/tools/fabricate.py | 20 ++ 12 files changed, 770 insertions(+), 131 deletions(-) create mode 100644 arbiter/fabricator/__tests__/test_process_event_org_tenancy_gate.py create mode 100644 backend/src/lambda/__tests__/fabricator-request-resolver-dispatch-gate-enumeration.test.ts create mode 100644 service/agent_intake_single/tests/test_fabricate_org_tenancy.py diff --git a/arbiter/fabricator/__tests__/test_deadline_guard_process_event.py b/arbiter/fabricator/__tests__/test_deadline_guard_process_event.py index 95142d08..4a57d922 100644 --- a/arbiter/fabricator/__tests__/test_deadline_guard_process_event.py +++ b/arbiter/fabricator/__tests__/test_deadline_guard_process_event.py @@ -63,6 +63,10 @@ def _base_event(): "agent_input": {"taskDetails": "Create an agent that does things"}, "agent_index": 0, "total_agents": 1, + # Tenancy fail-closed (design evidence, section C): process_event now + # refuses org-less messages outright, so every fixture must carry a + # non-empty org_id to reach the deadline-guard path this file tests. + "org_id": "org-test", } diff --git a/arbiter/fabricator/__tests__/test_design_assessment_gate.py b/arbiter/fabricator/__tests__/test_design_assessment_gate.py index 58032309..00bb1929 100644 --- a/arbiter/fabricator/__tests__/test_design_assessment_gate.py +++ b/arbiter/fabricator/__tests__/test_design_assessment_gate.py @@ -320,6 +320,10 @@ def _base_event(**agent_input_overrides: Any) -> dict[str, Any]: "agent_use_id": "use-1", "node": "fabricator", "agent_input": agent_input, + # Tenancy fail-closed (design evidence, section C): process_event + # now refuses org-less messages before it ever reaches the + # projectId-fallback / design-assessment gate this class tests. + "org_id": "org-test", } # ------------------------------------------------------------------ diff --git a/arbiter/fabricator/__tests__/test_fabrication_status.py b/arbiter/fabricator/__tests__/test_fabrication_status.py index 971903c6..bb48f0ab 100644 --- a/arbiter/fabricator/__tests__/test_fabrication_status.py +++ b/arbiter/fabricator/__tests__/test_fabrication_status.py @@ -39,6 +39,10 @@ def _base_event(): "agent_input": {"taskDetails": "Create an agent that does things"}, "agent_index": 0, "total_agents": 1, + # Tenancy fail-closed (design evidence, section C): process_event now + # refuses org-less messages outright, so every fixture must carry a + # non-empty org_id to reach the fabrication path this file tests. + "org_id": "org-test", } diff --git a/arbiter/fabricator/__tests__/test_intake_progress_scaling.py b/arbiter/fabricator/__tests__/test_intake_progress_scaling.py index 4c58489c..5b14f3df 100644 --- a/arbiter/fabricator/__tests__/test_intake_progress_scaling.py +++ b/arbiter/fabricator/__tests__/test_intake_progress_scaling.py @@ -103,6 +103,9 @@ def record_progress(orchestration_id, agent_index, total_agents, "agent_input": {"taskDetails": "Create an agent"}, "agent_index": 0, "total_agents": 1, + # Tenancy fail-closed (design evidence, section C): process_event now + # refuses org-less messages outright. + "org_id": "org-test", } with patch.object(index, "_write_fabrication_status"), \ patch.object(index, "check_design_assessment"), \ diff --git a/arbiter/fabricator/__tests__/test_process_event_org_tenancy_gate.py b/arbiter/fabricator/__tests__/test_process_event_org_tenancy_gate.py new file mode 100644 index 00000000..66c55f08 --- /dev/null +++ b/arbiter/fabricator/__tests__/test_process_event_org_tenancy_gate.py @@ -0,0 +1,162 @@ +""" +Tests for the org_id fail-closed guard on arbiter/fabricator/index.py's +process_event (design evidence, section C). + +The TS resolver (fabricator-request-resolver.ts) now derives org via +requireOrgId (fail-closed) and always stamps a non-empty org_id onto the +SQS message body. This consumer must mirror that discipline: refuse to +process (no registry record, no status row, no design-assessment gate +call, error-level log, no raise -- safe no-op so the poison message does +not redeliver forever) when org_id is missing/empty on an +agent-creation/tool-creation message, and process normally when it is +present. + +The 'manifest-proposal' request type is a DIFFERENT, unrelated event shape +(no agent_input/taskDetails) that already bypasses this whole code path +(see test_manifest_proposal.py) and is NOT in scope for this guard. +""" + +import logging +import os +import sys +from unittest.mock import MagicMock, patch + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +os.environ.setdefault("TOOL_CONFIG_TABLE", "fake-tool-table") +os.environ.setdefault("AGENT_CONFIG_TABLE", "fake-agent-table") +os.environ.setdefault("AGENT_BUCKET_NAME", "fake-bucket") +os.environ.setdefault("COMPLETION_BUS_NAME", "fake-bus") +os.environ.setdefault("WORKER_QUEUE_URL", "https://sqs.fake/queue") + +import index + + +def _base_event(org_id=None, request_type_field=None): + event = { + "orchestration_id": "sess-1", + "agent_use_id": "MyAgent", + "node": "fabricator", + "agent_input": {"taskDetails": "Create an agent that does things"}, + "agent_index": 0, + "total_agents": 1, + } + if org_id is not None: + event["org_id"] = org_id + if request_type_field is not None: + event["requestType"] = request_type_field + return event + + +class TestProcessEventRefusesOrglessMessage: + def setup_method(self): + os.environ["FABRICATION_JOBS_TABLE"] = "citadel-fabrication-jobs-test" + + def teardown_method(self): + os.environ.pop("FABRICATION_JOBS_TABLE", None) + + def test_missing_org_id_refuses_agent_creation_no_fabrication(self, caplog): + with patch.object(index, "check_design_assessment") as gate, \ + patch.object(index, "create_agent_fabricator") as mk_agent, \ + patch.object(index, "_write_fabrication_status") as status_write, \ + patch.object(index, "publish_intake_progress") as progress, \ + patch.object(index, "publish_fabrication_event") as fab_event: + with caplog.at_level(logging.ERROR): + result = index.process_event( + _base_event(), {}, request_type="agent-creation", + ) + + assert result is None + gate.assert_not_called() + mk_agent.assert_not_called() + status_write.assert_not_called() + progress.assert_not_called() + fab_event.assert_not_called() + assert any( + "org" in rec.message.lower() for rec in caplog.records + ), "expected an error-level log naming the missing org_id" + + def test_empty_string_org_id_refuses_tool_creation_no_fabrication(self): + with patch.object(index, "check_design_assessment") as gate, \ + patch.object(index, "create_tool_fabricator") as mk_tool, \ + patch.object(index, "_write_fabrication_status") as status_write: + result = index.process_event( + _base_event(org_id=""), {}, request_type="tool-creation", + ) + + assert result is None + gate.assert_not_called() + mk_tool.assert_not_called() + status_write.assert_not_called() + + def test_missing_org_id_refuses_legacy_direct_request_type_none(self): + # Legacy/direct requests (request_type=None) go through the same + # code-fabrication path and must be refused the same way. + with patch.object(index, "check_design_assessment") as gate, \ + patch.object(index, "create_agent_fabricator") as mk_agent, \ + patch.object(index, "_write_fabrication_status") as status_write: + result = index.process_event(_base_event(), {}, request_type=None) + + assert result is None + gate.assert_not_called() + mk_agent.assert_not_called() + status_write.assert_not_called() + + def test_missing_org_id_does_not_raise(self): + # Safe no-op: must NOT raise, so the SQS message is deleted rather + # than redelivered forever (poison-queue defence, mirrors the + # existing unrecognised-requestType no-op). + with patch.object(index, "check_design_assessment"), \ + patch.object(index, "create_agent_fabricator"): + index.process_event(_base_event(), {}, request_type="agent-creation") + + def test_present_org_id_processes_normally_agent_creation(self): + with patch.object(index, "check_design_assessment") as gate, \ + patch.object(index, "create_agent_fabricator") as mk, \ + patch.object(index, "_write_fabrication_status"), \ + patch.object(index, "publish_intake_progress"): + mk.return_value = MagicMock() + result = index.process_event( + _base_event(org_id="org-real"), {}, request_type="agent-creation", + ) + + assert result is None + gate.assert_called_once() + mk.assert_called_once() + + def test_present_org_id_processes_normally_tool_creation(self): + with patch.object(index, "check_design_assessment") as gate, \ + patch.object(index, "create_tool_fabricator") as mk, \ + patch.object(index, "_write_fabrication_status"), \ + patch.object(index, "publish_intake_progress"): + mk.return_value = MagicMock() + result = index.process_event( + _base_event(org_id="org-real"), {}, request_type="tool-creation", + ) + + assert result is None + gate.assert_called_once() + mk.assert_called_once() + + def test_manifest_proposal_request_type_bypasses_this_guard_entirely(self): + # Different event shape (requestId/correlationId/importId/signals), + # no org_id concept at all -- must NOT be refused by this guard. + event = { + "requestId": "req-1", + "correlationId": "corr-1", + "importId": "imp-1", + "signals": {"summary": "demo"}, + } + with patch.object(index, "_process_manifest_proposal") as proposal: + index.process_event(event, {}, request_type="manifest-proposal") + + proposal.assert_called_once() + + def test_unrecognised_request_type_still_safe_noop_independent_of_org_id(self): + with patch.object(index, "check_design_assessment") as gate: + result = index.process_event( + _base_event(org_id="org-real"), {}, request_type="totally-bogus", + ) + + assert result is None + gate.assert_not_called() diff --git a/arbiter/fabricator/index.py b/arbiter/fabricator/index.py index 61618fb0..aaf80e24 100644 --- a/arbiter/fabricator/index.py +++ b/arbiter/fabricator/index.py @@ -2114,10 +2114,26 @@ def process_event(event, context, request_type=None): orchestration_id = event.get("orchestration_id", "0") agent_use_id = event.get("agent_use_id", "unknown") requested_by = event.get("requested_by") or "fabricator" - # Phase 2b: caller's org, stamped into registry custom metadata so - # registry-service can scope reads. '' mirrors the resolver's defensive - # null fallback so absent-org events still fabricate rather than block. + # Tenancy fail-closed (design evidence, section C): org_id is now + # REQUIRED and must be non-empty for every agent-creation/tool-creation + # (and legacy direct, request_type=None) message. The TS resolver + # (fabricator-request-resolver.ts) always stamps a server-derived, + # non-null org_id via requireOrgId before enqueueing -- a missing/empty + # org_id here means either a pre-migration message or a bypass of that + # resolver, and must be refused rather than silently fabricated without + # tenancy. Refuse = log at ERROR, do NOT process, do NOT create any + # registry/status records, and do NOT raise (safe no-op, mirrors the + # unrecognised-requestType poison-queue defence above: a raise would + # nack the SQS message and retry the poison forever). org_id = event.get("org_id") or "" + if not org_id: + logger.error( + "process_event: refusing message with missing/empty org_id " + "(orchestration_id=%s, agent_use_id=%s, requestType=%r) -- " + "no organization is provisioned for this request", + orchestration_id, agent_use_id, request_type, + ) + return request = event.get("agent_input", {}) agent_name = event.get('node', 'fabricator') agent_index = event.get("agent_index", 0) diff --git a/backend/src/lambda/__tests__/fabricator-request-resolver-dispatch-gate-enumeration.test.ts b/backend/src/lambda/__tests__/fabricator-request-resolver-dispatch-gate-enumeration.test.ts new file mode 100644 index 00000000..c90b4730 --- /dev/null +++ b/backend/src/lambda/__tests__/fabricator-request-resolver-dispatch-gate-enumeration.test.ts @@ -0,0 +1,199 @@ +/** + * Enumeration-completeness guard for fabricator-request-resolver.ts's + * dispatch (design evidence, section C — "mirror requireOrgId exactly", + * modelled on task-runner-resolver-dispatch-gate-enumeration.test.ts). + * + * Root defect closed: `requestAgentCreation`/`requestToolCreation` derived + * org via a null-tolerant `extractOrgFromEvent` call and forwarded + * `org_id: orgId || null` onto the SQS message — an unresolvable org + * fabricated anyway. The fix threads BOTH ops through a shared + * `requireOrgId` fail-closed gate called once in the top-level `handler`, + * BEFORE either op's SQS send. + * + * Unlike the sibling guards' `switch (fieldName)` dispatch, this handler + * (like task-runner-resolver.ts) dispatches via + * `if (fieldName === '') { return await (...); }` / + * `else if` — no `else if` is actually used here (two independent `if` + * blocks that each `return`), so the branch regex tolerates a bare `if` + * chain without a leading `else`. + * + * This handler has exactly two ops. The EXEMPT_OPS list is intentionally + * empty — there is no legitimately ungated op on this dispatch surface. + */ +import * as fs from "fs"; +import * as path from "path"; + +const HANDLER_PATH = path.join( + __dirname, + "..", + "fabricator-request-resolver.ts", +); + +/** + * Extracts field names dispatched on an `if (fieldName === '')` / + * `else if (fieldName === '')` chain inside the handler's body, and + * the function name each branch delegates to via `await (`. + */ +function extractDispatchBranches( + source: string, +): Array<{ fieldName: string; handlerName: string | null }> { + const handlerStart = source.indexOf("export const handler"); + if (handlerStart === -1) { + throw new Error( + "Could not locate `export const handler` in fabricator-request-resolver.ts — " + + "dispatch structure changed; update this guard's parsing.", + ); + } + const handlerBody = source.slice(handlerStart); + + const branchRe = + /(?:if|else if)\s*\(\s*fieldName\s*===\s*['"]([^'"]+)['"]\s*\)\s*\{([^}]*)\}/g; + const branches: Array<{ fieldName: string; handlerName: string | null }> = []; + let m: RegExpExecArray | null; + while ((m = branchRe.exec(handlerBody)) !== null) { + const fieldName = m[1]; + const branchBody = m[2]; + const callRe = /await\s+([A-Za-z0-9_]+)\s*\(/; + const callMatch = callRe.exec(branchBody); + branches.push({ + fieldName, + handlerName: callMatch ? callMatch[1] : null, + }); + } + return branches; +} + +/** + * Extracts the `export const handler = async (event...) => { ... }` body + * by brace-counting from the arrow function's opening `{`. + */ +function extractHandlerBody(source: string): string { + const handlerStart = source.indexOf("export const handler"); + if (handlerStart === -1) { + throw new Error("Could not locate `export const handler`"); + } + const bodyStart = source.indexOf("{", handlerStart); + let depth = 0; + let i = bodyStart; + for (; i < source.length; i++) { + if (source[i] === "{") depth++; + else if (source[i] === "}") { + depth--; + if (depth === 0) break; + } + } + return source.slice(bodyStart, i + 1); +} + +const GATE_CALL_RE = /requireOrgId\s*\(/; +const SQS_SEND_MARKER = "sqsClient.send("; + +/** + * Ops verified to be reachable only after the top-level `requireOrgId` + * gate has run (finding: null-tolerant org derivation on the fabricator + * request path). + */ +const GATED_OPS: Record = { + requestAgentCreation: true, + requestToolCreation: true, +}; + +/** + * No legitimately ungated op exists on this dispatch surface — kept empty + * (rather than omitted) so the "every case accounted for" test below fails + * loudly, not silently, the moment a new branch is added without updating + * this file. + */ +const EXEMPT_OPS: Record = {}; + +describe("fabricator-request-resolver — dispatch enumeration completeness", () => { + const source = fs.readFileSync(HANDLER_PATH, "utf-8"); + const branches = extractDispatchBranches(source); + const fieldNames = branches.map((b) => b.fieldName); + const handlerBody = extractHandlerBody(source); + + test("the dispatch chain actually has branches to check (sanity check on the parser itself)", () => { + expect(fieldNames.length).toBe(2); + expect(fieldNames).toContain("requestAgentCreation"); + expect(fieldNames).toContain("requestToolCreation"); + }); + + test("every dispatch branch is accounted for in GATED_OPS or EXEMPT_OPS", () => { + const unaccounted = fieldNames.filter( + (c) => !(c in GATED_OPS) && !(c in EXEMPT_OPS), + ); + expect(unaccounted).toEqual([]); + }); + + test("GATED_OPS and EXEMPT_OPS do not both claim the same op", () => { + const overlap = Object.keys(GATED_OPS).filter((k) => k in EXEMPT_OPS); + expect(overlap).toEqual([]); + }); + + test("no GATED_OPS/EXEMPT_OPS entry references a branch that no longer exists in the dispatch chain", () => { + const known = new Set(fieldNames); + const staleGated = Object.keys(GATED_OPS).filter((k) => !known.has(k)); + const staleExempt = Object.keys(EXEMPT_OPS).filter((k) => !known.has(k)); + expect(staleGated).toEqual([]); + expect(staleExempt).toEqual([]); + }); + + test("the top-level handler calls requireOrgId before dispatching to any gated op", () => { + const gateIdx = handlerBody.search(GATE_CALL_RE); + expect(gateIdx).toBeGreaterThan(-1); + + for (const fieldName of Object.keys(GATED_OPS)) { + const branchMarker = `fieldName === "${fieldName}"`; + const branchIdx = handlerBody.indexOf(branchMarker); + expect(branchIdx).toBeGreaterThan(-1); + expect(gateIdx).toBeLessThan(branchIdx); + } + }); + + test("requireOrgId's result (orgId) is forwarded as an argument into each gated op's call", () => { + for (const fieldName of Object.keys(GATED_OPS)) { + const branch = branches.find((b) => b.fieldName === fieldName); + expect(branch).toBeDefined(); + expect(branch!.handlerName).not.toBeNull(); + + const callRe = new RegExp( + `await\\s+${branch!.handlerName}\\s*\\(([^)]*)\\)`, + ); + const callMatch = callRe.exec(handlerBody); + expect(callMatch).not.toBeNull(); + expect(/\borgId\b/.test(callMatch![1])).toBe(true); + } + }); + + describe.each(Object.keys(GATED_OPS))( + "GATED_OPS['%s'] handler's SQS send is downstream of orgId, not client input", + (fieldName) => { + test(`${fieldName} forwards orgId into its SQS-sending helper (sendToFabricatorQueue) rather than reading client input`, () => { + const branch = branches.find((b) => b.fieldName === fieldName); + expect(branch).toBeDefined(); + + // The gated function itself (requestAgentCreation/requestToolCreation) + // must accept an orgId parameter and pass it through to + // sendToFabricatorQueue, never re-deriving org itself and never + // reading event.arguments.input for org data. + const fnDeclRe = new RegExp( + `async function ${branch!.handlerName}\\s*\\(([^)]*)\\)`, + ); + const fnDeclMatch = fnDeclRe.exec(source); + expect(fnDeclMatch).not.toBeNull(); + expect(/\borgId\s*:/.test(fnDeclMatch![1])).toBe(true); + }); + }, + ); + + test("no branch in the null-tolerant style (`orgId || null`) remains in the source", () => { + expect(source.includes("orgId || null")).toBe(false); + }); + + test("sanity: the handler body still contains the SQS send marker used by downstream helpers", () => { + // Guards the parser's assumptions about this file's shape — if the SQS + // client call site is renamed/moved, this test flags it rather than the + // marker-based checks above silently no-oping. + expect(source.includes(SQS_SEND_MARKER)).toBe(true); + }); +}); diff --git a/backend/src/lambda/__tests__/fabricator-request-resolver-source-project.test.ts b/backend/src/lambda/__tests__/fabricator-request-resolver-source-project.test.ts index 5d9de5df..85db4d25 100644 --- a/backend/src/lambda/__tests__/fabricator-request-resolver-source-project.test.ts +++ b/backend/src/lambda/__tests__/fabricator-request-resolver-source-project.test.ts @@ -5,18 +5,23 @@ * Uses aws-sdk-client-mock for SQS + DynamoDB, matching the style of the * existing fabricator-request-resolver.test.ts file. */ -import { SQSClient, SendMessageCommand } from '@aws-sdk/client-sqs'; -import { DynamoDBDocumentClient, GetCommand } from '@aws-sdk/lib-dynamodb'; -import { mockClient } from 'aws-sdk-client-mock'; +import { SQSClient, SendMessageCommand } from "@aws-sdk/client-sqs"; +import { DynamoDBDocumentClient, GetCommand } from "@aws-sdk/lib-dynamodb"; +import { mockClient } from "aws-sdk-client-mock"; const sqsMock = mockClient(SQSClient); const ddbMock = mockClient(DynamoDBDocumentClient); -import { handler } from '../fabricator-request-resolver'; +import { handler } from "../fabricator-request-resolver"; const makeEvent = (fieldName: string, args: Record) => ({ info: { fieldName }, arguments: args, + // Tenancy fail-closed (design evidence, section C): the resolver now + // derives org via requireOrgId before any dispatch, so every fixture in + // this file must carry a resolvable caller org to reach the + // sourceProjectId-propagation behaviour under test. + identity: { sub: "user-123", "custom:organization": "org-caller" }, }); function parsePayload() { @@ -25,16 +30,17 @@ function parsePayload() { return JSON.parse(calls[0].args[0].input.MessageBody!); } -describe('fabricator-request-resolver — sourceProjectId propagation (US-ARB-017)', () => { +describe("fabricator-request-resolver — sourceProjectId propagation (US-ARB-017)", () => { let warnSpy: jest.SpyInstance; beforeEach(() => { sqsMock.reset(); ddbMock.reset(); - process.env.FABRICATOR_QUEUE_URL = 'https://sqs.us-west-2.amazonaws.com/123/test-queue'; - process.env.APPS_TABLE = 'citadel-apps-test'; + process.env.FABRICATOR_QUEUE_URL = + "https://sqs.us-west-2.amazonaws.com/123/test-queue"; + process.env.APPS_TABLE = "citadel-apps-test"; sqsMock.on(SendMessageCommand).resolves({}); - warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); + warnSpy = jest.spyOn(console, "warn").mockImplementation(() => undefined); }); afterEach(() => { @@ -43,116 +49,117 @@ describe('fabricator-request-resolver — sourceProjectId propagation (US-ARB-01 warnSpy.mockRestore(); }); - test('agent creation for app with sourceProjectId → payload includes projectId', async () => { + test("agent creation for app with sourceProjectId → payload includes projectId", async () => { ddbMock.on(GetCommand).resolves({ - Item: { appId: 'app-1', sourceProjectId: 'proj-1' }, + Item: { appId: "app-1", sourceProjectId: "proj-1" }, }); const result = await handler( - makeEvent('requestAgentCreation', { + makeEvent("requestAgentCreation", { input: { - agentName: 'GovernedAgent', - taskDescription: 'Build me', - appId: 'app-1', + agentName: "GovernedAgent", + taskDescription: "Build me", + appId: "app-1", }, }), ); expect(result.success).toBe(true); const body = parsePayload(); - expect(body.node).toBe('fabricator'); - expect(body.agent_input.projectId).toBe('proj-1'); - expect(body.agent_input.taskDetails).toContain('GovernedAgent'); + expect(body.node).toBe("fabricator"); + expect(body.agent_input.projectId).toBe("proj-1"); + expect(body.agent_input.taskDetails).toContain("GovernedAgent"); const getCalls = ddbMock.commandCalls(GetCommand); expect(getCalls).toHaveLength(1); - expect(getCalls[0].args[0].input.Key).toEqual({ appId: 'app-1' }); + expect(getCalls[0].args[0].input.Key).toEqual({ appId: "app-1" }); }); - test('agent creation for app WITHOUT sourceProjectId → payload has no projectId', async () => { + test("agent creation for app WITHOUT sourceProjectId → payload has no projectId", async () => { ddbMock.on(GetCommand).resolves({ - Item: { appId: 'app-1' }, // no sourceProjectId attribute + Item: { appId: "app-1" }, // no sourceProjectId attribute }); await handler( - makeEvent('requestAgentCreation', { + makeEvent("requestAgentCreation", { input: { - agentName: 'UngovernedAgent', - taskDescription: 'Build me', - appId: 'app-1', + agentName: "UngovernedAgent", + taskDescription: "Build me", + appId: "app-1", }, }), ); const body = parsePayload(); - expect(body.agent_input).not.toHaveProperty('projectId'); - expect(body.agent_input.taskDetails).toContain('UngovernedAgent'); + expect(body.agent_input).not.toHaveProperty("projectId"); + expect(body.agent_input.taskDetails).toContain("UngovernedAgent"); }); - test('agent creation without appId → payload has no projectId and no DDB lookup', async () => { + test("agent creation without appId → payload has no projectId and no DDB lookup", async () => { await handler( - makeEvent('requestAgentCreation', { + makeEvent("requestAgentCreation", { input: { - agentName: 'StandaloneAgent', - taskDescription: 'Build me', + agentName: "StandaloneAgent", + taskDescription: "Build me", // no appId }, }), ); const body = parsePayload(); - expect(body.agent_input).not.toHaveProperty('projectId'); + expect(body.agent_input).not.toHaveProperty("projectId"); // When appId is absent we must not hit DynamoDB at all. expect(ddbMock.commandCalls(GetCommand)).toHaveLength(0); }); - test('DDB GetItem failure → payload has no projectId and WARN is logged', async () => { - const boom = new Error('Simulated DynamoDB network failure'); + test("DDB GetItem failure → payload has no projectId and WARN is logged", async () => { + const boom = new Error("Simulated DynamoDB network failure"); ddbMock.on(GetCommand).rejects(boom); // Must not throw — the fabricator request must still succeed in degraded mode. const result = await handler( - makeEvent('requestAgentCreation', { + makeEvent("requestAgentCreation", { input: { - agentName: 'ResilientAgent', - taskDescription: 'Build me', - appId: 'app-err', + agentName: "ResilientAgent", + taskDescription: "Build me", + appId: "app-err", }, }), ); expect(result.success).toBe(true); const body = parsePayload(); - expect(body.agent_input).not.toHaveProperty('projectId'); + expect(body.agent_input).not.toHaveProperty("projectId"); // A warning was produced to document the degraded lookup. - const warnedDegradedLookup = warnSpy.mock.calls.some(callArgs => + const warnedDegradedLookup = warnSpy.mock.calls.some((callArgs) => callArgs.some( (arg: unknown) => - typeof arg === 'string' && arg.includes('Failed to look up sourceProjectId'), + typeof arg === "string" && + arg.includes("Failed to look up sourceProjectId"), ), ); expect(warnedDegradedLookup).toBe(true); }); - test('tool creation forwards projectId when app has sourceProjectId', async () => { + test("tool creation forwards projectId when app has sourceProjectId", async () => { ddbMock.on(GetCommand).resolves({ - Item: { appId: 'app-2', sourceProjectId: 'proj-xyz' }, + Item: { appId: "app-2", sourceProjectId: "proj-xyz" }, }); await handler( - makeEvent('requestToolCreation', { + makeEvent("requestToolCreation", { input: { - toolName: 'MyTool', - toolDescription: 'A tool', - appId: 'app-2', + toolName: "MyTool", + toolDescription: "A tool", + appId: "app-2", }, }), ); const body = parsePayload(); - expect(body.agent_input.projectId).toBe('proj-xyz'); - expect(body.agent_input.taskDetails).toContain('MyTool'); + expect(body.agent_input.projectId).toBe("proj-xyz"); + expect(body.agent_input.taskDetails).toContain("MyTool"); }); }); @@ -170,17 +177,19 @@ describe('fabricator-request-resolver — sourceProjectId propagation (US-ARB-01 // for assertion only, mirroring the file-private copy inside the resolver. // --------------------------------------------------------------------------- -import type { RegistryRecord } from '../../services/registry-service'; +import type { RegistryRecord } from "../../services/registry-service"; -function projectIdFromRegistryRecord(record: RegistryRecord): string | undefined { +function projectIdFromRegistryRecord( + record: RegistryRecord, +): string | undefined { if (!record.customDescriptorContent) return undefined; try { const parsed = JSON.parse(record.customDescriptorContent); if ( parsed && - typeof parsed === 'object' && + typeof parsed === "object" && !Array.isArray(parsed) && - typeof parsed.sourceProjectId === 'string' + typeof parsed.sourceProjectId === "string" ) { return parsed.sourceProjectId; } @@ -190,46 +199,46 @@ function projectIdFromRegistryRecord(record: RegistryRecord): string | undefined return undefined; } -describe('projectIdFromRegistryRecord (inlined helper — PR 6a)', () => { - test('returns undefined when customDescriptorContent is absent', () => { +describe("projectIdFromRegistryRecord (inlined helper — PR 6a)", () => { + test("returns undefined when customDescriptorContent is absent", () => { const record: RegistryRecord = { - recordId: 'r', - name: 'n', - status: 'DRAFT', + recordId: "r", + name: "n", + status: "DRAFT", }; expect(projectIdFromRegistryRecord(record)).toBeUndefined(); }); - test('returns undefined when sourceProjectId is missing from metadata', () => { + test("returns undefined when sourceProjectId is missing from metadata", () => { const record: RegistryRecord = { - recordId: 'r', - name: 'n', - status: 'DRAFT', + recordId: "r", + name: "n", + status: "DRAFT", customDescriptorContent: JSON.stringify({ categories: [], - icon: '', - state: 'active', + icon: "", + state: "active", }), }; expect(projectIdFromRegistryRecord(record)).toBeUndefined(); }); - test('returns the sourceProjectId string when present', () => { + test("returns the sourceProjectId string when present", () => { const record: RegistryRecord = { - recordId: 'r', - name: 'n', - status: 'DRAFT', - customDescriptorContent: JSON.stringify({ sourceProjectId: 'proj-99' }), + recordId: "r", + name: "n", + status: "DRAFT", + customDescriptorContent: JSON.stringify({ sourceProjectId: "proj-99" }), }; - expect(projectIdFromRegistryRecord(record)).toBe('proj-99'); + expect(projectIdFromRegistryRecord(record)).toBe("proj-99"); }); - test('returns undefined on malformed JSON rather than throwing', () => { + test("returns undefined on malformed JSON rather than throwing", () => { const record: RegistryRecord = { - recordId: 'r', - name: 'n', - status: 'DRAFT', - customDescriptorContent: 'definitely not json', + recordId: "r", + name: "n", + status: "DRAFT", + customDescriptorContent: "definitely not json", }; expect(() => projectIdFromRegistryRecord(record)).not.toThrow(); expect(projectIdFromRegistryRecord(record)).toBeUndefined(); diff --git a/backend/src/lambda/__tests__/fabricator-request-resolver.test.ts b/backend/src/lambda/__tests__/fabricator-request-resolver.test.ts index b38783af..eeaa2b68 100644 --- a/backend/src/lambda/__tests__/fabricator-request-resolver.test.ts +++ b/backend/src/lambda/__tests__/fabricator-request-resolver.test.ts @@ -7,38 +7,120 @@ * best-effort: a failure must NOT fail the enqueue (the caller already got a * queued request). */ -import { SQSClient, SendMessageCommand } from '@aws-sdk/client-sqs'; -import { DynamoDBDocumentClient, PutCommand } from '@aws-sdk/lib-dynamodb'; -import { mockClient } from 'aws-sdk-client-mock'; +import { SQSClient, SendMessageCommand } from "@aws-sdk/client-sqs"; +import { DynamoDBDocumentClient, PutCommand } from "@aws-sdk/lib-dynamodb"; +import { mockClient } from "aws-sdk-client-mock"; const sqsMock = mockClient(SQSClient); const ddbMock = mockClient(DynamoDBDocumentClient); -process.env.FABRICATOR_QUEUE_URL = 'https://sqs.test/queue'; -process.env.FABRICATION_JOBS_TABLE = 'citadel-fabrication-jobs-test'; +process.env.FABRICATOR_QUEUE_URL = "https://sqs.test/queue"; +process.env.FABRICATION_JOBS_TABLE = "citadel-fabrication-jobs-test"; -import { handler } from '../fabricator-request-resolver'; +import { handler } from "../fabricator-request-resolver"; -const makeEvent = (fieldName: string, input: Record) => ({ +const makeEvent = ( + fieldName: string, + input: Record, + identity: Record = { + sub: "user-123", + "custom:organization": "org-caller", + }, +) => ({ info: { fieldName }, arguments: { input }, - identity: { sub: 'user-123' }, + identity, }); -describe('fabricator-request-resolver', () => { +describe("fabricator-request-resolver", () => { beforeEach(() => { sqsMock.reset(); ddbMock.reset(); - sqsMock.on(SendMessageCommand).resolves({ MessageId: 'm1' }); + sqsMock.on(SendMessageCommand).resolves({ MessageId: "m1" }); ddbMock.on(PutCommand).resolves({}); - process.env.FABRICATION_JOBS_TABLE = 'citadel-fabrication-jobs-test'; + process.env.FABRICATION_JOBS_TABLE = "citadel-fabrication-jobs-test"; }); - test('writes a PENDING row after the SQS send for agent creation', async () => { + describe("org tenancy (fail-closed)", () => { + test("requestAgentCreation throws and sends zero SQS messages when caller has no organization", async () => { + await expect( + handler( + makeEvent( + "requestAgentCreation", + { agentName: "NoOrgAgent", taskDescription: "desc" }, + { sub: "user-no-org" }, + ), + ), + ).rejects.toThrow("Access denied: no organization is provisioned"); + + expect(sqsMock.commandCalls(SendMessageCommand)).toHaveLength(0); + expect(ddbMock.commandCalls(PutCommand)).toHaveLength(0); + }); + + test("requestToolCreation throws and sends zero SQS messages when caller has no organization", async () => { + await expect( + handler( + makeEvent( + "requestToolCreation", + { toolName: "NoOrgTool", toolDescription: "desc" }, + { sub: "user-no-org" }, + ), + ), + ).rejects.toThrow("Access denied: no organization is provisioned"); + + expect(sqsMock.commandCalls(SendMessageCommand)).toHaveLength(0); + expect(ddbMock.commandCalls(PutCommand)).toHaveLength(0); + }); + + test("stamps the server-derived orgId (never null) on the SQS message body", async () => { + await handler( + makeEvent("requestAgentCreation", { + agentName: "OrgAgent", + taskDescription: "desc", + }), + ); + + const call = sqsMock.commandCalls(SendMessageCommand)[0]; + const body = JSON.parse(call.args[0].input.MessageBody as string); + expect(body.org_id).toBe("org-caller"); + }); + + test("stamps the server-derived orgId on the PENDING status row", async () => { + await handler( + makeEvent("requestToolCreation", { + toolName: "OrgTool", + toolDescription: "desc", + }), + ); + + const item = ddbMock.commandCalls(PutCommand)[0].args[0].input.Item!; + expect(item.orgId).toBe("org-caller"); + }); + + test("ignores a client-smuggled orgId on the input and uses the server-derived org instead", async () => { + await handler( + makeEvent("requestAgentCreation", { + agentName: "SmuggledOrgAgent", + taskDescription: "desc", + orgId: "attacker-org", + }), + ); + + const call = sqsMock.commandCalls(SendMessageCommand)[0]; + const body = JSON.parse(call.args[0].input.MessageBody as string); + expect(body.org_id).toBe("org-caller"); + expect(body.org_id).not.toBe("attacker-org"); + + const item = ddbMock.commandCalls(PutCommand)[0].args[0].input.Item!; + expect(item.orgId).toBe("org-caller"); + }); + }); + + test("writes a PENDING row after the SQS send for agent creation", async () => { const result = await handler( - makeEvent('requestAgentCreation', { - agentName: 'InvoiceParser', - taskDescription: 'Parse invoices from PDFs', + makeEvent("requestAgentCreation", { + agentName: "InvoiceParser", + taskDescription: "Parse invoices from PDFs", }), ); @@ -48,24 +130,26 @@ describe('fabricator-request-resolver', () => { const puts = ddbMock.commandCalls(PutCommand); expect(puts).toHaveLength(1); const item = puts[0].args[0].input.Item!; - expect(puts[0].args[0].input.TableName).toBe('citadel-fabrication-jobs-test'); - expect(item.orchestrationId).toBe('0'); + expect(puts[0].args[0].input.TableName).toBe( + "citadel-fabrication-jobs-test", + ); + expect(item.orchestrationId).toBe("0"); expect(item.agentUseId).toBe(result.requestId); - expect(item.status).toBe('PENDING'); - expect(item.agentName).toBe('InvoiceParser'); - expect(item.requestType).toBe('agent-creation'); - expect(item.requestedBy).toBe('user-123'); - expect(typeof item.submittedAt).toBe('string'); - expect(typeof item.updatedAt).toBe('string'); - expect(typeof item.ttl).toBe('number'); + expect(item.status).toBe("PENDING"); + expect(item.agentName).toBe("InvoiceParser"); + expect(item.requestType).toBe("agent-creation"); + expect(item.requestedBy).toBe("user-123"); + expect(typeof item.submittedAt).toBe("string"); + expect(typeof item.updatedAt).toBe("string"); + expect(typeof item.ttl).toBe("number"); expect(item.ttl).toBeGreaterThan(Math.floor(Date.now() / 1000)); }); - test('truncates taskDescription to ~500 chars', async () => { - const longDesc = 'x'.repeat(2000); + test("truncates taskDescription to ~500 chars", async () => { + const longDesc = "x".repeat(2000); await handler( - makeEvent('requestAgentCreation', { - agentName: 'BigAgent', + makeEvent("requestAgentCreation", { + agentName: "BigAgent", taskDescription: longDesc, }), ); @@ -73,13 +157,13 @@ describe('fabricator-request-resolver', () => { expect((item.taskDescription as string).length).toBeLessThanOrEqual(500); }); - test('does NOT fail the enqueue when the status write throws', async () => { - ddbMock.on(PutCommand).rejects(new Error('ddb down')); + test("does NOT fail the enqueue when the status write throws", async () => { + ddbMock.on(PutCommand).rejects(new Error("ddb down")); const result = await handler( - makeEvent('requestToolCreation', { - toolName: 'CsvExporter', - toolDescription: 'Export rows to CSV', + makeEvent("requestToolCreation", { + toolName: "CsvExporter", + toolDescription: "Export rows to CSV", }), ); @@ -88,13 +172,13 @@ describe('fabricator-request-resolver', () => { expect(sqsMock.commandCalls(SendMessageCommand)).toHaveLength(1); }); - test('skips the status write when FABRICATION_JOBS_TABLE is unset', async () => { + test("skips the status write when FABRICATION_JOBS_TABLE is unset", async () => { delete process.env.FABRICATION_JOBS_TABLE; const result = await handler( - makeEvent('requestAgentCreation', { - agentName: 'NoTableAgent', - taskDescription: 'No table configured', + makeEvent("requestAgentCreation", { + agentName: "NoTableAgent", + taskDescription: "No table configured", }), ); diff --git a/backend/src/lambda/fabricator-request-resolver.ts b/backend/src/lambda/fabricator-request-resolver.ts index 80b59bf8..6fdd2cc2 100644 --- a/backend/src/lambda/fabricator-request-resolver.ts +++ b/backend/src/lambda/fabricator-request-resolver.ts @@ -65,7 +65,7 @@ async function writePendingFabricationStatus( taskDetails: string, requestType: "agent-creation" | "tool-creation", requestedBy: string, - orgId: string | null, + orgId: string, ): Promise { const table = getFabricationJobsTable(); if (!table) { @@ -82,12 +82,10 @@ async function writePendingFabricationStatus( Item: { orchestrationId: "0", agentUseId: requestId, - // Server-derived caller org (Phase 2b already resolves this via - // extractOrgFromEvent for the SQS org_id field) — stamped onto the - // row so the org-scoped getFabricatorQueue GSI query can find it. - // Omitted (never a blank string) when unresolvable so a missing - // orgId reads as absent, not as a false empty-string match. - ...(orgId ? { orgId } : {}), + // Server-derived caller org (requireOrgId, fail-closed — never + // null) — stamped onto the row so the org-scoped + // getFabricatorQueue GSI query can find it. + orgId, status: "PENDING", agentName: deriveAgentName(taskDetails), taskDescription: taskDetails.slice(0, TASK_DESCRIPTION_MAX), @@ -187,15 +185,42 @@ interface FabricatorRequestResolverEvent { arguments: Record; } +/** + * Fail-closed server-side organisation derivation (mirrors + * `task-runner-resolver.ts`'s `requireOrgId` exactly — same primitive + * (`extractOrgFromEvent`, JWT `custom:organization` claim with a Cognito + * AdminGetUser fallback), same error message, same fail-closed contract). + * + * Replaces the prior null-tolerant `extractOrgFromEvent` call ("Null is + * acceptable during the transition") now that the transition window is + * over (design evidence, section C): both `requestAgentCreation` and + * `requestToolCreation` must derive org BEFORE any SQS send / status + * write, and never fall back to client input or a default org. + * + * Throws (fails closed) when no organisation resolves — never returns + * null. Owner decision: tenancy-only, NO platform-role gate. + */ +async function requireOrgId( + event: FabricatorRequestResolverEvent, +): Promise { + const orgId = await extractOrgFromEvent(event); + if (!orgId) { + throw new Error( + "Access denied: no organization is provisioned for your account. Contact an administrator.", + ); + } + return orgId; +} + export const handler = async (event: FabricatorRequestResolverEvent) => { console.log("Event:", JSON.stringify(event, null, 2)); const fieldName = event.info.fieldName; const requestedBy = extractRequestedBy(event); - // Phase 2b: thread the caller's orgId so the fabricator can stamp it into - // custom metadata. Null is acceptable during the transition — the Python - // side falls back to '' rather than blocking fabrication. - const orgId = await extractOrgFromEvent(event); + // Fail closed BEFORE any SQS send / status write — an unresolved + // organisation must never reach the queue (design evidence, section C). + // Never derived from client input (`event.arguments.input`). + const orgId = await requireOrgId(event); try { if (fieldName === "requestAgentCreation") { @@ -226,7 +251,7 @@ async function sendToFabricatorQueue( taskDetails: string, requestType: "agent-creation" | "tool-creation", requestedBy: string, - orgId: string | null, + orgId: string, sourceProjectId?: string, ) { const agent_input: Record = { taskDetails }; @@ -242,9 +267,10 @@ async function sendToFabricatorQueue( node: "fabricator", agent_input, requested_by: requestedBy, - // Phase 2b: carry caller org through the SQS boundary. Python fabricator - // falls back to '' when this is null, so no blocking during transition. - org_id: orgId || null, + // Server-derived caller org (requireOrgId, fail-closed) — never null, + // never client input. The Python fabricator now requires this to be + // non-empty and refuses to process otherwise. + org_id: orgId, }; console.log("Sending message to Fabricator queue:", fabricatorMessage); @@ -329,7 +355,7 @@ async function resolveSourceProjectId( async function requestAgentCreation( input: CreateAgentRequest, requestedBy: string, - orgId: string | null, + orgId: string, ) { const requestId = randomUUID(); @@ -373,7 +399,7 @@ ${input.taskDescription}`; async function requestToolCreation( input: CreateToolRequest, requestedBy: string, - orgId: string | null, + orgId: string, ) { const requestId = randomUUID(); diff --git a/service/agent_intake_single/tests/test_fabricate_org_tenancy.py b/service/agent_intake_single/tests/test_fabricate_org_tenancy.py new file mode 100644 index 00000000..c45e8e6a --- /dev/null +++ b/service/agent_intake_single/tests/test_fabricate_org_tenancy.py @@ -0,0 +1,108 @@ +"""Tests for org-tenancy enforcement in tools/fabricate.py. + +Contract: +- _send_to_fabricator resolves org_id = _resolve_session_organization( + session_id) BEFORE sqs.send_message. +- If org_id is falsy: log at error, raise, and NEVER call sqs.send_message + (no partial/untenanted enqueue). +- If org_id resolves: the SQS MessageBody JSON carries "org_id": org_id. +- retry_failed_fabrication re-queues via the same _send_to_fabricator path, + so a resolved org_id must also land in the retry-path MessageBody. + +Run with: + .venv-arbiter-test/bin/python -m pytest \ + service/agent_intake_single/tests/test_fabricate_org_tenancy.py -q +from the repo root. +""" +import json +import os +import sys +from unittest import mock + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +os.environ.setdefault("FABRICATOR_QUEUE_URL", "https://sqs.fake/queue") + +import tools.fabricate as fab + + +@pytest.fixture +def fab_env(monkeypatch): + monkeypatch.setattr(fab, "sqs", mock.MagicMock()) + monkeypatch.setattr(fab, "FABRICATOR_QUEUE_URL", "https://sqs.fake/queue") + monkeypatch.setattr(fab, "FABRICATION_JOBS_TABLE", None) + # _write_pending_fabrication_status is best-effort/unrelated to this + # contract; no-op it so tests only assert on the tenancy behavior. + monkeypatch.setattr(fab, "_write_pending_fabrication_status", lambda *a, **k: None) + return fab + + +def _sent_bodies(): + return [json.loads(c.kwargs["MessageBody"]) + for c in fab.sqs.send_message.call_args_list] + + +def test_send_to_fabricator_body_carries_org_id(fab_env, monkeypatch): + monkeypatch.setattr(fab, "_resolve_session_organization", lambda session_id: "org-123") + + fab._send_to_fabricator("sess-1", {"name": "AgentA", "spec": "spec-a"}) + + fab.sqs.send_message.assert_called_once() + body = _sent_bodies()[0] + assert body["org_id"] == "org-123" + assert body["agent_use_id"] == "AgentA" + + +def test_send_to_fabricator_raises_and_does_not_enqueue_when_org_unresolvable(fab_env, monkeypatch): + monkeypatch.setattr(fab, "_resolve_session_organization", lambda session_id: None) + + with pytest.raises(Exception, match=r"cannot fabricate: no organisation resolved for session sess-2"): + fab._send_to_fabricator("sess-2", {"name": "AgentB", "spec": "spec-b"}) + + fab.sqs.send_message.assert_not_called() + + +def test_send_to_fabricator_raises_on_empty_string_org(fab_env, monkeypatch): + monkeypatch.setattr(fab, "_resolve_session_organization", lambda session_id: "") + + with pytest.raises(Exception, match=r"cannot fabricate: no organisation resolved for session sess-3"): + fab._send_to_fabricator("sess-3", {"name": "AgentC", "spec": "spec-c"}) + + fab.sqs.send_message.assert_not_called() + + +def test_retry_failed_fabrication_body_carries_org_id(fab_env, monkeypatch): + monkeypatch.setattr(fab, "_resolve_session_organization", lambda session_id: "org-999") + + table = mock.MagicMock() + ddb = mock.MagicMock() + ddb.Table.return_value = table + monkeypatch.setattr(fab, "dynamodb", ddb) + monkeypatch.setattr(fab, "FABRICATION_JOBS_TABLE", "jobs-test") + + plan_md = ( + "# Fabrication Plan\n\n" + "## Agents to Build\n\n" + "### AgentD\nspec: agent-d\n" + ) + monkeypatch.setattr(fab, "s3_get", lambda key: plan_md) + + table.query.return_value = { + "Items": [ + { + "orchestrationId": "sess-4", "agentUseId": "AgentD", + "agentName": "AgentD", "status": "FAILED", + "updatedAt": "2026-07-19T04:31:03.885692Z", + }, + ], + } + + result = json.loads(fab.retry_failed_fabrication(session_id="sess-4")) + + assert result["ok"] is True + fab.sqs.send_message.assert_called_once() + body = _sent_bodies()[0] + assert body["org_id"] == "org-999" + assert body["agent_use_id"] == "AgentD" diff --git a/service/agent_intake_single/tools/fabricate.py b/service/agent_intake_single/tools/fabricate.py index 434f1646..5522d834 100644 --- a/service/agent_intake_single/tools/fabricate.py +++ b/service/agent_intake_single/tools/fabricate.py @@ -397,7 +397,26 @@ def _get_existing_agents() -> dict[str, dict]: return agents +def _require_session_organization(session_id: str) -> str: + """Resolve the session's organization or raise, never enqueuing a + fabrication request without a tenant. + + Shared by every fabricator-enqueue call site (initial send and retry) + so tenancy is enforced identically regardless of path. + """ + org_id = _resolve_session_organization(session_id) + if not org_id: + logger.error( + "cannot fabricate: no organisation resolved for session %s", session_id, + ) + raise ValueError( + f"cannot fabricate: no organisation resolved for session {session_id}" + ) + return org_id + + def _send_to_fabricator(session_id: str, agent: dict, agent_index: int = 0, total_agents: int = 1): + org_id = _require_session_organization(session_id) sqs.send_message( QueueUrl=FABRICATOR_QUEUE_URL, MessageBody=json.dumps({ @@ -407,6 +426,7 @@ def _send_to_fabricator(session_id: str, agent: dict, agent_index: int = 0, tota "agent_input": {"taskDetails": f"Create an agent with the following specification:\n\n{agent['spec']}"}, "agent_index": agent_index, "total_agents": total_agents, + "org_id": org_id, }), MessageAttributes={ "requestType": {"DataType": "String", "StringValue": "agent-creation"}, From a6327e94cd39f2a4185a19c4d70059ec2d54777d Mon Sep 17 00:00:00 2001 From: Oliver Gibbs Date: Fri, 18 Sep 2026 01:25:46 +0000 Subject: [PATCH 2/3] fix(fabricator): accept both org_id and orgId on the consumer organisation gate The intake tool sends org_id (snake) and the supervisor sends orgId (camel); process_event now honours either and still refuses events with neither. Intake tests mock _resolve_session_organization and add fail-closed refuse-when-unresolved cases; the jobs_table fixture gains a terminating query() default so an unconfigured MagicMock can no longer spin the pagination loop forever. Ignore .venv-arbiter-test/. Refs finding fac16c02. --- .gitignore | 1 + .../test_process_event_org_tenancy_gate.py | 122 +++++++++++++++++- arbiter/fabricator/index.py | 12 +- .../tests/test_fabrication_retry.py | 27 ++++ .../tests/test_fabrication_status.py | 23 ++++ 5 files changed, 182 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 78b270e4..f1491a12 100644 --- a/.gitignore +++ b/.gitignore @@ -16,6 +16,7 @@ ENV/ .venv .venv-test/ .venv-check/ +.venv-arbiter-test/ # E2E test artifacts scripts/* diff --git a/arbiter/fabricator/__tests__/test_process_event_org_tenancy_gate.py b/arbiter/fabricator/__tests__/test_process_event_org_tenancy_gate.py index 66c55f08..5edeaa3d 100644 --- a/arbiter/fabricator/__tests__/test_process_event_org_tenancy_gate.py +++ b/arbiter/fabricator/__tests__/test_process_event_org_tenancy_gate.py @@ -32,7 +32,7 @@ import index -def _base_event(org_id=None, request_type_field=None): +def _base_event(org_id=None, request_type_field=None, org_id_key="org_id"): event = { "orchestration_id": "sess-1", "agent_use_id": "MyAgent", @@ -42,7 +42,7 @@ def _base_event(org_id=None, request_type_field=None): "total_agents": 1, } if org_id is not None: - event["org_id"] = org_id + event[org_id_key] = org_id if request_type_field is not None: event["requestType"] = request_type_field return event @@ -160,3 +160,121 @@ def test_unrecognised_request_type_still_safe_noop_independent_of_org_id(self): assert result is None gate.assert_not_called() + + +class TestProcessEventAcceptsBothOrgIdKeys: + """org_id key-mismatch fix (Supervisor sends camelCase 'orgId' on its + generic worker-dispatch payload — arbiter/supervisor/index.py — while + service/agent_intake_single/tools/fabricate.py's _send_to_fabricator + sends snake_case 'org_id'). The gate must accept either key so + Supervisor-originated fabrication is not refused. + """ + + def setup_method(self): + os.environ["FABRICATION_JOBS_TABLE"] = "citadel-fabrication-jobs-test" + + def teardown_method(self): + os.environ.pop("FABRICATION_JOBS_TABLE", None) + + def test_camelcase_orgid_key_processes_normally_agent_creation(self): + with patch.object(index, "check_design_assessment") as gate, \ + patch.object(index, "create_agent_fabricator") as mk, \ + patch.object(index, "_write_fabrication_status") as status_write, \ + patch.object(index, "publish_intake_progress"): + mk.return_value = MagicMock() + result = index.process_event( + _base_event(org_id="org-real", org_id_key="orgId"), + {}, request_type="agent-creation", + ) + + assert result is None + gate.assert_called_once() + mk.assert_called_once() + # org_id threaded from the camelCase key reaches the status write. + assert status_write.call_args.kwargs.get("org_id") == "org-real" + + def test_camelcase_orgid_key_processes_normally_tool_creation(self): + with patch.object(index, "check_design_assessment") as gate, \ + patch.object(index, "create_tool_fabricator") as mk, \ + patch.object(index, "_write_fabrication_status") as status_write, \ + patch.object(index, "publish_intake_progress"): + mk.return_value = MagicMock() + result = index.process_event( + _base_event(org_id="org-real", org_id_key="orgId"), + {}, request_type="tool-creation", + ) + + assert result is None + gate.assert_called_once() + mk.assert_called_once() + assert status_write.call_args.kwargs.get("org_id") == "org-real" + + def test_snake_case_org_id_key_still_processes_normally(self): + # Regression guard: the pre-existing snake_case producer path must + # keep working after the OR-fallback is introduced. + with patch.object(index, "check_design_assessment") as gate, \ + patch.object(index, "create_agent_fabricator") as mk, \ + patch.object(index, "_write_fabrication_status") as status_write, \ + patch.object(index, "publish_intake_progress"): + mk.return_value = MagicMock() + result = index.process_event( + _base_event(org_id="org-real", org_id_key="org_id"), + {}, request_type="agent-creation", + ) + + assert result is None + gate.assert_called_once() + mk.assert_called_once() + assert status_write.call_args.kwargs.get("org_id") == "org-real" + + def test_both_keys_missing_refuses_no_fabrication_no_raise(self, caplog): + with patch.object(index, "check_design_assessment") as gate, \ + patch.object(index, "create_agent_fabricator") as mk_agent, \ + patch.object(index, "_write_fabrication_status") as status_write, \ + patch.object(index, "publish_intake_progress") as progress, \ + patch.object(index, "publish_fabrication_event") as fab_event: + with caplog.at_level(logging.ERROR): + result = index.process_event( + _base_event(), {}, request_type="agent-creation", + ) + + assert result is None + gate.assert_not_called() + mk_agent.assert_not_called() + status_write.assert_not_called() + progress.assert_not_called() + fab_event.assert_not_called() + assert any( + "org" in rec.message.lower() for rec in caplog.records + ), "expected an error-level log naming the missing org_id" + + def test_both_keys_present_prefers_snake_case(self): + # Defensive: if a message somehow carries both keys, snake_case wins + # (matches this module's own attribute naming convention). + with patch.object(index, "check_design_assessment"), \ + patch.object(index, "create_agent_fabricator") as mk, \ + patch.object(index, "_write_fabrication_status") as status_write, \ + patch.object(index, "publish_intake_progress"): + mk.return_value = MagicMock() + event = _base_event(org_id="org-snake", org_id_key="org_id") + event["orgId"] = "org-camel" + index.process_event(event, {}, request_type="agent-creation") + + assert status_write.call_args.kwargs.get("org_id") == "org-snake" + + def test_empty_snake_case_falls_back_to_camelcase(self): + # org_id="" (falsy) present alongside a non-empty orgId must still + # resolve via the `or` fallback rather than refusing. + with patch.object(index, "check_design_assessment") as gate, \ + patch.object(index, "create_agent_fabricator") as mk, \ + patch.object(index, "_write_fabrication_status") as status_write, \ + patch.object(index, "publish_intake_progress"): + mk.return_value = MagicMock() + event = _base_event(org_id="", org_id_key="org_id") + event["orgId"] = "org-camel" + result = index.process_event(event, {}, request_type="agent-creation") + + assert result is None + gate.assert_called_once() + mk.assert_called_once() + assert status_write.call_args.kwargs.get("org_id") == "org-camel" diff --git a/arbiter/fabricator/index.py b/arbiter/fabricator/index.py index aaf80e24..c061a6b6 100644 --- a/arbiter/fabricator/index.py +++ b/arbiter/fabricator/index.py @@ -2125,7 +2125,17 @@ def process_event(event, context, request_type=None): # registry/status records, and do NOT raise (safe no-op, mirrors the # unrecognised-requestType poison-queue defence above: a raise would # nack the SQS message and retry the poison forever). - org_id = event.get("org_id") or "" + # + # Two producers stamp this value under DIFFERENT keys and both must be + # accepted: service/agent_intake_single/tools/fabricate.py's + # _send_to_fabricator sends snake_case "org_id" (matches this module's + # own DynamoDB/EventBridge attribute naming), while + # arbiter/supervisor/index.py's generic worker-dispatch payload (wave 2a) + # sends camelCase "orgId" (matches the orchestration row's `orgId` + # attribute it was read from). Reading only one key silently refuses + # Supervisor-originated fabrication requests, so both are checked here; + # snake_case is preferred when a message somehow carries both. + org_id = event.get("org_id") or event.get("orgId") or "" if not org_id: logger.error( "process_event: refusing message with missing/empty org_id " diff --git a/service/agent_intake_single/tests/test_fabrication_retry.py b/service/agent_intake_single/tests/test_fabrication_retry.py index 6774c2f8..cf88a475 100644 --- a/service/agent_intake_single/tests/test_fabrication_retry.py +++ b/service/agent_intake_single/tests/test_fabrication_retry.py @@ -101,6 +101,10 @@ def _live_rows(arbiter_updated_at=STALE_TS): @pytest.fixture def jobs_table(monkeypatch): table = mock.MagicMock() + # Terminating default: an unconfigured query() must return a page with no + # LastEvaluatedKey, otherwise fabricate.py's pagination loop spins forever + # on a truthy MagicMock and mock call history eats all memory. + table.query.return_value = {"Items": []} ddb = mock.MagicMock() ddb.Table.return_value = table monkeypatch.setattr(fab, "dynamodb", ddb) @@ -108,6 +112,12 @@ def jobs_table(monkeypatch): monkeypatch.setattr(fab, "FABRICATOR_QUEUE_URL", "https://sqs.fake/queue") monkeypatch.setattr(fab, "FABRICATION_JOBS_TABLE", "jobs-test") monkeypatch.setattr(fab, "s3_get", lambda key: PLAN_MD) + # Every retry re-enqueue goes through _send_to_fabricator, which now + # fail-closed refuses to enqueue without a resolved organisation (see + # test_fabricate_org_tenancy.py). Mock a resolved org here so these + # fixture-driven tests exercise the retry logic itself rather than the + # tenancy gate — tenancy behavior has its own dedicated test module. + monkeypatch.setattr(fab, "_resolve_session_organization", lambda session_id: "org-retry-test") return table @@ -273,3 +283,20 @@ def test_query_failure_returns_unavailable(jobs_table): assert result["ok"] is False assert result["status"] == "unavailable" assert fab.sqs.send_message.call_count == 0 + + +def test_retry_refuses_when_organisation_unresolved(jobs_table, monkeypatch): + # Explicit proof the fail-closed helper is NOT weakened by the fixture + # mock above: when the session's organisation cannot be resolved, + # _send_to_fabricator must raise and no SQS message must be sent for + # any eligible target, even though the row-eligibility logic itself + # would otherwise retry them. + monkeypatch.setattr(fab, "_resolve_session_organization", lambda session_id: None) + # Real rows (not a bare MagicMock) so the jobs-table pagination loop + # terminates and the eligible FAILED targets reach _send_to_fabricator. + jobs_table.query.return_value = {"Items": _live_rows()} + + with pytest.raises(Exception, match=r"cannot fabricate: no organisation resolved"): + fab.retry_failed_fabrication(session_id="sess-1") + + fab.sqs.send_message.assert_not_called() diff --git a/service/agent_intake_single/tests/test_fabrication_status.py b/service/agent_intake_single/tests/test_fabrication_status.py index 9cb09c96..32376948 100644 --- a/service/agent_intake_single/tests/test_fabrication_status.py +++ b/service/agent_intake_single/tests/test_fabrication_status.py @@ -34,6 +34,13 @@ def _stub_io(monkeypatch): # tools.state import inside confirm_fabrication_plan state_mod = mock.MagicMock() monkeypatch.setitem(sys.modules, "tools.state", state_mod) + # confirm_fabrication_plan enqueues each 'build' agent via + # _send_to_fabricator, which now fail-closed refuses to enqueue without + # a resolved organisation (see test_fabricate_org_tenancy.py). Mock a + # resolved org here so these status-write tests exercise status-write + # behavior rather than the tenancy gate — a dedicated test below proves + # the unresolved case still refuses. + monkeypatch.setattr(fab, "_resolve_session_organization", lambda session_id: "org-status-test") yield @@ -88,3 +95,19 @@ def test_skips_status_write_when_table_unset(monkeypatch): assert fab.sqs.send_message.call_count == 2 table.put_item.assert_not_called() + + +def test_enqueue_refuses_when_organisation_unresolved(monkeypatch): + # Explicit proof the fail-closed helper is NOT weakened by the + # autouse fixture's mock above: with no resolvable organisation, + # confirm_fabrication_plan must raise before any SQS send. + monkeypatch.setattr(fab, "FABRICATION_JOBS_TABLE", "citadel-fabrication-jobs-test") + monkeypatch.setattr(fab, "_resolve_session_organization", lambda session_id: None) + table = mock.MagicMock() + monkeypatch.setattr(fab.dynamodb, "Table", mock.MagicMock(return_value=table)) + + with pytest.raises(Exception, match=r"cannot fabricate: no organisation resolved"): + fab.confirm_fabrication_plan("sess-1", _plan()) + + fab.sqs.send_message.assert_not_called() + table.put_item.assert_not_called() From af56ef324eb9bd6c206a6a6f901e791c543e8f2d Mon Sep 17 00:00:00 2001 From: Oliver Gibbs Date: Fri, 18 Sep 2026 01:25:54 +0000 Subject: [PATCH 3/3] test(pytest): cap every test at 60s with pytest-timeout A hung test previously ran for an hour and grew mock call history to 63 GB before oomd killed the session. Pin pytest-timeout in both dev requirement files and set timeout = 60 in the root pytest.ini so any future infinite loop fails fast. --- arbiter/requirements-dev.txt | 1 + pytest.ini | 5 +++++ service/agent_intake_single/requirements-dev.txt | 1 + 3 files changed, 7 insertions(+) diff --git a/arbiter/requirements-dev.txt b/arbiter/requirements-dev.txt index 9a87eb3b..f42425a3 100644 --- a/arbiter/requirements-dev.txt +++ b/arbiter/requirements-dev.txt @@ -8,3 +8,4 @@ hypothesis==6.161.5 # Python, integrates directly with the existing pytest suite). Version resolved # from PyPI (latest, no advisories), not hand-written. moto[dynamodb]==5.2.3 +pytest-timeout==2.4.0 \ No newline at end of file diff --git a/pytest.ini b/pytest.ini index e7924473..c8dc3f43 100644 --- a/pytest.ini +++ b/pytest.ini @@ -10,6 +10,11 @@ # it via addopts so every invocation inherits it automatically. addopts = --import-mode=importlib +# Suite-wide per-test wall-clock cap (pytest-timeout). Any test that hangs +# (e.g. an infinite pagination loop over a truthy MagicMock) fails in a +# minute instead of exhausting machine memory via mock call history. +timeout = 60 + # Pin rootdir to the repo root so all arbiter/ and backend/ test discovery # anchors consistently. # diff --git a/service/agent_intake_single/requirements-dev.txt b/service/agent_intake_single/requirements-dev.txt index e046cfce..8af8592e 100644 --- a/service/agent_intake_single/requirements-dev.txt +++ b/service/agent_intake_single/requirements-dev.txt @@ -1,2 +1,3 @@ pytest==9.1.1 hypothesis==6.161.5 +pytest-timeout==2.4.0 \ No newline at end of file