Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 143 additions & 0 deletions .agents/skills/pgsql-lint/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
---
name: pgsql-lint
description: How to lint SQL/PL-pgSQL source with @pgsql/lint and how to author new rules, severities, and source adapters. Use when running the convention linter, adding a rule, wiring it into a tool (CLI, pre-commit, safegres), or debugging a finding.
---

# @pgsql/lint

`@pgsql/lint` (`packages/lint`) is a **source-level** convention linter: source
text in → findings out. It parses a `CREATE FUNCTION` definition, walks the AST,
and reports style/safety violations. It has **no `pg` / catalog dependency**, so
the same engine runs over a migration on disk, an editor buffer, a pre-commit
hook, or a definition read from a live catalog via `pg_get_functiondef`
(safegres consumes it exactly this way).

Runtime footprint is only the parser stack in this repo: `pgsql-parser`
(SQL → AST), `libpg-query` (`parsePlPgSQL`), `@pgsql/traverse` (`walk`).

## The built-in rules

| Code | Id | Flags | Reason required? |
|------|----|-------|------------------|
| `C1` | `no-set-search-path` | `SET search_path` clause **or** `set_config('search_path', …)` | no |
| `C2` | `no-variable-conflict` | a PL/pgSQL `#variable_conflict` directive | no |
| `C3` | `require-qualified-refs` | an unqualified relation reference (`FROM users`); CTE names excluded | no |
| `C4` | `no-dynamic-sql` | `EXECUTE`, `EXECUTE … USING`, `FOR … IN EXECUTE` | **yes** |

The discipline: never depend on `search_path` — fully qualify everything
(`C1` + `C3`); don't paper over ambiguity (`C2`); treat dynamic SQL as opaque
and exceptional (`C4`).

## Running it

```bash
pgsql-lint ./migrations # dir, recursive .sql
pgsql-lint schema.sql --json # machine-readable
pgsql-lint . --rules no-dynamic-sql # subset
pgsql-lint . --warn require-qualified-refs # downgrade (won't fail)
pgsql-lint . --off C2 # disable (id or code)
```

Exit code is `1` when any **error**-severity, non-waived finding remains, `0`
otherwise. `--warn` findings print but don't fail the run.

Programmatic entry points (all pure, DB-free):

```ts
import { lintDefinition, lintSqlText, lintFiles } from '@pgsql/lint';

await lintDefinition(defText, 'plpgsql'); // one definition (pg_get_functiondef)
await lintSqlText(migrationSql); // a source string, many statements
await lintFiles(['./migrations']); // files/dirs on disk
```

`lintSqlText`/`lintFiles` slice out each top-level `CREATE FUNCTION` using the
parser's `stmt_location`/`stmt_len`, lint each in isolation, and **re-anchor**
findings to absolute file lines — so a mixed migration is never treated as one
malformed definition.

## Authoring a new rule

A rule is a plain value — **no magic npm names**. Author it with `defineRule`
(type-only helper) and hand it to `createLinter`:

```ts
import { createLinter, defineRule, LINT_RULES } from '@pgsql/lint';

const noWritesInView = defineRule({
id: 'no-writes-in-view', // stable, ESLint-style id
code: 'X1', // registry code
title: 'views must be read-only',
reasonRequired: false, // true ⇒ a bare suppression won't silence it
run: (unit) => {
// unit.fragments — parsed SQL fragments, each with lineForOffset(offset)
// unit.dynamicSql — detected EXECUTE / dynamic sites (line + form)
// unit.lines — raw source lines (1-based reporting)
return []; // LintProblem[] { ruleId, line, message, hint?, context? }
}
});

const linter = createLinter({ rules: [...LINT_RULES, noWritesInView] });
await linter.lintFiles(['./migrations']);
```

Rule bodies must use `walk` from `@pgsql/traverse` (via the package's `findAll`
helper) — never hand-roll a `transformSync(..., { hydrate: true })` loop. See the
`ast-traversal` skill.

### Severity is config, not rule state

Severity (`off` / `warn` / `error`, ESLint-style) is decided by the *consumer*,
keyed by rule id or code; a rule never hard-codes its own severity. Unmapped
rules default to `error`; `off` rules don't run.

```ts
createLinter({ severity: { 'require-qualified-refs': 'warn', C2: 'off' } });
```

This is the safegres seam: its registry maps `high/medium/low` → `error/warn/off`
and passes a `severity` map in — no duplicated severity logic downstream.

### Source adapters — where definitions come from

A rule is pure `unit → problems`; an **adapter** decides *where* definitions come
from. The package ships `filesAdapter` and `sqlTextAdapter`; a consumer
implements `SourceAdapter` and calls `linter.lintSource(adapter)`:

```ts
interface SourceAdapter {
id: string;
definitions: () => Promise<LintDefinitionInput[]> | LintDefinitionInput[];
}
```

safegres is "the catalog adapter": it yields `LintDefinitionInput`s from
`pg_get_functiondef`, over the same engine and rules.

## Suppressions

ESLint/Prettier-style, authored in the function body (they survive
`pg_get_functiondef`). Keywords `pgsql-lint` and `safegres` are both accepted:

```sql
-- pgsql-lint-disable-next-line no-dynamic-sql -- lookup-only: building an IN-list of ints
EXECUTE format('SELECT … WHERE id = ANY(%L)', ids);
```

Forms: `disable-next-line`, `disable-line`, `disable`…`enable` (range),
`disable-file`. `no-dynamic-sql` **requires** a reason — a reasonless waiver does
not silence it (the finding stands, tagged `invalidSuppression: 'missing-reason'`).
Suppressed findings are reported as *acknowledged* accepted-risk, never dropped.

## Files

| File | What |
|------|------|
| `src/engine.ts` | `lintDefinition` — parse, run rules, apply suppressions, attach severity |
| `src/linter.ts` | `createLinter` — bind a rule set + severities + keyword; `lintDefinition`/`lintSqlText`/`lintFiles`/`lintSource` |
| `src/file-runner.ts` | file/sql-text slicing + re-anchoring; `filesAdapter`, `sqlTextAdapter`, `lintSource` |
| `src/rules/*` | the built-in C1–C4 rules |
| `src/suppressions.ts` | the ESLint/Prettier-style directive parser |
| `src/parse-unit.ts` | `CREATE FUNCTION` → `LintUnit` (SQL + PL/pgSQL bodies) |
| `src/cli.ts` | the `pgsql-lint` CLI |
| `src/types.ts` | public types + `defineRule` |
1 change: 1 addition & 0 deletions .github/workflows/run-tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ jobs:
- '@pgsql/transform-ast'
- '@pgsql/traverse'
- '@pgsql/semantics'
- '@pgsql/lint'
- '@pgsql/transform'
- '@pgsql/scripts'
steps:
Expand Down
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ Detailed workflow documentation lives in `.agents/skills/`:
| **AST Traversal** | `.agents/skills/ast-traversal/SKILL.md` | Walking SQL and PL/pgSQL ASTs: choosing `walk` / `walkSql` / `walkSqlAst` / `walkPlpgsqlAst` / `traverse`, statement context, visitor composition, abort, mutation |
| **Testing & Fixtures** | `.agents/skills/testing-fixtures/SKILL.md` | Fixture-based testing pipeline, adding new test fixtures, kitchen-sink workflow, PL/pgSQL fixtures, transform tests |
| **Code Generation** | `.agents/skills/code-generation/SKILL.md` | Protobuf codegen (`build:proto`), type inference/generation (`pgsql-types`), keyword generation (`@pgsql/quotes`), version-specific deparsers |
| **pgsql-lint** | `.agents/skills/pgsql-lint/SKILL.md` | Source-level SQL/PL-pgSQL convention linting (`@pgsql/lint`): running the CLI, authoring rules with `defineRule`/`createLinter`, severity config, source adapters, suppressions |

## Root Scripts

Expand Down
125 changes: 125 additions & 0 deletions packages/lint/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
# @pgsql/lint

<p align="center" width="100%">
<img height="250" src="https://raw.githubusercontent.com/constructive-io/constructive/refs/heads/main/assets/outline-logo.svg" />
</p>

<p align="center" width="100%">
<a href="https://github.com/constructive-io/pgsql-parser/actions/workflows/run-tests.yaml">
<img height="20" src="https://github.com/constructive-io/pgsql-parser/actions/workflows/run-tests.yaml/badge.svg" />
</a>
<a href="https://github.com/constructive-io/pgsql-parser/blob/main/LICENSE-MIT"><img height="20" src="https://img.shields.io/badge/license-MIT-blue.svg"/></a>
<a href="https://www.npmjs.com/package/@pgsql/lint"><img height="20" src="https://img.shields.io/github/package-json/v/constructive-io/pgsql-parser?filename=packages%2Flint%2Fpackage.json"/></a>
</p>

A source-level SQL / PL/pgSQL **convention linter**. It reasons about the *text*
of a `CREATE FUNCTION` definition — from its AST — and carries **no `pg` /
catalog dependency**, so the exact same engine runs over a definition in a
migration, an editor buffer, a pre-commit hook, or one read from a live catalog
via `pg_get_functiondef`.

## Installation

```bash
npm install @pgsql/lint
```

## Rules

| Code | Id | Flags |
|------|----|-------|
| `C1` | `no-set-search-path` | `SET search_path` clause, or `set_config('search_path', …)` |
| `C2` | `no-variable-conflict` | a PL/pgSQL `#variable_conflict` directive |
| `C3` | `require-qualified-refs` | an unqualified relation reference (`FROM users` → `FROM app_public.users`) |
| `C4` | `no-dynamic-sql` | `EXECUTE`, `EXECUTE … USING`, `FOR … IN EXECUTE` |

The rules encode a single discipline: never depend on `search_path` — fully
qualify everything (`C1` + `C3`) — don't paper over ambiguity (`C2`), and treat
dynamic SQL as opaque and exceptional (`C4`).

## CLI

```bash
pgsql-lint path/to/migrations # a directory (scanned recursively for .sql)
pgsql-lint schema.sql other.sql # explicit files
pgsql-lint . --rules no-dynamic-sql # only some rules
pgsql-lint . --warn require-qualified-refs # downgrade to a warning (won't fail)
pgsql-lint . --off C2 # disable a rule (by id or code)
pgsql-lint . --json # machine-readable
```

Exit code is `1` when any **error**-severity (and non-waived) finding remains,
`0` otherwise — drop it straight into CI. `--warn` findings print but don't fail.

## Suppressions

ESLint / Prettier-style comments, authored in the function body (they survive
`pg_get_functiondef`). The keyword is `pgsql-lint` (`safegres` is also accepted):

```sql
-- pgsql-lint-disable-next-line no-dynamic-sql -- lookup-only: building an IN-list of ints
EXECUTE format('SELECT … WHERE id = ANY(%L)', ids);
```

Forms: `disable-next-line`, `disable-line`, `disable` … `enable` (a range), and
`disable-file`. A directive with no rule listed applies to every rule; a reason
follows a second `--` or a `:`.

`no-dynamic-sql` **requires** a reason: a reasonless waiver does not silence it,
so an approved use always documents *why* (`lookup-only` / `codegen`). Suppressed
findings are reported as *acknowledged* accepted-risk, never dropped.

## Programmatic API

```ts
import { lintDefinition, lintFiles, lintSqlText } from '@pgsql/lint';

// one definition (e.g. from pg_get_functiondef)
const { problems, suppressed } = await lintDefinition(defText, 'plpgsql');

// a SQL source string with many statements
const report = await lintSqlText(migrationSql);

// files / directories on disk
const reports = await lintFiles(['./migrations']);
```

## Custom rules & severity (building an ecosystem)

Rules are **injected as values** — never discovered by a magic npm package name.
A rule is a plain object; publish it, `import` it, and pass it to `createLinter`.
Severity is *configuration* (ESLint-style `off` / `warn` / `error`), keyed by
rule id or code, so a consumer stays in full control of how loud each rule is:

```ts
import { createLinter, defineRule, LINT_RULES } from '@pgsql/lint';

const noWritesInView = defineRule({
id: 'no-writes-in-view',
code: 'X1',
title: 'views must be read-only',
reasonRequired: false,
run: (unit) => [/* … inspect unit.fragments / unit.dynamicSql … */]
});

const linter = createLinter({
rules: [...LINT_RULES, noWritesInView],
severity: { 'require-qualified-refs': 'warn', C2: 'off' }
});

await linter.lintFiles(['./migrations']); // also lintDefinition / lintSqlText / lintSource
```

### Source adapters

Rules are pure `unit → problems`; an **adapter** decides *where* the definitions
come from. `@pgsql/lint` ships `filesAdapter` and `sqlTextAdapter`; a consumer
(e.g. safegres, reading a live catalog via `pg_get_functiondef`) implements the
`SourceAdapter` interface and passes it to `linter.lintSource(adapter)`.

```ts
interface SourceAdapter {
id: string;
definitions: () => Promise<LintDefinitionInput[]> | LintDefinitionInput[];
}
```
19 changes: 19 additions & 0 deletions packages/lint/__tests__/__fixtures__/migration.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
-- A mixed migration file: schema DDL, a clean function, and a dirty one.
CREATE SCHEMA app_public;

CREATE TABLE app_public.users (
id serial primary key,
email text not null
);

CREATE FUNCTION app_public.clean() RETURNS setof app_public.users
LANGUAGE sql
AS $$
SELECT * FROM app_public.users
$$;

CREATE FUNCTION app_public.dirty() RETURNS setof app_public.users
LANGUAGE sql
AS $$
SELECT * FROM users
$$;
82 changes: 82 additions & 0 deletions packages/lint/__tests__/cli.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import { execFile } from 'child_process';
import * as fs from 'fs';
import * as path from 'path';
import { promisify } from 'util';

const execFileAsync = promisify(execFile);

const CLI = path.join(__dirname, '..', 'dist', 'cli.js');
const FIXTURES = path.join(__dirname, '__fixtures__');

interface RunResult {
code: number;
stdout: string;
stderr: string;
}

async function runCli(args: string[]): Promise<RunResult> {
try {
const { stdout, stderr } = await execFileAsync('node', [CLI, ...args]);
return { code: 0, stdout, stderr };
} catch (err) {
const e = err as { code?: number; stdout?: string; stderr?: string };
return { code: e.code ?? 1, stdout: e.stdout ?? '', stderr: e.stderr ?? '' };
}
}

// The CLI is exercised against the built dist/ (CI runs `pnpm build` first).
const built = fs.existsSync(CLI);
const describeIfBuilt = built ? describe : describe.skip;

describeIfBuilt('pgsql-lint CLI', () => {
it('exits 1 and reports the C3 finding as JSON', async () => {
const { code, stdout } = await runCli([path.join(FIXTURES, 'migration.sql'), '--json']);
expect(code).toBe(1);
const reports = JSON.parse(stdout);
const findings = reports.flatMap((r: { findings: unknown[] }) => r.findings);
expect(findings).toHaveLength(1);
expect(findings[0].code).toBe('C3');
});

it('exits 0 when only some rules are selected and none match', async () => {
const { code } = await runCli([path.join(FIXTURES, 'migration.sql'), '--rules', 'no-dynamic-sql']);
expect(code).toBe(0);
});

it('exits 0 when the only finding is downgraded to a warning', async () => {
const { code } = await runCli([
path.join(FIXTURES, 'migration.sql'),
'--warn',
'require-qualified-refs'
]);
expect(code).toBe(0);
});

it('reports the finding as a warning in JSON when downgraded', async () => {
const { stdout } = await runCli([
path.join(FIXTURES, 'migration.sql'),
'--warn',
'C3',
'--json'
]);
const reports = JSON.parse(stdout);
const findings = reports.flatMap((r: { findings: { severity: string }[] }) => r.findings);
expect(findings[0].severity).toBe('warn');
});

it('exits 0 when the rule is turned off', async () => {
const { code } = await runCli([
path.join(FIXTURES, 'migration.sql'),
'--off',
'require-qualified-refs'
]);
expect(code).toBe(0);
});

it('prints help and exits 0 with --help', async () => {
const { code, stdout } = await runCli(['--help']);
expect(code).toBe(0);
expect(stdout).toContain('pgsql-lint');
expect(stdout).toContain('no-dynamic-sql');
});
});
Loading
Loading