Conversation
Move skill service factories (MemoryService, TaskService, PromptService) from skills/shared to infrastructure layer. This eliminates duplicate code between skills and infrastructure while maintaining backwards compatibility through re-exports. Changes: - Create shared GitMemFactory with lazy singleton pattern - Move skill services to infrastructure/services/ - Move skill interfaces to infrastructure/services/skill-interfaces/ - Update bootstrap.ts to use shared git-mem instance - Re-export from skills/shared for backwards compatibility - Delete 6 duplicate files (-1341 lines) Skills entry points continue to work unchanged via re-exports. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…SA-59) Remove the obsolete group ID system that was used for Neo4j/Zep to scope memories by project directory. With git-mem, the git repository itself provides the scoping via refs/notes/mem. Changes: - Remove group tagging from all memory/task write operations - Remove group filtering from all memory/task read operations - Delete group-id.ts utility files (~350 lines) - Simplify all service interfaces by removing groupId parameters - Update all callers and tests The removal affects: - Domain interfaces (IMemoryService, ITaskService, etc.) - Infrastructure services (GitMemMemoryService, SkillMemoryService, etc.) - Application handlers and services - All related unit tests Backward compatible: existing memories with group: tags remain stored but are no longer filtered by them. 60 files changed, -1076 lines net Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR removes group-based scoping throughout the application, replacing it with git-mem repository-based scoping. Removes groupId parameters from core service interfaces, updates implementations and call sites, deletes group-ID utility modules, and introduces new git-mem adapters and skill-specific service interfaces. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a555ec4ffb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/lib/skills/review/ai-enrich.ts
Outdated
| const config = loadConfig(); | ||
| const groupId = getCurrentGroupId(projectRoot); | ||
| // Note: groupId is deprecated but kept for backwards compatibility with MCP protocol | ||
| const groupId = path.basename(projectRoot); |
There was a problem hiding this comment.
groupId is now derived with path.basename(projectRoot) and then passed into addEnrichedMemory(...), which forwards it as group_id in the MCP add_memory call; this skips the path-to-group normalization used by the init-review flow (InitReviewService.normalizePathToGroupId) and can break enrichment persistence for repos whose folder names contain unsupported characters (for example dots or spaces), or write to a different group than the rest of init-review metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f2a3ccb — removed \ parameter from \ and the \ field from the MCP request body. Group IDs are no longer used; scoping is handled by git-mem via git notes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (19)
tests/unit/src/lib/commands/doctor.test.ts (1)
274-287: 🧹 Nitpick | 🔵 TrivialRename test to follow project naming convention.
This test uses the old
method_givenCondition_shouldExpectedOutcomenaming style. Per project convention, tests should use theshould ...pattern (e.g.,should derive group from folder path when GROUP env is not set).✏️ Suggested rename
- it('runDoctor_givenNoGroupEnv_shouldDeriveGroupFromFolderPath', async () => { + it('should derive group from folder path when GROUP env is not set', async () => {Based on learnings: "Enforce the project-wide test naming convention: name tests using the 'should ...' pattern instead of the 'method_givenCondition_shouldExpectedOutcome' style."
src/lib/skills/memory/memory.ts (1)
6-14: 🧹 Nitpick | 🔵 TrivialConsider updating CLI documentation.
The header comments still reference
--group <id>flags for multiple commands. Since group IDs are no longer used for scoping, consider either:
- Removing the
--groupflag documentation if deprecated- Clarifying its new purpose (if any) in a backwards-compatibility note
src/lib/commands/doctor.ts (2)
546-558: 🧹 Nitpick | 🔵 TrivialRemove duplicate
getProjectNamecall.
getProjectName(cwd)is called twice: once at line 548 (assigned to unused_projectName) and again at line 558. Remove the redundant call.♻️ Proposed fix
const startTime = Date.now(); const cwd = opts.cwd; - const _projectName = getProjectName(cwd); // Reserved for future use // Load configuration const config = await loadConfig(cwd); const mode = config?.mode || 'local'; const endpoint = opts.endpoint || config?.endpoint || (mode === 'zep-cloud' ? ZEP_CLOUD_ENDPOINT : DEFAULT_ENDPOINT); // Group ID is derived from folder name for backwards compatibility - const group = getProjectName(cwd); + const group = getProjectName(cwd); // For backwards compatibility
140-153: 🧹 Nitpick | 🔵 TrivialConsider reusing
getProjectNamefrom shared/constants.This function duplicates the implementation in
src/lib/commands/shared/constants.ts:27-42. Consider importing from the shared module to maintain DRY principles.♻️ Proposed refactor
+import { getProjectName } from './shared/constants'; + // ... remove local getProjectName function (lines 140-153) - -/** - * Get project name from package.json or directory name. - */ -function getProjectName(cwd: string): string { - try { - const pkgPath = path.join(cwd, 'package.json'); - if (fs.existsSync(pkgPath)) { - const pkg = fs.readJsonSync(pkgPath); - if (pkg.name) { - return pkg.name.replace(/^@[^/]+\//, ''); - } - } - } catch { - // Ignore - } - return path.basename(cwd); -}Note: The shared function uses
process.cwd()instead of accepting acwdparameter. You may need to either:
- Update the shared function to accept an optional
cwdparameter- Keep the local function but add a comment referencing the shared implementation
src/lib/skills/github/github.ts (1)
425-453: 🧹 Nitpick | 🔵 TrivialConsider making
groupIdoptional inISyncOptionsor marking it deprecated.While setting
groupId = ''for interface compatibility works, theISyncOptionsinterface (inGitHubSyncService.tsline 53) still requiresgroupId: string. This forces callers to provide a meaningless value. Consider:
- Making it optional:
groupId?: string- Adding a JSDoc
@deprecatedannotationThis would make the API cleaner and signal the architectural change to consumers.
src/lib/skills/shared/services/GitHubSyncService.ts (1)
43-58: 🧹 Nitpick | 🔵 TrivialConsider making
groupIdoptional inISyncOptions.The
groupIdfield at line 53 is documented as ignored (per the note at lines 14-17), but it's still a required field. This forces callers to provide a meaningless value (e.g.,''). Making it optional would better reflect the actual contract:♻️ Proposed change to ISyncOptions
export interface ISyncOptions { /** Repository in owner/repo format */ repo: string; /** Preview changes without applying */ dryRun?: boolean; /** Filter GitHub issues by labels */ labels?: string[]; /** Sync direction */ direction: SyncDirection; - /** Group ID for Lisa tasks */ - groupId: string; + /** + * Group ID for Lisa tasks. + * `@deprecated` Group IDs are no longer used - the git repo provides scoping via git-mem. + */ + groupId?: string; /** Default repo for tasks */ defaultRepo?: string; /** Default assignee for tasks */ defaultAssignee?: string; }src/lib/infrastructure/services/RecursionService.ts (1)
250-254: 🧹 Nitpick | 🔵 TrivialRemove the unused
getTagmethod.The
getTagmethod (lines 250-254) is not called anywhere in the codebase and can be safely removed to eliminate dead code.src/lib/infrastructure/services/CurationService.ts (1)
83-106:⚠️ Potential issue | 🟠 MajorValidate
uuidbefore writing__curate:${uuid}/__promote:${uuid}keysSince
uuidnow directly influences the synthetic memory keys (Line 94, Line 104), add a minimal guard to avoid writing malformed keys (empty string, whitespace, unexpected separators). This prevents accidental “tag facts” from being created with ambiguous identifiers and reduces footguns in CLI/handler layers.Proposed guard (minimal, local)
async markFact( uuid: string, mark: CurationMark ): Promise<void> { + const normalizedUuid = uuid.trim(); + if (!normalizedUuid) { + throw new Error('Cannot curate fact: uuid is empty'); + } const curationTag = resolveCurationTag(mark); // Add the curation tag to the fact - await memoryWriter.addFact(`__curate:${uuid}`, [curationTag]); + await memoryWriter.addFact(`__curate:${normalizedUuid}`, [curationTag]); // Side effects by mark if (mark === 'deprecated') { - await memoryWriter.expireFact(uuid); + await memoryWriter.expireFact(normalizedUuid); } if (mark === 'authoritative') { // Promote confidence to verified const confidenceTag = resolveConfidenceTag('verified'); - await memoryWriter.addFact(`__promote:${uuid}`, [confidenceTag]); + await memoryWriter.addFact(`__promote:${normalizedUuid}`, [confidenceTag]); } },As per coding guidelines, “Always handle potentially undefined/null values explicitly.”
src/lib/application/handlers/SessionStopHandler.ts (2)
98-110:⚠️ Potential issue | 🟡 MinorConsider batching memory writes to avoid partial progress ambiguity
At Line 106 you write facts sequentially. If the Nth write fails, earlier facts were already persisted but the handler throws (tests indicate this is desired). If you want “all-or-nothing” semantics, you’ll need either:
- a bulk/transactional memory API, or
- a staged write + rollback strategy (if supported).
If the current semantics are intentionally “best effort until first failure,” consider documenting that in the handler JSDoc so callers know partial persistence is possible.
131-171:⚠️ Potential issue | 🟡 MinorLogging
erroras unknown object may be noisy/inconsistentIn the catch at Line 167-170 you log
{ error }whereerrorisunknown. Prefer normalizing to{ message, name }(and optionallystackbehind a debug flag) to keep logs stable.Proposed normalization
} catch (error) { // Don't fail session stop if suggestions fail - this.logger?.debug?.('Failed to generate suggestions', { error }); + const err = error instanceof Error ? error : new Error(String(error)); + this.logger?.debug?.('Failed to generate suggestions', { + error: err.message, + name: err.name, + }); }As per coding guidelines, “Never use
anytype - useunknownor specific interfaces instead.”src/lib/skills/tasks/tasks.ts (1)
33-46:⚠️ Potential issue | 🟠 MajorBug risk: popFlag will “eat” the next flag if user passes
--groupwithout a valueBecause
popFlagalways consumes the next token (even if it’s another flag like--limit), this can break parsing now that--groupis ignored. Example:tasks list --group --limit 10will likely remove--limitas the “group value”.Suggestion: consume only the flag, and consume a value only if the next token exists and does not start with
-.Proposed local fix (no shared util changes)
const command = args.shift() ?? ''; // --group flag is ignored but still consumed for backwards compatibility - popFlag(args, '--group', null); + const groupIdx = args.indexOf('--group'); + if (groupIdx !== -1) { + // Remove the flag itself + args.splice(groupIdx, 1); + // Remove value only if it looks like a value (not another flag) + if (args[groupIdx] && !String(args[groupIdx]).startsWith('-')) { + args.splice(groupIdx, 1); + } + }As per coding guidelines, “Always handle potentially undefined/null values explicitly.”
tests/unit/src/lib/application/handlers/SessionStopHandler.test.ts (2)
85-110:⚠️ Potential issue | 🟡 MinorMock task service updates look consistent; keep status defaults aligned with production
The changes at Line 97-108 look fine for a unit-test mock. Minor suggestion: consider using the same default status as the real implementation (if it differs) to reduce “mock drift”.
225-266: 🧹 Nitpick | 🔵 TrivialNice: updated addFactWithLifecycle capture without groupId; consider typing savedOptions more precisely
At Line 230-233 you capture
optionsasunknown[]and later cast. Similar to the other test file, you can tighten this usingParameters<IMemoryService['addFactWithLifecycle']>[1]to avoid repeated casting.As per coding guidelines, “Never use
anytype - useunknownor specific interfaces instead.”src/lib/application/services/GitIndexingService.ts (1)
161-205:⚠️ Potential issue | 🟡 MinorFix type inconsistency: use
IMemoryItem[]for existingFacts instead of manually narrowed shapeThe
addFactWithLifecycleoptions parameter is correctly typed asIMemorySaveOptionsat line 184 (✓ no issue there). However,existingFactsis explicitly declared as{ fact?: string; text?: string }[]whilesearchFacts()returnsPromise<IMemoryItem[]>. This narrow type declaration silently discardsuuid,name,tags, andcreated_atfrom the actual return shape, violating the guideline to use specific interfaces instead of ad-hoc object shapes. Replace the explicit type narrowing with the concreteIMemoryItem[]type, or—if the narrowing is intentional—add a code comment explaining why and explicitly map/filter the searchFacts result to justify the shape change. Avoid line 191's manual{ fact: fact.text }construction; use the full IMemoryItem objects returned from searchFacts to maintain type consistency.src/lib/commands/knowledge.ts (1)
21-21: 🧹 Nitpick | 🔵 TrivialConsider adding deprecation notice for
--groupoption.The
--groupoption is still accepted throughout the CLI commands but is no longer functional for scoping since group-based filtering was removed. Consider updating the option descriptions to indicate deprecation, or removing the options entirely in a follow-up to avoid user confusion.src/lib/application/handlers/SessionStartHandler.ts (1)
138-168:⚠️ Potential issue | 🟡 MinorRemove unused
hierarchicalGroupIdsandgroupIdfromsyncGitHubOnStartup.The
groupIdpassed togithubSync.sync()is not used—GitHubSyncService explicitly documents that "Group IDs are no longer used" and thegroupIdinISyncOptionsis "kept for interface compatibility but ignored." Remove thehierarchicalGroupIdsparameter and thegroupIdderivation from this method since they represent dead code.tests/unit/src/lib/skills/github/github.test.ts (1)
107-203:⚠️ Potential issue | 🟡 MinorAlign test case titles with the “should …” naming convention.
Rename the three test titles to the project’s “should …” style (e.g., “should add a task when no linked task exists”).
Based on learnings: “Enforce the project-wide test naming convention: name tests using the 'should ...' pattern (e.g., 'should return correct value when condition met') instead of the 'method_givenCondition_shouldExpectedOutcome' style.”
src/lib/infrastructure/services/SkillTaskService.ts (1)
175-184:⚠️ Potential issue | 🟠 MajorDelete only the newest matching task to prevent data loss with duplicate titles.
After group IDs were removed, duplicate task titles can now exist in the same memory store. The current code deletes every task with a matching title, which would remove unrelated tasks. Since
recall()returns memories newest-first, usefind()to delete only the most recent version:🔧 Suggested fix
- const existing = memories.filter(m => m.tags.includes('task')); - - for (const m of existing) { - const old = parseTaskContent(m.content); - if (old && old.title === title) { - gitMem.delete(m.id); - } - } + const existing = memories.find( + m => m.tags.includes('task') && parseTaskContent(m.content)?.title === title + ); + + if (existing) { + gitMem.delete(existing.id); + }src/lib/infrastructure/services/GitMemTaskService.ts (1)
101-129:⚠️ Potential issue | 🟠 MajorHard-coded recall limit of 200 truncates tasks, affecting count accuracy.
getTaskCounts()depends entirely ongetTasksSimple(), which applies a fixed 200-item limit. If more than 200 tasks exist, counts will be incomplete and inaccurate. This is inconsistent withSkillTaskService.ts, which uses a limit of 500. Either align the limits, make the limit configurable as a dependency, or fetch all tasks without a limit before deduplication.
🤖 Fix all issues with AI agents
In `@src/lib/infrastructure/di/bootstrap.ts`:
- Around line 59-61: The git-mem singleton created by getGitMemInstance()
ignores projectRoot and is never cleared; update the cleanup to call
resetGitMemInstance() inside dispose() (in addition to container.dispose()) to
clear NotesService/MemoryRepository state between runs, or refactor
getGitMemInstance to accept projectRoot and include that in its singleton cache
key so each project gets its own instance; ensure references to
getGitMemInstance(), resetGitMemInstance(), and dispose() are updated
accordingly and that NotesService/MemoryRepository are instantiated with the
configured projectRoot.
In `@src/lib/infrastructure/services/skill-interfaces/ISkillPromptService.ts`:
- Around line 12-50: The IPromptResult interface still exposes the retired group
field; mark it as deprecated (or remove it) so the interface matches current
semantics: update the IPromptResult declaration to either remove group?: string
or annotate it with a JSDoc `@deprecated` tag and a short note that group IDs are
retired, and ensure any consumers of ISkillPromptService.addPrompt are updated
to stop relying on IPromptResult.group; reference IPromptResult and the
addPrompt signature to locate and change the field and its usages.
In
`@tests/unit/src/lib/application/handlers/SessionStopHandler.quality-tags.test.ts`:
- Around line 72-77: Define a single type alias for the addFactWithLifecycle
options (e.g., type AddFactWithLifecycleOptions = { lifecycle: string; tags:
string[] }) and use it for savedOptions (const savedOptions:
AddFactWithLifecycleOptions[] = []) and to type the mock handler parameter
(addFactWithLifecycle: async (_fact, options: AddFactWithLifecycleOptions) => {
savedOptions.push(options); }). Apply the same pattern for the other repeated
hunks so you stop using repeated casts and keep the test aligned with the real
method signature.
| // git-mem factory (shared singleton) | ||
| import { getGitMemInstance } from '../git-mem'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the git-mem factory
find src/lib/infrastructure -name "*git-mem*" -type f | head -20Repository: TonyCasey/lisa
Length of output: 40
🏁 Script executed:
# Look at bootstrap.ts around the critical lines mentioned
cat -n src/lib/infrastructure/di/bootstrap.ts | sed -n '55,70p'Repository: TonyCasey/lisa
Length of output: 710
🏁 Script executed:
# Search for getGitMemInstance implementation and usage
rg -n "getGitMemInstance|resetGitMemInstance" src/lib/infrastructure -A 3 -B 1Repository: TonyCasey/lisa
Length of output: 1939
🏁 Script executed:
# Check the git-mem module structure
ls -la src/lib/infrastructure/git-mem/Repository: TonyCasey/lisa
Length of output: 422
🏁 Script executed:
# Examine the GitMemFactory implementation to see how it handles repo root
cat -n src/lib/infrastructure/git-mem/GitMemFactory.tsRepository: TonyCasey/lisa
Length of output: 2096
🏁 Script executed:
# Check NotesService and MemoryRepository to understand repo root usage
rg -n "new NotesService|new MemoryRepository|process\.cwd|projectRoot" src/lib/infrastructure/git-mem -B 2 -A 5Repository: TonyCasey/lisa
Length of output: 1457
🏁 Script executed:
# Check where resetGitMemInstance is called in dispose
rg -n "resetGitMemInstance|dispose" src/lib/infrastructure/di/bootstrap.ts -B 2 -A 5Repository: TonyCasey/lisa
Length of output: 438
🏁 Script executed:
# Check if NotesService and MemoryRepository are from npm git-mem and how they handle repo root
rg -n "NotesService|MemoryRepository" src/lib -B 2 -A 5 | grep -A 10 "git-mem"Repository: TonyCasey/lisa
Length of output: 2956
🏁 Script executed:
# Check the dispose function more thoroughly to see if it should be calling resetGitMemInstance
cat -n src/lib/infrastructure/di/bootstrap.ts | sed -n '340,360p'Repository: TonyCasey/lisa
Length of output: 416
🏁 Script executed:
# Check if projectRoot is used elsewhere in bootstrap.ts and how config flows
rg -n "config\.projectRoot|projectRoot" src/lib/infrastructure/di/bootstrap.ts -B 2 -A 2Repository: TonyCasey/lisa
Length of output: 940
🏁 Script executed:
# Search for any documentation or comments about git-mem behavior with multiple processes/containers
rg -n "git-mem|process\.cwd|projectRoot" src/lib/infrastructure/services/GitMemMemoryService.ts -B 1 -A 3Repository: TonyCasey/lisa
Length of output: 960
🏁 Script executed:
# Verify that NotesService/MemoryRepository from git-mem indeed use the current git repo
# Check if there's any way they accept a repo path
web_search "git-mem npm NotesService MemoryRepository repo path parameter"Repository: TonyCasey/lisa
Length of output: 108
🏁 Script executed:
# Check how the dispose function is structured and what container.dispose() does
cat -n src/lib/infrastructure/di/bootstrap.ts | sed -n '70,352p' | tail -40Repository: TonyCasey/lisa
Length of output: 1895
🏁 Script executed:
# Verify if resetGitMemInstance is exported from the index and available to bootstrap.ts
cat -n src/lib/infrastructure/git-mem/index.tsRepository: TonyCasey/lisa
Length of output: 615
🏁 Script executed:
# Check test files to see if they use resetGitMemInstance or have isolation issues
rg -n "resetGitMemInstance|getGitMemInstance" tests/ --type ts -B 2 -A 2 | head -50Repository: TonyCasey/lisa
Length of output: 40
🏁 Script executed:
# Verify that NotesService/MemoryRepository require no repo path parameter (from git-mem npm)
# by checking if there are any other instantiations or if parameters are used
rg -n "new NotesService\(|new MemoryRepository\(" src/ -B 2 -A 2Repository: TonyCasey/lisa
Length of output: 1583
🏁 Script executed:
# Check if dispose calls resetGitMemInstance or should
cat -n src/lib/infrastructure/di/bootstrap.ts | sed -n '1,65p'Repository: TonyCasey/lisa
Length of output: 3037
🏁 Script executed:
# Verify the full export from git-mem index to confirm parameterless constructors
rg -n "export.*NotesService|export.*MemoryRepository" --type tsRepository: TonyCasey/lisa
Length of output: 1069
🏁 Script executed:
# Check if dispose is called anywhere and in what context
rg -n "\.dispose\(\)" src/ --type ts -B 3 -A 1 | head -50Repository: TonyCasey/lisa
Length of output: 1155
🏁 Script executed:
# Verify that resetGitMemInstance is actually accessible from bootstrap.ts imports
cat -n src/lib/infrastructure/di/bootstrap.ts | sed -n '59,62p'Repository: TonyCasey/lisa
Length of output: 176
🏁 Script executed:
# Check git-mem npm package docs comment to understand if NotesService uses cwd
rg -n "NotesService|MemoryRepository" src/lib/infrastructure/git-mem/GitMemFactory.ts -B 5 -A 10 | head -40Repository: TonyCasey/lisa
Length of output: 1453
Call resetGitMemInstance() in dispose or parameterize the git-mem singleton to respect projectRoot
The singleton git-mem instance created via getGitMemInstance() (line 113) is never reset during cleanup. While resetGitMemInstance() exists and is exported, the dispose() function (line 349) only calls container.dispose() and does not invoke it. Since NotesService and MemoryRepository from the npm git-mem package are instantiated without path parameters, they use process.cwd() internally—not the configured projectRoot (line 82). This causes:
- Test isolation failure: git-mem state persists across test suite runs sharing the same Node process.
- Cross-project contamination: Different project roots in the same process instance see each other's memories.
Add resetGitMemInstance() to the dispose() cleanup function, or alternatively, parameterize getGitMemInstance(projectRoot) and include projectRoot in the singleton cache key.
🤖 Prompt for AI Agents
In `@src/lib/infrastructure/di/bootstrap.ts` around lines 59 - 61, The git-mem
singleton created by getGitMemInstance() ignores projectRoot and is never
cleared; update the cleanup to call resetGitMemInstance() inside dispose() (in
addition to container.dispose()) to clear NotesService/MemoryRepository state
between runs, or refactor getGitMemInstance to accept projectRoot and include
that in its singleton cache key so each project gets its own instance; ensure
references to getGitMemInstance(), resetGitMemInstance(), and dispose() are
updated accordingly and that NotesService/MemoryRepository are instantiated with
the configured projectRoot.
There was a problem hiding this comment.
Resolved — the merge with origin/main now includes the git-mem adapters (GitMemAdapter.ts, GitMemTaskAdapter.ts) which implement IMemoryService and ITaskService using git-mem. The bootstrap.ts wiring uses these adapters.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the git-mem adapter files
fd "GitMemAdapter.ts|GitMemTaskAdapter.ts" src/
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Check the GitMemAdapter implementation for projectRoot handling
cat src/lib/infrastructure/services/GitMemAdapter.ts 2>/dev/null || echo "File not found"
Length of output: 14
🏁 Script executed:
#!/bin/bash
# Check the GitMemTaskAdapter implementation
cat src/lib/infrastructure/services/GitMemTaskAdapter.ts 2>/dev/null || echo "File not found"
Length of output: 14
🏁 Script executed:
#!/bin/bash
# Verify how bootstrap.ts currently uses these adapters
cat src/lib/infrastructure/di/bootstrap.ts | grep -A 10 -B 5 "GitMemAdapter\|gitMemService\|getGitMemInstance"
Length of output: 2047
🏁 Script executed:
#!/bin/bash
# Check the dispose function in bootstrap.ts to see if cleanup is addressed
cat src/lib/infrastructure/di/bootstrap.ts | grep -A 20 "const dispose"
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Check the actual GitMemAdapter implementation
cat src/lib/infrastructure/adapters/gitmem/GitMemAdapter.ts
Length of output: 3139
🏁 Script executed:
#!/bin/bash
# Check the actual GitMemTaskAdapter implementation
cat src/lib/infrastructure/adapters/gitmem/GitMemTaskAdapter.ts
Length of output: 3704
🏁 Script executed:
#!/bin/bash
# Check if bootstrap.ts imports or uses these new adapters
rg -n "GitMemAdapter|adapters/gitmem" src/lib/infrastructure/di/bootstrap.ts
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the full current state of the memory/task service setup in bootstrap
cat src/lib/infrastructure/di/bootstrap.ts | sed -n '108,125p'
Length of output: 806
@TonyCasey, I can see the new adapters exist at src/lib/infrastructure/adapters/gitmem/, but there are still a few issues:
1. The adapters aren't being used in bootstrap.ts
Lines 113-119 still use the old services:
const gitMemService = getGitMemInstance();
const memoryService = new GitMemMemoryService(gitMemService);
const taskService = new GitMemTaskService(gitMemService);The new GitMemAdapter and GitMemTaskAdapter aren't imported or used.
2. The adapters have the same projectRoot issue
Both adapters use this pattern:
function createGitMemService(): GitMemMemoryService {
const notesService = new NotesService();
const repository = new MemoryRepository(notesService);
return new GitMemMemoryService(repository);
}Since NotesService() and MemoryRepository() are called without parameters, they use process.cwd() internally, not the configured projectRoot from line 82.
3. Cleanup is still missing
The dispose() function (line 349) doesn't reset the git-mem singleton state, so test isolation and cross-project issues remain.
To fully resolve this:
- Update bootstrap.ts to use
new GitMemAdapter()andnew GitMemTaskAdapter() - Modify the adapters to accept
projectRootin their constructors and pass it toNotesService(projectRoot)/MemoryRepository(notes, projectRoot)(or verify git-mem supports this) - Add cleanup:
resetGitMemInstance()in thedispose()function
Would you like me to help verify if git-mem supports projectRoot parameters, or create a plan to implement these changes?
🧬 Code Graph Analysis Results
<src/lib/infrastructure/git-mem/GitMemFactory.ts, lines 27-34>
export function getGitMemInstance(): IGitMemMemoryService {
if (!gitMemInstance) {
const notes = new NotesService();
const repo = new MemoryRepository(notes);
gitMemInstance = new MemoryService(repo);
}
return gitMemInstance;
}
<src/lib/infrastructure/git-mem/GitMemMemoryService.ts, lines 71-162>
export class GitMemMemoryService implements IMemoryService {
constructor(private readonly gitMem: IGitMemMemoryService) {}
async loadMemory(
_timeoutMs?: number
): Promise<IMemoryResult> {
const { memories } = this.gitMem.recall(undefined, { limit: 100 });
// No group filtering - git repo provides scoping
const facts = memories.map(toMemoryItem);
// Separate init-review from facts
const initReviewFact = facts.find(f =>
f.tags?.some(t => t === 'init-review' || t === 'type:init-review')
);
return {
facts: facts.filter(f => f !== initReviewFact),
nodes: [],
tasks: [],
initReview: initReviewFact?.fact ?? null,
timedOut: false,
};
}
async loadFactsDateOrdered(
limit?: number,
options?: IMemoryDateOptions
): Promise<IMemoryItem[]> {
const { memories } = this.gitMem.recall(undefined, { limit: limit || 50 });
// No group filtering - git repo provides scoping
let filtered = memories;
if (options?.since) {
const sinceTime = options.since.getTime();
filtered = filtered.filter(m => new Date(m.createdAt).getTime() >= sinceTime);
}
if (options?.until) {
const untilTime = options.until.getTime();
filtered = filtered.filter(m => new Date(m.createdAt).getTime() <= untilTime);
}
return filtered.map(toMemoryItem);
}
async searchFacts(
query: string,
limit?: number
): Promise<IMemoryItem[]> {
const { memories } = this.gitMem.recall(undefined, { limit: limit || 10 });
return memories.map(toMemoryItem);
}
async saveMemory(facts: readonly string[]): Promise<void> {
for (const fact of facts) {
this.gitMem.remember(fact, {});
}
}
async addFact(
fact: string,
tags?: readonly string[]
): Promise<void> {
this.gitMem.remember(fact, {
tags: buildTags(tags),
});
}
async addFactWithLifecycle(
fact: string,
options: IMemorySaveOptions
): Promise<void> {
const confidence = options.confidence as ConfidenceLevel | undefined;
this.gitMem.remember(fact, {
tags: buildTags(undefined, options),
lifecycle: options.lifecycle,
confidence,
});
}
async expireFact(uuid: string): Promise<void> {
this.gitMem.delete(uuid);
}
async cleanupExpired(): Promise<number> {
// git-mem doesn't support TTL-based cleanup yet
return 0;
}
}
<src/lib/infrastructure/git-mem/GitMemTaskService.ts, lines 98-170>
export class GitMemTaskService implements ITaskService {
constructor(private readonly gitMem: IGitMemMemoryService) {}
async getTasks(): Promise<readonly ITask[]> {
return this.getTasksSimple();
}
async getTasksSimple(): Promise<readonly ITask[]> {
const { memories } = this.gitMem.recall(undefined, {
tag: TASK_TAG,
limit: 200,
});
const tasks = memories.map(toTask);
return deduplicateByTaskId(tasks);
}
async getTaskCounts(): Promise<ITaskCounts> {
const tasks = await this.getTasksSimple();
const counts = emptyTaskCounts();
const mutable = counts as unknown as Record<string, number>;
for (const task of tasks) {
if (task.status in mutable) {
mutable[task.status]++;
} else {
mutable['unknown']++;
}
}
return counts;
}
async createTask(task: ITaskInput): Promise<ITask> {
const key = `task-${Date.now()}`;
const status = task.status || 'ready';
this.gitMem.remember(`TASK: ${task.title}`, {
tags: buildTaskTags(key, status, task.blocked),
});
return {
key,
status,
title: task.title,
blocked: task.blocked ? [...task.blocked] : [],
created_at: new Date().toISOString(),
};
}
async updateTask(taskId: string, updates: ITaskUpdate): Promise<ITask> {
// Find existing task
const tasks = await this.getTasksSimple();
const existing = tasks.find(t => t.key === taskId);
const title = updates.title ?? existing?.title ?? 'Unknown task';
const status = updates.status ?? existing?.status ?? 'ready';
const blocked = updates.blocked ?? existing?.blocked ?? [];
// Create updated task memory (new memory with same task_id, latest wins)
this.gitMem.remember(`TASK: ${title}`, {
tags: buildTaskTags(taskId, status, blocked),
});
return {
key: taskId,
status,
title,
blocked: [...blocked],
created_at: existing?.created_at ?? new Date().toISOString(),
};
}
}
🧠 Learnings used
Learnt from: TonyCasey
Repo: TonyCasey/lisa PR: 211
File: src/lib/skills/shared/services/MemoryService.ts:393-418
Timestamp: 2026-02-06T17:09:35.417Z
Learning: In src/lib/skills/shared/services/MemoryService.ts consolidate method with git-mem backend, returning a placeholder retainedUuid ('new-merged-fact') for the merge action is acceptable because git-mem's remember() API returns Promise<boolean> (success/failure) rather than the created fact's ID. The delete + remember pattern is the standard way to update memories in git-mem.
Learnt from: TonyCasey
Repo: TonyCasey/lisa PR: 174
File: src/lib/infrastructure/services/SummarizationService.ts:75-79
Timestamp: 2026-02-01T15:39:42.522Z
Learning: In src/lib/infrastructure/services/SummarizationService.ts, the SummarizationService implementation resides in the infrastructure layer because it directly depends on infrastructure concerns (LLM prompt construction, HTTP response parsing via LlmGuard). The domain interface ISummarizationService is defined in the domain layer as the contract. The team will consider lifting orchestration to an application service if complexity grows (e.g., combining multiple services).
Learnt from: TonyCasey
Repo: TonyCasey/lisa PR: 211
File: src/lib/infrastructure/services/GitMemMemoryService.ts:103-126
Timestamp: 2026-02-06T17:08:38.484Z
Learning: In src/lib/infrastructure/services/GitMemMemoryService.ts, git-mem's recall() method returns memories in reverse chronological order (newest first) by design, as it reads from git notes which are ordered by commit date. Therefore, loadFactsDateOrdered does not need explicit sorting—it correctly relies on recall's inherent ordering.
Learnt from: TonyCasey
Repo: TonyCasey/lisa PR: 131
File: src/lib/infrastructure/services/MemoryService.ts:449-465
Timestamp: 2026-01-31T19:27:51.707Z
Learning: In src/lib/infrastructure/services/MemoryService.ts, the verifyFact method is an infrastructure-level primitive that only expires a fact. The full verification flow (reading tags, replacing confidence tags, writing back) is orchestrated by the skill-level MemoryService which has direct Neo4j client access. This separation follows clean architecture where infrastructure provides primitives and the skill layer composes them.
Learnt from: CR
Repo: TonyCasey/lisa PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-30T20:19:52.042Z
Learning: Applies to src/lib/infrastructure/dal/repositories/**/*Memory*Repository.{ts,tsx} : Memory system must support facts (discrete information), nodes (entities in knowledge graph), and tasks (tracked work items) with CRUD operations via MCP/Neo4j routing.
Learnt from: TonyCasey
Repo: TonyCasey/lisa PR: 211
File: src/lib/infrastructure/services/GitMemTaskService.ts:140-155
Timestamp: 2026-02-06T17:08:35.411Z
Learning: In the Lisa codebase, task key generation using `task-${Date.now()}` in GitMemTaskService is acceptable for the current single-user CLI context, where collision risk is negligible; more robust UUID-based generation may be considered for future hardening if concurrency requirements change.
Learnt from: TonyCasey
Repo: TonyCasey/lisa PR: 193
File: src/lib/infrastructure/adapters/claude/session-stop.ts:18-39
Timestamp: 2026-02-03T17:39:25.638Z
Learning: In src/lib/infrastructure/adapters/claude/session-start.ts and session-stop.ts: Keep thin adapters self-contained rather than extracting shared shutdown handling utilities. The module-level state (shutdownRequested, cleanup) makes extraction awkward, and the clarity benefit of independent, self-contained adapters outweighs DRY concerns for this minimal duplication.
Learnt from: TonyCasey
Repo: TonyCasey/lisa PR: 27
File: tests/unit/src/lib/infrastructure/services/MemoryService.timeout.test.ts:136-223
Timestamp: 2026-01-23T15:19:20.556Z
Learning: In tests/unit/**/MemoryService.timeout.test.ts and similar timeout tests: TonyCasey prefers using real timers with conservative thresholds (e.g., 500ms for operations expected to complete in <50ms) to test actual timeout behavior, rather than fake timers. This approach is intentional to validate production-like conditions, with a plan to adjust if CI flakiness occurs.
tests/unit/src/lib/application/handlers/SessionStopHandler.quality-tags.test.ts
Outdated
Show resolved
Hide resolved
Merge origin/main into lisa-59, resolving conflicts in skill service interfaces. Favored lisa-59 (HEAD) for groupId removal. Added git-mem adapters with updated signatures matching the groupId-free interfaces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove groupId from ai-enrich.ts addEnrichedMemory (P2 Major) - Add @deprecated JSDoc to IPromptResult.group field (Trivial) - Extract SavedOptions type alias to eliminate repeated casts in SessionStopHandler.quality-tags.test.ts (Trivial) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/lib/infrastructure/adapters/gitmem/GitMemAdapter.ts`:
- Around line 51-102: Wrap every call to this.gitMem inside try/catch in the
methods loadMemory, loadFactsDateOrdered, searchFacts, saveMemory, addFact,
addFactWithLifecycle, and expireFact; on catch create and throw a domain-level
Error subclass instance containing a clear message, a statusCode and a data
payload with the original error (or its message/stack) so infrastructure errors
are transformed into domain errors with context before propagating. Ensure each
catch includes which method failed (e.g., "GitMemAdapter.loadMemory failed") and
include the original error under data; keep the thrown type consistent with your
domain error class used elsewhere (extend Error and populate message,
statusCode, data).
In `@src/lib/infrastructure/adapters/gitmem/GitMemTaskAdapter.ts`:
- Around line 99-103: In updateTask of GitMemTaskAdapter, replace the generic
throw new Error(...) when existing is falsy with a domain-specific error (e.g.,
TaskNotFoundError or DomainError) that extends Error and includes statusCode and
data; ensure the thrown instance carries a clear message, statusCode (e.g., 404)
and any relevant data (taskId) so infrastructure callers can catch/transform it
per the domain-error contract.
- Around line 70-96: The git-mem methods (recall/get/delete/remember) are being
used synchronously in GitMemTaskAdapter (methods getTasks, getTasksSimple,
getTaskCounts, createTask and any other uses around lines ~99-133) but may be
async; update the adapter to await all gitMem calls and make loadTasks
asynchronous (ensure loadTasks is declared async and that callers await it),
e.g., await this.gitMem.remember(...) / await this.gitMem.recall(...) / await
this.gitMem.get(...) / await this.gitMem.delete(...), and propagate async/await
through getTasks, getTasksSimple, getTaskCounts, createTask and any helpers so
operations complete before returning.
In
`@tests/unit/src/lib/application/handlers/SessionStopHandler.quality-tags.test.ts`:
- Around line 250-254: The test uses a defensive nullish coalescing in the mock
memory callback (savedTags.push(options.tags ?? [])) causing a style mismatch
with other tests that access savedOptions[0].tags directly; update the mock to
follow the same pattern used elsewhere — either remove the defensive operator
and push options.tags directly (savedTags.push(options.tags)) or, preferably,
standardize all tests to use nullish coalescing by changing other tests to read
savedOptions[0].tags ?? [] so the handling of missing tags is consistent; locate
the callback inside createMockMemory (the addFactWithLifecycle mock) and apply
the chosen consistent pattern across the related tests.
| async loadMemory( | ||
| _timeoutMs?: number, | ||
| ): Promise<IMemoryResult> { | ||
| const result = this.gitMem.recall(undefined, { limit: 100 }); | ||
| return this.toMemoryResult(result); | ||
| } | ||
|
|
||
| async loadFactsDateOrdered( | ||
| limit?: number, | ||
| options?: IMemoryDateOptions, | ||
| ): Promise<IMemoryItem[]> { | ||
| const result = this.gitMem.recall(undefined, { | ||
| limit: limit ?? 100, | ||
| since: options?.since?.toISOString(), | ||
| }); | ||
| return result.memories.map(toMemoryItem); | ||
| } | ||
|
|
||
| async searchFacts( | ||
| query: string, | ||
| limit?: number, | ||
| ): Promise<IMemoryItem[]> { | ||
| const result = this.gitMem.recall(query, { limit: limit ?? 20 }); | ||
| return result.memories.map(toMemoryItem); | ||
| } | ||
|
|
||
| async saveMemory(facts: readonly string[]): Promise<void> { | ||
| for (const fact of facts) { | ||
| this.gitMem.remember(fact); | ||
| } | ||
| } | ||
|
|
||
| async addFact(fact: string, tags?: readonly string[]): Promise<void> { | ||
| this.gitMem.remember(fact, { | ||
| tags: tags ? [...tags] : undefined, | ||
| }); | ||
| } | ||
|
|
||
| async addFactWithLifecycle( | ||
| fact: string, | ||
| options: IMemorySaveOptions, | ||
| ): Promise<void> { | ||
| const tags: string[] = options.tags ? [...options.tags] : []; | ||
| if (options.lifecycle) { | ||
| tags.push(`lifecycle:${options.lifecycle}`); | ||
| } | ||
| this.gitMem.remember(fact, { tags }); | ||
| } | ||
|
|
||
| async expireFact(uuid: string): Promise<void> { | ||
| this.gitMem.delete(uuid); | ||
| } |
There was a problem hiding this comment.
Map infrastructure errors to domain errors.
These methods call external infrastructure but do not catch and rethrow domain errors with context (message/statusCode/data). Please wrap git-mem failures accordingly.
As per coding guidelines: "Domain errors must extend Error class with proper context (message, statusCode, data). Infrastructure errors must be transformed to domain errors in catch blocks."
🤖 Prompt for AI Agents
In `@src/lib/infrastructure/adapters/gitmem/GitMemAdapter.ts` around lines 51 -
102, Wrap every call to this.gitMem inside try/catch in the methods loadMemory,
loadFactsDateOrdered, searchFacts, saveMemory, addFact, addFactWithLifecycle,
and expireFact; on catch create and throw a domain-level Error subclass instance
containing a clear message, a statusCode and a data payload with the original
error (or its message/stack) so infrastructure errors are transformed into
domain errors with context before propagating. Ensure each catch includes which
method failed (e.g., "GitMemAdapter.loadMemory failed") and include the original
error under data; keep the thrown type consistent with your domain error class
used elsewhere (extend Error and populate message, statusCode, data).
| async getTasks(): Promise<readonly ITask[]> { | ||
| return this.loadTasks(); | ||
| } | ||
|
|
||
| async getTasksSimple(): Promise<readonly ITask[]> { | ||
| return this.loadTasks(); | ||
| } | ||
|
|
||
| async getTaskCounts(): Promise<ITaskCounts> { | ||
| const tasks = await this.loadTasks(); | ||
| const counts = { ...emptyTaskCounts() }; | ||
| for (const task of tasks) { | ||
| const key = task.status in counts ? task.status : 'unknown'; | ||
| (counts as Record<string, number>)[key] += 1; | ||
| } | ||
| return counts; | ||
| } | ||
|
|
||
| async createTask(task: ITaskInput): Promise<ITask> { | ||
| const tags = ['task', `status:${task.status ?? 'ready'}`]; | ||
| if (task.blocked) { | ||
| for (const b of task.blocked) { | ||
| tags.push(`blocked_by:${b}`); | ||
| } | ||
| } | ||
| const entity = this.gitMem.remember(task.title, { tags }); | ||
| return toTask(entity); |
There was a problem hiding this comment.
Await git-mem calls if they are async.
recall/get/delete/remember are used synchronously here. If git-mem returns Promises, you’ll return before data is persisted or loaded, and loadTasks() should be async.
✅ Suggested fix (pattern)
- async createTask(task: ITaskInput): Promise<ITask> {
+ async createTask(task: ITaskInput): Promise<ITask> {
const tags = ['task', `status:${task.status ?? 'ready'}`];
...
- const entity = this.gitMem.remember(task.title, { tags });
+ const entity = await this.gitMem.remember(task.title, { tags });
return toTask(entity);
}
- private loadTasks(): ITask[] {
- const result = this.gitMem.recall(undefined, { tag: 'task', limit: 200 });
- return result.memories.map(toTask);
- }
+ private async loadTasks(): Promise<ITask[]> {
+ const result = await this.gitMem.recall(undefined, { tag: 'task', limit: 200 });
+ return result.memories.map(toTask);
+ }git-mem MemoryService recall/get/delete/remember return types (Promise vs sync)?
As per coding guidelines: "Use async/await for all asynchronous operations. Avoid raw Promise patterns and callback nesting."
Also applies to: 99-128, 130-133
🤖 Prompt for AI Agents
In `@src/lib/infrastructure/adapters/gitmem/GitMemTaskAdapter.ts` around lines 70
- 96, The git-mem methods (recall/get/delete/remember) are being used
synchronously in GitMemTaskAdapter (methods getTasks, getTasksSimple,
getTaskCounts, createTask and any other uses around lines ~99-133) but may be
async; update the adapter to await all gitMem calls and make loadTasks
asynchronous (ensure loadTasks is declared async and that callers await it),
e.g., await this.gitMem.remember(...) / await this.gitMem.recall(...) / await
this.gitMem.get(...) / await this.gitMem.delete(...), and propagate async/await
through getTasks, getTasksSimple, getTaskCounts, createTask and any helpers so
operations complete before returning.
| async updateTask(taskId: string, updates: ITaskUpdate): Promise<ITask> { | ||
| const existing = this.gitMem.get(taskId); | ||
| if (!existing) { | ||
| throw new Error(`Task not found: ${taskId}`); | ||
| } |
There was a problem hiding this comment.
Use a domain error type for missing tasks.
Throwing a generic Error violates the domain error requirement. Please throw a domain error with statusCode/data.
As per coding guidelines: "Domain errors must extend Error class with proper context (message, statusCode, data). Infrastructure errors must be transformed to domain errors in catch blocks."
🤖 Prompt for AI Agents
In `@src/lib/infrastructure/adapters/gitmem/GitMemTaskAdapter.ts` around lines 99
- 103, In updateTask of GitMemTaskAdapter, replace the generic throw new
Error(...) when existing is falsy with a domain-specific error (e.g.,
TaskNotFoundError or DomainError) that extends Error and includes statusCode and
data; ensure the thrown instance carries a clear message, statusCode (e.g., 404)
and any relevant data (taskId) so infrastructure callers can catch/transform it
per the domain-error contract.
| const savedTags: string[][] = []; | ||
| const memory = createMockMemory({ | ||
| addFactWithLifecycle: async (_groupId, _fact, options) => { | ||
| const opts = options as { tags: string[] }; | ||
| savedTags.push(opts.tags); | ||
| addFactWithLifecycle: async (_fact, options) => { | ||
| savedTags.push(options.tags ?? []); | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor style inconsistency: nullish coalescing used here but not elsewhere.
Line 253 uses options.tags ?? [] defensively, while other tests access savedOptions[0].tags directly. Both work since the handler always provides tags, but consider applying the same pattern consistently across all tests for uniformity.
🤖 Prompt for AI Agents
In
`@tests/unit/src/lib/application/handlers/SessionStopHandler.quality-tags.test.ts`
around lines 250 - 254, The test uses a defensive nullish coalescing in the mock
memory callback (savedTags.push(options.tags ?? [])) causing a style mismatch
with other tests that access savedOptions[0].tags directly; update the mock to
follow the same pattern used elsewhere — either remove the defensive operator
and push options.tags directly (savedTags.push(options.tags)) or, preferably,
standardize all tests to use nullish coalescing by changing other tests to read
savedOptions[0].tags ?? [] so the handling of missing tags is consistent; locate
the callback inside createMockMemory (the addFactWithLifecycle mock) and apply
the chosen consistent pattern across the related tests.
1 participant
Summary
refs/notes/memgroupIdparametersChanges
group-id.tsutility files (skills/common, skills/shared/utils)Backward Compatibility
Existing memories with
group:tags remain stored but are no longer filtered by them. All memories in the repo are now loaded together.Test plan
Stats
60 files changed, -1076 lines net
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements
Refactor