fix(cli): expose direct-message enqueue state - #1468
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughThe change adds direct-message delivery receipts across CLI and MCP tools. Receipts report recipient resolution, delivery mode, enqueue status, and reader confirmation. Documentation defines ChangesDirect-message delivery reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant send_dm
participant AgentDirectory
participant directMessageReceipt
participant messageReadersReceipt
Sender->>send_dm: Send message with mode
send_dm->>AgentDirectory: Resolve exact recipient
AgentDirectory-->>send_dm: Return matching agent
send_dm->>directMessageReceipt: Normalize enqueue response
directMessageReceipt-->>Sender: Return delivery receipt
Sender->>messageReadersReceipt: Check message readers
messageReadersReceipt-->>Sender: Return read or queued_or_unread status
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38496ccbf5
ℹ️ 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".
| }) | ||
| ); | ||
| return this.normalizeDirectResponse(response, 'dm'); | ||
| return this.normalizeDirectResponse(response, 'dm', undefined, input.to); |
There was a problem hiding this comment.
normalizeDirectResponse receives the requested input.to as agentName and emits it as the message target, while discarding any recipient information from the raw response. Because directMessageReceipt prioritizes this target, agent-relay message dm send will always report recipientMatched: true and echo the request as resolvedRecipient, even when the backend response identifies a different or canonicalized recipient. Preserve the response-derived recipient or report it as unresolved rather than presenting the request value as independently resolved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0f9a620. The receipt no longer trusts the SDK/request target as an independent resolution. MCP and CLI now list the workspace directory, require a full exact agent-name match before sending, and pass that independently resolved name into the receipt. With no resolver the helper reports recipient_unresolved; the SDK target echo change was removed. Added exact hyphenated, strict-prefix, missing-name, mismatch, and unresolved regressions. Local follow-up: 32/32 changed tests, 163/163 SDK tests, root typecheck, lint, diff check, and secret scan all exit 0.
![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
🤖 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`:
- Line 12: Rename the changelog heading containing “Unreleased - Patch” to the
required exact heading “Unreleased,” preserving the new entry and all other
changelog content unchanged.
In `@packages/cli/src/cli/commands/message.ts`:
- Line 11: Update the inbox get_readers handler to import messageReadersReceipt
alongside directMessageReceipt, wrap messages.readers(messageId) with
messageReadersReceipt, and pass the resulting receipt to printJson so empty
reader lists retain the queued_or_unread signal.
In `@packages/sdk/src/messaging/relaycast.ts`:
- Line 288: Update the direct-message response flow around
normalizeDirectResponse so the requested input.to and the server-resolved
recipient remain separate, with message.target representing the server value
used by directMessageReceipt. Ensure recipient validation can return
recipient_mismatch when resolution differs, and add an end-to-end test covering
a response that resolves to a different recipient.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7a1e053-4d44-4337-9299-611847879109
📒 Files selected for processing (15)
.agents/skills/using-agent-relay/SKILL.md.claude/skills/using-agent-relay/SKILL.mdCHANGELOG.mdpackages/cli/README.mdpackages/cli/src/cli/commands/message.tspackages/cli/src/cli/commands/relaycast-groups.test.tspackages/cli/src/cli/lib/message-delivery-receipts.tspackages/cli/src/cli/mcp/messaging-tools.delivery.test.tspackages/cli/src/cli/mcp/messaging-tools.protocol.test.tspackages/cli/src/cli/mcp/messaging-tools.tspackages/sdk-py/README.mdpackages/sdk/src/__tests__/messaging.test.tspackages/sdk/src/messaging/normalize.tspackages/sdk/src/messaging/relaycast.tspackages/sdk/src/messaging/types.ts
![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 15 files
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="packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts">
<violation number="1" location="packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts:10">
P3: The mocked `dm` response carries no target/recipient identifier, so `resolvedRecipient` here is just the `requestedRecipient` fallback in `resolvedDirectRecipient`; the assertion `resolvedRecipient: 'chief-khaliq'` passes regardless of whether receipt resolution works. Since the PR explicitly aims to "preserve and assert the exact requested/resolved DM recipient", this regression test doesn't exercise that path and would not catch a broken resolve.</violation>
</file>
<file name="packages/cli/src/cli/lib/message-delivery-receipts.ts">
<violation number="1" location="packages/cli/src/cli/lib/message-delivery-receipts.ts:69">
P3: The new `messageReadersReceipt` helper (which turns an empty reader list into an explicit `queued_or_unread` signal) is wired only into the MCP `get_message_readers` tool, not into the CLI's `inbox get_readers` / `dm get_readers` command in `commands/message.ts`. The CLI still prints the raw `readers` array with no delivery signal, so at the CLI public choice point an empty result is not surfaced as `queued_or_unread` — inconsistent with the PR's goal of exposing this signal. Consider applying the same receipt in the CLI `get_readers` handlers for a consistent contract across surfaces.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| describe('messaging delivery receipts over MCP', () => { | ||
| it('exposes enqueue state on send and an explicit signal for an empty reader list', async () => { | ||
| const dm = vi.fn(async () => ({ id: 'msg_1', text: 'hello' })); |
There was a problem hiding this comment.
P3: The mocked dm response carries no target/recipient identifier, so resolvedRecipient here is just the requestedRecipient fallback in resolvedDirectRecipient; the assertion resolvedRecipient: 'chief-khaliq' passes regardless of whether receipt resolution works. Since the PR explicitly aims to "preserve and assert the exact requested/resolved DM recipient", this regression test doesn't exercise that path and would not catch a broken resolve.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts, line 10:
<comment>The mocked `dm` response carries no target/recipient identifier, so `resolvedRecipient` here is just the `requestedRecipient` fallback in `resolvedDirectRecipient`; the assertion `resolvedRecipient: 'chief-khaliq'` passes regardless of whether receipt resolution works. Since the PR explicitly aims to "preserve and assert the exact requested/resolved DM recipient", this regression test doesn't exercise that path and would not catch a broken resolve.</comment>
<file context>
@@ -0,0 +1,51 @@
+
+describe('messaging delivery receipts over MCP', () => {
+ it('exposes enqueue state on send and an explicit signal for an empty reader list', async () => {
+ const dm = vi.fn(async () => ({ id: 'msg_1', text: 'hello' }));
+ const readers = vi.fn(async () => []);
+ const server = new McpServer({ name: 'messaging-test', version: '1.0.0' });
</file context>
There was a problem hiding this comment.
Fixed in 0f9a620. The protocol test now supplies an independent workspace-directory callback containing both chief and chief-khaliq; the send receipt exact recipient comes from that directory lookup, not the DM response.
| }; | ||
| } | ||
|
|
||
| export function messageReadersReceipt(readers: unknown[]): { |
There was a problem hiding this comment.
P3: The new messageReadersReceipt helper (which turns an empty reader list into an explicit queued_or_unread signal) is wired only into the MCP get_message_readers tool, not into the CLI's inbox get_readers / dm get_readers command in commands/message.ts. The CLI still prints the raw readers array with no delivery signal, so at the CLI public choice point an empty result is not surfaced as queued_or_unread — inconsistent with the PR's goal of exposing this signal. Consider applying the same receipt in the CLI get_readers handlers for a consistent contract across surfaces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli/lib/message-delivery-receipts.ts, line 69:
<comment>The new `messageReadersReceipt` helper (which turns an empty reader list into an explicit `queued_or_unread` signal) is wired only into the MCP `get_message_readers` tool, not into the CLI's `inbox get_readers` / `dm get_readers` command in `commands/message.ts`. The CLI still prints the raw `readers` array with no delivery signal, so at the CLI public choice point an empty result is not surfaced as `queued_or_unread` — inconsistent with the PR's goal of exposing this signal. Consider applying the same receipt in the CLI `get_readers` handlers for a consistent contract across surfaces.</comment>
<file context>
@@ -0,0 +1,84 @@
+ };
+}
+
+export function messageReadersReceipt(readers: unknown[]): {
+ readers: unknown[];
+ delivery: { status: 'read' | 'queued_or_unread'; readConfirmed: boolean; signal: string };
</file context>
There was a problem hiding this comment.
Fixed in 49f8ba3. CLI message inbox get_readers now emits the same read or queued_or_unread receipt contract as the MCP tool, with a dedicated regression.
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
Fixes #1467
Summary
get_message_readersresult into an explicitqueued_or_unreadsignalwait(on-idle) versussteer(immediate, possibly interrupting) at every public choice point#1466is intentionally not included: Relay's local broker/CLI/SDK has no mention tokenizer; mention resolution occurs in the upstream Relaycast service. The receipt helper does defensively flag a supplied recipient mismatch.Verification
The runtime dependency audit still exits 1 on 11 existing findings; this branch changes no dependencies. Veto was unavailable in the lane, so no Veto verdict is claimed. This PR is not merged.
Review follow-up
The first revision echoed the requested SDK target as though it were independently resolved. Commit
0f9a6201fixes that P2: MCP and CLI now resolve against the workspace directory before sending, exact matching handles hyphenated names and strict-prefix agents correctly, a missing exact name fails before enqueue, and an unavailable resolver reportsrecipient_unresolvedrather than claiming a match.Commit
49f8ba3bcloses the remaining receipt gaps found in review: unresolved receipts discard any untrusted response target, CLI reader output now carries the samequeued_or_unreadsignal as MCP, and the changelog entry is split into short impact-first bullets.Documentation-only follow-up
3698f0f7restores the exact MCP and CLI interface names required by the changelog guidance. Remote HEAD is confirmed as3698f0f7f6a34fae69d6cf3d81097fb1dc74b0f5; all 11 Actions workflows triggered for that exact SHA completed successfully. Codegen Models, Test Build Scripts, Test Install Script, Rust Auto-Format, and Cancel PR Jobs on Merge were absent due their event/path conditions and are not counted as passing.