-
Notifications
You must be signed in to change notification settings - Fork 60
fix(broker): route all delivery through Relaycast + honor spawn startup timeout #1221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
71d62e9
597d045
38abb3c
11ac64b
7235a1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -507,7 +507,9 @@ impl RelaycastHttpClient { | |
| /// via [`AgentClient::reply`] instead of a plain channel post is what | ||
| /// actually creates real thread/conversation grouping on the Relaycast | ||
| /// side, as opposed to passing an opaque value the server doesn't | ||
| /// interpret as a reply. | ||
| /// interpret as a reply. `reply` takes no injection mode, so a threaded | ||
| /// reply is always delivered with Wait semantics — a `Steer` request with | ||
| /// a `thread_id` is downgraded to a normal reply (logged, not dropped). | ||
| pub async fn send_with_mode( | ||
| &self, | ||
| to: &str, | ||
|
|
@@ -523,6 +525,18 @@ impl RelaycastHttpClient { | |
| MessageInjectionMode::Steer => relaycast::MessageInjectionMode::Steer, | ||
| }; | ||
| if let Some(thread_id) = thread_id { | ||
| // `AgentClient::reply` has no injection-mode parameter, so a | ||
| // threaded reply is always delivered with Wait semantics. | ||
| // `Steer` can't be honored on a reply; downgrade rather than | ||
| // drop the message, but log it so the loss of steer is visible | ||
| // instead of silent. | ||
| if matches!(mode, MessageInjectionMode::Steer) { | ||
| tracing::warn!( | ||
| target = "relay_broker::relaycast", | ||
| thread_id = %thread_id, | ||
| "steer injection mode is not supported on threaded replies; delivering as a normal reply" | ||
| ); | ||
| } | ||
| agent_client | ||
| .reply(thread_id, text, None, None) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Thread replies now require a Relaycast message id, but the broker send API exposes broker Prompt for AI agents
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in bf68b92 — see reply above. Synthetic
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| .await | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -709,12 +709,17 @@ impl BrokerRuntime { | |||||||||||||||
| // worse, ROTATE (and thereby invalidate) the live token of | ||||||||||||||||
| // an unrelated, already-registered agent that happens to | ||||||||||||||||
| // share the name. Falling back to the broker's own identity | ||||||||||||||||
| // is always safe; impersonation is not. | ||||||||||||||||
| let publish_from = if workers.has_worker(&delivery_from) { | ||||||||||||||||
| delivery_from.as_str() | ||||||||||||||||
| } else { | ||||||||||||||||
| workspace_self_name.as_str() | ||||||||||||||||
| }; | ||||||||||||||||
| // is always safe; impersonation is not. The worker must also | ||||||||||||||||
| // belong to the workspace we're publishing into — a worker | ||||||||||||||||
| // attached to another attached workspace is not ours to | ||||||||||||||||
| // impersonate here (it would register/rotate that name in the | ||||||||||||||||
| // wrong Relaycast workspace). | ||||||||||||||||
| let publish_from = | ||||||||||||||||
| if workers.has_worker_in_workspace(&delivery_from, &selected_workspace_id) { | ||||||||||||||||
| delivery_from.as_str() | ||||||||||||||||
| } else { | ||||||||||||||||
| workspace_self_name.as_str() | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| record_thread_history_event( | ||||||||||||||||
| recent_thread_messages, | ||||||||||||||||
|
|
@@ -755,6 +760,24 @@ impl BrokerRuntime { | |||||||||||||||
| relaycast_timeout_ms = %relaycast_timeout.as_millis(), | ||||||||||||||||
| "publishing to relaycast" | ||||||||||||||||
| ); | ||||||||||||||||
| // Only forward `thread_id` to the Relaycast publish when it's a | ||||||||||||||||
| // real message id we can reply to. Broker-minted synthetic ids | ||||||||||||||||
| // (`http_*`) and channel/DM grouping keys (`#general`, | ||||||||||||||||
| // `direct:*`) that a client may echo back from `/api/send` or | ||||||||||||||||
| // `/api/threads` aren't reply targets — Relaycast would reject | ||||||||||||||||
| // the reply and fail the whole send. Fall back to a plain post | ||||||||||||||||
| // (unthreaded) for those, preserving delivery. | ||||||||||||||||
| let reply_thread_id = thread_id | ||||||||||||||||
| .as_deref() | ||||||||||||||||
| .filter(|tid| is_relaycast_reply_target(tid)); | ||||||||||||||||
|
Comment on lines
+770
to
+772
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Pass the trimmed reply id to Relaycast, not the original untrimmed slice. Current validation trims for classification but forwards whitespace to Prompt for AI agents
Suggested change
|
||||||||||||||||
| if thread_id.is_some() && reply_thread_id.is_none() { | ||||||||||||||||
| tracing::debug!( | ||||||||||||||||
| target = "relay_broker::http_api", | ||||||||||||||||
| event_id = %event_id, | ||||||||||||||||
| thread_id = ?thread_id, | ||||||||||||||||
| "thread_id is not a Relaycast message id; publishing without a thread reply" | ||||||||||||||||
| ); | ||||||||||||||||
| } | ||||||||||||||||
| let relaycast_start = Instant::now(); | ||||||||||||||||
| match timeout( | ||||||||||||||||
| relaycast_timeout, | ||||||||||||||||
|
|
@@ -763,7 +786,7 @@ impl BrokerRuntime { | |||||||||||||||
| &text, | ||||||||||||||||
| mode.clone(), | ||||||||||||||||
| publish_from, | ||||||||||||||||
| thread_id.as_deref(), | ||||||||||||||||
| reply_thread_id, | ||||||||||||||||
| ), | ||||||||||||||||
| ) | ||||||||||||||||
| .await | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -209,6 +209,25 @@ impl WorkerRegistry { | |
| self.workers.contains_key(name) | ||
| } | ||
|
|
||
| /// True when a worker named `name` exists and either has no recorded | ||
| /// workspace or belongs to `workspace_id`. Gates sender impersonation on | ||
| /// Relaycast publish: a worker attached to workspace A must not be | ||
| /// impersonated when publishing into workspace B, which would register or | ||
| /// rotate that name's token in the wrong workspace. | ||
| pub(crate) fn has_worker_in_workspace( | ||
| &self, | ||
| name: &str, | ||
| workspace_id: &crate::ids::WorkspaceId, | ||
| ) -> bool { | ||
| match self.workers.get(name) { | ||
| Some(handle) => match &handle.workspace_id { | ||
| Some(worker_ws) => worker_ws == workspace_id, | ||
| None => true, | ||
| }, | ||
| None => false, | ||
| } | ||
| } | ||
|
|
||
|
Comment on lines
+212
to
+230
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: check where WorkerHandle.workspace_id is set to None vs Some,
# to confirm None only occurs in single-workspace configurations.
rg -n -B3 -A3 'workspace_id\s*:\s*None' crates/broker/src/worker.rs crates/broker/src/runtime/*.rs
rg -n -B3 -A10 'struct WorkerHandle' crates/broker/src/worker.rsRepository: AgentWorkforce/relay Length of output: 1240 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Map the worker and API call sites first.
ast-grep outline crates/broker/src/worker.rs --view expanded
ast-grep outline crates/broker/src/runtime/api.rs --view expanded
ast-grep outline crates/broker/src/runtime/app_server.rs --view expanded
# Read the relevant sections around workspace_id handling and the impersonation gate.
sed -n '1,140p' crates/broker/src/worker.rs
sed -n '1,240p' crates/broker/src/runtime/api.rs
sed -n '340,430p' crates/broker/src/runtime/app_server.rs
# Find all assignments/usages of WorkerHandle.workspace_id and related workspace scoping.
rg -n -B3 -A3 'workspace_id\s*:' crates/broker/src
rg -n -B3 -A6 'has_worker_in_workspace|from:|workspace_id' crates/broker/src/runtime/api.rs crates/broker/src/worker.rs
# Check tests around the new helper and any multi-workspace behavior.
rg -n -B2 -A8 'has_worker_in_workspace|workspace_id: None|workspace_id: Some|RelayDelivery' crates/broker/srcRepository: AgentWorkforce/relay Length of output: 50377 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the impersonation gate and worker creation paths with tight ranges.
sed -n '210,260p' crates/broker/src/worker.rs
sed -n '760,900p' crates/broker/src/worker.rs
sed -n '1,220p' crates/broker/src/runtime/api.rs
# Find all places a WorkerHandle is constructed and how workspace_id is populated.
rg -n -B4 -A12 'WorkerHandle \{' crates/broker/src
rg -n -B4 -A8 'workspace_id:\s*(Some\(|None|workspace_id)' crates/broker/src/worker.rs crates/broker/src/runtime crates/broker/src
# Read the routing rule that already treats None as match-all.
sed -n '1,80p' crates/broker/src/routing.rs
# Narrow tests around the new helper and any workspace-related behavior.
sed -n '1570,1610p' crates/broker/src/worker.rsRepository: AgentWorkforce/relay Length of output: 50376 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Show the exact impersonation gate in runtime/api.rs.
rg -n -B8 -A18 'has_worker_in_workspace|impersonat|sender|from' crates/broker/src/runtime/api.rs
# Inspect the worker workspace propagation path from spawn call sites.
rg -n -B6 -A10 'spawn\(' crates/broker/src/runtime/api.rs crates/broker/src/runtime/tests.rs crates/broker/src/listen_api.rs
# Check the helper tests and add any nearby assertions around None/workspace match behavior.
sed -n '1588,1608p' crates/broker/src/worker.rs
sed -n '1,80p' crates/broker/src/routing.rsRepository: AgentWorkforce/relay Length of output: 50376 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Find every production construction of WorkerHandle and inspect its workspace_id value.
rg -n -B6 -A14 'WorkerHandle \{' crates/broker/src --glob '!**/*test*' --glob '!**/tests.rs'
# Find every direct use of `workspace_id: None` in worker creation contexts.
rg -n -B4 -A6 'workspace_id:\s*None' crates/broker/src --glob '!**/*test*' --glob '!**/tests.rs'
# Show the full spawn signature and the workspace_id value path again, plus any other spawn call sites.
rg -n -B4 -A12 'pub\(crate\) async fn spawn\(' crates/broker/src/worker.rs
rg -n -n 'spawn\(' crates/broker/src/runtime/api.rs crates/broker/src/listen_api.rs crates/broker/src/pty_worker.rs crates/broker/src/node_control.rs crates/broker/src/fleet.rsRepository: AgentWorkforce/relay Length of output: 14237 Keep the workspace check, but add coverage for all branches. The 🤖 Prompt for AI Agents |
||
| pub(crate) fn worker_pid(&self, name: &str) -> Option<u32> { | ||
| self.workers.get(name).and_then(|h| h.child.id()) | ||
| } | ||
|
|
@@ -1568,6 +1587,13 @@ mod tests { | |
| assert!(!reg.has_worker("nonexistent")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn has_worker_in_workspace_returns_false_for_unknown() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The test for Prompt for AI agents |
||
| let reg = make_registry(vec![]); | ||
| let workspace = crate::ids::WorkspaceId::new("ws_1".to_string()); | ||
| assert!(!reg.has_worker_in_workspace("nonexistent", &workspace)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn worker_log_path_rejects_path_traversal() { | ||
| let reg = make_registry(vec![]); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For channel targets, any supplied
thread_idis now treated as a Relaycast message id. Broker clients commonly pass theevent_idreturned by/api/send, which is still a synthetichttp_*id, and/api/threadscan also surface synthetic grouping keys such as#general/direct:*; Relaycast cannot reply to those ids, so these channel follow-ups now fail instead of posting with the provided thread context. Gate this path to real Relaycast message ids or return/use the Relaycast id from the initial publish.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in bf68b92. Added
is_relaycast_reply_target()(reuses the existing synthetic-event-id classifier and also rejects#channel/direct:*grouping keys); the send handler only forwardsthread_idtoreply()when it's a real message id and otherwise falls back to a plain post, so a synthetichttp_*/grouping id no longer fails the send.