Repository navigation
feat(OMN-9632): scaffold remote agent invoke effect - #1410
Merged
jonahgabriel merged 8 commits intoApr 27, 2026
Merged
jonahgabriel merged 8 commits into
jonahgabriel merged 8 commits into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntroduces a new remote agent invocation effect node ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as HandlerA2ATask
participant Transport as A2ASdkTransport
participant Repository as RemoteTaskStateRepository
participant EventBus as Event Bus
participant Peer as Remote A2A Peer
rect rgba(100, 150, 200, 0.5)
Note over Client,EventBus: Submit Phase
Client->>Handler: submit(command, context)
Handler->>Repository: upsert(SUBMITTED state, no handle)
Repository-->>Handler: ✓
Handler->>EventBus: publish(lifecycle: SUBMITTED)
EventBus-->>Handler: ✓
Handler->>Transport: submit_a2a_task(command.payload)
Transport->>Peer: POST /a2a/submit
Peer-->>Transport: task_handle
Transport-->>Handler: ModelA2ATaskResponse
Handler->>Repository: upsert(state with remote_handle)
Repository-->>Handler: ✓
Handler-->>Client: response
end
rect rgba(150, 200, 150, 0.5)
Note over Handler,Peer: Watch Phase (polling loop)
loop Poll until terminal status
Handler->>Transport: get_task(remote_handle)
Transport->>Peer: GET /a2a/task/{handle}
Peer-->>Transport: task_status, artifacts
Transport-->>Handler: status_response
Handler->>Handler: map_remote_status() → lifecycle_type
alt Status changed
Handler->>EventBus: publish(lifecycle event)
EventBus-->>Handler: ✓
end
alt Artifact changed
Handler->>EventBus: publish(ARTIFACT event)
EventBus-->>Handler: ✓
end
Handler->>Repository: upsert(updated state)
Repository-->>Handler: ✓
end
end
rect rgba(200, 150, 100, 0.5)
Note over Handler,Repository: Completion
Handler->>Repository: upsert(terminal status, error if failed)
Repository-->>Handler: ✓
Handler->>EventBus: publish(lifecycle: COMPLETED/FAILED)
EventBus-->>Handler: ✓
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jonahgabriel
force-pushed
the
jonah/omn-9632-task-p12-scaffold-remote-agent-invoke-effect
branch
2 times, most recently
from
April 25, 2026 16:24
9032f2d to
8758778
Compare
jonahgabriel
marked this pull request as ready for review
April 25, 2026 16:32
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/integration/test_remote_agent_invoke_effect_contract.py (1)
24-30: Prefer repo-root-based contract path to avoid CWD-sensitive tests.
CONTRACT_PATHcurrently assumes tests always run from repository root. Using__file__parent traversal is more robust and aligns with existing integration-test convention in this repo.♻️ Suggested change
-CONTRACT_PATH = ( - Path("src") - / "omnibase_infra" - / "nodes" - / "node_remote_agent_invoke_effect" - / "contract.yaml" -) +REPO_ROOT = Path(__file__).parent.parent.parent +CONTRACT_PATH = ( + REPO_ROOT + / "src" + / "omnibase_infra" + / "nodes" + / "node_remote_agent_invoke_effect" + / "contract.yaml" +)Based on learnings: In this repository’s integration tests, follow the established convention for computing
REPO_ROOTwithout calling.resolve().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_remote_agent_invoke_effect_contract.py` around lines 24 - 30, The test's CONTRACT_PATH is CWD-dependent; change it to compute REPO_ROOT by traversing from __file__ (e.g., REPO_ROOT = Path(__file__).parent.parent.parent.parent) without calling .resolve(), then set CONTRACT_PATH = REPO_ROOT / "src" / "omnibase_infra" / "nodes" / "node_remote_agent_invoke_effect" / "contract.yaml" so the path is repo-root-based and robust to different working directories; update any references to CONTRACT_PATH accordingly.tests/unit/nodes/node_remote_agent_invoke_effect/test_node_scaffold.py (1)
67-69: Minor: Test name references "p14" but PR mentions "P12".The test method is named
test_handlers_are_deferred_to_p14, but the PR summary and objectives reference "P12" as the current phase. Consider updating the name to match the actual phase reference for consistency, or clarify if handlers are indeed deferred to a later P14 phase.Suggested rename if P12 is correct
- def test_handlers_are_deferred_to_p14(self) -> None: + def test_handlers_are_deferred(self) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_remote_agent_invoke_effect/test_node_scaffold.py` around lines 67 - 69, Rename the test method test_handlers_are_deferred_to_p14 to match the PR's phase naming (e.g., test_handlers_are_deferred_to_p12) so names align with the PR intent; update the method name in tests/unit/nodes/node_remote_agent_invoke_effect/test_node_scaffold.py (and any references to test_handlers_are_deferred_to_p14 such as in test discovery or related docstrings) or alternatively confirm and document if the test really targets P14 and leave the name as-is.tests/integration/nodes/test_node_remote_agent_invoke_effect_integration.py (2)
140-147: Consider documenting thecreated_topics.clear()workaround.The
created_topics.clear()call prevents the topic manager from deleting the contract topic during cleanup, which is intentional for integration tests against canonical topics. A brief comment explaining this would help future maintainers understand why this is necessary.Suggested documentation
async def _ensure_contract_topic_exists(topic: str, bootstrap_servers: str) -> None: """Create the contract topic if absent without deleting canonical topics.""" manager = KafkaTopicManager(bootstrap_servers) try: await manager.create_topic(topic, partitions=1, replication_factor=1) + # Clear created_topics so cleanup() won't delete the canonical contract topic manager.created_topics.clear() finally: await manager.cleanup()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/nodes/test_node_remote_agent_invoke_effect_integration.py` around lines 140 - 147, Add a short comment above the created_topics.clear() call in _ensure_contract_topic_exists explaining that clearing manager.created_topics is an intentional workaround to prevent KafkaTopicManager.cleanup() from deleting the contract topic used by integration tests against canonical topics; reference KafkaTopicManager, the create_topic call, and the cleanup() invocation so maintainers know this is deliberate and tied to test isolation.
308-310: Consider increasing the timeout for CI stability.The 1-second timeout on line 310 may be tight for CI environments under load. The Kafka-based test uses
timeout=10.0(line 227). Consider using a slightly longer timeout for the in-memory test as well for consistency and to reduce flakiness.Suggested change
- await asyncio.wait_for(lifecycle_received.wait(), timeout=1.0) + await asyncio.wait_for(lifecycle_received.wait(), timeout=5.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/nodes/test_node_remote_agent_invoke_effect_integration.py` around lines 308 - 310, Increase the asyncio.wait_for timeout used when awaiting lifecycle_received.wait() to reduce CI flakiness; locate the call to asyncio.wait_for(lifecycle_received.wait(), timeout=1.0) and change the timeout value to a larger value (e.g., 10.0) to match the Kafka-based test's timeout and provide more headroom in CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/nodes/test_node_remote_agent_invoke_effect_integration.py`:
- Around line 140-147: Add a short comment above the created_topics.clear() call
in _ensure_contract_topic_exists explaining that clearing manager.created_topics
is an intentional workaround to prevent KafkaTopicManager.cleanup() from
deleting the contract topic used by integration tests against canonical topics;
reference KafkaTopicManager, the create_topic call, and the cleanup() invocation
so maintainers know this is deliberate and tied to test isolation.
- Around line 308-310: Increase the asyncio.wait_for timeout used when awaiting
lifecycle_received.wait() to reduce CI flakiness; locate the call to
asyncio.wait_for(lifecycle_received.wait(), timeout=1.0) and change the timeout
value to a larger value (e.g., 10.0) to match the Kafka-based test's timeout and
provide more headroom in CI.
In `@tests/integration/test_remote_agent_invoke_effect_contract.py`:
- Around line 24-30: The test's CONTRACT_PATH is CWD-dependent; change it to
compute REPO_ROOT by traversing from __file__ (e.g., REPO_ROOT =
Path(__file__).parent.parent.parent.parent) without calling .resolve(), then set
CONTRACT_PATH = REPO_ROOT / "src" / "omnibase_infra" / "nodes" /
"node_remote_agent_invoke_effect" / "contract.yaml" so the path is
repo-root-based and robust to different working directories; update any
references to CONTRACT_PATH accordingly.
In `@tests/unit/nodes/node_remote_agent_invoke_effect/test_node_scaffold.py`:
- Around line 67-69: Rename the test method test_handlers_are_deferred_to_p14 to
match the PR's phase naming (e.g., test_handlers_are_deferred_to_p12) so names
align with the PR intent; update the method name in
tests/unit/nodes/node_remote_agent_invoke_effect/test_node_scaffold.py (and any
references to test_handlers_are_deferred_to_p14 such as in test discovery or
related docstrings) or alternatively confirm and document if the test really
targets P14 and leave the name as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2360817f-392c-45d3-99f1-0588db56c3a7
⛔ Files ignored due to path filters (1)
src/omnibase_infra/enums/generated/enum_omnibase_infra_topic.pyis excluded by!**/generated/**
📒 Files selected for processing (12)
src/omnibase_infra/nodes/node_remote_agent_invoke_effect/__init__.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/contract.yamlsrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/handlers/__init__.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/__init__.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/enum_agent_task_status.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/model_agent_task_lifecycle_event.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/model_invocation_command.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/node.pytests/integration/nodes/test_node_remote_agent_invoke_effect_integration.pytests/integration/test_remote_agent_invoke_effect_contract.pytests/unit/nodes/node_remote_agent_invoke_effect/__init__.pytests/unit/nodes/node_remote_agent_invoke_effect/test_node_scaffold.py
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/check_subscribe_wiring_health.py (1)
77-78:⚠️ Potential issue | 🟡 MinorUpdate baseline metadata count after adding this allowlist entry.
_BASELINE_DEAD_LETTER_ALLOWLISTnow has one additional topic, but the header comment still says45 entries. Please update the baseline metadata to keep debt tracking accurate.Suggested patch
-# Current baseline: 2026-04-10 (45 entries) +# Current baseline: 2026-04-26 (46 entries)Also applies to: 156-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check_subscribe_wiring_health.py` around lines 77 - 78, The header comment above the _BASELINE_DEAD_LETTER_ALLOWLIST variable still shows "45 entries" even though one topic was added; update that metadata count to "46 entries" wherever the header appears (including the duplicate header instance near the other allowlist block referenced), ensuring both header comments correctly reflect the new total for _BASELINE_DEAD_LETTER_ALLOWLIST.
🧹 Nitpick comments (1)
src/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/remote_task_state_repository.py (1)
235-272: Consider operation-specific error codes for read operations.
_run_checkedusesEnumPostgresErrorCode.UPSERT_ERRORfor all operations including reads (get,get_by_remote_task_handle,load_unfinished). While this works, using a more generic error code or operation-specific codes might improve error diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/remote_task_state_repository.py` around lines 235 - 272, Change _run_checked to accept an op_error_code parameter (type EnumPostgresErrorCode) instead of hardcoding EnumPostgresErrorCode.UPSERT_ERROR, and pass that value into the call to _execute_postgres_op; update callers (get, get_by_remote_task_handle, load_unfinished) to supply an appropriate operation-specific code (e.g., EnumPostgresErrorCode.SELECT_ERROR or a generic READ_ERROR) so read operations produce correct diagnostics; reference symbols: _run_checked, _execute_postgres_op, EnumPostgresErrorCode, and the caller methods get/get_by_remote_task_handle/load_unfinished.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/omnibase_infra/nodes/node_remote_agent_invoke_effect/handlers/handler_a2a_task.py`:
- Around line 313-315: Wrap both calls to self._transport.submit(...) and
self._transport.watch(...) in try/except blocks that catch transport/SDK
exceptions and re-raise them using ModelInfraErrorContext.with_correlation(...),
passing the current correlation_id, transport_type, and the operation name
("submit" for the submit call and "watch" for the watch call); ensure you
preserve the original exception as the cause when re-raising so stack traces are
retained. Use the existing correlation_id value in scope (or
request_model.correlation_id if that’s the source) and the transport type from
self._transport (or its .type attribute) when calling
ModelInfraErrorContext.with_correlation to provide full infra error context.
Ensure both locations (the submit call around line ~313 and the watch call
around line ~348) follow the same pattern.
- Around line 256-275: HandlerA2ATask currently takes and stores _event_bus and
_lifecycle_topic and calls publish() directly; remove direct event-bus usage
from the handler by deleting the event_bus and lifecycle_topic
parameters/attributes (and any calls to publish()) in HandlerA2ATask
(constructor and all methods that call publish()), and instead surface lifecycle
emission to the caller layer: either accept an abstract LifecyclePublisher
interface (e.g., LifecyclePublisher.publish_event(...)) injected by the
orchestration/service layer or remove publishing entirely and rely on the
external orchestrator to subscribe to repository state changes and publish
lifecycle events; update any call sites that construct HandlerA2ATask to use the
new publisher or to publish after invoking the handler, and ensure references to
_event_bus, _lifecycle_topic, and publish() are removed from HandlerA2ATask
(also apply same change where _event_bus is used around lines ~496-500).
- Around line 290-325: The current flow creates and persists a
ModelRemoteTaskState with status SUBMITTED and emits SUBMITTED before calling
self._transport.submit, which can leave a phantom or stale SUBMITTED row if
submit() fails or returns a different lifecycle; instead, change the sequence in
the handler_a2a_task logic so you only persist/emit SUBMITTED after a successful
transport call that confirms the remote_task_handle/status (or persist an
intermediate PENDING state if you must record intent), i.e., call
self._transport.submit(...) first, capture response.remote_task_handle and
response.status, then upsert the ModelRemoteTaskState with the correct
remote_task_handle and last_remote_status (and emit SUBMITTED only if
response.status indicates SUBMITTED/ACCEPTED as appropriate), and ensure
exceptions from _transport.submit are handled to avoid leaving the repository in
SUBMITTED — update references to ModelRemoteTaskState, _transport.submit,
_repository.upsert, and _emit_submitted accordingly.
In `@tests/integration/nodes/test_node_remote_agent_invoke_effect_integration.py`:
- Around line 154-157: The integration test function
test_remote_agent_invoke_contract_wires_real_kafka_dispatch is missing the
`@pytest.mark.serial` marker; update its decorator list to include
`@pytest.mark.serial` alongside `@pytest.mark.integration`, `@pytest.mark.kafka`, and
`@pytest.mark.asyncio` so the test runs non-parallel and avoids flaky
message-stealing with the real Kafka contract topic and deterministic consumer
group.
---
Outside diff comments:
In `@scripts/check_subscribe_wiring_health.py`:
- Around line 77-78: The header comment above the
_BASELINE_DEAD_LETTER_ALLOWLIST variable still shows "45 entries" even though
one topic was added; update that metadata count to "46 entries" wherever the
header appears (including the duplicate header instance near the other allowlist
block referenced), ensuring both header comments correctly reflect the new total
for _BASELINE_DEAD_LETTER_ALLOWLIST.
---
Nitpick comments:
In
`@src/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/remote_task_state_repository.py`:
- Around line 235-272: Change _run_checked to accept an op_error_code parameter
(type EnumPostgresErrorCode) instead of hardcoding
EnumPostgresErrorCode.UPSERT_ERROR, and pass that value into the call to
_execute_postgres_op; update callers (get, get_by_remote_task_handle,
load_unfinished) to supply an appropriate operation-specific code (e.g.,
EnumPostgresErrorCode.SELECT_ERROR or a generic READ_ERROR) so read operations
produce correct diagnostics; reference symbols: _run_checked,
_execute_postgres_op, EnumPostgresErrorCode, and the caller methods
get/get_by_remote_task_handle/load_unfinished.
🪄 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: a81054f2-c44e-4f29-ad28-5b87ecde920a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
docker/migrations/forward/069_create_remote_task_state.sqldocker/migrations/rollback/rollback_069_create_remote_task_state.sqlpyproject.tomlscripts/check_subscribe_wiring_health.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/contract.yamlsrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/handlers/__init__.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/handlers/handler_a2a_task.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/__init__.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/remote_task_state_repository.pysrc/omnibase_infra/runtime/version_compatibility.pytests/integration/db/test_remote_task_state_repository.pytests/integration/nodes/test_node_remote_agent_invoke_effect_integration.pytests/unit/contracts/test_protocol_ownership.pytests/unit/db/test_migration_069.pytests/unit/nodes/node_remote_agent_invoke_effect/test_handler_a2a_task.pytests/unit/nodes/node_remote_agent_invoke_effect/test_node_scaffold.py
✅ Files skipped from review due to trivial changes (5)
- docker/migrations/rollback/rollback_069_create_remote_task_state.sql
- docker/migrations/forward/069_create_remote_task_state.sql
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/init.py
- tests/unit/db/test_migration_069.py
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/contract.yaml
- Contract module references changed from omnibase_core.models.delegation (not yet merged) to node-local models package - Remove unsupported 'persistence' field from contract.yaml - Add EnumAgentTaskStatus, ModelInvocationCommand, ModelAgentTaskLifecycleEvent as importable stubs satisfying the contract linter import check - Add integration test at tests/integration/ (glob-visible to CI gate) - Replace persistence assertion in unit test with module-reference assertion Closes OMN-9632.
* feat(OMN-9633): add remote task state repository * fix(OMN-9633): normalize postgres retry typing * fix(OMN-9633): bump core pin to merged delegation models Bumps omnibase-core pin from 9d64d5a3 to 6a65f885 (post-OMN-9637 merge of PR 912) so ModelRemoteTaskState is importable. Also adjusts test imports in tests/integration/nodes/test_node_remote_agent_invoke_effect_integration.py to use full module paths since the delegation package __init__.py does not re-export the model classes. OMN-9633 --------- Co-authored-by: jonahgabriel <jonahgabriel@users.noreply.github.com>
* feat(OMN-9634): implement A2A submit handler
* feat(OMN-9635): implement A2A watch lifecycle loop
* feat(OMN-9634): use real a2a sdk transport
* fix(OMN-9634): align A2A handler with core delegation models
The A2A handler/tests were written against the older ModelA2ATaskResponse
shape (status: str, ModelTargetAgent.agent_ref, etc). The merged core
delegation models (PR omnibase_core#912) tightened the schema:
- ModelA2ATaskResponse.status -> EnumAgentTaskLifecycleType
- ModelTargetAgent fields: target_ref + base_url + protocol (no agent_ref,
no protocol_version)
Map raw A2A peer status -> typed lifecycle enum at the SDK boundary in
_task_to_response and stop double-mapping inside watch(). Persist the raw
peer status string into ModelRemoteTaskState.last_remote_status via
response.status.value so the existing str | None field stays satisfied.
Other changes required to unblock the suite:
- bump omnibase-core pin to OMN-9637 branch HEAD (delegation models)
- bump dep spec + version_compatibility _FALLBACK_MATRIX to 0.40/<0.41
so the auto-derived runtime VERSION_MATRIX accepts the new core
- register ProtocolA2ATransport in KNOWN_INFRA_PROTOCOLS (test_protocol_ownership)
- flip test_handlers_are_deferred_to_p14 -> test_p14_handler_is_wired
now that this PR _is_ P14 and the handler is registered in contract.yaml
- add remote-agent-invoke topic to _BASELINE_DEAD_LETTER_ALLOWLIST
(publisher node_delegation_orchestrator pending in OMN-9620 epic)
- integration test imports delegation models from each module directly
instead of the package level (the pinned core branch's
delegation/__init__.py is still empty)
---------
Co-authored-by: jonahgabriel <jonahgabriel@users.noreply.github.com>
Drift discovered during rebase onto origin/main: the new node was present in src/ but missing from [project.entry-points."onex.nodes"]. Required by tests/scripts/test_generate_entry_points.py. OMN-9632
jonahgabriel
force-pushed
the
jonah/omn-9632-task-p12-scaffold-remote-agent-invoke-effect
branch
from
April 27, 2026 07:28
d3e8e3a to
b7f5b1e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/remote_task_state_repository.py (1)
248-253: Consider operation-specific error codes for non-upsert operations.
_run_checkedalways passesEnumPostgresErrorCode.UPSERT_ERRORregardless of the actual operation (get,get_by_handle,load_unfinished). While connection/auth errors are still properly classified by the mixin, this could make error categorization less precise for operation failures.If the enum doesn't have a
QUERY_ERRORor similar, this is acceptable for now, but consider adding one if query-specific error classification becomes important.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/remote_task_state_repository.py` around lines 248 - 253, In _run_checked, don't always pass EnumPostgresErrorCode.UPSERT_ERROR to _execute_postgres_op; instead determine an operation-specific error code based on the caller/operation (e.g., use EnumPostgresErrorCode.QUERY_ERROR or EnumPostgresErrorCode.GET_ERROR for get/get_by_handle/load_unfinished) and pass that into _execute_postgres_op so failures are categorized correctly; if EnumPostgresErrorCode lacks a suitable value, add a QUERY_ERROR (or GET_ERROR/LOAD_UNFINISHED_ERROR) member to EnumPostgresErrorCode and use it when invoking _execute_postgres_op from _run_checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/remote_task_state_repository.py`:
- Around line 248-253: In _run_checked, don't always pass
EnumPostgresErrorCode.UPSERT_ERROR to _execute_postgres_op; instead determine an
operation-specific error code based on the caller/operation (e.g., use
EnumPostgresErrorCode.QUERY_ERROR or EnumPostgresErrorCode.GET_ERROR for
get/get_by_handle/load_unfinished) and pass that into _execute_postgres_op so
failures are categorized correctly; if EnumPostgresErrorCode lacks a suitable
value, add a QUERY_ERROR (or GET_ERROR/LOAD_UNFINISHED_ERROR) member to
EnumPostgresErrorCode and use it when invoking _execute_postgres_op from
_run_checked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ef8f279-d233-4e07-87f3-8c6494dd0085
⛔ Files ignored due to path filters (2)
src/omnibase_infra/enums/generated/enum_omnibase_infra_topic.pyis excluded by!**/generated/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
pyproject.tomlscripts/check_subscribe_wiring_health.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/__init__.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/contract.yamlsrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/handlers/__init__.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/handlers/handler_a2a_task.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/__init__.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/enum_agent_task_status.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/model_agent_task_lifecycle_event.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/model_invocation_command.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/node.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/__init__.pysrc/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/remote_task_state_repository.pysrc/omnibase_infra/runtime/version_compatibility.pytests/integration/db/test_remote_task_state_repository.pytests/integration/nodes/test_node_remote_agent_invoke_effect_integration.pytests/integration/test_remote_agent_invoke_effect_contract.pytests/unit/contracts/test_protocol_ownership.pytests/unit/nodes/node_remote_agent_invoke_effect/__init__.pytests/unit/nodes/node_remote_agent_invoke_effect/test_handler_a2a_task.pytests/unit/nodes/node_remote_agent_invoke_effect/test_node_scaffold.py
✅ Files skipped from review due to trivial changes (12)
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/init.py
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/persistence/init.py
- scripts/check_subscribe_wiring_health.py
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/init.py
- tests/unit/contracts/test_protocol_ownership.py
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/enum_agent_task_status.py
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/handlers/init.py
- src/omnibase_infra/runtime/version_compatibility.py
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/model_agent_task_lifecycle_event.py
- tests/unit/nodes/node_remote_agent_invoke_effect/init.py
- tests/unit/nodes/node_remote_agent_invoke_effect/test_node_scaffold.py
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/contract.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/models/model_invocation_command.py
- src/omnibase_infra/nodes/node_remote_agent_invoke_effect/node.py
- pyproject.toml
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Address CodeRabbit review threads on PR #1410: - Wrap transport.submit() in try/except: on failure, mark repository row FAILED with error and re-raise as InfraConnectionError with ModelInfraErrorContext.with_correlation (operation=a2a_submit, transport_type=HTTP). Prevents phantom SUBMITTED rows and preserves correlation_id traceability. - Wrap transport.get_task() in watch() with the same error-context wrapping (operation=a2a_watch). - Add @pytest.mark.serial to the Kafka integration test so deterministic consumer-group assertions don't race parallel workers. - Replace Ellipsis (...) Protocol method bodies with docstrings to satisfy CodeQL "statement has no effect" findings. - Add unit tests covering submit-failure cleanup and watch-failure error-context wrapping.
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Apr 27, 2026
This was referenced Apr 27, 2026
jonahgabriel
added this pull request to the merge queue
Apr 27, 2026
jonahgabriel
deleted the
jonah/omn-9632-task-p12-scaffold-remote-agent-invoke-effect
branch
April 27, 2026 11:55
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
2 participants
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
node_remote_agent_invoke_effectas a declarative effect nodecontract.yamlfor remote-agent invoke commands and agent task lifecycle eventstests/integration/satisfying CI coverage gateNotes
Verification
uv run pytest tests/unit/nodes/node_remote_agent_invoke_effect/ tests/integration/test_remote_agent_invoke_effect_contract.py -v— 13 passedCloses OMN-9632.
Summary by CodeRabbit
Release Notes
New Features
Chores
omnibase-coredependency to version 0.40.0 and introduceda2a-sdkdependency.Tests