feat(cli): add storage resync-identity-sequences admin subcommand

[Kiran01bm]

Adds a schemabot storage resync-identity-sequences admin subcommand that wires ResyncPostgresIdentitySequences to an operator entry point.

Why

PostgreSQL's GENERATED BY DEFAULT AS IDENTITY accepts explicit ids without advancing the backing sequence, so after an id-preserving bulk load into the storage database — a cross-dialect data move, or a restore from a dump without sequence state — the next default insert collides with a loaded row. The resync function landed with the storage layer but had no caller an operator could run.

What

A new storage command group for operations that connect to SchemaBot's own storage database directly (they work while the server is down, which is exactly when a data-move resync runs). Its first subcommand resolves the storage DSN from --dsn, or from the server config (--config, falling back to $SCHEMABOT_CONFIG_FILE) — the config route fails closed unless the configured storage dialect is postgres — then runs the advance-only, idempotent resync and logs one outcome per identity column.

Before / after

Before:
  explicit-id bulk load ──▶ sequences behind stored maxima
                              │
                              ▼
                    next default insert collides
                    (duplicate key on a loaded id)
                    no operator entry point to fix it

After:
  explicit-id bulk load ──▶ schemabot storage resync-identity-sequences
                              │  --dsn ... | --config ... | $SCHEMABOT_CONFIG_FILE
                              ▼
                    sequences advanced past stored maxima
                    next default insert draws max+1

Sample output

$ schemabot storage resync-identity-sequences --dsn postgres://…
time=… level=INFO msg="resolved storage DSN" source="--dsn flag"
time=… level=INFO msg="advanced identity sequence past stored maximum" table=settings column=id sequence_value=3
time=… level=INFO msg="identity sequence resync complete"

[Copilot]

Pull request overview

Adds an operator-facing CLI entry point to run api.ResyncPostgresIdentitySequences directly against SchemaBot’s PostgreSQL storage database (useful during maintenance when the server is down) to advance IDENTITY sequences after explicit-id bulk loads/restores.

Changes:

• Adds a new top-level storage CLI command group with resync-identity-sequences.
• Implements DSN resolution from --dsn or a server config file (with $SCHEMABOT_CONFIG_FILE fallback) and enforces postgres storage dialect for config-based runs.
• Adds unit tests for DSN resolution and integration tests proving the command unblocks default inserts after explicit-id loads.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
pkg/cmd/main.go	Registers the new storage command group in the root CLI.
pkg/cmd/commands/storage.go	Implements storage resync-identity-sequences command behavior and DSN resolution.
pkg/cmd/commands/storage_test.go	Unit tests for storage DSN resolution behavior and error cases.
pkg/cmd/commands/storage_integration_test.go	Integration tests validating end-to-end sequence resync behavior against a real Postgres storage DB.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

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

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head ceda099a, in a worktree, with the unit lane and the new integration tests run locally against a real PostgreSQL, plus a mutation battery over every guard.

Verdict: clean — nothing blocks. This is the best-tested PR in the batch: every guard I tried to disable died, including the ones I expected to be decorative, and the integration test proves the actual story — a real 23505 collision before the resync, id = 4 after. The DSN resolution fails closed at each step and the "this doesn't look like SchemaBot's storage database" guard is the right shape and is pinned. Two things, both about what the operator sees rather than what the command does.

#	Finding	Severity
1	The confirmation line names a config file, never the database it is about to modify	observability
2	The sample output shows a line the command no longer prints at default level	doc

---

1. The one line before the mutation doesn't say what it's pointed at

logger.Info("resolved storage DSN", "source", source)

source is deliberately DSN-free, and that's right — there's a test asserting the password never reaches it, which is exactly the discipline I'd want here. But the consequence is that the only pre-mutation line an operator sees says server config /etc/schemabot/config.yaml or STORAGE_DSN environment variable. Neither answers the question they actually have, which is which database am I about to advance sequences on.

That question is the whole safety story for this command. It runs in a maintenance window, with the server down, typically with more than one storage DSN in play — and the resolution chain is deep enough (--config → $SCHEMABOT_CONFIG_FILE → storage.dsn → storage.dsn_from → STORAGE_DSN → MYSQL_DSN) that "which one won" is genuinely non-obvious. The missing-tables guard catches a target that isn't a storage database at all; it does not catch staging when production was meant, because both pass.

pgx's ParseConfig already gives Host, Port and Database with no credentials attached, so "target", "storage-host:5432/schemabot" alongside the source is one line and turns a source label into a confirmation. Given the command's own docs say "Confirm the target and complete the resync before restarting the server", this is the line that lets an operator do the confirming.

2. The sample output doesn't match what the command prints

The body's sample block shows:

level=INFO msg="advanced identity sequence past stored maximum" table=settings column=id sequence_value=3

That case now logs at Debug, so a default-level run never prints it; and the block omits the identity sequence resync summary line that replaced it. So the one artifact a reviewer or operator would use to know what a successful run looks like shows a line they won't see and hides the line they will.

Worth reconsidering the downgrade itself rather than just fixing the sample. The three sequenceSkipped* cases at Debug is clearly right — they're the no-op majority. sequenceAdvanced is different in kind: it's the record of a mutation, in a one-shot admin command that runs a handful of times per database, not a hot loop. advanced=3 tells an operator that three sequences moved; it doesn't tell them which, or to what, which is the thing they'd want in the terminal scrollback if a default insert still collides afterwards. Keeping sequenceAdvanced at Info and keeping the new summary gives both, at a volume bounded by the number of identity columns in the storage schema.

Also

(nit) The env-var source label reimplements StorageDSN()'s precedence — Storage.DSN == "" && DSNFrom == nil, then STORAGE_DSN, then MYSQL_DSN. I traced both and they agree on every input today, including the whitespace case (StorageDSN would return a whitespace-only STORAGE_DSN, and the trim here catches it). Copilot asked for this and the answer is correct. The durable version is for StorageDSN() to report where it got the value, so the two can't drift; failing that, a comment on each side naming the other, because a divergence here is silent and produces a confidently wrong label on the very line Finding 1 is about.

(nit) --dsn skips the dialect gate that --config enforces. Nothing unsafe follows — pgx won't open a Go MySQL driver DSN, and a wrong PostgreSQL target is caught by the missing-tables guard — but the docs present the postgres-only check as a property of the command, and it holds on one of the two routes.

(nit) resolveStorageDSN splits on cmd.Config == "" to choose api.LoadServerConfig() over LoadServerConfigFromFile(configPath), but configPath has already been resolved from $SCHEMABOT_CONFIG_FILE at that point and LoadServerConfig() just re-reads the same variable. One call to LoadServerConfigFromFile(configPath) covers both branches and removes a second copy of the same resolution.

---

Action items

1. (Finding 1) Log the sanitized target (host, port, database — never credentials) next to the source.
2. (Finding 2) Update the sample output, and keep sequenceAdvanced at Info so a run records which sequences moved.
3. (optional) Have StorageDSN() report its own source; apply the dialect gate to --dsn; collapse the duplicate config-load branch.

---

Verified — tried to break, couldn't

Every guard is pinned. All six mutations died:

Mutation	Result
the "doesn't look like SchemaBot's storage database" guard never fires	🔴 TestResyncPostgresIdentitySequences_RejectsTargetWithoutStorageTables (integration)
the postgres-dialect gate never fires	🔴 TestResolveStorageDSN_RejectsNonPostgresStorageDialect
--dsn and --config stop being mutually exclusive	🔴 TestResolveStorageDSN_DSNAndConfigAreMutuallyExclusive
the env-var source label is never applied	🔴 TestResolveStorageDSN_ReportsEnvironmentSource
the connection check is skipped	🔴 TestResyncIdentitySequencesCmd_PingFailure
the resync itself becomes a no-op	🔴 both TestResyncIdentitySequencesCmd_* (integration)

I went in expecting the storage-tables guard to be decorative — it's the kind of check that usually ships untested — and it isn't.

The integration tests prove the behavior, not the plumbing. requireDefaultInsertCollides asserts a real pgconn.PgError with code 23505 before the resync, and requireDefaultInsertResumes asserts the first default insert draws exactly 4 — max+1, not merely "no error". Both the --dsn and --config routes are exercised end to end against a real postgres:16 with the full storage schema bootstrapped by EnsureSchema. That's the whole user-facing claim, tested as the user experiences it.

The schema scoping is consistent across all three queries, which is the failure mode I went looking for. missingPostgresTables and postgresIdentityColumns both filter on table_schema = current_schema(), and advancePostgresIdentitySequence's MAX(...) and pg_get_serial_sequence both resolve unqualified names through the same search_path. So there is no DSN — including one that sets a non-default search_path — where the guard inspects one schema and the resync mutates another.

Credentials can't reach the log. The source label is built from the config path and env var names, never the value, and TestResolveStorageDSN_ConfigResolvesPostgresStorageDSN asserts a distinctive password string is absent from it. Finding 1 asks for more target information, not for the DSN.

The connection handling follows the repo's rules exactly — postgresconn.Open rather than OpenReloadable (correct: this is a short-lived command pool, not the long-lived storage pool), PingContext immediately after open under a bounded 10s timeout so an unreachable target fails promptly, and utils.CloseAndLog on the handle it owns.

The advance-only claim holds at the SQL level. The setval is gated by a WHERE comparing against the sequence's next draw read from the sequence relation — correctly handling both is_called states — so a sequence already ahead returns no row and is left alone, and a descending sequence is skipped before any write. Rerunning is a no-op, which is what makes "safe to rerun" in the docs true rather than aspirational.

Error wrapping got materially better in the same pass. The bare return errs in ResyncPostgresIdentitySequences now carry what was being attempted and which column, so a failure mid-resync names the table and column rather than surfacing a naked driver error.

Copilot's thread was answered properly and resolved — the reply names commit 8cf9e6f4 and the source-detection behavior it describes really is in the tree with a test on it. The nit above is about the duplication that fix introduced, not a re-raise.

Ran locally at head: go build ./..., ./pkg/cmd/... ./pkg/api/... green, and the new integration tests passing against a real container. CI 34/34. No test deletions or weakened assertions. Leak check on the body, diff and docs clean, terminology clean.

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

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/1157, follow-up commit

#	Concern	Status
1	Blocking — a wrong target (typo'd dbname, foreign database) exits 0 with a success line byte-identical to the legitimate no-op; the operator's green signal to restart the server conceals a post-maintenance PK-collision outage	fixed (8cf9e6f) — guard inside ResyncPostgresIdentitySequences (per the review's placement note) errors when all 13 storage tables are missing from the target; pinned by TestResyncPostgresIdentitySequences_RejectsTargetWithoutStorageTables. The collateral-mutation half rides along: a foreign database now fails before any sequence is touched.
2	Deleting the Storage field from CLI leaves every test green — no regression protection that the subcommand is invocable	fixed (8cf9e6f) — TestStorageResyncIdentitySequencesIsInvocable in pkg/cmd/main_test.go parses ["storage","resync-identity-sequences","--dsn",...] via kong, following the existing TestRollbackRequiresEnvironmentFlag pattern.
3	Integration tests never prove the pre-resync collision, so they show inserts work — not that the resync made them work	fixed (8cf9e6f) — requireDefaultInsertCollides asserts the duplicate-key collision (SQLSTATE 23505) before Run in both integration tests.
4	Per-table outcomes log at Debug; the default-verbosity operator gets no confirmation of what was examined	fixed — Info summary line identity sequence resync summary with tables/examined/advanced/skipped counts (8cf9e6f); the follow-up commit additionally keeps each advance at Info per-table (it mutates production storage — the audit record of table, column, and new value belongs at default verbosity), with skips staying at Debug.
5	Storage password printed in clear on a pgx parse failure (?password=/keyword-with-space forms escape both pgx redaction and dsnParseError's *url.Error strip) — pre-existing, third exposure path	deferred — tracked as an internal follow-up (PRF-30): fix once in postgresconn.dsnParseError by also unwrapping *pgconn.ParseConfigError chains, covering all three call sites including the bootStorage retry Warn-loop that ships to indexed logs (the exposure that matters, per the review's own bucketing).
6	Three guards unpinned: empty resolved DSN, the post-open ping, and the loose credential-leak assertion (generic test password)	fixed (8cf9e6f) — TestResolveStorageDSN_EmptyConfigDSN + TestResolveStorageDSN_WhitespaceDirectDSN pin the emptiness guard, TestResyncIdentitySequencesCmd_PingFailure pins the ping storage database: prefix, and the fixture password is now the distinctive hunter2-distinctive with a NotContains on it, catching partial leaks.
7	Help text carries none of the operational preconditions; nothing lands in docs/	fixed (8cf9e6f) — the help string now carries the run-after-commit / before-default-inserts / advance-only-rerun-safe clause, and docs/configuration.md gains a "Resyncing PostgreSQL identity sequences" operator section (per the check-runs.md backfill precedent).
8a	Diagnostics go to stdout, unlike every other CLI command in the package	fixed (8cf9e6f) — logger writes to stderr; stdout stays free for future machine-readable output.
8b	Run omits *Globals, so the audit record carries no schemabot_version	fixed (8cf9e6f) — Run(ctx, g *Globals); logger carries schemabot_version.
8c	Text vs JSON handler — worth a deliberate call	reply — text kept deliberately, now documented in a code comment: this is a one-shot operator command read at a terminal during a maintenance window, not a long-running server whose stdout feeds a JSON collector; even pod usage is an interactive kubectl exec, and the durable audit record is the Info summary + version line.
8d	Whitespace --dsn resolves to libpq local defaults; config route swallows whitespace identically	fixed (8cf9e6f) — both sources are TrimSpaced before the emptiness test; whitespace-only input fails fast with the existing clear message (pinned per row 6).
8e	storage.go re-implements api.LoadServerConfig(); open/ping block duplicates ensure_schema_postgres.go	fixed/reply — the env-var resolution now goes through api.LoadServerConfig() (single site decides what $SCHEMABOT_CONFIG_FILE means). The open/ping block stays duplicated deliberately: the two sites bound the ping differently (EnsureSchema pings under its overall EnsureSchemaTimeout; the CLI's resync context is deliberately unbounded, so it needs its own storagePingTimeout) — a shared helper would need timeout parameterization that hides exactly that difference, for two call sites.
8f	requireDefaultInsertResumes is named for require but carries its load-bearing check in assert	fixed (8cf9e6f) — the id assertion is now require.Equal.