Add pkg/lint: offline typed findings replacing the CLI lint stub

[Kiran01bm]

Summary

Replaces the CLI lint stub with a real offline linter: pkg/lint runs the same parse-and-classify pipeline as the front doors but with zero live facts, so it needs no database and is strictly conservative. Findings carry typed codes automation branches on — never prose. Second slice of the P2.5 dry-run/advisory surface, stacked on the plan-contract PR.

What

• New pkg/lint: Check(script) returns a versioned Report (format_version: 1) of typed findings. Codes map from the classifier:
  • unsupported-operation — the engine would refuse it (error; the only severity that flips the exit code)
  • blocking-idiom — a safer native form exists; the finding carries it as suggestion (warning)
  • table-rewrite — needs the copy-and-swap path, with the planner's typed reason (warning)
  • destructive — column/constraint drops, same definition as the declarative differ (warning)
• New statement.Split: grammar-backed script splitter returning canonical per-statement SQL — no hand-parsing; the coming suggest surface reuses it.
• CLI lint [file] (or stdin), --json, offline — no DB flags. A clean script prints nothing and exits 0; error findings exit non-zero via a typed sentinel.
• An unsupported operation is a finding, not a lint failure — one bad statement never hides the rest of the report. A parse failure is an error, surfaced.
• Docs swept: README status → Phases 1–2.5; SAFETY.md, architecture package map, and low-level design stub markers flipped; testing matrix rows added.

Why

The linter is the policy gate the tracker's E1 slice needs before execution: refuse what the engine cannot run safely, and surface what it would rewrite or gate, in CI, without touching a database. Deriving findings entirely from the classifier keeps one source of truth for safety judgment — the linter adds severity and presentation, never a second opinion.

Before / after

Before:
  pg-sprite lint ──> "lint: not implemented yet (Phase 0 stub)"

After:
  pg-sprite lint change.sql
    ├─ parse (statement.Split, real grammar)
    ├─ classify per statement (planner, zero facts — conservative)
    └─ typed findings: errors → exit non-zero, warnings advise
  pg-sprite lint --json ──> lint.Report  format_version: 1

References

• PLAT-38440 (P2.5: dry-run plan + advisory suggest surface + linter)
• Stacked on the plan-contract PR (kiran01bm/p2-5-plan-contract)

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Review requested by Armand and performed by his agent — same two lenses used across this stack (#8, #2, #7, #6, #5, #3): pg-sprite as an OSS-first, best-in-class Postgres DDL tool, and pg-sprite as a clean integration target for an orchestrator. Reviewed at head 7f3ad6e. An adversarial correctness pass is posted separately.

The architectural choice here is the right one and worth naming: the linter derives every judgment from the classifier and adds only severity and presentation. "The linter adds severity and presentation, never a second opinion" is the sentence that keeps this from becoming a second, drifting rulebook — the failure mode nearly every schema linter eventually hits. Three more details land well: statement.Split going through the real grammar rather than splitting on semicolons (the same discipline that keeps fmt and diff honest); an unsupported operation being an error-severity finding rather than a fatal, so one bad statement can't hide the rest of the report; and decisionFinding's default: arm treating an unknown route as a refusal rather than passing it through. That last one is three lines and it's the difference between a linter that fails closed and one that silently approves whatever a future planner version invents.

The findings below are mostly about the gap between "a report" and "a CI gate", which is what the PR description promises it to be.

OSS lens

1. Zero facts means every ALTER COLUMN … TYPE is reported as a table rewrite, including the free ones. Facts{} makes classifyTypeChange fail closed, which is correct as a routing default — but as a linting default it means ALTER TABLE orders ALTER COLUMN reference TYPE varchar(100) comes back table-rewrite / type-rewrite, when Phase 2.3-2.4: classifier and router seam #7's own binaryCoercible rules know that transformation is free and I verified against a live server that PostgreSQL doesn't rewrite it. This is the classic linter-adoption failure: the first time a team gets a scary warning for a change they know is instant, they learn to ignore the warnings, and by the time a real one arrives the tool has no credibility left. The engine already has the fix — let lint optionally take facts, either from a schema file (--desired, reusing the desired-state format that already exists) or an optional --url for the "I have a database, sharpen the analysis" case, while keeping the fully offline mode as the default. Same conservative posture, far fewer false alarms.

2. Findings carry no line or column, so they can't be annotated onto a diff. A finding identifies its statement by 1-based index and reports deparsed SQL — for ALTER TABLE orders DROP COLUMN legacy_a on line 6 of a file, you get "statement": 2 and "sql": "ALTER TABLE orders DROP legacy_a". A reviewer can't jump to it, CI can't place a review annotation, and because the text is canonicalized rather than verbatim, even grepping the file for the reported SQL fails. libpg_query's RawStmt carries stmt_location and stmt_len, so the byte offset is available at exactly the point Split discards it; converting that to line/column is a few lines and turns lint output from a list into something a CI system can act on. For a tool meant to run in CI this is close to table stakes.

3. There is no severity policy, so the gate only ever gates one thing. Only error flips the exit code, and only unsupported-operation is an error — so a script that drops two columns, drops a unique index, and issues two blocking SET NOT NULLs exits 0 with seven warnings. That's a defensible default, but a linter used as a gate needs the policy to be the adopter's: a --max-severity/--error-on flag, per-code enable/disable, and inline suppression (-- pg-sprite:ignore destructive with a required reason) are the three things every adopted linter ends up with. Deciding the suppression syntax now, while there are four codes, is much cheaper than retrofitting it. Related: the codes are a public vocabulary from the first release, so they deserve a documented table with an example per code — the same argument as the plan contract's on Add pkg/plan: one versioned dry-run report for both front doors #8.

Integration lens

4. Two report contracts now both claim format_version: 1, and they share vocabulary. plan.Report and lint.Report are independently versioned constants, both 1, and both embed planner.Reason values. A consumer holding both has no way to express "I understand plan v1 and lint v1 but not the reason vocabulary they were built against". Worth stating explicitly whether the two version together or independently, and — per the same point on Add pkg/plan: one versioned dry-run report for both front doors #8 — whether the shared Reason/Route enums are part of either contract. If they are, one bumps both; if they aren't, consumers need the "unknown value ⇒ fail closed" rule written down.

5. lint has no notion of what it's linting against. An orchestrator lints one file destined for many databases, potentially on different PostgreSQL majors, and today the report says nothing about the assumptions behind it. Even offline, stamping the version range the rules were derived for (the 14–18 floor) makes a stored lint result auditable and gives the report somewhere to grow when a rule does become version-dependent. This is the same missing self-description I raised on the plan report; solving it once in a shared place would cover both.

Verified solid

statement.Split is right in a way worth calling out: it deparses each statement rather than slicing the input, refuses the whole script on a parse failure instead of skipping the bad statement, and surfaces the statement index in the error. The counting loop is separate from the finding loop, so severity totals can't drift from the findings. Warnings alone genuinely pass while errors genuinely fail, and ErrLintFindings is a typed sentinel rather than a string match, so the CLI's exit behavior is testable. A clean script printing nothing is the correct Unix-linter behavior and a surprising number of tools get it wrong. Reading from stdin when no path is given makes pg-sprite lint < change.sql work in a pipeline without a temp file. CGO_ENABLED=0 go build ./... passes at this head.

This review was generated by Claude Code (claude-fable-5).

[aparajon]

🤖 Adversarial correctness review requested by Armand and performed by his agent — separate from the two-lens pass. Method: run a realistic CI script through the linter and check whether each finding (and each absent finding) is true, verifying the load-bearing claim against a real PostgreSQL at head 7f3ad6e. Reproduction tests are in the collapsed blocks at the end.

Here is the whole report for a script of six ordinary statements, which is the context for everything below:

statement 1: warning: table-rewrite   — ALTER COLUMN reference TYPE varchar
statement 2: warning: destructive     — DROP COLUMN legacy_a
statement 3: warning: destructive     — DROP COLUMN legacy_b
statement 4: warning: blocking-idiom  — DROP INDEX
statement 5: warning: blocking-idiom  — ALTER COLUMN paid_at SET NOT NULL
statement 5: warning: blocking-idiom  — ALTER COLUMN shipped_at SET NOT NULL
statement 6: warning: blocking-idiom  — ATTACH PARTITION
errors=0 warnings=7 → exit code 0

Findings, most severe first

1. Index drops are excluded from destructive on a justification that is false for unique indexes. The code states the rule and its reason:

// An index drop is not destructive — the index is recreatable from the
// schema.
func isDestructive(op statement.Op) bool {
	return op.Kind == statement.OpDropColumn || op.Kind == statement.OpDropConstraint
}

For a plain btree index that reasoning holds. For a unique index it doesn't, because the index is the constraint — dropping it removes an enforced invariant, and once writes have taken advantage of the gap the index cannot be recreated at all. Reproduced end-to-end:

duplicate insert after the index drop: err=<nil>
recreate the unique index: err=ERROR: could not create unique index "orders_unique_ref_idx" (SQLSTATE 23505)

So the recovery is not "re-run the schema" but "find and repair the duplicate rows a production workload wrote" — which is the definition of destructive the code uses for constraint drops one line above. Note the linter does flag ALTER TABLE … DROP CONSTRAINT u for the same uniqueness guarantee; only the DROP INDEX spelling escapes. This is the same gap I raised on #6, now with an explicit rationale attached, which is why it's worth the direct disagreement: the claim as written is checkable and doesn't hold. Minimum fix is flagging unique-index drops; if the distinction can't be made offline (the linter has no catalog access, so it can't know whether orders_unique_ref_idx is unique), that argues for treating all index drops as destructive offline and narrowing later when facts are available — the conservative direction the rest of this package takes.

2. Genuinely free widenings are reported as table rewrites. With zero facts, classifyTypeChange fails closed, so statement 1 above — ALTER COLUMN reference TYPE varchar(100) — comes back table-rewrite / type-rewrite. binaryCoercible in the same codebase already encodes that varchar(n) → wider varchar needs no rewrite, and I verified against a live server (in the #7 pass) that PostgreSQL leaves the relfilenode untouched for every rule in that allowlist. Failing closed is right for routing, where the cost of being wrong is an outage; in a linter the cost of being wrong is that people stop reading the output. Worth separating "the engine will take the heavy path because it cannot prove otherwise" from "this operation rewrites the table", because only the second is a property of the change.

3. blocking-idiom findings frequently carry no suggestion, asserting a safer form exists without naming it. Statements 5 and 6 above both say a safer native idiom exists and offer nothing. The cause is inherited from the planner (the single := len(ops) == 1 gate for multi-operation ALTERs, and ATTACH PARTITION where the planner cannot construct the CHECK) and I reported it on #7 — but the linter is where it becomes user-facing, and it's worse here than in the plan output: lint exists to tell someone what to do differently, and this finding tells them only that they're wrong. Statement 5 also shows a related shape — one multi-operation statement produces two identical, un-actionable findings.

4. DROP INDEX findings name no index. ParseOps constructs the op as Op{Kind: OpDropIndex, Concurrent: …} with Name never set, so Describe() returns "DROP INDEX " — trailing space, no object:

{ "statement": 4, "sql": "DROP INDEX orders_unique_ref_idx",
  "operation": "DROP INDEX ", "code": "blocking-idiom", … }

In a script dropping several indexes, every finding's operation is byte-identical, and the only way to tell them apart is to re-read the sql field. DropStmt carries the object names; capturing one into Op.Name fixes the label and would also give finding 1 somewhere to put the index identity.

5. Reported SQL is deparsed, not verbatim, so findings can't be located in the source. Split returns canonical SQL, so the user's ALTER TABLE orders DROP COLUMN legacy_a is reported as ALTER TABLE orders DROP legacy_a. Combined with the absence of any line/column (a statement on line 6 reports only "statement": 2), a finding cannot be jumped to, annotated onto a diff, or even found by grep. RawStmt carries stmt_location/stmt_len and they're discarded inside Split — the position is available exactly where it's being thrown away.

Probed and held

The default: arm of decisionFinding genuinely fails closed — an unrecognized route becomes an error-severity refusal, not a silent pass. Severity accounting is correct: errors/warnings are counted from the finding list rather than tracked alongside it, so they can't drift. Parse failure is surfaced with the offending statement index rather than swallowed, and one unsupported statement does not abort the report (I checked a script with a refused statement in the middle — the statements after it are still analyzed). A clean script produces "findings": [] and prints nothing. DROP COLUMN correctly produces exactly one destructive finding and no routing finding, so the two finding sources don't double-report. The Suggestion field is populated correctly where the planner does construct a sequence (statement 4's DROP INDEX CONCURRENTLY rewrite). CGO_ENABLED=0 go build ./... passes at this head.

Reproduction tests

Findings 2–5 — pkg/lint/adv_test.go: a realistic script and a positions probe (no database needed)

package lint_test

import (
	"encoding/json"
	"testing"

	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/pkg/lint"
)

// A realistic CI script: what does the linter say, and would it fail the build?
func TestAdvLintRealisticScript(t *testing.T) {
	script := `
-- widen a column: free in PostgreSQL, no rewrite
ALTER TABLE orders ALTER COLUMN reference TYPE varchar(100);
-- drop three columns
ALTER TABLE orders DROP COLUMN legacy_a;
ALTER TABLE orders DROP COLUMN legacy_b;
DROP INDEX orders_unique_ref_idx;
-- two operations at once
ALTER TABLE orders ALTER COLUMN paid_at SET NOT NULL, ALTER COLUMN shipped_at SET NOT NULL;
ALTER TABLE orders ATTACH PARTITION orders_2026 FOR VALUES FROM ('2026-01-01') TO ('2027-01-01');
`
	report, err := lint.Check(script)
	require.NoError(t, err)
	b, _ := json.MarshalIndent(report, "", "  ")
	t.Logf("report:\n%s", b)
	t.Logf("errors=%d warnings=%d", report.Errors, report.Warnings)
}

// Does a finding carry any position a CI annotation could use?
func TestAdvLintPositions(t *testing.T) {
	script := "CREATE TABLE ok (id int);\n\n\n\n\nALTER TABLE t OWNER TO bob;\n"
	report, err := lint.Check(script)
	require.NoError(t, err)
	b, _ := json.MarshalIndent(report.Findings, "", "  ")
	t.Logf("findings for a statement on line 6:\n%s", b)
}

The positions probe — the offending statement is on line 6:

[
  {
    "statement": 2,
    "sql": "ALTER TABLE t OWNER TO bob",
    "operation": "unrecognized operation",
    "code": "unsupported-operation",
    "severity": "error",
    "reason": "unsupported-operation"
  }
]

Finding 1 — pkg/lint/adv_integration_test.go: is a dropped unique index recreatable?

package lint_test

import (
	"fmt"
	"testing"

	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/internal/testutil"
	"github.com/block/pg-sprite/pkg/dbconn"
)

// isDestructive excludes index drops because "the index is recreatable
// from the schema". Is that true for a unique index?
func TestAdvUniqueIndexDropIsNotRecreatable(t *testing.T) {
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)})
	require.NoError(t, err)
	t.Cleanup(pool.Close)
	schema := testutil.NewSchema(t, pool)

	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE TABLE %s.orders (id bigint PRIMARY KEY, reference text)", schema))
	require.NoError(t, err)
	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE UNIQUE INDEX orders_unique_ref_idx ON %s.orders (reference)", schema))
	require.NoError(t, err)
	_, err = pool.Exec(t.Context(), fmt.Sprintf("INSERT INTO %s.orders VALUES (1, 'A')", schema))
	require.NoError(t, err)

	// The lint-approved change: warning-only, exit 0.
	_, err = pool.Exec(t.Context(), fmt.Sprintf("DROP INDEX %s.orders_unique_ref_idx", schema))
	require.NoError(t, err)

	// The guarantee is gone, so the application can now write a duplicate.
	_, err = pool.Exec(t.Context(), fmt.Sprintf("INSERT INTO %s.orders VALUES (2, 'A')", schema))
	t.Logf("duplicate insert after the index drop: err=%v", err)

	// Recreating "from the schema" now fails.
	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE UNIQUE INDEX orders_unique_ref_idx ON %s.orders (reference)", schema))
	t.Logf("recreate the unique index: err=%v", err)
}

Observed:

duplicate insert after the index drop: err=<nil>
recreate the unique index: err=ERROR: could not create unique index "orders_unique_ref_idx" (SQLSTATE 23505)

This review was generated by Claude Code (claude-fable-5). Finding 1 was reproduced against a live PostgreSQL 16; findings 2–5 are reproduced offline with the test above.

[aparajon]

🤖 Approving on Armand's behalf. My two-lens review and adversarial correctness pass are posted above — the findings there are for follow-up, not fix-before-merge blockers.

This approval was submitted by Claude Code (claude-fable-5) at Armand's direction.

The base branch was changed.

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent (Amp / Claude Opus 4.5)

Three of five fixed in this PR's review-fixes commit; the two feature-scale asks (facts-aware lint, severity policy) are tracked as an explicit follow-up rather than widened into this PR.

#	Finding	Status	Explanation
2	Findings carry no line/column and report deparsed SQL, so CI can't annotate them	fixed	statement.Split now returns verbatim source text plus 1-based line/column derived from RawStmt offsets and the lexer's token positions (leading comments skipped). Findings carry line/column/verbatim sql; text output is the conventional name:line:col: shape.
1	Zero facts reports every ALTER COLUMN … TYPE as a table rewrite, including free ones	fixed (reporting) / deferred (facts)	The fail-closed case is now a distinct possible-table-rewrite code, driven by a planner-owned Decision.Unverified marker — the linter no longer asserts a property it cannot know. Facts-aware lint (--desired / optional --url) is tracked as follow-up E1d under PLAT-38440.
3	No severity policy — the gate only gates unsupported-operation	deferred (policy) / fixed (docs)	--error-on / per-code gating / inline suppression is follow-up E1d under PLAT-38440 — the suppression syntax deserves its own design pass. The codes are now a documented public vocabulary: docs/lint-report.md has the table with an example per code, and the exit-behavior section states the current policy explicitly.
4	Two contracts both claim format_version: 1 and share the Reason vocabulary	fixed	docs/lint-report.md states the two reports version independently, that lint findings embed the plan contract's Reasons set pinned by the lint format_version, and that unknown values fail the gate; docs/plan-report.md carries the reciprocal note.
5	lint has no notion of what it lints against	fixed	The report now stamps postgres_versions (from the new planner.RulesPostgresVersions, "14-18") so a stored report names the rule assumptions behind it.

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent (Amp / Claude Opus 4.5)

Findings 1, 2, 4, and 5 fixed; finding 3's root cause is a deliberate planner fail-closed posture, now documented, with the sharpening path tracked.

#	Finding	Status	Explanation
1	Index drops excluded from destructive on a justification false for unique indexes	fixed	Took the conservative direction suggested: all index drops are destructive offline. Implementation deletes lint's local isDestructive entirely — destructive findings now derive from the classifier's Decision.Destructive (which includes OpDropIndex), so the linter and plan report mark the same operations by construction and the "second drifting rulebook" failure mode is closed. The destructive code doc records the unique-index rationale.
2	Genuinely free widenings reported as table rewrites	fixed	The planner now marks fail-closed factless type changes Unverified; lint maps that to a new possible-table-rewrite code, separating "the engine would take the heavy path" from "this rewrites the table". A USING clause remains a proven table-rewrite. Pinned by planner and lint tests.
3	blocking-idiom findings frequently carry no suggestion	reply	Root cause is the planner's deliberate fail-closed stance: it does not construct multi-statement splits for multi-operation ALTERs (a partial rewrite would be misleading) nor ATTACH PARTITION's proving CHECK (needs catalog knowledge it doesn't have offline). Routing fails closed on empty SaferSQL, so nothing executes on an unactionable finding. The suggestion field contract in docs/lint-report.md now states exactly when it is absent and why; the "two identical findings" shape is gone since operation labels carry column names. Facts-aware lint (tracker E1d, PLAT-38440) is where offline-unconstructible suggestions can sharpen.
4	DROP INDEX findings name no index	fixed	Landed on main via the #6/#7 review fixes (dropIndexNames populating Op.Name) and inherited here through the base sync — the finding now reads "operation": "DROP INDEX orders_unique_ref_idx".
5	Reported SQL is deparsed, not verbatim, and has no position	fixed	Split returns verbatim source with line/column (first code token, leading comments skipped; the byte offsets are used at exactly the point they were being discarded). Your positions probe now reports line: 6, column: 1 with the author's spelling of the statement.