feat(cli): enhanced AI provider stack#29
Closed
ntanwir10 wants to merge 13 commits into
Closed
Conversation
…ience Add decorator-based providers (retry, circuit breaker, rate limit, cache, observability), model registry/router, cost guard, usage tracking, and new models/routing/budget/metrics/cache commands. Wire createForCli across AI commands and expand config with enhanced defaults. Co-authored-by: Cursor <cursoragent@cursor.com>
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
guardscan-backend | 9a1df6b | Jun 12 2026, 02:06 AM |
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 tasks
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces robust AI management features to the GuardScan CLI, including smart model routing, semantic caching, budget controls, reliability decorators (retry, rate limiting, circuit breaker), and observability metrics. The code review identified several critical issues and improvement opportunities: potential content duplication during stream retries, parsing errors in Ollama streaming due to unbuffered chunks, a redundant spinlock in the rate limiter, potential TypeErrors from unvalidated inputs in token counting and Gemini streaming, a lack of caching for streaming requests, blocking synchronous I/O in the metrics collector, unescaped fields in the CSV exporter, and duplicated progress bar helpers prone to RangeErrors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+35
to
+62
| this.config = { ...DEFAULT_RETRY_CONFIG, ...config }; | ||
| } | ||
|
|
||
| /** | ||
| * Chat with automatic retry on failure | ||
| */ | ||
| async chat( | ||
| messages: AIMessage[], | ||
| options?: ChatOptions | ||
| ): Promise<AIResponse> { | ||
| return this.retryWithBackoff( | ||
| () => this.wrapped.chat(messages, options), | ||
| 'chat' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Stream with automatic retry on failure | ||
| * Note: Streaming retry is more complex as we need to handle partial responses | ||
| */ | ||
| async *stream( | ||
| messages: AIMessage[], | ||
| options?: ChatOptions | ||
| ): AsyncGenerator<string, void, unknown> { | ||
| let attempt = 0; | ||
| let lastError: any; | ||
|
|
||
| while (attempt <= this.config.maxRetries) { |
Contributor
There was a problem hiding this comment.
Retrying a partially consumed stream results in duplicated content being yielded to the caller. If the stream fails mid-way (after yielding some chunks), starting a new stream from the beginning will re-yield the entire response. To prevent this, we should track if any chunks have already been yielded, and if so, disable retry and propagate the error immediately.
async *stream(
messages: AIMessage[],
options?: ChatOptions
): AsyncGenerator<string, void, unknown> {
let attempt = 0;
let lastError: any;
let yieldedAny = false;
while (attempt <= this.config.maxRetries) {
try {
for await (const chunk of this.wrapped.stream(messages, options)) {
yieldedAny = true;
yield chunk;
}
return; // Success, exit
} catch (error: any) {
lastError = error;
if (yieldedAny || !this.isRetryable(error) || attempt >= this.config.maxRetries) {
throw error;
}
// Calculate delay and wait
const delay = this.calculateDelay(attempt);
await this.sleep(delay);
attempt++;
}
}
throw lastError;
}
Comment on lines
+41
to
+76
| messages: AIMessage[], | ||
| options?: ChatOptions | ||
| ): AsyncGenerator<string, void, unknown> { | ||
| const response = await axios.post( | ||
| `${this.endpoint}/api/chat`, | ||
| { | ||
| model: options?.model || this.defaultModel, | ||
| messages: messages.map(msg => ({ | ||
| role: msg.role, | ||
| content: msg.content, | ||
| })), | ||
| stream: true, | ||
| }, | ||
| { | ||
| responseType: 'stream', | ||
| } | ||
| ); | ||
|
|
||
| const stream = response.data; | ||
|
|
||
| for await (const chunk of stream) { | ||
| const lines = chunk.toString().split('\n').filter((line: string) => line.trim()); | ||
|
|
||
| for (const line of lines) { | ||
| try { | ||
| const json = JSON.parse(line); | ||
| if (json.message && json.message.content) { | ||
| yield json.message.content; | ||
| } | ||
| } catch (error) { | ||
| // Skip invalid JSON lines | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Splitting chunks directly by \n assumes that each chunk contains complete lines. In TCP/HTTP streaming, chunks can be cut off at arbitrary byte boundaries, splitting a single JSON line across chunks. This results in invalid JSON parsing errors and dropped response content. To fix this, incomplete lines must be buffered across chunks.
async *stream(
messages: AIMessage[],
options?: ChatOptions
): AsyncGenerator<string, void, unknown> {
const response = await axios.post(
`${this.endpoint}/api/chat`,
{
model: options?.model || this.defaultModel,
messages: messages.map(msg => ({
role: msg.role,
content: msg.content,
})),
stream: true,
},
{
responseType: 'stream',
}
);
const stream = response.data;
let buffer = '';
for await (const chunk of stream) {
buffer += chunk.toString();
const lines = buffer.split('\n');
buffer = lines.pop() || '';
for (const line of lines) {
if (!line.trim()) {
continue;
}
try {
const json = JSON.parse(line);
if (json.message && json.message.content) {
yield json.message.content;
}
} catch (error) {
// Skip invalid JSON lines
}
}
}
if (buffer.trim()) {
try {
const json = JSON.parse(buffer);
if (json.message && json.message.content) {
yield json.message.content;
}
} catch (error) {
// Skip
}
}
}
Comment on lines
+48
to
+66
| countTokens(text: string, model: string): TokenCountResult { | ||
| // OpenAI models | ||
| if (model.startsWith('gpt')) { | ||
| return this.countOpenAITokens(text, model); | ||
| } | ||
|
|
||
| // Claude models | ||
| if (model.startsWith('claude')) { | ||
| return this.countClaudeTokens(text); | ||
| } | ||
|
|
||
| // Gemini models (similar tokenization to GPT-4) | ||
| if (model.startsWith('gemini')) { | ||
| return this.countGeminiTokens(text); | ||
| } | ||
|
|
||
| // Ollama/LM Studio (use estimation) | ||
| return this.estimateTokens(text, model); | ||
| } |
Contributor
There was a problem hiding this comment.
If model is undefined, null, or not a string, calling model.startsWith will throw a TypeError and crash the application. Adding a defensive guard to handle invalid or missing model names and falling back to estimation improves robustness.
countTokens(text: string, model: string): TokenCountResult {
if (!model || typeof model !== 'string') {
return this.estimateTokens(text, 'unknown');
}
// OpenAI models
if (model.startsWith('gpt')) {
return this.countOpenAITokens(text, model);
}
// Claude models
if (model.startsWith('claude')) {
return this.countClaudeTokens(text);
}
// Gemini models (similar tokenization to GPT-4)
if (model.startsWith('gemini')) {
return this.countGeminiTokens(text);
}
// Ollama/LM Studio (use estimation)
return this.estimateTokens(text, model);
}
Comment on lines
+138
to
+171
| messages: AIMessage[], | ||
| options?: ChatOptions | ||
| ): AsyncGenerator<string, void, unknown> { | ||
| // For streaming, we collect the response and cache it at the end | ||
| const chunks: string[] = []; | ||
|
|
||
| try { | ||
| for await (const chunk of this.wrapped.stream(messages, options)) { | ||
| chunks.push(chunk); | ||
| yield chunk; | ||
| } | ||
|
|
||
| // Cache the complete response after streaming | ||
| if (this.config.enabled && chunks.length > 0) { | ||
| const prompt = this.messagesToPrompt(messages); | ||
| const model = options?.model || 'default'; | ||
| const fullResponse = chunks.join(''); | ||
|
|
||
| // Store in exact match cache (fire and forget) | ||
| this.cache.set(prompt, model, fullResponse).catch(err => { | ||
| console.warn('Failed to cache streamed response:', err); | ||
| }); | ||
|
|
||
| // Store in semantic cache (fire and forget) | ||
| if (this.semanticCache && this.config.useSemanticSimilarity) { | ||
| this.semanticCache.set(prompt, model, fullResponse, this.config.ttlSeconds).catch(err => { | ||
| console.warn('Failed to store streamed response in semantic cache:', err); | ||
| }); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| throw error; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Streaming requests currently bypass the cache lookup entirely, meaning users get zero caching benefits (speed or cost savings) when streaming is enabled. Checking the cache before calling the provider allows cached streaming requests to be served instantly.
async *stream(
messages: AIMessage[],
options?: ChatOptions
): AsyncGenerator<string, void, unknown> {
if (this.config.enabled) {
const prompt = this.messagesToPrompt(messages);
const model = options?.model || 'default';
// Try semantic cache
if (this.semanticCache && this.config.useSemanticSimilarity) {
try {
const semanticMatch = await this.semanticCache.getSimilar(prompt, model, this.config.semanticThreshold);
if (semanticMatch) {
yield semanticMatch.response;
return;
}
} catch (error) {
console.warn('Semantic cache lookup failed:', error);
}
}
// Try exact cache
const cached = await this.cache.get(prompt, model);
if (cached) {
yield cached;
return;
}
}
// For streaming, we collect the response and cache it at the end
const chunks: string[] = [];
try {
for await (const chunk of this.wrapped.stream(messages, options)) {
chunks.push(chunk);
yield chunk;
}
// Cache the complete response after streaming
if (this.config.enabled && chunks.length > 0) {
const prompt = this.messagesToPrompt(messages);
const model = options?.model || 'default';
const fullResponse = chunks.join('');
// Store in exact match cache (fire and forget)
this.cache.set(prompt, model, fullResponse).catch(err => {
console.warn('Failed to cache streamed response:', err);
});
// Store in semantic cache (fire and forget)
if (this.semanticCache && this.config.useSemanticSimilarity) {
this.semanticCache.set(prompt, model, fullResponse, this.config.ttlSeconds).catch(err => {
console.warn('Failed to store streamed response in semantic cache:', err);
});
}
}
} catch (error) {
throw error;
}
}
Comment on lines
+73
to
+98
| async *stream( | ||
| messages: AIMessage[], | ||
| options?: ChatOptions | ||
| ): AsyncGenerator<string, void, unknown> { | ||
| const model = this.client.getGenerativeModel({ | ||
| model: options?.model || this.defaultModel, | ||
| }); | ||
|
|
||
| // Convert messages to Gemini format | ||
| const history = messages.slice(0, -1).map((msg) => ({ | ||
| role: msg.role === "assistant" ? "model" : "user", | ||
| parts: [{ text: msg.content }], | ||
| })); | ||
|
|
||
| const lastMessage = messages[messages.length - 1]; | ||
|
|
||
| const chat = model.startChat({ history }); | ||
| const result = await chat.sendMessageStream(lastMessage.content); | ||
|
|
||
| for await (const chunk of result.stream) { | ||
| const text = chunk.text(); | ||
| if (text) { | ||
| yield text; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
If messages is empty, messages[messages.length - 1] will be undefined, causing a TypeError when accessing lastMessage.content. Adding a defensive guard to handle empty message arrays prevents potential crashes.
async *stream(
messages: AIMessage[],
options?: ChatOptions
): AsyncGenerator<string, void, unknown> {
if (messages.length === 0) {
return;
}
const model = this.client.getGenerativeModel({
model: options?.model || this.defaultModel,
});
// Convert messages to Gemini format
const history = messages.slice(0, -1).map((msg) => ({
role: msg.role === "assistant" ? "model" : "user",
parts: [{ text: msg.content }],
}));
const lastMessage = messages[messages.length - 1];
const chat = model.startChat({ history });
const result = await chat.sendMessageStream(lastMessage.content);
for await (const chunk of result.stream) {
const text = chunk.text();
if (text) {
yield text;
}
}
}
Comment on lines
+269
to
+293
| const metricsDir = this.getMetricsDir(); | ||
|
|
||
| try { | ||
| fs.mkdirSync(metricsDir, { recursive: true }); | ||
| } catch (error) { | ||
| // Directory might already exist | ||
| } | ||
|
|
||
| const metricsPath = path.join(metricsDir, 'spans.json'); | ||
|
|
||
| // Save last 1000 spans to disk | ||
| const spansToSave = this.spans.slice(-1000); | ||
|
|
||
| try { | ||
| fs.writeFileSync( | ||
| metricsPath, | ||
| JSON.stringify(spansToSave, null, 2), | ||
| 'utf-8' | ||
| ); | ||
| } catch (error: any) { | ||
| console.warn('Failed to save metrics to disk:', error.message); | ||
| // Continue without crashing - metrics just won't persist | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
The saveToDisk method is declared as async but uses fs.writeFileSync and fs.mkdirSync which are synchronous and block the event loop. To make it truly non-blocking and match the comment // Persist to disk (async, non-blocking), it should use fs.promises.writeFile and fs.promises.mkdir.
private async saveToDisk(): Promise<void> {
const metricsDir = this.getMetricsDir();
try {
await fs.promises.mkdir(metricsDir, { recursive: true });
} catch (error) {
// Directory might already exist
}
const metricsPath = path.join(metricsDir, 'spans.json');
// Save last 1000 spans to disk
const spansToSave = this.spans.slice(-1000);
try {
await fs.promises.writeFile(
metricsPath,
JSON.stringify(spansToSave, null, 2),
'utf-8'
);
} catch (error: any) {
console.warn('Failed to save metrics to disk:', error.message);
// Continue without crashing - metrics just won't persist
}
}
Comment on lines
+20
to
+31
| static validate(modelName: string, provider?: string): ValidationResult { | ||
| const warnings: string[] = []; | ||
|
|
||
| // Check if model exists | ||
| const modelInfo = modelRegistry.getModelInfo(modelName); | ||
|
|
||
| if (!modelInfo) { | ||
| return { | ||
| valid: false, | ||
| error: `Model "${modelName}" not found in registry. Available models: ${this.getSuggestions(modelName).join(', ')}`, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Inside the static validate method, this.getSuggestions is called. If validate is ever called as a standalone function (e.g., destructured as const { validate } = ModelValidator), this will be undefined, throwing a TypeError. Referencing ModelValidator.getSuggestions directly is safer and more robust.
Suggested change
| static validate(modelName: string, provider?: string): ValidationResult { | |
| const warnings: string[] = []; | |
| // Check if model exists | |
| const modelInfo = modelRegistry.getModelInfo(modelName); | |
| if (!modelInfo) { | |
| return { | |
| valid: false, | |
| error: `Model "${modelName}" not found in registry. Available models: ${this.getSuggestions(modelName).join(', ')}`, | |
| }; | |
| } | |
| static validate(modelName: string, provider?: string): ValidationResult { | |
| const warnings: string[] = []; | |
| // Check if model exists | |
| const modelInfo = modelRegistry.getModelInfo(modelName); | |
| if (!modelInfo) { | |
| return { | |
| valid: false, | |
| error: `Model "${modelName}" not found in registry. Available models: ${ModelValidator.getSuggestions(modelName).join(', ')}`, | |
| }; | |
| } |
Comment on lines
+244
to
+260
| exportToCSV(outputPath: string, days: number = 30): void { | ||
| const cutoffDate = new Date(Date.now() - days * 24 * 60 * 60 * 1000); | ||
| const relevantRecords = this.records.filter( | ||
| (r) => r.timestamp >= cutoffDate | ||
| ); | ||
|
|
||
| const lines: string[] = []; | ||
| lines.push('timestamp,cost,provider,model,operation,tokens'); | ||
|
|
||
| for (const record of relevantRecords) { | ||
| lines.push( | ||
| `${record.timestamp.toISOString()},${record.cost},${record.provider},${record.model},${record.operation},${record.tokens || ''}` | ||
| ); | ||
| } | ||
|
|
||
| fs.writeFileSync(outputPath, lines.join('\n'), 'utf-8'); | ||
| } |
Contributor
There was a problem hiding this comment.
The CSV exporter does not escape fields. If provider, model, or operation contains a comma, double quote, or newline, it will break the CSV structure. Adding a helper to escape CSV values ensures correct formatting.
private escapeCSV(val: string | number | undefined): string {
if (val === undefined) return '';
const str = String(val);
if (str.includes(',') || str.includes('"') || str.includes('\n')) {
return `"${str.replace(/"/g, '""')}"`;
}
return str;
}
/**
* Export usage records to CSV
*/
exportToCSV(outputPath: string, days: number = 30): void {
const cutoffDate = new Date(Date.now() - days * 24 * 60 * 60 * 1000);
const relevantRecords = this.records.filter(
(r) => r.timestamp >= cutoffDate
);
const lines: string[] = [];
lines.push('timestamp,cost,provider,model,operation,tokens');
for (const record of relevantRecords) {
lines.push(
[
record.timestamp.toISOString(),
record.cost,
this.escapeCSV(record.provider),
this.escapeCSV(record.model),
this.escapeCSV(record.operation),
record.tokens || ''
].join(',')
);
}
fs.writeFileSync(outputPath, lines.join('\n'), 'utf-8');
}
Comment on lines
+221
to
+234
| function createProgressBar(percent: number): string { | ||
| const width = 20; | ||
| const filled = Math.floor((percent / 100) * width); | ||
| const empty = width - filled; | ||
|
|
||
| let color = chalk.green; | ||
| if (percent >= 90) { | ||
| color = chalk.red; | ||
| } else if (percent >= 80) { | ||
| color = chalk.yellow; | ||
| } | ||
|
|
||
| return '[' + color('█'.repeat(filled)) + '░'.repeat(empty) + ']'; | ||
| } |
Contributor
There was a problem hiding this comment.
The createProgressBar helper is duplicated across budget.ts, cache.ts, and metrics.ts. Additionally, if percent is NaN or negative, Math.floor can produce NaN or negative values, causing '█'.repeat(filled) to throw a RangeError. We should extract this helper to a shared utility file (like cli/src/utils/progress.ts) and clamp/validate the input.
Suggested change
| function createProgressBar(percent: number): string { | |
| const width = 20; | |
| const filled = Math.floor((percent / 100) * width); | |
| const empty = width - filled; | |
| let color = chalk.green; | |
| if (percent >= 90) { | |
| color = chalk.red; | |
| } else if (percent >= 80) { | |
| color = chalk.yellow; | |
| } | |
| return '[' + color('█'.repeat(filled)) + '░'.repeat(empty) + ']'; | |
| } | |
| function createProgressBar(percent: number): string { | |
| const safePercent = isNaN(percent) || percent < 0 ? 0 : Math.min(100, percent); | |
| const width = 20; | |
| const filled = Math.floor((safePercent / 100) * width); | |
| const empty = width - filled; | |
| let color = chalk.green; | |
| if (safePercent >= 90) { | |
| color = chalk.red; | |
| } else if (safePercent >= 80) { | |
| color = chalk.yellow; | |
| } | |
| return '[' + color('█'.repeat(filled)) + '░'.repeat(empty) + ']'; | |
| } |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
1 similar comment
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
✅ Unit tests committed locally. Commit: |
Contributor
|
✅ Created PR with unit tests: #31 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
cli/__tests__/integration/ai-providers-enhanced.test.ts-24-49 (1)
24-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent call tracking between
chat()andstream().The
chat()method incrementscallCount(line 25), butstream()does not. This inconsistency will causegetCallCount()to return inaccurate results whenstream()is invoked, breaking any test that relies on call counting to verify retry or circuit breaker behavior.🔧 Proposed fix
async *stream(messages: AIMessage[]) { + this.callCount++; + if (this.failureCount < this.maxFailures) { this.failureCount++; const error: any = new Error('Transient failure'); error.status = 500; throw error; } yield 'test'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/__tests__/integration/ai-providers-enhanced.test.ts` around lines 24 - 49, The test double increments this.callCount in chat() but not in stream(), causing getCallCount() to underreport when stream() is used; update the stream() generator (stream) to increment this.callCount at the same point chat() does (before checking failureCount/throwing) so callCount, failureCount and maxFailures behavior remain consistent between chat() and stream().cli/__tests__/providers/decorators/retry-provider.test.ts-112-127 (1)
112-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the non-retryable test assertion to count actual invocations.
At Line 126,
mock.getCallCount()stays0becausemock.chatis replaced and no longer incrementscallCount, so this test does not actually verify that only one attempt occurred.Suggested patch
it('should not retry on non-retryable errors', async () => { const mock = new MockProvider(0); + let calls = 0; mock.chat = async () => { + calls++; const error: any = new Error('Bad request'); error.status = 400; // Non-retryable throw error; }; const retry = new RetryProvider(mock); await expect( retry.chat([{ role: 'user', content: 'test' }]) ).rejects.toThrow('Bad request'); - expect(mock.getCallCount()).toBe(0); // Mock's chat was overridden + expect(calls).toBe(1); // Must not retry non-retryable errors });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/__tests__/providers/decorators/retry-provider.test.ts` around lines 112 - 127, The test overrides MockProvider.chat which bypasses its internal call-counting, so change the test to assert the actual number of attempts by either (a) incrementing mock.getCallCount()/mock.callCount when overriding chat (e.g., increment mock.callCount inside the new mock.chat) or (b) avoid overriding and instead wrap the original MockProvider.chat to set error with status 400 while preserving its call-count behavior; then update the expectation from 0 to 1 to verify a single non-retryable attempt. Ensure you modify the test that constructs MockProvider and RetryProvider and references mock.chat and mock.getCallCount().cli/package.json-40-40 (1)
40-40:⚠️ Potential issue | 🟡 MinorSwitch
homepageto HTTPS for security
cli/package.jsonuseshttp://guardscancli.com, buthttps://guardscancli.comis reachable (HTTP 200). Update the homepage value tohttps://guardscancli.comto avoid insecure HTTP metadata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/package.json` at line 40, The package.json "homepage" field is set to an insecure URL; update the "homepage" property value from "http://guardscancli.com" to "https://guardscancli.com" so metadata uses HTTPS instead of HTTP.cli/src/commands/cache.ts-21-57 (1)
21-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider adding error handling around cache operations.
The
statsandclearsubcommands call cache methods (getStats(),getSizeMB(),getUtilization(),clear()) without try-catch blocks. If these operations throw, the command will crash ungracefully. Consider wrapping cache operations in try-catch blocks to provide user-friendly error messages.Also applies to: 89-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/commands/cache.ts` around lines 21 - 57, Wrap all cache interactions in a try-catch inside the command action handlers (the async function passed to .action) so exceptions from AICache methods (AICache.getStats, AICache.getSizeMB, AICache.getUtilization, AICache.clear) are caught and presented as user-friendly messages; on catch, log a concise error via console.error (or processLogger) with context (e.g., "Failed to read cache stats:" or "Failed to clear cache:") and exit the command gracefully (return or process.exit(1)). Apply the same pattern to the other subcommand handlers around lines where stats are read or clear() is called so all cache operations are protected.cli/src/core/cost-guard.ts-237-239 (1)
237-239:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unnecessary
asyncmarking.The
exportUsagemethod is markedasyncbut performs no asynchronous operations.UsageTracker.exportToCSV()is synchronous (returnsvoid), so theasynckeyword is misleading.📝 Suggested fix
- async exportUsage(outputPath: string, days: number = 30): Promise<void> { + exportUsage(outputPath: string, days: number = 30): void { this.usage.exportToCSV(outputPath, days); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/core/cost-guard.ts` around lines 237 - 239, The exportUsage method is marked async but calls the synchronous UsageTracker.exportToCSV (which returns void), so remove the unnecessary async modifier and any Promise<void> return to make exportUsage a plain synchronous method; locate the exportUsage function (and references to this.usage.exportToCSV) and change its signature from async exportUsage(outputPath: string, days: number = 30): Promise<void> to exportUsage(outputPath: string, days: number = 30): void.cli/src/providers/token-counter.ts-182-199 (1)
182-199:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
countMessagesTokensalways reports'accurate'even when estimation is used.When individual message tokens are estimated (because tokenizers are unavailable), the aggregate result still returns
method: 'accurate'. This misrepresents the counting method to callers.🔧 Proposed fix
countMessagesTokens(messages: Array<{role: string; content: string}>, model: string): TokenCountResult { let totalCount = 0; + let anyEstimated = false; for (const message of messages) { // Count message content const result = this.countTokens(message.content, model); totalCount += result.count; + if (result.method === 'estimated') { + anyEstimated = true; + } // Add overhead for role and formatting (~4 tokens per message) totalCount += 4; } return { count: totalCount, - method: 'accurate', // Mixed method, but report as accurate if most are accurate + method: anyEstimated ? 'estimated' : 'accurate', model, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/token-counter.ts` around lines 182 - 199, The countMessagesTokens function currently always returns method: 'accurate' even when countTokens used an estimated path; update countMessagesTokens to track whether any per-message call to this.countTokens(Model) returned an estimated result (e.g., by having countTokens return an object with a flag or checking result.method), accumulate a boolean like sawEstimated while looping over messages, and set the returned method to 'estimated' if any message was estimated (or 'accurate' only if all were accurate); reference countMessagesTokens and countTokens when making this change so callers receive the correct method.cli/src/providers/model-router.ts-220-226 (1)
220-226:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap switch case variables in block scope.
The linter correctly flags that
costScoreandqualityScoreare declared without block scope and could technically be accessed from other cases. Wrap the case body in braces to restrict variable scope.🔧 Proposed fix
case 'balanced': - // Balance cost and quality - const costScore = -(model.inputPricing + model.outputPricing) * 50; - const qualityScore = model.contextWindow / 2000; - score += costScore + qualityScore; - break; + { + // Balance cost and quality + const costScore = -(model.inputPricing + model.outputPricing) * 50; + const qualityScore = model.contextWindow / 2000; + score += costScore + qualityScore; + break; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/model-router.ts` around lines 220 - 226, The 'balanced' switch case in model-router.ts declares costScore and qualityScore without block scope; wrap the entire case body in braces (case 'balanced': { ... break; }) so costScore and qualityScore are block-scoped, keeping them local to the 'balanced' branch and satisfying the linter for the switch in the code that computes score using model.inputPricing, model.outputPricing and model.contextWindow.Source: Linters/SAST tools
cli/src/providers/decorators/rate-limited-provider.ts-229-231 (1)
229-231:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
awaiton asyncrefill()call.
getStats()callsthis.refill()which isasync, but doesn'tawaitit. This meanscurrentTokensandutilizationPercentmay reflect stale values.🐛 Proposed fix — make getStats async or use synchronous refill
Option 1: Make
getStatsasync:- getStats(): RateLimitStats { - this.refill(); + async getStats(): Promise<RateLimitStats> { + await this.refill();Option 2: Create a synchronous
refillSync()method for use in non-async contexts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/decorators/rate-limited-provider.ts` around lines 229 - 231, getStats currently calls the async method refill() without awaiting it, causing stale token stats; fix by either (A) making getStats async (change signature getStats(): Promise<RateLimitStats>) and await this.refill() inside it, and update all callers to handle the returned Promise, or (B) implement a synchronous refillSync() that performs the same token update logic used by refill() and call this.refillSync() from getStats; locate methods by name getStats, refill and add refillSync if choosing the sync route.
🧹 Nitpick comments (25)
cli/__tests__/providers/token-counter.test.ts (1)
43-56: 💤 Low valueToken overhead assertion may be fragile.
The assertion on line 55 assumes a minimum of 4 tokens overhead per message. While this aligns with typical OpenAI message formatting overhead, the exact value can vary by model and tokenizer implementation. If the overhead calculation changes in
AccurateTokenCounter, this test will fail even if the implementation is correct.Consider either:
- Making the threshold more lenient (e.g.,
> messages.length)- Testing a property instead (e.g., messages with content should have more tokens than
messages.length)- Documenting that this test intentionally validates the specific overhead value
The current assertion is acceptable if the 4-token overhead is a stable contract worth enforcing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/__tests__/providers/token-counter.test.ts` around lines 43 - 56, The test's hardcoded assertion that each message adds at least 4 tokens is fragile; update the test in cli/__tests__/providers/token-counter.test.ts to relax the expectation by comparing relative growth instead of a fixed overhead: use AccurateTokenCounter and its countMessagesTokens to assert that the total token count is greater than the number of messages (e.g., expect(result.count).toBeGreaterThan(messages.length)) or otherwise document the 4-token contract; this keeps the test stable if AccurateTokenCounter's tokenizer/overhead changes.cli/__tests__/features/code-explainer.test.ts (1)
72-74: ⚡ Quick winStream implementation doesn't simulate real streaming behavior.
The
stream()method yields the entire result fromchat()at once, which doesn't test actual streaming scenarios. Real streaming involves multiple yields with partial content chunks.♻️ Enhanced streaming simulation
async *stream(messages: any[]): AsyncGenerator<any, void, unknown> { - yield await this.chat(messages); + const result = await this.chat(messages); + const content = result.content; + // Simulate streaming by yielding content in chunks + const chunkSize = Math.max(10, Math.floor(content.length / 3)); + for (let i = 0; i < content.length; i += chunkSize) { + yield { content: content.slice(i, i + chunkSize) }; + } }This simulates realistic streaming behavior and enables tests to verify chunk handling, error recovery, and progressive content assembly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/__tests__/features/code-explainer.test.ts` around lines 72 - 74, The test stub's stream() currently yields the full chat() result at once, so update the AsyncGenerator implementation in cli/__tests__/features/code-explainer.test.ts to simulate real streaming: call the chat(messages) helper (or ChatMock.chat) to get the full content, split the returned text into multiple chunks (e.g., by sentences or fixed-size slices), then asynchronously yield each chunk in sequence (optionally with small await/delay between yields) so consumers of stream() receive multiple partial chunks rather than a single payload; keep references to the stream() method and chat() call so the change is easy to locate and test.cli/__tests__/core/cost-guard.test.ts (1)
9-12: Review test isolation incli/__tests__/core/cost-guard.test.ts(lines 9-12)
beforeEachcallsCostGuard.clearUsage(), which delegates toUsageTracker.clear().UsageTrackerpersistsrecords.jsontoconfigManager.getCacheDir()/<repoId>/usage/records.json, so clearing forrepoId = 'test-repo'actually isolates subsequentCostGuardinstances in the tests.- The tests still share the real on-disk cache (e.g.,
~/.guardscan/cache), which can leak state to/from the developer/CI machine; route the cache to a temp directory or mockconfigManagerfor tests to keep isolation within the test environment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/__tests__/core/cost-guard.test.ts` around lines 9 - 12, The tests call CostGuard.clearUsage (which uses UsageTracker.clear) but still use the real on-disk cache at configManager.getCacheDir(), leaking state; update the test setup to isolate the cache by stubbing/mocking configManager.getCacheDir to return a fresh temp directory (e.g., created with fs.mkdtempSync or os.tmpdir) before each test and remove it after each test, so UsageTracker.clear/records.json operate only within that temp dir; target the test's beforeEach/afterEach and use jest.spyOn(configManager, 'getCacheDir').mockReturnValue(tempDir) (or an equivalent mock) to ensure CostGuard/UsageTracker use the isolated cache.cli/src/commands/chat.ts (1)
653-653: 💤 Low valueRemove unused variable assignment.
The
highlightedvariable is assigned but never read. The function builds its result using thepartsarray instead.♻️ Remove the unused assignment
- const highlighted = text; let lastIndex = 0; const parts: Array<{ text: string; isCode: boolean; isFilePath?: boolean }> = [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/commands/chat.ts` at line 653, The variable highlighted is assigned but never used; remove the unused assignment and the variable declaration so the function builds its result from the parts array only. Locate the highlighted variable in the function (search for "highlighted") and delete the assignment (and declaration if present), leaving the existing parts-based construction intact and ensuring no remaining references to highlighted remain.cli/src/commands/metrics.ts (1)
140-154: ⚡ Quick winExtract duplicated createProgressBar helper.
The
createProgressBarfunction is duplicated betweenbudget.tsandmetrics.ts. Consider extracting it to a shared utility module to reduce code duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/commands/metrics.ts` around lines 140 - 154, The createProgressBar function is duplicated in metrics.ts and budget.ts; extract it into a shared utility and import it from both places. Create a new helper (e.g., export function createProgressBar(percent: number): string) in a common module (utilities/metricsUtils or similar), move the implementation (including chalk usage and width/colors logic) there, then replace the local createProgressBar definitions in both metrics.ts and budget.ts with imports from the new module and remove the duplicate code.cli/src/commands/budget.ts (1)
139-150: ⚡ Quick winDisplay parsed values in success message for consistency.
The success message displays raw input strings (
options.daily,options.monthly, etc.) rather than the parsed and validated values that were actually saved to the config. This can be misleading if the user enters"5.0"but the saved value is5.♻️ Proposed fix to display parsed values
console.log(chalk.green('\n✅ Budget configuration updated')); if (options.daily) { - console.log(chalk.dim(` Daily limit: $${options.daily}`)); + console.log(chalk.dim(` Daily limit: $${config.budget.dailyLimit.toFixed(2)}`)); } if (options.monthly) { - console.log(chalk.dim(` Monthly limit: $${options.monthly}`)); + console.log(chalk.dim(` Monthly limit: $${config.budget.monthlyLimit.toFixed(2)}`)); } if (options.perRequest) { - console.log(chalk.dim(` Per-request limit: $${options.perRequest}`)); + console.log(chalk.dim(` Per-request limit: $${config.budget.perRequestLimit.toFixed(2)}`)); } if (options.warningThreshold) { - console.log(chalk.dim(` Warning threshold: ${(parseFloat(options.warningThreshold) * 100).toFixed(0)}%`)); + console.log(chalk.dim(` Warning threshold: ${(config.budget.warningThreshold * 100).toFixed(0)}%`)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/commands/budget.ts` around lines 139 - 150, The success output is printing raw input strings (options.daily, options.monthly, options.perRequest, options.warningThreshold) instead of the parsed/validated numeric values that were actually persisted; update the console.log calls to display the parsed values used when saving (use the parsed variables or the values read back from the saved config object—e.g., the parsedDaily/parsedMonthly/parsedPerRequest and parsedWarningThreshold or config.daily/config.monthly/config.perRequest/config.warningThreshold) and format warningThreshold as a percentage the same way it’s currently formatted. Ensure you replace references to options.* in the four console.log lines with those parsed/config values so the success message matches what was stored.cli/src/commands/routing.ts (1)
156-156: ⚡ Quick winAvoid
as anytype assertion.The type assertion
(config.modelRouting.taskOverrides as any)[task]bypasses TypeScript's type checking. Consider using a type-safe approach such as definingtaskOverrideswith an index signature or using a proper type guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/commands/routing.ts` at line 156, Replace the unsafe cast "(config.modelRouting.taskOverrides as any)[task] = override" with a typed-safe assignment: give modelRouting.taskOverrides a proper indexable type (e.g., declare its type as Record<string, OverrideType> or add an index signature) or narrow it with a type guard/cast to that Record before assignment; reference the symbol config.modelRouting.taskOverrides and the variable task to locate the site and then perform the assignment via the typed object (e.g., typedOverrides[task] = override).cli/src/core/ai-cache.ts (1)
239-239: 💤 Low valueStyle inconsistency: prefer multi-line formatting.
The single-line return statement with braces is inconsistent with the multi-line formatting style used throughout the codebase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/core/ai-cache.ts` at line 239, The single-line guard "if (this.accessOrder.length === 0) {return;}" should be expanded to the project's multi-line style: replace it with a multi-line if block (if (this.accessOrder.length === 0) { return; } => if (this.accessOrder.length === 0) { return; } formatted across multiple lines) so the early-return is on its own line with opening and closing braces; update the method in ai-cache.ts where this.accessOrder is checked (the cache eviction/access-order routine) to use the multi-line form to match codebase style.cli/src/commands/review.ts (1)
235-235: 💤 Low valueStyle inconsistency: prefer multi-line formatting.
The single-line return statements with braces are inconsistent with the multi-line formatting style used throughout the rest of the codebase.
Also applies to: 276-278
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/commands/review.ts` at line 235, The single-line conditional "if (comments.length === 0) {continue;}" is inconsistent with the project's multi-line brace style; change it to a multi-line if block with opening and closing braces and the continue on its own line (e.g., if (comments.length === 0) { newline continue; newline }), and apply the same multi-line formatting to the other single-line if/continue occurrences referenced (the similar statements around the other comment-processing block).cli/src/commands/config.ts (1)
267-269: 💤 Low valueStyle inconsistency: prefer multi-line formatting.
The single-line return statements with braces (
if (...) {return ...;}) are inconsistent with the multi-line formatting style used throughout the rest of this file and the codebase.♻️ Suggested formatting for consistency
- if (provider === "none") {return "static";} - if (provider === "ollama" || provider === "lmstudio") {return "local";} + if (provider === "none") { + return "static"; + } + if (provider === "ollama" || provider === "lmstudio") { + return "local"; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/commands/config.ts` around lines 267 - 269, The three single-line conditional returns using the provider variable should be reformatted to multi-line if blocks for consistency with the codebase: replace `if (provider === "none") {return "static";}` and `if (provider === "ollama" || provider === "lmstudio") {return "local";}` with standard multi-line form (if (...) { return "..."; }) and keep the final `return "cloud";` as-is; locate this logic where provider is evaluated (the conditional branch that returns "static", "local", or "cloud") and update the formatting only—no logic changes.cli/src/core/metrics-collector.ts (1)
268-291: ⚡ Quick winShared pattern: misleading
asyncmarking on sync filesystem operations. BothMetricsCollector.saveToDisk()andUsageTracker.saveToDisk()are markedasync Promise<void>but use only synchronous filesystem operations (mkdirSync,writeFileSync). Theasynckeyword suggests non-blocking I/O, but these methods block the event loop. Consider removingasyncfrom both to clarify that they perform synchronous work, or switch to the async fs/promises API if non-blocking behavior is desired.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/core/metrics-collector.ts` around lines 268 - 291, The saveToDisk functions (MetricsCollector.saveToDisk and UsageTracker.saveToDisk) are marked async but only perform synchronous fs calls (mkdirSync, writeFileSync), which is misleading; either remove the async keyword and the Promise<void return to reflect synchronous behavior, or convert the implementation to use non-blocking APIs (fs.promises.mkdir, fs.promises.writeFile or their async variants) and await them inside the async function. Update error handling accordingly (catch awaited promise rejections instead of try/catch around sync calls) and ensure the function signatures and callers reflect the chosen sync or async approach.cli/src/core/config.ts (2)
385-405: ⚖️ Poor tradeoffConsider adding validation for config value ranges.
The merge function accepts user-provided values without range validation. Users could supply invalid values like negative retry counts or budget limits. Consider adding validation logic to ensure values are within reasonable bounds before merging.
Example validation approach:
export function mergeConfigWithEnhancedDefaults(config: Config): Config { const merged = { ...config, retry: { ...DEFAULT_ENHANCED_CONFIG.retry, ...config.retry }, // ... other merges }; // Validate ranges if (merged.retry && merged.retry.maxRetries < 0) { throw new Error('retry.maxRetries must be non-negative'); } if (merged.budget && merged.budget.dailyLimit < 0) { throw new Error('budget.dailyLimit must be non-negative'); } return merged as Config; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/core/config.ts` around lines 385 - 405, Add range validation after constructing the merged config in mergeConfigWithEnhancedDefaults: build merged = { ...config, retry: { ...DEFAULT_ENHANCED_CONFIG.retry, ...config.retry }, ... } then validate numeric fields (e.g., retry.maxRetries >= 0, retry.backoffMs >= 0, budget.dailyLimit >= 0, circuitBreaker.threshold between 0 and 1, rateLimit.requestsPerSecond >= 0, cache.ttl >= 0, observability.samplingRate between 0 and 1, etc.); on invalid values either throw a descriptive Error identifying the field (e.g., "retry.maxRetries must be non-negative") or clamp to safe bounds before returning merged as Config. Ensure validations reference mergeConfigWithEnhancedDefaults, Config, and DEFAULT_ENHANCED_CONFIG symbols so reviewers can locate changes.
342-382: 💤 Low valueConsider documenting default value semantics.
The enhanced config defaults would benefit from JSDoc comments explaining the rationale behind specific values (e.g., why
semanticThresholdis 0.95, what the budget limits represent in terms of currency, etc.). This helps users understand what they're opting into.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/core/config.ts` around lines 342 - 382, Add JSDoc above DEFAULT_ENHANCED_CONFIG documenting default value semantics: explain why cache.semanticThreshold is 0.95 and how it affects semantic cache hits, clarify units/meaning for budget.dailyLimit, monthlyLimit and perRequestLimit (e.g., number of free requests or currency units), describe ttlSeconds and maxSizeMB units for cache, explain circuitBreaker.failureThreshold/resetTimeoutMs/halfOpenSuccessThreshold behavior, note retry.maxRetries/baseDelayMs/maxDelayMs semantics, and mark rateLimit.enabled and modelRouting.enabled as opt-in with what strategy 'balanced' implies; reference DEFAULT_ENHANCED_CONFIG and the specific keys (semanticThreshold, ttlSeconds, maxSizeMB, dailyLimit, monthlyLimit, perRequestLimit, failureThreshold, resetTimeoutMs, halfOpenSuccessThreshold, maxRetries, baseDelayMs, maxDelayMs, rateLimit.enabled, modelRouting.strategy) so future readers understand rationale and units.cli/src/providers/decorators/circuit-breaker-provider.ts (1)
261-276: ⚡ Quick winConsider removing or supplementing
JSON.stringify(error)in error detection.Line 262 uses
JSON.stringify(error).toLowerCase()to serialize the error for pattern matching. However,Errorobjects often have non-enumerable properties (likemessage,stack,cause), soJSON.stringify()may produce"{}"or miss critical error details.Your code already checks
error.message,error.code, anderror.statusdirectly (lines 263-265), which are the reliable paths. TheJSON.stringifycheck on line 262 is likely redundant and could give a false sense of coverage.🔧 Suggested simplification
Remove the
JSON.stringify(error)check and rely on the direct property checks:private isMonitoredError(error: any): boolean { - const errorStr = JSON.stringify(error).toLowerCase(); const messageStr = (error.message || '').toLowerCase(); const codeStr = (error.code || '').toLowerCase(); const statusStr = String(error.status || error.statusCode || ''); return this.config.monitoredErrors.some((monitoredError) => { const monitored = monitoredError.toLowerCase(); return ( - errorStr.includes(monitored) || messageStr.includes(monitored) || codeStr.includes(monitored) || statusStr.includes(monitored) ); }); }If you want to keep a broad "catch any property" check, consider serializing specific enumerable properties or using a custom error stringifier instead of
JSON.stringify.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/decorators/circuit-breaker-provider.ts` around lines 261 - 276, In isMonitoredError, remove the unreliable JSON.stringify(error) check (errorStr) because Error objects often have non-enumerable properties; instead rely on the existing direct checks (error.message, error.code, error.status/statusCode) against this.config.monitoredErrors, or if you need a broader fallback implement a custom stringifier that extracts enumerable props plus message/stack/cause and then lowercase it before matching; update the return logic in isMonitoredError to use only those reliable strings (messageStr, codeStr, statusStr, and optional customErrorStr) when testing monitoredErrors.cli/src/providers/token-counter.ts (3)
18-18: ⚡ Quick winStrengthen encoder Map typing.
Map<string, any>loses type safety. Consider defining an interface or using a more specific type for the cached encoders.💡 Suggested improvement
- private openaiEncoders: Map<string, any> = new Map(); + private openaiEncoders: Map<string, { encode: (text: string) => number[]; free?: () => void }> = new Map();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/token-counter.ts` at line 18, The openaiEncoders cache is typed as Map<string, any>, losing type safety; define a concrete Encoder interface (e.g., methods/properties returned/used from the encoder such as encode(text: string): number[] and optional decode or modelName) and replace the field declaration openaiEncoders: Map<string, any> with Map<string, Encoder>, then update any usages in token-counter.ts (functions that call encoder.encode/decoder) to use the typed interface so TypeScript can validate calls and return types.
248-256: ⚡ Quick winGuard against missing
free()method on encoders.Line 251 calls
encoder.free(), buttiktokenencoder types may not always include afreemethod. The current check usestypeof encoder.free === 'function', which is safe, but the iteration on Line 250 should guard againstnullorundefinedencoders.🛡️ Proposed defensive check
cleanup(): void { // Free encoder resources for (const [, encoder] of this.openaiEncoders) { - if (encoder && typeof encoder.free === 'function') { + if (encoder != null && typeof encoder.free === 'function') { encoder.free(); } } this.openaiEncoders.clear(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/token-counter.ts` around lines 248 - 256, The cleanup() loop assumes every entry in openaiEncoders yields a non-null encoder before checking encoder.free; update the iteration over this.openaiEncoders in cleanup() to first guard against null/undefined encoders (e.g., skip when encoder == null) and then check typeof encoder.free === 'function' before calling encoder.free(), ensuring you reference the existing cleanup(), this.openaiEncoders and encoder.free symbols to locate the change.
190-191: 💤 Low valueHardcoded 4-token message overhead may not be accurate for all models.
The comment states "~4 tokens per message", but this is an approximation and may vary by provider. Consider documenting the source or making it configurable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/token-counter.ts` around lines 190 - 191, The hardcoded per-message overhead addition ("totalCount += 4") in token counting (token-counter.ts, where totalCount is computed) assumes a ~4-token cost for role/formatting which may be inaccurate across models; replace the magic constant with a configurable value or a model-specific lookup (e.g., messageOverheadForModel(modelName) used by the function that computes totalCount) and add a comment pointing to the source/heuristic used, or load the overhead from provider metadata so different providers/models can override it. Ensure callers pass the model/provider identifier into the token counting function so the code can select the correct overhead instead of always adding 4.cli/src/providers/embedding-lmstudio.ts (1)
66-66: 💤 Low valueConsider standard formatting for early return.
Same style issue as in
embedding-gemini.ts: the inline brace notation is non-idiomatic.♻️ Proposed style fix
- if (texts.length === 0) {return [];} + if (texts.length === 0) return [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/embedding-lmstudio.ts` at line 66, Replace the non-idiomatic inline early-return "if (texts.length === 0) {return [];}" in embedding-lmstudio.ts with a properly formatted early return (e.g., either "if (texts.length === 0) return [];" or "if (texts.length === 0) { return []; }") so the check on texts and the return are spaced consistently with the style used in embedding-gemini.ts; update the line that references texts to follow this formatting.cli/src/providers/embedding-gemini.ts (1)
46-46: 💤 Low valueConsider standard formatting for early return.
The inline brace style
{return [];}is non-idiomatic. Standard TypeScript convention for simple early returns is either:
if (texts.length === 0) return [];(no braces), or- Multi-line with proper spacing.
♻️ Proposed style fix
- if (texts.length === 0) {return [];} + if (texts.length === 0) return [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/embedding-gemini.ts` at line 46, The inline early-return "if (texts.length === 0) {return [];}" in embedding-gemini.ts should be reformatted to standard TypeScript style: replace the brace-wrapped single-line return with an idiomatic single-line early return without braces or, alternatively, expand it to a properly spaced multi-line if-block; locate the exact conditional using the token "if (texts.length === 0) {return [];}" and update it accordingly.cli/src/providers/ollama.ts (1)
37-75: 💤 Low valueConsider adding a comment to explain the empty catch block.
The empty catch at lines 70-72 silently skips invalid JSON lines during streaming, which is appropriate for handling partial or malformed chunks. A brief comment would clarify the intent.
♻️ Suggested comment
try { const json = JSON.parse(line); if (json.message && json.message.content) { yield json.message.content; } } catch (error) { - // Skip invalid JSON lines + // Skip invalid JSON lines (partial chunks or malformed data) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/ollama.ts` around lines 37 - 75, In the stream method (async *stream) of the Ollama provider, add a short explanatory comment inside the empty catch block that clarifies why invalid JSON lines are being silently ignored (e.g., to handle partial/malformed chunks from the streaming response without breaking the generator); update the comment near the catch handling inside the inner loop that parses chunked lines so future readers know this is intentional and not a swallowed error.cli/src/providers/gemini.ts (1)
205-211: ⚡ Quick winClarify that tiktoken is an approximation for Gemini tokenization.
The comment states "using tiktoken (similar to GPT models)" but tiktoken is OpenAI's tokenizer, not Google's. For Gemini models, this is an estimation/approximation rather than the actual tokenizer. The comment should clarify this to avoid misleading developers.
♻️ Clearer comment
/** - * Count tokens accurately using tiktoken (similar to GPT models) + * Count tokens using tiktoken-based estimation + * Note: This is an approximation; Gemini uses its own tokenizer */ countTokens(text: string): number {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/gemini.ts` around lines 205 - 211, The comment above countTokens() is misleading about tokenization: update the JSDoc for the countTokens(text: string) method to clarify that tokenCounter.countTokens(text, this.defaultModel) uses OpenAI's tiktoken as an approximation for Gemini tokenization (not Gemini's native tokenizer), and that the returned value (result.count) is an estimate; reference the countTokens method, the tokenCounter variable, and this.defaultModel in the comment so future readers know it's an estimated count for Gemini-like usage.cli/src/providers/decorators/cached-provider.ts (2)
168-170: ⚡ Quick winRemove redundant try-catch block.
The catch block only re-throws the error, making the try-catch redundant. The generator will propagate exceptions naturally.
♻️ Simplified code
// For streaming, we collect the response and cache it at the end const chunks: string[] = []; - try { - for await (const chunk of this.wrapped.stream(messages, options)) { - chunks.push(chunk); - yield chunk; - } + for await (const chunk of this.wrapped.stream(messages, options)) { + chunks.push(chunk); + yield chunk; + } - // Cache the complete response after streaming - if (this.config.enabled && chunks.length > 0) { - const prompt = this.messagesToPrompt(messages); - const model = options?.model || 'default'; - const fullResponse = chunks.join(''); - - // Store in exact match cache (fire and forget) - this.cache.set(prompt, model, fullResponse).catch(err => { - console.warn('Failed to cache streamed response:', err); - }); + // Cache the complete response after streaming + if (this.config.enabled && chunks.length > 0) { + const prompt = this.messagesToPrompt(messages); + const model = options?.model || 'default'; + const fullResponse = chunks.join(''); + + // Store in exact match cache (fire and forget) + this.cache.set(prompt, model, fullResponse).catch(err => { + console.warn('Failed to cache streamed response:', err); + }); - // Store in semantic cache (fire and forget) - if (this.semanticCache && this.config.useSemanticSimilarity) { - this.semanticCache.set(prompt, model, fullResponse, this.config.ttlSeconds).catch(err => { - console.warn('Failed to store streamed response in semantic cache:', err); - }); - } + // Store in semantic cache (fire and forget) + if (this.semanticCache && this.config.useSemanticSimilarity) { + this.semanticCache.set(prompt, model, fullResponse, this.config.ttlSeconds).catch(err => { + console.warn('Failed to store streamed response in semantic cache:', err); + }); } - } catch (error) { - throw error; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/decorators/cached-provider.ts` around lines 168 - 170, Remove the redundant try-catch that simply re-throws errors in cli/src/providers/decorators/cached-provider.ts: locate the block containing "try { ... } catch (error) { throw error; }" (the catch in the cached-provider decorator/function) and delete the catch (and its empty rethrow) so the function lets exceptions propagate naturally; ensure surrounding logic (e.g., any async function or the cached-provider decorator wrapper) remains intact and still returns the same values.
95-95: ⚡ Quick winConsider using a debug logger instead of
console.warnfor consistency.Other providers in this PR (e.g.,
gemini.ts) now usecreateDebugLoggerfor structured logging. Usingconsole.warnhere is inconsistent and makes log management harder in production.♻️ Suggested refactor
At the top of the file:
+import { createDebugLogger } from '../../utils/debug-logger'; + +const logger = createDebugLogger('cached-provider');Then replace console.warn calls:
- console.warn('Semantic cache lookup failed:', error); + logger.warn('Semantic cache lookup failed:', error);- console.warn('Failed to store in semantic cache:', error); + logger.warn('Failed to store in semantic cache:', error);- console.warn('Failed to cache streamed response:', err); + logger.warn('Failed to cache streamed response:', err);- console.warn('Failed to store streamed response in semantic cache:', err); + logger.warn('Failed to store streamed response in semantic cache:', err);Also applies to: 126-126, 158-158, 164-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/decorators/cached-provider.ts` at line 95, Import and instantiate a structured debug logger at the top of cached-provider.ts (e.g., const debug = createDebugLogger('cached-provider')), then replace all console.warn(...) calls in this file (the "Semantic cache lookup failed:" and the other warn sites) with debug.warn(...) so logging is consistent with other providers; ensure createDebugLogger is imported and used instead of console.warn in the functions/methods where those warnings occur.cli/src/providers/decorators/rate-limited-provider.ts (1)
61-61: ⚡ Quick winUnused field:
refillLockis declared but never used.The
refillLockfield is initialized but never referenced. If concurrent access protection is intended, it should be implemented; otherwise, remove the field.♻️ Proposed fix — remove unused field
private tokens: number; private lastRefill: number; - private refillLock: boolean = false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/decorators/rate-limited-provider.ts` at line 61, The private field refillLock declared as "refillLock" in the rate-limited-provider decorator is unused; either remove it or implement its intended concurrency protection. To fix, delete the unused field declaration "private refillLock: boolean = false;" from the RateLimitedProvider class (or the surrounding decorator class) if no locking is required, or wire it into the existing token-refill logic (e.g., inside methods like refillTokens, acquireToken, or similar) to guard concurrent refills by checking and setting refillLock while performing the refill operation. Ensure any added logic uses the same symbol name "refillLock" so references are consistent and remove any unused imports after the change.cli/src/providers/decorators/observable-provider.ts (1)
11-11: ⚡ Quick winUnused import:
cryptois never used.The
cryptomodule is imported but the code usesMetricsCollector.generateTraceId()andMetricsCollector.generateSpanId()instead. Remove the unused import.♻️ Proposed fix
-import * as crypto from 'crypto';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/providers/decorators/observable-provider.ts` at line 11, The file imports the Node 'crypto' module but never uses it because trace/span IDs are produced via MetricsCollector.generateTraceId() and MetricsCollector.generateSpanId(); remove the unused "import * as crypto from 'crypto';" statement from observable-provider.ts to eliminate the unused import and keep the file clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fce94bbe-2bbf-414d-a5a3-45c754314d45
⛔ Files ignored due to path filters (1)
cli/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (110)
cli/__tests__/core/cost-guard.test.tscli/__tests__/features/code-explainer.test.tscli/__tests__/features/refactoring-suggestions.test.tscli/__tests__/integration/ai-providers-enhanced.test.tscli/__tests__/providers/decorators/cached-provider.test.tscli/__tests__/providers/decorators/circuit-breaker-provider.test.tscli/__tests__/providers/decorators/observable-provider.test.tscli/__tests__/providers/decorators/rate-limited-provider.test.tscli/__tests__/providers/decorators/retry-provider.test.tscli/__tests__/providers/model-registry.test.tscli/__tests__/providers/model-router.test.tscli/__tests__/providers/token-counter.test.tscli/package.jsoncli/src/commands/budget.tscli/src/commands/cache.tscli/src/commands/chat.tscli/src/commands/commit.tscli/src/commands/config.tscli/src/commands/docs.tscli/src/commands/explain.tscli/src/commands/init.tscli/src/commands/metrics.tscli/src/commands/migrate.tscli/src/commands/models.tscli/src/commands/perf.tscli/src/commands/refactor.tscli/src/commands/review.tscli/src/commands/routing.tscli/src/commands/rules.tscli/src/commands/run.tscli/src/commands/scan.tscli/src/commands/security.tscli/src/commands/test-gen.tscli/src/commands/test.tscli/src/commands/threat-model.tscli/src/core/ai-cache.tscli/src/core/api-scanner.tscli/src/core/ast-parser.tscli/src/core/chatbot-engine.tscli/src/core/code-metrics.tscli/src/core/code-smells.tscli/src/core/codebase-indexer.tscli/src/core/compliance-checker.tscli/src/core/config.tscli/src/core/context-builder.tscli/src/core/cost-guard.tscli/src/core/dependency-scanner.tscli/src/core/dockerfile-scanner.tscli/src/core/embedding-chunker.tscli/src/core/embedding-search.tscli/src/core/embedding-store.tscli/src/core/embeddings.tscli/src/core/iac-scanner.tscli/src/core/license-scanner.tscli/src/core/linter-integration.tscli/src/core/metrics-collector.tscli/src/core/mutation-tester.tscli/src/core/owasp-scanner.tscli/src/core/performance-tester.tscli/src/core/rag-context.tscli/src/core/repository.tscli/src/core/rule-engine.tscli/src/core/secrets-detector.tscli/src/core/telemetry.tscli/src/core/test-runner.tscli/src/core/usage-tracker.tscli/src/features/code-explainer.tscli/src/features/code-review.tscli/src/features/commit-generator.tscli/src/features/docs-generator.tscli/src/features/fix-suggestions.tscli/src/features/migration-assistant.tscli/src/features/refactoring-suggestions.tscli/src/features/test-generator.tscli/src/features/threat-modeling.tscli/src/index.tscli/src/parsers/csharp-parser.tscli/src/parsers/go-parser.tscli/src/parsers/java-parser.tscli/src/parsers/php-parser.tscli/src/parsers/python-parser.tscli/src/parsers/ruby-parser.tscli/src/parsers/rust-parser.tscli/src/providers/base.tscli/src/providers/claude.tscli/src/providers/decorators/base-decorator.tscli/src/providers/decorators/cached-provider.tscli/src/providers/decorators/circuit-breaker-provider.tscli/src/providers/decorators/observable-provider.tscli/src/providers/decorators/rate-limited-provider.tscli/src/providers/decorators/retry-provider.tscli/src/providers/embedding-gemini.tscli/src/providers/embedding-lmstudio.tscli/src/providers/embedding-ollama.tscli/src/providers/embedding-openai.tscli/src/providers/factory.tscli/src/providers/gemini.tscli/src/providers/model-registry.tscli/src/providers/model-router.tscli/src/providers/model-validator.tscli/src/providers/ollama.tscli/src/providers/openai.tscli/src/providers/token-counter.tscli/src/utils/debug-logger.tscli/src/utils/performance-tracker.tscli/src/utils/progress.tscli/src/utils/reporter.tscli/src/utils/version.tsdocs/AI_QUICK_REFERENCE.mddocs/adrs/002-supabase-postgresql.md
💤 Files with no reviewable changes (1)
- docs/adrs/002-supabase-postgresql.md
Comment on lines
+129
to
+163
| describe('retry with circuit breaker', () => { | ||
| it('should retry failures and track in circuit breaker', async () => { | ||
| // This test verifies retry and circuit breaker work together | ||
| // In practice, this would use mocked providers | ||
| expect(true).toBe(true); // Placeholder | ||
| }); | ||
| }); | ||
|
|
||
| describe('caching with observability', () => { | ||
| it('should track cache hits in metrics', async () => { | ||
| // This test verifies cache hits are properly tracked by observability | ||
| expect(true).toBe(true); // Placeholder | ||
| }); | ||
| }); | ||
|
|
||
| describe('rate limiting with retry', () => { | ||
| it('should rate limit and retry on failures', async () => { | ||
| // This test verifies rate limiting and retry work together | ||
| expect(true).toBe(true); // Placeholder | ||
| }); | ||
| }); | ||
|
|
||
| describe('full stack end-to-end', () => { | ||
| it('should handle complex scenario with all features', async () => { | ||
| // This test runs a full scenario: | ||
| // 1. Request made | ||
| // 2. Rate limited (waits) | ||
| // 3. Fails first time | ||
| // 4. Retries successfully | ||
| // 5. Cached for next request | ||
| // 6. All tracked by observability | ||
| // 7. Circuit breaker stays closed | ||
| expect(true).toBe(true); // Placeholder | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove or implement placeholder tests.
Multiple test blocks contain only expect(true).toBe(true) assertions that provide no value. Placeholder tests create a false sense of coverage and should either be fully implemented or removed before merge.
Affected tests:
- "should retry failures and track in circuit breaker" (lines 130-134)
- "should track cache hits in metrics" (lines 138-141)
- "should rate limit and retry on failures" (lines 145-148)
- "should handle complex scenario with all features" (lines 152-161)
Consider removing these tests now and tracking actual implementation in a follow-up issue, or implement them in this PR if the underlying decorator behavior is ready to test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/__tests__/integration/ai-providers-enhanced.test.ts` around lines 129 -
163, Remove or implement the four placeholder tests in
cli/__tests__/integration/ai-providers-enhanced.test.ts: delete the it blocks
titled "should retry failures and track in circuit breaker", "should track cache
hits in metrics", "should rate limit and retry on failures", and "should handle
complex scenario with all features" (or replace each placeholder
expect(true).toBe(true) with real assertions). If you choose removal, leave a
TODO comment referencing a follow-up issue to add integration tests for
retry+circuit breaker, caching+observability, rate limiting+retry, and
full-stack scenarios; if you implement them, replace the placeholders with
proper mocked providers and assertions exercising the decorator behavior
(targeting the retry/circuit breaker, cache metrics, rate limiter, and
end-to-end flow respectively).
Comment on lines
+21
to
+67
| .action(async () => { | ||
| const config = configManager.loadOrInit(); | ||
| const repo = new Repository(process.cwd()); | ||
| const repoId = repo.getId(); | ||
|
|
||
| const costGuard = new CostGuard( | ||
| repoId, | ||
| config.budget | ||
| ); | ||
|
|
||
| const status = await costGuard.getBudgetStatus(); | ||
|
|
||
| console.log(chalk.bold('\n💰 Budget Status\n')); | ||
| console.log('─'.repeat(60)); | ||
|
|
||
| // Daily budget | ||
| console.log(chalk.bold('\n📅 Daily Budget:')); | ||
| console.log(` Used: $${status.daily.used.toFixed(2)}`); | ||
| console.log(` Limit: $${status.daily.limit.toFixed(2)}`); | ||
| console.log(` Remaining: $${status.daily.remaining.toFixed(2)}`); | ||
|
|
||
| const dailyBar = createProgressBar(status.daily.percentUsed); | ||
| console.log(` Progress: ${dailyBar} ${status.daily.percentUsed.toFixed(0)}%`); | ||
|
|
||
| // Monthly budget | ||
| console.log(chalk.bold('\n📆 Monthly Budget:')); | ||
| console.log(` Used: $${status.monthly.used.toFixed(2)}`); | ||
| console.log(` Limit: $${status.monthly.limit.toFixed(2)}`); | ||
| console.log(` Remaining: $${status.monthly.remaining.toFixed(2)}`); | ||
|
|
||
| const monthlyBar = createProgressBar(status.monthly.percentUsed); | ||
| console.log(` Progress: ${monthlyBar} ${status.monthly.percentUsed.toFixed(0)}%`); | ||
|
|
||
| // Per-request limit | ||
| console.log(chalk.bold('\n💳 Per-Request Limit:')); | ||
| console.log(` Limit: $${status.perRequest.limit.toFixed(2)}`); | ||
|
|
||
| // Warnings | ||
| if (status.warnings.length > 0) { | ||
| console.log(chalk.yellow.bold('\n⚠️ Warnings:')); | ||
| status.warnings.forEach((warning) => { | ||
| console.log(chalk.yellow(` • ${warning}`)); | ||
| }); | ||
| } | ||
|
|
||
| console.log('\n'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add error handling for status retrieval.
The status command does not wrap costGuard.getBudgetStatus() in a try/catch block. If the CostGuard instance or underlying storage fails, the command will crash with an unhandled error instead of displaying a user-friendly message.
🛡️ Proposed fix to add error handling
.action(async () => {
- const config = configManager.loadOrInit();
- const repo = new Repository(process.cwd());
- const repoId = repo.getId();
-
- const costGuard = new CostGuard(
- repoId,
- config.budget
- );
-
- const status = await costGuard.getBudgetStatus();
+ try {
+ const config = configManager.loadOrInit();
+ const repo = new Repository(process.cwd());
+ const repoId = repo.getId();
+
+ const costGuard = new CostGuard(
+ repoId,
+ config.budget
+ );
+
+ const status = await costGuard.getBudgetStatus();
- console.log(chalk.bold('\n💰 Budget Status\n'));
+ console.log(chalk.bold('\n💰 Budget Status\n'));
// ... rest of display logic
+ } catch (error: any) {
+ console.error(chalk.red(`❌ Failed to retrieve budget status: ${error.message}`));
+ process.exit(1);
+ }
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/commands/budget.ts` around lines 21 - 67, The command's action calls
CostGuard.getBudgetStatus() without error handling, so failures will crash the
CLI; wrap the await costGuard.getBudgetStatus() call inside a try/catch in the
.action(async () => { ... }) handler, catch any error, log a user-friendly
message via console.error (or chalk.red) including the error.message for
debugging, and return/exit the command gracefully instead of proceeding to
render the status; ensure references to CostGuard, getBudgetStatus, and the
action callback are used to locate the change.
Comment on lines
+160
to
+215
| .action(async (options) => { | ||
| const config = configManager.loadOrInit(); | ||
| const repo = new Repository(process.cwd()); | ||
| const repoId = repo.getId(); | ||
|
|
||
| const costGuard = new CostGuard(repoId, config.budget); | ||
| const days = parseInt(options.days); | ||
|
|
||
| const report = await costGuard.getUsageReport(days); | ||
|
|
||
| console.log(chalk.bold(`\n📊 Usage Report (Last ${days} days)\n`)); | ||
| console.log('─'.repeat(60)); | ||
|
|
||
| // Summary | ||
| console.log(chalk.bold('Summary:')); | ||
| console.log(` Daily: $${report.summary.daily.toFixed(2)}`); | ||
| console.log(` Weekly: $${report.summary.weekly.toFixed(2)}`); | ||
| console.log(` Monthly: $${report.summary.monthly.toFixed(2)}`); | ||
| console.log(` All Time: $${report.summary.allTime.toFixed(2)}`); | ||
|
|
||
| // By provider | ||
| if (Object.keys(report.byProvider).length > 0) { | ||
| console.log(chalk.bold('\nBy Provider:')); | ||
| for (const [provider, cost] of Object.entries(report.byProvider)) { | ||
| console.log(` ${provider.padEnd(15)} $${cost.toFixed(2)}`); | ||
| } | ||
| } | ||
|
|
||
| // By model | ||
| if (Object.keys(report.byModel).length > 0) { | ||
| console.log(chalk.bold('\nBy Model:')); | ||
| const sorted = Object.entries(report.byModel).sort((a, b) => b[1] - a[1]); | ||
| sorted.slice(0, 5).forEach(([model, cost]) => { | ||
| console.log(` ${model.padEnd(25)} $${cost.toFixed(2)}`); | ||
| }); | ||
| } | ||
|
|
||
| // Top costly operations | ||
| if (report.topCostlyOperations.length > 0) { | ||
| console.log(chalk.bold('\nTop 5 Costly Operations:')); | ||
| report.topCostlyOperations.slice(0, 5).forEach((op, i) => { | ||
| console.log( | ||
| ` ${i + 1}. ${op.timestamp.toLocaleString()} - ` + | ||
| `${op.model} - $${op.cost.toFixed(4)}` | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| // Export option | ||
| if (options.export) { | ||
| await costGuard.exportUsage(options.export, days); | ||
| console.log(chalk.green(`\n✅ Report exported to: ${options.export}`)); | ||
| } | ||
|
|
||
| console.log('\n'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add error handling and input validation for report command.
The report command has two issues:
parseInt(options.days)can returnNaNif the input is invalid, leading to incorrect behavior ingetUsageReport(days).- No try/catch around
costGuard.getUsageReport()orcostGuard.exportUsage(), so failures will crash the command.
🛡️ Proposed fix to add validation and error handling
.action(async (options) => {
- const config = configManager.loadOrInit();
- const repo = new Repository(process.cwd());
- const repoId = repo.getId();
-
- const costGuard = new CostGuard(repoId, config.budget);
- const days = parseInt(options.days);
-
- const report = await costGuard.getUsageReport(days);
+ try {
+ const config = configManager.loadOrInit();
+ const repo = new Repository(process.cwd());
+ const repoId = repo.getId();
+
+ const costGuard = new CostGuard(repoId, config.budget);
+ const days = parseInt(options.days);
+
+ if (isNaN(days) || days <= 0) {
+ console.error(chalk.red('❌ Invalid days value (must be positive integer)'));
+ process.exit(1);
+ }
+
+ const report = await costGuard.getUsageReport(days);
- console.log(chalk.bold(`\n📊 Usage Report (Last ${days} days)\n`));
+ console.log(chalk.bold(`\n📊 Usage Report (Last ${days} days)\n`));
// ... rest of display logic
- // Export option
- if (options.export) {
- await costGuard.exportUsage(options.export, days);
- console.log(chalk.green(`\n✅ Report exported to: ${options.export}`));
- }
+ // Export option
+ if (options.export) {
+ await costGuard.exportUsage(options.export, days);
+ console.log(chalk.green(`\n✅ Report exported to: ${options.export}`));
+ }
- console.log('\n');
+ console.log('\n');
+ } catch (error: any) {
+ console.error(chalk.red(`❌ Failed to generate report: ${error.message}`));
+ process.exit(1);
+ }
});📝 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.
Suggested change
| .action(async (options) => { | |
| const config = configManager.loadOrInit(); | |
| const repo = new Repository(process.cwd()); | |
| const repoId = repo.getId(); | |
| const costGuard = new CostGuard(repoId, config.budget); | |
| const days = parseInt(options.days); | |
| const report = await costGuard.getUsageReport(days); | |
| console.log(chalk.bold(`\n📊 Usage Report (Last ${days} days)\n`)); | |
| console.log('─'.repeat(60)); | |
| // Summary | |
| console.log(chalk.bold('Summary:')); | |
| console.log(` Daily: $${report.summary.daily.toFixed(2)}`); | |
| console.log(` Weekly: $${report.summary.weekly.toFixed(2)}`); | |
| console.log(` Monthly: $${report.summary.monthly.toFixed(2)}`); | |
| console.log(` All Time: $${report.summary.allTime.toFixed(2)}`); | |
| // By provider | |
| if (Object.keys(report.byProvider).length > 0) { | |
| console.log(chalk.bold('\nBy Provider:')); | |
| for (const [provider, cost] of Object.entries(report.byProvider)) { | |
| console.log(` ${provider.padEnd(15)} $${cost.toFixed(2)}`); | |
| } | |
| } | |
| // By model | |
| if (Object.keys(report.byModel).length > 0) { | |
| console.log(chalk.bold('\nBy Model:')); | |
| const sorted = Object.entries(report.byModel).sort((a, b) => b[1] - a[1]); | |
| sorted.slice(0, 5).forEach(([model, cost]) => { | |
| console.log(` ${model.padEnd(25)} $${cost.toFixed(2)}`); | |
| }); | |
| } | |
| // Top costly operations | |
| if (report.topCostlyOperations.length > 0) { | |
| console.log(chalk.bold('\nTop 5 Costly Operations:')); | |
| report.topCostlyOperations.slice(0, 5).forEach((op, i) => { | |
| console.log( | |
| ` ${i + 1}. ${op.timestamp.toLocaleString()} - ` + | |
| `${op.model} - $${op.cost.toFixed(4)}` | |
| ); | |
| }); | |
| } | |
| // Export option | |
| if (options.export) { | |
| await costGuard.exportUsage(options.export, days); | |
| console.log(chalk.green(`\n✅ Report exported to: ${options.export}`)); | |
| } | |
| console.log('\n'); | |
| }); | |
| .action(async (options) => { | |
| try { | |
| const config = configManager.loadOrInit(); | |
| const repo = new Repository(process.cwd()); | |
| const repoId = repo.getId(); | |
| const costGuard = new CostGuard(repoId, config.budget); | |
| const days = parseInt(options.days); | |
| if (isNaN(days) || days <= 0) { | |
| console.error(chalk.red('❌ Invalid days value (must be positive integer)')); | |
| process.exit(1); | |
| } | |
| const report = await costGuard.getUsageReport(days); | |
| console.log(chalk.bold(`\n📊 Usage Report (Last ${days} days)\n`)); | |
| console.log('─'.repeat(60)); | |
| // Summary | |
| console.log(chalk.bold('Summary:')); | |
| console.log(` Daily: $${report.summary.daily.toFixed(2)}`); | |
| console.log(` Weekly: $${report.summary.weekly.toFixed(2)}`); | |
| console.log(` Monthly: $${report.summary.monthly.toFixed(2)}`); | |
| console.log(` All Time: $${report.summary.allTime.toFixed(2)}`); | |
| // By provider | |
| if (Object.keys(report.byProvider).length > 0) { | |
| console.log(chalk.bold('\nBy Provider:')); | |
| for (const [provider, cost] of Object.entries(report.byProvider)) { | |
| console.log(` ${provider.padEnd(15)} $${cost.toFixed(2)}`); | |
| } | |
| } | |
| // By model | |
| if (Object.keys(report.byModel).length > 0) { | |
| console.log(chalk.bold('\nBy Model:')); | |
| const sorted = Object.entries(report.byModel).sort((a, b) => b[1] - a[1]); | |
| sorted.slice(0, 5).forEach(([model, cost]) => { | |
| console.log(` ${model.padEnd(25)} $${cost.toFixed(2)}`); | |
| }); | |
| } | |
| // Top costly operations | |
| if (report.topCostlyOperations.length > 0) { | |
| console.log(chalk.bold('\nTop 5 Costly Operations:')); | |
| report.topCostlyOperations.slice(0, 5).forEach((op, i) => { | |
| console.log( | |
| ` ${i + 1}. ${op.timestamp.toLocaleString()} - ` + | |
| `${op.model} - $${op.cost.toFixed(4)}` | |
| ); | |
| }); | |
| } | |
| // Export option | |
| if (options.export) { | |
| await costGuard.exportUsage(options.export, days); | |
| console.log(chalk.green(`\n✅ Report exported to: ${options.export}`)); | |
| } | |
| console.log('\n'); | |
| } catch (error: any) { | |
| console.error(chalk.red(`❌ Failed to generate report: ${error.message}`)); | |
| process.exit(1); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/commands/budget.ts` around lines 160 - 215, Validate the days input
and add error handling around CostGuard calls: ensure the parsed days from
parseInt(options.days) is a finite positive integer (e.g., check
Number.isInteger(days) && days > 0) and handle invalid inputs by printing a
clear error and returning/exiting; wrap the call to
CostGuard.getUsageReport(days) in a try/catch and handle/report errors
(console.error and non-zero exit or return) so failures don't crash the CLI, and
likewise wrap CostGuard.exportUsage(options.export, days) in a try/catch to
report export failures to the user; update the .action handler where
parseInt(options.days) is used and the calls to CostGuard.getUsageReport and
exportUsage accordingly.
Comment on lines
+23
to
+88
| .action(async (options) => { | ||
| const config = configManager.loadOrInit(); | ||
| const repo = new Repository(process.cwd()); | ||
| const repoId = repo.getId(); | ||
|
|
||
| const collector = new MetricsCollector(repoId, config.telemetryEnabled || false); | ||
| const days = parseInt(options.days); | ||
| const timeRangeMs = days * 24 * 60 * 60 * 1000; | ||
|
|
||
| const metrics = collector.getMetrics(timeRangeMs); | ||
|
|
||
| console.log(chalk.bold(`\n📈 AI Metrics (Last ${days} days)\n`)); | ||
| console.log('─'.repeat(60)); | ||
|
|
||
| // Overall stats | ||
| console.log(chalk.bold('Overview:')); | ||
| console.log(` Total Calls: ${metrics.totalCalls}`); | ||
| console.log(` Success Rate: ${metrics.successRate.toFixed(1)}%`); | ||
| console.log(` Total Cost: $${metrics.totalCost.toFixed(2)}`); | ||
| console.log(` Total Tokens: ${metrics.totalTokens.toLocaleString()}`); | ||
| console.log(` Cache Hit Rate: ${metrics.cacheHitRate.toFixed(1)}%`); | ||
|
|
||
| // Latency | ||
| console.log(chalk.bold('\n⏱️ Latency:')); | ||
| console.log(` Average: ${metrics.averageLatency.toFixed(0)}ms`); | ||
| console.log(` P50: ${metrics.p50Latency.toFixed(0)}ms`); | ||
| console.log(` P95: ${metrics.p95Latency.toFixed(0)}ms`); | ||
| console.log(` P99: ${metrics.p99Latency.toFixed(0)}ms`); | ||
|
|
||
| // By provider | ||
| if (Object.keys(metrics.callsByProvider).length > 0) { | ||
| console.log(chalk.bold('\n🏢 By Provider:')); | ||
| for (const [provider, count] of Object.entries(metrics.callsByProvider)) { | ||
| if (!options.provider || provider === options.provider) { | ||
| console.log(` ${provider.padEnd(15)} ${count} calls`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // By model | ||
| if (Object.keys(metrics.callsByModel).length > 0) { | ||
| console.log(chalk.bold('\n🤖 Top Models:')); | ||
| const sorted = Object.entries(metrics.callsByModel).sort((a, b) => b[1] - a[1]); | ||
| sorted.slice(0, 5).forEach(([model, count]) => { | ||
| console.log(` ${model.padEnd(30)} ${count} calls`); | ||
| }); | ||
| } | ||
|
|
||
| // By operation | ||
| if (Object.keys(metrics.callsByOperation).length > 0) { | ||
| console.log(chalk.bold('\n⚙️ By Operation:')); | ||
| for (const [operation, count] of Object.entries(metrics.callsByOperation)) { | ||
| console.log(` ${operation.padEnd(15)} ${count} calls`); | ||
| } | ||
| } | ||
|
|
||
| // Errors | ||
| if (Object.keys(metrics.errorsByType).length > 0) { | ||
| console.log(chalk.yellow.bold('\n❌ Errors:')); | ||
| for (const [errorType, count] of Object.entries(metrics.errorsByType)) { | ||
| console.log(chalk.yellow(` ${errorType.padEnd(20)} ${count} errors`)); | ||
| } | ||
| } | ||
|
|
||
| console.log('\n'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add input validation and error handling for show command.
The show command has issues similar to the budget report command:
parseInt(options.days)can returnNaNif invalid, leading to incorrecttimeRangeMscalculation.- No try/catch around
collector.getMetrics(), so failures will crash the command.
🛡️ Proposed fix to add validation and error handling
.action(async (options) => {
- const config = configManager.loadOrInit();
- const repo = new Repository(process.cwd());
- const repoId = repo.getId();
-
- const collector = new MetricsCollector(repoId, config.telemetryEnabled || false);
- const days = parseInt(options.days);
- const timeRangeMs = days * 24 * 60 * 60 * 1000;
-
- const metrics = collector.getMetrics(timeRangeMs);
+ try {
+ const config = configManager.loadOrInit();
+ const repo = new Repository(process.cwd());
+ const repoId = repo.getId();
+
+ const collector = new MetricsCollector(repoId, config.telemetryEnabled || false);
+ const days = parseInt(options.days);
+
+ if (isNaN(days) || days <= 0) {
+ console.error(chalk.red('❌ Invalid days value (must be positive integer)'));
+ process.exit(1);
+ }
+
+ const timeRangeMs = days * 24 * 60 * 60 * 1000;
+
+ const metrics = collector.getMetrics(timeRangeMs);
- console.log(chalk.bold(`\n📈 AI Metrics (Last ${days} days)\n`));
+ console.log(chalk.bold(`\n📈 AI Metrics (Last ${days} days)\n`));
// ... rest of display logic
+ } catch (error: any) {
+ console.error(chalk.red(`❌ Failed to retrieve metrics: ${error.message}`));
+ process.exit(1);
+ }
});📝 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.
Suggested change
| .action(async (options) => { | |
| const config = configManager.loadOrInit(); | |
| const repo = new Repository(process.cwd()); | |
| const repoId = repo.getId(); | |
| const collector = new MetricsCollector(repoId, config.telemetryEnabled || false); | |
| const days = parseInt(options.days); | |
| const timeRangeMs = days * 24 * 60 * 60 * 1000; | |
| const metrics = collector.getMetrics(timeRangeMs); | |
| console.log(chalk.bold(`\n📈 AI Metrics (Last ${days} days)\n`)); | |
| console.log('─'.repeat(60)); | |
| // Overall stats | |
| console.log(chalk.bold('Overview:')); | |
| console.log(` Total Calls: ${metrics.totalCalls}`); | |
| console.log(` Success Rate: ${metrics.successRate.toFixed(1)}%`); | |
| console.log(` Total Cost: $${metrics.totalCost.toFixed(2)}`); | |
| console.log(` Total Tokens: ${metrics.totalTokens.toLocaleString()}`); | |
| console.log(` Cache Hit Rate: ${metrics.cacheHitRate.toFixed(1)}%`); | |
| // Latency | |
| console.log(chalk.bold('\n⏱️ Latency:')); | |
| console.log(` Average: ${metrics.averageLatency.toFixed(0)}ms`); | |
| console.log(` P50: ${metrics.p50Latency.toFixed(0)}ms`); | |
| console.log(` P95: ${metrics.p95Latency.toFixed(0)}ms`); | |
| console.log(` P99: ${metrics.p99Latency.toFixed(0)}ms`); | |
| // By provider | |
| if (Object.keys(metrics.callsByProvider).length > 0) { | |
| console.log(chalk.bold('\n🏢 By Provider:')); | |
| for (const [provider, count] of Object.entries(metrics.callsByProvider)) { | |
| if (!options.provider || provider === options.provider) { | |
| console.log(` ${provider.padEnd(15)} ${count} calls`); | |
| } | |
| } | |
| } | |
| // By model | |
| if (Object.keys(metrics.callsByModel).length > 0) { | |
| console.log(chalk.bold('\n🤖 Top Models:')); | |
| const sorted = Object.entries(metrics.callsByModel).sort((a, b) => b[1] - a[1]); | |
| sorted.slice(0, 5).forEach(([model, count]) => { | |
| console.log(` ${model.padEnd(30)} ${count} calls`); | |
| }); | |
| } | |
| // By operation | |
| if (Object.keys(metrics.callsByOperation).length > 0) { | |
| console.log(chalk.bold('\n⚙️ By Operation:')); | |
| for (const [operation, count] of Object.entries(metrics.callsByOperation)) { | |
| console.log(` ${operation.padEnd(15)} ${count} calls`); | |
| } | |
| } | |
| // Errors | |
| if (Object.keys(metrics.errorsByType).length > 0) { | |
| console.log(chalk.yellow.bold('\n❌ Errors:')); | |
| for (const [errorType, count] of Object.entries(metrics.errorsByType)) { | |
| console.log(chalk.yellow(` ${errorType.padEnd(20)} ${count} errors`)); | |
| } | |
| } | |
| console.log('\n'); | |
| }); | |
| .action(async (options) => { | |
| try { | |
| const config = configManager.loadOrInit(); | |
| const repo = new Repository(process.cwd()); | |
| const repoId = repo.getId(); | |
| const collector = new MetricsCollector(repoId, config.telemetryEnabled || false); | |
| const days = parseInt(options.days); | |
| if (isNaN(days) || days <= 0) { | |
| console.error(chalk.red('❌ Invalid days value (must be positive integer)')); | |
| process.exit(1); | |
| } | |
| const timeRangeMs = days * 24 * 60 * 60 * 1000; | |
| const metrics = collector.getMetrics(timeRangeMs); | |
| console.log(chalk.bold(`\n📈 AI Metrics (Last ${days} days)\n`)); | |
| console.log('─'.repeat(60)); | |
| // Overall stats | |
| console.log(chalk.bold('Overview:')); | |
| console.log(` Total Calls: ${metrics.totalCalls}`); | |
| console.log(` Success Rate: ${metrics.successRate.toFixed(1)}%`); | |
| console.log(` Total Cost: $${metrics.totalCost.toFixed(2)}`); | |
| console.log(` Total Tokens: ${metrics.totalTokens.toLocaleString()}`); | |
| console.log(` Cache Hit Rate: ${metrics.cacheHitRate.toFixed(1)}%`); | |
| // Latency | |
| console.log(chalk.bold('\n⏱️ Latency:')); | |
| console.log(` Average: ${metrics.averageLatency.toFixed(0)}ms`); | |
| console.log(` P50: ${metrics.p50Latency.toFixed(0)}ms`); | |
| console.log(` P95: ${metrics.p95Latency.toFixed(0)}ms`); | |
| console.log(` P99: ${metrics.p99Latency.toFixed(0)}ms`); | |
| // By provider | |
| if (Object.keys(metrics.callsByProvider).length > 0) { | |
| console.log(chalk.bold('\n🏢 By Provider:')); | |
| for (const [provider, count] of Object.entries(metrics.callsByProvider)) { | |
| if (!options.provider || provider === options.provider) { | |
| console.log(` ${provider.padEnd(15)} ${count} calls`); | |
| } | |
| } | |
| } | |
| // By model | |
| if (Object.keys(metrics.callsByModel).length > 0) { | |
| console.log(chalk.bold('\n🤖 Top Models:')); | |
| const sorted = Object.entries(metrics.callsByModel).sort((a, b) => b[1] - a[1]); | |
| sorted.slice(0, 5).forEach(([model, count]) => { | |
| console.log(` ${model.padEnd(30)} ${count} calls`); | |
| }); | |
| } | |
| // By operation | |
| if (Object.keys(metrics.callsByOperation).length > 0) { | |
| console.log(chalk.bold('\n⚙️ By Operation:')); | |
| for (const [operation, count] of Object.entries(metrics.callsByOperation)) { | |
| console.log(` ${operation.padEnd(15)} ${count} calls`); | |
| } | |
| } | |
| // Errors | |
| if (Object.keys(metrics.errorsByType).length > 0) { | |
| console.log(chalk.yellow.bold('\n❌ Errors:')); | |
| for (const [errorType, count] of Object.entries(metrics.errorsByType)) { | |
| console.log(chalk.yellow(` ${errorType.padEnd(20)} ${count} errors`)); | |
| } | |
| } | |
| console.log('\n'); | |
| } catch (error: any) { | |
| console.error(chalk.red(`❌ Failed to retrieve metrics: ${error.message}`)); | |
| process.exit(1); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/commands/metrics.ts` around lines 23 - 88, Validate and guard against
invalid days input and wrap metrics retrieval in error handling: check
options.days (used in parseInt(options.days)) and if parseInt returns NaN or
<=0, print a clear error and return/exit; compute days only after validation and
clamp to a safe integer before deriving timeRangeMs; surround the call to
collector.getMetrics(timeRangeMs) (MetricsCollector.getMetrics) with a
try/catch, log the caught error via console.error (or processLogger) with
context and return/exit gracefully to avoid crashing the command.
Comment on lines
+97
to
+112
| .action(async (options) => { | ||
| const config = configManager.loadOrInit(); | ||
| const repo = new Repository(process.cwd()); | ||
| const repoId = repo.getId(); | ||
|
|
||
| const collector = new MetricsCollector(repoId, config.telemetryEnabled || false); | ||
| const days = parseInt(options.days); | ||
| const timeRangeMs = days * 24 * 60 * 60 * 1000; | ||
|
|
||
| if (options.format === 'json') { | ||
| collector.exportToJSON(options.output, timeRangeMs); | ||
| console.log(chalk.green(`✅ Metrics exported to: ${options.output}\n`)); | ||
| } else { | ||
| console.error(chalk.red('❌ Only JSON format is currently supported')); | ||
| process.exit(1); | ||
| } |
Contributor
There was a problem hiding this comment.
Add input validation and error handling for export command.
Same issues as the show command: no validation for days input and no error handling for exportToJSON().
🛡️ Proposed fix
.action(async (options) => {
- const config = configManager.loadOrInit();
- const repo = new Repository(process.cwd());
- const repoId = repo.getId();
-
- const collector = new MetricsCollector(repoId, config.telemetryEnabled || false);
- const days = parseInt(options.days);
- const timeRangeMs = days * 24 * 60 * 60 * 1000;
-
- if (options.format === 'json') {
- collector.exportToJSON(options.output, timeRangeMs);
- console.log(chalk.green(`✅ Metrics exported to: ${options.output}\n`));
- } else {
- console.error(chalk.red('❌ Only JSON format is currently supported'));
- process.exit(1);
- }
+ try {
+ const config = configManager.loadOrInit();
+ const repo = new Repository(process.cwd());
+ const repoId = repo.getId();
+
+ const collector = new MetricsCollector(repoId, config.telemetryEnabled || false);
+ const days = parseInt(options.days);
+
+ if (isNaN(days) || days <= 0) {
+ console.error(chalk.red('❌ Invalid days value (must be positive integer)'));
+ process.exit(1);
+ }
+
+ const timeRangeMs = days * 24 * 60 * 60 * 1000;
+
+ if (options.format === 'json') {
+ collector.exportToJSON(options.output, timeRangeMs);
+ console.log(chalk.green(`✅ Metrics exported to: ${options.output}\n`));
+ } else {
+ console.error(chalk.red('❌ Only JSON format is currently supported'));
+ process.exit(1);
+ }
+ } catch (error: any) {
+ console.error(chalk.red(`❌ Failed to export metrics: ${error.message}`));
+ process.exit(1);
+ }
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/commands/metrics.ts` around lines 97 - 112, The export command lacks
validation for the days input and error handling around
MetricsCollector.exportToJSON: validate that options.days is a positive integer
before using parseInt and computing timeRangeMs (check for NaN and days > 0),
and wrap the call to collector.exportToJSON(options.output, timeRangeMs) in a
try/catch that logs the error via console.error (with a clear message including
error.message) and exits non‑zero; update the action handler that constructs
MetricsCollector (referencing MetricsCollector, exportToJSON,
parseInt/options.days, and timeRangeMs) to perform these checks and handle
failures gracefully.
Comment on lines
+175
to
+180
| if (!validTasks.includes(task as TaskType)) { | ||
| console.error( | ||
| chalk.red(`❌ Invalid task "${task}". Valid tasks: ${validTasks.join(', ')}`) | ||
| ); | ||
| process.exit(1); | ||
| } |
Contributor
There was a problem hiding this comment.
Replace process.exit(1) with error throwing.
The test and clear subcommands call process.exit(1) directly in validation branches, bypassing proper error handling. Consider throwing errors instead.
Also applies to: 204-207, 230-233, 236-241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/commands/routing.ts` around lines 175 - 180, Replace direct
process.exit(1) calls in the validation branches with thrown errors so the CLI's
error-handling flow and tests can catch them; specifically, in the routing
command where you check if (!validTasks.includes(task as TaskType)) (and the
analogous checks at the other locations referenced), throw a descriptive Error
(e.g., `throw new Error("Invalid task: ...")`) instead of calling process.exit,
and ensure the surrounding handlers (command runner or top-level promise
rejection handler) will handle the thrown error. This applies to the blocks that
reference validTasks, task, and TaskType as well as the validation branches
around the test and clear subcommands.
Comment on lines
+380
to
+386
| getChatModels(provider?: string): ModelInfo[] { | ||
| const models = provider | ||
| ? this.listProviderModels(provider) | ||
| : this.getAllModels(); | ||
|
|
||
| return models.filter((model) => !model.supportsEmbeddings || model.embeddingDimensions === undefined); | ||
| } |
Contributor
There was a problem hiding this comment.
Incorrect filter logic excludes dual-capability chat models.
The filter !model.supportsEmbeddings || model.embeddingDimensions === undefined will exclude any model that both supports embeddings AND has defined dimensions, even if it's primarily a chat model. For example, gemini-2.5-flash (lines 134-146) supports embeddings with 768 dimensions but is also tagged for chat/coding tasks—it would be filtered out here.
The intended logic is probably to exclude embedding-only models. A better approach would be to check for the absence of chat/generation capabilities rather than the presence of embedding support.
🔧 Proposed fix to correctly identify chat models
getChatModels(provider?: string): ModelInfo[] {
const models = provider
? this.listProviderModels(provider)
: this.getAllModels();
- return models.filter((model) => !model.supportsEmbeddings || model.embeddingDimensions === undefined);
+ return models.filter((model) =>
+ !model.supportsEmbeddings ||
+ model.embeddingDimensions === undefined ||
+ model.tags.some(tag => ['coding', 'chat', 'reasoning', 'general', 'analysis'].includes(tag))
+ );
}Alternatively, add a dedicated supportsChat or modelType field to ModelInfo to make the distinction explicit.
📝 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.
Suggested change
| getChatModels(provider?: string): ModelInfo[] { | |
| const models = provider | |
| ? this.listProviderModels(provider) | |
| : this.getAllModels(); | |
| return models.filter((model) => !model.supportsEmbeddings || model.embeddingDimensions === undefined); | |
| } | |
| getChatModels(provider?: string): ModelInfo[] { | |
| const models = provider | |
| ? this.listProviderModels(provider) | |
| : this.getAllModels(); | |
| return models.filter((model) => | |
| !model.supportsEmbeddings || | |
| model.embeddingDimensions === undefined || | |
| model.tags.some(tag => ['coding', 'chat', 'reasoning', 'general', 'analysis'].includes(tag)) | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/providers/model-registry.ts` around lines 380 - 386, getChatModels
currently filters out any model that supports embeddings and has
embeddingDimensions, which wrongly excludes dual-capability chat+embedding
models (e.g. gemini-2.5-flash); update the filter in getChatModels to select
models that have chat/generation capability (e.g. check an existing flag like
supportsGeneration or add/supportsChat or a modelType field on ModelInfo) and
exclude only models that are embedding-only, i.e. those that lack
chat/generation capability rather than those that merely support embeddings.
Comment on lines
+174
to
+177
| // Must support chat (not embedding-only models) | ||
| if (model.supportsEmbeddings && model.embeddingDimensions && !model.tags.includes('chat')) { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Chat filter references non-existent tag.
The condition checks !model.tags.includes('chat'), but scanning the MODEL_REGISTRY (lines 27-254 in model-registry.ts), no models have a 'chat' tag. Models use tags like 'coding', 'reasoning', 'fast', 'general', etc. This means dual-capability models (e.g., gemini-2.5-flash which supports embeddings but is also a chat/coding model) will be incorrectly filtered out.
This is part of a broader inconsistency: model-registry.ts's getChatModels (line 385) also has flawed logic for distinguishing chat vs embedding-only models.
🔧 Proposed fix (align with registry tags)
// Must support chat (not embedding-only models)
- if (model.supportsEmbeddings && model.embeddingDimensions && !model.tags.includes('chat')) {
- return false;
- }
+ // Exclude embedding-only models by checking for chat/generation/reasoning capability tags
+ const chatCapabilityTags = ['coding', 'reasoning', 'general', 'analysis', 'multimodal'];
+ if (model.supportsEmbeddings && model.embeddingDimensions) {
+ const hasGenerationCapability = model.tags.some(tag => chatCapabilityTags.includes(tag));
+ if (!hasGenerationCapability) {
+ return false;
+ }
+ }Better long-term solution: Add explicit modelType: 'chat' | 'embedding' | 'dual' to ModelInfo in model-registry.ts to avoid fragile tag-based heuristics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/providers/model-router.ts` around lines 174 - 177, The filter wrongly
checks for a non-existent 'chat' tag (in the condition around
model.supportsEmbeddings / model.embeddingDimensions / model.tags), causing
dual-capability models to be excluded; update the check to detect chat-capable
models using existing registry tags (e.g., treat models as chat-capable if they
lack an explicit 'embedding-only' tag or if they include common chat-capable
tags like 'coding', 'reasoning', 'general', 'fast'), and apply the same
corrected logic inside getChatModels in MODEL_REGISTRY; for a robust long-term
fix, add a ModelInfo field (e.g., modelType: 'chat'|'embedding'|'dual') in
model-registry.ts and switch both model-router and getChatModels to use that
instead of tag heuristics.
Comment on lines
+146
to
+154
| ### By Cost (Per 1M tokens) | ||
|
|
||
| | Model | Input | Output | Best For | | ||
| | --------------------- | ------ | ------- | -------------------------- | | ||
| | gpt-4.1-mini | $0.15 | $0.60 | Chat, simple tasks | | ||
| | gemini-2.5-flash-lite | $37.50 | $150 | Fast, balanced tasks | | ||
| | gemini-2.5-flash | $75 | $300 | Code generation | | ||
| | gpt-4o | $2,500 | $10,000 | Code review, quality tasks | | ||
| | claude-sonnet-4.5 | $3,000 | $15,000 | Reasoning, explanation | |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Extract pricing from model-registry and compare with docs table
echo "=== Registry pricing values ==="
rg -A1 "name: 'gpt-4o'" cli/src/providers/model-registry.ts | rg "Pricing"
rg -A1 "name: 'gpt-4.1-mini'" cli/src/providers/model-registry.ts | rg "Pricing"
echo -e "\n=== Calculation references ==="
rg -n "/ 1000000.*Pricing" cli/src/providers/
echo -e "\n=== Docs table ==="
rg -A1 "gpt-4o.*\|" docs/AI_QUICK_REFERENCE.md | head -2Repository: ntanwir10/GuardScan
Length of output: 651
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate gpt-4o in model-registry ==="
rg -n "gpt-4o" cli/src/providers/model-registry.ts || true
echo
echo "=== Locate gpt-4.1-mini in model-registry ==="
rg -n "gpt-4\.1-mini" cli/src/providers/model-registry.ts || true
echo
echo "=== Extract full registry block(s) for those models (context) ==="
rg -n "gpt-4o|gpt-4\.1-mini" -n cli/src/providers/model-registry.ts -C 8 || true
echo
echo "=== List all inputPricing/outputPricing occurrences around those models ==="
rg -n "gpt-4o|gpt-4\.1-mini" -C 30 cli/src/providers/model-registry.ts | rg -n "inputPricing|outputPricing" || true
echo
echo "=== Check for unit/semantics hints near pricing fields ==="
rg -n "per 1M|per 1K|1M tokens|1K tokens|pricing unit|Pricing unit" cli/src/providers/model-registry.ts || true
echo
echo "=== Confirm calculation divisors in model-router/model-validator ==="
rg -n "/ 1000000.*inputPricing|/ 1000000.*outputPricing|/ 1000000.*Pricing" cli/src/providers/model-router.ts cli/src/providers/model-validator.ts || trueRepository: ntanwir10/GuardScan
Length of output: 3035
🏁 Script executed:
#!/bin/bash
set -euo pipefail
DOCS_FILE="docs/AI_QUICK_REFERENCE.md"
REGISTRY_FILE="cli/src/providers/model-registry.ts"
# Models as they appear in the docs snippet
MODELS=(
"gpt-4.1-mini"
"gemini-2.5-flash-lite"
"gemini-2.5-flash"
"gpt-4o"
"claude-sonnet-4.5"
)
echo "=== Docs table extracted rows (sanity) ==="
for m in "${MODELS[@]}"; do
rg -n "${m//./\\.}" "$DOCS_FILE" || true
done
echo
echo "=== Registry pricing for models ==="
for m in "${MODELS[@]}"; do
echo "--- ${m} ---"
# Print the object block around the model key
rg -n "'${m}'\\s*:" "$REGISTRY_FILE" -n -C 6 || true
# Specifically print input/output pricing lines near the model key
rg -n "'${m}'\\s*:" "$REGISTRY_FILE" -C 25 | rg "inputPricing|outputPricing|embeddingPricing|Pricing|per 1M|per 1K" || true
echo
done
echo "=== Unit comment near pricing field in model-registry ==="
rg -n "inputPricing: number;|outputPricing: number;|per 1M tokens|per 1K tokens|pricing unit|Pricing unit" "$REGISTRY_FILE" || trueRepository: ntanwir10/GuardScan
Length of output: 5340
Fix critical: pricing unit mismatch causes ~1000× wrong cost estimates (and GPT-4o docs row is off by 1000×).
cli/src/providers/model-registry.tsdocumentsinputPricing/outputPricingas per 1M tokens, but the values (e.g.,gpt-4.1-mini0.00015/0.0006,gemini-2.5-flash-lite0.0375/0.15,claude-sonnet-4.53.0/15.0) align with $ per 1K tokens (multiply by 1000 matches the docs table).cli/src/providers/model-router.tsandcli/src/providers/model-validator.tscompute cost usingtokens / 1_000_000, so estimates are 1000× too low:Example:const estimatedCost = (1000 / 1000000) * model.inputPricing + (500 / 1000000) * model.outputPricing;
gpt-4oinputPricing: 0.0025⇒(1000/1_000_000)*0.0025 = $0.0000025(instead of ~$2.5 per 1M).docs/AI_QUICK_REFERENCE.mdshowsgpt-4oas $2,500/$10,000 per 1M tokens, but the registry-consistent figures from0.0025/0.01imply $2.50/$10.00 per 1M.
Fix by standardizing units: either convert registry pricing to true per-1M dollars to match the tokens / 1_000_000 math, or change the cost math to tokens / 1000 (and update the docs unit/values accordingly, including the GPT-4o row).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/AI_QUICK_REFERENCE.md` around lines 146 - 154, The registry currently
stores inputPricing/outputPricing as $ per 1K tokens while cost math in
model-router/model-validator uses tokens/1_000_000, causing 1000×
underestimates; fix by standardizing to $ per 1M tokens: update the numeric
constants in the model registry (the inputPricing and outputPricing entries) by
multiplying each value by 1000 so they represent dollars per 1M tokens, then
verify and update the docs table row for gpt-4o and other models to the new
per-1M values, and run/adjust any unit tests or assertions that reference
estimatedCost (the computed variable using tokens / 1_000_000) to ensure
calculations now match the corrected registry values.
Contributor
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)
cli/__tests__/providers/decorators/rate-limited-provider.test.ts (1)
79-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTiming assertions are CI-flaky and add avoidable runtime.
Line 79 onward relies on real elapsed wall-clock timing and explicit sleeps, which can produce nondeterministic failures and slow runs. Prefer fake timers (and controlled
Date.now) for deterministic token-refill/wait validation.Also applies to: 101-120, 122-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/__tests__/providers/decorators/rate-limited-provider.test.ts` around lines 79 - 99, The test uses real wall-clock timing (Date.now and actual waits) causing flakiness; change the test(s) that exercise RateLimitedProvider.chat (including those around line ranges 79-99, 101-120, 122-137) to use fake timers and a controlled Date.now instead of real sleeps: call jest.useFakeTimers(), set an initial system time (or mock Date.now), invoke the first chat to consume tokens, then advance timers and system time with jest.advanceTimersByTime(...) (or jest.setSystemTime(...)) to simulate token refill before calling chat again, and finally assert expected behavior without relying on real elapsed ms. Ensure you restore real timers after the test (jest.useRealTimers()) and reference RateLimitedProvider, MockProvider, and chat when updating the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/__tests__/commands/cache.test.ts`:
- Around line 212-233: The test only checks for console output but doesn't
assert the default cache size; update the test to verify AICache was constructed
with maxSizeMB = 100 when the loaded config lacks a cache property: use the
existing require('../../src/core/ai-cache') to spy/mock the AICache constructor
(AICache or its factory) and after await statsCmd.parseAsync(...) assert the
constructor was called and that the config/argument passed to AICache contains
cache: { maxSizeMB: 100 } (or that the created instance exposes a maxSizeMB
property equal to 100). Keep using createCacheCommand and
mockedConfigManager.loadOrInit as the setup and reuse consoleLogSpy only for the
logging assertion.
---
Outside diff comments:
In `@cli/__tests__/providers/decorators/rate-limited-provider.test.ts`:
- Around line 79-99: The test uses real wall-clock timing (Date.now and actual
waits) causing flakiness; change the test(s) that exercise
RateLimitedProvider.chat (including those around line ranges 79-99, 101-120,
122-137) to use fake timers and a controlled Date.now instead of real sleeps:
call jest.useFakeTimers(), set an initial system time (or mock Date.now), invoke
the first chat to consume tokens, then advance timers and system time with
jest.advanceTimersByTime(...) (or jest.setSystemTime(...)) to simulate token
refill before calling chat again, and finally assert expected behavior without
relying on real elapsed ms. Ensure you restore real timers after the test
(jest.useRealTimers()) and reference RateLimitedProvider, MockProvider, and chat
when updating the tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cdabef4-702f-4fa3-b69e-ac3a574d89a9
📒 Files selected for processing (3)
cli/__tests__/commands/budget.test.tscli/__tests__/commands/cache.test.tscli/__tests__/providers/decorators/rate-limited-provider.test.ts
Comment on lines
+212
to
+233
| it('should use default maxSizeMB when cache config is absent', async () => { | ||
| mockedConfigManager.loadOrInit.mockReturnValueOnce({ | ||
| clientId: 'test-client', | ||
| provider: 'openai', | ||
| telemetryEnabled: false, | ||
| offlineMode: false, | ||
| createdAt: new Date().toISOString(), | ||
| lastUsed: new Date().toISOString(), | ||
| // No cache property | ||
| } as any); | ||
|
|
||
| const { AICache } = require('../../src/core/ai-cache'); | ||
| const constructorSpy = jest.spyOn(AICache, 'mock' as any); | ||
|
|
||
| const cmd = createCacheCommand(); | ||
| const statsCmd = cmd.commands.find((c) => c.name() === 'stats')!; | ||
|
|
||
| await statsCmd.parseAsync([], { from: 'user' }); | ||
|
|
||
| // Should still run without errors | ||
| expect(consoleLogSpy).toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Default maxSizeMB test does not validate the claimed behavior.
This case currently only proves the command logs output; it does not verify that the fallback maxSizeMB=100 path was used. Please assert constructor/config arguments (or emitted value) directly so regressions are caught.
Proposed test tightening
- const { AICache } = require('../../src/core/ai-cache');
- const constructorSpy = jest.spyOn(AICache, 'mock' as any);
+ const { AICache } = require('../../src/core/ai-cache');
const cmd = createCacheCommand();
const statsCmd = cmd.commands.find((c) => c.name() === 'stats')!;
await statsCmd.parseAsync([], { from: 'user' });
- // Should still run without errors
- expect(consoleLogSpy).toHaveBeenCalled();
+ expect(AICache).toHaveBeenCalled();
+ const allOutput = consoleLogSpy.mock.calls.flat().join(' ');
+ expect(allOutput).toMatch(/100/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/__tests__/commands/cache.test.ts` around lines 212 - 233, The test only
checks for console output but doesn't assert the default cache size; update the
test to verify AICache was constructed with maxSizeMB = 100 when the loaded
config lacks a cache property: use the existing
require('../../src/core/ai-cache') to spy/mock the AICache constructor (AICache
or its factory) and after await statsCmd.parseAsync(...) assert the constructor
was called and that the config/argument passed to AICache contains cache: {
maxSizeMB: 100 } (or that the created instance exposes a maxSizeMB property
equal to 100). Keep using createCacheCommand and mockedConfigManager.loadOrInit
as the setup and reuse consoleLogSpy only for the logging assertion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
models,routing,budget,metrics, andcacheCLI commandsProviderFactory.createForCli()across AI commands with enhanced config defaultslintscripts, and optionaltiktokendeps inpackage.jsonStacks on: #28
Test plan
npm run buildpassesnpm test— 422 tests passguardscan models listandguardscan reviewwith configured providerSummary by CodeRabbit
Release Notes
New Features
budgetcommandmetricscommandmodelscommand for discovery, comparison, and recommendationscachecommand for viewing stats and clearing entriesDocumentation