feat: implement 5 claude-mem gap features (privacy, reaper, bun-path, relevance, progressive disclosure)#3
Conversation
…port Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…tions Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
📝 WalkthroughWalkthroughRemoves per-date recentActivity from dashboard stats; adds relevance scoring and sorting for progressive context; introduces Bun path resolution with caching; implements orphan daemon reaper; refactors privacy redaction into utilities; updates HTTP server dashboard fields and allows injected dashboardDir; adds comprehensive tests and bumps package version. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/context/relevance.ts`:
- Around line 15-22: Remove the unused recentFileContext property from the
ScoringContext interface: edit the ScoringContext declaration to drop
recentFileContext?: string[] (leaving currentSessionId?: string and now: Date),
update any type references or imports that mentioned ScoringContext if they
assumed that field, run/type-check tests to ensure no usages remain, and remove
any related comments/docs or tests referencing recentFileContext so the codebase
no longer declares an unused scoring field.
In `@src/daemon/reaper.ts`:
- Around line 50-52: The reaper currently calls removePid(pidPath) which
swallows unlinkSync errors, so result.reaped is incremented and a success log is
emitted even if deletion failed; change the logic in reaper.ts to either call
fs.unlinkSync(pidPath) directly inside the existing try/catch so any exception
is caught and pushed into result.errors before deciding to increment
result.reaped and log, or—if you keep removePid—immediately verify removal with
fs.existsSync(pidPath) and only increment result.reaped and log when the file no
longer exists; make sure to reference removePid, pidPath, result.reaped and
update the catch to record the error (e.g., push to result.errors) rather than
silently ignoring failures.
In `@src/servers/http-server.ts`:
- Around line 267-268: The dashboardDir assignment uses new URL(...).pathname
which can produce URL-encoded segments; change the logic that computes
dashboardDir (the dashboardDir variable initialization and the fallback new URL
call) to convert the file URL to a proper filesystem path using a URL-to-path
helper (e.g., import and use fileURLToPath from 'node:url' or Bun.fileURLToPath)
so that injectedDashboardDir remains unchanged but the fallback uses
fileURLToPath(new URL(..., import.meta.url)); ensure references to
injectedDashboardDir and dashboardDir remain the same.
In `@src/utils/privacy.ts`:
- Around line 20-25: The loop compiles user/operator-supplied patterns into
RegExp directly (patterns and new RegExp(...) in src/utils/privacy.ts), which
risks ReDoS; validate and sanitize each pattern before compiling: enforce a max
pattern length (e.g., <= 200), reject patterns that contain nested/adjacent
quantifiers or common catastrophic constructs (heuristic check for sequences
like (.+)+, (a+)+, [^)]*+, backreferences like \1, or nested groups with +/*/?)
and skip or log them, and only then call new RegExp(pattern, "g") and run
result.replace; additionally mark these patterns come from
config.sensitivePatterns so validation occurs when reading that config to
fail-fast.
In `@tests/context/relevance.test.ts`:
- Around line 22-33: The helper makeEntry currently returns an ObservationIndex
but omits the required importance property; update makeEntry to include a
default importance:number (e.g., importance: 0 or another sensible default) in
the returned object and ensure any test call sites that rely on overrides either
pass importance or rely on the new default; locate the function makeEntry in
tests/context/relevance.test.ts and add the importance field to the returned
object so it matches the ObservationIndex type.
In `@tests/utils/bun-path.test.ts`:
- Line 40: The test currently sets process.env.BUN_INSTALL via
"process.env.BUN_INSTALL = undefined as unknown as string", which creates the
literal string "undefined" and breaks getCandidatePaths(); change these to
actually remove the env var using delete process.env.BUN_INSTALL (apply the same
fix for the other occurrences noted around the test at the locations
corresponding to lines 55 and 80) so getCandidatePaths() sees the variable as
unset rather than the string "undefined".
- Around line 34-58: The test manipulates global state (Bun.which,
process.env.BUN_INSTALL, and process.env.PATH) but currently restores
process.env.PATH outside the finally, which can leak an empty PATH on assertion
failure; move the saved originalPath and its restoration into the finally block
alongside restoring (Bun as { which: typeof Bun.which }).which = originalWhich
and process.env.BUN_INSTALL to ensure resolveBunPath test fully cleans up even
when assertions fail (remove the out-of-finally PATH restore).
🧹 Nitpick comments (5)
tests/daemon/reaper.test.ts (1)
19-29: Prefer the static import over dynamicrequireforunlinkSync.Line 22 uses
require("node:fs")to getunlinkSync, butexistsSync,mkdirSync, andwriteFileSyncare already statically imported on Line 7. AddunlinkSyncto that import and use it directly here.Also,
tmpDir()directories are never cleaned up. Consider usingrmSync(dir, { recursive: true })inafterEachfor a tidier teardown.Proposed cleanup
-import { existsSync, mkdirSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, rmSync, unlinkSync, writeFileSync } from "node:fs";+let cleanupDirs: string[] = []; + function tmpDir(): string { const dir = `/tmp/open-mem-reaper-test-${randomUUID()}`; mkdirSync(dir, { recursive: true }); + cleanupDirs.push(dir); return dir; }afterEach(() => { for (const p of cleanupPaths) { try { - const { unlinkSync } = require("node:fs"); unlinkSync(p); } catch { // file may not exist } } cleanupPaths = []; + for (const d of cleanupDirs) { + try { + rmSync(d, { recursive: true }); + } catch { + // dir may not exist + } + } + cleanupDirs = []; });src/servers/http-server.ts (1)
278-283: Consider logging swallowed errors in the emptycatchblocks.Both catch blocks silently discard all errors, including unexpected ones (e.g., permission denied). At minimum, logging at
debug/warnlevel would aid troubleshooting without affecting the fallback flow.Also applies to: 290-297
src/utils/bun-path.ts (1)
47-60: Consider logging which path was resolved for debuggability.When the daemon fails to start, knowing which Bun binary was resolved (and how —
Bun.which, candidate scan, or bare fallback) would be valuable for troubleshooting. A debug-level log here would help operators diagnose path issues without adding runtime overhead.src/context/relevance.ts (2)
64-83: Docstring says "exponential decay" but implementation is a step function.The comment on line 66 says "Compute recency score using exponential decay" but the implementation uses discrete buckets (1.0 / 0.8 / 0.5 / 0.2). Consider updating the doc to say "discrete buckets" or "step function" to avoid confusion.
164-175:scoreObservationis recomputed on every comparison during sort.
Array.sortcalls the comparator O(n log n) times, and each call invokesscoreObservationfor bothaandb(includingnew Date()parsing in the sub-scores). For large observation sets, pre-computing scores into aMapand looking them up in the comparator would be significantly more efficient.♻️ Pre-compute scores before sorting
export function sortByRelevance( entries: ReadonlyArray<ObservationIndex>, context: ScoringContext, ): ObservationIndex[] { + const scores = new Map<string, number>(); + for (const entry of entries) { + scores.set(entry.id, scoreObservation(entry, context)); + } return [...entries].sort((a, b) => { - const scoreA = scoreObservation(a, context); - const scoreB = scoreObservation(b, context); + const scoreA = scores.get(a.id)!; + const scoreB = scores.get(b.id)!; // Descending — higher score first if (scoreB !== scoreA) return scoreB - scoreA; // Tie-break: more recent first return new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime(); }); }
Greptile OverviewGreptile SummaryImplements 5 feature enhancements bridging gaps identified from claude-mem comparison. The changes are well-architected with comprehensive test coverage (+64 new tests, 509 total passing). Major Features:
Minor Fixes:
All implementations follow the existing codebase patterns with proper error handling, edge case coverage, and clean separation of concerns. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Plugin as Plugin Init
participant Reaper as Orphan Reaper
participant Privacy as Privacy Utils
participant ToolHook as Tool Capture
participant ChatHook as Chat Capture
participant BunPath as Bun Path Resolver
participant Daemon as Daemon Manager
participant Context as Context Builder
participant Relevance as Relevance Scorer
Plugin->>Reaper: reapOrphanDaemons(dbPath)
Reaper->>Reaper: Check PID file
alt PID file exists and process dead
Reaper->>Reaper: Remove stale PID
end
Reaper-->>Plugin: ReapResult
Plugin->>BunPath: resolveBunPathCached()
BunPath->>BunPath: Try Bun.which("bun")
alt Bun.which fails
BunPath->>BunPath: Scan common install paths
end
BunPath-->>Plugin: Bun executable path
Plugin->>Daemon: start(bunPath)
Daemon->>Daemon: Spawn daemon with resolved path
Note over ToolHook,ChatHook: Runtime - Tool & Chat Capture
ToolHook->>Privacy: stripPrivateBlocks(output)
Privacy-->>ToolHook: Sanitized output
ToolHook->>Privacy: redactSensitive(output, patterns)
Privacy-->>ToolHook: Redacted output
ChatHook->>Privacy: stripPrivateBlocks(message)
Privacy-->>ChatHook: Sanitized message
ChatHook->>Privacy: redactSensitive(message, patterns)
Privacy-->>ChatHook: Redacted message
Note over Context,Relevance: Context Injection - Progressive Disclosure
Context->>Relevance: sortByRelevance(observations, scoringContext)
Relevance->>Relevance: Calculate weighted scores<br/>(recency 40%, type 30%,<br/>session 20%, efficiency 10%)
Relevance-->>Context: Sorted observations
Context->>Context: Apply token budget with<br/>3-layer disclosure guidance
Context-->>Plugin: Context string with prioritized observations
|
- Remove unused recentFileContext from ScoringContext interface - Fix docstring: 'exponential decay' -> 'step function' in scoreRecency - Pre-compute scores in sortByRelevance with Map for efficiency - Verify PID removal succeeded with existsSync in reaper - Use fileURLToPath instead of .pathname for dashboard dir resolution - Add ReDoS protection with max length and nested quantifier checks - Add importance field to test helper makeEntry - Move PATH restoration into finally block in bun-path tests - Use delete instead of = undefined for env var cleanup Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
- Use static import for unlinkSync in reaper tests (replace require()) - Add temp directory cleanup in reaper test afterEach - Log swallowed errors in http-server catch blocks - Add debug log for bun path resolved via candidate scan Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Summary
Implements 5 features identified as gaps from the claude-mem comparison:
Features
stripPrivateBlocks+redactSensitiveutility used by both tool-capture and chat-capture hooks. Chat messages now get private blocks stripped and sensitive content redacted before storage.Bun.which()→ common install paths (~/.bun/bin/bun,/usr/local/bin/bun,/opt/homebrew/bin/bun) → fallback. Cached for process lifetime.Also includes
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests