fix: improve reasoning, streaming, and agent generation behavior - #4271
Merged
Conversation
📝 WalkthroughWalkthroughThe PR unifies streaming typewriter pacing, adds grapheme-safe reveals, filters regenerated messages from summaries and memories, normalizes retired built-in tools, improves OpenAI reasoning-summary streaming, updates a notification effect dependency, and adds regression scripts. ChangesUnified streaming typewriter
Regeneration context filtering
Built-in agent tool normalization
OpenAI reasoning-summary streaming
Supporting updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GenerateRoutes
participant SummaryResolver
participant MemoryRecall
Client->>GenerateRoutes: regenerate assistant message
GenerateRoutes->>SummaryResolver: exclude regenerated message ID
GenerateRoutes->>MemoryRecall: apply regenerateContextCutoff
MemoryRecall-->>GenerateRoutes: filtered memory context
GenerateRoutes-->>Client: generation prompt without regenerated content
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
3 tasks
SpicyMarinara
marked this pull request as ready for review
July 29, 2026 19:42
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/client/src/hooks/use-generate.ts`:
- Around line 1491-1492: The typewriter frame calculation currently caps output
to a fixed per-frame allowance, reducing the configured rate at lower frame
rates. Update the logic around typewriterRemainder and the derived n to
calculate the frame cap from elapsedMs while preserving maxCharsThisFrame and
pendingText limits, then add a regression test simulating 30 FPS to verify the
configured characters-per-second rate remains stable.
In `@packages/server/src/routes/generate/generate-route-utils.ts`:
- Around line 640-645: Update the summary filtering flow around
normalizeChatSummaryEntries so a synthesized legacy entry from summary is
discarded when summaryEntries is absent or invalid during regeneration, rather
than retained without message-ID provenance. Preserve valid explicitly provided
entries and compile only the entries whose covered IDs are not excluded. Add a
regression case covering input with summary only and no summaryEntries.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c48037b-0903-4ccf-b359-4f41c1d75b96
📒 Files selected for processing (15)
package.jsonpackages/client/src/components/noodle/NoodleHome.tsxpackages/client/src/hooks/use-generate.tspackages/client/src/lib/generation-stream-policy.tspackages/server/src/routes/generate.routes.tspackages/server/src/routes/generate/generate-route-utils.tspackages/server/src/routes/generate/retry-agents-route.tspackages/server/src/services/generation/memory-recall-context.tspackages/server/src/services/llm/providers/openai.provider.tspackages/server/src/services/memory-recall.tspackages/shared/src/types/agent.tsscripts/regressions/expression-tool-retirement.regression.tsscripts/regressions/provider-compat.regression.tsscripts/regressions/regeneration-context.regression.tsscripts/regressions/roleplay-streaming.regression.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/routes/generate/generate-route-utils.ts (1)
640-648: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude unprovenanced entries during regeneration.
normalizeChatSummaryEntriespreserves explicit entries that have neithermessageIdsnorhiddenMessageIds. Those entries produce an emptycoveredMessageIdsarray and are retained, so their content can still contain the discarded response. During regeneration, conservatively drop entries without provenance and add a regression case covering a mixed set of provenanced and unprovenanced entries.Proposed fix
const retainedEntries = entries.filter((entry) => { const coveredMessageIds = [...(entry.messageIds ?? []), ...(entry.hiddenMessageIds ?? [])]; - return !coveredMessageIds.some((messageId) => excludedMessageIds.has(messageId)); + return ( + coveredMessageIds.length > 0 && + !coveredMessageIds.some((messageId) => excludedMessageIds.has(messageId)) + ); });🤖 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/server/src/routes/generate/generate-route-utils.ts` around lines 640 - 648, Update the retainedEntries filtering in the regeneration logic to exclude any summary entry lacking both messageIds and hiddenMessageIds, while preserving the existing excludedMessageIds checks for provenanced entries. Add a regression case covering mixed provenanced and unprovenanced entries and verify the unprovenanced entry is discarded.
🤖 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.
Outside diff comments:
In `@packages/server/src/routes/generate/generate-route-utils.ts`:
- Around line 640-648: Update the retainedEntries filtering in the regeneration
logic to exclude any summary entry lacking both messageIds and hiddenMessageIds,
while preserving the existing excludedMessageIds checks for provenanced entries.
Add a regression case covering mixed provenanced and unprovenanced entries and
verify the unprovenanced entry is discarded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb2e01d9-c29f-4ccb-b251-7dbfef13f159
📒 Files selected for processing (5)
packages/client/src/hooks/use-generate.tspackages/client/src/lib/generation-stream-policy.tspackages/server/src/routes/generate/generate-route-utils.tsscripts/regressions/regeneration-context.regression.tsscripts/regressions/roleplay-streaming.regression.ts
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Several generation paths were producing degraded or misleading behavior:
NoodleHomeretained a stale hook dependency warning.What changed
set_expressionfor Expression Engine, including compatibility normalization for older installed package manifests and saved settings, so it can batch with compatible JSON tracker agents.localizeUihook dependency and refresh a stale Roleplay source assertion.The downloadable Expression Engine
1.0.2package/catalog update is in Pasta-Devs/Marinara-Agents#127.User impact
Reasoning summaries become available when the provider emits them, streamed text reveals smoothly at the configured rate, and Expression Engine no longer pays for a redundant tool round-trip. Regenerating a message no longer supplies its discarded response to the model as settled summary or memory context.
When one structured summary entry covers many messages including the regenerated one, that entire opaque entry is omitted for the regeneration request. This conservatively loses some summary context for one request rather than leaking the response being replaced; stored summary data is not mutated.
Validation
pnpm checkpnpm regression:providerspnpm regression:expression-toolspnpm regression:regeneration-contextpnpm regression:roleplaygit diff --checkManual verification
Closes #4270
Summary by CodeRabbit