Repository navigation
feat(ltm): integrate Long-Term Memory package host - #4133
SpicyMarinara merged 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughLong-Term Memory support is added across capability contracts, package runtime services, prompt generation, client settings, backups, documentation, and regression tests. Capability API compatibility advances from minor version 3 to 6. ChangesLong-Term Memory host integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
|
…memory-release # Conflicts: # packages/server/src/routes/generate.routes.ts
|
@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: 4
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)
5748-5780: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRecording "acceptance" before the experiment has even concluded!
In the tool-call loop (Line 5466),
recordAcceptedLongTermMemoryPromptfires only after the fullchatCompletecall resolves. But in this non-tool streaming path, it fires at Line 5750 — right after the very firstgen.next()— before thewhile (!result.done)loop has drained the rest of the stream. Should a latergen.next()throw (mid-stream provider failure, dropped connection, etc.), thecatchblock below simply returnsnull, yet the Long-Term Memory package has already been told the prompt was "accepted." A minor inconsistency that quietly corrupts the very accounting this feature exists to provide.🔬 Proposed fix — record only after the specimen survives in full
try { let result = await withLlmRequestTimeout(chatGenerationTimeoutMs, () => gen.next()); - await recordAcceptedLongTermMemoryPrompt(initialProviderMessages); while (!result.done) { if (abortController.signal.aborted) { return null; } fullResponse += result.value; // ... result = await withLlmRequestTimeout(chatGenerationTimeoutMs, () => gen.next()); } + await recordAcceptedLongTermMemoryPrompt(initialProviderMessages); // Generator return value contains usage if (result.value) {🤖 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 5748 - 5780, Move the recordAcceptedLongTermMemoryPrompt call out of the initial gen.next() setup and execute it only after the while (!result.done) loop completes successfully, preserving the existing abort and error-return behavior so failed or interrupted streams are never recorded as accepted.
🧹 Nitpick comments (3)
packages/server/src/routes/generate.routes.ts (1)
4596-4615: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA redundant clone of my own creation.
Curious — you have already forged the elegant
placeRuntimeAgentInjectionhelper (Lines 4182-4188) and put it to good use forknowledge-retrievalandknowledge-router(Lines 4456-4461, 4472-4473), yet the Long-Term Memory recall here reimplements the identical token-replace-or-append-or-drop dance by hand. Logically equivalent today, yes — but two specimens of the same organism drift apart over time, and the next mutation toplaceRuntimeAgentInjectionwill silently fail to propagate here.♻️ Proposed consolidation
if (recall) { - const tokens = runtimeAgentSectionTokens.get("long-term-memory"); - const handledByPresetSection = - tokens !== undefined && replaceRuntimeAgentSection(finalMessages, tokens, recall.text); - if (handledByPresetSection || !presetOwnsAgentPlacement) { - if (!handledByPresetSection) appendSeparateAgentInjection("long-term-memory", recall.text); - contextInjections.push({ agentType: "long-term-memory", text: recall.text }); + const injection = { agentType: "long-term-memory", text: recall.text }; + if (placeRuntimeAgentInjection(injection)) { + contextInjections.push(injection); longTermMemoryRecallReceipt = recall.receipt; } }🤖 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 4596 - 4615, Replace the manual runtime-section placement logic in the long-term-memory recall block with the existing placeRuntimeAgentInjection helper, passing the recall text and relevant agent type. Preserve the current behavior of replacing preset-owned sections, appending when placement is not preset-owned, and skipping injection when placement is unavailable; continue recording contextInjections and longTermMemoryRecallReceipt only when placement succeeds.scripts/regressions/capability-package-lifecycle.regression.ts (1)
198-241: 📐 Maintainability & Code Quality | 🔵 TrivialGood coverage of the logical contract — but the mock can't see Fastify's boot-order guard.
routeAppis a bare{ hasRoute, route }stub, so these assertions correctly validate prefix/duplicate-detection logic, but they can't catch the runtime hazard raised atcapability-route-registration.service.ts(Lines 56-134): a real Fastify instance rejectsroute()calls once it has started listening. Worth an integration-level regression (or at least a code comment) once that concern is resolved, so the two don't silently diverge again.🤖 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 `@scripts/regressions/capability-package-lifecycle.regression.ts` around lines 198 - 241, Add integration-level regression coverage for registerCapabilityPrivilegedRoutes using a real Fastify instance, exercising route registration after the server has started listening and verifying the expected boot-order behavior. Keep the existing routeApp unit-style assertions for prefix and duplicate-route validation, and align the test with the implementation in registerCapabilityPrivilegedRoutes.packages/server/src/services/capability-packages/capability-resources.service.ts (1)
47-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAn N+1 experiment I did not authorize, Doctor.
listLorebooksissues onelistEntries(record.id)call per lorebook. Your ownlistEligibleLorebookEntriesa few lines below already demonstrates the correct instrument:lorebooks.listEntriesByLorebooks(ids)fetches entries for many lorebooks in a single batched call. Reuse it here instead of the per-record loop.♻️ Proposed batched fetch
async listLorebooks(lorebookIds): Promise<CapabilityLorebookRecord[]> { const requestedIds = lorebookIds ? uniqueStrings(lorebookIds) : null; const records = (requestedIds ? await Promise.all(requestedIds.map((lorebookId) => lorebooks.getById(lorebookId))) : await lorebooks.list()) as Array<({ id: string } & Record<string, unknown>) | null>; - return Promise.all( - records - .flatMap((record) => - record ? [{ id: record.id, data: record, entries: lorebooks.listEntries(record.id) }] : [], - ) - .map(async (record) => ({ ...record, entries: await record.entries })), - ); + const validRecords = records.filter((record): record is NonNullable<typeof record> => Boolean(record)); + const entriesByLorebook = await lorebooks.listEntriesByLorebooks(validRecords.map((record) => record.id)); + const entriesById = new Map<string, unknown[]>(); + for (const entry of entriesByLorebook as Array<{ lorebookId: string }>) { + entriesById.set(entry.lorebookId, [...(entriesById.get(entry.lorebookId) ?? []), entry]); + } + return validRecords.map((record) => ({ + id: record.id, + data: record, + entries: entriesById.get(record.id) ?? [], + })); },🤖 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/capability-packages/capability-resources.service.ts` around lines 47 - 58, Update listLorebooks to fetch all lorebook entries through a single lorebooks.listEntriesByLorebooks call using the requested record IDs, then associate the returned entries with each lorebook by ID. Remove the per-record lorebooks.listEntries(record.id) calls while preserving the existing output shape and handling records with no entries.
🤖 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/components/chat/ChatSettingsDrawer.tsx`:
- Around line 8896-8930: The isGame rendering branch currently lacks the
Long-Term Memory package settings. Add a matching agent.id ===
"long-term-memory" && ltmPackage CapabilityElement case alongside the existing
hierarchical-maps handling, passing the same chatSettings and callbacks used in
the non-Game branch so Game chats expose the controls while preserving their
existing toggle behavior.
In
`@packages/server/src/services/capability-packages/capability-language-model.service.ts`:
- Around line 29-44: Update chatComplete to pass options.reasoningEffort
directly to provider.chatComplete instead of converting "none" to undefined.
Preserve the existing handling of all other completion options and ensure an
explicit "none" request reaches the provider unchanged.
In
`@packages/server/src/services/capability-packages/capability-route-registration.service.ts`:
- Around line 56-134: Update registerCapabilityPrivilegedRoutes so activation
after Fastify has started does not call app.route for previously unseen routes,
while preserving normal registration before startup and existing-slot
reactivation. Add an appropriate post-startup guard or runtime-safe registration
path around app.route, and ensure activatePackage remains able to install
packages with privileged routes without FST_ERR_INSTANCE_ALREADY_LISTENING.
In `@packages/server/src/services/generation/long-term-memory-runtime.ts`:
- Around line 30-56: Update recallLongTermMemory and
recordLongTermMemoryPromptAccepted to wrap their service.recall and
service.recordPromptAccepted awaits with the existing withLlmRequestTimeout(...)
helper, using the appropriate request timeout and preserving caller cancellation
where supported. Ensure both capability calls terminate within the bounded
timeout, including recordPromptAccepted which has no input.signal.
---
Outside diff comments:
In `@packages/server/src/routes/generate.routes.ts`:
- Around line 5748-5780: Move the recordAcceptedLongTermMemoryPrompt call out of
the initial gen.next() setup and execute it only after the while (!result.done)
loop completes successfully, preserving the existing abort and error-return
behavior so failed or interrupted streams are never recorded as accepted.
---
Nitpick comments:
In `@packages/server/src/routes/generate.routes.ts`:
- Around line 4596-4615: Replace the manual runtime-section placement logic in
the long-term-memory recall block with the existing placeRuntimeAgentInjection
helper, passing the recall text and relevant agent type. Preserve the current
behavior of replacing preset-owned sections, appending when placement is not
preset-owned, and skipping injection when placement is unavailable; continue
recording contextInjections and longTermMemoryRecallReceipt only when placement
succeeds.
In
`@packages/server/src/services/capability-packages/capability-resources.service.ts`:
- Around line 47-58: Update listLorebooks to fetch all lorebook entries through
a single lorebooks.listEntriesByLorebooks call using the requested record IDs,
then associate the returned entries with each lorebook by ID. Remove the
per-record lorebooks.listEntries(record.id) calls while preserving the existing
output shape and handling records with no entries.
In `@scripts/regressions/capability-package-lifecycle.regression.ts`:
- Around line 198-241: Add integration-level regression coverage for
registerCapabilityPrivilegedRoutes using a real Fastify instance, exercising
route registration after the server has started listening and verifying the
expected boot-order behavior. Keep the existing routeApp unit-style assertions
for prefix and duplicate-route validation, and align the test with the
implementation in registerCapabilityPrivilegedRoutes.
🪄 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 Plus
Run ID: b2e7cfe9-4729-40ca-bf09-5c056b28a475
📒 Files selected for processing (21)
docs/agents/agents-overview.mddocs/agents/built-in-agents.mdpackages/client/src/components/chat/ChatSettingsDrawer.tsxpackages/client/src/components/presets/PresetEditor.tsxpackages/server/src/routes/backup.routes.tspackages/server/src/routes/generate.routes.tspackages/server/src/routes/generate/generate-route-utils.tspackages/server/src/services/capability-packages/capability-embedding.service.tspackages/server/src/services/capability-packages/capability-language-model.service.tspackages/server/src/services/capability-packages/capability-module-runtime.service.tspackages/server/src/services/capability-packages/capability-persistence.service.tspackages/server/src/services/capability-packages/capability-resources.service.tspackages/server/src/services/capability-packages/capability-route-registration.service.tspackages/server/src/services/generation/long-term-memory-runtime.tspackages/server/src/services/generation/runtime-agent-sections.tspackages/server/src/services/local-embedder.tspackages/shared/src/schemas/capability-package.schema.tspackages/shared/src/types/capability-runtime.tsscripts/regressions/capability-package-lifecycle.regression.tsscripts/regressions/open-issues.regression.tsscripts/regressions/prompt.regression.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
SpicyMarinara
left a comment
SpicyMarinara
left a comment
There was a problem hiding this comment.
Maintainer review: merge conflict resolved, all CodeRabbit findings addressed and confirmed, review threads resolved, and local plus GitHub validation is green.
2 participants
Linked issue
Closes #4132
Why this change
Pasta-Devs/Marinara-Agentswhile keeping host integration in Marinara Engine, matching the repository ownership boundary.What changed
Validation
Automated commands completed locally:
pnpm checkpnpm --filter @marinara-engine/server exec tsx ../../scripts/regressions/capability-package-lifecycle.regression.tspnpm regression:promptpnpm --filter @marinara-engine/server exec tsx ../../scripts/regressions/open-issues.regression.tspnpm checkpasses locallyContainer (Docker / Podman) built and ran without issue
Ran the app, clicked through the changes manually
Checked edge cases (light + dark mode, mobile viewport, empty states, error paths)
Above manual verification completed (describe below)
Read and followed
CONTRIBUTING.mdManual verification notes
Risk And Proof Boundary
Docs and release impact
docs-i18nbranch updated to match, or a[docs-i18n]follow-up issue opened (see CONTRIBUTING.md § Translated documentation)Documentation was updated in
docs/agents/agents-overview.mdanddocs/agents/built-in-agents.md. Translation follow-up: #4131. No version bump is included.UI evidence (if applicable)
Not yet attached. Screenshots or recordings are required before moving this draft to ready for review.
Summary by CodeRabbit