Land reviewed stack on main: CI foundations, Phase 1 front door, Phase 2.1-2.4 (#4-#7) - #16
Merged
Merged
Conversation
Compose-based local database mirrors the CI matrix (PG_DSN path in the harness); docs-only PRs skip the heavy jobs behind a fail-open filter with a single ci-ok required status; pre-commit/pre-push hooks give fast local feedback and skip cleanly on Markdown-only changes. A TLS-only server harness proves the verify-full CACertPath path live (trusted CA encrypts, plaintext and untrusted CA fail closed) — the one peer-CI pattern (pg-delta) applicable to Phase 0. docs/testing.md records Phase 0 coverage, Phases 1-7 test obligations including the topology patterns surveyed from peer-tool CIs, and the vanilla PostgreSQL vs real-Aurora validation boundary. The lint config adopts SchemaBot's proven linter set plus gofmt/goimports formatters, with a staged-files lint-fix pre-commit flow ported from the same repo.
Compose-based local database mirrors the CI matrix (PG_DSN path in the harness); docs-only PRs skip the heavy jobs behind a fail-open filter with a single ci-ok required status; pre-commit/pre-push hooks give fast local feedback and skip cleanly on Markdown-only changes. A TLS-only server harness proves the verify-full CACertPath path live (trusted CA encrypts, plaintext and untrusted CA fail closed) — the one peer-CI pattern (pg-delta) applicable to Phase 0. docs/testing.md records Phase 0 coverage, Phases 1-7 test obligations including the topology patterns surveyed from peer-tool CIs, and the vanilla PostgreSQL vs real-Aurora validation boundary. The lint config adopts SchemaBot's proven linter set plus gofmt/goimports formatters, with a staged-files lint-fix pre-commit flow ported from the same repo. AGENTS.md stays canonical for agent guidance: CLAUDE.md, GEMINI.md, .cursorrules, .goosehints, and copilot-instructions.md are symlinks to it, and review-agent checks live in .agents/checks/review.md.
Make the testing bar explicit and binding: same-PR tests, regression-first bug fixes, real-DB core validation, full-matrix CI as a merge gate, and a coverage ratchet (numeric gate wired when Phase 1 lands core code). Add eight phase-tagged test-methodology invariants (TM-1..TM-8) mined from the pgroll / pg_repack / pg-delta suites: lifecycle fixtures, two-oracle SQL tests, real fault injection asserting durable state, an adversarial schema corpus, the convergence oracle for declarative diff, mutation-direction coverage, correctness-asserting benchmarks, and a compiled-binary e2e CI path — plus seeded generative DDL as the beyond-peers goal.
…S.md Doc comments on exported symbols, no init(), no package-level mutable state, and no ctx-in-struct move to .golangci.yml (revive, gochecknoinits, gochecknoglobals, containedctx) so prose stays short. New maxims (panic policy, SQLSTATE error matching, goroutine lifetimes, injected clock, comment hygiene, OSS hygiene) ported from schemabot's AGENTS.md and Go community style canon, mirrored into the review checks.
…ute-and-introspect) The DDL-understanding trade-off is decided: classification parses via wasilibs/go-pgquery (Wasm libpg_query; cgo pg_query_go is the API-compatible escape hatch), shadow-table DDL and checkpoint fingerprints come from execute-and-introspect on an engine-owned scratch database (pg_sprite_scratch pre-provisioned, or CREATEDB to self-provision — verified in preflight), and refusals use both layers. Updates CO-7/ST-2/ST-6, the TCB dependency list, and the low-level design; supersedes the pg_query_go + AST-surgery assumption.
migrate gates statements through go-pgquery (ALTER TABLE only; index maintenance is pointed at CONCURRENTLY), size-guards the table via pg_table_size over the partition tree, attempts the change under SET LOCAL lock/statement budgets, and ends in a structured verdict — executed-natively, or refused with a typed reason and exit code 2. The verdict seam is what Phase 11 maps to ExecutionModeBlocked.
Closes the two behavioral coverage gaps from the Phase 1 coverage review: the status command's live-session output (pid/state/query fields against a held pg-sprite session) and the lock-budget refusal wording, which were previously untested paths.
…flag Establishes the observability posture early: stdout carries only command output, diagnostics go through log/slog to stderr, errors are logged once at the entry point, and credentials are never logged. sloglint/forbidigo enforce the mechanical half; --debug wires the previously unreachable pgx statement tracing in pkg/dbconn plus migrate lifecycle events.
Log text and human-facing prose are no longer a test surface (rule added to AGENTS.md, aligned with SchemaBot's observability guidance). The lock-vs-statement budget distinction was only visible in prose, so the verdict gains a typed machine-readable cause field, and status gains --json so its tests can assert structured session fields.
Admit a declarative desired-state file (one CREATE TABLE plus its CREATE INDEXes) through the real grammar, materialize it in an always-rolled-back transaction-scoped scratch schema on the target, and introspect both live and desired state into one canonical model from server catalogs — semantic truth comes from PostgreSQL, never from AST transformation, per the layered DDL-understanding decision.
Diff live vs desired canonical models into dependency-ordered SQL changes with a destructive marker and typed refusals for unsupported identity/generation changes. `diff` emits an executable text plan or a JSON report; `fmt` canonicalizes offline via the parser/deparser.
The unpinned lint action installed a v1 binary built with an older Go than the module targets, so it could not load the v2 config. Matrix jobs now render as "test (PostgreSQL NN)" in the checks UI.
Checks now render as CI / lint, CI / test (PostgreSQL NN), and CI / all-green instead of the ci / ci-ok stutter. "all-green" is the context to require once branch protection is wired.
Semgrep and zizmor code-scanning gates require third-party actions pinned to a full commit SHA, matching the existing paths-filter pin.
Extend the parse boundary with ParseOps — one typed shape descriptor per operation (default constancy, generated/identity, NOT VALID, USING INDEX, CONCURRENTLY, renames) — plus two syntactic advisory rewriters: Concurrently and AddNotValid. These are the classifier's inputs; no semantics are derived from the AST.
pkg/planner maps each operation to a route with a typed reason, golden-tested against every row of the online-DDL reference. Risky literals get the safer native sequence (CONCURRENTLY, NOT VALID + VALIDATE, USING INDEX attach, the four-step SET NOT NULL pattern). Conservative by construction: unproven defaults are volatile, type changes without live column facts are rewrites, unknown operations are refused.
pkg/router is the policy layer between the classifier and the executors: every classified statement gets a backend (native / copy-and-swap) and a typed disposition; copy-and-swap routes come back unavailable until that executor exists, instead of pretending to run. diff and the new migrate --dry-run share the identical classify-and-route pipeline, with live column types feeding the classifier. Refs PLAT-38439.
The action's default binary is built with an older Go than the module targets and cannot load the v2 config; SHA-pinning also satisfies the semgrep and zizmor unpinned-action checks.
The action's default binary is built with an older Go than the module targets and cannot load the v2 config; SHA-pinning also satisfies the semgrep and zizmor unpinned-action checks.
Addresses the scoped agent review on PR #4: git/PR workflow section, identifier-quoting and DSN-handling rules, schema-change terminology, flake-verification script, and TM-9 (operation must outlive the observer).
…iran01bm/phase-1-front-door * origin/kiran01bm/ci-foundations: docs: port reviewed SchemaBot AGENTS.md conventions ci: pin golangci-lint-action and lint binary version Amp-Thread-ID: https://ampcode.com/threads/T-019fcb83-1db5-74dd-8aa5-b27a21407b7f Co-authored-by: Amp <amp@ampcode.com> # Conflicts: # .agents/checks/review.md # AGENTS.md
Every PR, design, and review is judged as (a) an OSS-first standalone Postgres online-DDL tool and (b) a clean SchemaBot engine integration; trading one off against the other needs an explicit decision.
Addresses the two-lens and adversarial review findings: - ST-7: the executor accepts only a ParseOne-constructed Statement whose target matches the preflight proof, so multi-statement SQL and cross-table smuggling are unrepresentable - size guard measures pg_total_relation_size (heap, indexes, TOAST) — the rewrite it fears rebuilds every index under the same lock - sub-millisecond budgets are rejected (they truncate to 0ms, which disables the PostgreSQL limit) - table-targeted RENAME / SET SCHEMA route through the front door instead of being refused as non-ALTER TABLE - status coalesces pg_stat_activity fields nulled for other roles - already-concurrent index forms no longer get circular safer-idiom advice; verdict reasons are flat kebab-case tokens; byte-size parsing rejects int64 overflow; CI builds with CGO_ENABLED=0
…to kiran01bm/phase-2-1-2-2-diff * origin/kiran01bm/phase-1-front-door: Harden the front door per PR #5 reviews Add the two project lenses to AGENTS.md and review checks docs: port reviewed SchemaBot AGENTS.md conventions ci: pin golangci-lint-action and lint binary version ci: pin golangci-lint-action and lint binary version Amp-Thread-ID: https://ampcode.com/threads/T-019fcb83-1db5-74dd-8aa5-b27a21407b7f Co-authored-by: Amp <amp@ampcode.com> # Conflicts: # SAFETY.md # internal/cli/migrate.go # pkg/statement/statement.go # pkg/statement/statement_test.go
…comments Fixes from the two-lens and adversarial reviews at 14c9dee: - refuse REFERENCES at desired-file admission (typed ErrForeignKey) — the scratch transaction cannot faithfully bind an unqualified FK - refuse serial/sequence-backed default adoption (ErrUnsupportedChange) via pg_depend-backed Column.SequenceDefault — the plan would name a sequence that only existed in the rolled-back scratch transaction - classify every Change with a typed Kind; mark DROP INDEX destructive - annotate lock-hazardous statements in the text plan and point the header at migrate as the executing front door - fmt fails closed on commented input (ErrCommentLoss) instead of silently discarding comments - document column-order-by-name convergence and the two-canon boundary
…to kiran01bm/phase-2-3-2-4-classifier-router * origin/kiran01bm/phase-2-1-2-2-diff: Address PR #6 review: FK refusal, serial adoption, change kinds, fmt comments Harden the front door per PR #5 reviews Add the two project lenses to AGENTS.md and review checks docs: port reviewed SchemaBot AGENTS.md conventions ci: pin golangci-lint-action and lint binary version ci: pin golangci-lint-action and lint binary version # Conflicts: # SAFETY.md # internal/cli/cli.go # internal/cli/diff.go # internal/cli/diff_integration_test.go
Address PR #7 review findings: inline ADD COLUMN constraints and CREATE TABLE ... PARTITION OF are now classified as the work they do, and routing refuses (rewrite-required) any statement whose submitted form blocks when no complete online rewrite was constructed — partial rewrites of multi-operation statements can no longer execute the original SQL. Generated scaffold names fit PostgreSQL's 63-byte identifier limit deterministically.
…outer Phase 2.3-2.4: classifier and router seam
Kiran01bm
marked this pull request as ready for review
August 7, 2026 00:52
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lands the already-reviewed and already-merged stack of PRs #4 → #5 → #6 → #7 onto
main. The stack merged top-down (each PR into its parent branch) after the parent had itself merged, so the work cascaded into stale stack branches and never propagated —maincurrently holds only #3.What
The full content of, in stack order:
No new code:
kiran01bm/phase-2-1-2-2-diffis the branch that received #7's merge and contains every commit of the stack (verified:classifier-routeris an ancestor; the only commits the lower branches carry that this one doesn't are the GitHub merge commits themselves, plus two commits already onmainvia #3). The merge intomainis conflict-free.Why
Every change here was individually reviewed and approved on its own PR; this PR exists solely to move that content to
main. Review it as a formality or spot-check the diff against the individual PRs.Before / after
References
#4, #5, #6, #7