Compact oversized chat context for provider windows - #1280
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 selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe chat request pipeline caches attachment uploads, detects matching context-route 503 responses, compacts conversations to 14,000 UTF-8 bytes, and retries once. Tests cover turn retention, multibyte text, streaming, and attachment token reuse. ChangesContext compaction and attachment reuse
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR adds a targeted context-compaction retry while preserving the full local conversation and leaving unrelated failures unchanged; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant MeshConnection
participant InputBuilder
participant ContextRoute
participant StreamConsumer
MeshConnection->>InputBuilder: Build request with shared attachment cache
InputBuilder-->>MeshConnection: Return Responses input
MeshConnection->>ContextRoute: Send conversation request
ContextRoute-->>MeshConnection: Return 503 context-route rejection
MeshConnection->>MeshConnection: Build compacted request
MeshConnection->>ContextRoute: Retry with compacted request
ContextRoute-->>StreamConsumer: Stream response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![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 `@crates/mesh-llm-ui/src/features/chat/api/mesh-connection.ts`:
- Around line 111-112: Update the retry-body size check in the
buildResponsesInput flow to measure the UTF-8 byte length of the serialized
candidate, not JSON.stringify(candidate).length, before accepting it. Add a
regression test using multibyte text and assert the compacted serialized body is
at most CONTEXT_COMPACTION_RETRY_BODY_BYTES (14,000 bytes).
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c737881-9972-4ecb-bbea-adabece54ce5
📒 Files selected for processing (2)
crates/mesh-llm-ui/src/features/chat/api/mesh-connection.test.tscrates/mesh-llm-ui/src/features/chat/api/mesh-connection.ts
Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
ndizazzo
left a comment
ndizazzo
left a comment
There was a problem hiding this comment.
Thanks for keeping this recovery path narrowly scoped. The error matching and UTF-8 measurement are heading in the right direction, but I found three cases that need addressing before this is safe to merge: the new multibyte test currently fails, candidate construction can repeat attachment uploads, and compaction can split a user/assistant turn. I reproduced the focused test failure locally (1 failed, 14 passed); typecheck and the production build both pass.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@ndizazzo this is ready |
ndizazzo
left a comment
ndizazzo
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest commits (6dd1e01, 0cff72b). All three change requests from my prior review are addressed:
- Turn-boundary split:
compactResponsesInputnow only starts a candidate at arole: 'user'message, so a retry can no longer detach an assistant reply from the user turn that produced it. - Repeated attachment uploads:
buildResponsesInputnow threads a sharedAttachmentUploadCachethrough all compaction probes and the final selected candidate, keyed by mime type + filename + base64 value, with cache eviction on upload failure. A new test asserts/api/objectsis called exactly once across the probe-and-retry flow. - Failing multibyte fixture: scaled down so one retained turn fits the 14 KB budget while the full history still exceeds it, still exercising UTF-8 vs UTF-16 measurement.
Verified locally on the PR head (0cff72b):
pnpm exec vitest run src/features/chat/api/mesh-connection.test.ts→ 19/19 passing (was 1 failing/14 passing)pnpm exec tsc -b --pretty false→ cleanpnpm exec prettier --checkon the touched files → clean
Resolved the three addressed threads. No further issues found — approving.
|
This pull request has not been updated in at least 5 days. It will be closed after 7 days of inactivity to keep the active review queue current. Please update it within 2 days if the changes are still moving forward. |
2 participants
Summary
Apple system models expose a 4096-token context window. The console can send a persisted conversation plus its system prompt that exceeds that window, causing the router to reject the request before it reaches the Apple sidecar with:
no context-compatible target for model 'apple/system' can fit approximately ... tokensThis PR adds one targeted recovery path:
Follow-up to #1277.
Validation
pnpm exec prettier --check src/features/chat/api/mesh-connection.ts src/features/chat/api/mesh-connection.test.tspnpm exec vitest run src/features/chat/api/mesh-connection.test.ts(14 passed)pnpm exec tsc -b --pretty falsegit diff --checkNotes
This is intentionally a separate PR from the model-route retry change. It addresses the 503 context-admission failure observed with
apple/systemand applies to any provider reporting the same router error.Summary by CodeRabbit