Repository navigation
fix(OMN-16690): declare the reads projection contracts already perform + fail-closed access-class gate - #2164
Merged
Conversation
A db_io contract declares a per-table `access` capability. The runtime
enforces it fail-closed at ProjectionTableOperation._assert_read_declared,
which admits only {"read", "read_write"}. Thirteen omnimarket nodes declared
`access: write` on a table their handler QUERIES — 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
onex.dlq.omnibase-infra.quarantine.v1, the offset committed, consumer lag read
0 — and zero rows were ever written, while the gateway kept answering 202.
Contract-first: the declaration must cover what the handler actually does.
This widens `access` to read_write on the 14 tables whose handler reads. It
does NOT weaken the runtime guard and does NOT remove any idempotency read —
both would be the wrong fix. The reads are load-bearing: the injected adapter's
upsert() has no ON CONFLICT ... DO NOTHING mode, so dropping the read would
bump updated_at on every Kafka redelivery.
Swept all 47 omnimarket contracts declaring db_io.db_tables (62 tables). The
ticket named 2 instances; the sweep found 14 across 13 nodes. No other repo in
the registry declares db_io.db_tables — the 91 hits under omnibase_infra are
staged copies of this same tree in its build workspace.
Fixed (node — table):
node_evidence_dashboard_reducer evidence_readiness_aggregate_projection
node_hook_event_capture hook_events
node_llm_delegation_projection llm_delegation_daily_projection
node_projection_capsule_store capsule_store
node_projection_delegation delegation_events, delegation_budget_state
node_projection_event_chain event_chain
node_projection_pattern_learning pattern_learning_artifacts
node_projection_registration node_service_registry
node_projection_routing_decision agent_routing_decisions
node_projection_sandbox_decisions sandbox_decisions
node_projection_traces traces
node_projection_voice_sessions voice_sessions
node_renderer_capability_projection renderer_capability_projection
Enforcement (Rule 5 — detection without a gate gets ignored):
scripts/ci/check_projection_contract_access.py, wired as BOTH a pre-commit
hook (`projection-contract-access`) and a ci.yml step. Scoped to any node
contract/handler, not node_projection_* — neither live instance matched that
prefix.
Why a static gate: this class is invisible to isolation tests. Every
projection test in the repo injects a fake adapter implementing upsert/query
with no access enforcement at all, so the handler's read passes in CI and is
refused in production. test_omn16090_real_dispatch_path.py drives the real
dispatch callback and still passed against the broken contract for exactly
that reason.
Tests (tests/test_omn16690_projection_contract_access.py) close that gap by
building the REAL ProjectionTableOperation over the REAL contract-declared
ModelDbTableDeclaration and faking only psycopg2 execution beneath it:
- real dispatch path asserts the event is NOT quarantined AND a row lands
(a 202 with zero rows is the failure, so "no exception" is not evidence)
- redelivery stays idempotent, pinning the read against being deleted
- a write-only declaration is STILL refused — pins the guard as fail-closed
so the symptom cannot be "fixed" by relaxing it
- class sweep over every contract; gate proven RED-derivable on a planted
defect; test-only queries not miscounted; per-file constant resolution
(a node-wide TABLE map masked delegation_events on the first sweep)
RED-before on dev @ fecf14b with contracts reverted:
PermissionError: public.hook_events declares access='write'; read refused
... routing malformed/erroring event to the platform quarantine sink
onex.dlq.omnibase-infra.quarantine.v1
matching the live pod log (omninode-runtime-effects-849bcb486-6sftf, onex-dev)
verbatim. GREEN after: 8 passed; 171 affected-node unit tests pass.
AC4/AC5 (release into the deployed omninode-runtime image, re-run the
OMN-16667 canary, un-fence hook-event-capture) require a deploy and are NOT
done here — recorded as follow-up on the ticket.
Evidence-Ticket: OMN-16690
…generate the source-derived relation inventory The OMN-16690 contract change widens twelve projection contracts from access: write to access: read_write, declaring the reads those projections already perform. Three tests were pinned to the pre-change shape: * test_event_chain_projection_omn13620 and test_traces_projection_omn13083 asserted the bare literal table["access"] == "write". Their own docstrings state the intent is the WRITE-side capability the materialization path needs. Runtime auto-wiring keys off db_io.db_tables being non-empty (omnibase_infra.runtime.auto_wiring.handler_wiring._contract_declares_db_io, which never inspects the access class), so read_write wires materialization identically -- verified by reading the live installed implementation, not inferred. The literal pin forbade a strictly more accurate declaration; the assertion now requires write capability without forbidding declared reads. * docs/evidence/OMN-15423-relation-inventory.json is generated from the contracts. Regenerated via scripts/generate_application_relation_inventory.py --write; the whole diff is the source-derived consequence of the change -- each projection now correctly appears as a reader of its own table. Verified: 46 passed (the two projection files, the inventory test, and the ticket test file tests/test_omn16690_projection_contract_access.py). OMN-16690
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 130 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
Comment |
✅ Architectural Review — PASSEDErrors: 0 What this checks
No architectural violations found. Static architectural lint — no model inference (OMN-14176). |
✅ Hostile Reviewer — PASSEDBlocking findings (critical/error): 0 Gate semantics
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524) |
jonahgabriel
enabled auto-merge (squash)
August 27, 2026 12:22
jonahgabriel
pushed a commit
to OmniNode-ai/onex_change_control
that referenced
this pull request
Aug 27, 2026
…imarket#2164 (#7287) * evidence(OMN-16690): author OCC companion for OmniNode-ai/omnimarket#2164 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 778e6780b46e0b1579de8366e170f20a325423fa. * evidence(OMN-16690): self-bind OCC#7287 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
jonahgabriel
added a commit
that referenced
this pull request
Sep 2, 2026
With no event_bus.dlq_topics declared, every erroring event from this node fell through to the platform-wide quarantine sink onex.dlq.omnibase-infra.quarantine.v1, shared by every handler on the runtime. Two consequences, both live on onex-dev on 2026-09-01: - Unfindable. That sink held 8,878,926 retained records as of OMN-16769. A per-node failure signal deposited there is not triageable. - Blast radius. The Kafka circuit breaker is keyed on the CONNECTION (kafka.onex-dev). This node's quarantine publishes were failing OutOfOrderSequenceNumber, which opened the shared breaker 108 times in two hours and intermittently broke the gateway attach/heartbeat/detach path on the same pod. The node now names its own sink, so its failures stay attributable to it. The infra-side cut -- a failure-path publish never charges the shared breaker -- lands in the companion omnibase_infra PR under the same ticket. Version bumped 0.4.15 -> 0.4.16: packaged source changed, and the release-identity gate (OMN-16344) refuses to alias two code states under one published wheel version. The test also pins the OMN-16690 declaration (access: read_write on omninode_internal.renderer_capability_projection) that produced the poison event, so the two halves of this chain cannot drift apart. That declaration is ADOPTED from OMN-16690 / #2164 (dev e438c32), not re-landed here. Ticket: OMN-17497 Related: OMN-16690, OMN-13548, OMN-16769, OMN-15957
This was referenced Sep 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Projection contracts that declared a table
access: writewhile their handlersquery()that same table were refused at the guard (_assert_read_declared()admits onlyread/read_write) and every event was routed to the quarantine DLQ. Zero rows were ever written.accesstoread_writeforpublic.hook_events(node_hook_event_capture) andomninode_internal.renderer_capability_projection(node_renderer_capability_projection).scripts/ci/check_projection_contract_access.py, a fail-closed guard that resolves each contract's declared access against the operations its handler actually invokes and fails on mismatch. Wired as both a CI job (.github/workflows/ci.yml) and a pre-commit hook in this same PR, per Rule 5 (detection without enforcement gets ignored).Why
Live evidence, pod
omninode-runtime-effects-849bcb486-6sftf, nsonex-dev:The failure mode is the worst kind: the gateway answers 202, the runtime logs an ERROR, the event lands in quarantine, and the caller sees success.
hook_eventsread backtotal_rows = 0against the real serving DB. The siblingHandlerRendererCapabilityProjectionwas refusing continuously (measured 234x/20min) on the identical root cause, which is why the fix is the class and the guard, not the two instances.Verification
tests/test_omn16690_projection_contract_access.py, RED-first against the pre-fix declarations.is_full_suite=True reason=changed_test_unnarrowable) and ran it on the designated.200host: 16250 passed, 105 skipped, 3 deselected, 1 xfailed, 0 failed in 710.97s.PREPUSH_FULL_SUITE,PREPUSH_ALLOW_LOCAL_FULL_SUITE,ENABLE_SMART_TESTS=offand--no-verifywere all unused. The escalation target itself had to be repaired first (OMN-16752); this push is the proof that the governed path works end to end.AC4 (rebuild into the deployed runtime image) and AC5 (canary showing a real row keyed on
event_sha ce574770…) follow this merge and are tracked on the ticket.OMN-16690
Evidence-Ticket: OMN-16690
Evidence-Source: OCC#7287