diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6bda2b307e..8294325837 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -237,6 +237,14 @@ jobs: # silently. SYNC with pre-commit hook 'projection-dlq-path'. - name: Projection-DLQ path gate run: uv run python scripts/ci/check_projection_dlq_path.py + # OMN-16690: a db_io contract's declared table `access` must cover the + # operations its handler performs. A handler that reads for idempotency + # under `access: write` is refused fail-closed by the runtime and every + # event is quarantined while the caller still sees a 202 — invisible to + # isolation tests, which inject fakes with no access enforcement. SYNC + # with pre-commit hook 'projection-contract-access'. + - name: Projection contract-access gate + run: uv run python scripts/ci/check_projection_contract_access.py # OMN-14285 (WI-1 / S1d): exactly one OCC companion producer. Fails if a # second writer surface (inline receipt/contract template outside the # occ_evidence_stamp seam, duplicate OccCompanionEmitter, or a resurrected diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 2015027c2b..115b76efcc 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -532,6 +532,25 @@ repos: pass_filenames: false stages: [pre-commit] + # OMN-16690: a db_io contract's declared table `access` must cover the + # operations its handler actually performs. Thirteen nodes declared + # `access: write` on a table their handler queries for idempotency; the + # runtime refuses the read fail-closed and quarantines every event while + # the gateway still answers 202 — zero rows, no visible error. Isolation + # tests could not catch it (they inject fakes with no access + # enforcement), so detection has to be static. Scoped to ANY node + # contract or handler, not just node_projection_*: the two live + # instances were node_hook_event_capture and + # node_renderer_capability_projection, neither of which matches that + # prefix. SYNC with ci.yml step "Projection contract-access gate". + - id: projection-contract-access + name: Projection contract-access gate (OMN-16690) + language: system + entry: uv run python scripts/ci/check_projection_contract_access.py + files: ^(src/omnimarket/nodes/.*/(contract\.yaml|.*\.py)|scripts/ci/check_projection_contract_access\.py)$ + pass_filenames: false + stages: [pre-commit] + # OMN-14768 / F-11: renaming an OPEN PR's head branch closes the PR. Scan # the whole merge-controller tree whenever any of its files (or the gate # itself) changes. SYNC with the ci.yml `lint` job step diff --git a/docs/evidence/OMN-15423-relation-inventory.json b/docs/evidence/OMN-15423-relation-inventory.json index ea1660aee8..1119045982 100644 --- a/docs/evidence/OMN-15423-relation-inventory.json +++ b/docs/evidence/OMN-15423-relation-inventory.json @@ -1389,6 +1389,7 @@ "readers": [ "node_data_flow_sweep", "node_database_sweep", + "node_projection_routing_decision", "node_retention_cleanup", "node_session_orchestrator" ], @@ -1861,7 +1862,8 @@ "partitioning": [], "producer": "node_projection_capsule_store", "readers": [ - "node_capsule_effectiveness_feedback_reducer" + "node_capsule_effectiveness_feedback_reducer", + "node_projection_capsule_store" ], "target_schema": "omninode_internal", "writers": [ @@ -2079,6 +2081,7 @@ "partitioning": [], "producer": "node_projection_delegation", "readers": [ + "node_projection_delegation", "node_projection_delegation_inference_response" ], "target_schema": "tenant", @@ -2151,6 +2154,7 @@ "node_delegate_skill_orchestrator", "node_projection_baselines", "node_projection_cost_by_repo", + "node_projection_delegation", "node_projection_delegation_inference_response", "node_projection_live_events", "node_projection_pattern_learning", @@ -2531,7 +2535,9 @@ "owner_declaration": "node_projection_event_chain", "partitioning": [], "producer": "node_projection_event_chain", - "readers": [], + "readers": [ + "node_projection_event_chain" + ], "target_schema": "omninode_internal", "writers": [ "node_projection_event_chain" @@ -2691,7 +2697,9 @@ "owner_declaration": "node_evidence_dashboard_reducer", "partitioning": [], "producer": "node_evidence_dashboard_reducer", - "readers": [], + "readers": [ + "node_evidence_dashboard_reducer" + ], "target_schema": "omninode_internal", "writers": [ "node_evidence_dashboard_reducer" @@ -2913,7 +2921,9 @@ "owner_declaration": "node_hook_event_capture", "partitioning": [], "producer": "node_hook_event_capture", - "readers": [], + "readers": [ + "node_hook_event_capture" + ], "target_schema": "public", "writers": [ "node_hook_event_capture" @@ -3282,7 +3292,9 @@ "owner_declaration": "node_llm_delegation_projection", "partitioning": [], "producer": "node_llm_delegation_projection", - "readers": [], + "readers": [ + "node_llm_delegation_projection" + ], "target_schema": "omninode_internal", "writers": [ "node_llm_delegation_projection" @@ -3729,6 +3741,7 @@ "node_projection_delegation", "node_projection_live_events", "node_projection_pattern_learning", + "node_projection_registration", "node_projection_routing_decision", "node_projection_savings", "node_projection_session_outcome" @@ -3960,6 +3973,7 @@ "producer": "node_projection_pattern_learning", "readers": [ "node_data_flow_sweep", + "node_projection_pattern_learning", "node_session_orchestrator" ], "target_schema": "tenant", @@ -4288,7 +4302,9 @@ "owner_declaration": "node_renderer_capability_projection", "partitioning": [], "producer": "node_renderer_capability_projection", - "readers": [], + "readers": [ + "node_renderer_capability_projection" + ], "target_schema": "omninode_internal", "writers": [ "node_renderer_capability_projection" @@ -4340,7 +4356,9 @@ "owner_declaration": "node_projection_sandbox_decisions", "partitioning": [], "producer": "node_projection_sandbox_decisions", - "readers": [], + "readers": [ + "node_projection_sandbox_decisions" + ], "target_schema": "omninode_internal", "writers": [ "node_projection_sandbox_decisions" @@ -4827,7 +4845,8 @@ "readers": [ "node_build_loop_orchestrator", "node_generation_consumer", - "node_occ_companion_compute" + "node_occ_companion_compute", + "node_projection_traces" ], "target_schema": "omninode_internal", "writers": [ @@ -4880,7 +4899,9 @@ "owner_declaration": "node_projection_voice_sessions", "partitioning": [], "producer": "node_projection_voice_sessions", - "readers": [], + "readers": [ + "node_projection_voice_sessions" + ], "target_schema": "omninode_internal", "writers": [ "node_projection_voice_sessions" diff --git a/scripts/ci/check_projection_contract_access.py b/scripts/ci/check_projection_contract_access.py new file mode 100644 index 0000000000..c5885e8fcb --- /dev/null +++ b/scripts/ci/check_projection_contract_access.py @@ -0,0 +1,246 @@ +#!/usr/bin/env python3 +# SPDX-FileCopyrightText: 2025 OmniNode.ai Inc. +# SPDX-License-Identifier: MIT +"""CI gate: a contract's declared table ``access`` must cover what its handler does. + +OMN-16690. A ``db_io`` contract declares, per table, an ``access`` capability +(``read`` / ``write`` / ``read_write``). The runtime enforces that declaration +fail-closed at the operation seam -- +``omnibase_infra.runtime.auto_wiring.handler_wiring.ProjectionTableOperation`` +raises ``PermissionError`` from ``_assert_read_declared`` / +``_assert_write_declared`` when the handler asks for a capability the contract +did not declare. + +The defect class this gate closes: a projection handler that performs an +**idempotency read before writing** (``db.query(TABLE, ...)`` then +``db.upsert(TABLE, ...)``) under a contract that declares only ``access: +write``. Every such event is refused at the read, the projection arm's generic +``except Exception`` routes the envelope to the platform quarantine sink +(``onex.dlq.omnibase-infra.quarantine.v1``), the offset commits, consumer lag +reads 0 -- and **zero rows are ever written**. The gateway still answers 202, +so the caller sees success. It is a silent black hole: the contract promised a +capability strictly narrower than the one the code is built to use, so the code +path could never have written a row. + +Doctrine: the contract must declare what the handler actually does. The fix for +a violation is to WIDEN the declaration to ``read_write`` (or to remove the +read from the handler if the read is genuinely unnecessary) -- never to weaken +the runtime guard, which is the only thing standing between an undeclared +capability and production. + +WHY A STATIC GATE AND NOT JUST A TEST. The live failure is invisible to +isolation tests: every projection unit test in this repo injects a *fake* +adapter that implements ``upsert``/``query`` without the access enforcement the +real ``ProjectionDatabaseOperations`` applies. So the handler's read passes in +CI and refuses in production. Detection has to be static, over the contract and +the handler source, or it does not fire at all (Rule 5 -- detection that is not +a pre-merge gate gets ignored). + +SCOPE. Every ``src/omnimarket/nodes/*/contract.yaml`` declaring +``db_io.db_tables``. Production handler modules only -- ``tests/`` and +``test_*.py`` are excluded, because a test's ``db.query(...)`` against its own +fake adapter says nothing about the handler's runtime capability needs (and +several nodes deliberately assert *no* read happens). + +ESCAPE HATCH. A handler line carrying ``# projection-access-ok: `` +is skipped. Reserved for a call the resolver mis-attributes (e.g. a table name +built at runtime); never for an actual undeclared capability. + +Exit codes: 0 = clean; 1 = a declaration does not cover the handler's +operations; 2 = invocation error (run from repo root). +""" + +from __future__ import annotations + +import pathlib +import re +import sys +from dataclasses import dataclass + +import yaml + +_NODES_GLOB = "src/omnimarket/nodes/*/contract.yaml" + +_READ_OK = {"read", "read_write"} +_WRITE_OK = {"write", "read_write"} + +_ALLOW_MARKER = "# projection-access-ok:" + +# Module-level ``NAME = "literal"`` (optionally annotated). Resolved PER FILE: +# several nodes ship multiple handler modules that each define their own +# ``TABLE``, so a repo- or node-wide constant map silently mis-attributes a +# read to the wrong table (and hides real violations). +_CONST_RE = re.compile( + r'^([A-Za-z_][A-Za-z_0-9]*)\s*(?::[^=\n]+)?=\s*(["\'])([^"\']+)\2', re.MULTILINE +) + +# ``.query(FIRST_ARG`` / ``.upsert(FIRST_ARG`` -- first positional argument is +# the table name in the ``DatabaseAdapter`` protocol +# (``upsert(table, conflict_key, row)`` / ``query(table, filters)``). +_CALL_RE_TMPL = r'\.{op}\(\s*\n?\s*([A-Za-z_0-9"\'.]+)' + + +@dataclass(frozen=True) +class Violation: + node: str + table: str + declared: str + operation: str + sites: tuple[str, ...] + + def render(self) -> str: + where = ", ".join(self.sites) + needed = "read_write" if self.operation == "read" else "write" + return ( + f"{self.node}: table {self.table!r} declares access={self.declared!r} " + f"but the handler performs a {self.operation.upper()} at {where}. " + f"The runtime refuses this with PermissionError and quarantines every " + f"event. Declare access: {needed} in contract.yaml (or drop the " + f"{self.operation})." + ) + + +def _resolve_constants(src: str) -> dict[str, str]: + return {m.group(1): m.group(3) for m in _CONST_RE.finditer(src)} + + +def _handler_sources(node_dir: pathlib.Path) -> list[pathlib.Path]: + """Production modules only: a test's fake-adapter call proves nothing.""" + return sorted( + p + for p in node_dir.rglob("*.py") + if "tests" not in p.parts and not p.name.startswith("test_") + ) + + +def _calls(src: str, op: str, rel: str) -> dict[str, list[str]]: + """Map resolved table name -> call sites for one operation in one file.""" + consts = _resolve_constants(src) + found: dict[str, list[str]] = {} + for match in re.finditer(_CALL_RE_TMPL.format(op=op), src): + line_no = src.count("\n", 0, match.start()) + 1 + line = src.splitlines()[line_no - 1] if line_no <= len(src.splitlines()) else "" + if _ALLOW_MARKER in line: + continue + arg = match.group(1).strip() + if arg[:1] in {'"', "'"}: + table = arg.strip("\"'") + elif arg in consts: + table = consts[arg] + elif "." in arg and arg.rsplit(".", 1)[-1] in consts: + table = consts[arg.rsplit(".", 1)[-1]] + else: + # Unresolvable first argument. Recorded under a sentinel so a + # write-only table in the same node is reported rather than + # silently passed -- fail closed, not open. + table = f"" + found.setdefault(table, []).append(f"{rel}:{line_no}") + return found + + +def scan(repo_root: pathlib.Path) -> list[Violation]: + """Return every contract declaration narrower than its handler's usage.""" + violations: list[Violation] = [] + for contract_path in sorted(repo_root.glob(_NODES_GLOB)): + node_dir = contract_path.parent + try: + raw = yaml.safe_load(contract_path.read_text(encoding="utf-8")) + except yaml.YAMLError as exc: # pragma: no cover - malformed contract + raise SystemExit(f"ERROR: cannot parse {contract_path}: {exc}") from exc + if not isinstance(raw, dict): + continue + db_io = raw.get("db_io") or {} + tables = db_io.get("db_tables") or [] + if not tables: + continue + declared: dict[str, str] = { + str(t["name"]): str(t.get("access")) + for t in tables + if isinstance(t, dict) and t.get("name") + } + + reads: dict[str, list[str]] = {} + writes: dict[str, list[str]] = {} + for module in _handler_sources(node_dir): + src = module.read_text(encoding="utf-8") + rel = str(module.relative_to(repo_root)) + for table, sites in _calls(src, "query", rel).items(): + reads.setdefault(table, []).extend(sites) + for table, sites in _calls(src, "upsert", rel).items(): + writes.setdefault(table, []).extend(sites) + + unresolved_reads = [t for t in reads if t.startswith(" int: + repo_root = pathlib.Path.cwd() + if not (repo_root / "src" / "omnimarket" / "nodes").is_dir(): + print( + "ERROR: run from repo root (src/omnimarket/nodes not found)", + file=sys.stderr, + ) + return 2 + + violations = scan(repo_root) + if violations: + print( + "Projection contract-access gate FAILED (OMN-16690) — a db_io " + "contract declares an access capability narrower than the one its " + "handler uses. The runtime enforces the declaration fail-closed, so " + "every event on these paths is refused and quarantined while the " + "caller still sees a 202:", + file=sys.stderr, + ) + for violation in violations: + print(f" - {violation.render()}", file=sys.stderr) + print( + "\nFix the CONTRACT (declare what the handler does) — never weaken " + "the runtime guard.", + file=sys.stderr, + ) + return 1 + + print( + "Projection contract-access gate OK: every db_io declaration covers its " + "handler's operations." + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/src/omnimarket/nodes/node_evidence_dashboard_reducer/contract.yaml b/src/omnimarket/nodes/node_evidence_dashboard_reducer/contract.yaml index b3de357ab9..5f42f1b907 100644 --- a/src/omnimarket/nodes/node_evidence_dashboard_reducer/contract.yaml +++ b/src/omnimarket/nodes/node_evidence_dashboard_reducer/contract.yaml @@ -63,7 +63,12 @@ db_io: database_ref: application schema: omninode_internal migration: "0001_create_evidence_dashboard_projection_tables.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: readiness_aggregate event_bus: diff --git a/src/omnimarket/nodes/node_hook_event_capture/contract.yaml b/src/omnimarket/nodes/node_hook_event_capture/contract.yaml index 80ffaa2063..431df2ff13 100644 --- a/src/omnimarket/nodes/node_hook_event_capture/contract.yaml +++ b/src/omnimarket/nodes/node_hook_event_capture/contract.yaml @@ -103,7 +103,12 @@ db_io: # cutover physically moves this table's family into `tenant` for real. schema: public migration: "0001_create_hook_events.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: hook_events event_bus: diff --git a/src/omnimarket/nodes/node_llm_delegation_projection/contract.yaml b/src/omnimarket/nodes/node_llm_delegation_projection/contract.yaml index 0d6eab8410..4594539149 100644 --- a/src/omnimarket/nodes/node_llm_delegation_projection/contract.yaml +++ b/src/omnimarket/nodes/node_llm_delegation_projection/contract.yaml @@ -41,7 +41,12 @@ db_io: database_ref: application schema: omninode_internal migration: "0001_create_llm_delegation_daily_projection.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: projection reducer_contract: diff --git a/src/omnimarket/nodes/node_projection_capsule_store/contract.yaml b/src/omnimarket/nodes/node_projection_capsule_store/contract.yaml index 5a2cace224..8fbd610ba3 100644 --- a/src/omnimarket/nodes/node_projection_capsule_store/contract.yaml +++ b/src/omnimarket/nodes/node_projection_capsule_store/contract.yaml @@ -42,7 +42,12 @@ db_io: database_ref: application schema: omninode_internal migration: "078_create_capsule_store.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: capsules event_bus: diff --git a/src/omnimarket/nodes/node_projection_delegation/contract.yaml b/src/omnimarket/nodes/node_projection_delegation/contract.yaml index 08e0615b4d..16d02d8be1 100644 --- a/src/omnimarket/nodes/node_projection_delegation/contract.yaml +++ b/src/omnimarket/nodes/node_projection_delegation/contract.yaml @@ -55,7 +55,12 @@ db_io: database_ref: application schema: tenant migration: "0020_delegation_context_pack_hash.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: events - name: delegation_shadow_comparisons database_ref: application @@ -92,7 +97,12 @@ db_io: database_ref: application schema: tenant migration: "0019_delegation_budget_state.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: budget_state event_bus: diff --git a/src/omnimarket/nodes/node_projection_event_chain/contract.yaml b/src/omnimarket/nodes/node_projection_event_chain/contract.yaml index be07637d7d..2402ac4bc1 100644 --- a/src/omnimarket/nodes/node_projection_event_chain/contract.yaml +++ b/src/omnimarket/nodes/node_projection_event_chain/contract.yaml @@ -56,7 +56,12 @@ db_io: database_ref: application schema: omninode_internal migration: "0001_create_event_chain.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: event_chain projection_api: diff --git a/src/omnimarket/nodes/node_projection_pattern_learning/contract.yaml b/src/omnimarket/nodes/node_projection_pattern_learning/contract.yaml index 00b3350cc2..73ba1cb03d 100644 --- a/src/omnimarket/nodes/node_projection_pattern_learning/contract.yaml +++ b/src/omnimarket/nodes/node_projection_pattern_learning/contract.yaml @@ -51,7 +51,12 @@ db_io: # are the house tenant's rows. ADR-0027 classification, not a rename. schema: tenant migration: "0000_create_pattern_learning_artifacts.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: artifacts event_bus: diff --git a/src/omnimarket/nodes/node_projection_registration/contract.yaml b/src/omnimarket/nodes/node_projection_registration/contract.yaml index 60d53a6462..1d7223c92f 100644 --- a/src/omnimarket/nodes/node_projection_registration/contract.yaml +++ b/src/omnimarket/nodes/node_projection_registration/contract.yaml @@ -62,7 +62,12 @@ db_io: database_ref: application schema: omninode_internal migration: "0000_create_node_service_registry.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: registry - name: projection_watermarks database_ref: application diff --git a/src/omnimarket/nodes/node_projection_routing_decision/contract.yaml b/src/omnimarket/nodes/node_projection_routing_decision/contract.yaml index 4b479a9e25..44d9ae1118 100644 --- a/src/omnimarket/nodes/node_projection_routing_decision/contract.yaml +++ b/src/omnimarket/nodes/node_projection_routing_decision/contract.yaml @@ -60,7 +60,12 @@ db_io: # are the house tenant's rows. ADR-0027 classification, not a rename. schema: tenant migration: "0021_create_agent_routing_decisions.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write # OMN-13150 (FIX-1): canonical DB home is the application projection # database, with the unqualified migration/handler SQL and projection API # all resolving the physical public.agent_routing_decisions relation. diff --git a/src/omnimarket/nodes/node_projection_sandbox_decisions/contract.yaml b/src/omnimarket/nodes/node_projection_sandbox_decisions/contract.yaml index 1070e5ac2c..667d39ae65 100644 --- a/src/omnimarket/nodes/node_projection_sandbox_decisions/contract.yaml +++ b/src/omnimarket/nodes/node_projection_sandbox_decisions/contract.yaml @@ -53,7 +53,12 @@ db_io: database_ref: application schema: omninode_internal migration: "0001_create_sandbox_decisions.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: decisions event_bus: diff --git a/src/omnimarket/nodes/node_projection_traces/contract.yaml b/src/omnimarket/nodes/node_projection_traces/contract.yaml index 6671a74a82..d96b005b89 100644 --- a/src/omnimarket/nodes/node_projection_traces/contract.yaml +++ b/src/omnimarket/nodes/node_projection_traces/contract.yaml @@ -50,7 +50,12 @@ db_io: database_ref: application schema: omninode_internal migration: "0001_create_traces.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: traces projection_api: diff --git a/src/omnimarket/nodes/node_projection_voice_sessions/contract.yaml b/src/omnimarket/nodes/node_projection_voice_sessions/contract.yaml index 74efb4d466..69c7b777f0 100644 --- a/src/omnimarket/nodes/node_projection_voice_sessions/contract.yaml +++ b/src/omnimarket/nodes/node_projection_voice_sessions/contract.yaml @@ -56,7 +56,12 @@ db_io: database_ref: application schema: omninode_internal migration: "0001_create_voice_sessions.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: voice_sessions event_bus: diff --git a/src/omnimarket/nodes/node_renderer_capability_projection/contract.yaml b/src/omnimarket/nodes/node_renderer_capability_projection/contract.yaml index fd5f2f3c39..53d7d603ff 100644 --- a/src/omnimarket/nodes/node_renderer_capability_projection/contract.yaml +++ b/src/omnimarket/nodes/node_renderer_capability_projection/contract.yaml @@ -114,7 +114,12 @@ db_io: database_ref: application schema: omninode_internal migration: "0001_create_renderer_capability_projection.sql" - access: write + # OMN-16690: this handler QUERIES this table (idempotency / lookup) + # before writing. `access: write` is refused fail-closed at the + # runtime read seam (ProjectionTableOperation._assert_read_declared), + # which quarantines every event while the caller still sees a 202. + # The declaration must cover what the handler actually does. + access: read_write role: projection descriptor: diff --git a/tests/test_event_chain_projection_omn13620.py b/tests/test_event_chain_projection_omn13620.py index 874f302ce4..cbdc656cec 100644 --- a/tests/test_event_chain_projection_omn13620.py +++ b/tests/test_event_chain_projection_omn13620.py @@ -112,7 +112,14 @@ def test_contract_declares_db_io_write_so_runtime_wires_materialization() -> Non tables = [t for t in db_io["db_tables"] if t["name"] == TABLE] assert len(tables) == 1, "db_io must declare the event_chain table" table = tables[0] - assert table["access"] == "write" + # OMN-16690: the access class widened to "read_write" because the + # projection genuinely reads this table as well as writing it. What this + # test pins is the WRITE-side capability the materialization path needs; + # runtime auto-wiring keys off db_io.db_tables being non-empty + # (handler_wiring._contract_declares_db_io) and never inspects the access + # class, so "read_write" wires identically. Pinning the bare literal + # "write" would forbid a strictly more accurate declaration. + assert table["access"] in {"write", "read_write"} assert table["database_ref"] == "application" assert table["schema"] == "omninode_internal" assert table["migration"] == "0001_create_event_chain.sql" diff --git a/tests/test_omn16690_projection_contract_access.py b/tests/test_omn16690_projection_contract_access.py new file mode 100644 index 0000000000..15b27a199f --- /dev/null +++ b/tests/test_omn16690_projection_contract_access.py @@ -0,0 +1,485 @@ +# SPDX-License-Identifier: MIT +"""Projection contract ``access`` must cover the handler's operations (OMN-16690). + +THE DEFECT. A ``db_io`` contract declares a per-table ``access`` capability. +The runtime enforces it fail-closed at +``omnibase_infra.runtime.auto_wiring.handler_wiring.ProjectionTableOperation``: +``_assert_read_declared`` admits only ``{"read", "read_write"}``. Thirteen +omnimarket projection nodes declared ``access: write`` on a table their handler +**reads** — an idempotency lookup before the upsert. Every event on those paths +raised ``PermissionError`` at the read, the projection arm's generic +``except Exception`` routed the envelope to the platform quarantine sink, the +offset committed, and **zero rows were ever written** while the gateway kept +answering 202. + +WHY THE EXISTING TESTS ALL PASSED (the real-dispatch-path lesson). Every +projection test in this repo injects a hand-written fake adapter that +implements ``upsert``/``query`` as plain dict operations — with **no access +enforcement at all**. ``test_omn16090_real_dispatch_path.py`` drives the real +``_make_projection_dispatch_callback`` and still passed against the broken +contract, because its ``_FakeAdapter`` never consulted ``table.access``. The +enforcement that fires in production was simply absent from the harness. So +this file does the one thing that reproduces the live failure: it builds the +**real** ``ProjectionTableOperation`` over the **real** contract-declared +``ModelDbTableDeclaration``, and fakes only the psycopg2 SQL execution +underneath it. Capability enforcement is production code here, not a stub. + +RED-before / GREEN-after (captured 2026-08-27 on `dev` @ fecf14bb, before the +contract fix in this PR):: + + FAILED test_real_dispatch_path_does_not_quarantine_the_event + E PermissionError: public.hook_events declares access='write'; read refused + + FAILED test_no_projection_contract_declares_less_than_its_handler_uses + E AssertionError: 14 projection table declaration(s) narrower than handler usage + +Matching the live pod log this ticket was opened from +(``omninode-runtime-effects-849bcb486-6sftf``, ns ``onex-dev``). +""" + +from __future__ import annotations + +import asyncio +import pathlib +import sys +from typing import Any +from unittest.mock import MagicMock + +import pytest +import yaml +from omnibase_core.enums.enum_database_schema_domain import EnumDatabaseSchemaDomain +from omnibase_core.models.contracts.subcontracts.model_db_table_declaration import ( + ModelDbTableDeclaration, +) +from omnibase_infra.runtime.auto_wiring import handler_wiring +from omnibase_infra.runtime.auto_wiring.handler_wiring import ( + ProjectionDatabaseBindingTarget, + ProjectionDatabaseTarget, + ProjectionTableOperation, + ProjectionTableTarget, + _make_projection_dispatch_callback, +) + +from omnimarket.nodes.node_hook_event_capture.handlers.handler_hook_event_capture import ( + TABLE, + HandlerHookEventCapture, +) + +pytestmark = pytest.mark.unit + +REPO_ROOT = pathlib.Path(__file__).resolve().parents[1] +NODES_DIR = REPO_ROOT / "src" / "omnimarket" / "nodes" +NODE_DIR = NODES_DIR / "node_hook_event_capture" + +COMMAND_TOPIC = "onex.cmd.omnimarket.hook-event-capture-requested.v1" +_DSN_ENV = "OMNIMARKET_TEST_HOOK_EVENTS_DSN" +_PATCH_BUILD_ADAPTER = ( + "omnibase_infra.runtime.auto_wiring.handler_wiring._build_projection_db_adapter" +) + +# The gate module is the single detection implementation, shared by the CI gate, +# the pre-commit hook and these tests, so the three can never disagree. +sys.path.insert(0, str(REPO_ROOT / "scripts" / "ci")) +from check_projection_contract_access import ( # noqa: E402 + Violation, + scan, +) + + +# -------------------------------------------------------------------------- +# The access-enforcing adapter: REAL enforcement, faked SQL. +# -------------------------------------------------------------------------- +class _SqlRecorder: + """Stands in for psycopg2 execution ONLY — never for capability checks.""" + + def __init__(self) -> None: + self.rows: list[dict[str, Any]] = [] + + def _execute_upsert( + self, + target: ProjectionTableTarget, + conflict_key: str, + row: dict[str, object], + *, + tenant_context: object | None, + ) -> bool: + self.rows.append(dict(row)) + return True + + def _execute_query( + self, + target: ProjectionTableTarget, + filters: dict[str, object] | None, + *, + tenant_context: object | None, + ) -> list[dict[str, object]]: + applied = filters or {} + return [ + dict(row) + for row in self.rows + if all(row.get(key) == value for key, value in applied.items()) + ] + + +class _AccessEnforcingAdapter: + """``DatabaseAdapter`` protocol backed by REAL ``ProjectionTableOperation``. + + This is the whole point of the file. The production adapter routes each + ``query``/``upsert`` through a ``ProjectionTableOperation`` built from the + contract's own ``ModelDbTableDeclaration``, and that operation is what + raises ``PermissionError`` on an undeclared capability. Reusing the real + class here means the declaration under test is enforced by the same code + that enforces it in the pod. + """ + + def __init__(self, table_targets: tuple[ProjectionTableTarget, ...]) -> None: + self.recorder = _SqlRecorder() + self._ops = { + target.table.name: ProjectionTableOperation(self.recorder, target) # type: ignore[arg-type] + for target in table_targets + } + + def upsert(self, table: str, conflict_key: str, row: dict[str, Any]) -> bool: + return self._ops[table].upsert(conflict_key, row) + + def query( + self, table: str, filters: dict[str, Any] | None = None + ) -> list[dict[str, Any]]: + return self._ops[table].query(filters) + + +def _real_table_target() -> ProjectionDatabaseTarget: + """Target built from THIS node's REAL contract-declared table. + + ``access`` is read straight off the shipped ``contract.yaml`` — the field + under test is never overridden, so a regression in the declaration fails + this test rather than being papered over by a fixture. + """ + contract = yaml.safe_load((NODE_DIR / "contract.yaml").read_text(encoding="utf-8")) + declared = contract["db_io"]["db_tables"][0] + table = ModelDbTableDeclaration(**declared) + binding = ProjectionDatabaseBindingTarget( + binding_ref="tenant_projection", + database_ref=table.database_ref, + physical_database="application", + principal="tenant_projection_writer", + dsn_env=_DSN_ENV, + ) + table_target = ProjectionTableTarget( + table=table, + database_ref=table.database_ref, + physical_database="application", + physical_schema=table.schema, + domain=EnumDatabaseSchemaDomain.TENANT, + read_binding=binding, + write_binding=binding, + ) + return ProjectionDatabaseTarget( + tables=(table,), + table_targets=(table_target,), + physical_database="application", + ) + + +def _gateway_wire_payload() -> dict[str, Any]: + """The OMN-16667 canary shape: what the gateway actually puts on the bus.""" + return { + "source": "local_macos_claude_hooks", + "batch_sha": "b" * 64, + "events": [ + { + "event_type": "onex.evt.omniclaude.skill-started.v1", + "event_sha": "a" * 64, + "occurred_at": "2026-08-16T18:00:00Z", + "payload_json": '{"skill_name": "node_dod_verify"}', + } + ], + "correlation_id": "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + "emitted_at": "2026-08-16T18:00:00Z", + "tenant_id": "omninode", + "tenant_principal_id": "t-" + "a" * 32, + } + + +# -------------------------------------------------------------------------- +# AC1 / AC5-shaped: the real dispatch path must not quarantine. +# -------------------------------------------------------------------------- +def test_real_dispatch_path_does_not_quarantine_the_event( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The exact production seam: real callback, real handler, real access guard. + + Drives ``_make_projection_dispatch_callback`` — the function + ``wire_from_manifest`` builds for a ``db_io`` contract — against the real + ``HandlerHookEventCapture`` with an adapter whose capability checks are the + production ``ProjectionTableOperation``. Asserts BOTH halves of the live + symptom are gone: no event routed to the DLQ/quarantine sink, and a row + actually lands. A green 202 with zero rows is precisely the failure this + ticket exists to kill, so "no exception" alone would not be evidence. + """ + monkeypatch.setenv(_DSN_ENV, "postgresql://unused-because-sql-is-faked") + target = _real_table_target() + adapter = _AccessEnforcingAdapter(target.table_targets) + + quarantined: list[str] = [] + + async def _spy_route( + event_bus: object | None, + dlq_topics: list[str], + envelope: object, + handler_name: str, + failure_reason: str, + ) -> bool: + quarantined.append(f"{handler_name}: {failure_reason}") + return True + + callback = _make_projection_dispatch_callback( + HandlerHookEventCapture(), + target, + (COMMAND_TOPIC,), + ) + + envelope = MagicMock() + envelope.topic = COMMAND_TOPIC + envelope.event_type = COMMAND_TOPIC + envelope.payload = _gateway_wire_payload() + + def _inject_adapter(*_args: object, **_kwargs: object) -> _AccessEnforcingAdapter: + return adapter + + monkeypatch.setattr(_PATCH_BUILD_ADAPTER, _inject_adapter) + monkeypatch.setattr( + handler_wiring, "_route_projection_error_to_dlq", _spy_route, raising=True + ) + asyncio.run(callback(envelope)) + + assert not quarantined, ( + "the event was routed to the quarantine/DLQ sink — the projection " + f"handler was refused a declared capability: {quarantined}" + ) + assert len(adapter.recorder.rows) == 1, ( + "no row landed through the REAL dispatch path with REAL access " + "enforcement — a 202 with zero rows is the exact silent black hole " + "OMN-16690 reports" + ) + assert adapter.recorder.rows[0]["event_sha"] == "a" * 64 + + +def test_redelivery_is_idempotent_through_the_enforcing_adapter( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The read the contract now declares is the one that makes replay safe. + + Guards the fix from being "corrected" by deleting the handler's read: a + second delivery of the same batch must not add a row. If someone removes + the idempotency read to satisfy a write-only declaration, this fails. + """ + monkeypatch.setenv(_DSN_ENV, "postgresql://unused-because-sql-is-faked") + target = _real_table_target() + adapter = _AccessEnforcingAdapter(target.table_targets) + + callback = _make_projection_dispatch_callback( + HandlerHookEventCapture(), target, (COMMAND_TOPIC,) + ) + + def _envelope() -> MagicMock: + env = MagicMock() + env.topic = COMMAND_TOPIC + env.event_type = COMMAND_TOPIC + env.payload = _gateway_wire_payload() + return env + + def _inject_adapter(*_args: object, **_kwargs: object) -> _AccessEnforcingAdapter: + return adapter + + monkeypatch.setattr(_PATCH_BUILD_ADAPTER, _inject_adapter) + asyncio.run(callback(_envelope())) + asyncio.run(callback(_envelope())) + + assert len(adapter.recorder.rows) == 1, ( + "redelivery wrote a second row: the contract-declared read is what " + "provides DO-NOTHING semantics the adapter's upsert lacks" + ) + + +def test_write_only_declaration_is_still_refused_at_the_read_seam() -> None: + """The guard stays fail-closed — the fix is the contract, not the guard. + + Pins the runtime behaviour this ticket must NOT change. If a future edit + relaxes ``_assert_read_declared`` to make the symptom disappear, this test + fails and says why that is the wrong fix. + """ + contract = yaml.safe_load((NODE_DIR / "contract.yaml").read_text(encoding="utf-8")) + declared = dict(contract["db_io"]["db_tables"][0]) + declared["access"] = "write" # deliberately narrow, the pre-fix declaration + table = ModelDbTableDeclaration(**declared) + binding = ProjectionDatabaseBindingTarget( + binding_ref="tenant_projection", + database_ref=table.database_ref, + physical_database="application", + principal="tenant_projection_writer", + dsn_env=_DSN_ENV, + ) + target = ProjectionTableTarget( + table=table, + database_ref=table.database_ref, + physical_database="application", + physical_schema=table.schema, + domain=EnumDatabaseSchemaDomain.TENANT, + read_binding=binding, + write_binding=binding, + ) + adapter = _AccessEnforcingAdapter((target,)) + + with pytest.raises(PermissionError, match="read refused"): + adapter.query(TABLE, {"event_sha": "a" * 64}) + + +# -------------------------------------------------------------------------- +# AC2 / AC3: the class-level sweep and the gate that keeps it closed. +# -------------------------------------------------------------------------- +def test_no_projection_contract_declares_less_than_its_handler_uses() -> None: + """AC2: the whole repo, not just the two nodes the ticket named. + + The ticket was filed on two observed instances; the sweep behind this + assertion found 14 mismatched tables across 13 nodes. Driven by the same + ``scan()`` the CI gate runs, so the gate cannot pass while this fails. + """ + violations = scan(REPO_ROOT) + assert not violations, ( + f"{len(violations)} projection table declaration(s) narrower than " + "handler usage — every event on these paths is quarantined:\n" + + "\n".join(f" - {v.render()}" for v in violations) + ) + + +def test_every_projection_contract_declares_a_valid_access_mode() -> None: + """A typo like ``acccess:`` or ``write_read`` must not read as 'no read'.""" + valid = {"read", "write", "read_write"} + bad: list[str] = [] + for contract_path in sorted(NODES_DIR.glob("*/contract.yaml")): + raw = yaml.safe_load(contract_path.read_text(encoding="utf-8")) + if not isinstance(raw, dict): + continue + for table in (raw.get("db_io") or {}).get("db_tables") or []: + access = table.get("access") + if access not in valid: + bad.append( + f"{contract_path.parent.name}/{table.get('name')}: " + f"access={access!r} not in {sorted(valid)}" + ) + assert not bad, "invalid access declaration(s):\n" + "\n".join(bad) + + +def test_gate_detects_a_synthetic_violation(tmp_path: pathlib.Path) -> None: + """The gate is RED-derivable: it fails on a planted defect. + + A gate that cannot fail is not a gate. Plants the exact shape of the live + defect — ``access: write`` plus a handler that queries the same table — in + a throwaway tree and asserts ``scan()`` reports it. + """ + node = tmp_path / "src" / "omnimarket" / "nodes" / "node_projection_synthetic" + (node / "handlers").mkdir(parents=True) + (node / "contract.yaml").write_text( + "db_io:\n" + " db_tables:\n" + " - name: synthetic_rows\n" + " database_ref: application\n" + " schema: public\n" + " access: write\n", + encoding="utf-8", + ) + (node / "handlers" / "handler_synthetic.py").write_text( + 'TABLE = "synthetic_rows"\n' + "def handle(db):\n" + " if db.query(TABLE, {'id': 1}):\n" + " return 0\n" + " return db.upsert(TABLE, 'id', {'id': 1})\n", + encoding="utf-8", + ) + + violations = scan(tmp_path) + + assert len(violations) == 1, f"gate missed the planted defect: {violations}" + found: Violation = violations[0] + assert found.table == "synthetic_rows" + assert found.declared == "write" + assert found.operation == "read" + assert "read_write" in found.render() + + +def test_gate_ignores_reads_that_only_exist_in_tests(tmp_path: pathlib.Path) -> None: + """A test's fake-adapter ``query`` says nothing about runtime capability. + + Several nodes (OMN-15707) deliberately removed the handler read and now + assert in tests that no ``.query()`` happens — those tests call ``query`` + on their own fakes. Counting them would produce false violations and press + contracts to over-declare ``read_write`` they do not need. + """ + node = tmp_path / "src" / "omnimarket" / "nodes" / "node_projection_writeonly" + (node / "handlers").mkdir(parents=True) + (node / "tests").mkdir(parents=True) + (node / "contract.yaml").write_text( + "db_io:\n" + " db_tables:\n" + " - name: writeonly_rows\n" + " database_ref: application\n" + " schema: public\n" + " access: write\n", + encoding="utf-8", + ) + (node / "handlers" / "handler_writeonly.py").write_text( + 'TABLE = "writeonly_rows"\n' + "def handle(db):\n" + " return db.upsert(TABLE, 'id', {'id': 1})\n", + encoding="utf-8", + ) + (node / "tests" / "test_writeonly.py").write_text( + 'TABLE = "writeonly_rows"\n' + "def test_no_read(db):\n" + " assert db.query(TABLE) == []\n", + encoding="utf-8", + ) + + assert scan(tmp_path) == [], "a test-only query was miscounted as a handler read" + + +def test_gate_resolves_table_constants_per_file(tmp_path: pathlib.Path) -> None: + """Per-file constant resolution — a node-wide map hides real violations. + + ``node_projection_delegation`` ships several handler modules that each + define their own ``TABLE``. Resolving constants across the whole node let a + later module's ``TABLE`` overwrite an earlier one, which is how + ``delegation_events`` (three read sites) was missed on the first sweep of + this very ticket. This pins the per-file behaviour. + """ + node = tmp_path / "src" / "omnimarket" / "nodes" / "node_projection_multi" + (node / "handlers").mkdir(parents=True) + (node / "contract.yaml").write_text( + "db_io:\n" + " db_tables:\n" + " - name: alpha_rows\n" + " database_ref: application\n" + " schema: public\n" + " access: write\n" + " - name: beta_rows\n" + " database_ref: application\n" + " schema: public\n" + " access: read_write\n", + encoding="utf-8", + ) + (node / "handlers" / "handler_alpha.py").write_text( + 'TABLE = "alpha_rows"\ndef handle(db):\n return db.query(TABLE)\n', + encoding="utf-8", + ) + (node / "handlers" / "handler_beta.py").write_text( + 'TABLE = "beta_rows"\ndef handle(db):\n return db.query(TABLE)\n', + encoding="utf-8", + ) + + violations = scan(tmp_path) + + assert [v.table for v in violations] == ["alpha_rows"], ( + "per-file constant resolution failed: a node-wide TABLE map masks the " + f"alpha_rows read, got {violations}" + ) diff --git a/tests/test_traces_projection_omn13083.py b/tests/test_traces_projection_omn13083.py index 07d7f0b6ea..f5a973bb42 100644 --- a/tests/test_traces_projection_omn13083.py +++ b/tests/test_traces_projection_omn13083.py @@ -180,7 +180,14 @@ def test_contract_declares_db_io_so_runtime_wires_materialization() -> None: traces_tables = [t for t in db_tables if t["name"] == TABLE] assert len(traces_tables) == 1, "db_io must declare the traces table" table = traces_tables[0] - assert table["access"] == "write" + # OMN-16690: the access class widened to "read_write" because the + # projection genuinely reads this table as well as writing it. What this + # test pins is the WRITE-side capability the materialization path needs; + # runtime auto-wiring keys off db_io.db_tables being non-empty + # (handler_wiring._contract_declares_db_io) and never inspects the access + # class, so "read_write" wires identically. Pinning the bare literal + # "write" would forbid a strictly more accurate declaration. + assert table["access"] in {"write", "read_write"} assert table["database_ref"] == "application" assert table["schema"] == "omninode_internal" assert table["migration"] == "0001_create_traces.sql"