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. 📝 WalkthroughWalkthroughThese changes add comprehensive unit tests for PromptSubmitHandler and IntentExtractor functionality, introducing test scaffolding for config loading and intent extraction, organizing tests into nested describe blocks, and validating config-driven behavior, intent extraction interactions, regex patterns, and word-count validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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 retrieval for the prompt:submit flow (via an Anthropic-backed IntentExtractor) and expands unit test coverage around prompt-submit config/intent behavior.
Changes:
- Introduces
IIntentExtractor+IntentExtractor(Anthropic SDK) and wires it through DI +PromptSubmitHandler. - Adds query-based memory loading (
IMemoryContextLoader.loadWithQuery) backed byMemoryService.recall()(notes + trailers). - Updates hook config defaults/schema for prompt-submit intent extraction and adds/updates unit tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/infrastructure/llm/IntentExtractor.test.ts | Adds tests for confirmation regex + word-count logic (currently not exercising IntentExtractor.extract()). |
| tests/unit/hooks/utils/config.test.ts | Updates config default expectations for prompt-submit (now enabled + intent settings). |
| tests/unit/application/handlers/PromptSubmitHandler.test.ts | Adds unit tests covering surfaceContext/memoryLimit and intent-extraction decision paths. |
| src/infrastructure/llm/IntentExtractor.ts | New Anthropic-based intent extractor with min-word + confirmation skipping and timeout wrapper. |
| src/infrastructure/di/types.ts | Registers intentExtractor in the DI cradle. |
| src/infrastructure/di/container.ts | Wires intentExtractor into PromptSubmitHandler and registers extractor singleton (nullable). |
| src/hooks/utils/config.ts | Expands prompt-submit defaults (enabled + intent extraction fields). |
| src/domain/interfaces/IMemoryContextLoader.ts | Adds loadWithQuery() to the loader interface. |
| src/domain/interfaces/IIntentExtractor.ts | New domain interface for intent extraction results/reasons. |
| src/domain/interfaces/IHookConfig.ts | Extends promptSubmit config to include intent extraction controls and memoryLimit. |
| src/application/services/MemoryContextLoader.ts | Implements loadWithQuery() using MemoryService.recall(). |
| src/application/handlers/PromptSubmitHandler.ts | Loads prompt-submit config, conditionally extracts intent, and chooses loadWithQuery() vs load(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| promptSubmit: { | ||
| enabled: true, | ||
| recordPrompts: false, | ||
| surfaceContext: true, | ||
| extractIntent: true, | ||
| intentTimeout: 3000, | ||
| minWords: 5, | ||
| memoryLimit: 20, | ||
| }, |
There was a problem hiding this comment.
PR metadata indicates this change is about adding unit tests, but this hunk changes runtime defaults (enabling promptSubmit and turning on extractIntent by default) and introduces several new config fields. If the intent is to ship new behavior, the PR title/description should be updated to reflect that; if not, the defaults should remain unchanged and be moved to the feature PR.
| // Get total count for stats | ||
| const allResult = this.memoryRepository.query({ cwd }); | ||
| const total = allResult.total; | ||
|
|
||
| // Search with query | ||
| const result = this.memoryService.recall(query, { limit, cwd }); |
There was a problem hiding this comment.
loadWithQuery() computes total via memoryRepository.query() (notes) even though the actual search uses memoryService.recall() (notes + trailers). This can make total inconsistent with the rest of the system (and inconsistent with load() semantics) when trailer-backed memories exist. Consider deriving total from memoryService (e.g., a “total store” query that includes trailers) or clearly documenting that total only counts note-backed memories.
| // Get total count for stats | |
| const allResult = this.memoryRepository.query({ cwd }); | |
| const total = allResult.total; | |
| // Search with query | |
| const result = this.memoryService.recall(query, { limit, cwd }); | |
| // Search with query across the same backing store that defines `total` | |
| const result = this.memoryService.recall(query, { limit, cwd }); | |
| const total = result.memories.length; |
| // Load config | ||
| const config = this.hookConfigLoader?.loadConfig(event.cwd); | ||
| const promptConfig = config?.hooks.promptSubmit ?? { | ||
| surfaceContext: true, | ||
| extractIntent: false, | ||
| memoryLimit: 20, | ||
| minWords: 5, | ||
| intentTimeout: 3000, | ||
| }; |
There was a problem hiding this comment.
promptConfig includes minWords and intentTimeout (from config defaults and YAML), but these values are never applied to the IntentExtractor. As a result, user config changes to these fields have no effect and the values are misleading. Consider wiring these through (e.g., constructing/configuring the extractor using the repo config, or letting extract() accept per-call overrides), or removing the fields from config until they’re actually used.
| // 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' }; | ||
| } |
There was a problem hiding this comment.
The confirmation check is currently unreachable for typical confirmations under the default minWords (e.g., "yes", "ok", "thank you" will return reason: 'too_short' before the confirmation pattern is evaluated). If you want confirmations to be classified as reason: 'confirmation', check CONFIRMATION_PATTERN before enforcing minWords, or exempt confirmations from the word-count threshold.
| 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.
extractWithTimeout() starts a timer but never cancels it when the LLM call finishes first. This leaves unnecessary timers running on the happy path and can become noisy under frequent prompt submissions. Consider capturing the timeout handle and clearTimeout() in a finally, and (optionally) aborting the in-flight Anthropic request when the timeout fires (e.g., via AbortController if supported).
| 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]); | |
| let timeoutId: NodeJS.Timeout | undefined; | |
| const timeoutPromise = new Promise<never>((_, reject) => { | |
| timeoutId = setTimeout( | |
| () => reject(new Error('Intent extraction timed out')), | |
| this.timeout, | |
| ); | |
| }); | |
| const extractPromise = this.callLLM(prompt); | |
| try { | |
| return await Promise.race([extractPromise, timeoutPromise]); | |
| } finally { | |
| if (timeoutId !== undefined) { | |
| clearTimeout(timeoutId); | |
| } | |
| } |
|
|
||
| // Test the confirmation pattern matching logic directly | ||
| describe('IntentExtractor', () => { | ||
| // The CONFIRMATION_PATTERN from IntentExtractor.ts | ||
| const CONFIRMATION_PATTERN = /^(yes|no|ok|okay|go|sure|proceed|continue|done|y|n|yep|nope|thanks|thank you|\d+)$/i; |
There was a problem hiding this comment.
These tests re-declare CONFIRMATION_PATTERN (and implement their own word-count helper) instead of asserting behavior via the actual IntentExtractor.extract() API. That means the tests can still pass even if production logic changes, and they don’t cover key behaviors like skip reasons (too_short/confirmation/timeout) or handling of SKIP responses. Prefer testing extract() directly (mocking Anthropic) or exporting the shared pattern/word-count helper from production code so the tests can’t drift.
| // Test the confirmation pattern matching logic directly | |
| describe('IntentExtractor', () => { | |
| // The CONFIRMATION_PATTERN from IntentExtractor.ts | |
| const CONFIRMATION_PATTERN = /^(yes|no|ok|okay|go|sure|proceed|continue|done|y|n|yep|nope|thanks|thank you|\d+)$/i; | |
| import { CONFIRMATION_PATTERN } from '../../../../src/infrastructure/llm/IntentExtractor'; | |
| // Test the confirmation pattern matching logic directly | |
| describe('IntentExtractor', () => { | |
| // The CONFIRMATION_PATTERN from IntentExtractor.ts |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/application/services/MemoryContextLoader.ts`:
- Around line 53-77: In loadWithQuery, the total count is taken from
memoryRepository.query({ cwd }) which excludes trailer memories; instead use the
total returned by memoryService.recall. After calling result =
this.memoryService.recall(query, { limit, cwd }), set total = result.total and
use that total in the debug log and returned object (memories/total/filtered) so
stats reflect the merged notes+trailers; update references to
memoryRepository.query({ cwd }).total accordingly.
In `@src/infrastructure/di/container.ts`:
- Around line 116-130: The catch in the intentExtractor factory silently returns
null; update the asFunction factory for intentExtractor to catch the thrown
error from new IntentExtractor(...) and log it via the container.cradle.logger
(or container.cradle.logger.error) before returning null so failures during
IntentExtractor construction (invalid key, runtime errors) are recorded;
reference the intentExtractor asFunction block and the IntentExtractor
constructor and use the existing container.cradle.logger for the log entry.
In `@src/infrastructure/llm/IntentExtractor.ts`:
- Around line 101-109: The timeout created in extractWithTimeout (the setTimeout
inside timeoutPromise) isn't cleared when extractPromise (from callLLM)
resolves; update extractWithTimeout to capture the timer ID and ensure
clearTimeout is called when either promise settles (on success or rejection) so
the timer is cleared to avoid leftover timers. Specifically, modify
extractWithTimeout to use a single Promise that races callLLM(prompt) against a
setTimeout but calls clearTimeout(timerId) when callLLM resolves or when the
timeout handler runs, or alternatively wrap both results so you clearTimeout in
the finally/settle path; reference extractWithTimeout, callLLM, and the
timeoutPromise/setTimeout usage to locate the change. Ensure behavior and return
types remain unchanged.
In `@tests/unit/application/handlers/PromptSubmitHandler.test.ts`:
- Around line 326-348: Add a unit test covering the branch where
configLoader.extractIntent is true but event.prompt is empty: instantiate
PromptSubmitHandler with a mock loader (track loadCalled), a mock
intentExtractor (track extractCalled), and configLoader with extractIntent:
true, then call handler.handle(createEvent({ prompt: '' })) and assert that
loader.load was called and intentExtractor.extract was not; this verifies the
handler checks event.prompt truthiness before calling intentExtractor.extract.
In `@tests/unit/infrastructure/llm/IntentExtractor.test.ts`:
- Around line 13-109: The tests are duplicating production logic
(CONFIRMATION_PATTERN and the local countWords) which risks drift; either export
CONFIRMATION_PATTERN and the word-count helper from IntentExtractor (so tests
import and assert against the real CONFIRMATION_PATTERN and exported wordCount
function) or refactor tests to call IntentExtractor.extract with a stubbed LLM
and assert behavior end-to-end; update tests to remove the locally reimplemented
regex and countWords and reference the exported symbols (CONFIRMATION_PATTERN,
wordCount, IntentExtractor.extract) instead.
| 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 }); | ||
| } | ||
|
|
||
| // Get total count for stats | ||
| const allResult = this.memoryRepository.query({ cwd }); | ||
| const total = allResult.total; | ||
|
|
||
| // Search with query | ||
| const result = this.memoryService.recall(query, { limit, cwd }); | ||
|
|
||
| this.logger?.debug('Memories loaded with query', { | ||
| query, | ||
| total, | ||
| matched: result.memories.length, | ||
| limit, | ||
| }); | ||
|
|
||
| return { | ||
| memories: result.memories, | ||
| total, | ||
| filtered: result.memories.length, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "interface IMemoryService" src -g '*.ts'Repository: TonyCasey/git-mem
Length of output: 142
🏁 Script executed:
rg -n "recall\s*\(" src -g '*.ts' -A 3Repository: TonyCasey/git-mem
Length of output: 2400
🏁 Script executed:
cat -n src/application/services/MemoryContextLoader.ts | head -80Repository: TonyCasey/git-mem
Length of output: 3209
🏁 Script executed:
rg -n "interface IMemoryQueryResult" src -g '*.ts' -A 5Repository: TonyCasey/git-mem
Length of output: 529
🏁 Script executed:
cat -n src/application/interfaces/IMemoryService.ts | head -40Repository: TonyCasey/git-mem
Length of output: 1535
🏁 Script executed:
cat -n src/application/services/MemoryService.ts | sed -n '87,115p'Repository: TonyCasey/git-mem
Length of output: 1499
Use result.total from recall instead of repository total.
loadWithQuery currently uses memoryRepository.query({ cwd }).total for the total count, which includes notes only. Since memoryService.recall merges both notes and trailers (line 105 of MemoryService), the returned result already contains an accurate total count that accounts for trailer memories. Use result.total instead to keep stats consistent with the actual matched results.
🤖 Prompt for AI Agents
In `@src/application/services/MemoryContextLoader.ts` around lines 53 - 77, In
loadWithQuery, the total count is taken from memoryRepository.query({ cwd })
which excludes trailer memories; instead use the total returned by
memoryService.recall. After calling result = this.memoryService.recall(query, {
limit, cwd }), set total = result.total and use that total in the debug log and
returned object (memories/total/filtered) so stats reflect the merged
notes+trailers; update references to memoryRepository.query({ cwd }).total
accordingly.
| 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.
🧹 Nitpick | 🔵 Trivial
Consider logging when IntentExtractor construction fails.
The catch block silently returns null without any logging. If construction fails for reasons other than a missing API key (e.g., invalid key format), this could make debugging difficult.
🔧 Proposed enhancement
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 {
+ } catch (error) {
+ container.cradle.logger?.warn('IntentExtractor construction failed', {
+ error: error instanceof Error ? error.message : String(error),
+ });
return null;
}
}).singleton(),🤖 Prompt for AI Agents
In `@src/infrastructure/di/container.ts` around lines 116 - 130, The catch in the
intentExtractor factory silently returns null; update the asFunction factory for
intentExtractor to catch the thrown error from new IntentExtractor(...) and log
it via the container.cradle.logger (or container.cradle.logger.error) before
returning null so failures during IntentExtractor construction (invalid key,
runtime errors) are recorded; reference the intentExtractor asFunction block and
the IntentExtractor constructor and use the existing container.cradle.logger for
the log entry.
| 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]); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Consider clearing the timeout on success.
The setTimeout timer isn't cleared if extractPromise resolves before the timeout. While this won't cause functional issues (the rejection is ignored), it's a minor resource inefficiency. For a 3-second timeout this is negligible, but worth noting for best practices.
🔧 Optional improvement using AbortController pattern
private async extractWithTimeout(prompt: string): Promise<string | null> {
+ let timeoutId: NodeJS.Timeout;
const timeoutPromise = new Promise<never>((_, reject) => {
- setTimeout(() => reject(new Error('Intent extraction timed out')), this.timeout);
+ timeoutId = setTimeout(() => reject(new Error('Intent extraction timed out')), this.timeout);
});
const extractPromise = this.callLLM(prompt);
- return Promise.race([extractPromise, timeoutPromise]);
+ try {
+ return await Promise.race([extractPromise, timeoutPromise]);
+ } finally {
+ clearTimeout(timeoutId!);
+ }
}🤖 Prompt for AI Agents
In `@src/infrastructure/llm/IntentExtractor.ts` around lines 101 - 109, The
timeout created in extractWithTimeout (the setTimeout inside timeoutPromise)
isn't cleared when extractPromise (from callLLM) resolves; update
extractWithTimeout to capture the timer ID and ensure clearTimeout is called
when either promise settles (on success or rejection) so the timer is cleared to
avoid leftover timers. Specifically, modify extractWithTimeout to use a single
Promise that races callLLM(prompt) against a setTimeout but calls
clearTimeout(timerId) when callLLM resolves or when the timeout handler runs, or
alternatively wrap both results so you clearTimeout in the finally/settle path;
reference extractWithTimeout, callLLM, and the timeoutPromise/setTimeout usage
to locate the change. Ensure behavior and return types remain unchanged.
| it('should use load when intentExtractor is null', async () => { | ||
| let loadCalled = false; | ||
| const loader: IMemoryContextLoader = { | ||
| load: () => { | ||
| loadCalled = true; | ||
| return { memories: [], total: 0, filtered: 0 }; | ||
| }, | ||
| loadWithQuery: () => ({ memories: [], total: 0, filtered: 0 }), | ||
| }; | ||
| const formatter = createMockFormatter(''); | ||
| const configLoader = createMockConfigLoader({ extractIntent: true }); | ||
| const handler = new PromptSubmitHandler( | ||
| loader, | ||
| formatter, | ||
| undefined, | ||
| configLoader, | ||
| null, // No intent extractor available | ||
| ); | ||
|
|
||
| const result = await handler.handle(createEvent()); | ||
| await handler.handle(createEvent()); | ||
|
|
||
| assert.equal(result.success, false); | ||
| assert.equal(result.error!.message, 'format failed'); | ||
| assert.ok(loadCalled, 'load should be called'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding test for empty/missing prompt with intent extraction enabled.
The handler at line 62 checks event.prompt truthiness before extraction. When extractIntent: true and intentExtractor is present but event.prompt is empty/undefined, it falls back to load. This path isn't explicitly tested.
📝 Suggested test case
it('should use load when prompt is empty even with extractIntent enabled', async () => {
let loadCalled = false;
let extractCalled = false;
const loader: IMemoryContextLoader = {
load: () => {
loadCalled = true;
return { memories: [], total: 0, filtered: 0 };
},
loadWithQuery: () => ({ memories: [], total: 0, filtered: 0 }),
};
const formatter = createMockFormatter('');
const configLoader = createMockConfigLoader({ extractIntent: true });
const intentExtractor: IIntentExtractor = {
extract: async () => {
extractCalled = true;
return { intent: 'keywords', skipped: false };
},
};
const handler = new PromptSubmitHandler(
loader,
formatter,
undefined,
configLoader,
intentExtractor,
);
await handler.handle(createEvent({ prompt: '' }));
assert.ok(loadCalled, 'load should be called');
assert.ok(!extractCalled, 'extract should not be called when prompt is empty');
});🤖 Prompt for AI Agents
In `@tests/unit/application/handlers/PromptSubmitHandler.test.ts` around lines 326
- 348, Add a unit test covering the branch where configLoader.extractIntent is
true but event.prompt is empty: instantiate PromptSubmitHandler with a mock
loader (track loadCalled), a mock intentExtractor (track extractCalled), and
configLoader with extractIntent: true, then call handler.handle(createEvent({
prompt: '' })) and assert that loader.load was called and
intentExtractor.extract was not; this verifies the handler checks event.prompt
truthiness before calling intentExtractor.extract.
| // The CONFIRMATION_PATTERN from IntentExtractor.ts | ||
| const CONFIRMATION_PATTERN = /^(yes|no|ok|okay|go|sure|proceed|continue|done|y|n|yep|nope|thanks|thank you|\d+)$/i; | ||
|
|
||
| describe('confirmation pattern', () => { | ||
| it('should match "yes"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('yes')); | ||
| }); | ||
|
|
||
| it('should match "no"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('no')); | ||
| }); | ||
|
|
||
| it('should match "ok"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('ok')); | ||
| }); | ||
|
|
||
| it('should match "okay"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('okay')); | ||
| }); | ||
|
|
||
| it('should match "go"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('go')); | ||
| }); | ||
|
|
||
| it('should match "sure"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('sure')); | ||
| }); | ||
|
|
||
| it('should match "proceed"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('proceed')); | ||
| }); | ||
|
|
||
| it('should match "continue"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('continue')); | ||
| }); | ||
|
|
||
| it('should match "done"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('done')); | ||
| }); | ||
|
|
||
| it('should match "y"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('y')); | ||
| }); | ||
|
|
||
| it('should match "n"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('n')); | ||
| }); | ||
|
|
||
| it('should match "yep"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('yep')); | ||
| }); | ||
|
|
||
| it('should match "nope"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('nope')); | ||
| }); | ||
|
|
||
| it('should match "thanks"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('thanks')); | ||
| }); | ||
|
|
||
| it('should match "thank you"', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('thank you')); | ||
| }); | ||
|
|
||
| it('should match single digit', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('1')); | ||
| }); | ||
|
|
||
| it('should match multi-digit number', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('42')); | ||
| }); | ||
|
|
||
| it('should be case-insensitive', () => { | ||
| assert.ok(CONFIRMATION_PATTERN.test('YES')); | ||
| assert.ok(CONFIRMATION_PATTERN.test('Yes')); | ||
| assert.ok(CONFIRMATION_PATTERN.test('NO')); | ||
| assert.ok(CONFIRMATION_PATTERN.test('OK')); | ||
| }); | ||
|
|
||
| it('should NOT match substantive prompts', () => { | ||
| assert.ok(!CONFIRMATION_PATTERN.test('fix the bug')); | ||
| assert.ok(!CONFIRMATION_PATTERN.test('yes please fix it')); | ||
| assert.ok(!CONFIRMATION_PATTERN.test('start on GIT-95')); | ||
| assert.ok(!CONFIRMATION_PATTERN.test('implement authentication')); | ||
| }); | ||
|
|
||
| it('should NOT match partial matches', () => { | ||
| assert.ok(!CONFIRMATION_PATTERN.test('yes please')); | ||
| assert.ok(!CONFIRMATION_PATTERN.test('okay then')); | ||
| assert.ok(!CONFIRMATION_PATTERN.test('continue with')); | ||
| }); | ||
| }); | ||
|
|
||
| describe('word count logic', () => { | ||
| function countWords(prompt: string): number { | ||
| return prompt.trim().split(/\s+/).filter(w => w.length > 0).length; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests reimplement production logic; risk of drift.
These assertions duplicate the regex and word-count logic rather than verifying the actual IntentExtractor implementation. Consider exporting the confirmation pattern/word-count helper (or exercising IntentExtractor.extract with a stubbed LLM) so tests stay aligned with production behavior.
🤖 Prompt for AI Agents
In `@tests/unit/infrastructure/llm/IntentExtractor.test.ts` around lines 13 - 109,
The tests are duplicating production logic (CONFIRMATION_PATTERN and the local
countWords) which risks drift; either export CONFIRMATION_PATTERN and the
word-count helper from IntentExtractor (so tests import and assert against the
real CONFIRMATION_PATTERN and exported wordCount function) or refactor tests to
call IntentExtractor.extract with a stubbed LLM and assert behavior end-to-end;
update tests to remove the locally reimplemented regex and countWords and
reference the exported symbols (CONFIRMATION_PATTERN, wordCount,
IntentExtractor.extract) instead.
|
Most comments in this PR are duplicates of issues raised in other PRs in this series:
|
- Update PromptSubmitHandler tests with intent extraction scenarios - Add tests for config handling (surfaceContext, memoryLimit) - Add tests for loadWithQuery vs load fallback - Add IntentExtractor tests for confirmation pattern and word count logic Test coverage: - Intent extraction enabled + intent found → loadWithQuery - Intent extraction enabled + skipped → load fallback - Intent extraction disabled → load (no extract call) - No intent extractor available → load fallback - surfaceContext disabled → empty output - memoryLimit from config respected 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: and word count logic AI-Confidence: medium AI-Tags: tests, unit, application, handlers, infrastructure, llm, pattern:convention AI-Lifecycle: project AI-Memory-Id: 112209d6 AI-Source: heuristic
Summary
Add comprehensive unit tests for the intent extraction feature.
Test Coverage
PromptSubmitHandler Tests
loadWithQueryloadload, no extract callloadfallbackIntentExtractor Tests
Test Results
Dependencies
Closes GIT-102
🤖 Generated with Claude Code
Summary by CodeRabbit