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. 📝 WalkthroughWalkthroughThis PR implements intent extraction functionality by introducing a new IntentExtractor service using the Anthropic API to extract searchable keywords from prompts. The result structure is refactored from a discriminated union to a unified interface, and the DI container is updated to register and provide the service with graceful degradation. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller/Hook
participant Extractor as IntentExtractor
participant Timeout as Timeout Handler
participant LLM as Anthropic API
participant Logger as Logger
Caller->>Extractor: extract(prompt)
Extractor->>Extractor: Validate min word count
alt Too Short
Extractor->>Extractor: Mark skipped: too_short
else Confirmation Pattern
Extractor->>Extractor: Mark skipped: confirmation
else Valid Prompt
Extractor->>Timeout: Start timeout race
par Promise Race
Timeout->>LLM: callLLM(prompt)
LLM->>LLM: API request (Haiku)
LLM-->>Timeout: Text response
and
Timeout->>Timeout: Wait timeout duration
end
alt Timeout Exceeded
Timeout->>Logger: Log timeout warning
Timeout->>Extractor: Return skipped: timeout
else LLM Returns SKIP
Extractor->>Extractor: Mark skipped: llm_skip
else Success
Extractor->>Extractor: Parse intent string
else Error
Extractor->>Logger: Log error
Extractor->>Extractor: Mark skipped: error
end
end
Extractor-->>Caller: IIntentExtractorResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
This PR implements the IntentExtractor service that uses Claude Haiku to extract searchable keywords from user prompts for memory retrieval. It introduces the infrastructure implementation of the IIntentExtractor interface (from GIT-97) and registers it in the DI container with graceful degradation when no API key is available.
Changes:
- Added
IntentExtractorclass with LLM-based keyword extraction using Claude Haiku - Registered
intentExtractorin DI container as nullable service (returns null when no ANTHROPIC_API_KEY) - Added new
IPromptSubmitConfigfields for intent extraction configuration (extractIntent, intentTimeout, minWords, memoryLimit)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/domain/interfaces/IIntentExtractor.ts | New domain interface defining the intent extraction service contract with input/output types and skip reasons |
| src/infrastructure/llm/IntentExtractor.ts | Infrastructure implementation using Anthropic SDK with timeout handling, confirmation filtering, and graceful error handling |
| src/infrastructure/di/types.ts | Added intentExtractor to ICradle type as nullable dependency |
| src/infrastructure/di/container.ts | Registered intentExtractor with graceful null return when no API key available |
| src/domain/interfaces/IHookConfig.ts | Extended IPromptSubmitConfig with intent extraction configuration fields |
| src/hooks/utils/config.ts | Updated default config with intent extraction enabled and configuration values |
| tests/unit/hooks/utils/config.test.ts | Updated test assertions to reflect new default configuration values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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]); |
There was a problem hiding this comment.
The timeout is not cleared when the LLM call completes first, which can lead to resource leaks. The existing CommitMsgHandler pattern (lines 165-175 in src/application/handlers/CommitMsgHandler.ts) stores the timeoutId and calls clearTimeout after the Promise.race completes. This implementation should follow the same pattern to prevent the timeout callback from executing unnecessarily.
| 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]); | |
| const extractPromise = this.callLLM(prompt); | |
| return new Promise<string | null>((resolve, reject) => { | |
| const timeoutId = setTimeout( | |
| () => reject(new Error('Intent extraction timed out')), | |
| this.timeout, | |
| ); | |
| Promise.race<Promise<string | null> | Promise<never>>([ | |
| extractPromise, | |
| ]) | |
| .then(result => { | |
| clearTimeout(timeoutId); | |
| resolve(result as string | null); | |
| }) | |
| .catch(error => { | |
| clearTimeout(timeoutId); | |
| reject(error); | |
| }); | |
| }); |
| if (!apiKey) { | ||
| throw new Error('Anthropic API key required for IntentExtractor'); | ||
| } |
There was a problem hiding this comment.
The constructor throws a generic Error instead of a domain-specific error. The existing AnthropicLLMClient uses LLMError (imported from '../../domain/errors/LLMError') when the API key is missing. For consistency, IntentExtractor should either use LLMError or create a similar domain error class.
| 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.
Missing documentation constraint. Following the pattern established for enrichTimeout (line 58 in IHookConfig.ts: "Must be under hook timeout (10s)"), this field should include the same constraint documentation since it's also used within the hook's 10-second 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). */ |
| export class IntentExtractor implements IIntentExtractor { | ||
| private readonly client: Anthropic; | ||
| private readonly timeout: number; | ||
| private readonly minWords: number; | ||
| private readonly logger?: ILogger; | ||
|
|
||
| constructor(options: IIntentExtractorOptions) { | ||
| const apiKey = options.apiKey || process.env.ANTHROPIC_API_KEY; | ||
| if (!apiKey) { | ||
| throw new Error('Anthropic API key required for IntentExtractor'); | ||
| } | ||
|
|
||
| this.client = new Anthropic({ apiKey }); | ||
| this.timeout = options.timeout ?? 3000; | ||
| this.minWords = options.minWords ?? 5; | ||
| this.logger = options.logger; | ||
| } | ||
|
|
||
| async extract(input: IIntentExtractorInput): Promise<IIntentExtractorResult> { | ||
| const prompt = input.prompt.trim(); | ||
|
|
||
| // Check minimum words | ||
| const words = prompt.split(/\s+/).filter(w => w.length > 0); | ||
| if (words.length < this.minWords) { | ||
| this.logger?.debug('Intent extraction skipped: too short', { wordCount: words.length }); | ||
| return { intent: null, skipped: true, reason: 'too_short' }; | ||
| } | ||
|
|
||
| // Check confirmation patterns | ||
| if (CONFIRMATION_PATTERN.test(prompt)) { | ||
| this.logger?.debug('Intent extraction skipped: confirmation'); | ||
| return { intent: null, skipped: true, reason: 'confirmation' }; | ||
| } | ||
|
|
||
| // Extract via LLM with timeout | ||
| try { | ||
| const intent = await this.extractWithTimeout(prompt); | ||
|
|
||
| if (!intent || intent.toUpperCase() === 'SKIP') { | ||
| this.logger?.debug('Intent extraction: LLM returned SKIP'); | ||
| return { intent: null, skipped: true, reason: 'llm_skip' }; | ||
| } | ||
|
|
||
| this.logger?.debug('Intent extracted', { intent }); | ||
| return { intent, skipped: false }; | ||
| } catch (error) { | ||
| const isTimeout = error instanceof Error && error.message.includes('timed out'); | ||
| this.logger?.warn('Intent extraction failed', { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| isTimeout, | ||
| }); | ||
| return { | ||
| intent: null, | ||
| skipped: true, | ||
| reason: isTimeout ? 'timeout' : 'error', | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Call LLM with timeout enforcement. | ||
| */ | ||
| 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}"` }], | ||
| }); | ||
|
|
||
| const text = response.content | ||
| .filter((block): block is Anthropic.TextBlock => block.type === 'text') | ||
| .map(block => block.text) | ||
| .join('') | ||
| .trim(); | ||
|
|
||
| return text || null; | ||
| } | ||
| } |
There was a problem hiding this comment.
No unit tests exist for IntentExtractor, despite the presence of comprehensive test coverage for AnthropicLLMClient (258 lines in tests/unit/infrastructure/llm/AnthropicLLMClient.test.ts). Tests should cover: constructor validation (API key handling), minWords threshold logic, confirmation pattern matching, timeout behavior, LLM skip response handling, and error cases.
| constructor(options: IIntentExtractorOptions) { | ||
| const apiKey = options.apiKey || process.env.ANTHROPIC_API_KEY; | ||
| if (!apiKey) { | ||
| throw new Error('Anthropic API key required for IntentExtractor'); | ||
| } | ||
|
|
||
| this.client = new Anthropic({ apiKey }); | ||
| this.timeout = options.timeout ?? 3000; | ||
| this.minWords = options.minWords ?? 5; | ||
| this.logger = options.logger; |
There was a problem hiding this comment.
Design mismatch: IntentExtractor accepts timeout and minWords in the constructor (lines 20-23, 52-53), but the DI container creates it without config (line 121-124 doesn't pass these options). Unlike llmClient which accepts config per-call, IntentExtractor's config is fixed at construction. Since hook config is only available at runtime in handlers (see CommitMsgHandler line 48-49 pattern), the configured values (intentTimeout, minWords from IPromptSubmitConfig) cannot be used. Either: (1) Add timeout/minWords parameters to the extract() method, or (2) Make intentExtractor a factory function that accepts config.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/infrastructure/di/container.ts`:
- Around line 114-128: The DI registration for intentExtractor either needs to
accept and pass the promptSubmit config values into IntentExtractor or be
removed if unused: update the intentExtractor asFunction factory to read
promptSubmit.intentTimeout and promptSubmit.minWords from your config (e.g., the
same source that defines promptSubmit defaults) and pass them as timeout and
minWords into new IntentExtractor({ apiKey, logger: container.cradle.logger,
timeout: <intentTimeout>, minWords: <minWords> }), and ensure any service that
relies on intent extraction obtains container.cradle.intentExtractor;
alternatively, if no service consumes container.cradle.intentExtractor, delete
the intentExtractor registration to avoid dead code.
In `@src/infrastructure/llm/IntentExtractor.ts`:
- Around line 101-108: In extractWithTimeout, the timeout timer created for
timeoutPromise can continue to run and later reject after Promise.race settles;
modify extractWithTimeout to store the timer id when creating timeoutPromise
(instead of an inline setTimeout), race extractPromise and timeoutPromise, and
ensure you clearTimeout(timerId) after the race settles (in a finally block or
via .then/.catch) so the timer is cancelled and no unhandled rejection occurs;
reference extractWithTimeout, timeoutPromise, extractPromise and callLLM when
locating the change.
| intentExtractor: asFunction(() => { | ||
| // Intent extraction requires an API key. Return null for graceful degradation. | ||
| const apiKey = process.env.ANTHROPIC_API_KEY; | ||
| if (!apiKey) { | ||
| return null; | ||
| } | ||
| try { | ||
| return new IntentExtractor({ | ||
| apiKey, | ||
| logger: container.cradle.logger, | ||
| }); | ||
| } catch { | ||
| return null; | ||
| } | ||
| }).singleton(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for usages of intentTimeout/minWords/extractIntent across the codebase
rg -n --type=ts '\bintentTimeout\b|\bminWords\b|\bextractIntent\b' | head -100Repository: TonyCasey/git-mem
Length of output: 1197
🏁 Script executed:
#!/bin/bash
# Also search for IntentExtractor class definition and usage
rg -n --type=ts 'class IntentExtractor|IntentExtractor(' | head -50Repository: TonyCasey/git-mem
Length of output: 187
🏁 Script executed:
#!/bin/bash
# Search for promptSubmit config references
rg -n --type=ts 'promptSubmit' | head -50Repository: TonyCasey/git-mem
Length of output: 3445
🏁 Script executed:
#!/bin/bash
# Check the IntentExtractor class definition to see what parameters it accepts
cat -n src/infrastructure/llm/IntentExtractor.ts | head -60Repository: TonyCasey/git-mem
Length of output: 2777
🏁 Script executed:
#!/bin/bash
# Check where IntentExtractor is instantiated - look for "new IntentExtractor"
rg -n --type=ts 'new IntentExtractor' -A5Repository: TonyCasey/git-mem
Length of output: 441
🏁 Script executed:
#!/bin/bash
# Check the container.ts file around the intentExtractor registration
cat -n src/infrastructure/di/container.ts | sed -n '100,150p'Repository: TonyCasey/git-mem
Length of output: 2305
🏁 Script executed:
#!/bin/bash
# Check how intentExtractor is used in the hooks layer
rg -n --type=ts 'intentExtractor|extractIntent' src/hooks --context=3Repository: TonyCasey/git-mem
Length of output: 436
🏁 Script executed:
#!/bin/bash
# Check if the hook config is passed to the DI container
rg -n --type=ts 'hookConfig|HookConfig' src/infrastructure/di --context=2Repository: TonyCasey/git-mem
Length of output: 2030
🏁 Script executed:
#!/bin/bash
# Check the entire container.ts file to see what options are passed in
cat -n src/infrastructure/di/container.ts | head -50Repository: TonyCasey/git-mem
Length of output: 2798
🏁 Script executed:
#!/bin/bash
# Check where intentExtractor is actually used (dependencies)
rg -n --type=ts 'intentExtractor|IIntentExtractor' src/application --context=3Repository: TonyCasey/git-mem
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check PromptSubmitHandler to see how it uses intentExtractor
cat -n src/application/handlers/PromptSubmitHandler.ts | head -100Repository: TonyCasey/git-mem
Length of output: 2447
🏁 Script executed:
#!/bin/bash
# Check if hookConfig is somehow passed to PromptSubmitHandler
rg -n --type=ts 'PromptSubmitHandler' src --context=5Repository: TonyCasey/git-mem
Length of output: 7080
🏁 Script executed:
#!/bin/bash
# Check if intentExtractor is actually used/injected anywhere in the codebase
rg -n --type=ts 'intentExtractor' src --context=2 | grep -v 'asFunction\|singleton\|test'Repository: TonyCasey/git-mem
Length of output: 639
🏁 Script executed:
#!/bin/bash
# Check if the config values are actually read and used anywhere
rg -n --type=ts 'intentTimeout|minWords' src/infrastructure/di --context=3Repository: TonyCasey/git-mem
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check all services that depend on intentExtractor in the DI container
cat -n src/infrastructure/di/container.ts | sed -n '50,150p'Repository: TonyCasey/git-mem
Length of output: 4454
🏁 Script executed:
#!/bin/bash
# Search for any service that has intentExtractor as a constructor dependency
rg -n --type=ts 'intentExtractor|IIntentExtractor' src/application src/infrastructure/services --context=3Repository: TonyCasey/git-mem
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if intentExtractor is ever retrieved from the container after registration
rg -n --type=ts 'container.cradle.intentExtractor' srcRepository: TonyCasey/git-mem
Length of output: 43
Wire intentTimeout and minWords from hook config to IntentExtractor, or remove unused registration.
promptSubmit.intentTimeout and promptSubmit.minWords are defined in config defaults (lines 50-51 in src/hooks/utils/config.ts), and IntentExtractor accepts these as timeout and minWords options. However, the DI container instantiation (lines 121-124 in src/infrastructure/di/container.ts) passes only apiKey and logger, ignoring the config values. Additionally, intentExtractor is registered in the container but appears unused—no service depends on it or retrieves it via container.cradle.intentExtractor. Either thread the config values through the container and wire them into services that use intentExtractor, or remove the registration if it's not needed.
🤖 Prompt for AI Agents
In `@src/infrastructure/di/container.ts` around lines 114 - 128, The DI
registration for intentExtractor either needs to accept and pass the
promptSubmit config values into IntentExtractor or be removed if unused: update
the intentExtractor asFunction factory to read promptSubmit.intentTimeout and
promptSubmit.minWords from your config (e.g., the same source that defines
promptSubmit defaults) and pass them as timeout and minWords into new
IntentExtractor({ apiKey, logger: container.cradle.logger, timeout:
<intentTimeout>, minWords: <minWords> }), and ensure any service that relies on
intent extraction obtains container.cradle.intentExtractor; alternatively, if no
service consumes container.cradle.intentExtractor, delete the intentExtractor
registration to avoid dead code.
|
Addressed review feedback:
|
- Add IIntentExtractor interface for keyword extraction from prompts - Extend IPromptSubmitConfig with extractIntent, intentTimeout, minWords, and memoryLimit fields - Update config defaults: enable promptSubmit by default, extractIntent=true, intentTimeout=3000ms, minWords=5, memoryLimit=20 - Update config tests for new defaults 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 IIntentExtractor interface and config fields (GIT-97). - Add IIntentExtractor interface for keyword extraction from prompts AI-Confidence: medium AI-Tags: domain, hooks, utils, tests, unit AI-Lifecycle: project AI-Memory-Id: c64a5417 AI-Source: heuristic
- 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
Address review feedback: - Clear timeout after Promise.race to avoid lingering timers - Use LLMError instead of generic Error for consistency with AnthropicLLMClient 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-Gotcha: Timeouts should be cleared after Promise.race to prevent lingering timers that could cause memory leaks or unexpected behavior. AI-Confidence: verified AI-Tags: timeout, promise-race, memory-management, async, error-handling, llm, consistency, domain-errors, infrastructure AI-Lifecycle: project AI-Memory-Id: f1a98c73 AI-Source: llm-enrichment
Summary
IntentExtractorclass ininfrastructure/llmthat extracts searchable keywords from user prompts using Claude HaikuintentExtractorin DI container (nullable for graceful degradation when no API key)IIntentExtractortoICradletypesFeatures
Changes
src/infrastructure/llm/IntentExtractor.ts- New service implementationsrc/infrastructure/di/types.ts- Added intentExtractor to ICradlesrc/infrastructure/di/container.ts- Registered intentExtractorTest plan
Note: Requires GIT-97 to be merged first (depends on IIntentExtractor interface).
Closes GIT-98
🤖 Generated with Claude Code
Summary by CodeRabbit