feat(scheduled_action): support remote agents as a schedulable ActionKind - #5612
feat(scheduled_action): support remote agents as a schedulable ActionKind#5612SergiorCode wants to merge 4 commits into
Conversation
…Kind Adds ActionKind::RemoteAgent so an agent running outside Macro can be scheduled like a native one. Macro stays the scheduler, the system of record and the UI; the remote endpoint is just another executor. The kind column is plain TEXT and task is untyped JSONB, so no migration and no .sqlx changes are needed. Execution mirrors the existing kind: the run chat is created up front, the prompt is stored as the user message, the endpoint's reply is stored as the assistant message, and the completion notification and execution record are unchanged — which is what lets the Agents UI work for remote agents without a client change. RemoteAgentClient is a port in domain/ports.rs; the reqwest adapter lives in outbound/remote_agent_http.rs and owns the transport concerns. The endpoint URL is user-supplied, so it gets HTTPS-only, no redirects, a request timeout, a capped response body, and rejection of hosts that resolve to private, loopback, link-local or metadata addresses — mirrored from crates/webhook/src/outbound/http_validator.rs, whose validator is pub(super) and so not reachable from this service. That copy also fixes a gap in the original: IPv4-mapped IPv6 addresses are folded back onto the IPv4 rules, so ::ffff:169.254.169.254 is blocked. Request signing is deliberately not included — that is question 1 on the issue, and it is the only part that needs a maintainer decision about where the secret lives. The port is shaped so signing slots into the adapter without touching the executor. On the web side the getAgentTask cast is now guarded by the kind discriminant instead of being unconditional, and the agent editor refuses to update an action of another kind rather than overwriting its task payload.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe scheduled action service now supports 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…he DNS gap
Two follow-ups on the same change:
- The agent editor preserved the stored kind but still overwrote `task` with
an Agent-shaped payload. Since an update is a full overwrite, editing a
RemoteAgent action from that editor would have written `{model, prompt,
user_prompt}` under `kind: "RemoteAgent"`, dropping `endpoint_url` and
failing every later run with "invalid remote agent task definition" — with
no way to restore it from the UI. Name, schedule and enabled still apply to
every kind; only the task payload is now kind-gated.
- Document the DNS-rebinding window between validating the resolved address
and the request resolving the host again. The webhook validator this mirrors
has the identical gap, so both are left consistent rather than diverging.
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
services/scheduled_action/src/outbound/remote_agent_http.rs (1)
27-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse
rootcausefor the new remote-agent error path.The
RemoteAgentClienttrait and executor call chain currently returnanyhow::Result. Update those signatures, addrootcausetoservices/scheduled_action/Cargo.toml, and replace the newanyhowconversions and context calls.🤖 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 `@services/scheduled_action/src/outbound/remote_agent_http.rs` around lines 27 - 28, Replace anyhow with rootcause throughout the new remote-agent error path: update the RemoteAgentClient trait and executor call-chain signatures in services/scheduled_action/src/outbound/remote_agent_http.rs (lines 27-28) and services/scheduled_action/src/outbound/inprocess_executor/remote_agent_task.rs (line 11), replace newly added anyhow conversions and context calls, and add rootcause to services/scheduled_action/Cargo.toml.Source: Coding guidelines
🤖 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 `@services/scheduled_action/src/outbound/inprocess_executor/mod.rs`:
- Around line 84-89: Update the setup flow around create_run_chat in the
ActionKind match so any error from chat creation explicitly calls release_action
before returning the error. Preserve the existing successful chat_id path and
ensure both Agent and RemoteAgent claims are released when run-chat setup fails.
In `@services/scheduled_action/src/outbound/remote_agent_http.rs`:
- Around line 244-267: Update is_blocked_ip to reject the RFC6598 shared IPv4
range 100.64.0.0/10, while preserving the existing mapped-IPv6 handling and
other public-endpoint exclusions. Add a regression assertion in
services/scheduled_action/src/outbound/remote_agent_http/test.rs at lines 30-34
verifying that 100.64.0.1 is blocked.
---
Nitpick comments:
In `@services/scheduled_action/src/outbound/remote_agent_http.rs`:
- Around line 27-28: Replace anyhow with rootcause throughout the new
remote-agent error path: update the RemoteAgentClient trait and executor
call-chain signatures in
services/scheduled_action/src/outbound/remote_agent_http.rs (lines 27-28) and
services/scheduled_action/src/outbound/inprocess_executor/remote_agent_task.rs
(line 11), replace newly added anyhow conversions and context calls, and add
rootcause to services/scheduled_action/Cargo.toml.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07322bcb-68ee-4321-a21c-35c7aaa6b1fd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (13)
apps/web/src/features/block-automation/component/automationUtils.tsservices/scheduled_action/Cargo.tomlservices/scheduled_action/src/bins/service.rsservices/scheduled_action/src/domain/models.rsservices/scheduled_action/src/domain/ports.rsservices/scheduled_action/src/outbound.rsservices/scheduled_action/src/outbound/inprocess_executor/mod.rsservices/scheduled_action/src/outbound/inprocess_executor/remote_agent_task.rsservices/scheduled_action/src/outbound/pg_scheduled_action_repo.rsservices/scheduled_action/src/outbound/pg_scheduled_action_repo/test.rsservices/scheduled_action/src/outbound/remote_agent_http.rsservices/scheduled_action/src/outbound/remote_agent_http/test.rsservices/scheduled_action/src/swagger.rs
…RFC 6598 Both from review: - execute_action claims the action before creating the run chat, and only the spawned run releases it. If chat creation failed, the claim survived until MAX_ACTION_TIME (20 minutes) and the action could not be run again in the meantime. Release it on that path. Pre-existing, but this change touches the same lines and both kinds go through it. - Block RFC 6598 shared address space (100.64.0.0/10) in the endpoint address rules; it is routable inside carrier-grade NAT and some cloud networks. Ipv4Addr::is_shared is still unstable, so the prefix is checked directly. Not taken: converting the new error path to rootcause. This service is on anyhow end to end, and the inbound error type downcasts anyhow::Error to map AlreadyRunningError onto 409, so a single rootcause path here would either break that mapping or leave two error idioms in one service. Worth doing as a service-wide change rather than in a feature PR.
|
On the
The two actionable comments are fixed in the same commit; replies are on their threads. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@services/scheduled_action/src/outbound/inprocess_executor/remote_agent_task.rs`:
- Around line 48-53: Update the successful response handling in the remote task
executor so empty or whitespace-only response.output values are not returned
early; store every successful response with store_assistant_message and invoke
notify_completion, preserving the existing success flow.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9bf752f-7f92-4428-b0ef-39e1b2281633
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (13)
apps/web/src/features/block-automation/component/automationUtils.tsservices/scheduled_action/Cargo.tomlservices/scheduled_action/src/bins/service.rsservices/scheduled_action/src/domain/models.rsservices/scheduled_action/src/domain/ports.rsservices/scheduled_action/src/outbound.rsservices/scheduled_action/src/outbound/inprocess_executor/mod.rsservices/scheduled_action/src/outbound/inprocess_executor/remote_agent_task.rsservices/scheduled_action/src/outbound/pg_scheduled_action_repo.rsservices/scheduled_action/src/outbound/pg_scheduled_action_repo/test.rsservices/scheduled_action/src/outbound/remote_agent_http.rsservices/scheduled_action/src/outbound/remote_agent_http/test.rsservices/scheduled_action/src/swagger.rs
From review: an empty or whitespace-only `output` returned before storing the assistant message and notifying, while the executor still recorded the run as successful — a run marked green with an empty chat and no explanation. Treated as a failure rather than stored, which is the alternative the review offered. A 200 with nothing in it is a misconfigured endpoint, and failing puts the reason on the execution record where the UI already surfaces it. Storing an empty assistant message and firing a notification with empty summary text would also diverge from the Agent kind, which skips both when the model produced no text (agent_task.rs: store_conversation returns early on empty parts, and notify_completion is gated on non-empty text).
|
@SergiorCode We are already working on a big set of feature to support this. And while I really appreciate you putting up a pr, it would just conflict with the implementation we are already working on. I'm going to close this. |
2 participants
Closes #5606.
Adds
ActionKind::RemoteAgentso an agent running outside Macro can be scheduled like a native one.kindis plain TEXT andtaskis untyped JSONB, so there is no migration and no.sqlxchange.Execution mirrors the existing kind — run chat created up front, prompt stored as the user message, the endpoint's reply stored as the assistant message, same execution record and completion notification — which is what lets the Agents surface work for remote agents without a client change.
RemoteAgentClientis a port; thereqwestadapter inoutbound/remote_agent_http.rsowns the transport concerns (HTTPS only, no redirects, timeout, capped response body, and rejection of hosts resolving to private/loopback/link-local/metadata addresses). Those address rules are mirrored fromcrates/webhook/src/outbound/http_validator.rs, whose validator ispub(super)and so unreachable from this service; the copy also folds IPv4-mapped IPv6 back onto the IPv4 rules, so::ffff:169.254.169.254is blocked — the original doesn't do that today.Request signing is not in this PR. That is question 1 on the issue and the only part that needs your decision about where the secret lives. The port is shaped so signing slots into the adapter without touching the executor. Retry is unchanged (fire-and-forget, same as the existing kind, question 2). The SSRF policy is reusable via
RemoteAgentEndpointPolicy::AllowLocalbut nothing configures it yet, pending your answer to question 3.What I could not run
I don't have a Rust toolchain or a database on the machine I wrote this on, so nothing here has been compiled, linted or tested — no
cargo fmt, nojust clippy, nocargo test -p scheduled_action, and I have not seen it work in a local dev environment. Flagging that up front rather than implying otherwise. If you want the direction but not an unverified diff, say so and I'll set up the Nix shell and come back with it built and exercised, or close this and keep the discussion on the issue.Two known gaps that follow from the same constraint:
apps/web/src/lib/service-clients/service-scheduled-action/openapi.json, the orval output, andpackages/sdk/generated/scheduled-action/*still sayActionKind = 'Agent'. Those come frombun gen-apiagainst a running service, which I can't do here.getAgentTaskis now guarded by the discriminant instead of casting unconditionally, and the agent editor refuses to update an action of another kind rather than overwriting its task payload (which would drop a RemoteAgent'sendpoint_url). Creating or editing a RemoteAgent action from the UI is deliberately out of scope.One residual risk worth naming: the endpoint host is resolved for validation and then resolved again by the request, so a record that changes in between can still steer the request at a blocked address.
crates/webhook's validator has the identical window, so I left both consistent rather than diverging — happy to close it in both if you'd rather.Tests included are the ones that don't need a database: kind round-trips through the TEXT column,
RemoteAgentTaskdeserialisation from stored JSON, and the endpoint/address rules including the IPv4-mapped case.