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. 📝 WalkthroughWalkthroughThree files are modified to introduce config-driven, intent-aware memory retrieval in PromptSubmitHandler. The handler now loads hook configuration and conditionally extracts intent to determine memory-loading strategy. IntentExtractor simplifies error handling by removing explicit timeout cleanup. DI container wiring connects the new dependencies. Changes
Sequence DiagramsequenceDiagram
participant Handler as PromptSubmitHandler
participant ConfigLoader as hookConfigLoader
participant IntentExt as intentExtractor
participant MemLoader as MemoryContextLoader
Handler->>ConfigLoader: Load hook config
ConfigLoader-->>Handler: Config with surfaceContext, extractIntent, memoryLimit
alt surfaceContext disabled
Handler-->>Handler: Early exit, return empty output
else surfaceContext enabled
alt extractIntent enabled
Handler->>IntentExt: Extract intent from prompt
IntentExt-->>Handler: Intent with keywords (or SKIP)
alt Intent extracted successfully
Handler->>MemLoader: loadWithQuery(keywords, memoryLimit)
MemLoader-->>Handler: Memories matching keywords
else Intent extraction failed
Handler->>MemLoader: Load recent memories(memoryLimit)
MemLoader-->>Handler: Recent memories
end
else extractIntent disabled
Handler->>MemLoader: Load recent memories(memoryLimit)
MemLoader-->>Handler: Recent memories
end
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 |
There was a problem hiding this comment.
Pull request overview
Adds intent-based memory recall for the prompt-submit hook by introducing an LLM-backed intent extractor and plumbing a query-based memory loading path into the handler and DI container.
Changes:
- Add
IIntentExtractor+ Anthropic-basedIntentExtractorimplementation for keyword extraction. - Extend
IMemoryContextLoader/MemoryContextLoaderwithloadWithQuery()and updatePromptSubmitHandlerto use it when intent extraction is enabled. - Update hook config defaults/schema (prompt-submit now enabled by default; adds extractIntent/intentTimeout/minWords/memoryLimit) and related unit tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/hooks/utils/config.test.ts | Updates expectations for new prompt-submit defaults and fields. |
| src/infrastructure/llm/IntentExtractor.ts | New Anthropic SDK-based intent extraction service with timeout logic. |
| src/infrastructure/di/types.ts | Adds intentExtractor to the DI cradle typing. |
| src/infrastructure/di/container.ts | Registers IntentExtractor and injects it + config loader into PromptSubmitHandler. |
| src/hooks/utils/config.ts | Updates default hook config (enables prompt-submit; adds intent extraction settings). |
| src/domain/interfaces/IMemoryContextLoader.ts | Adds loadWithQuery() to support query-based memory loading. |
| src/domain/interfaces/IIntentExtractor.ts | New domain interface + result contract for intent extraction. |
| src/domain/interfaces/IHookConfig.ts | Extends prompt-submit config schema with intent extraction controls. |
| src/application/services/MemoryContextLoader.ts | Implements query-based loading via MemoryService.recall() (notes + trailers). |
| src/application/handlers/PromptSubmitHandler.ts | Uses intent extraction + query recall when enabled; otherwise falls back to recent memories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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.
intentTimeout is governed by the same hard hook timeout (10s via setupShutdown(10_000) in the hook entrypoint) as other LLM timeouts. The doc comment should note that this value must be under the hook timeout, similar to enrichTimeout, so users don’t configure a value that can never be honored.
| /** 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). */ |
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| setTimeout(() => reject(new Error('Intent extraction timed out')), this.timeout); | ||
| }); | ||
|
|
||
| const extractPromise = this.callLLM(prompt); | ||
|
|
||
| return Promise.race([extractPromise, timeoutPromise]); | ||
| } | ||
|
|
||
| /** | ||
| * Make the actual LLM call. | ||
| */ | ||
| private async callLLM(prompt: string): Promise<string | null> { | ||
| const response = await this.client.messages.create({ | ||
| model: HAIKU_MODEL, | ||
| max_tokens: MAX_TOKENS, | ||
| system: SYSTEM_PROMPT, | ||
| messages: [{ role: 'user', content: `User prompt: "${prompt}"` }], | ||
| }); |
There was a problem hiding this comment.
extractWithTimeout starts a setTimeout but never clears (or unrefs) it when the LLM call completes first. This can keep the Node event loop alive until the timeout elapses, delaying hook completion; also the underlying Anthropic request is not aborted on timeout. Track the timeout ID and clearTimeout on success, and consider using AbortController/request signal support to cancel the in-flight call when timing out.
| const timeoutPromise = new Promise<never>((_, reject) => { | |
| setTimeout(() => reject(new Error('Intent extraction timed out')), this.timeout); | |
| }); | |
| const extractPromise = this.callLLM(prompt); | |
| return Promise.race([extractPromise, timeoutPromise]); | |
| } | |
| /** | |
| * Make the actual LLM call. | |
| */ | |
| private async callLLM(prompt: string): Promise<string | null> { | |
| const response = await this.client.messages.create({ | |
| model: HAIKU_MODEL, | |
| max_tokens: MAX_TOKENS, | |
| system: SYSTEM_PROMPT, | |
| messages: [{ role: 'user', content: `User prompt: "${prompt}"` }], | |
| }); | |
| const controller = new AbortController(); | |
| let timeoutId: ReturnType<typeof setTimeout> | null = null; | |
| const timeoutPromise = new Promise<never>((_, reject) => { | |
| timeoutId = setTimeout(() => { | |
| controller.abort(); | |
| reject(new Error('Intent extraction timed out')); | |
| }, this.timeout); | |
| }); | |
| try { | |
| const extractPromise = this.callLLM(prompt, controller.signal); | |
| return await Promise.race([extractPromise, timeoutPromise]); | |
| } finally { | |
| if (timeoutId !== null) { | |
| clearTimeout(timeoutId); | |
| } | |
| } | |
| } | |
| /** | |
| * Make the actual LLM call. | |
| */ | |
| private async callLLM(prompt: string, signal?: AbortSignal): Promise<string | null> { | |
| const response = await this.client.messages.create( | |
| { | |
| model: HAIKU_MODEL, | |
| max_tokens: MAX_TOKENS, | |
| system: SYSTEM_PROMPT, | |
| messages: [{ role: 'user', content: `User prompt: "${prompt}"` }], | |
| }, | |
| signal ? { signal } : undefined, | |
| ); |
| minWords: 5, | ||
| intentTimeout: 3000, |
There was a problem hiding this comment.
The fallback promptConfig includes minWords and intentTimeout, but those values are never applied (they aren’t passed to IntentExtractor and aren’t used in the handler). As a result, the new config fields have no effect. Either wire these settings into the extraction flow (e.g., pass per-call options / enforce timeout & minWords in the handler) or remove them from the config shape to avoid misleading configuration.
| minWords: 5, | |
| intentTimeout: 3000, |
| } catch { | ||
| return null; | ||
| } | ||
| }).singleton(), |
There was a problem hiding this comment.
intentExtractor is registered as a singleton with hardcoded defaults (timeout/minWords/model). Since prompt-submit config is repo- and cwd-dependent, this makes it difficult/impossible to honor hooks.promptSubmit.intentTimeout and minWords per repo/config, leaving those config fields effectively inert. Consider constructing the extractor with config-derived options at call time, or extending the extractor API to accept per-request options.
| }).singleton(), | |
| }), |
| promptSubmit: { | ||
| enabled: true, | ||
| recordPrompts: false, | ||
| surfaceContext: true, | ||
| extractIntent: true, | ||
| intentTimeout: 3000, | ||
| minWords: 5, | ||
| memoryLimit: 20, | ||
| }, |
There was a problem hiding this comment.
Changing the default to promptSubmit.enabled: true is a behavior change and currently appears inconsistent with the config generated by init commands (e.g., buildGitMemConfig() in src/commands/init-hooks.ts and src/commands/init.ts still sets promptSubmit.enabled: false). This inconsistency can lead to confusing defaults depending on whether a config file exists. Align the defaults across the codebase (either keep prompt-submit disabled by default everywhere, or update the init-generated config and related expectations).
![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
🤖 Fix all issues with AI agents
In `@src/application/handlers/PromptSubmitHandler.ts`:
- Around line 39-90: The handler currently ignores promptConfig.minWords and
promptConfig.intentTimeout; update PromptSubmitHandler so intent extraction only
runs when promptConfig.extractIntent is true AND
(event.prompt?.split(/\s+/).length ?? 0) >= promptConfig.minWords, and enforce
intentTimeout by passing the timeout into the extractor call (extend
IIntentExtractorInput to accept intentTimeout or wrap
intentExtractor.extract(...) in a Promise.race with a timeout) so extract
respects per-repo overrides; ensure branches that call
intentExtractor.extract(...) and the fallback
memoryContextLoader.load/loadWithQuery remain consistent and still set result:
IMemoryContextResult, and pass promptConfig.memoryLimit and event.cwd unchanged
to memoryContextLoader.loadWithQuery/load.
In `@src/domain/interfaces/IIntentExtractor.ts`:
- Line 32: Add a short JSDoc note explaining the intended usage of the 'no_llm'
reason (e.g., it's intended for upstream/handler-level fallbacks or absence of
an LLM client rather than being produced by the current IntentExtractor
implementation which throws on missing API keys) next to the definition of the
reasons enum/constant and/or on the IIntentExtractor interface; reference the
symbol name 'IIntentExtractor' and the literal 'no_llm' so readers can find it,
and indicate that current implementations may instead throw or return other
reasons (like 'llm_skip'), so handlers should perform null-checks or fallbacks
when they see 'no_llm'.
In `@src/infrastructure/llm/IntentExtractor.ts`:
- Around line 101-120: The current extractWithTimeout implementation races a
timeout against callLLM causing the Anthropic request to continue running;
update IntentExtractor to cancel in-flight requests by removing the Promise.race
pattern and passing a cancellation option to the SDK call in callLLM: call
this.client.messages.create(...) with the per-request timeout option ({ timeout:
this.timeout }) or create an AbortController and pass its signal as the
second/options argument so that extractWithTimeout triggers controller.abort()
on timeout and callLLM receives the cancellation; ensure you reference and
update extractWithTimeout, callLLM, this.client.messages.create and this.timeout
to implement the SDK-provided timeout or AbortController-based cancellation.
| // Load config | ||
| const config = this.hookConfigLoader?.loadConfig(event.cwd); | ||
| const promptConfig = config?.hooks.promptSubmit ?? { | ||
| surfaceContext: true, | ||
| extractIntent: false, | ||
| memoryLimit: 20, | ||
| minWords: 5, | ||
| intentTimeout: 3000, | ||
| }; | ||
|
|
||
| // Early exit if context surfacing is disabled | ||
| if (!promptConfig.surfaceContext) { | ||
| this.logger?.debug('Context surfacing disabled'); | ||
| return { | ||
| handler: 'PromptSubmitHandler', | ||
| success: true, | ||
| output: '', | ||
| }; | ||
| } | ||
|
|
||
| // Try intent extraction if enabled | ||
| let result: IMemoryContextResult; | ||
|
|
||
| if (promptConfig.extractIntent && this.intentExtractor && event.prompt) { | ||
| const intentResult = await this.intentExtractor.extract({ prompt: event.prompt }); | ||
|
|
||
| if (!intentResult.skipped && intentResult.intent) { | ||
| this.logger?.debug('Intent extracted, querying with keywords', { | ||
| intent: intentResult.intent, | ||
| }); | ||
| result = this.memoryContextLoader.loadWithQuery( | ||
| intentResult.intent, | ||
| promptConfig.memoryLimit, | ||
| event.cwd, | ||
| ); | ||
| } else { | ||
| this.logger?.debug('Intent extraction skipped', { | ||
| reason: intentResult.reason, | ||
| }); | ||
| // Fall back to loading recent memories | ||
| result = this.memoryContextLoader.load({ | ||
| cwd: event.cwd, | ||
| limit: promptConfig.memoryLimit, | ||
| }); | ||
| } | ||
| } else { | ||
| // No intent extraction, load recent memories | ||
| result = this.memoryContextLoader.load({ | ||
| cwd: event.cwd, | ||
| limit: promptConfig.memoryLimit, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Configured minWords/intentTimeout are ignored.
These values are loaded but never applied, so user config has no effect on extraction gating or timeout behavior. Either gate extraction in the handler and/or pass per-repo overrides into the extractor.
🔧 Example fix to honor minWords in the handler
- if (promptConfig.extractIntent && this.intentExtractor && event.prompt) {
- const intentResult = await this.intentExtractor.extract({ prompt: event.prompt });
+ if (promptConfig.extractIntent && this.intentExtractor && event.prompt) {
+ const wordCount = event.prompt.trim().split(/\s+/).filter(Boolean).length;
+ if (wordCount < promptConfig.minWords) {
+ this.logger?.debug('Intent extraction skipped: too short', { wordCount });
+ result = this.memoryContextLoader.load({
+ cwd: event.cwd,
+ limit: promptConfig.memoryLimit,
+ });
+ } else {
+ const intentResult = await this.intentExtractor.extract({ prompt: event.prompt });
- if (!intentResult.skipped && intentResult.intent) {
+ if (!intentResult.skipped && intentResult.intent) {
this.logger?.debug('Intent extracted, querying with keywords', {
intent: intentResult.intent,
});
result = this.memoryContextLoader.loadWithQuery(
intentResult.intent,
promptConfig.memoryLimit,
event.cwd,
);
- } else {
+ } else {
this.logger?.debug('Intent extraction skipped', {
reason: intentResult.reason,
});
// Fall back to loading recent memories
result = this.memoryContextLoader.load({
cwd: event.cwd,
limit: promptConfig.memoryLimit,
});
- }
+ }
+ }
} else {Note: intentTimeout still needs wiring (e.g., extend IIntentExtractorInput with overrides or inject a repo-scoped extractor instance).
🤖 Prompt for AI Agents
In `@src/application/handlers/PromptSubmitHandler.ts` around lines 39 - 90, The
handler currently ignores promptConfig.minWords and promptConfig.intentTimeout;
update PromptSubmitHandler so intent extraction only runs when
promptConfig.extractIntent is true AND (event.prompt?.split(/\s+/).length ?? 0)
>= promptConfig.minWords, and enforce intentTimeout by passing the timeout into
the extractor call (extend IIntentExtractorInput to accept intentTimeout or wrap
intentExtractor.extract(...) in a Promise.race with a timeout) so extract
respects per-repo overrides; ensure branches that call
intentExtractor.extract(...) and the fallback
memoryContextLoader.load/loadWithQuery remain consistent and still set result:
IMemoryContextResult, and pass promptConfig.memoryLimit and event.cwd unchanged
to memoryContextLoader.loadWithQuery/load.
| * - 'too_short': Prompt has fewer words than minWords threshold. | ||
| * - 'confirmation': Prompt is a simple confirmation (yes/no/ok/etc). | ||
| * - 'no_llm': No LLM client available. | ||
| * - 'llm_skip': LLM returned SKIP (no extractable keywords). |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting where no_llm is used.
The no_llm reason is defined but may not be returned by the current IntentExtractor implementation (which throws if no API key is provided). This reason appears to be for handler-level fallback scenarios or future implementations. Consider adding a brief note in the JSDoc clarifying this is for upstream null-check scenarios.
📝 Optional: Clarify no_llm usage
* - 'confirmation': Prompt is a simple confirmation (yes/no/ok/etc).
- * - 'no_llm': No LLM client available.
+ * - 'no_llm': No LLM client available (returned by handler when extractor is null).
* - 'llm_skip': LLM returned SKIP (no extractable keywords).📝 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.
| * - 'llm_skip': LLM returned SKIP (no extractable keywords). | |
| * - 'confirmation': Prompt is a simple confirmation (yes/no/ok/etc). | |
| * - 'no_llm': No LLM client available (returned by handler when extractor is null). | |
| * - 'llm_skip': LLM returned SKIP (no extractable keywords). |
🤖 Prompt for AI Agents
In `@src/domain/interfaces/IIntentExtractor.ts` at line 32, Add a short JSDoc note
explaining the intended usage of the 'no_llm' reason (e.g., it's intended for
upstream/handler-level fallbacks or absence of an LLM client rather than being
produced by the current IntentExtractor implementation which throws on missing
API keys) next to the definition of the reasons enum/constant and/or on the
IIntentExtractor interface; reference the symbol name 'IIntentExtractor' and the
literal 'no_llm' so readers can find it, and indicate that current
implementations may instead throw or return other reasons (like 'llm_skip'), so
handlers should perform null-checks or fallbacks when they see 'no_llm'.
| private async extractWithTimeout(prompt: string): Promise<string | null> { | ||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| setTimeout(() => reject(new Error('Intent extraction timed out')), this.timeout); | ||
| }); | ||
|
|
||
| const extractPromise = this.callLLM(prompt); | ||
|
|
||
| return Promise.race([extractPromise, timeoutPromise]); | ||
| } | ||
|
|
||
| /** | ||
| * Make the actual LLM call. | ||
| */ | ||
| private async callLLM(prompt: string): Promise<string | null> { | ||
| const response = await this.client.messages.create({ | ||
| model: HAIKU_MODEL, | ||
| max_tokens: MAX_TOKENS, | ||
| system: SYSTEM_PROMPT, | ||
| messages: [{ role: 'user', content: `User prompt: "${prompt}"` }], | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "package.json" -type f | head -5Repository: TonyCasey/git-mem
Length of output: 75
🏁 Script executed:
cat package.json | grep -A 2 -B 2 "@anthropic-ai/sdk"Repository: TonyCasey/git-mem
Length of output: 205
🌐 Web query:
@anthropic-ai/sdk v0.73.0 AbortController timeout support messages.create
💡 Result:
In @anthropic-ai/sdk@0.73.0, client.messages.create(body, options?) supports both per-request timeouts and AbortController cancellation via the second argument (“request options”):
-
Timeout (ms):
await client.messages.create( { model, max_tokens: 1024, messages: [{ role: 'user', content: 'hi' }] }, { timeout: 5_000 }, );
The SDK documents
timeoutas the per-request override (default is 10 minutes). [1] -
AbortController / AbortSignal:
const controller = new AbortController(); const p = client.messages.create( { model, max_tokens: 1024, messages: [{ role: 'user', content: 'hi' }] }, { signal: controller.signal }, ); setTimeout(() => controller.abort(), 5_000); await p;
RequestOptionsincludessignal?: AbortSignal(and alsotimeout?: number). [2]
If you’re streaming (stream: true), the returned stream object also exposes a controller you can abort (separately from the per-request signal). [1]
Sources:
[1] Anthropic TypeScript SDK README (timeouts + streaming cancellation) (github.com)
[2] SDK RequestOptions type (includes timeout + signal) (cdn.jsdelivr.net)
Citations:
- 1: https://github.com/anthropics/anthropic-sdk-typescript
- 2: https://cdn.jsdelivr.net/npm/%40anthropic-ai/sdk%400.74.0/internal/request-options.d.ts
🏁 Script executed:
wc -l src/infrastructure/llm/IntentExtractor.tsRepository: TonyCasey/git-mem
Length of output: 106
🏁 Script executed:
sed -n '95,130p' src/infrastructure/llm/IntentExtractor.tsRepository: TonyCasey/git-mem
Length of output: 1014
Use the SDK's built-in timeout option to properly cancel the LLM request.
Promise.race returns when the timeout fires, but the Anthropic API call continues consuming tokens and resources. The @anthropic-ai/sdk@0.73.0 directly supports per-request timeouts via the options parameter to messages.create():
await this.client.messages.create(
{ model, max_tokens, system, messages },
{ timeout: this.timeout }
);Alternatively, use an AbortController with the signal option to gain fine-grained cancellation control. Either approach properly terminates the in-flight request instead of orphaning it.
🤖 Prompt for AI Agents
In `@src/infrastructure/llm/IntentExtractor.ts` around lines 101 - 120, The
current extractWithTimeout implementation races a timeout against callLLM
causing the Anthropic request to continue running; update IntentExtractor to
cancel in-flight requests by removing the Promise.race pattern and passing a
cancellation option to the SDK call in callLLM: call
this.client.messages.create(...) with the per-request timeout option ({ timeout:
this.timeout }) or create an AbortController and pass its signal as the
second/options argument so that extractWithTimeout triggers controller.abort()
on timeout and callLLM receives the cancellation; ensure you reference and
update extractWithTimeout, callLLM, this.client.messages.create and this.timeout
to implement the SDK-provided timeout or AbortController-based cancellation.
|
Addressed review feedback:
|
- Add IntentExtractor class in infrastructure/llm that extracts searchable keywords from user prompts using Claude Haiku - Features: skip short prompts, skip confirmations, 3s timeout, graceful error handling - Register intentExtractor in DI container (nullable for graceful degradation when no API key) - Add IIntentExtractor to ICradle types 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: implement IntentExtractor service (GIT-98). - Add IntentExtractor class in infrastructure/llm that extracts AI-Confidence: medium AI-Tags: infrastructure, llm, typescript AI-Lifecycle: project AI-Memory-Id: 24793fe1 AI-Source: heuristic
- Add IHookConfigLoader and IIntentExtractor dependencies - Extract keywords from substantive prompts using IntentExtractor - Use loadWithQuery for intent-based memory search (notes + trailers) - Fall back to recent memories when extraction is skipped - Respect surfaceContext and extractIntent config options This enables smarter memory retrieval: instead of dumping all memories, the handler extracts keywords from the prompt (e.g., "GIT-95", "authentication", "LoginHandler") and queries for relevant memories. 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: dumping all memories, AI-Confidence: medium AI-Tags: application, handlers, infrastructure, pattern:instead-of AI-Lifecycle: project AI-Memory-Id: bf416b0d AI-Source: heuristic
Summary
This is the main feature PR that enables smarter memory retrieval for the prompt-submit hook. Instead of dumping all memories, the handler now:
How It Works
Changes
src/application/handlers/PromptSubmitHandler.ts- Added intent extraction logicsrc/infrastructure/di/container.ts- Pass hookConfigLoader and intentExtractor to handlerDependencies
This PR depends on:
Test plan
Closes GIT-99
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Refactor