feat(api): let a control plane ask a data plane about its own storage

[aparajon]

Why this matters

A data plane's storage database is reachable from the data plane, not from the control plane an operator talks to. So "which storage DDL is outstanding on deployment west" has to be asked of west itself, over the connection that already exists between the two: the Tern gRPC endpoint.

What it does

• PlanStorageSchema and ApplyStorageSchema become Tern RPCs.
• Every serving instance registers an adapter bound to the storage it booted with, so an instance reads its own database with its own embedded schema files. No instance can report another's storage under its name.
• An endpoint built without the adapter refuses these RPCs rather than guessing at a database — which is what a build that never resolved a storage DSN should do.

 operator                 control plane            data plane "west"
    │                          │                          │
    │  storage plan            │                          │
    │  --deployment west ─────▶│                          │
    │                          │  PlanStorageSchema ─────▶│
    │                          │        (gRPC)            │
    │                          │                          ▼
    │                          │                   its own storage
    │                          │                   + its own schema
    │                          │                        files
    │                          │◀───── report ────────────│
    │◀──────── report ─────────│                          │

StorageSchemaService is deliberately not part of tern.Client. Client is the schema change surface a data plane exposes about the databases it manages; this is about the data plane's own bookkeeping, and keeping them apart is what makes "an endpoint that cannot answer refuses" expressible.

Invariants

• AV-9, upholds. The convergence an RPC reaches is the startup bootstrap, and it applies that bootstrap's destructive refusal and manual-remediation gate whatever the caller asked for. The apply message carries no schema files at this layer; #1413 adds the field, deliberately, so a release can be converged before its first pod starts. It changes which statements are computed and nothing about which of them are permitted.

Opened by Claude Code (Opus 5).

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain around storage binding, cancellation, version reporting, and error status handling.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds Tern RPCs for inspecting and applying a data plane’s own storage schema through its existing gRPC connection, using embedded schemas to uphold AV-9.

Changes:

• Adds protobuf, gRPC, and HTTP gateway storage-schema APIs.
• Registers storage adapters and client forwarding.
• Adds schema report conversions and tests.

File summaries

File	Summary / final review notes
pkg/tern/storage_schema.go	Defines the storage-schema service interface.
pkg/tern/server.go	Adds RPC handlers. Moderate (2): preserve validation status; moderate (1): preserve cancellation/deadline status. Nit (1): add focused RPC tests.
pkg/tern/grpc_client.go	Forwards storage-schema RPCs.
pkg/serve/storage_schema.go	Implements local storage operations. Critical (1): pin the boot-resolved database identity. Moderate (3): make convergence context-aware or explicitly document detached execution.
pkg/serve/storage_schema_test.go	Tests adapter policies and schema resolution.
pkg/serve/serve.go	Registers the adapter. Moderate (3): persist the fallback module version. Nit (1): add successful end-to-end RPC coverage.
pkg/serve/serve_build_test.go	Updates server registration coverage.
pkg/proto/ternv1/tern.pb.gw.go	Generated HTTP gateway bindings.
pkg/proto/ternv1/tern.pb.go	Generated protobuf types.
pkg/proto/ternv1/tern_grpc.pb.go	Generated gRPC bindings.
pkg/proto/tern.proto	Defines storage-schema RPCs and messages.
pkg/api/storage_schema_proto.go	Converts storage-schema reports to and from protobuf.
pkg/api/storage_schema_proto_test.go	Tests report round trips.

Review details

Files not reviewed (3)

• pkg/proto/ternv1/tern.pb.go: Generated file
• pkg/proto/ternv1/tern.pb.gw.go: Generated file
• pkg/proto/ternv1/tern_grpc.pb.go: Generated file

Suppressed comments (3)

pkg/serve/serve.go:675

• The new adapter is only tested for option/DSN helpers; no test exercises a successful StorageSchemaPlan or StorageSchemaApply through the gRPC server and GRPCClient. A regression in registration, dialect/DSN wiring, context or policy propagation, or proto conversion would pass the current suite. Add integration coverage with a storage test database and a loopback Tern RPC.

	// The storage-schema service answers for this instance's own storage
	// database, which is the only way a control plane can read it: a data
	// plane's storage is reachable from the data plane, and the gRPC endpoint
	// is the connection that already exists between the two.
	tern.NewServer(client, s.logger, tern.WithStorageSchemaService(s.storageSchemaService())).Register(gs)

pkg/tern/server.go:71

• All adapter errors are converted to codes.Internal here. A canceled request or the adapter's 30-second plan deadline therefore reaches callers as a server failure rather than Canceled/DeadlineExceeded, so the control plane cannot distinguish an abandoned or timed-out read from a storage failure. Preserve cancellation/deadline status codes while keeping non-context errors sanitized.

	if err != nil {
		// The caller sees a sanitized message, so this log is the only place
		// the cause survives — and a diff that cannot be computed is exactly
		// what an operator is trying to see during a failed deploy.
		s.logger.ErrorContext(ctx, "storage schema diff failed", "error", err)
		return nil, status.Error(codes.Internal, "storage schema diff failed; see data plane logs")

pkg/tern/server.go:71

• The new RPC boundary has no tests in pkg/tern/server_test.go, which otherwise covers this server's status mapping. In particular, no test proves that an endpoint without an adapter returns Unimplemented or that adapter failures are sanitized to Internal; a regression in either compatibility/safety behavior would pass the current suite. Add focused tests for both storage-schema methods, ideally including one actual gRPC round trip.

func (s *Server) StorageSchemaPlan(ctx context.Context, req *ternv1.StorageSchemaPlanRequest) (*ternv1.StorageSchemaPlanResponse, error) {
	if s.storageSchema == nil {
		return nil, errStorageSchemaUnsupported
	}
	resp, err := s.storageSchema.StorageSchemaPlan(ctx, req)
	if err != nil {
		// The caller sees a sanitized message, so this log is the only place
		// the cause survives — and a diff that cannot be computed is exactly
		// what an operator is trying to see during a failed deploy.
		s.logger.ErrorContext(ctx, "storage schema diff failed", "error", err)
		return nil, status.Error(codes.Internal, "storage schema diff failed; see data plane logs")

• Files reviewed: 10/13 changed files
• Comments generated: 4
• 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/1393, bf2290a.
Verdict: 4 findings — 2 non-blocking (test gaps), 2 suggestions.

Both finder lenses completed. Adversarial verification was capped at 8 candidates, so 4 lower-ranked candidates were never verified either way and are not reported here.

Non-blocking

The Unimplemented branch — the thing the whole client-side contract keys on — is untested. Both new tests build the server with WithStorageSchemaService, so the if s.storageSchema == nil guard (server.go#L62, #L79) and both GRPCClient wrappers (grpc_client.go#L590, #L601) never run — the string "does not support storage schema" appears in no test. Swap that return to FailedPrecondition and the suite stays green while operators lose the upgrade guidance. TestGRPCClientPullSchemaKeepsInfrastructureUnimplemented is the precedent to copy (storage_schema_server_test.go#L45).

The proto round-trip fixture omits Host and SchemaSource, so the only dropped-field guard can't catch those two. Deleting SchemaSource from both directions of the conversion (storage_schema_proto.go#L24) still passes assert.Equal(t, report, round) — verified by doing it in a clone at this SHA; the package stayed green. That silently drops the attribution the proto comment calls mandatory, and Host (populated by both planners) has the same hole (storage_schema_proto_test.go#L16).

General suggestions

The storageSchema field comment states the inverse of the code. "It is nil unless the embedder supplies one with WithStorageSchemaService, in which case those RPCs are refused" attaches the refusal to supplying the service, while the guard refuses when the field is nil. "otherwise those RPCs are refused" reads correctly (server.go#L23).

The unsupported-endpoint early return logs nothing, against AGENTS.md's "Every continue, return, or early-exit in a conditional branch must have a log statement explaining why". Minor in practice — the refusal message is emitted only by this in-process branch, so its presence on the client already proves the RPC arrived — but a one-line log makes misconfigured-embedder triage local to the data plane (server.go#L62).

The one thing that could have broken, verified

StorageSchemaApply is the state-changing RPC, so a gRPC-level retry would mean a duplicated destructive convergence. Checked retryServiceConfig's method allowlist at this SHA: the method is not listed, so gRPC never auto-retries it — the only apply is the one the caller issued.

Verified correct

• All 9 api.StorageSchemaReport fields are carried in both directions; proto field numbers 1-9 match the generated tags.
• Converged is deliberately absent from the wire and recomputed locally by APIType() — no control plane trusts a remote convergence verdict.
• storageSchemaStatementsProto/FromProto normalize empty to nil symmetrically; round trips are stable.
• Both nil-message directions are covered by TestStorageSchemaReport_NilProtoIsNotConvergence.
• storageSchemaStatus maps ErrInvalidStorageSchemaRequest to InvalidArgument, everything else to Internal with a fixed message; both failures log server-side first.
• The DSN/password sanitization test proves no infrastructure text reaches the wire.
• NewServer's variadic ServerOption change is source-compatible — both existing call sites and server_test.go:251 compile unchanged.
• The proto request messages carry no database/deployment/environment target, so no caller-supplied field can point an instance's answer at another database.

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 bf2290a. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[aparajon]

🤖 All four addressed in 8f0ae24a, and the branch is now rebased onto 7cb9fccc.

The Unimplemented branch is untested. Covered, following TestGRPCClientPullSchemaKeepsInfrastructureUnimplemented as the precedent you pointed at. The test builds the server without WithStorageSchemaService so the nil-adapter guard actually runs, and asserts through both client wrappers — the thing an operator on an old data plane sees is the upgrade instruction, not a bare code.

The proto round-trip fixture omits Host and SchemaSource. Fixed: the fixture now carries all nine fields, so deleting either direction of any field's conversion fails the assertion. I reproduced your clone experiment first — dropping SchemaSource did keep the package green — which is what made this worth fixing rather than noting.

The storageSchema field comment states the inverse of the code. Reworded to "otherwise those RPCs are refused", which is what the guard does.

The unsupported-endpoint early return logs nothing. It logs now. Your read was right that the client already proves the RPC arrived, but the log is what separates "this embedder registered no adapter" from "this deployment predates the RPCs" without leaving the data plane.

---

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

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1393, 8f0ae24.

Verdict: 3 findings — 2 non-blocking (dead contract, unwired server), 1 suggestion.

Non-blocking

The whole of pkg/apitypes/storage_schema_requests.go is unreferenced, and the HTTP contract it documents is served by nothing. storage_schema_requests.go:11 advertises POST /api/storage/schema/plan and /apply, but pkg/api/service.go:793 calls apiRoutes "the service's complete route table" and registers neither path, so an OSS consumer of this consumer-facing package gets a 404. The "required" invariants are equally unenforced: nothing rejects SchemaFiles with an empty SchemaSource, or Deployment with an empty Environment — and pkg/tern/storage_schema.go:36 contradicts the latter outright.

RegisterGRPC gives no way to supply a storage-schema adapter, so the two new RPCs are permanently Unimplemented in every serve-based binary. pkg/serve/serve.go:674 calls tern.NewServer(client, s.logger).Register(gs) with no ServerOption, and tern.WithStorageSchemaService has zero non-test call sites; serve.Server exposes no field or setter an embedder could use either. The operator-facing symptom ("upgrade that data plane" advice after the operator already upgraded) is latent at this SHA, since no non-test caller of StorageSchemaPlan and no serving adapter exist yet — worth wiring before the caller lands.

General suggestions

All four new apitypes structs are dead and their doc comments point at the wrong paths. grep for StorageSchemaPlanRequest outside pkg/proto/ternv1 matches only its own declaration, and the PR's real gateway routes are post: "/v1/storage-schema/plan" / "/v1/storage-schema/apply" (tern.proto:237, :253). Either delete the file until a handler exists, or fix the comments to name the paths that are actually registered.

The one thing that could have broken, verified

Capability negotiation: an old data plane must tell a new client "I don't do storage schema" in a way the client can act on. pkg/tern/server.go:117 returns an immutable package-level *status.Error (safe from concurrent handlers), and pkg/tern/grpc_client.go:588 wraps it with fmt.Errorf("%w", …) — status.Code still reports Unimplemented because grpc-go's FromError unwraps via errors.As, so the wrapping is lossless and byte-for-byte the shape of the existing Logs wrapper.

Verified correct

• All 9 api.StorageSchemaReport fields and all 4 StorageSchemaStatement fields survive both conversion directions; the round-trip test sets every one.
• storage_schema_proto.go:39 — nil-in/nil-out is deliberate and documented; the statement helpers guard on len() before allocating, so no nil deref.
• Converged is absent from the proto and recomputed in APIType(), so no stale convergence verdict can cross the wire.
• server.go:48 — the variadic opts on NewServer is source-compatible; all 4 existing non-test call sites compile unchanged.
• server.go:103 — storageSchemaStatus returns a fixed "see data plane logs" string; the sanitization test proves no DSN/host reaches the wire, per AGENTS.md's untrusted-error-string rule.
• server.go:125 — refuseStorageSchema's append([]any{"rpc", rpc}, attrs...) yields well-formed key/value pairs on both call paths; the test pins rpc and caller.

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 8f0ae24. 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/1393, e69866f.

Verdict: 2 findings — 1 non-blocking (round-trip-only conversion test), 1 suggestion (stale RPC list in architecture.md).

Non-blocking

The only conversion test is a Go→proto→Go round trip, so a symmetric field swap passes undetected. pkg/api/storage_schema_proto_test.go:44 guards the mapping with a single assert.Equal(t, report, round), which is invariant under any symmetric permutation of same-typed fields (Outstanding↔Destructive, Host↔SchemaSource). Applying that exact swap in both directions still passes the suite, while the wire form — grpc-gateway JSON at /v1/storage-schema/plan, or a peer binary without the same swap — would list a refused DROP TABLE under outstanding, which an operator reads as a statement that runs automatically. Asserting one direction against an explicit *ternv1.StorageSchemaReport literal would catch it.

General suggestions

architecture.md's exhaustive Tern RPC list is not updated for the two new RPCs. docs/architecture.md:400 still names exactly the 13 pre-PR RPCs while tern.proto now defines 15, the PR having added gateway-routed StorageSchemaPlan and StorageSchemaApply. A reader using that line to learn the Tern surface concludes storage-schema convergence is unreachable over gRPC — the one RPC that mutates the serving instance's own bookkeeping database. No doc/proto sync test guards this, and the file mentions storage schema nowhere.

The one thing that could have broken, verified

Adding a variadic ServerOption parameter to NewServer could have silently changed the shipped binary's behaviour. It does not: both existing call sites (pkg/serve/serve.go:674, integration/grpc_server_test.go:21) compile unchanged and neither registers a storage-schema adapter, so both new RPCs answer Unimplemented in production — consistent with green CI.

Verified correct

• pkg/api/storage_schema_proto.go maps every field of StorageSchemaReport and StorageSchemaStatement; the fixture sets all nine.
• Converged is deliberately off the wire — the control plane recomputes it, so no stale bool can contradict the statement lists.
• Nil handling is symmetric and non-inventing: both entry points return nil for nil, pinned by TestStorageSchemaReport_NilProtoIsNotConvergence.
• GRPCClient.StorageSchemaPlan/Apply follow the existing Logs() precedent; status.Code still reports Unimplemented through the fmt.Errorf wrap, proven against a real in-process server.
• The server refuses both RPCs with a package-level *status.Status-backed error, safely shared across concurrent handlers.
• refuseStorageSchema's append([]any{"rpc", rpc}, attrs...) allocates a fresh slice per call, and req.GetCaller() is nil-safe.
• Generated code is a genuine regeneration: NumMessages 55→62 matches the 6 new messages plus the map entry, with depIdxs shifted consistently.

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 e69866f. 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/1393, 3d81eff.

Verdict: 2 findings — 2 non-blocking (unwired server adapter, untested success path).

Non-blocking

The new RPCs are dead on every shipped binary, and the error message misdirects the operator. The only non-test construction of the tern gRPC server is tern.NewServer(client, s.logger) — no WithStorageSchemaService, and no serving-side adapter type exists — so s.storageSchema is always nil and server.go:62 returns errStorageSchemaUnsupported. grpc_client.go:591 rewrites that to "upgrade that data plane", which an operator already at head cannot act on. Reachable today only via a direct gRPC call (no in-repo caller yet), so it can land, but wire the adapter — or soften the remedy text — before anyone depends on it.

The success path of both handlers is untested. storageSchemaErrorService returns nil, s.err for both methods and the only other adapter any test installs is nil, so nothing asserts the adapter's response reaches the caller. Mutating both return resp, nil in server.go:73 to return nil, nil leaves ./pkg/tern, ./pkg/api and ./pkg/apitypes green. A fake adapter returning a populated report, exercised over a real bufconn round trip, would also cover the protobuf marshalling that the in-process conversion test skips.

The one thing that could have broken, verified

NewServer gaining a variadic opts ...ServerOption parameter could have broken every existing caller. It does not: all 20+ existing call sites compile unchanged (variadic params are optional), and Build/Lint/Unit CI is green at this head.

Verified correct

• pkg/api/storage_schema_proto.go maps all nine exported fields of api.StorageSchemaReport in both directions.
• Converged is a derived method, so correctly has no wire field.
• storageSchemaStatementsFromProto uses generated getters, so a nil repeated element cannot panic.
• Nil handling is symmetric: StorageSchemaReportProto(nil)/FromProto(nil) both return nil; empty slices convert to nil.
• Server.StorageSchemaApply's nil-adapter path calls req.GetCaller() on a possibly-nil request — safe, generated getters nil-check the receiver.
• refuseStorageSchema's append([]any{"rpc", rpc}, attrs...) builds a fresh slice per call, so the two refusal logs cannot alias.
• The client wrappers mirror the existing Logs Unimplemented wrapper exactly; %w preserves status.Code through the wrap, asserted by unit tests.
• Generated gateway/grpc/pb.go hunks match tern.proto: field numbers 1-9 line up and the new patterns use the 3-segment /v1/storage-schema/{plan,apply} form.

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 3d81eff. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[morgo]

🤖 Automated review on Morgan's behalf, at 3184444.

Reviewing the delta since the approval at 3d81eff4: purely additive — one docs/architecture.md paragraph and test coverage. No production Go changed; the rest of the tree drift is main moving underneath.

The new TestServerStorageSchemaAnswersWhatTheAdapterReported is the right test to have added, and it is not tautological. It drives both RPCs over a real connection and then asserts the two statement lists arrive distinct: Outstanding carrying the apply_operations alter, Destructive carrying the DROP with its reason. Then on the apply it checks the two halves separately — Remaining.Outstanding empty because the column converged, Remaining.Destructive still length 1 because the refusal stands.

That is the shape of assertion that actually catches the failure the comment names. A proto field mis-mapping that swapped the two lists, or dropped one, would leave a report that still looks well-formed — and would tell an operator either that a DROP runs automatically or that nothing is outstanding. Asserting the lists are non-empty would not catch it; asserting which statement is in which list does.

The docs paragraph is accurate to the code: an embedder that registered no storage-schema adapter does answer both RPCs Unimplemented (pkg/tern/server.go, refuseStorageSchema → errStorageSchemaUnsupported), and the stated reason — the alternative is silently reading some other instance's storage — is the actual justification for refusing rather than falling back.

CI 41/41 SUCCESS.

[aparajon]

🤖 Both rounds, e69866f5 and 3d81eff4. Four findings; three are fixed in 31844440, the fourth is this PR's shape rather than a defect.

The round trip is blind to a symmetric swap. Right, and I reproduced it: exchanging Outstanding↔Destructive in both conversions leaves assert.Equal(t, report, round) green, and so does Host↔SchemaSource. The report is now asserted one direction at a time against named wire fields — including that a refused statement arrives with its reason and an outstanding one has none — with the round trip kept for the reverse. Both swaps now fail.

The success path of both handlers was untested. Confirmed by mutation: return resp, nil → return nil, nil in both handlers left ./pkg/tern, ./pkg/api and ./pkg/apitypes green. A static adapter now answers a populated report over a real in-process connection (newRetryTestClient, the same path the Unimplemented test uses), asserting both plan fields and both halves of a convergence — what ran, and the refused DROP still outstanding. That mutation now dies.

architecture.md named 13 of 15 RPCs. Fixed, with a sentence on why these two are different in kind: they address the serving instance's own bookkeeping database rather than one it manages, and a data plane with no adapter registered answers Unimplemented rather than reading some other instance's storage.

The RPCs being dead on a shipped binary is the stack, not a defect. The serving adapter and its WithStorageSchemaService wiring are #1404, the next PR up, and nothing in-repo calls either RPC until then — reachable only by a direct gRPC call, as you noted. On the remedy text: "upgrade that data plane" is written for the case it will actually be read in, a control plane at head talking to a data plane from before these RPCs existed. Softening it now would leave the wrong instruction for the lifetime of the feature to spare a window in which no caller exists.

---

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