From e211154dc7f6aa5483b0d7172a07d804530edbf9 Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Thu, 21 May 2026 03:13:19 -0700 Subject: [PATCH 1/3] Add reusable model-backed code review tool --- .github/workflows/ci.yml | 6 + .gitignore | 1 + README.md | 53 +++ bin/review-code.mjs | 169 ++++++++ eslint.config.js | 19 + lib/code-review.mjs | 816 ++++++++++++++++++++++++++++++++++++++ package-lock.json | 6 +- package.json | 7 +- test/code-review.test.mjs | 214 ++++++++++ 9 files changed, 1286 insertions(+), 5 deletions(-) create mode 100755 bin/review-code.mjs create mode 100644 eslint.config.js create mode 100644 lib/code-review.mjs create mode 100644 test/code-review.test.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4893934..1276e45 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,11 +31,14 @@ jobs: run: | node --check bin/rewrite-release-notes.mjs node --check bin/build-updater-manifest.mjs + node --check bin/review-code.mjs + node --check lib/code-review.mjs - name: Help works run: | node bin/rewrite-release-notes.mjs --help node bin/build-updater-manifest.mjs --help + node bin/review-code.mjs --help # Smoke: dry-run renders without an LLM call. Uses the repo's own tags # if any exist; otherwise creates synthetic ones so the script has @@ -83,3 +86,6 @@ jobs: } console.log("manifest smoke ok"); ' + + - name: Test reusable review tool + run: npm test diff --git a/.gitignore b/.gitignore index f4f426b..91d7640 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,7 @@ node_modules/ .DS_Store .env .env.local +.release-tools-review/ coverage/ dist/ *.tgz diff --git a/README.md b/README.md index 596d8de..778955b 100644 --- a/README.md +++ b/README.md @@ -185,12 +185,65 @@ and writes a manifest in the exact shape Tauri's updater expects. Exits non-zero if any binary is missing its signature, so the `publish` job fails loudly when signing didn't run. +## Model-backed code review + +`review-code` is a reusable, Clawpatch-inspired review harness for protoLabs +repos. It maps a repository into bounded feature records, asks an +OpenAI-compatible gateway for strict JSON findings, persists those findings +locally, and emits a Markdown report. It does not edit code. + +```bash +# In the repo you want reviewed: +npx -p @protolabsai/release-tools review-code init +npx -p @protolabsai/release-tools review-code map +GATEWAY_API_KEY=... OPENAI_BASE_URL=https://api.proto-labs.ai/v1 \ + npx -p @protolabsai/release-tools review-code run --model protolabs/fast --limit 1 +npx -p @protolabsai/release-tools review-code report +``` + +Review state is written to `.release-tools-review/`. Add that directory to a +repo's `.gitignore` if you use the tool regularly. The default model is +`protolabs/smart`; override it with `--model` or `CODE_REVIEW_MODEL` when a +gateway key is routed to another review model such as Minimax. + +For better feature boundaries, add a repo-local `review-code.config.json`: + +```json +{ + "features": [ + { + "feature_id": "engine_core", + "name": "Engine core", + "description": "Deterministic resolver and replay-critical state.", + "owned_globs": ["src/engine/**/*.js"], + "context_globs": ["docs/engine/**/*.md"], + "test_globs": ["test/engine/**/*.test.js"] + } + ] +} +``` + +Useful commands: + +```bash +review-code status +review-code run --feature-id engine_core +review-code run --all +review-code report --output ./review-report.md +``` + +The harness keeps prompts bounded, skips external symlinks, rejects findings +outside the reviewed file allowlist, and preserves existing triage fields when +a finding is regenerated. + ## Development ```bash npm install node bin/rewrite-release-notes.mjs --help node bin/build-updater-manifest.mjs --help +node bin/review-code.mjs --help +npm test ``` CI runs `node --check`, `--help`, and `--dry-run` smoke tests on every push. diff --git a/bin/review-code.mjs b/bin/review-code.mjs new file mode 100755 index 0000000..420256d --- /dev/null +++ b/bin/review-code.mjs @@ -0,0 +1,169 @@ +#!/usr/bin/env node +/** + * @license + * Copyright 2026 protoLabs + * SPDX-License-Identifier: Apache-2.0 + * + * Runs a bounded model-backed code review loop for any repo. + * + * Usage: + * review-code init [flags] + * review-code map [flags] + * review-code run [flags] + * review-code status [flags] + * review-code report [flags] + * + * Flags: + * --repo-root Repository to review. Default: current directory. + * --state-dir Local review state. Default: .release-tools-review + * --config Optional review-code config JSON with feature specs. + * --model Gateway model alias. Default: CODE_REVIEW_MODEL or protolabs/smart. + * --feature-id Review one mapped feature. + * --limit Review first n mapped features. Default for run: 1. + * --all Review every mapped feature. + * --output Markdown report path for report command. + * --help Show this help. + * + * Environment: + * GATEWAY_API_KEY or OPENAI_API_KEY Bearer token for the gateway. + * OPENAI_BASE_URL Default: https://api.proto-labs.ai/v1 + * CODE_REVIEW_MODEL Default model override. + */ + +import { + CodeReviewError, + DEFAULT_REVIEW_MODEL, + DEFAULT_REVIEW_STATE_DIR, + buildReviewReport, + initReviewState, + mapReviewFeatures, + reviewMappedFeatures, + reviewStatus, +} from '../lib/code-review.mjs'; + +if (process.argv.includes('--help') || process.argv.includes('-h')) { + const fs = await import('node:fs'); + const url = await import('node:url'); + const self = url.fileURLToPath(import.meta.url); + const src = fs.readFileSync(self, 'utf8').split('\n'); + const start = src.findIndex((line) => line.startsWith(' * Runs')); + const end = src.findIndex((line, index) => index > start && line.startsWith(' */')); + const help = src + .slice(start, end) + .map((line) => line.replace(/^ \* ?/, '')) + .join('\n'); + console.log(help); + process.exit(0); +} + +const { command, flags } = parseArgs(process.argv.slice(2)); +if (!command) { + console.error('Missing command. Use --help for usage.'); + process.exit(1); +} + +const repoRoot = flags['repo-root'] ?? process.cwd(); +const stateDir = flags['state-dir'] ?? DEFAULT_REVIEW_STATE_DIR; +const configPath = flags.config ?? null; +const model = + flags.model ?? process.env.CODE_REVIEW_MODEL ?? DEFAULT_REVIEW_MODEL; + +try { + if (command === 'init') { + const result = initReviewState({ repoRoot, stateDir, model, configPath }); + console.log(`review_state=${result.state_dir}`); + console.log(`model=${result.config.provider.model}`); + process.exit(0); + } + + if (command === 'map') { + const result = mapReviewFeatures({ repoRoot, stateDir, configPath }); + console.log(`features=${result.features.length}`); + console.log(`path=${resultPath(repoRoot, stateDir, 'features.json')}`); + process.exit(0); + } + + if (command === 'run') { + if (!process.env.GATEWAY_API_KEY && !process.env.OPENAI_API_KEY) { + throw new CodeReviewError('GATEWAY_API_KEY or OPENAI_API_KEY is required'); + } + const limit = runLimit(flags); + const result = await reviewMappedFeatures({ + repoRoot, + stateDir, + model, + limit, + featureId: flags['feature-id'] ?? null, + configPath, + }); + console.log( + `reviewed=${result.reviewed.length} findings=${result.finding_count} ` + + `rejected=${result.rejected_count} model=${result.model}`, + ); + process.exit(0); + } + + if (command === 'status') { + const result = reviewStatus({ repoRoot, stateDir }); + console.log( + `features=${result.feature_count} findings=${result.finding_count} ` + + `statuses=${JSON.stringify(result.status_counts)}`, + ); + process.exit(0); + } + + if (command === 'report') { + const result = buildReviewReport({ + repoRoot, + stateDir, + output: flags.output ?? null, + }); + console.log(`report=${result.path} findings=${result.finding_count}`); + process.exit(0); + } + + console.error(`Unknown command: ${command}`); + process.exit(1); +} catch (error) { + console.error(error instanceof Error ? error.message : String(error)); + process.exit(1); +} + +function parseArgs(args) { + const positional = []; + const flags = {}; + for (let index = 0; index < args.length; index += 1) { + const arg = args[index]; + if (!arg.startsWith('--')) { + positional.push(arg); + continue; + } + const key = arg.slice(2); + if (key === 'all') { + flags[key] = true; + continue; + } + const value = args[index + 1]; + if (!value || value.startsWith('--')) { + throw new CodeReviewError(`Missing value for --${key}`); + } + flags[key] = value; + index += 1; + } + return { command: positional[0], flags }; +} + +function runLimit(flags) { + if (flags.all) return null; + const raw = flags.limit ?? '1'; + const parsed = Number(raw); + if (!Number.isInteger(parsed) || parsed <= 0) { + throw new CodeReviewError('Invalid --limit: must be a positive integer'); + } + return parsed; +} + +function resultPath(repoRoot, stateDir, fileName) { + if (stateDir.startsWith('/')) return `${stateDir}/${fileName}`; + return `${repoRoot.replace(/\/$/, '')}/${stateDir}/${fileName}`; +} diff --git a/eslint.config.js b/eslint.config.js new file mode 100644 index 0000000..e9dd3b8 --- /dev/null +++ b/eslint.config.js @@ -0,0 +1,19 @@ +import js from '@eslint/js'; +import globals from 'globals'; + +export default [ + { + ignores: ['node_modules/**', 'coverage/**', 'dist/**', '*.tgz'], + }, + js.configs.recommended, + { + files: ['**/*.js', '**/*.mjs'], + languageOptions: { + ecmaVersion: 'latest', + sourceType: 'module', + globals: { + ...globals.node, + }, + }, + }, +]; diff --git a/lib/code-review.mjs b/lib/code-review.mjs new file mode 100644 index 0000000..db772e1 --- /dev/null +++ b/lib/code-review.mjs @@ -0,0 +1,816 @@ +/** + * @license + * Copyright 2026 protoLabs + * SPDX-License-Identifier: Apache-2.0 + */ + +import { execFileSync } from 'node:child_process'; +import { createHash } from 'node:crypto'; +import fs from 'node:fs'; +import path from 'node:path'; + +export const REVIEW_SCHEMA_VERSION = '0.1.0'; +export const DEFAULT_REVIEW_STATE_DIR = '.release-tools-review'; +export const DEFAULT_REVIEW_MODEL = 'protolabs/smart'; +export const DEFAULT_REVIEW_BASE_URL = 'https://api.proto-labs.ai/v1'; +export const MAX_REVIEW_CONTEXT_CHARS = 12000; +export const MAX_REVIEW_FILE_CHARS = 4000; + +const DEFAULT_FEATURE_SPECS = [ + { + feature_id: 'release_package', + name: 'Release package metadata', + description: 'Package metadata, README usage, actions, and workflow wiring.', + owned_globs: [ + 'package.json', + 'README.md', + 'action.yml', + '.github/workflows/*.yml', + '.github/workflows/*.yaml', + ], + context_globs: [], + test_globs: ['test/**/*.mjs', 'test/**/*.js', 'tests/**/*.mjs', 'tests/**/*.js'], + }, + { + feature_id: 'release_tools_cli', + name: 'Release tools CLI', + description: 'Executable CLI entrypoints and reusable release tooling logic.', + owned_globs: ['bin/*.mjs', 'lib/**/*.mjs', 'lib/*.mjs'], + context_globs: ['README.md', 'package.json'], + test_globs: ['test/**/*.mjs', 'test/**/*.js', 'tests/**/*.mjs', 'tests/**/*.js'], + }, + { + feature_id: 'tests', + name: 'Tests and smoke coverage', + description: 'Node tests, smoke scripts, and CI validation paths.', + owned_globs: ['test/**/*.mjs', 'test/**/*.js', 'tests/**/*.mjs', 'tests/**/*.js'], + context_globs: ['package.json', '.github/workflows/*.yml', '.github/workflows/*.yaml'], + test_globs: [], + }, +]; + +export class CodeReviewError extends Error { + constructor(message) { + super(message); + this.name = 'CodeReviewError'; + } +} + +export function initReviewState({ + repoRoot = process.cwd(), + stateDir = DEFAULT_REVIEW_STATE_DIR, + model = DEFAULT_REVIEW_MODEL, + configPath = null, +} = {}) { + const root = path.resolve(repoRoot); + const state = resolveStateDir(root, stateDir); + fs.mkdirSync(state, { recursive: true }); + const config = { + schema_version: REVIEW_SCHEMA_VERSION, + state_dir: state, + provider: { + name: 'openai-compatible', + model, + env: { + api_key: 'GATEWAY_API_KEY or OPENAI_API_KEY', + base_url: 'OPENAI_BASE_URL', + model: 'CODE_REVIEW_MODEL', + }, + }, + review: { + max_context_chars: MAX_REVIEW_CONTEXT_CHARS, + max_file_chars: MAX_REVIEW_FILE_CHARS, + max_findings_per_feature: 8, + }, + config_path: configPath, + feature_specs: loadFeatureSpecs(root, configPath), + }; + const project = { + schema_version: REVIEW_SCHEMA_VERSION, + project: path.basename(root), + kind: 'code-review-target', + root, + git_head: gitHead(root), + generated_at: utcNow(), + }; + writeJson(path.join(state, 'config.json'), config); + writeJson(path.join(state, 'project.json'), project); + return { state_dir: state, config, project }; +} + +export function mapReviewFeatures({ + repoRoot = process.cwd(), + stateDir = DEFAULT_REVIEW_STATE_DIR, + configPath = null, +} = {}) { + const root = path.resolve(repoRoot); + const state = resolveStateDir(root, stateDir); + const allFiles = repoFiles(root, state); + const featureSpecs = loadFeatureSpecs(root, configPath, state); + const features = featureSpecs.map((spec) => featureRecord(root, allFiles, spec)); + const payload = { + schema_version: REVIEW_SCHEMA_VERSION, + generated_at: utcNow(), + git_head: gitHead(root), + features, + }; + fs.mkdirSync(state, { recursive: true }); + writeJson(path.join(state, 'features.json'), payload); + return payload; +} + +export async function reviewMappedFeatures({ + repoRoot = process.cwd(), + stateDir = DEFAULT_REVIEW_STATE_DIR, + model = DEFAULT_REVIEW_MODEL, + limit = null, + featureId = null, + configPath = null, + client = null, +} = {}) { + const root = path.resolve(repoRoot); + const state = resolveStateDir(root, stateDir); + let features = [...loadOrMapFeatures(root, state, configPath).features]; + if (featureId) { + features = features.filter((feature) => feature.feature_id === featureId); + if (features.length === 0) { + throw new CodeReviewError(`Unknown feature_id: ${featureId}`); + } + } + if (limit !== null && limit !== undefined) { + const parsedLimit = Number(limit); + if (!Number.isInteger(parsedLimit) || parsedLimit <= 0) { + throw new CodeReviewError('Invalid --limit: must be a positive integer'); + } + features = features.slice(0, parsedLimit); + } + if (features.length === 0) { + throw new CodeReviewError('No feature records selected for review'); + } + + const provider = client ?? { + createStructuredOutput: (request) => callStructuredReview(request), + }; + const findingsDir = path.join(state, 'findings'); + fs.mkdirSync(findingsDir, { recursive: true }); + const acceptedFindings = []; + const reviewed = []; + let rejectedCount = 0; + + for (const feature of features) { + const response = await provider.createStructuredOutput({ + model, + systemPrompt: reviewSystemPrompt(), + userPrompt: reviewPrompt(root, feature), + schema: reviewOutputSchema(), + }); + const { findings, rejected } = normalizeFindings(root, feature, response); + rejectedCount += rejected; + for (const finding of findings) { + const findingPath = path.join(findingsDir, `${finding.finding_id}.json`); + const existing = loadJson(findingPath, null); + const merged = preserveTriage(existing, finding); + writeJson(findingPath, merged); + acceptedFindings.push(merged); + } + reviewed.push({ + feature_id: feature.feature_id, + finding_count: findings.length, + rejected_count: rejected, + }); + } + + const run = { + schema_version: REVIEW_SCHEMA_VERSION, + generated_at: utcNow(), + git_head: gitHead(root), + model, + reviewed, + finding_count: acceptedFindings.length, + rejected_count: rejectedCount, + findings: acceptedFindings.map((finding) => finding.finding_id), + }; + const runsDir = path.join(state, 'runs'); + fs.mkdirSync(runsDir, { recursive: true }); + writeJson(path.join(runsDir, `review_${timestampId()}.json`), run); + return run; +} + +export function reviewStatus({ + repoRoot = process.cwd(), + stateDir = DEFAULT_REVIEW_STATE_DIR, +} = {}) { + const root = path.resolve(repoRoot); + const state = resolveStateDir(root, stateDir); + const featuresPayload = loadJson(path.join(state, 'features.json'), {}); + const findings = loadReviewFindings({ stateDir: state }); + const statusCounts = {}; + const severityCounts = {}; + for (const finding of findings) { + const status = String(finding.status ?? 'open'); + const severity = String(finding.severity ?? 'low'); + statusCounts[status] = (statusCounts[status] ?? 0) + 1; + severityCounts[severity] = (severityCounts[severity] ?? 0) + 1; + } + return { + state_dir: state, + git_head: gitHead(root), + feature_count: Array.isArray(featuresPayload.features) + ? featuresPayload.features.length + : 0, + finding_count: findings.length, + status_counts: statusCounts, + severity_counts: severityCounts, + }; +} + +export function loadReviewFindings({ stateDir = DEFAULT_REVIEW_STATE_DIR } = {}) { + const findingsDir = path.join(path.resolve(stateDir), 'findings'); + if (!fs.existsSync(findingsDir)) return []; + const findings = []; + for (const name of fs.readdirSync(findingsDir).sort()) { + if (!name.endsWith('.json')) continue; + const payload = loadJson(path.join(findingsDir, name), null); + if (payload && typeof payload === 'object' && payload.finding_id) { + findings.push(payload); + } + } + return findings.sort((a, b) => { + const left = [ + severityRank(a.severity), + String(a.feature_id ?? ''), + String(a.path ?? ''), + Number(a.line ?? 0), + ]; + const right = [ + severityRank(b.severity), + String(b.feature_id ?? ''), + String(b.path ?? ''), + Number(b.line ?? 0), + ]; + return JSON.stringify(left).localeCompare(JSON.stringify(right)); + }); +} + +export function buildReviewReport({ + repoRoot = process.cwd(), + stateDir = DEFAULT_REVIEW_STATE_DIR, + output = null, +} = {}) { + const root = path.resolve(repoRoot); + const state = resolveStateDir(root, stateDir); + const reportPath = output + ? path.resolve(root, output) + : path.join(state, 'report.md'); + const status = reviewStatus({ repoRoot: root, stateDir: state }); + const findings = loadReviewFindings({ stateDir: state }); + const lines = [ + '# protoLabs Code Review Report', + '', + `- Generated: \`${utcNow()}\``, + `- Git head: \`${status.git_head}\``, + `- Features mapped: \`${status.feature_count}\``, + `- Findings: \`${status.finding_count}\``, + '', + ]; + if (findings.length === 0) { + lines.push('No findings recorded.', ''); + } else { + lines.push('## Findings', ''); + for (const finding of findings) { + lines.push( + `### ${String(finding.severity).toUpperCase()} - ${finding.title}`, + '', + `- ID: \`${finding.finding_id}\``, + `- Status: \`${finding.status ?? 'open'}\``, + `- Feature: \`${finding.feature_id ?? ''}\``, + `- Category: \`${finding.category ?? ''}\``, + `- Confidence: \`${finding.confidence ?? ''}\``, + `- Location: \`${finding.path ?? ''}:${finding.line ?? ''}\``, + '', + '**Evidence**', + '', + String(finding.evidence ?? '').trim(), + '', + '**Recommendation**', + '', + String(finding.recommendation ?? '').trim(), + '', + ); + } + } + fs.mkdirSync(path.dirname(reportPath), { recursive: true }); + fs.writeFileSync(reportPath, `${lines.join('\n')}\n`, 'utf8'); + return { path: reportPath, finding_count: findings.length }; +} + +export function reviewPrompt(repoRoot, feature) { + const snippets = []; + for (const [label, key, maxFiles] of [ + ['owned', 'owned_files', 6], + ['tests', 'test_files', 4], + ['context', 'context_files', 3], + ]) { + for (const relPath of (feature[key] ?? []).slice(0, maxFiles)) { + snippets.push(fileExcerpt(repoRoot, relPath, label)); + } + } + return [ + 'Review this bounded feature record.', + '', + `Feature:\n${JSON.stringify(feature, null, 2)}`, + '', + 'Relevant files:', + '', + boundedFileContext(snippets), + '', + 'Return findings only when they are still-valid and actionable.', + 'If no issue is worth filing, return {"findings": []}.', + ].join('\n'); +} + +export function reviewOutputSchema() { + return { + type: 'object', + additionalProperties: false, + properties: { + findings: { + type: 'array', + maxItems: 8, + items: { + type: 'object', + additionalProperties: false, + properties: { + title: { type: 'string' }, + category: { + type: 'string', + enum: [ + 'bug', + 'security', + 'performance', + 'docs-gap', + 'test-gap', + 'maintainability', + ], + }, + severity: { + type: 'string', + enum: ['critical', 'high', 'medium', 'low'], + }, + confidence: { + type: 'string', + enum: ['high', 'medium', 'low'], + }, + path: { type: 'string' }, + line: { type: 'integer', minimum: 1 }, + evidence: { type: 'string' }, + recommendation: { type: 'string' }, + }, + required: [ + 'title', + 'category', + 'severity', + 'confidence', + 'path', + 'line', + 'evidence', + 'recommendation', + ], + }, + }, + }, + required: ['findings'], + }; +} + +function reviewSystemPrompt() { + return [ + 'You are reviewing a software repository through protoLabs release-tools.', + 'Return strict JSON only.', + 'Focus on actionable correctness, security, determinism, contract drift, and missing tests.', + 'Do not report style nits. Do not invent files or private context.', + ].join(' '); +} + +async function callStructuredReview({ model, systemPrompt, userPrompt, schema }) { + const apiKey = process.env.GATEWAY_API_KEY || process.env.OPENAI_API_KEY; + if (!apiKey) { + throw new CodeReviewError('GATEWAY_API_KEY or OPENAI_API_KEY is required'); + } + const baseUrl = normalizeBaseUrl( + process.env.OPENAI_BASE_URL || DEFAULT_REVIEW_BASE_URL, + ); + const res = await fetch(`${baseUrl}/chat/completions`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${apiKey}`, + }, + body: JSON.stringify({ + model, + messages: [ + { role: 'system', content: systemPrompt }, + { role: 'user', content: userPrompt }, + ], + response_format: { + type: 'json_schema', + json_schema: { + name: 'code_review_findings', + strict: true, + schema, + }, + }, + }), + }); + if (!res.ok) { + throw new CodeReviewError(`LLM API error ${res.status}: ${await res.text()}`); + } + const payload = await res.json(); + const content = payload.choices?.[0]?.message?.content; + if (typeof content !== 'string') { + throw new CodeReviewError('LLM response did not include message content'); + } + try { + const parsed = JSON.parse(content); + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + throw new CodeReviewError('LLM response JSON must be an object'); + } + return parsed; + } catch (error) { + if (error instanceof CodeReviewError) throw error; + throw new CodeReviewError(`LLM response was not valid JSON: ${error.message}`); + } +} + +function normalizeFindings(repoRoot, feature, response) { + const rawFindings = response?.findings; + if (!Array.isArray(rawFindings)) { + throw new CodeReviewError('Review provider response must include findings list'); + } + const findings = []; + let rejected = 0; + for (const rawFinding of rawFindings) { + if (!rawFinding || typeof rawFinding !== 'object') { + rejected += 1; + continue; + } + const location = validateFindingLocation(repoRoot, feature, rawFinding); + if (!location) { + rejected += 1; + continue; + } + const finding = { + schema_version: REVIEW_SCHEMA_VERSION, + status: 'open', + feature_id: String(feature.feature_id ?? ''), + title: requiredString(rawFinding, 'title'), + category: requiredString(rawFinding, 'category'), + severity: requiredString(rawFinding, 'severity'), + confidence: requiredString(rawFinding, 'confidence'), + path: location.path, + line: location.line, + evidence: requiredString(rawFinding, 'evidence'), + recommendation: requiredString(rawFinding, 'recommendation'), + created_at: utcNow(), + }; + finding.finding_id = findingId(feature, finding); + findings.push(finding); + } + return { findings, rejected }; +} + +function validateFindingLocation(repoRoot, feature, finding) { + const relPath = typeof finding.path === 'string' ? normalizeRelPath(finding.path) : ''; + const line = Number(finding.line); + if (!relPath || !Number.isInteger(line) || line <= 0) return null; + const allowlist = new Set([ + ...(feature.owned_files ?? []), + ...(feature.context_files ?? []), + ...(feature.test_files ?? []), + ]); + if (!allowlist.has(relPath)) return null; + const absolute = safeRepoPath(repoRoot, relPath); + if (!absolute) return null; + let text; + try { + text = fs.readFileSync(absolute, 'utf8'); + } catch { + return null; + } + const lineCount = Math.max(1, text.split('\n').length); + if (line > lineCount) return null; + return { path: relPath, line }; +} + +function preserveTriage(existing, finding) { + if (!existing || typeof existing !== 'object') return finding; + const preserved = { ...finding }; + for (const key of ['status', 'triage', 'resolution', 'notes', 'owner']) { + if (existing[key] !== undefined) preserved[key] = existing[key]; + } + return preserved; +} + +function loadFeatureSpecs(repoRoot, configPath = null, stateDir = null) { + const candidates = []; + if (configPath) candidates.push(path.resolve(repoRoot, configPath)); + if (stateDir) candidates.push(path.join(stateDir, 'config.json')); + candidates.push(path.join(repoRoot, 'review-code.config.json')); + for (const candidate of candidates) { + const payload = loadJson(candidate, null); + if (!payload || typeof payload !== 'object') continue; + const specs = payload.features ?? payload.feature_specs; + if (Array.isArray(specs) && specs.length > 0) { + return specs.map(normalizeFeatureSpec); + } + } + return DEFAULT_FEATURE_SPECS.map(normalizeFeatureSpec); +} + +function normalizeFeatureSpec(spec) { + return { + feature_id: requiredConfigString(spec, 'feature_id'), + name: spec.name ? String(spec.name) : String(spec.feature_id), + description: spec.description ? String(spec.description) : '', + owned_globs: normalizeStringList(spec.owned_globs), + context_globs: normalizeStringList(spec.context_globs), + test_globs: normalizeStringList(spec.test_globs), + }; +} + +function normalizeStringList(value) { + if (!Array.isArray(value)) return []; + return value.filter((item) => typeof item === 'string' && item.trim()).map((item) => item.trim()); +} + +function requiredConfigString(spec, key) { + const value = spec?.[key]; + if (typeof value !== 'string' || !value.trim()) { + throw new CodeReviewError(`Feature spec field ${key} must be a non-empty string`); + } + return value.trim(); +} + +function loadOrMapFeatures(repoRoot, stateDir, configPath) { + const payload = loadJson(path.join(stateDir, 'features.json'), null); + if (payload && Array.isArray(payload.features)) return payload; + return mapReviewFeatures({ repoRoot, stateDir, configPath }); +} + +function featureRecord(repoRoot, allFiles, spec) { + const ownedFiles = matchFiles(allFiles, spec.owned_globs); + const contextFiles = matchFiles(allFiles, spec.context_globs).filter( + (relPath) => !ownedFiles.includes(relPath), + ); + const testFiles = matchFiles(allFiles, spec.test_globs); + return { + feature_id: spec.feature_id, + name: spec.name, + description: spec.description, + entrypoints: entrypoints(ownedFiles), + owned_files: ownedFiles, + context_files: contextFiles, + test_files: testFiles, + owned_file_count: ownedFiles.length, + context_file_count: contextFiles.length, + test_file_count: testFiles.length, + owned_bytes: ownedFiles.reduce((total, relPath) => { + try { + return total + fs.statSync(path.join(repoRoot, relPath)).size; + } catch { + return total; + } + }, 0), + }; +} + +function repoFiles(repoRoot, stateDir) { + const excludeDirs = new Set([ + '.git', + 'node_modules', + 'coverage', + 'dist', + '.clawpatch', + '.release-tools-review', + path.basename(stateDir), + ]); + const files = []; + walkRepo(repoRoot, repoRoot, excludeDirs, files); + return files.sort(); +} + +function walkRepo(repoRoot, currentDir, excludeDirs, files) { + let entries; + try { + entries = fs.readdirSync(currentDir, { withFileTypes: true }); + } catch { + return; + } + for (const entry of entries) { + if (excludeDirs.has(entry.name)) continue; + const absolute = path.join(currentDir, entry.name); + const relPath = normalizeRelPath(path.relative(repoRoot, absolute)); + if (entry.isSymbolicLink()) { + const resolved = safeResolvedPath(repoRoot, absolute); + if (!resolved) continue; + let stats; + try { + stats = fs.statSync(resolved); + } catch { + continue; + } + if (stats.isFile()) files.push(relPath); + continue; + } + if (entry.isDirectory()) { + walkRepo(repoRoot, absolute, excludeDirs, files); + continue; + } + if (entry.isFile()) files.push(relPath); + } +} + +function matchFiles(files, patterns) { + const matched = []; + for (const pattern of patterns) { + const regex = globToRegExp(pattern); + for (const relPath of files) { + if (regex.test(relPath) && !matched.includes(relPath)) { + matched.push(relPath); + } + } + } + return matched; +} + +function globToRegExp(pattern) { + const normalized = normalizeRelPath(pattern); + let source = ''; + for (let index = 0; index < normalized.length; index += 1) { + const char = normalized[index]; + const next = normalized[index + 1]; + const afterNext = normalized[index + 2]; + if (char === '*' && next === '*' && afterNext === '/') { + source += '(?:.*/)?'; + index += 2; + continue; + } + if (char === '*' && next === '*') { + source += '.*'; + index += 1; + continue; + } + if (char === '*') { + source += '[^/]*'; + continue; + } + source += char.replace(/[.+^${}()|[\]\\]/g, '\\$&'); + } + return new RegExp(`^${source}$`); +} + +function entrypoints(ownedFiles) { + const priority = ['package.json', 'action.yml']; + const prioritized = priority.filter((relPath) => ownedFiles.includes(relPath)); + return prioritized.length > 0 ? prioritized : ownedFiles.slice(0, 2); +} + +function fileExcerpt(repoRoot, relPath, label) { + const absolute = safeRepoPath(repoRoot, relPath); + if (!absolute) return `### ${label}: ${relPath}\n(unreadable: outside repo)`; + let text; + try { + text = fs.readFileSync(absolute, 'utf8'); + } catch (error) { + return `### ${label}: ${relPath}\n(unreadable: ${error.message})`; + } + if (text.length > MAX_REVIEW_FILE_CHARS) { + text = `${text.slice(0, MAX_REVIEW_FILE_CHARS)}\n......`; + } + const numbered = text + .split('\n') + .map((line, index) => `${String(index + 1).padStart(4, '0')}: ${line}`) + .join('\n'); + return `### ${label}: ${relPath}\n\`\`\`text\n${numbered}\n\`\`\``; +} + +function boundedFileContext(snippets) { + const kept = []; + let total = 0; + let omitted = 0; + for (const snippet of snippets) { + const extra = snippet.length + (kept.length > 0 ? 2 : 0); + if (kept.length > 0 && total + extra > MAX_REVIEW_CONTEXT_CHARS) { + omitted += 1; + continue; + } + if (kept.length === 0 && extra > MAX_REVIEW_CONTEXT_CHARS) { + const marker = '\n......'; + const budget = Math.max(0, MAX_REVIEW_CONTEXT_CHARS - marker.length); + kept.push(`${snippet.slice(0, budget)}${marker}`); + total = kept[0].length; + continue; + } + kept.push(snippet); + total += extra; + } + let rendered = kept.join('\n\n'); + if (omitted > 0) { + rendered += `\n\n...<${omitted} file excerpt(s) omitted by review context budget>...`; + } + return rendered; +} + +function requiredString(mapping, key) { + const value = mapping?.[key]; + if (typeof value !== 'string' || !value.trim()) { + throw new CodeReviewError(`Finding field ${key} must be a non-empty string`); + } + return value.trim(); +} + +function findingId(feature, finding) { + const raw = [ + feature.feature_id ?? '', + finding.title ?? '', + finding.path ?? '', + finding.line ?? '', + finding.evidence ?? '', + ].join('|'); + return createHash('sha256').update(raw).digest('hex').slice(0, 12); +} + +function severityRank(severity) { + return { critical: 0, high: 1, medium: 2, low: 3 }[severity] ?? 4; +} + +function resolveStateDir(repoRoot, stateDir) { + return path.isAbsolute(stateDir) ? stateDir : path.join(repoRoot, stateDir); +} + +function safeRepoPath(repoRoot, relPath) { + const absolute = path.resolve(repoRoot, relPath); + return isInside(repoRoot, absolute) ? absolute : null; +} + +function safeResolvedPath(repoRoot, absolute) { + try { + const resolved = fs.realpathSync(absolute); + return isInside(repoRoot, resolved) ? resolved : null; + } catch { + return null; + } +} + +function isInside(root, target) { + const relative = path.relative(path.resolve(root), path.resolve(target)); + return relative === '' || (!relative.startsWith('..') && !path.isAbsolute(relative)); +} + +function normalizeRelPath(value) { + return String(value).replaceAll(path.sep, '/').replace(/^\.\/+/, ''); +} + +function normalizeBaseUrl(value) { + const url = new URL(value); + if (!['http:', 'https:'].includes(url.protocol) || !url.host) { + throw new CodeReviewError('OPENAI_BASE_URL must use http or https'); + } + if (url.pathname === '' || url.pathname === '/') { + url.pathname = '/v1'; + } + return url.toString().replace(/\/$/, ''); +} + +function gitHead(repoRoot) { + try { + return execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: repoRoot, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + }).trim(); + } catch { + return 'unknown'; + } +} + +function writeJson(filePath, payload) { + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, `${JSON.stringify(payload, null, 2)}\n`, 'utf8'); +} + +function loadJson(filePath, fallback) { + try { + return JSON.parse(fs.readFileSync(filePath, 'utf8')); + } catch { + return fallback; + } +} + +function utcNow() { + return new Date().toISOString().replace(/\.\d{3}Z$/, 'Z'); +} + +function timestampId() { + return new Date().toISOString().replace(/[-:.]/g, '').replace('T', 'T'); +} diff --git a/package-lock.json b/package-lock.json index 6891843..3c44a17 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,14 +1,16 @@ { "name": "@protolabsai/release-tools", - "version": "0.1.0", + "version": "1.0.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@protolabsai/release-tools", - "version": "0.1.0", + "version": "1.0.0", "license": "Apache-2.0", "bin": { + "build-updater-manifest": "bin/build-updater-manifest.mjs", + "review-code": "bin/review-code.mjs", "rewrite-release-notes": "bin/rewrite-release-notes.mjs" }, "devDependencies": { diff --git a/package.json b/package.json index eec884e..cd7c359 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,8 @@ }, "bin": { "rewrite-release-notes": "./bin/rewrite-release-notes.mjs", - "build-updater-manifest": "./bin/build-updater-manifest.mjs" + "build-updater-manifest": "./bin/build-updater-manifest.mjs", + "review-code": "./bin/review-code.mjs" }, "files": [ "bin/", @@ -33,8 +34,8 @@ }, "scripts": { "lint": "eslint .", - "smoke": "node bin/rewrite-release-notes.mjs --help && node bin/build-updater-manifest.mjs --help", - "test": "node --test test/" + "smoke": "node bin/rewrite-release-notes.mjs --help && node bin/build-updater-manifest.mjs --help && node bin/review-code.mjs --help", + "test": "node --test \"test/**/*.mjs\"" }, "devDependencies": { "eslint": "^9.0.0", diff --git a/test/code-review.test.mjs b/test/code-review.test.mjs new file mode 100644 index 0000000..145cffb --- /dev/null +++ b/test/code-review.test.mjs @@ -0,0 +1,214 @@ +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import test from 'node:test'; + +import { + MAX_REVIEW_CONTEXT_CHARS, + buildReviewReport, + initReviewState, + mapReviewFeatures, + reviewMappedFeatures, + reviewPrompt, + reviewStatus, +} from '../lib/code-review.mjs'; + +test('maps configured features and skips external symlinks', () => { + const repo = createRepo(); + const outside = path.join(os.tmpdir(), `release-tools-leak-${process.pid}.js`); + fs.writeFileSync(outside, 'secret();\n', 'utf8'); + try { + fs.symlinkSync(outside, path.join(repo, 'src', 'leak.js')); + } catch { + // Some platforms or filesystems disallow symlinks; the mapping behavior is + // still covered by the real file assertions below. + } + + const mapped = mapReviewFeatures({ + repoRoot: repo, + stateDir: '.review-state', + configPath: 'review-code.config.json', + }); + + const feature = mapped.features.find((item) => item.feature_id === 'app'); + assert.ok(feature); + assert.deepEqual(feature.owned_files, ['src/app.js']); + assert.deepEqual(feature.test_files, ['test/app.test.js']); + assert.ok(!feature.owned_files.includes('src/leak.js')); +}); + +test('review run persists findings, preserves triage, and reports status', async () => { + const repo = createRepo(); + initReviewState({ + repoRoot: repo, + stateDir: '.review-state', + configPath: 'review-code.config.json', + }); + mapReviewFeatures({ + repoRoot: repo, + stateDir: '.review-state', + configPath: 'review-code.config.json', + }); + const client = new FakeClient([ + { + title: 'App bug', + category: 'bug', + severity: 'medium', + confidence: 'high', + path: 'src/app.js', + line: 1, + evidence: 'The app exports a value.', + recommendation: 'Add a clearer assertion.', + }, + ]); + + const first = await reviewMappedFeatures({ + repoRoot: repo, + stateDir: '.review-state', + featureId: 'app', + client, + }); + + assert.equal(first.finding_count, 1); + const findingsDir = path.join(repo, '.review-state', 'findings'); + const findingFile = path.join(findingsDir, fs.readdirSync(findingsDir)[0]); + const persisted = JSON.parse(fs.readFileSync(findingFile, 'utf8')); + persisted.status = 'resolved'; + persisted.notes = 'Handled elsewhere.'; + fs.writeFileSync(findingFile, `${JSON.stringify(persisted, null, 2)}\n`, 'utf8'); + + const second = await reviewMappedFeatures({ + repoRoot: repo, + stateDir: '.review-state', + featureId: 'app', + client, + }); + const status = reviewStatus({ repoRoot: repo, stateDir: '.review-state' }); + const report = buildReviewReport({ repoRoot: repo, stateDir: '.review-state' }); + const finalFinding = JSON.parse(fs.readFileSync(findingFile, 'utf8')); + + assert.equal(second.finding_count, 1); + assert.equal(status.finding_count, 1); + assert.equal(finalFinding.status, 'resolved'); + assert.equal(finalFinding.notes, 'Handled elsewhere.'); + assert.equal(report.finding_count, 1); + assert.match( + fs.readFileSync(path.join(repo, '.review-state', 'report.md'), 'utf8'), + /App bug/, + ); +}); + +test('review run rejects out-of-scope findings', async () => { + const repo = createRepo(); + mapReviewFeatures({ + repoRoot: repo, + stateDir: '.review-state', + configPath: 'review-code.config.json', + }); + const client = new FakeClient([ + { + title: 'Outside file', + category: 'bug', + severity: 'high', + confidence: 'high', + path: 'package.json', + line: 1, + evidence: 'Not in the reviewed feature allowlist.', + recommendation: 'Do not persist this.', + }, + { + title: 'Bad line', + category: 'bug', + severity: 'low', + confidence: 'high', + path: 'src/app.js', + line: 999, + evidence: 'Line is outside file bounds.', + recommendation: 'Do not persist this either.', + }, + ]); + + const result = await reviewMappedFeatures({ + repoRoot: repo, + stateDir: '.review-state', + featureId: 'app', + client, + }); + + assert.equal(result.finding_count, 0); + assert.equal(result.rejected_count, 2); +}); + +test('review prompts stay bounded', () => { + const repo = createRepo(); + for (let index = 0; index < 8; index += 1) { + fs.writeFileSync( + path.join(repo, 'src', `large-${index}.js`), + `${'x'.repeat(5000)}\n`, + 'utf8', + ); + } + const mapped = mapReviewFeatures({ + repoRoot: repo, + stateDir: '.review-state', + configPath: 'review-code.config.json', + }); + const feature = mapped.features.find((item) => item.feature_id === 'app'); + const prompt = reviewPrompt(repo, feature); + + assert.ok(prompt.length < MAX_REVIEW_CONTEXT_CHARS + 2500); + assert.match(prompt, /omitted by review context budget/); +}); + +test('review-code CLI help works', () => { + const output = execFileSync('node', ['bin/review-code.mjs', '--help'], { + cwd: path.resolve(import.meta.dirname, '..'), + encoding: 'utf8', + }); + assert.match(output, /review-code run/); +}); + +class FakeClient { + constructor(findings) { + this.findings = findings; + } + + async createStructuredOutput() { + return { findings: this.findings }; + } +} + +function createRepo() { + const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'release-tools-review-')); + fs.mkdirSync(path.join(repo, 'src'), { recursive: true }); + fs.mkdirSync(path.join(repo, 'test'), { recursive: true }); + fs.writeFileSync(path.join(repo, 'src', 'app.js'), 'export const value = 1;\n', 'utf8'); + fs.writeFileSync( + path.join(repo, 'test', 'app.test.js'), + 'import assert from "node:assert/strict";\nassert.equal(1, 1);\n', + 'utf8', + ); + fs.writeFileSync( + path.join(repo, 'review-code.config.json'), + JSON.stringify( + { + features: [ + { + feature_id: 'app', + name: 'App', + description: 'App source and tests.', + owned_globs: ['src/**/*.js'], + context_globs: [], + test_globs: ['test/**/*.js'], + }, + ], + }, + null, + 2, + ), + 'utf8', + ); + return repo; +} From b6d43ae730bb549e18ed12d17aa32891cd77784f Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Thu, 21 May 2026 03:15:26 -0700 Subject: [PATCH 2/3] Fix review-code test script for CI --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index cd7c359..8acd7e7 100644 --- a/package.json +++ b/package.json @@ -35,7 +35,7 @@ "scripts": { "lint": "eslint .", "smoke": "node bin/rewrite-release-notes.mjs --help && node bin/build-updater-manifest.mjs --help && node bin/review-code.mjs --help", - "test": "node --test \"test/**/*.mjs\"" + "test": "node --test test/*.mjs" }, "devDependencies": { "eslint": "^9.0.0", From 14a901b5ebac55e6d5acda7b4decf3b239a505aa Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Thu, 21 May 2026 03:19:51 -0700 Subject: [PATCH 3/3] Address review-code hardening feedback --- bin/review-code.mjs | 8 +- lib/code-review.mjs | 60 +++++--- test/code-review.test.mjs | 285 ++++++++++++++++++++------------------ 3 files changed, 197 insertions(+), 156 deletions(-) diff --git a/bin/review-code.mjs b/bin/review-code.mjs index 420256d..effa5e0 100755 --- a/bin/review-code.mjs +++ b/bin/review-code.mjs @@ -30,6 +30,8 @@ * CODE_REVIEW_MODEL Default model override. */ +import path from 'node:path'; + import { CodeReviewError, DEFAULT_REVIEW_MODEL, @@ -164,6 +166,8 @@ function runLimit(flags) { } function resultPath(repoRoot, stateDir, fileName) { - if (stateDir.startsWith('/')) return `${stateDir}/${fileName}`; - return `${repoRoot.replace(/\/$/, '')}/${stateDir}/${fileName}`; + return path.join( + path.isAbsolute(stateDir) ? stateDir : path.join(repoRoot, stateDir), + fileName, + ); } diff --git a/lib/code-review.mjs b/lib/code-review.mjs index db772e1..f7faaab 100644 --- a/lib/code-review.mjs +++ b/lib/code-review.mjs @@ -400,28 +400,44 @@ async function callStructuredReview({ model, systemPrompt, userPrompt, schema }) const baseUrl = normalizeBaseUrl( process.env.OPENAI_BASE_URL || DEFAULT_REVIEW_BASE_URL, ); - const res = await fetch(`${baseUrl}/chat/completions`, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - Authorization: `Bearer ${apiKey}`, - }, - body: JSON.stringify({ - model, - messages: [ - { role: 'system', content: systemPrompt }, - { role: 'user', content: userPrompt }, - ], - response_format: { - type: 'json_schema', - json_schema: { - name: 'code_review_findings', - strict: true, - schema, - }, + const rawTimeoutMs = Number(process.env.CODE_REVIEW_TIMEOUT_MS || '30000'); + const timeoutMs = + Number.isFinite(rawTimeoutMs) && rawTimeoutMs > 0 ? rawTimeoutMs : 30000; + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), timeoutMs); + let res; + try { + res = await fetch(`${baseUrl}/chat/completions`, { + method: 'POST', + signal: controller.signal, + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${apiKey}`, }, - }), - }); + body: JSON.stringify({ + model, + messages: [ + { role: 'system', content: systemPrompt }, + { role: 'user', content: userPrompt }, + ], + response_format: { + type: 'json_schema', + json_schema: { + name: 'code_review_findings', + strict: true, + schema, + }, + }, + }), + }); + } catch (error) { + if (error?.name === 'AbortError') { + throw new CodeReviewError(`LLM API request timed out after ${timeoutMs}ms`); + } + throw error; + } finally { + clearTimeout(timeout); + } if (!res.ok) { throw new CodeReviewError(`LLM API error ${res.status}: ${await res.text()}`); } @@ -812,5 +828,5 @@ function utcNow() { } function timestampId() { - return new Date().toISOString().replace(/[-:.]/g, '').replace('T', 'T'); + return new Date().toISOString().replace(/[-:.]/g, ''); } diff --git a/test/code-review.test.mjs b/test/code-review.test.mjs index 145cffb..a984dab 100644 --- a/test/code-review.test.mjs +++ b/test/code-review.test.mjs @@ -4,6 +4,7 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import test from 'node:test'; +import { fileURLToPath } from 'node:url'; import { MAX_REVIEW_CONTEXT_CHARS, @@ -16,155 +17,172 @@ import { } from '../lib/code-review.mjs'; test('maps configured features and skips external symlinks', () => { - const repo = createRepo(); + const { repo, cleanup } = createRepo(); const outside = path.join(os.tmpdir(), `release-tools-leak-${process.pid}.js`); fs.writeFileSync(outside, 'secret();\n', 'utf8'); try { - fs.symlinkSync(outside, path.join(repo, 'src', 'leak.js')); - } catch { - // Some platforms or filesystems disallow symlinks; the mapping behavior is - // still covered by the real file assertions below. + try { + fs.symlinkSync(outside, path.join(repo, 'src', 'leak.js')); + } catch { + // Some platforms or filesystems disallow symlinks; the mapping behavior is + // still covered by the real file assertions below. + } + + const mapped = mapReviewFeatures({ + repoRoot: repo, + stateDir: '.review-state', + configPath: 'review-code.config.json', + }); + + const feature = mapped.features.find((item) => item.feature_id === 'app'); + assert.ok(feature); + assert.deepEqual(feature.owned_files, ['src/app.js']); + assert.deepEqual(feature.test_files, ['test/app.test.js']); + assert.ok(!feature.owned_files.includes('src/leak.js')); + } finally { + cleanup(); + fs.rmSync(outside, { force: true }); } - - const mapped = mapReviewFeatures({ - repoRoot: repo, - stateDir: '.review-state', - configPath: 'review-code.config.json', - }); - - const feature = mapped.features.find((item) => item.feature_id === 'app'); - assert.ok(feature); - assert.deepEqual(feature.owned_files, ['src/app.js']); - assert.deepEqual(feature.test_files, ['test/app.test.js']); - assert.ok(!feature.owned_files.includes('src/leak.js')); }); test('review run persists findings, preserves triage, and reports status', async () => { - const repo = createRepo(); - initReviewState({ - repoRoot: repo, - stateDir: '.review-state', - configPath: 'review-code.config.json', - }); - mapReviewFeatures({ - repoRoot: repo, - stateDir: '.review-state', - configPath: 'review-code.config.json', - }); - const client = new FakeClient([ - { - title: 'App bug', - category: 'bug', - severity: 'medium', - confidence: 'high', - path: 'src/app.js', - line: 1, - evidence: 'The app exports a value.', - recommendation: 'Add a clearer assertion.', - }, - ]); - - const first = await reviewMappedFeatures({ - repoRoot: repo, - stateDir: '.review-state', - featureId: 'app', - client, - }); - - assert.equal(first.finding_count, 1); - const findingsDir = path.join(repo, '.review-state', 'findings'); - const findingFile = path.join(findingsDir, fs.readdirSync(findingsDir)[0]); - const persisted = JSON.parse(fs.readFileSync(findingFile, 'utf8')); - persisted.status = 'resolved'; - persisted.notes = 'Handled elsewhere.'; - fs.writeFileSync(findingFile, `${JSON.stringify(persisted, null, 2)}\n`, 'utf8'); - - const second = await reviewMappedFeatures({ - repoRoot: repo, - stateDir: '.review-state', - featureId: 'app', - client, - }); - const status = reviewStatus({ repoRoot: repo, stateDir: '.review-state' }); - const report = buildReviewReport({ repoRoot: repo, stateDir: '.review-state' }); - const finalFinding = JSON.parse(fs.readFileSync(findingFile, 'utf8')); - - assert.equal(second.finding_count, 1); - assert.equal(status.finding_count, 1); - assert.equal(finalFinding.status, 'resolved'); - assert.equal(finalFinding.notes, 'Handled elsewhere.'); - assert.equal(report.finding_count, 1); - assert.match( - fs.readFileSync(path.join(repo, '.review-state', 'report.md'), 'utf8'), - /App bug/, - ); + const { repo, cleanup } = createRepo(); + try { + initReviewState({ + repoRoot: repo, + stateDir: '.review-state', + configPath: 'review-code.config.json', + }); + mapReviewFeatures({ + repoRoot: repo, + stateDir: '.review-state', + configPath: 'review-code.config.json', + }); + const client = new FakeClient([ + { + title: 'App bug', + category: 'bug', + severity: 'medium', + confidence: 'high', + path: 'src/app.js', + line: 1, + evidence: 'The app exports a value.', + recommendation: 'Add a clearer assertion.', + }, + ]); + + const first = await reviewMappedFeatures({ + repoRoot: repo, + stateDir: '.review-state', + featureId: 'app', + client, + }); + + assert.equal(first.finding_count, 1); + const findingsDir = path.join(repo, '.review-state', 'findings'); + const findingFile = path.join(findingsDir, fs.readdirSync(findingsDir)[0]); + const persisted = JSON.parse(fs.readFileSync(findingFile, 'utf8')); + persisted.status = 'resolved'; + persisted.notes = 'Handled elsewhere.'; + fs.writeFileSync(findingFile, `${JSON.stringify(persisted, null, 2)}\n`, 'utf8'); + + const second = await reviewMappedFeatures({ + repoRoot: repo, + stateDir: '.review-state', + featureId: 'app', + client, + }); + const status = reviewStatus({ repoRoot: repo, stateDir: '.review-state' }); + const report = buildReviewReport({ repoRoot: repo, stateDir: '.review-state' }); + const finalFinding = JSON.parse(fs.readFileSync(findingFile, 'utf8')); + + assert.equal(second.finding_count, 1); + assert.equal(status.finding_count, 1); + assert.equal(finalFinding.status, 'resolved'); + assert.equal(finalFinding.notes, 'Handled elsewhere.'); + assert.equal(report.finding_count, 1); + assert.match( + fs.readFileSync(path.join(repo, '.review-state', 'report.md'), 'utf8'), + /App bug/, + ); + } finally { + cleanup(); + } }); test('review run rejects out-of-scope findings', async () => { - const repo = createRepo(); - mapReviewFeatures({ - repoRoot: repo, - stateDir: '.review-state', - configPath: 'review-code.config.json', - }); - const client = new FakeClient([ - { - title: 'Outside file', - category: 'bug', - severity: 'high', - confidence: 'high', - path: 'package.json', - line: 1, - evidence: 'Not in the reviewed feature allowlist.', - recommendation: 'Do not persist this.', - }, - { - title: 'Bad line', - category: 'bug', - severity: 'low', - confidence: 'high', - path: 'src/app.js', - line: 999, - evidence: 'Line is outside file bounds.', - recommendation: 'Do not persist this either.', - }, - ]); - - const result = await reviewMappedFeatures({ - repoRoot: repo, - stateDir: '.review-state', - featureId: 'app', - client, - }); - - assert.equal(result.finding_count, 0); - assert.equal(result.rejected_count, 2); + const { repo, cleanup } = createRepo(); + try { + mapReviewFeatures({ + repoRoot: repo, + stateDir: '.review-state', + configPath: 'review-code.config.json', + }); + const client = new FakeClient([ + { + title: 'Outside file', + category: 'bug', + severity: 'high', + confidence: 'high', + path: 'package.json', + line: 1, + evidence: 'Not in the reviewed feature allowlist.', + recommendation: 'Do not persist this.', + }, + { + title: 'Bad line', + category: 'bug', + severity: 'low', + confidence: 'high', + path: 'src/app.js', + line: 999, + evidence: 'Line is outside file bounds.', + recommendation: 'Do not persist this either.', + }, + ]); + + const result = await reviewMappedFeatures({ + repoRoot: repo, + stateDir: '.review-state', + featureId: 'app', + client, + }); + + assert.equal(result.finding_count, 0); + assert.equal(result.rejected_count, 2); + } finally { + cleanup(); + } }); test('review prompts stay bounded', () => { - const repo = createRepo(); - for (let index = 0; index < 8; index += 1) { - fs.writeFileSync( - path.join(repo, 'src', `large-${index}.js`), - `${'x'.repeat(5000)}\n`, - 'utf8', - ); + const { repo, cleanup } = createRepo(); + try { + for (let index = 0; index < 8; index += 1) { + fs.writeFileSync( + path.join(repo, 'src', `large-${index}.js`), + `${'x'.repeat(5000)}\n`, + 'utf8', + ); + } + const mapped = mapReviewFeatures({ + repoRoot: repo, + stateDir: '.review-state', + configPath: 'review-code.config.json', + }); + const feature = mapped.features.find((item) => item.feature_id === 'app'); + const prompt = reviewPrompt(repo, feature); + + assert.ok(prompt.length < MAX_REVIEW_CONTEXT_CHARS + 2500); + assert.match(prompt, /omitted by review context budget/); + } finally { + cleanup(); } - const mapped = mapReviewFeatures({ - repoRoot: repo, - stateDir: '.review-state', - configPath: 'review-code.config.json', - }); - const feature = mapped.features.find((item) => item.feature_id === 'app'); - const prompt = reviewPrompt(repo, feature); - - assert.ok(prompt.length < MAX_REVIEW_CONTEXT_CHARS + 2500); - assert.match(prompt, /omitted by review context budget/); }); test('review-code CLI help works', () => { const output = execFileSync('node', ['bin/review-code.mjs', '--help'], { - cwd: path.resolve(import.meta.dirname, '..'), + cwd: path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'), encoding: 'utf8', }); assert.match(output, /review-code run/); @@ -210,5 +228,8 @@ function createRepo() { ), 'utf8', ); - return repo; + return { + repo, + cleanup: () => fs.rmSync(repo, { recursive: true, force: true }), + }; }