Repository navigation
fix(OMN-16798): a record already on a dead-letter topic must not be re-DLQ'd onto itself - #2951
Merged
Merged
Conversation
…e-DLQ'd onto itself Self-inflicted, found by the first post-deploy matrix run and fixed rather than carried. The OMN-16798 totality guards answer an undeliverable record by routing it to the topic's DLQ. On a record CONSUMED FROM a dlq topic that answer is circular: get_dlq_topic_for_original resolves a onex.dlq.* topic back to itself, so the guard republished the record onto the very topic it had just read it from. Measured on the .201 dev lane 2026-08-27T23:49-23:51Z: node_dlq_replay_effect and node_ledger_projection_compute both consume onex.dlq.omnibase-infra.commands.v1; a record neither could route was re-DLQ'd onto that same topic, and 24 of the last 38 records on it carried original_topic: onex.dlq.omnibase-infra.commands.v1 -- records the guard itself had authored. It converged that run (HWM stable at 188, both groups Stable at lag 0), but convergence was luck, not mechanism. A record already on a dead-letter sink is ALREADY durably captured, which is what the guards exist to secure. AC4 -- dispatch, DLQ, or refuse with evidence -- is satisfied by that durable record plus a loud structured log, so both guards now log and stop instead of republishing. The amplification was measured on the NO_DISPATCHER leg; the undeliverable-output leg is guarded by symmetry, and the comment says so rather than implying it was observed. Evidence-Ticket: OMN-16798
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 131 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 (2)
Comment |
Contributor
|
| Verdict | Meaning | Blocks merge? |
|---|---|---|
passed |
No critical findings | No |
blocked |
CRITICAL findings found | Yes |
degraded |
All models unavailable (infra) | No (pilot) |
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review (OMN-8468/OMN-8524)
jonahgabriel
pushed a commit
to OmniNode-ai/onex_change_control
that referenced
this pull request
Aug 28, 2026
…ibase_infra#2951 (#7347) * evidence(OMN-16798): author OCC companion for OmniNode-ai/omnibase_infra#2951 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 43c4139b029ef90f337b1a75e4adc4cadb21de98. * evidence(OMN-16798): self-bind OCC#7347 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
This was referenced Aug 28, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 28, 2026
…chain (#2953) OMN-16690: node_hook_event_capture/contract.yaml declared access='write' while its handler called db.query(). The runtime read seam refused, the projection arm swallowed the PermissionError and DLQ'd the event, callers kept seeing 202, and the quarantine sink reached HWM 8,878,932. It ran about a week behind green CI. The platform testing inventory names why nothing caught it (section 7.1): the golden-chain DB double carries NO capability enforcement, so a contract/handler capability mismatch is invisible to all 194 golden chains BY CONSTRUCTION. That is equally true in THIS repo -- tests/integration/runtime/test_projection_handler_db_injection_integration.py injects a FakeDb through _build_projection_db_adapter whose upsert/query do no access checking, so it is green for every declared access value. omnimarket#2164 closed the statically-visible half (a handler that TEXTUALLY calls db.query() must declare read_write). It cannot see a read reached through a helper, a base class or an adapter, and nothing anywhere proved the RUNTIME refusal is still wired or that a mismatch turns a whole CHAIN red. Where the seam boundary is drawn -------------------------------- _assert_read_declared / _assert_write_declared both fire BEFORE the adapter touches a connection, so the whole capability decision is reachable with zero DB contact. Real here: _prepare_handler_wiring arm selection, _make_projection_dispatch_callback, _build_projection_db_adapter, ProjectionDatabaseOperations domain routing, and ProjectionTableOperation's assertions and SQL construction. Substituted: the psycopg2 DRIVER only, via sys.modules -- the technique test_sync_psycopg2_adapter_preserves_text_array_lists already uses here. Substituting the adapter OBJECT is the fidelity gap being closed; do not "simplify" this file back into that. The matrix, both directions (a one-sided fixture can pass by accident): read_write declared + query -> terminal emitted, SQL executed read_write declared + upsert -> terminal emitted, SQL executed write declared + query -> REFUSED (the OMN-16690 shape) read declared + upsert -> REFUSED (mirror image) Only `access` varies across the four rows. Same handler, same wiring, same wire bytes, same table -- so any divergence is attributable to the capability seam and to nothing else. Each refused row asserts THREE separate facts, because any one alone is weak: no terminal event (the chain does not report success -- the half the live outage got wrong at the HTTP boundary); the event reaches the contract-declared DLQ carrying the PermissionError (bus-observable, not log-only -- log-only detection is what let the live defect run a week); and ZERO SQL was constructed (the refusal precedes statement construction, so it is a gate and not a late check). Two fixture facts that are load-bearing, both found by measurement ----------------------------------------------------------------- 1. The table is projection_watermarks, a REAL table from the shipped local topology. A fabricated name cannot reach the capability seam at all: _require_projection_binding_privileges rejects it three layers earlier, and the only sanctioned grant-synthesis helper refuses for already-shipped tables. A fabricated row would have proven nothing about access. 2. The contract must set terminal_event AND list it in publish_topics (handler_wiring.py:8486-8492) or the arm emits no terminal -- which would make a positive row's observation identical to a refused row's, i.e. vacuous. RED-first proof, and what it corrected -------------------------------------- Stubbing BOTH production assertions to `lambda self: None` does NOT make the PermissionError disappear -- the chain then fails a SECOND, independent refusal ("Projection operation has no declared workload binding"), because a write-declared table has no read binding to resolve either. So an assertion of the form `"PermissionError" in dlq_text` STAYS GREEN with the capability seam switched off and would gate nothing. Asserting the seam's OWN WORDING is what makes these rows non-vacuous. Measured, then recorded at the assertion: neutered -> 2 failed, 3 passed (both refused rows RED, on that line only) restored -> 9 passed (whole tests/integration/chains/ suite) AC3 also anchors by importing ProjectionTableOperation and asserting both refusal methods exist on the production class. omnimarket's node_projection_live_events/tests/test_projection_live_events.py:38 re-implements `access not in {"read","read_write"}` and asserts its own copy -- it was green throughout the outage, because a mirror of a rule cannot tell you the rule is still wired. Enforcement, not detection (rule 5): no workflow edit. The Event Chain Gate job already runs `uv run pytest tests/integration/chains/` wholesale and is registered in ci_summary_gate.py::STRICT_GATE_JOBS, so a skipped/absent conclusion fails CI Summary closed. NO runtime behavior change. In particular this does NOT implement inventory section 7.2 (a PermissionError from the access seam is a CONTRACT defect and should fail the consumer loudly rather than DLQ every event forever). That finding is correct; the quarantine router is owned by OMN-16798 (#2949, #2951, landed 2026-08-27), so this suite PINS the current behavior instead, making that change a visible diff when someone makes it rather than a silent one. Evidence: uv run pytest tests/integration/chains/ -q -> 9 passed Evidence-Ticket: OMN-16814
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 28, 2026
… dead-chain report misread (#2952) * test(OMN-16813): gate the in-process intent-execution arm — the leg OMN-16755 read as dead The OMN-16774 Event Chain Gate asserts exactly two things: a terminal event lands, and nothing reaches the quarantine sink. Both are statements about DispatchResultApplier's Phase 2 (output-event publish). Neither says anything about Phase 1 -- intent execution -- or about the IntentExecutor behind it. That hole has already cost a day. Re-probed live on the .201 dev lane 2026-08-27, the gateway link-health chain -- recorded as a DEAD CHAIN by a High-priority ticket (OMN-16755), a status doc, and the platform testing inventory (verdict row 7, "the canonical wired-but-never-fires shape") -- delivers its 17,717 upserts entirely in-process: handler -> DispatchResultApplier -> IntentExecutor -> intent effect never over the bus. The chain was alive; every observer was blind, and blind in the same direction, because the only leg any of them watches is the one this chain does not use. Why this cannot be a row in CHAIN_CASES --------------------------------------- handler_wiring._normalize_handler_result routes a bare BaseModel return to output_EVENTS; only ModelHandlerOutput.intents (or a ModelIntent in .result) reaches output_intents. Every existing gate row returns a bare typed model, so no number of appended rows reaches this leg. It needs an assertion aimed at the effect side of the seam, which is what this module adds. Every hop is production: EventBusInmemory, raw JSON wire bytes, EventBusSubcontractWiring's real consumer + deserializer, _prepare_handler_wiring arm selection, MessageDispatchEngine, the handler, DispatchResultApplier.apply, IntentExecutor.execute_all, and a real ProtocolIntentEffect. The single patched seam is _import_handler_class, exactly as the OMN-16774 gate patches it. Zero infrastructure: no broker, no database, no container, no lane. Negative control, and what it measured -------------------------------------- test_a_dead_intent_leg_is_invisible_on_every_topic_the_gate_watches runs the identical chain twice, differing only in whether the executor carries a handler for the intent_type. Measured, not assumed: - terminal topic ............. EMPTY in both runs - quarantine sink ............ EMPTY in both runs (the ONLY sink OMN-16774 watches) - boundary DLQ ............... EMPTY in both runs The refusal IS raised -- IntentExecutor names the unroutable intent_type -- but EventBusInmemory exposes no _publish_raw_to_dlq and a RuntimeHostError is classified retryable, not exhausted, on first delivery, so the exception is logged as "Subscriber callback failed" and never reaches the publisher. The initial draft of this suite asserted pytest.raises around bus.publish and was WRONG for exactly that reason; the assertions were rewritten against what the transport actually does rather than what it was assumed to do. So on this leg, delivered work and bus-observable state are not the same measurement, and the effect_calls assertion is the only one in this repo that can go red when the intent arm breaks. That is what makes it non-vacuous. RED-first proof (not a claim -- run and captured): register_effect default flipped to False, positive test re-run -> E AssertionError: the intent effect was called 0 time(s), expected exactly 1. 1 failed. Reverted; 4 passed. Two further refusals are pinned so they cannot regress into silent commits: an unregistered intent_type, and an applier built with no executor at all (which must refuse in Phase 1 rather than fall through and publish a terminal event advertising a write that never happened -- a dead leg that reports success). AC3: IntentExecutor.execute answers a None payload with a WARNING and a bare return, no raise. That branch is UNREACHABLE from validated data -- ModelIntent.payload is required and pydantic rejects None -- and that refusal is now asserted rather than assumed, so the guarantee cannot be relaxed by a model edit without going red. The branch stays as defence-in-depth. Enforcement, not detection (rule 5): no workflow edit. The Event Chain Gate job already runs `uv run pytest tests/integration/chains/` wholesale and is registered in ci_summary_gate.py::STRICT_GATE_JOBS, so a skipped or absent conclusion fails CI Summary closed. This file is collected the moment it lands. NO runtime behavior change. The DLQ/quarantine router is owned by OMN-16798 (#2949, #2951, both landed 2026-08-27) and the inventory's section 7.2 PermissionError-swallow finding; deliberately not touched here. Evidence: uv run pytest tests/integration/chains/ -q -> 8 passed Evidence-Ticket: OMN-16813 * chore(OMN-16813): refresh receipt metadata after OCC merge --------- Co-authored-by: branch-preview <preview@omninode.local>
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.
OMN-16798 follow-up — the totality guards amplified themselves on a dead-letter topic
omnibase_infra#2949(merged35d97fc5, deployed to the .201 dev lane at 23:35:30Z, receipt20260827T233530Z-35d97fc51650.json, health-gate PASS) fixed both OMN-16798 defects, and the chain is alive: the first post-deploy probe returned a terminal in 2.4 s instead of timing out,routing-decision.v1moved off its flat HWM of 59 for the first time, and inference was reached.The 13-class matrix immediately afterwards surfaced a defect that PR introduced, and this fixes it rather than carrying it.
The defect
Both OMN-16798 totality guards answer an undeliverable record by routing it to the topic's DLQ. On a record consumed from a
onex.dlq.*topic that answer is circular:get_dlq_topic_for_originalresolves a dlq topic back to itself, so the guard republished the record onto the very topic it had just read it from, and the subscriber read it again.Measured on the dev lane 2026-08-27T23:49-23:51Z.
node_dlq_replay_effectandnode_ledger_projection_computeboth consumeonex.dlq.omnibase-infra.commands.v1:24 of the last 38 records on that topic carry
original_topic: onex.dlq.omnibase-infra.commands.v1anderror_type: HandlerDispatchFailureError— records the guard itself authored. The run converged (HWM stable at 188 across a 30 s re-probe, both consumer groupsStableatTOTAL-LAG 0, zerono_dispatcherlines in the following 2 minutes, andonex.dlq.omnibase-infra.omnibase-infra.v1still at HWM 0), but convergence there was luck, not mechanism, and a self-referential DLQ write is not something to leave standing on that basis.The fix
_is_dead_letter_source_topic(topic)— a record already sitting on a dead-letter sink is already durably captured, which is precisely what the guards exist to secure. AC4's requirement — dispatch, DLQ, or refuse with evidence — is satisfied by that durable record plus a loud structured log (metric_name=boundary_dead_letter_unroutable dlq_routed=false reason=already_on_dead_letter_topic). Republishing adds no evidence and risks amplification, so both guards log and stop.The amplification was measured on the NO_DISPATCHER leg only. The undeliverable-output leg is guarded by symmetry — the loop mechanism is identical — and the comment says so rather than implying it was observed.
RED before GREEN
Two tests added to
tests/unit/runtime/auto_wiring/test_omn16798_boundary_delivery_totality.py(11 in the file). With only the guard reverted (the test file left in place — a surgical revert):The first failure emits the live log line verbatim. With the guard restored: 11 passed.
dod_evidence
dlq_routed=trueon a dead-letter source topic.tests/unit/runtime+tests/unit/topology: 5692 passed, 5 skipped.impacted tests passed; allowing push). NoPREPUSH_FULL_SUITE, noENABLE_SMART_TESTS=off, no override grant.ruff format+ruff checkclean;mypy --strict: Success: no issues found in 2858 source files;pre-commit run --all-filesclean.What #2949 proved live, for the record
POST :8085/skillterminaldispatch_timeoutafter 180 sonex.evt.omnibase-infra.routing-decision.v1onex.cmd.omnibase-infra.delegation-inference-request.v1status=timeoutstatus=failed,terminal_failure_cause=provider_quota_exhausted— a real attributed causeAuto-wiring result applier registered from published_events (contract=node_delegation_routing_reducer …)Dev lane only.
stability-test,judgeandprodwere not touched.Evidence-Ticket: OMN-16798
Evidence-Source: OCC#7347