Repository navigation
feat(OMN-14589): canonicalize node_coding_agent_invoke_effect to def-B - #2302
Merged
jonahgabriel merged 3 commits intoJul 14, 2026
Merged
jonahgabriel merged 3 commits into
jonahgabriel merged 3 commits into
Conversation
RSD mechanical-wave canary (path (b), OMN-14355/14371 pattern): flip HandlerCodingAgentInvoke.handle() from the def-A envelope-wrapping shape (handle(envelope: ModelEventEnvelope[T]) -> ModelHandlerOutput[None]) to the canonical def-B shape (handle(command: ModelCodingAgentInvokeCommand) -> ModelCodingAgentResult). The contract already declares the typed event_model; the runtime's _make_dispatch_callback already validates and passes the typed payload directly once the handler no longer accepts an envelope-shaped parameter, and DispatchResultApplier already unwraps an envelope-shaped output event to its inner payload before publish — so this is byte-for-byte behavior-identical on the real dispatch path (confirmed by the existing test_effect_result_publishes_to_invoke_completed_with_result_payload, unmodified, still green). Proof (per the canon-shape ratchet's 3-part flip gate, scripts/ci/canonical_handler_shape.py::verify_flip_receipt): - RED->GREEN: classify_node() on the pre-change file = envelope_in_core (non-canonical); on the post-change file = canonical (magic:command). - Adequacy receipt (scripts/ci/adequacy_receipts/*.json): 85.45% branch coverage of the handler module, measured from the node's existing 37-test suite (golden chain + subprocess/git probes + real dispatch multitopic). - Equivalence: 7 scenarios replayed through both the pre-flip envelope-wrapping handle() body and the post-flip handle(command), confirmed byte-equivalent (duration_ms masked); recorded as goldens under tests/fixtures/golden/node_coding_agent_invoke_effect/ with a durable replay regression test (test_invoke_effect_golden_equivalence.py) and the paired .equivalence.json artifact (same selected_input_hashes as the receipt). verify_flip_receipt() returns True end-to-end against these. Vendors scripts/ci/adequacy_receipt.py + compute_golden.py (per the omnimarket OMN-14371 canary's per-repo convention — the recorder/comparator libraries are vendored locally, the classifier script itself stays centralized in omnibase_core). Scope note: node_coding_agent_fsm_reducer (the ticket's second target) is NOT converted here. Its handle() currently returns TWO reducer projections (FSM state + trace projection) via ModelHandlerOutput.for_reducer(...); tracing the live runtime wiring (_make_dispatch_callback/_normalize_handler_result in auto_wiring/handler_wiring.py) shows no code path reads ModelHandlerOutput.projections or constructs ModelProjectionIntent from a bare typed return, so the ticket's assumed adapter-side reconstruction of for_reducer(projections=(...)) from a single typed return does not exist yet. Converting to a single-model return would either silently drop the trace projection or require inventing new adapter behavior outside this ticket's mechanical scope. Flagged to the team lead rather than forced. Evidence: OMN-14589.
📝 WalkthroughWalkthroughAdds CI utilities for deterministic golden comparisons and coverage adequacy receipts, updates the coding-agent handler to the canonical typed contract, records adequacy and equivalence artifacts, and adds scenario fixtures with handler equivalence tests. ChangesCanonical compute-node validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GoldenFixture
participant HandlerCodingAgentInvoke
participant ModelCodingAgentResult
participant compare_output
GoldenFixture->>HandlerCodingAgentInvoke: build command and call handle(command)
HandlerCodingAgentInvoke->>ModelCodingAgentResult: return typed result
GoldenFixture->>compare_output: compare recorded output with fresh result
compare_output-->>GoldenFixture: masked structural differences
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/ci/compute_golden.py (1)
35-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeep copy via JSON round-trip works but is indirect.
json.loads(json.dumps(payload))is a valid deep-copy for JSON-safe dicts, butcopy.deepcopy(payload)is more direct, avoids re-serialization cost, and doesn't risk surprises with non-JSON-native types ifapply_maskis ever reused on non-model_dump(mode="json")payloads.♻️ Optional simplification
+import copy + def apply_mask(payload: dict[str, Any], volatile_mask: list[str]) -> dict[str, Any]: """Return a deep copy of ``payload`` with each dotted-path volatile field removed. ... """ - masked: dict[str, Any] = json.loads(json.dumps(payload)) # deep copy via round-trip + masked: dict[str, Any] = copy.deepcopy(payload) for dotted in volatile_mask: _remove_path(masked, dotted.split(".")) return masked🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/compute_golden.py` around lines 35 - 45, The deep-copy implementation in apply_mask should use copy.deepcopy(payload) instead of the JSON serialization round-trip. Add or reuse the standard copy module import, preserve the existing dict shape and _remove_path processing, and avoid changing masking behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci/adequacy_receipt.py`:
- Around line 259-270: Update the ModelUncoveredWaiver construction in the
coverage-miss path to set uncovered_branch_count from the actual missing-branch
total reported by the coverage data, replacing the hardcoded 0. Preserve the
existing waiver reason and behavior for coverage targets that are met.
---
Nitpick comments:
In `@scripts/ci/compute_golden.py`:
- Around line 35-45: The deep-copy implementation in apply_mask should use
copy.deepcopy(payload) instead of the JSON serialization round-trip. Add or
reuse the standard copy module import, preserve the existing dict shape and
_remove_path processing, and avoid changing masking behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9810cc4d-4f26-49ef-8961-1ac046f5c0ba
📒 Files selected for processing (16)
scripts/ci/__init__.pyscripts/ci/adequacy_receipt.pyscripts/ci/adequacy_receipts/omnibase_infra.nodes.node_coding_agent_invoke_effect.equivalence.jsonscripts/ci/adequacy_receipts/omnibase_infra.nodes.node_coding_agent_invoke_effect.jsonscripts/ci/compute_golden.pysrc/omnibase_infra/nodes/node_coding_agent_invoke_effect/contract.yamlsrc/omnibase_infra/nodes/node_coding_agent_invoke_effect/handlers/handler_coding_agent_invoke.pytests/fixtures/golden/node_coding_agent_invoke_effect/empty_response.jsontests/fixtures/golden/node_coding_agent_invoke_effect/subprocess_error.jsontests/fixtures/golden/node_coding_agent_invoke_effect/success_claude_read_only.jsontests/fixtures/golden/node_coding_agent_invoke_effect/success_codex_workspace_write.jsontests/fixtures/golden/node_coding_agent_invoke_effect/timeout.jsontests/fixtures/golden/node_coding_agent_invoke_effect/unavailable_binary_missing.jsontests/fixtures/golden/node_coding_agent_invoke_effect/unavailable_cred_home_unset.jsontests/unit/nodes/node_coding_agent/test_golden_chain_coding_agent.pytests/unit/nodes/node_coding_agent/test_invoke_effect_golden_equivalence.py
jonahgabriel
deleted the
jonah/omn-14589-canonicalize-node_coding_agent_invoke_effect
branch
July 14, 2026 03:47
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.
Summary
RSD mechanical-wave canary (path (b)): flips
HandlerCodingAgentInvoke.handle()from the def-A envelope-wrapping shape
(
handle(envelope: ModelEventEnvelope[T]) -> ModelHandlerOutput[None]) to thecanonical def-B shape (
handle(command: ModelCodingAgentInvokeCommand) -> ModelCodingAgentResult), per the OMN-14355 canonical handler-shape ratchet.event_model(
ModelCodingAgentInvokeCommand); the runtime's_make_dispatch_callback(
omnibase_infra/runtime/auto_wiring/handler_wiring.py) already validates andpasses the typed payload directly once the handler no longer accepts an
envelope-shaped parameter (
_handler_accepts_event_envelopeinspects thesignature).
DispatchResultApplier._publish_payload_for_output_eventalready unwraps anenvelope-shaped output event to its inner payload before publish — so
returning the bare
ModelCodingAgentResultis byte-for-bytebehavior-identical to the prior inner-envelope-wrapping form on the real
dispatch path. Confirmed by the existing
test_effect_result_publishes_to_invoke_completed_with_result_payload,unmodified, still green.
Proof (canon-shape ratchet 3-part flip gate:
canonical_handler_shape.py::verify_flip_receipt)classify_node()on the pre-change file =envelope_in_core(non-canonical); on the post-change file =canonical(
magic:command).scripts/ci/adequacy_receipts/omnibase_infra.nodes.node_coding_agent_invoke_effect.json):85.45% branch coverage of the handler module, measured from the node's
existing 37-test suite (golden chain + subprocess/git probes + real dispatch
multitopic).
envelope-wrapping
handle()body and the post-fliphandle(command),confirmed byte-equivalent (
duration_msmasked as volatile); recorded asgoldens under
tests/fixtures/golden/node_coding_agent_invoke_effect/with adurable replay regression test
(
test_invoke_effect_golden_equivalence.py) and the paired.equivalence.jsonartifact (sameselected_input_hashesas the receipt).verify_flip_receipt()returnsTrueend-to-end against these artifacts.Vendors
scripts/ci/adequacy_receipt.py+compute_golden.py(per theomnimarket OMN-14371 canary's per-repo convention: the recorder/comparator
libraries are vendored locally per-repo; the classifier script itself stays
centralized in
omnibase_core).Scope note —
node_coding_agent_fsm_reducerNOT converted hereThe ticket's second target (
node_coding_agent_fsm_reducer) is intentionallyleft unconverted. Its
handle()currently returns two reducer projections(FSM state + trace projection) via
ModelHandlerOutput.for_reducer(...).Tracing the live runtime wiring (
_make_dispatch_callback/_normalize_handler_resultinruntime/auto_wiring/handler_wiring.py) showsno code path reads
ModelHandlerOutput.projectionsor constructs aModelProjectionIntentfrom a bare typed return — so the ticket's assumed"adapter wraps a plain return into
for_reducer(projections=(...))" does notexist in the current runtime. Converting to a single-model return per the
ticket's literal target snippet would either silently drop the trace
projection or require inventing new adapter behavior outside this ticket's
declared mechanical scope. Flagged to the team lead rather than forced through
— see OMN-14589 for follow-up.
Depends on / should be reconciled with OMN-14587 (#2300), which freezes the
omnibase_infra canon-shape baseline including this node as non-canonical
pre-flip; once both land, the baseline's
NON_CANONICALtuple should drop thisnode's entry (via
--updateor a one-line removal) to record the proven flip.Test plan
uv run pytest tests/unit/nodes/node_coding_agent/ -v -m unit— 57 passeduv run ruff format --check/ruff check/mypy— cleanpre-commit run --all-files(targeted) — all hooks passedverify_flip_receipt()(canonical_handler_shape.py) returns(True, "recomputed-meets-target+equivalence-pass")against thecommitted receipt + equivalence artifact
handle()output is unchangedacross 7 scenarios (success x2, timeout, subprocess error, empty
response, unavailable x2)
Evidence-Source: OCC#4132
Evidence-Ticket: OMN-14589
Evidence-Commit: b5963486f45fe6287bdb633b25e2a44b1955a58a
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Evidence-Superseded-Product-Head: 1011fcb
Evidence-Current-Product-Head: 6477d6a
Evidence-Superseded-Product-Head: cc51e48