feat(checks): a check name is a category, not an instance - #480
Merged
Conversation
Backup checks spelled the backup type into the name: backup-staleness:tamanu-postgres, backup-staleness:caddy-config, and so on. A deployment backing up four things produced four checks to configure, four catalog rows, and four entries in every listing — for one condition. That defeats the point of a catalog you configure once and then use to get your eyes on things quickly. The five backup sweeps now file one check per server with the types as instances. Each instance is graded through policy on its own, against its own detail, so an operator writing a rule or a silence for one backup type still affects only that type — the reason the type was in the name in the first place. The check then settles on its most urgent instance, carries every degraded instance in its detail with that instance's own result, and names them in its message. New in issues.rs: file_check_instances, with file_check reduced to its one-instance case so nothing else changes behaviour. The catalog entry and scoped chain are read once per check rather than once per instance, so a server with a dozen backup types still costs two queries. Skipped instances (a silence matched) leave the aggregate rather than counting as healthy, and a check whose instances are all skipped is itself skipped. The observed result still spans every instance including the silenced ones: silencing changes what canopy acts on, never what it saw, which the CHK spec requires. One behaviour subtlety kept deliberately: a stale repo inventory makes the missing verdict undecidable, not resolved. Where it is stale for every type there is nothing to conclude, so an already-open finding is left alone instead of being cleared on the strength of a lagging inspector. Covered by a test, since it is easy to lose. The rule is now normative: a new Names section in the CHK spec (a name is a category; instances go in detail; how an instanced check aggregates), its application in BKJ, and the AGENTS.md bullet added alongside the severity rule. The migration collapses the stored state: per-type issue rows onto one per target (survivor is the most urgent, then most recently seen, and losers leave their incidents so emptied ones retire), per-type silences into check.type rule guards so operators keep exactly the suppression they configured rather than having it widen to every type or vanish, and per-type catalog rows onto one, preferring an operator-reviewed policy over a canopy-seeded one. restore-verification, redaction, and migration-test still carry (type, intent) in their names. They are filed from an ingest path rather than a sweep, so the filer never holds the full instance set; routing them through a shared recompute-per-server is a restructure of the restore subsystem and belongs in its own change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Stacked on #479 — review that first; this branch is based on it because both touch the same files.
Why
Backup checks spelled the backup type into the name:
backup-staleness:tamanu-postgres,backup-staleness:caddy-config,backup-staleness:tamanu-config,backup-staleness:postgres-config. A deployment backing up four things produced four checks to configure, four catalog rows, and four entries in every listing — for one condition. That defeats the point of a catalog you configure once and then use to get your eyes on things quickly.The rule
A check name is a category. Anything that varies between instances of the same condition — which backup configuration, which restore intent — goes in the check's detail, where policy rules already read it as
check.<field>.New normative sections:
CHK#names(a name is a category, instances go in detail, and how an instanced check aggregates), its application inBKJ, and theAGENTS.mdbullet that landed with #479.How it aggregates
The five backup sweeps — staleness, never, reconcile-missing, report-gap, size-mismatch — now file one check per server with the types as instances:
So an operator configures staleness once for the fleet, and where a particular type warrants different treatment, writes
check.type == '…'rather than acquiring another check.New in
issues.rs:file_check_instances, withfile_checkreduced to its one-instance case so nothing else changes behaviour. The catalog entry and scoped chain are read once per check rather than once per instance, so a server with a dozen backup types still costs two queries.Two details worth a look
Silences. A skipped instance leaves the aggregate rather than counting as healthy, and a check whose instances are all skipped is itself skipped. But the observed result still spans every instance including the silenced ones — silencing changes what canopy acts on, never what it saw, which
CHKrequires. Getting this wrong initially brokescoped_check_policies::server_silence_grades_filings_to_skipped, which is a good test.Undecidable is not resolved. A stale repo inventory makes the missing verdict undecidable. Where it is stale for every type there is nothing to conclude, so an already-open finding is left alone instead of being cleared on the strength of a lagging inspector. That is easy to lose in a refactor, so there is a test for it.
Migration
Collapses the stored state three ways:
backup-staleness:tamanu-postgresbecomes a rule guard oncheck.type, so operators keep exactly the suppression they configured instead of it widening to every type or being dropped. Verified to produce the correct if-ladder:{"if": [{"==": [{"var": "check.type"}, "tamanu-postgres"]}, "skipped"]}, with pre-existing rules preserved behind the guard.All three verified against synthetic data in a rolled-back transaction.
Deliberately not in scope
restore-verification,redaction, andmigration-teststill carry(type, intent)in their names. A follow-up is in progress.