Repository navigation
feat(memory): add per-call memory control options (read/write) for ge… - #906
Conversation
…nerate and stream in NeuroLink - add per-call memory control options (read/write) for generate and stream in NeuroLink
|
@adarshpandey-cs21 is attempting to deploy a commit to the Sachin Sharma's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughPer-call memory control feature added to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds per-call overrides to control NeuroLink’s condensed memory behavior (read/write) on generate() and stream() calls, without changing global memory configuration.
Changes:
- Added
memoryper-call flags (enabled,read,write) toGenerateOptionsandStreamOptions. - Introduced
shouldReadMemory()/shouldWriteMemory()helpers and applied them to stream retrieval and generate/stream background storage. - Updated memory feature docs to describe per-call override semantics and precedence.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/lib/types/streamTypes.ts |
Adds StreamOptions.memory per-call flags for memory control. |
src/lib/types/generateTypes.ts |
Adds GenerateOptions.memory per-call flags for memory control. |
src/lib/neurolink.ts |
Implements helper gates and wires per-call flags into stream read + generate/stream write logic. |
docs/features/memory.md |
Documents per-call memory override flags, defaults, and precedence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private shouldReadMemory( | ||
| perCallMemory: { enabled?: boolean; read?: boolean } | undefined, | ||
| userId: unknown, | ||
| ): boolean { | ||
| if ( | ||
| !this.conversationMemoryConfig?.conversationMemory?.memory?.enabled || | ||
| !userId | ||
| ) { |
There was a problem hiding this comment.
memory.read is now part of the public per-call API, but generate() never performs condensed-memory retrieval (there’s no call path that uses shouldReadMemory()/retrieveMemory() for generate). As a result, GenerateOptions.memory.read has no effect, and the docs/API contract are misleading. Consider adding the same pre-call retrieval step to generate() (after auth/requestContext merging so context.userId is available) or remove/rename the read option for generate if it’s intentionally stream-only.
| if ( | ||
| this.conversationMemoryConfig?.conversationMemory?.memory?.enabled && | ||
| this.shouldReadMemory(options.memory, options.context?.userId) && | ||
| options.context?.userId | ||
| ) { |
There was a problem hiding this comment.
Per-call memory overrides (memory.enabled / memory.read / memory.write) introduce new branching behavior for both stream retrieval and background storage, but there doesn’t appear to be automated coverage asserting the precedence rules (global disabled vs per-call overrides, enabled=false overriding read/write, etc.). Adding coverage in the existing continuous test suite for memory would help prevent regressions and ensure the new flags behave as documented.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
docs/features/memory.md (1)
278-283:⚠️ Potential issue | 🟡 MinorClarify read vs write activation conditions explicitly.
“all three conditions must be met” is slightly inaccurate for read-only calls. Non-empty response content is only a write prerequisite, not a read prerequisite. Consider splitting into separate bullets for read and write activation to avoid ambiguity.
Suggested wording
-For memory to activate on a call, all three conditions must be met: - -1. `memory.enabled` is `true` in the config -2. `options.context.userId` is provided in the generate/stream call -3. The response has non-empty content (for write) +Memory activation depends on operation: + +- **Read (inject into prompt)** requires: + 1. Global memory enabled + 2. `options.context.userId` provided + 3. Per-call flags allow read (`memory.enabled !== false` and `memory.read !== false`) + +- **Write (store after call)** requires: + 1. Global memory enabled + 2. `options.context.userId` provided + 3. Per-call flags allow write (`memory.enabled !== false` and `memory.write !== false`) + 4. Response content is non-empty (trimmed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/memory.md` around lines 278 - 283, The current wording implies all three conditions (memory.enabled, options.context.userId, and non-empty response content) apply to both read and write—update the text to separate read vs write activation: state that for reads only memory.enabled and options.context.userId are required (e.g., when calling generate/stream to fetch memory), and for writes both memory.enabled and options.context.userId are required plus the response must have non-empty content before persisting; reference the config flag memory.enabled, the call options.context.userId used in generate/stream, and the response content requirement so readers can see which conditions apply to read-only vs write operations.
🧹 Nitpick comments (2)
src/lib/types/generateTypes.ts (2)
475-482: Consider extracting sharedPerCallMemoryOptionstype.The
memoryfield shape is identical in bothGenerateOptionsandStreamOptions. Extracting it to a shared type would ensure consistency and simplify future modifications.♻️ Optional: Extract shared type
In
src/lib/types/common.tsor a dedicated file:/** * Per-call memory control options. * Override global memory SDK behavior for individual generate/stream calls. */ export type PerCallMemoryOptions = { /** Master toggle. When false, both read and write are skipped. Defaults to true. */ enabled?: boolean; /** Whether to read condensed memory and prepend to prompt. Defaults to true. */ read?: boolean; /** Whether to write the conversation into memory after completion. Defaults to true. */ write?: boolean; };Then in both
GenerateOptionsandStreamOptions:- memory?: { - /** Master toggle for this call. When false, both read and write are skipped. Defaults to true. */ - enabled?: boolean; - /** Whether to read condensed memory and prepend to prompt. Defaults to true. */ - read?: boolean; - /** Whether to write (add/condense) the conversation into memory after completion. Defaults to true. */ - write?: boolean; - }; + memory?: PerCallMemoryOptions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/generateTypes.ts` around lines 475 - 482, Extract the duplicated memory shape into a shared type and replace the inline definitions in both GenerateOptions and StreamOptions: create a new exported type PerCallMemoryOptions (or add it to src/lib/types/common.ts) with enabled?: boolean, read?: boolean, write?: boolean and then update the memory?: { ... } fields in GenerateOptions and StreamOptions to use memory?: PerCallMemoryOptions so both interfaces reference the single definition (adjust imports/exports accordingly).
475-482: CLI does not expose per-call memory flags.Based on the context snippets,
commandFactory.tsconstructsGenerateOptionswithout thememoryfield, andprocessOptionshas no--memory,--disable-memory-read, or--disable-memory-writeargument parsing. CLI users won't be able to leverage this feature until corresponding flags are added.Consider adding CLI support in a follow-up if per-call memory control is needed from the command line.
Would you like me to open an issue to track adding CLI flags for per-call memory control (e.g.,
--memory-read=false,--memory-write=false)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/generateTypes.ts` around lines 475 - 482, The PR adds per-call memory settings to GenerateOptions but the CLI never exposes them: update commandFactory.ts where GenerateOptions is constructed to include the new memory object (memory.enabled, memory.read, memory.write) based on parsed flags, and extend processOptions (or the argument parsing setup) to accept CLI flags like --memory, --disable-memory-read, and --disable-memory-write (or synonymous names) so the parsed values feed into GenerateOptions; ensure flag names map to booleans and default behavior matches the types' defaults when flags are omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/features/memory.md`:
- Around line 278-283: The current wording implies all three conditions
(memory.enabled, options.context.userId, and non-empty response content) apply
to both read and write—update the text to separate read vs write activation:
state that for reads only memory.enabled and options.context.userId are required
(e.g., when calling generate/stream to fetch memory), and for writes both
memory.enabled and options.context.userId are required plus the response must
have non-empty content before persisting; reference the config flag
memory.enabled, the call options.context.userId used in generate/stream, and the
response content requirement so readers can see which conditions apply to
read-only vs write operations.
---
Nitpick comments:
In `@src/lib/types/generateTypes.ts`:
- Around line 475-482: Extract the duplicated memory shape into a shared type
and replace the inline definitions in both GenerateOptions and StreamOptions:
create a new exported type PerCallMemoryOptions (or add it to
src/lib/types/common.ts) with enabled?: boolean, read?: boolean, write?: boolean
and then update the memory?: { ... } fields in GenerateOptions and StreamOptions
to use memory?: PerCallMemoryOptions so both interfaces reference the single
definition (adjust imports/exports accordingly).
- Around line 475-482: The PR adds per-call memory settings to GenerateOptions
but the CLI never exposes them: update commandFactory.ts where GenerateOptions
is constructed to include the new memory object (memory.enabled, memory.read,
memory.write) based on parsed flags, and extend processOptions (or the argument
parsing setup) to accept CLI flags like --memory, --disable-memory-read, and
--disable-memory-write (or synonymous names) so the parsed values feed into
GenerateOptions; ensure flag names map to booleans and default behavior matches
the types' defaults when flags are omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5a03519-e13c-4774-97ba-8196144e4713
📒 Files selected for processing (4)
docs/features/memory.mdsrc/lib/neurolink.tssrc/lib/types/generateTypes.tssrc/lib/types/streamTypes.ts
|
🎉 This PR is included in version 9.33.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…nerate and stream in NeuroLink
Pull Request
Description
What does this PR do?
A clear and concise description of the changes in this pull request.
Related Issues
Does this PR close any issues?
Fixes #(issue number)
Closes #(issue number)
Relates to #(issue number)
Type of Change
Please select the type of change:
Motivation and Context
Why is this change needed? What problem does it solve?
Provide context for reviewers:
Changes Made
What specific changes were made?
Provide a bullet-point list of the key changes:
Breaking Changes
Does this PR introduce breaking changes?
If yes, describe:
Testing
How has this been tested?
Please describe the tests you ran and their results:
Test Coverage
Manual Testing Steps
Provide steps for manual testing:
Code Quality
Have you followed code quality standards?
Documentation
Have you updated documentation?
Commit Message Format
Does your commit follow semantic commit conventions?
type(scope): descriptionExample:
feat(providers): add support for LiteLLM proxyDependencies
Does this PR add, update, or remove dependencies?
If yes, list dependencies and justification:
Performance Impact
Does this change affect performance?
If applicable, provide benchmark results:
Security Considerations
Are there any security implications?
If applicable, describe:
Deployment Notes
Special deployment instructions?
Screenshots / Videos
If applicable, add screenshots or videos to demonstrate changes:
[Add screenshots or videos here]
Reviewer Checklist
For reviewers:
Additional Notes
Any additional information for reviewers:
[Add any extra context, concerns, or questions here]
Pre-submission Checklist
Before submitting, ensure you have:
pnpm testpnpm buildpnpm run validate:alland all checks passThank you for contributing to NeuroLink!
Summary by CodeRabbit
Release Notes
New Features
generate()andstream()methods, allowing you to override global memory settings on individual API calls with granularenabled,read, andwriteflags.Documentation