feat(alertd): grade and apply the reporting schema canopy offers

[dannash100]

Canopy has read and displayed reportingSchemaVersion for a while, and nothing produced it. This produces it, and grades the server against the schema canopy offers its group.

• New reporting_schema check: read the version the schema stamped on itself, ask canopy what it offers for the version this server runs, fail when they differ.
• The stamp is reported whether or not it matches. A server on the wrong schema is when knowing which one it has matters most.
• Applying is the heal action, not part of the check. It is the only thing bestool does that writes to Tamanu's database, so it stays daemon-only and behind the heal backoff.

🦸 Review Hero

• Run Review Hero

[review-hero]

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

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

Location	Agent	Severity	Comment
crates/alertd/src/doctor/checks.rs:322	Bugs & Correctness	critical	reporting_schema is registered with the host variant, so unlike every other Tamanu-specific check it runs even when ctx.tamanu.is_tamanu is false — the context synthesised from the generic `D...
crates/alertd/src/doctor/checks/reporting_schema.rs:59	Bugs & Correctness	nitpick	Every error from the stamp query becomes Check::broken, which in this codebase means "the healthcheck's own SQL is at fault" and is deliberately non-fatal. query_error_check(NAME, &err) is the ...
crates/alertd/src/doctor/checks/reporting_schema.rs:100	Bugs & Correctness	nitpick	When canopy offers only a range-registered reporting schema, the range is recorded with tracing::warn! and the check then reports "none offered for this version / canopy has no reporting schema...
crates/alertd/src/doctor/checks/reporting_schema.rs:129	Bugs & Correctness	suggestion	The stamp is compared to the offered version with exact string equality on the raw obj_description text. A schema comment written by generated SQL very easily picks up surrounding whitespace or a...
crates/alertd/src/doctor/checks/reporting_schema.rs:155	Security	suggestion	The reporting schema comment is promoted unvalidated to reportingSchemaVersion, a top-level status fact posted to canopy and rendered in the fleet view. obj_description is arbitrary databas...
crates/alertd/src/doctor/checks/reporting_schema.rs:250	Performance	nitpick	fetch_offered buffers the whole schema SQL into a String via .text() with no size bound before handing it to batch_execute. A reporting schema covering every view for a group can be large, ...
crates/alertd/src/doctor/checks/reporting_schema.rs:260	Bugs & Correctness	suggestion	offered_path unconditionally discards the origin of download_url and re-issues the request against canopy's own transport. That holds only if canopy serves every reporting-schema artifact itsel...

Nitpicks

File	Line	Agent	Comment
crates/alertd/src/doctor/checks/reporting_schema.rs	281	Performance	heal re-runs offered_schema, repeating the artifact-listing round trip the check just made moments earlier in the same sweep (line 80) before it graded the failure that triggered the heal. Since the heal only fires off the back of that check, the resolved Offered could be carried across (e....

Local fix prompt (copy to your coding agent)

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

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:300`: `heal` reports `Healed` as soon as `batch_execute` returns Ok, without re-reading the stamp. If the applied SQL doesn't leave a stamp equal to `offered.version` — the build stamps a differently-formatted string, the comment has trailing whitespace/newline, the artifact was built for another version, or it stamps nothing at all — the check keeps failing, and because `Healed` resets `failures` to 0 in `heal::finish`, the next attempt is allowed after exactly `DEFAULT_MIN_INTERVAL` with no escalating backoff. The result is the reporting schema being dropped and recreated every 5 minutes indefinitely on a production database, with reports failing during each rebuild, and nothing in the backoff to slow it. Re-run `STAMP_SQL` after applying and return `HealOutcome::Failed` (or `Deferred`) when the stamp still doesn't match, so a repair that doesn't take backs off rather than looping.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:84`: The spec says the check skips when canopy is unreachable, but only the `ctx.canopy == None` case skips — that is "canopy not configured", not "unreachable". An actual connection failure/timeout/5xx from `versions_artifacts` returns `Err`, which lands here as `Check::warning`. So a canopy outage or deploy raises a warning on every server in the fleet for something no operator can act on locally, which is exactly what the skip branch was meant to avoid. Map transport-level errors (and arguably 5xx) to the same skip-with-stamp path used above, keeping `warning` for answers that indicate a real problem with this server (e.g. 401/403).

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:300`: The apply runs on `tamanu.db`, which is the single `Arc<PgClient>` shared by every DB-backed check in the sweep (`CheckContext.db`, checks.rs:109). `batch_execute` of a whole reporting schema (drop + recreate every view) is a long, single round-trip simple-query batch, and tokio-postgres serialises everything else queued on that connection behind it. Heal is spawned as a detached background task that can outlast the 60s `DOCTOR_INTERVAL` (see heal.rs docs), so a multi-minute apply stalls every other DB check for one or more full sweeps, and a DDL blocked on a lock stalls them indefinitely (there is no statement timeout on this path). Open a dedicated connection from `tamanu.database_url` for the apply (as `db_connect` does) and set a `statement_timeout`/`lock_timeout` on it, so the heal cannot contend with the read-only checks on the shared client.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:80`: `run` issues `versions_artifacts` to canopy on every sweep — once per 60s per server, fleet-wide — even when the check has been passing and nothing can have changed for a fixed Tamanu version. The offered artifact list for a given version is near-static, so this is a steady request rate against canopy for an answer that rarely changes. Consider memoising the offered version per Tamanu version with a short TTL (a few sweeps) and only re-asking after it expires or after a heal, so a passing check costs nothing on the wire.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:281`: `heal` re-runs `offered_schema`, repeating the artifact-listing round trip the check just made moments earlier in the same sweep (line 80) before it graded the failure that triggered the heal. Since the heal only fires off the back of that check, the resolved `Offered` could be carried across (e.g. cached alongside the memoisation suggested above) rather than paying a second authenticated request per attempt.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:260`: `offered_path` drops the origin but keeps the raw path, and the mTLS branch of `ReqwestTransport::get` resolves that path with `base_url.join(mtls_path)` (crates/canopy/src/reqwest_transport.rs:315-317). A path beginning with `//` is a scheme-relative reference: if canopy returns `download_url = "https://canopy.example//evil.host/x.sql"`, `url.path()` is `//evil.host/x.sql` and `base_url.join("//evil.host/x.sql")` resolves to `https://evil.host/x.sql`. The origin-drop that this function exists to enforce is bypassed, the device mTLS identity is presented to an attacker-chosen host, and the body it returns is handed straight to `batch_execute` against Tamanu's database. Anyone able to register/modify an artifact record for the group (not only a full canopy compromise) can reach this. Reject or normalise paths that start with `//` (and any path with an authority component) before building the request, e.g. bail unless `url.path()` starts with a single `/`, or better, validate that the offered URL's origin equals the transport's expected canopy origin instead of reconstructing a path.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:300`: The heal path executes whatever text comes back over the wire as SQL against Tamanu's database — the only write bestool makes — with no integrity check at all: no digest/signature comparison against what canopy advertised, and no content-type assertion, so any 2xx body (an HTML error page from a proxy, a captive-portal response, a truncated transfer) is executed. If canopy publishes a checksum for the artifact, verify it before `batch_execute`; at minimum assert the response content-type and a non-empty body, and consider capping the size read via `.text()` so an unbounded body can't be pulled into memory and run.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:246`: The offer's query string is preserved and replayed against a different endpoint than the one canopy named. If canopy ever hands back a pre-signed/tokened download URL, that credential is sent to the tailscale/mTLS endpoint (where it is useless) and, worse, is logged verbatim by the transport's `debug!(%url, "GET via canopy")` — a signed-URL token in the daemon's debug logs. Either drop the query along with the origin (if the transport-addressed endpoint genuinely doesn't need it) or keep the URL canopy gave and validate its origin, rather than splicing half of it onto another host.

[review-hero]

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

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

Location	Agent	Severity	Comment
crates/alertd/src/doctor/checks/reporting_schema.rs:131	Bugs & Correctness	nitpick	read_stamp takes the reporting schema's comment verbatim as Stamp::Version, with no validation that it looks like a version. A pre-existing hand-written comment ("Tamanu reporting views", a t...
crates/alertd/src/doctor/checks/reporting_schema.rs:202	Performance	nitpick	The range-registered-schema warn! fires from offered_schema(), which runs on every 60s sweep, so a single mis-registered artifact in canopy emits a warning per minute per host indefinitely — 14...
crates/alertd/src/doctor/checks/reporting_schema.rs:210	Bugs & Correctness	suggestion	offered_schema takes .find(is_exact_schema), i.e. whichever exact-version reporting-schema artifact canopy happens to list first, and heal re-resolves it independently of the sweep that gra...
crates/alertd/src/doctor/checks/reporting_schema.rs:270	Security	critical	fetch_offered runs on the canopy reqwest client, which sets no redirect policy anywhere in the workspace, so it follows up to 10 redirects by default. That reopens exactly the hole `download_path...
crates/alertd/src/doctor/checks/reporting_schema.rs:276	Performance	critical	fetch_offered buffers the whole schema SQL with .text() over the shared canopy transport client, whose request timeout is a total request deadline sized for API calls, not payload downloads: ...
crates/alertd/src/doctor/checks/reporting_schema.rs:289	Security	suggestion	id is interpolated into the request path unescaped. The unit test feeds a UUID string, but nothing here constrains it — if schema::Artifact::id is (or becomes) a String, a canopy-supplied val...
crates/alertd/src/doctor/checks/reporting_schema.rs:427	Bugs & Correctness	nitpick	This test does DROP SCHEMA IF EXISTS reporting CASCADE against the shared tamanu-central test database, taking an ACCESS EXCLUSIVE lock on the real reporting schema and holding it until the `RO...

Local fix prompt (copy to your coding agent)

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

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:254`: `canopy_is_out` maps *every* error without an HTTP status onto "canopy unreachable", and `run` turns that into a silent skip. `bestool_canopy::Error` has non-HTTP variants beyond a failed connection — a body canopy answered 200 with that doesn't deserialise into `Vec<Artifact>`, for instance, has no status either. A canopy that changed its artifact shape would then make this check skip on every server in the fleet indefinitely, with nothing visible to say the grading stopped working. Consider matching the transport/connection variants explicitly and letting decode failures fall through to the `Check::warning` arm below.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:368`: `apply_connection` opens its own connection with `tokio_postgres::connect(database_url, NoTls)`, bypassing `bestool_postgres::pool::connect_one`, which every other DB open in the project goes through. `connect_one` -> `create_pool` parses the URL with `url::parse_connection_url`, sets an application name, and — critically — selects a real TLS connector whenever `ssl_mode != Disable` (`crates/postgres/src/pool/manager.rs:52`). So on any deployment whose URL is `sslmode=require`/`verify-full`, or whose server rejects non-SSL connections, the sweep's shared client connects fine while the apply connection fails outright: heal returns `Failed` forever, the schema is never applied, and the check stays red with only a log line to show for it. Use `connect_one` (or at minimum the same TLS selection) here, then issue the `SET statement_timeout/lock_timeout` batch on it.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:38`: The check is registered with the `host` variant of `entry!`, which — unlike the plain (Tamanu) variant used by e.g. `reporting_roles` — does not filter on `is_tamanu`. `run` only guards on `ctx.tamanu.is_none()`, so on a host whose context came from the generic `DATABASE_URL` fallback (`is_tamanu == false`, the case the macro's own comment says "there's no Tamanu here") this check queries `pg_namespace` on an unrelated database, and if canopy happens to offer an artifact for the fallback version, `heal` will run `DROP SCHEMA reporting CASCADE`-style DDL against it. That contradicts the spec line "It skips on a host with no Tamanu" and the skip reason this function itself prints. Add `if !tamanu.is_tamanu { return Check::skip(...) }` alongside the `None` guard, and the same guard in `heal`.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:338`: The read-back guard's comment says reporting `Failed` on a stamp mismatch avoids "the schema is dropped and rebuilt on every interval, forever", but it doesn't — `Failed` only grows the backoff, which `heal::MAX_INTERVAL` caps at one hour. If canopy's SQL stamps something that never equals `offered.version` (a normalisation difference, a prerelease/build-metadata version string, a schema built for one version published under another), the check keeps failing, heal keeps firing, and the reporting schema is dropped and rebuilt hourly on that server indefinitely — with reports broken for the duration of each rebuild. Consider recording the mismatch and refusing to re-apply the same artifact id after it has already been applied once without producing the expected stamp.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:195`: `run()` issues an authenticated `versions_artifacts` request to canopy on every sweep. The doctor task ticks every 60s (`DOCTOR_INTERVAL` in doctor/task.rs:20), so each Tamanu host polls canopy's artifact listing 1440×/day for an answer that only changes when canopy publishes a new schema or the server is upgraded — fleet-wide that is a constant listing load on canopy for a near-static result. Other canopy-derived checks avoid this (billing_tags.rs:28 reads a cached snapshot rather than calling out), and `SweepContext::canopy` is documented as being for heal actions in particular. Suggest caching the offer keyed by Tamanu version with a TTL of several minutes (or refreshing it on the same cadence as other canopy state) rather than fetching per sweep.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:306`: `heal()` re-runs `offered_schema()` even though the check that just graded as failing (and triggered the heal, sweep.rs:231) resolved exactly the same offer moments earlier — two identical round trips to canopy per heal cycle, and the second can disagree with the grade the heal was authorised by. Carrying the resolved `Offered` from the check result into the heal (or sharing it through the cache suggested above) removes the duplicate request and makes the apply act on the offer that was actually graded.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:368`: The apply path has no overall time bound, and `heal::spawn_if_due` latches `in_flight = true` until the action returns (crates/alertd/src/doctor/heal.rs:105-118). `tokio_postgres::connect` here is called with no `connect_timeout`, and the DDL batch is only bounded per-statement (5min × however many statements the schema SQL contains). If the DB host blackholes the connection or the batch stalls behind locks in a way `lock_timeout` doesn't cover, the spawned task stays pending and the heal slot is never released — self-heal for `reporting_schema` is disabled for the rest of the process lifetime, silently, with a leaked task holding the connection. Wrap the heal body in `tokio::time::timeout` (or at least set `connect_timeout` on the connect config) so a stuck attempt reports `Failed` and returns the slot.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:276`: The heal fetches the artifact body with `.text()` (no size cap, no content-type check) and feeds it straight to `batch_execute` on the Tamanu database — the one write path bestool has into production data. `error_for_status()` only rejects non-2xx: a 200 response that isn't the SQL artifact (an HTML page from an intervening proxy or the tailscale `/public` mount, a redirect landing on object storage returning an error document with 200, a truncated body) is executed verbatim, and since the offered SQL drops and recreates the `reporting` schema, a partial or wrong body can destroy the existing schema without replacing it. Unbounded `.text()` also means a mis-sized artifact is buffered whole into the daemon's memory. Suggest asserting the response content-type (or at least that the body is non-empty and parses as the expected leading DDL), and capping the body length before applying.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:173`: The stamp is whatever text `obj_description` returns for the `reporting` schema — arbitrary, unbounded, and settable by anyone with COMMENT rights on that schema (the Tamanu app role, report authors, any DBA), not just by the build pipeline. It is reported verbatim to canopy as the top-level `reportingSchemaVersion` status fact on every sweep, and rendered in the fleet view. Nothing validates its shape or length here, so a multi-megabyte or markup-bearing comment rides into every status POST and into canopy's UI unchecked. Suggest validating the stamp at read time (parse as a version, or at minimum trim and reject/truncate beyond a small length) before both grading on it and publishing it as a status fact.

[review-hero]

🦸 Review Hero Summary
7 agents reviewed this PR | 2 failed | 1 critical | 6 suggestions | 1 nitpick | Filtering: consensus 3 voters, 2 below threshold

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

Location	Agent	Severity	Comment
crates/alertd/src/doctor/checks/reporting_schema.rs:68	Bugs & Correctness	suggestion	tokio_postgres::Error's top-level Display is the unhelpful literal "db error" — that is precisely why checks::fmt_db_error exists (see its doc comment at checks.rs:154). Every place this modu...
crates/alertd/src/doctor/checks/reporting_schema.rs:319	Bugs & Correctness	suggestion	The media-type gate compares the raw header value case-sensitively against a three-entry whitelist. Media types are case-insensitive (RFC 9110 §8.3.1), so a body served as Application/SQL, `TEXT/...

Nitpicks

File	Line	Agent	Comment
crates/alertd/src/doctor/checks/reporting_schema.rs	486	Performance	connect_one builds a full mobc pool, runs check_pool (which takes a connection and issues SELECT 1), then opens a second connection via manager.connect() and drops the pool (postgres/src/pool.rs:98-180). Each apply therefore costs two TCP+TLS handshakes and discards one of them. Heal is...

Local fix prompt (copy to your coding agent)

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

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:486`: `connect_one` builds a full mobc pool, runs `check_pool` (which takes a connection and issues `SELECT 1`), then opens a *second* connection via `manager.connect()` and drops the pool (postgres/src/pool.rs:98-180). Each apply therefore costs two TCP+TLS handshakes and discards one of them. Heal is rare enough that this is not urgent, but a direct `PgConnectionManager::connect` would halve the connection churn on a path that already runs behind a 10-minute deadline.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:324`: The body is streamed into memory and only rejected once 32 MB have already crossed the wire, so an oversized (or wrong) artifact costs a full 32 MB transfer on every heal attempt before it is thrown away. Check `response.content_length()` against `MAX_SCHEMA_BYTES` before the read loop and bail immediately when it is over, and `Vec::with_capacity(content_length.min(cap))` so the buffer isn't grown by repeated reallocation up to 32 MB.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:86`: This is the only check that makes a live canopy round-trip on every sweep, and the daemon sweeps every 30s (crates/alertd/src/daemon.rs:273). That is an authenticated TLS GET of /versions/{version}/artifacts twice a minute per server, ~2900/day/server, for an answer keyed by (group, tamanu version) that only changes when a schema is published or the server upgrades — so the fleet-wide load scales with server count for near-constant data. Other canopy-derived checks avoid this (billing_tags reads `load_cached_tags()` instead of calling out per sweep). Suggest memoising the offer per version behind a short TTL (a few minutes) shared by `run` and `heal`, so the sweep cadence doesn't drive canopy request volume.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:387`: `heal` re-issues `offered_schema` even though the sweep that graded the check as failing just fetched the same answer moments earlier, so each heal attempt costs an extra artifact listing on top of the download. Threading the `Offered` from the check's own resolution (or the cache suggested above) removes a redundant network round-trip per heal attempt, which matters most exactly when canopy is slow or degraded.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:159`: `stamp_of` only rejects empty/over-64-byte text, but its doc comment and the test `a_comment_that_is_not_a_version_is_not_a_stamp` claim "what is not plausibly a version is read as no stamp". Any DB role with COMMENT rights on `reporting` can therefore put arbitrary 64-byte text — including newlines, control characters, or markup — into the schema comment, and `with_version` publishes it unchanged to canopy as the top-level `reportingSchemaVersion` status fact rendered in the fleet view. Parse it (e.g. `node_semver::Version::parse`, already a dependency here) and treat an unparseable comment as `Unstamped` so only a real version can reach the fleet-wide fact.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:305`: The credentialed artifact GET follows redirects. Neither the canopy client factories (`crates/alertd/src/lib.rs:31`, `crates/canopy/src/connect.rs:80`) nor `ReqwestTransport::get` set a redirect policy, so reqwest's default (follow up to 10) applies. Whatever body the final hop returns is then handed to `apply.batch_execute(&sql)` (line 426) as privileged DDL against Tamanu's database, and the schema's own SQL drops the `reporting` schema before recreating it — so a bad body destroys what it does not replace. The careful `download_path` construction (line 352) is meant to stop canopy naming an arbitrary authority, but a 302 from the download endpoint reopens exactly that: the SQL now comes from an unconstrained host, and the only remaining guard is a `Content-Type` header that host also controls (`application/octet-stream` is accepted). Fix: build this request with `redirect::Policy::none()` (or assert the final `response.url()` host/scheme matches the transport base) before executing the body.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:319`: The `Content-Type` allowlist is documented as the guard against a wrong body being executed as SQL, but it is not an integrity control — it is a response header from whatever served the bytes, and `application/octet-stream`/`text/plain` accept essentially anything. Since this is the only code path in the project that writes to Tamanu's database, and it does so by wholesale drop-and-recreate, the fetched artifact should be verified against a value canopy publishes out of band (artifact digest/length from the artifacts listing) rather than trusted on media type alone. If canopy's `Artifact` carries no digest field today, that is worth raising with canopy rather than papering over here; at minimum, tighten the accepted types to the one canopy actually registers schemas under instead of including `application/octet-stream`.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:352`: `id` is interpolated into the request path with no escaping or validation. It comes from canopy's JSON artifact listing, so if `Artifact::id` is a `String` rather than a `Uuid`, a value containing `../` or a `?`/`#` character re-points this authenticated GET at a different canopy endpoint (`Url::join` normalises dot segments), which then feeds the body to `batch_execute`. The impact is bounded to the same host, but it is cheap to close: either rely on a typed `Uuid` (assert it, e.g. accept `id: Uuid` in `download_path`) or percent-encode/reject ids that are not a plain UUID before building the path.

[review-hero]

🦸 Review Hero Summary
7 agents reviewed this PR | 2 failed | 2 critical | 3 suggestions | 1 nitpick | Filtering: consensus 3 voters, 11 below threshold

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

Location	Agent	Severity	Comment
crates/alertd/src/doctor/checks/reporting_schema.rs:89	Bugs & Correctness	suggestion	When there's no install on disk and no recorded currentVersion, tamanu_version is the 0.0.0 placeholder (see reporting_roles::is_unknown_version), and this check asks canopy for `/versions/...
crates/alertd/src/doctor/checks/reporting_schema.rs:187	Security	suggestion	Stamp::Unstamped grades as Fail, and heal runs on failure, so the daemon will drop and replace a reporting schema that this pipeline did not build — the very case the doc comment describes as...
crates/alertd/src/doctor/checks/reporting_schema.rs:243	Bugs & Correctness	suggestion	find(is_exact_schema) takes whichever reporting-schema artifact canopy happens to list first, with no tie-break on group, platform or recency. The doc comment above says the unauthenticated lis...
crates/alertd/src/doctor/checks/reporting_schema.rs:278	Bugs & Correctness	suggestion	offers_nothing maps any 404 from versions_artifacts to "canopy offers none", which also swallows a 404 caused by the wrong path, a wrong parameter shape, or an endpoint canopy has moved. In tha...
crates/alertd/src/doctor/checks/reporting_schema.rs:310	Bugs & Correctness	critical	The heal fetches the schema from a hand-built path (/versions/{version}/artifacts/{id}/download) that nothing else in this repo uses and that canopy_contract.rs — the test that exists precisely...
crates/alertd/src/doctor/checks/reporting_schema.rs:328	Security	suggestion	The fetched body is executed as unrestricted DDL on Tamanu's production database, but the only checks on it are a Content-Type allowlist (which includes text/plain and `application/octet-stream...
crates/alertd/src/doctor/checks/reporting_schema.rs:423	Bugs & Correctness	suggestion	apply_offered never re-reads the current stamp before applying, so it drops and recreates the reporting schema on the strength of a sweep verdict that can be up to a heal interval old. If someone...
crates/alertd/src/doctor/checks/reporting_schema.rs:441	Security	suggestion	apply.batch_execute(&sql) runs canopy-supplied SQL with no transaction of bestool's own, and the whole apply can be cancelled mid-flight by HEAL_DEADLINE (line 379) or by the 5min server-side `...
crates/alertd/src/doctor/checks/reporting_schema.rs:480	Security	suggestion	The guard against re-applying an artifact that failed to stamp itself lives in a process-local OnceLock<Mutex<HashSet>>, so it is lost on every daemon restart. The apply is destructive (the schem...
crates/alertd/src/doctor/checks/reporting_schema.rs:504	Performance	nitpick	connect_one builds a whole mobc pool, opens a connection for check_pool's SELECT 1, then opens a second connection via manager.connect() and drops the pool (postgres/src/pool.rs:98-180). ...
crates/alertd/src/doctor/checks/reporting_schema.rs:559	Bugs & Correctness	suggestion	This test opens a transaction on the shared central_ctx() connection and runs DROP SCHEMA IF EXISTS reporting CASCADE, which takes ACCESS EXCLUSIVE locks on every object in the real local `tama...

Nitpicks

File	Line	Agent	Comment
crates/alertd/src/doctor/checks/reporting_schema.rs	338	Performance	sql starts empty and grows by doubling while streaming a payload permitted to reach 32 MiB, so a large schema pays repeated reallocation and memcpy of the accumulated buffer. content_length() is already read just above; preallocate with `Vec::with_capacity(len.min(MAX_SCHEMA_BYTES as u64) as ...

Local fix prompt (copy to your coding agent)

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

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:179`: Grading and the post-apply read-back compare version *strings* (`stamp == offered`, line 450), but `stamp_of` accepts anything `node_semver::Version::parse` takes — which is loose: a leading `v`, `=`, surrounding whitespace, build metadata. A schema stamped `v2.60.0` or `2.60.0+build7` parses fine, so it is `Stamp::Version`, yet never string-equals `tamanu_version.to_string()`. Result: the check fails permanently on a server that actually has the right schema, heal drops and rebuilds the schema once (breaking reports while it runs), the read-back mismatches, and the artifact is then jammed into the `unstamped` registry so heal defers forever with no way out short of a restart. Parse both sides and compare `node_semver::Version` values (keeping the raw trimmed text only for display/the status fact).

-------

`crates/canopy/src/reqwest_transport.rs:337`: `Policy::none()` is now set on both the probe client and the mTLS client, and the raw `get()` additionally hard-fails any 3xx. That makes heal unable to fetch the schema if canopy's `/versions/{v}/artifacts/{id}/download` answers with a 302 to object storage — which is the usual shape for an artifact download endpoint here (artifacts carry an absolute `download_url` to another host, and `fetch_offered` even accepts `application/octet-stream`, the media type storage returns). If that endpoint redirects, `reporting_schema::heal` fails on every attempt forever and only logs a warning. Also note the policy change applies to every existing canopy request, not just the new raw GET, so any other endpoint that relied on a redirect silently breaks. Please confirm the download endpoint streams bytes itself; if it redirects, allow one same-origin (or storage-origin) hop instead of refusing outright.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:313`: The schema download runs on canopy's shared client, which in tailscale mode is `build_probe_client` with `.timeout(TAILSCALE_PROBE_TIMEOUT)` (5s, crates/canopy/src/reqwest_transport.rs:58). `reqwest`'s builder timeout covers the whole request including body read, so any schema that takes more than 5s to stream — well within the 32 MiB ceiling this code explicitly allows — is aborted mid-body. Heal then retries under backoff, re-downloading and discarding the same partial payload forever without ever applying it. Give the raw GET its own longer per-request timeout (`RequestBuilder::timeout(...)`) or build a separate client for artifact downloads instead of reusing the 5s probe client.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:89`: `offered_schema` issues a canopy `versions_artifacts` request on every sweep, and the sweep runs every 60s (DOCTOR_INTERVAL in doctor/task.rs:20). The answer only changes when a new artifact is published, so this is one HTTP round trip per server per minute against canopy for effectively static data — multiplied across the fleet. Cache the offer with a short TTL (a few minutes) keyed on the Tamanu version, so the common steady state costs nothing.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:402`: `apply_offered` re-runs `offered_schema` even though the sweep that graded the check as failing already fetched exactly the same listing moments earlier, so every heal attempt costs a duplicate canopy round trip (and can race a differing answer between grade and apply). Pass the resolved `Offered` from the check into the heal, or share it via the same cache suggested for the check path.

-------

`crates/alertd/src/doctor/checks/reporting_schema.rs:338`: `sql` starts empty and grows by doubling while streaming a payload permitted to reach 32 MiB, so a large schema pays repeated reallocation and memcpy of the accumulated buffer. `content_length()` is already read just above; preallocate with `Vec::with_capacity(len.min(MAX_SCHEMA_BYTES as u64) as usize)` when it is present.

[passcod]

To apply a new reporting schema, the old one is dropped first. Either this can be fully done within a migration, so it's safe, or it can't, and every self heal has the potential to not only not upgrade the schema, but also to break it permanently. Either way, that needs documenting, and also maybe reconsidering the concept of a healthcheck effectively performing upgrades.

[dannash100]

🤖 Follow-up, not in this PR: fetch_offered still trusts the Content-Type alone. Canopy #552 gives artifacts a sha256 SRI digest in the listing, so the fetched bytes can be verified end to end, but bes-canopy-api 1.0.0 has no digest field and the generated struct is #[non_exhaustive] with no flatten, so it is unreachable until the crate is republished from the epic's OpenAPI. Picking it up after merge.