Address staging issue sweep through #4341 - #4340
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds character-ID prompt context, summary combination, embedded lorebook visibility controls, custom image-agent settings, Recent chat sorting, shared development build ordering, shell-free Windows process execution, and corrected reasoning handling for OpenAI-compatible providers. ChangesApplication feature updates
Compatibility and development tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 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 review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
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.routes.ts (1)
8259-8283: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame fallback-agent misattribution bug in both routes. Both
illustratorAgent(generate.routes.ts) andimagePromptAgent(retry-agents-route.ts) fall back to the built-in Illustrator agent's resolved config even when the currentimage_promptresult did not come from the Illustrator and the actual agent could not be found by ID. This can leak the Illustrator'simageConnectionId,useAvatarReferences,includeCharacterAppearance, and (in generate.routes.ts) prompt settings onto a different agent's image request.
packages/server/src/routes/generate.routes.ts#L8259-L8283: changeillustratorAgent = resultAgent ?? fallbackIllustratorAgentto fall back to the Illustrator agent only whenresult.agentType === "illustrator", mirroring the correctillustratorBackgroundAgentternary a few lines below.packages/server/src/routes/generate/retry-agents-route.ts#L3194-L3199: apply the identical fix toimagePromptAgent.🤖 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.routes.ts` around lines 8259 - 8283, Prevent fallback Illustrator configuration from being applied to unrelated image-prompt results. In packages/server/src/routes/generate.routes.ts lines 8259-8283, update illustratorAgent to use fallbackIllustratorAgent only when result.agentType === "illustrator"; in packages/server/src/routes/generate/retry-agents-route.ts lines 3194-3199, apply the same condition to imagePromptAgent. Preserve direct resultAgent resolution and leave illustratorBackgroundAgent unchanged.
🤖 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 `@docs/prompts/macros.md`:
- Line 41: Replace the literal-looking {{CHARACTER_ID}} syntax with one clearly
marked placeholder form in docs/prompts/macros.md lines 41-41 and
packages/shared/src/utils/macro-engine.ts lines 312-316, such as
{{<21-character-card-ID>}}; update the registry syntax field to match the
documentation, and verify any client-side macro picker using this registry
inserts a real 21-character ID rather than the placeholder text.
In `@packages/server/src/routes/chats.routes.ts`:
- Around line 3927-3971: Before the provider.chatComplete call in the
selectedEntries flow, sum each selected entry’s tokenEstimate and reject the
request with a client error when the total exceeds a defined combined-summary
budget. Reuse the existing summary size/context limit symbols where applicable,
and ensure the guard runs before constructing or sending the oversized prompt
while preserving the current valid-request behavior.
In `@packages/server/src/services/prompt/assembler.ts`:
- Around line 358-372: Update the referenced-character scanning call to include
only enabled sections whose groups are enabled, reusing the existing
sectionMap/groupMap checks from hasDialogueExamplesMarker before collecting
section content. Extend sources to also include input.chatSummary,
input.personaDescription, and input.personaFields so character-ID macros in
those values are detected and resolved.
In `@packages/server/src/services/prompt/macro-context.ts`:
- Around line 74-92: Export the character-reference regex from the shared
macro-engine module and remove the local CHARACTER_REFERENCE_ID_PATTERN
definition in extractCharacterReferenceIds. Import and reuse the shared pattern
in macro-context.ts, preserving the existing matchAll extraction behavior and
limits.
- Around line 211-227: Parallelize the character lookups in the active-character
and referenced-character processing around the two loops by mapping IDs to
asynchronous getById operations and awaiting them with Promise.all. Preserve the
existing parsing, source accumulation, active-ID filtering, and referenced
result shape while eliminating sequential database round trips.
---
Outside diff comments:
In `@packages/server/src/routes/generate.routes.ts`:
- Around line 8259-8283: Prevent fallback Illustrator configuration from being
applied to unrelated image-prompt results. In
packages/server/src/routes/generate.routes.ts lines 8259-8283, update
illustratorAgent to use fallbackIllustratorAgent only when result.agentType ===
"illustrator"; in packages/server/src/routes/generate/retry-agents-route.ts
lines 3194-3199, apply the same condition to imagePromptAgent. Preserve direct
resultAgent resolution and leave illustratorBackgroundAgent unchanged.
🪄 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: 243c5ed0-c6c0-4965-bfab-8b0c68710ff9
📒 Files selected for processing (34)
CHANGELOG.mdCONTRIBUTING.mddocs/prompts/macros.mdpackage.jsonpackages/client/scripts/build.mjspackages/client/src/components/agents/AgentEditor.tsxpackages/client/src/components/characters/CharacterEditor.tsxpackages/client/src/components/chat/SummaryPopover.tsxpackages/client/src/components/layout/ChatSidebar.tsxpackages/client/src/components/lorebooks/LorebookAssignmentSection.tsxpackages/client/src/components/ui/GenerationParametersEditor.tsxpackages/client/src/hooks/use-chats.tspackages/client/src/hooks/use-lorebooks.tspackages/client/src/lib/chat-recency.tspackages/client/src/localization/locales/en.jsonpackages/server/src/config/build-info.tspackages/server/src/db/schema/lorebooks.tspackages/server/src/routes/chats.routes.tspackages/server/src/routes/generate.routes.tspackages/server/src/routes/generate/retry-agents-route.tspackages/server/src/routes/updates.routes.tspackages/server/src/services/llm/providers/openai.provider.tspackages/server/src/services/mari-db/mari-db.service.tspackages/server/src/services/prompt/assembler.tspackages/server/src/services/prompt/macro-context.tspackages/server/src/services/storage/lorebooks.storage.tspackages/shared/src/schemas/lorebook.schema.tspackages/shared/src/types/lorebook.tspackages/shared/src/utils/macro-engine.tsscripts/check-tracked-installers.mjsscripts/ensure-native-deps.mjsscripts/regressions/open-issues.regression.tsscripts/regressions/prompt.regression.tsscripts/regressions/provider-compat.regression.ts
💤 Files with no reviewable changes (2)
- scripts/check-tracked-installers.mjs
- packages/server/src/config/build-info.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/server/src/routes/chats.routes.ts (2)
3963-3987: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd debug prompt logging for the summary-combination request.
This new model-generation path builds provider messages but does not call
logDebugOverride(...)or an equivalent debug callback. HonordebugModeandDEBUG_AGENTSso debug mode shows the final provider prompt.As per coding guidelines, “New agent, model-generation, image-generation, and prompt-building paths must wire prompt logging, honor
debugModeandDEBUG_AGENTS, and uselogDebugOverride(...)or an equivalent debug callback so the final provider prompt is visible in debug mode.”🤖 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/chats.routes.ts` around lines 3963 - 3987, Add prompt-debug logging to the summary-combination request surrounding provider.chatComplete, using the existing logDebugOverride-equivalent callback and honoring both debugMode and DEBUG_AGENTS. Log the finalized system and user messages sent to the provider without changing the summary generation behavior.Source: Coding guidelines
3969-4003: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject concurrent summary-entry changes before replacement.
The provider receives
sourceTextfrom the original entries. The metadata callback only verifies that the selected IDs still exist.If a user edits an entry during generation, this code removes the edited content and replaces it with a summary of stale content. If a user deletes an entry, Line 4002 throws and the route can return HTTP 500.
Capture a fingerprint of each selected entry before generation. In the metadata callback, compare the fresh entries with that fingerprint. Return HTTP 409 when they differ.
🤖 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/chats.routes.ts` around lines 3969 - 4003, Capture fingerprints of the selected entries before calling provider.chatComplete, including the content used to build sourceText. In the patchMetadata callback, compare each freshly selected entry against those fingerprints and reject any added, removed, reordered, or edited entry as a conflict. Handle that conflict at the route level by returning HTTP 409 instead of allowing the callback error to become HTTP 500, while preserving the existing replacement flow for unchanged entries.packages/server/src/services/prompt/macro-context.ts (1)
75-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winApply the eight-character limit after excluding active IDs.
extractCharacterReferenceIdsstops after eight unique IDs. Line 221 removes active IDs only after that limit is applied. If the first eight IDs are active characters, later external references are discarded even though they are the only IDs that this function loads.Make the extractor exclude active IDs before counting the eight-character limit, or apply the limit after that exclusion.
Also applies to: 221-225
🤖 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/services/prompt/macro-context.ts` around lines 75 - 92, Update extractCharacterReferenceIds and its active-ID filtering flow so active character IDs are excluded before enforcing MAX_REFERENCED_CHARACTERS. Ensure later non-active references are still collected when earlier matches are active, while preserving uniqueness and the eight-ID cap for returned external references.
🤖 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/server/src/routes/chats.routes.ts`:
- Around line 3939-3955: The combined-summary budget uses
effectiveSummaryMaxTokens, but the provider request uses the larger
summaryMaxTokens. Update the provider.chatComplete call in the selected-summary
combination flow to pass effectiveSummaryMaxTokens, preserving the existing
budget calculation and validation.
In `@packages/server/src/routes/generate.routes.ts`:
- Around line 8273-8278: Rename illustratorAgent to imagePromptAgent throughout
this handler block, including its declaration and all downstream references,
while preserving the existing fallback selection and usesChatIllustratorSettings
behavior. Match the equivalent naming used in retry-agents-route.ts.
In `@packages/server/src/services/prompt/assembler.ts`:
- Around line 358-366: Update the enabledSectionContents collection to skip
sections under the same impersonation condition used by the render loop: when
input.impersonate is true and input.preserveImpersonatePresetSections is not
true, exclude non-marker sections from reference scanning while retaining marker
sections and existing enabled/group checks.
In `@packages/shared/src/utils/macro-engine.ts`:
- Around line 1769-1771: Update the character-reference replacement flow and the
later generic variable pass in the macro engine so unresolved 21-character
character IDs remain unchanged even when a variable has the same name. Track
unresolved IDs from the CHARACTER_REFERENCE_ID_PATTERN replacement and exclude
exact matches from generic variable substitution, including the corresponding
logic near the later replacement block.
In `@scripts/regressions/open-issues.regression.ts`:
- Around line 2578-2584: Replace the source-text assertions in
scripts/regressions/open-issues.regression.ts:2578-2584 with behavioral
regression tests that execute image generation and prompt-review retry using a
custom producing agent, then verify the actual calls use that agent. Also update
scripts/regressions/open-issues.regression.ts:3482-3485 to submit an oversized
summary combination, assert it is rejected, and verify provider.chatComplete is
not called.
---
Outside diff comments:
In `@packages/server/src/routes/chats.routes.ts`:
- Around line 3963-3987: Add prompt-debug logging to the summary-combination
request surrounding provider.chatComplete, using the existing
logDebugOverride-equivalent callback and honoring both debugMode and
DEBUG_AGENTS. Log the finalized system and user messages sent to the provider
without changing the summary generation behavior.
- Around line 3969-4003: Capture fingerprints of the selected entries before
calling provider.chatComplete, including the content used to build sourceText.
In the patchMetadata callback, compare each freshly selected entry against those
fingerprints and reject any added, removed, reordered, or edited entry as a
conflict. Handle that conflict at the route level by returning HTTP 409 instead
of allowing the callback error to become HTTP 500, while preserving the existing
replacement flow for unchanged entries.
In `@packages/server/src/services/prompt/macro-context.ts`:
- Around line 75-92: Update extractCharacterReferenceIds and its active-ID
filtering flow so active character IDs are excluded before enforcing
MAX_REFERENCED_CHARACTERS. Ensure later non-active references are still
collected when earlier matches are active, while preserving uniqueness and the
eight-ID cap for returned external references.
🪄 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: 702c0440-e0eb-4220-889b-44b391fa2eca
📒 Files selected for processing (8)
docs/prompts/macros.mdpackages/server/src/routes/chats.routes.tspackages/server/src/routes/generate.routes.tspackages/server/src/routes/generate/retry-agents-route.tspackages/server/src/services/prompt/assembler.tspackages/server/src/services/prompt/macro-context.tspackages/shared/src/utils/macro-engine.tsscripts/regressions/open-issues.regression.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/server/src/routes/chats.routes.ts (4)
3972-3987: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winWire this model-generation path into debug prompt logging.
This new
provider.chatCompletepath does not handledebugModeorDEBUG_AGENTS, and it does not calllogDebugOverride(...)or an equivalent callback. Add the shared prompt logging hook so the final system and user messages are visible in debug mode.As per coding guidelines, new agent, model-generation, image-generation, and prompt-building paths must wire prompt logging, honor
debugModeandDEBUG_AGENTS, and uselogDebugOverride(...)or an equivalent debug callback so the final provider prompt is visible in debug mode.🤖 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/chats.routes.ts` around lines 3972 - 3987, Update the model-generation flow containing provider.chatComplete to wire in the shared prompt-debug logging hook, including logDebugOverride(...) or the established equivalent callback. Ensure the final system and user messages are logged when debugMode or DEBUG_AGENTS is enabled, while preserving the existing prompt content and completion behavior.Source: Coding guidelines
4005-4006: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve hidden-message ownership for legacy entries.
This code collects only
entry.hiddenMessageIds. Older entries can omit that field. The delete path at Lines 1174-1176 explicitly falls back tomessageIds. When such entries are combined, the replacement can lose ownership of messages that were hidden by the original entries. Use the same fallback when buildinghiddenMessageIds.Also applies to: 4025-4026
🤖 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/chats.routes.ts` around lines 4005 - 4006, Update the hiddenMessageIds aggregation in the selected-entry handling, including the corresponding logic around the second occurrence, to fall back to each entry’s messageIds when hiddenMessageIds is absent, matching the delete path’s legacy-entry behavior while preserving deduplication.
4000-4003: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject stale combination results before replacing entries.
The provider combines entries captured before
provider.chatComplete. The final callback checks only that the selected IDs still exist. If a selected entry changes while generation runs, the stalesummaryTextreplaces newer content. Compare selected entry versions, timestamps, or content before committing, and return a conflict without writing the stale result.🤖 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/chats.routes.ts` around lines 4000 - 4003, Update the final callback in the provider combination flow to validate that each selected entry is unchanged from the entries captured before provider.chatComplete, not merely that its ID still exists. Compare the available version, timestamp, or content fields, and return the existing conflict outcome without replacing entries when any selected entry has changed; only commit summaryText when all selected entries still match.
3939-3955: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAccount for the complete prompt in the input budget.
The new entry-content estimate does not include the resolved summary prompt, entry titles, or combination instructions. The fixed 1,024-token reserve can be exceeded by a custom prompt from
CHAT_SUMMARY_PROMPT_SETTINGS_KEY. The provider request can then exceed its context limit after validation succeeds.Resolve the prompt before the budget check and estimate the complete system and user prompt.
Also applies to: 3963-3971
🤖 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/chats.routes.ts` around lines 3939 - 3955, Update the combined-summary budget validation around effectiveSummaryMaxTokens and combinedTokenEstimate to resolve the configured summary prompt before checking the limit, then estimate the complete system and user prompt including the prompt, entry titles, contents, and combination instructions. Use that full estimate against the provider context budget instead of relying on the fixed SUMMARY_COMBINE_PROMPT_RESERVE_TOKENS, while preserving the existing 400 response when the complete request exceeds the budget.packages/server/src/services/prompt/assembler.ts (1)
370-393: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
groupScenarioOverrideTextin referenced-character scanning.
buildPromptMacroContext()addsgroupScenarioOverrideTexttocharacterFields.scenario, butbuildReferencedCharacterContext()only pulls character data fromreferencedCharacterSourceFields(data), which usesdata.scenario. A character-ID macro in the group scenario override is not found during extraction unless the override is added tosources.🤖 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/services/prompt/assembler.ts` around lines 370 - 393, Update the sources passed to buildReferencedCharacterContext in buildPromptMacroContext to include input.groupScenarioOverrideText, ensuring character-ID macros in the group scenario override are scanned alongside the existing prompt content.
🤖 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/server/src/routes/generate.routes.ts`:
- Around line 8717-8720: Update both illustration retry failure payloads near
the image generation error handling to use result.agentType instead of the
hardcoded "illustrator", preserving the originating custom image_prompt agent
for retryAgents(). Also update the no-connection payload’s agent-specific
metadata to reflect the custom agent when applicable, while keeping the
illustration retry target unchanged.
In `@scripts/regressions/prompt.regression.ts`:
- Around line 2053-2061: Update the regression test’s unknownId value used in
the collision assertions to a 21-character identifier containing only letters,
digits, and underscores, while preserving the existing
unresolved-character-reference expectations.
---
Outside diff comments:
In `@packages/server/src/routes/chats.routes.ts`:
- Around line 3972-3987: Update the model-generation flow containing
provider.chatComplete to wire in the shared prompt-debug logging hook, including
logDebugOverride(...) or the established equivalent callback. Ensure the final
system and user messages are logged when debugMode or DEBUG_AGENTS is enabled,
while preserving the existing prompt content and completion behavior.
- Around line 4005-4006: Update the hiddenMessageIds aggregation in the
selected-entry handling, including the corresponding logic around the second
occurrence, to fall back to each entry’s messageIds when hiddenMessageIds is
absent, matching the delete path’s legacy-entry behavior while preserving
deduplication.
- Around line 4000-4003: Update the final callback in the provider combination
flow to validate that each selected entry is unchanged from the entries captured
before provider.chatComplete, not merely that its ID still exists. Compare the
available version, timestamp, or content fields, and return the existing
conflict outcome without replacing entries when any selected entry has changed;
only commit summaryText when all selected entries still match.
- Around line 3939-3955: Update the combined-summary budget validation around
effectiveSummaryMaxTokens and combinedTokenEstimate to resolve the configured
summary prompt before checking the limit, then estimate the complete system and
user prompt including the prompt, entry titles, contents, and combination
instructions. Use that full estimate against the provider context budget instead
of relying on the fixed SUMMARY_COMBINE_PROMPT_RESERVE_TOKENS, while preserving
the existing 400 response when the complete request exceeds the budget.
In `@packages/server/src/services/prompt/assembler.ts`:
- Around line 370-393: Update the sources passed to
buildReferencedCharacterContext in buildPromptMacroContext to include
input.groupScenarioOverrideText, ensuring character-ID macros in the group
scenario override are scanned alongside the existing prompt content.
🪄 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: d968d2c6-7c59-4432-8bc2-35fe4d923ee5
📒 Files selected for processing (5)
packages/server/src/routes/chats.routes.tspackages/server/src/routes/generate.routes.tspackages/server/src/services/prompt/assembler.tspackages/shared/src/utils/macro-engine.tsscripts/regressions/prompt.regression.ts
Important
Contributions target
staging. OnlySpicyMarinaramay promotestagingtomain.Linked issues
Closes #4327
Closes #4332
Closes #4333
Closes #4334
Closes #4335
Closes #4336
Closes #4337
Closes #4341
Follow-up documentation: #4342
Follow-up browser-suite isolation: #4343
Why this change
The open-issue sweep exposed several small but user-visible gaps across development startup, Windows subprocesses, lorebooks, summaries, provider reasoning, character macros, image agents, and chat ordering. This patch keeps each fix on the existing Engine path and adds focused regression coverage instead of parallel implementations.
What changed
Automated verification completed locally
pnpm checkpnpm regression:promptpnpm regression:providerspnpm regression:issuesgit diff --checkThe full
pnpm smoke:uirun exposed pre-existing shared appearance/background fixture failures: 153 passed, 88 skipped, and 13 failed. A serial failed-only retry still reproduced 12 failures, showing stale live-state assertions in addition to parallel sharing. This is isolated in #4343 and deliberately excluded from this product patch.Validation
pnpm checkpasses locallyCONTRIBUTING.mdManual verification needed
Docs and release impact
docs-i18nbranch updated to match, or a[docs-i18n]follow-up issue openedTranslated macro documentation is tracked in #4342.
Summary by CodeRabbit