fix(ws): derive attachment caps from the ws frame limit (#347) - #380
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change centralizes WebSocket payload limits in the protocol package. The server, message validation, attachment validation, and recorder tests now use these shared limits. New tests cover oversized messages and attachment size boundaries. ChangesWebSocket payload budget
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
There was a problem hiding this comment.
Pull request overview
Aligns attachment and message limits with the shared WebSocket frame budget to prevent oversized RPC frames from closing a tab’s shared socket.
Changes:
- Centralizes WebSocket payload limits in protocol.
- Derives attachment caps and adds aggregate send-size validation.
- Adds boundary and recorder frame-budget tests.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Adds the serve-to-protocol dependency and updates resolutions. |
packages/ui-kit-chat/test/attachment-adapter.test.ts |
Tests image-size boundaries. |
packages/ui-kit-chat/src/primitives/attachment/attachment-adapter.ts |
Derives the image cap from protocol. |
packages/serve/src/serve.ts |
Uses the shared WebSocket limit. |
packages/serve/package.json |
Adds the protocol dependency. |
packages/protocol/src/rpc-types.ts |
Defines shared payload limits and budget. |
packages/protocol/src/chat-types.ts |
Derives raw and base64 attachment caps. |
packages/extensions/recorder/test/flush-socket.it.test.ts |
Uses the shared frame budget in assertions. |
apps/conciv/test/send-checks.test.ts |
Tests aggregate payload rejection. |
apps/conciv/src/pane/send-checks.ts |
Blocks messages exceeding the RPC budget. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const MAX_ATTACHMENT_BASE64_LENGTH = 27_962_028 | ||
| const MEBIBYTE = 1024 * 1024 | ||
| export const MAX_ATTACHMENT_RAW_BYTES = Math.floor((WS_RPC_PAYLOAD_BUDGET_BYTES * 3) / 4 / MEBIBYTE) * MEBIBYTE | ||
| const MAX_ATTACHMENT_BASE64_LENGTH = Math.ceil(MAX_ATTACHMENT_RAW_BYTES / 3) * 4 |
| '@tanstack/react-router': | ||
| specifier: latest | ||
| version: 1.170.22(react-dom@19.2.7(react@19.2.7))(react@19.2.7) | ||
| version: 1.170.23(react-dom@19.2.7(react@19.2.7))(react@19.2.7) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/ui-kit-chat/test/attachment-adapter.test.ts (2)
52-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the oversized fixture from
MAX_ATTACHMENT_RAW_BYTES.Line 52 hardcodes 12 MiB. The fixture is oversized only with the current shared constants. Use
MAX_ATTACHMENT_RAW_BYTES + 1so the test remains tied to the attachment contract.Proposed fixture update
- const big = new File([new Uint8Array(12 * 1024 * 1024)], 'huge.png', {type: 'image/png'}) + const big = new File([new Uint8Array(MAX_ATTACHMENT_RAW_BYTES + 1)], 'huge.png', {type: 'image/png'})🤖 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/ui-kit-chat/test/attachment-adapter.test.ts` around lines 52 - 57, Update the oversized file fixture in the adapter test to allocate MAX_ATTACHMENT_RAW_BYTES + 1 bytes instead of the hardcoded 12 MiB value, preserving the existing incomplete status and send rejection assertions.
59-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the actual encoded payload at the cap.
Lines 59-72 check the add status and recompute the Base64 formula. They do not inspect the value produced by
fileToDataSource. Calladapter.send(pending)orfileToDataSource(largest)and assert the returned data-source value length.🤖 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/ui-kit-chat/test/attachment-adapter.test.ts` around lines 59 - 72, Update the cap test around createSimpleImageAttachmentAdapter to exercise the actual encoded payload by calling adapter.send(pending) or fileToDataSource(largest), then assert the returned data-source value length is below WS_RPC_PAYLOAD_BUDGET_BYTES. Remove the standalone largestBase64Length formula assertion while preserving the existing add-status checks.
🤖 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 `@apps/conciv/src/pane/send-checks.ts`:
- Around line 22-27: The checkSend payload-size validation currently measures
only content; update it to serialize and measure the complete outbound ORPC
frame containing sessionId, runId, and content, reusing the serializer used by
chatConnection where available. Keep the existing rejection response and
WS_RPC_PAYLOAD_BUDGET_BYTES threshold unchanged.
---
Nitpick comments:
In `@packages/ui-kit-chat/test/attachment-adapter.test.ts`:
- Around line 52-57: Update the oversized file fixture in the adapter test to
allocate MAX_ATTACHMENT_RAW_BYTES + 1 bytes instead of the hardcoded 12 MiB
value, preserving the existing incomplete status and send rejection assertions.
- Around line 59-72: Update the cap test around
createSimpleImageAttachmentAdapter to exercise the actual encoded payload by
calling adapter.send(pending) or fileToDataSource(largest), then assert the
returned data-source value length is below WS_RPC_PAYLOAD_BUDGET_BYTES. Remove
the standalone largestBase64Length formula assertion while preserving the
existing add-status checks.
🪄 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: 1c6efd31-b21a-48d1-a6ba-8c0e5c970672
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/conciv/src/pane/send-checks.tsapps/conciv/test/send-checks.test.tspackages/extensions/recorder/test/flush-socket.it.test.tspackages/protocol/src/chat-types.tspackages/protocol/src/rpc-types.tspackages/serve/package.jsonpackages/serve/src/serve.tspackages/ui-kit-chat/src/primitives/attachment/attachment-adapter.tspackages/ui-kit-chat/test/attachment-adapter.test.ts
| if (new TextEncoder().encode(JSON.stringify(content)).length > WS_RPC_PAYLOAD_BUDGET_BYTES) | ||
| return { | ||
| ok: false, | ||
| message: 'Message too large to send. Remove an attachment or shorten it.', | ||
| tone: 'warn', | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 '\bcheckSend\s*\(' apps/conciv packages --glob '*.{ts,tsx}'
rg -n -C 12 'JSON\.stringify|socket\.send|\.send\s*\(|WebSocket|rpc-ws' apps/conciv/src packages --glob '*.{ts,tsx}'Repository: conciv-dev/conciv
Length of output: 50374
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- skill files ---'
find .. -type f \( -iname '*skill*' -o -iname 'AGENTS.md' -o -iname 'CLAUDE.md' \) -print 2>/dev/null | head -200
printf '%s\n' '--- target file ---'
sed -n '1,180p' apps/conciv/src/pane/send-checks.ts
printf '%s\n' '--- direct call sites ---'
rg -n -C 12 '\b(checkSend|WS_RPC_PAYLOAD_BUDGET_BYTES)\b' apps/conciv/src packages --glob '*.{ts,tsx,js,jsx}' --glob '!**/*.test.*'Repository: conciv-dev/conciv
Length of output: 37444
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- repository instructions ---'
cat ../git/AGENTS.md
cat ../git/CLAUDE.md
cat ../git/.github/skills/code-review/SKILL.md
cat ../git/.claude/skills/conciv-review/SKILL.md
printf '%s\n' '--- chat send path ---'
rg -n -C 18 '\b(sendMessage|create.*Chat|make.*Chat|RPCLink|createORPCClient|chat\.send|chatSend)\b' ../git/apps/conciv/src ../git/packages/client ../git/packages/protocol --glob '*.{ts,tsx,js,jsx}'
printf '%s\n' '--- RPC transport definitions ---'
rg -n -C 20 'rpc-ws|RPCLink|WS_RPC_PAYLOAD_BUDGET_BYTES|WebSocket|socket\.send|JSON\.stringify' ../git/packages ../git/apps/conciv/src --glob '*.{ts,tsx,js,jsx}' --glob '!**/*.test.*'Repository: conciv-dev/conciv
Length of output: 50374
🏁 Script executed:
set -euo pipefail
cd ../git
printf '%s\n' '--- root instructions and review skill ---'
sed -n '1,220p' AGENTS.md
sed -n '1,220p' CLAUDE.md
sed -n '1,220p' .claude/skills/conciv-review/SKILL.md
printf '%s\n' '--- relevant source files ---'
rg -l --glob '!**/*.test.*' --glob '!**/*.stories.*' --glob '!**/node_modules/**' \
'sendMessage|createChat|makeChat|RPCLink|chat\.send|WS_RPC_PAYLOAD_BUDGET_BYTES' \
apps/conciv/src packages/client packages/core packages/contract packages/protocol
printf '%s\n' '--- chat pane and client setup ---'
sed -n '240,285p' apps/conciv/src/pane/chat-pane.tsx
rg -n -C 16 --glob '!**/*.test.*' --glob '!**/*.stories.*' \
'sendMessage|createChat|makeChat|connectionStatus' apps/conciv/src packages/client
printf '%s\n' '--- RPC client and router definitions ---'
rg -n -C 14 --glob '!**/*.test.*' --glob '!**/*.stories.*' \
'RPCLink|createORPCClient|chat:|send:' packages apps/conciv/src \
| grep -v '/node_modules/' | head -500Repository: conciv-dev/conciv
Length of output: 50377
🏁 Script executed:
set -euo pipefail
cd ../git
printf '%s\n' '--- chat connection ---'
sed -n '1,260p' packages/client/src/chat-connection.ts
printf '%s\n' '--- browser transport ---'
sed -n '1,240p' packages/contract/src/browser-transport.ts
sed -n '1,180p' packages/contract/src/client.ts
printf '%s\n' '--- chat session hook and construction ---'
rg -n -l --glob '!**/*.test.*' --glob '!**/*.stories.*' \
'useChatSession|ChatSession|chat-connection' apps/conciv/src packages
rg -n -C 24 --glob '!**/*.test.*' --glob '!**/*.stories.*' \
'useChatSession|createChat|sendMessage' apps/conciv/src packages/client packages/ui-kit-chat
printf '%s\n' '--- core chat input contract ---'
sed -n '1,220p' packages/core/src/api/rpc/chat.ts
rg -n -C 12 'chat:|send:|ChatContentPartSchema|MultimodalContent|UIMessage' packages/contract/src packages/protocol/srcRepository: conciv-dev/conciv
Length of output: 50373
🏁 Script executed:
set -euo pipefail
cd ../git
printf '%s\n' '--- chat contract and content schema ---'
rg -n -C 24 --glob '!**/*.test.*' \
'chat\s*:|send\s*:|ChatContentPartSchema|type ChatContent|content:' \
packages/contract/src packages/protocol/src packages/core/src/api/rpc/chat.ts
printf '%s\n' '--- ORPC websocket serializer source ---'
rg -n -l --glob '*.js' --glob '*.mjs' --glob '*.cjs' \
'class RPCLink|function RPCLink|websocket.*send|JSON.stringify|input.*json' \
node_modules/.pnpm/@orpc+client* node_modules/.pnpm/@orpc+server* 2>/dev/null | head -80
printf '%s\n' '--- relevant ORPC implementation excerpts ---'
rg -n -C 20 --glob '*.js' --glob '*.mjs' --glob '*.cjs' \
'websocket\.send|JSON\.stringify|input.*json|json.*input|message\.' \
node_modules/.pnpm/@orpc+client* node_modules/.pnpm/@orpc+server* 2>/dev/null | head -500
printf '%s\n' '--- direct send input shape ---'
sed -n '1,180p' packages/contract/src/contract.ts
sed -n '1,180p' packages/core/src/api/rpc/chat.tsRepository: conciv-dev/conciv
Length of output: 16976
🏁 Script executed:
set -euo pipefail
cd ../git
printf '%s\n' '--- installed ORPC package paths ---'
find node_modules/.pnpm -maxdepth 1 -type d -iname '`@orpc`+client*' -o -iname '`@orpc`+server*' | sort
printf '%s\n' '--- ORPC websocket files ---'
find node_modules/.pnpm -path '*/node_modules/@orpc/client/*' -type f \
\( -name '*.js' -o -name '*.mjs' -o -name '*.ts' \) -print | sort | head -120
printf '%s\n' '--- ORPC websocket implementation matches ---'
rg -n -C 18 --glob '*.{js,mjs,ts}' \
'websocket\.send|JSON\.stringify|input.*json|json.*input|procedure|path' \
node_modules/.pnpm -g '*`@orpc`*' 2>/dev/null | head -600Repository: conciv-dev/conciv
Length of output: 2599
🏁 Script executed:
set -euo pipefail
cd ../git
ORPC=node_modules/.pnpm/@orpc+client@1.14.7_@opentelemetry+api@1.9.1/node_modules/@orpc/client/dist
printf '%s\n' '--- websocket adapter ---'
cat "$ORPC/adapters/websocket/index.mjs"
printf '%s\n' '--- shared client modules ---'
for file in "$ORPC"/shared/*.mjs; do
printf '\n--- %s ---\n' "$file"
rg -n -C 12 'JSON\.stringify|send\(|input|message|path|t:' "$file" | head -240
done
printf '%s\n' '--- server websocket adapter ---'
find node_modules/.pnpm/@orpc+server@1.14.7* -path '*/node_modules/@orpc/server/dist/adapters/websocket/*' \
-type f -name '*.mjs' -print -exec sed -n '1,260p' {} \;Repository: conciv-dev/conciv
Length of output: 2241
🏁 Script executed:
set -euo pipefail
cd ../git
printf '%s\n' '--- ORPC peer and standard-server files ---'
find node_modules/.pnpm -path '*/node_modules/@orpc/standard-server-peer/dist/*' -o \
-path '*/node_modules/@orpc/standard-server/dist/*' -o \
-path '*/node_modules/@orpc/standard-server-fetch/dist/*' \
-type f -name '*.mjs' -print | sort | head -120
printf '%s\n' '--- request serialization and peer send logic ---'
rg -n -C 24 --glob '*.mjs' \
'class ClientPeer|request\(|JSON\.stringify|serialize|message|input|url|path' \
node_modules/.pnpm/*/node_modules/@orpc/standard-server-peer/dist \
node_modules/.pnpm/*/node_modules/@orpc/standard-server/dist \
node_modules/.pnpm/*/node_modules/@orpc/standard-server-fetch/dist \
node_modules/.pnpm/*/node_modules/@orpc/client/dist/shared 2>/dev/null | head -1000Repository: conciv-dev/conciv
Length of output: 50374
🏁 Script executed:
set -euo pipefail
cd ../git
printf '%s\n' '--- standard-server-peer package ---'
find node_modules/.pnpm -maxdepth 1 -type d -iname '*standard-server-peer*' -print
find node_modules/.pnpm -path '*/node_modules/@orpc/standard-server-peer/*' \
-type f \( -name '*.mjs' -o -name '*.js' \) -print | sort
printf '%s\n' '--- peer request frame implementation ---'
rg -n -C 30 --glob '*.{mjs,js}' \
'class ClientPeer|request\(|JSON\.stringify|message|id|method|path|input|body' \
node_modules/.pnpm/*standard-server-peer*/node_modules/@orpc/standard-server-peer 2>/dev/null | head -1000
printf '%s\n' '--- package dependency links ---'
readlink -f node_modules/.pnpm/@orpc+client@1.14.7_@opentelemetry+api@1.9.1/node_modules/@orpc/standard-server-peer || true
readlink -f node_modules/.pnpm/@orpc+client@1.14.7_@opentelemetry+api@1.9.1/node_modules/@orpc/standard-server || trueRepository: conciv-dev/conciv
Length of output: 445
🏁 Script executed:
set -euo pipefail
cd ../git
PEER=node_modules/.pnpm/@orpc+standard-server-peer@1.14.7_@opentelemetry+api@1.9.1/node_modules/@orpc/standard-server-peer/dist/index.mjs
printf '%s\n' '--- peer implementation ---'
cat "$PEER"
printf '%s\n' '--- peer exports and package metadata ---'
cat node_modules/.pnpm/@orpc+standard-server-peer@1.14.7_@opentelemetry+api@1.9.1/node_modules/@orpc/standard-server-peer/package.jsonRepository: conciv-dev/conciv
Length of output: 23260
🏁 Script executed:
set -euo pipefail
cd ../git
CLIENT=node_modules/.pnpm/@orpc+client@1.14.7_@opentelemetry+api@1.9.1/node_modules/@orpc/client/dist/shared/client.WEeVKeV_.mjs
printf '%s\n' '--- websocket link codec and standard link ---'
sed -n '245,430p' "$CLIENT"
printf '%s\n' '--- stringifyJSON implementation ---'
rg -n -l --glob '*.mjs' 'function stringifyJSON|const stringifyJSON|export.*stringifyJSON' \
node_modules/.pnpm/*/node_modules/@orpc/shared/dist 2>/dev/null | head -20
rg -n -C 16 --glob '*.mjs' 'function stringifyJSON|const stringifyJSON' \
node_modules/.pnpm/*/node_modules/@orpc/shared/dist 2>/dev/null | head -120Repository: conciv-dev/conciv
Length of output: 25648
🏁 Script executed:
set -euo pipefail
cd ../git
node - <<'JS'
const budget = 16 * 1024 * 1024 - 512 * 1024
const sessionId = 'conciv_session'
const runId = 'run-123'
const id = 1
function bytes(value) {
return new TextEncoder().encode(JSON.stringify(value)).length
}
const content = 'x'.repeat(budget - 3)
const input = {sessionId, runId, content}
const frame = {
i: id,
p: {
u: '/chat/send',
b: {json: input},
},
}
const guardedBytes = bytes(content)
const wireBytes = bytes(frame)
console.log(JSON.stringify({
budget,
guardedBytes,
wireBytes,
omittedBytes: wireBytes - guardedBytes,
guardPasses: guardedBytes <= budget,
wireFits: wireBytes <= budget,
wireFrame: JSON.stringify(frame).slice(0, 180),
}, null, 2))
if (!(guardedBytes <= budget && wireBytes > budget)) {
throw new Error('The reproduced ORPC frame did not demonstrate the boundary mismatch')
}
JSRepository: conciv-dev/conciv
Length of output: 522
Measure the complete ORPC WebSocket frame.
chatConnection sends {sessionId, runId, content}, but checkSend measures only JSON.stringify(content). A message can pass while the ORPC envelope exceeds WS_RPC_PAYLOAD_BUDGET_BYTES. Reuse the outbound serializer or include the envelope in the size check.
🤖 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 `@apps/conciv/src/pane/send-checks.ts` around lines 22 - 27, The checkSend
payload-size validation currently measures only content; update it to serialize
and measure the complete outbound ORPC frame containing sessionId, runId, and
content, reusing the serializer used by chatConnection where available. Keep the
existing rejection response and WS_RPC_PAYLOAD_BUDGET_BYTES threshold unchanged.
Any >16MB frame 1009-closes the shared rpc socket. The image adapter capped at 20MB raw, which base64-inflates past the limit, so an 11.6-20MB image poisoned the whole tab. Wire limit now lives in @conciv/protocol/rpc-types as the single source; serve, the adapter cap, the server-side base64 schema cap, and a new aggregate send-time guard all derive from it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…churn (#347) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6881562 to
197419f
Compare
Closes #347.
Problem
@conciv/servecloses the ws at 16MiB (maxPayload, close code 1009). Since #349 every rpc procedure of a tab rides one shared/rpc-wssocket, so a single oversized frame kills the whole tab's rpc.The composer image adapter capped raw files at 20MB — but base64 inflates ×4/3, so a 20MB image becomes a ~28MB frame. Any image between ~11.6MB and 20MB passed the adapter check and 1009-poisoned the socket. The server-side zod cap (
MAX_ATTACHMENT_BASE64_LENGTH = 27_962_028) agreed with the 20MB adapter cap, not with the wire limit — and never runs anyway, becausewskills the frame before orpc parses it.Bonus edge:
chat.sendallows 16 content parts; 16 × 1MB text parts alone exactly reach the frame limit before the JSON envelope, so per-attachment caps alone can't prevent the poison.Fix — one source, everything derives
@conciv/protocol/rpc-types:WS_MAX_PAYLOAD_BYTES(16MiB),WS_PAYLOAD_MARGIN_BYTES(512KiB),WS_RPC_PAYLOAD_BUDGET_BYTES. (The constant moved to protocol rather than being imported from@conciv/serveas the issue sketched, because serve importsws/node:eventsat module top and browser bundles can't take that.)@conciv/serveimports the limit from protocol (new workspace dep) and no longer exportsDEFAULT_MAX_PAYLOAD_BYTES.@conciv/protocol/chat-types:MAX_ATTACHMENT_RAW_BYTES= MiB-floored ¾ of budget = 11MiB; the base64 schema cap derives from it.checkSendgains an aggregate guard: serialized content over budget → composer warning ("Message too large to send…"), nothing hits the socket. Covers multi-part sums the per-attachment cap can't.WS_RPC_PAYLOAD_BUDGET_BYTESdirectly (its local margin constant was a duplicate).Audit of other large-input procedures
elements.upserttakes an unboundedrowsarray — same bug class, needs recorder-flusher-style chunking. Left for a follow-up PR: the whiteboard suite is CI-only, so a chunking change can't be behavior-verified in this one.Testing
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes