Repository navigation
fix(OMN-20386): a deployed-lane reply timeout is a typed failure, not a usage error - #4509
Conversation
|
Lab: host=h201 lane=next3-OMN-20386-2a21 head=1853041cb33ac3db7e9191ee300c1fd6d927f9fd command= |
…12561) * evidence(OMN-20386): author OCC companion for OmniNode-ai/omnibase_infra#4509 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 1853041cb33ac3db7e9191ee300c1fd6d927f9fd. * evidence(OMN-20386): self-bind OCC#12561 + 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, gpt-oss-review
Models failed: none
New finding threads: 8
Deduped (already posted on this PR): 0
Below quorum (one model only, reported not threaded): 9
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: 9 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]
src/omnibase_infra/cli/cli_delegate.py, src/omnibase_infra/cli/receipt_mode.py(qwen3-review) — DelegateCallerInterruptedError is a BaseException, not an Exception | DelegateCallerInterruptedError subclasses BaseException. The except clause in run_delegate catches it explicitly. However, any cod - [MAJOR]
src/omnibase_infra/cli/receipt_mode.py(qwen3-review) — workflow_result comparison in receipt_mode uses .value on an enum that may not be TIMEOUT | In _run_receipt_mode, the new code checks workflow_result is EnumWorkflowResult.TIMEOUT and workflow_data.ge - [MAJOR]
tests/unit/cli/test_delegate_incomplete_receipt_omn17427.py(qwen3-review) — No test for the SIGKILL path leaving an in-flight receipt | The test suite includes a test that observes the in-flight receipt mid-wait (test_a_receipt_is_on_disk_before_the_wait_so_a_sigkill_leaves_o - [MAJOR]
src/omnibase_infra/cli/cli_delegate.py(gpt-oss-review) — Potential race condition between terminal writer and in‑flight receipt settlement | The function _settle_in_flight_receipt reads the provisional receipt file and, if it still contains the IN_FLIGHT cl - [MINOR]
src/omnibase_infra/cli/cli_delegate.py(qwen3-review) — Signal handler restoration may restore a None handler to SIG_DFL | In _caller_interrupt_guard, the finally block restores signal handlers with signal.signal(signum, handler if handler is not None else - [MINOR]
src/omnibase_infra/cli/delegate_terminal_resolver.py(qwen3-review) — is_reply_timeout checks wire_correlation_id truthiness, which may be too strict | is_reply_timeout returns True only when workflow_result is "timeout" and wire_correlation_id is truthy. If the runtime - [MINOR]
src/omnibase_infra/cli/cli_delegate.py(qwen3-review) — exit_code variable in run_delegate may be unbound if an exception is raised | In run_delegate, exit_code = run_receipt_mode(...) is assigned inside the with block. If run_receipt_mode raises an except - [MINOR]
src/omnibase_infra/cli/receipt_mode.py(qwen3-review) — run_id parameter added to run_receipt_mode changes its public API | run_receipt_mode is a public function (it is in all or at least imported by other modules). Adding a run_id parameter is backwar - [MINOR]
src/omnibase_infra/cli/receipt_mode.py(gpt-oss-review) — run_receipt_mode signature change breaks external callers | run_receipt_mode now accepts an optional run_id parameter, but external code that imports the function without updating call sites will rece
Findings demoted from threads (anchor rejected)
-
[MAJOR] hostile-reviewer (qwen3-review)
In-flight receipt settlement reads a file that may not exist | _settle_in_flight_receipt reads state_root / runs / run_id / receipt.json. If the in-flight receipt was never written (the OSError path in _file_in_flight_receipt swallows the failure and continues), the file does not exist. The read_text call raises FileNotFoundError, which is a subclass of OSError, so it is caught by the except (OSError, ValueError) clause and the function returns silently. This is correct behavior, but the function's docstrin
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (qwen3-review)
Race between in-flight receipt write and terminal receipt write | _file_in_flight_receipt writes receipt.json with terminal_class=in_flight. The terminal receipt writer (inside run_receipt_mode) also writes receipt.json. If the terminal writer completes and writes a terminal receipt, and then _settle_in_flight_receipt runs, it reads the terminal receipt, sees terminal_class is not in_flight, and returns without modifying it. This is correct. However, if the terminal writer is in the middle of writing receip
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (gpt-oss-review)
Unterminated receipt JSON lacks required workflow_result field | _write_incomplete_run_files creates receipt.json for incomplete runs but does not include the "workflow_result" key that receipt validation expects. Subsequent validation in _run_receipt_mode may raise a validation error, breaking downstream consumers. | Evidence: Function _write_incomplete_run_files (lines 770‑845) builds the receipt JSON without a "workflow_result" entry, while _run_receipt_mode (lines 1470‑1485) validates receipt.workflow_r
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (gpt-oss-review)
Missing error handling when in‑flight receipt cannot be created | _file_in_flight_receipt catches OSError, logs a message, and continues execution. Subsequent code assumes the provisional receipt exists (e.g., _settle_in_flight_receipt), leading to silent failures or missing receipt files. | Evidence: Exception handling in _file_in_flight_receipt (lines 1025‑1035) only echoes the error without aborting or flagging the condition. | Fix: Propagate the OSError (or raise a custom exception) so that the caller c
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (qwen3-review)
In-flight receipt write adds I/O before every delegation wait | _file_in_flight_receipt writes three files (result.txt, receipt.json, run.json) before the terminal wait begins. This adds three file writes and directory creation to the critical path of every delegation. For a system that dispatches many delegations, this is a measurable overhead. The writes are atomic, so they are safe, but the cost is non-trivial. The docstring acknowledges that a failure to write is reported and does not stop the delegatio
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (qwen3-review)
Test for signal handler restoration uses SIGTERM only | test_signal_handlers_are_restored_after_the_run checks that SIGTERM's handler is restored. The _caller_interrupt_guard also installs handlers for SIGHUP and SIGINT. The test does not verify that SIGHUP and SIGINT handlers are also restored. If a regression breaks the restoration for SIGHUP or SIGINT, this test would not catch it. | Evidence: def test_signal_handlers_are_restored_after_the_run(self, tmp_path, monkeypatch): ... before = signal.getsignal(
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (gpt-oss-review)
Inefficient JSON serialization for large receipt writes | All receipt writes use json.dumps with sort_keys=True and default=str, which incurs unnecessary CPU overhead for large payloads and may affect latency in high‑throughput scenarios. | Evidence: Calls to json.dumps in _write_incomplete_run_files (lines 795‑822) and _write_unterminalized_run_files (lines 860‑879). | Fix: Remove sort_keys=True for production writes or replace with a faster serializer (or pre‑sorted dict) when ordering is not required.
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (gpt-oss-review)
EnumDelegateIncompleteClass.IN_FLIGHT path lacks unit test verification of settlement logic | The new IN_FLIGHT enum value is exercised only for receipt creation; there is no test confirming that _settle_in_flight_receipt correctly transitions the receipt to FAILED when the terminal writer does not replace it. | Evidence: Test suite includes TestCallerTimeoutLeavesAReceipt but does not assert the final receipt after settlement for the IN_FLIGHT case. | Fix: Add a unit test that simulates a run where the ter
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
✅ Hostile Reviewer — REVIEWEDCritical findings: 1
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), gpt-oss-review (gpt-oss-120b) (OMN-8468/OMN-8524/OMN-17492) |
1853041 to
86f2f59
Compare
A caller that ends `onex delegate` at its own timeout (300 s, SIGKILL, on the landing lanes) left no receipt.json, so the run had a delegation_events row and nothing on disk (4 cases on h201). Two h201 runs (52e9d386, 87fabfec) failed with a receipt that held no terminal payload and also left nothing, because the resolver raised before the writer ran. - An in-flight receipt (status pending, terminal_class in_flight, run id, correlation id, declared timeout, command topic) is filed before the terminal wait. The terminal receipt replaces it; a run that ends with it still in place is rewritten as failed. - The CLI's hard timeout writes a timeout receipt; SIGTERM, SIGHUP and SIGINT write an interrupted receipt and exit 128 + the signal number. - A receipt with no resolvable terminal, including a run refused before publish, writes a failed or timeout receipt with the runtime's error and type, then raises as before. - run_receipt_mode takes an optional run_id, so the id the CLI prints and files under is the id the final receipt lands in. Two tests that pinned "nothing is written" now assert the receipt and the unchanged raise.
… a usage error Before: onex delegate with a deployed-lane run whose reply never arrives exited 2, printed the Usage banner, and left a run folder with only workflow_result.json. Cause: the receipt validator _delegate_receipt_evidence_error in cli_delegate.py caught only DelegatePrePublishFailureError; the resolver's DelegateTerminalUnresolvedError (a ValueError) escaped run_receipt_mode and delegate_command maps ValueError to click.UsageError. The receipt writer never ran. Change 1: delegate_terminal_resolver.py gains DelegateReplyTimeoutError (subclass of DelegateTerminalUnresolvedError), raised by _delegation_result when the runtime recorded workflow_result timeout plus a wire_correlation_id and no terminal resolves. Its message names "cause timeout", the run id and the wire correlation id. Change 2: the validator returns the unresolved-terminal message instead of raising, so the exit is 1, with no Usage banner. Change 3: receipt_mode keeps workflow_result timeout (instead of rewriting it as error) when a validator refuses a timed-out run that has a wire correlation id. Change 4: the unterminalized receipt (introduced by PR 4502) now records wire_correlation_id, so receipt.json for this case holds terminal_class timeout, run_id, correlation_id and wire_correlation_id. Tests: new tests/unit/cli/test_delegate_reply_timeout_omn20386.py (3 tests, red before, green after); one existing assertion in test_delegate_pre_publish_failure_omn19131.py extended. 524 delegate and receipt tests pass. Stacked on PR 4502 (OMN-17427), which supplies the receipt writer.
Update test_an_unresolvable_terminal_refuses_and_names_the_field to assert receipt.json exists with terminal_class "failed", terminal_recorded False, and "attempts" in failure_reason. This corrects the assertion that previously checked the runs directory did not exist, which failed after commit e4f9e54 (OMN-17427) changed behavior to write receipts on all exit paths. The test still asserts DelegateTerminalUnresolvedError is raised with "attempts" in the message. test_another_nodes_run_is_still_silent is unchanged. Verified 7 tests pass in the file, 118 pass across the six touched test files, and ruff/mypy are clean.
Add test case for the new except DelegateCallerInterruptedError branch in cli_delegate.py introduced by OMN-17427 (omnibase_infra#4502). This branch handles SIGTERM/SIGHUP/SIGINT by writing an interrupted receipt and exiting with 128 + signal number. The new "caller-interrupted" case in BRANCH_CASES asserts a non-zero exit, one run directory, and a failed receipt.json with failure_reason carrying "ended by SIGTERM". 46 tests pass; ruff and mypy clean.
86f2f59 to
71a66b8
Compare
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: qwen3-review, gpt-oss-review
Models failed: none
New finding threads: 2
Deduped (already posted on this PR): 0
Below quorum (one model only, reported not threaded): 11
Nit-level findings suppressed: 2
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: 11 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/cli/cli_delegate.py(gpt-oss-review) — is_reply_timeout called with wrong argument type | In delegation_result the function is_reply_timeout is invoked with the variable 'result', which holds a ModelDelegateTerminal or None, but is_reply - [MAJOR]
src/omnibase_infra/cli/cli_delegate.py(qwen3-review) — Race condition in _settle_in_flight_receipt allows overwriting terminal receipts | The _settle_in_flight_receipt function performs a read-then-write on receipt.json without any file locking or atomic - [MAJOR]
src/omnibase_infra/cli/cli_delegate.py(qwen3-review) — DelegateCallerInterruptedError raised inside signal handler may not propagate correctly | The _on_signal handler in _caller_interrupt_guard raises DelegateCallerInterruptedError directly from the sign - [MAJOR]
src/omnibase_infra/cli/delegate_terminal_resolver.py(qwen3-review) — is_reply_timeout checks summary.get('wire_correlation_id') but envelope structure may not match | The is_reply_timeout function expects asummarydict with keysworkflow_resultand `wire_correlati - [MAJOR]
src/omnibase_infra/cli/cli_delegate.py(gpt-oss-review) — Potential race condition when settling in‑flight receipt | The _settle_in_flight_receipt function reads the receipt file without any file‑system lock while another process (or a signal handler) could - [MAJOR]
plan(gpt-oss-review) — Missing test for reply‑timeout detection path | New reply‑timeout logic introduced via is_reply_timeout and reply_timeout_error is not exercised by the test suite; the existing tests only cover pre‑pu - [MINOR]
src/omnibase_infra/cli/cli_delegate.py(qwen3-review) — Unsanitized envelope data written to receipt.json | The _write_incomplete_run_files function writes the entireenvelopedict to receipt.json via"receipt": envelope. The envelope is a dict[str, ob - [MINOR]
tests/unit/cli/test_delegate_incomplete_receipt_omn17427.py(qwen3-review) — Missing test for SIGKILL leaving in-flight receipt with concurrent terminal writer | The test suite covers SIGKILL leaving an in-flight receipt, but does not test the race condition where a terminal w - [MINOR]
src/omnibase_infra/cli/cli_delegate.py(qwen3-review) — _unterminalized_receipt_fields may produce misleading terminal_class for pre-publish failures | The _unterminalized_receipt_fields function classifies a receipt as TIMEOUT if workflow_result is 'timeo - [MINOR]
tests/unit/cli/test_delegate_incomplete_receipt_omn17427.py(qwen3-review) — Test for signal handler restoration uses identity comparison that may be fragile | The testtest_signal_handlers_are_restored_after_the_runassertssignal.getsignal(signal.SIGTERM) is before. This - [MINOR]
src/omnibase_infra/cli/receipt_mode.py(qwen3-review) — run_id parameter added to run_receipt_mode changes the function's identity semantics | The run_receipt_mode function previously always minted a fresh run_id via uuid.uuid4(). The new run_id parameter
Findings demoted from threads (anchor rejected)
-
[MINOR] hostile-reviewer (qwen3-review)
Synchronous file I/O in signal handler path may delay signal delivery | When DelegateCallerInterruptedError is caught, _write_incomplete_run_files performs multiple synchronous file writes (result.txt, receipt.json, run.json) before returning. If the caller sent SIGTERM because it has a hard deadline, these file writes may consume significant time, potentially causing the caller to escalate to SIGKILL before the process exits. The docstring acknowledges this risk ('a second wait here is how a SIGTERM become
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (gpt-oss-review)
Increased I/O due to provisional in‑flight receipt writes | Every delegation now writes an additional provisional receipt before waiting for the terminal, and may later rewrite it. This doubles the number of file writes for successful runs, potentially impacting performance on high‑throughput workloads. | Evidence: Calls to _file_in_flight_receipt before the wait and later _settle_in_flight_receipt after the wait (src/omnibase_infra/cli/cli_delegate.py lines ~560 and ~720). | Fix: Make the provisional recei
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
OMN-20386: Delegate Reply Timeout Handling
Summary
Implements gate OMN-19432 AC-1 for ticket OMN-20386. Addresses the issue where a deployed-lane delegation whose reply never arrives previously exited with code 2, printed the Usage banner, and left the run folder with only workflow_result.json.
Cause
The function
_delegate_receipt_evidence_error(the receipt validator incli_delegate.py) caught onlyDelegatePrePublishFailureError. The resolver'sDelegateTerminalUnresolvedErroris aValueError, which escapedrun_receipt_mode. Consequently,delegate_commandmappedValueErrortoclick.UsageError. The receipt writer was never reached. The brief's "error string at line 443" corresponds to the resolver sentence "delegate receipt carries no resolvable delegation terminal".Change
delegate_terminal_resolver.pygainsDelegateReplyTimeoutError(subclass ofDelegateTerminalUnresolvedError) plusis_reply_timeoutandreply_timeout_error._delegation_resultraises it whenworkflow_resultis timeout, awire_correlation_idis recorded, and no terminal resolves. The message names "cause timeout", the run id, and the wire correlation id.receipt_modekeepsworkflow_resultas timeout (not error) when a validator refuses a timed-out run that has a wire correlation id.wire_correlation_id.Verification
runs/<run_id>/receipt.jsonhasterminal_classtimeout,run_id,correlation_id, andwire_correlation_id.tests/unit/cli/test_delegate_reply_timeout_omn20386.pycontains 3 tests, all failed before (exit 2, no timeout on stderr, only an in_flight receipt) and pass after. One assertion extended intests/integration/cli/test_delegate_pre_publish_failure_omn19131.py.-k "delegate or receipt"undertests/unit/cliandtests/integration/clipass.ruffandmypyclean on the touched source.pre-commitpassed on commit.delegate_commandand realrun_receipt_mode; only the runtime is replaced by a stand-in that records result timeout and awire_correlation_id.Stack and lab
e4f9e54daf, rebased onto dev from1c387309ea) as its first commit, because the receipt writer lives there. The OMN-20386 commit is the second,1853041cb3. When 4502 lands, rebase and the first commit drops out. PR 4503 (tenant stamp) is not touched.Evidence-Ticket: OMN-20386
Evidence-Source: OCC#12561