Repository navigation
feat(cli): expose memory commands to cli from sdk - #155
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a new CLI "memory" command group (stats, history , clear [sessionId]) via a factory method and parser registration, documents the memory-CLI exposure and prior interactive loop mode, updates bash completion, and adjusts several type-only import paths. Notes duplicate insertions of memory handlers in commandFactory.ts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as neurolink (yargs)
participant CF as CLICommandFactory
participant GS as globalSession
participant SDK as NeuroLink SDK
U->>CLI: neurolink memory [stats|history|clear] [args] [--dry-run] [--format]
CLI->>CF: invoke memory handler
CF->>CF: processOptions(args)
alt Dry-run
CF-->>U: mock result (json/text) via handleOutput
else Real execution
CF->>GS: getOrCreateNeuroLink()
GS-->>CF: sdk instance
alt stats
CF->>SDK: getConversationStats()
SDK-->>CF: stats
else history
CF->>SDK: getConversationHistory(sessionId)
SDK-->>CF: history[]
else clear
CF->>SDK: clearAllConversations()/clearConversationSession(sessionId)
SDK-->>CF: result
end
CF-->>U: formatted output (json/text/table)
end
note over CLI,CF: Bash completion suggests memory subcommands (stats, history, clear)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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
🧹 Nitpick comments (6)
memory-bank/activeContext.md (1)
25-46: Unify “CURRENT STATUS” sections and fix chronologyYou now have three “CURRENT STATUS” headers with different dates. Demote this block to “PREVIOUS STATUS” (date 2025-09-06) or mark it “Recent Update” to avoid conflicting “current” labels with the 2025-09-07 Redis section and the earlier loop section.
-## 🚀 **CURRENT STATUS: MEMORY CLI COMMANDS IMPLEMENTED** (2025-09-06) +## 🚀 **PREVIOUS STATUS: MEMORY CLI COMMANDS IMPLEMENTED** (2025-09-06)Also consider removing or demoting the earlier “CURRENT STATUS: INTERACTIVE LOOP MODE IMPLEMENTED” above for consistency.
src/cli/factories/commandFactory.ts (3)
1821-1880: Honor --format table for stats outputWhen users pass --format table, stats currently render as text. Support table by mapping the object to rows; also handle dry-run the same way for consistency.
if (options.dryRun) { - const mockStats = { + const mockStats = { totalSessions: 5, totalTurns: 47, memoryUsage: "Active", }; - if (spinner) { + if (spinner) { spinner.succeed(chalk.green("✅ Memory stats retrieved (dry-run)")); } - this.handleOutput(mockStats, options); + if (options.format === "table") { + const rows = [ + { metric: "Total Sessions", value: mockStats.totalSessions }, + { metric: "Total Turns", value: mockStats.totalTurns }, + { metric: "Memory Status", value: mockStats.memoryUsage }, + ]; + this.handleOutput(rows, options); + } else { + this.handleOutput(mockStats, options); + } return; } @@ - if (options.format === "json") { + if (options.format === "json") { this.handleOutput(stats, options); + } else if (options.format === "table") { + const rows = [ + { metric: "Total Sessions", value: stats.totalSessions }, + { metric: "Total Turns", value: stats.totalTurns }, + { + metric: "Memory Status", + value: stats.totalSessions > 0 ? "Active" : "Empty", + }, + ]; + this.handleOutput(rows, options); } else { logger.always(chalk.blue("📊 Conversation Memory Stats:")); logger.always(` Total Sessions: ${stats.totalSessions}`); logger.always(` Total Turns: ${stats.totalTurns}`); logger.always( ` Memory Status: ${stats.totalSessions > 0 ? "Active" : "Empty"}`, ); }
1881-1963: Honor --format table for history outputFor non-JSON, you always pretty-print text; users requesting --format table won’t get a table. Delegate to handleOutput when table is requested.
- if (options.format === "json") { + if (options.format === "json") { this.handleOutput(history, options); + } else if (options.format === "table") { + const rows = history.map((m, i) => ({ + index: i + 1, + role: m.role, + content: m.content, + })); + this.handleOutput(rows, options); } else { logger.always( chalk.blue(`💬 Conversation History (${argv.sessionId}):`), ); for (const message of history) { const roleColor = message.role === "user" ? chalk.cyan : chalk.green; const roleLabel = message.role === "user" ? "User" : "Assistant"; logger.always(` [${roleColor(roleLabel)}]: ${message.content}`); } }
2086-2153: Completion: include memory in the “Available commands” footerYou added memory to top-level opts and its branch. The footer echo still omits memory; add it for consistency.
-echo "Available commands: generate, stream, batch, provider, status, models, mcp, discover, config, get-best-provider, completion" +echo "Available commands: generate (gen), stream, batch, provider, status, models, mcp, discover, memory, config, get-best-provider, completion"memory-bank/progress.md (2)
32-33: Clarify required vs optional args for subcommands.Bracket notation shows
clear [sessionId]buthistory <sessionId>. If history can default to the active session, note it; if not, add “(required)” to avoid ambiguity.
34-35: Document default output format and include a short JSON sample.State the default (e.g., text) and add a minimal JSON example for
statsto set expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
memory-bank/activeContext.md(1 hunks)memory-bank/progress.md(1 hunks)src/cli/factories/commandFactory.ts(4 hunks)src/cli/parser.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cli/factories/commandFactory.ts (4)
src/lib/types/cli.ts (1)
BaseCommandArgs(13-24)src/lib/session/globalSessionState.ts (1)
globalSession(110-110)src/lib/utils/logger.ts (2)
logger(341-380)error(223-225)src/cli/errorHandler.ts (1)
handleError(11-69)
🔇 Additional comments (6)
src/cli/factories/commandFactory.ts (3)
633-704: Nice addition: coherent memory command groupCommand structure and examples are clear. No blockers.
1965-2061: Behavior LGTM; JSON shape is stable and messages are clearClear all vs single-session handling, and dry-run shape look good.
1821-2061: SDK surface verification passedAll required NeuroLink SDK methods (getConversationStats, getConversationHistory, clearConversationSession, clearAllConversations) are implemented and exported—no further action needed.
src/cli/parser.ts (1)
172-174: Good integration pointMemory command group is registered in a sensible spot. Help/strict parsing will surface it correctly.
memory-bank/progress.md (2)
49-56: Enforce explicit confirmation formemory clear
neurolink memory clearcurrently wipes all sessions with no safeguard. Require--all(and optionally--forceor an interactive confirmation) for destructive actions. Update docs accordingly:-# Clear conversation history (all or specific session) -neurolink memory clear # Clear all sessions -neurolink memory clear session-123 # Clear specific session +# Clear conversation history (all or specific session) +# Destructive: requires explicit confirmation or flags +neurolink memory clear --all [--force] # Clear all sessions +neurolink memory clear session-123 # Clear specific sessionFile: memory-bank/progress.md
Verify that the implementation supports these flags or add the necessary yargs options/confirmation prompt.
36-36: Ensure bash completion for memory commands
Single registration ofcreateMemoryCommandsatsrc/cli/parser.ts:173; no.completion()invocation detected—verify your completion script or parser wiring includes thememorysubcommands (stats, history, clear) and there are no duplicate registrations.
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
please don't merge, fixing a build issue |
47eb638 to
be61b74
Compare
|
this is ready to merge, build shouldn't fail now |
be61b74 to
3166bfd
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 (2)
src/cli/factories/commandFactory.ts (2)
186-191: Expose dry-run as--dry-run(keep--dryRunas alias).Current code defines only
dryRun. Users expect kebab-case. Accept both to avoid breaking changes and update completion opts.- dryRun: { + dryRun: { type: "boolean" as const, default: false, - description: "Test command without making actual API calls (for testing)", - }, + description: "Test command without making actual API calls (for testing)", + alias: "dry-run", + },Also update completion options below (see separate comment).
2086-2109: Completion: include both--dry-runand--dryRun.Also aligns with doc fixes.
- 'opts="--provider --model --temperature --maxTokens --system --format --output --timeout --delay --disableTools --enableAnalytics --enableEvaluation --debug --quiet --noColor --configFile --dryRun"' + 'opts="--provider --model --temperature --maxTokens --system --format --output --timeout --delay --disableTools --enableAnalytics --enableEvaluation --debug --quiet --noColor --configFile --dry-run --dryRun"'
♻️ Duplicate comments (1)
memory-bank/progress.md (1)
35-36: Standardize dry-run flag to kebab-case.Replace
--dryRunwith--dry-runin bullets and examples. Also update the completion script and yargs option alias so both forms work.- - ✅ **Dry-Run Integration**: All commands support `--dryRun` for safe testing + - ✅ **Dry-Run Integration**: All commands support `--dry-run` for safe testing @@ -neurolink memory stats --dry-run -neurolink memory clear --dry-run +neurolink memory stats --dry-run +neurolink memory clear --dry-runAlso applies to: 54-56
🧹 Nitpick comments (4)
memory-bank/activeContext.md (1)
24-47: Duplicate “CURRENT STATUS” headings — demote the older one.Two adjacent “CURRENT STATUS” sections (Loop Mode and Memory CLI) are confusing. Make the Loop Mode one “PREVIOUS STATUS” to match the block already added below.
Apply:
-## 🚀 **CURRENT STATUS: INTERACTIVE LOOP MODE IMPLEMENTED** (2025-09-06) +## 🚀 **PREVIOUS STATUS: INTERACTIVE LOOP MODE IMPLEMENTED** (2025-09-06)src/cli/factories/commandFactory.ts (3)
274-275: Remove unsupported 'yaml' from the format union or add real support.
commonOptions.formatpermits only text/json/table, butprocessOptionstypes include"yaml". Drop it to avoid misleading types.- format: argv.format as "text" | "json" | "table" | "yaml" | undefined, + format: argv.format as "text" | "json" | "table" | undefined,
633-703: Scope memory command options to essentials.Using
buildOptions(y)exposes generation-only flags (provider, model, temperature) onmemorycommands. Limit to generic flags (format/output/debug/quiet/noColor/configFile/dry-run) for cleaner UX.If you want, I can send a small patch introducing
buildBasicOptions()and switch these three subcommands to it.
2181-2183: Completion: update final “Available commands” list.Add the newly supported commands so users see them on load.
-'echo "Available commands: generate, stream, batch, provider, status, models, mcp, discover, config, get-best-provider, completion"'; +'echo "Available commands: generate, stream, batch, provider, status, models, mcp, discover, memory, config, get-best-provider, completion, ollama, sagemaker, loop"';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
memory-bank/activeContext.md(1 hunks)memory-bank/progress.md(1 hunks)src/cli/factories/commandFactory.ts(5 hunks)src/cli/loop/session.ts(1 hunks)src/cli/parser.ts(3 hunks)src/lib/session/globalSessionState.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: test (18)
src/cli/factories/commandFactory.ts
[warning] 2007-2007:
Forbidden non-null assertion
🪛 GitHub Check: test (20)
src/cli/factories/commandFactory.ts
[warning] 2007-2007:
Forbidden non-null assertion
🪛 GitHub Actions: CI
src/cli/factories/commandFactory.ts
[warning] 1349-1349: Promise without error handling
[warning] 1454-1454: Promise without error handling
[warning] 1506-1506: Promise without error handling
[warning] 1732-1732: Promise without error handling
🔇 Additional comments (4)
src/cli/loop/session.ts (1)
6-6: Approve type-only import path change
No lingeringtypes/conversationTypes.jsimports found;types/conversation.jsis referenced in 16 places.src/lib/session/globalSessionState.ts (1)
3-3: LGTM on type import path update.Matches the new
types/conversation.jsconvention and is type-only.src/cli/parser.ts (2)
8-8: Good move to use the shared logger.
Switching fromconsole.errortologger.errormakes CLI output consistent.
173-175: Memory command group registration looks correct.Factory method is added in the right spot and won’t interfere with existing commands.
- added commands to expose commands regarding conversation memory in cli
3166bfd to
b428fb3
Compare
2 participants
Pull Request
Description
Type of Change
Related Issues
Changes Made
AI Provider Impact
Component Impact
Testing
Test Environment
Performance Impact
Breaking Changes
not a breaking change
Screenshots/Demo
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Documentation