Skip to content

fix(OMN-18978): union caller criteria with the task class definition of done, and never lose a terminal to a validation failure - #2742

Merged
jonahgabriel merged 2 commits into
devfrom
jonah/omn-18978-union-criteria-and-terminal-failsafe
Sep 21, 2026
Merged

jonahgabriel merged 2 commits into
devfrom
jonah/omn-18978-union-criteria-and-terminal-failsafe

Conversation

@jonahgabriel

@jonahgabriel jonahgabriel commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

OMN-18978

A caller naming an acceptance criterion that its task class already declares as part of
its definition of done got that rule evaluated twice. The terminal event model refuses a
rule recorded more than once, so the terminal could not be constructed at all, nothing
was published, and the caller waited out its whole 240-second handler budget before a
synthesized timeout arrived reporting zero attempts for a run that had dispatched five
times and been decided.

Measured end to end on the dev lane, correlation e379a4b9-8fbc-4408-b277-07ec32ac1876:
five rungs ran, five models answered inside 37 seconds, the quality gate refused every
one at 0.373 against a bar of 0.800, and at the exact second of the final decision the
runtime logged a validation error saying rule evaluations must record each rule at most
once. Then 196 seconds of silence.

Both halves land together, because either one alone still leaves a silent hang.

1. The merge unions by rule name

handler_quality_gate.delta concatenated three sequences: the class's deterministic
rules, its heuristic rules, and the caller's criteria. A name appearing in two of them
produced two evaluations. It now unions them, under two deliberate policies:

  • A declared rule keeps the tier its task class gave it. Naming a scored rule as a
    criterion does not promote it to blocking, because the caller named a rule, not an
    enforcement tier, and letting a caller escalate one would silently make the bar
    stricter than the contract the response is graded against.
  • A rule a class declares in both of its own sets stays blocking, since a contract
    asking for a veto gets the veto.

A criterion naming no declared rule still joins the deterministic set, unchanged, which
is what makes the flag mean anything. There is a positive control for that, because a
fix that silently discarded the caller's criteria would pass every dedup assertion and
defeat the feature.

2. Terminal construction degrades instead of propagating

The duplicate is fixed above, but the silence is the other half and it generalises: any
future rejected field reproduces the same 240-second wait. So the guard is written
against terminal CONSTRUCTION, not against this one field. A validation failure there
now publishes a typed failure terminal carrying the run's identity, its answer, and a
reason naming both the validation error and the workflow it happened to, and logs at
error under a countable metric name.

The degraded terminal is deliberately narrow. It is emitted as failed even where the run
had been decided completed, because a terminal whose evidence fields could not be
constructed cannot honestly claim it was graded and passed. It states no cost and no
token count, so it cannot become a second, divergent writer of a row the single-builder
invariant protects, and that property is asserted structurally rather than trusted. It
never borrows one of the provider-side failure causes, none of which is true here.

Both directions proven against the pre-image at origin/dev, path-scoped, no shared
state touched.

Part 1, the merge, before the fix:

AssertionError: the exact invocation that hung the lane; both duplicate HEURISTIC rules:
['covers_edge_cases', 'no_refusal'] in ['compiles_without_errors', 'final_artifact_only',
'uses_pytest_mark_unit', 'covers_edge_cases', 'no_refusal', 'no_refusal',
'covers_edge_cases', 'covers_error_paths']

Part 2, the guard, same inputs on the pre-image and then with the fix:

=== PRE-IMAGE (origin/dev) ===
RESULT: NO TERMINAL PUBLISHED. ValidationError raised after 0.01s

=== WITH THE FIX ===
metric_name=delegation_terminal_unconstructible correlation_id=ac3afb8b-... completed=False
RESULT: 1 event(s) published in 0.01s
  -> ModelDelegationFailed terminal_construction_failed

Local: 375 passed, 1 skipped across the 44 test modules touching acceptance criteria,
the contract mode, the terminal builder or rule evaluations. Full lint, format and strict
type check clean. All hooks green.

The single-builder structure test was amended rather than relaxed: it now admits exactly
one additional construction site and pins that it passes no measured cost or token field,
which is the property that invariant actually protects.

Not done here

The lane readback with a fresh overlapping-criteria delegate is owed and is blocked on
an unrelated broken state in the shared canonical clone, which has taken the CLI down on
the launching host. Reported to the coordinator; it needs its owner, not a workaround
from this lane.

Evidence-Ticket: OMN-18978
Evidence-Source: OCC#10651

…ver lose a terminal to a ValidationError

A caller naming a criterion its task class already declares produced two
evaluations of one rule. The terminal DTO refuses that, so the terminal
could not be constructed, nothing was published, and the caller waited out
its whole 240-second handler budget for a synthesized timeout carrying an
empty attempt list.

Both halves land together because either one alone leaves a silent hang:
the merge now unions by rule name, and terminal construction now degrades
to a typed failure terminal instead of propagating.
@github-actions

Copy link
Copy Markdown

⚠️ Architectural Review — WARNINGS

Errors: 0
Warnings: 1

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

ARCH-DI-002: Direct Handler instantiation (warning)\n\n```\nsrc/omnimarket/nodes/node_delegation_orchestrator/handlers/handler_delegation_workflow.py:980: loop = HandlerComplianceLoop()

src/omnimarket/nodes/node_delegation_orchestrator/handlers/handler_delegation_workflow.py:1753: HandlerEscalationDecision()\n```\n\nHandler classes should be wired only in workflow_runner.py or adapter modules. Direct instantiation elsewhere bypasses the contract system.\n\n

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

@jonahgabriel
jonahgabriel enabled auto-merge (squash) September 21, 2026 08:29
onexbot-occ-writer Bot added a commit to OmniNode-ai/onex_change_control that referenced this pull request Sep 21, 2026
…nimarket#2742

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/35578123280
@github-actions

github-actions Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

✅ Hostile Reviewer — PASSED

Blocking findings (critical/error): 0
Total findings: 12
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 pushed a commit to OmniNode-ai/onex_change_control that referenced this pull request Sep 21, 2026
…imarket#2742 (#10651)

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

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

* evidence(OMN-18978): self-bind OCC#10651 + rebind contract_sha256

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

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/35578123280

---------

Co-authored-by: omnimarket-bot <bot@omninode.ai>
Co-authored-by: onexbot-occ-writer[bot] <307849072+onexbot-occ-writer[bot]@users.noreply.github.com>
…criteria-and-terminal-failsafe

# Conflicts:
#	src/omnimarket/nodes/node_delegation_quality_gate_reducer/handlers/handler_quality_gate.py
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