feat(P04): 3-persona code review, fix L4 pass/fail, flesh CodeReviewerAgent

---ci---
project: ci
phase: 4
milestone: v0.8
status: complete
decisions:
  - id: D-031
    decision: 3-persona quality review: security, performance, maintainability
    rationale: Each persona detects different class of issues; aggregate gives complete picture
    confidence: 0.82
  - id: D-032
    decision: L4 P0>0 = fail (not P0>3); P1 = warning (not pass)
    rationale: Any P0 finding is critical; P1 findings should never pass silently
    confidence: 0.95
requirements:
  covered: [QUAL-01, QUAL-02, QUAL-03, QUAL-04, QUAL-05]
---/ci---

QUAL-01: Added 3-persona review with distinct pattern sets: SecurityReviewer
(injection, auth, crypto), PerformanceReviewer (sync I/O, timer leaks,
DoS), MaintainabilityReviewer (type safety, dead code, tech debt).

QUAL-02: CodeReviewerAgent fleshed with mechanical 3-persona review. Works
without backend by running regex-based scan across all personas.

QUAL-03: L4 passed=false when ANY P0 finding exists (was >3). P1 findings
now return status='warning' (was always 'pass').

QUAL-04: TypeScript strict mode check remains in quality layer.

QUAL-05: CodeReviewerAgent.mechanicalReview() provides regex-based review
as fallback when no backend is available.

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>|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<AgentResult> {
     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,
+      success: p0Count === 0,
-      output: "Code review requires an intelligence backend. Configure one with: ci init --backend",
+      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");
+  }
 }

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>|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>|any\[\s*\])/g,
+    pattern: /(?:return|throw)\s+[^;]+;\s*\n\s*(?:return|throw|const|let|var|function)/g,
     severity: "P1",
-    category: "type_safety",
+    category: "dead_code",
-    message: "Use of 'any' type — loses type safety",
+    message: "Code after return/throw — unreachable dead code",
-  },
-  {
-    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'",
   },
 ];
 
@@ -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 p0Findings = allFindings.filter((f) => f.severity === "P0");
-    const p1Findings = findings.filter((f) => f.severity === "P1");
+    const p1Findings = allFindings.filter((f) => f.severity === "P1");
-    const p2p3Findings = findings.filter((f) => f.severity === "P2" || f.severity === "P3");
+    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 (pattern.test(content)) {
-          if (matches) {
             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",
+      "fail",
-      `${p0Findings.length} P0 finding(s) — should be auto-fixed`,
+      `${p0Findings.length} P0 finding(s) — must be fixed`,
-      p0Findings.map((f) => `[${f.category}] ${f.message}`).join("\n")
+      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")
     );
   }