ci: check that decisions.md entry pointers resolve - #14
Merged
Conversation
Three of these shipped across two PRs, all from renumbering an entry while a branch was open: a manifest comment pointing at the wrong entry (#9, caught by hand), the same again (#12, caught by Copilot), and an entry in decisions.md referring to itself (#12, caught by Copilot). Manual sweeps are not working — the audit I ran after the second one reported clean and missed the third, because it matched only the `decisions.md entry N` form that OTHER files use and was structurally blind to the bare `entry N` form used inside decisions.md. The job asserts four things, each corresponding to a failure that has actually occurred or that the same edit would produce: - entry numbers are a gap-free run from 1 — a gap is what a rebase leaves when another PR takes the number a branch was using - every entry has an index row, and every index row links to a real anchor - bare `entry N` references inside decisions.md resolve, and none points at the entry containing it — a self-reference sends the reader back where they started - `decisions.md entry N` references in every other tracked file resolve Verified against all four failure shapes rather than the happy path alone: a self-reference, a cross-file pointer at a non-existent entry, a numbering gap, and an entry missing its index row. Each fails with a message naming the file, the line and what to fix. Clean tree: 13 entries, 37 pointers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 24, 2026
hakan-persson
added a commit
that referenced
this pull request
Aug 26, 2026
Follow-up to #11. You flagged `VeleroNoRecentBackup` as the rule you'd trust least — you were right, and the weakness turned out to apply to all five. ## The defect Every rule needed its series to **exist**. `== 1`, `increase()` and `time() - metric` all return nothing when the metric is absent, so each alert went silent at the exact moment the component it watches disappeared: | Rule | Blind when | |---|---| | `ExternalSecretNotReady` | ESO is down | | `VeleroBackupFailing` / `VeleroNoRecentBackup` | Velero is down, or has never succeeded | | `PostgresWALArchivingFailing` | the CNPG instance exporter stops | | `ArgoCDAppNotSynced` | ArgoCD stops reporting — i.e. when GitOps is already broken | `VeleroNoRecentBackup` was the clearest: `time() - velero_backup_last_successful_timestamp` produces no series if Velero is uninstalled, crashed, or has simply never completed a backup. An alert that exists, parses, and cannot fire. I checked whether the chart already covered this. `TargetDown` is the generic net, but it is `warning`-severity, needs **more than 10%** of a job's targets down, and cannot fire at all if the ServiceMonitor itself is gone. ## The fix Two companions, for the cases where *absence is itself the failure*: - **`VeleroBackupMetricsAbsent`** — `absent_over_time(...[48h])`, `critical`. The 48h window is deliberate: a fresh cluster clears its first 02:00 run before this complains that nothing has ever been backed up. - **`ArgoCDMetricsAbsent`** — `absent_over_time(argocd_app_info[1h])`, `warning`. The more useful of the two, because it also catches the **scrape** breaking rather than ArgoCD breaking. That ServiceMonitor selects on labels the *upstream* ArgoCD manifest owns, which can change on an upgrade — the exact fragility you noted when approving #11. Read them as *"the rule above has gone blind"*, not as the underlying fault. They **deliberately do not suppress their siblings**: the chart's inhibit rules match on `alertname`, so a firing absence alert still lets the failure alert through if it can fire at all. Two messages, saying different things. ## Three judgement calls, flagged rather than buried **`ArgoCDMetricsAbsent` is `warning`, not `critical`,** because absence there is ambiguous — ArgoCD down, or the scrape broken. The annotation says to check the ServiceMonitor before assuming ArgoCD itself is the problem. **ESO and CNPG get no companion.** Their failures surface elsewhere (a workload breaks on the next rotation; `TargetDown` covers the endpoint), so a companion each would be noise for little signal. **The CNPG half of that is the weaker argument, and is recorded as such.** Archiving could fail while its exporter is also down. The better fix is a staleness rule on `cnpg_pg_stat_archiver_seconds_since_last_archival` — a metric the committed dashboards already use — which detects the real bad state rather than the monitoring gap. But it needs to know whether `archive_timeout` is set, or an idle database false-alarms. Left open and documented rather than guessed at. ## Verification - `promtool check rules` — 7 rules parse. - All five CI jobs pass locally, including the new `decisions-pointers` job from #14 (14 entries, 40 pointers). `install.md` §11 gains two things: a note that `VeleroBackupMetricsAbsent` is **expected to sit pending on a fresh install** until the first backup completes — so the 48h window is not mistaken for a fault — and a check that both rules loaded at all, since a `PrometheusRule` missing `release: kps` is picked up by nobody and looks identical to a healthy cluster. Also worth recording from your #11 review: the real count was **252 rules across 36 PrometheusRule objects**, not the ~158 I estimated. --------- Co-authored-by: Håkan Persson <hakan@mabadiliko.se> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Three stale entry pointers shipped across two PRs, all from renumbering an entry
while a branch was open:
aks.bicepcomment said entry 11 for what became entry 10 (#9)default.yamlcomment said entry 13 for what became entry 14 (#12)decisions.mdentry 13 referred to itself instead of entry 12 (#12)Manual sweeps are not working, and the third one shows why. The audit I ran
after the second reported "all 19 resolve" — but it matched only the
decisions.md entry Nform that other files use, so it was structurally blindto the bare
entry Nform used insidedecisions.mditself. It returned a cleanresult on an incomplete search, which is worse than not running it.
What the job asserts
Each check corresponds to a failure that has actually happened, or that the same
edit produces:
when another PR claims the number your branch was using — which is how Record why GitOps is ArgoCD, and empty the permissive default AppProject #12 ended
up needing 12/13 instead of 13/14.
entry Nreferences insidedecisions.mdresolve, and none points atthe entry containing it. A self-reference sends the reader back where they
started; that was Copilot's second finding.
decisions.md entry Nreferences in every other tracked file resolve.Verified against the failures, not just the happy path
A job that only passes proves less than one that rejects what it claims to reject.
This is a narrower relative of the
doc-refsjob written on the #3 branch anddropped before merge — that one checked file paths and
§sections, this onechecks entry numbers. Worth considering whether to bring the other back too, but
that is a separate change.