CI matrix, hooks, test-methodology registry, lint enforcement

[Kiran01bm]

Summary

CI and test-methodology foundations. Stacked on kiran01bm/p0-foundations.

What

• GitHub Actions CI: build, golangci-lint, PostgreSQL 14–18 test matrix (testcontainers), docs-only gating, all-green sentinel job.
• Git hooks (make setup): pre-commit and pre-push gates.
• Coverage invariant + TM-* test-methodology registry in docs/testing.md (lifecycle fixtures, two-oracle SQL tests, real fault injection, convergence oracle).
• Mechanical style rules moved into .golangci.yml; judgment maxims kept in AGENTS.md.

Why

The coverage invariant and the version matrix are merge gates for every later phase; they need to exist before the first core package lands.

[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 — scoped to one question: what from block/schemabot's AGENTS.md is worth porting into this repo's AGENTS.md? Reviewed at head d4e45d4.

Verdict: most of it is already here, usually in tighter form — error wrapping with identifiers, no silent branches, no conflated causes, named compound predicates, comment hygiene, testify/t.Context()/named polling deadlines, no timeout-increases-for-flakes, no negative regression tests, no re-exports, the redundant-closer exception, typed state constants, parse-don't-split SQL, "will"-not-"may" logging. The TM registry is stronger than anything schemabot has. The items below are the residue, ranked.

Worth porting

1. A Git/PR workflow section — the biggest gap. This AGENTS.md has zero git conventions, yet the repo is explicitly multi-agent (five harness files symlinked to one AGENTS.md — verified the symlinks, nice setup). None of these are inferable from code, which is exactly this file's stated bar for earning a line:

   • Do not create PRs automatically; create in draft; the author marks ready.
   • Never squash after a human has reviewed (add commits so reviewers see increments); squash freely before.
   • Agent disclosure lines at the bottom of PR bodies.
   • The Copilot-comment protocol (reply describing the fix + commit link, or a reasoned rejection; disclose; resolve only after replying) vs. never touching human review threads without the author's approval. With multiple agents in one repo, this line stops agents speaking for the author.
   • After pushing commits, refresh the PR title/summary — unless a human edited it.
   • The "leaf approach" for upstreaming large branches — directly relevant to the stacked phase plan.

2. Identifier quoting in generated SQL. TM-4's adversarial corpus tests quoted/whitespace identifiers, but there's no authoring rule that generated SQL must quote them — schemabot's equivalent is "always backtick-quote identifiers"; the translation here is pgx.Identifier{...}.Sanitize() / quote_ident(), never raw interpolation. For an engine that builds DDL from user-supplied names this is the most safety-critical single line schemabot has that this file lacks. One line under Conventions.

3. Terminology: pick "schema change" vs "migration" now. Schemabot hard-bans "migration"; this PR's docs use it (TM-1 "executor/migration integration test", TM-3 "no wedged migration record"). Spirit uses pkg/migration, so engine-tier precedent exists for either choice — but if pg-sprite strings ever surface through SchemaBot-facing output, drift will leak. Cheapest moment to decide is this PR; a one-line terminology rule either way.

4. Flake verification, not just flake prohibition. "Never increase timeouts" is here; schemabot pairs it with a convention to prove a flake fix holds (scripts/test-flaky.sh <Test> [iterations] — run the fixed test N times). One line + a trivial script; composes with the coverage invariant.

5. Connection-string manipulation ban. pkg/dbconn centralizes creation, but nothing bans string-manipulating DSNs elsewhere (breaks on passwords containing / etc.). Schemabot's rule translated: parse → modify fields → re-serialize, never string ops.

6. A future TM-9 rather than an AGENTS.md line: seed enough rows that the operation outlives the observer. Schemabot's testing rule (seed large row counts so an in-flight operation demonstrably spans a poll interval) is precisely the vacuous-test race TM-1's lifecycle fixture will hit at Phase 4, when kill/resume must land mid-copy: if the copy can finish before the injected fault, the test passes without testing anything — the same failure mode TM-3 already guards for lock injection. It fits the registry's phase-binding format (Binds: Phase 4).

Deliberately not ported

MySQL-specific items (instant-DDL terminology, connection helpers, Spirit/TiDB-parser integration, backtick style per se), schemabot's storage-schema bootstrap rules, its LogAttrs() helpers (the principle — identifiers in every error — is already here), commit-scope conventions (the github/observability scope split doesn't map yet), and the untrusted-error-rendering rule (that boundary belongs to the caller rendering engine errors into PR surfaces, not to this engine).

Verified solid: the five agent-config symlinks are real symlinks (mode 120000, all pointing at AGENTS.md); the review-checks distillation in .agents/checks/review.md is consistent with AGENTS.md with no contradictions; the TM registry entries all carry sources and phase bindings; the "rules earn a line only when an agent can't infer them from the code" bar is the right instinct and everything above clears it.

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

[aparajon]

🤖 Follow-up review requested by Armand and performed by his agent — a second pass through two lenses: (a) pg-sprite as an OSS-first project aiming to be the preferred Postgres DDL tool, and (b) clean integration into SchemaBot as an engine. Reviewed at head d4e45d4.

Lens 1 — OSS-first

1. Aurora-first framing works against the ambition (the one strategic item). The README headline is "An online schema-change engine for Aurora PostgreSQL (and RDS/community PostgreSQL 14+)" — and AGENTS.md and the new main.go doc comment repeat "for Aurora PostgreSQL." A community-Postgres user evaluating tools reads "Aurora" and bounces; meanwhile the CI story this PR builds is vanilla PostgreSQL 14–18. The positioning can simply invert: "for PostgreSQL 14+ — built for the production realities of managed platforms like Aurora/RDS." Same product, wider funnel — and this PR already touches all three surfaces, so it's the cheapest moment to flip the wording.

2. Three different lint versions. CI pins v2.12.2 (with a comment explaining v2-config compatibility), make lint runs whatever's on PATH, and lint-fix.sh's Docker fallback floats on golangci/golangci-lint:latest. An outside contributor with v1 on PATH gets the config-load error the CI comment predicts; one with a stale Docker image gets different findings than CI. One version variable in the Makefile, consumed by all three, closes it.

3. The GOROOT probing in both hooks is environment-specific and silent. The candidate list is macOS/homebrew/hermit-shaped, and when a match is found the hook silently swaps toolchains. For outside contributors, an explicit error ("go.mod wants 1.26, PATH has 1.24 — see ") beats a probe that works on maintainer laptops and mysteriously no-ops elsewhere.

4. Workflow hygiene nits. No top-level least-privilege permissions: block (only the changes job declares one; the others inherit repo defaults). Third-party actions are SHA-pinned (good); GitHub-owned ones are tag-pinned — a defensible split, just worth making deliberate. OpenSSF Scorecard (a signal serious adopters check) likes both pinned.

5. The # sadscan:disable np.postgres.1 directive in the Makefile is an org-internal scanner artifact in a public repo. Harmless, but cryptic to outsiders — a generic explanatory comment ("suppresses a credential-scanner false positive on test-only defaults") keeps it legible.

Genuinely strong OSS moves worth naming: the vanilla-PG matrix as a merge gate with the Aurora validation boundary honestly documented; the fail-open docs filter (detection failure → run CI, never skip); the ci-ok sentinel distinguishing skipped from failed correctly; the compose fast loop; the TLS harness with a negative untrusted-CA case; and the peer-tool survey in testing.md — that document is community-credibility material in its own right.

Lens 2 — clean SchemaBot integration

1. Typed, enumerable outcomes at the public API — seed the requirement now. SchemaBot's side of the boundary has a hard rule: engine error text is never rendered into PR-facing markdown; the orchestrator needs classified outcomes so it can render fixed, sanitized lines. The foundations here point the right way (SQLSTATE matching, ErrInvariantViolation, the Phase-1 not-native-safe refusal contract) — but nothing yet says refusals and failures must be typed and enumerable at the package boundary, not prose. That's the difference between the orchestrator switching on a refusal kind and the orchestrator regex-ing an error string. Suggest one OC-* invariant in docs/schemabot-integration.md, plus a TM-style obligation for adapter-contract tests — a suite that pins the API surface the orchestrator consumes, so an engine upgrade can't silently change semantics (the role Spirit's status.State.String() / progress API plays for SchemaBot today).

2. Terminology, carrying over from my previous comment: the TM docs say "migration"; SchemaBot bans the word in favor of "schema change." Whichever way the decision goes, deciding it in this PR is what keeps the seam clean.

3. Release discipline is an integration prerequisite, not just OSS polish. SchemaBot will pin pg-sprite via go.mod exactly as it pins Spirit — that needs tagged semver releases and a stability statement for the imported packages. "No releases yet" is right for Phase 0; docs/schemabot-integration.md is the natural owner of the eventual policy.

4. The test harness will want reuse. A SchemaBot Postgres storage backend would need exactly the harness this PR builds (StartPostgres/NewSchema/TLS), but it lives under internal/, unimportable. Either promote it to an exported pkg/testutil when a second consumer exists, or accept the duplication knowingly.

5. Small nits: withParam in the TLS test string-manipulates a connection URL — test-only and harmless, but it's the exact pattern a parse-modify-reserialize connstring rule (suggested in my previous comment) would catch. And pre-push tests only changed packages, so a change that compiles but breaks a dependent package slips to CI — documented as intended ("CI remains the authoritative gate"), so just noting it.

Verified solid for integration: Go versions align across the two modules (1.26 both sides); github.com/block/pg-sprite imports cleanly; testcontainers in the module graph is a non-issue for SchemaBot (already present there) and only mild go.sum noise for other importers, since internal/testutil cannot compile into consumers; and docs/schemabot-integration.md as the single home for the integration contract is exactly the right structure.

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

[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

Summary: all six ranked findings accepted and addressed in a follow-up commit on this PR (no history rewrite — the branch has been reviewed); the only remainder is a repo-wide docs terminology sweep that touches files outside this PR, tracked as a follow-up.

#	Finding	Status	Explanation
1	No git/PR workflow section in a multi-agent repo	fixed	New "Git and PRs" section in AGENTS.md: draft PRs / no auto-create, no squash after human review, disclosure at bottom, Copilot-vs-human thread protocol, refresh PR summary after pushes, leaf approach.
2	No authoring rule that generated SQL must quote identifiers	fixed	Conventions line added: pgx.Identifier{...}.Sanitize() / quote_ident(), never raw interpolation; mirrored in .agents/checks/review.md.
3	"schema change" vs "migration" undecided; this PR's docs use "migration"	fixed (decision + this PR's strings)	Decided "schema change" — rule in AGENTS.md + review checks; TM-1/TM-3 and docs/README wording fixed here. The sweep of pre-existing docs (invariants.md, tcb-model.md, …) is deferred to a follow-up PR since those files aren't in this diff.
4	Flake prohibition without flake verification	fixed	scripts/test-flaky.sh <TestName> [iterations] [package] added (fail-fast, env passthrough) + AGENTS.md line pairing it with the no-timeout-increase rule.
5	No ban on string-manipulating connection strings	fixed	Conventions line: parse (pgx.ParseConfig) → modify fields → re-serialize, never string ops.
6	Vacuous in-flight test race — belongs in the registry, not AGENTS.md	fixed	Added TM-9 — The operation must outlive the observer (Binds: Phase 4), with the stronger form: assert the interruption actually landed mid-operation, not just final state.