Skip to content

start broker integration#404

Closed
khaliqgant wants to merge 1 commit intomainfrom
relay-broker-integration
Closed

start broker integration#404
khaliqgant wants to merge 1 commit intomainfrom
relay-broker-integration

Conversation

@khaliqgant
Copy link
Copy Markdown
Member

@khaliqgant khaliqgant commented Feb 11, 2026

  • change references and update implementation

Open with Devin

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 potential issues.

View 10 additional findings in Devin Review.

Open in Devin Review

Comment on lines +235 to +237
if (this.child) {
this.child.kill("SIGKILL");
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Shutdown always sends SIGKILL to child process after graceful exit

In AgentRelayClient.shutdown(), after the broker process exits gracefully (via the shutdown request or the SIGTERM timeout), the finally block unconditionally sends SIGKILL to this.child. However, disposeProcessHandles() (called from the exit event handler at packages/sdk/src/client.ts:299) sets this.child = undefined. So if the process exits gracefully before the timeout, this.child will be undefined and the SIGKILL is skipped — this path is fine.

Root Cause: SIGKILL sent to already-exited process when timeout fires first

The real issue is the opposite scenario: when the timeout fires and sends SIGTERM (line 227), and then the process exits, disposeProcessHandles sets this.child = undefined. But the finally block at line 235 checks this.child — if the process hasn't exited yet by the time await wait resolves (which it always does since exitPromise resolves on exit), then this.child may still be set and SIGKILL is sent to a process that already received SIGTERM and is shutting down gracefully.

More critically, if the exitPromise was already resolved (e.g., the process exited during the requestOk("shutdown") call), then wait resolves immediately, the timeout hasn't fired yet, and the finally block sends SIGKILL to a process that already exited cleanly. The child.kill("SIGKILL") call on an already-exited process will throw an error (ESRCH) that is unhandled.

Impact: On clean shutdown, an unhandled ESRCH error may be thrown when trying to SIGKILL an already-exited process. This could cause the shutdown promise to reject unexpectedly.

Suggested change
if (this.child) {
this.child.kill("SIGKILL");
}
if (this.child && !this.child.killed) {
try { this.child.kill("SIGKILL"); } catch { /* already exited */ }
}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

],
"command": "npx",
"env": {
"RELAY_AGENT_NAME": "broker",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Hardcoded API key leaked in committed .mcp.json file

The file relay-broker/scripts/multi-review/.mcp.json contains a hardcoded live Relaycast API key (rk_live_b00fd0ddeca96468d72140c9c4b3a910). This key is committed to the repository and visible to anyone with access.

Security Impact

The RELAY_API_KEY value rk_live_b00fd0ddeca96468d72140c9c4b3a910 is a live production key (prefix rk_live_). Anyone with access to this repository can use this key to:

  • Create workspaces
  • Register agents
  • Send messages
  • Access the Relaycast API

This should be removed from version control and the key should be rotated.

Prompt for agents
Remove the hardcoded API key from relay-broker/scripts/multi-review/.mcp.json. Replace the value with a placeholder like "$RELAY_API_KEY" or remove the file from version control entirely and add it to .gitignore. The key rk_live_b00fd0ddeca96468d72140c9c4b3a910 should be rotated immediately since it has been committed to the repository.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +193 to 204
async sendMessage(
input: SendMessageInput,
): Promise<{ event_id: string; targets: string[] }> {
await this.start();
return this.requestOk<{ event_id: string; targets: string[] }>("send_message", {
to: input.to,
text: input.text,
from: input.from,
thread_id: input.threadId,
priority: input.priority,
});
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 SDK sendMessage returns error but is exposed as a working API

The AgentRelayClient.sendMessage() method is exposed as a public API in the SDK (packages/sdk/src/client.ts:193-204), but the broker's handle_sdk_frame handler for send_message always returns an error.

Detailed Explanation

In relay-broker/src/main.rs:2003-2028, the send_message handler unconditionally returns an error:

"send_message" => {
    // ...
    send_error(
        out_tx,
        frame.request_id,
        "unsupported_operation",
        format!("send_message is not supported for broker-local injection ..."),
        false,
        None,
    ).await?;
    Ok(false)
}

However, the SDK exposes sendMessage() as a normal public method that callers would expect to work. The README (packages/sdk/README.md:30) even shows await client.release("Worker1") as a working example, implying the SDK is fully functional.

The sendMessage method will always throw an AgentRelayProtocolError with code unsupported_operation and retryable: false. This is confusing for SDK consumers who see it as a public API.

Impact: Any SDK consumer calling client.sendMessage(...) will get an unexpected error at runtime. The method should either be removed from the public API, documented as unsupported, or actually implemented.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

khaliqgant added a commit that referenced this pull request Feb 13, 2026
Pull relay-broker directory from relay-broker-integration branch as the
foundation for the hybrid approach. Will apply bug fixes from PR #412 on top.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
khaliqgant added a commit that referenced this pull request Feb 13, 2026
* feat: add relay-broker Rust binary from PR #404

Pull relay-broker directory from relay-broker-integration branch as the
foundation for the hybrid approach. Will apply bug fixes from PR #412 on top.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(security): add sender_kind guard in control.rs

Prevent agents from spoofing human identity by naming themselves
"human:..." to bypass release ACL checks. When sender_kind is
explicitly Agent, immediately return false regardless of name string.

Cherry-picked from PR #412.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(relaycast): prevent URL double-path construction

When base URL already ends with /stream (e.g. wss://rt.relaycast.dev/stream),
avoid appending /v1/stream which produced invalid double-path URLs like
wss://host/stream/v1/stream. Now correctly preserves the existing /stream path.

Cherry-picked from PR #412.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(snippets): correct relaycast typo in function name

Rename ensure_reaycast_mcp_config to ensure_relaycast_mcp_config
(missing 'l' in relaycast). Updated all call sites in main.rs.

Cherry-picked from PR #412.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(scheduler): add 32KiB coalescing body size limit

Without a body-size bound, a burst of large messages from the same
sender within the coalesce window would concatenate without limit,
potentially causing large memory allocations. Add MAX_COALESCED_BODY_SIZE
(32 KiB) check that flushes the current group when exceeded.

Cherry-picked from PR #412.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor(spawner): export terminate_child as pub

Make terminate_child public so it can be reused from main.rs instead
of being duplicated inline, reducing code duplication.

Cherry-picked from PR #412.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(security): remove leaked live API key from .mcp.json

Replace committed rk_live_... key with environment variable placeholder.
The exposed key (rk_live_b00fd0ddeca96468d72140c9c4b3a910) must be
rotated immediately.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(compat): add agent-relay-path aliases and env fallback in utils

* fix(compat): export RelayBrokerOrchestrator alias from wrapper

* fix(utf8): use char-boundary-safe string truncation

Fix potential panics when truncating multi-byte UTF-8 strings (emoji,
CJK, accented characters) in conversation_log.rs and main.rs listen mode.

Adds floor_char_boundary helper to conversation_log.rs and applies it
to truncate(), pad_or_truncate(), and short_id(). Also fixes the same
pattern in main.rs listen-mode message display.

Addresses Devin review finding on PR #415.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
khaliqgant added a commit that referenced this pull request Feb 13, 2026
khaliqgant added a commit that referenced this pull request Feb 13, 2026
Pull relay-broker directory from relay-broker-integration branch as the
foundation for the hybrid approach. Will apply bug fixes from PR #412 on top.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@khaliqgant khaliqgant closed this Feb 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant