From f4a4db818562fe1fe5ba3a6b502cfaf955883ec1 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Sat, 12 Sep 2026 12:29:11 +1000 Subject: [PATCH 1/4] docs: bring the design and testing pages in line with the shipped code A sweep of the docs against the code found statements that the implementation has overtaken: - capabilities-contract: pkg/verdict emits the refusal class on every refusal; the page said it did not yet. - testing: TM-8 is met by CI's demo job (make demo-check); the how-to-run table lists demo-check, check-capabilities, and the replay targets; the coverage table covers the executor, migrate, suggest, pull, capabilities, progress, and Supabase suites; Phase 3 obligations are recorded as done. - optimistic-attempt: a lock-timeout overrun is retried under the bounded RetryPolicy (three attempts by default, --lock-attempts 1 disables it); a statement-timeout overrun is never retried. The page said the form ran once. - cli-output-examples: create-name-mismatch and create-names-unverified are executor failure codes on a failed verdict (exit 1), not refusal reasons; the destructive example named DROP TABLE, which never reaches classification. - low-level-design and architecture: the package layout, the eight CLI commands, the executor's scope, and the plan report's versioning as shipped; the declarative front door is diff --desired, not a migrate flag. - SAFETY: internal/cli row lists pull and capabilities. - schemabot-integration: names schemadiff.ListManagedTables as the exported catalog query for undeclared-table enumeration. - capabilities, limitations, vision: the accepted-blocking primitive ships as a library call with exit 3 and no CLI flag yet, so "planned", "must run outside pg-sprite", and "every capability is reachable from the CLI" are qualified accordingly. Amp-Thread-ID: https://ampcode.com/threads/T-01a07fb2-9632-732b-bd7b-2f143a81bc06 --- SAFETY.md | 2 +- docs/architecture.md | 22 ++++++++++++---------- docs/capabilities-contract.md | 9 +++++---- docs/capabilities.md | 19 ++++++++++++------- docs/cli-output-examples.md | 12 ++++++++++-- docs/limitations.md | 2 +- docs/low-level-design.md | 30 +++++++++++++++++++++--------- docs/optimistic-attempt.md | 29 ++++++++++++++++++++--------- docs/schemabot-integration.md | 7 ++++--- docs/testing.md | 31 ++++++++++++++++++++++++++----- docs/vision.md | 10 ++++++---- 11 files changed, 118 insertions(+), 55 deletions(-) diff --git a/SAFETY.md b/SAFETY.md index c7fe0f3..6b31109 100644 --- a/SAFETY.md +++ b/SAFETY.md @@ -32,7 +32,7 @@ The invariant registry (invariant IDs referenced below) lives in | `pkg/capabilities` — embedded, validated support matrix and Markdown rendering | ❌ periphery | exists | — | | `pkg/diffplan` — desired schema → routed convergence plan, the declarative front door as a library (the CLI `diff` and embedding orchestrators share it) | ❌ periphery | exists | — | | `pkg/migrate` — one gated statement → resolve, classify, route, execute → one verdict; the imperative front door as a library (the CLI `migrate` and embedding orchestrators share it), plus the desired-state execution loop (`RunDesired`: derive the convergence plan, admit it as a whole, run each planned statement back through the same pipeline) | ❌ periphery² | exists | — | -| `internal/cli` — CLI, flags, help, prompts | ❌ periphery | `migrate`, `status`, `diff`, `fmt`, `lint`, and `suggest` exist | — | +| `internal/cli` — CLI, flags, help, prompts | ❌ periphery | `migrate`, `pull`, `diff`, `fmt`, `lint`, `suggest`, `capabilities`, and `status` exist | — | | `pkg/progress` — strategy-wide progress snapshots; the executors' observation seam (core imports it, so its locking discipline is core-critical); copy counters reserved for later | ✅ core | native progress exists | — | | orchestrator adapter | ❌ periphery | planned (Phase 11) | OC-* hold *at* the boundary | | `internal/testutil` | ❌ test-only | exists | — | diff --git a/docs/architecture.md b/docs/architecture.md index bb85b86..5aa0df3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -21,18 +21,19 @@ pg-sprite is a decoupled **planner → router → executor** engine. The planner changes, the router decides *which strategy*, interchangeable executors decide *how*. The planner is itself a pipeline of five distinct stages. The two front-ends enter it at -different points — an imperative `--alter` already *is* DDL, so it goes straight to parse; -a declarative `--desired` schema must first be compared against the live database to +different points — an imperative `migrate --alter` already *is* DDL, so it goes straight to parse; +a declarative `diff --desired` schema must first be compared against the live database to *produce* DDL — and the derived statements then re-enter the parse boundary like any hand-written statement, so both routes converge on the same parse → classify → lint tail and every operation is judged by the same rules regardless of how it arrived: ``` - user: --alter "ALTER TABLE …" user: --desired schema.sql + user: migrate --alter "ALTER TABLE …" user: diff --desired schema.sql (imperative: statements) (declarative: whole schema) │ │ ╭─────────▼──────────────────────────────────────▼─────────╮ - │ CLI: migrate · diff · fmt · lint · suggest · status │ + │ CLI: migrate · pull · diff · fmt · lint · suggest · │ + │ capabilities · status │ ╰─────────┬──────────────────────────────────────┬─────────╯ │ │ ┌──────────────▼───── PLANNER (shared front-end) ─────▼──────────────┐ @@ -161,10 +162,11 @@ architectural decision; this is the permission slip. Two integration surfaces ex different levels of commitment: - **The CLI and its JSON output** — the intended seam for orchestrators. `diff` and - `--dry-run` emit machine-readable verdicts and plans; the plan report freezes as a - single versioned contract (an explicit schema-version field, additive-only changes - within a version) in Phase 2.5. Until that lands its shape may change in any PR — wait - for the versioned report rather than pinning the interim shape. + `--dry-run` emit machine-readable verdicts and plans; the plan report is a single + versioned contract (`format_version`, `plan.FormatVersion`; additive-only changes + within a version, a consumer rejects a version it does not understand), documented in + [plan-report.md](plan-report.md). The suggest report and `capabilities --json` carry + their own version fields on the same rule. - **The Go packages** — everything under `pkg/` is importable, and the front-end seams (`pkg/statement`, `pkg/schemadiff`, `pkg/planner`, `pkg/router`, `pkg/verdict`) are each designed as a standalone entry point; `internal/` is unimportable by @@ -177,8 +179,8 @@ different levels of commitment: | Package | Role | Status | | --- | --- | --- | -| `cmd/pg-sprite` | CLI entry point (Kong): `migrate` · `diff` · `fmt` · `lint` · `suggest` · `status` | all six exist | -| `internal/cli` | Command tree and flag handling (including `migrate --dry-run`) | all six exist | +| `cmd/pg-sprite` | CLI entry point (Kong): `migrate` · `pull` · `diff` · `fmt` · `lint` · `suggest` · `capabilities` · `status` | all eight exist | +| `internal/cli` | Command tree and flag handling (including `migrate --dry-run`) | all eight exist | | `internal/testutil` | Test harness: containerized PostgreSQL, throwaway schemas | exists | | `pkg/dbconn` | Pool with bounded session timeouts, retries, RDS/Aurora auto-TLS (embedded CA bundle), terminate-blockers; advisory-lock mutual exclusion lands here | exists | | `pkg/statement` | `go-pgquery` (Wasm `libpg_query`) parse boundary, typed per-operation descriptors, and advisory rewrites (never hand-parse SQL); shadow DDL is validated by executing the retargeted statement on the empty shadow, and fingerprints come from `pkg/schemadiff`'s transaction-scoped scratch schema — execute-and-introspect, never AST surgery | exists | diff --git a/docs/capabilities-contract.md b/docs/capabilities-contract.md index b377898..be4ba21 100644 --- a/docs/capabilities-contract.md +++ b/docs/capabilities-contract.md @@ -203,10 +203,11 @@ understand. ## Shared refusal vocabulary -A refusal `class` on the verdict JSON is a separate decision, recorded in -[refusal-classes.md](refusal-classes.md); `pkg/verdict` does not emit one today. When that -field ships, that contract owns the vocabulary and the matrix uses the same words, so a -consumer reading a row and a consumer reading a verdict reach the same route: +A refusal `class` on the verdict JSON is a separate contract, recorded in +[refusal-classes.md](refusal-classes.md) and emitted by `pkg/verdict` on every refusal +(`Verdict.WithRefusal`; the reason → class mapping is the registry in `pkg/migrate`). That +contract owns the vocabulary and the matrix uses the same words, so a consumer reading a +row and a consumer reading a verdict reach the same route: | Matrix row | Refusal `class` | | --- | --- | diff --git a/docs/capabilities.md b/docs/capabilities.md index cc479ce..70ee0c4 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -333,13 +333,18 @@ where one exists — the exact safer sequence or the statement an operator can r deliberately, outside the engine, in a maintenance window. The operator stays in control; the engine stays honest. `--force` never bypasses a policy refusal. -A constrained variant is **planned**: an explicit, dedicated flag (distinct from -`--force`) that executes an otherwise-refused change through the engine's own bounded -`lock_timeout` sessions ("unsafe DDL under a bounded lock budget", which raw psql does -not give you), with the refusal analysis still printed before execution and the verdict -unmistakably marked as executed without an online-safety guarantee. The plain success -contract stays reserved for online-safe paths, and refusals for unrecognized SQL are -never eligible — only changes the engine understands but cannot run *safely*. +A constrained variant exists as a **library primitive, with no CLI flag yet**: an +explicit, dedicated acceptance (distinct from `--force`) executes an otherwise-refused +change through the engine's own bounded `lock_timeout` sessions ("unsafe DDL under a +bounded lock budget", which raw psql does not give you), with the refusal analysis still +produced before execution and the verdict unmistakably marked as executed without an +online-safety guarantee (`outcome: executed-without-online-safety`, exit 3 — never 0). +`executor.ExecuteAcceptedBlocking` runs it and `Verdict.WithAcceptedBlocking` records it; +the refused statement's `blocking_passthrough_eligible` field on the plan report says +whether a refusal qualifies. Only changes the engine understands but cannot run *safely* +are eligible — refusals for unrecognized SQL never are. A `migrate` flag that reaches the +primitive is the remaining step; until it lands the CLI exits 2 for these refusals. The +design is [lock-budgeted-passthrough.md](lock-budgeted-passthrough.md). ## Deliberately operator-owned diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index cc33a98..68202cc 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -71,7 +71,7 @@ is a one-line summary; the linked reference entry is authoritative. | [`unsupported-partitioned-parent`](postgres-online-ddl-reference.md#unsupported-partitioned-parent) | The routed plan builds an index concurrently but the target is a partitioned parent, where PostgreSQL cannot `CREATE INDEX CONCURRENTLY`. Refused. | | [`unsupported-statement`](postgres-online-ddl-reference.md#unsupported-statement) | The planner knows no safe path for the statement (for example `SET UNLOGGED`, `CLUSTER ON`). Refused — the same typed reason the run path's refusal verdict carries. | | [`table-not-found`](postgres-online-ddl-reference.md#table-not-found) | The target table does not exist, so classification fell back to zero facts; running without `--dry-run` would fail. The dry run exits 2 and the report carries `table_exists: false`. | -| [`destructive`](postgres-online-ddl-reference.md#destructive) | The change discards live data or structure (`DROP COLUMN`, `DROP TABLE`, truncating conversions). A warning alongside the routing decision, not a refusal. | +| [`destructive`](postgres-online-ddl-reference.md#destructive) | The change discards live data or structure — a dropped column, constraint, index, or `NOT NULL` (dropping a `DEFAULT` is not destructive). A warning alongside the routing decision, not a refusal; `DROP TABLE` never reaches classification, it refuses as `unsupported-statement`. | | [`blocking-idiom`](lint-report.md#codes-code) | Lint-only code: the submitted form blocks readers or writers and a safer native form exists; the finding's `suggestion` carries the safer SQL when the linter can construct it. | ## Refusal reasons @@ -93,7 +93,15 @@ in `detail`. The set is closed and pinned by test (`verdict.Reasons()`). | `destructive-change` | The desired-state plan discards live structure — a dropped column, constraint, index, or `NOT NULL` — and desired-state execution runs no destructive statement; run the drop deliberately instead ([execution model](execution-model.md)). | | `plan-fingerprint-mismatch` | The plan recomputed at execution time does not carry the pinned fingerprint: the plan a reviewer approved is not the plan that would execute, so nothing runs ([execution model](execution-model.md)). | | `create-collision` | The greenfield create plan's table name or a claimed index, constraint-index, or sequence name is occupied. Nothing runs; re-derive the plan against the live catalog to see what holds the name, then drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column — re-planning alone reproduces the refusal. Catalog absence checks handle existing occupants. Duplicate-name SQLSTATEs backstop races for explicit names; for server-chosen names, the probe narrows the race to the time-of-check window, and after the `CREATE TABLE` commits the executor reads the constraint-index and sequence names the table actually owns and compares them against the claimed first-choice names — a name taken inside the window makes the server pick a suffixed replacement, which surfaces as a typed `create-name-mismatch` failure at step 1 with the born table left in place for an operator to rename the relation or drop, then re-diff. | -| `create-names-unverified` | The `CREATE TABLE` committed but the read of the constraint-index and sequence names the table owns did not complete, so whether every first-choice claim was honoured is unknown. The born table is left in place; compare its names against the desired file, rename or drop, then re-diff. | + +Two codes that look like refusals are not: `create-name-mismatch` and +`create-names-unverified` are executor failure codes on a `failed` verdict (exit 1, +`failed_step` 1), because the `CREATE TABLE` has already committed when they arise. +The first means a claimed first-choice constraint-index or sequence name went to an +occupant inside the probe's window and the server suffixed it; the second means the +read of the names the table owns did not complete, so the claims are unproven, not +failed. In both the born table is left in place — compare its names against the +desired file, rename or drop, then re-diff. ## Migrate diff --git a/docs/limitations.md b/docs/limitations.md index 62b1f08..f34c38a 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -10,7 +10,7 @@ not escape hatches: | Change | Current behavior | | --- | --- | -| Index builds on a partitioned parent | PostgreSQL cannot build an index concurrently at the parent level. PostgreSQL supports a plain blocking build, but pg-sprite refuses it by policy because it takes `ACCESS EXCLUSIVE`; `--force` does not bypass this decision. An operator who chooses a maintenance-window blocking build must run it outside pg-sprite. The partition-aware `CREATE INDEX ON ONLY` → per-partition `CREATE INDEX CONCURRENTLY` → `ATTACH PARTITION` flow is planned but not yet implemented. | +| Index builds on a partitioned parent | PostgreSQL cannot build an index concurrently at the parent level. PostgreSQL supports a plain blocking build, but pg-sprite refuses it by policy because it takes `ACCESS EXCLUSIVE`; `--force` does not bypass this decision. An operator who chooses a maintenance-window blocking build runs it outside the CLI today; a library caller can run it through `executor.ExecuteAcceptedBlocking`, which marks the verdict as executed without online safety ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)). The partition-aware `CREATE INDEX ON ONLY` → per-partition `CREATE INDEX CONCURRENTLY` → `ATTACH PARTITION` flow is planned but not yet implemented. | | `ADD CONSTRAINT ... USING INDEX` on a partitioned parent | PostgreSQL does not support adopting an existing index on a partitioned parent in any supported version. pg-sprite refuses before execution. | | `ADD FOREIGN KEY ... NOT VALID` on a partitioned parent | PostgreSQL does not support this before version 18, so pg-sprite refuses it on versions 14–17. It is supported on version 18 and later. | | Copy-and-swap | The copy-and-swap backend is not yet available. Statements that require it route to `refuse`; pg-sprite never falls through to a blocking rewrite. | diff --git a/docs/low-level-design.md b/docs/low-level-design.md index 61ef8c7..57df542 100644 --- a/docs/low-level-design.md +++ b/docs/low-level-design.md @@ -185,7 +185,9 @@ pattern *per migration*: The classifier, declarative diff, dry-run, lint, and status reporting are shared by every backend. An `Executor` interface (`Plan`, `Execute`, `Status`, `Abort`) is also -planned; `pkg/executor` currently provides only the bounded optimistic native attempt. Until the +planned; `pkg/executor` currently provides the native backend — the bounded optimistic +attempt, the caller-owned concurrent index build with invalid-index recovery, the autocommit +safer-sequence runner, the greenfield create path, and the accepted-blocking primitive. Until the in-house copy-and-swap executor lands in a later phase, every `needs-rewrite` change is refused as **not native-safe** rather than delegated to an external tool. pgroll remains a possible still-later backend. @@ -618,7 +620,7 @@ These have **no MySQL counterpart** but are hard requirements for the logical-de > packages show the intended execution architecture. ``` -cmd/pg-sprite/ -> CLI (migrate, diff, fmt, lint, status) - Kong, like Spirit +cmd/pg-sprite/ -> CLI (migrate, pull, diff, fmt, lint, suggest, capabilities, status) - Kong, like Spirit Existing: pkg/statement/ -> Wasm go-pgquery boundary + typed operation descriptors and rewrites @@ -627,17 +629,26 @@ pkg/planner/ -> classify each operation and construct safer native SQL pkg/router/ -> assign classified statements to available backends pkg/plan/ -> versioned machine-readable dry-run plan report (both front doors) pkg/lint/ -> offline typed lint findings (errors refuse, warnings advise) -pkg/executor/ -> bounded optimistic native attempt only +pkg/suggest/ -> offline advisory rewrites with typed caveats +pkg/diffplan/ -> declarative front door as a library: desired schema -> routed plan +pkg/migrate/ -> imperative front door as a library + desired-state execution loop +pkg/executor/ -> native backend: bounded optimistic attempt, concurrent index build + and recovery, safer-sequence runner, greenfield create, accepted-blocking +pkg/progress/ -> pollable progress snapshots (the executors' observation seam) pkg/dbconn/ -> bounded database connections -pkg/preflight/ -> migration preflight checks -pkg/verdict/ -> typed outcomes +pkg/preflight/ -> schema-change preflight checks +pkg/verdict/ -> typed outcomes, refusal classes, exit codes +pkg/capabilities/ -> embedded, validated support matrix -Planned: -pkg/migration/ -> orchestrator + runner + cutover +Contracts and types exist; implementation lands with the copy-and-swap phases: +pkg/schemachange/ -> orchestrator + runner + cutover pkg/decode/ -> logical-decoding client pkg/copier/ -> PK-range chunker, dynamic sizing, parallel chunked copy pkg/applier/ -> captured-change apply pkg/checksum/ -> chunked verification and cutover gate +pkg/checkpoint/ -> durable resume state + +Planned: pkg/throttler/ -> chunk-time / slot-lag throttle (replica lag deferred, D12) Executor -> Plan/Execute/Status/Abort backend interface ``` @@ -801,8 +812,9 @@ introspection and an ordered declarative diff complete the plan. ## Next step -Phases 1 and 2.1–2.4, including the CLI front ends, classifier, router, and declarative diff, are -implemented. Phase 3 native execution is in progress: the `CREATE INDEX CONCURRENTLY` execution +Phases 1 and 2.1–2.5, including the CLI front ends, classifier, router, declarative diff, +versioned plan report, offline lint, and suggest, are implemented. Phase 3 native execution is +implemented: the `CREATE INDEX CONCURRENTLY` execution path exists in `pkg/executor` — session-scoped, outside any transaction, under the CONCURRENTLY wait policy (no per-lock timeout, one overall deadline), with invalid-index detection that fails closed into a typed, state-specific outcome, and a separate recovery diff --git a/docs/optimistic-attempt.md b/docs/optimistic-attempt.md index 7c08ac8..2af81b9 100644 --- a/docs/optimistic-attempt.md +++ b/docs/optimistic-attempt.md @@ -41,14 +41,18 @@ pg-sprite executes a PostgreSQL schema change through an escalation ladder: any size check — what executes is a planner-authored sequence or an already-online form, never a blind statement. 2. **Bounded attempt:** every other executing shape — *including those the classifier - labels `metadata-only`* — runs the submitted form once, blind, under two tight + labels `metadata-only`* — runs the submitted form blind, under two tight budgets — `lock_timeout` and `statement_timeout` — set with `SET LOCAL` inside the attempt's transaction. If the change was really instant, it succeeds in - milliseconds. If it turns out to do real work (a table rewrite), the server cancels - it, PostgreSQL's transactional DDL rolls it back cleanly, and a typed budget error - surfaces. Nothing executed, no debris. The classification is a prediction, not an - assertion PostgreSQL honours — which is why even a `metadata-only` verdict earns a - budget, not an exemption. + milliseconds. If the lock never arrives, the attempt is retried a bounded number of + times with exponential backoff, each in a fresh transaction (the default policy is + three attempts; `--lock-attempts 1` disables the retry). If it turns out to do real + work (a table rewrite), the server cancels it, PostgreSQL's transactional DDL rolls + it back cleanly, and a typed budget error surfaces at once — a statement-budget + overrun is never retried, because repeating work that exceeded its execution + budget is not a lock-acquisition strategy. Nothing committed, no debris. The + classification is a prediction, not an assertion PostgreSQL honours — which is why + even a `metadata-only` verdict earns a budget, not an exemption. 3. **The table-size guard** protects rung 2 only: above `Options.MaxTableSizeBytes`, the bounded attempt is refused *before any DDL runs* with a typed `*preflight.SizeError`, because on a big table even a losing gamble costs a full @@ -201,9 +205,11 @@ What happens to one statement, in order: refusal, whose committed prefix stays committed ([execution-model.md](execution-model.md)). - **Lane B → bounded attempt.** `SET LOCAL lock_timeout` + `statement_timeout`, - then run the submitted form once ([the budget mechanics](#the-budget-mechanics)). + then run the submitted form, retrying only a lock-timeout overrun within the bounded + retry policy ([the budget mechanics](#the-budget-mechanics)). Three endings: it really was catalog-only and commits in milliseconds (Exit 2); - the lock never arrived on a contended table (Exit 3a, nothing executed); or the + the lock never arrived on a contended table across every attempt (Exit 3a, nothing + executed); or the statement did real work — a rewrite — and the server cancelled it, transactional DDL rolling it back cleanly (Exit 3b). 7. **Every ending is typed.** Success verdicts, SQLSTATE-mapped budget refusals, @@ -335,7 +341,12 @@ weaken them: - **`lock_timeout`** — how long we will wait to *acquire* the `ACCESS EXCLUSIVE` lock. Overrun ⇒ SQLSTATE `55P03`, typed as "the table is too contended for a blind - attempt right now". Nothing was executed. + attempt right now". Nothing was executed. This is the one failure the attempt + retries: `executor.RetryPolicy` bounds the attempts (`DefaultRetryPolicy` is three, + with exponential backoff from 100 ms capped at 1 s; the CLI flags are + `--lock-attempts`, `--lock-backoff`, `--lock-backoff-max`), each attempt is a new + transaction, and the `*BudgetError` returned when every attempt fails carries the + attempt count. - **`statement_timeout`** — how long the DDL may *run while holding* the lock. Overrun ⇒ SQLSTATE `57014`, typed as "the change is doing real work — a rewrite — not an in-place catalog change". Rolled back cleanly. diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 8247076..411c829 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -120,9 +120,10 @@ implementation time): - **So is the set of tables.** A live table with no desired file is invisible to a per-table diff, and under a declarative model its only convergence is `DROP TABLE` — which pg-sprite refuses at both front doors and never executes. There is no `verdict.Verdict` to map: pg-sprite - never saw the table. The adapter enumerates the namespace's live tables itself (the catalog - query and its exclusions — partitions and extension-owned tables — are - under [Deliberately operator-owned](capabilities.md#deliberately-operator-owned)) and + never saw the table. The adapter enumerates the namespace's live tables itself — + `schemadiff.ListManagedTables` is the exported catalog query `pull` uses, and its + exclusions (partitions and extension-owned tables) are explained under + [Deliberately operator-owned](capabilities.md#deliberately-operator-owned) — and *synthesizes* an `engine.TableChange` per undeclared table: `ExecutionMode = ExecutionModeBlocked`, `IsUnsafe` with a data-loss `UnsafeReason`, and a `ModeReason` that names the table and the two remedies (restore or write its file; drop it through a reviewed diff --git a/docs/testing.md b/docs/testing.md index 8922082..e001c0c 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -134,8 +134,12 @@ results with commit SHA + PG version. A fast wrong answer is a failure. Separate from Go package tests, CI runs the **built `pg-sprite` binary** against a real database with checked-in example inputs as the acceptance corpus — exit codes, output, and resulting database state asserted. -This obligation is not yet wired into CI: CI builds the binary but does not -run this acceptance path. *Binds:* Phase 2 (first executing command). *Source:* pgroll `make +CI's `demo` job does this: `make demo-check` runs the demo tour +([demo/tour.sh](../demo/tour.sh)) in check mode against the compose +database, asserting on `--json` fields and exit codes only, never on prose. +The operator-run [corpus replay](../replay/README.md) (`make replay`) drives +the same binary through a real project's schema-change history and is not +a CI gate. *Binds:* Phase 2 (first executing command). *Source:* pgroll `make examples` CI job; pg_repack driving its CLI through `pg_regress`. ### TM-9 — The operation must outlive the observer @@ -167,6 +171,9 @@ capability no peer suite has. | `make test-supported-postgres` | Full suite against every supported major, 14 → 18 — the local mirror of the CI matrix. | | `make db-up` / `make test-db` / `make db-down` | Long-lived compose database on localhost; the suite connects to it via `PG_DSN` instead of starting per-test containers. Fastest loop for repeated integration runs, and the path CI's version matrix uses — per-test containers oversubscribe a small CI runner and get killed mid-test. | | `make test-aws-boundary` | AWS-boundary tests against Ministack's RDS/Aurora control plane. Needs Docker only; see the tier table below. | +| `make demo-check` | The demo tour in check mode against the built `bin/pg-sprite` and the compose database — CI's artifact smoke test (TM-8). Needs Docker and `jq`. | +| `make check-capabilities` | Regenerates the generated regions of [capabilities.md](capabilities.md) from `pkg/capabilities/capabilities.yaml` and fails if the committed page differs. CI runs it; no Docker. | +| `make replay` / `make replay-refresh` / `make replay-down` | Corpus replay ([replay/README.md](../replay/README.md)): drive the built binary through a pinned real-project schema-change history, asserting each statement's typed outcome. Operator-run, not a CI gate. | The harness is [internal/testutil](../internal/testutil/postgres.go): `StartPostgres` returns a connection URL (container, or `PG_DSN` when set) @@ -333,7 +340,10 @@ DSN resolution) lands — the day a failure means something an author can fix. It is also intentionally **not** part of the pre-push hook, which stays unit-only so pushes remain fast. -## Current coverage (Phases 1 and 2.1–2.4) +## Current coverage + +The table names the entry-point test file for each area; sibling `*_test.go` files in the +same package cover the rest of it. | Area | Tests | | --- | --- | @@ -355,6 +365,17 @@ stays unit-only so pushes remain fast. | CLI `diff`, `fmt`, and classified `migrate --dry-run`, including applying text output and re-diffing to empty (`TestDiffTextPlanIsExecutableSQL`) | [diff integration](../internal/cli/diff_integration_test.go), [fmt](../internal/cli/diff_test.go), [dry-run integration](../internal/cli/dryrun_integration_test.go) | | Library front door (`diffplan.Plan`): ordered routed plan, missing-table, no-op, copy-and-swap refusal, never-writes, deterministic fingerprint | [diffplan unit](../pkg/diffplan/diffplan_test.go), [diffplan integration](../pkg/diffplan/diffplan_integration_test.go) | | Bounded optimistic native attempt and table preflight | [pkg/executor](../pkg/executor/optimistic_integration_test.go), [pkg/preflight](../pkg/preflight/preflight_integration_test.go) | +| Safer-sequence runner: autocommit steps, committed-prefix semantics on a mid-sequence failure | [pkg/executor](../pkg/executor/sequence_integration_test.go) | +| Concurrent index build: caller-owned session, invalid-index detection into a typed state, and the proven recovery (`RebuildAbandonedIndex`, `DropAbandonedIndex`) | [pkg/executor](../pkg/executor/recover_integration_test.go) | +| Greenfield `CREATE TABLE` path: shape refusals, claimed-name collisions, post-commit name verification | [pkg/executor](../pkg/executor/create_integration_test.go) | +| Accepted-blocking execution primitive (`ExecuteAcceptedBlocking`) and its eligibility rule | [pkg/executor](../pkg/executor/accepted_blocking_integration_test.go), [pkg/verdict](../pkg/verdict/accepted_blocking_test.go) | +| Imperative front door (`migrate.Run`): gate, resolve, route, execute, verdict; `--force` acknowledgement; refusal reason → class registry | [pkg/migrate](../pkg/migrate/run_integration_test.go), [registry](../pkg/migrate/refusal_registry_test.go) | +| Desired-state execution loop (`migrate.RunDesired`): whole-plan admission, destructive guard, fingerprint pin, per-statement verdicts | [pkg/migrate](../pkg/migrate/desired_integration_test.go) | +| Offline `suggest`: safer-form mapping, typed caveats, and residue checks against a live server | [pkg/suggest](../pkg/suggest/suggest_test.go), [residue](../pkg/suggest/residue_integration_test.go) | +| `pull`: per-table export, refuse-don't-guess shapes, create-only file writes, and the managed-table catalog query | [CLI pull](../internal/cli/pull_integration_test.go), [pkg/schemadiff](../pkg/schemadiff/managed_tables_integration_test.go) | +| Capability matrix: YAML validity, tier rules, rendered page agreement, `capabilities --json` contract | [pkg/capabilities](../pkg/capabilities/capabilities_test.go), [CLI](../internal/cli/capabilities_test.go) | +| Progress snapshots, the observer seam, and `CancelBuild` | [pkg/progress](../pkg/progress/progress_test.go) | +| Supabase compatibility: pooler connections through `pkg/dbconn`, and the DDL, RLS, realtime, and failure matrix against pinned Supabase services | [pkg/dbconn](../pkg/dbconn/supabase_integration_test.go), [integration/supabase](../integration/supabase/README.md) | ## Landed and deferred test obligations @@ -366,8 +387,8 @@ points. | Status | Test obligations (summary) | | --- | --- | -| Done — Phases 2.1–2.4 | Parse-based operation descriptors and classification, refusal contracts, declarative desired-state → ordered `ALTER` derivation, routing, and convergence testing against real PostgreSQL. | -| Remaining — Phase 3 native executor | Each native idiom (`CONCURRENTLY`, `NOT VALID` + `VALIDATE`, fast default, `USING INDEX`) exercised against all supported majors; bounded lock behavior under contention; invalid-index cleanup. | +| Done — Phases 2.1–2.5 | Parse-based operation descriptors and classification, refusal contracts, declarative desired-state → ordered `ALTER` derivation, routing, the versioned plan report, offline lint and suggest, and convergence testing against real PostgreSQL. | +| Done — Phase 3 native executor | Each native idiom (`CONCURRENTLY`, `NOT VALID` + `VALIDATE`, fast default, `USING INDEX`) exercised against all supported majors; bounded lock behavior under contention with the lock-timeout retry; invalid-index detection and the proven recovery; the greenfield create path; the accepted-blocking primitive. | | Remaining — later copy-and-swap phases | Shadow table, CDC, checksum-gate, cutover, checkpoint/resume, and fault-injection obligations land with their implementations. | Copy-and-swap obligations include checksum-gate and checkpoint/resume diff --git a/docs/vision.md b/docs/vision.md index e562504..8ec872a 100644 --- a/docs/vision.md +++ b/docs/vision.md @@ -70,10 +70,12 @@ SchemaBot first. What that buys today is the refusal discipline: every session r a bounded `lock_timeout`, and a change the engine cannot prove safe ends in seconds with a typed verdict — not a hand-typed `ALTER` in a bare `psql` session holding an `ACCESS EXCLUSIVE` lock on a hot table. Each capability the phased plan lands -(copy-and-swap, the checksum gate, durable crash-resume) reaches the direct user the -moment it ships, because both front doors are held to one design rule: every capability is -reachable from the CLI, and the CLI consumes the same plan, verdict, and lint contracts an -orchestrator would. Meeting users where they are is part of the point: standalone CLI use +(copy-and-swap, the checksum gate, durable crash-resume) reaches the direct user when it +ships, because both front doors are held to one design rule: every capability lands in the +library and becomes reachable from the CLI, and the CLI consumes the same plan, verdict, and +lint contracts an orchestrator would. A capability may ship library-first — the README's +"What pg-sprite does not do yet" names the ones whose CLI verb is still owed — but never +CLI-only. Meeting users where they are is part of the point: standalone CLI use is a supported front door, not a demo mode. ### 3. Developer-friendly, application-invisible From 8bbc411636f8b4cc29f5a4147acb00287e6f131e Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Sat, 12 Sep 2026 15:27:52 +1000 Subject: [PATCH 2/4] docs: finish the CLI-surface sweep and name the passthrough in the README The README's 'does not do yet' list now names the accepted-blocking passthrough as library-only, so the three capability pages move together. architecture.md no longer claims capabilities --json carries a format version: its version is the binary version, as the contract page says. The end-to-end diagrams in both design docs name all eight CLI verbs and the real migrate --alter / diff --desired entry points; the package map's pkg/executor row lists the greenfield create path and the passthrough primitive; and the 'contracts exist' list separates the two packages that hold only a package doc. Amp-Thread-ID: https://ampcode.com/threads/T-01a07fb2-9632-732b-bd7b-2f143a81bc06 --- README.md | 6 ++++++ docs/architecture.md | 10 +++++++--- docs/high-level-design.md | 12 +++++++----- docs/low-level-design.md | 11 +++++++---- 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index e1a9342..b1edbd3 100644 --- a/README.md +++ b/README.md @@ -84,6 +84,12 @@ refusal — never a silently wrong or incomplete result: (`executor.RebuildAbandonedIndex`, or `executor.DropAbandonedIndex` when the caller must not rebuild) is library-only; from the CLI the [runbook](docs/invalid-index-recovery.md) applies. +- **The accepted-blocking passthrough has no CLI flag yet** — running an + otherwise-refused change deliberately under the engine's bounded lock + budget, with the verdict marked `executed-without-online-safety` (exit 3), + is library-only (`executor.ExecuteAcceptedBlocking`); from the CLI these + refusals exit 2. Design: + [docs/lock-budgeted-passthrough.md](docs/lock-budgeted-passthrough.md). - **Non-table objects** — views, standalone sequences, enums, domains, extensions, functions, triggers — are outside the declarative model, which covers one ordinary table plus its indexes per file. diff --git a/docs/architecture.md b/docs/architecture.md index 5aa0df3..7baee4c 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -165,8 +165,12 @@ different levels of commitment: `--dry-run` emit machine-readable verdicts and plans; the plan report is a single versioned contract (`format_version`, `plan.FormatVersion`; additive-only changes within a version, a consumer rejects a version it does not understand), documented in - [plan-report.md](plan-report.md). The suggest report and `capabilities --json` carry - their own version fields on the same rule. + [plan-report.md](plan-report.md). The suggest report carries its own `format_version` + on the same rule ([suggest-report.md](suggest-report.md)). `capabilities --json` is + deliberately not versioned separately: its `version` is the binary version string, + because the matrix is committed with and tested against one binary, so consumers pin + the binary rather than a matrix version and ignore fields they do not recognize + ([capabilities-contract.md](capabilities-contract.md#versioning)). - **The Go packages** — everything under `pkg/` is importable, and the front-end seams (`pkg/statement`, `pkg/schemadiff`, `pkg/planner`, `pkg/router`, `pkg/verdict`) are each designed as a standalone entry point; `internal/` is unimportable by @@ -194,7 +198,7 @@ different levels of commitment: | `pkg/diffplan` | The declarative front door as a library: desired schema in, routed `plan.Report` out — the CLI `diff` and embedding orchestrators share this one pipeline | exists | | `pkg/migrate` | The imperative front door as a library: one parsed statement in — gate, resolve, classify, route, execute — one `verdict.Verdict` out; the CLI `migrate` and embedding orchestrators share this one pipeline. Also the desired-state execution loop: `RunDesired` derives the convergence plan (`diffplan.Plan`), admits it as a whole (existence, destructive guard, dispositions, optional fingerprint pin), and runs each planned statement back through `Run` — per-statement verdicts, committed-prefix semantics | exists | | `pkg/router` | Route classified statements to native / copy-and-swap / refuse dispositions; copy-and-swap reports unavailable until that backend lands | exists (Phase 2.4) | -| `pkg/executor` | Bounded optimistic native attempt, the concurrent index build, and the autocommit safer-sequence runner, with stable outcome codes; the full `Executor` contract (`Plan`/`Execute`/`Status`/`Abort`) arrives with the copy-and-swap backend | native execution exists | +| `pkg/executor` | Native backend with stable outcome codes: the bounded optimistic attempt, the concurrent index build and its invalid-index recovery, the autocommit safer-sequence runner, the greenfield `CREATE TABLE` path, and the accepted-blocking passthrough primitive; the full `Executor` contract (`Plan`/`Execute`/`Status`/`Abort`) arrives with the copy-and-swap backend | native execution exists | | `pkg/progress` | Strategy-wide, pollable progress snapshots: native phase/elapsed time, sequence position, retry attempt, and server-reported concurrent-index work; optional copy counters are reserved for copy-and-swap | native progress exists | | `pkg/copier` | PK-range chunker over one integer-family primary key with dynamic time-based sizing (produces `Chunk` and `Watermark`; composite keys refused in v1), and the parallel chunked copy into the shadow table (never overwrites) — there is no separate chunker package | contracts exist; copy loop Phase 4 | | `pkg/checksum` | The mandatory correctness gate; continuous checker; repair primitive | Phase 5 | diff --git a/docs/high-level-design.md b/docs/high-level-design.md index 2b88874..658f5ef 100644 --- a/docs/high-level-design.md +++ b/docs/high-level-design.md @@ -127,12 +127,14 @@ is the right design on PostgreSQL specifically — is ## Architecture at a glance ```diagram - user: --alter "..." OR --desired schema.sql + user: migrate --alter "ALTER TABLE …" OR diff --desired schema.sql │ - ╭──────────▼──────────-╮ - │ CLI: migrate · diff ·│ - │ fmt · lint · status │ - ╰──────────┬──────────-╯ + ╭──────────▼───────────────╮ + │ CLI: migrate · pull · │ + │ diff · fmt · lint · │ + │ suggest · capabilities · │ + │ status │ + ╰──────────┬───────────────╯ ▼ ╭───────────────╮ shared front-end: │ PLANNER │ parse · introspect · diff --git a/docs/low-level-design.md b/docs/low-level-design.md index 57df542..2caf3bc 100644 --- a/docs/low-level-design.md +++ b/docs/low-level-design.md @@ -76,10 +76,11 @@ seam inside the copy-and-swap executor is the same idea applied one level down. ### Proposed architecture (end-to-end) ``` - user: --alter "..." OR --desired schema.sql + user: migrate --alter "ALTER TABLE …" OR diff --desired schema.sql │ ╭────────────────▼─────────────────────────────────────────────────────╮ - │ CLI (Kong) migrate · diff · fmt · lint · status │ + │ CLI (Kong) migrate · pull · diff · fmt · lint · suggest · │ + │ capabilities · status │ ╰────────────────┬─────────────────────────────────────────────────────╯ │ ┌─────────────────────▼──────────────────── PLANNER / front-end (shared) ────┐ @@ -641,13 +642,15 @@ pkg/verdict/ -> typed outcomes, refusal classes, exit codes pkg/capabilities/ -> embedded, validated support matrix Contracts and types exist; implementation lands with the copy-and-swap phases: -pkg/schemachange/ -> orchestrator + runner + cutover pkg/decode/ -> logical-decoding client pkg/copier/ -> PK-range chunker, dynamic sizing, parallel chunked copy -pkg/applier/ -> captured-change apply pkg/checksum/ -> chunked verification and cutover gate pkg/checkpoint/ -> durable resume state +Package doc only (the invariants it will enforce are named; no types yet): +pkg/schemachange/ -> orchestrator + runner + cutover +pkg/applier/ -> captured-change apply + Planned: pkg/throttler/ -> chunk-time / slot-lag throttle (replica lag deferred, D12) Executor -> Plan/Execute/Status/Abort backend interface From 7683d0b76ba7c34617aa04236527dccba5500046 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Sun, 13 Sep 2026 18:10:38 +1000 Subject: [PATCH 3/4] docs: state the retry in the exit inventory and pin the refusal-reason table both ways The Exit inventory's lock-budget row still described the bounded attempt as a single try, and the orchestrator table beside it told an operator to retry without saying the engine had already exhausted its own retries; both rows now carry the retry the prose above them describes. The test that pins the refusal-reason table checked only that every reason has a row, so a row for a token that is not a reason went unnoticed. It now compares the table's row set with verdict.Reasons() in both directions, the way the exit-code ladder test does. Amp-Thread-ID: https://ampcode.com/threads/T-01a07fb2-9632-732b-bd7b-2f143a81bc06 --- docs/optimistic-attempt.md | 4 ++-- pkg/verdict/docs_test.go | 48 ++++++++++++++++++++++++++++++++++---- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/docs/optimistic-attempt.md b/docs/optimistic-attempt.md index 5e146d6..d923091 100644 --- a/docs/optimistic-attempt.md +++ b/docs/optimistic-attempt.md @@ -236,7 +236,7 @@ failed sequence. |---|---|---|---| | 1 | Online idiom / substituted sequence completed | Success verdict | Yes — online by proof | | 2 | Bounded attempt completed within budget | Success verdict | Yes — it was catalog-only | -| 3a | Lock not granted within `lock_timeout` | `reason: not-native-safe-budget-exceeded`, `cause: lock-budget` (SQLSTATE `55P03`; executor code `budget-lock-exceeded`) | No — nothing executed | +| 3a | Lock not granted within `lock_timeout` on any attempt | `reason: not-native-safe-budget-exceeded`, `cause: lock-budget` (SQLSTATE `55P03`; executor code `budget-lock-exceeded`) | No — nothing executed | | 3b | Statement ran past `statement_timeout` | `reason: not-native-safe-budget-exceeded`, `cause: statement-budget` (SQLSTATE `57014`; executor code `budget-statement-exceeded`) | No — rolled back cleanly | | 4 | Table exceeds the size limit | `reason: not-native-safe-table-too-large` (a typed `*preflight.SizeError` underneath) | No — refused before any DDL, no lock taken | | 5 | Shape routes to an unimplemented strategy, or is refused by a plan/partition/tier gate | Typed plan refusal | No | @@ -255,7 +255,7 @@ Per refusal, the operational move an orchestrator makes | Refusal | What an orchestrator does | |---|---| -| `not-native-safe-budget-exceeded`, `cause: lock-budget` | Transient — retry off-peak, same plan | +| `not-native-safe-budget-exceeded`, `cause: lock-budget` | Transient — the engine's own bounded retries are already exhausted; retry off-peak, same plan | | `not-native-safe-budget-exceeded`, `cause: statement-budget` | Terminal today; the future copy-and-swap on-ramp | | `not-native-safe-table-too-large` | Policy — an operator raises the threshold deliberately | | `insufficient-privileges` | Operator action — `detail` names the exact `GRANT` | diff --git a/pkg/verdict/docs_test.go b/pkg/verdict/docs_test.go index 7f72967..6a74ca0 100644 --- a/pkg/verdict/docs_test.go +++ b/pkg/verdict/docs_test.go @@ -137,16 +137,54 @@ func TestExitCodeLadderDocsListEveryCode(t *testing.T) { } } -// Every refusal reason automation can meet must be documented: a Reason -// constant added without a row in the doc's refusal-reason table fails here. +// refusalReasonsHeader is the header row of the refusal-reason table; only +// rows under this header count as the closed set, so a reason token quoted +// in another table on the page is neither credited nor rejected here. +const refusalReasonsHeader = "| Reason | Meaning |" + +// documentedRefusalReasons returns the backticked first-cell token of every +// row under the refusal-reason header. A page with no such table yields an +// empty set, which the equality check then reports as missing every reason. +func documentedRefusalReasons(t *testing.T, doc string) map[Reason]struct{} { + t.Helper() + reasons := map[Reason]struct{}{} + lines := strings.Split(doc, "\n") + for i, line := range lines { + if !strings.HasPrefix(line, refusalReasonsHeader) { + continue + } + // Skip the header and the separator row, then read rows until the + // table ends. + for _, row := range lines[i+2:] { + if !strings.HasPrefix(row, "|") { + break + } + cells := strings.SplitN(row, "|", 3) + require.Lenf(t, cells, 3, "refusal-reason row has a first cell: %q", row) + token, ok := strings.CutPrefix(strings.TrimSpace(cells[1]), "`") + require.Truef(t, ok, "refusal-reason row's first cell is a backticked token: %q", row) + token, ok = strings.CutSuffix(token, "`") + require.Truef(t, ok, "refusal-reason row's first cell is a backticked token: %q", row) + reasons[Reason(token)] = struct{}{} + } + } + return reasons +} + +// The refusal-reason table must list exactly the reasons automation can +// meet: a Reason constant added without a row saying what it means fails +// here, and so does a row for a token that is not a refusal reason — an +// executor failure code, say, which a reader would otherwise take as a +// nothing-ran refusal when the DDL has in fact committed. func TestDocListsEveryRefusalReason(t *testing.T) { raw, err := os.ReadFile(cliOutputExamplesDoc) require.NoError(t, err) - doc := string(raw) + want := map[Reason]struct{}{} for _, r := range Reasons() { - assert.Contains(t, doc, fmt.Sprintf("| `%s` |", string(r)), - "docs/cli-output-examples.md is missing a refusal-reason row for %q", r) + want[r] = struct{}{} } + assert.Equal(t, want, documentedRefusalReasons(t, string(raw)), + "docs/cli-output-examples.md must state the refusal reasons as a table under %q with one row per reason", refusalReasonsHeader) } // Every refusal reason must be classified: a Reason constant added without a From e1ffc1e4278e62d4e9fddaa30c94684d3eea283e Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Mon, 14 Sep 2026 08:25:27 +1000 Subject: [PATCH 4/4] test: pin every closed docs vocabulary in both directions The refusal-class map and the class and owner tables were checked only for missing rows, so a row for a value the contract does not carry would have gone unnoticed. Each is now compared with its constant set as a whole, sharing one table reader so the checks in this file take the same shape. docs/progress-report.md states a current format_version that no test kept honest; it now has the same pin as the plan and suggest reports. Amp-Thread-ID: https://ampcode.com/threads/T-01a07fb2-9632-732b-bd7b-2f143a81bc06 --- pkg/progress/docs_test.go | 25 ++++++++ pkg/verdict/docs_test.go | 122 ++++++++++++++++++++++++-------------- 2 files changed, 104 insertions(+), 43 deletions(-) create mode 100644 pkg/progress/docs_test.go diff --git a/pkg/progress/docs_test.go b/pkg/progress/docs_test.go new file mode 100644 index 0000000..1abdd7f --- /dev/null +++ b/pkg/progress/docs_test.go @@ -0,0 +1,25 @@ +package progress_test + +import ( + "fmt" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/progress" +) + +// progressReportDoc is the human-facing contract page this test keeps +// honest. +const progressReportDoc = "../../docs/progress-report.md" + +// The doc's stated current version is the constant, not prose that can +// drift: a FormatVersion bump without the matching doc sentence fails here. +func TestDocStatesCurrentFormatVersion(t *testing.T) { + raw, err := os.ReadFile(progressReportDoc) + require.NoError(t, err) + assert.Contains(t, string(raw), fmt.Sprintf("The current version is **%d**", progress.FormatVersion), + "docs/progress-report.md's stated version drifted from progress.FormatVersion") +} diff --git a/pkg/verdict/docs_test.go b/pkg/verdict/docs_test.go index 6a74ca0..444e120 100644 --- a/pkg/verdict/docs_test.go +++ b/pkg/verdict/docs_test.go @@ -1,7 +1,6 @@ package verdict import ( - "fmt" "go/ast" "go/parser" "go/token" @@ -137,20 +136,29 @@ func TestExitCodeLadderDocsListEveryCode(t *testing.T) { } } -// refusalReasonsHeader is the header row of the refusal-reason table; only -// rows under this header count as the closed set, so a reason token quoted -// in another table on the page is neither credited nor rejected here. -const refusalReasonsHeader = "| Reason | Meaning |" +// Headers of the vocabulary tables the docs state as closed sets; only rows +// under a header count, so a token quoted in another table on the page is +// neither credited nor rejected by the set checks below. +const ( + refusalReasonsHeader = "| Reason | Meaning |" + refusalSitesHeader = "| Existing `reason` | Refusal site or shape |" + refusalClassesHeader = "| Class | Meaning |" + refusalOwnersHeader = "| Owner | Work it names |" +) + +// causeTableHeading is the prefix of a heading that classifies one reason by +// its own cause-keyed table rather than by a row in the site-keyed table. +const causeTableHeading = "### `" -// documentedRefusalReasons returns the backticked first-cell token of every -// row under the refusal-reason header. A page with no such table yields an -// empty set, which the equality check then reports as missing every reason. -func documentedRefusalReasons(t *testing.T, doc string) map[Reason]struct{} { +// documentedTokens returns the backticked first-cell token of every row +// under every occurrence of the header. A page with no such table yields an +// empty set, which an equality check then reports as missing every value. +func documentedTokens(t *testing.T, doc, header string) map[string]struct{} { t.Helper() - reasons := map[Reason]struct{}{} + tokens := map[string]struct{}{} lines := strings.Split(doc, "\n") for i, line := range lines { - if !strings.HasPrefix(line, refusalReasonsHeader) { + if !strings.HasPrefix(line, header) { continue } // Skip the header and the separator row, then read rows until the @@ -160,17 +168,52 @@ func documentedRefusalReasons(t *testing.T, doc string) map[Reason]struct{} { break } cells := strings.SplitN(row, "|", 3) - require.Lenf(t, cells, 3, "refusal-reason row has a first cell: %q", row) - token, ok := strings.CutPrefix(strings.TrimSpace(cells[1]), "`") - require.Truef(t, ok, "refusal-reason row's first cell is a backticked token: %q", row) - token, ok = strings.CutSuffix(token, "`") - require.Truef(t, ok, "refusal-reason row's first cell is a backticked token: %q", row) - reasons[Reason(token)] = struct{}{} + require.Lenf(t, cells, 3, "row under %q has a first cell: %q", header, row) + tokens[backtickedToken(t, strings.TrimSpace(cells[1]))] = struct{}{} } } + return tokens +} + +// backtickedToken strips the code fence from a `token` cell, rejecting a +// cell that is prose or a bare word so a mistyped row fails loudly rather +// than counting as an unknown value. +func backtickedToken(t *testing.T, cell string) string { + t.Helper() + token, ok := strings.CutPrefix(cell, "`") + require.Truef(t, ok, "cell is a backticked token: %q", cell) + token, ok = strings.CutSuffix(token, "`") + require.Truef(t, ok, "cell is a backticked token: %q", cell) + return token +} + +// causeTableReasons returns the reason each cause-keyed table classifies: the +// backticked token that opens its heading. +func causeTableReasons(t *testing.T, doc string) map[string]struct{} { + t.Helper() + reasons := map[string]struct{}{} + for line := range strings.SplitSeq(doc, "\n") { + rest, ok := strings.CutPrefix(line, causeTableHeading) + if !ok { + continue + } + token, _, ok := strings.Cut(rest, "`") + require.Truef(t, ok, "cause-table heading names a backticked reason: %q", line) + reasons[token] = struct{}{} + } return reasons } +// tokenSet is the closed set a docs table must match, as the strings the +// page prints. +func tokenSet[T ~string](values []T) map[string]struct{} { + set := map[string]struct{}{} + for _, v := range values { + set[string(v)] = struct{}{} + } + return set +} + // The refusal-reason table must list exactly the reasons automation can // meet: a Reason constant added without a row saying what it means fails // here, and so does a row for a token that is not a refusal reason — an @@ -179,44 +222,37 @@ func documentedRefusalReasons(t *testing.T, doc string) map[Reason]struct{} { func TestDocListsEveryRefusalReason(t *testing.T) { raw, err := os.ReadFile(cliOutputExamplesDoc) require.NoError(t, err) - want := map[Reason]struct{}{} - for _, r := range Reasons() { - want[r] = struct{}{} - } - assert.Equal(t, want, documentedRefusalReasons(t, string(raw)), + assert.Equal(t, tokenSet(Reasons()), documentedTokens(t, string(raw), refusalReasonsHeader), "docs/cli-output-examples.md must state the refusal reasons as a table under %q with one row per reason", refusalReasonsHeader) } -// Every refusal reason must be classified: a Reason constant added without a -// place in the refusal-class map fails here, so a new reason cannot land -// without a routing decision. A reason is classified either by a row in the -// site-keyed table or by its own cause-keyed table, whose heading names it. +// The refusal-class map must classify exactly the refusal reasons: a Reason +// constant added without a routing decision fails here, and so does a row +// or cause table for a token that is not a reason. A reason is classified +// either by rows in the site-keyed table or by its own cause-keyed table, +// whose heading names it; a reason may appear in both. func TestRefusalClassesDocListsEveryRefusalReason(t *testing.T) { raw, err := os.ReadFile(refusalClassesDoc) require.NoError(t, err) doc := string(raw) - for _, r := range Reasons() { - row := fmt.Sprintf("| `%s` |", string(r)) - causeTable := fmt.Sprintf("### `%s`, keyed on", string(r)) - assert.True(t, strings.Contains(doc, row) || strings.Contains(doc, causeTable), - "docs/refusal-classes.md has neither a class row nor a cause table for refusal reason %q", r) + classified := documentedTokens(t, doc, refusalSitesHeader) + for r := range causeTableReasons(t, doc) { + classified[r] = struct{}{} } + assert.Equal(t, tokenSet(Reasons()), classified, + "docs/refusal-classes.md must classify each refusal reason by a row under %q or a heading opening with %q, and nothing else", refusalSitesHeader, causeTableHeading) } -// Every class and owner the contract closes over must be documented: a -// Class or Owner constant added without a row in the refusal-classes -// vocabulary tables fails here, so a new value cannot land without a stated -// meaning and consumer action. +// The class and owner vocabulary tables must list exactly the values the +// contract closes over: a Class or Owner constant added without a stated +// meaning and consumer action fails here, and so does a row for a value the +// contract does not carry. func TestRefusalClassesDocListsEveryClassAndOwner(t *testing.T) { raw, err := os.ReadFile(refusalClassesDoc) require.NoError(t, err) doc := string(raw) - for _, c := range Classes() { - assert.Contains(t, doc, fmt.Sprintf("| `%s` |", string(c)), - "docs/refusal-classes.md is missing a vocabulary row for class %q", c) - } - for _, o := range Owners() { - assert.Contains(t, doc, fmt.Sprintf("| `%s` |", string(o)), - "docs/refusal-classes.md is missing a vocabulary row for owner %q", o) - } + assert.Equal(t, tokenSet(Classes()), documentedTokens(t, doc, refusalClassesHeader), + "docs/refusal-classes.md must state the classes as a table under %q with one row per class", refusalClassesHeader) + assert.Equal(t, tokenSet(Owners()), documentedTokens(t, doc, refusalOwnersHeader), + "docs/refusal-classes.md must state the owners as a table under %q with one row per owner", refusalOwnersHeader) }