Conversation
- 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
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds an intent-extraction interface and enables per-prompt intent extraction via four new IPromptSubmitConfig properties; updates default hook configuration to enable the feature and adjusts unit tests to validate the new defaults. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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.
Pull request overview
This pull request adds foundational support for intent extraction in the prompt-submit hook by introducing the IIntentExtractor interface and extending the IPromptSubmitConfig with configuration fields for LLM-based intent extraction, minimum word thresholds, and memory limits. The PR also changes the default behavior to enable the prompt-submit hook by default.
Changes:
- Introduces
IIntentExtractorinterface defining the contract for extracting searchable keywords from user prompts - Extends
IPromptSubmitConfigwith four new fields:extractIntent,intentTimeout,minWords, andmemoryLimit - Updates default configuration to enable prompt-submit hook and set intent extraction defaults
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/domain/interfaces/IIntentExtractor.ts |
New interface defining input/output contracts for intent extraction service with support for skip scenarios |
src/domain/interfaces/IHookConfig.ts |
Extends IPromptSubmitConfig with intent extraction configuration fields and updates surfaceContext documentation |
src/hooks/utils/config.ts |
Updates defaults to enable prompt-submit hook and configures intent extraction with 3s timeout, 5-word minimum, and 20-memory limit |
tests/unit/hooks/utils/config.test.ts |
Adds test assertions for new config fields to verify defaults are loaded correctly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Result of intent extraction. | ||
| */ | ||
| export interface IIntentExtractorResult { | ||
| /** | ||
| * Extracted keywords for memory search. | ||
| * null if extraction was skipped or no keywords found. | ||
| */ | ||
| readonly intent: string | null; | ||
| /** Whether extraction was skipped. */ | ||
| readonly skipped: boolean; | ||
| /** | ||
| * Reason for skipping. | ||
| * - '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). | ||
| * - 'timeout': LLM call timed out. | ||
| * - 'error': LLM call failed with error. | ||
| */ | ||
| readonly reason?: 'too_short' | 'confirmation' | 'no_llm' | 'llm_skip' | 'timeout' | 'error'; | ||
| } |
There was a problem hiding this comment.
The interface allows inconsistent states between intent, skipped, and reason fields. Consider adding documentation or using a discriminated union to enforce consistency. For example:
- When
skippedistrue,intentshould always benullandreasonshould be defined - When
skippedisfalse,intentmay benullor a string, andreasonshould be undefined
A discriminated union approach would make these relationships explicit and prevent invalid states at compile time.
| * Result of intent extraction. | |
| */ | |
| export interface IIntentExtractorResult { | |
| /** | |
| * Extracted keywords for memory search. | |
| * null if extraction was skipped or no keywords found. | |
| */ | |
| readonly intent: string | null; | |
| /** Whether extraction was skipped. */ | |
| readonly skipped: boolean; | |
| /** | |
| * Reason for skipping. | |
| * - '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). | |
| * - 'timeout': LLM call timed out. | |
| * - 'error': LLM call failed with error. | |
| */ | |
| readonly reason?: 'too_short' | 'confirmation' | 'no_llm' | 'llm_skip' | 'timeout' | 'error'; | |
| } | |
| * Reason for skipping intent extraction. | |
| */ | |
| export type IntentSkipReason = | |
| | 'too_short' | |
| | 'confirmation' | |
| | 'no_llm' | |
| | 'llm_skip' | |
| | 'timeout' | |
| | 'error'; | |
| /** | |
| * Result of intent extraction. | |
| * | |
| * This is a discriminated union keyed by `skipped`: | |
| * - When `skipped` is `true`, `intent` is always `null` and `reason` is defined. | |
| * - When `skipped` is `false`, `intent` may be `null` or a string, and `reason` is undefined. | |
| */ | |
| export type IIntentExtractorResult = | |
| | { | |
| /** Whether extraction was skipped. */ | |
| readonly skipped: true; | |
| /** | |
| * Extracted keywords for memory search. | |
| * Always null when extraction is skipped. | |
| */ | |
| readonly intent: null; | |
| /** | |
| * Reason for skipping. | |
| * - '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). | |
| * - 'timeout': LLM call timed out. | |
| * - 'error': LLM call failed with error. | |
| */ | |
| readonly reason: IntentSkipReason; | |
| } | |
| | { | |
| /** Whether extraction was skipped. */ | |
| readonly skipped: false; | |
| /** | |
| * Extracted keywords for memory search. | |
| * null if no keywords were found. | |
| */ | |
| readonly intent: string | null; | |
| /** | |
| * Reason is not present when extraction is not skipped. | |
| */ | |
| readonly reason?: undefined; | |
| }; |
| /** 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.
The comment on surfaceContext had "Reserved — not yet wired to handler" removed, but these new fields (extractIntent, intentTimeout, minWords, memoryLimit) are also not yet wired to PromptSubmitHandler. For consistency with other config fields in this file, consider marking these as "Reserved — not yet wired to handler" until the implementation is added. This follows the pattern used for recordPrompts on line 28 and other fields like memoryLimit in ISessionStartConfig on line 10.
| /** 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. */ | |
| /** 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.
For consistency with enrichTimeout documentation on line 58, consider adding "Must be under hook timeout (10s)" to clarify the constraint. This helps users understand the operational limit when configuring this timeout value.
| /** 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). */ |
![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
🤖 Fix all issues with AI agents
In `@src/domain/interfaces/IIntentExtractor.ts`:
- Around line 19-36: The current IIntentExtractorResult allows inconsistent
states between skipped, intent, and reason; change it to a discriminated union
type: one variant (e.g., { skipped: true; intent: null; reason:
'too_short'|'confirmation'|'no_llm'|'llm_skip'|'timeout'|'error' }) and another
variant (e.g., { skipped: false; intent: string; reason?: undefined }) to
enforce valid combinations; update the exported type name IIntentExtractorResult
and ensure code using properties (e.g., checks on skipped, access to intent, and
usage in the intent extractor implementation) uses type-narrowing (if/skipped
switch) so callers get correct typings.
Address review feedback: - Change IIntentExtractorResult from interface to discriminated union for better type safety (skipped=true implies intent=null and reason defined) - Add IntentSkipReason type alias - Add hook timeout constraint to intentTimeout doc comment 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-Convention: use discriminated union for IIntentExtractorResult. Address review feedback: AI-Confidence: low AI-Tags: domain, typescript AI-Lifecycle: project AI-Memory-Id: e04f84fb AI-Source: heuristic
|
Addressed review feedback:
|
Summary
IIntentExtractorinterface for extracting searchable keywords from user promptsIPromptSubmitConfigwith new fields:extractIntent,intentTimeout,minWords,memoryLimitChanges
src/domain/interfaces/IIntentExtractor.ts- New interfacesrc/domain/interfaces/IHookConfig.ts- Extended config interfacesrc/hooks/utils/config.ts- Updated defaultstests/unit/hooks/utils/config.test.ts- Updated test assertionsTest plan
Closes GIT-97
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
Tests