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>
📝 WalkthroughWalkthroughThis PR consolidates the git-mem factory into a shared singleton pattern at the infrastructure level, renames service interfaces and factories to skill-specific variants (e.g., IMemoryService → ISkillMemoryService), introduces a centralized skill-interfaces module, and updates the DI bootstrap to reuse the shared git-mem singleton instance. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
🚥 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)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0db0b2f26e
ℹ️ 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".
| const memoryRepo = new MemoryRepository(notesService); | ||
| const gitMemService = new GitMemService(memoryRepo); | ||
| // Use shared git-mem instance from GitMemFactory | ||
| const gitMemService = getGitMemInstance(); |
There was a problem hiding this comment.
Scope git-mem instance per project bootstrap
bootstrapContainer now always takes the process-wide getGitMemInstance() path, which means all later container bootstraps reuse the first git-mem backend created in this process. Since that singleton is not keyed by projectRoot, any multi-repo usage in one process (e.g., repeated bootstrap calls in adapters/tests or long-lived hosts) can persist/read memory and tasks against the wrong repository state; this is a data-isolation regression compared to constructing a fresh instance per bootstrap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for the detailed analysis! Let me explain why this is safe in Lisa's architecture:
git-mem is stateless regarding cwd:
NotesService.read/write/list()takescwdas a parameter on each method callMemoryRepository.create/query/delete()passescwdthrough to NotesService- The singleton stores no repo state - it just holds the wired service instances
Lisa's usage pattern:
- CLI commands are short-lived processes (one command = one process)
- Hooks run per-session in the context of a single project
- There's no multi-repo handling within a single process
For tests:
- We export
resetGitMemInstance()which tests can call between bootstrap invocations - This clears the singleton when needed
Fallback behavior:
When cwd is not passed (as in Lisa's services), git-mem defaults to process.cwd() at execution time - which is correct for our CLI/hook pattern since they're always invoked from the project root.
If we need multi-repo support in the future, we could add explicit cwd passing to the skill services, but for current usage the singleton is safe.
Summary
Changes
Created (in infrastructure)
infrastructure/git-mem/GitMemFactory.ts- Shared lazy singleton factoryinfrastructure/services/skill-interfaces/- Skill service interfacesinfrastructure/services/SkillMemoryService.ts- Rich memory service for CLIinfrastructure/services/SkillTaskService.ts- Rich task service for CLIinfrastructure/services/SkillPromptService.ts- Prompt capture serviceModified
bootstrap.ts- Uses sharedgetGitMemInstance()instead of inline creationDeleted (6 files, -1341 lines)
skills/shared/clients/GitMemFactory.tsskills/shared/services/MemoryService.tsskills/shared/services/TaskService.tsskills/shared/services/PromptService.tsskills/shared/services/interfaces/IMemoryService.tsskills/shared/services/interfaces/ITaskService.tsTest plan
npm run buildsucceedsnpm testpasses (1456 tests)npm run lintpasses (only pre-existing warning)lisa memory loadworkslisa tasks listworksCloses LISA-58
🤖 Generated with Claude Code
Summary by CodeRabbit