Conversation
…LAUDE.md updates - Add Claude Code lifecycle hooks (SessionStart, PostToolUse, SessionEnd) for automatic memory capture and injection - Implement 3-layer progressive disclosure for token-optimized retrieval (Index ~50 tokens, Timeline ~200 tokens, Details ~600 tokens) - Add CLAUDE.md auto-updater to populate LEARNED section with learnings - Add auto-compression threshold trigger in observation store - Add new CLI commands: `memory sync-claude` and `memory index` - Update documentation with new memory features - Remove 139 lines of unnecessary inline comments (code cleanup)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
rohitg00
changed the title
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds Claude Code memory lifecycle: session hooks (start, post-tool-use, end), a MemoryHookManager, progressive-disclosure retrieval, CLAUDE.md updater, CLI commands (sync-claude, index), observation-store auto-compression, and related public exports and types. Changes
Sequence Diagram(s)sequenceDiagram
participant Session as Claude Code Session
participant Start as SessionStartHook
participant PostTool as PostToolUseHook
participant Store as ObservationStore / LearningStore
participant Compress as SessionEndHook / MemoryCompressor
participant CLAUDE as ClaudeMdUpdater
Session->>Start: onSessionStart(context)
Start->>Store: queryInjectableLearnings(...)
Store-->>Start: learnings + tokenCount
Start-->>Session: inject(formatted learnings)
loop tool uses
Session->>PostTool: onToolUse(event)
PostTool->>Store: addObservation(observation)
PostTool-->>Session: captureResult
end
Session->>Compress: onSessionEnd(context)
Compress->>Store: getObservations(sessionId)
Store-->>Compress: observations
Compress->>Compress: compress -> learnings
Compress->>Store: saveLearnings + deleteObservations
Compress-->>Session: compressionResult
Session->>CLAUDE: syncClaudeMd(options)
CLAUDE->>Store: getLearnings(scope, filters)
Store-->>CLAUDE: learnings
CLAUDE->>CLAUDE: format + merge manual entries
CLAUDE->>Filesystem: write/update CLAUDE.md
CLAUDE-->>Session: syncComplete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@docs/fumadocs/content/docs/commands.mdx`:
- Around line 180-185: The docs table incorrectly lists unsupported flags
`--min-effectiveness` and `--max`; update the options table to remove those and
instead list the actual CLI flags `--limit` (with description "Limit number of
results") and `--tags` (with description for filtering by tags), while keeping
`--global` if supported; ensure the table entries match the CLI flag names used
in the command implementation (replace occurrences of
`--min-effectiveness`/`--max` with `--limit`/`--tags` in the commands.mdx
content).
In `@packages/core/src/memory/claude-md-updater.ts`:
- Around line 399-406: The project name extraction in createNewClaudeMd uses
this.projectPath.split('/').pop() which fails on Windows; replace the split
approach with a cross-platform extraction using path.basename from Node's path
module (import path) when computing projectName in createNewClaudeMd, and apply
the same fix to the analogous extraction in session-end.ts (the code near line
66) so both use path.basename(this.projectPath) (or
path.basename(projectPathVariable)) to reliably get the project folder name on
all platforms.
In `@packages/core/src/memory/hooks/manager.ts`:
- Around line 115-126: The onSessionEnd implementation currently only calls
sessionEndHook.generateHookOutput and never runs the hook side-effects; modify
onSessionEnd to first invoke sessionEndHook.execute(context) so the
compression/cleanup runs (and update any manager stats/counters from the execute
result if applicable), then call sessionEndHook.generateHookOutput(context) and
return that output; reference the existing onSessionEnd method,
SessionEndContext construction, sessionEndHook.execute, and
sessionEndHook.generateHookOutput when making the change.
- Around line 224-228: The setConfig method currently updates this.config and
propagates it to sessionStartHook and sessionEndHook but forgets to apply it to
postToolUseHook; update setConfig (in the class that defines setConfig) to also
call this.postToolUseHook.setConfig(this.config) so PostToolUseHook receives the
new MemoryHookConfig (e.g., enable/threshold changes) along with
sessionStartHook and sessionEndHook.
In `@packages/core/src/memory/hooks/post-tool-use.ts`:
- Line 60: pendingErrors map can grow unbounded because clearOldPendingErrors()
is never invoked automatically; modify the class to call clearOldPendingErrors()
at the start of the execute() method so stale entries are purged before each
run, ensuring you reference the pendingErrors Map and the
clearOldPendingErrors() helper when making the change and keep the call near the
top of execute() to run periodically for long‑running sessions.
In `@packages/core/src/memory/hooks/session-end.ts`:
- Around line 64-67: The projectName passed into the MemoryCompressor can be
undefined or empty when context.project_path is missing or ends with a slash;
update the code that computes projectName (used in the MemoryCompressor
constructor) to safely derive a fallback: normalize context.project_path
(coalesce to ''), trim trailing slashes, split on '/' and take the last
non-empty segment, and if that results in ''/undefined use a sensible default
(e.g., null or a literal fallback name) before passing it to new
MemoryCompressor so downstream code always receives a stable value.
In `@packages/core/src/memory/observation-store.ts`:
- Around line 109-111: The call to the async method checkAutoCompression() from
the synchronous add() method is currently fire-and-forget and can cause
unhandled promise rejections and race conditions; fix it by either (A) making
add() async and awaiting checkAutoCompression() so callers get deterministic
behavior (update any callers/tests accordingly) or (B) keep fire-and-forget but
explicitly handle errors and ordering by wrapping the call (e.g., void
checkAutoCompression().catch(err => /* log via processLogger or storeLogger */)
or enqueue it via setImmediate/nextTick to avoid unhandled rejections),
referencing the add() function and checkAutoCompression() method to locate the
change. Ensure logging of errors when choosing option B and update function
signatures if you choose option A.
In `@packages/core/src/memory/progressive-disclosure.ts`:
- Around line 205-226: The token accounting currently sums index (+timeline)
tokens even when smartRetrieve returns only timeline (layer 2) or details (layer
3) entries; update the logic so tokensUsed and tokensRemaining reflect only the
tokens for the entries actually returned (for layer 1 use indexTokens, for layer
2 use timelineTokens, for layer 3 use detailsTokens), e.g., compute tokensUsed =
(layer === 1 ? indexTokens : layer === 2 ? indexTokensRemoved? no:
timelineTokens : detailsTokens) — more simply: when returning timeline (from
this.getTimeline) set tokensUsed = timelineTokens and tokensRemaining =
tokenBudget - timelineTokens; when returning details (from this.getDetails) set
tokensUsed = detailsTokens and tokensRemaining = tokenBudget - detailsTokens;
keep TOKEN_ESTIMATES, relevantIds, tokenBudget, indexTokens, timelineTokens,
detailsTokens, getTimeline and getDetails references to locate and update the
branch that returns layer 2/3.
🧹 Nitpick comments (4)
packages/core/src/memory/observation-store.ts (1)
147-152: Consider validatingthresholdparameter.
setCompressionThresholdaccepts any number including zero, negative, or non-integer values. A threshold of 0 or negative would trigger compression on everyadd()call.🛡️ Proposed validation
setCompressionThreshold(threshold: number): void { + if (threshold < 1 || !Number.isInteger(threshold)) { + throw new Error('Compression threshold must be a positive integer'); + } this.compressionThreshold = threshold; }packages/core/src/memory/hooks/session-end.ts (1)
55-62: Hard-coded threshold may not align withcompressionThresholdconfig.The check
observations.length < 3uses a hard-coded value, whileMemoryHookConfig.compressionThresholddefaults to 50. This could be confusing—the config threshold appears unused in theexecuteflow.Consider using the config value or documenting the distinction between these thresholds.
♻️ Proposed fix to use config threshold
- if (observations.length < 3) { + const minObservationsForCompression = Math.min(3, this.config.compressionThreshold); + if (observations.length < minObservationsForCompression) { return { compressed: false, observationCount: observations.length, learningCount: 0, learnings: [], }; }packages/core/src/memory/hooks/post-tool-use.ts (1)
369-394: Solution matching heuristic may produce false positives.The
findMatchingSolutionlogic matches ifmatchCount >= 1 && hasKeywordMatch, which could link unrelated actions to errors. For example, an action containing "fix" and sharing a single common word (>3 chars) with any pending error would be marked as a solution.Consider requiring a higher match threshold or semantic similarity.
💡 Suggested tightening of match criteria
- if (matchCount >= 2 || (matchCount >= 1 && hasKeywordMatch)) { + // Require at least 2 word matches to reduce false positives + if (matchCount >= 2) { this.pendingErrors.delete(errorId); return errorData; }packages/core/src/memory/hooks/session-start.ts (1)
63-74: Same cross-platform path issue and hard-coded configuration values.Line 65 uses
split('/')which won't work on Windows. Additionally,minRelevance: 30,maxLearnings: 10, anddisclosureLevel: 'preview'are hard-coded rather than derived fromMemoryHookConfig.Consider using
basename()for the path and exposing these as config options.♻️ Proposed improvements
+import { basename } from 'node:path'; + const injector = new MemoryInjector( this.projectPath, - context.project_path?.split('/').pop() + context.project_path ? basename(context.project_path) : undefined ); const result = await injector.injectForAgent(this.agent, { maxTokens: this.config.maxTokensForInjection, - minRelevance: 30, - maxLearnings: 10, + minRelevance: this.config.minRelevanceForCapture, + maxLearnings: 10, // Consider adding to MemoryHookConfig includeGlobal: true, disclosureLevel: 'preview', });
| private createNewClaudeMd(learnedSection: string): string { | ||
| const projectName = this.projectPath.split('/').pop() || 'Project'; | ||
|
|
||
| return `# ${projectName} | ||
|
|
||
| ${learnedSection} | ||
| `; | ||
| } |
There was a problem hiding this comment.
Path splitting doesn't work correctly on Windows.
this.projectPath.split('/').pop() uses Unix path separators. On Windows, paths use \, so this would return the entire path instead of the project name.
🔧 Proposed cross-platform fix
+import { basename } from 'node:path';
+
private createNewClaudeMd(learnedSection: string): string {
- const projectName = this.projectPath.split('/').pop() || 'Project';
+ const projectName = basename(this.projectPath) || 'Project';
return `# ${projectName}Also update line 66 in session-end.ts similarly.
🤖 Prompt for AI Agents
In `@packages/core/src/memory/claude-md-updater.ts` around lines 399 - 406, The
project name extraction in createNewClaudeMd uses
this.projectPath.split('/').pop() which fails on Windows; replace the split
approach with a cross-platform extraction using path.basename from Node's path
module (import path) when computing projectName in createNewClaudeMd, and apply
the same fix to the analogous extraction in session-end.ts (the code near line
66) so both use path.basename(this.projectPath) (or
path.basename(projectPathVariable)) to reliably get the project folder name on
all platforms.
| this.checkAutoCompression(); | ||
|
|
||
| return observation; |
There was a problem hiding this comment.
Unhandled async call creates potential unhandled promise rejection.
checkAutoCompression() is an async method but is called without await in the synchronous add() method. This causes:
- Unhandled promise rejection if the callback throws
- No guarantee compression completes before subsequent operations
- Potential race conditions if
add()is called multiple times rapidly
If fire-and-forget is intentional, catch errors explicitly. Otherwise, consider making add() async or using a separate queue mechanism.
🔧 Proposed fix to handle the promise
Option 1: Fire-and-forget with error handling:
- this.checkAutoCompression();
+ this.checkAutoCompression().catch((err) => {
+ console.error('Auto-compression failed:', err);
+ });Option 2: Make add() async (breaking change):
- add(
+ async add(
type: ObservationType,
content: ObservationContent,
agent: AgentType,
relevance = 50
- ): Observation {
+ ): Promise<Observation> {
// ...
- this.checkAutoCompression();
+ await this.checkAutoCompression();
return observation;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.checkAutoCompression(); | |
| return observation; | |
| this.checkAutoCompression().catch((err) => { | |
| console.error('Auto-compression failed:', err); | |
| }); | |
| return observation; |
🤖 Prompt for AI Agents
In `@packages/core/src/memory/observation-store.ts` around lines 109 - 111, The
call to the async method checkAutoCompression() from the synchronous add()
method is currently fire-and-forget and can cause unhandled promise rejections
and race conditions; fix it by either (A) making add() async and awaiting
checkAutoCompression() so callers get deterministic behavior (update any
callers/tests accordingly) or (B) keep fire-and-forget but explicitly handle
errors and ordering by wrapping the call (e.g., void
checkAutoCompression().catch(err => /* log via processLogger or storeLogger */)
or enqueue it via setImmediate/nextTick to avoid unhandled rejections),
referencing the add() function and checkAutoCompression() method to locate the
change. Ensure logging of errors when choosing option B and update function
signatures if you choose option A.
| const remainingBudget = tokenBudget - indexTokens; | ||
| const maxTimelineEntries = Math.floor(remainingBudget / TOKEN_ESTIMATES.timeline); | ||
|
|
||
| if (maxTimelineEntries >= 1) { | ||
| const timelineIds = relevantIds.slice(0, Math.min(maxTimelineEntries, 10)); | ||
| const timeline = this.getTimeline(timelineIds, options); | ||
| const timelineTokens = timeline.length * TOKEN_ESTIMATES.timeline; | ||
|
|
||
| const afterTimelineBudget = remainingBudget - timelineTokens; | ||
| const maxDetailsEntries = Math.floor(afterTimelineBudget / TOKEN_ESTIMATES.details); | ||
|
|
||
| if (maxDetailsEntries >= 1) { | ||
| const detailsIds = timelineIds.slice(0, Math.min(maxDetailsEntries, 5)); | ||
| const details = this.getDetails(detailsIds, options); | ||
| const detailsTokens = details.length * TOKEN_ESTIMATES.details; | ||
|
|
||
| return { | ||
| layer: 3, | ||
| entries: details, | ||
| tokensUsed: indexTokens + timelineTokens + detailsTokens, | ||
| tokensRemaining: tokenBudget - (indexTokens + timelineTokens + detailsTokens), | ||
| }; |
There was a problem hiding this comment.
Token accounting doesn’t match returned layer.
smartRetrieve returns only timeline/details entries for layers 2/3, but tokensUsed/tokensRemaining include index (+timeline) estimates as if those layers were injected. This overstates usage and underreports remaining budget, contradicting the “minimum tokens” intent. Either return a composite payload that includes all layers or compute token usage from the returned layer only.
Proposed fix (align accounting with returned entries)
- return {
- layer: 3,
- entries: details,
- tokensUsed: indexTokens + timelineTokens + detailsTokens,
- tokensRemaining: tokenBudget - (indexTokens + timelineTokens + detailsTokens),
- };
+ return {
+ layer: 3,
+ entries: details,
+ tokensUsed: detailsTokens,
+ tokensRemaining: tokenBudget - detailsTokens,
+ };
...
- return {
- layer: 2,
- entries: timeline,
- tokensUsed: indexTokens + timelineTokens,
- tokensRemaining: tokenBudget - (indexTokens + timelineTokens),
- };
+ return {
+ layer: 2,
+ entries: timeline,
+ tokensUsed: timelineTokens,
+ tokensRemaining: tokenBudget - timelineTokens,
+ };🤖 Prompt for AI Agents
In `@packages/core/src/memory/progressive-disclosure.ts` around lines 205 - 226,
The token accounting currently sums index (+timeline) tokens even when
smartRetrieve returns only timeline (layer 2) or details (layer 3) entries;
update the logic so tokensUsed and tokensRemaining reflect only the tokens for
the entries actually returned (for layer 1 use indexTokens, for layer 2 use
timelineTokens, for layer 3 use detailsTokens), e.g., compute tokensUsed =
(layer === 1 ? indexTokens : layer === 2 ? indexTokensRemoved? no:
timelineTokens : detailsTokens) — more simply: when returning timeline (from
this.getTimeline) set tokensUsed = timelineTokens and tokensRemaining =
tokenBudget - timelineTokens; when returning details (from this.getDetails) set
tokensUsed = detailsTokens and tokensRemaining = tokenBudget - detailsTokens;
keep TOKEN_ESTIMATES, relevantIds, tokenBudget, indexTokens, timelineTokens,
detailsTokens, getTimeline and getDetails references to locate and update the
branch that returns layer 2/3.
- Fix double execution bug in MemoryHookManager by adding generateHookOutputFromResult methods to avoid calling execute() twice - Add setConfig method to PostToolUseHook for runtime config updates - Update manager.setConfig() to propagate config to all hooks
- Fix cross-platform path handling using path.basename() instead of split('/')
in claude-md-updater.ts, session-start.ts, and session-end.ts
- Fix onSessionEnd to call execute() before generateHookOutput to run
compression/cleanup side effects
- Add generateHookOutputFromResult() to SessionEndHook to avoid double execution
- Call clearOldPendingErrors() in PostToolUseHook.execute() to prevent
unbounded pendingErrors map growth
- Handle async checkAutoCompression() errors in observation-store.ts
- Add validation for compression threshold (must be positive integer)
- Fix docs/commands.mdx to use correct CLI flags (--limit, --tags)
- Clarify token accounting in progressive-disclosure smartRetrieve() JSDoc
- Use config.minRelevanceForCapture in session-start.ts instead of hardcoded value
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/core/src/memory/hooks/manager.ts`:
- Around line 246-271: generateClaudeCodeHooksConfig currently injects
this.projectPath directly into shell command strings creating injection/quoting
issues; fix by constructing a shell-escaped version of projectPath (escape
single quotes by replacing ' with '\'' and wrap the result in single quotes)
before interpolating it into the command templates in
generateClaudeCodeHooksConfig so each command uses the safely quoted path
(reference generateClaudeCodeHooksConfig and the projectPath property).
- Around line 231-241: Add a concurrency guard to checkAutoCompression to avoid
overlapping compressions: introduce a private boolean field (e.g.,
compressionInProgress) and early-return if it's true or if this.config.enabled
is false; when count from postToolUseHook.getObservationCount() meets or exceeds
this.config.compressionThreshold set compressionInProgress = true before calling
sessionEndHook.forceCompress(this.sessionId) and reset it in a finally block so
only one compression runs at a time.
In `@packages/core/src/memory/hooks/session-start.ts`:
- Around line 63-66: The code uses context.project_path?.split('/').pop() which
fails on Windows; change it to use Node's path utilities: import path from
'path' and pass path.basename(context.project_path || '') (or undefined when
project_path is falsy) into the MemoryInjector constructor (replace the
split/pop expression with path.basename(...) and ensure the top of the file
imports path and handles a missing project_path appropriately).
🧹 Nitpick comments (1)
packages/core/src/memory/hooks/post-tool-use.ts (1)
401-410: Redundant condition in solution matching.The condition
matchCount >= 1 && hasKeywordMatchat line 406 is always true when inside theif (hasKeywordMatch)block—hasKeywordMatchis guaranteed to be truthy. Consider simplifying:♻️ Proposed simplification
- if (matchCount >= 2 || (matchCount >= 1 && hasKeywordMatch)) { + if (matchCount >= 1) {
| /** | ||
| * Check if auto-compression should trigger | ||
| */ | ||
| private async checkAutoCompression(): Promise<void> { | ||
| if (!this.config.enabled) return; | ||
|
|
||
| const count = this.postToolUseHook.getObservationCount(); | ||
| if (count >= this.config.compressionThreshold) { | ||
| await this.sessionEndHook.forceCompress(this.sessionId); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "forceCompress|autoCompress|compressionThreshold" --type ts -A 3 -B 1Repository: rohitg00/skillkit
Length of output: 7446
🏁 Script executed:
rg -n "checkAutoCompression\(\)" --type ts -B 3 -A 1Repository: rohitg00/skillkit
Length of output: 1509
🏁 Script executed:
sed -n '85,120p' packages/core/src/memory/hooks/manager.tsRepository: rohitg00/skillkit
Length of output: 1079
🏁 Script executed:
sed -n '1,150p' packages/core/src/memory/hooks/post-tool-use.ts | head -100Repository: rohitg00/skillkit
Length of output: 2767
🏁 Script executed:
sed -n '85,130p' packages/core/src/memory/hooks/post-tool-use.tsRepository: rohitg00/skillkit
Length of output: 1535
🏁 Script executed:
sed -n '1,100p' packages/core/src/memory/hooks/manager.tsRepository: rohitg00/skillkit
Length of output: 2988
🏁 Script executed:
rg -n "getObservationCount" --type ts -B 2 -A 5Repository: rohitg00/skillkit
Length of output: 5421
🏁 Script executed:
sed -n '108,180p' packages/core/src/memory/hooks/session-end.tsRepository: rohitg00/skillkit
Length of output: 1953
🏁 Script executed:
sed -n '50,80p' packages/core/src/memory/hooks/post-tool-use.tsRepository: rohitg00/skillkit
Length of output: 938
🏁 Script executed:
sed -n '234,242p' packages/core/src/memory/hooks/manager.tsRepository: rohitg00/skillkit
Length of output: 344
🏁 Script executed:
rg -n "onToolUse" --type ts -B 2 -A 2 | head -50Repository: rohitg00/skillkit
Length of output: 447
🏁 Script executed:
rg -n "onThresholdReached|AutoCompressCallback" --type ts -B 2 -A 2Repository: rohitg00/skillkit
Length of output: 3270
🏁 Script executed:
rg -n "setAutoCompressCallback" --type ts -B 2 -A 2Repository: rohitg00/skillkit
Length of output: 461
🏁 Script executed:
sed -n '93,110p' packages/core/src/memory/hooks/manager.tsRepository: rohitg00/skillkit
Length of output: 625
Add concurrency protection to prevent overlapping compression attempts.
The checkAutoCompression() method lacks a guard against concurrent compression calls. If multiple onToolUse events occur in quick succession, or if forceCompress() is slow, the next call to checkAutoCompression() can trigger compression again before the previous one completes.
Unlike ObservationStore.checkAutoCompression() which has a compressionInProgress flag, the manager version has no such protection. Consider adding a flag to prevent overlapping compression attempts:
private compressionInProgress = false;
private async checkAutoCompression(): Promise<void> {
if (!this.config.enabled || this.compressionInProgress) return;
const count = this.postToolUseHook.getObservationCount();
if (count >= this.config.compressionThreshold) {
this.compressionInProgress = true;
try {
await this.sessionEndHook.forceCompress(this.sessionId);
} finally {
this.compressionInProgress = false;
}
}
}🤖 Prompt for AI Agents
In `@packages/core/src/memory/hooks/manager.ts` around lines 231 - 241, Add a
concurrency guard to checkAutoCompression to avoid overlapping compressions:
introduce a private boolean field (e.g., compressionInProgress) and early-return
if it's true or if this.config.enabled is false; when count from
postToolUseHook.getObservationCount() meets or exceeds
this.config.compressionThreshold set compressionInProgress = true before calling
sessionEndHook.forceCompress(this.sessionId) and reset it in a finally block so
only one compression runs at a time.
| generateClaudeCodeHooksConfig(): Record<string, unknown> { | ||
| return { | ||
| hooks: [ | ||
| { | ||
| matcher: '.*', | ||
| hooks: [ | ||
| { | ||
| type: 'command', | ||
| command: `npx skillkit memory hook session-start --project "${this.projectPath}"`, | ||
| event: 'SessionStart', | ||
| }, | ||
| { | ||
| type: 'command', | ||
| command: `npx skillkit memory hook post-tool-use --project "${this.projectPath}"`, | ||
| event: 'PostToolUse', | ||
| }, | ||
| { | ||
| type: 'command', | ||
| command: `npx skillkit memory hook session-end --project "${this.projectPath}"`, | ||
| event: 'SessionEnd', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
| } |
There was a problem hiding this comment.
Project path should be escaped for shell safety.
The projectPath is interpolated directly into shell commands. Paths containing spaces, quotes, or special characters could break execution or pose injection risks. Consider using proper escaping or recommending that consumers handle this.
🛡️ Proposed fix: escape the path
+ private escapeShellArg(arg: string): string {
+ return `'${arg.replace(/'/g, "'\\''")}'`;
+ }
+
generateClaudeCodeHooksConfig(): Record<string, unknown> {
+ const escapedPath = this.escapeShellArg(this.projectPath);
return {
hooks: [
{
matcher: '.*',
hooks: [
{
type: 'command',
- command: `npx skillkit memory hook session-start --project "${this.projectPath}"`,
+ command: `npx skillkit memory hook session-start --project ${escapedPath}`,
event: 'SessionStart',
},
// ... apply to other commands
],
},
],
};
}🤖 Prompt for AI Agents
In `@packages/core/src/memory/hooks/manager.ts` around lines 246 - 271,
generateClaudeCodeHooksConfig currently injects this.projectPath directly into
shell command strings creating injection/quoting issues; fix by constructing a
shell-escaped version of projectPath (escape single quotes by replacing ' with
'\'' and wrap the result in single quotes) before interpolating it into the
command templates in generateClaudeCodeHooksConfig so each command uses the
safely quoted path (reference generateClaudeCodeHooksConfig and the projectPath
property).
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/core/src/memory/claude-md-updater.ts`:
- Around line 343-397: The extractManualEntries function is currently capturing
auto-generated "### ..." blocks and causing duplicates when
combineWithManualEntries appends preserved entries; update extractManualEntries
to stop collecting when it encounters the SKILLKIT_MARKER or any heading that
contains "Auto-populated" (treat that as an explicit end of manual region) and
ignore/skip any entries whose first line matches the auto-populated heading
pattern, then ensure combineWithManualEntries still appends only the filtered
manualEntries array; reference extractManualEntries, combineWithManualEntries,
and SKILLKIT_MARKER when making the change.
In `@packages/core/src/memory/observation-store.ts`:
- Around line 37-44: The constructor sets compressionThreshold directly from
options allowing invalid values; ensure the constructor validates and normalizes
options.compressionThreshold using the same rules as setCompressionThreshold
(must be a positive integer) before assigning to this.compressionThreshold; call
or reuse the setCompressionThreshold method (or inline its validation logic)
inside the constructor to enforce the same checks for
ObservationStoreOptions.compressionThreshold while keeping other assignments
(projectPath, filePath, sessionId, autoCompress, onThresholdReached) unchanged.
In `@packages/core/src/memory/progressive-disclosure.ts`:
- Around line 199-207: The minRelevance option is declared but never used;
update code paths in progressive-disclosure.ts to pass the minRelevance value
into findRelevantIds and have findRelevantIds filter out any candidate ids whose
relevance score < minRelevance (or >= threshold semantics used elsewhere), then
use the filtered relevantIds in the logic that returns layer/entries/tokensUsed;
specifically modify the call sites that compute const relevantIds =
this.findRelevantIds(index, query) to include the minRelevance param (e.g.,
this.findRelevantIds(index, query, options.minRelevance)) and change
findRelevantIds to accept the minRelevance argument and apply the score
threshold when building its result so the index returned respects the configured
minimum relevance.
- Around line 95-107: Replace the use of the || fallback for maxResults with the
nullish coalescing operator so callers can request 0; specifically update
occurrences like options.maxResults || 50 to options.maxResults ?? 50 in
getIndex (and the other spots where options.maxResults is used), ensuring any
slices use the ?? default instead of ||.
- Around line 165-197: The smartRetrieve method must guard against non-positive
tokenBudget: before computing indexTokens or slicing index, add an early-return
when tokenBudget <= 0 that returns layer: 1, entries: [], tokensUsed: 0,
tokensRemaining: tokenBudget (or 0 if you prefer non-negative remaining) to
avoid negative slice sizes and bad token math; insert this check at the top of
smartRetrieve (immediately after obtaining index and before computing
indexTokens/TOKEN_ESTIMATES.index usage) so functions like tokenBudget,
TOKEN_ESTIMATES.index, and the index.slice logic are never given a non-positive
budget.
🧹 Nitpick comments (2)
packages/core/src/memory/hooks/post-tool-use.ts (1)
391-416: Consider edge case in solution matching.The
findMatchingSolutionmethod iterates overpendingErrorsand deletes entries while iterating (line 409). While this works in JavaScript/TypeScript Maps, it's worth noting that this means only the first matching error will be resolved per tool use, even if multiple pending errors might match. This may be intentional, but if a single action could resolve multiple related errors, they won't all be cleared.Additionally, the word matching at line 404 filters to words longer than 3 characters, which could miss short but significant error identifiers like "npm", "tsx", or "404".
packages/core/src/memory/hooks/session-end.ts (1)
120-154:forceCompressandpreviewdon't setprojectNamefor compressor.The
execute()method derivesprojectNamefromcontext.project_path(lines 65-67) and passes it toMemoryCompressor, butforceCompress()(lines 133-135) andpreview()(lines 173-175) don't. This could result in learnings being stored/previewed without proper project scoping.Consider deriving
projectNamefromthis.projectPathfor consistency:🔧 Proposed fix for forceCompress
const compressor = new MemoryCompressor(this.projectPath, { scope: 'project', + projectName: basename(this.projectPath.replace(/\/+$/, '')) || undefined, });
| private extractManualEntries(sectionContent: string): string[] { | ||
| const lines = sectionContent.split('\n'); | ||
| const manualEntries: string[] = []; | ||
| let inManualEntry = false; | ||
| let currentEntry: string[] = []; | ||
|
|
||
| for (const line of lines) { | ||
| if (line.includes(SKILLKIT_MARKER)) { | ||
| inManualEntry = false; | ||
| continue; | ||
| } | ||
|
|
||
| if (line.startsWith('### ') && !line.includes('Auto-populated')) { | ||
| if (currentEntry.length > 0) { | ||
| manualEntries.push(currentEntry.join('\n')); | ||
| } | ||
| inManualEntry = true; | ||
| currentEntry = [line]; | ||
| } else if (inManualEntry) { | ||
| currentEntry.push(line); | ||
| } | ||
| } | ||
|
|
||
| if (currentEntry.length > 0) { | ||
| manualEntries.push(currentEntry.join('\n')); | ||
| } | ||
|
|
||
| return manualEntries.filter((e) => e.trim().length > 0); | ||
| } | ||
|
|
||
| private combineWithManualEntries(autoSection: string, manualEntries: string[]): string { | ||
| if (manualEntries.length === 0) { | ||
| return autoSection; | ||
| } | ||
|
|
||
| const lines = autoSection.split('\n'); | ||
| const combinedLines = [...lines]; | ||
|
|
||
| combinedLines.push(''); | ||
| combinedLines.push('### Manual Entries'); | ||
| combinedLines.push('<!-- Preserved from previous edits -->'); | ||
| combinedLines.push(''); | ||
|
|
||
| for (const entry of manualEntries) { | ||
| combinedLines.push(entry); | ||
| combinedLines.push(''); | ||
| } | ||
|
|
||
| return combinedLines.join('\n'); | ||
| } | ||
|
|
||
| private countLearnings(content: string): number { | ||
| const matches = content.match(/^\*\*[^*]+\*\*/gm); | ||
| return matches ? matches.length : 0; | ||
| } |
There was a problem hiding this comment.
PreserveManualEntries duplicates auto content across updates.
extractManualEntries() re-captures auto-generated “### …” blocks after the SKILLKIT marker, so combineWithManualEntries() appends them again. With preserveManualEntries: true, each update grows CLAUDE.md and duplicates auto content.
✅ Suggested fix: add an end marker and ignore auto-populated blocks
-const SKILLKIT_MARKER = '<!-- Auto-populated by SkillKit -->';
+const SKILLKIT_MARKER = '<!-- Auto-populated by SkillKit -->';
+const SKILLKIT_END_MARKER = '<!-- End auto-populated by SkillKit -->';
@@
for (const [category, categoryLearnings] of byCategory) {
if (categoryLearnings.length > 0) {
lines.push(`### ${category}`);
for (const learning of categoryLearnings) {
const title = this.formatLearningTitle(learning);
const summary = this.extractSummary(learning.content);
lines.push(`${title}`);
lines.push(summary);
lines.push('');
}
}
}
+
+ lines.push(SKILLKIT_END_MARKER);
return lines.join('\n');
}
@@
const lines = sectionContent.split('\n');
const manualEntries: string[] = [];
let inManualEntry = false;
+ let inAutoSection = false;
let currentEntry: string[] = [];
for (const line of lines) {
if (line.includes(SKILLKIT_MARKER)) {
+ if (currentEntry.length > 0) {
+ manualEntries.push(currentEntry.join('\n'));
+ currentEntry = [];
+ }
inManualEntry = false;
+ inAutoSection = true;
continue;
}
+ if (line.includes(SKILLKIT_END_MARKER)) {
+ inAutoSection = false;
+ continue;
+ }
+ if (inAutoSection) {
+ continue;
+ }
if (line.startsWith('### ') && !line.includes('Auto-populated')) {
if (currentEntry.length > 0) {
manualEntries.push(currentEntry.join('\n'));
}
inManualEntry = true;
currentEntry = [line];
} else if (inManualEntry) {
currentEntry.push(line);
}
}🤖 Prompt for AI Agents
In `@packages/core/src/memory/claude-md-updater.ts` around lines 343 - 397, The
extractManualEntries function is currently capturing auto-generated "### ..."
blocks and causing duplicates when combineWithManualEntries appends preserved
entries; update extractManualEntries to stop collecting when it encounters the
SKILLKIT_MARKER or any heading that contains "Auto-populated" (treat that as an
explicit end of manual region) and ignore/skip any entries whose first line
matches the auto-populated heading pattern, then ensure combineWithManualEntries
still appends only the filtered manualEntries array; reference
extractManualEntries, combineWithManualEntries, and SKILLKIT_MARKER when making
the change.
| constructor(projectPath: string, sessionId?: string, options: ObservationStoreOptions = {}) { | ||
| this.projectPath = projectPath; | ||
| this.filePath = join(projectPath, '.skillkit', 'memory', 'observations.yaml'); | ||
| this.sessionId = sessionId || randomUUID(); | ||
| this.compressionThreshold = options.compressionThreshold ?? 50; | ||
| this.autoCompress = options.autoCompress ?? true; | ||
| this.onThresholdReached = options.onThresholdReached; | ||
| } |
There was a problem hiding this comment.
Constructor accepts invalid compressionThreshold values.
The constructor accepts any value for compressionThreshold via options (line 41), but setCompressionThreshold() validates that it must be a positive integer (lines 153-155). This inconsistency allows invalid thresholds to be set during construction.
Consider validating the threshold in the constructor as well:
🔧 Proposed fix
constructor(projectPath: string, sessionId?: string, options: ObservationStoreOptions = {}) {
this.projectPath = projectPath;
this.filePath = join(projectPath, '.skillkit', 'memory', 'observations.yaml');
this.sessionId = sessionId || randomUUID();
- this.compressionThreshold = options.compressionThreshold ?? 50;
+ const threshold = options.compressionThreshold ?? 50;
+ if (threshold < 1 || !Number.isInteger(threshold)) {
+ throw new Error('Compression threshold must be a positive integer');
+ }
+ this.compressionThreshold = threshold;
this.autoCompress = options.autoCompress ?? true;
this.onThresholdReached = options.onThresholdReached;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(projectPath: string, sessionId?: string, options: ObservationStoreOptions = {}) { | |
| this.projectPath = projectPath; | |
| this.filePath = join(projectPath, '.skillkit', 'memory', 'observations.yaml'); | |
| this.sessionId = sessionId || randomUUID(); | |
| this.compressionThreshold = options.compressionThreshold ?? 50; | |
| this.autoCompress = options.autoCompress ?? true; | |
| this.onThresholdReached = options.onThresholdReached; | |
| } | |
| constructor(projectPath: string, sessionId?: string, options: ObservationStoreOptions = {}) { | |
| this.projectPath = projectPath; | |
| this.filePath = join(projectPath, '.skillkit', 'memory', 'observations.yaml'); | |
| this.sessionId = sessionId || randomUUID(); | |
| const threshold = options.compressionThreshold ?? 50; | |
| if (threshold < 1 || !Number.isInteger(threshold)) { | |
| throw new Error('Compression threshold must be a positive integer'); | |
| } | |
| this.compressionThreshold = threshold; | |
| this.autoCompress = options.autoCompress ?? true; | |
| this.onThresholdReached = options.onThresholdReached; | |
| } |
🤖 Prompt for AI Agents
In `@packages/core/src/memory/observation-store.ts` around lines 37 - 44, The
constructor sets compressionThreshold directly from options allowing invalid
values; ensure the constructor validates and normalizes
options.compressionThreshold using the same rules as setCompressionThreshold
(must be a positive integer) before assigning to this.compressionThreshold; call
or reuse the setCompressionThreshold method (or inline its validation logic)
inside the constructor to enforce the same checks for
ObservationStoreOptions.compressionThreshold while keeping other assignments
(projectPath, filePath, sessionId, autoCompress, onThresholdReached) unchanged.
- Use nullish coalescing (??) for maxResults defaults in progressive-disclosure - Guard against non-positive tokenBudget in smartRetrieve() - Thread minRelevance option through to findRelevantIds() - Fix memory index CLI to validate --limit and pass maxResults to getIndex()
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| private async checkAutoCompression(): Promise<void> { | ||
| if (!this.config.enabled) return; | ||
|
|
||
| const count = this.postToolUseHook.getObservationCount(); | ||
| if (count >= this.config.compressionThreshold) { | ||
| await this.sessionEndHook.forceCompress(this.sessionId); | ||
| } |
There was a problem hiding this comment.
🔴 Stale ObservationStore cache causes deleted observations to reappear after auto-compression
When MemoryHookManager.checkAutoCompression() triggers compression, it calls sessionEndHook.forceCompress() which creates a new ObservationStore instance and deletes processed observations from the file. However, the PostToolUseHook maintains its own ObservationStore instance with cached in-memory data that never gets invalidated.
Root Cause and Impact
The ObservationStore caches data in memory via this.data (observation-store.ts:30). When load() is called, it returns the cached data without re-reading the file (observation-store.ts:53-54):
private load(): ObservationStoreData {
if (this.data) return this.data; // Returns stale cache!The flow that causes the bug:
PostToolUseHookcreatesthis.store = new ObservationStore(projectPath, sessionId)(post-tool-use.ts:70)- Observations are added and cached in
this.store.data checkAutoCompression()callssessionEndHook.forceCompress(this.sessionId)(manager.ts:241)forceCompresscreates a new store:const store = new ObservationStore(this.projectPath, sessionId)(session-end.ts:121)- This new store deletes observations and saves to file (
session-end.ts:144-145) PostToolUseHook.storestill has stale cached observations- On the next
store.add(), it callsload()which returns the stale cache, appends new observation, thensave()writes all stale observations back to the file
Impact: Deleted observations reappear after compression, causing data corruption and defeating the purpose of compression.
Prompt for agents
Fix the stale cache issue in MemoryHookManager by ensuring the PostToolUseHook's ObservationStore cache is invalidated after compression. Options:
1. Add a method to ObservationStore like `invalidateCache()` that sets `this.data = null`, and expose a way for SessionEndHook to notify PostToolUseHook after compression.
2. Have PostToolUseHook use the same ObservationStore instance as SessionEndHook by sharing the store reference.
3. Add a `reload()` method to ObservationStore that clears the cache and re-reads from disk, then call it from PostToolUseHook after checkAutoCompression completes.
The simplest fix would be to add an `invalidateCache()` method to ObservationStore and call `this.postToolUseHook.getStore().invalidateCache()` in checkAutoCompression after forceCompress completes (around line 241 in manager.ts).
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
memory sync-claudeandmemory indexSummary by CodeRabbit
New Features
Documentation