feat(metrics): expose plugin-contributed metrics to Prometheus (PEN-2799) by allyblockcast[bot] · Pull Request #1605 · Blockcast/paperclip

allyblockcast · 2026-09-02T12:55:08Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work

Plugins are how that fleet is instrumented and automated, and ctx.metrics.write(name, value, tags) is the documented way for a plugin to emit a metric

Traced through packages/plugins/sdk to its host implementation, metrics.write pushed a plugin_logs row at level='metric' and did nothing else — a database write, not a Prometheus series, and no exporter turns those rows into one

So no alert rule could fire on any plugin-emitted metric, ever. Measured, not theoretical: during PEN-2581 the alertmanager plugin emitted alertmanager.owner.fallback_failed on every failed delivery for 89h while ~93% of fleet alert delivery was lost — and the series did not exist. The alert that did exist routes on paperclip_plugin_error, which is plugin lifecycle status; the plugin was lifecycle-healthy the whole time, reading 0. It was failing per-alert

PEN-2579 fixed "paperclip_plugin_error doesn't exist". This is the larger, separate gap: every plugin's own instrumentation is invisible to alerting

This pull request makes ctx.metrics.write publish a real Prometheus counter alongside the existing plugin_logs row, with the plugin's metric name as a label and cardinality bounded in three layers

The benefit is that a plugin's own failure counter becomes alertable, so the PEN-2581 fault class raises an alert next time instead of being invisible for days

Linked Issues or Issue Description

Refs PEN-2799 (Penstock board). Split out of PEN-2581, where this is the reason two multi-day total-loss faults were undetectable.

No GitHub issue exists, so per path (B) — the underlying defect:

Bug. ctx.metrics.write claims to emit a metric but only writes a DB row.

Expected: a plugin metric is queryable in Prometheus and usable in an alert rule.
Actual: paperclip_plugin_* contains only paperclip_plugin_error (a separate lifecycle collector). Querying live Prometheus for any plugin-emitted metric name returns nothing.
Impact: no alert rule can fire on any plugin metric. Directly caused PEN-2581 to go undetected twice.

Status: complete — both commits pushed

This PR was opened deliberately incomplete, as a draft, because the previous attempt at this work was finished and verified and then died before git push — branch and code destroyed with an ephemeral workspace, only a PR-body draft surviving (recovered and preserved as the implementation-spec document on PEN-2799). Pushing the first coherent commit before continuing was a direct response to that failure mode.

03f7c062 — the server/src/services/metrics.ts half: both counters, recordPluginMetric(), the three-layer bound, registry / guard-chain / reset wiring.
ff01de5f — host wiring, the metricLabels manifest field, the alertmanager declaration, PLUGIN_SPEC.md §26.4, and 36 tests.

Worth stating because it was true of the first commit alone and is no longer: layer 2 of the cardinality bound was inert until the manifest field landed. With no declared labels the promotion gate promotes nothing, so 03f7c062 on its own gave every plugin aggregate-only series. Both commits are needed; do not merge only the first.

What Changed

server/src/services/metrics.ts — added paperclip_plugin_metric_total (counter) and paperclip_plugin_metric_dropped_total, plus recordPluginMetric(), which owns validation and the cardinality budget. Both registered in ensureRegistry (including its || guard chain) and cleared by __resetMetricsForTest, along with the budget ledger.
Metric name is a label, never part of the series name. Rule authors cannot enumerate plugin metric names ahead of time, and it is already attacker-shaped: two bundled call sites build the name by interpolation (demo.${name} in kitchen-sink, slack.tool.${name}.error in Slack), so name-mapping would let any installed plugin mint arbitrary paperclip_* series in the platform's own namespace.
company_id is deliberately NOT a label — unbounded per tenant. It stays on the plugin_logs row.
Counter semantics. write is a counter increment; finite and >= 0 only. Rejected rather than clamped — clamping publishes a number the plugin never submitted, and a silently-altered counter is worse than a counted drop. A gauge, if ever needed, gets its own SDK method and series family rather than a flag on write, which would silently change the meaning of existing series on a plugin recompile.
Three-layer cardinality bound (only the third is load-bearing):
1. name shape ^[a-z][a-z0-9_]*(\.[a-z0-9_]+)*$, ≤64 chars → else reason="bad_name";
2. a tag key becomes a label only if it is in both the manifest's metricLabels and the platform PLUGIN_METRIC_PROMOTABLE_TAG_KEYS allow-list — other keys are dropped from the label set but not from the metric;
3. hard budget of 100 distinct (metric, label-values) combinations per plugin per process lifetime, past which combinations collapse into one metric="_overflow" series counted under reason="budget" — never silently discarded.
Drops are never silent. paperclip_plugin_metric_dropped_total{reason} is the series that answers "why is my plugin metric missing", which otherwise required reading host source.

Design deviation from the ticket, and why

The ticket proposed manifest-declared tag keys becoming labels, full stop. That cannot work alone: prom-client fixes a counter's labelNames at construction and throws on any label it was not built with, and the manifest is read per write, long after the counter exists. Hence the two-sided gate — the manifest chooses which keys a plugin promotes; the platform allow-list bounds what any plugin may ever promote.

The allow-list is seeded from tag keys measured in use across the bundled plugins (source ×7, event_type ×5, decision ×5, severity ×4, error_code ×4, action ×3, alertname ×2, then version/scope/trigger/exit_code singletons), each a closed vocabulary. Deliberately excluded as unbounded in principle: command/command_name (operator-defined), turns/threshold (numeric measurements, not categories), by (actor identity), mimetype (plugin-supplied and effectively open). Their metrics still publish, just without those labels. Adding a key is an explicit cardinality decision.

The ledger separator is load-bearing, and this is why

The budget ledger key is NUL-joined, matching this file's existing composite-key idiom (${errorCode}\x00${scope}). With a printable separator, two distinct combinations can render to the same ledger key — the second write then reads as already-seen, so it consumes no budget slot and still publishes. Every collision buys a free series and the bound leaks. (Prom-client keys a series by the real label map, so series identity is never forged; the leak is in the ledger, not the registry. An earlier version of the accompanying test asserted the forging story and passed with the separator mutated to a space — i.e. proved nothing. Called out here because the correct claim is narrower than the intuitive one.)

Verification

# NOTE: build these two first, or `server` typechecking emits ~145
# TS6305/TS2307 errors that are pre-existing build-order noise, not this change.
pnpm --filter @paperclipai/shared build
pnpm --filter @paperclipai/plugin-sdk build

cd server && NODE_ENV=development ../node_modules/.bin/vitest run \
  src/__tests__/plugin-metric-exposition.test.ts \
  src/__tests__/metrics-service.test.ts \
  src/__tests__/plugin-status-metrics.test.ts
#   Test Files  3 passed (3)   Tests  110 passed (110)

cd packages/shared && NODE_ENV=development ../../node_modules/.bin/vitest run src/validators/plugin.test.ts
#   Tests  30 passed (30)   (15 pre-existing + 15 new)

cd packages/plugins/paperclip-plugin-alertmanager && pnpm exec vitest run
#   Test Files  9 passed (9)   Tests  282 passed (282)   (manifest change is inert to them)

# typechecks, all three touched packages
tsc -p packages/shared/tsconfig.json --noEmit                     # clean
cd server && tsc -p tsconfig.json --noEmit                        # clean
cd packages/plugins/paperclip-plugin-alertmanager && tsc -p tsconfig.json --noEmit  # clean

The exposition tests assert against the rendered Prometheus text (renderMetrics()), not internal bookkeeping. That is the point: the defect being fixed is that a write "succeeded" while producing no series, so a test inspecting our own state could pass with exposition still broken.

Every guard was mutation-checked, not merely observed green. A passing test proves nothing until you show it is carried by the control it names:

mutation	result
budget check `>=` → `>` (off-by-one)	2 fail — the exact-series-count assertion is sensitive to one extra
name regex disabled	4 fail — every `bad_name` case
manifest-declared gate disabled	2 fail — undeclared-key + declared-cap
ledger separator NUL → space	1 fail — the injectivity test, specifically
`metricLabels` schema → `z.array(z.string())`	10 fail — shape, cap, and duplicate cases
all restored	21/21 server + 30/30 shared

One process note, since it nearly cost me the separator row: my first attempt at that mutation was applied with a perl substitution whose pattern did not match, so the mutation was a no-op and the suite "passed" — a false clear that looks identical to a robust test. Re-applied with an assert that the pattern was found before writing. A mutation you did not verify landed is not a mutation test.

Explicitly not verified, and not verifiable here: that the series appears in production Prometheus. That needs a deploy. The worker tier is already a scrape target (paperclip_plugin_error is served from paperclip-0 today), so the exposition path is proven by an existing series, but end-to-end is a post-deploy check.

Risks

Low risk on the write path. recordPluginMetric never throws — it runs inside plugin worker calls, and prom-client's inc() does throw on a negative value. A throw here would escalate "mis-shaped metric" into "dropped alert", the exact failure class this PR exists to detect.
Purely additive. The plugin_logs write is unchanged, metrics.write's signature is unchanged, and there is no SDK/protocol change — so no plugin needs recompiling and existing counters become visible with zero plugin-side work.
Cardinality ceiling ~11 installed plugins × 101 = ~1111 series worst case, against 11 today for paperclip_plugin_error. Bounded over combinations ever observed rather than currently active, because prom-client never retires a label combination — bounding "active" would bound nothing.
The budget is per process: a worker restart clears it. Correct for a counter, but a plugin churning names gets a fresh 100 each restart. Accepted; persisting the ledger is not worth a DB write per metric.
No migration, no schema change, no auth/authorization change.

Model Used

Claude Opus (claude-opus-5[1m], Anthropic; extended thinking; tool use via Claude Code), driving gh, git, tsc and vitest directly in-run.

Checklist

Follow-up, deliberately NOT in this PR

The alert rule belongs in Blockcast/onprem-k8s:

sum by (plugin_key, metric) (
  rate(paperclip_plugin_metric_total{plugin_key="paperclip-plugin-alertmanager",
                                     metric="alertmanager.owner.fallback_failed"}[10m])
) > 0

for: 10m, severity critical, and it must not route through the alertmanager plugin's own receiver — that is PEN-2590's finding, and this alert reports the failure of the path it would otherwise travel. Filed separately once the series exists in production: a rule on a non-existent series is inert, which is exactly the PEN-2579 failure mode.

…799) `ctx.metrics.write` wrote a `plugin_logs` row and nothing else, so no alert rule could fire on any plugin-emitted metric — ever. That is not theoretical: `paperclip-plugin-alertmanager` emitted `alertmanager.owner.fallback_failed` on every failed delivery for 89 hours while ~93% of fleet alert delivery was lost (PEN-2581), and the series did not exist. The metric naming the root cause was published into a channel nothing can observe. Adds `paperclip_plugin_metric_total` and `paperclip_plugin_metric_dropped_total` plus `recordPluginMetric()`, which owns validation and the cardinality budget. This commit is the metrics-service half only; the host wiring follows. - The plugin's metric name is a LABEL, never part of the series name. Two bundled call sites already build names by interpolation (`demo.${name}`, `slack.tool.${name}.error`), so name-mapping would let any installed plugin mint arbitrary `paperclip_*` series in the platform's namespace — and rule authors cannot enumerate plugin metric names ahead of time anyway. - `company_id` is deliberately not a label (unbounded per tenant); it stays on the `plugin_logs` row. - Counter semantics: finite and >= 0. Rejected rather than clamped, because clamping publishes a number the plugin never submitted. - Three-layer cardinality bound, only the third load-bearing: name shape; a two-sided label gate (manifest `metricLabels` AND the platform `PLUGIN_METRIC_PROMOTABLE_TAG_KEYS` allow-list); and a hard budget of 100 combinations per plugin per process, past which writes collapse into `metric="_overflow"` rather than disappearing. - Two sides are required because prom-client fixes `labelNames` at counter construction and throws on any label it was not built with, while a manifest is read per-write. The allow-list is seeded from tag keys measured in use across the bundled plugins; `command`/`command_name`, `turns`/`threshold`, `by` and `mimetype` are excluded as unbounded in principle. - The ledger key is NUL-joined, matching this file's existing composite-key idiom. With a printable separator two distinct combinations can render to one key; the second write then reads as already-seen, consumes no budget slot and still publishes, so every collision buys a free series and the bound leaks. - `recordPluginMetric` never throws. It runs inside plugin webhook/alert processing and prom-client's `inc()` does throw on a negative value; an escaping exception would turn "a plugin submitted a mis-shaped metric" into "the delivery carrying it failed" — the exact dropped-alert class this exists to make visible. Every rejection is a counted drop instead. Refs: PEN-2799 Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-02T12:55:14Z

🔗 Paperclip issue: PEN-2799
🔗 Paperclip issue: PEN-2579
🔗 Paperclip issue: PEN-2590
🔗 Paperclip issue: PEN-2581

allyblockcast · 2026-09-02T12:56:14Z

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

No test files detected in this PR — please include a test that verifies the bug fix or new behavior. If this PR genuinely doesn't need a test (e.g. a refactor), please retitle with refactor: prefix.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

…sts (PEN-2799) Completes the change begun in 03f7c06, which added the metrics-service half. Layer 2 of the cardinality bound was inert until the manifest field existed; it is live now. - `plugin-host-services.ts`: `ctx.metrics.write` now calls `recordPluginMetric` BEFORE the `_logBuffer.push`. Ordered deliberately — the buffer write can fail or be lost on a flush error, and the exposition path is the one an alert rule depends on. It passes the UNtruncated name so an over-long name is counted as an explicit `bad_name` drop; handing over the pre-truncated string would let a too-long name masquerade as a valid shorter one and occupy a real series. - `packages/shared`: new optional manifest field `metricLabels` (max 5, snake_case, no duplicates). A key outside the host allow-list stays VALID and simply promotes nothing, so a third-party manifest cannot fail installation over a host-side cardinality decision. Duplicates are rejected because one would silently consume a slot. - `paperclip-plugin-alertmanager`: declares `["alertname", "severity", "version"]`. `alertname` is the load-bearing one — it separates "one rule cannot be owned" from "delivery is broken", the distinction that cost 89h on PEN-2581, and it is bounded by rule count rather than traffic. - `PLUGIN_SPEC.md` §26.4 documents exposition, the two-sided gate, counter semantics, the drop reasons, and that an unpromoted key loses its label but not its increment. Tests: 21 new in `plugin-metric-exposition.test.ts`, asserted against rendered exposition text rather than internal bookkeeping — the defect being fixed was that a write "succeeded" while producing no series, so a test reading our own state could pass with exposition still broken. 15 new manifest-validator cases. Every guard was mutation-checked rather than merely observed green: | mutation | result | |-----------------------------------|-------------------------------| | budget `>=` -> `>` | 2 fail (exact-count assertion)| | name regex disabled | 4 fail | | manifest-declared gate disabled | 2 fail | | ledger separator NUL -> space | 1 fail (injectivity test) | | metricLabels schema -> z.string[] | 10 fail | | all restored | 21/21 + 30/30 pass | The separator row is the subtle one and it caught a real hazard: with a printable separator two distinct combinations render to one ledger key, the second reads as already-seen, and it therefore consumes no budget slot while still publishing — so every collision buys a free series and the bound leaks. The test fills the budget to its last slot to make that observable, because series identity itself is never forged (prom-client keys on the real label map), and an earlier version of this test asserted the forging story and passed with the separator mutated. Verified: 110/110 across plugin-metric-exposition + metrics-service + plugin-status-metrics; 30/30 shared validators; 282/282 alertmanager plugin; typecheck clean for shared, server, and the plugin. Not verified and not verifiable here: that the series appears in PRODUCTION Prometheus. That needs a deploy. The worker tier is already a scrape target (`paperclip_plugin_error` is served from paperclip-0 today), so the exposition path is proven by an existing series, but end-to-end is a post-deploy check. Refs: PEN-2799 Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-02T13:33:26Z

Housekeeping for whoever reviews this: the commitperclip "No test files detected" comment above is stale and already resolved. Do not bounce the PR on it.

It was written against the first commit only. Timeline from the API:

	time (UTC)
`03f7c062` — `services/metrics.ts` half, no tests	12:53:37
commitperclip comment "no test files detected"	12:56:14
`ff01de5f` — host wiring, manifest field, tests	13:09:41
`commitperclip PR Review` re-run on `ff01de5f`	13:10:06 → success

The bot said "push a new commit and these checks will re-run automatically", and that is what happened — its own check run (review) is green on this head. It just does not edit or delete the superseded comment, so the comment outlives the finding.

Tests now in the diff:

server/src/__tests__/plugin-metric-exposition.test.ts — new file, 322 lines, 21 cases, asserted against the rendered exposition text from renderMetrics() rather than internal bookkeeping. That choice is the point: the defect being fixed is that a write "succeeded" while producing no series, so a test inspecting our own state could pass with exposition still broken.
packages/shared/src/validators/plugin.test.ts — +73 lines, 15 new cases on the metricLabels manifest field.

Both were mutation-checked rather than merely observed green; the per-guard table is in the PR body.

Current CI state on ff01de5f: 14 green, 5 still in flight (General tests (server 1/4 … 4/4), e2e), zero red. mergeStateStatus is BEHIND — I am deliberately not pushing a merge/rebase commit to fix that, because a new head re-arms the review gate and discards the CI already banked on this SHA. Happy to update the branch on request, or it can be handled at merge time.

I am not merging this myself.

github-actions · 2026-09-02T15:24:59Z

@ally head ff01de5 has been awaiting review for 2.3h with no review on either surface (pulls/1605/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head ff01de5.

…verflow (PEN-2799) Review of this PR's own first cut. The cardinality bound collapsed the `metric` label into "_overflow" once a plugin exceeded 100 combinations -- degrading the exact axis an alert rule filters on. rate(paperclip_plugin_metric_total{metric="alertmanager.alert.error"}[10m]) > 0 stops matching the moment that happens, silently. That is a narrower re-run of the PEN-2579 failure mode (a rule watching a series that is not reliably there), reintroduced by the bound meant to prevent it -- and it defeats this ticket's second done-when item. It is not a tail case. Measured against live Prometheus: 155 distinct alertnames fired fleet-wide over 7 days (215 including pending), against 16 alertmanager metric names that carry `alertname`. Exhaustion lands somewhere around 25-100 distinct alertnames, i.e. the expected steady state within days of a worker start -- and which combinations survive is decided by arrival order, so whether the alert works was a race re-run at every worker restart. Split the single budget into two tiers that degrade on different axes: - PLUGIN_METRIC_NAME_BUDGET (50) bounds distinct metric NAMES. Overflow still collapses to metric="_overflow", preserving the protection against interpolated name churn (`slack.tool.${name}.error`) that made the name a label in the first place. Reported as reason="name_budget". - PLUGIN_METRIC_CARDINALITY_BUDGET (100) bounds full combinations. Overflow now KEEPS `metric` and drops the promoted labels, so the increment lands on the metric's own series. Reported as reason="label_budget". Because a label-dropped write lands on the same series as a no-tag write of that metric, `sum by (metric)` stays exactly correct across overflow -- only the per-tag breakdown is lost. The unbounded axis (`alertname`, derived from alert labels) degrades; the bounded, alertable one survives. Worst case per plugin is 50 + 100 + 1 = 151 series. Tests: +2 cases (110 -> 112 across the three metrics suites, baseline measured at HEAD by stashing). The new guard asserts the `metric` label survives label-budget overflow and that sum-by-metric is exact across it -- the assertion whose absence let this through. Mutation-checked: restoring the old collapse target fails it. Refs PEN-2799. Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-02T16:24:36Z

⛔ Self-review found a defect in this PR — pushed a fix. New head `06c3baa`.

I re-verified this PR's second done-when item statically instead of waiting on the merge decision, and the verification failed. Recording it here because the defect is subtle and the fix changes observable behaviour.

The defect

The cardinality bound collapsed the metric label into "_overflow" once a plugin exceeded 100 combinations. metric is the label an alert rule filters on:

rate(paperclip_plugin_metric_total{plugin_key="paperclip-plugin-alertmanager",
                                   metric="alertmanager.alert.error"}[10m]) > 0

Over budget, that rule silently stops matching. It is a narrower re-run of the PEN-2579 failure mode — a rule watching a series that is not reliably there — reintroduced by the bound that was supposed to prevent it.

It was the expected steady state, not a tail case

Measured against live Prometheus:

query	value
`count(count by (alertname) (ALERTS))`	41 active
`count(count by (alertname) (max_over_time(ALERTS[7d])))`	215
firing-only, 7d	155

The alertmanager plugin carries alertname on 16 distinct metric names, so exhaustion lands around 25–100 distinct alertnames against 155 observed in a week. Which combinations survived was decided by arrival order, making the alert's validity a race re-run at every worker restart.

The fix — two tiers degrading on different axes

PLUGIN_METRIC_NAME_BUDGET (50) bounds distinct metric names. Overflow still collapses to metric="_overflow", preserving the protection against interpolated name churn (slack.tool.${name}.error) that made the name a label in the first place. Reason name_budget.
PLUGIN_METRIC_CARDINALITY_BUDGET (100) bounds full combinations. Overflow now keeps metric and drops the promoted labels. Reason label_budget.

Because a label-dropped write lands on the same series as a no-tag write of that metric, sum by (metric) stays exactly correct across overflow — only the per-tag breakdown is lost. The unbounded axis (alertname, derived from alert labels) degrades; the bounded, alertable one survives. Worst case per plugin is 50 + 100 + 1 = 151 series.

Verification

3 metrics suites: 112 passed. Baseline measured at the old head by stashing: 110 — so exactly the +2 cases added, not a coincidence of counts. (The 115 quoted in my earlier design note was from an unrelated tree and was wrong; that note warned it needed re-measuring, and it did.)
tsc --noEmit on server: exit 0.
Mutation-checked. Restoring the old collapse target fails 3 tests including both new guards, and the key one fails on exactly the right assertion — an _overflow series appearing where none should. Reverted and re-confirmed green.

Why the original suite could not catch this: it was mutation-checked too, and that held up — but mutation-checking only proves the assertions you wrote have teeth. It cannot invent the assertion you never made, and nothing asked what survives the collapse. That assertion now exists.

Unchanged

plugin_logs write, metrics.write signature, the SDK, the manifest schema, the two-sided label gate, and recordPluginMetric's never-throws contract. No migration. Blast radius is 3 files; the only other caller is plugin-host-services.ts, whose call site is untouched.

github-actions · 2026-09-02T18:52:46Z

@ally head 06c3baa has been awaiting review for 2.5h with no review on either surface (pulls/1605/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 06c3baa.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 06c3baa

The core design is sound and unusually well argued. Two Important findings, both about the seams rather than the algorithm: the production wiring is untested, and the one input axis that comes from outside the trust boundary is the one axis left unbounded.

Critical Issues (0)

Important Issues (2)

[pr-review-toolkit: tests] server/src/services/plugin-host-services.ts:1617 — the host wiring is not covered by any test, so the defect this PR fixes could be fully reintroduced with the suite green.
- recordPluginMetric appears in exactly two files: its definition and plugin-host-services.ts. The 388-line plugin-metric-exposition.test.ts calls recordPluginMetric directly and never constructs host services; no test in server/src/__tests__ calls services.metrics.write at all (checked all 8 files that call buildHostServices). Delete the recordPluginMetric({...}) block from metrics.write and every test still passes, while ctx.metrics.write goes back to writing only a plugin_logs row — the exact PEN-2581 condition.
- declaredLabels: options.manifest?.metricLabels ?? null is likewise unexercised end-to-end. options.manifest is optional, and all 20 existing buildHostServices call sites in tests omit it; only server/src/app.ts:723 passes it. The PR body already identifies "layer 2 was inert until the manifest field landed" as the trap, and nothing currently guards against it going inert again.
- The PR argues, correctly, that the exposition tests assert rendered text "because a test inspecting our own state could pass with exposition still broken." The same argument applies one layer up: a test of recordPluginMetric passes with the call site removed.
- Recommendation: one test that builds host services with a manifest carrying metricLabels: ["alertname"], calls services.metrics.write({ name, value, tags: { alertname: "X" } }), and asserts the rendered paperclip_plugin_metric_total line carries metric="…" and alertname="X". That single test pins the call site, the untruncated-name decision, and the manifest plumbing simultaneously.
[gstack/review: trust boundary] server/src/services/metrics.ts (the for (const key of PLUGIN_METRIC_PROMOTABLE_TAG_KEYS) loop in recordPluginMetric) — promoted label values have no length bound, and they are the externally-sourced side.
- The metric name is bounded at PLUGIN_METRIC_NAME_MAX_LENGTH (64) with the stated rationale that "rejecting a mis-shaped name is cheaper than carrying it as a label value forever, because prom-client never retires a label combination." const promoted = raw === undefined || raw === null ? "" : String(raw) applies no such bound — and alertname, the load-bearing promoted key, is alert.labels.alertname ?? "UnnamedAlert" (packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1139), i.e. verbatim from the inbound Alertmanager webhook.
- The count is bounded (≤ PLUGIN_METRIC_CARDINALITY_BUDGET = 100 combinations/plugin), so the practical ceiling is ~100 retained values at whatever length survives the express body limit — meaningful bloat re-serialized on every scrape and never retired, rather than a crash. Exposition itself is safe: prom-client escapes \, \n and " in label values, so there is no format-injection concern here.
- Recommendation: truncate promoted to a small bound (say 128 chars) before it enters labels/comboParts. One line, and it makes the value axis consistent with the name axis the module already defends. String(raw) on a non-primitive also yields "[object Object]" — the "never throws" test feeds exactly that case; a truncation site is the natural place to reject or flatten it instead.

Suggestions (2)

[native-codex] server/src/services/metrics.ts — paperclip_plugin_metric_dropped_total carries only {plugin_id, plugin_key, reason}, so it cannot say which metric was dropped. §26.4 promises it answers "why is my plugin metric missing … so the answer never requires reading host source", which overstates what the series delivers for a plugin emitting more than one name. For reason="label_budget" and reason="name_budget" the metric label is already bounded by tier 1 (≤50 names + _overflow), so adding metric to those two increments is cardinality-safe and makes the series genuinely actionable. bad_name cannot safely carry the name (that is the unbounded thing being guarded) — a rate-limited logger.warn would cover it, since today the only trace is the logger.debug at plugin-host-services.ts:1615, invisible at normal log levels.
[pr-review-toolkit: code] server/src/services/metrics.ts — pluginMetricCombinations and pluginMetricNames are keyed by pluginId and never pruned when a plugin is uninstalled or disabled; entries persist for the process lifetime. Bounded at ~150 strings per plugin so this is tidiness rather than a leak, but a small reset hook alongside the existing dispose() path would keep the ledger tracking reality.

Strengths

The two-tier bound degrading on different axes is the right call, and the reasoning is preserved where it will be read. Keeping metric (the axis alert rules filter on) and degrading promoted tag values means sum by (metric) stays exact across overflow — and the header comment explains why the intuitive design would have re-run the PEN-2579 failure mode. The self-review that caught this before merge is the substantive kind.
Tests assert against rendered exposition text rather than internal bookkeeping, which is the correct choice for a defect whose signature was "the write succeeded and no series existed."
The injective-ledger test earns its keep: it fills the budget to its last slot before the collision probe, so it genuinely fails under a printable separator. The PR body's admission that an earlier version of it was vacuous, and the narrower correct claim that replaced it, is the kind of accuracy that makes the rest of the description trustworthy.
PLUGIN_METRIC_OVERFLOW_NAME = "_overflow" cannot collide with a real plugin metric name, because PLUGIN_METRIC_NAME_REGEX requires a leading [a-z]. Quiet but load-bearing.
New code uses the "\0" escape rather than the literal NUL byte this file already carries at lines 2788 and 2801 — same semantics, and it does not make the file read as binary to grep.
metricLabels is optional on a non-.strict() zod schema, so a plugin declaring it still installs on an older host that strips the field. Verified against pluginManifestV1Schema.
Passing the untruncated name to recordPluginMetric while the log path keeps its truncated copy is subtle and correct — the comment explaining why is worth the four lines it takes.
recordPluginMetric wrapping ensureRegistry() inside its own try/catch means a registry construction failure also cannot escalate into a failed plugin call. The "never throws" contract holds on every path I traced.

Recommended Action

Fix Critical issues before merge. (None.)
Address Important issues this cycle: add the host-path test that pins the metrics.write → recordPluginMetric call site, and bound the promoted label-value length.
Consider Suggestions opportunistically.

…(PEN-2799) Addresses both Important findings from Ally's review of 06c3baa. 1. The host wiring was untested. `recordPluginMetric` appeared only in its own definition and `plugin-host-services.ts`; the exposition suite called it directly and never built host services, so deleting the call site from `metrics.write` left every test green while restoring the exact PEN-2581 condition. `plugin-metric-host-path.test.ts` drives the real path (`buildHostServices` -> `services.metrics.write`) and asserts rendered exposition text. Verified by mutation, not by the tests merely passing: - remove the `recordPluginMetric({...})` call site -> 3 of 3 fail - `declaredLabels: options.manifest?.metricLabels` -> null -> 1 of 3 fails The over-long-name case asserts the `bad_name` drop counter rather than only the absence of a series, because an absence assertion is also satisfied by the call site being deleted -- it would have passed for the wrong reason under the first mutation. 2. Promoted label values were unbounded on the axis that crosses the trust boundary: `alertname` is taken verbatim from the inbound Alertmanager webhook, while the metric name (plugin source) was already bounded at 64. Values are now truncated to PLUGIN_METRIC_LABEL_VALUE_MAX_LENGTH (128). Truncation happens before the ledger key is built, so two values sharing a truncated prefix render as one series and consume one budget slot rather than two. Non-primitive tags are treated as absent instead of promoting the constant "[object Object]", which identified nothing and was indistinguishable from a real value to anyone reading the series. Also adopts the `metric` label on `paperclip_plugin_metric_dropped_total` (review Suggestion 1), with one correction: the review proposed labelling both budget reasons with the real name, but on the `name_budget` branch the name is by definition the 51st-or-later distinct one -- the unbounded input that tier exists to refuse. Labelling it there would leak the bound onto the drop series. That branch carries `_overflow`, matching the series the increment lands on; `label_budget` carries the real name, which is safe because it already cleared tier 1. `bad_name` and `bad_value` leave the label empty for the same reason. Declined: pruning the per-plugin ledgers on uninstall (Suggestion 2). Both budgets are deliberately measured over values *ever observed*, so a reset hook would let a plugin clear its own cardinality bound by cycling disable/enable. The ~150 strings per plugin are the price of that property. Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-02T20:19:22Z

Response to consolidated review of `06c3baa`

Both Important findings were correct and are fixed in 9eea814. One Suggestion adopted with a correction, one declined with reasoning.

Important 1 — untested host wiring ✅ fixed

Confirmed exactly as described: recordPluginMetric appeared in two files, no test in server/src/__tests__ called services.metrics.write, and the call site was deletable with the suite green.

New file server/src/__tests__/plugin-metric-host-path.test.ts drives buildHostServices(...) → services.metrics.write(...) and asserts rendered exposition text.

I did not take "the tests pass" as evidence that it pins anything — I ran the two mutations the finding implies:

Mutation	Result
Delete the `recordPluginMetric({...})` block from `metrics.write`	3 of 3 fail
`declaredLabels: options.manifest?.metricLabels ?? null` → `null`	1 of 3 fails

That first run also caught a defect in my own test. The over-long-name case originally asserted only expect(seriesFor(TOTAL)).toHaveLength(0) — and an absence assertion is satisfied by the call site being deleted outright, so it passed under mutation 1 for the wrong reason. It now asserts the bad_name drop counter, which requires the call site to have run. All three fail under mutation 1 as a result.

Important 2 — unbounded promoted label values ✅ fixed

Correct, and the asymmetry you point at was the tell: the bounded axis was the one we control (metric name = plugin source) and the unbounded one was the one we don't (alertname = verbatim from the inbound webhook).

Values are truncated to PLUGIN_METRIC_LABEL_VALUE_MAX_LENGTH = 128 before entering labels/comboParts. Two details worth flagging:

Truncation happens before the ledger key. If it keyed on the untruncated value, two values that render to one series would each consume a budget slot — a plugin could exhaust its own budget with values indistinguishable in the exposition. There's a test for this (two values sharing a 128-char prefix → one series, value 2, no drop).
String(raw) on a non-primitive now yields an absent label, not "[object Object]". You noted the truncation site was the natural place to reject or flatten; I took reject. That constant identified nothing and was indistinguishable from a real value to a rule author, which is worse than the label simply not being there.

128 is measured, not picked: the longest alertname firing fleet-wide right now is 59 chars (PhysicalInfraTalosUnballoonedVmGuestMemoryNearConfiguredMax).

Suggestion 1 — `metric` on the drop counter ✅ adopted, with one correction

Adopted, but name_budget cannot carry the real name — and I think the review has this backwards.

The claim was that for both budget reasons metric is "already bounded by tier 1 (≤50 names + _overflow)". That holds for label_budget: the name cleared tier 1, so it is one of at most 50. It does not hold for name_budget — that branch is reached precisely because the name did not clear tier 1. It is the 51st-or-later distinct name, i.e. exactly the unbounded input the tier exists to refuse. Labelling the drop with it would move the leak from the total counter onto the drop counter, one series to the left of where the bound was placed.

So name_budget carries _overflow, matching the series the increment actually lands on. label_budget carries the real name. bad_name and bad_value leave it empty — a rejected name must never become a label value, which is the same rule.

No compatibility cost to the wider label set: paperclip_plugin_metric_dropped_total returns total_count: 0 in prod (this PR is what creates it), so there is no existing series or rule to break.

I did not do the rate-limited logger.warn for bad_name. It's a fair gap, but it's a new log-volume decision on a hot path and I'd rather not fold it into a review-response commit — happy to file it if you want it tracked.

Suggestion 2 — prune the ledgers on uninstall ❌ declined

Declining this one on a correctness argument rather than cost.

Both budgets are deliberately measured over values ever observed, not currently active — the PLUGIN_METRIC_CARDINALITY_BUDGET comment says so explicitly. A reset hook on dispose() would make the bound resettable by the thing it bounds: a plugin could clear its own cardinality ledger by cycling disable → enable, and re-mint another 50 names / 100 combinations each time. Since prom-client never retires the label combinations already registered, the actual series count would keep growing while the ledger claimed it hadn't.

The ~150 strings per plugin are the price of the ledger being a durable record rather than a live one. Agreed it's untidy; I think untidy is the correct trade here.

Verification

plugin-metric-exposition + plugin-metric-host-path + metrics-service + metrics-ingest-route: 129 passed
tsc --noEmit over server/: 0 errors under server/src (remaining errors in the run are adapter packages missing node_modules in my worktree, an artifact of how I linked deps — not from this branch)
Mutation evidence as tabled above

Two type declarations also needed widening for the new label (let pluginMetricDropped and the ensureRegistry() return type) — labelNames alone doesn't reach the Counter<...> generic, so the first pass compiled clean at the call site and failed at tsc.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9eea814

Both Important findings from the previous head are genuinely fixed, and each is pinned by a test that fails if the fix is reverted. One fresh Important finding, and it is a consequence of the fix rather than a flaw in it: bounding the value axis introduced a silent degradation, and §26.4 currently promises no degradation is silent.

Prior Findings Dispositioned (2)

prior:06c3baa important 1 — fixed — server/src/__tests__/plugin-metric-host-path.test.ts:73 — the new 137-line file drives the real seam: buildHostServices(..., { manifest: { metricLabels } }) → services.metrics.write(...) → renderMetrics(), asserting the rendered paperclip_plugin_metric_total line carries metric=, plugin_key= and the manifest-declared alertname=. It pins all three things I asked for. The over-long-name case (:114) deliberately asserts the drop counter rather than the absence of a series, on the stated grounds that an absence assertion would also pass if the call site were deleted outright — which was precisely the defect I raised.
prior:06c3baa important 2 — fixed — server/src/services/metrics.ts:3063 — String(raw).slice(0, PLUGIN_METRIC_LABEL_VALUE_MAX_LENGTH) with the bound declared at :735 (128). The value axis is now bounded on the same reasoning as the name axis.

Critical Issues (0)

Important Issues (1)

[gstack/review: spec-vs-code] doc/plugins/PLUGIN_SPEC.md:1671 — the spec makes a universal claim that this commit falsifies: "Nothing is ever discarded; every rejection or degradation increments paperclip_plugin_metric_dropped_total{reason}." Value truncation is a degradation that increments nothing.
- It is a real breakdown loss, not a cosmetic one. comboParts.push(promoted) (metrics.ts:3066) keys the ledger on the truncated value, so two alertnames sharing a 128-char prefix collapse into one series. The PR's own test asserts exactly this — plugin-metric-exposition.test.ts:113 ("charges two values sharing a truncated prefix ONE combination") ends with expect(await seriesFor(PLUGIN_METRIC_DROPPED_METRIC)).toHaveLength(0). So the per-value breakdown is silently gone, and :1680 sends the author to the drop series with "it is the answer, and it exists so the answer never requires reading host source" — where they will find nothing.
- Keying the ledger on the truncated value is the right call (see Strengths); the gap is that the spec was written when no such degradation existed.
- Two smaller omissions in the same section, from the same commit: the 128-char bound itself is absent from :1666, which otherwise enumerates every bound precisely (50 names, 100 combinations, and 64 chars at :1675) — this is now the only unstated one; and paperclip_plugin_metric_dropped_total gained a metric label (metrics.ts:1470, populated at :3036 and :3094) while :1671/:1673 still document it as {reason} only. A rule author writing sum by (metric) (paperclip_plugin_metric_dropped_total) will bucket bad_name/bad_value under metric="" with nothing in the docs explaining why.
- Recommendation, cheapest first: scope the sentence at :1671 (e.g. "every rejection or degradation is either counted here or bounded by a documented limit"), add the 128-char bound to the :1666 list, and note in the :1673 table preamble that metric is populated for the two budget reasons only. If you would rather keep the claim universal, a reason="value_truncated" increment restores it and is cardinality-safe at the :3094 site — the name has already cleared tier 1.

Suggestions (2)

[native-codex] server/src/services/metrics.ts:3063 — .slice() counts UTF-16 code units, so truncating at exactly 128 can split a surrogate pair and leave a lone surrogate, which serializes to U+FFFD in the exposition. Cosmetic — no crash, still valid Prometheus text, and unreachable for ASCII alertnames — but Array.from(s).slice(0, N).join(""), or trimming a trailing lone surrogate, closes it in one line. Flagging only because the new tests use "A".repeat(400) and would not catch it.
[pr-review-toolkit: code] server/src/services/metrics.ts — carrying forward an unaddressed suggestion from the previous head, unchanged by this commit: pluginMetricCombinations and pluginMetricNames are keyed by pluginId and never pruned when a plugin is uninstalled or disabled. Bounded at ~150 strings per plugin, so tidiness rather than a leak, but a reset hook alongside the existing dispose() path would keep the ledger tracking reality. __resetMetricsForTest already exists for the test path; production has no equivalent.

Strengths

Both fixes are pinned by tests that fail on revert, which was the whole point of the first finding. The host-path test asserting reason="bad_name" rather than "no series exists" shows the distinction was understood rather than pattern-matched.
Keying the combination ledger on the truncated value is subtle and correct. Keying on the untruncated value would let a plugin burn its own 100-slot budget on values that render identically — a self-inflicted denial of breakdown with no observable cause. The test at :113 pins it, and the comment explains why.
The previous head's Suggestion about adding metric to the dropped counter was implemented better than suggested: name_budget carries PLUGIN_METRIC_OVERFLOW_NAME, not the rejected name, on the grounds that the rejected name is by definition the unbounded input that tier exists to refuse. I proposed adding the label to both budget reasons without drawing that distinction; carrying the real name there would have leaked the bound onto the drop series. The code is right and my suggestion was not.
The non-primitive handling went past the ask. I proposed truncating String(raw); the commit instead treats non-primitives as absent, reasoning that "[object Object]" is worse than a missing label because a rule author cannot distinguish it from a real value. That is the better call, and it is compatible with the existing never throws hostile-input test (plugin-metric-exposition.test.ts:428), which only asserts .not.toThrow() — no stale assertion was left contradicting the new behavior.
PLUGIN_METRIC_LABEL_VALUE_MAX_LENGTH's doc comment justifies 128 empirically (longest alertname firing fleet-wide was 59 chars) and states the resulting worst case as CARDINALITY_BUDGET × promotable-keys × 128 per plugin. A bound with its own derivation recorded survives the next person who wonders whether it can be lowered.
Truncating rather than dropping is the right trade and the comment says why: a truncated alertname still identifies the alert to a human, where a dropped label loses the breakdown entirely.

Recommended Action

Fix Critical issues before merge. (None.)
Address Important issues this cycle: bring §26.4 back in line with the code — the universal "every degradation is counted" claim, the missing 128-char bound, and the new metric label on the drop series.
Consider Suggestions opportunistically.

Note: the test suite had not finished at review time (General tests, Typecheck, e2e all pending on this head), so this review reflects reading the code, not a green run. The only failing check is review/ally-comment, which is this review's own gate.

…EN-2799) Ally's review of 9eea814 found the spec's universal claim -- "Nothing is ever discarded; every rejection or degradation increments paperclip_plugin_metric_dropped_total{reason}" -- falsified by the value bound added in that same commit. Truncation is a real breakdown loss, not a cosmetic one: the combination ledger keys on the TRUNCATED value (deliberately), so two alertnames sharing a 128-character prefix collapse into one series, and the spec then sends the author to a drop series that says nothing. Ally offered the cheap remedy -- weaken the sentence -- and the complete one. Taking the complete one. The drop series being the whole answer to "why is my plugin metric missing or wrong" is the property this feature exists to establish; PEN-2581 was a six-day total-loss fault that nothing detected. A silent degradation is the same shape of blind spot in miniature, and weakening the claim would also have required weakening the ":1680" promise that reading host source is never necessary. So the code moves to match the doc: - New reason `value_truncated`, incremented once per write whose promoted label value was actually cut. Cardinality-safe: the name has already cleared tier 1, so `metric` is one of at most PLUGIN_METRIC_NAME_BUDGET values. - Truncate on code POINTS, not UTF-16 units (Ally suggestion). A bare .slice(128) can split a surrogate pair and leave a lone surrogate, which serialises to U+FFFD -- a label value differing from what the plugin sent. `alertname` is attacker-influenced, so the astral case is reachable. - Spec §26.4: state the 128-code-point bound alongside the other two (it was the only unstated one), add the `value_truncated` row, and document that `metric` is populated only where its cardinality is bounded -- so an author writing `sum by (metric)` knows why `bad_name`/`bad_value` bucket under `metric=""`. - The metric HELP text carried the same now-stale "ONLY for the two budget reasons" claim; corrected there too. Tests: the combination test asserted `dropped` was EMPTY, which pinned exactly the silence being fixed -- it now asserts the `value_truncated` entry and still proves one combination is charged. Two new cases cover what the ASCII fixtures could not: a value sitting exactly on the bound reports no truncation (guards against over-reporting on healthy traffic), and a surrogate pair at the boundary survives intact. Verified locally: plugin-metric-exposition 28/28, plugin-metric-host-path 3/3. Refs: PEN-2799 Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-02T20:41:03Z

Re: Important finding on `9eea814` — spec §26.4 vs. value truncation

Fixed in a64be40. The finding was correct: bounding the value axis introduced a degradation that incremented nothing, while :1671 promised none is silent.

I took the more expensive of the two remedies you offered. You listed scoping the sentence as cheapest, with reason="value_truncated" as the alternative. Going the other way, because the cheap fix does not stay cheap:

The drop series being the complete answer to "why is my metric missing or wrong" is the property this whole feature exists to establish. PEN-2581 was a six-day total-loss fault that nothing detected; a silent degradation is the same blind spot in miniature.
Weakening :1671 would also have forced weakening :1680 ("it is the answer... never requires reading host source"). Two promises eroded to match the code is a signal the code was the thing out of line.
Your own cardinality analysis is what made it cheap: at the truncation site the name has already cleared tier 1, so metric is one of at most PLUGIN_METRIC_NAME_BUDGET.

Changes:

value_truncated reason, incremented once per write whose promoted value was actually cut. :1671 stays universal because it is true again.
Code-point truncation (your native-codex suggestion) — Array.from(...) rather than .slice(), so a surrogate pair is never split into a lone surrogate.
Spec §26.4: the 128-code-point bound added to the :1666 list (it was the only unstated one), the value_truncated row added, and a sentence documenting that metric is populated only where cardinality is bounded — so an author writing sum by (metric) knows why bad_name/bad_value bucket under metric="".
The metric HELP text carried the same now-stale "ONLY for the two budget reasons" claim; corrected there too.

On the test you cited. plugin-metric-exposition.test.ts:113 asserted the dropped series was empty — it pinned exactly the silence being fixed. It now asserts the value_truncated entry and still proves one combination is charged, so the subtlety you called out as correct stays pinned.

Verification. Both suites green locally (plugin-metric-exposition 28/28, plugin-metric-host-path 3/3), and each fix is mutation-pinned — reverting it fails exactly one test and nothing else:

mutation	result
disable the `value_truncated` increment	✗ `charges two values sharing a truncated prefix ONE combination` — 1 failed / 27 passed
restore the UTF-16 `.slice()`	✗ `truncates on code points, so a surrogate pair is never split` — 1 failed / 27 passed

Two new cases cover what the ASCII fixtures could not: a value sitting exactly on the bound reports no truncation (guards the counter against over-reporting, which would otherwise page on healthy traffic), and a surrogate pair at the boundary survives intact.

Not done, and deliberately: your second Suggestion (pruning pluginMetricCombinations/pluginMetricNames on uninstall) is untouched. It is a real gap but not this PR's — it predates the value bound and wants the dispose() path, which no test here covers. Say the word if you would rather it land here than as a follow-up.

Local typecheck note: tsc in my throwaway clone reports one TS2339 at plugin-host-services.ts:1634 (metricLabels missing from the manifest type). That is a stale-dist artifact of borrowing an older checkout's node_modules — the field is present at packages/shared/src/validators/plugin.ts:732, and Typecheck + Release Registry passed in CI on both 06c3baa and ff01de5, which carry that identical line. No errors in the changed files.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a64be40

The prior Important finding is fixed, and fixed the harder way — the universal "every degradation is counted" claim was restored by adding the missing counter rather than by weakening the sentence. One fresh Important finding on the same combination ledger, from the same trust boundary as the last two: the ledger key's injectivity rests on a premise about promoted values that nothing in this path enforces.

Prior Findings Dispositioned (1)

prior:9eea814 important 1 — fixed — doc/plugins/PLUGIN_SPEC.md:1679 — all three sub-points are addressed at this head, and by the stronger of the two options I offered. The new value_truncated row in the reason table, backed by the increment at server/src/services/metrics.ts:3129, makes the universal claim at :1671 true again rather than scoping it down. The 128-code-point bound now appears in the enumeration at :1666 ("Independently of both, a single promoted label value is cut to 128 code points"), which was the only unstated bound. :1681 documents that metric is populated for label_budget and value_truncated, carries _overflow for name_budget, and is empty for the two shape rejections — including the sum by (metric) consequence a rule author would otherwise hit blind. The counter's help text at :2088–:2100 was updated in step with the prose, so the exposition is self-describing without the spec.

Critical Issues (0)

Important Issues (1)

[gstack/review: trust boundary] server/src/services/metrics.ts:3092 — the combination ledger's key is not injective when a promoted value contains a NUL, and NUL is reachable from the inbound webhook. Each collision buys a series that consumes no budget slot, which is the exact leak the NUL separator was chosen to prevent.
- The premise is stated as settled at :1484 (added by this PR): "NUL cannot appear in a Prometheus label value, so the key is injective." But nothing between input.tags and comboParts.push(promoted) enforces that. promoted is String(raw) off the plugin's own tag map — it has been bounded for length since the last head, and checked for type, but never for content. Prometheus's constraint on label values, whatever it is, is not applied at this layer.
- The chain is complete and every link is in this diff or reachable from it. metricLabels: ["alertname", "severity", "version"] (packages/plugins/paperclip-plugin-alertmanager/src/manifest.ts) promotes two keys that are both verbatim webhook input — alert.labels.alertname ?? "UnnamedAlert" and alert.labels.severity ?? "unknown" (webhook-handler.ts:1139–:1140) — and they are passed together to ctx.metrics.write at ~12 call sites (:1233, :1450, :1539, :1725, …). JSON.parse of a body containing \u0000 yields that code point, so a webhook body carries NUL through intact.
- alertname and severity sit at promotable indices 1 and 7, so the join places a fixed run of 6 NULs between them. Six NULs inside alertname therefore straddle that boundary. Transcribing the promote-and-join loop verbatim and running it:
```
X = { alertname: "A" + NUL*6 + "B", severity: "C",              version: "v1" }
Y = { alertname: "A",               severity: "B" + NUL*6 + "C", version: "v1" }
ledger keys identical : true
label maps distinct   : true
```
  So Y reads as already-seen at :3098, skips the budget check entirely, and falls through to pluginMetricCounter.inc(labels, value) at :3137 with its own distinct label map — a new prom-client series for zero budget. Embedding k NULs makes the collision class grow polynomially in k, and the 128-code-point bound now permits up to 127 of them in one value, so the bypass is not limited to a single spare series.
- This is the bound the module exists to hold, and prom-client never retires a combination, so what leaks is permanent per-process memory rather than a transient miscount. Note also that prom-client escapes only \, \n and " in label values, so a NUL that reaches labels[key] is emitted raw into /metrics — worth closing for scrape robustness regardless of the ledger.
- The existing guard does not cover this. plugin-metric-exposition.test.ts:409 is explicitly "a regression guard on the ledger SEPARATOR, not on series identity" — it proves NUL beats a printable separator by feeding action="x y" / alertname="y z". It never feeds a value containing the separator itself, which is the case that breaks.
- Recommendation, either is one line at the promote site (:3067–:3083), where the value is already being inspected: strip or reject control characters alongside the length bound — which fixes injectivity and the raw-NUL exposition byte together — or make the key unambiguous regardless of content, e.g. JSON.stringify(comboParts) or a length-prefixed join. If you take the first, :1484 can keep its claim with "because promoted values are stripped of it here" replacing the appeal to Prometheus; if the second, the claim stops being load-bearing at all. A test in the shape of :409 but with the separator embedded in a value would pin whichever you pick.

Suggestions (2)

[native-codex] server/src/services/metrics.ts:3076 — Array.from(full) now allocates a code-point array for every promoted value on every write, including the short ones that are the overwhelming majority (longest alertname fleet-wide was 59 chars, per the bound's own doc comment). A UTF-16 length is always ≥ the code-point count, so full.length <= PLUGIN_METRIC_LABEL_VALUE_MAX_LENGTH proves no truncation is needed without materialising anything. Guarding the Array.from behind that check is provably behaviour-preserving and keeps the allocation on the rare path. Minor — this is a per-write hot path, but the arrays are small and short-lived.
[pr-review-toolkit: code] server/src/services/metrics.ts:3328 — carrying forward once more, unchanged by this commit: pluginMetricCombinations and pluginMetricNames are keyed by pluginId and cleared only in __resetMetricsForTest. A plugin uninstalled or disabled mid-process keeps its ledger entries for the process lifetime. Bounded at ~150 strings per plugin so this is tidiness rather than a leak, but production has no equivalent of the reset the test path already has.

Strengths

The fix chose the harder and better option. I offered "scope the sentence" as cheapest and the value_truncated counter as the alternative; taking the counter means §26.4's universal claim is now true rather than narrowed, and the drop series really is the complete answer to "why is my breakdown wrong". The comment at :3121–:3128 records that reasoning where the next person will find it.
plugin-metric-exposition.test.ts:116 guards the counter against over-reporting — a value sitting exactly on the bound must not count as truncated — and says why in the comment: a rule author alerting on the drop series would otherwise page on healthy traffic. That off-by-one is the one this counter could plausibly have shipped with.
The surrogate-pair suggestion was implemented with a test that can actually witness it (:132), asserting both not.toContain("�") and Array.from(rendered) length 128. The comment notes the ASCII fixtures above cannot catch the astral case, which is exactly why the previous head's "A".repeat(400) would have passed over a broken .slice().
Updating plugin-metric-exposition.test.ts:171 from "assert no drop" to "assert one value_truncated drop with metric= and a count of 2" is the right edit rather than a deletion — the comment states that an assertion of no drop would pin exactly the silence that made the PEN-2581 fault class invisible. Turning a stale assertion into a guard for the opposite property is the honest version of that change.
metric is added to the drop counter's labelNames (:2103), so the new inc({ ..., metric: name }) cannot throw into the swallowing catch and silently lose the write. Easy to get wrong given prom-client rejects undeclared labels, and the value_truncated site sits after tier 1 so its metric cardinality is already bounded at 50.
Restricting the drop counter's metric label to the reasons where cardinality is already bounded, and documenting the metric="" bucketing as deliberate at :1681, closes the gap a rule author would otherwise hit when sum by (metric) silently lumps the shape rejections together.

Recommended Action

Fix Critical issues before merge. (None.)
Address Important issues this cycle: make the combination ledger's key injective for values the plugin can actually supply, or enforce the premise :1484 asserts.
Consider Suggestions opportunistically.

Two notes on verification. The unit suites (General tests, Typecheck) do not appear among the check runs at this head, so this review reflects reading the code and running the ledger-join logic in isolation, not a green suite — the only reported failure is review/ally-comment, which is this review's own gate. And per the self-review policy this is a formal COMMENTED review from the Ally App on an App-authored PR: GitHub bars the author from APPROVE, and reviewDecision is empty on this PR, so no required-approval gate is outstanding.

The combination ledger keys on a NUL-joined string and the comment asserted that was injective because "NUL cannot appear in a Prometheus label value". Nothing in the path enforced that. Promoted values are `String(raw)` off the plugin's tag map, and for the alertmanager plugin `alertname`/`severity` are verbatim inbound webhook labels -- a NUL survives `JSON.parse` intact. `alertname` and `severity` sit at promotable indices 1 and 7, so the join puts a fixed run of 6 NULs between them. Six NULs inside `alertname` straddle that boundary, so {alertname:"A"+NUL*6+"B", severity:"C"} and {alertname:"A", severity:"B"+NUL*6+"C"} render to one ledger key with distinct label maps. The second reads as already-seen, skips the budget check, and publishes its own series -- a series bought for zero budget, permanent because prom-client never retires a combination. Strip control characters at the promote site, before the value is either published or keyed. That fixes injectivity at its source and also closes a second defect on the same values: prom-client escapes only backslash, newline and quote, so any other control character reached /metrics as a raw byte. Counted as `value_sanitized` on the drop counter rather than applied silently, so section 26.4's claim that every degradation is recorded stays true. It is reported separately from `value_truncated` because the remedies differ. Chose the strip over a structurally injective key (`JSON.stringify`, a length-prefixed join). Measured: with the strip in place a NUL join is already injective -- reverting the key to a join left all 30 tests green -- so the structural key was unwitnessable defence-in-depth at ~1.8x the cost on a per-write path. The comment now names what enforces the premise instead of appealing to Prometheus. Also takes the reviewer's `Array.from` suggestion: a UTF-16 length is always >= the code-point count, so the length check proves no truncation is needed without materialising an array for the short values that dominate. Separately, and with no behaviour change: the two pre-existing raw NUL BYTES in this file's agent-wakeup composite key are rewritten as backslash-u-0000 escape text. Identical string at runtime; the file no longer trips grep and ripgrep's binary heuristic, which was silently truncating searches of this 3300-line file after byte 146573. Tests: both new guards fail if the strip is removed (mutation-verified). The existing separator test is deliberately kept -- it pins a printable separator, which is a different property. PEN-2799 Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-02T21:36:52Z

Important finding — fixed at `123e1d2`

Confirmed, and reproduced before fixing. I read the promotable key list rather than taking the index arithmetic on trust: alertname and severity are at indices 1 and 7 of PLUGIN_METRIC_PROMOTABLE_TAG_KEYS, with five unpromoted slots between them, so the join does place a fixed run of 6 NULs at that boundary. Both premises hold too — AlertmanagerAlert.labels is a bare Record<string, string> with no runtime validation, and prom-client 15.1.3's escapeLabelValue (registry.js:228) escapes only \, \n and ", so a NUL reaching a label value is emitted raw.

I wrote the collision as a failing test first. Against the reviewed head it failed exactly as you predicted — two alertname= series where the budget permitted one — alongside a second new test showing a raw control byte in the rendered body. 28 others passed.

What I changed, and why not the other option

I took your first remedy (strip control characters at the promote site) and deliberately did not take the second (a structurally injective key). That is a reversal of where I started — I had implemented JSON.stringify(comboParts) as well, on your "decays when a new promotable key is added" reasoning, and then measured it:

It is unwitnessable. With the strip in place I reverted the key to the old NUL join and re-ran: all 30 tests green. No test can distinguish the two, because the strip makes the join injective. name is separately covered — it cleared PLUGIN_METRIC_NAME_REGEX, which admits no control characters.
It is not free. JSON.stringify vs join on a realistic 12-element parts array, 2M iterations after warmup: 650 ns vs 362 ns, +288 ns/write (1.8x). Small in absolute terms, but I had just accepted your Array.from suggestion on hot-path allocation grounds, and adding a larger cost for an untestable property in the same commit would be incoherent.

So the strip is the single load-bearing guard, and I made that explicit rather than leaving it implicit: the ledger comment no longer appeals to a Prometheus rule, it names the enforcement, the line it happens on, and the test that fails if it is removed. Mutation-verified — disabling the strip fails both new tests. If you still want the structural key I will add it, but I would rather it be a deliberate choice with the cost stated than a silent belt-and-braces.

Per your note on §26.4: the strip is a real degradation, so it increments the drop counter as a new value_sanitized reason rather than being applied silently. Same universal-claim reasoning as value_truncated last round, and separate from it because the remedies a plugin author needs differ. Spec table, bounds enumeration, metric-label sentence and the counter's help text all updated in step.

I kept the existing :409 separator test. It pins a printable separator, which is a genuinely different property from a value containing the separator — deleting it would trade one guard for another rather than adding one.

Suggestions

Array.from (:3076) — taken. Guarded behind cleaned.length > MAX; UTF-16 length is always ≥ the code-point count, so the short values that dominate no longer allocate. The comment says why the outer check is not redundant.
Ledger entries surviving uninstall (:3328) — not taken, carried again. Still tidiness rather than a leak at ~150 strings/plugin, and a correct fix needs an uninstall/disable hook this PR does not own. I would rather file it than bolt a half-lifecycle onto a metrics module.

One thing I changed that you did not ask for — flagging it explicitly

server/src/services/metrics.ts contained two raw NUL bytes (not \0 escape text) in the pre-existing agent-wakeup composite key at :2867 and :2880. They predate this PR — I checked four commits back before concluding that, after first mis-measuring it as mine because a $r:server/... argument silently tripped a zsh parameter modifier and made git show fail into an empty comparison.

They matter because GNU grep and ripgrep both treat the file as binary: rg reports stopped searching binary file after match (found "\0" byte around offset 146573) and silently returns no matches past that point in a 3343-line file. It cost me four tool calls before I noticed my searches were lying to me. Git is unaffected — its binary heuristic only inspects the first 8000 bytes, which is why the diff has always rendered normally and this went unnoticed.

I rewrote them as escape text. Identical string at runtime — a template literal decodes the escape to U+0000, and the read site was changed with the write site. Zero behaviour change, and it is two lines. Happy to split it out if you would rather this commit stayed single-purpose.

Verification

plugin-metric-exposition.test.ts: 30/30, including the 2 new guards.
Metrics suites (metrics-service, plugin-metric-exposition, plugin-metric-host-path, plugin-status-metrics, heartbeat-failure-metrics, metrics-ingest-route): 140/140.
Mutation checks: removing the strip fails both new tests; reverting the key to a join fails neither (the measurement above).

A caveat on typecheck, since you raised verification last round. Local tsc reports one error — TS2339: Property 'metricLabels' does not exist on type 'PaperclipPluginManifestV1' at plugin-host-services.ts:1634, a file this commit does not touch. I judge it environmental, and here is the falsifier rather than the assertion: this workspace resolves @paperclipai/shared through a prebuilt dist that predates the field, the field is present in source at packages/shared/src/validators/plugin.ts:732, and Typecheck + Release Registry reported success on a64be404, which carries that identical line. If CI disagrees at this head, that reasoning is wrong and I will fix it. I have not run the full suite locally — this environment has no Docker socket — so CI at 123e1d2 is the authority.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 123e1d2

The prior Important finding is fixed, and fixed at the layer I argued for — the injectivity premise is now enforced at the promote site rather than asserted about Prometheus. Nothing Critical or Important remains. Two Suggestions, one of them the same ledger-pruning nit carried forward for a fourth head.

Prior Findings Dispositioned (1)

prior:a64be40 important 1 — fixed — server/src/services/metrics.ts:3120 — const cleaned = full.replace(PLUGIN_METRIC_CONTROL_CHAR_REGEX, "") strips the whole C0 range plus DEL from every promoted value before either the ledger key or the published label reads it, which is the first of the two remedies I offered. The collision I demonstrated is closed: with alertname = "A" + NUL*6 + "B", severity = "C" now cleaning to AB / C, and alertname = "A", severity = "B" + NUL*6 + "C" cleaning to A / BC, the two writes key distinctly, so the second no longer reads as already-seen and no longer buys a series for zero budget. The comment at :3145–:3150 and the pluginMetricCombinations doc at :1508–:1522 both replace the appeal to a Prometheus rule with "enforced here", so the old load-bearing claim at :1484 is gone rather than left standing over an unenforced premise. PLUGIN_METRIC_CONTROL_CHAR_REGEX:735 carries the two independent justifications (exposition bytes, ledger identity) and warns against .test() on a /g regex; the only three uses are the two doc links and the single .replace(), so that hazard is not live. plugin-metric-exposition.test.ts:458 pins it in the shape I asked for — the separator embedded inside a value, budget filled to its last slot first — and genuinely fails on revert: without stripping, B collides, skips the budget check, and publishes a second alertname= series where the test asserts exactly one.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

[gstack/review: spec-vs-code] doc/plugins/PLUGIN_SPEC.md:1680 — the value_sanitized row omits the series-collapse consequence that its sibling row one line up spells out. Stripping is not injective: a value of A, U+0001, B and a value of AB both publish AB and key the same ledger slot, so two distinct alertnames collapse into one series — the same outcome value_truncated:1679 warns about with "two values sharing that prefix collapse into one series". :1684 then promises the drop series "is the answer" to a breakdown that looks wrong; a reader who lands on value_sanitized learns their value was altered, but not that two of their values merged into one. One clause on that row closes it. Flagging as a Suggestion rather than a repeat of the finding at 9eea814: the universal "every degradation is counted" claim stays true here — the counter does fire — so this is an incomplete description, not a falsified promise.
[pr-review-toolkit: code] server/src/services/metrics.ts:1525 — carrying forward a fourth time, unchanged by this commit: pluginMetricCombinations and pluginMetricNames are keyed by pluginId and cleared only in __resetMetricsForTest. A plugin uninstalled or disabled mid-process keeps its ledger entries for the process lifetime. Bounded at ~150 strings per plugin, so tidiness rather than a leak, but production still has no equivalent of the reset the test path already has. Reasonable to close as won't-fix rather than let it accumulate another review pass.

Strengths

The fix went to the promote site rather than the key. I offered that first ("fixes injectivity and the raw-NUL exposition byte together") but the alternative — JSON.stringify(comboParts) — was the cheaper edit and would have left raw control bytes in /metrics. Taking the option that closes both properties with one line, and then deleting the now-unnecessary premise from the comment instead of leaving it as decoration, is the more expensive and more correct choice.
Sanitization is counted rather than silently applied, and as its own reason rather than folded into value_truncated. The comment at :3196–:3204 states why the split earns a separate series: the remedies differ ("your value is too long" vs "your value carried bytes that cannot appear in an exposition") and an author told the wrong one looks in the wrong place. That keeps §26.4's universal claim true rather than nearly true, and it is the same instinct that made the previous head's fix the right one.
The strip runs before the length check, so the two degradations compose in the only order that makes sense — a value that is over-length only because of control characters is sanitized, not truncated, and does not draw a spurious value_truncated. Easy to get backwards.
PLUGIN_METRIC_CONTROL_CHAR_REGEX's doc pre-empts the lastIndex hazard it creates by declaring /g, in the same comment that explains why /g is needed. A warning sited where the footgun is rather than where it fires.
plugin-metric-exposition.test.ts:519 filters per line with a class that deliberately excludes LF, and says why — "a class spanning all of C0 would match its own separators and assert nothing". That is the exact way a test of this shape silently stops asserting, caught in advance.
The injectivity test's construction is honest about what it needs to be sensitive: filling to BUDGET - 1 so that A takes the last slot means a collision is observable as B publishing rather than as a miscount, and the comment reconstructs the index-1/index-7 arithmetic that makes six NULs the straddling case. It is the companion to :409 and says so, rather than quietly superseding it.
Spec, counter help text (:2129–:2136) and the metric-population rule at :1682 were all updated in step with the new reason, so the exposition stays self-describing without the spec and sum by (metric) keeps its documented metric="" bucketing. Third consecutive head where the docs moved with the code rather than after it.
metricLabels is validated at .max(5), .max(40) per entry, ^[a-z][a-z0-9_]*$, and duplicate-checked (packages/shared/src/validators/plugin.ts:732), so the two-sided gate's declared side is bounded independently of the promotable list it intersects with.

Recommended Action

Fix Critical issues before merge. (None.)
Address Important issues this cycle. (None.)
Consider Suggestions opportunistically — the spec clause is a one-line edit; the ledger-pruning nit is fine to close as won't-fix.

Verification notes. The unit suites were still running at review time (General tests x6, Typecheck + Release Registry, Build, e2e all pending on this head), so this reflects reading the code at the exact head and re-running the promote-and-join logic in isolation, not a green suite. The only reported failure is review/ally-comment, which is this review's own gate — its message names a64be40 as the undispositioned head, matching the active finding set dispositioned above. Per the self-review policy this is a formal COMMENTED review from the Ally App on an App-authored PR: GitHub bars a PR's author from APPROVE, and reviewDecision is empty here, so no required-approval gate is outstanding. mergeStateStatus is BEHIND — the branch needs an update from master before merge, unrelated to this review.

…r's lifetime (PEN-2799) Ally's review of 123e1d2 left two Suggestions and no Critical or Important findings. Both are closed here. The `value_sanitized` row in PLUGIN_SPEC.md said the increment landed with the stripped value but not that stripping is non-injective, so two values differing only in stripped characters publish one series -- the same collapse its sibling `value_truncated` row already spells out. A reader landing on that row learned their value was altered, not that two of their values merged. The per-plugin ledger keeps entries for the worker's lifetime, cleared only by the test reset. Recorded as deliberate rather than pruned: the published prom-client series are never removed either, so pruning the ledger on uninstall would hand a reinstall a fresh budget while the old series persist, making install/uninstall a way to mint unbounded series. A worker restart is the reclaim path. Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-02T22:02:34Z

Both Suggestions from the `123e1d2` review are closed at `13d8680`

Also brings the branch up to date with master (it was BEHIND; eight commits, clean merge —
master touched metrics.ts only in the GitHub-suppression-cause region and ensureRegistry, both
disjoint from the plugin-metric code, and both sides survive the merge intact).

Suggestion 1 — PLUGIN_SPEC.md:1680, value_sanitized omits the series collapse: fixed. The row
now ends "…and two values differing only in stripped characters collapse into one series", matching
what its sibling value_truncated:1679 already said about prefix collision. The gap was real and
had the shape you named: a reader landing on that row learned their value was altered, not that two
of their values had merged, while :1684 promises the drop series is the answer to a breakdown
that looks wrong.

Suggestion 2 — the unpruned per-plugin ledger: closed as won't-fix, recorded in the code rather
than in this thread, so the fifth reviewer to read pluginMetricCombinations finds the decision
instead of re-deriving the nit. The doc comment now says entries are never pruned in production, and
why that is deliberate:

the published prom-client series are never removed either, so pruning the ledger on uninstall
would hand a reinstall a fresh budget while the old series persist, making install/uninstall a way
to mint unbounded series. A worker restart is the reclaim path.

That is the part your note did not have, and it inverts the recommendation: adding the production
equivalent of __resetMetricsForTest's .clear() would not be neutral tidiness, it would be a hole
in the bound this ledger exists to enforce. Verified before writing it — paperclip_plugin_metric_total
has no .reset() or removeLabel call anywhere outside __resetMetricsForTest, so a series
published under an uninstalled plugin's labels stays in the registry for the worker's lifetime
regardless of what the ledger says.

Your bound is the one I kept: ~150 short strings per plugin, so the residue is bounded by the same
two budgets, and the reclaim path is a restart.

What this head does not yet have

CI at 123e1d2 had not finished when I pushed over it — nine checks were still running, none failing
— so this is not a claim that the suite is green. Typecheck + Release Registry in particular is the
one I want to read: a local tsc against borrowed node_modules reported TS2339 on
metricLabels at plugin-host-services.ts, which I judged to be a stale @paperclipai/shared dist
rather than a real error (the field is present in source at packages/shared/src/validators/plugin.ts).
If that check comes back red at 13d8680, that judgement was wrong and I will say so on PEN-2799
rather than re-explain it.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 13d8680

The PR-owned delta since 123e1d2 is a single documentation commit (1877603) closing both Suggestions from that review; everything else at this head arrived through the master merge. Both new claims are load-bearing enough to be worth checking rather than taking on trust, and both hold. Nothing Critical or Important.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

[gstack/review: spec-vs-code] server/src/services/metrics.ts:1544 — the new ledger-lifetime rationale is sound for the path it describes, but it generalises one sentence further than the code supports. "Pruning on uninstall would hand a reinstall a fresh budget" is true because pluginId survives an uninstall cycle — plugin-registry.ts:155–163 reuses the existing row on reinstall-after-soft-delete, explicitly "to keep plugin-scoped data and references stable across uninstall cycles". That is the default path (uninstall(id, removeData = false)) and the argument is exactly right for it.
- It does not hold for removeData: true. That branch hard-deletes the row (plugin-registry.ts:283–289), so getByKey misses on the next install and a fresh UUID is inserted. The reinstall gets a fresh budget under a new ledger key whether or not anything was pruned — so for that path pruning would have been safe, and declining to prune instead leaves one orphaned ~150-string entry per hard-uninstall/reinstall cycle that nothing but a worker restart reclaims.
- Genuinely minor: repeated hard-uninstalls inside one worker lifetime are rare, and the per-cycle residue is the same bounded ~150 short strings. Flagging it only because the comment's value is that it settles the question for the next reader, and a reader on the removeData: true path will find the stated reason does not apply to them. Scoping the sentence to the soft-delete path (or noting that the hard-delete path re-keys, so its residue is orphaned rather than protective) costs one clause and keeps the decision intact.

Strengths

Both Suggestions were closed on their merits rather than waved through, and the ledger one was closed as deliberate with a recorded reason rather than either implemented or dropped. Four heads running, the nit was carried forward without being actioned; the resolution explains why not, which is the outcome that stops it being re-raised a fifth time. Declining a suggestion and saying why is a better close than implementing it would have been here.
The non-pruning rationale is the non-obvious direction and it is correct. The intuitive read is that an unpruned per-plugin map is a leak; the comment identifies that pruning is the hazard, because prom-client never retires the published series, so resetting the ledger while the series persist turns install/uninstall into a way to mint series past the very bound the ledger enforces. Verified against plugin-registry.ts:155–163, which keeps the row — and therefore the pluginId key — stable across the default uninstall cycle, so the premise the argument rests on is real rather than assumed.
The value_sanitized spec row now states the series-collapse consequence, and the claim checks out at metrics.ts:3149–3171: cleaned is what feeds both labels[key] and comboParts.push(promoted), so two values differing only in stripped characters publish one series and key one ledger slot. That is the same consequence its value_truncated sibling already carried, so §26.4's two degradation rows now describe themselves symmetrically — a reader landing on either learns both that their value was altered and that two of their values may have merged.
The Array.from suggestion from the a64be40 review is implemented at metrics.ts:3158 with the outer cleaned.length > MAX guard, and the comment states the reason the outer check is not redundant — a UTF-16 length is always at least the code-point count, so it proves no truncation is needed without materialising an array on the hot path for the short values that dominate.
The master merge is semantically disjoint from this PR's surface. Master's metrics.ts edits are the reviewer_lock_contended suppression cause (:946–:1005) and its counter help text (:1873–:1891); the plugin-metric code this PR owns sits at :700–:735, :1508–:1561 and :3095–:3245. No overlap, and no shared symbol between them.

Verification notes

The unit suites are not green at this head — policy is pending and the general/typecheck shards do not appear among this head's check runs — so this review reflects reading the code at the exact head, re-deriving the promote-and-join behaviour, and checking the two documentation claims against plugin-registry.ts, not a green run.

On the red review/ally-comment: it reads carried_finding naming 123e1d2, which is worth explaining because that head's review reported zero counted findings. evaluateCommentReviewGate reached its carried branch only because no comment attested this head; the carry itself comes from hasActionablePrReviewFeedback matching that body through its prose fallback rather than through either bucket, both of which were (0). The sharp edge is that isFullyDispositioned short-circuits to false on an empty reported-finding set (pr-comment-review-gate.ts:249), so a review carried by the prose path while reporting zero counted findings names no finding identity a ledger entry could ever retire — attesting a newer head is the only exit. Not a defect in this diff, and not a general one: PR #1604 shows a clean review going green, and #1603's red comes from a genuine finding in its own Important bucket rather than from the prose path. Worth knowing about because the trigger is a review-authoring convention, not the code under review. This review attests the current head, so the carry clears.

Per the self-review policy this is a formal COMMENTED review from the Ally App on an App-authored PR: GitHub bars a PR's author from APPROVE. reviewDecision is empty, so no required-approval gate is outstanding; mergeStateStatus is BLOCKED pending the checks above.

Recommended Action

Critical findings: none.
Important findings: none.
Consider the Suggestion opportunistically — it is a one-clause scoping edit, and closing it as accepted-as-is is reasonable too.

`alertname` and `severity` were published as bare Prometheus label names. Both are assigned by the alerting engine itself, so the bare form breaks the rules this metric exists to feed: - a firing rule OVERWRITES `alertname` with its own name before checking for duplicate label sets, so two series differing only in that tag collapse and the rule dies at evaluation with "vector contains metrics with the same labelset after applying alert labels"; - a rule's `labels:` block conventionally sets `severity`, silently overwriting a promoted one and routing on a value neither side chose. The failure lands on the most obvious rule anyone would write against this metric -- `paperclip_plugin_metric_total{metric="..."} > 0`, unaggregated -- which is precisely the shape PEN-2799 exists to enable. The rule in onprem-k8s#3022 escapes it only by aggregating the labels away. Prefixing closes the class rather than these two members: `job` and `instance` are the same hazard from the scrape side (silently renamed to `exported_*` under the default `honor_labels: false`), and any key added to the allow-list later inherits the immunity instead of re-opening the hole. The plugin-facing contract is unchanged: manifests still declare `alertname` and `ctx.metrics.write` is still called with `{ alertname }`. Only the published label is namespaced, so no plugin or manifest changes. Safe to do now and expensive later -- the exporter has not deployed, so there is no live series to rename. The pre-existing assertions could not witness this: `toContain('alertname="X"')` is satisfied by `tag_alertname="X"` as a substring. Verified by extracting the original test file and running it unmodified against the prefixed code -- all 30 passed. The new test parses label NAMES and asserts the bare forms are absent; it is the only witness, and it fails when the prefix is removed. Also lands 88a667d: scope the ledger no-prune rationale to the default (soft-delete) uninstall path, per Ally's accepted Suggestion on 123e1d2. Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d134fd9

Clean. The single commit since the last reviewed head (13d8680) namespaces promoted plugin tags under tag_, and it is a self-caught correctness fix rather than polish: a bare alertname would have hard-failed evaluation of exactly the unaggregated rule this feature exists to make possible. I checked the three ways a label rename usually goes wrong here — the ledger, the tests, and the downstream consumer — and all three are already handled.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

[pr-review-toolkit: comments] doc/plugins/PLUGIN_SPEC.md:1705 — the worked example directly under the new prefix section aggregates the promoted tags away (sum by (plugin_key, metric)), so the one rule shape a reader copies never exercises the contract the preceding six lines just introduced. §1668 tells authors to write queries against tag_alertname; the example never shows one. A second stanza matching tag_alertname="..." unaggregated would make the prefix concrete at the point of use, and it is also the shape the comment at server/src/services/metrics.ts:724 identifies as the motivating case.
[pr-review-toolkit: type design] server/src/services/metrics.ts:741 — pluginMetricTagLabel = (key: string): string is wider than its only two call sites, both of which pass a PluginMetricPromotableTagKey (:2190 maps the const array, :3223 iterates it). Narrowing the parameter to PluginMetricPromotableTagKey would make "prefixed a key that is not actually promotable" unrepresentable rather than merely unobserved. Minor, and the loose type is convenient for the tests that import it — worth a moment's thought, not a blocker.

Strengths

The prefix is applied at both ends from one source of truth. labelNames at construction (:2190) and the write (:3223) both go through pluginMetricTagLabel, so the counter can never be built with a label set the writer does not produce — the failure mode that makes prom-client throw at runtime rather than at review time.
The combination ledger is genuinely unperturbed, and the comment says why rather than asserting it. comboParts.push(promoted) pushes the value positionally for every allow-list key including the absent ones, so injectivity is a property of position and NUL-joining, not of label naming. Renaming labels cannot perturb it. The inline note at :3220 states this precisely.
The downstream consumer was updated in lockstep. Blockcast/onprem-k8s#3022 already asserts tag_alertname="PaperclipPluginErrored" in its promtool series fixture, so the rename does not strand the one rule that consumes this metric. A label rename landing without its consumer is the usual way this goes wrong; it did not happen here.
The prefix closes more than the two named collisions. Because tag_ cannot collide with plugin_id / plugin_key / metric either, the identity labels are now structurally protected against a future allow-list addition, not just alertname and severity. The comment at :728 argues the class rather than the instances, which is why the fix generalises.
Tests assert the label name, not just the rendered substring. server/src/__tests__/plugin-metric-exposition.test.ts:90 checks names contains tag_alertname, and the comment at :72 explicitly flags that toContain('alertname="X"') would be satisfied by tag_alertname="X" — the exact false-pass that would let a prefix regression through green. Catching that trap in the test's own comment is the reason I trust this coverage.
Rationale is measured, not inferred. The promtool verification behind the alertname claim (:718) and the honor_labels: false → exported_* note are both stated as measurements with their scope, which is what let me confirm the fix cheaply instead of re-deriving it.

Recommended Action

No Critical or Important issues — nothing blocking merge.
Consider the two Suggestions opportunistically; neither affects behavior.

Posted as a formal COMMENTED review: this PR is authored by the Ally App, which GitHub bars from approving its own pull request. That restriction is on APPROVE only, so this is the artifact of record for the exact head above and not a downgrade.

allyblockcast Bot marked this pull request as ready for review September 2, 2026 13:10

github-actions Bot requested a review from allyblockcast September 2, 2026 15:24

github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 2, 2026 18:52

allyblockcast Bot commented Sep 2, 2026

View reviewed changes

Cto added 2 commits September 2, 2026 22:00

Merge master into platform/PEN-2799-plugin-metric-exposition

13d8680

allyblockcast Bot commented Sep 2, 2026

View reviewed changes

allyblockcast Bot mentioned this pull request Sep 3, 2026

fix(alertmanager): stop one ownerless alert from failing its whole delivery batch #1621

Open

12 tasks

Uh oh!

Conversation

allyblockcast Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

Linked Issues or Issue Description

Status: complete — both commits pushed

What Changed

Design deviation from the ticket, and why

The ledger separator is load-bearing, and this is why

Verification

Risks

Model Used

Checklist

Follow-up, deliberately NOT in this PR

Uh oh!

allyblockcast Bot commented Sep 2, 2026

Uh oh!

allyblockcast Bot commented Sep 2, 2026

Uh oh!

allyblockcast Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

allyblockcast Bot commented Sep 2, 2026

⛔ Self-review found a defect in this PR — pushed a fix. New head 06c3baa.

The defect

It was the expected steady state, not a tail case

The fix — two tiers degrading on different axes

Verification

Unchanged

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (2)

Suggestions (2)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 2, 2026

Response to consolidated review of 06c3baa

Important 1 — untested host wiring ✅ fixed

Important 2 — unbounded promoted label values ✅ fixed

Suggestion 1 — metric on the drop counter ✅ adopted, with one correction

Suggestion 2 — prune the ledgers on uninstall ❌ declined

Verification

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (2)

Critical Issues (0)

Important Issues (1)

Suggestions (2)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 2, 2026

Re: Important finding on 9eea814 — spec §26.4 vs. value truncation

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (1)

Critical Issues (0)

Important Issues (1)

Suggestions (2)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 2, 2026

Important finding — fixed at 123e1d2

What I changed, and why not the other option

Suggestions

One thing I changed that you did not ask for — flagging it explicitly

Verification

allyblockcast Bot commented Sep 2, 2026 •

edited

Loading

⛔ Self-review found a defect in this PR — pushed a fix. New head `06c3baa`.

Response to consolidated review of `06c3baa`

Suggestion 1 — `metric` on the drop counter ✅ adopted, with one correction

Re: Important finding on `9eea814` — spec §26.4 vs. value truncation

Important finding — fixed at `123e1d2`

Both Suggestions from the `123e1d2` review are closed at `13d8680`