[dannash100]

A reporting-schema build restores a real site replica and migrates it, but advertised no pre-migrate defaults, so a cross-version build meets the site's reporting views and undecryptable S1: secrets that upgrade already handles. A failed build also reported only "the build job failed" and then deleted the pod holding the reason.

• reporting-schema now advertises the same pre_migrate_drop_schemas / pre_migrate_sql defaults as upgrade, from the same constants. The builder writes its SQL from the dbt manifest, so dropping the old reporting first changes nothing it emits.
• A failed build's result carries the last 40 lines (max 2000 bytes) of the build container's log, read before the Job is deleted, ANSI stripped, with the DB password and callback token masked.
• The log-tail helper moves from the migration code to jobs.rs so both share it.

tests/migration.rs (integration, CI only) only picks up the constant rename.

🦸 Review Hero

• Run Review Hero
• Run Review Hero on Claude (before merge) Final review with Claude just before merging a substantive PR. Tick one or the other.

[review-hero]

🦸 Review Hero Summary (round 1) · reviewed ef1f115 · on z-ai/glm-5.3-flash:nitro
9 agents reviewed this PR | 0 critical | 3 suggestions | 1 nitpick | Filtering: consensus 3 voters, 4 below threshold, 1 confirmed on a second look

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

Location	Agent	Severity	Comment
src/controllers/jobs.rs:131	Bugs & Correctness	suggestion	container_log picks list.items.first() from a label-selector list whose order the API makes no guarantee about. A Job that consumed its backoffLimit leaves several job-name=<job> pods behin...
src/controllers/replica/schema_build.rs:610	Performance	nitpick	The Failed branch performs four mutually independent API calls strictly sequentially: container_log (pod list, then pod logs) followed by build_secrets (creds Secret GET, then the build-token J...
src/controllers/replica/schema_build.rs:647	Security	nitpick	read_secret_field(&secret, "password").ok() silently discards a failed read, so masking is silently skipped if the Secret lacks or renames the field — only a whole-Secret get error is warned. A pas...
src/controllers/replica/schema_build.rs:653	Performance	suggestion	build_secrets calls build_token, which re-fetches the build Job with jobs.get_opt(&build_job_name(replica_name)) just to read its BUILD_TOKEN_ANNOTATION — but reconcile_schema_build alrea...

Nitpicks

File	Line	Agent	Comment
src/controllers/replica/schema_build.rs	611	Bugs & Correctness	build_secrets is awaited unconditionally, but its result is only consumed by build_failure when log is Some — when the pod's log could not be read, build_failure(None, …) discards the secrets entirely. On every failed build with no readable pod this costs two API reads (creds Secret plu...

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:611`: `build_secrets` is awaited unconditionally, but its result is only consumed by `build_failure` when `log` is `Some` — when the pod's log could not be read, `build_failure(None, …)` discards the secrets entirely. On every failed build with no readable pod this costs two API reads (creds Secret plus the Job annotation for the token) and can emit misleading "reading the build credentials/token failed" warnings for work that yields nothing. Skip the lookups when `log.is_none()`, e.g. `let secrets = if log.is_some() { build_secrets(...).await } else { Vec::new() };`.

-------

`src/controllers/replica/schema_build.rs:672`: Masking by raw string replace misses percent-encoded credentials. The repo's own test establishes the builder logs its connection as `postgresql://analytics:PASSWORD@host`; when the password (or token) contains URL-special characters, that URI carries them percent-encoded, so `log.replace(secret, "[redacted]")` doesn't match and the encoded credential is recorded in the restore status and reported to canopy. Since the password is read from the Secret, mask a percent-encoded variant of each secret too (e.g. `utf8_percent_encode(secret, NON_ALPHANUMERIC)` from `percent-encoding`) alongside the raw form, or redact the userinfo component of any `scheme://user:pass@host` match.

-------

`src/controllers/restore/migration.rs:652`: The migration failure path now shares the new `jobs::container_log`/`log_tail` helpers but records the log tail into the restore status via `migration_error` with none of the redaction `build_failure` applies, even though the migrate Job runs against the same restore database and carries the same credentials — a migrate container that logs its connection string or the password on failure puts it straight into the status field. The redaction concept introduced in this PR stops at the schema-build path. Suggested fix: apply the same secret masking (the restore's password, at minimum) in `migration_error`/`read_result` before the tail is stored.

-------

`src/controllers/jobs.rs:194`: `is_error_headline` doesn't recognise dbt's error lines. The builder image is a dbt build, and dbt reports failures as `Database Error in model ds__encounters (models/datasets/ds__encounters.sql)` / `Failure in model …` — none of the markers ("Error:", "DatabaseError", …) match those (dbt uses a space and no colon, and these lines don't start the line). When a long dbt log is truncated, the preserved headline is only the generic `Error: the build failed` at the very end, so the one line that names the failing model is cut and canopy can't show which model broke. Add dbt-shaped markers such as `"Database Error"` and `"Failure in model"` (or a `" in model ` check) to the list.

[review-hero]

🦸 Review Hero Summary (round 2) · reviewed cdc17fa · on claude-opus-5
12 agents reviewed this PR | 1 critical | 6 suggestions | 1 nitpick | Filtering: consensus 3 voters, 8 below threshold, 1 suppressed

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

Location	Agent	Severity	Comment
src/controllers/canopy/intent.rs:149	Design & Architecture	nitpick	The constant was generalised from DEFAULT_UPGRADE_DROP_SCHEMAS to DEFAULT_PRE_MIGRATE_DROP_SCHEMAS and its doc now reads "A migrating replica that left this unset would fail" — but `analytics_p...
src/controllers/canopy/intent.rs:213	Design & Architecture	suggestion	upgrade_param_schema and reporting_schema_param_schema now contain byte-identical entries for PRE_MIGRATE_DROP_SCHEMAS and PRE_MIGRATE_SQL, and the new test asserts the two must stay equal....
src/controllers/jobs.rs:137	Performance	suggestion	tail_lines: Some(200) bounds the number of lines but not the number of bytes: a single log line can be arbitrarily long (Sequelize/dbt errors embed whole SQL statements, and JSON-formatted logger...
src/controllers/jobs.rs:164	Bugs & Correctness	suggestion	The preserved headline is a single line, which works for the node/sequelize format (message on the same line) but not for the dbt markers added here. dbt prints `Database Error in model ds__encount...
src/controllers/jobs.rs:341	Design & Architecture	nitpick	jobs.rs now has two #[cfg(test)] mod blocks (tests at the top and log_tail_tests appended after it). The split is an artifact of moving code from migration.rs rather than a meaningful gro...
src/controllers/replica/schema_build.rs:611	Design & Architecture	nitpick	build_secrets → build_token does a fresh GET of the build Job, but build_outcome fetched that exact Job a few lines earlier and discarded it after pulling delivered_bytes off its annotation...
src/controllers/replica/schema_build.rs:619	Security	suggestion	The masking list covers only the two values pgro itself hands the pod, but the build runs dbt against a restore of real production data, and the errors it emits quote row contents — the tests in th...
src/controllers/restore/migration.rs:465	Design & Architecture	nitpick	The file already imports from this module at line 28 (controllers::jobs::{env_from_secret_name, env_literal}), but the two new call sites use fully-qualified `crate::controllers::jobs::container_...

Nitpicks

File	Line	Agent	Comment
src/controllers/jobs.rs	200	Performance	is_error_headline allocates a String via format!(" {marker}") for each marker on each line it inspects. With the marker list now at 10 entries and logs of up to 200 lines, that is up to ~2000 throwaway allocations per failed Job just to find the headline. Avoid the per-line allocation by bu...

Local fix prompt (copy to your coding agent)

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

-------

`src/controllers/restore/migration.rs:465`: The refactor shares log reading and tailing with the build path but not the new redaction, so the migration path still publishes an unmasked log tail. The migrate container is handed `CONFIG_SYNC_DB_PASSWORD` and a `DATABASE_URL` containing it (lines 105-119), and tamanu/sequelize failures routinely echo the connection config or the URL; that tail lands in `migrationResult.error`, which is reported to canopy. `read_result` already has `creds: &(String, String)` in scope, so masking is cheap here: move the strip-ANSI + mask step from `schema_build::build_failure` into the shared `jobs` module (e.g. `log_tail_masked(log, &secrets)`) and pass `creds.1` from this call site.

-------

`src/controllers/replica/schema_build.rs:673`: The URL-encoded masking uses `NON_ALPHANUMERIC`, which percent-encodes the unreserved characters `-`, `.`, `_` and `~` as well. Real encoders (libpq, `encodeURIComponent`, most client libraries) leave those alone, so for a password like `p.ss/w0rd` the log holds `p.ss%2Fw0rd` while this computes `p%2Ess%2Fw0rd` — no match, and the encoded password survives into the status. Use a set that only encodes what real encoders encode, e.g. `const ENCODE: &AsciiSet = &NON_ALPHANUMERIC.remove(b'-').remove(b'.').remove(b'_').remove(b'~');`, so the computed form matches what actually appears in a connection string.

-------

`src/controllers/jobs.rs:185`: `is_error_headline` moved into the generic `controllers/jobs.rs` (a module that otherwise only knows about Job status and env vars) while simultaneously gaining dbt-specific markers (`Database Error in`, `Compilation Error in`, `Runtime Error in`, `Failure in model`) alongside the Sequelize/Tamanu ones. The shared module now encodes the vocabulary of two unrelated workloads, and each consumer gets the other's heuristics: a migration log line mentioning "Failure in model" is now promoted to a headline in a restore's status even though no dbt runs there. Keep `log_tail` generic by taking the markers as a parameter (or a small `&[&str]` const owned by each caller), so `jobs.rs` stays about Jobs and each caller owns what an error looks like in its own tool's output.

-------

`src/controllers/replica/schema_build.rs:665`: The refactor shares the log *reading* (`container_log`) and the log *bounding* (`log_tail`) between the migration and build paths, but puts sanitising — ANSI stripping and secret masking — only on the build side in `build_failure`. Both paths take a pod log that was handed the same database credentials via env and write it into a CR status that canopy renders, so the responsibility is identical; only one path discharges it. Either move the strip/mask step into the shared helper (e.g. `container_log` returning already-stripped text, plus a `redact(log, secrets)` in `jobs.rs` both callers use), or make it explicit why migration logs need neither. As it stands the next consumer of `container_log` inherits the reading but not the safety.

-------

`src/controllers/replica/schema_build.rs:670`: `build_failure` does all of its work on the *whole* log before truncating: `strip_ansi_escapes::strip` allocates a full copy, `from_utf8_lossy(..).into_owned()` another, and then each secret causes two more full-log `String::replace` copies (plaintext + percent-encoded) — roughly 2 + 2×N full copies of a log that is finally reduced to 2000 bytes by `log_tail`. With an unbounded log (see the `limit_bytes` note on `container_log`) this is the most expensive part of the failure path. Cheap fix: bound the input first (take the last ~64KB on a char boundary), then strip/mask/tail. Masking must still run over a region larger than the final tail so a secret can't survive at the cut boundary, so keep the order strip → mask → tail within that bounded slice.

-------

`src/controllers/jobs.rs:127`: `container_log` lists every Pod matching `job-name=<job>` with no `limit`, then uses only `items.first()`. A Job that retried (backoffLimit) leaves several Pods, and each Pod object fetched is a full spec/status payload of several KB. Since only one item is ever used, bound the request: `ListParams::default().labels(...).limit(1)` — the same pattern already used at `src/controllers/replica.rs:1449`. This path now has two callers (migration failures and schema-build failures), so the waste is paid on every failed Job.

-------

`src/controllers/jobs.rs:200`: `is_error_headline` allocates a `String` via `format!(" {marker}")` for each marker on each line it inspects. With the marker list now at 10 entries and logs of up to 200 lines, that is up to ~2000 throwaway allocations per failed Job just to find the headline. Avoid the per-line allocation by building the spaced markers once (e.g. a `const` array of `" Error:"`-style literals alongside the bare ones, or match on `line.contains(marker)` plus a boundary check) and keep the loop allocation-free.

-------

`src/controllers/replica/schema_build.rs:647`: The secret masking fails open. `build_secrets` discards errors (`read_secret_field(...).ok()`, `warn!` on `creds.get` / `build_token` failure) and returns an empty or partial list, and `build_failure` then publishes the raw log tail anyway. A transient API error, a rotated/renamed creds Secret, a missing `password` key, or a Job whose token annotation can't be read is enough to write the unmasked build log into the restore status — which `canopy/verification.rs` forwards off-cluster via `maybe_error(result.error.clone())`. Since the whole point of this path is that the build log can contain `postgresql://analytics:<password>@...` and the callback token, the failure mode is credential disclosure to an external service. Make masking required rather than best effort: have `build_secrets` return a `Result`/flag, and when the password or token could not be read, record plain `BUILD_FAILED` (or a log with the whole URL/credential-shaped substrings elided) instead of the log.

[review-hero]

[Bugs & Correctness] suggestion

The URL-encoded masking variant is computed with percent_encoding::NON_ALPHANUMERIC, which percent-encodes every non-alphanumeric byte — including the RFC 3986 unreserved characters '-', '.', '' and '~'. The encoders that actually produce the connection strings a builder logs (JS encodeURIComponent, Go/Python URL quoting) leave those characters unencoded, so for a secret like "p-ss.w0rd" the log carries e.g. "postgresql://analytics:p-ss.w0rd%2F..." while the computed variant is "p%2Dss%2Ew0rd%2F..." — neither the raw replacement nor the encoded replacement matches, and the secret is written into the recorded restore status. Build the variant with an AsciiSet that leaves unreserved characters alone (NON_ALPHANUMERIC.remove(b'-').remove(b'.').remove(b'').remove(b'~'), plus !'()* if the builder is JS), or mask both variants, so the mask covers passwords containing hyphens and dots — which are common.

[review-hero]

🦸 Review Hero Summary (round 3) · reviewed cdc17fa · on z-ai/glm-5.3-flash:nitro
12 agents reviewed this PR | 0 critical | 6 suggestions | 1 nitpick | Filtering: consensus 3 voters, 7 below threshold, 1 confirmed on a second look, 1 suppressed

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

Location	Agent	Severity	Comment
src/controllers/canopy/intent.rs:149	Bugs & Correctness	nitpick	The updated comment on the shared default says "A migrating replica that left this unset would fail on something the deployment it models never meets", but the default is applied only to the `upgra...
src/controllers/canopy/intent.rs:190	Design & Architecture	suggestion	The two param-schema functions now carry identical defaults for the same two params, and a test has to assert the coincidence instead of the code guaranteeing it. After this change both `upgrade_pa...
src/controllers/jobs.rs:131	Design & Architecture	suggestion	list.items.first() picks an arbitrary pod among all pods carrying the job-name label. A Job with the default backoffLimit can leave several attempt pods behind, and the list order is by pod nam...
src/controllers/jobs.rs:185	Performance	nitpick	is_error_headline allocates a format!(" {marker}") string per marker for every line scanned — up to 15 markers × ~200 tail lines per failed log when no headline matches. Precompute the space-pref...
src/controllers/jobs.rs:197	Design & Architecture	nitpick	The shared is_error_headline now accumulates caller-domain error formats for every consumer. The function moved out of migration.rs carried the Sequelize/Tamanu markers, and this PR appends dbt-s...
src/controllers/replica/schema_build.rs:649	Security	suggestion	build_secrets reads the password from the creds Secret at failure time, but the build pod received its password via env when the Job was created. If the credential Secret is rotated while a build i...
src/controllers/replica/schema_build.rs:675	Security	suggestion	Secret masking in build_failure is fragile in two ways. First, secrets are replaced in list order: if one secret is a substring of another (e.g. the generated password also appearing inside the lon...

Nitpicks

File	Line	Agent	Comment
src/controllers/replica/schema_build.rs	647	Security	read_secret_field(...).ok() silently discards the per-field error while the surrounding get failure warns: if the Secret exists but lacks a usable 'password' field, masking quietly loses its most important entry with no trace in the logs. Log the field-level error at warn like the two neighbourin...

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:673`: The URL-encoded masking variant is computed with percent_encoding::NON_ALPHANUMERIC, which percent-encodes every non-alphanumeric byte — including the RFC 3986 unreserved characters '-', '.', '_' and '~'. The encoders that actually produce the connection strings a builder logs (JS encodeURIComponent, Go/Python URL quoting) leave those characters unencoded, so for a secret like "p-ss.w0rd" the log carries e.g. "postgresql://analytics:p-ss.w0rd%2F..." while the computed variant is "p%2Dss%2Ew0rd%2F..." — neither the raw replacement nor the encoded replacement matches, and the secret is written into the recorded restore status. Build the variant with an AsciiSet that leaves unreserved characters alone (NON_ALPHANUMERIC.remove(b'-').remove(b'.').remove(b'_').remove(b'~'), plus !'()* if the builder is JS), or mask both variants, so the mask covers passwords containing hyphens and dots — which are common.

-------

`src/controllers/replica/schema_build.rs:611`: The failed-build path unconditionally awaits `build_secrets`, two Secret API round-trips, even when `container_log` returned None — and `build_failure` ignores the secrets entirely when there is no log. Restructure so the secrets are only read when there is something to mask, e.g. make the failure cause an async helper that takes the Option<String> log and fetches secrets only inside the Some branch. This runs on every failed-build reconcile, so it also keeps the path from warning about secret reads it never uses.

-------

`src/controllers/replica/schema_build.rs:664`: Secret masking lives in the schema_build call site rather than in the shared log-capture path, leaving the sibling consumer unmasked. The PR extracts `container_log`/`log_tail` into `controllers::jobs` precisely so both the migration job and the build job read failure logs the same way, but the masking concern (`build_failure` + `build_secrets`) is applied only to the build path. The migration job is handed the same credentials (`DATABASE_URL` with the password interpolated, src/controllers/restore/migration.rs:114-119) and its failure tail is recorded into the replica status via `migration_error` → `jobs::log_tail` with no masking, so a migrator that logs its connection string leaks the password into a status field. Since the migration path is pre-existing this is not a new leak, but the PR's design bakes the asymmetry in: the shared pipeline knows how to capture and bound a log but not how to make it safe to publish. Suggested fix: move masking into a shared helper (e.g. `jobs::redact(log, secrets)` or a `log_tail_redacted` that takes a secrets list) and pass the replica's credentials from both call sites, so the invariant 'anything recorded from a job log is masked' holds in one place.

-------

`src/controllers/replica/schema_build.rs:671`: build_failure masks the entire fetched log before truncating it. container_log fetches up to 200 tail *lines* with no byte cap, so a failed build that dumped a large SQL statement or a big error dump can make the log megabytes; the ANSI-strip copy, the from_utf8_lossy copy, and two full-log `replace` passes per secret then allocate that whole log several times, only for log_tail to keep ~2000 bytes. Masking the ~2000-byte tail (plus the ≤500-byte headline) after log_tail instead of the full log bounds the work to the size of the result with no loss of safety — any secret occurrence that survives intact into the kept text is still masked. Same total allocations avoided for secrets whose occurrences were truncated away.

-------

`src/controllers/replica/schema_build.rs:610`: The Failed branch awaits four independent Kubernetes round-trips sequentially before deleting the Job: pod list + log fetch (container_log), the credentials Secret GET, and the Job GET build_token makes. Each is one API-server latency bump on the failure path. container_log and build_secrets are independent and could run concurrently (e.g. tokio::join!), roughly halving the wall-clock of this branch. Relatedly, build_secrets does both Secret GETs even when `log` is None and the secrets can never be used — skipping it when the log could not be read saves two calls in exactly the case (pod already gone) where the Job deletion deadline pressure is highest.

-------

`src/controllers/restore/migration.rs:652`: The migration failure path still writes the job's log tail into the replica status unmasked, while this PR adds secret masking to the otherwise identical build-failure path. The migration job receives the plaintext database password as env (DATABASE_URL interpolates CONFIG_SYNC_DB_PASSWORD at src/controllers/restore/migration.rs:114-119), and connection failures are commonly echoed with the DSN by tamanu/Sequelize, so the password can land in the status field that canopy reads and stores. Since the PR built the masking machinery and this is now the only failure-log path without it, pass the replica's password (and any callback-style tokens) through the same redaction as build_failure does — e.g. let the shared helper accept a secrets list, or mask the log in read_result before calling log_tail.

-------

`src/controllers/replica/schema_build.rs:647`: read_secret_field(...).ok() silently discards the per-field error while the surrounding get failure warns: if the Secret exists but lacks a usable 'password' field, masking quietly loses its most important entry with no trace in the logs. Log the field-level error at warn like the two neighbouring error branches do, so an operator can tell when a recorded failure log was not fully masked.

[passcod]

lgtm can you also release and deploy so juliana's fix also comes with