Skip to content

docs(mcp): state what every MCP tool returns - #1420

Open
willwashburn wants to merge 5 commits into
mainfrom
claude/agent-relay-feedback-i6ijz8
Open

docs(mcp): state what every MCP tool returns#1420
willwashburn wants to merge 5 commits into
mainfrom
claude/agent-relay-feedback-i6ijz8

Conversation

@willwashburn

@willwashburn willwashburn commented Aug 2, 2026

Copy link
Copy Markdown
Member

Summary

Addresses the gradable.dev report on Relay's exposed MCP surface: 28 defects, overwhelmingly no_return_description, plus a name_restates_behavior on register_agent.

Every tool description said what the call did and none said what came back. An agent reading the tool list couldn't tell whether post_message hands back a message ID it can reply to, whether a successful spawn means the worker is running or merely queued, or whether an empty list_agents array means "no agents exist" or "the status filter excluded them".

30 tool descriptions across agent-relay-mcp.ts, mcp/messaging-tools.ts, and mcp/action-tools.ts gain a return clause, each grounded in what the handler actually returns rather than restated from the output schema:

  • textContent tools describe the confirmation string (add_reaction notes the returned emoji is the resolved one, so :+1: comes back as 👍).
  • List tools name the array key and say what empty means.
  • jsonContent tools describe the record, and name the field callers need next — post_message returns the ID that reply_to_thread and add_reaction consume.
  • Asynchronous tools (spawn, add_agent, remove_agent) now say acceptance is not completion.
  • create_workspace explains that warning appears only when the workspace exists but its session couldn't be persisted — the case where the key must be kept manually.

register_agent also carried name_restates_behavior: "Register an agent identity in the current workspace..." restated the tool name. It now leads with why an agent calls it — claiming an identity is a prerequisite for every messaging tool — and notes the registered name can differ from the requested one when the session rebinds.

Descriptions state only what the code guarantees. Where a handler is looser than the obvious wording would imply, the description follows the handler: submit_result says a timeout leaves the outcome unknown rather than promising a safe retry, and get_message_thread records that limit: 0 is ignored because the handler tests truthiness.

Deliberately unchanged

  • invoke_action already documented its return ("returns an ack with an invocation id"). Left alone rather than churned.
  • Dynamically registered action tools (action-tools.ts:133) take their description from the action descriptor, supplied by whoever registered the action. Not ours to rewrite — worth a follow-up that validates descriptions at registration time if the score should cover them.
  • No handler, schema, or annotation was touched. The diff is description strings plus SECURITY.md. Two review findings proposed changing get_message_thread's runtime handling of limit: 0; I documented the behavior instead, since neither fix is verifiable from the client side in a description-only change. See the review reply for the follow-up worth opening — check_inbox and get_message_thread treat the same parameter differently.

Also in this PR

Commit f61fdaa adds SECURITY.md — a private vulnerability disclosure channel, which the repo lacked despite publishing 17 npm packages and running CodeQL/gitleaks/npm-audit in CI. It predates my seeing the report and is unrelated to the score. Happy to split it into its own PR — say the word and I'll rebase it out.

Two items in it want a human's call: the 3-day/10-day response targets are conventional defaults nobody has committed to, and I deliberately did not invent a security@ address. It also needs private vulnerability reporting enabled in Settings → Code security for the advisory link to work.

Test Plan

  • Tests added/updated — n/a, no behavior changes; the 26 existing MCP tests in agent-relay-mcp.startup.test.ts pass unchanged
  • Manual testing completed
    • npm run typecheck — clean
    • npx vitest run packages/cli/src/cli/agent-relay-mcp.startup.test.ts packages/cli/src/cli/mcp — 26/26 pass
    • npx prettier --check — clean
    • Scripted scan over all 31 statically registered tools confirming each description documents a return
    • Claims verified against the code before writing them: the relay://dm/{conversation_id} resource referenced by list_dms exists at resources.ts:202, and the security.yml coverage described in SECURITY.md matches the job-level if: conditions

Screenshots

n/a

Relay publishes a CLI, a Rust broker, and 17 npm packages that broker
credentials and tool invocations between agents, but the repo documented
no way to report a vulnerability privately. The only channels on offer
were public GitHub issues and Discord, both of which disclose a defect to
every workspace running the affected version before a fix exists.

Add SECURITY.md pointing at GitHub Security Advisories, with supported
versions, scope, and reporting guidance. Scope and out-of-scope reflect
what Relay actually owns: the message protocol's trust boundaries and
credential handling are in, third-party harness defects are out. The
report checklist asks for credentials to be redacted, since workspace
keys, broker keys, and observer tokens routinely appear in Relay logs
and terminal transcripts.

Link it from the README references list.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vfi46d3iWyTEm3NAWi31vy

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

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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Added SECURITY.md with vulnerability-reporting procedures, scope details, supported versions, response expectations, and security-tooling information. Linked the policy from the README. Expanded MCP tool descriptions with return-value and behavior details. Recorded the completed trajectory.

Changes

Documentation updates

Layer / File(s) Summary
Publish and link security policy
SECURITY.md, README.md
Added vulnerability-reporting procedures, report requirements, response timelines, supported-version guidance, scope boundaries, and security-tooling details.
Document agent-relay and action results
packages/cli/src/cli/agent-relay-mcp.ts, packages/cli/src/cli/mcp/action-tools.ts, CHANGELOG.md
Documented result semantics, filtering, persistence, registration, lifecycle behavior, and the related unreleased patch.
Document messaging tool results
packages/cli/src/cli/mcp/messaging-tools.ts
Documented returned records and identifiers, filtering, pagination, conversation, reaction, inbox, and reader behavior.
Record completed trajectory
.agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md, .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/trajectory.json
Recorded completion status, timestamps, decisions, retrospective details, review findings, and CI results.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: khaliqgant

Poem

A rabbit checks each tool reply,
With message IDs lined up nearby.
Security guidance marks the way,
Clearer MCP words ship today.
The burrow rests with docs in place.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: documenting return values for MCP tools.
Description check ✅ Passed The description follows the template and clearly documents scope, testing, screenshots, and deliberate exclusions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/agent-relay-feedback-i6ijz8

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f61fdaa593

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread SECURITY.md Outdated
Comment on lines +89 to +90
- Findings that require an attacker to already have local filesystem or shell
access to the machine running the agent.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep local credential disclosure reports in scope

When a credential file is accidentally group- or world-readable, exploitation necessarily involves another local user or process reading it, so this blanket exclusion rejects exactly the Rust credential-file-mode weakness already tracked as pending security work. Narrow the exclusion to attackers who already have equivalent same-user access rather than excluding every finding involving local filesystem or shell access.

AGENTS.md reference: AGENTS.md:L271-L276

Useful? React with 👍 / 👎.

Comment thread SECURITY.md Outdated

| Component | Version | Supported |
| ----------------------------------------------- | ------- | --------- |
| `agent-relay` CLI and `@agent-relay/*` packages | 11.x | Yes |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Align the supported table with the backport policy

For users on 11.0 through 11.3, this row says their release is supported even though the preceding policy says fixes land only on the latest released minor and are not backported; the current release in CHANGELOG.md is 11.4.0. List 11.4.x here, or change the stated maintenance policy if all 11.x minors are actually supported.

Useful? React with 👍 / 👎.

Comment thread SECURITY.md Outdated
Comment on lines +100 to +102
Every push and pull request runs CodeQL, `npm audit`, dependency review,
license compliance, and gitleaks secret scanning
(`.github/workflows/security.yml`), plus a weekly scheduled run. These catch

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Describe the security workflow's actual gates

For a documentation-only change such as this one, .github/workflows/detect-changes.yml classifies root Markdown as inert, so node_changed is false and .github/workflows/security.yml skips CodeQL, npm audit, and license checking; dependency review also runs only for pull requests, while push events are limited to main. Therefore not every push or pull request runs the listed suite, and this security-posture claim should describe those gates rather than promising universal execution.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
SECURITY.md (1)

58-66: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Clarify the supported-version contract.

Lines 58-59 say fixes land on the latest released minor, but lines 63 and 65 mark the full 11.x and 3.x ranges as supported. State whether older minors are supported but receive no fixes, or narrow the table to the latest minor.

🤖 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 `@SECURITY.md` around lines 58 - 66, Clarify the security-support contract in
the version-support table: either explicitly state that older minors within 11.x
and 3.x remain supported without receiving fixes, or replace those ranges with
the latest supported minor versions. Keep the table entries and introductory
policy consistent so “Supported” has one unambiguous meaning.
🤖 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 `@SECURITY.md`:
- Around line 100-103: Update the security-scan coverage paragraph in
SECURITY.md to accurately describe the conditions in
.github/workflows/security.yml: npm audit, CodeQL, and license compliance run
only when node_changed applies, dependency review runs only for pull requests,
and gitleaks coverage should match the workflow. Retain the weekly scheduled-run
statement if applicable.

---

Nitpick comments:
In `@SECURITY.md`:
- Around line 58-66: Clarify the security-support contract in the
version-support table: either explicitly state that older minors within 11.x and
3.x remain supported without receiving fixes, or replace those ranges with the
latest supported minor versions. Keep the table entries and introductory policy
consistent so “Supported” has one unambiguous meaning.
🪄 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: 672b17cc-b54d-44a3-8eb2-e0624e44e893

📥 Commits

Reviewing files that changed from the base of the PR and between e6fbe9e and f61fdaa.

📒 Files selected for processing (2)
  • README.md
  • SECURITY.md

Comment thread SECURITY.md Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

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.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread SECURITY.md Outdated
Comment thread SECURITY.md Outdated
Comment thread SECURITY.md Outdated
The gradable.dev report on the exposed MCP surface found 28 defects,
almost all of them no_return_description: a tool description said what
the call did but never what came back. An agent reading the tool list
could not tell whether post_message hands back a message ID it can reply
to, whether a successful spawn means the worker is running or merely
queued, or whether an empty list_agents array means "no agents" or "the
filter excluded them".

Add a return clause to all 30 tool descriptions that lacked one, each
grounded in what the handler actually returns — the confirmation string
for textContent tools, the named array for list tools, the record shape
for jsonContent tools. Where the return is asynchronous (spawn,
add_agent, remove_agent, invoke_action) the description now says that
acceptance is not completion.

register_agent also carried a name_restates_behavior defect: "Register
an agent identity..." restated the tool name without adding anything. It
now leads with why an agent calls it — claiming an identity is a
prerequisite for every messaging tool — and notes that the registered
name can differ from the requested one when the session rebinds.

invoke_action already documented its return and is unchanged. Tools
registered dynamically from action descriptors carry descriptions
supplied by whoever registered the action, so they are out of scope.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vfi46d3iWyTEm3NAWi31vy
@willwashburn willwashburn changed the title docs: add security policy with private disclosure channel docs(mcp): state what every MCP tool returns Aug 2, 2026

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 4

🤖 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 `@CHANGELOG.md`:
- Around line 8-12: Change the changelog heading from “## [Unreleased - Patch]”
to “## [Unreleased]”. Rewrite the single bullet in the Changed section to lead
with the user impact—agents can determine what each agent-relay MCP tool
returns—then retain the existing post_message, spawn, list_agents, and
register_agent examples in that same bullet.

In `@packages/cli/src/cli/agent-relay-mcp.ts`:
- Around line 234-235: Update the submit_result tool description in
agent-relay-mcp.ts to state that timeouts or non-2xx responses leave recording
status unknown, rather than asserting the result was not recorded. Do not imply
retries are safe unless the submission uses a stable idempotency or
deduplication guarantee.

In `@packages/cli/src/cli/mcp/messaging-tools.ts`:
- Around line 302-304: Update the description for the list_dms MCP tool to
remove the claim that conversation IDs can be used to send follow-up messages;
describe the conversations array as identifying one-to-one and group DM
conversations only. Do not add a follow-up tool as part of this change.
- Around line 257-259: Update the get_message_thread input handling and thread
invocation so limit: 0 follows the documented contract: either preserve zero
when constructing the options object or reject zero explicitly in the Zod
schema. Keep positive limits and omitted limits behaving as currently
documented.
🪄 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: 06944b4c-23c7-49cd-be56-696db48c7e69

📥 Commits

Reviewing files that changed from the base of the PR and between f61fdaa and ce6b49b.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • packages/cli/src/cli/agent-relay-mcp.ts
  • packages/cli/src/cli/mcp/action-tools.ts
  • packages/cli/src/cli/mcp/messaging-tools.ts

Comment thread CHANGELOG.md
Comment on lines +8 to +12
## [Unreleased - Patch]

### Changed

- Every `agent-relay` MCP tool description now states what the call returns, so an agent can tell from the tool list whether `post_message` hands back a message ID, whether `spawn` means the worker is running, and what an empty `list_agents` result implies. `register_agent` also explains that the registered name can differ from the requested one.

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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Restore the standard [Unreleased] heading and lead with user impact.

## [Unreleased - Patch] does not satisfy the repository changelog rule. Use ## [Unreleased].

Rewrite the bullet so it starts with what agents can now determine, then include the examples.

Suggested changelog entry
-## [Unreleased - Patch]
+## [Unreleased]
@@
-- Every `agent-relay` MCP tool description now states what the call returns, so an agent can tell from the tool list whether `post_message` hands back a message ID, whether `spawn` means the worker is running, and what an empty `list_agents` result implies. `register_agent` also explains that the registered name can differ from the requested one.
+- Agents can now see what each `agent-relay` MCP tool returns, including message IDs, asynchronous spawn status, empty-list meaning, and registered-name rebinding.

As per coding guidelines, CHANGELOG.md must use the correctly leveled [Unreleased] heading and impact-first user-visible entries.
Based on learnings, keep this single user-visible change in one bullet.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## [Unreleased - Patch]
### Changed
- Every `agent-relay` MCP tool description now states what the call returns, so an agent can tell from the tool list whether `post_message` hands back a message ID, whether `spawn` means the worker is running, and what an empty `list_agents` result implies. `register_agent` also explains that the registered name can differ from the requested one.
## [Unreleased]
### Changed
- Agents can now see what each `agent-relay` MCP tool returns, including message IDs, asynchronous spawn status, empty-list meaning, and registered-name rebinding.
🤖 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 `@CHANGELOG.md` around lines 8 - 12, Change the changelog heading from “##
[Unreleased - Patch]” to “## [Unreleased]”. Rewrite the single bullet in the
Changed section to lead with the user impact—agents can determine what each
agent-relay MCP tool returns—then retain the existing post_message, spawn,
list_agents, and register_agent examples in that same bullet.

Sources: Coding guidelines, Learnings

Comment thread packages/cli/src/cli/agent-relay-mcp.ts Outdated
Comment on lines +234 to +235
'Submit the structured result for this spawned Agent Relay task. Call this when the requested work is complete and the result object is ready. ' +
'Returns the acknowledgement payload from the spawning caller. Throws if the caller rejects the submission or the request times out, in which case the result was not recorded and the call can be retried.' +

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.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== candidate file =="
fd -a 'agent-relay-mcp\.ts$' . || true

echo "== relevant sections =="
file="$(fd 'agent-relay-mcp\.ts$' . | head -n 1)"
if [ -n "${file:-}" ]; then
  wc -l "$file"
  sed -n '200,270p' "$file" | cat -n
  echo "---- submit_result/registration call sites around idempotency ----"
  rg -n "submit_result|register_agent|idempotentHint|fetch\(|AbortController|fetchTimeout|timeout|result|token" "$file" -C 3
fi

echo "== repository-wide related searches =="
rg -n "submit_result|registerAgentWithRebind|register_agent|idempotentHint|dedup|idempoten" . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 200

Repository: AgentWorkforce/relay

Length of output: 41627


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="$(fd 'agent-relay-mcp\.ts$' . | head -n 1)"

echo "== submit_result implementation full =="
sed -n '57,95p' "$file" | cat -n

echo "== result submit route/config references =="
rg -n "AGENT_RELAY_RESULT|result/result|submit_result|result submission|unique|idempotency|dedup|deduplicate|insert_if_new|POST" packages crates scripts evals --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 250

echo "== focused result env/schema references =="
python3 - <<'PY'
from pathlib import Path
import re
paths = [Path('packages/cli/src/cli/agent-relay-mcp.ts')]
for p in paths:
    print(f"FILE {p}")
    for i,line in enumerate(p.read_text().splitlines(),1):
        if 'AGENT_RELAY_RESULT' in line or 'submit_result' in line or 'idempotentHint' in line:
            print(f"{i}:{line}")
PY

Repository: AgentWorkforce/relay

Length of output: 33832


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== runtime AGENT_RELAY_RESULT injection and spawn implementation =="
sed -n '520,555p' crates/broker/src/runtime/api.rs | cat -n
echo "----"
sed -n '268,320p' crates/broker/src/runtime/worker_events.rs | cat -n
wc -l crates/broker/src/runtime/worker_events.rs

echo "== submit_result tests =="
sed -n '550,645p' packages/cli/src/cli/agent-relay-mcp.startup.test.ts | cat -n

echo "== deterministic semantic probe: fetch signal abort timing possibilities =="
python3 - <<'PY'
# Model the observed control-flow: server receives request; then either returns 2xx/Error or remains slow;
# client has 2 independent outcomes from request-level network/server delay: AbortError or HTTP error.
options = [
    ("server_committed_then_timeout", {"server_status": "delayed_2xx", "client_obtained_response": False, "outcome": "unknown"}),
    ("server_committed_then_abort_error", {"server_status": "delayed_2xx", "client_obtained_response": False, "client_response_type": "AbortError", "outcome": "unknown"}),
    ("server_committed_then_5xx", {"server_status": "delayed_5xx", "client_obtained_response": True, "client_response_type": "5xx", "outcome": "unknown"}),
    ("server_rejected_immediately", {"server_status": "immediate_4xx", "client_obtained_response": True, "client_response_type": "4xx", "outcome": "rejected_not_recorded"}),
]
for name, body in options:
    print(f"{name}: {body}")
PY

Repository: AgentWorkforce/relay

Length of output: 10352


Do not document result submission timeouts as definite non-recordings.

submit_result sends an unkeyed POST and marks the tool non-idempotent. The server may accept and commit the request before the local AbortController fires, so a timeout or non-2xx response does not guarantee the result was not recorded. Document the outcome as unknown and only allow retries with a stable idempotency or deduplication guarantee.

🤖 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/cli/src/cli/agent-relay-mcp.ts` around lines 234 - 235, Update the
submit_result tool description in agent-relay-mcp.ts to state that timeouts or
non-2xx responses leave recording status unknown, rather than asserting the
result was not recorded. Do not imply retries are safe unless the submission
uses a stable idempotency or deduplication guarantee.

Comment on lines +257 to +259
description:
'Retrieve a message thread. ' +
'Returns the parent message together with its replies, capped by `limit` when supplied.',

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.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

rg -n -C 8 \
  'get_message_thread|\.thread\(|limit \?|limit != null|limit: 0' \
  packages/cli --glob '*.ts'

Repository: AgentWorkforce/relay

Length of output: 7434


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "Locate agent client thread definitions/usages:"
rg -n -C 6 'async thread|thread\(|interface .*Client|class .*Client' packages --glob '*.ts' | head -n 220

echo
echo "Inspect messaging-tools relevant section:"
sed -n '230,275p' packages/cli/src/cli/mcp/messaging-tools.ts

echo
echo "Locate and inspect possible thread implementation files:"
fd -a '.*\.(ts|js)$' packages | rg 'message|relay|agent' | sed -n '1,120p'

Repository: AgentWorkforce/relay

Length of output: 18922


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "AgentRelay outline focusing messaging/thread methods:"
ast-grep outline packages/sdk/src/agent-relay.ts --match 'thread' --view expanded || true

echo
echo "Relevant messaging/thread implementation sections:"
rg -n -C 12 'thread|RelayMessaging|threads|limit' packages/sdk/src --glob '*.ts'

echo
echo "Relevant MCP client factory methods and imports:"
rg -n -C 8 'getAgentClient|agentRelay|AgentRelay|registerTool|thread\\(' packages/cli packages/harnesses packages/sdk --glob '*.ts' | r sed -n '1,260p'

Repository: AgentWorkforce/relay

Length of output: 50379


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "agent-relay thread method candidates:"
rg -n -C 16 'thread\(' packages/sdk/src/agent-relay.ts packages/sdk/src/messaging/relaycast*.ts packages/sdk/src/messaging/relay*.ts

echo
echo "messaging types around limit/thread options:"
rg -n -C 8 'RelayMessage|limit.*number|threads:' packages/sdk/src/messaging --glob '*.ts'

echo
echo "tool/client factory imports and getAgentClient implementation:"
rg -n -C 12 'getAgentClient|export .*getAgentClient|function getAgentClient|const getAgentClient' packages/cli/src packages/harnesses/src --glob '*.ts'

Repository: AgentWorkforce/relay

Length of output: 50377


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Tool registration and getAgentClient implementation:"
sed -n '1,45p' packages/cli/src/cli/agent-relay-mcp.ts
sed -n '240,280p' packages/cli/src/cli/agent-relay-mcp.ts
rg -n -C 20 'function getAgentClient|const getAgentClient|export .*getAgentClient|getAgentClient\s*=' packages --glob '*.ts'

echo
echo "SDK facade AgentRelay agent client/threads implementation section:"
rg -n -C 22 'readonly threads|threads|toMessageListOptions|class AgentRelay|getAgentClient|RelayClientAgent' packages/sdk/src --glob '*.ts'

Repository: AgentWorkforce/relay

Length of output: 50376


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Thin client createAgentClient implementation:"
rg -n -C 22 'function createAgentClient|export function createAgentClient|const createAgentClient|createAgentClient|^.*createAgentClient' packages/sdk/src --glob '*.ts'

echo
echo "Thin client thread implementation and type:"
rg -n -C 22 'thread\(id: string|thread\(messageId: string|toMessageListOptions|defineOptions|definedOptions|limit.*number|export type RelayMessageListOptions' packages/sdk/src/messaging --glob '*.ts'

echo
echo "MCP get_message_thread implementation with line numbers:"
nl -ba packages/cli/src/cli/mcp/messaging-tools.ts | sed -n '253,271p'

Repository: AgentWorkforce/relay

Length of output: 50379


Handle limit: 0 consistently with the documented contract.

get_message_thread accepts limit: 0 through z.number().optional(), but limit ? { limit } : undefined drops it and calls thread(message_id) without options. If zero is valid, pass { limit: 0 }; otherwise, reject it in the input schema.

Suggested implementation fix
-      jsonContent(await getAgentClient(as).thread(message_id, limit ? { limit } : undefined))
+      jsonContent(await getAgentClient(as).thread(message_id, limit != null ? { limit } : undefined))
🤖 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/cli/src/cli/mcp/messaging-tools.ts` around lines 257 - 259, Update
the get_message_thread input handling and thread invocation so limit: 0 follows
the documented contract: either preserve zero when constructing the options
object or reject zero explicitly in the Zod schema. Keep positive limits and
omitted limits behaving as currently documented.

Comment thread packages/cli/src/cli/mcp/messaging-tools.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

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.

All reported issues were addressed across 4 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/cli/src/cli/mcp/messaging-tools.ts
Comment thread packages/cli/src/cli/mcp/messaging-tools.ts
Comment thread packages/cli/src/cli/agent-relay-mcp.ts Outdated
Three tool descriptions asserted more than the code guarantees:

submit_result said a timeout meant the result was not recorded and the
call could be retried. A timeout only means the client aborted before
reading a response; the server may already have recorded the result, so
a retry can duplicate it. The description now says the outcome is
unknown.

list_dms said conversation IDs are used to send follow-up messages. No
tool accepts a conversation ID — send_dm addresses a recipient by name.
The IDs do address the relay://dm/{conversation_id} resource, so the
description now points there and says how to actually reply.

get_message_thread said results are capped by limit "when supplied", but
the handler tests limit for truthiness, so limit: 0 is dropped and the
full thread comes back. Documented the actual behavior rather than
changing the handler, which would alter runtime behavior in a
description-only change.

SECURITY.md carried three inaccuracies of its own:

- The supported-versions table marked all of 11.x and 3.x supported
  while the text said only the latest minor gets fixes. Split into
  "reports investigated" and "fix delivered in" so each column means one
  thing.
- The local-access exclusion was broad enough to reject reports about
  world-readable key files, since exploiting those needs another local
  user. Narrowed to same-user access, with cross-user weaknesses named
  as in scope.
- The tooling section claimed every push runs the full suite. In fact
  CodeQL, npm audit, and license compliance are gated on Node-toolchain
  changes, dependency review is PR-only, CodeQL covers JS/TS only (not
  the in-scope Rust broker), and several jobs are advisory. Replaced
  with the real coverage.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vfi46d3iWyTEm3NAWi31vy

Copy link
Copy Markdown
Member Author

Worked through the review feedback in 8c9e563. Six findings fixed, one declined.

Fixed

submit_result retry guidance (cubic P3, CodeRabbit) — Valid, and the more serious of the two code findings. I'd written that a timeout means "the result was not recorded and the call can be retried." A timeout only means the client aborted before reading a response; the server may already have recorded the result. That wording could have driven an agent to double-submit. Now states the outcome is unknown and a retry is a possible duplicate.

list_dms conversation IDs (cubic P2, CodeRabbit) — Valid that no tool consumes a conversation ID; send_dm addresses a recipient by name. But the IDs aren't inert: resources.ts:202 registers relay://dm/{conversation_id}, which serves that conversation's messages. Rather than delete the clause I pointed it at the resource and said how to actually reply. Verified before writing it.

get_message_thread and limit: 0 (cubic P3, CodeRabbit) — Real mismatch. The handler is limit ? { limit } : undefined, so 0 is dropped and the full thread returns, contradicting "capped by limit when supplied."

I documented the behavior instead of changing the handler. Both suggested fixes — honoring 0, or rejecting it in the schema — change runtime behavior, and I can't verify what the API does with limit: 0 from the client side. That doesn't belong in a description-only change. Worth noting for a follow-up: check_inbox uses limit != null ? { limit } : undefined for the same parameter, so the two tools genuinely disagree. Happy to open an issue.

SECURITY.md supported versions (cubic P2, CodeRabbit) — Valid contradiction: the prose said only the latest minor gets fixes while the table marked all of 11.x supported. Split into "reports investigated" and "fix delivered in" so each column asserts one thing.

SECURITY.md local-access exclusion (cubic P2) — Good catch, and the finding I'd have been most annoyed to ship. As written it would have rejected reports about world-readable key files, since exploiting those inherently requires another local user. Narrowed to same-user access, with cross-user weaknesses — permissive file modes, predictable shared temp paths — named as explicitly in scope.

SECURITY.md tooling claims (cubic P2, CodeRabbit) — Valid and worth getting right, since reporters judge scanning posture from this. I'd claimed every push runs the full suite. Actually CodeQL, npm audit, and license compliance are gated on node_changed; dependency review is PR-only; CodeQL covers javascript-typescript only, so the in-scope Rust broker isn't scanned by it; and several jobs are continue-on-error. Replaced with the real coverage, including that a green run doesn't mean no findings.

Declined

Changing ## [Unreleased - Patch] to ## [Unreleased] (CodeRabbit) — This conflicts with the repo's own convention. CLAUDE.md requires the first pending user-visible change to set the heading to [Unreleased - Patch], [Unreleased - Minor], or [Unreleased - Major] by SemVer impact, and the release tooling reads it. Reverting to a bare [Unreleased] would break that. Leaving as-is.

The same review asked the bullet to lead with user impact; it already does — it opens with what an agent can now determine from the tool list, then gives the examples.

Verification

npm run typecheck clean · 26/26 MCP tests pass · prettier --check clean.

All 40 checks were green on the previous head; this push re-runs them. mergeable_state is blocked only on the requested review from @khaliqgant — nothing on my side is outstanding.


Generated by Claude Code

@coderabbitai coderabbitai Bot left a comment

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.

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 @.agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md:
- Around line 1-13: Replace the durable trajectory record as a single
task-specific update: in
.agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md
lines 1-13, replace the escape-aware paced injection title and summary with the
current MCP documentation and SECURITY.md work; in trajectory.json lines 11-13,
update completion metadata for the correct task, line 27, update the work end
time, and lines 88-108, retain only decisions and retrospective findings from
this PR.
🪄 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: 1e718c46-bed3-4fed-be49-9fa11f4f2af3

📥 Commits

Reviewing files that changed from the base of the PR and between 8c9e563 and 4242b9c.

📒 Files selected for processing (2)
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/trajectory.json

Comment on lines +1 to +13
# Trajectory: Implement escape-aware paced injection (issue #801)

> **Status:** ✅ Completed
> **Task:** 801
> **Confidence:** 85%
> **Started:** July 15, 2026 at 07:48 PM
> **Completed:** August 2, 2026 at 10:53 PM

---

## Summary

Fixed 6 review findings (3 MCP description overclaims, 3 SECURITY.md inaccuracies); declined CodeRabbit's CHANGELOG heading change as contrary to CLAUDE.md convention. All 40 CI checks green.

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.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Keep the durable trajectory record aligned with this PR.

The current PR concerns MCP documentation and SECURITY.md, but this record describes issue #801 escape-aware paced injection work. Replace the record as one unit instead of mixing completion metadata, decisions, and retrospective findings from different tasks.

  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md#L1-L13: replace the task title and summary.
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/trajectory.json#L11-L13: set completion metadata for the correct task.
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/trajectory.json#L27-L27: record the correct work end time.
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/trajectory.json#L88-L108: store only decisions and retrospective data for this PR.
📍 Affects 2 files
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md#L1-L13 (this comment)
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/trajectory.json#L11-L13
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/trajectory.json#L27-L27
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/trajectory.json#L88-L108
🤖 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-08/traj_9835z9cvpl9q/summary.md
around lines 1 - 13, Replace the durable trajectory record as a single
task-specific update: in
.agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md
lines 1-13, replace the escape-aware paced injection title and summary with the
current MCP documentation and SECURITY.md work; in trajectory.json lines 11-13,
update completion metadata for the correct task, line 27, update the work end
time, and lines 88-108, retain only decisions and retrospective findings from
this PR.

@coderabbitai coderabbitai Bot left a comment

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.

♻️ Duplicate comments (1)
.agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md (1)

1-59: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Replace this trajectory record with metadata for the current PR.

This file still records issue #801 escape-aware paced injection work, including unrelated PTY, WebSocket, SIGKILL, and smoke-test decisions. The current PR documents MCP tool results and adds SECURITY.md. Replace the summary, decisions, and work list with task-specific content, and update the companion trajectory.json in the same change.

🤖 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-08/traj_9835z9cvpl9q/summary.md
around lines 1 - 59, Replace the contents of the trajectory summary with
metadata describing the current PR’s MCP tool-results documentation and
SECURITY.md changes, removing the unrelated issue `#801`, PTY, WebSocket, SIGKILL,
and smoke-test details. Update the summary, key decisions, and work list
consistently, and modify the companion trajectory.json to match the new
task-specific metadata.
🤖 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.

Duplicate comments:
In @.agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md:
- Around line 1-59: Replace the contents of the trajectory summary with metadata
describing the current PR’s MCP tool-results documentation and SECURITY.md
changes, removing the unrelated issue `#801`, PTY, WebSocket, SIGKILL, and
smoke-test details. Update the summary, key decisions, and work list
consistently, and modify the companion trajectory.json to match the new
task-specific metadata.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5c90c6a9-70df-4637-945f-809662fad030

📥 Commits

Reviewing files that changed from the base of the PR and between 4242b9c and c098328.

📒 Files selected for processing (2)
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/trajectory.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/trajectory.json

@cubic-dev-ai cubic-dev-ai Bot left a comment

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.

1 issue found across 2 files (changes from recent commits).

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=".agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md">

<violation number="1" location=".agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md:1">
P2: The trajectory file is internally inconsistent: the title/Task reference issue #801 "escape-aware paced injection" and the Key Decisions/Chapters detail code changes (forced WebSocket teardown via ws.terminate for view detach, post-SIGKILL reaping wait in the node provider integration test, replacing the fixed standalone smoke delay, drainer-thread paced injection) — none of which exist in this documentation-only PR, which explicitly makes "no behavior changes." The Summary only describes 3 MCP description overclaims and 3 SECURITY.md inaccuracies. As committed documentation this is misleading (stale/unrelated task metadata), so please correct the title/task and remove or replace the unrelated decision content, or generate the trajectory from the actual PR work.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

@@ -0,0 +1,59 @@
# Trajectory: Implement escape-aware paced injection (issue #801)

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.

P2: The trajectory file is internally inconsistent: the title/Task reference issue #801 "escape-aware paced injection" and the Key Decisions/Chapters detail code changes (forced WebSocket teardown via ws.terminate for view detach, post-SIGKILL reaping wait in the node provider integration test, replacing the fixed standalone smoke delay, drainer-thread paced injection) — none of which exist in this documentation-only PR, which explicitly makes "no behavior changes." The Summary only describes 3 MCP description overclaims and 3 SECURITY.md inaccuracies. As committed documentation this is misleading (stale/unrelated task metadata), so please correct the title/task and remove or replace the unrelated decision content, or generate the trajectory from the actual PR work.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .agentworkforce/trajectories/completed/2026-08/traj_9835z9cvpl9q/summary.md, line 1:

<comment>The trajectory file is internally inconsistent: the title/Task reference issue #801 "escape-aware paced injection" and the Key Decisions/Chapters detail code changes (forced WebSocket teardown via ws.terminate for view detach, post-SIGKILL reaping wait in the node provider integration test, replacing the fixed standalone smoke delay, drainer-thread paced injection) — none of which exist in this documentation-only PR, which explicitly makes "no behavior changes." The Summary only describes 3 MCP description overclaims and 3 SECURITY.md inaccuracies. As committed documentation this is misleading (stale/unrelated task metadata), so please correct the title/task and remove or replace the unrelated decision content, or generate the trajectory from the actual PR work.</comment>

<file context>
@@ -0,0 +1,59 @@
+# Trajectory: Implement escape-aware paced injection (issue #801)
+
+> **Status:** ✅ Completed
</file context>

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.

2 participants