feat(canopy): build reporting schemas from migrated restores

[dannash100]

A reporting schema follows from a Tamanu version's schema and a group's configuration together, so it can only be built against a database of that group at that version. A migrated restore is the only place one exists.

• New reporting-schema intent: restore, migrate to the version canopy names, build before switchover, register, discard.
• The build runs from the image builder_image names. pgro hands it a database, version and group and takes back SQL over a callback, since a Job's termination message is 4 KiB.
• Registration goes through the transport, not a generated method: the generator does path params and JSON bodies only.

🦸 Review Hero

• Run Review Hero

[dannash100]

🤖 Known gap, left until canopy#553 is deployed: the build outcome isn't reported back, so no pair settles, the worklist re-dispatches every pass, and a failed build raises no check.

VerificationArgs.reporting_schema only appears in pgro's generated types once live canopy serves it, since bestool-canopy builds them from canopy's OpenAPI at compile time. Doing it sooner means dropping the typed report for a hand-built body.

Order: deploy canopy#553, rebuild, then .maybe_reporting_schema(..) on the existing builder.

[review-hero]

🦸 Review Hero Summary
9 agents reviewed this PR | 4 critical | 9 suggestions | 2 nitpicks | Filtering: consensus 3 voters, 4 below threshold

Below consensus threshold (4 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
src/controllers/replica.rs:1830	Bugs & Correctness	suggestion	ctx.schema_build_results.take(...) removes the SQL from the in-memory store before the outcome is persisted. If register or record_schema_build returns an error (a transient API-server failur...
src/controllers/replica.rs:1843	Design & Architecture	suggestion	The registration decision lives inline in reconcile_schema_build as a nested match over a tuple and an Option<Uuid> parse, while schema_build.rs — the module that exists precisely for this ...
src/controllers/replica.rs:1899	Design & Architecture	nitpick	BuildToDo::NoImage is unreachable: the only caller already gates on replica.spec.builder_image.is_some() at line 307 before invoking reconcile_schema_build, so the variant exists only to be a...
src/controllers/replica.rs:1955	Bugs & Correctness	suggestion	schema_build_result is written to the restore status but nothing reads it: the canopy verification report (verification.rs:440 builds VerificationArgs from migration_result only) carries no b...

Nitpicks

File	Line	Agent	Comment
src/controllers/replica.rs	1797	Performance	The Job is fetched up to three times per reconcile pass: schema_build::build_outcome does a get_opt, the Running arm does a second get_opt to disambiguate 'missing' from 'still going', and ensure_build_job does a third before creating. While a build runs, that is two API GETs every 30s ...
src/controllers/replica.rs	1866	Performance	Bytes::from(sql.to_owned()) clones the entire schema purely because sql is borrowed for the later completed_build_result(sql.as_deref(), ...) call, which only needs is_some() and len(). Capture let schema_bytes = sql.as_ref().map(|s| s.len() as i64) up front, then move the String in...

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

-------

`src/controllers/replica/schema_build.rs:138`: The build Job is named per-replica (`{replica}-schema-build`) but is never deleted, unlike the migration Job which `reconcile_schema_migration` deletes on both Succeeded and Failed (replica.rs:2113/2134). On the replica's *next* restore cycle, `build_to_do` returns `Build` (the new restore has no `schemaBuildResult`), `build_outcome` reads the *stale* completed Job from the previous cycle, and `ensure_build_job` sees a Job of that name already exists so it never creates a new one. The result: no build runs, `schema_build_results.take` returns `None` (already taken last cycle), and the restore is permanently settled with `built: false, error: "the build produced no schema"`. Delete the Job once its outcome has been recorded (as the migration path does), or name it per-restore.

-------

`src/controllers/replica.rs:307`: The build gate holds switchover forever if the build Job never terminates. `reconcile_schema_build` returns `Ok(false)` → requeue 30s for as long as `BuildOutcome::Running`, and there is no deadline anywhere: no `active_deadline_seconds` on the JobSpec and no equivalent of `timeout_schema_migration` (replica.rs:1746). An unpullable `builder_image`, an unschedulable pod, or a hung dbt run leaves the replica stuck before switchover indefinitely — which contradicts the stated invariant that a failed build must not fail/hold the restore. Add `active_deadline_seconds` to the Job and/or a wall-clock cap in the controller that records a failed build and proceeds.

-------

`src/bin/operator.rs:701`: The callback exists because the schema is too big for the 4 KiB termination message, but the `String` body extractor is subject to axum's default 2 MiB body limit and `build_router` applies no `DefaultBodyLimit` override. A reporting schema larger than 2 MiB is rejected with 413; the builder's POST fails, the Job may still exit 0, and the restore is settled as `built: false, "the build produced no schema"` with no retry. Apply `DefaultBodyLimit::max(...)` (or `disable()`) to this route with a limit sized for real dbt output.

-------

`src/controllers/replica.rs:1923`: `build_to_do` reads `replica.spec.builder_image` while it reads the target from `restore.spec.migrate_to`, so the image is taken live from the parent even though `resources.rs:474` deliberately snapshots `builder_image` onto the restore (with the comment that a snapshotted field 'must not change under it if canopy's plan moves mid-restore'), and `tests/schema_build.rs` asserts that snapshot. `restore.spec.builder_image` is written and read nowhere. Editing or clearing the replica's image mid-restore silently changes or cancels this restore's build. Read the image from `restore.spec.builder_image` (and gate on it at replica.rs:307 too).

-------

`src/controllers/replica.rs:1818`: A missing canopy group label yields `unwrap_or_default()`, so the Job is created with `TAMANU_DEPLOYMENT=""` and runs a full build against the wrong (or no) deployment configuration; only afterwards does the Succeeded branch discover the group is unparseable and record 'the replica names no group to register the schema for'. The group is required both for the build and for registration — resolve and parse it to a `Uuid` before creating the Job and record the failed result immediately rather than spending a build that can never be registered.

-------

`src/controllers/replica.rs:1840`: In the `Succeeded` arm, the `(sql, ctx.canopy)` match falls into `_ => None` when a schema came back but no canopy client is configured, so `completed_build_result` records `built: true` with no error — a build reported as settled-and-published when nothing was published anywhere. That conflates "no canopy configured" with "registration succeeded", which is exactly the state the `a_schema_canopy_did_not_take_is_not_built` test says must not be recorded as built. Split the arms: `(Some(_), None)` should record a distinct reason (e.g. "no canopy client to register the schema with") rather than success.

-------

`src/controllers/replica/schema_build.rs:195`: `build_outcome` re-derives Job state from `status.succeeded`/`status.failed` by hand, duplicating `controllers::jobs::classify_job` — which `replica.rs` already imports and uses for the migration Job. The hand-rolled version also drops the `Failed` *condition* check, so a Job killed by `activeDeadlineSeconds` or pod-failure policy (which sets the condition without necessarily bumping `failed`) reads as `Running` forever and the switchover gate never releases. Reuse `classify_job` and keep only the elapsed-seconds computation here.

-------

`src/controllers/replica.rs:1774`: `reconcile_schema_build` carries the domain logic that the new `schema_build` module exists to hold: secret reading, database discovery, group-label extraction, the three-deep nested match that decides registration, and the outcome-to-`SchemaBuildResult` mapping all sit in `replica.rs`, while the module next door holds only Job construction. The `Succeeded` arm in particular is a match on a tuple containing a match containing an await — hard to follow and untestable without a cluster. Move the "register and classify the result" step into `schema_build` as a single function taking `(sql, canopy, group, version, run_id)` and returning a `SchemaBuildResult`; `reconcile_schema_build` then reads as gate → create → record.

-------

`src/controllers/replica/schema_build.rs:106`: The build Job sets neither `active_deadline_seconds` nor `ttl_seconds_after_finished`, unlike every other Job this operator creates (`schema_migration.rs:177` sets ttl 300, `restore/builders.rs:133-134` sets deadline 120 / ttl 600, `replica/resources.rs:343-344` sets 300/120). Two consequences: (1) a dbt build that hangs (deadlocked on a lock, stuck network call) runs forever, and because `reconcile_schema_build` returns `Ok(false)` → 30s requeue on every pass, the switchover is blocked indefinitely and the whole restore — PVC, Postgres Deployment, and for an `ephemeral: true` intent a replica that should have been discarded — is held for the lifetime of the hung pod; (2) with `restart_policy: Never` and no TTL, the completed pod and Job object are never garbage-collected, and since the Job name is per-replica (`{replica}-schema-build`) the stale object also makes `ensure_build_job` a no-op for the next restore's build. Set `active_deadline_seconds` to a realistic ceiling for a dbt build and `ttl_seconds_after_finished` in line with the migration Job.

-------

`src/controllers/replica/schema_build.rs:110`: The build container declares no `resources`, so the pod is BestEffort. Every other job container in this repo pins requests/limits (`schema_migration.rs:224`, `restore/builders.rs:166,323,899`). A dbt build against a freshly migrated Tamanu database is CPU- and memory-hungry; unbounded it can starve the co-located Postgres Deployment it is querying (same node, since placement is shared) and it is the first thing the kubelet evicts under node pressure — an eviction here reads as 'the build produced no schema'. Note that `IntentConfig::resources_floor` for `reporting-schema` (`intent.rs`) sizes the *restore's* Postgres, not this builder, so it does not cover this pod. Add explicit requests and a memory limit.

-------

`src/controllers/replica.rs:1846`: `ctx.schema_build_results.take(...)` is only reached on the `Succeeded` arm. `CallbackStore` (`controllers/jobs.rs:42-55`) is a plain `HashMap<String, String>` with no eviction, TTL, or size cap — entries leave only via `take`. A build that POSTs its schema and then exits non-zero (the `Failed` arm), or whose restore is deleted / already `Settled` before this arm runs, leaves a multi-megabyte SQL string resident in the operator process forever. Because the key is `{namespace}/{replica}` rather than per-restore, that stale entry is also what the *next* build's `take` would return. Take (and drop) the entry on the `Failed` path too, and consider a bounded/TTL'd store given the payload size.

-------

`src/controllers/replica.rs:1797`: The Job is fetched up to three times per reconcile pass: `schema_build::build_outcome` does a `get_opt`, the `Running` arm does a second `get_opt` to disambiguate 'missing' from 'still going', and `ensure_build_job` does a third before creating. While a build runs, that is two API GETs every 30s per building replica. Have `build_outcome` return a `NotCreated` variant (or the fetched `Option<Job>`) so the caller can branch without re-fetching, and let `ensure_build_job` create unconditionally, treating `AlreadyExists` as success.

-------

`src/controllers/replica.rs:1866`: `Bytes::from(sql.to_owned())` clones the entire schema purely because `sql` is borrowed for the later `completed_build_result(sql.as_deref(), ...)` call, which only needs `is_some()` and `len()`. Capture `let schema_bytes = sql.as_ref().map(|s| s.len() as i64)` up front, then move the `String` into `Bytes::from` — avoids a second full-size allocation of a payload that is by design too large for a termination message.

-------

`src/controllers/replica.rs:1845`: `schema_build_results.take` removes the SQL from the store before `record_schema_build` patches the status. If the patch fails (conflict, transient API error) the `?` aborts the reconcile and the SQL is gone: the next pass sees the Job still `Succeeded`, `take` returns `None`, and a build that actually succeeded and was registered with canopy is permanently recorded as `built: false, "the build produced no schema"`. Record the status first, or only remove the entry from the store after the patch succeeds.

-------

`src/controllers/replica/resources.rs:474`: `builder_image` is snapshotted onto `PostgresPhysicalRestoreSpec` and documented in the README/CRD as "copied from the parent replica" (the integration test even asserts it), but nothing ever reads it: both the gate in `reconcile` and `build_to_do` read `replica.spec.builder_image`. The snapshot's whole purpose — that a mid-flight edit to the replica cannot change what this restore builds with — is not achieved, and the field is dead state that will drift from the code. Read `restore.spec.builder_image` in `build_to_do` (and gate on it in `reconcile`), or drop the field from the restore spec.

[review-hero]

🦸 Review Hero Summary
12 agents reviewed this PR | 4 critical | 8 suggestions | 2 nitpicks | Filtering: consensus 3 voters, 15 below threshold

Below consensus threshold (15 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
src/bin/operator.rs:25	Design & Architecture	nitpick	MAX_SCHEMA_BODY_BYTES is declared in the middle of the import block, splitting use tower_http::... from use tracing::.... Move it below the imports with the other module items so the use gr...
src/canopy.rs:340	Design & Architecture	nitpick	Three tests over a single two-line format string is more than the code carries. the_registration_names_an_exact_version in particular asserts !uri.contains(".x") about a version literal the tes...
src/controllers/replica.rs:1773	Bugs & Correctness	suggestion	A failed build is recorded only on the restore's status, and nothing ever tells canopy about it. migration_for (verification.rs:433) is the only place a post-restore outcome reaches the verificat...
src/controllers/replica.rs:1782	Design & Architecture	suggestion	Two "cannot build" conditions are handled inconsistently: BuildToDo::NoTarget warns and returns without recording anything, so the restore switches over with no schemaBuildResult and canopy is ...
src/controllers/replica.rs:1788	Design & Architecture	nitpick	BuildToDo::NoImage is unreachable in production: the only caller guards with switching.spec.builder_image.is_some() (replica.rs:~310) before invoking reconcile_schema_build, and build_to_do...
src/controllers/replica.rs:1826	Bugs & Correctness	nitpick	On the NO_GROUP early return the Job has not been created yet, but record_schema_build still writes schemaBuildJob: job_name onto the restore status. The field is documented as "Name of the r...
src/controllers/replica.rs:1851	Design & Architecture	suggestion	In the Succeeded arm the irreversible outward side effect (registering the schema with canopy) runs before the fallible status patch that records it. record_schema_build propagates with ?, so...
src/controllers/replica.rs:1852	Bugs & Correctness	suggestion	The posted schema lives only in the operator's in-memory CallbackStore, so an operator restart (rollout, OOM, node drain) between the build Job posting its SQL and this reconcile observing `Succe...
src/controllers/replica.rs:1868	Bugs & Correctness	suggestion	A transient canopy failure permanently loses a completed build. register swallows the error and returns false, the result is recorded (built: false), which makes build_to_do return Settled ...
src/controllers/replica.rs:1933	Bugs & Correctness	critical	build_to_do gates only on spec.migrate_to being present, never on whether the migration actually succeeded. A failed migration does not fail the restore: restore/migration.rs:257 writes `phas...
src/controllers/replica/schema_build.rs:33	Performance	suggestion	BUILD_TTL_SECONDS (300s) can delete the finished Job before the operator records its outcome — the reconcile only observes it on a 30s requeue, so any operator downtime or requeue backlog longer ...
src/controllers/replica/schema_build.rs:62	Design & Architecture	suggestion	SchemaBuildArgs.group is &str, and the caller stringifies a Uuid into it (group: &group.to_string()), so the type no longer says what the builder image is actually handed. The unit test the...
src/controllers/replica/schema_build.rs:122	Security	suggestion	The build pod runs an image named by an operator-set canopy parameter — the least trusted image pgro launches — yet it is the only job here with no PodSecurityContext: it runs as root by default ...
src/controllers/replica/schema_build.rs:155	Performance	suggestion	The build container's limits are hardcoded at 2 CPU / 2Gi regardless of database size, while every other workload in this operator derives sizing from the snapshot (see resources_floor/`resources...
tests/schema_build.rs:108	Design & Architecture	suggestion	a_builder_image_drives_a_schema_build_job never observes a build Job: the target version and builder image are both deliberately unpullable, so the test asserts the Job is absent and then finis...

Nitpicks

File	Line	Agent	Comment
src/controllers/replica.rs	1864	Design & Architecture	run_id_from_status was widened from private to pub(crate) and is called here by fully-qualified path from the replica controller. That makes a helper of the canopy verification-reporting module part of the crate surface for an unrelated caller, and the inline path signals the layering is of...
src/controllers/replica.rs	1799	Performance	build_outcome already did a get_opt on this Job, and the Running branch immediately repeats it (and ensure_build_job does a third on the creation pass). With a 30s requeue against a 30-minute build deadline that is ~60 redundant API GETs per build, per replica. Have build_outcome return...

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

-------

`src/controllers/replica.rs:1913`: `build_group` reads only the replica CR's `canopy.../group` label, but the verification path deliberately does not trust that: `canopy_ids` + `fill_missing_labels`/`namespace_labels` (verification.rs:207-286) fall back to the namespace's copy of the same label set precisely because a CR can be missing it. Here a replica without the label silently records NO_GROUP and skips the build entirely — the restore is discarded and nothing is ever built for that group/version pair. Reuse the same namespace fallback (or read `spec.canopy_source.group`, which the syncer always sets) instead of the bare label lookup.

-------

`src/controllers/replica.rs:1805`: The pre-creation path of the build gate has no failure exit: `secrets.get(...).await?` and `discover_restore_database(...).await?` propagate errors out of `reconcile_schema_build`, and the gate only ever returns `Ok(true)` once a Job has been created and observed settled. If the reader secret is missing or the restore's Postgres is unreachable at this moment, the reconcile errors, requeues, and re-enters the same branch forever — the restore never leaves `Switching`, and for an ephemeral `reporting-schema` replica it is never discarded either. That directly contradicts the stated invariant that a failed build does not fail the restore (the Job's `active_deadline_seconds` only bounds a Job that got created). Wrap these two calls so a failure records a `SchemaBuildResult { built: false, error: ... }` and returns `Ok(true)`, as the `NO_GROUP` branch already does.

-------

`src/controllers/replica/schema_build.rs:186`: `jobs.delete(job_name, &Default::default())` deletes the Job with no propagation policy, which for batch/v1 orphans its pods — the completed build pod is left behind holding its 2Gi memory limit's worth of node accounting and never gets collected, since its owner is gone. The repo already knows this: replica.rs:1679 uses `kube::api::DeleteParams::background()` with the comment "background propagation so its pods are GC'd too". Use `DeleteParams::background()` here too.

-------

`src/controllers/replica.rs:1820`: In the create path the group check runs last, after fetching the credentials secret and after `discover_restore_database`, which opens a connection to the restore. When the replica carries no parseable group label, all of that work is done and thrown away to record `NO_GROUP`. `build_group(replica)` is a pure label read — hoist it above the secret/discovery block (or fold it into `build_to_do`, which is already the "has this reconcile anything to do" predicate) so the cheap precondition gates the expensive work. That also removes the second `build_group` call in the `Succeeded` arm, which currently re-derives the same value and duplicates the `NO_GROUP` handling.

-------

`src/controllers/replica/schema_build.rs:193`: `BuildOutcome` has no variant for "no Job yet": `build_outcome` returns `Running` both when the Job is absent and when it is active. The caller then has to re-`get_opt` inside the `Running` arm to tell the two apart (replica.rs:1796), and `ensure_build_job` does a third `get_opt` before creating. Three API round-trips and a control flow where the comment `// Not created yet on the first pass through.` sits above a check that means the opposite. The existing migration gate (`reconcile_schema_migration`, replica.rs:2059) does one `get_opt` and matches `Some(job)`/else-create. Either add a `NotStarted` variant or return `Option<BuildStatus>`, and drop the duplicate existence checks.

-------

`src/controllers/replica/schema_build.rs:257`: `register` collapses the canopy error into a `bool`, so the only place the real failure survives is a `warn!` line. The caller then substitutes the constant "canopy did not take the schema in" into `SchemaBuildResult::error`, a field whose own doc says "What went wrong, where it did". An operator reading the restore status learns nothing about whether it was a 403, a 413 over `MAX_SCHEMA_BODY_BYTES`, or a transport error. Return `Result<()>` (or `Option<String>`) and record the formatted error on the status; the log line can stay.

-------

`src/controllers/replica.rs:1864`: `run_id_from_status` was widened from private to `pub(crate)` and is called here by fully-qualified path from the replica controller. That makes a helper of the canopy *verification-reporting* module part of the crate surface for an unrelated caller, and the inline path signals the layering is off. It reads a field off `PostgresPhysicalRestoreStatus`, so it belongs as an accessor on the restore type (or in the shared canopy module) rather than being re-exported from the reporting path.

-------

`src/controllers/replica.rs:1852`: The posted schema (up to `MAX_SCHEMA_BODY_BYTES` = 32 MiB) is copied twice on the success path: `CallbackStore::get` clones the stored `String`, then `Bytes::from(sql.to_owned())` clones it again for registration. Peak resident bytes for one build are ~3× the schema size, and the copies happen inside the reconcile loop. Either hand the store `Arc<str>`/`Bytes` so reads are refcount bumps, or `take()` the value and re-`store()` it if the status patch fails (which is what the current `get`-then-`take` ordering is protecting against) so at most one copy exists.

-------

`src/context.rs:52`: `schema_build_results` is an unbounded in-memory map whose entries are up to 32 MiB each, and nothing evicts an entry unless the build gate reaches its `Succeeded`/`Failed` branch. A payload posted after the pair is `Settled`, after the restore was deleted or switched over, or for a replica whose `switching_restore` is `None`, stays in the map for the operator's lifetime — one leaked entry per replica, at 32 MiB apiece, versus the kilobyte-sized payloads the other `CallbackStore`s hold. Consider evicting on replica/restore teardown or storing entries with an insertion timestamp and dropping stale ones during reconcile.

-------

`src/controllers/replica.rs:1799`: `build_outcome` already did a `get_opt` on this Job, and the `Running` branch immediately repeats it (and `ensure_build_job` does a third on the creation pass). With a 30s requeue against a 30-minute build deadline that is ~60 redundant API GETs per build, per replica. Have `build_outcome` return whether the Job exists (e.g. `BuildOutcome::Absent`) and drop the second lookup.

-------

`src/bin/operator.rs:707`: `post_schema_build_results` is unauthenticated and unvalidated: the handler accepts any body for any `{namespace}/{replica}` path and stores it verbatim, and `reconcile_schema_build` then ships whatever is under that key to canopy via `register_reporting_schema` as a group-scoped artifact of the exact version — SQL that downstream consumers execute. Any pod that can reach the operator's HTTP port (including the third-party `builderImage`, which is handed this URL and is by design outside pgro's trust) can race or simply overwrite the real build's output and get arbitrary SQL published under another deployment's group. The other callbacks share the no-auth shape but only ever feed status text; this one is a publish path, so it needs its own proof of origin — e.g. mint a per-build nonce, pass it to the Job as the callback path segment or a bearer header, and reject a POST whose token doesn't match the currently-running build for that replica.

-------

`src/bin/operator.rs:653`: The 32 MiB body limit combined with `CallbackStore`'s unbounded `HashMap` makes this endpoint a cheap memory-exhaustion vector: the key is caller-supplied (`{namespace}/{replica}` is never checked against an existing replica or a build that is actually running), and an entry is only ever removed by a reconcile that finds a matching build Job. A few dozen POSTs to invented names pin gigabytes in the operator process for its lifetime, with no eviction path. Reject the callback when there is no in-flight build Job for that namespace/replica (or at minimum when the replica does not exist), and cap the number of retained entries.

-------

`src/controllers/replica/schema_build.rs:129`: The restore's Postgres password is embedded as a literal env value in the build Job spec, so the plaintext credential is stored in the Job/Pod object (visible to anyone with `get jobs`/`get pods` in the namespace, to `kubectl describe`, to audit logs and to etcd) rather than only in the Secret. Every other Job in this repo takes credentials by reference — see `src/controllers/restore/migration.rs:104-105` and `src/controllers/replica/schema_migration.rs:203-213`, which use `env_from_secret_name(..., reader_secret_name, "password")`. Do the same here: pass `replica.creds_secret_name()` into `SchemaBuildArgs` and build `TAMANU_DL_DB_USER`/`TAMANU_DL_DB_PASSWORD` with `env_from_secret_name`, dropping the `secrets.get(...)`/`read_secret_field` fetch of the password in `reconcile_schema_build` (the username is still needed for `discover_restore_database`, the password is not needed in operator memory to template the Job).

-------

`src/canopy.rs:266`: `version` is interpolated raw into the registration URI with no validation or percent-encoding. `MigrationTarget.version` (src/types/restore.rs:149) is a free-form `String` taken from the canopy worklist entry or the CRD — nothing anywhere validates it as semver. A value containing `/`, `?`, `#` or `&` rewrites the request target: e.g. `2.60.0?group=<other-group>&x=` yields `/artifacts/2.60.0?group=OTHER&x=/reporting-schema/any?group=REAL`, where the first `?` starts the query and the group scoping is attacker-chosen, and `../..` walks to a different canopy endpoint entirely. Since the tests in this file assert the exact authorised path is the only thing protecting group scoping, reject or encode the version before building the URI — validate it against a strict `[A-Za-z0-9.+-]` / semver pattern and return `Error::Canopy` otherwise, or percent-encode the path segment.

[review-hero]

🦸 Review Hero Summary
12 agents reviewed this PR | 1 critical | 11 suggestions | 0 nitpicks | Filtering: consensus 3 voters, 16 below threshold, 2 suppressed

Below consensus threshold (16 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
src/bin/operator.rs:22	Design & Architecture	nitpick	MAX_SCHEMA_BODY_BYTES is declared in the middle of the use block, between tower_http and tracing, splitting the imports in two. AGENTS.md asks for imports merged and grouped, with use bef...
src/bin/operator.rs:715	Design & Architecture	suggestion	The callback handler authenticates by doing a live Kubernetes Job GET and reading an annotation off it, which puts the HTTP layer in bin/operator.rs directly on the k8s API for every POST and is ...
src/bin/operator.rs:719	Security	nitpick	expected.as_deref() != Some(token.as_str()) is a short-circuiting byte comparison on a secret. The exposure is small (the token is a v4 UUID and each probe costs a k8s API round-trip, which domin...
src/context.rs:164	Design & Architecture	nitpick	Two doc summary lines stacked on the same item ("Where a reporting-schema build POSTs the SQL it produced." then "Build the callback URL a reporting-schema build Job POSTs its SQL to.") — looks lik...
src/controllers/replica.rs:306	Design & Architecture	suggestion	The gate here already tests switching.spec.builder_image.is_some() before calling reconcile_schema_build, and build_to_do tests it again and returns BuildToDo::NoImage, which the caller map...
src/controllers/replica.rs:1856	Bugs & Correctness	suggestion	The Succeeded and Failed branches both call delete_build_job as soon as the outcome is patched — typically within one 30s requeue of the Job finishing — which contradicts BUILD_TTL_SECONDS ("ho...
src/controllers/replica.rs:1868	Bugs & Correctness	suggestion	A registration failure permanently discards the schema. The SQL is taken out of the callback store, register fails (a SOCKS5/Tailscale hiccup, a canopy 502 — all transient), `completed_build_re...
src/controllers/replica.rs:1885	Bugs & Correctness	suggestion	On a record_schema_build failure the SQL is put back into the store and Err is returned, but register() has already run and succeeded. The next reconcile finds the restore still unsettled (th...
src/controllers/replica.rs:1930	Security	nitpick	build_group falls back to the pgro.bes.au/group label whenever canopy_source.group is absent or unparseable. The registration is group-scoped and canopy authorises publication per group, so...
src/controllers/replica.rs:2024	Bugs & Correctness	critical	build_to_do only checks that migrate_to is set, never that the migration actually succeeded. A failed migration does not fail the restore: restore/migration.rs writes migrationResult (wit...
src/controllers/replica.rs:2082	Bugs & Correctness	nitpick	The NoGroup path calls settle_failed, which patches schemaBuildJob: job_name even though no Job was ever created — an operator following the status looks for a Job that does not exist (same f...
src/controllers/replica/schema_build.rs:45	Design & Architecture	suggestion	The build Job is named per replica ({replica}-schema-build) while the state it produces is per restore (status.schemaBuildJob / schemaBuildResult live on PostgresPhysicalRestore), and t...
src/controllers/replica/schema_build.rs:203	Security	suggestion	The callback token is a bearer credential for publishing group-scoped SQL that other servers execute, but it is embedded as a plaintext literal in SCHEMA_CALLBACK_URL on the Job (and hence the Po...
src/controllers/replica/schema_build.rs:224	Design & Architecture	suggestion	The build container's resources are hardcoded (100m/256Mi requests, 2 CPU/2Gi limits) while every other workload shape in this repo is parametrised through IntentConfig — and this PR even adds `r...
src/controllers/replica/schema_build.rs:238	Bugs & Correctness	nitpick	The doc comment links [ensure_build_job], but no such function exists — the creator is create_build_job. Broken intra-doc link; rustdoc will warn.
src/controllers/replica/schema_build.rs:240	Bugs & Correctness	critical	delete_build_job is best-effort — a failed delete is only warned about — but the Job name is per-replica and build_outcome keys purely off that name. If the delete ever fails (API error, or the...

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

-------

`src/controllers/replica.rs:1828`: Any error out of `discover_build_database` settles the pair as a permanent build failure on the first attempt. That call reads a Secret and then *opens a postgres connection* to the restore's service — which has just come out of the migration Job, so a pod restart or a momentary missing endpoint is an ordinary occurrence, not a build defect. Because `settle_failed` writes `schemaBuildResult`, `build_to_do` returns `Settled` on every later reconcile and the build is never attempted again for this snapshot. Avoiding the infinite-requeue trap the comment describes doesn't require settling on attempt one: requeue a bounded number of times (e.g. track attempts, or only settle once the restore has been in `Switching` past a deadline) before recording the failure.

-------

`src/controllers/replica.rs:1863`: The built schema only ever lives in `ctx.schema_build_results`, an in-memory `CallbackStore`. If the operator restarts (rollout, OOM, eviction) in the window between the build POSTing its SQL and the reconcile that observes the Job as `Succeeded`, the store comes back empty, `sql` is `None`, and `completed_build_result` records `built: false` / "the build produced no schema" — a false failure that is then permanently `Settled`, with no way to rebuild for that restore. Unlike the other callbacks (small migration/stat payloads), this one is the sole copy of an artifact that took up to 30 minutes to produce. Consider spilling the payload to a ConfigMap/Secret/PVC keyed to the Job, or registering with canopy directly from the callback handler so the durable copy is canopy's.

-------

`src/bin/operator.rs:718`: `build_token` swallows every kube API error into `None` (`.ok().flatten()?`), so a transient API-server hiccup or a momentary RBAC/connection failure while fetching the Job makes this handler answer 403 and drop the body. The build Job runs with `backoff_limit: 0` and `restart_policy: Never`, so there is no retry: the SQL is gone, and the reconcile then settles the pair as "the build produced no schema" — a false failure recorded against a build that actually succeeded (and, for a 30-minute dbt run, 30 minutes wasted). Distinguish the cases: propagate a lookup error as 503 (or 500) so the poster can retry, and reserve 403 for a Job that exists with a different/absent token.

-------

`src/controllers/canopy/verification.rs:433`: The build outcome never reaches canopy. `.workhorse/specs/canopy/intents.md` added in this PR states "pgro reports the build's outcome to canopy alongside the replica's own health, as the migration outcome is reported", but `SchemaBuildResult` is only written to the restore's status; nothing in `report_verification` reads it, and `migration_for` still early-returns for any intent that isn't `upgrade`, so a `reporting-schema` restore reports neither its migration nor its build. A failed build is therefore invisible to canopy except as a missing artifact — exactly the silent drift the module docs warn about. Either add the build block to `VerificationArgs` or drop the claim from the spec.

-------

`src/canopy.rs:293`: `is_path_safe_version` accepts `..` — every character is `'.'`, which the `matches!(c, '.' | '+' | '-')` arm allows — so a version of `..` yields `/artifacts/../reporting-schema/any?group=…`. Dot-segment normalisation in a proxy or in canopy's router resolves that to `/reporting-schema/any`, which is precisely the path rewrite this function exists to prevent (the `../../devices` test case only fails because of its slashes, not its dots). Reject any version that is all dots, or require it to start with an ASCII alphanumeric, and add `".."` to the rejection list in `a_version_that_would_rewrite_the_path_is_not_a_version`.

-------

`src/controllers/replica.rs:1767`: The whole build orchestration (~330 lines: `reconcile_schema_build`, `build_to_do`, `build_group`, `settle_failed`, `discover_build_database`, `completed_build_result`, `record_schema_build`, `NO_GROUP`) landed in `replica.rs`, while the purpose-built `replica/schema_build.rs` added by the same PR holds only Job construction and Job polling. That splits one concern across two files by *layer* rather than by *topic*, and grows an already 2000+ line controller. The redaction gate is the precedent to follow: `redaction::reconcile_redaction_step` keeps its state machine in its own module and `replica.rs` holds just the two-line gate. Move everything except the gate into `schema_build.rs`, which also lets `build_to_do`/`completed_build_result` stop being `pub(super)`-by-accident and drops the `pub` on the job-plumbing helpers.

-------

`src/controllers/replica/schema_build.rs:355`: `register` is a pass-through that adds nothing but a `warn!` and an `Error → String` conversion, which the only caller immediately re-wraps into another string (`format!("canopy did not take the schema in: {err}")`), so the error text gets built twice and logged in one place while being formatted in another. Drop the wrapper and call `canopy.register_reporting_schema(...)` from `reconcile_schema_build` directly, mapping the error where the message is actually composed.

-------

`src/controllers/replica.rs:1873`: `Bytes::from(sql.to_owned())` allocates a second full copy of the schema, which the surrounding comments say can be tens of megabytes — exactly the copy the `take`-not-`clone` comment above (line 1863) claims to avoid. Peak memory for the reconcile is therefore 2× the payload, and this runs inside the reconcile loop. `Bytes::from(String)` is zero-copy, so convert the owned `String` once (`let bytes = Bytes::from(sql)`), take `schema_bytes` from `bytes.len()`, and reconstruct the `String` for the put-back-on-patch-failure path only in the error branch (or store `Bytes` in the callback store to begin with).

-------

`src/context.rs:52`: `CallbackStore` (src/controllers/jobs.rs:42) has no eviction — entries leave only via `take`. The other stores hold small payloads, but `schema_build_results` holds up to 32 MiB per namespace/replica key, and several paths leave an entry with no future taker: if the replica is deleted while its build is in flight, or the build POSTs after the reconcile already took a payload and deleted the Job, the bytes stay resident for the operator's process lifetime, and `BuildToDo::Settled` returns early without taking. Consider dropping the entry when a replica is finalised/deleted, or giving the store a timestamped entry with a max-age sweep, so an abandoned build cannot pin tens of megabytes indefinitely.

-------

`src/bin/operator.rs:653`: Carrying the token as a path segment puts a bearer credential in every place a URL is recorded: the router is wrapped in `TraceLayer::new_for_http()`, whose `DefaultMakeSpan` records the full `uri`, so running the operator at debug level writes the live token to the operator's own logs, and any ingress/proxy/service-mesh access log in front of it does the same regardless of level. Move the token to a request header (e.g. `Authorization: Bearer` or `X-Pgro-Build-Token`) and keep `{namespace}/{replica}` in the path; the handler's rejection `warn!` already avoids logging it, which suggests the sensitivity was recognised but the transport undoes it.

-------

`src/controllers/replica/schema_build.rs:143`: This is the only Job in the repo that runs an image pgro does not choose — `builder_image` is free text off a canopy worklist param — and it is the one with no `security_context` and no `automount_service_account_token: Some(false)`. The sibling migration Job (schema_migration.rs:191) pins `run_as_non_root`/`run_as_user: 26` for an image the operator itself names. As written the build container runs as whatever UID its image declares (root by default) in the operator's namespace, with the `default` ServiceAccount token mounted at /var/run/secrets/kubernetes.io/serviceaccount and the restore's database credentials in its environment. Add a `PodSecurityContext` with `run_as_non_root: Some(true)` plus `automount_service_account_token: Some(false)` on the PodSpec — the build talks only to Postgres and the callback URL, so it needs no API access at all.

-------

`src/bin/operator.rs:716`: Axum runs extractors in declaration order and `body: String` is last, so the full body — up to the 32 MiB `DefaultBodyLimit` this route raises — is buffered into the operator's heap *before* `build_token` is consulted. Any unauthenticated caller that can reach the callback port can therefore make the operator allocate 32 MiB per concurrent request with no valid token, and the operator is the process holding all reconcile state. Validate the token first (e.g. resolve `expected` in a small middleware or `from_request_parts` guard on this route, rejecting with 403 before the body extractor runs), so the large-body path is only reachable by an authenticated build.

[review-hero]

🦸 Review Hero Summary
9 agents reviewed this PR | 1 critical | 8 suggestions | 0 nitpicks | Filtering: consensus 3 voters, 6 below threshold, 1 suppressed

Below consensus threshold (6 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
src/canopy.rs:280	Design & Architecture	suggestion	registration_uri assembles the target by string concatenation and then defends it with a hand-rolled is_path_safe_version allowlist. The allowlist has to encode knowledge of every character tha...
src/controllers/replica/schema_build.rs:48	Design & Architecture	suggestion	The build Job is named per-replica ({replica}-schema-build) while the unit of work it represents is per-restore: the result, the attempt counter and the migration it depends on all live on the re...
src/controllers/replica/schema_build.rs:237	Design & Architecture	suggestion	The module's stated premise is that pgro does not know how a schema is made and only hands the image a database — yet it hardcodes cpu 2 / memory 2Gi limits on that image. Every other property of t...
src/controllers/replica/schema_build.rs:391	Bugs & Correctness	suggestion	ttl_seconds_after_finished: 300 races the reconcile that reads the finished Job, and the NotStarted arm discards the delivered schema when it loses. The success path deletes the Job itself, so ...
src/controllers/replica/schema_build.rs:487	Performance	nitpick	String::from_utf8_lossy(&sql).into_owned() re-allocates and copies the whole schema (tens of MiB) just to put it back in the store after a failed status patch, and it holds both copies at once. T...
src/controllers/replica/schema_build.rs:598	Design & Architecture	suggestion	BUILD_ATTEMPTS is one counter (status.schemaBuildAttempts) shared by two unrelated causes: setup failures in retry_or_settle (secret read / database discovery) and lost-payload rebuilds in `e...

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

-------

`src/controllers/replica/schema_build.rs:495`: `BuildOutcome::Failed` discards a schema the build already delivered. The Job is only marked Failed after its container exits non-zero, which can happen *after* a successful POST to the callback (teardown, a non-zero exit from a wrapper script, the deadline firing during cleanup). This branch takes and drops `schema_build_results` unconditionally and records `built: false, error: "the build job failed"`, throwing away up to 30 minutes of work that is sitting in memory and whose delivery is recorded on the Job's `BUILD_RECEIPT_ANNOTATION`. The module goes to some length elsewhere (`empty_build`/`rebuild`) to avoid losing a delivered schema; this path is the asymmetry. Consider checking `build.receipted` / the store here and registering the schema anyway, or at least recording `schemaBytes` so an operator can tell a build that produced nothing from one that produced a schema and then fell over.

-------

`src/controllers/replica/schema_build.rs:503`: Nothing carries the build's outcome to canopy. `intents.md` in this PR states "pgro reports the build's outcome to canopy alongside the replica's own health, as the migration outcome is reported", but `verification::report` is unchanged: it only builds a `MigrationArgs`, and `migration_for` returns `None` for any intent that is not `upgrade`. So for a `reporting-schema` restore, neither the migration result nor `schemaBuildResult` reaches canopy — every one of the failure paths here (`NoGroup`, `Unmigrated`, "the build job failed", "the build produced no schema", "canopy did not take the schema in") is written to the CR status and then never read by anything. Canopy sees a healthy restore and simply no artifact, which is exactly the silent drift the `registration_uri` tests warn about. Either wire the result into the verification report or drop the claim from the spec.

-------

`src/controllers/replica/schema_build.rs:462`: A failed canopy registration is terminal and destroys the built schema. `sql` has already been `take`n out of `schema_build_results`, and on registration error the code writes `built: false` via `record_schema_build`, then falls through to `delete_build_job` and returns `Ok(true)`. The next reconcile sees `schema_build_result` present, so `build_to_do` returns `Settled` and nothing is ever retried — a single transient failure on the Tailscale/SOCKS hop to canopy (timeout, 502, proxy restart) permanently loses a build that took up to 30 minutes, and since canopy keys the entry to (group, version) a newer snapshot won't rebuild it. The setup path already has `BUILD_ATTEMPTS` for exactly this class of failure; registration should use it too: on error, re-`store` the SQL, `record_build_attempt`, leave the Job in place and return `Ok(false)` so the next pass re-registers, and only settle as `built: false` once the attempts are exhausted (or the error is a 4xx canopy refusal rather than a transport error).

-------

`src/bin/operator.rs:661`: Carrying the build's bearer token as a path segment forces a chain of compensating machinery: a `SCHEMA_BUILD_CALLBACK_PREFIX` constant that must stay in sync with the route and with `Context::schema_build_callback_url`, a custom `make_span_with` replacing the default TraceLayer span, a `loggable_target` redaction helper, and two tests to hold it in place. A header (`Authorization: Bearer`, read by `verify_build_token`) keeps the credential out of every URI-recording surface by construction — access logs, proxies, and any future span — and lets the route stay `{namespace}/{replica}` like every other callback. All three of those pieces plus the path-sync comment then disappear.

-------

`src/controllers/replica.rs:304`: The caller gates on `switching.spec.builder_image.is_some()` and then `build_to_do` re-tests the very same field, so `BuildToDo::NoImage` (schema_build.rs:759) is unreachable from the only production call site — it exists solely to satisfy a unit test. Pick one owner for the decision: either drop the `builder_image` clause here and let `reconcile_schema_build` return early on `NoImage`, or drop the `NoImage` variant. Two gates on one condition is where the next change diverges.

-------

`src/context.rs:55`: `CallbackStore` is a shared `HashMap<String, String>` behind a single global `std::sync::Mutex`, built for the small JSON payloads the other callbacks post. Reusing it for a body capped at 32 MiB changes what that component is: one lock now serialises multi-megabyte inserts against every other callback, and the payload's lifetime becomes the reconciler's problem — schema_build.rs has to hand-place `take()` in five separate branches (383, 399, 440, 511, plus a `store()` rollback at 484) purely so the memory is not leaked. That scattering is the design smell, not a bug list. Either give the build its own store with an explicit owner/expiry, or spool the body to a temp file / register it with canopy directly from the handler and keep only the receipt in memory.

-------

`src/controllers/replica/schema_build.rs:370`: `NoTarget` warns and returns `Ok(true)` without recording anything, while `NoGroup` and `Unmigrated` — equally "pgro cannot build this" cases — both call `settle_failed` and leave a graded record. A `reporting-schema` replica whose restore lost its target therefore switches over with no `schemaBuildResult` at all, so canopy sees a healthy restore and no build outcome, indistinguishable from a replica that was never meant to build. Settle it the same way the other two are.

-------

`src/context.rs:55`: `schema_build_results` is a `CallbackStore` (`src/controllers/jobs.rs:42`): an unbounded `HashMap<String, String>` with no TTL, size cap or eviction, and entries here are up to 32 MiB each (`MAX_SCHEMA_BODY_BYTES`) rather than the few KB the other stores hold. The only drains are inside `reconcile_schema_build`, which the replica reconciler calls only while a restore is in `Switching` with a `builder_image`. If the build POSTs its schema and the restore then leaves `Switching` by any other route — the restore is deleted, the deployment goes unhealthy, or the replica/namespace is torn down while the Job is finishing — nothing ever takes the entry and the 32 MiB is pinned for the process lifetime, multiplied by the number of replicas that hit that path. Give `CallbackStore` a bounded/expiring variant for this store (timestamped entries swept on a timer, or an aggregate byte cap), or drain the key from the replica's delete/finalizer path.

-------

`src/bin/operator.rs:750`: `verify_build_token` does a live `Api::<Job>::get_opt` against the API server on every request to `/api/v1/schema-build-results/...`, before any credential has been proven. Anything that can reach the operator's HTTP port can therefore amplify each cheap request into a kube API GET, with no rate limit or caching in front of it — enough traffic will get the operator client-side throttled and slow every other reconcile that shares the client. Caching the token per (namespace, replica) for the lifetime of the build (it is generated once in `reconcile_schema_build`) or reading it from the existing Job watch/informer cache would remove the per-request round trip.