fix(sdk): fail fast when agent messaging has no encryption#260
Conversation
The Agent E2E facade (sendMessage) silently sent the body as plaintext
when the client was built without `encryption: { store }`. The relay
rejects a plaintext JSON body with a cryptic `HTTP 400: body must be
encrypted ciphertext` — surfacing far from the real cause (a client/daemon
constructed without a signer or session store).
Guard the facade on `client.encryptionEnabled` and throw a clear, actionable
error instead of leaking plaintext. The low-level transparent `client.messages.send`
path is untouched, so intentional plain relay transport still works.
Co-Authored-By: Claude <noreply@anthropic.com>
|
@sanil-23 is attempting to deploy a commit to the Vezures Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesEncrypted messaging
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@sdk/typescript/src/agent/messaging.ts`:
- Around line 104-116: The sendMessage function currently exceeds the 20-line
limit and lacks asynchronous error handling. Extract the encryption guard or
envelope construction into a focused helper, then wrap sendMessage’s directory
and transport operations in try/catch while preserving and propagating the
original error and cause.
🪄 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
Run ID: a130d731-92a9-4966-bca3-728334a3d7fc
📒 Files selected for processing (2)
sdk/typescript/src/agent/messaging.tssdk/typescript/tests/agent-messaging.test.ts
| export async function sendMessage( | ||
| client: TinyPlaceClient, | ||
| signer: AgentSigner, | ||
| recipient: string, | ||
| text: string, | ||
| ): Promise<SendMessageResult> { | ||
| if (!client.encryptionEnabled) { | ||
| throw new Error( | ||
| "agent messaging requires encryption: construct the client with " + | ||
| "`encryption: { store }` and a signer. Sending a plaintext body over the " + | ||
| 'E2E channel is rejected by the relay ("body must be encrypted ciphertext").', | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the guard and add async error handling.
sendMessage now exceeds the 20-line limit, and its asynchronous directory/transport operations still lack a try/catch. Extract the encryption assertion or envelope construction into helpers, then catch failures while preserving the original error/cause.
As per coding guidelines, async functions must use try-catch blocks and functions are limited to 20 lines.
🤖 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 `@sdk/typescript/src/agent/messaging.ts` around lines 104 - 116, The
sendMessage function currently exceeds the 20-line limit and lacks asynchronous
error handling. Extract the encryption guard or envelope construction into a
focused helper, then wrap sendMessage’s directory and transport operations in
try/catch while preserving and propagating the original error and cause.
Source: Coding guidelines
There was a problem hiding this comment.
Addressed the quick win: extracted the encryption assertion into assertEncryptionEnabled() in 1561dc6, so sendMessage is back to a focused ~14-line body.
Declined the paired try/catch: no function in this module (resolveRecipientKey, publishKeys, readMessages) wraps its async directory/transport calls — the established idiom here is to let errors propagate to the caller with their original stack/cause intact. A re-throw wrapper would be inconsistent noise and could obscure the cause rather than preserve it.
There was a problem hiding this comment.
@sanil-23 Thanks for the update — extracting assertEncryptionEnabled() brings sendMessage back under the line limit while keeping the guard clearly named and documented.
Your reasoning on the try/catch is fair: since resolveRecipientKey, publishKeys, and readMessages all let async errors propagate untouched, adding a catch-and-rethrow just in sendMessage would be inconsistent with the module's existing convention and risks flattening the original stack/cause for no real benefit. I won't push further on that point.
✏️ Learnings added
Learnt from: sanil-23
Repo: tinyhumansai/tiny.place PR: 260
File: sdk/typescript/src/agent/messaging.ts:0-0
Timestamp: 2026-07-15T15:48:16.593Z
Learning: In `sdk/typescript/src/agent/messaging.ts`, async functions (`resolveRecipientKey`, `publishKeys`, `sendMessage`, `readMessages`) intentionally do not wrap their directory/transport calls in try/catch. The established idiom in this module is to let errors from `client.directory.resolve`, `client.messages.send`, `client.messages.list`, etc. propagate to the caller with their original stack/cause intact, rather than catching and re-throwing.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Addresses CodeRabbit: keep sendMessage focused/short by moving the encryption assertion into assertEncryptionEnabled(). Declined the paired try/catch suggestion — no sibling in this module wraps its async directory/transport calls, the file's idiom is to let errors propagate. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Problem
The Agent E2E facade (
agent.sendMessage/sendMessage()) silently sent the body as plaintext when the underlyingTinyPlaceClientwas built withoutencryption: { store }(i.e. no encryption context — e.g. a daemon whose client was constructed without a signer/session store).The relay rejects a plaintext JSON body via its
looksLikeJSONguard with:That error surfaces far from the real cause: the send code looks correct, the encrypt stack (
EncryptionContext.encryptEnvelope→session.encrypt) never actually ran, and the operator sees a confusing relay 400 rather than "this client has no encryption configured." Every encrypt path in the SDK either produces real ciphertext or throws — so a plaintext body reaching the relay always means encryption was never wired, which is exactly the case worth catching loudly.Fix
Guard the E2E facade on the existing
client.encryptionEnabledgetter and throw a clear, actionable error before building/sending the envelope:The low-level transparent
client.messages.sendpath is untouched, so callers that genuinely want plain relay transport (and its documented transparent-plaintext behavior) are unaffected.Validation
pnpm --filter @tinyhumansai/tinyplace build(tsc) ✅pnpm --filter @tinyhumansai/tinyplace test— 578 passed / 99 skipped ✅ (adds a case asserting the facade throws when the client has no encryption)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests