diff --git a/README.md b/README.md index 4d5bac6..61353d0 100644 --- a/README.md +++ b/README.md @@ -402,11 +402,13 @@ ALTER TABLE users DROP CONSTRAINT chk_email_verified; ### GitHub Actions +Use a concrete migration path here. The Action does not shell-expand globs, so `migrations/*.sql` belongs in a `run:` step instead. + ```yaml - name: Check migration safety uses: flvmnt/pgfence@v1 with: - path: migrations/*.sql + path: migrations/add-users.sql max-risk: medium ``` @@ -415,7 +417,7 @@ ALTER TABLE users DROP CONSTRAINT chk_email_verified; ```yaml - name: Analyze migrations run: | - npx pgfence analyze --output github migrations/*.sql > pgfence-report.md + npx @flvmnt/pgfence analyze --output github migrations/*.sql > pgfence-report.md - name: Comment on PR uses: marocchino/sticky-pull-request-comment@v2 with: diff --git a/action.yml b/action.yml index fc090d2..0ce4b1c 100644 --- a/action.yml +++ b/action.yml @@ -38,7 +38,14 @@ runs: INPUT_DB_URL: ${{ inputs.db-url }} INPUT_STATS_FILE: ${{ inputs.stats-file }} run: | - ARGS=("$INPUT_PATH" "--ci" "--max-risk" "$INPUT_MAX_RISK") + shopt -s nullglob + FILES=($INPUT_PATH) + if [ "${#FILES[@]}" -eq 0 ]; then + echo "No files matched path: $INPUT_PATH" + exit 1 + fi + + ARGS=("${FILES[@]}" "--ci" "--max-risk" "$INPUT_MAX_RISK") if [ "$INPUT_FORMAT" != "auto" ]; then ARGS+=("--format" "$INPUT_FORMAT") fi diff --git a/package.json b/package.json index 842f6fc..52e01c5 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,9 @@ }, "scripts": { "dev": "tsx watch src/index.ts", - "build": "tsc", + "clean": "node -e \"require('node:fs').rmSync('dist', { recursive: true, force: true })\"", + "build": "pnpm clean && tsc", + "prepack": "pnpm build", "test": "vitest run tests/", "test:watch": "vitest tests/", "lint": "eslint src/ tests/", diff --git a/scripts/check-public-boundaries.sh b/scripts/check-public-boundaries.sh index 2b9aafe..029bfa5 100755 --- a/scripts/check-public-boundaries.sh +++ b/scripts/check-public-boundaries.sh @@ -39,7 +39,15 @@ fi PACK_JSON=$(npm pack --dry-run --json) PACK_JSON="$PACK_JSON" node <<'NODE' -const pack = JSON.parse(process.env.PACK_JSON ?? '[]'); +function parsePackJson(raw) { + const jsonStart = raw.search(/\[\s*{/s); + if (jsonStart === -1) { + throw new Error('npm pack --json did not emit a JSON payload'); + } + return JSON.parse(raw.slice(jsonStart)); +} + +const pack = parsePackJson(process.env.PACK_JSON ?? '[]'); const files = pack.flatMap((entry) => entry.files ?? []).map((file) => file.path); const forbidden = files.filter((file) => /^(src\/cloud\/|src\/agent\/|dist\/cloud\/|dist\/agent\/|tests\/cloud\/)/.test(file)); if (forbidden.length > 0) { @@ -55,6 +63,31 @@ if (missing.length > 0) { for (const file of missing) console.error(file); process.exit(1); } + +const { execSync } = require('node:child_process'); +const trackedSources = new Set( + execSync('git ls-files src', { encoding: 'utf8' }) + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.endsWith('.ts')), +); + +const distArtifacts = files.filter((file) => /^dist\/.+\.(?:js|js\.map|d\.ts|d\.ts\.map)$/.test(file)); +const orphanArtifacts = distArtifacts.filter((file) => { + const sourcePath = file + .replace(/^dist\//, 'src/') + .replace(/\.d\.ts\.map$/, '.ts') + .replace(/\.d\.ts$/, '.ts') + .replace(/\.js\.map$/, '.ts') + .replace(/\.js$/, '.ts'); + return !trackedSources.has(sourcePath); +}); + +if (orphanArtifacts.length > 0) { + console.error('ERROR: npm package contains dist artifacts without tracked source files:'); + for (const file of orphanArtifacts) console.error(file); + process.exit(1); +} NODE echo "Public repo boundaries look good." diff --git a/src/analyzer.ts b/src/analyzer.ts index 0041dbc..83974b0 100644 --- a/src/analyzer.ts +++ b/src/analyzer.ts @@ -285,7 +285,7 @@ export function applyRules( results.push(...checkBestPractices(stmt)); results.push(...checkPreferRobustStmts(stmt)); results.push(...checkAlterEnum(stmt, config)); - results.push(...checkReindex(stmt)); + results.push(...checkReindex(stmt, config.minPostgresVersion)); results.push(...checkRefreshMatView(stmt)); results.push(...checkTrigger(stmt)); results.push(...checkPartition(stmt, config)); diff --git a/src/extractors/typeorm.ts b/src/extractors/typeorm.ts index 7b4fc17..05124fc 100644 --- a/src/extractors/typeorm.ts +++ b/src/extractors/typeorm.ts @@ -111,6 +111,7 @@ export async function extractTypeORMSQLFromSource( line: loc.line, column: loc.column, message: `TypeORM builder API detected (${upInfo.paramName}.${methodName}) -- pgfence can only analyze ${upInfo.paramName}.query() raw SQL calls`, + unanalyzable: true, }); return; } diff --git a/src/init.ts b/src/init.ts index b90c353..294f91a 100644 --- a/src/init.ts +++ b/src/init.ts @@ -1,6 +1,10 @@ import { existsSync } from 'node:fs'; -import { mkdir, writeFile, chmod } from 'node:fs/promises'; -import { join } from 'node:path'; +import { chmod, mkdir, readFile, writeFile } from 'node:fs/promises'; +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; +import { join, resolve } from 'node:path'; + +const execFileAsync = promisify(execFile); const PRE_COMMIT_HOOK_CONTENT = `#!/bin/sh # pgfence pre-commit hook @@ -25,50 +29,54 @@ fi `; export async function installHooks(): Promise { - const cwd = process.cwd(); - const huskyPath = join(cwd, '.husky'); - const gitHooksPath = join(cwd, '.git', 'hooks'); + const cwd = process.cwd(); + const huskyPath = join(cwd, '.husky'); - let targetDir: string; - let usingHusky = false; + let targetDir: string; + let usingHusky = false; - if (existsSync(huskyPath)) { - targetDir = huskyPath; - usingHusky = true; - } else if (existsSync(join(cwd, '.git'))) { - if (!existsSync(gitHooksPath)) { - await mkdir(gitHooksPath, { recursive: true }); - } - targetDir = gitHooksPath; - } else { - throw new Error('Neither .husky nor .git directory found. Are you in a git repository?'); + if (existsSync(huskyPath)) { + targetDir = huskyPath; + usingHusky = true; + } else { + try { + const { stdout } = await execFileAsync('git', ['rev-parse', '--git-path', 'hooks'], { cwd }); + targetDir = resolve(cwd, stdout.trim()); + if (!targetDir) { + throw new Error('git did not return a hooks directory'); + } + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error(`Neither .husky nor a git hooks directory could be resolved. Are you in a git repository? ${message}`); } + } - const hookFile = join(targetDir, 'pre-commit'); + const hookFile = join(targetDir, 'pre-commit'); - let existingContent = ''; - try { - const { readFile } = await import('node:fs/promises'); - existingContent = await readFile(hookFile, 'utf8'); - } catch (err: unknown) { - const error = err as { code?: string }; - if (error.code !== 'ENOENT') throw err; - } + let existingContent = ''; + try { + existingContent = await readFile(hookFile, 'utf8'); + } catch (err: unknown) { + const error = err as { code?: string }; + if (error.code !== 'ENOENT') throw err; + } - if (existingContent.includes('pgfence analyze')) { - console.log(`✅ pgfence pre-commit hook is already installed in ${targetDir}`); - return; - } + if (existingContent.includes('pgfence analyze')) { + console.log(`✅ pgfence pre-commit hook is already installed in ${targetDir}`); + return; + } - const newContent = existingContent - ? existingContent + '\n' + PRE_COMMIT_HOOK_CONTENT.replace('#!/bin/sh\n', '') - : PRE_COMMIT_HOOK_CONTENT; + const existingBody = existingContent.replace(/^#![^\n]*\n?/, ''); + const newContent = existingBody + ? `${PRE_COMMIT_HOOK_CONTENT}\n# Existing pre-commit hook preserved below\n${existingBody}` + : PRE_COMMIT_HOOK_CONTENT; - await writeFile(hookFile, newContent); - await chmod(hookFile, '755'); + await mkdir(targetDir, { recursive: true }); + await writeFile(hookFile, newContent); + await chmod(hookFile, '755'); - console.log(`✅ pgfence pre-commit hook installed successfully in ${targetDir}`); - if (usingHusky) { - console.log('💡 Note: You are using husky. Ensure husky is installed and enabled.'); - } + console.log(`✅ pgfence pre-commit hook installed successfully in ${targetDir}`); + if (usingHusky) { + console.log('💡 Note: You are using husky. Ensure husky is installed and enabled.'); + } } diff --git a/src/lsp/code-actions.ts b/src/lsp/code-actions.ts index e81800f..e08c670 100644 --- a/src/lsp/code-actions.ts +++ b/src/lsp/code-actions.ts @@ -55,6 +55,51 @@ function isExecutableSafeRewrite(safeRewrite: SafeRewrite): boolean { return hasExecutableStep; } +function statementStartInsertPos(text: string, startOffset: number): Position { + const startPos = offsetToPosition(text, startOffset); + const startChar = text.charCodeAt(startOffset); + if (startChar === 10 || startChar === 13) { + return Position.create(startPos.line + 1, 0); + } + return Position.create(startPos.line, 0); +} + +function rangesEqual( + left: { start: { line: number; character: number }; end: { line: number; character: number } }, + right: { start: { line: number; character: number }; end: { line: number; character: number } }, +): boolean { + return ( + left.start.line === right.start.line && + left.start.character === right.start.character && + left.end.line === right.end.line && + left.end.character === right.end.character + ); +} + +function getPolicyIgnoreInsertPos( + analysis: AnalyzeTextResult, + diagnostic: CodeActionParams['context']['diagnostics'][number], + text: string, +): Position { + for (let i = 0; i < analysis.policyViolations.length; i++) { + const violation = analysis.policyViolations[i]; + if (violation.ruleId !== diagnostic.code) continue; + + const sourceRange = analysis.policySourceRanges[i]; + if (!sourceRange) { + return Position.create(0, 0); + } + + const startPos = offsetToPosition(text, sourceRange.startOffset); + const endPos = offsetToPosition(text, sourceRange.endOffset); + if (!rangesEqual({ start: startPos, end: endPos }, diagnostic.range)) continue; + + return statementStartInsertPos(text, sourceRange.startOffset); + } + + return Position.create(0, 0); +} + /** * Generate code actions for diagnostics in the requested range. */ @@ -108,8 +153,7 @@ export function getCodeActions( } // 2. Ignore this rule for this statement - const ignoreLine = startPos.line; - const ignoreInsertPos = Position.create(ignoreLine, 0); + const ignoreInsertPos = statementStartInsertPos(text, range.startOffset); actions.push({ title: `pgfence-ignore: ${check.ruleId}`, kind: CodeActionKind.QuickFix, @@ -131,6 +175,7 @@ export function getCodeActions( if (!matchedCheck) { for (const violation of analysis.policyViolations) { if (violation.ruleId !== ruleId) continue; + const ignoreInsertPos = getPolicyIgnoreInsertPos(analysis, diagnostic, text); actions.push({ title: `pgfence-ignore: ${violation.ruleId}`, @@ -139,7 +184,7 @@ export function getCodeActions( edit: { changes: { [params.textDocument.uri]: [ - TextEdit.insert(Position.create(0, 0), `-- pgfence-ignore: ${violation.ruleId}\n`), + TextEdit.insert(ignoreInsertPos, `-- pgfence-ignore: ${violation.ruleId}\n`), ], }, }, diff --git a/src/plugins.ts b/src/plugins.ts index 6306f72..a96e58e 100644 --- a/src/plugins.ts +++ b/src/plugins.ts @@ -11,7 +11,7 @@ import { pathToFileURL } from 'node:url'; import type { ParsedStatement } from './parser.js'; import type { CheckResult, ExtractionWarning, PgfenceConfig, PolicyViolation } from './types.js'; -const ALLOWED_PLUGIN_EXTENSIONS = new Set(['.js', '.mjs', '.cjs', '.ts', '.mts']); +const ALLOWED_PLUGIN_EXTENSIONS = new Set(['.js', '.mjs', '.cjs']); function isWithinRoot(root: string, candidate: string): boolean { const relative = path.relative(root, candidate); diff --git a/src/rules/alter-column.ts b/src/rules/alter-column.ts index 9512faf..54a34bc 100644 --- a/src/rules/alter-column.ts +++ b/src/rules/alter-column.ts @@ -5,12 +5,13 @@ * - ALTER COLUMN TYPE (table rewrite, ACCESS EXCLUSIVE) * - ALTER COLUMN SET NOT NULL (ACCESS EXCLUSIVE, full table scan) * - * Safe type changes (metadata-only, no table rewrite): - * - varchar(N) -> text (removing length constraint) - * - varchar(N) -> varchar (removing length constraint) - * - varchar/text-family -> text (text is unbounded, metadata-only for text-family sources) - * - varchar(N) -> varchar(M) where M > N (widening, needs schema to verify) - * - numeric(P,S) -> numeric(P2,S) where P2 > P (widening, needs schema to verify) + * Safe type changes require schema context: + * - varchar(N) -> text or varchar without length, only when the current column is already text-family + * - varchar(N) -> varchar(M) where M > N, only when the current length is known + * - numeric(P,S) -> numeric(P2,S) where P2 > P, only when the current precision is known + * + * When source schema is unavailable, we default to HIGH risk rather than + * understating the chance of a table rewrite. */ import type { ParsedStatement } from '../parser.js'; @@ -100,9 +101,15 @@ function classifyTypeChange(target: TargetType | null, sourceColumn?: SourceColu const { name, modifier } = target; const sourceType = sourceColumn?.udtName.toLowerCase() ?? null; const sourceIsTextFamily = sourceType ? TEXT_FAMILY_TYPES.has(sourceType) : false; - const sourceTypeLabel = sourceColumn?.dataType ?? null; + const sourceTypeLabel = sourceColumn?.dataType ?? 'unknown'; if (name === 'text') { + if (!sourceColumn) { + return { + risk: RiskLevel.HIGH, + message: 'TYPE text: source type is unknown, this can still require a table rewrite', + }; + } if (sourceColumn && !sourceIsTextFamily) { return { risk: RiskLevel.HIGH, @@ -116,6 +123,12 @@ function classifyTypeChange(target: TargetType | null, sourceColumn?: SourceColu } if (name === 'varchar' && modifier === null) { + if (!sourceColumn) { + return { + risk: RiskLevel.HIGH, + message: 'TYPE varchar: source type is unknown, this can still require a table rewrite', + }; + } if (sourceColumn && !sourceIsTextFamily) { return { risk: RiskLevel.HIGH, @@ -129,27 +142,37 @@ function classifyTypeChange(target: TargetType | null, sourceColumn?: SourceColu } if (name === 'varchar' && modifier !== null) { - if (sourceColumn) { - if (!sourceIsTextFamily) { - return { - risk: RiskLevel.HIGH, - message: `TYPE varchar(${modifier}): rewrites the table when the current type is ${sourceTypeLabel}`, - }; - } - if (sourceColumn.characterMaximumLength !== null && sourceColumn.characterMaximumLength <= modifier) { - return { - risk: RiskLevel.MEDIUM, - message: `TYPE varchar(${modifier}), safe if widening from varchar(${sourceColumn.characterMaximumLength}), but requires schema to verify. Narrowing causes a table rewrite.`, - }; - } + if (!sourceColumn) { + return { + risk: RiskLevel.HIGH, + message: `TYPE varchar(${modifier}): source type is unknown, this may require a table rewrite`, + }; + } + if (!sourceIsTextFamily) { + return { + risk: RiskLevel.HIGH, + message: `TYPE varchar(${modifier}): rewrites the table when the current type is ${sourceTypeLabel}`, + }; + } + if (sourceColumn.characterMaximumLength !== null && sourceColumn.characterMaximumLength <= modifier) { + return { + risk: RiskLevel.MEDIUM, + message: `TYPE varchar(${modifier}), safe if widening from varchar(${sourceColumn.characterMaximumLength}), but requires schema to verify. Narrowing causes a table rewrite.`, + }; } return { - risk: RiskLevel.MEDIUM, - message: `TYPE varchar(${modifier}), safe if widening (increasing length), but requires schema to verify. Narrowing causes a table rewrite.`, + risk: RiskLevel.HIGH, + message: `TYPE varchar(${modifier}): narrowing from ${sourceTypeLabel} may require a table rewrite`, }; } if (name === 'numeric' && modifier !== null) { + if (!sourceColumn) { + return { + risk: RiskLevel.HIGH, + message: 'TYPE numeric: source type is unknown, this may require a table rewrite', + }; + } if (sourceColumn && sourceType !== 'numeric') { return { risk: RiskLevel.HIGH, diff --git a/src/rules/policy.ts b/src/rules/policy.ts index dea90c2..19c876c 100644 --- a/src/rules/policy.ts +++ b/src/rules/policy.ts @@ -272,7 +272,7 @@ export function checkPolicies( // Operations that require autocommit must not run inside explicit transactions. if (txState.active) { - const autocommitOnlyOp = getAutocommitOnlyOperation(stmt); + const autocommitOnlyOp = getAutocommitOnlyOperation(stmt, config.minPostgresVersion); if (autocommitOnlyOp) { violations.push({ ruleId: 'concurrent-in-transaction', @@ -538,7 +538,7 @@ function getStatementLockMode(stmt: ParsedStatement): LockMode | null { } } -function getAutocommitOnlyOperation(stmt: ParsedStatement): string | null { +function getAutocommitOnlyOperation(stmt: ParsedStatement, minPostgresVersion: number): string | null { if (stmt.nodeType === 'IndexStmt') { const node = stmt.node as { concurrent?: boolean }; return node.concurrent === true ? 'CREATE INDEX CONCURRENTLY' : null; @@ -582,5 +582,9 @@ function getAutocommitOnlyOperation(stmt: ParsedStatement): string | null { } } + if (stmt.nodeType === 'AlterEnumStmt' && minPostgresVersion < 12) { + return 'ALTER TYPE ... ADD VALUE'; + } + return null; } diff --git a/src/rules/reindex.ts b/src/rules/reindex.ts index 9566917..59b06c4 100644 --- a/src/rules/reindex.ts +++ b/src/rules/reindex.ts @@ -13,7 +13,7 @@ import type { CheckResult } from '../types.js'; import { LockMode, RiskLevel, getBlockedOperations } from '../types.js'; import { makePreview } from '../parser.js'; -export function checkReindex(stmt: ParsedStatement): CheckResult[] { +export function checkReindex(stmt: ParsedStatement, minPostgresVersion = 14): CheckResult[] { if (stmt.nodeType !== 'ReindexStmt') return []; const node = stmt.node as { @@ -38,6 +38,7 @@ export function checkReindex(stmt: ParsedStatement): CheckResult[] { const kindLabel = kindMap[node.kind] ?? node.kind; const targetName = node.relation?.relname ?? node.name ?? ''; const tableName = node.relation?.relname ?? null; + const supportsConcurrentReindex = minPostgresVersion >= 12; const isWide = node.kind === 'REINDEX_OBJECT_SCHEMA' || node.kind === 'REINDEX_OBJECT_DATABASE' || node.kind === 'REINDEX_OBJECT_SYSTEM'; @@ -60,10 +61,16 @@ export function checkReindex(stmt: ParsedStatement): CheckResult[] { : `REINDEX ${kindLabel} "${targetName}": acquires ACCESS EXCLUSIVE lock on the index, queries using this index will block`, ruleId: 'reindex-non-concurrent', safeRewrite: { - description: 'Use REINDEX CONCURRENTLY (PG12+) to avoid ACCESS EXCLUSIVE lock', + description: supportsConcurrentReindex + ? 'Use REINDEX CONCURRENTLY (PG12+) to avoid ACCESS EXCLUSIVE lock' + : 'Upgrade to PG12+ before using REINDEX CONCURRENTLY to avoid ACCESS EXCLUSIVE lock', steps: [ - `REINDEX ${kindLabel} CONCURRENTLY ${targetName};`, - `-- Note: REINDEX CONCURRENTLY must run outside a transaction block`, + supportsConcurrentReindex + ? `REINDEX ${kindLabel} CONCURRENTLY ${targetName};` + : '-- REINDEX CONCURRENTLY requires Postgres 12+.', + supportsConcurrentReindex + ? '-- Note: REINDEX CONCURRENTLY must run outside a transaction block' + : `-- After upgrading to PG12+, run REINDEX ${kindLabel} CONCURRENTLY ${targetName} outside a transaction block.`, ], }, }]; diff --git a/tests/analyzer.test.ts b/tests/analyzer.test.ts index 5dd9a88..2af4627 100644 --- a/tests/analyzer.test.ts +++ b/tests/analyzer.test.ts @@ -505,6 +505,18 @@ ALTER TABLE users ADD COLUMN x int;`; expect(missingLockTimeout).toBeUndefined(); }); + it('should mark TypeORM builder API files as unanalyzable for coverage reporting', async () => { + const results = await analyze( + [fixture('typeorm-builder-api.ts')], + { ...defaultConfig, format: 'typeorm' }, + ); + + expect(results).toHaveLength(1); + expect(results[0].statementCount).toBe(0); + expect(results[0].extractionWarnings).toBeDefined(); + expect(results[0].extractionWarnings?.every((w) => w.unanalyzable)).toBe(true); + }); + // --- P0: Inline ignore --- it('should respect -- pgfence: ignore inline directives (legacy syntax)', async () => { @@ -691,50 +703,66 @@ DROP TABLE old_data;`; expect(tsCheck).toBeDefined(); }); - // --- FP #1: ALTER COLUMN TYPE,safe type changes --- + // --- FP #1: ALTER COLUMN TYPE, conservative defaults without schema --- - it('should detect ALTER COLUMN TYPE to text as LOW risk (metadata-only)', async () => { + it('should detect ALTER COLUMN TYPE to text as HIGH risk when source schema is unknown', async () => { const results = await analyze([fixture('alter-column-varchar-widening.sql')], defaultConfig); const checks = results[0].checks.filter((c) => c.ruleId === 'alter-column-type'); - const textChange = checks.find((c) => c.message.includes('target is text')); + const textChange = checks.find((c) => c.message.startsWith('ALTER COLUMN "bio" TYPE text')); expect(textChange).toBeDefined(); - expect(textChange!.risk).toBe(RiskLevel.LOW); + expect(textChange!.risk).toBe(RiskLevel.HIGH); expect(textChange!.lockMode).toBe(LockMode.ACCESS_EXCLUSIVE); - expect(textChange!.safeRewrite).toBeUndefined(); + expect(textChange!.safeRewrite).toBeDefined(); + expect(textChange!.message).toContain('source type is unknown'); }); - it('should detect ALTER COLUMN TYPE to varchar (no length) as LOW risk', async () => { + it('should detect ALTER COLUMN TYPE to varchar (no length) as HIGH risk when source schema is unknown', async () => { const results = await analyze([fixture('alter-column-varchar-widening.sql')], defaultConfig); const checks = results[0].checks.filter((c) => c.ruleId === 'alter-column-type'); - const varcharNoLen = checks.find((c) => c.message.includes('removing varchar length constraint')); + const varcharNoLen = checks.find((c) => c.message.startsWith('ALTER COLUMN "name" TYPE varchar')); expect(varcharNoLen).toBeDefined(); - expect(varcharNoLen!.risk).toBe(RiskLevel.LOW); - expect(varcharNoLen!.safeRewrite).toBeUndefined(); + expect(varcharNoLen!.risk).toBe(RiskLevel.HIGH); + expect(varcharNoLen!.safeRewrite).toBeDefined(); + expect(varcharNoLen!.message).toContain('source type is unknown'); }); - it('should detect ALTER COLUMN TYPE to varchar(N) as MEDIUM risk (needs schema to verify)', async () => { + it('should detect ALTER COLUMN TYPE to varchar(N) as HIGH risk when source schema is unknown', async () => { const results = await analyze([fixture('alter-column-varchar-widening.sql')], defaultConfig); const checks = results[0].checks.filter((c) => c.ruleId === 'alter-column-type'); - const varcharWithLen = checks.filter((c) => c.risk === RiskLevel.MEDIUM); - // Two varchar(N) statements: varchar(64) and varchar(255) + const varcharWithLen = checks.filter((c) => c.message.includes('TYPE varchar(')); expect(varcharWithLen).toHaveLength(2); - expect(varcharWithLen[0].message).toContain('safe if widening'); - expect(varcharWithLen[0].safeRewrite).toBeDefined(); - expect(varcharWithLen[0].safeRewrite!.steps.every((step) => step.trim().startsWith('--'))).toBe(true); + expect(varcharWithLen.every((c) => c.risk === RiskLevel.HIGH)).toBe(true); + expect(varcharWithLen.every((c) => c.safeRewrite !== undefined)).toBe(true); + }); + + it('should detect ALTER COLUMN TYPE to text as LOW risk when schema proves the source type is text-family', async () => { + const tmpFile = `/tmp/pgfence-text-family-${process.pid}-${Date.now()}.sql`; + const { writeFile } = await import('node:fs/promises'); + await writeFile(tmpFile, 'ALTER TABLE public.users ALTER COLUMN name TYPE text;\n', 'utf8'); + + const results = await analyze([tmpFile], { + ...defaultConfig, + snapshotFile: fixture('pgfence-snapshot-sample.json'), + }); + const textChange = results[0].checks.find((c) => c.ruleId === 'alter-column-type'); + expect(textChange).toBeDefined(); + expect(textChange!.risk).toBe(RiskLevel.LOW); + expect(textChange!.safeRewrite).toBeUndefined(); }); it('should detect ALTER COLUMN TYPE cross-family change as HIGH risk', async () => { const results = await analyze([fixture('alter-column-varchar-widening.sql')], defaultConfig); const checks = results[0].checks.filter((c) => c.ruleId === 'alter-column-type'); - const highRisk = checks.filter((c) => c.risk === RiskLevel.HIGH); - expect(highRisk).toHaveLength(1); - expect(highRisk[0].message).toContain('rewrites the entire table'); - expect(highRisk[0].safeRewrite).toBeDefined(); - expect(highRisk[0].safeRewrite!.steps.every((step) => step.trim().startsWith('--'))).toBe(true); + const highRisk = checks.find((c) => c.statement.includes('status TYPE integer')); + expect(highRisk).toBeDefined(); + expect(highRisk!.risk).toBe(RiskLevel.HIGH); + expect(highRisk!.message).toContain('rewrites the entire table'); + expect(highRisk!.safeRewrite).toBeDefined(); + expect(highRisk!.safeRewrite!.steps.every((step) => step.trim().startsWith('--'))).toBe(true); }); it('should use schema snapshot data for ALTER COLUMN TYPE classification', async () => { @@ -812,6 +840,30 @@ DROP TABLE old_data;`; expect(checks[0].safeRewrite).toBeDefined(); }); + it('should reject ALTER TYPE ADD VALUE inside a transaction on PG11', async () => { + const tmpFile = `/tmp/pgfence-alter-enum-tx-${process.pid}-${Date.now()}.sql`; + const { writeFile } = await import('node:fs/promises'); + await writeFile( + tmpFile, + `SET lock_timeout = '2s'; +SET statement_timeout = '5min'; +SET application_name = 'migrate:alter-enum-tx'; +SET idle_in_transaction_session_timeout = '30s'; +BEGIN; +ALTER TYPE status ADD VALUE 'pending'; +COMMIT; +`, + 'utf8', + ); + + const results = await analyze([tmpFile], { ...defaultConfig, minPostgresVersion: 11 }); + const violations = results[0].policyViolations; + const concurrentInTx = violations.find((v) => v.ruleId === 'concurrent-in-transaction'); + expect(concurrentInTx).toBeDefined(); + expect(concurrentInTx!.message).toContain('ALTER TYPE ... ADD VALUE'); + expect(concurrentInTx!.severity).toBe('error'); + }); + // --- Gap 3: REINDEX --- it('should detect REINDEX TABLE without CONCURRENTLY as HIGH risk with SHARE lock', async () => { @@ -833,6 +885,17 @@ DROP TABLE old_data;`; expect(indexReindex!.lockMode).toBe(LockMode.ACCESS_EXCLUSIVE); }); + it('should upgrade REINDEX rewrite guidance on PG11 instead of suggesting unsupported syntax directly', async () => { + const results = await analyze([fixture('reindex.sql')], { ...defaultConfig, minPostgresVersion: 11 }); + const checks = results[0].checks.filter((c) => c.ruleId === 'reindex-non-concurrent'); + const tableReindex = checks.find((c) => c.message.includes('TABLE')); + expect(tableReindex).toBeDefined(); + expect(tableReindex!.safeRewrite).toBeDefined(); + expect(tableReindex!.safeRewrite!.description).toContain('Upgrade to PG12+'); + expect(tableReindex!.safeRewrite!.steps[0]).toContain('requires Postgres 12+'); + expect(tableReindex!.safeRewrite!.steps.join('\n')).not.toContain('REINDEX TABLE CONCURRENTLY appointments;'); + }); + it('should NOT flag REINDEX CONCURRENTLY', async () => { const results = await analyze([fixture('reindex.sql')], defaultConfig); const checks = results[0].checks.filter((c) => c.ruleId === 'reindex-non-concurrent'); diff --git a/tests/cli.test.ts b/tests/cli.test.ts index 5c976b0..3e7b17e 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -1,18 +1,27 @@ -import { describe, it, expect } from 'vitest'; -import { exec } from 'child_process'; +import { describe, it, expect, beforeEach } from 'vitest'; +import { exec, execFile } from 'child_process'; import util from 'util'; import path from 'path'; import { existsSync } from 'node:fs'; +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; import { analyze, RISK_ORDER } from '../src/analyzer.js'; import type { PgfenceConfig } from '../src/types.js'; import { RiskLevel } from '../src/types.js'; +import { installHooks } from '../src/init.js'; const execPromise = util.promisify(exec); +const execFilePromise = util.promisify(execFile); const cliPath = path.join(process.cwd(), 'src', 'index.ts'); const distCliPath = path.join(process.cwd(), 'dist', 'index.js'); const fixturesDir = path.join(process.cwd(), 'tests', 'fixtures'); const hasBuiltCli = () => existsSync(distCliPath); +async function git(cwd: string, args: string[]): Promise { + const { stdout } = await execFilePromise('git', args, { cwd }); + return stdout.trim(); +} + /** Run CLI: use built binary if available, otherwise tsx (can fail in restricted envs). */ function cliCommand(args: string): string { return hasBuiltCli() @@ -71,6 +80,12 @@ describe('CI exit code logic', () => { }); describe.skipIf(!hasBuiltCli())('CLI e2e (built binary)', () => { + beforeEach(async () => { + if (!existsSync(distCliPath)) { + await execPromise('pnpm build'); + } + }); + it('exits 0 and prints coverage when analyzing safe migration', async () => { const fixture = path.join(fixturesDir, 'safe-migration.sql'); const { stdout, stderr } = await execPromise(`node "${distCliPath}" analyze "${fixture}"`); @@ -87,13 +102,72 @@ describe.skipIf(!hasBuiltCli())('CLI e2e (built binary)', () => { }); it('exits 2 on system error (snapshot with bad db url)', async () => { - await expect( - execPromise(`node "${distCliPath}" snapshot --db-url postgres://bad:bad@localhost:0/noexist`), - ).rejects.toMatchObject({ code: 2 }); + try { + await execPromise(cliCommand('snapshot --db-url postgres://bad:bad@localhost:0/noexist')); + throw new Error('expected snapshot to fail'); + } catch (err: unknown) { + const error = err as { code?: number; stderr?: string }; + expect(error.code).toBe(2); + expect(error.stderr).toContain('pgfence snapshot error'); + } }); }); describe('CLI tests', () => { + it('expands action-style glob inputs before invoking pgfence', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'pgfence-action-glob-')); + const migrationsDir = path.join(root, 'migrations'); + await mkdir(migrationsDir, { recursive: true }); + await writeFile(path.join(migrationsDir, '001.sql'), 'SELECT 1;\n'); + await writeFile(path.join(migrationsDir, '002.sql'), 'SELECT 2;\n'); + + try { + const script = ` +shopt -s nullglob +INPUT_PATH='migrations/*.sql' +FILES=($INPUT_PATH) +printf '%s\\n' "\${FILES[@]}" +`; + const { stdout } = await execFilePromise('bash', ['-lc', script], { cwd: root }); + expect(stdout.trim().split('\n').sort()).toEqual(['migrations/001.sql', 'migrations/002.sql']); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + it('installs hooks in worktrees and keeps existing hook behavior intact', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'pgfence-init-worktree-')); + const repoDir = path.join(root, 'repo'); + const worktreeDir = path.join(root, 'worktree'); + await mkdir(repoDir, { recursive: true }); + + const previousCwd = process.cwd(); + try { + await git(repoDir, ['init']); + await git(repoDir, ['config', 'user.email', 'flavius.mnt11@gmail.com']); + await git(repoDir, ['config', 'user.name', 'Munteanu Flavius-Ioan']); + await writeFile(path.join(repoDir, 'README.md'), 'init\n', 'utf8'); + await git(repoDir, ['add', 'README.md']); + await git(repoDir, ['commit', '-m', 'init']); + await git(repoDir, ['worktree', 'add', '--detach', worktreeDir, 'HEAD']); + + const hooksPath = await git(worktreeDir, ['rev-parse', '--git-path', 'hooks']); + const resolvedHooksDir = path.isAbsolute(hooksPath) ? hooksPath : path.resolve(worktreeDir, hooksPath); + await mkdir(resolvedHooksDir, { recursive: true }); + await writeFile(path.join(resolvedHooksDir, 'pre-commit'), '#!/bin/sh\nexit 0\n', 'utf8'); + + process.chdir(worktreeDir); + await installHooks(); + + const installed = await readFile(path.join(resolvedHooksDir, 'pre-commit'), 'utf8'); + expect(installed).toContain('pgfence analyze'); + expect(installed.indexOf('pgfence analyze')).toBeLessThan(installed.indexOf('exit 0')); + } finally { + process.chdir(previousCwd); + await rm(root, { recursive: true, force: true }); + } + }); + it('runs default cli analysis', async () => { const fixture = path.join(fixturesDir, 'safe-migration.sql'); const { stdout } = await execPromise(cliCommand(`analyze "${fixture}"`)); diff --git a/tests/extractors.test.ts b/tests/extractors.test.ts index 5298571..ac47ad8 100644 --- a/tests/extractors.test.ts +++ b/tests/extractors.test.ts @@ -192,6 +192,7 @@ describe('Extractor: TypeORM builder API', () => { const filePath = path.join(fixturesDir, 'typeorm-builder-api.ts'); const result = await extractTypeORMSQL(filePath); expect(result.warnings.length).toBeGreaterThan(0); + expect(result.warnings.every((w) => w.unanalyzable)).toBe(true); const builderWarnings = result.warnings.filter(w => w.message.includes('TypeORM builder API detected') ); diff --git a/tests/lsp/code-actions.test.ts b/tests/lsp/code-actions.test.ts index 1dcdcf7..715c7bf 100644 --- a/tests/lsp/code-actions.test.ts +++ b/tests/lsp/code-actions.test.ts @@ -4,7 +4,7 @@ import { TextDocument } from 'vscode-languageserver-textdocument'; import { getCodeActions } from '../../src/lsp/code-actions.js'; import { analyzeText } from '../../src/lsp/analyze-text.js'; import type { AnalyzeTextResult } from '../../src/lsp/analyze-text.js'; -import { checkResultToDiagnostic } from '../../src/lsp/diagnostics.js'; +import { checkResultToDiagnostic, policyViolationToDiagnostic } from '../../src/lsp/diagnostics.js'; import { RiskLevel } from '../../src/types.js'; import type { PgfenceConfig } from '../../src/types.js'; import type { CodeActionParams } from 'vscode-languageserver'; @@ -79,6 +79,30 @@ describe('Code Actions', () => { expect(edit[0].newText).toContain('-- pgfence-ignore:'); }); + it('should insert policy ignore actions at the violating statement', async () => { + const sql = `CREATE INDEX idx ON users (email); +SET lock_timeout = 0; +SET statement_timeout = '5min'; +SET application_name = 'migrate:test-zero'; +SET idle_in_transaction_session_timeout = '30s';`; + const uri = 'file:///test.sql'; + const doc = makeDoc(uri, sql); + const analysis = await analyzeText({ content: sql, filePath: '/test.sql', config: defaultConfig }); + const lockTimeout = analysis.policyViolations.find(v => v.ruleId === 'lock-timeout-zero'); + expect(lockTimeout).toBeDefined(); + const lockTimeoutIndex = analysis.policyViolations.findIndex(v => v.ruleId === 'lock-timeout-zero'); + const diagnostic = policyViolationToDiagnostic(lockTimeout!, analysis.policySourceRanges[lockTimeoutIndex], sql); + + const params = makeParams(uri, diagnostic.range, [diagnostic]); + const actions = getCodeActions(params, analysis, doc); + + const ignore = actions.find(a => a.title.startsWith('pgfence-ignore:')); + expect(ignore).toBeDefined(); + const edit = ignore!.edit!.changes![uri]; + expect(edit[0].range.start.line).toBe(1); + expect(edit[0].newText).toContain('lock-timeout-zero'); + }); + it('should not offer a quick fix for placeholder-heavy ADD COLUMN rewrites', async () => { const sql = 'ALTER TABLE users ADD COLUMN name text NOT NULL;'; const uri = 'file:///test.sql'; diff --git a/tests/lsp/e2e-fixtures.test.ts b/tests/lsp/e2e-fixtures.test.ts index aed4431..5b2c922 100644 --- a/tests/lsp/e2e-fixtures.test.ts +++ b/tests/lsp/e2e-fixtures.test.ts @@ -56,7 +56,7 @@ function listen(child: ChildProcess) { } }); - function waitFor(pred: (m: JsonRpcMessage) => boolean, ms = 5000): Promise { + function waitFor(pred: (m: JsonRpcMessage) => boolean, ms = 20000): Promise { const existing = msgs.find(pred); if (existing) return Promise.resolve(existing); return new Promise((resolve, reject) => { @@ -70,7 +70,7 @@ function listen(child: ChildProcess) { type WaitFn = (pred: (m: JsonRpcMessage) => boolean, ms?: number) => Promise; -describe('LSP E2E on fixtures', () => { +describe('LSP E2E on fixtures', { timeout: 30000 }, () => { let child: ChildProcess | undefined; afterEach(() => { child?.kill(); child = undefined; }); @@ -405,6 +405,22 @@ describe('LSP E2E on fixtures', () => { expect(ignore!.edit!.changes[uri][0].newText).toContain('-- pgfence-ignore: drop-table'); }); + it('code-action: policy ignore insertion uses the statement range', async () => { + const { child: c, waitFor } = await init(); + const { uri, diags } = await openAndGetDiags(c, waitFor, 'concurrent-in-tx.sql'); + const txDiag = hasRule(diags, 'concurrent-in-transaction')!; + + sendMsg(c, { + jsonrpc: '2.0', id: 12, method: 'textDocument/codeAction', + params: { textDocument: { uri }, range: txDiag.range, context: { diagnostics: [txDiag] } }, + }); + const resp = await waitFor(m => m.id === 12); + const actions = resp.result as Array<{ title: string; edit?: { changes: Record> } }>; + const ignore = actions.find(a => a.title.startsWith('pgfence-ignore:')); + expect(ignore).toBeDefined(); + expect(ignore!.edit!.changes[uri][0].range.start.line).toBe(txDiag.range.start.line + 1); + }); + // ── ADD PRIMARY KEY rules ────────────────────────────────── it('add-pk: without USING INDEX', async () => { @@ -435,10 +451,8 @@ describe('LSP E2E on fixtures', () => { const { child: c, waitFor } = await init(); const { diags } = await openAndGetDiags(c, waitFor, 'alter-column-varchar-widening.sql'); const typeChanges = diags.filter(d => d.code === 'alter-column-type'); - // Widening is flagged with "safe if widening" note (can't verify without schema) - const wideningDiag = typeChanges.find(d => d.message.includes('safe if widening')); - expect(wideningDiag).toBeDefined(); - // Cross-type change (status TYPE integer) is also flagged as a full rewrite + expect(typeChanges.length).toBeGreaterThan(0); + // Cross-type change (status TYPE integer) is flagged as a full rewrite const crossTypeDiag = typeChanges.find(d => d.message.includes('rewrites the entire table')); expect(crossTypeDiag).toBeDefined(); }); diff --git a/tests/lsp/server.test.ts b/tests/lsp/server.test.ts index 0c18266..71994ad 100644 --- a/tests/lsp/server.test.ts +++ b/tests/lsp/server.test.ts @@ -65,7 +65,7 @@ function collectMessages(child: ChildProcess): { messages: JsonRpcMessage[]; wai } }); - function waitFor(pred: (m: JsonRpcMessage) => boolean, timeoutMs = 5000): Promise { + function waitFor(pred: (m: JsonRpcMessage) => boolean, timeoutMs = 10000): Promise { // Check already received messages const existing = messages.find(pred); if (existing) return Promise.resolve(existing); @@ -85,7 +85,7 @@ function collectMessages(child: ChildProcess): { messages: JsonRpcMessage[]; wai return { messages, waitFor }; } -describe('LSP Server Integration', () => { +describe('LSP Server Integration', { timeout: 15000 }, () => { let child: ChildProcess | undefined; afterEach(() => { diff --git a/tests/package-surface.test.ts b/tests/package-surface.test.ts new file mode 100644 index 0000000..f14a344 --- /dev/null +++ b/tests/package-surface.test.ts @@ -0,0 +1,54 @@ +import { existsSync } from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { describe, expect, it } from 'vitest'; + +function listTrackedSourceFiles(): Set { + const stdout = execFileSync('git', ['ls-files', 'src'], { + cwd: process.cwd(), + encoding: 'utf8', + }); + + return new Set( + stdout + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.endsWith('.ts')), + ); +} + +function buildSourcePathForArtifact(filePath: string): string { + return filePath + .replace(/^dist\//, 'src/') + .replace(/\.d\.ts\.map$/, '.ts') + .replace(/\.d\.ts$/, '.ts') + .replace(/\.js\.map$/, '.ts') + .replace(/\.js$/, '.ts'); +} + +function parsePackJson(rawPackJson: string): Array<{ files?: Array<{ path: string }> }> { + const jsonStart = rawPackJson.search(/\[\s*{/s); + if (jsonStart === -1) { + throw new Error('npm pack --json did not emit a JSON payload'); + } + return JSON.parse(rawPackJson.slice(jsonStart)) as Array<{ files?: Array<{ path: string }> }>; +} + +describe('package surface', () => { + it('only ships dist artifacts backed by tracked public source files', () => { + expect(existsSync('dist/index.js')).toBe(true); + expect(existsSync('dist/lsp/server.js')).toBe(true); + + const packJson = execFileSync('npm', ['pack', '--dry-run', '--json', '--ignore-scripts'], { + cwd: process.cwd(), + encoding: 'utf8', + }); + const pack = parsePackJson(packJson); + const files = pack.flatMap((entry) => entry.files ?? []).map((file) => file.path); + + const trackedSources = listTrackedSourceFiles(); + const distArtifacts = files.filter((file) => /^dist\/.+\.(?:js|js\.map|d\.ts|d\.ts\.map)$/.test(file)); + const orphanArtifacts = distArtifacts.filter((file) => !trackedSources.has(buildSourcePathForArtifact(file))); + + expect(orphanArtifacts).toEqual([]); + }); +}); diff --git a/tests/security.test.ts b/tests/security.test.ts index 40495a2..8b38a7b 100644 --- a/tests/security.test.ts +++ b/tests/security.test.ts @@ -181,6 +181,36 @@ describe('security boundaries', () => { } }); + it.each(['.ts', '.mts'])('rejects %s plugin files in the shipped binary', async (extension) => { + const projectRoot = await makeRepoLocalTempDir('.pgfence-plugin-ts-'); + const pluginFile = path.join(projectRoot, `plugin${extension}`); + await writeFile( + pluginFile, + `export default { + name: 'ts-plugin', + rules: [ + { + ruleId: 'plugin:ts', + check() { + return []; + }, + }, + ], + };`, + 'utf8', + ); + + const previousCwd = process.cwd(); + process.chdir(projectRoot); + try { + const { loadPlugins } = await import('../src/plugins.js'); + await expect(loadPlugins([`plugin${extension}`])).rejects.toThrow(`unsupported extension "${extension}"`); + } finally { + process.chdir(previousCwd); + await rm(projectRoot, { recursive: true, force: true }); + } + }); + it('rejects malformed plugin rule entries', async () => { const projectRoot = await makeRepoLocalTempDir('.pgfence-plugin-malformed-'); const pluginFile = path.join(projectRoot, 'bad.mjs');