Skip to content

fix(OMN-19131): omit requested_timeout_seconds from the delegation wire when unset - #2777

Merged
jonahgabriel merged 2 commits into
devfrom
jonah/omn-19131-exclude-unset-requested-timeout-from-the-delegation-wire
Sep 23, 2026
Merged

jonahgabriel merged 2 commits into
devfrom
jonah/omn-19131-exclude-unset-requested-timeout-from-the-delegation-wire

Conversation

@jonahgabriel

@jonahgabriel jonahgabriel commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

OMN-19131

requested_timeout_seconds landed on ModelDelegateSkillRequest (omnimarket#2718, ce11a870d, merged 2026-09-22T03:49:58Z) without exclude_if. RuntimeLocal publishes the delegation command record as model_payload.model_dump_json().encode("utf-8"), so the field serialises as "requested_timeout_seconds": null on every delegation — including the overwhelming majority that never passed --timeout and have no interest in it.

The model declares extra="forbid". A consumer baked before the field declares no such field and refuses the record as publisher-malformed. This is the second instance of the rollout class whose remedy is spelled out three fields further down this same file.

Lab readback (rule 24) — .201, 2026-09-22

Each container's own copy of the model validating the exact published bytes, rather than comparing version strings (both sides reported the same version last time, which is why the first instance went undetected):

container lane pre-fix bytes post-fix bytes
omninode-runtime omnibase-infra (dev) ACCEPTED ACCEPTED
omninode-runtime-effects omnibase-infra (dev) field declared field declared
omninode-stability-test-runtime omnibase-infra-stability-test REFUSED ACCEPTED
omninode-stability-test-runtime-effects omnibase-infra-stability-test no field, extra=forbid —

Refusal text on the pre-fix bytes:

1 validation error for ModelDelegateSkillRequest
requested_timeout_seconds
  Extra inputs are not permitted [type=extra_forbidden]

One producer, one record, accepted on one live lane and refused on another at the same moment. Read-only docker exec ... python -c on both lanes; nothing built, restarted, retagged or otherwise mutated.

RED then GREEN

tests/unit/delegation/test_omn19131_requested_timeout_excluded_when_unset.py
RED:   2 failed, 3 passed   (the 3 are the over-reach controls, correct to pass before the fix)
GREEN: 5 passed

Neighbouring suites after the fix, all green: test_omn18852_published_at_excluded_when_unset, test_omn18852_queue_wait_accounting, test_models, test_handler_delegate_skill_budget_omn15504, test_runtime_dispatch_port, test_response_contract_conformance_runner_omn18700, test_omn18852_inference_deadline_bound — 85 passing across the two runs.

Why the existing guard was green while this reached the wire

test_omn18852_published_at_excluded_when_unset derives its accepted field set as ModelDelegateSkillRequest.model_fields.keys() - {"published_at"}, so it ratifies whatever field the model gains next — exactly the hazard its own docstring warns about, applied to every field but the one it names. The new module therefore spells the permitted wire keyset as a literal, in both directions: a new optional field that forgets exclude_if names itself in the failure, and silently dropping a key an existing consumer requires fails too.

Scope

One field gains exclude_if=lambda value: value is None. No behaviour change for any caller: a request that does set a timeout still serialises it (covered by a positive control), the ge=1 bound is unchanged, and the handler's ceiling refusal and budget evidence are untouched. The other optional fields on this contract are older than every deployed consumer and are deliberately left on the wire — that is what the literal keyset records.

This closes the wire-safety half of Required outcome A. Halves B (AC3/AC4 — a pre-publish validation failure reported as itself rather than as an absent terminal) and AC5 (the cross-repo payload/model contract test) live in omnibase_infra's cli_delegate.py and are not touched here.


Merge sequencing (OMN-17427): this is runtime-affecting under the canonical deploy-gate path patterns, so it lands only after omnibase_infra#3939 under the 2026-09-21 runtime merge-order ruling. It is ready for review so its change-control companion can mint and every gate can run on a non-draft head; the hold:auto-merge label keeps the auto-merge workflow from arming it. Fresh head merges current dev, including the OMN-18868 wire-compatibility gate; run locally on that head it reports WIRE_COMPAT_PASS against released consumer v0.4.198.

Evidence-Ticket: OMN-19131
Evidence-Source: OCC#10822

…re when unset

The field landed on ModelDelegateSkillRequest without exclude_if, so
RuntimeLocal's model_dump_json publish seam emitted
"requested_timeout_seconds": null on every delegation, including every
caller that never passed --timeout. The model declares extra=forbid, so a
consumer baked before the field refuses the record as publisher-malformed.

This is the second instance of the OMN-18852 rollout class, whose remedy is
documented three fields further down the same file.

Lab readback, .201, 2026-09-22 -- each container's own copy of the model
validating the exact published bytes:

  omninode-runtime                 PRE  ACCEPTED   POST ACCEPTED
  omninode-stability-test-runtime  PRE  REFUSED    POST ACCEPTED

PRE is refused with requested_timeout_seconds: Extra inputs are not
permitted [type=extra_forbidden] -- one producer, one record, accepted on
one live lane and refused on another at the same moment.

The new test module also spells the permitted wire keyset as a literal. The
OMN-18852 guard derives its accepted set from the model under test, so it
ratifies any field the model gains next, which is why it was green while
this one reached the wire.

Onex-Lane: omn19131-timeout-field
Onex-Session: 900b19dbb7614ad6962d988b724cb778
@github-actions

Copy link
Copy Markdown

✅ Architectural Review — PASSED

Errors: 0
Warnings: 0

What this checks

Rule Description
ARCH-TOPIC-001 No hardcoded Kafka topic strings in handler code
ARCH-DI-001 No event_bus=None bypass
ARCH-DI-002 No direct Handler instantiation outside workflow_runner/adapters
ARCH-DI-003 No reinvented DI containers
ARCH-TOPIC-002 contract.yaml topics follow `onex.{cmd

No architectural violations found.

Static architectural lint — no model inference (OMN-14176).

@github-actions

github-actions Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

✅ Hostile Reviewer — PASSED

Blocking findings (critical/error): 0
Total findings: 8
Models succeeded: qwen3-review,qwen3-review-b


Gate semantics

Verdict Meaning Blocks merge?
passed >=2 models succeeded, no critical/error findings No
blocked CRITICAL or ERROR findings found Yes
degraded Fewer than 2 models succeeded (infra unavailable/timeout) Yes (OMN-15110)

Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524)

@jonahgabriel jonahgabriel added the hold:auto-merge Holds auto-merge (check_auto_merge_hold.py); remove to release label Sep 22, 2026
@jonahgabriel
jonahgabriel marked this pull request as ready for review September 22, 2026 19:21
onexbot-occ-writer Bot added a commit to OmniNode-ai/onex_change_control that referenced this pull request Sep 22, 2026
…nimarket#2777

The declared checks were executed for real in the OmniNode-ai/omnimarket checkout at the PR head by the product-repo OCC receipt runner, and the results written append-only into this companion. Producer-minted PENDING receipts are superseded by a net-new record; no existing receipt was edited.

Ticket: OMN-16859 (AC3b)
Onex-Workflow: OCC Receipt Runner
Onex-Run: https://github.com/OmniNode-ai/omnimarket/actions/runs/35773286738
jonahgabriel pushed a commit to OmniNode-ai/onex_change_control that referenced this pull request Sep 22, 2026
…imarket#2777 (#10822)

* evidence(OMN-19131): author OCC companion for OmniNode-ai/omnimarket#2777

OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 96c7c3e9e7424a4889b7727b0a797f7cbf516620.

* evidence(OMN-19131): self-bind OCC#10822 + rebind contract_sha256

* evidence(OMN-16859): executed test_passes receipts for OmniNode-ai/omnimarket#2777

The declared checks were executed for real in the OmniNode-ai/omnimarket checkout at the PR head by the product-repo OCC receipt runner, and the results written append-only into this companion. Producer-minted PENDING receipts are superseded by a net-new record; no existing receipt was edited.

Ticket: OMN-16859 (AC3b)
Onex-Workflow: OCC Receipt Runner
Onex-Run: https://github.com/OmniNode-ai/omnimarket/actions/runs/35773286738

---------

Co-authored-by: omnimarket-bot <bot@omninode.ai>
Co-authored-by: onexbot-occ-writer[bot] <307849072+onexbot-occ-writer[bot]@users.noreply.github.com>
jonahgabriel added a commit that referenced this pull request Sep 22, 2026
…to infra (#2783)

* fix(OMN-19194): raise shadow-slice timeout budget and route exit 124 to infra

market-drain measured (2026-09-22, OMN-17427): the scoped shadow slice
(-m "unit and not kafka") takes 621-663s of pytest time plus ~90s of
collection, 711-753s total. The old --timeout 900 left only 150-190s of
margin, and a ~25% slower runner (observed on PR #2777/#2779) pushed the
same correctly-scoped slice past the wall -- exit 124 -- which this step
fell through to the generic "failure" branch on, misreporting reason-graph
and product-readiness/evaluate as PRODUCT_FAILED for a hang/timeout, not a
real product defect.

Two-part fix, matching the OMN-18820 exit-125 precedent:
- raise the budget to 1200s, giving 300s+ (27%+) margin over the
  worst-observed timed-out case -- this is runner-speed variance on an
  already-narrow slice, unlike the sibling onex_change_control finding
  under this same ticket, where the slice itself needed narrowing;
- mirror the exit-125 branch for exit 124: report product_conclusion=timeout
  and succeed the step, so a genuine remaining hang still routes to the
  infra dimension instead of a false PRODUCT_FAILED root.

* fix(OMN-19194): declare exit-124 timeout as an explicit INFRA conclusion

The exit-124 -> product_conclusion=timeout branch this ticket wires into
product-readiness-shadow.yml relies on product_readiness.py's fail-closed
default (an unrecognized conclusion falls through to INFRA) rather than an
explicit vocabulary entry. OMN-18820 declared runner_crashed explicitly for
exactly this reason: its own test (now mirrored here) proves the entry is
load-bearing documentation, not a functional no-op, because the fail-closed
default could be tightened later with nothing left to catch it.

Adds "timeout" to _INFRA_CONCLUSIONS with the same measured justification
already in the workflow comment, plus a focused test file mirroring
test_omn18820_shadow_runner_crash_class.py's TestConclusionVocabulary and
TestReasonGraphRoot classes, scoped to this addition only (the harness's own
EXIT_HANG=124 classification is unchanged and already covered by
test_run_shadow_slice.py).
@jonahgabriel jonahgabriel removed the hold:auto-merge Holds auto-merge (check_auto_merge_hold.py); remove to release label Sep 23, 2026
@jonahgabriel
jonahgabriel merged commit ad1cc83 into dev Sep 23, 2026
139 of 149 checks passed
@jonahgabriel
jonahgabriel deleted the jonah/omn-19131-exclude-unset-requested-timeout-from-the-delegation-wire branch September 23, 2026 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant