docs+lint: safer forms are not semantic equivalents

[Kiran01bm]

Summary

CREATE INDEX and CREATE INDEX CONCURRENTLY converge on the same declared end state but
are not semantic equivalents — different locking, transactionality, and failure modes.
This PR sweeps docs, API comments, and CLI output so every safer-rewrite recommendation
reads as an advisory safer form the engine owns executing, never as an equivalent or an
instruction to run manually. No behavior change.

What

• docs/design-principles.md, docs/high-level-design.md, docs/low-level-design.md:
  replace "native equivalent" / "safer equivalent" with "safer native form" and state the
  operational differences (a failed CONCURRENTLY build leaves an INVALID index the
  executor must detect via pg_index.indisvalid and recover); the HLD advisory diagram now
  says safer form: instead of run instead:.
• pkg/planner (Decision.SaferSQL) and pkg/lint (CodeBlockingIdiom,
  Finding.Suggestion) doc comments carry the same caveat.
• CLI output: lint prints safer form (not equivalent — see docs/postgres-online-ddl-reference.md); diff dry-run prints safer form the engine would run (not equivalent — …) instead of the engine would run instead:.

Why

A recommendation that prints "run instead: CREATE INDEX CONCURRENTLY …" invites an operator
to paste it into psql — bypassing the engine's execution-time guards (invalid-index
detection and recovery) while implying identical semantics. The distinction is declarative
vs. operational: from a desired-state diff standpoint the end state is identical
(CONCURRENTLY is not catalog state), but the execution paths fail differently, and only
the engine owns the recovery. Wording everywhere now matches the execution-safety model in
docs/postgres-online-ddl-reference.md.

[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, #9, #7, #2): 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 4e54c6e. A short adversarial pass is posted separately.

This is a small PR making a distinction that most tools in this space never make at all, and it's the right one. "Converges on the same declared end state" and "is a semantic equivalent" are genuinely different claims, and collapsing them is how operators end up pasting a CONCURRENTLY build into psql, having it fail at 80%, and leaving an INVALID index nobody notices until a query plan gets slow. Naming pg_index.indisvalid as the executor's obligation, in the doc comment of the field that produces the recommendation, is exactly where that knowledge belongs — it's the note the person implementing Phase 3 will actually read. safer form: over run instead: is a small wording change carrying a real behavioral claim, and the surrounding docs now explain rather than assert it.

OSS lens

1. The point being made argues for saying more, not less, about what the operator should do today. The new text tells an operator that running the safer form by hand forgoes the engine's guards — but the executor that provides those guards is Phase 3, and at this head there is nothing that can run the sequence for them. So the honest reading of lint's output right now is "here is a safer form; don't run it yourself; nothing else can run it either". One clause fixes it: name the guard the operator is taking on (after a CONCURRENTLY build, check pg_index.indisvalid and REINDEX/drop-and-rebuild if invalid), or state that engine-owned execution lands in Phase 3. The project's honesty discipline elsewhere — the *Planned enforcement:* sweep on Docs: vision, architecture, status sweep; tag-driven release machinery #2 — is exactly this instinct applied to docs; this is the same thing applied to CLI output.

2. The caveat should be a link, not a repo path. Both new output strings point at docs/postgres-online-ddl-reference.md, which is a valid path in a checkout and nothing at all to someone who downloaded a release archive — .goreleaser.yaml declares no files:, so the archives carry GoReleaser's defaults (LICENSE/README/CHANGELOG) and no docs/ tree. A URL pinned to the tag would work for both audiences, and the reference doc is genuinely worth reading, so it's worth being reachable.

Integration lens

3. The sweep fixed the human surface and left the machine surface untouched, and the machine surface is the one that gets automated. The whole argument is that these sequences carry different transactionality and failure modes and therefore need engine-owned execution — but lint --json's suggestion and plan's exec_sql still emit the bare SQL array with nothing marking any of that. A consumer building on the JSON (the stated purpose of both contracts) sees a list of statements and the obvious thing to do with a list of statements is run it, quite possibly wrapped in a transaction, which for CREATE INDEX CONCURRENTLY fails outright. This is the same gap I raised on Phase 2.3-2.4: classifier and router seam #7 from the other direction, and this PR's reasoning is the strongest argument yet for closing it: a per-step shape ({sql, in_transaction: false}) or a requires_engine_execution flag would put the caveat where the automation can see it. Worth doing before suggest widens the surface further.

Verified solid

The sweep is thorough on the surfaces it targeted: I grepped the tree at this head and the remaining uses of "equivalent" are all either the new deliberate ones ("is not a semantic equivalent") or unrelated senses ("the PostgreSQL equivalent of MySQL's InnoDB Online DDL reference", "the equivalent of Spirit's TiDB parser"). run instead survives in exactly one place (noted in the adversarial comment). The claim itself checks out — a CONCURRENTLY build genuinely cannot run inside a transaction block and genuinely leaves indisvalid = false on failure, so this is a real distinction and not pedantry. Keeping it to wording plus doc comments, with no behavior change, makes it trivially reviewable. 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. Reviewed at head 4e54c6e. This is a wording-and-doc-comment sweep with no behavior change, so the attack is completeness: did the sweep reach every surface that makes the claim it is correcting? Findings are static against the tree at this head.

Findings, most severe first

1. The JSON surfaces still hand out the sequence with none of the caveat. writeLintText and writeChangeText now both carry "not equivalent"; lint --json's suggestion and the plan report's exec_sql are unchanged — a bare []string of SQL. The PR's own argument is that these sequences are non-transactional and fail in ways that require engine-owned recovery, which makes an automated consumer the most important audience for that caveat and the only one that didn't get it. The failure is concrete rather than theoretical: a consumer that wraps exec_sql in a transaction (the natural thing to do with an ordered statement list) fails immediately on CREATE INDEX CONCURRENTLY, and one that doesn't wrap it has no way to know it must check pg_index.indisvalid afterward. A field on the wire — a per-step in_transaction: false, or a requires_engine_execution marker — puts the claim where the automation can act on it.

2. Both new output strings cite a path that isn't present in an installed build. docs/postgres-online-ddl-reference.md resolves in a git checkout and nowhere else: .goreleaser.yaml declares no files: block, so release archives get GoReleaser's defaults — LICENSE*, README*, CHANGELOG* — and no docs/ directory. Anyone who installed pg-sprite from a release tarball is directed to a file that does not exist on their machine, by the output of the command that just warned them to be careful. A tag-pinned URL serves both audiences.

3. run instead survives in the one place the sweep was aimed at. Decision.SaferSQL's doc comment still opens "the ordered native sequence to run instead of the submitted form", with the new not-an-equivalent caveat appended after it:

// SaferSQL is the ordered native sequence to run instead of the
// submitted form, present only for safer-idiom decisions where the
// planner could construct it. It is a safer form, not a semantic
// equivalent: …

The first sentence still says the thing the PR is removing everywhere else, and it's the definition a consumer reads first. Cosmetic, but it's the field the whole sweep is about.

4. The advice has no addressee while the executor doesn't exist. "Running it by hand forgoes the engine's execution-time guards (invalid-index detection after a concurrent build)" is true and useful — but at this head there is no executor, so an operator acting on a blocking-idiom finding today has exactly one option, and it is the one being warned against. The warning as written leaves them with a hazard and no procedure. Naming the manual check (pg_index.indisvalid, then rebuild) or the phase that removes the need for it turns a caution into instructions.

Probed and held

The sweep is genuinely complete on the surfaces it aimed at: grepping the tree at this head, every remaining "equivalent" is either one of the new deliberate uses or an unrelated sense (postgres-online-ddl-reference.md's "the PostgreSQL equivalent of MySQL's InnoDB Online DDL Operations", low-level-design.md's "the equivalent of Spirit's TiDB parser", tcb-model.md's "this is our equivalent"). The technical claim underpinning the whole PR holds: CREATE INDEX CONCURRENTLY cannot run in a transaction block, and a failed build leaves indisvalid = false rather than rolling back — so "same end state, different failure modes" is accurate, and pg_index.indisvalid is the right catalog check to name. No behavior changed: the only non-comment edits are two format strings. CGO_ENABLED=0 go build ./... passes at this head.

This review was generated by Claude Code (claude-fable-5). Findings are static analysis of the tree at this head; no runtime behavior changed in this PR.

[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.

[Kiran01bm]

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

One-line summary: all four findings across both passes are fixed — the execution-semantics caveat is now a typed, machine-readable contract on every JSON surface that hands out a sequence (safer_sql_execution, suggestion_execution, execution), the CLI cites a reachable URL instead of a repo path, the surviving "run instead" doc comment is reworded, and the operator caveat now names the recovery check it warns about.

#	Finding	Status	Explanation
A1 / T3	JSON surfaces (lint --json suggestion, plan exec_sql) hand out bare SQL with none of the caveat — the automated consumer is the most important audience and the only one that didn't get it	fixed	Added a typed execution contract, not prose: planner.Execution with the closed vocabulary value autocommit-each-step (each step in its own implicit transaction, never inside an enclosing transaction block; a failed step leaves partial state the runner must detect and recover). It rides three surfaces — Decision.safer_sql_execution, Finding.suggestion_execution, and statement-level execution beside exec_sql — each present exactly when its sequence is. Stamped centrally in Classify so a constructed sequence can never ship without it. Documented in both contract pages (field rows plus an Execution contracts vocabulary section, enforced by the existing doc-drift and vocabulary tests); excluded from the plan fingerprint like the other explanatory fields, so pinned plans keep their identity. Additive under format_version: 1.
A2 / T2	Both new output strings cite docs/postgres-online-ddl-reference.md, a path that doesn't exist in an installed build (release archives carry no docs/ tree)	fixed	Both strings now cite https://github.com/block/pg-sprite/blob/main/docs/postgres-online-ddl-reference.md via a shared constant whose doc comment records why a URL rather than a repo path. Pinned by a renderer unit test. A tag-pinned URL needs version plumbing the binary doesn't have yet; main serves both audiences until it does.
A3	Decision.SaferSQL's doc comment still opens "the ordered native sequence to run instead of the submitted form" — the exact wording the sweep removes everywhere else, on the field the sweep is about	fixed	Reworded to "the ordered safer native sequence … a safer form of the submitted statement, not a semantic equivalent", merged with main's execution-contract paragraph and pointing at the new SaferSQLExecution field. The same wording survived in both contract docs' field tables (safer_sql, suggestion) — reworded there too.
A4 / T1	The caveat warns an operator off running the safer form by hand but leaves them no procedure while the executor doesn't exist	fixed	The lint suggestion block gains a second line naming the contract and the guard: run each statement in its own transaction, never one block; after a failed CONCURRENTLY build, check pg_index.indisvalid and rebuild. The diff plan comment names the per-step transaction contract inline (its header already routes execution through pg-sprite migrate). Chose naming the guard over citing a roadmap phase — output that references future phases goes stale.