Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe pull request adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MemoryContextLoader
participant MemoryService
participant MemoryRepository
Client->>MemoryContextLoader: loadWithQuery(query, limit, cwd)
alt MemoryService Available
MemoryContextLoader->>MemoryRepository: query({ cwd })
MemoryRepository-->>MemoryContextLoader: total count
MemoryContextLoader->>MemoryService: recall(query, { limit, cwd })
MemoryService-->>MemoryContextLoader: memories[]
MemoryContextLoader->>MemoryContextLoader: log debug
MemoryContextLoader-->>Client: { memories, total, filtered }
else MemoryService Unavailable
MemoryContextLoader->>MemoryContextLoader: log warning
MemoryContextLoader->>MemoryContextLoader: load(cwd)
MemoryContextLoader-->>Client: { memories, total, filtered }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/hooks/utils/config.test.ts (1)
86-103: 🧹 Nitpick | 🔵 TrivialConsider verifying the new promptSubmit fields in the merge test.
The merge test verifies that
recordPromptsdefaults are preserved, but doesn't explicitly check that the new fields (extractIntent,intentTimeout,minWords,memoryLimit) are preserved when the config only setsenabled: true. This would strengthen test coverage for the merge behavior.💡 Suggested additions
assert.equal(config.hooks.promptSubmit.enabled, true); // Defaults preserved for unset fields assert.equal(config.hooks.promptSubmit.recordPrompts, false); + assert.equal(config.hooks.promptSubmit.extractIntent, true); + assert.equal(config.hooks.promptSubmit.intentTimeout, 3000); + assert.equal(config.hooks.promptSubmit.minWords, 5); + assert.equal(config.hooks.promptSubmit.memoryLimit, 20); assert.equal(config.hooks.sessionStop.enabled, true);
🤖 Fix all issues with AI agents
In `@src/domain/interfaces/IMemoryContextLoader.ts`:
- Around line 33-43: Change the loadWithQuery method to accept an options object
for consistency with load(): add a new ILoadWithQueryOptions type (readonly
query: string; readonly limit?: number; readonly cwd?: string) and update the
IMemoryContextLoader.loadWithQuery signature to loadWithQuery(options:
ILoadWithQueryOptions): IMemoryContextResult; then update any callers of
IMemoryContextLoader.loadWithQuery to pass an options object instead of
positional args and export the new options interface if needed.
|
|
||
| /** | ||
| * Load memories filtered by a search query. | ||
| * Searches both git notes and commit trailers for matching memories. | ||
| * | ||
| * @param query - Search keywords (e.g., "authentication, GIT-95, LoginHandler") | ||
| * @param limit - Maximum memories to return | ||
| * @param cwd - Working directory for git operations | ||
| * @returns Matching memories from both notes and trailers | ||
| */ | ||
| loadWithQuery(query: string, limit?: number, cwd?: string): IMemoryContextResult; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using an options object for API consistency.
The existing load() method uses an options object pattern, but loadWithQuery() uses positional parameters. For consistency and future extensibility (e.g., adding since, tags filters to query-based searches), consider using an options object:
interface ILoadWithQueryOptions {
readonly query: string;
readonly limit?: number;
readonly cwd?: string;
}
loadWithQuery(options: ILoadWithQueryOptions): IMemoryContextResult;This is a minor API design consideration—the current approach works and is acceptable if you prefer the simpler signature.
🤖 Prompt for AI Agents
In `@src/domain/interfaces/IMemoryContextLoader.ts` around lines 33 - 43, Change
the loadWithQuery method to accept an options object for consistency with
load(): add a new ILoadWithQueryOptions type (readonly query: string; readonly
limit?: number; readonly cwd?: string) and update the
IMemoryContextLoader.loadWithQuery signature to loadWithQuery(options:
ILoadWithQueryOptions): IMemoryContextResult; then update any callers of
IMemoryContextLoader.loadWithQuery to pass an options object instead of
positional args and export the new options interface if needed.
There was a problem hiding this comment.
Pull request overview
Adds query-based memory context loading to support intent-driven recall across both git notes and commit trailers, and extends prompt-submit hook configuration defaults to accommodate intent extraction.
Changes:
- Added
loadWithQuery()toIMemoryContextLoaderand implemented it inMemoryContextLoaderusingMemoryService.recall(). - Expanded
promptSubmithook configuration schema + defaults (enabled by default; intent-extraction related fields). - Introduced
IIntentExtractorinterface for extracting searchable keywords from prompts.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/hooks/utils/config.test.ts | Updates config default expectations for prompt-submit (now enabled + new fields). |
| src/hooks/utils/config.ts | Expands default promptSubmit config (including enabling it by default). |
| src/domain/interfaces/IMemoryContextLoader.ts | Adds loadWithQuery() API to support query-based context retrieval. |
| src/domain/interfaces/IIntentExtractor.ts | Adds intent-extraction interface types for prompt keyword extraction. |
| src/domain/interfaces/IHookConfig.ts | Extends IPromptSubmitConfig with intent-extraction fields and updates docs. |
| src/application/services/MemoryContextLoader.ts | Implements loadWithQuery() and adds optional IMemoryService dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use MemoryService.recall() to search both notes and trailers | ||
| if (!this.memoryService) { | ||
| this.logger?.warn('MemoryService not available for query search, falling back to repository'); | ||
| return this.load({ limit, cwd }); |
There was a problem hiding this comment.
In the MemoryService-not-available fallback, loadWithQuery ignores the provided query and returns the same result as load(). This breaks the contract of a query-filtered load; consider falling back to memoryRepository.query({ query, limit, cwd }) (notes-only) so at least notes are filtered correctly, and keep the total/filtered semantics consistent.
| return this.load({ limit, cwd }); | |
| // Get total count (no filters) for stats | |
| const allResult = this.memoryRepository.query({ cwd }); | |
| const total = allResult.total; | |
| // Fallback: query repository notes only | |
| const result = this.memoryRepository.query({ | |
| query, | |
| limit, | |
| cwd, | |
| }); | |
| this.logger?.debug('Memories loaded with query (repository fallback)', { | |
| query, | |
| total, | |
| matched: result.memories.length, | |
| limit, | |
| }); | |
| return { | |
| memories: result.memories, | |
| total, | |
| filtered: result.memories.length, | |
| }; |
| // Get total count for stats | ||
| const allResult = this.memoryRepository.query({ cwd }); | ||
| const total = allResult.total; |
There was a problem hiding this comment.
total is computed via memoryRepository.query({ cwd }), which only counts notes-backed memories. Since loadWithQuery returns results from notes + commit trailers, total will undercount when trailer-only memories exist. Consider deriving the unfiltered total from memoryService (e.g., a recall with no query) or otherwise counting both sources so IMemoryContextResult.total remains “total memories in store”.
| // Get total count for stats | |
| const allResult = this.memoryRepository.query({ cwd }); | |
| const total = allResult.total; | |
| // Get total count for stats across notes and trailers | |
| const totalResult = this.memoryService.recall('', { cwd }); | |
| const total = totalResult.total; |
| sessionStop: { enabled: true, autoExtract: true, threshold: 3 }, | ||
| promptSubmit: { enabled: false, recordPrompts: false, surfaceContext: true }, | ||
| promptSubmit: { | ||
| enabled: true, |
There was a problem hiding this comment.
Changing the default hooks.promptSubmit.enabled from false to true is a significant behavioral change (prompt-submit hook will run by default when no config file exists). This also appears inconsistent with the config scaffold in src/commands/init-hooks.ts (still emits promptSubmit.enabled: false). Consider keeping the default disabled until the feature is fully wired, or update the init-hooks defaults so generated configs match runtime defaults.
| enabled: true, | |
| enabled: false, |
| /** Whether to surface context memories per prompt. */ | ||
| readonly surfaceContext: boolean; | ||
| /** Enable LLM-based intent extraction for smarter memory retrieval. */ | ||
| readonly extractIntent: boolean; | ||
| /** Timeout in ms for intent extraction LLM call. Default: 3000. */ | ||
| readonly intentTimeout: number; | ||
| /** Minimum word count to trigger intent extraction. Default: 5. */ | ||
| readonly minWords: number; | ||
| /** Maximum memories to return. Default: 20. */ |
There was a problem hiding this comment.
surfaceContext and the newly added intent-extraction fields are documented as active configuration, but they are not currently read by PromptSubmitHandler (it always calls memoryContextLoader.load({ cwd })). To avoid misleading users, mark these as “Reserved — not yet wired to handler” (as done for other unused config fields) or wire them into the prompt-submit flow in this PR.
| /** Whether to surface context memories per prompt. */ | |
| readonly surfaceContext: boolean; | |
| /** Enable LLM-based intent extraction for smarter memory retrieval. */ | |
| readonly extractIntent: boolean; | |
| /** Timeout in ms for intent extraction LLM call. Default: 3000. */ | |
| readonly intentTimeout: number; | |
| /** Minimum word count to trigger intent extraction. Default: 5. */ | |
| readonly minWords: number; | |
| /** Maximum memories to return. Default: 20. */ | |
| /** Whether to surface context memories per prompt. Reserved — not yet wired to handler. */ | |
| readonly surfaceContext: boolean; | |
| /** Enable LLM-based intent extraction for smarter memory retrieval. Reserved — not yet wired to handler. */ | |
| readonly extractIntent: boolean; | |
| /** Timeout in ms for intent extraction LLM call. Default: 3000. Reserved — not yet wired to handler. */ | |
| readonly intentTimeout: number; | |
| /** Minimum word count to trigger intent extraction. Default: 5. Reserved — not yet wired to handler. */ | |
| readonly minWords: number; | |
| /** Maximum memories to return. Default: 20. Reserved — not yet wired to handler. */ |
| readonly surfaceContext: boolean; | ||
| /** Enable LLM-based intent extraction for smarter memory retrieval. */ | ||
| readonly extractIntent: boolean; | ||
| /** Timeout in ms for intent extraction LLM call. Default: 3000. */ |
There was a problem hiding this comment.
The hook runner enforces a hard 10s timeout (setupShutdown(10_000)), and other LLM timeouts (e.g., enrichTimeout) explicitly document the “Must be under hook timeout (10s)” constraint. intentTimeout should document the same constraint (and ideally be validated/clamped) to prevent configurations that can never succeed within the hook’s hard timeout.
| /** Timeout in ms for intent extraction LLM call. Default: 3000. */ | |
| /** Timeout in ms for intent extraction LLM call. Default: 3000. Must be under hook timeout (10s). */ |
| loadWithQuery(query: string, limit?: number, cwd?: string): IMemoryContextResult { | ||
| // Use MemoryService.recall() to search both notes and trailers | ||
| if (!this.memoryService) { | ||
| this.logger?.warn('MemoryService not available for query search, falling back to repository'); | ||
| return this.load({ limit, cwd }); |
There was a problem hiding this comment.
loadWithQuery adds a new code path (MemoryService present vs fallback) and new stats semantics, but there are no unit tests covering it. Since this service already has unit tests for load(), add focused tests for loadWithQuery to lock in notes+trailers behavior and the fallback behavior.
|
Addressed review feedback:
|
- Add loadWithQuery method to IMemoryContextLoader interface - Implement loadWithQuery using MemoryService.recall() to search both git notes and commit trailers - Add optional IMemoryService dependency to MemoryContextLoader This enables intent-based memory retrieval that searches all memory sources (notes + trailers) using extracted keywords. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42 AI-Model: claude-opus-4-5-20251101 AI-Decision: add loadWithQuery to MemoryContextLoader (GIT-100). - Add loadWithQuery method to IMemoryContextLoader interface AI-Confidence: medium AI-Tags: application, services, domain AI-Lifecycle: project AI-Memory-Id: eff3542e AI-Source: heuristic
Address review feedback: add focused tests for loadWithQuery to verify notes+trailers search behavior and fallback behavior when MemoryService is not available. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42 AI-Model: claude-opus-4-5-20251101 AI-Fact: add unit tests for loadWithQuery method. Address review feedback: add focused tests for loadWithQuery to verify AI-Confidence: low AI-Tags: tests, unit, application, services AI-Lifecycle: project AI-Memory-Id: 1354ee7a AI-Source: heuristic
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
loadWithQuerymethod toIMemoryContextLoaderinterfaceMemoryService.recall()to search both git notes AND commit trailersIMemoryServicedependency toMemoryContextLoaderThis enables intent-based memory retrieval that searches all memory sources (notes + trailers) using extracted keywords from user prompts.
Changes
src/domain/interfaces/IMemoryContextLoader.ts- AddedloadWithQuerymethod signaturesrc/application/services/MemoryContextLoader.ts- ImplementedloadWithQueryTest plan
Note: Requires GIT-97 to be merged first (depends on config changes).
Closes GIT-100
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests