Repository navigation
fix(OMN-20767): bound auto-wired route and dispatcher ids to the ModelDispatchRoute limits - #4767
Conversation
…lDispatchRoute limits with a stable digest suffix Onex-Lane: routeid-9f8a Onex-Session: 162993f429764af6956bad5e50e59133
…oute ids Onex-Lane: routeid-9f8a Onex-Session: 162993f429764af6956bad5e50e59133
…13419) * evidence(OMN-20767): author OCC companion for OmniNode-ai/omnibase_infra#4767 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 364a20c4e9bd9ecd34a5799fd0c789e12001a7d9. * evidence(OMN-20767): self-bind OCC#13419 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: qwen3-review, local-studio-planner
Models failed: none
New finding threads: 4
Deduped (already posted on this PR): 0
Below quorum (one model only, reported not threaded): 10
Nit-level findings suppressed: 0
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Below quorum: 10 finding(s) raised by one model only (OMN-18479)
These are reported and NOT dropped, but they get no thread and do not block: a single model's finding no other model reproduced is not evidence enough to stop a merge. Read them; act on them if they are right.
- [CRITICAL]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(qwen3-review) — Missing hashlib import causes NameError at runtime | The new function_bound_dispatch_identifiercallshashlib.sha256(...), but the diff adds noimport hashlibto the module. The existing import - [CRITICAL]
tests/unit/runtime/auto_wiring/test_route_id_bounded.py(local-studio-planner) — Test boundary calculation is incorrect and untested | The testtest_ids_within_the_limit_are_unchangedcalculates the contract name length using a hardcoded string literal 'route.auto...t' which con - [MAJOR]
tests/unit/runtime/auto_wiring/test_route_id_bounded.py(qwen3-review) — Boundary test uses empty handler_key producing malformed id | The boundary test constructs a route id withhandler_key="", yieldingroute.auto.<contract>..t(double dot). This is not a valid ident - [MAJOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(qwen3-review) — prefix_length can be negative or zero for short max_length values |prefix_length = max_length - len(digest) - 1. Ifmax_lengthis less thanlen(digest) + 1(i.e., less than 17 for the current 1 - [MAJOR]
tests/unit/runtime/auto_wiring/test_route_id_bounded.py(qwen3-review) — No test for the exact boundary where natural_id length equals max_length | AC3 states ids within the limit are returned unchanged. The testtest_ids_within_the_limit_are_unchangedincludes a boundar - [MAJOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — Runtime model introspection introduces coupling and fragility | _dispatch_route_field_max_length` dynamically inspects Pydantic model metadata at runtime. This couples the wiring logic to the internal - [MINOR]
tests/unit/runtime/auto_wiring/test_route_id_bounded.py(qwen3-review) — No test for dispatcher_id bounded form with realistic long names | The testtest_long_ids_sharing_a_prefix_stay_distinctchecks dispatcher distinctness but usesHandlerAandHandlerBas handler - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(qwen3-review) — Deferred import of ModelDispatchRoute inside a cached function creates a hidden coupling |_dispatch_route_field_max_lengthperforms a deferred import ofModelDispatchRouteand `ProtocolConfigurat - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(qwen3-review) — SHA-256 digest is of the full natural id, not the truncated portion | The docstring and comment state the digest is of the 'full natural identifier'. This is correct for determinism, but it means two - [MINOR]
tests/unit/runtime/auto_wiring/test_route_id_bounded.py(local-studio-planner) — Missing test for dispatcher ID collision logic | The tests verify that route IDs are distinct when they share a prefix, but do not explicitly test that dispatcher IDs follow the same bounded logic wit
Findings demoted from threads (anchor rejected)
-
[MAJOR] hostile-reviewer (qwen3-review)
16 hex chars (64 bits) digest is insufficient for collision resistance at scale | The comment states 64 bits keeps collisions 'out of reach', but the birthday bound for 64 bits is ~2^32 distinct over-long ids before a 50% collision probability. In a multi-tenant or high-cardinality deployment with many contracts, handlers, and topics, this is not out of reach. The digest is of the full natural id, so two distinct natural ids that happen to share the same 64-bit SHA-256 prefix will produce identical bounded
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (local-studio-planner)
Insufficient collision resistance in bounded identifiers | The implementation uses a 16-character (64-bit) SHA-256 digest suffix to resolve collisions for long identifiers. This provides only 2^64 possible values, which is insufficient for distributed systems where multiple nodes may independently generate distinct long identifiers. The birthday paradox implies a 50% probability of collision after approximately 2^32 (4 billion) distinct inputs per node, or significantly fewer across a cluster. The comment c
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (qwen3-review)
@cache on _dispatch_route_field_max_length is unbounded and unnecessary |
_dispatch_route_field_max_lengthis decorated with@cache(unboundedfunctools.cache). It takes a single string argument and is called at most twice per process lifetime (once forroute_id, once forhandler_id). The cache provides no benefit and holds a reference to theModelDispatchRouteclass and its metadata indefinitely. If the model is reloaded or the class is redefined (e.g., in tests), the cached value becomes staleResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (local-studio-planner)
Redundant import of lru_cache | The diff imports
lru_cachebut the code only usescache. While not a functional error, it introduces unnecessary dependency on the LRU implementation if the intent is an unbounded cache. If bounded caching is required for memory safety in high-throughput scenarios,cacheshould be replaced withlru_cache(maxsize=...). | Evidence: from functools import cache, lru_cache | Fix: Removelru_cachefrom the import statement if it is not used elsewhere in the file. If `lru_Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
✅ Hostile Reviewer — REVIEWEDCritical findings: 2
Semantics (OMN-17492 — the model finds, thread resolution gates)
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review: qwen3-review (Qwen3.8-27B), local-studio-planner (Qwen3.6-35B-A3B) (OMN-8468/OMN-8524/OMN-17492) |
PR 4767 was red on Integration Test Coverage because no test existed under tests/integration. The dod-verify control dod-omn-20767-ac3 passed at the merge base with an unchanged test. Added tests/integration/runtime/test_auto_wiring_bounded_route_ids.py to drive wire_from_manifest with the 206-char omnimarket contract name. The unchanged-id unit test now also calls _bound_dispatch_identifier so it fails at the merge base. 5 tests pass.
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: qwen3-review, local-studio-planner
Models failed: none
New finding threads: 0
Deduped (already posted on this PR): 0
Below quorum (one model only, reported not threaded): 12
Nit-level findings suppressed: 0
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Below quorum: 12 finding(s) raised by one model only (OMN-18479)
These are reported and NOT dropped, but they get no thread and do not block: a single model's finding no other model reproduced is not evidence enough to stop a merge. Read them; act on them if they are right.
- [MAJOR]
tests/unit/runtime/auto_wiring/test_route_id_bounded.py:105-107(qwen3-review) — Dispatcher ID bounded to route_id limit instead of handler_id limit | The test test_long_ids_sharing_a_prefix_stay_distinct asserts that dispatcher IDs are at most _ROUTE_ID_MAX (200) characters. Howe - [MAJOR]
tests/unit/runtime/auto_wiring/test_route_id_bounded.py:113-119(qwen3-review) — Boundary test constructs an invalid natural ID with empty handler_key | The boundary test test_ids_within_the_limit_are_unchanged constructs a route ID with an empty handler_key: _derive_route_id(cont - [MAJOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — Insufficient collision resistance in bounded identifiers | The implementation uses only the first 16 hex characters (64 bits) of the SHA-256 digest to bound long identifiers. This provides insufficien - [MAJOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — Dispatcher ID collision risk due to shared prefix truncation | The_bound_dispatch_identifierfunction truncates the natural ID to a fixed prefix length (max_length - len(digest) - 1) and appends - [MAJOR]
tests/integration/runtime/test_auto_wiring_bounded_route_ids.py(local-studio-planner) — Integration test does not verify collision resistance or edge cases | The integration testtest_long_contract_name_wires_with_bounded_route_idonly verifies that a single long contract name wires su - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py:8380-8383(qwen3-review) — SHA-256 digest truncated to 16 hex chars (64 bits) for collision resistance | The comment states '16 hex characters (64 bits) keeps a collision between two distinct over-long ids on one runtime out of - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py:8410(qwen3-review) — SHA-256 computation on every call to _bound_dispatch_identifier | _bound_dispatch_identifier computes hashlib.sha256(natural_id.encode()).hexdigest() on every call. For IDs within the limit, this is s - [MINOR]
tests/unit/runtime/auto_wiring/test_route_id_bounded.py:85-100(qwen3-review) — Missing test for collision when two IDs differ only in the truncated portion | The test suite does not include a test that verifies two natural IDs that share the first (max_length - 17) characters bu - [MINOR]
tests/integration/runtime/test_auto_wiring_bounded_route_ids.py:88(qwen3-review) — Integration test asserts dispatcher ID length against route ID limit | The integration test test_long_contract_name_wires_with_bounded_route_id asserts len(result.dispatchers_registered[0]) <= ROUTE - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — Unnecessary import oflru_cachealongsidecache| The diff imports bothcacheandlru_cachefromfunctools. The new code uses@cachefor_dispatch_route_field_max_length, butlru_cache - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — Hardcoded digest length constant lacks documentation of security rationale | The constant_BOUNDED_ID_DIGEST_HEX = 16is defined with a comment claiming it keeps collisions 'out of reach'. As noted - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — Potential IndexError if max_length is too small for digest + separator | The_bound_dispatch_identifierfunction calculatesprefix_length = max_length - len(digest) - 1. Ifmax_lengthis less th
- Add tests/integration/runtime/test_auto_wiring_bounded_route_ids.py - Edit tests/unit/runtime/auto_wiring/test_route_id_bounded.py - New integration test runs wire_from_manifest against a real MessageDispatchEngine for the two omnimarket 0.4.308 contract shapes (node_delegation_acceptance_judged_replay_compute, node_delegation_acceptance_judged_publish_effect) - Asserts total_failed 0, both wired, route and dispatcher ids at most 200 characters, ids registered on the engine, and identical ids across two boots - At the merge base handler_wiring.py both integration tests fail with a ModelDispatchRoute route_id ValidationError (the production failure); at the PR head both pass - Unit test test_ids_within_the_limit_are_unchanged now also calls _bound_dispatch_identifier directly (in-limit id returned as the same string), so it fails against a tree with no bound - Before this the AC3 bound test passed at the merge base and dod-verify refused it as an always-pass control - Reason: Integration Test Coverage required a tests/integration test for the changed src/ file; dod-verify required AC3 to fail at the merge base - Merges dev (48ece44) into the branch
Keeps this branch's versions of tests/integration/runtime/test_auto_wiring_bounded_route_ids.py and tests/unit/runtime/auto_wiring/test_route_id_bounded.py, which both commits add.
|
Lab: head=28b945ef0e9cbf033f189e41a1dfbdb184c63ad9 |
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: qwen3-review, local-studio-planner
Models failed: none
New finding threads: 0
Deduped (already posted on this PR): 0
Below quorum (one model only, reported not threaded): 13
Nit-level findings suppressed: 0
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Below quorum: 13 finding(s) raised by one model only (OMN-18479)
These are reported and NOT dropped, but they get no thread and do not block: a single model's finding no other model reproduced is not evidence enough to stop a merge. Read them; act on them if they are right.
- [CRITICAL]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(qwen3-review) — Missing hashlib import causes NameError in _bound_dispatch_identifier | The function _bound_dispatch_identifier calls hashlib.sha256(natural_id.encode()).hexdigest(), but the diff adds no import for h - [CRITICAL]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — SHA-256 truncation to 16 hex chars creates collision vulnerability | The implementation uses only the first 16 hexadecimal characters of a SHA-256 digest (64 bits) to distinguish identifiers. This pro - [MAJOR]
tests/unit/runtime/auto_wiring/test_route_id_bounded.py(qwen3-review) — Test boundary case produces malformed route id with double dot | The test test_ids_within_the_limit_are_unchanged asserts that _derive_route_id(contract_name, "", "t") equals f"route.auto.{contract_na - [MAJOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(qwen3-review) — Cache on _dispatch_route_field_max_length couples infra to core model introspection | The function _dispatch_route_field_max_length uses @cache and introspects ModelDispatchRoute.model_fields at runti - [MAJOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — Inconsistent max_length retrieval logic between route_id and handler_id | The_dispatch_route_field_max_lengthfunction iterates throughModelDispatchRoute.model_fields[field_name].metadatato fin - [MAJOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — Unbounded LRU cache on_dispatch_route_field_max_lengthrisks memory leak | The_dispatch_route_field_max_lengthfunction is decorated with@cache, which is equivalent to `lru_cache(maxsize=None - [MAJOR]
tests/integration/runtime/test_auto_wiring_bounded_route_ids.py(local-studio-planner) — Integration test mocks critical dependencies, invalidating real-world behavior | The integration testtest_auto_wiring_bounded_route_ids.pypatches_import_handler_classto return a dummy `_Handle - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(qwen3-review) — 16 hex characters (64 bits) of SHA-256 may be insufficient for adversarial collision resistance | The comment states that 16 hex characters (64 bits) keeps a collision between two distinct over-long i - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(qwen3-review) — No test for the case where max_length is less than digest length plus one | The function _bound_dispatch_identifier computes prefix_length = max_length - len(digest) - 1. If max_length is less than 17 - [MINOR]
tests/integration/runtime/test_auto_wiring_bounded_route_ids.py(qwen3-review) — Integration test hardcodes _ROUTE_ID_MAX instead of reading from the model | The integration test defines _ROUTE_ID_MAX = 200 as a constant. If the ModelDispatchRoute.route_id max_length changes, the - [MINOR]
tests/integration/runtime/test_auto_wiring_bounded_route_ids.py(qwen3-review) — Dispatcher id uses handler_id field limit but route id uses route_id field limit | The _derive_dispatcher_id function bounds the dispatcher id to the max_length of ModelDispatchRoute.handler_id, while - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — Prefix truncation may split multi-byte UTF-8 characters | The_bound_dispatch_identifierfunction truncates the natural ID by byte count (via Python string slicing). If the contract name or handler - [MINOR]
src/omnibase_infra/runtime/auto_wiring/handler_wiring.py(local-studio-planner) — Hardcoded digest length constant lacks documentation for trade-offs | The constant_BOUNDED_ID_DIGEST_HEX = 16is hardcoded without any reference to the security analysis or collision probability ca
1 participant
Lab: host=.201 lane=dev (compose omnibase-infra, warm, no --cold) head=364a20c4e command="docker cp of this branch's handler_wiring.py plus omnibase_infra#4766 (135912f) src and entrypoint into omninode-runtime, omninode-runtime-effects, omnibase-infra-runtime-worker-1 (image rev 2a12898, omnimarket 0.4.308), replacing the temporary route-id cap; then docker inspect health/RestartCount and curl :8085/:8086 /health every 30 s" observed="all three healthy, RestartCount 0, :8085 and :8086 /health 200 continuously 11:24:51Z-11:30:32Z (5m41s); node_delegation_acceptance_judged_replay_compute consumer group joined on runtime-main and node_delegation_acceptance_judged_publish_effect on runtime-effects; zero route_id validation errors in all three logs"
Summary
OMN-20767. Auto-wiring derived the dispatch route id as
route.auto.<contract>.<handler entry key>.<sanitized topic>with no bound. For omnimarket 0.4.308'snode_delegation_acceptance_judged_replay_computeandnode_delegation_acceptance_judged_publish_effectthat id is 206 characters, over the 200-charactermax_lengthonModelDispatchRoute.route_id, so the route constructor raised and the dev-lane runtime failed at auto-wiring. The lane was kept up only by a temporary truncate-plus-hash cap copied into the containers by lane revert4750-9f8a; that cap is in no PR and was lost on the deploy agent's 11:08Z recreate.Changes
_bound_dispatch_identifierinruntime/auto_wiring/handler_wiring.py: an id within the limit is returned unchanged (no existing route or dispatcher id moves); a longer one keeps its leading characters and ends in.plus 16 hex characters of the SHA-256 of the FULL natural id, so names sharing a prefix stay distinct and the same inputs give the same id on every boot. No silent truncation.ModelDispatchRoute.model_fields[...]metadata in core, so it has one source; no core change and no core release are needed (derivation is implementation, the model stays in core)._derive_dispatcher_idgets the same bound, because the dispatcher id fillsModelDispatchRoute.handler_id, alsomax_length=200.route_idis stored in no DB column, topic name or projection. It reaches logs, metrics andModelDispatchResult.route_id(unbounded). The duplicate-route-id check inhandler_wiringstill sees every id.contracts/OMN-20767.yamlwith dod_evidence.Test plan
uv run pytest tests/unit/runtime/auto_wiring/test_route_id_bounded.py -qfailed at the merge base (206 > 200 for both real contracts, 289 > 200 for the prefix case), passes now.ModelDispatchRoute; two long names sharing a 230-character prefix stay distinct and deterministic; ids within the limit (including exactly 200) are unchanged.uv run pytest tests/unit/runtime/auto_wiring/ -n 4: 858 passed. ruff format/check andmypy --stricton the file clean; commit and pre-push hooks passed.Runtime startup gate
wire_from_manifestin the real kernel on all three runtime containers, healthy with RestartCount 0.Strict-gate ordering
Evidence-Ticket: OMN-20767
Evidence-Source: OCC#13419