relay#1056 §5: broker delivery read-ack bridge#1062
Conversation
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR introduces delivery read-ack support to the broker. A new protocol event signals when a delivery message has been read on behalf of a worker; the broker tracks seeded agent tokens, marks messages read asynchronously with dedup caching and timeout protection, and emits telemetry for success, failure, and skipped outcomes. ChangesDelivery Read-Ack Protocol and System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for explicit workspace keys and broker instance names across agent-relay-broker and @agent-relay/harness-driver, enabling local and cloud brokers to join the same workspace with stable names. It also implements delivery read-ack tracking to attribute read-acks to the recipient worker's agent identity. The review feedback highlights important edge cases where empty or whitespace-only strings in instance_name and workspace_key could bypass fallback logic, and suggests skipping synthetic flush_ event IDs during read-ack classification along with adding a test case for verification.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pub(crate) fn resolved_instance_name(&self, fallback: Option<&str>) -> String { | ||
| self.instance_name | ||
| .clone() | ||
| .or_else(|| std::env::var("AGENT_RELAY_BROKER_NAME").ok()) | ||
| .or_else(|| { | ||
| let name = self.name.trim(); | ||
| if name.is_empty() { | ||
| None | ||
| } else { | ||
| Some(name.to_string()) | ||
| } | ||
| }) | ||
| .or_else(|| fallback.map(ToOwned::to_owned)) | ||
| .unwrap_or_default() | ||
| .trim() | ||
| .to_string() | ||
| } |
There was a problem hiding this comment.
If self.instance_name is passed as an empty or whitespace-only string (e.g., Some(" ")), clone() will return Some(" "), which is not None. Consequently, the or_else chains will be skipped, and the fallback to AGENT_RELAY_BROKER_NAME or self.name will never be evaluated. To prevent this, we should filter out empty or whitespace-only strings before chaining or_else.
pub(crate) fn resolved_instance_name(&self, fallback: Option<&str>) -> String {
self.instance_name
.as_ref()
.map(|s| s.trim().to_string())
.filter(|s| !s.is_empty())
.or_else(|| {
std::env::var("AGENT_RELAY_BROKER_NAME")
.ok()
.map(|s| s.trim().to_string())
.filter(|s| !s.is_empty())
})
.or_else(|| {
let name = self.name.trim();
if name.is_empty() {
None
} else {
Some(name.to_string())
}
})
.or_else(|| fallback.map(|s| s.trim().to_string()))
.unwrap_or_default()
}| pub(crate) fn resolved_workspace_key(&self) -> Option<String> { | ||
| self.workspace_key | ||
| .clone() | ||
| .or_else(|| std::env::var("AGENT_RELAY_WORKSPACE_KEY").ok()) | ||
| .map(|key| key.trim().to_string()) | ||
| .filter(|key| !key.is_empty()) | ||
| } |
There was a problem hiding this comment.
If self.workspace_key is passed as an empty or whitespace-only string (e.g., Some(" ")), the or_else chain is skipped because it is Some. Then, the .map() and .filter() operations will evaluate to None, completely ignoring the fallback to AGENT_RELAY_WORKSPACE_KEY. To fix this, we should filter out empty or whitespace-only strings before chaining or_else.
pub(crate) fn resolved_workspace_key(&self) -> Option<String> {
self.workspace_key
.as_ref()
.map(|key| key.trim().to_string())
.filter(|key| !key.is_empty())
.or_else(|| {
std::env::var("AGENT_RELAY_WORKSPACE_KEY")
.ok()
.map(|key| key.trim().to_string())
.filter(|key| !key.is_empty())
})
}| pub(crate) fn synthetic_delivery_read_ack_reason(event_id: &EventId) -> Option<&'static str> { | ||
| let event_id = event_id.as_str().trim(); | ||
| if event_id.is_empty() { | ||
| return Some("blank_event_id"); | ||
| } | ||
| if event_id.starts_with("http_") { | ||
| return Some("http_api_synthetic_event_id"); | ||
| } | ||
| if event_id.starts_with("init_") { | ||
| return Some("initial_task_synthetic_event_id"); | ||
| } | ||
| if event_id.starts_with("cont_load_") { | ||
| return Some("continuity_synthetic_event_id"); | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
The broker generates synthetic event IDs starting with flush_ when flushing the queue (e.g., flush_<uuid>). These synthetic event IDs are not real Relaycast message IDs and should be skipped for read-ack purposes to avoid unnecessary failed API calls to Relaycast.
pub(crate) fn synthetic_delivery_read_ack_reason(event_id: &EventId) -> Option<&'static str> {
let event_id = event_id.as_str().trim();
if event_id.is_empty() {
return Some("blank_event_id");
}
if event_id.starts_with("http_") {
return Some("http_api_synthetic_event_id");
}
if event_id.starts_with("init_") {
return Some("initial_task_synthetic_event_id");
}
if event_id.starts_with("cont_load_") {
return Some("continuity_synthetic_event_id");
}
if event_id.starts_with("flush_") {
return Some("flush_synthetic_event_id");
}
None
}| fn delivery_read_ack_classification_skips_synthetic_event_ids() { | ||
| let cases = [ | ||
| ("", Some("blank_event_id")), | ||
| (" ", Some("blank_event_id")), | ||
| ("http_123", Some("http_api_synthetic_event_id")), | ||
| ("init_123", Some("initial_task_synthetic_event_id")), | ||
| ("cont_load_123", Some("continuity_synthetic_event_id")), | ||
| ("msg_123", None), | ||
| ("1780911342_317109", None), | ||
| ]; |
There was a problem hiding this comment.
Add a test case for the synthetic flush_ event ID to ensure it is correctly classified and skipped.
| fn delivery_read_ack_classification_skips_synthetic_event_ids() { | |
| let cases = [ | |
| ("", Some("blank_event_id")), | |
| (" ", Some("blank_event_id")), | |
| ("http_123", Some("http_api_synthetic_event_id")), | |
| ("init_123", Some("initial_task_synthetic_event_id")), | |
| ("cont_load_123", Some("continuity_synthetic_event_id")), | |
| ("msg_123", None), | |
| ("1780911342_317109", None), | |
| ]; | |
| fn delivery_read_ack_classification_skips_synthetic_event_ids() { | |
| let cases = [ | |
| ("", Some("blank_event_id")), | |
| (" ", Some("blank_event_id")), | |
| ("http_123", Some("http_api_synthetic_event_id")), | |
| ("init_123", Some("initial_task_synthetic_event_id")), | |
| ("cont_load_123", Some("continuity_synthetic_event_id")), | |
| ("flush_123", Some("flush_synthetic_event_id")), | |
| ("msg_123", None), | |
| ("1780911342_317109", None), | |
| ]; |
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/broker/src/runtime/api.rs (1)
78-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip auto-registration when
agent_tokenis already supplied.Lines 79-107 still run preregistration before honoring
agent_token. That means a fatal preregistration error aborts the spawn even when the caller already provided a usable worker token, and retry exhaustion can surface a bogusregistration_warningfor a path that did not need preregistration at all.Suggested fix
let mut preregistration_warning: Option<String> = None; - let registration_result = - retry_agent_registration(relaycast_http, &name, Some(&cli)).await; - let worker_relay_key = match registration_result { - Ok(token) => Some(token), - Err(RegRetryOutcome::RetryableExhausted(error)) => { - let message = format_worker_preregistration_error(&name, &error); - tracing::warn!( - worker = %name, - error = %error, - "continuing spawn without pre-registration after retries exhausted" - ); - preregistration_warning = Some(message); - None - } - Err(RegRetryOutcome::Fatal(error)) => { - let _ = reply.send(Err(format_worker_preregistration_error(&name, &error))); - return; - } - }; - - // Caller-supplied agent_token overrides auto-registration. - // Seed it so broker-side read-acks later act as this exact - // recipient identity instead of minting a replacement token. - let worker_relay_key = if let Some(token) = agent_token { + let worker_relay_key = if let Some(token) = agent_token { seed_supplied_agent_token(relaycast_http, &name, &token); Some(token) } else { - worker_relay_key + match retry_agent_registration(relaycast_http, &name, Some(&cli)).await { + Ok(token) => Some(token), + Err(RegRetryOutcome::RetryableExhausted(error)) => { + let message = format_worker_preregistration_error(&name, &error); + tracing::warn!( + worker = %name, + error = %error, + "continuing spawn without pre-registration after retries exhausted" + ); + preregistration_warning = Some(message); + None + } + Err(RegRetryOutcome::Fatal(error)) => { + let _ = reply.send(Err(format_worker_preregistration_error(&name, &error))); + return; + } + } };🤖 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 `@crates/broker/src/runtime/api.rs` around lines 78 - 107, The preregistration flow runs unconditionally and can abort or set warnings even when a caller-supplied agent_token exists; change the logic in the spawn path to check agent_token first and, if present, call seed_supplied_agent_token(relaycast_http, &name, &token) and set worker_relay_key = Some(token) immediately, skipping retry_agent_registration, preregistration_warning, registration_result handling, and any fatal reply send via format_worker_preregistration_error/RegRetryOutcome; only run retry_agent_registration(relaycast_http, &name, Some(&cli)).await when agent_token is None and preserve the existing handling of RetryableExhausted and Fatal cases, keeping variables name, relaycast_http, cli, reply, and preregistration_warning consistent.
🧹 Nitpick comments (1)
crates/broker/src/protocol.rs (1)
340-347: ⚡ Quick winAdd a wire-contract round-trip test for
delivery_read_ack.This new broker event shape is part of the SDK contract but isn’t covered in this file’s serde tests yet. Add one round-trip asserting
kind,status, andreasonomission whenNoneto lock the format.Also applies to: 424-431
🤖 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 `@crates/broker/src/protocol.rs` around lines 340 - 347, Add a serde round-trip test that constructs the broker event for "delivery_read_ack" (use the same event struct/type used in this file), serializes to JSON, and deserializes back to assert the wire contract: the top-level "kind" equals "delivery_read_ack", the enum DeliveryReadAckStatus values round-trip and serialize as snake_case (test Marked, Failed, SkippedSynthetic, SuppressedDuplicate), and when the event's optional reason field is None the serialized JSON omits the "reason" key (and when Some(reason) it round-trips). Add this test to the existing serde tests in this file (also cover the same checks for the analogous case referenced at lines ~424-431).
🤖 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 `@crates/broker/src/cli/mod.rs`:
- Around line 261-277: In the resolved_instance_name method, whitespace-only
values in self.instance_name and the AGENT_RELAY_BROKER_NAME environment
variable are being treated as valid "Some" values, which short-circuits the
fallback chain and prevents fallback resolution. To fix this, trim and validate
self.instance_name and the environment variable result early in the chain,
converting empty-after-trim results to None before proceeding to the next
or_else clause. Apply the same whitespace-normalization pattern that is already
correctly used for self.name to both the initial instance_name clone and the
environment variable lookup.
In `@crates/broker/src/runtime/worker_events.rs`:
- Around line 9-10: The code currently captures self.relaycast_http and routes
read-acks through the broker's default client (relaycast_http) when handling
PendingDelivery/delivery_ack; instead, use the originating delivery's workspace
client or workspace_id to call mark_read_as_agent so the ack targets the correct
workspace. Locate the handling of PendingDelivery/delivery_ack (references:
PendingDelivery, delivery_ack, relaycast_http, ws_control_tx,
mark_read_as_agent) and replace uses of self.relaycast_http.clone() with the
workspace-specific client from the delivery (e.g., delivery.relaycast_http or
lookup by delivery.workspace_id) and pass that client into mark_read_as_agent;
ensure both the immediate ack path and the looped logic in the 69-100 region
consistently use the delivery's workspace client rather than
self.relaycast_http.
In `@packages/harness-driver/src/client.ts`:
- Around line 456-463: The code assigns workspaceKey from several fallbacks
including RELAY_API_KEY without validating its form, causing non-workspace API
keys to be treated as explicit workspace keys; change the selection logic so
workspaceKey only accepts values that match a workspace key pattern (e.g., start
with "rk_") when picking from options?.workspaceKey,
options?.env.RELAY_WORKSPACE_KEY, options?.env.RELAY_API_KEY,
process.env.RELAY_WORKSPACE_KEY, and process.env.RELAY_API_KEY; leave other API
keys available for auth but do not set workspaceKey from them, and ensure the
same validation is applied where explicit-join inputs are created (the code
around the workspaceKey variable and the explicit join/explicit-join input
construction) so invalid RELAY_API_KEY values no longer trigger explicit-join
behavior.
---
Outside diff comments:
In `@crates/broker/src/runtime/api.rs`:
- Around line 78-107: The preregistration flow runs unconditionally and can
abort or set warnings even when a caller-supplied agent_token exists; change the
logic in the spawn path to check agent_token first and, if present, call
seed_supplied_agent_token(relaycast_http, &name, &token) and set
worker_relay_key = Some(token) immediately, skipping retry_agent_registration,
preregistration_warning, registration_result handling, and any fatal reply send
via format_worker_preregistration_error/RegRetryOutcome; only run
retry_agent_registration(relaycast_http, &name, Some(&cli)).await when
agent_token is None and preserve the existing handling of RetryableExhausted and
Fatal cases, keeping variables name, relaycast_http, cli, reply, and
preregistration_warning consistent.
---
Nitpick comments:
In `@crates/broker/src/protocol.rs`:
- Around line 340-347: Add a serde round-trip test that constructs the broker
event for "delivery_read_ack" (use the same event struct/type used in this
file), serializes to JSON, and deserializes back to assert the wire contract:
the top-level "kind" equals "delivery_read_ack", the enum DeliveryReadAckStatus
values round-trip and serialize as snake_case (test Marked, Failed,
SkippedSynthetic, SuppressedDuplicate), and when the event's optional reason
field is None the serialized JSON omits the "reason" key (and when Some(reason)
it round-trips). Add this test to the existing serde tests in this file (also
cover the same checks for the analogous case referenced at lines ~424-431).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 64e74e77-492a-41bd-9a7c-fba0852b6d32
📒 Files selected for processing (17)
.agentworkforce/trajectories/active/traj_b1jrutolckfb/trajectory.jsonCHANGELOG.mdcrates/broker/src/cli/mod.rscrates/broker/src/protocol.rscrates/broker/src/relaycast/auth.rscrates/broker/src/relaycast/ws.rscrates/broker/src/runtime/api.rscrates/broker/src/runtime/delivery.rscrates/broker/src/runtime/init.rscrates/broker/src/runtime/mod.rscrates/broker/src/runtime/relaycast_events.rscrates/broker/src/runtime/tests.rscrates/broker/src/runtime/worker_events.rscrates/broker/src/spawner.rspackages/harness-driver/src/client.tspackages/harness-driver/src/lifecycle-hooks.tspackages/harness-driver/src/protocol.ts
| let relaycast_http = self.relaycast_http.clone(); | ||
| let ws_control_tx = &self.ws_control_tx; |
There was a problem hiding this comment.
Use the delivery’s workspace client for mark_read.
Lines 9-10 and Lines 69-100 always route read-acks through self.relaycast_http, but the confirmed PendingDelivery still carries the originating workspace_id. In a multi-workspace broker, a delivery_ack for workspace B will call mark_read_as_agent(...) against the default workspace client instead, so the read-ack either fails or targets the wrong workspace.
Suggested fix
- let relaycast_http = self.relaycast_http.clone();
+ let workspace_lookup = &self.workspace_lookup;
+ let default_workspace = &self.default_workspace;
...
if let Some(pending) = pending_for_confirmation {
let read_ack_delivery_id = pending.delivery.delivery_id.clone();
let read_ack_event_id = pending.delivery.event_id.clone();
+ let read_ack_http = pending
+ .delivery
+ .workspace_id
+ .as_deref()
+ .and_then(|id| workspace_lookup.get(id))
+ .map(|workspace| workspace.http_client.clone())
+ .unwrap_or_else(|| default_workspace.http_client.clone());
let cli_hint = workers
.workers
.get(&name)
.and_then(|handle| handle.spec.cli.as_deref())
.map(str::to_string);
...
mark_delivery_read_ack(
- &relaycast_http,
+ &read_ack_http,
sdk_out_tx,
dedup,
&name,
cli_hint.as_deref(),
&read_ack_delivery_id,
&read_ack_event_id,
);Also applies to: 69-100
🤖 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 `@crates/broker/src/runtime/worker_events.rs` around lines 9 - 10, The code
currently captures self.relaycast_http and routes read-acks through the broker's
default client (relaycast_http) when handling PendingDelivery/delivery_ack;
instead, use the originating delivery's workspace client or workspace_id to call
mark_read_as_agent so the ack targets the correct workspace. Locate the handling
of PendingDelivery/delivery_ack (references: PendingDelivery, delivery_ack,
relaycast_http, ws_control_tx, mark_read_as_agent) and replace uses of
self.relaycast_http.clone() with the workspace-specific client from the delivery
(e.g., delivery.relaycast_http or lookup by delivery.workspace_id) and pass that
client into mark_read_as_agent; ensure both the immediate ack path and the
looped logic in the 69-100 region consistently use the delivery's workspace
client rather than self.relaycast_http.
| const workspaceKey = | ||
| nonEmptyString(options?.workspaceKey) ?? | ||
| nonEmptyString(options?.env?.AGENT_RELAY_WORKSPACE_KEY) ?? | ||
| nonEmptyString(options?.env?.RELAY_WORKSPACE_KEY) ?? | ||
| nonEmptyString(options?.env?.RELAY_API_KEY) ?? | ||
| nonEmptyString(process.env.AGENT_RELAY_WORKSPACE_KEY) ?? | ||
| nonEmptyString(process.env.RELAY_WORKSPACE_KEY) ?? | ||
| nonEmptyString(process.env.RELAY_API_KEY); |
There was a problem hiding this comment.
RELAY_API_KEY fallback is being promoted to strict workspace join without validation.
If RELAY_API_KEY is present but not an rk_ workspace key, Line 460/Line 463 still set workspaceKey, and then Line 490 + Line 478 force explicit-join inputs. Broker startup then hard-fails on invalid explicit workspace key.
Proposed fix
function nonEmptyString(value: string | undefined): string | undefined {
const trimmed = value?.trim();
return trimmed ? trimmed : undefined;
}
+function nonEmptyWorkspaceKey(value: string | undefined): string | undefined {
+ const trimmed = nonEmptyString(value);
+ return trimmed?.startsWith('rk_') ? trimmed : undefined;
+}
+
// ...
const workspaceKey =
nonEmptyString(options?.workspaceKey) ??
nonEmptyString(options?.env?.AGENT_RELAY_WORKSPACE_KEY) ??
nonEmptyString(options?.env?.RELAY_WORKSPACE_KEY) ??
- nonEmptyString(options?.env?.RELAY_API_KEY) ??
+ nonEmptyWorkspaceKey(options?.env?.RELAY_API_KEY) ??
nonEmptyString(process.env.AGENT_RELAY_WORKSPACE_KEY) ??
nonEmptyString(process.env.RELAY_WORKSPACE_KEY) ??
- nonEmptyString(process.env.RELAY_API_KEY);
+ nonEmptyWorkspaceKey(process.env.RELAY_API_KEY);Also applies to: 476-490
🤖 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 `@packages/harness-driver/src/client.ts` around lines 456 - 463, The code
assigns workspaceKey from several fallbacks including RELAY_API_KEY without
validating its form, causing non-workspace API keys to be treated as explicit
workspace keys; change the selection logic so workspaceKey only accepts values
that match a workspace key pattern (e.g., start with "rk_") when picking from
options?.workspaceKey, options?.env.RELAY_WORKSPACE_KEY,
options?.env.RELAY_API_KEY, process.env.RELAY_WORKSPACE_KEY, and
process.env.RELAY_API_KEY; leave other API keys available for auth but do not
set workspaceKey from them, and ensure the same validation is applied where
explicit-join inputs are created (the code around the workspaceKey variable and
the explicit join/explicit-join input construction) so invalid RELAY_API_KEY
values no longer trigger explicit-join behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 10 files
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Reviewed and fixed the validated PR issues. Changes made:
Verification run:
Addressed comments
I did not print |
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
✅ pr-reviewer applied fixes — committed and pushed Fixed the validated PR issues in the current checkout. Changes made:
Verification run:
Addressed comments
I did not print |
|
Reviewed PR #1062 against Addressed commentsNo bot or reviewer comments were present in the provided Validation run:
I did not verify GitHub CI status or mergeability because the task prohibited |
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Reviewed PR #1062 against Addressed commentsNo bot or reviewer comments were present in the provided Validation run:
I did not verify GitHub CI status or mergeability because the task prohibited |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.agentworkforce/trajectories/completed/2026-06/traj_b1jrutolckfb/summary.md:
- Line 28: Normalize the ambiguous tokens in the decision rationale by replacing
accidental asterisks: change "workspace*key" to a clear form such as
"workspace_key" or a quoted literal "workspace key", and change "flush*
synthetic" to either "flush synthetic" or simply "flush" (or quote as "flush
synthetic") so the sentence is unambiguous; update the summary string that
contains those tokens accordingly (look for the phrase containing workspace*key
and flush* synthetic in the summary.md content and perform the token
replacements).
- Line 38: Remove the duplicated sentence in the Chapter 1 work item by keeping
only one copy of the sentence "events.connect() falls back to the relaycast 2.5
workspace stream when no agent client; fixes relay#1031 so workspace
relay.addListener streams. Bumped `@relaycast/sdk` to ^2.5.1. Also fixed
pre-existing vitest-4 constructor-mock breakage in agent-relay.test.ts (main
'Test' workflow was red)." so the line appears once (delete the redundant
repeated copy after the colon).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c5eb48d7-7b10-47c0-8939-3f768607eb1e
📒 Files selected for processing (7)
.agentworkforce/trajectories/active/traj_b1jrutolckfb/trajectory.json.agentworkforce/trajectories/completed/2026-06/traj_b1jrutolckfb.trace.json.agentworkforce/trajectories/completed/2026-06/traj_b1jrutolckfb/summary.md.agentworkforce/trajectories/completed/2026-06/traj_b1jrutolckfb/trajectory.jsoncrates/broker/src/cli/mod.rscrates/broker/src/runtime/delivery.rscrates/broker/src/runtime/tests.rs
💤 Files with no reviewable changes (1)
- .agentworkforce/trajectories/active/traj_b1jrutolckfb/trajectory.json
✅ Files skipped from review due to trivial changes (1)
- .agentworkforce/trajectories/completed/2026-06/traj_b1jrutolckfb/trajectory.json
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/broker/src/runtime/tests.rs
- crates/broker/src/runtime/delivery.rs
|
|
||
| _Agent: default_ | ||
|
|
||
| - events.connect() falls back to the relaycast 2.5 workspace stream when no agent client; fixes relay#1031 so workspace relay.addListener streams. Bumped @relaycast/sdk to ^2.5.1. Also fixed pre-existing vitest-4 constructor-mock breakage in agent-relay.test.ts (main 'Test' workflow was red).: events.connect() falls back to the relaycast 2.5 workspace stream when no agent client; fixes relay#1031 so workspace relay.addListener streams. Bumped @relaycast/sdk to ^2.5.1. Also fixed pre-existing vitest-4 constructor-mock breakage in agent-relay.test.ts (main 'Test' workflow was red). |
There was a problem hiding this comment.
Remove duplicated sentence in Chapter 1 work item.
This bullet repeats the same full sentence on both sides of the colon, which adds noise to the trajectory record. Keep only one copy.
🤖 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 @.agentworkforce/trajectories/completed/2026-06/traj_b1jrutolckfb/summary.md
at line 38, Remove the duplicated sentence in the Chapter 1 work item by keeping
only one copy of the sentence "events.connect() falls back to the relaycast 2.5
workspace stream when no agent client; fixes relay#1031 so workspace
relay.addListener streams. Bumped `@relaycast/sdk` to ^2.5.1. Also fixed
pre-existing vitest-4 constructor-mock breakage in agent-relay.test.ts (main
'Test' workflow was red)." so the line appears once (delete the redundant
repeated copy after the colon).
There was a problem hiding this comment.
3 issues found across 7 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/broker/src/cli/mod.rs">
<violation number="1" location="crates/broker/src/cli/mod.rs:285">
P3: Duplicated `non_empty_trimmed` inner function in both `resolved_instance_name` and `resolved_workspace_key` — extract to a module-level private helper to avoid the DRY violation and keep future trimming behavior consistent.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
|
|
||
| pub(crate) fn resolved_workspace_key(&self) -> Option<String> { | ||
| fn non_empty_trimmed(value: &str) -> Option<String> { |
There was a problem hiding this comment.
P3: Duplicated non_empty_trimmed inner function in both resolved_instance_name and resolved_workspace_key — extract to a module-level private helper to avoid the DRY violation and keep future trimming behavior consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/cli/mod.rs, line 285:
<comment>Duplicated `non_empty_trimmed` inner function in both `resolved_instance_name` and `resolved_workspace_key` — extract to a module-level private helper to avoid the DRY violation and keep future trimming behavior consistent.</comment>
<file context>
@@ -282,11 +282,23 @@ impl InitCommand {
}
pub(crate) fn resolved_workspace_key(&self) -> Option<String> {
+ fn non_empty_trimmed(value: &str) -> Option<String> {
+ let trimmed = value.trim();
+ if trimmed.is_empty() {
</file context>
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
There was a problem hiding this comment.
3 issues found across 7 files (changes from recent commits).
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/broker/src/runtime/tests.rs">
<violation number="1" location="crates/broker/src/runtime/tests.rs:1657">
P2: Test assertions assume deterministic ordering of events emitted from `tokio::spawn` tasks. Both `mark_delivery_read_ack` calls emit telemetry via spawned tasks with no `.await` between them, making the receive order scheduler-dependent. This can produce a flaky test if the runtime schedules the tasks out of spawn order.</violation>
</file>
<file name=".agentworkforce/trajectories/active/traj_b1jrutolckfb/trajectory.json">
<violation number="1">
P3: The committed trajectory task title appears to describe a different feature than this PR, which can mislead trajectory/automation consumers and review traceability.</violation>
</file>
<file name="crates/broker/src/cli/mod.rs">
<violation number="1">
P2: `resolved_workspace_key` changed precedence so a blank `--workspace-key` no longer falls back to `AGENT_RELAY_WORKSPACE_KEY`, causing unintended loss of env-based workspace selection.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| &EventId::new("init_123"), | ||
| ); | ||
|
|
||
| let first = tokio::time::timeout(Duration::from_secs(1), rx.recv()) |
There was a problem hiding this comment.
P2: Test assertions assume deterministic ordering of events emitted from tokio::spawn tasks. Both mark_delivery_read_ack calls emit telemetry via spawned tasks with no .await between them, making the receive order scheduler-dependent. This can produce a flaky test if the runtime schedules the tasks out of spawn order.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/runtime/tests.rs, line 1657:
<comment>Test assertions assume deterministic ordering of events emitted from `tokio::spawn` tasks. Both `mark_delivery_read_ack` calls emit telemetry via spawned tasks with no `.await` between them, making the receive order scheduler-dependent. This can produce a flaky test if the runtime schedules the tasks out of spawn order.</comment>
<file context>
@@ -1654,15 +1654,21 @@ async fn synthetic_delivery_read_ack_skips_mark_read() {
- assert_eq!(frame.payload["status"], "skipped_synthetic");
- assert_eq!(frame.payload["reason"], "initial_task_synthetic_event_id");
- }
+ let first = tokio::time::timeout(Duration::from_secs(1), rx.recv())
+ .await
+ .expect("synthetic skip telemetry should arrive")
</file context>
1 participant
Scope
Implements relay#1056 §5 broker delivery read-ack bridge only.
This PR adds broker-side best-effort read-ack plumbing on the confirmed delivery path:
delivery_read_acktelemetry withmarked,failed,skipped_synthetic, andsuppressed_duplicatestatuses.clear_pending_delivery_if_event_matches(...)confirms adelivery_ack, schedule Relaycastmark_read(event_id)as the recipient agent.blank,http_*,init_*,cont_load_*).(recipient,event_id)to suppress duplicate/replayed delivery read-ack work and telemetry.delivery_read_ack.Verification
cargo test -p agent-relay-broker read_ack -- --nocapturecargo test -p agent-relay-broker stale_delivery_ack_event_id_does_not_mark_read -- --nocapturecargo test -p agent-relay-broker mark_read_as_agent_uses_seeded_recipient_token_without_respawn -- --nocapturecargo check -p agent-relay-broker --testsgit diff --checkrelay-worker also reran the same verification set and completed non-author review with no blocking findings.
Known / Accepted Notes
Best-effort by design: read-ack is fire-and-forget with a detached 2s timeout, non-blocking on the delivery hot path. Delivery remains the guarantee; read-ack is a consistency optimization.
Follow-up, not a blocker: dedupe currently suppresses retry even after a failed first
mark_readwithin the dedup window. Consider dedupe-on-success-only plus bounded retry-on-failure so a transient Relaycast blip does not permanently skip the read-ack. Filed separately as #1061.Fleet Landing Note
Merging this PR lands the code in relay main, but does not make read-acks live in the fleet. Fleet landing needs a follow-up relay publish and sandbox snapshot rebake.