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_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..5edeaa3d --- /dev/null +++ b/arbiter/fabricator/__tests__/test_process_event_org_tenancy_gate.py @@ -0,0 +1,280 @@ +""" +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, org_id_key="org_id"): + 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_key] = 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() + + +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 61618fb0..c061a6b6 100644 --- a/arbiter/fabricator/index.py +++ b/arbiter/fabricator/index.py @@ -2114,10 +2114,36 @@ 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. - org_id = event.get("org_id") or "" + # 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). + # + # 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 " + "(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/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/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/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 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/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() 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"},