diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d96423..b09c7bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ All notable changes will be documented here. This project follows Semantic Versi - Added strict versioned `.agentskit-review.json` policy with lens coverage, budgets, thresholds, context, and safe CI precedence; incomplete profiles require explicit local opt-in. - Hardened the shared local CLI worker with cancellation, process-tree cleanup, isolated temporary environments, bounded output, and redacted diagnostics. +- Added bounded source snapshots with infrastructure/configuration file support, denylisted sensitive paths, symlink checks, input limits, and data-boundary-aware secret redaction. - Made reviews fail closed when any reviewable file has no successful primary lens or cannot be ingested; advisory mode now suppresses finding-based failures only, never source/provider/execution failures. - Added primary-lens execution coverage to review summaries so partial provider degradation is visible. - Repositioned the CLI and GitHub Action as provider-neutral. diff --git a/README.md b/README.md index 83ccc65..2ad27bf 100644 --- a/README.md +++ b/README.md @@ -259,6 +259,7 @@ In shortened examples, replace `...` with `npx --yes github:AgentsKit-io/code-re | `--no-fail` | Keep findings advisory | | `--conventions ` | Inject project conventions | | `--allow-incomplete` | Local-only exception for a config that declares incomplete lens coverage | +| `--allow-unredacted` | Local-only exception; rejected in CI | | `--api` | Back-compatible alias for `--provider anthropic` | | `doctor --provider ` | Offline provider diagnostics; no model request | | `doctor --live` | Explicit provider smoke-test mode | @@ -295,6 +296,9 @@ never accepted in CI. Provider, model, transport, context trust, redaction, and permissions are trusted execution inputs; a project config cannot set them in CI. Put provider credentials only in the environment or provider login, never in this file. +Remote and unknown provider boundaries redact high-confidence credential +patterns before the model sees source. Unsafe, oversized, binary, or excluded +paths are reported as `UNREVIEWED`; content is never silently truncated. ### Doctor diff --git a/agents/code-review/agent.ts b/agents/code-review/agent.ts index e75be3a..119487a 100644 --- a/agents/code-review/agent.ts +++ b/agents/code-review/agent.ts @@ -53,6 +53,9 @@ export interface ReviewTarget { isChanged: boolean /** Head commit SHA, for github-pr (needed to anchor inline comments). */ commitId?: string + /** Source normalization could not safely review this path. */ + reviewStatus?: 'UNREVIEWED' + unreviewedReason?: string } export interface Finding { @@ -106,6 +109,7 @@ export interface ReviewResult { droppedNote?: string /** Provider execution coverage for primary review lenses. */ execution: LensExecutionStats + unreviewed?: Array<{ file: string; reason: string }> summary: string } @@ -420,6 +424,7 @@ export function createCodeReviewAgent(config: CodeReviewConfig) { reviewed: number, droppedFiles: number, execution: LensExecutionStats, + unreviewedCount: number, ): ReviewResult { const counts = (['blocker', 'high', 'med', 'nit'] as Severity[]).map((s) => ({ s, n: kept.filter((f) => f.severity === s).length })) const worst = kept.length ? Math.min(...kept.map((f) => SEV_RANK[f.severity])) : 3 @@ -432,6 +437,7 @@ export function createCodeReviewAgent(config: CodeReviewConfig) { const summary = `${kept.length} finding(s) (${breakdown}) across ${reviewed} file(s)` + (config.incompleteProfile ? ' Incomplete profile; this review is not an approval.' : '') + + (unreviewedCount ? ` ${unreviewedCount} file(s) UNREVIEWED.` : '') + (droppedFiles ? `, ${droppedFiles} file(s) skipped for budget` : '') + `. ${executionSummary}.` return { verdict, blocking, findings: kept, dropped, execution, summary } @@ -444,8 +450,10 @@ export function createCodeReviewAgent(config: CodeReviewConfig) { emit('ingest', 'start') const t0 = Date.now() const all = await loadTargets(config.source) + const unreviewed = all.filter((target) => target.reviewStatus === 'UNREVIEWED') + for (const target of unreviewed) emit('ingest', 'skip', `${target.file}: ${target.unreviewedReason ?? 'unreviewed'}`) // Prioritise: changed first, then by amount of change, then size. - const ranked = [...all].sort( + const ranked = all.filter((target) => target.reviewStatus !== 'UNREVIEWED').sort( (a, b) => Number(b.isChanged) - Number(a.isChanged) || (b.changedRanges?.length ?? 0) - (a.changedRanges?.length ?? 0) || @@ -462,7 +470,8 @@ export function createCodeReviewAgent(config: CodeReviewConfig) { findings: [], dropped: [], execution: { attempted: 0, succeeded: 0, failed: 0 }, - summary: 'Nothing to review.', + unreviewed: unreviewed.map((target) => ({ file: target.file, reason: target.unreviewedReason ?? 'unreviewed' })), + summary: unreviewed.length ? `${unreviewed.length} file(s) UNREVIEWED; nothing else to review.` : 'Nothing to review.', } } @@ -518,7 +527,8 @@ export function createCodeReviewAgent(config: CodeReviewConfig) { emit('validate-patch', 'ok', undefined, Date.now() - t3) } - const result = synthesize(kept, dropped, targets.length, droppedFiles, execution) + const result = synthesize(kept, dropped, targets.length, droppedFiles, execution, unreviewed.length) + result.unreviewed = unreviewed.map((target) => ({ file: target.file, reason: target.unreviewedReason ?? 'unreviewed' })) result.droppedNote = `${refuted.length} refuted by skeptics; ${belowThreshold.length} below threshold` + (thresholded.length - kept.length ? `; ${thresholded.length - kept.length} merged as duplicates` : '') + '.' diff --git a/agents/code-review/sources.ts b/agents/code-review/sources.ts index ca59910..d012b7e 100644 --- a/agents/code-review/sources.ts +++ b/agents/code-review/sources.ts @@ -1,140 +1,228 @@ import { execFile } from 'node:child_process' -import { readdirSync, readFileSync, statSync } from 'node:fs' -import { extname, join } from 'node:path' +import { closeSync, fstatSync, lstatSync, openSync, readdirSync, readFileSync, realpathSync } from 'node:fs' +import { extname, isAbsolute, join, relative } from 'node:path' import { promisify } from 'node:util' import type { ReviewTarget } from './agent.js' +import { redactSecrets } from '../../src/local-cli-process.js' const run = promisify(execFile) +const DEFAULT_SNAPSHOT_FILES = 100 +const DEFAULT_TOTAL_BYTES = 5 * 1024 * 1024 +const DEFAULT_PROMPT_FILE_BYTES = 256 * 1024 +const ABSOLUTE_SNAPSHOT_FILES = 500 +const ABSOLUTE_TOTAL_BYTES = 25 * 1024 * 1024 +const ABSOLUTE_PROMPT_FILE_BYTES = 1024 * 1024 -/** - * Source adapters normalize every input shape into the same `ReviewTarget[]` the - * pipeline reviews. This is deterministic orchestration code (git / fs / fetch) — the - * model only ever sees the normalized targets, never the raw ingestion. - */ +export type ContextMode = 'prompt' | 'isolated-snapshot' +export interface SourceLimits { + readonly maxFiles?: number + readonly maxBytes?: number + readonly maxFileBytes?: number +} export type SourceConfig = - | { kind: 'git-diff'; base: string; head?: string; cwd?: string } - | { kind: 'github-pr'; owner: string; repo: string; number: number; token: string } - | { kind: 'paths'; paths: string[]; cwd?: string } - | { kind: 'stdin'; content: string; filename?: string } + | { kind: 'git-diff'; base: string; head?: string; cwd?: string; redact?: boolean; limits?: SourceLimits } + | { kind: 'github-pr'; owner: string; repo: string; number: number; token: string; redact?: boolean; limits?: SourceLimits } + | { kind: 'paths'; paths: string[]; cwd?: string; redact?: boolean; limits?: SourceLimits } + | { kind: 'stdin'; content: string; filename?: string; redact?: boolean; limits?: SourceLimits } + | { kind: 'isolated-snapshot'; cwd: string; patterns: string[]; redact?: boolean; limits?: SourceLimits } const CODE_EXT = new Set([ - '.ts', '.tsx', '.js', '.jsx', '.mjs', '.cjs', '.py', '.go', '.rs', '.java', '.kt', - '.rb', '.php', '.cs', '.c', '.h', '.cpp', '.hpp', '.swift', '.scala', '.sql', '.sh', '.vue', '.svelte', + '.ts', '.tsx', '.js', '.jsx', '.mjs', '.cjs', '.py', '.go', '.rs', '.java', '.kt', '.rb', '.php', '.cs', '.c', '.h', '.cpp', '.hpp', + '.swift', '.scala', '.sql', '.sh', '.vue', '.svelte', '.json', '.jsonc', '.yaml', '.yml', '.toml', '.ini', '.cfg', '.conf', '.xml', + '.graphql', '.gql', '.tf', '.tfvars', '.hcl', ]) +const SPECIAL_FILES = new Set(['Dockerfile', 'Containerfile', 'Makefile', 'Jenkinsfile', 'Procfile', ' justfile '].map((name) => name.trim())) +const DENY_DIRS = new Set(['.git', 'node_modules', 'dist', 'build', 'coverage', '.next', 'out', 'vendor']) +const DENY_FILE = /^(?:\.env(?:\..*)?|credentials(?:\..*)?|secrets?(?:\..*)?|.*\.(?:key|pem|crt|cer|p12|pfx))$/i + +const langOf = (file: string): string => { + const base = file.split('/').pop() ?? file + if (SPECIAL_FILES.has(base) || base.startsWith('.github/')) return base.toLowerCase().includes('docker') ? 'dockerfile' : 'config' + return extname(file).replace('.', '') || 'text' +} -const langOf = (file: string): string => extname(file).replace('.', '') || 'text' +function normalize(file: string): string { return file.replaceAll('\\', '/') } + +function deniedPath(file: string): string | undefined { + const parts = normalize(file).split('/') + if (parts.some((part) => DENY_DIRS.has(part))) return 'sensitive or generated directory' + if (DENY_FILE.test(parts.at(-1) ?? '')) return 'sensitive file' + return undefined +} + +function isReviewableName(file: string): boolean { + const base = file.split('/').pop() ?? file + return SPECIAL_FILES.has(base) || CODE_EXT.has(extname(file).toLowerCase()) || normalize(file).startsWith('.github/workflows/') +} -/** Parse the `+a,b` hunk headers of a unified diff into 1-based line ranges. */ function changedRanges(patch: string): Array<{ start: number; end: number }> { const ranges: Array<{ start: number; end: number }> = [] for (const m of patch.matchAll(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/gm)) { - const start = Number(m[1]) - const count = m[2] === undefined ? 1 : Number(m[2]) + const start = Number(m[1]); const count = m[2] === undefined ? 1 : Number(m[2]) if (count > 0) ranges.push({ start, end: start + count - 1 }) } return ranges } +function unreviewed(file: string, reason: string): ReviewTarget { + return { file: normalize(file), language: langOf(file), fullContent: '', isChanged: true, reviewStatus: 'UNREVIEWED', unreviewedReason: reason } +} + +function readTarget(file: string, cwd: string, limits: SourceLimits, redact: boolean, changed?: ReviewTarget['changedRanges']): ReviewTarget { + const normalized = normalize(file) + const denied = deniedPath(normalized) + if (denied) return unreviewed(normalized, denied) + if (!isReviewableName(normalized)) return unreviewed(normalized, 'unsupported text format') + const abs = join(cwd, normalized) + let fd: number + try { fd = openSync(abs, 'r') } catch { return unreviewed(normalized, 'file unavailable') } + try { + const size = fstatSync(fd).size + const maxFileBytes = limits.maxFileBytes ?? DEFAULT_PROMPT_FILE_BYTES + if (size > maxFileBytes) return unreviewed(normalized, `file exceeds ${maxFileBytes} byte limit`) + const fullContent = readFileSync(fd, 'utf8') + if (fullContent.includes('\0')) return unreviewed(normalized, 'binary content') + return { file: normalized, language: langOf(normalized), fullContent: redact ? redactSecrets(fullContent) : fullContent, changedRanges: changed, isChanged: Boolean(changed) } + } catch { return unreviewed(normalized, 'file is not readable text') + } finally { closeSync(fd) } +} + +function withinRoot(root: string, candidate: string): boolean { + const rel = relative(root, candidate) + return rel === '' || (!rel.startsWith('..') && !isAbsolute(rel)) +} + +function globRegex(pattern: string): RegExp { + let out = '^' + for (let i = 0; i < pattern.length; i++) { + const char = pattern[i]! + if (char === '*' && pattern[i + 1] === '*') { + if (pattern[i + 2] === '/') { out += '(?:.*/)?'; i += 2; continue } + out += '.*'; i++; continue + } + if (char === '*') { out += '[^/]*'; continue } + if (char === '?') { out += '[^/]'; continue } + out += /[.+^${}()|[\]\\]/.test(char) ? `\\${char}` : char + } + return new RegExp(`${out}$`) +} + +function matchesAny(file: string, patterns: readonly string[]): boolean { + return patterns.some((pattern) => globRegex(normalize(pattern.replace(/^!/, ''))).test(file)) +} + +function validatePatterns(patterns: readonly string[]): void { + if (!patterns.length) throw new Error('isolated-snapshot needs at least one context pattern') + for (const pattern of patterns) { + const value = pattern.replace(/^!/, '') + if (!value || value.startsWith('/') || value.startsWith('\\') || /^[A-Za-z]:[\\/]/.test(value) || value.split('/').includes('..')) { + throw new Error(`invalid context pattern "${pattern}": use a repository-relative pattern without .. traversal`) + } + } +} + +function walkFiles(root: string, current: string, out: string[], unreviewedFiles: ReviewTarget[]): void { + for (const entry of readdirSync(current)) { + const abs = join(current, entry) + const rel = normalize(relative(root, abs)) + if (DENY_DIRS.has(entry)) continue + let stat + try { stat = lstatSync(abs) } catch { continue } + if (stat.isSymbolicLink()) { + let target: string + try { target = realpathSync(abs) } catch { unreviewedFiles.push(unreviewed(rel, 'broken symlink')); continue } + if (!withinRoot(root, target)) unreviewedFiles.push(unreviewed(rel, 'symlink escapes repository root')) + continue + } + if (stat.isDirectory()) walkFiles(root, abs, out, unreviewedFiles) + else out.push(rel) + } +} + async function fromGitDiff(c: Extract): Promise { - const cwd = c.cwd ?? process.cwd() - const head = c.head ?? 'HEAD' + const cwd = c.cwd ?? process.cwd(); const head = c.head ?? 'HEAD' const git = async (args: string[]) => (await run('git', ['-C', cwd, ...args], { maxBuffer: 64 * 1024 * 1024 })).stdout const diff = await git(['diff', '--unified=0', `${c.base}...${head}`]) - const targets: ReviewTarget[] = [] - // Split the combined diff into per-file blocks. for (const block of diff.split(/^diff --git /m).slice(1)) { - const pathMatch = block.match(/^a\/(.+?) b\/(.+)$/m) - const file = pathMatch?.[2] + const pathMatch = block.match(/^a\/(.+?) b\/(.+)$/m); const file = pathMatch?.[2] if (!file || block.includes('\ndeleted file mode')) continue - if (!CODE_EXT.has(extname(file))) continue - let fullContent: string - try { - fullContent = await git(['show', `${head}:${file}`]).catch(() => readFileSync(join(cwd, file), 'utf8')) - } catch (error) { - const detail = error instanceof Error ? error.message.split('\n')[0] : String(error) - throw new Error(`Failed to load review target ${file}: ${detail}`) - } - targets.push({ file, language: langOf(file), fullContent, changedRanges: changedRanges(block), isChanged: true }) + const target = readTarget(file, cwd, { maxFileBytes: c.limits?.maxFileBytes }, Boolean(c.redact), changedRanges(block)) + targets.push(target) } return targets } async function fromGithubPr(c: Extract): Promise { const api = async (path: string): Promise => { - const res = await fetch(`https://api.github.com${path}`, { - headers: { authorization: `Bearer ${c.token}`, accept: 'application/vnd.github+json', 'user-agent': 'agentskit-code-review' }, - }) + const res = await fetch(`https://api.github.com${path}`, { headers: { authorization: `Bearer ${c.token}`, accept: 'application/vnd.github+json', 'user-agent': 'agentskit-code-review' } }) if (!res.ok) throw new Error(`GitHub ${path} → ${res.status}`) return res.json() as Promise } - const pr = await api<{ head: { sha: string } }>(`/repos/${c.owner}/${c.repo}/pulls/${c.number}`) - const sha = pr.head.sha + const pr = await api<{ head: { sha: string } }>(`/repos/${c.owner}/${c.repo}/pulls/${c.number}`); const sha = pr.head.sha const files: Array<{ filename: string; patch?: string; status: string }> = [] - for (let page = 1; ; page++) { - const batch = await api(`/repos/${c.owner}/${c.repo}/pulls/${c.number}/files?per_page=100&page=${page}`) - files.push(...batch) - if (batch.length < 100) break - } + for (let page = 1; ; page++) { const batch = await api(`/repos/${c.owner}/${c.repo}/pulls/${c.number}/files?per_page=100&page=${page}`); files.push(...batch); if (batch.length < 100) break } const targets: ReviewTarget[] = [] for (const f of files) { - if (f.status === 'removed' || !CODE_EXT.has(extname(f.filename))) continue - const content = await api<{ content: string; encoding: string }>( - `/repos/${c.owner}/${c.repo}/contents/${encodeURIComponent(f.filename)}?ref=${sha}`, - ) - const fullContent = Buffer.from(content.content, content.encoding as BufferEncoding).toString('utf8') - targets.push({ - file: f.filename, - language: langOf(f.filename), - fullContent, - changedRanges: f.patch ? changedRanges(f.patch) : [], - isChanged: true, - commitId: sha, - }) + if (f.status === 'removed') continue + const denied = deniedPath(f.filename) + if (denied || !isReviewableName(f.filename)) { targets.push(unreviewed(f.filename, denied ?? 'unsupported text format')); continue } + const content = await api<{ content: string; encoding: string }>(`/repos/${c.owner}/${c.repo}/contents/${encodeURIComponent(f.filename)}?ref=${sha}`) + const raw = Buffer.from(content.content, content.encoding as BufferEncoding).toString('utf8') + const size = Buffer.byteLength(raw, 'utf8'); const limit = c.limits?.maxFileBytes ?? DEFAULT_PROMPT_FILE_BYTES + if (size > limit || raw.includes('\0')) { targets.push(unreviewed(f.filename, size > limit ? `file exceeds ${limit} byte limit` : 'binary content')); continue } + targets.push({ file: f.filename, language: langOf(f.filename), fullContent: c.redact ? redactSecrets(raw) : raw, changedRanges: f.patch ? changedRanges(f.patch) : [], isChanged: true, commitId: sha }) } return targets } -function walk(root: string, cwd: string, out: string[]): void { - const abs = join(cwd, root) - const st = statSync(abs) - if (st.isFile()) { - if (CODE_EXT.has(extname(root))) out.push(root) - return - } - for (const entry of readdirSync(abs)) { - if (entry === 'node_modules' || entry === '.git' || entry === 'dist') continue - walk(join(root, entry), cwd, out) +function fromPaths(c: Extract): ReviewTarget[] { + const cwd = c.cwd ?? process.cwd(); const files: string[] = []; const skipped: ReviewTarget[] = [] + for (const p of c.paths) { + const abs = join(cwd, p); const stat = lstatSync(abs) + if (stat.isSymbolicLink()) { + try { + if (!withinRoot(realpathSync(cwd), realpathSync(abs))) skipped.push(unreviewed(p, 'symlink escapes repository root')) + else files.push(normalize(p)) + } catch { skipped.push(unreviewed(p, 'broken symlink')) } + continue + } + if (stat.isDirectory()) walkFiles(cwd, abs, files, skipped) + else files.push(normalize(p)) } + return [...skipped, ...files.map((file) => readTarget(file, cwd, { maxFileBytes: c.limits?.maxFileBytes }, Boolean(c.redact)))] } -function fromPaths(c: Extract): ReviewTarget[] { - const cwd = c.cwd ?? process.cwd() - const files: string[] = [] - for (const p of c.paths) walk(p, cwd, files) - return files.map((file) => ({ - file, - language: langOf(file), - fullContent: readFileSync(join(cwd, file), 'utf8'), - isChanged: false, - })) +function fromSnapshot(c: Extract): ReviewTarget[] { + validatePatterns(c.patterns) + const root = realpathSync(c.cwd); const files: string[] = []; const skipped: ReviewTarget[] = [] + walkFiles(root, root, files, skipped) + const includes = c.patterns.filter((pattern) => !pattern.startsWith('!')); const excludes = c.patterns.filter((pattern) => pattern.startsWith('!')) + const selected = files.filter((file) => matchesAny(file, includes) && !matchesAny(file, excludes)).sort() + const maxFiles = Math.min(c.limits?.maxFiles ?? DEFAULT_SNAPSHOT_FILES, ABSOLUTE_SNAPSHOT_FILES) + const targets = selected.slice(0, maxFiles).map((file) => readTarget(file, root, { maxFileBytes: c.limits?.maxFileBytes ?? ABSOLUTE_PROMPT_FILE_BYTES }, Boolean(c.redact))) + for (const file of selected.slice(maxFiles)) skipped.push(unreviewed(file, `snapshot exceeds ${maxFiles} file limit`)) + const maxBytes = Math.min(c.limits?.maxBytes ?? DEFAULT_TOTAL_BYTES, ABSOLUTE_TOTAL_BYTES); let total = 0 + for (const target of targets) { + total += Buffer.byteLength(target.fullContent, 'utf8') + if (total > maxBytes && target.reviewStatus !== 'UNREVIEWED') { target.fullContent = ''; target.reviewStatus = 'UNREVIEWED'; target.unreviewedReason = `snapshot exceeds ${maxBytes} byte limit` } + } + return [...skipped, ...targets] } function fromStdin(c: Extract): ReviewTarget[] { - const file = c.filename ?? 'snippet.txt' - return [{ file, language: langOf(file), fullContent: c.content, isChanged: true }] + const file = c.filename ?? 'snippet.txt'; const content = c.redact ? redactSecrets(c.content) : c.content + const size = Buffer.byteLength(content, 'utf8'); const limit = c.limits?.maxFileBytes ?? DEFAULT_PROMPT_FILE_BYTES + return [size > limit ? unreviewed(file, `file exceeds ${limit} byte limit`) : { file, language: langOf(file), fullContent: content, isChanged: true }] } export async function loadTargets(source: SourceConfig): Promise { switch (source.kind) { - case 'git-diff': - return fromGitDiff(source) - case 'github-pr': - return fromGithubPr(source) - case 'paths': - return fromPaths(source) - case 'stdin': - return fromStdin(source) + case 'git-diff': return fromGitDiff(source) + case 'github-pr': return fromGithubPr(source) + case 'paths': return fromPaths(source) + case 'stdin': return fromStdin(source) + case 'isolated-snapshot': return fromSnapshot(source) } } diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index 2036bc4..e79c2cd 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -57,6 +57,18 @@ request and diagnostics do not print config values. Keep policy-only configuration in the file. Use trusted workflow flags or the runner environment for provider selection, credentials, and execution mode. +`prompt` is the default context mode. To review an explicit repository snapshot, +set `context.mode` to `isolated-snapshot` and provide repository-relative +patterns such as `src/**` or `!src/generated/**`. Sensitive directories/files, +symlink escapes, binaries, and over-limit inputs are excluded and shown as +`UNREVIEWED`. The default snapshot ceiling is 100 files/5 MiB; the absolute +ceiling is 500 files/25 MiB. Prompt files default to 256 KiB with a 1 MiB +absolute per-file ceiling. + +Remote and unknown provider boundaries receive high-confidence credential +redaction while preserving file and line context. `--allow-unredacted` is a +local-only escape hatch and is rejected in CI; never use it for untrusted code. + ## Local Ollama review Ollama serves its local API at `http://localhost:11434` by default. Verify the service without sending repository content: diff --git a/llms-full.txt b/llms-full.txt index 8fc81ba..52f0a67 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -277,6 +277,7 @@ In shortened examples, replace `...` with `npx --yes github:AgentsKit-io/code-re | `--no-fail` | Keep findings advisory | | `--conventions ` | Inject project conventions | | `--allow-incomplete` | Local-only exception for a config that declares incomplete lens coverage | +| `--allow-unredacted` | Local-only exception; rejected in CI | | `--api` | Back-compatible alias for `--provider anthropic` | | `doctor --provider ` | Offline provider diagnostics; no model request | | `doctor --live` | Explicit provider smoke-test mode | @@ -313,6 +314,9 @@ never accepted in CI. Provider, model, transport, context trust, redaction, and permissions are trusted execution inputs; a project config cannot set them in CI. Put provider credentials only in the environment or provider login, never in this file. +Remote and unknown provider boundaries redact high-confidence credential +patterns before the model sees source. Unsafe, oversized, binary, or excluded +paths are reported as `UNREVIEWED`; content is never silently truncated. ### Doctor @@ -447,6 +451,18 @@ request and diagnostics do not print config values. Keep policy-only configuration in the file. Use trusted workflow flags or the runner environment for provider selection, credentials, and execution mode. +`prompt` is the default context mode. To review an explicit repository snapshot, +set `context.mode` to `isolated-snapshot` and provide repository-relative +patterns such as `src/**` or `!src/generated/**`. Sensitive directories/files, +symlink escapes, binaries, and over-limit inputs are excluded and shown as +`UNREVIEWED`. The default snapshot ceiling is 100 files/5 MiB; the absolute +ceiling is 500 files/25 MiB. Prompt files default to 256 KiB with a 1 MiB +absolute per-file ceiling. + +Remote and unknown provider boundaries receive high-confidence credential +redaction while preserving file and line context. `--allow-unredacted` is a +local-only escape hatch and is rejected in CI; never use it for untrusted code. + ## Local Ollama review Ollama serves its local API at `http://localhost:11434` by default. Verify the service without sending repository content: @@ -799,6 +815,7 @@ All notable changes will be documented here. This project follows Semantic Versi - Added strict versioned `.agentskit-review.json` policy with lens coverage, budgets, thresholds, context, and safe CI precedence; incomplete profiles require explicit local opt-in. - Hardened the shared local CLI worker with cancellation, process-tree cleanup, isolated temporary environments, bounded output, and redacted diagnostics. +- Added bounded source snapshots with infrastructure/configuration file support, denylisted sensitive paths, symlink checks, input limits, and data-boundary-aware secret redaction. - Made reviews fail closed when any reviewable file has no successful primary lens or cannot be ingested; advisory mode now suppresses finding-based failures only, never source/provider/execution failures. - Added primary-lens execution coverage to review summaries so partial provider degradation is visible. - Repositioned the CLI and GitHub Action as provider-neutral. diff --git a/readme-standard-v1.json b/readme-standard-v1.json index 0c40267..81a5243 100644 --- a/readme-standard-v1.json +++ b/readme-standard-v1.json @@ -221,7 +221,7 @@ "docs/OPERATIONS.md", "test/cli-smoke.test.mjs" ], - "sourceHash": "sha256:23a5ee0d6e4a39e12a8fa1e0b05df4f470b2dd2904968ce96b45894e0263f7e9" + "sourceHash": "sha256:608f0984d346b0defcbfb81efd7b962fdba0bdb45a4be7c52ed95de3bea7fc0b" }, "exceptions": [] } diff --git a/src/cli.ts b/src/cli.ts index ad86c4d..fd5eb9f 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -55,6 +55,7 @@ Review options: --concurrency Parallel model calls (default: 4) --conventions Project conventions file --allow-incomplete Local-only exception for an explicitly incomplete profile + --allow-unredacted Local-only exception for secret redaction --validate-patch Validate suggested patches with git apply --check --sarif Also write a SARIF report --post Post a PR review (with --pr) @@ -93,24 +94,33 @@ function readStdin(): Promise { }) } -async function resolveSource(): Promise { +function shouldRedact(reviewConfig: ResolvedReviewConfig): boolean { + const provider = reviewConfig.provider && resolveProviderId(reviewConfig.provider) + const boundary = provider && providerEntry(provider)?.dataBoundary + return (boundary === 'remote' || boundary === 'unknown') && !reviewConfig.allowUnredacted +} + +async function resolveSource(reviewConfig: ResolvedReviewConfig): Promise { + const redact = shouldRedact(reviewConfig) + const limits = { maxFiles: reviewConfig.budget.maxFiles, maxBytes: reviewConfig.budget.maxBytes } const pr = flag('pr') if (pr) { const m = pr.match(/^([^/]+)\/([^#]+)#(\d+)$/) if (!m) throw new Error('--pr must be owner/repo#number') const token = process.env.GITHUB_TOKEN if (!token) throw new Error('--pr needs GITHUB_TOKEN') - return { kind: 'github-pr', owner: m[1]!, repo: m[2]!, number: Number(m[3]), token } + return { kind: 'github-pr', owner: m[1]!, repo: m[2]!, number: Number(m[3]), token, redact, limits } } - if (has('stdin')) return { kind: 'stdin', content: await readStdin(), filename: `snippet.${flag('lang') ?? 'txt'}` } + if (has('stdin')) return { kind: 'stdin', content: await readStdin(), filename: `snippet.${flag('lang') ?? 'txt'}`, redact, limits: { ...limits, maxFileBytes: 1024 * 1024 } } if (has('paths')) { const i = process.argv.indexOf('--paths') const paths: string[] = [] for (let j = i + 1; j < process.argv.length && !process.argv[j]!.startsWith('--'); j++) paths.push(process.argv[j]!) if (!paths.length) throw new Error('--paths needs at least one file/dir') - return { kind: 'paths', paths, cwd: process.cwd() } + return { kind: 'paths', paths, cwd: process.cwd(), redact, limits: { ...limits, maxFileBytes: 1024 * 1024 } } } - return { kind: 'git-diff', base: flag('base') ?? 'origin/main', cwd: process.cwd() } + if (reviewConfig.context.mode === 'isolated-snapshot') return { kind: 'isolated-snapshot', cwd: process.cwd(), patterns: reviewConfig.context.patterns, redact, limits: { ...limits, maxFileBytes: 1024 * 1024 } } + return { kind: 'git-diff', base: flag('base') ?? 'origin/main', cwd: process.cwd(), redact, limits } } async function main() { @@ -129,6 +139,7 @@ async function main() { const reviewConfig = loadReviewConfig(process.cwd(), { ci: has('ci') || process.env.CI === 'true' || process.env.CI === '1', allowIncomplete: has('allow-incomplete'), + allowUnredacted: has('allow-unredacted'), overrides: { provider: flag('provider') ?? (has('api') ? 'anthropic' : undefined), model: flag('model'), @@ -141,7 +152,7 @@ async function main() { conventions: flag('conventions'), }, }) - const source = await resolveSource() + const source = await resolveSource(reviewConfig) await preflightProvider(reviewConfig) const adapter = buildAdapter(reviewConfig) diff --git a/src/local-cli-process.ts b/src/local-cli-process.ts index 40bce08..a5b026f 100644 --- a/src/local-cli-process.ts +++ b/src/local-cli-process.ts @@ -31,11 +31,16 @@ const SECRET_PATTERNS = [ /-----BEGIN [^-]*PRIVATE KEY-----[\s\S]*?-----END [^-]*PRIVATE KEY-----/g, ] -export function redactDiagnostic(value: string, secrets: readonly string[] = []): string { +export function redactSecrets(value: string, secrets: readonly string[] = []): string { let redacted = value for (const secret of secrets.filter(Boolean)) redacted = redacted.split(secret).join('[REDACTED]') for (const pattern of SECRET_PATTERNS) redacted = redacted.replace(pattern, '[REDACTED]') - return redacted.slice(0, 4000) + redacted = redacted.replace(/((?:api[_-]?key|secret|token|password)\s*[:=]\s*["']?)[A-Za-z0-9._~+/=-]{12,}/gi, '$1[REDACTED]') + return redacted +} + +export function redactDiagnostic(value: string, secrets: readonly string[] = []): string { + return redactSecrets(value, secrets).slice(0, 4000) } function terminateProcessTree(child: ChildProcess): void { diff --git a/src/review-config.ts b/src/review-config.ts index f60f723..666d0b8 100644 --- a/src/review-config.ts +++ b/src/review-config.ts @@ -72,6 +72,7 @@ export interface ResolvedReviewConfig { worker: { timeoutMs: number; maxOutputBytes: number } conventions?: string context: { mode: 'prompt' | 'isolated-snapshot'; patterns: string[] } + allowUnredacted: boolean provider?: string; model?: string; transport?: 'api' | 'acp' | 'headless' | 'http' trustMode: 'isolated'; redaction: 'required' | 'high-confidence' permissions: { tools?: boolean; write?: boolean; shell?: boolean; mcp?: boolean } @@ -105,14 +106,15 @@ function parseFile(raw: unknown): FileConfig { export function resolveReviewConfig( fileConfig: unknown | undefined, - options: { ci?: boolean; allowIncomplete?: boolean; overrides?: ReviewConfigOverrides } = {}, + options: { ci?: boolean; allowIncomplete?: boolean; allowUnredacted?: boolean; overrides?: ReviewConfigOverrides } = {}, ): ResolvedReviewConfig { const file = fileConfig === undefined ? undefined : parseFile(fileConfig) const overrides = options.overrides ?? {} if (file) { - const restricted = TRUSTED_ONLY_KEYS.filter((key) => file[key] !== undefined) + const restricted = TRUSTED_ONLY_KEYS.filter((key) => key !== 'context' || file.context?.mode !== 'isolated-snapshot').filter((key) => file[key] !== undefined) if (options.ci && restricted.length) throw new ReviewConfigError(`project config cannot set trusted execution inputs in CI: ${restricted.join(', ')}`) if (file.trustMode === 'trusted-local') throw new ReviewConfigError('project config cannot enable trusted-local mode; use an explicit trusted CLI invocation') + if (file.context?.mode === 'isolated-snapshot' && !file.context.patterns?.length) throw new ReviewConfigError('isolated-snapshot requires at least one context pattern') } const lenses = Object.fromEntries(BUILTIN_LENS_KEYS.map((key) => [key, { ...DEFAULT_LENSES[key], ...(file?.lenses?.[key] ?? {}) }])) as Record @@ -120,6 +122,7 @@ export function resolveReviewConfig( if (impossibleRequired.length && !file?.incompleteProfile) throw new ReviewConfigError(`required lens cannot be disabled without incompleteProfile: ${impossibleRequired.join(', ')}`) const incompleteProfile = Boolean(file?.incompleteProfile || impossibleRequired.length) if (options.ci && (incompleteProfile || options.allowIncomplete)) throw new ReviewConfigError('--allow-incomplete and incomplete profiles are local-only; CI cannot approve an incomplete review') + if (options.ci && options.allowUnredacted) throw new ReviewConfigError('--allow-unredacted is local-only and cannot disable CI redaction') if (incompleteProfile && !options.allowIncomplete) throw new ReviewConfigError('incomplete profile requires explicit --allow-incomplete for a local run') const thresholds = { ...file?.thresholds, ...(overrides.minSeverity === undefined ? {} : { minSeverity: overrides.minSeverity }), ...(overrides.minConfidence === undefined ? {} : { minConfidence: overrides.minConfidence }) } @@ -131,6 +134,7 @@ export function resolveReviewConfig( worker: { timeoutMs: file?.worker?.timeoutMs ?? localCliTimeoutMs(), maxOutputBytes: file?.worker?.maxOutputBytes ?? DEFAULT_LOCAL_CLI_OUTPUT_BYTES }, conventions: overrides.conventions ?? file?.conventions, context: { mode: file?.context?.mode ?? 'prompt', patterns: file?.context?.patterns ?? [] }, + allowUnredacted: Boolean(options.allowUnredacted), provider: overrides.provider ?? file?.provider, model: overrides.model ?? file?.model, transport: (overrides.transport ?? file?.transport) as ResolvedReviewConfig['transport'], trustMode: 'isolated' as const, redaction: file?.redaction ?? 'required', permissions: file?.permissions ?? {}, @@ -146,7 +150,7 @@ export function resolveReviewConfig( export function loadReviewConfig( cwd: string, - options: { ci?: boolean; allowIncomplete?: boolean; overrides?: ReviewConfigOverrides } = {}, + options: { ci?: boolean; allowIncomplete?: boolean; allowUnredacted?: boolean; overrides?: ReviewConfigOverrides } = {}, ): ResolvedReviewConfig { const file = join(cwd, '.agentskit-review.json') if (!existsSync(file)) return resolveReviewConfig(undefined, options) diff --git a/test/review-config.test.mjs b/test/review-config.test.mjs index c8928ad..6ad65e1 100644 --- a/test/review-config.test.mjs +++ b/test/review-config.test.mjs @@ -56,6 +56,8 @@ test('CI cannot accept trusted execution inputs or secrets from the project file assert.throws(() => resolveReviewConfig({ configVersion: 1, provider: 'openai' }, { ci: true }), /provider/) assert.throws(() => resolveReviewConfig({ configVersion: 1, apiKey: 'secret-value' }), /apiKey/) assert.doesNotThrow(() => resolveReviewConfig({ configVersion: 1, context: { mode: 'prompt' } })) + assert.doesNotThrow(() => resolveReviewConfig({ configVersion: 1, context: { mode: 'isolated-snapshot', patterns: ['src/**'] } }, { ci: true })) + assert.throws(() => resolveReviewConfig({ configVersion: 1, context: { mode: 'isolated-snapshot', patterns: ['../**'] } }), /repository-relative/) }) test('invalid config exits 2 before provider execution and does not echo secret values', () => { diff --git a/test/sources-context.test.mjs b/test/sources-context.test.mjs new file mode 100644 index 0000000..0f04510 --- /dev/null +++ b/test/sources-context.test.mjs @@ -0,0 +1,52 @@ +import assert from 'node:assert/strict' +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import test from 'node:test' +import { loadTargets } from '../dist/agents/code-review/sources.js' + +function fixtureRepo() { + const root = mkdtempSync(join(tmpdir(), 'agentskit-review-source-')) + mkdirSync(join(root, 'src'), { recursive: true }) + mkdirSync(join(root, '.github/workflows'), { recursive: true }) + mkdirSync(join(root, 'node_modules/ignored'), { recursive: true }) + writeFileSync(join(root, 'src/app.ts'), 'const apiKey = "sk-1234567890123456"\n') + writeFileSync(join(root, 'Dockerfile'), 'FROM node:22\n') + writeFileSync(join(root, 'Makefile'), 'test:\n\tnpm test\n') + writeFileSync(join(root, '.github/workflows/ci'), 'name: CI\n') + writeFileSync(join(root, '.env'), 'API_KEY=never-read\n') + writeFileSync(join(root, 'node_modules/ignored/index.ts'), 'const ignored = true\n') + const outside = mkdtempSync(join(tmpdir(), 'agentskit-review-outside-')) + writeFileSync(join(outside, 'secret.ts'), 'const outside = true\n') + symlinkSync(join(outside, 'secret.ts'), join(root, 'src/outside-link.ts')) + return { root, outside } +} + +test('isolated snapshots select deterministic code/config files and redact secrets', async () => { + const { root, outside } = fixtureRepo() + try { + const targets = await loadTargets({ kind: 'isolated-snapshot', cwd: root, patterns: ['**/*'], redact: true }) + const selected = targets.filter((target) => target.reviewStatus !== 'UNREVIEWED') + assert.deepEqual(selected.map((target) => target.file), ['.github/workflows/ci', 'Dockerfile', 'Makefile', 'src/app.ts']) + assert.equal(selected.find((target) => target.file === 'Dockerfile')?.language, 'dockerfile') + assert.match(selected.find((target) => target.file === 'src/app.ts')?.fullContent ?? '', /\[REDACTED\]/) + assert.doesNotMatch(selected.find((target) => target.file === 'src/app.ts')?.fullContent ?? '', /sk-1234567890123456/) + assert.ok(targets.some((target) => target.file === '.env' && target.unreviewedReason === 'sensitive file')) + assert.ok(targets.some((target) => target.file === 'src/outside-link.ts' && target.unreviewedReason.includes('symlink'))) + } finally { + rmSync(root, { recursive: true, force: true }) + rmSync(outside, { recursive: true, force: true }) + } +}) + +test('snapshot file and byte ceilings mark excess input UNREVIEWED', async () => { + const root = mkdtempSync(join(tmpdir(), 'agentskit-review-limits-')) + try { + mkdirSync(join(root, 'src')) + writeFileSync(join(root, 'src/a.ts'), 'a'.repeat(20)) + writeFileSync(join(root, 'src/b.ts'), 'b'.repeat(20)) + const targets = await loadTargets({ kind: 'isolated-snapshot', cwd: root, patterns: ['src/**'], limits: { maxFiles: 1, maxBytes: 10 }, redact: false }) + assert.equal(targets.filter((target) => target.reviewStatus !== 'UNREVIEWED').length, 0) + assert.ok(targets.some((target) => target.unreviewedReason.includes('file limit'))) + } finally { rmSync(root, { recursive: true, force: true }) } +})