From 2c40dfb5d4092d8366e1112524ea400c981c0e96 Mon Sep 17 00:00:00 2001 From: GodsBoy Date: Sat, 28 Mar 2026 00:23:38 +0200 Subject: [PATCH 1/4] feat(ai): return structured response with token usage from AI providers Replace the plain string return from AIProvider.complete() with a structured AIResponse containing text and usage (inputTokens, outputTokens). Update all three providers (Claude, OpenAI, Ollama), all callers, and all test mocks to use the new shape. --- src/ai/claude.ts | 19 +++++++++++++------ src/ai/index.ts | 2 +- src/ai/ollama.ts | 21 +++++++++++++++++---- src/ai/openai.ts | 15 +++++++++++---- src/ai/provider.ts | 16 +++++++++++++++- src/pr/summariser.ts | 4 ++-- src/review/reviewer.ts | 4 ++-- src/triage/classifier.ts | 2 +- src/triage/duplicate.ts | 2 +- src/triage/responder.ts | 2 +- tests/classifier.test.ts | 2 +- tests/duplicate.test.ts | 10 ++++++---- tests/responder.test.ts | 12 +++++++----- tests/summariser.test.ts | 2 +- 14 files changed, 79 insertions(+), 34 deletions(-) diff --git a/src/ai/claude.ts b/src/ai/claude.ts index cc2e3c7..9c4af40 100644 --- a/src/ai/claude.ts +++ b/src/ai/claude.ts @@ -1,5 +1,5 @@ import Anthropic from '@anthropic-ai/sdk'; -import type { AIProvider } from './provider.js'; +import type { AIProvider, AIResponse, AIRequestOptions } from './provider.js'; export class ClaudeProvider implements AIProvider { private client: Anthropic; @@ -14,17 +14,24 @@ export class ClaudeProvider implements AIProvider { this.model = model; } - async complete(prompt: string): Promise { + async complete(prompt: string, options?: AIRequestOptions): Promise { const response = await this.client.messages.create({ model: this.model, - max_tokens: 1024, + max_tokens: options?.maxTokens ?? 1024, messages: [{ role: 'user', content: prompt }], }); const block = response.content[0]; - if (block.type === 'text') { - return block.text; + if (block.type !== 'text') { + throw new Error('Unexpected response type from Claude API'); } - throw new Error('Unexpected response type from Claude API'); + + return { + text: block.text, + usage: { + inputTokens: response.usage.input_tokens, + outputTokens: response.usage.output_tokens, + }, + }; } } diff --git a/src/ai/index.ts b/src/ai/index.ts index 4457c4d..2c96fed 100644 --- a/src/ai/index.ts +++ b/src/ai/index.ts @@ -4,7 +4,7 @@ import { ClaudeProvider } from './claude.js'; import { OpenAIProvider } from './openai.js'; import { OllamaProvider } from './ollama.js'; -export type { AIProvider } from './provider.js'; +export type { AIProvider, AIResponse, AIUsage, AIRequestOptions } from './provider.js'; export function createAIProvider(config: RepoKeeperConfig['ai']): AIProvider { switch (config.provider) { diff --git a/src/ai/ollama.ts b/src/ai/ollama.ts index 809403a..98813f6 100644 --- a/src/ai/ollama.ts +++ b/src/ai/ollama.ts @@ -1,4 +1,10 @@ -import type { AIProvider } from './provider.js'; +import type { AIProvider, AIResponse, AIRequestOptions } from './provider.js'; + +interface OllamaResponse { + response: string; + prompt_eval_count?: number; + eval_count?: number; +} export class OllamaProvider implements AIProvider { private url: string; @@ -9,7 +15,7 @@ export class OllamaProvider implements AIProvider { this.model = model; } - async complete(prompt: string): Promise { + async complete(prompt: string, options?: AIRequestOptions): Promise { const response = await fetch(`${this.url}/api/generate`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, @@ -17,6 +23,7 @@ export class OllamaProvider implements AIProvider { model: this.model, prompt, stream: false, + ...(options?.maxTokens ? { options: { num_predict: options.maxTokens } } : {}), }), }); @@ -24,7 +31,13 @@ export class OllamaProvider implements AIProvider { throw new Error(`Ollama API error: ${response.status} ${response.statusText}`); } - const data = (await response.json()) as { response: string }; - return data.response; + const data = (await response.json()) as OllamaResponse; + return { + text: data.response, + usage: { + inputTokens: data.prompt_eval_count ?? 0, + outputTokens: data.eval_count ?? 0, + }, + }; } } diff --git a/src/ai/openai.ts b/src/ai/openai.ts index 1e877d2..bc5e1f0 100644 --- a/src/ai/openai.ts +++ b/src/ai/openai.ts @@ -1,5 +1,5 @@ import OpenAI from 'openai'; -import type { AIProvider } from './provider.js'; +import type { AIProvider, AIResponse, AIRequestOptions } from './provider.js'; export class OpenAIProvider implements AIProvider { private client: OpenAI; @@ -15,17 +15,24 @@ export class OpenAIProvider implements AIProvider { this.model = model; } - async complete(prompt: string): Promise { + async complete(prompt: string, options?: AIRequestOptions): Promise { const response = await this.client.chat.completions.create({ model: this.model, messages: [{ role: 'user', content: prompt }], - max_tokens: 1024, + max_tokens: options?.maxTokens ?? 1024, }); const content = response.choices[0]?.message?.content; if (!content) { throw new Error('Empty response from OpenAI API'); } - return content; + + return { + text: content, + usage: { + inputTokens: response.usage?.prompt_tokens ?? 0, + outputTokens: response.usage?.completion_tokens ?? 0, + }, + }; } } diff --git a/src/ai/provider.ts b/src/ai/provider.ts index 453e0f4..d5c75ff 100644 --- a/src/ai/provider.ts +++ b/src/ai/provider.ts @@ -1,3 +1,17 @@ +export interface AIUsage { + inputTokens: number; + outputTokens: number; +} + +export interface AIResponse { + text: string; + usage: AIUsage; +} + +export interface AIRequestOptions { + maxTokens?: number; +} + export interface AIProvider { - complete(prompt: string): Promise; + complete(prompt: string, options?: AIRequestOptions): Promise; } diff --git a/src/pr/summariser.ts b/src/pr/summariser.ts index d65c8fd..6394783 100644 --- a/src/pr/summariser.ts +++ b/src/pr/summariser.ts @@ -80,7 +80,7 @@ export async function handlePullRequest( .replace('{description}', body || '(no description)') .replace('{diff}', truncatedDiff); - const summary = await ai.complete(prompt); + const { text: summary } = await ai.complete(prompt); // Build the comment const fileList = files @@ -122,7 +122,7 @@ export async function handlePullRequestMerged( .replace('{description}', body || '(no description)') .replace('{fileSummary}', fileSummary); - const releaseNotes = await ai.complete(prompt); + const { text: releaseNotes } = await ai.complete(prompt); const comment = `## Release Notes\n\n${releaseNotes}\n\n` + diff --git a/src/review/reviewer.ts b/src/review/reviewer.ts index 4437ead..2dd7563 100644 --- a/src/review/reviewer.ts +++ b/src/review/reviewer.ts @@ -182,8 +182,8 @@ export async function handleCodeReview( // Build prompt and call AI const prompt = buildReviewPrompt(truncatedDiff, enrichedFiles, reviewConfig, acceptedPatterns); - const aiResponse = await ai.complete(prompt); - const result = parseAIResponse(aiResponse); + const { text: aiResponseText } = await ai.complete(prompt); + const result = parseAIResponse(aiResponseText); // Post review via GitHub API const octokit = new Octokit({ auth: config.github.token }); diff --git a/src/triage/classifier.ts b/src/triage/classifier.ts index b77649f..0d72f8a 100644 --- a/src/triage/classifier.ts +++ b/src/triage/classifier.ts @@ -51,7 +51,7 @@ export async function classifyIssue( } const prompt = CLASSIFY_PROMPT.replace('{title}', title).replace('{body}', body || '(empty)'); - const result = (await ai.complete(prompt)).trim().toLowerCase(); + const result = (await ai.complete(prompt)).text.trim().toLowerCase(); const valid: IssueCategory[] = ['bug', 'feature', 'question', 'duplicate', 'docs', 'invalid']; if (valid.includes(result as IssueCategory)) { diff --git a/src/triage/duplicate.ts b/src/triage/duplicate.ts index 4a04a52..9d80012 100644 --- a/src/triage/duplicate.ts +++ b/src/triage/duplicate.ts @@ -87,7 +87,7 @@ async function findDuplicatesWithAI( .replace('{existingTitle}', candidate.title) .replace('{existingBody}', candidate.body || '(empty)'); - const response = (await ai.complete(prompt)).trim(); + const response = (await ai.complete(prompt)).text.trim(); const score = parseFloat(response); if (!isNaN(score) && score >= 0 && score <= 1 && score >= threshold) { diff --git a/src/triage/responder.ts b/src/triage/responder.ts index 610e620..ce13664 100644 --- a/src/triage/responder.ts +++ b/src/triage/responder.ts @@ -110,7 +110,7 @@ async function generateComment( .replace('{category}', category); try { - const response = (await ai.complete(prompt)).trim(); + const response = (await ai.complete(prompt)).text.trim(); // Sanity check: if AI returns something too short or suspicious, use fallback if (response.length < 20) { return buildFallbackComment(category, title); diff --git a/tests/classifier.test.ts b/tests/classifier.test.ts index 48450fb..288248d 100644 --- a/tests/classifier.test.ts +++ b/tests/classifier.test.ts @@ -3,7 +3,7 @@ import { classifyIssue, categoryToLabel, isVagueIssue } from '../src/triage/clas import type { AIProvider } from '../src/ai/provider.js'; function mockAI(response: string): AIProvider { - return { complete: async () => response }; + return { complete: async () => ({ text: response, usage: { inputTokens: 0, outputTokens: 0 } }) }; } describe('isVagueIssue', () => { diff --git a/tests/duplicate.test.ts b/tests/duplicate.test.ts index bf89797..582ccf5 100644 --- a/tests/duplicate.test.ts +++ b/tests/duplicate.test.ts @@ -14,17 +14,19 @@ const installIssues = [ { number: 4, title: 'Add dark mode support', body: 'Please add dark mode theme to the settings page' }, ]; +const usage = { inputTokens: 0, outputTokens: 0 }; + function mockAI(response: string): AIProvider { - return { complete: async () => response }; + return { complete: async () => ({ text: response, usage }) }; } function mockAIDynamic(responses: Map): AIProvider { return { complete: async (prompt: string) => { for (const [key, value] of responses) { - if (prompt.includes(key)) return value; + if (prompt.includes(key)) return { text: value, usage }; } - return '0.1'; + return { text: '0.1', usage }; }, }; } @@ -168,7 +170,7 @@ describe('findDuplicates with AI', () => { const countingAI: AIProvider = { complete: async () => { aiCallCount++; - return '0.3'; + return { text: '0.3', usage }; }, }; diff --git a/tests/responder.test.ts b/tests/responder.test.ts index a8cb281..29045ed 100644 --- a/tests/responder.test.ts +++ b/tests/responder.test.ts @@ -4,16 +4,18 @@ import type { AIProvider } from '../src/ai/provider.js'; import type { GitHubClient } from '../src/github/client.js'; import type { RepoKeeperConfig } from '../src/config.js'; +const usage = { inputTokens: 0, outputTokens: 0 }; + function createMockAI(classifyResponse: string, commentResponse: string): AIProvider { let callCount = 0; return { complete: async (prompt: string) => { // Duplicate detection calls come first, then classify, then comment - if (prompt.includes('duplicate issue detector')) return '0.1'; + if (prompt.includes('duplicate issue detector')) return { text: '0.1', usage }; callCount++; // Even calls = classify, odd calls = comment (roughly) - if (prompt.includes('classifier') || prompt.includes('Classify')) return classifyResponse; - return commentResponse; + if (prompt.includes('classifier') || prompt.includes('Classify')) return { text: classifyResponse, usage }; + return { text: commentResponse, usage }; }, }; } @@ -116,8 +118,8 @@ describe('handleIssueOpened', () => { it('flags duplicates with possible-duplicate label instead of closing', async () => { const ai: AIProvider = { complete: async (prompt: string) => { - if (prompt.includes('duplicate issue detector')) return '0.85'; - return 'comment'; + if (prompt.includes('duplicate issue detector')) return { text: '0.85', usage }; + return { text: 'comment', usage }; }, }; const github = createMockGithub(); diff --git a/tests/summariser.test.ts b/tests/summariser.test.ts index cdbefa4..7a7a832 100644 --- a/tests/summariser.test.ts +++ b/tests/summariser.test.ts @@ -4,7 +4,7 @@ import type { AIProvider } from '../src/ai/provider.js'; import type { RepoKeeperConfig } from '../src/config.js'; function mockAI(response: string): AIProvider { - return { complete: vi.fn().mockResolvedValue(response) }; + return { complete: vi.fn().mockResolvedValue({ text: response, usage: { inputTokens: 0, outputTokens: 0 } }) }; } function mockGitHub() { From 70567818aab6c50dc0f78d9dac5bce9ea2a1829c Mon Sep 17 00:00:00 2001 From: GodsBoy Date: Sat, 28 Mar 2026 00:26:05 +0200 Subject: [PATCH 2/4] feat(review): add file ignore patterns for code review Add ignore glob patterns to CodeReviewConfig so specific files (e.g. lock files, generated code, dist/) can be excluded from AI code review. Filtered files are also excluded from the diff sent to the AI provider and from context building. --- src/config.ts | 2 ++ src/review/reviewer.ts | 47 +++++++++++++++++++++--- src/review/types.ts | 1 + tests/reviewer.test.ts | 82 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+), 5 deletions(-) diff --git a/src/config.ts b/src/config.ts index 5ba0769..3067e1e 100644 --- a/src/config.ts +++ b/src/config.ts @@ -41,6 +41,7 @@ export interface RepoKeeperConfig { focus: string[]; maxContextFiles: number; minDiffLines: number; + ignore?: string[]; }; port: number; } @@ -71,6 +72,7 @@ const defaults: RepoKeeperConfig = { focus: ['security', 'performance', 'test-coverage', 'breaking-changes'], maxContextFiles: 5, minDiffLines: 10, + ignore: [], }, port: 3001, }; diff --git a/src/review/reviewer.ts b/src/review/reviewer.ts index 2dd7563..e7b5f9c 100644 --- a/src/review/reviewer.ts +++ b/src/review/reviewer.ts @@ -9,6 +9,35 @@ import { isAlreadyReviewed, markReviewed, parseDiffHunks, cleanupPR } from './hu import { getAcceptedPatterns, formatAcceptedPatternsPrompt, learnFromMergedPR } from './memory.js'; import type { PRReviewPayload, ReviewResult, ReviewFinding, CodeReviewConfig, EnrichedFile } from './types.js'; +export function matchesIgnorePattern(file: string, pattern: string): boolean { + const regexStr = pattern + .replace(/\./g, '\\.') + .replace(/\*\*\//g, '{{GLOBSTAR_SLASH}}') + .replace(/\*\*/g, '{{GLOBSTAR}}') + .replace(/\*/g, '[^/]*') + .replace(/\?/g, '[^/]') + .replace(/{{GLOBSTAR_SLASH}}/g, '(?:.+/)?') + .replace(/{{GLOBSTAR}}/g, '.*'); + return new RegExp(`^${regexStr}$`).test(file); +} + +export function filterIgnoredFiles(files: string[], ignorePatterns: string[]): string[] { + if (!ignorePatterns || ignorePatterns.length === 0) return files; + return files.filter((file) => !ignorePatterns.some((pattern) => matchesIgnorePattern(file, pattern))); +} + +function filterDiffByFiles(diff: string, allowedFiles: string[]): string { + const allowedSet = new Set(allowedFiles); + const sections = diff.split(/(?=^diff --git )/m); + return sections + .filter((section) => { + const match = section.match(/^diff --git a\/(.+?) b\//); + if (!match) return false; + return allowedSet.has(match[1]); + }) + .join(''); +} + const DEFAULT_REVIEW_CONFIG: CodeReviewConfig = { enabled: true, focus: ['security', 'performance', 'test-coverage', 'breaking-changes'], @@ -154,11 +183,18 @@ export async function handleCodeReview( // Get changed file names from hunks const changedFiles = [...new Set(hunks.map((h) => h.file))]; + // Filter out ignored files + const reviewableFiles = filterIgnoredFiles(changedFiles, reviewConfig.ignore ?? []); + if (reviewableFiles.length === 0) { + log('info', `PR #${prNumber}: all changed files match ignore patterns, skipping review`); + return; + } + // Build codebase context let enrichedFiles: EnrichedFile[] = []; try { enrichedFiles = await buildContext( - changedFiles, + reviewableFiles, repository.clone_url, owner, repo, @@ -174,11 +210,12 @@ export async function handleCodeReview( // Get accepted patterns from memory const acceptedPatterns = formatAcceptedPatternsPrompt(getAcceptedPatterns()); - // Truncate diff for AI + // Filter diff to only include reviewable files and truncate for AI + const filteredDiff = filterDiffByFiles(diff, reviewableFiles); const maxDiffChars = 30_000; - const truncatedDiff = diff.length > maxDiffChars - ? diff.slice(0, maxDiffChars) + '\n\n... (diff truncated)' - : diff; + const truncatedDiff = filteredDiff.length > maxDiffChars + ? filteredDiff.slice(0, maxDiffChars) + '\n\n... (diff truncated)' + : filteredDiff; // Build prompt and call AI const prompt = buildReviewPrompt(truncatedDiff, enrichedFiles, reviewConfig, acceptedPatterns); diff --git a/src/review/types.ts b/src/review/types.ts index d364c54..80f6522 100644 --- a/src/review/types.ts +++ b/src/review/types.ts @@ -36,6 +36,7 @@ export interface CodeReviewConfig { focus: string[]; maxContextFiles: number; minDiffLines: number; + ignore?: string[]; } export interface PRReviewPayload { diff --git a/tests/reviewer.test.ts b/tests/reviewer.test.ts index bbf1d3b..fa9d72c 100644 --- a/tests/reviewer.test.ts +++ b/tests/reviewer.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { parseImports } from '../src/review/context-builder.js'; import { parseDiffHunks, isAlreadyReviewed, markReviewed, getReviewedShas, cleanupPR } from '../src/review/hunk-tracker.js'; import { getAcceptedPatterns, addAcceptedPattern, learnFromMergedPR, formatAcceptedPatternsPrompt } from '../src/review/memory.js'; +import { matchesIgnorePattern, filterIgnoredFiles } from '../src/review/reviewer.js'; import { existsSync, unlinkSync, mkdirSync } from 'node:fs'; // --- context-builder tests --- @@ -270,6 +271,87 @@ describe('comment-poster', () => { }); }); +// --- review file filtering tests --- + +describe('matchesIgnorePattern', () => { + it('matches exact file paths', () => { + expect(matchesIgnorePattern('package-lock.json', 'package-lock.json')).toBe(true); + }); + + it('matches single wildcard patterns', () => { + expect(matchesIgnorePattern('src/utils.test.ts', 'src/*.test.ts')).toBe(true); + expect(matchesIgnorePattern('src/deep/utils.test.ts', 'src/*.test.ts')).toBe(false); + }); + + it('matches globstar patterns', () => { + expect(matchesIgnorePattern('src/deep/nested/file.test.ts', '**/*.test.ts')).toBe(true); + expect(matchesIgnorePattern('file.test.ts', '**/*.test.ts')).toBe(true); + }); + + it('matches directory globstar patterns', () => { + expect(matchesIgnorePattern('dist/index.js', 'dist/**')).toBe(true); + expect(matchesIgnorePattern('dist/sub/file.js', 'dist/**')).toBe(true); + expect(matchesIgnorePattern('src/dist/file.js', 'dist/**')).toBe(false); + }); + + it('matches single character wildcard', () => { + expect(matchesIgnorePattern('src/a.ts', 'src/?.ts')).toBe(true); + expect(matchesIgnorePattern('src/ab.ts', 'src/?.ts')).toBe(false); + }); + + it('does not match unrelated files', () => { + expect(matchesIgnorePattern('src/index.ts', '**/*.test.ts')).toBe(false); + expect(matchesIgnorePattern('README.md', 'dist/**')).toBe(false); + }); + + it('handles dots in patterns correctly', () => { + expect(matchesIgnorePattern('.env', '.env')).toBe(true); + expect(matchesIgnorePattern('xenv', '.env')).toBe(false); + }); +}); + +describe('filterIgnoredFiles', () => { + const files = [ + 'src/index.ts', + 'src/utils.test.ts', + 'dist/index.js', + 'package-lock.json', + 'docs/README.md', + ]; + + it('returns all files when no ignore patterns', () => { + expect(filterIgnoredFiles(files, [])).toEqual(files); + }); + + it('filters files matching a single pattern', () => { + const result = filterIgnoredFiles(files, ['dist/**']); + expect(result).not.toContain('dist/index.js'); + expect(result).toHaveLength(4); + }); + + it('filters files matching multiple patterns', () => { + const result = filterIgnoredFiles(files, ['dist/**', 'package-lock.json']); + expect(result).not.toContain('dist/index.js'); + expect(result).not.toContain('package-lock.json'); + expect(result).toHaveLength(3); + }); + + it('filters test files with globstar', () => { + const result = filterIgnoredFiles(files, ['**/*.test.ts']); + expect(result).not.toContain('src/utils.test.ts'); + expect(result).toContain('src/index.ts'); + }); + + it('returns all files when patterns match nothing', () => { + const result = filterIgnoredFiles(files, ['**/*.xyz']); + expect(result).toEqual(files); + }); + + it('handles undefined/empty ignore patterns gracefully', () => { + expect(filterIgnoredFiles(files, undefined as unknown as string[])).toEqual(files); + }); +}); + // --- integration-style test for the reviewer module --- describe('reviewer module', () => { From 4e678b0eed7fc9e8c1824677c9e57a811ab6c4ac Mon Sep 17 00:00:00 2001 From: GodsBoy Date: Sat, 28 Mar 2026 00:27:39 +0200 Subject: [PATCH 3/4] feat(review): add commit status checks as merge gate Add optional commitStatus flag to CodeReviewConfig. When enabled, the reviewer posts a GitHub commit status after each review: failure if any BLOCKING findings exist, success otherwise. This allows branch protection rules to require passing reviews. --- src/config.ts | 2 ++ src/github/client.ts | 17 +++++++++++ src/review/reviewer.ts | 12 ++++++++ src/review/types.ts | 1 + tests/reviewer.test.ts | 68 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 100 insertions(+) diff --git a/src/config.ts b/src/config.ts index 3067e1e..747ea1e 100644 --- a/src/config.ts +++ b/src/config.ts @@ -42,6 +42,7 @@ export interface RepoKeeperConfig { maxContextFiles: number; minDiffLines: number; ignore?: string[]; + commitStatus?: boolean; }; port: number; } @@ -73,6 +74,7 @@ const defaults: RepoKeeperConfig = { maxContextFiles: 5, minDiffLines: 10, ignore: [], + commitStatus: false, }, port: 3001, }; diff --git a/src/github/client.ts b/src/github/client.ts index 3fd5caa..2576213 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -125,6 +125,23 @@ export class GitHubClient { })); } + async createCommitStatus( + sha: string, + state: 'success' | 'failure' | 'pending', + description: string, + context: string = 'repokeeper/review', + ): Promise { + await this.octokit.repos.createCommitStatus({ + owner: this.owner, + repo: this.repo, + sha, + state, + description, + context, + }); + log('info', `Set commit status ${state} on ${sha.slice(0, 7)}: ${description}`); + } + private async ensureLabelsExist(labels: string[]): Promise { const existing = await this.octokit.paginate(this.octokit.issues.listLabelsForRepo, { owner: this.owner, diff --git a/src/review/reviewer.ts b/src/review/reviewer.ts index e7b5f9c..d65115a 100644 --- a/src/review/reviewer.ts +++ b/src/review/reviewer.ts @@ -226,6 +226,18 @@ export async function handleCodeReview( const octokit = new Octokit({ auth: config.github.token }); await postReview(octokit, owner, repo, prNumber, headSha, result); + // Post commit status if enabled + if (reviewConfig.commitStatus) { + const hasBlocking = result.findings.some((f) => f.severity === 'BLOCKING'); + const statusState = hasBlocking ? 'failure' : 'success'; + const description = hasBlocking + ? `${result.findings.filter((f) => f.severity === 'BLOCKING').length} blocking issue(s) found` + : result.findings.length > 0 + ? `${result.findings.length} non-blocking finding(s)` + : 'No issues found'; + await github.createCommitStatus(headSha, statusState, description); + } + // Mark this SHA as reviewed markReviewed(owner, repo, prNumber, headSha); diff --git a/src/review/types.ts b/src/review/types.ts index 80f6522..433649c 100644 --- a/src/review/types.ts +++ b/src/review/types.ts @@ -37,6 +37,7 @@ export interface CodeReviewConfig { maxContextFiles: number; minDiffLines: number; ignore?: string[]; + commitStatus?: boolean; } export interface PRReviewPayload { diff --git a/tests/reviewer.test.ts b/tests/reviewer.test.ts index fa9d72c..7d9cf0f 100644 --- a/tests/reviewer.test.ts +++ b/tests/reviewer.test.ts @@ -352,6 +352,74 @@ describe('filterIgnoredFiles', () => { }); }); +// --- commit status tests --- + +describe('commit status logic', () => { + // Test the commit status decision logic directly by examining + // the conditions that determine success vs failure + + it('identifies BLOCKING findings correctly', () => { + const findings = [ + { file: 'a.ts', line: 1, severity: 'BLOCKING' as const, message: 'Security issue' }, + { file: 'b.ts', line: 2, severity: 'WARNING' as const, message: 'Code smell' }, + ]; + const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); + expect(hasBlocking).toBe(true); + + const blockingCount = findings.filter((f) => f.severity === 'BLOCKING').length; + expect(blockingCount).toBe(1); + }); + + it('identifies no BLOCKING findings as success', () => { + const findings = [ + { file: 'a.ts', line: 1, severity: 'WARNING' as const, message: 'Code smell' }, + { file: 'b.ts', line: 2, severity: 'SUGGESTION' as const, message: 'Style' }, + ]; + const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); + expect(hasBlocking).toBe(false); + }); + + it('generates correct description for blocking findings', () => { + const findings = [ + { file: 'a.ts', line: 1, severity: 'BLOCKING' as const, message: 'SQL injection' }, + { file: 'b.ts', line: 2, severity: 'BLOCKING' as const, message: 'Hardcoded secret' }, + { file: 'c.ts', line: 3, severity: 'WARNING' as const, message: 'Missing error handling' }, + ]; + const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); + const description = hasBlocking + ? `${findings.filter((f) => f.severity === 'BLOCKING').length} blocking issue(s) found` + : findings.length > 0 + ? `${findings.length} non-blocking finding(s)` + : 'No issues found'; + expect(description).toBe('2 blocking issue(s) found'); + }); + + it('generates correct description for non-blocking findings only', () => { + const findings = [ + { file: 'a.ts', line: 1, severity: 'WARNING' as const, message: 'Code smell' }, + { file: 'b.ts', line: 2, severity: 'SUGGESTION' as const, message: 'Style' }, + ]; + const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); + const description = hasBlocking + ? `${findings.filter((f) => f.severity === 'BLOCKING').length} blocking issue(s) found` + : findings.length > 0 + ? `${findings.length} non-blocking finding(s)` + : 'No issues found'; + expect(description).toBe('2 non-blocking finding(s)'); + }); + + it('generates correct description when no findings', () => { + const findings: Array<{ severity: string }> = []; + const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); + const description = hasBlocking + ? `${findings.filter((f) => f.severity === 'BLOCKING').length} blocking issue(s) found` + : findings.length > 0 + ? `${findings.length} non-blocking finding(s)` + : 'No issues found'; + expect(description).toBe('No issues found'); + }); +}); + // --- integration-style test for the reviewer module --- describe('reviewer module', () => { From 5c0250bf499e2107169294d31f26730bebafd4ab Mon Sep 17 00:00:00 2001 From: GodsBoy Date: Sat, 28 Mar 2026 00:34:57 +0200 Subject: [PATCH 4/4] fix(review): escape regex metacharacters in ignore patterns and extract buildCommitStatus - matchesIgnorePattern now escapes [](){}+^$|\ before converting glob to regex - Extract buildCommitStatus() as testable function instead of inline logic - Tests call the real function instead of duplicating implementation --- src/review/reviewer.ts | 25 ++++++---- tests/reviewer.test.ts | 105 ++++++++++++++++------------------------- 2 files changed, 57 insertions(+), 73 deletions(-) diff --git a/src/review/reviewer.ts b/src/review/reviewer.ts index d65115a..8dc1b6c 100644 --- a/src/review/reviewer.ts +++ b/src/review/reviewer.ts @@ -10,8 +10,9 @@ import { getAcceptedPatterns, formatAcceptedPatternsPrompt, learnFromMergedPR } import type { PRReviewPayload, ReviewResult, ReviewFinding, CodeReviewConfig, EnrichedFile } from './types.js'; export function matchesIgnorePattern(file: string, pattern: string): boolean { + // Escape regex metacharacters first (except glob chars * ? which we handle separately) const regexStr = pattern - .replace(/\./g, '\\.') + .replace(/[.+^${}()|[\]\\]/g, '\\$&') .replace(/\*\*\//g, '{{GLOBSTAR_SLASH}}') .replace(/\*\*/g, '{{GLOBSTAR}}') .replace(/\*/g, '[^/]*') @@ -38,6 +39,18 @@ function filterDiffByFiles(diff: string, allowedFiles: string[]): string { .join(''); } +export function buildCommitStatus(findings: ReviewFinding[]): { state: 'success' | 'failure'; description: string } { + const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); + if (hasBlocking) { + const count = findings.filter((f) => f.severity === 'BLOCKING').length; + return { state: 'failure', description: `${count} blocking issue(s) found` }; + } + if (findings.length > 0) { + return { state: 'success', description: `${findings.length} non-blocking finding(s)` }; + } + return { state: 'success', description: 'No issues found' }; +} + const DEFAULT_REVIEW_CONFIG: CodeReviewConfig = { enabled: true, focus: ['security', 'performance', 'test-coverage', 'breaking-changes'], @@ -228,14 +241,8 @@ export async function handleCodeReview( // Post commit status if enabled if (reviewConfig.commitStatus) { - const hasBlocking = result.findings.some((f) => f.severity === 'BLOCKING'); - const statusState = hasBlocking ? 'failure' : 'success'; - const description = hasBlocking - ? `${result.findings.filter((f) => f.severity === 'BLOCKING').length} blocking issue(s) found` - : result.findings.length > 0 - ? `${result.findings.length} non-blocking finding(s)` - : 'No issues found'; - await github.createCommitStatus(headSha, statusState, description); + const { state, description } = buildCommitStatus(result.findings); + await github.createCommitStatus(headSha, state, description); } // Mark this SHA as reviewed diff --git a/tests/reviewer.test.ts b/tests/reviewer.test.ts index 7d9cf0f..aeb3f82 100644 --- a/tests/reviewer.test.ts +++ b/tests/reviewer.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { parseImports } from '../src/review/context-builder.js'; import { parseDiffHunks, isAlreadyReviewed, markReviewed, getReviewedShas, cleanupPR } from '../src/review/hunk-tracker.js'; import { getAcceptedPatterns, addAcceptedPattern, learnFromMergedPR, formatAcceptedPatternsPrompt } from '../src/review/memory.js'; -import { matchesIgnorePattern, filterIgnoredFiles } from '../src/review/reviewer.js'; +import { matchesIgnorePattern, filterIgnoredFiles, buildCommitStatus } from '../src/review/reviewer.js'; import { existsSync, unlinkSync, mkdirSync } from 'node:fs'; // --- context-builder tests --- @@ -308,6 +308,13 @@ describe('matchesIgnorePattern', () => { expect(matchesIgnorePattern('.env', '.env')).toBe(true); expect(matchesIgnorePattern('xenv', '.env')).toBe(false); }); + + it('escapes regex metacharacters in patterns', () => { + expect(matchesIgnorePattern('src/file[1].ts', 'src/file[1].ts')).toBe(true); + expect(matchesIgnorePattern('src/file(test).ts', 'src/file(test).ts')).toBe(true); + expect(matchesIgnorePattern('src/file+extra.ts', 'src/file+extra.ts')).toBe(true); + expect(matchesIgnorePattern('src/fileX.ts', 'src/file[1].ts')).toBe(false); + }); }); describe('filterIgnoredFiles', () => { @@ -354,69 +361,39 @@ describe('filterIgnoredFiles', () => { // --- commit status tests --- -describe('commit status logic', () => { - // Test the commit status decision logic directly by examining - // the conditions that determine success vs failure - - it('identifies BLOCKING findings correctly', () => { - const findings = [ - { file: 'a.ts', line: 1, severity: 'BLOCKING' as const, message: 'Security issue' }, - { file: 'b.ts', line: 2, severity: 'WARNING' as const, message: 'Code smell' }, - ]; - const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); - expect(hasBlocking).toBe(true); - - const blockingCount = findings.filter((f) => f.severity === 'BLOCKING').length; - expect(blockingCount).toBe(1); - }); - - it('identifies no BLOCKING findings as success', () => { - const findings = [ - { file: 'a.ts', line: 1, severity: 'WARNING' as const, message: 'Code smell' }, - { file: 'b.ts', line: 2, severity: 'SUGGESTION' as const, message: 'Style' }, - ]; - const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); - expect(hasBlocking).toBe(false); - }); - - it('generates correct description for blocking findings', () => { - const findings = [ - { file: 'a.ts', line: 1, severity: 'BLOCKING' as const, message: 'SQL injection' }, - { file: 'b.ts', line: 2, severity: 'BLOCKING' as const, message: 'Hardcoded secret' }, - { file: 'c.ts', line: 3, severity: 'WARNING' as const, message: 'Missing error handling' }, - ]; - const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); - const description = hasBlocking - ? `${findings.filter((f) => f.severity === 'BLOCKING').length} blocking issue(s) found` - : findings.length > 0 - ? `${findings.length} non-blocking finding(s)` - : 'No issues found'; - expect(description).toBe('2 blocking issue(s) found'); - }); - - it('generates correct description for non-blocking findings only', () => { - const findings = [ - { file: 'a.ts', line: 1, severity: 'WARNING' as const, message: 'Code smell' }, - { file: 'b.ts', line: 2, severity: 'SUGGESTION' as const, message: 'Style' }, - ]; - const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); - const description = hasBlocking - ? `${findings.filter((f) => f.severity === 'BLOCKING').length} blocking issue(s) found` - : findings.length > 0 - ? `${findings.length} non-blocking finding(s)` - : 'No issues found'; - expect(description).toBe('2 non-blocking finding(s)'); - }); - - it('generates correct description when no findings', () => { - const findings: Array<{ severity: string }> = []; - const hasBlocking = findings.some((f) => f.severity === 'BLOCKING'); - const description = hasBlocking - ? `${findings.filter((f) => f.severity === 'BLOCKING').length} blocking issue(s) found` - : findings.length > 0 - ? `${findings.length} non-blocking finding(s)` - : 'No issues found'; - expect(description).toBe('No issues found'); +describe('buildCommitStatus', () => { + it('returns failure when BLOCKING findings exist', () => { + const result = buildCommitStatus([ + { file: 'a.ts', line: 1, severity: 'BLOCKING', message: 'Security issue' }, + { file: 'b.ts', line: 2, severity: 'WARNING', message: 'Code smell' }, + ]); + expect(result.state).toBe('failure'); + expect(result.description).toBe('1 blocking issue(s) found'); + }); + + it('counts multiple BLOCKING findings', () => { + const result = buildCommitStatus([ + { file: 'a.ts', line: 1, severity: 'BLOCKING', message: 'SQL injection' }, + { file: 'b.ts', line: 2, severity: 'BLOCKING', message: 'Hardcoded secret' }, + { file: 'c.ts', line: 3, severity: 'WARNING', message: 'Missing error handling' }, + ]); + expect(result.state).toBe('failure'); + expect(result.description).toBe('2 blocking issue(s) found'); + }); + + it('returns success with count for non-blocking findings only', () => { + const result = buildCommitStatus([ + { file: 'a.ts', line: 1, severity: 'WARNING', message: 'Code smell' }, + { file: 'b.ts', line: 2, severity: 'SUGGESTION', message: 'Style' }, + ]); + expect(result.state).toBe('success'); + expect(result.description).toBe('2 non-blocking finding(s)'); + }); + + it('returns success with clean message when no findings', () => { + const result = buildCommitStatus([]); + expect(result.state).toBe('success'); + expect(result.description).toBe('No issues found'); }); });