diff --git a/src/agents/code-reviewer.ts b/src/agents/code-reviewer.ts index 45c85c6..dce4453 100644 --- a/src/agents/code-reviewer.ts +++ b/src/agents/code-reviewer.ts @@ -1,5 +1,52 @@ +import * as fs from "node:fs"; +import * as path from "node:path"; import { BaseAgent, AgentContext, AgentResult } from "./base.js"; +interface ReviewFinding { + persona: "security" | "performance" | "maintainability"; + severity: "P0" | "P1" | "P2" | "P3"; + category: string; + file: string; + message: string; +} + +const SECURITY_PATTERNS: Array<{ + pattern: RegExp; + severity: "P0" | "P1"; + category: string; + message: string; +}> = [ + { pattern: /(?:exec|execSync|spawn|spawnSync)\s*\(\s*[^'"]*[\$`]/g, severity: "P0", category: "command_injection", message: "Command execution with dynamic input" }, + { pattern: /eval\s*\(\s*[^'"]*\$\{/g, severity: "P0", category: "code_injection", message: "eval() with dynamic content" }, + { pattern: /(?:password|secret|api[_-]?key|token)\s*[:=]\s*['"][^'"]{3,}['"]/gi, severity: "P0", category: "credential_exposure", message: "Hardcoded credential in source" }, + { pattern: /catch\s*\(\w*\)\s*\{\s*\}/g, severity: "P0", category: "swallowed_errors", message: "Empty catch block" }, + { pattern: /(?:__proto__|constructor\s*\[|prototype\s*\[)/g, severity: "P0", category: "prototype_pollution", message: "Prototype chain manipulation" }, + { pattern: /(?:md5|sha1|des|rc4)\s*\(/gi, severity: "P1", category: "weak_crypto", message: "Weak cryptographic algorithm" }, +]; + +const PERFORMANCE_PATTERNS: Array<{ + pattern: RegExp; + severity: "P1" | "P2"; + category: string; + message: string; +}> = [ + { pattern: /(?:execSync|spawnSync)\s*\(\s*['"]/g, severity: "P1", category: "sync_exec", message: "Synchronous process spawn" }, + { pattern: /setTimeout\s*\((?![^)]*clearTimeout)/g, severity: "P2", category: "timer_leak", message: "setTimeout without clearTimeout" }, + { pattern: /express\.json\s*\(\s*\)/g, severity: "P1", category: "no_body_limit", message: "JSON body parser without size limit" }, +]; + +const MAINTAINABILITY_PATTERNS: Array<{ + pattern: RegExp; + severity: "P1" | "P2" | "P3"; + category: string; + message: string; +}> = [ + { pattern: /(?:as\s+any\b|:\s*any\b||any\[\s*\])/g, severity: "P1", category: "type_safety", message: "Use of 'any' type" }, + { pattern: /\bvar\s+/g, severity: "P1", category: "modern_js", message: "Use of 'var'" }, + { pattern: /\b(?:TODO|FIXME|HACK|XXX)\b/g, severity: "P2", category: "tech_debt", message: "Technical debt marker" }, + { pattern: /console\.(log|warn|error)\s*\(/g, severity: "P2", category: "logging", message: "Direct console.log usage" }, +]; + export class CodeReviewerAgent extends BaseAgent { readonly name = "code-reviewer"; readonly description = "Multi-persona code review. Auto-applies P0 fixes. Flags P1+ for post-hoc review."; @@ -8,6 +55,7 @@ export class CodeReviewerAgent extends BaseAgent { async execute(context: AgentContext): Promise { const start = Date.now(); this.log("Running code review..."); + if (context.backend) { const result = await this.executeViaBackend( context, @@ -15,14 +63,83 @@ export class CodeReviewerAgent extends BaseAgent { ); return { ...result, duration_ms: Date.now() - start }; } + + const findings = this.mechanicalReview(context.project_path); + const p0Count = findings.filter((f) => f.severity === "P0").length; + const output = this.formatFindings(findings); + return { - success: false, - output: "Code review requires an intelligence backend. Configure one with: ci init --backend", + success: p0Count === 0, + output, artifacts_created: [], decisions: 0, - escalations: 0, + escalations: p0Count, duration_ms: Date.now() - start, - error: "No intelligence backend available", + error: p0Count > 0 ? `${p0Count} P0 finding(s) require immediate attention` : undefined, }; } + + mechanicalReview(projectPath: string): ReviewFinding[] { + const findings: ReviewFinding[] = []; + const srcDir = path.join(projectPath, "src"); + + if (!fs.existsSync(srcDir)) return findings; + + const allPatterns: Array<{ + patterns: typeof SECURITY_PATTERNS; + persona: ReviewFinding["persona"]; + }> = [ + { patterns: SECURITY_PATTERNS as unknown as typeof SECURITY_PATTERNS, persona: "security" }, + { patterns: PERFORMANCE_PATTERNS as unknown as typeof SECURITY_PATTERNS, persona: "performance" }, + { patterns: MAINTAINABILITY_PATTERNS as unknown as typeof SECURITY_PATTERNS, persona: "maintainability" }, + ]; + + this.scanDirectory(srcDir, projectPath, allPatterns, findings); + return findings; + } + + private scanDirectory( + dir: string, + projectPath: string, + personaPatterns: Array<{ patterns: Array<{ pattern: RegExp; severity: "P0" | "P1" | "P2" | "P3"; category: string; message: string }>; persona: ReviewFinding["persona"] }>, + findings: ReviewFinding[] + ): void { + const entries = fs.readdirSync(dir, { withFileTypes: true }); + for (const entry of entries) { + const fullPath = path.join(dir, entry.name); + if (entry.isDirectory() && entry.name !== "node_modules" && entry.name !== ".git") { + this.scanDirectory(fullPath, projectPath, personaPatterns, findings); + } else if ( + entry.isFile() && + entry.name.endsWith(".ts") && + !entry.name.endsWith(".test.ts") && + !entry.name.endsWith(".d.ts") + ) { + const content = fs.readFileSync(fullPath, "utf-8"); + for (const { patterns, persona } of personaPatterns) { + for (const { pattern, severity, category, message } of patterns) { + pattern.lastIndex = 0; + if (pattern.test(content)) { + findings.push({ + persona, + severity: severity as ReviewFinding["severity"], + category, + file: path.relative(projectPath, fullPath), + message, + }); + } + } + } + } + } + } + + private formatFindings(findings: ReviewFinding[]): string { + if (findings.length === 0) return "No findings — code review passed."; + const lines: string[] = ["Code Review Findings:", ""]; + for (const f of findings) { + lines.push(`[${f.persona}|${f.severity}] ${f.category}: ${f.message} (${f.file})`); + } + return lines.join("\n"); + } } \ No newline at end of file diff --git a/src/verification/quality.ts b/src/verification/quality.ts index a43e917..a91309d 100644 --- a/src/verification/quality.ts +++ b/src/verification/quality.ts @@ -6,22 +6,141 @@ import { VerificationLayer, VerificationResult, VerificationCheck } from "./type interface CodeFinding { severity: "P0" | "P1" | "P2" | "P3"; category: string; + persona: "security" | "performance" | "maintainability"; message: string; file?: string; } -const CODE_QUALITY_PATTERNS: Array<{ +const SECURITY_REVIEW_PATTERNS: Array<{ pattern: RegExp; - severity: "P0" | "P1" | "P2" | "P3"; + severity: "P0" | "P1" | "P2"; category: string; message: string; }> = [ + { + pattern: /(?:exec|execSync|spawn|spawnSync)\s*\(\s*[^'"]*[\$`]/g, + severity: "P0", + category: "command_injection", + message: "Command execution with dynamic input — injection risk", + }, + { + pattern: /eval\s*\(\s*[^'"]*\$\{/g, + severity: "P0", + category: "code_injection", + message: "eval() with dynamic content — code injection risk", + }, + { + pattern: /(?:innerHTML|outerHTML|insertAdjacentHTML)\s*=/g, + severity: "P0", + category: "xss", + message: "Unsanitized HTML assignment — XSS risk", + }, + { + pattern: /(?:password|secret|api[_-]?key|token)\s*[:=]\s*['"][^'"]{3,}['"]/gi, + severity: "P0", + category: "credential_exposure", + message: "Hardcoded credential in source", + }, + { + pattern: /(?:__proto__|constructor\s*\[|prototype\s*\[)/g, + severity: "P0", + category: "prototype_pollution", + message: "Prototype chain manipulation — privilege escalation risk", + }, + { + pattern: /jwt\.decode\s*\(/g, + severity: "P0", + category: "auth_bypass", + message: "JWT decoded without verification — authentication bypass", + }, + { + pattern: /(?:md5|sha1|des|rc4)\s*\(/gi, + severity: "P1", + category: "weak_crypto", + message: "Weak cryptographic algorithm", + }, + { + pattern: /JSON\.parse\s*\(\s*(?:req|ctx|input|data|body|params)\.\w+/g, + severity: "P1", + category: "unsafe_deserialization", + message: "Unsafe deserialization of untrusted input", + }, { pattern: /catch\s*\(\w*\)\s*\{\s*\}/g, severity: "P0", - category: "error_handling", + category: "swallowed_errors", message: "Empty catch block — errors silently swallowed", }, +]; + +const PERFORMANCE_REVIEW_PATTERNS: Array<{ + pattern: RegExp; + severity: "P1" | "P2"; + category: string; + message: string; +}> = [ + { + pattern: /await\s+.*(?:readFileSync|writeFileSync|execSync)/g, + severity: "P1", + category: "blocking_io", + message: "Synchronous I/O in async context — blocks event loop", + }, + { + pattern: /(?:execSync|spawnSync)\s*\(\s*['"]/g, + severity: "P1", + category: "sync_exec", + message: "Synchronous process spawn — blocks event loop", + }, + { + pattern: /setTimeout\s*\((?![^)]*clearTimeout)/g, + severity: "P2", + category: "timer_leak", + message: "setTimeout without clearTimeout — potential timer leak", + }, + { + pattern: /\.(?:on|addEventListener)\s*\(['"]\w+['"]/g, + severity: "P2", + category: "listener_leak", + message: "Event listener registration — verify corresponding .off() exists", + }, + { + pattern: /\.map\s*\(\s*(?:async\s+)?\([^)]*\)\s*=>\s*(?!.*(?:filter|slice|take|limit))/g, + severity: "P2", + category: "unbounded_iteration", + message: "Full array traversal without pagination or limit", + }, + { + pattern: /express\.json\s*\(\s*\)/g, + severity: "P1", + category: "no_body_limit", + message: "JSON body parser without size limit — DoS risk", + }, +]; + +const MAINTAINABILITY_REVIEW_PATTERNS: Array<{ + pattern: RegExp; + severity: "P1" | "P2" | "P3"; + category: string; + message: string; +}> = [ + { + pattern: /(?:as\s+any\b|:\s*any\b||any\[\s*\])/g, + severity: "P1", + category: "type_safety", + message: "Use of 'any' type — loses type safety", + }, + { + pattern: /\bvar\s+/g, + severity: "P1", + category: "modern_js", + message: "Use of 'var' — prefer 'const' or 'let'", + }, + { + pattern: /\b(?:TODO|FIXME|HACK|XXX)\b/g, + severity: "P2", + category: "tech_debt", + message: "Technical debt marker found", + }, { pattern: /console\.(log|warn|error)\s*\(/g, severity: "P2", @@ -29,22 +148,10 @@ const CODE_QUALITY_PATTERNS: Array<{ message: "Direct console.log usage — consider structured logging", }, { - pattern: /(?:as\s+any\b|:\s*any\b||any\[\s*\])/g, + pattern: /(?:return|throw)\s+[^;]+;\s*\n\s*(?:return|throw|const|let|var|function)/g, severity: "P1", - category: "type_safety", - message: "Use of 'any' type — loses type safety", - }, - { - pattern: /TODO|FIXME|HACK|XXX/g, - severity: "P2", - category: "tech_debt", - message: "Technical debt marker found", - }, - { - pattern: /\bvar\s+/g, - severity: "P1", - category: "modern_js", - message: "Use of 'var' — prefer 'const' or 'let'", + category: "dead_code", + message: "Code after return/throw — unreachable dead code", }, ]; @@ -56,20 +163,26 @@ export class QualityVerification extends VerificationLayer { const start = Date.now(); const checks: VerificationCheck[] = []; - const findings = this.scanForFindings(projectPath); + const securityFindings = this.scanWithPersona(projectPath, SECURITY_REVIEW_PATTERNS, "security"); + const perfFindings = this.scanWithPersona(projectPath, PERFORMANCE_REVIEW_PATTERNS, "performance"); + const maintFindings = this.scanWithPersona(projectPath, MAINTAINABILITY_REVIEW_PATTERNS, "maintainability"); + const allFindings = [...securityFindings, ...perfFindings, ...maintFindings]; - const p0Findings = findings.filter((f) => f.severity === "P0"); - const p1Findings = findings.filter((f) => f.severity === "P1"); - const p2p3Findings = findings.filter((f) => f.severity === "P2" || f.severity === "P3"); + const p0Findings = allFindings.filter((f) => f.severity === "P0"); + const p1Findings = allFindings.filter((f) => f.severity === "P1"); + const p2p3Findings = allFindings.filter((f) => f.severity === "P2" || f.severity === "P3"); checks.push(this.checkP0Findings(p0Findings)); checks.push(this.checkP1Findings(p1Findings)); checks.push(this.checkP2P3Findings(p2p3Findings)); + checks.push(this.checkSecurityReview(securityFindings)); + checks.push(this.checkPerformanceReview(perfFindings)); + checks.push(this.checkMaintainabilityReview(maintFindings)); checks.push(this.checkTypeScriptStrictness(projectPath)); checks.push(this.checkConsistentNaming(projectPath)); checks.push(this.checkTypeScriptCompilation(projectPath)); - const hasP0Fail = p0Findings.length > 3; + const hasP0Fail = p0Findings.length > 0; const passed = !hasP0Fail; return { @@ -77,12 +190,16 @@ export class QualityVerification extends VerificationLayer { name: this.name, passed, checks, - summary: `${findings.length} findings (P0: ${p0Findings.length}, P1: ${p1Findings.length}, P2/P3: ${p2p3Findings.length})`, + summary: `${allFindings.length} findings across 3 personas (P0: ${p0Findings.length}, P1: ${p1Findings.length}, P2/P3: ${p2p3Findings.length})`, duration_ms: Date.now() - start, }; } - private scanForFindings(projectPath: string): CodeFinding[] { + private scanWithPersona( + projectPath: string, + patterns: Array<{ pattern: RegExp; severity: "P0" | "P1" | "P2" | "P3"; category: string; message: string }>, + persona: CodeFinding["persona"] + ): CodeFinding[] { const findings: CodeFinding[] = []; const srcDir = path.join(projectPath, "src"); @@ -90,16 +207,22 @@ export class QualityVerification extends VerificationLayer { return findings; } - this.scanDirectory(srcDir, projectPath, findings); + this.scanDirectory(srcDir, projectPath, patterns, persona, findings); return findings; } - private scanDirectory(dir: string, projectPath: string, findings: CodeFinding[]): void { + private scanDirectory( + dir: string, + projectPath: string, + patterns: Array<{ pattern: RegExp; severity: "P0" | "P1" | "P2" | "P3"; category: string; message: string }>, + persona: CodeFinding["persona"], + findings: CodeFinding[] + ): void { const entries = fs.readdirSync(dir, { withFileTypes: true }); for (const entry of entries) { const fullPath = path.join(dir, entry.name); if (entry.isDirectory() && entry.name !== "node_modules") { - this.scanDirectory(fullPath, projectPath, findings); + this.scanDirectory(fullPath, projectPath, patterns, persona, findings); } else if ( entry.isFile() && entry.name.endsWith(".ts") && @@ -107,13 +230,13 @@ export class QualityVerification extends VerificationLayer { !entry.name.endsWith(".d.ts") ) { const content = fs.readFileSync(fullPath, "utf-8"); - for (const { pattern, severity, category, message } of CODE_QUALITY_PATTERNS) { + for (const { pattern, severity, category, message } of patterns) { pattern.lastIndex = 0; - const matches = pattern.test(content); - if (matches) { + if (pattern.test(content)) { findings.push({ severity, category, + persona, message: `${message} (${path.relative(projectPath, fullPath)})`, file: path.relative(projectPath, fullPath), }); @@ -133,9 +256,9 @@ export class QualityVerification extends VerificationLayer { } return this.check( "P0 findings (auto-fix)", - p0Findings.length > 3 ? "fail" : "warning", - `${p0Findings.length} P0 finding(s) — should be auto-fixed`, - p0Findings.map((f) => `[${f.category}] ${f.message}`).join("\n") + "fail", + `${p0Findings.length} P0 finding(s) — must be fixed`, + p0Findings.map((f) => `[${f.persona}|${f.category}] ${f.message}`).join("\n") ); } @@ -149,9 +272,9 @@ export class QualityVerification extends VerificationLayer { } return this.check( "P1 findings (review)", - "pass", + "warning", `${p1Findings.length} P1 finding(s) flagged for post-hoc review`, - p1Findings.map((f) => `[${f.category}] ${f.message}`).join("\n") + p1Findings.map((f) => `[${f.persona}|${f.category}] ${f.message}`).join("\n") ); } @@ -167,6 +290,43 @@ export class QualityVerification extends VerificationLayer { "P2/P3 findings (informational)", "pass", `${findings.length} informational finding(s)`, + findings.map((f) => `[${f.persona}|${f.category}] ${f.message}`).join("\n") + ); + } + + private checkSecurityReview(findings: CodeFinding[]): VerificationCheck { + if (findings.length === 0) { + return this.check("Security persona review", "pass", "No security review findings"); + } + const p0 = findings.filter((f) => f.severity === "P0").length; + return this.check( + "Security persona review", + p0 > 0 ? "fail" : "warning", + `${findings.length} finding(s) from security reviewer (P0: ${p0})`, + findings.map((f) => `[${f.category}] ${f.message}`).join("\n") + ); + } + + private checkPerformanceReview(findings: CodeFinding[]): VerificationCheck { + if (findings.length === 0) { + return this.check("Performance persona review", "pass", "No performance review findings"); + } + return this.check( + "Performance persona review", + "warning", + `${findings.length} finding(s) from performance reviewer`, + findings.map((f) => `[${f.category}] ${f.message}`).join("\n") + ); + } + + private checkMaintainabilityReview(findings: CodeFinding[]): VerificationCheck { + if (findings.length === 0) { + return this.check("Maintainability persona review", "pass", "No maintainability review findings"); + } + return this.check( + "Maintainability persona review", + "pass", + `${findings.length} finding(s) from maintainability reviewer`, findings.map((f) => `[${f.category}] ${f.message}`).join("\n") ); }