Fix the release-signoff metric lint skipping fenced files - #38487
Fix the release-signoff metric lint skipping fenced files#38487antiguru wants to merge 2 commits into
Conversation
The extractor paired backticks sequentially across the whole document. A fenced block contains backticks of its own, so pairing across one flipped the parity of every span after it: prose was captured as code and the real code spans became the separators between matches. Three of the six files in the skill contain a fence, and in those the lint checked almost nothing. SKILL.md and compute.md contributed no names at all and sources-and-sinks.md contributed two of forty, so a rename in any of them passed CI silently, which is the one failure the lint exists to prevent. Pull fenced blocks out as whole tokens before pairing backticks in what remains, and test the wildcard skip per word rather than per token, since a fence arrives as a single token and one wildcard inside it must not exempt the block. Also expand the abbreviations that roster rows use. A row writes a family as `mz_foo_bar`, `_baz`, and a bare continuation carries no mz_ prefix, so it was never a candidate. Which prefix of the base it attaches to is ambiguous, because rows drop either one component or two, so accept the first expansion that resolves and report only when none does. Continuations are sought within a single line: carried across lines they attach to whatever name appeared in an earlier paragraph and manufacture failures. Bare histogram suffixes are excluded, being prose about the parts of a histogram rather than family members. Together these take the guard from 98 names to every file catching a rename, and they surface two roster entries that name metrics the product does not export. `mz_storage_upsert_state_rocksdb_autospill_in_use` went with the autospill upsert backend, and its row still explained how to read the percentage panel, so a verifier would conclude no worker is spilling rather than that the concept is gone. There is no `mz_persist_gc_skipped`: the already-done case is `mz_persist_gc_noop`, and the only skipped counter is `mz_persist_compaction_skipped`. Both rows now record the metric as gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
QA LLM Review1. MEDIUM -- Bare-suffix guard silently exempts a real roster continuation
The guard that treats a bare DetailsThe comment justifying the skip says a bare histogram suffix is "never a family member abbreviated in a roster row", but
The PR's verification method cannot surface this. Mutating the abbreviation in the doc ( Suggested fix: keep the guard and spell that one row out, rather than teaching the guard to tell a roster row from prose. A "table rows are never prose" refinement does not hold either, since -| `mz_persist_read_batch_part_bytes`, `_count` | counter, by `op` |
+| `mz_persist_read_batch_part_bytes`, `mz_persist_read_batch_part_count` | counter, by `op` |With that, the lint still exits 0 on a clean tree and reports 2. LOW -- First-resolving-prefix hides the removal of four abbreviated metrics
For four of the 55 continuations in the skill more than one prefix cut resolves, so the abbreviation stays green after its intended target is removed and silently re-binds to an unrelated live metric. Details
The first pair is the worse one, because Cheap tightening: accept only the longest resolving cut, and report when a shorter cut also resolves so the row gets disambiguated once. Together with finding 1 these four are the only mz-namespace roster entries left unprotected after this change, so pinning them costs little. |
An abbreviated roster entry such as `mz_foo_bar`, `_baz` resolves by trying each prefix of the base, since the row does not say how many components the abbreviation drops. Four entries resolve against more than one prefix. The longest cut is the intended metric and is the one accepted, so nothing binds wrongly today, but deleting that metric leaves a shorter cut resolving, and the lint stays green on a row that points at nothing. Report an abbreviation that resolves more than one way, and spell those four out in full. Also spell out `mz_persist_read_batch_part_count`, which the bare-suffix guard was skipping as prose about a histogram. It is a separately registered counter, so its removal was undetectable. Record the two remaining bounds on what a green run proves: a name covered by a glob catalog entry cannot be falsified, and a bare `_count`, `_sum`, or `_bucket` is read as prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Motivation
A post-merge review of #38355 found that the metric lint added there checks almost nothing in half the files it covers, and that the gap hid two roster entries naming metrics the product does not export.
Description
The extractor paired backticks sequentially across the whole document. A fenced block contains backticks of its own, so pairing across one flips the parity of every span after it: prose is captured as code and the real code spans become the separators between matches. Three of the six files in the skill contain a fence.
SKILL.mdcompute.mdsources-and-sinks.mdadapter.mdpersist.mdreference-dashboards.mdA rename in any fenced file passed CI silently, which is the one failure the lint exists to prevent. Fenced blocks are now pulled out as whole tokens before backticks are paired, and the wildcard skip is tested per word rather than per token, since a fence arrives as a single token and one wildcard inside it must not exempt the block.
The lint also now expands the abbreviations that roster rows use. A row writes a family as
`mz_foo_bar`, `_baz`, and a bare continuation carries nomz_prefix, so it was never a candidate. Which prefix of the base it attaches to is ambiguous, because rows drop either one component or two, so the first expansion that resolves is accepted and only a continuation with no resolving expansion is reported. Continuations are sought within a single line, since carried across lines they attach to whatever name appeared in an earlier paragraph and manufacture failures rather than finding them. Bare histogram suffixes are excluded, being prose about the parts of a histogram rather than family members.Two dead roster entries
Both were sitting in roster tables as live metrics with reading instructions, which is worse than being absent, because the skill itself names an empty panel as the most dangerous reading on any dashboard.
mz_storage_upsert_state_rocksdb_autospill_in_usewent with the autospill upsert backend. It appears nowhere insrc/, indoc/user/data/metrics.yml, or in Prometheus, and the surviving members of the family are_rehydration_latency,_rehydration_total, and_rehydration_updates. Its row still explained that the percentage panel dividessumbycount, so a verifier following it would conclude no worker is spilling rather than that the concept is gone.mz_persist_gc_skipped. The already-done case ismz_persist_gc_noop, whose help text reads "count of garbage collections skipped because they were already done", and the only skipped counter ismz_persist_compaction_skipped.Both rows now record the metric as gone. Names that the references document as dead on purpose, such as the deliberate
orhedge across the storage controller protocol rename, moved into the allowlist with their reason.Verification
Every file in the skill now fails the lint when one of its metric names is mutated, including the three fenced ones, and the continuation path fails when a roster abbreviation is mutated. The stale-allowlist check still fires when an exemption starts resolving.
🤖 Generated with Claude Code