add make demo: runnable CLI tour and artifact smoke test

[Kiran01bm]

Adds make demo: a runnable tour of the CLI that doubles as CI's artifact smoke test for the built bin/pg-sprite binary.

Why

The Go suite tests the code, not the artifact: nothing exercised the shipped binary the way a user invokes it — flag parsing, Kong wiring, exit-code mapping, JSON encoding on real stdout. There was also no low-friction way for a newcomer to see every planner route, the safer-sequence substitutions, and a structured refusal happen against a real database.

What

• demo/tour.sh — four sections: one statement per planner route/reason (dry-run), declarative diff plans, the offline commands (lint/suggest/fmt), and real executions ending in a backend-unavailable refusal (exit 2). CHECK=1 turns it into a smoke test asserting on --json fields and exit codes only — never prose.
• make demo (build → compose DB up → reseed → tour), make demo-seed, make demo-check.
• CI job "smoke test (built pg-sprite artifact)" running make demo-check; added to all-green.
• AGENTS.md binds the expectation rows to planner/CLI contract changes so the tour cannot silently rot.

Before:
┌──────────┐    go test     ┌───────────────┐
│ Go suite │ ─────────────▶ │ packages/code │      bin/pg-sprite: built, never run by CI
└──────────┘                └───────────────┘

After:
┌──────────┐    go test     ┌───────────────┐
│ Go suite │ ─────────────▶ │ packages/code │      (correctness oracle, unchanged)
└──────────┘                └───────────────┘
┌────────────────┐  CHECK=1  ┌───────────────┐  routes/verdicts/  ┌──────────────┐
│ demo/tour.sh   │ ────────▶ │ bin/pg-sprite │ ─────────────────▶ │ compose PG16 │
│ (make demo[-   │           │ (the artifact)│   exit codes +     │ seeded demo  │
│  check])       │           └───────────────┘   --json fields    │ tables       │
└────────────────┘                                                └──────────────┘

[chatgpt-codex-connector]

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

[morgo]

🤖 Approved on Morgan's behalf (agent review, liberal pg-sprite bar).

Verified:

• Hermetic by construction: the tour's DSN host is hardcoded localhost in the Makefile and passed explicitly on the recipe line, so an exported PG_DSN can't leak in; the only destructive SQL (DROP TABLE ... CASCADE in demo/seed.sql) runs via docker compose exec strictly inside the compose container; and if a real PostgreSQL already holds localhost:5432, db-up --wait fails to bind and make aborts before the seed or tour runs. Exec-section writes are additive only.
• CI smoke job is least-privilege: inherits permissions: contents: read, persist-credentials: false, checkout/setup-go SHAs byte-identical to the existing jobs, no new third-party actions, no secrets; docs-only-skip semantics of all-green preserved.
• No startup race: compose up --wait + 1s healthcheck gates the seed; seed fixtures line up with every check-mode assertion (all 10k emails non-null so SET NOT NULL validates, both diff plans exactly 2 statements).

Non-blocking nits:

1. demo: build db-up demo-seed relies on serial-make prerequisite ordering — under make -j, demo-seed can race db-up, and standalone make demo-seed fails if the DB isn't up. A demo-seed: db-up dependency fixes both. (CI runs serial, so unaffected.)
2. Individual tour sections assume the freshly seeded baseline — a second exec or standalone diff after one pass fails its expectations. make demo always reseeds, so just worth a line in demo/README.md.
3. In demo (non-check) mode the fmt step swallows a failure silently (empty output, no (exit $?) echo like the other steps). Cosmetic.
4. The dry-run exit-code assertion deliberately accepts 0 or 2 (documented "tighten later") — fine for a smoke test, remembering it for when the exit-code contract firms up.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by his agent. Reviewed at head 5a4fe35, with make demo-check run against a live compose PostgreSQL 16.14 and each assertion driven independently to see what it can and cannot distinguish.

Verdict: the job is the right idea and it works — but in check mode it cannot observe the one behaviour the product exists for. The tour asserts routes, reasons, dispositions and counts well. It does not assert a single substituted statement, so the safer-sequence machinery the PR body, demo/README.md and AGENTS.md all name as covered is, in check mode, unobserved. Four findings, two nits.

Findings

1. The smoke test cannot tell a safer substitution from no substitution at all. execute_native asserts .outcome == "executed-natively" and nothing else — and that same string is emitted whether or not a substitution happened. From the tour's own output: ADD COLUMN bio text runs as written, CREATE INDEX idx_users_email runs as CREATE INDEX CONCURRENTLY, and both print executed natively. The distinguishing field is right there in the JSON the tour already parses:

$ pg-sprite migrate --json --alter 'ALTER TABLE users ALTER COLUMN email SET NOT NULL'
{
  "outcome": "executed-natively",
  "executed_sql": [
    "... ADD CONSTRAINT \"users_email_not_null\" CHECK (\"email\" IS NOT NULL) NOT VALID",
    "... VALIDATE CONSTRAINT \"users_email_not_null\"",
    "... ALTER COLUMN \"email\" SET NOT NULL",
    "... DROP CONSTRAINT \"users_email_not_null\""
  ]
}

A regression that dropped CONCURRENTLY from the index build, or collapsed the four-step SET NOT NULL into the bare blocking form, keeps this job green — while demo/README.md says the exec section shows "the concurrent index substitution, the four-step SET NOT NULL sequence" and AGENTS.md binds the tour to safer-idiom changes. executed_sql is a typed JSON field, so asserting it is squarely inside the "never assert on prose" rule. One caveat that shapes the fix: PostgreSQL 18 can add NOT NULL NOT VALID directly, so the step list may not be version-stable — assert the shape (executed_sql | length, and that it contains CONCURRENTLY / NOT VALID) rather than literal SQL, or state that the demo job pins one major.

2. The dry-run exit code is accepted as either answer for every refusal. 5a4fe35 loosened the assertion to "0 or the refusal code 2" for non-execute dispositions, with a comment about spanning builds on both sides of the contract. At head there is only one side — I drove every non-execute disposition and all of them exit 2, deterministically:

statement	disposition	dry-run exit
ALTER TABLE orders ALTER COLUMN user_id TYPE bigint	unavailable	2
ALTER TABLE users ADD COLUMN joined timestamptz DEFAULT now()	unavailable	2
ALTER TABLE users SET TABLESPACE pg_default	unavailable	2
ALTER TABLE users ENABLE ROW LEVEL SECURITY	refuse	2
ALTER TABLE users ADD COLUMN nick text UNIQUE	rewrite-required	2

demo-check builds bin/pg-sprite from the tree it is testing, so there is no other supported build to span — the binary under test is always this commit's. The net effect is that the refusal exit code, one of the four things the PR body gives as the reason this job exists ("flag parsing, Kong wiring, exit-code mapping, JSON encoding"), is the one thing left unpinned. Tighten to exactly 2.

3. rewrite-required has no row, though the section header claims one statement per route and reason. The table covers execute, unavailable and refuse; the fourth disposition is missing, and it is the one carrying the guidance token that landed in #38 — so that field has no artifact coverage at all. One row closes it:

dry_run native  safer-idiom  false rewrite-required "ALTER TABLE users ADD COLUMN nick text UNIQUE"

(verified: disposition=rewrite-required, route=native, reason=safer-idiom).

4. The lint assertion is satisfied by any failure, including the binary not existing. run_offline checks "exit was non-zero" and ".errors is not 0". Run with a bogus PGS, both pass:

$ CHECK=1 PGS=/nonexistent/pg-sprite ... # lint block only
status=127 failures=0

out is empty, jq -r '.errors' yields empty, and empty != 0. It is not live-reachable — earlier sections die first — but it is the wrong assertion shape in the one job whose entire premise is "prove the artifact runs", and it is the block a contributor will copy when adding the next offline command. Assert the exit code lint actually uses and .errors == 1; the fixture is fixed, so the count is knowable.

5. (nit) PGS must be absolute and nothing says so. tour.sh does cd "$(dirname "$0")", so a relative binary path breaks after the cd:

$ PGS=bin/pg-sprite PG_DSN=... demo/tour.sh offline
demo/tour.sh: line 172: bin/pg-sprite: No such file or directory

The Makefile passes $(CURDIR)/bin/pg-sprite and is correct; demo/README.md points readers at "PGS and PG_DSN set — see the Makefile", which is where they will get it wrong. Resolve PGS before the cd, or say "absolute path" in the :? message — the :? text is already the right place for it.

6. (nit) demo-seed is not ordered after db-up. demo: build db-up demo-seed relies on left-to-right prerequisite order, which make -j does not guarantee; the seed would then exec into a container that is not up. demo-seed: db-up makes the dependency real, and it is also the honest statement of what the target needs when invoked on its own (which the Makefile comment invites).

Action items

1. (Finding 1) Assert executed_sql in execute_native — a step count and a fragment (CONCURRENTLY, NOT VALID) — so the substitution itself is what the artifact test observes. Without it, the safer-idiom contract that AGENTS.md binds to this file is not actually pinned by it.
2. (Finding 2) Require exit 2 for non-execute dispositions; the build under test is always this tree's.
3. (Finding 3) Add the rewrite-required row.
4. (Finding 4) Assert lint's specific exit code and error count rather than "something failed".
5. (nits) Absolute-path PGS (5) and a real demo-seed: db-up edge (6).

Verified (tried to break, couldn't)

The tour is genuinely rerunnable: make demo-check twice in a row is clean because demo-seed is a prerequisite and seed.sql drops and recreates, and the dry-run section takes no locks and writes nothing, so section order is free. Check mode fails closed on a missing jq before doing anything (line 26), and the assertion helper's empty-string comparison means a crashed binary produces an empty out and a failed assert rather than a silent pass, in every block except finding 4's. set -euo pipefail is correct throughout — the out=$(...) || status=$? form is used consistently so set -e never eats a deliberate non-zero exit, pipefail covers the one pipeline (fmt on stdin), and failures accumulates rather than short-circuiting so a run reports every mismatch at once. The route/reason/destructive expectations all match live behaviour on 16.14, including the two that are easy to get wrong: DROP COLUMN status is metadata-only with destructive=true rather than a route of its own, and CREATE INDEX CONCURRENTLY is online-idiom and not safer-idiom. The CI wiring is right: demo is in all-green's needs, and the changes filter counts demo/** as code (it excludes only **/*.md and docs/**) so a PR touching only tour.sh or a fixture still runs the job — the one skip case is a demo/README.md-only change, which is correct. ubuntu-latest ships jq, the job pins the same action SHAs as its neighbours, and persist-credentials: false matches the rest of the file. seed.sql is defensible as a fixture: identity keys so diff never meets a sequence-backed default it must refuse, 10k rows so VALIDATE and the index build do real work, and every email non-null so the SET NOT NULL step's validate can pass — the comments say so, and both claims hold.

One thing outside this diff, surfaced by reading the tour's real output: pg-sprite lint risky.sql prints findings anchored at an absolute path (/Users/<you>/…/demo/risky.sql) even when given a relative one — Kong's existingfile mapper resolves it. Every other linter emits the path as given, because editors and CI annotations (::error file=…) anchor on repo-relative paths; absolutizing also puts the runner's home directory into public CI logs.

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

[aparajon]

🤖 Second pass on 5a4fe35, same two lenses as the earlier pg-sprite reviews, weighted this time toward adoption: what a stranger who has never heard of pg-sprite takes away from running make demo once. Correctness findings are in the adversarial comment. Read against a full live make demo run, not just the diff.

Lens 1 — OSS adoption

Show the starting schema before the first statement. Right now the tour opens with ALTER TABLE users ADD COLUMN bio text and the viewer has no idea what users is. Every classification in that section is a function of the current schema — binary-coercible is only true because name is varchar(50), metadata-only on DROP COLUMN status only because nothing depends on it, type-rewrite only because orders.user_id is integer — so without the baseline on screen the section reads as assertions rather than reasoning. Printing the seeded tables (and their row counts) as section 0 turns the whole tour from "the tool says things" into "the tool is reading my schema and reasoning about it", which is the entire pitch. It also makes the diff section land: right now desired/users_v2.sql produces two statements and the viewer has to infer the current shape backwards from the plan.

And pg-sprite cannot currently do that to its own fixtures — which is the more interesting finding. The obvious implementation is pg-sprite fmt demo/seed.sql, and both halves fail:

$ pg-sprite fmt demo/seed.sql
pg-sprite: error: input contains comments, which formatting would discard   # exit 1

$ pg-sprite fmt demo/desired/users_v2.sql
CREATE TABLE users (id bigint GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY, email text, name varchar(50), status text DEFAULT 'active', bio text);

Two things worth separating out of that:

fmt flattens; every developer alive expects it to pretty-print. The input to that second command is a nicely aligned multi-line CREATE TABLE; the output is one 140-column line. The name fmt sets a hard expectation set by gofmt, prettier, black, and rustfmt — canonical and readable, run in a pre-commit hook, diff-friendly one-declaration-per-line. pg-sprite's fmt is canonical and less readable than what it was given, and it produces a single-line diff hunk for any column change. I would rank this the highest-leverage adoption fix in the tool, above anything in this PR: fmt is the only command with genuinely zero friction — no database, no Docker, no clone, no risk — so it is the natural first contact and the one that earns a place in someone's repo. A tool that is already in the pre-commit hook is the tool people reach for when they need the dangerous thing done safely.

fmt refusing commented input means it cannot format a real schema file. Every checked-in schema has comments. The tour works around this by feeding fmt a synthetic one-liner over stdin (with a comment in tour.sh explaining the workaround), which is the weakest showcase in the run — and the workaround is the signal. Refusing rather than silently dropping comments is the right call given the deparser; the fix is for the deparser to carry them, and until then the error should say what to do next rather than only what went wrong.

Fixing both would let the demo open with pg-sprite fmt demo/seed.sql, which shows the baseline schema and demonstrates fmt on a real file, replacing the stdin one-liner. One artifact, two jobs, and the tour stops needing a workaround comment.

The tour buries its headline and closes on a limitation. The single most persuasive fact about pg-sprite — you wrote CREATE INDEX, it ran CREATE INDEX CONCURRENTLY; you wrote SET NOT NULL, it ran a four-step online sequence — is section 4, roughly 350 lines in. The last line a first-time viewer sees is:

(refused, exit 2 — expected: this route's backend is not available yet)

The final impression of the tour is a feature that does not exist yet. That refusal is honest and belongs in the run — it already appears three times in the classification section — but it should not be the closing frame. Two cheap changes: lead with exec (show the substitution, then explain the classification that produced it), and end with a summary — N statements classified, 3 executed, 0 blocking locks held, 4 refused before they could hurt you. Right now make demo just stops.

Roughly a third of the dry-run section is repeated chrome. plan: … — 1 statement, 1 step to run, 0 refused, dry-run: nothing was executed, and apply: re-run without --dry-run print on all fourteen rows and carry no new information after the first. That is ~42 lines of a ~350-line tour spent restating the invocation. A compact dry-run rendering — or the tour selecting one — would let the classification section read as the table it conceptually is, which is also how someone skims a README GIF.

The most persuasive demo is the one that is missing: lock contention. detail: committed within budgets (lock 3s, statement 30s) is the first and only mention of lock budgets and it goes by in a single line. Every Postgres user who would adopt this tool has been burned by exactly one thing — a schema change that queued behind a lock and took the table down with it. A section that opens a competing transaction, runs a change, and shows pg-sprite backing off instead of joining the queue would be worth more than the entire classification section for adoption, because it is the only part a reader cannot get from reading docs. It is also the clearest statement of what pg-sprite is for, as opposed to what it knows.

The zero-friction path is documented but not sequenced. Correcting myself here: the ## Commands table added in #40 does name it — "Half the CLI works offline on DDL text alone", with fmt and lint marked none — so this is an ordering point, not a missing one. The table sits at README line 155 and the Demo section right after it leads with make demo and Docker, while the tour's own offline section is third of four and framed by mechanism ("offline commands") rather than by what it buys you. For a stranger, first contact should be go install then pg-sprite lint <their own file> — a useful answer about their schema in ten seconds — and only then the full tour. That ordering is also what makes the no-cgo promise the build job already guards pay off.

Smaller things from the live run. The docs: links all point at blob/main, and a single tour prints about twenty of them — same note as #36–#38, now multiplied, and a released binary can point a user at a page describing behaviour it does not have. detail: … all 1 steps committed (internal/cli/migrate.go:284) needs a plural guard; it appears on the showcase path of the very first run. And the header comment diff emits — "running this script directly bypasses that gate" — is the best sentence in the whole run; it is doing real work and is worth keeping verbatim wherever else a plan is rendered.

Lens 2 — the seam an orchestrator consumes

This job is now the only thing in the repo that exercises the JSON contracts as a shipped artifact, which makes it the natural place to pin what the Go tests structurally cannot: that plan, suggest and verdict still decode as an outside consumer will decode them, from a real stdout, out of a real process. The tour already asserts .disposition, .statements[].route, .outcome and .reason — the missing sibling is format_version on the two contracts that carry it. An adapter that pins a version gets no signal today when the field it pins moves; asserting it here costs one line per front door and turns a silent break into a red job. It is also the first place I would notice that verdict still has no version to assert (raised on #38, still open).

The AGENTS.md binding is the right mechanism and the right wording — "a contract with the planner and CLI, not documentation" is exactly the framing that keeps this file from rotting into a README. Its force depends entirely on the rows actually observing the contract, which is finding 1 in the other comment.

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

[aparajon]

🤖 Approving on @aparajon's behalf. Adding an artifact smoke test to all-green is the right call and the wiring is correct — nothing here can reach a user's database, so the findings are coverage and adoption rather than gates.

The one I would not let slide past this PR is finding 1 in the adversarial comment: in check mode the tour asserts .outcome, and executed-natively is emitted whether or not a safer substitution happened — so the safer-sequence behaviour that AGENTS.md binds to this file is not actually observed by it. executed_sql is already in the JSON and is exactly the typed field the rules permit asserting on.

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

[Kiran01bm]

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

Summary: All four correctness findings and both nits are fixed in <commit>; the orchestrator-lens format_version gap is fixed in the same commit; the adoption-lens items (fmt behaviour, demo restructuring, docs-link pinning, verdict versioning) are tracked as internal follow-ups.

Adversarial correctness review (comment)

#	Finding	Status	Explanation
1	Smoke test can't distinguish a safer substitution from no substitution	fixed	execute_native now takes an expected step count + fragment; executed_sql is omitempty, so 0 pins an as-written run and the substitutions pin 1/CONCURRENTLY and 4/NOT VALID — shape, not literal SQL, with a comment noting the shape is stable because the demo compose pins one PostgreSQL major.
2	Dry-run exit accepted as 0 or 2 for every refusal	fixed	Non-execute dispositions now assert exactly 2; the binary under test is always this tree's build, as you noted.
3	rewrite-required has no row	fixed	Added your verified row (ADD COLUMN nick text UNIQUE → native / safer-idiom / rewrite-required, exit 2).
4	Lint assertion satisfied by any failure, including a missing binary	fixed	Asserts exit exactly 1 and .errors == 1; suggest similarly pins count 2 — the fixtures are fixed, so the counts are knowable.
5	(nit) relative PGS breaks after the cd	fixed	A relative path containing / is anchored to $(pwd) before the cd; a bare command name still resolves via PATH.
6	(nit) demo-seed not ordered after db-up	fixed	demo-seed: db-up is now a real prerequisite edge.
—	(outside diff) lint absolutizes given paths into findings and CI logs	deferred	Tracked as an internal follow-up (CLI rendering polish) — emit the path as given.

The "verified (tried to break, couldn't)" section — rerunnability, fail-closed jq gate, set -euo pipefail discipline, CI wiring, seed fixture claims — no action; thanks for driving each assertion independently.

Adoption / orchestrator review (comment)

#	Finding	Status	Explanation
1	format_version unasserted on the shipped-artifact contracts	fixed	Asserted (== 1) on the plan report (every dry-run row), diff, lint, and suggest.
2	Tour buries the headline and closes on a refusal	fixed (partial)	make demo now ends on a "Tour complete" summary frame naming the substitutions; leading with exec needs reseeds between sections → tracked with the demo adoption pass below.
3	fmt flattens instead of pretty-printing	deferred	Tracked as an internal follow-up (R10), recorded with your ranking as the highest-leverage adoption fix.
4	fmt refuses commented input	deferred	Tracked as an internal follow-up (R11): carry comments through the deparser; better refusal text until then.
5	Baseline-schema opener, lock-contention section, README zero-friction sequencing	deferred	Tracked as one demo adoption pass (R12); the baseline opener depends on the fmt comment fix.
6	Dry-run repeated chrome; all 1 steps committed plural	deferred	Tracked as CLI rendering polish (R13).
7	docs: links point at blob/main	deferred	Tracked (R14); wants the release-tag plumbing — the -ldflags version stamp already exists.
8	Verdict still has no format_version	deferred	Tracked (R15); a contract change that gets its own PR with docs and demo assertions.