feat(api): answer storage schema requests for the storage a server booted on

[aparajon]

Why this matters

The RPCs exist one layer down; this is the data plane implementing them. A server answers storage schema requests for the storage it is actually running on, and refuses to answer for anything else.

What it does

The database is pinned at boot, and credentials are not. StorageDSN() reads mutable environment, files and structured references, so re-resolving it per call means a config edit that moves storage from database A to B silently makes plan and apply read and write B while the instance is still initialized against A. So Build records the target it booted against, and every request checks against it:

  boot                      request N                    request N+1
  ────                      ─────────                    ───────────
  resolve DSN               re-resolve DSN               re-resolve DSN
  parse → address/database  parse → same identity        parse → different database
  store on the adapter      ✓ answer                     ✗ refuse, naming both

The identity is deliberately address plus database name and nothing else, so a rotated password still compares equal. That asymmetry is the point and it matches the pool: the reloadable storage pool re-resolves its own DSN only after an authentication failure, so credentials are allowed to move underneath a running instance and the database is not. The refusal names both sides, because an operator who sees it has either edited the wrong config or is talking to the wrong pod, and the message has to be enough to tell which:

the configured storage now names db-2.example:3306/schemabot, but this instance booted against db-1.example:3306/schemabot; the storage schema surface only answers for the database this instance is running on, so restart it to adopt the new storage

A caller fault reads as a caller fault. Validation failures on a supplied schema — a missing source, a file name that is a path, an empty file — are wrapped in tern.ErrInvalidStorageSchemaRequest and surface as InvalidArgument carrying the full message, since the text is the caller's own input and naming the offending file is the only way they can fix it. Everything else stays Internal with a fixed summary pointing at the logs, so a dial failure or a DSN fragment never reaches a client.

Apply observes the caller's context on the plans either side of the convergence, and not on the convergence. That is deliberate. EnsureSchema takes no context and bounds itself with EnsureSchemaTimeout; a convergence abandoned part-way because a caller hung up leaves the storage schema between two releases with nobody watching, while running it out leaves a state the next plan can describe exactly. A caller that disconnects stops waiting for an answer rather than stopping the work.

The same pin is enforced one level lower, on the pool itself. The reloadable pool's callback re-resolves the DSN after an authentication failure, and an unguarded callback means one auth failure is all it takes for a config that now names another database to answer the dial — leaving the server on storage it never bootstrapped while the adapter refuses requests because its own pin still says otherwise. Two components disagreeing about which database the process is on is worse than either being wrong, so the callback permits a rotated credential and refuses a moved database, failing the connection rather than relocating the server.

A host binary that embeds SchemaBot and omits WithBuildInfo now gets its module-graph version stored on the server, not just added to the logger — otherwise every report from that instance is unattributable to a build, which defeats the reason a report names a version at all.

Invariants

• AV-9, extends. Enforcement reaches the data plane: the adapter is bound at construction to the storage its instance booted on, so no caller — not the control plane, not an operator — can point a convergence at a different database. A later layer (#1413) lets a caller name which schema converges; the binding this layer adds is what keeps that from also naming which database.

• AZ-5, upholds. The adapter answers only for the database the instance booted against, and the pool refuses to dial a database that moved underneath it. A config edit cannot silently relocate either one.

Opened by Claude Code (Opus 5).

[Copilot]

🟡 Changes recommended

Critical target-pinning and credential-redaction issues, along with additional correctness and coverage gaps, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Implements storage-schema plan/apply RPCs for the database the server booted against, including target checks, HTTP/gRPC wiring, and version attribution.

Changes:

• Captures and validates boot storage identity while allowing credential rotation.
• Adds storage-schema planning and convergence behavior.
• Updates build, target, adapter, and integration tests.

File summaries

File	Review summary
pkg/serve/storage_target.go	Adds storage identity parsing. Critical (3 votes): parsing bypasses DSN redaction and may leak credentials in logs.
pkg/serve/storage_target_test.go	Tests target parsing, credential rotation, and target movement behavior.
pkg/serve/storage_schema.go	Implements schema plan/apply handling. Moderate (1 vote each): source-only requests are misclassified; caller validation occurs too late; malformed supplied DDL is classified as internal; successful/cancellation paths lack coverage. Nit (1 vote): update the AV-9 enforcement pointer.
pkg/serve/storage_schema_test.go	Tests adapter validation, policies, and target handling.
pkg/serve/serve.go	Records boot state and wires services. Critical (1 vote): pool reloads can switch storage targets. Moderate (1 vote): missing end-to-end registration coverage. Nit (1 vote): update the AV-9 enforcement pointer.
pkg/serve/serve_storage_integration_test.go	Updates storage boot integration coverage.
pkg/serve/serve_build_test.go	Updates server construction fixtures.

Review details

Suppressed comments (7)

pkg/serve/serve.go:542

• The new Build wiring is not covered end to end: the added tests exercise storageSchemaAdapter directly and exercise HTTP/gRPC routing with fakes, but no test builds a server and verifies that its local HTTP handler and registered gRPC service expose this concrete adapter. A regression that drops either SetStorageSchemaService or WithStorageSchemaService would therefore leave the advertised data-plane surface unavailable while all current tests pass. Add a storage-backed build/integration test for both registrations.

	storageSchema, err := srv.newStorageSchemaService()
	if err != nil {
		return nil, fmt.Errorf("build storage schema service: %w", err)
	}
	srv.storageSchema = storageSchema
	svc.SetStorageSchemaService(storageSchema)

pkg/serve/serve.go:542

• This introduces the data-plane binding that makes AV-9's "the binary running it" guarantee true, but docs/invariants.md:327-329 still lists only the API bootstrap and storage-schema implementation as enforcement. Update the canonical AV-9 enforcement pointer to include this serving adapter/boot-target check; otherwise the registry does not identify the new safety gate.

	storageSchema, err := srv.newStorageSchemaService()
	if err != nil {
		return nil, fmt.Errorf("build storage schema service: %w", err)
	}
	srv.storageSchema = storageSchema
	svc.SetStorageSchemaService(storageSchema)

pkg/serve/storage_schema.go:193

• This adds the data-plane side of AV-9: checkBootTarget is what prevents the storage surface from reading or converging a DSN different from the database this instance booted on. The AV-9 registry still lists only pkg/api/ensure_schema*.go and pkg/api/storage_schema.go in its Enforced: line, so the invariant documentation no longer identifies all of the enforcement added here. Please update that pointer in docs/invariants.md in the same change.

func (a *storageSchemaAdapter) checkBootTarget(dsn string) error {
	resolved, err := storageTargetFor(a.dialect, dsn)
	if err != nil {
		return fmt.Errorf("read the storage target the current configuration names: %w", err)
	}

pkg/serve/storage_schema.go:106

• This branch treats every request with zero schema_files as a request for the running binary's embedded schema. A StorageSchemaPlanRequest can also arrive directly over gRPC with only schema_source set (the HTTP validator is not on that path), so the source is silently ignored and the response is attributed to the wrong schema instead of returning InvalidArgument. Distinguish an actually empty request from a source-without-files before defaulting to the embedded schema.

	if len(files) == 0 {

pkg/serve/storage_schema.go:81

• The caller's schema is validated only after resolving and checking the mutable storage DSN. If the request has an invalid schema (for example, files without a source) while the DSN is unreadable or has moved, target returns a non-sentinel error first, so the gRPC wrapper reports Internal and hides the fixable caller error. Validate desiredSchema before target so caller input consistently reaches ErrInvalidStorageSchemaRequest.

	dsn, opts, err := a.target(req.GetAllowDestructive())
	if err != nil {
		return nil, err
	}
	desired, err := a.desiredSchema(req)

pkg/serve/storage_schema.go:90

• When the supplied file contents are syntactically invalid or violate a dialect's schema-file shape, api.PlanStorageSchema returns a plain parser/shape error here. Because only desiredSchema's metadata errors are wrapped with tern.ErrInvalidStorageSchemaRequest, the gRPC/HTTP layers classify these caller-authored files as Internal and hide the actionable reason. Propagate the caller-error sentinel from the validation/parser path (without wrapping storage/dial failures) so malformed supplied DDL is returned as InvalidArgument.

	report, err := api.PlanStorageSchema(diffCtx, dsn, desired, a.logger, opts...)
	if err != nil {
		return nil, fmt.Errorf("diff storage schema (dialect %s) against %s: %w", a.dialect, desired.Description, err)

pkg/serve/storage_schema.go:142

• The new adapter methods are only tested on pre-database branches (target and desiredSchema); no test exercises a successful plan/apply through this adapter or the documented context split where the two plans observe cancellation but EnsureSchema continues. That leaves the central data-plane behavior and option/version wiring unverified. Add an integration test (or seams around the API calls) covering the successful RPC path and cancellation during each phase.

	planned, remaining, err := api.ApplyStorageSchema(ctx, dsn, a.logger, opts...)
	if err != nil {
		return nil, fmt.Errorf("converge storage schema (dialect %s): %w", a.dialect, err)
	}
	planned.AttributeTo(a.version)
	remaining.AttributeTo(a.version)

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

---

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

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1404, 79956e6.
Verdict: 2 findings — 1 non-blocking (untested wiring), 1 suggestion.

Non-blocking

No test covers the wiring this PR exists to add. Neither Build setting the adapter on the Server and the Service (serve.go:541) nor RegisterGRPC's new non-nil branch is exercised. Mutating svc.SetStorageSchemaService(storageSchema) to _ = storageSchema leaves go test ./... green: the one RegisterGRPC test builds a Server literal with storageSchema nil, the new adapter tests construct storageSchemaAdapter by hand, and nothing under e2e/ or integration/ touches the storage-schema routes. The operator-visible regression — this server does not expose its own storage schema — would ship green.

General suggestions

The plan timeout is applied twice. storageSchemaAdapter.StorageSchemaPlan wraps the call in api.StorageSchemaPlanTimeout (storage_schema.go:85) while api.PlanStorageSchema already imposes the same 30s budget on its own ctx (storage_schema.go:65, untouched here). Behaviour is unchanged, but the duplicated bound invites a future reader to change one constant site and assume the other followed. The apply path already relies on the inner bound alone — the plan path could do the same.

The one thing that could have broken, verified

Server.storageDSN naming a database the pool never actually dials. Verified closed on three counts: bootStorage/connectStorage return "", err on every failure path so the recorded DSN is exactly the one the successful attempt opened, pinnedStorageDSN captures bootErr eagerly and fails every reload on an unparseable boot DSN rather than returning a zero storageTarget that would compare equal to another zero target, and connreload.runReload keeps the current connector and arms the cooldown when Reload errors (connreload.go:278) — so a refused pin leaves the pool dialing the boot database, matching the documented claim.

Verified correct

• success = true moves after newStorageSchemaService, so the deferred CloseAndLog(db) and CloseAndLog(svc) still run on the new error path — no pool or service leak.
• storageTargetFor fails closed on an unknown dialect and on an unparseable DSN for both families.
• mysql.ParseDSN normalizes Addr/DBName, making the zero-target-equality hole unreachable.
• github.com/block/mysql is the same fork pkg/mysqlconn parses with, so boot target and reloadable pool agree on DSN normalization.
• PostgreSQL routes through postgresconn.ConnectionDSN before pgx.ParseConfig, keeping the *url.Error password echo out of the error chain.
• AttributeTo is idempotent, so ApplyStorageSchema's manual-drift path (same report as planned and remaining) is stamped twice harmlessly.
• ApplyStorageSchema never returns a nil remaining with a nil error, so the adapter's remaining.Database log is nil-safe.
• Boot-target and DSN text never reach the caller: gRPC maps to codes.Internal and HTTP to 500 with a logs pointer; the cause is logged server-side at three sites.

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

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at 79956e6. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[aparajon]

🤖 Both addressed in 45e303eb, and the branch is now rebased onto 7cb9fccc.

No test covers the wiring this PR exists to add. Fixed, and your mutation is the one the new test is built around: svc.SetStorageSchemaService(storageSchema) → _ = storageSchema now fails. The wiring also has a name of its own instead of two loose assignments in Build, so there is one place that binds the adapter to both surfaces and one test that fails when either half goes away. You were right about the shape of the regression — one surface answering every request as unsupported reads exactly like a data plane too old to serve it, which is the least debuggable version of this bug.

The plan timeout is applied twice. Removed the outer one. The inner bound in PlanStorageSchema is the one that stays, so plan and apply now both rely on a single budget site, as you suggested.

---

Replied by Claude Code (claude-opus-5) on Armand's behalf.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1404, 45e303e.
Verdict: 4 findings — none blocking; one robustness gap in the boot-target pin, the rest test coverage.

Non-blocking

The boot-target pin compares the host spelling byte-for-byte, so the same database under a differently-cased hostname reads as "the storage moved." serve.go:657 compares resolved != boot where the address is cfg.Host un-folded (storage_target.go:64), and neither mysql.ParseDSN nor pgx.ParseConfig case-folds it — RDS endpoints can arrive uppercased, which is why postgresconn.go:446 lowercases before IsRDSHost. If a mounted secret is rewritten with the lowercase spelling plus a rotated password, pinnedStorageDSN errors, connreload keeps the stale credentials behind its 30s cooldown, and checkBootTarget tells every plan/apply to restart for the database it is already on. The tests only cover differing hosts (db-1 vs db-2), so the equal-host/different-case case is uncaught; strings.EqualFold on the host would close it.

Nothing binds the storage pool to pinnedStorageDSN, so the one line that installs the new guard is untested. serve.go:615 is the sole production use, and reverting its 4th argument to the pre-PR cfg.StorageDSN leaves the whole suite green: TestPinnedStorageDSN builds the callback itself, TestStorageDialectDispatchFailsClosed supplies a stub reload, and both connectStorage integration tests discard the DSN and never reload. The exact failure the 40-line comment at serve.go:628-642 exists to prevent could be reintroduced silently.

No test builds a Server with a non-nil storageSchema and calls RegisterGRPC, so the branch attaching tern.WithStorageSchemaService is dead code to the suite. The only caller, TestServerRegisterGRPCRegistersTernService (serve_build_test.go:29), leaves the field nil, so serve.go:752 never evaluates true and deleting opts... still compiles; its assertion cannot move either, since the static ServiceDesc already carries the methods. TestRegisterStorageSchemaBindsBothSurfacesToTheBootStorage only asserts srv.storageSchema != nil — a proxy for the field, not the registration — so a dropped option would answer every control-plane call Unimplemented, telling operators to upgrade an already-current deployment.

General suggestions

A schema that passes file-name validation but fails the dialect parser is reported as Internal/500 pointing at the data plane's logs. StorageSchemaFromFiles checks only *.sql and non-emptiness, so a typo in a CREATE TABLE first fails inside Spirit, and the wrap at storage_schema.go:108 carries no tern.ErrInvalidStorageSchemaRequest. That contradicts writeStorageSchemaFailure's own promise that "a caller who sent an unreadable schema file gets 400 and the reason", and sends the operator to read someone else's logs to find their own typo.

The one thing that could have broken, verified

pinnedStorageDSN returning an error on a moved target could have wedged or dropped the live pool. It does not: connreload's runReload treats a Reload error as "keep current credentials" and arms the cooldown, so the existing connections keep serving and only new credential adoption is blocked until restart — which is the intended fail-closed behaviour, not a pool failure.

Verified correct

• Build calls registerStorageSchema before success = true, so the deferred CloseAndLog(db)/CloseAndLog(svc) still run — no new pool or service leak.
• registerStorageSchema cannot newly fail a boot: storageTargetFor dispatches on the same two dialects and uses the same parse calls the pool open already makes.
• s.storageSchema is only ever assigned a non-nil *storageSchemaAdapter (the error path returns first), so the != nil check cannot hit the typed-nil-in-interface trap.
• ApplyStorageSchema never returns a nil report with a nil error, and AttributeTo is nil-safe, so the double AttributeTo/StorageSchemaReportProto is harmless.
• AttributeTo rewrites SchemaSource only when empty or the reserved embedded-binary phrase, so a caller-supplied diff keeps its attribution.
• target() builds exactly the three options connectStorage passes to the boot's EnsureSchema, so an operator convergence runs under the same policy and statement budget.
• MySQL/Postgres parsers normalize consistently on both sides of the pin (port defaults, empty addr, pgx env defaults), so port-omitted and keyword-form respellings do not trip the guard.

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

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at 45e303e. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1404, d6a3471.
Verdict: 4 findings — all non-blocking (1 error-class, 1 invariant doc, 1 multi-host DSN, 1 test gap).

Non-blocking

A caller's unparseable schema file returns 500/Internal instead of the promised 400/InvalidArgument. pkg/serve/storage_schema.go:108 wraps the diff error without tern.ErrInvalidStorageSchemaRequest, unlike its sibling at line 131; StorageSchemaFromFiles validates names only, never content, so a typo like SELCT 1 first fails inside spirit's ParseCreateTable. The operator gets "see the answering deployment's logs" for their own typo, and the one supplied-schema refusal test covers the missing-description branch instead.

AV-9's Enforced: citation does not name the file that now enforces it. docs/invariants.md:330 still points only at pkg/api/storage_schema.go, but the widen-never-narrow rule is newly created by effectiveAllowDestructive at pkg/serve/storage_schema.go:236 (return a.configAllowsDestructive || requestAllowsDestructive). AGENTS.md requires the citation move in the same PR and warns about exactly this — a pointer that resolves cleanly to the wrong file, so a future ||→&& regression lands where nobody is looking.

The PostgreSQL boot-target pin compares only the DSN's first host. pkg/serve/storage_target.go:64 reads cfg.Host while pgconn puts the rest in cfg.Fallbacks and still dials them, so host=db-a,db-b … and host=db-a,db-other … resolve to the identical target. Both the pool reload guard and checkBootTarget pass the rewritten DSN, and if db-a is down the server silently connects to storage it never bootstrapped. Triggering state is unconfirmed — no multi-host storage DSN exists in any config example or test — so this is a latent hole, not a live defect.

Nothing tests that RegisterGRPC threads the adapter onto the gRPC server. The sole test at pkg/serve/serve_build_test.go:28 builds a Server with storageSchema nil, so the branch and opts... spread at pkg/serve/serve.go:753 never execute, and TestRegisterStorageSchemaBindsBothSurfacesToTheBootStorage only asserts the field is non-nil. Dropping the spread keeps the suite green while every control-plane request returns Unimplemented, rendered to operators as 501 "upgrade that deployment and retry" — a confidently wrong remedy.

The one thing that could have broken, verified

Pinning the storage DSN at boot could have wedged credential rotation: if pinnedStorageDSN handed the pool a stale password, or a Reload error killed the pool, every rotation would become an outage. Traced pkg/connreload/connreload.go:278-282 — Connector.Connect calls Reload only when Refused(err), and a failed reload leaves the pool on its current credentials with a cooldown armed rather than wedging it. Equality in storageTarget ignores exactly credentials and nothing else (mysql.ParseDSN normalizes Addr with the default port; pgx.ParseConfig applies the same env defaults on both sides), so a rotation compares equal while a moved host or renamed database does not — pinned by the six cases in storage_target_test.go, including the keyword-vs-URL form.

Verified correct

• success = true moved after registerStorageSchema (serve.go:544), so both deferred utils.CloseAndLog calls still run — pool and service are closed, not leaked, on the new error path.
• bootStorage/connectStorage multi-return threading is correct: every error path returns "" with the error, and the DSN returned is the one the successful attempt opened the pool with, not a re-resolve.
• storageTargetFor cannot newly fail a boot that previously succeeded — both pools already resolve through the same parsers, so a DSN that opened a pool parses here.
• api.ApplyStorageSchema never returns nil planned/remaining with a nil error, so the unconditional AttributeTo calls at storage_schema.go:159 cannot nil-panic; the manual-remediation branch returns the same pointer twice and AttributeTo is idempotent.
• RegisterGRPC's nil guard keeps tern.NewServer answering Unimplemented for Servers not built by Build, and api.New has exactly one non-test caller (serve.go:465), so every real server gets the adapter on both surfaces.
• AttributeTo rewrites SchemaSource only when empty or the reserved embedded phrase, and StorageSchemaFromFiles refuses that exact phrase — a caller-supplied schema can never be relabelled with the responder's version.

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

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at d6a3471. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1404, 9d62c4f.
Verdict: 4 findings — no blocking; 2 non-blocking (PostgreSQL boot-target granularity, untested gRPC wiring), 2 suggestions.

Non-blocking

The PostgreSQL boot target ignores the schema namespace, so a search_path rewrite passes the guard. storage_target.go:65 builds the target from address + cfg.Database only, and checkBootTarget compares the whole struct — but pgx keeps search_path in RuntimeParams and the bootstrap resolves every table through current_schema() (ensure_schema_postgres.go:660). Boot against …/schemabot?search_path=sb_v1, have the mounted secret rewritten to search_path=public, and the next plan reports every storage table outstanding and an apply converges a second copy of the schema while the server keeps reading sb_v1. PLAUSIBLE rather than confirmed: it needs a storage DSN carrying search_path, which is undocumented for storage.dsn.

The gRPC half of "one adapter on both surfaces" has no test. Mutating serve.go:753 back to the pre-PR tern.NewServer(client, s.logger).Register(gs) under go test -overlay still yields ok github.com/block/schemabot/pkg/serve — the only nearby assertion checks the srv.storageSchema field, not the registration, and no integration or e2e test references StorageSchema. A refactor that drops the option ships silently, and control-plane plans come back Unimplemented → HTTP 501 telling the operator to upgrade a deployment that is already current.

General suggestions

Nothing asserts that connectStorage is handed the pinned reload. serve.go:615 is the PR's central safety wiring, yet TestPinnedStorageDSN builds the closure itself and TestStorageDialectDispatchFailsClosed passes its own stub. Since cfg.StorageDSN is a type-compatible substitute, reverting the call site compiles and leaves every test in the diff green while the pool again follows a rewritten secret.

moduleVersion()'s "unknown" sentinel leaks from the log field into the report. It is documented at serve.go:96 as a logging concern, but serve.go:373 feeds it to api.EmbeddedStorageSchema, whose empty-version fallback is skipped because "unknown" is non-empty. A host binary using a local replace directive therefore shows schema_source: "the schema embedded in unknown"; passing "" to the report while keeping "unknown" for the log attr gives the intended wording.

The one thing that could have broken, verified

The pinned-DSN guard is the load-bearing mechanism: if it could wedge or mislead the pool, an auth-refused dial would take storage down. Verified it cannot — target() resolves the DSN once, checks it, and returns that exact string to Plan/Apply (no TOCTOU), and connreload's runReload/refresh treats a refusing Reload as an error that keeps the current credentials, arms the 30s cooldown and surfaces the original dial error, so a refusal neither blocks the pool nor spins the secrets backend.

Verified correct

• Build failure still closes the storage pool and service: registerStorageSchema runs before success = true (serve.go:423).
• No new startup-failure mode: storageTargetFor uses the same parsers openStoragePool already ran the DSN through.
• bootStorage/connectStorage return the successful attempt's DSN, so Server.storageDSN names the database api.EnsureSchema actually converged.
• AttributeTo is idempotent (storage_schema_source.go:150), so the manual-remediation double-stamp is harmless.
• Refusal text naming hosts stays server-side: non-InvalidArgument errors map to Internal / HTTP 500, upholding AV-8.
• effectiveAllowDestructive only widens the deployment's configured policy, per AV-9.

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

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at 9d62c4f. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[aparajon]

🤖 Three rounds — 45e303eb, d6a34716, 9d62c4f5 — answered together, since they overlap. I checked each against the current head rather than reasoning from the diff, and six of these are real and still open. None of them is fixed by 45dcac32, including the one that commit's message claims.

Both "untested wiring" findings reproduce. I ran them:

• Reverting RegisterGRPC to tern.NewServer(client, s.logger).Register(gs) — dropping the option spread — leaves ./pkg/serve green. You flagged this in all three rounds and you were right each time. TestRegisterStorageSchemaBindsBothSurfacesToTheBootStorage, which I added in 45dcac32 for exactly this, asserts srv.storageSchema != nil and then calls the adapter directly. It never goes through RegisterGRPC, so it proves the field is populated and nothing about the registration that reads it. The commit message overstated what it covered.
• Substituting cfg.StorageDSN for pinnedStorageDSN(...) at the pool open also leaves the suite green. The 40-line comment above pinnedStorageDSN explains a failure mode that no test would notice being reintroduced.

A caller's unparseable schema still answers 500. Confirmed at pkg/serve/storage_schema.go:108: the diff error is wrapped without tern.ErrInvalidStorageSchemaRequest, while its sibling in desiredSchema carries it. So a file that clears name validation and fails in the parser contradicts writeStorageSchemaFailure's own promise and sends the operator to read a log they cannot reach for their own typo.

AV-9's Enforced: line does not name the file that now enforces it. Confirmed: it names the two bootstrappers and pkg/api/storage_schema.go, but the widen-never-narrow rule is implemented by effectiveAllowDestructive in pkg/serve/storage_schema.go. This is the failure mode AGENTS.md warns about specifically — a pointer that resolves cleanly to the wrong file, so a future ||→&& lands where nobody is looking.

moduleVersion()'s sentinel reaches the report. Confirmed: "unknown" is non-empty, so EmbeddedStorageSchema skips its own fallback and an embedded host with no recorded version reports the schema embedded in unknown instead of the schema embedded in this binary. The fallback exists; the sentinel defeats it.

The host comparison is byte-for-byte. Confirmed — net.JoinHostPort(cfg.Host, …) with no folding, and the tests only vary the host itself (db-1 vs db-2), never its case. An RDS endpoint arriving uppercased after a secret rewrite would read as a moved target, which is why postgresconn lowercases before IsRDSHost. strings.EqualFold on the host closes it.

The two latent ones I am leaving as latent, deliberately. The PostgreSQL pin reading only cfg.Host (ignoring Fallbacks) and ignoring search_path are both real gaps in the pin's granularity, and both need a storage DSN shape that no config example, test, or deployment uses — multi-host, or carrying search_path. Your own verdicts said PLAUSIBLE rather than confirmed, and I agree. Widening the pin on speculation would make it reject respellings it should accept, which turns a latent hole into a live restart loop. If search_path ever becomes a documented storage option, the pin has to grow a namespace component in the same change.

All six confirmed items are owed work, not disputed findings — tracked, and they land before anything ships this surface. Thanks for holding the line on the gRPC registration across three rounds; the test I wrote for it was the wrong test and only the third repetition made that obvious.

---

Replied by Claude Code (claude-opus-5) on Armand's behalf.