From b7ec4bca56a8c59724c3f014c60f4e671ed87038 Mon Sep 17 00:00:00 2001
From: Cto <cto@penstock.local>
Date: Thu, 3 Sep 2026 04:59:08 +0000
Subject: [PATCH 1/5] fix(alertmanager): stop one ownerless alert from failing
 its whole batch

An alert whose `fallbackAgentName` resolves to no invokable agent threw a
bare Error from handleFiring. The per-alert catch was written against a
taxonomy where every throw is transient (issue-RPC, state-store, event and
metric faults), so it recorded the fingerprint and the delivery ended in
AlertDeliveryIncompleteError -> HTTP 502.

Two consequences, both observed in production (PEN-2581):

1. A permanent fault travelled the transient-retry channel. Unresolvable
   ownership is a config/roster fact, not process state, so no retry can
   fix it: Alertmanager retried 15-17x and dropped the delivery anyway.
   The resulting notifications_failed_total storm also masked concurrent
   genuinely-transient failures that retrying would have fixed.

2. One ownerless alert dark-tiered every other alert sharing its batch,
   because a single accumulated fingerprint fails the whole delivery.

Introduce PermanentAlertError for faults no retry can resolve, and give it
the same "log + 200" treatment the malformed-payload and permanent-policy
drops already get: log, record a distinct
`alertmanager.alert.permanent_error` metric, drop the alert, and leave the
fingerprint out of failedFingerprints so the rest of the batch still lands.

The fail-closed guarantee is unchanged: an ownerless alert still creates no
issue and writes no state row (BLO-26613). Only the delivery's reported
outcome changes. The throw is reachable solely from the firing path --
handleResolved never runs owner resolution -- so a dropped alert is still
firing and returns on Alertmanager's next repeat_interval; no resolve can
be stranded.

Transient faults are deliberately untouched and still fail the delivery,
preserving the silent-loss guard from BLO-20467.

Refs PEN-2581

Signed-off-by: Cto <cto@paperclip.blockcast.net>
---
 .../src/__tests__/worker.test.ts              | 93 ++++++++++++++++++-
 .../src/webhook-handler.ts                    | 64 +++++++++++--
 2 files changed, 149 insertions(+), 8 deletions(-)

diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
index 92394e57d7e4..fecd5fd5d7c8 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
@@ -669,9 +669,13 @@ describe("handleWebhook — firing first time", () => {
   it("fails closed when the fallback agent configuration is missing", async () => {
     const { ctx, mocks } = mkCtx();
     const config = baseConfig({ ownerMap: {}, fallbackAgentName: undefined });
+    // The fail-closed guarantee (BLO-26613) is unchanged and still asserted
+    // below: no ownerless issue, no state row. PEN-2581 changed only how the
+    // drop is *reported* — an unresolvable owner is a config/roster fact no
+    // retry can fix, so the delivery acknowledges it instead of failing.
     await expect(
       handleWebhook(ctx, config, true, baseInput()),
-    ).rejects.toBeInstanceOf(AlertDeliveryIncompleteError);
+    ).resolves.toBeUndefined();
     expect(mocks.issues.create).not.toHaveBeenCalled();
     expect(mocks.state.set).not.toHaveBeenCalled();
     expect(mocks.metrics.write).toHaveBeenCalledWith(
@@ -679,6 +683,93 @@ describe("handleWebhook — firing first time", () => {
       1,
       { alertname: "CiliumPolicyDropsHigh", severity: "critical" },
     );
+    expect(mocks.metrics.write).toHaveBeenCalledWith(
+      "alertmanager.alert.permanent_error",
+      1,
+      { alertname: "CiliumPolicyDropsHigh" },
+    );
+  });
+
+  it("does not let one ownerless alert dark-tier the rest of its batch", async () => {
+    // The PEN-2581 outage in one test: a single alert with no resolvable owner
+    // threw, the throw was accumulated into AlertDeliveryIncompleteError, and
+    // the whole delivery 502'd — so every *other* alert in the batch was lost
+    // too, and Alertmanager retried the doomed batch 15-17× before dropping it.
+    const { ctx, mocks } = mkCtx();
+    const config = baseConfig({ fallbackAgentName: undefined });
+    // team=platform resolves through ownerMap; team=storage is unmapped, and
+    // with no fallbackAgentName it is permanently ownerless.
+    mocks.users.findByEmail.mockResolvedValue({
+      id: "user-42",
+      email: "alice@example.com",
+      name: "Alice",
+    });
+    const owned = baseAlert({
+      labels: {
+        alertname: "CiliumPolicyDropsHigh",
+        severity: "critical",
+        team: "platform",
+        node: "pve-3",
+      },
+      fingerprint: "aaaa1111",
+    });
+    const ownerless = baseAlert({
+      labels: {
+        alertname: "CephOsdNearFull",
+        severity: "critical",
+        team: "storage",
+        node: "pve-4",
+      },
+      fingerprint: "bbbb2222",
+    });
+
+    await expect(
+      handleWebhook(
+        ctx,
+        config,
+        true,
+        baseInput({
+          parsedBody: baseEnvelope({ alerts: [ownerless, owned] }),
+        }),
+      ),
+    ).resolves.toBeUndefined();
+
+    // The healthy alert still became tracked work despite sharing a batch with
+    // the ownerless one — and it is ordered FIRST in the payload, so this also
+    // pins that the drop does not abort the remainder of the loop.
+    expect(mocks.issues.create).toHaveBeenCalledTimes(1);
+    expect(mocks.issues.create.mock.calls[0][0].title).toBe(
+      "[critical] CiliumPolicyDropsHigh · platform",
+    );
+    expect(mocks.metrics.write).toHaveBeenCalledWith(
+      "alertmanager.alert.permanent_error",
+      1,
+      { alertname: "CephOsdNearFull" },
+    );
+  });
+
+  it("still fails the delivery for a transient per-alert fault", async () => {
+    // Control for the two tests above: the permanent carve-out must not have
+    // widened into "swallow every per-alert failure". A transient fault still
+    // owes Alertmanager a retry, so it still fails the delivery and still
+    // reports through the transient metric (BLO-20467's silent-loss guard).
+    const { ctx, mocks } = mkCtx();
+    const config = baseConfig();
+    mocks.issues.create.mockRejectedValueOnce(new Error("issue RPC timed out"));
+
+    await expect(
+      handleWebhook(ctx, config, true, baseInput()),
+    ).rejects.toBeInstanceOf(AlertDeliveryIncompleteError);
+    expect(mocks.metrics.write).toHaveBeenCalledWith(
+      "alertmanager.alert.error",
+      1,
+      { alertname: "CiliumPolicyDropsHigh" },
+    );
+    expect(mocks.metrics.write).not.toHaveBeenCalledWith(
+      "alertmanager.alert.permanent_error",
+      1,
+      expect.anything(),
+    );
   });
 
   it("joins an active aggregate winner before requiring fallback ownership", async () => {
diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
index 68cd26712588..05e0d8b59000 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
@@ -65,6 +65,36 @@ export class AlertDeliveryIncompleteError extends Error {
   }
 }
 
+/**
+ * Raised by a per-alert path whose failure no retry can fix — a configuration
+ * or roster fact rather than process state.
+ *
+ * The per-alert catch treats these as *handled*: the alert is dropped, the
+ * failure is recorded (log + metric), and the fingerprint is deliberately NOT
+ * added to `failedFingerprints`, so the delivery still answers 200.
+ *
+ * This is the same "log + 200" treatment the malformed-payload and
+ * permanent-policy drops already get. It exists because the taxonomy the
+ * per-alert catch was written against — "these failures are issue-RPC,
+ * state-store, event, and metric errors, which are transient" — stopped being
+ * true once `handleFiring` began throwing on unresolvable fallback ownership
+ * (PEN-2581). Reporting a permanent fault through the transient channel makes
+ * Alertmanager retry it 15-17× and drop the delivery anyway, and the resulting
+ * `alertmanager_notifications_failed_total` storm masks concurrent *transient*
+ * failures that retrying would genuinely have fixed.
+ *
+ * Only reachable from the firing path (owner resolution is never run on
+ * resolve), so a dropped alert that is still firing returns on Alertmanager's
+ * next `repeat_interval` — this trades a doomed retry burst for a later
+ * re-delivery, not for silent permanent loss.
+ */
+export class PermanentAlertError extends Error {
+  constructor(message: string) {
+    super(message);
+    this.name = "PermanentAlertError";
+  }
+}
+
 const AGGREGATE_CREATION_CLAIMS_TABLE = "alertmanager_aggregate_creation_claims";
 const AGGREGATE_MEMBERS_TABLE = "alertmanager_aggregate_members";
 const AGGREGATE_LIFECYCLE_FENCES_TABLE = "alertmanager_aggregate_lifecycle_fences";
@@ -1540,7 +1570,7 @@ export async function handleFiring(
       alertname,
       severity,
     });
-    throw new Error(
+    throw new PermanentAlertError(
       `Fallback owner resolution failed for ${alertname}; refusing ownerless issue creation`,
     );
   }
@@ -2227,17 +2257,37 @@ export async function handleWebhook(
       // so Alertmanager stopped retrying and the alert was destroyed with no
       // durable issue or state row — the same silent-loss class as the outage
       // this plugin already suffered (BLO-20467).
+      //
+      // `PermanentAlertError` is the one documented exception to that taxonomy:
+      // a config/roster fault no retry can fix, so it takes the same "log + 200"
+      // route as the malformed payload above instead of the transient-retry
+      // route. Retrying it burns Alertmanager's 15-17 attempts, drops the
+      // delivery anyway, and storms the failure metric that transient faults
+      // need to stay legible. See the class doc (PEN-2581).
+      const permanent = err instanceof PermanentAlertError;
       ctx.logger.error(
-        `paperclip-plugin-alertmanager: error processing alert ${alert.fingerprint}: ${String(err)}`,
+        permanent
+          ? `paperclip-plugin-alertmanager: permanently dropping alert ${alert.fingerprint}: ${String(err)} — no retry can resolve this, so the delivery is not failed`
+          : `paperclip-plugin-alertmanager: error processing alert ${alert.fingerprint}: ${String(err)}`,
       );
-      failedFingerprints.push(alert.fingerprint);
+      if (!permanent) {
+        failedFingerprints.push(alert.fingerprint);
+      }
       try {
-        await ctx.metrics.write("alertmanager.alert.error", 1, {
-          alertname: alert.labels.alertname ?? "unknown",
-        });
+        await ctx.metrics.write(
+          permanent
+            ? "alertmanager.alert.permanent_error"
+            : "alertmanager.alert.error",
+          1,
+          {
+            alertname: alert.labels.alertname ?? "unknown",
+          },
+        );
       } catch (metricErr) {
         // Telemetry is best-effort; a metrics outage must not be the thing that
-        // aborts the remaining alerts. The delivery already counts as failed.
+        // aborts the remaining alerts. The delivery's outcome is already
+        // decided either way — failed for a transient fault, 200 for a
+        // permanent one.
         ctx.logger.error(
           `paperclip-plugin-alertmanager: failed to record alert error metric for ${alert.fingerprint}: ${String(metricErr)}`,
         );

From 52199d32bd6ffa834190c524368209dafc023364 Mon Sep 17 00:00:00 2001
From: Cto <cto@paperclip.blockcast.net>
Date: Fri, 4 Sep 2026 06:59:23 +0000
Subject: [PATCH 2/5] fix(alertmanager): classify a refusal transient when the
 owner self-clears
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Addresses both Important findings from Ally's review of #1621.

1. `PermanentAlertError` was raised for every unresolvable fallback owner,
   but only `terminated` / wrong-name / genuinely-ambiguous is a config or
   roster fact. `paused`, `pending_approval` and `invalid_org_chain` are
   process state: an unpause, a board approval, or a restored manager flips
   them with nobody editing config. Dropping those at 200 gave up
   Alertmanager's retry window, which is the only thing that lets an alert
   land within minutes of the pause lifting rather than waiting out a whole
   `repeat_interval` — a time-to-detect regression for `critical` alerts.

   `resolveFallbackAgentId` now returns `FallbackOwnerResolution` — either an
   `agentId` or a `refusal` classified from the `invokabilityReason` that
   `getAgentWorkEligibility` already computes. The mapping is an exhaustive
   `Record`, so adding a lifecycle reason fails to compile here instead of
   silently defaulting into either class. `unknown_status` is transient by
   design: misclassifying transient-as-permanent drops an alert, while
   permanent-as-transient only costs a retry burst, so the unrecognised case
   takes the survivable error. The unreachable `eligible` entry maps the same
   way for the same reason.

   Where several same-named agents are all non-invokable, any one
   self-clearing candidate makes the whole refusal transient.

2. The `alertmanager.owner.fallback_failed` metric write immediately before
   the throw was unguarded. A metrics outage would surface a *metrics* error
   instead of `PermanentAlertError`, the per-alert catch would treat it as
   transient and push the fingerprint, and the delivery would 502 —
   reinstating the doomed retry burst this path exists to remove and taking
   the rest of the batch down with it. It is now best-effort with an ERROR
   log, matching the severity-floor and opt-out drops.

The refusal class is also named in the operator-facing warning alongside the
per-agent `invokabilityReason`, so the next occurrence is diagnosable from
the log alone (PEN-2581, where the holding sub-cause was never recoverable
from any signal the fault emitted).

Fail-closed is unchanged: an ownerless alert still creates no issue and
writes no state row (BLO-26613). Only the reported outcome differs.

Tests: the per-status resolver cases become a (status, class) table rather
than one `it.each` over statuses, which would have asserted only "refuses"
and lost the distinction. Two new delivery-outcome tests cover a paused
owner keeping the retry window and a permanent drop surviving a metrics
outage; both were confirmed to fail under targeted mutation (`paused` flipped
to permanent; the guard removed). 286 tests pass, `tsc --noEmit` clean.

Refs: PEN-2581

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Cto <cto@paperclip.blockcast.net>
---
 .../src/__tests__/owner-resolver.test.ts      | 70 ++++++++------
 .../src/__tests__/worker.test.ts              | 72 ++++++++++++++
 .../src/owner-resolver.ts                     | 94 ++++++++++++++++---
 .../src/webhook-handler.ts                    | 56 +++++++----
 4 files changed, 235 insertions(+), 57 deletions(-)

diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts
index c36bb89cdcc6..016203b04b7c 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts
@@ -383,7 +383,7 @@ describe("resolveFallbackAgentId", () => {
         "company-1",
         "alert fallback",
       ),
-    ).resolves.toBe("agent-fallback");
+    ).resolves.toEqual({ agentId: "agent-fallback" });
     expect(agents.list).toHaveBeenCalledWith({ companyId: "company-1" });
   });
 
@@ -397,12 +397,14 @@ describe("resolveFallbackAgentId", () => {
     const logger = { warn: vi.fn() };
     const ctx = { agents, logger } as unknown as Parameters<typeof resolveFallbackAgentId>[0];
 
+    // Both are config facts a retry cannot change, so both must be `permanent`:
+    // two same-named invokable agents stay ambiguous, and a wrong name stays wrong.
     await expect(
       resolveFallbackAgentId(ctx, "company-1", "Alert Fallback"),
-    ).resolves.toBeUndefined();
+    ).resolves.toEqual({ refusal: "permanent" });
     await expect(
       resolveFallbackAgentId(ctx, "company-1", "Missing Agent"),
-    ).resolves.toBeUndefined();
+    ).resolves.toEqual({ refusal: "permanent" });
     expect(logger.warn).toHaveBeenCalledTimes(2);
   });
 });
@@ -518,45 +520,45 @@ describe("resolveFallbackAgentId", () => {
     ]);
     await expect(
       resolveFallbackAgentId(ctx, "company-1", "Ops Triage"),
-    ).resolves.toBe("agent-1");
+    ).resolves.toEqual({ agentId: "agent-1" });
   });
 
   it("matches case-insensitively after trimming", async () => {
     const { ctx } = mkAgentCtx([{ id: "agent-1", name: "  Ops Triage " }]);
     await expect(
       resolveFallbackAgentId(ctx, "company-1", "ops triage"),
-    ).resolves.toBe("agent-1");
+    ).resolves.toEqual({ agentId: "agent-1" });
   });
 
-  it("returns undefined without querying when no name is configured", async () => {
+  it("refuses permanently without querying when no name is configured", async () => {
     const { ctx, list } = mkAgentCtx([{ id: "agent-1", name: "Ops Triage" }]);
     await expect(
       resolveFallbackAgentId(ctx, "company-1", undefined),
-    ).resolves.toBeUndefined();
+    ).resolves.toEqual({ refusal: "permanent" });
     await expect(
       resolveFallbackAgentId(ctx, "company-1", "   "),
-    ).resolves.toBeUndefined();
+    ).resolves.toEqual({ refusal: "permanent" });
     expect(list).not.toHaveBeenCalled();
   });
 
-  it("returns undefined and warns when the name matches nothing", async () => {
+  it("refuses permanently and warns when the name matches nothing", async () => {
     const { ctx, logger } = mkAgentCtx([{ id: "agent-1", name: "Ops Triage" }]);
     await expect(
       resolveFallbackAgentId(ctx, "company-1", "Nobody"),
-    ).resolves.toBeUndefined();
+    ).resolves.toEqual({ refusal: "permanent" });
     expect(logger.warn).toHaveBeenCalledWith(
       expect.stringContaining("resolved to 0 invokable agents"),
     );
   });
 
-  it("returns undefined and warns when the name is ambiguous", async () => {
+  it("refuses permanently and warns when the name is ambiguous", async () => {
     const { ctx, logger } = mkAgentCtx([
       { id: "agent-1", name: "Ops Triage" },
       { id: "agent-2", name: "ops triage" },
     ]);
     await expect(
       resolveFallbackAgentId(ctx, "company-1", "Ops Triage"),
-    ).resolves.toBeUndefined();
+    ).resolves.toEqual({ refusal: "permanent" });
     expect(logger.warn).toHaveBeenCalledWith(
       expect.stringContaining("resolved to 2 invokable agents"),
     );
@@ -576,20 +578,26 @@ describe("resolveFallbackAgentId", () => {
   // it or the ownerless-issue metric reads clean while the harm continues.
   // -------------------------------------------------------------------------
 
-  it.each(["paused", "pending_approval", "terminated"])(
-    "refuses a sole name match that is %s",
-    async (status) => {
-      const { ctx, logger } = mkAgentCtx([
-        { id: "agent-1", name: "Ops Triage", status },
-      ]);
-      await expect(
-        resolveFallbackAgentId(ctx, "company-1", "Ops Triage"),
-      ).resolves.toBeUndefined();
-      expect(logger.warn).toHaveBeenCalledWith(
-        expect.stringContaining("none are invokable"),
-      );
-    },
-  );
+  // Each status carries its own refusal class, so these are a table of
+  // (status, class) pairs rather than one shared `it.each` over statuses: a
+  // paused owner becomes invokable on its own and must keep Alertmanager's
+  // retry window, while a terminated one needs a roster edit and must not.
+  // Bundling them would assert only "refuses" and lose the distinction.
+  it.each([
+    ["paused", "transient"],
+    ["pending_approval", "transient"],
+    ["terminated", "permanent"],
+  ])("refuses a sole name match that is %s as %s", async (status, refusal) => {
+    const { ctx, logger } = mkAgentCtx([
+      { id: "agent-1", name: "Ops Triage", status },
+    ]);
+    await expect(
+      resolveFallbackAgentId(ctx, "company-1", "Ops Triage"),
+    ).resolves.toEqual({ refusal });
+    expect(logger.warn).toHaveBeenCalledWith(
+      expect.stringContaining("none are invokable"),
+    );
+  });
 
   it("names the blocking reason rather than reporting an unmatched name", async () => {
     // "paused" and "name is wrong" need different fixes, so an operator must
@@ -611,7 +619,7 @@ describe("resolveFallbackAgentId", () => {
       ]);
       await expect(
         resolveFallbackAgentId(ctx, "company-1", "Ops Triage"),
-      ).resolves.toBe("agent-1");
+      ).resolves.toEqual({ agentId: "agent-1" });
     },
   );
 
@@ -624,7 +632,7 @@ describe("resolveFallbackAgentId", () => {
     ]);
     await expect(
       resolveFallbackAgentId(ctx, "company-1", "Ops Triage"),
-    ).resolves.toBe("agent-live");
+    ).resolves.toEqual({ agentId: "agent-live" });
   });
 
   it("still refuses when two invokable agents share the name", async () => {
@@ -634,7 +642,7 @@ describe("resolveFallbackAgentId", () => {
     ]);
     await expect(
       resolveFallbackAgentId(ctx, "company-1", "Ops Triage"),
-    ).resolves.toBeUndefined();
+    ).resolves.toEqual({ refusal: "permanent" });
     expect(logger.warn).toHaveBeenCalledWith(
       expect.stringContaining("resolved to 2 invokable agents"),
     );
@@ -652,9 +660,11 @@ describe("resolveFallbackAgentId", () => {
         reportsTo: "agent-gone",
       } as { id: string; name: string; status: string },
     ]);
+    // Transient, not permanent: the chain becomes valid again when the manager
+    // upstream is restored, with nobody editing this plugin's config.
     await expect(
       resolveFallbackAgentId(ctx, "company-1", "Ops Triage"),
-    ).resolves.toBeUndefined();
+    ).resolves.toEqual({ refusal: "transient" });
     expect(logger.warn).toHaveBeenCalledWith(
       expect.stringContaining("none are invokable"),
     );
diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
index fecd5fd5d7c8..cd5bf39bf3dc 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
@@ -748,6 +748,78 @@ describe("handleWebhook — firing first time", () => {
     );
   });
 
+  it("keeps the retry window when the fallback agent is only paused", async () => {
+    // The counterpart to the permanent drop above, and the case that makes the
+    // refusal *class* load-bearing rather than cosmetic. A paused fallback owner
+    // becomes invokable again with nobody editing config, so Alertmanager's
+    // 15-17 retries are the only thing that lets the alert land within minutes
+    // of the unpause instead of waiting out a whole `repeat_interval`. Dropping
+    // it at 200 here would be a time-to-detect regression for a critical alert.
+    const { ctx, mocks } = mkCtx();
+    const config = baseConfig({ ownerMap: {} });
+    mocks.agents.list.mockResolvedValue([
+      { id: "agent-fallback", name: "Alert Fallback", status: "paused" },
+    ]);
+
+    await expect(
+      handleWebhook(ctx, config, true, baseInput()),
+    ).rejects.toBeInstanceOf(AlertDeliveryIncompleteError);
+
+    // Fail-closed is still intact — a paused owner is no more assignable than a
+    // terminated one; only the reporting channel differs.
+    expect(mocks.issues.create).not.toHaveBeenCalled();
+    expect(mocks.state.set).not.toHaveBeenCalled();
+    // Reported as transient, NOT permanent.
+    expect(mocks.metrics.write).toHaveBeenCalledWith(
+      "alertmanager.alert.error",
+      1,
+      { alertname: "CiliumPolicyDropsHigh" },
+    );
+    expect(mocks.metrics.write).not.toHaveBeenCalledWith(
+      "alertmanager.alert.permanent_error",
+      1,
+      expect.anything(),
+    );
+    // The operator-facing warning names the blocking reason, so the next
+    // occurrence is diagnosable from the log alone (PEN-2581).
+    expect(mocks.logger.warn).toHaveBeenCalledWith(
+      expect.stringContaining("agent-fallback=paused"),
+    );
+  });
+
+  it("still acknowledges a permanent drop when the metrics write fails", async () => {
+    // The permanent drop's own invariant must not depend on telemetry being up.
+    // If the `fallback_failed` write threw, handleFiring would surface a
+    // *metrics* error instead of PermanentAlertError, the per-alert catch would
+    // treat it as transient and push the fingerprint, and the delivery would
+    // 502 — reinstating the doomed retry burst this path exists to remove, and
+    // taking the rest of the batch down with it.
+    const { ctx, mocks } = mkCtx();
+    const config = baseConfig({ ownerMap: {}, fallbackAgentName: undefined });
+    mocks.metrics.write.mockImplementation(async (name: string) => {
+      if (name === "alertmanager.owner.fallback_failed") {
+        throw new Error("metrics backend unavailable");
+      }
+    });
+
+    await expect(
+      handleWebhook(ctx, config, true, baseInput()),
+    ).resolves.toBeUndefined();
+
+    expect(mocks.issues.create).not.toHaveBeenCalled();
+    expect(mocks.state.set).not.toHaveBeenCalled();
+    // The permanent classification survived the telemetry outage.
+    expect(mocks.metrics.write).toHaveBeenCalledWith(
+      "alertmanager.alert.permanent_error",
+      1,
+      { alertname: "CiliumPolicyDropsHigh" },
+    );
+    // And the swallowed metrics failure is still audible in the log.
+    expect(mocks.logger.error).toHaveBeenCalledWith(
+      expect.stringContaining("failed to record fallback owner metric"),
+    );
+  });
+
   it("still fails the delivery for a transient per-alert fault", async () => {
     // Control for the two tests above: the permanent carve-out must not have
     // widened into "swallow every per-alert failure". A transient fault still
diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts b/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts
index 1a0b2c4f6ff4..5302df4001d5 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts
@@ -7,6 +7,7 @@
 
 import type { PluginContext } from "@paperclipai/plugin-sdk";
 import { getAgentWorkEligibility } from "@paperclipai/shared";
+import type { AgentEligibilityLifecycleReason } from "@paperclipai/shared";
 import {
   ASSIGNEE_OVERRIDE_ANNOTATION,
   ASSIGNEE_OVERRIDE_LABEL,
@@ -176,12 +177,12 @@ export async function resolveAssigneeUserId(
  * can review in a config diff. That makes the lookup ambiguous in principle,
  * so anything other than exactly one match is refused: zero matches means the
  * name is wrong, and more than one means the caller cannot know which agent
- * the operator meant. Both return `undefined`, and the caller fails closed
- * rather than filing an ownerless issue.
+ * the operator meant. Both refuse, and the caller fails closed rather than
+ * filing an ownerless issue.
  *
- * Returning `undefined` for blank/absent config is deliberate: an instance
- * with no `fallbackAgentName` at all is a misconfiguration for this plugin,
- * and the caller — not this resolver — decides how loudly to fail.
+ * Refusing for blank/absent config is deliberate: an instance with no
+ * `fallbackAgentName` at all is a misconfiguration for this plugin, and the
+ * caller — not this resolver — decides how loudly to fail.
  *
  * A name match is not enough: the agent must also be *invokable*. The host's
  * `agents.list` filters only `terminated` (`server/src/services/agents.ts`), so
@@ -195,14 +196,71 @@ export async function resolveAssigneeUserId(
  * in step with what `invoke` actually enforces (including an invalid reporting
  * chain, which blocks invoke just as surely as a paused status). The whole
  * company snapshot is already in hand, which is exactly the input it needs.
+ *
+ * A refusal is additionally classified `permanent` or `transient`, because the
+ * caller reports the two through different channels: a permanent refusal is
+ * dropped with a 200 (retrying cannot help), while a transient one keeps
+ * Alertmanager's retry window, which is the only thing that lets an alert land
+ * within minutes of a pause lifting. See `REFUSAL_CLASS_BY_INVOKABILITY_REASON`.
+ */
+export type FallbackOwnerRefusal = "permanent" | "transient";
+
+/**
+ * Either a resolved fallback owner, or a refusal with its class. Exactly one
+ * field is ever set — the caller checks `agentId` first and only consults
+ * `refusal` on the miss.
+ */
+export type FallbackOwnerResolution =
+  | { agentId: string; refusal?: undefined }
+  | { agentId?: undefined; refusal: FallbackOwnerRefusal };
+
+/**
+ * Which non-invokable reasons clear on their own, and which need a human to
+ * change config or the roster.
+ *
+ * `paused` and `pending_approval` are process state: an unpause or a board
+ * approval flips them with nobody editing config, and `invalid_org_chain`
+ * clears when the manager upstream is restored. Only `terminated` is the
+ * roster fact that no amount of waiting fixes.
+ *
+ * `unknown_status` is deliberately transient: it means this resolver does not
+ * recognise the status, so it cannot claim the condition is unfixable.
+ * Misclassifying transient-as-permanent drops an alert; permanent-as-transient
+ * only costs a retry burst, so the unknown case takes the survivable error.
+ *
+ * Declared as an exhaustive `Record` rather than a `Set` of the permanent ones
+ * so that adding a reason to `AgentEligibilityLifecycleReason` fails to compile
+ * here instead of silently defaulting a new condition into either class.
+ */
+const REFUSAL_CLASS_BY_INVOKABILITY_REASON: Record<
+  AgentEligibilityLifecycleReason,
+  FallbackOwnerRefusal
+> = {
+  terminated: "permanent",
+  paused: "transient",
+  pending_approval: "transient",
+  invalid_org_chain: "transient",
+  unknown_status: "transient",
+  // Never reached: an `eligible` agent is invokable and so never refused.
+  // Present only to keep the record exhaustive. Mapped to `transient` rather
+  // than `permanent` so that the unreachable case, if a future refactor ever
+  // does reach it, fails in the same survivable direction as `unknown_status`:
+  // a needless retry burst, not a dropped alert.
+  eligible: "transient",
+};
+
+/**
+ * Resolve the configured `fallbackAgentName` to exactly one invokable agent id,
+ * or explain why it could not.
  */
 export async function resolveFallbackAgentId(
   ctx: Pick<PluginContext, "agents" | "logger">,
   companyId: string,
   fallbackAgentName: string | undefined,
-): Promise<string | undefined> {
+): Promise<FallbackOwnerResolution> {
   const target = fallbackAgentName?.trim().toLowerCase();
-  if (!target) return undefined;
+  // No name configured at all: nothing resolves until someone edits config.
+  if (!target) return { refusal: "permanent" };
   // One unwindowed company-wide snapshot rather than a paged scan: the host's
   // list is unordered, so paging could drift a match across page boundaries
   // and turn a stable config into an intermittent ownerless-issue bug.
@@ -227,17 +285,31 @@ export async function resolveFallbackAgentId(
             `${entry.agent.id}=${entry.eligibility.invokabilityReason}`,
         )
         .join(", ");
+      // Any one self-clearing candidate makes the whole refusal transient: a
+      // paused duplicate alongside a terminated one still becomes resolvable
+      // the moment the pause lifts, with no config edit.
+      const refusal: FallbackOwnerRefusal = evaluated.some(
+        (entry) =>
+          REFUSAL_CLASS_BY_INVOKABILITY_REASON[
+            entry.eligibility.invokabilityReason
+          ] === "transient",
+      )
+        ? "transient"
+        : "permanent";
       ctx.logger.warn(
-        `Fallback agent "${fallbackAgentName}" matched ${evaluated.length} agent(s) but none are invokable (${reasons}); refusing ownerless issue creation`,
+        `Fallback agent "${fallbackAgentName}" matched ${evaluated.length} agent(s) but none are invokable (${reasons}); refusing ownerless issue creation (${refusal})`,
       );
-      return undefined;
+      return { refusal };
     }
     ctx.logger.warn(
       `Fallback agent "${fallbackAgentName}" resolved to ${invokable.length} invokable agents; refusing ownerless issue creation`,
     );
-    return undefined;
+    // Zero name matches (wrong name) and two or more invokable matches
+    // (genuinely ambiguous) are both config facts: neither changes on its own.
+    return { refusal: "permanent" };
   }
-  return invokable[0]?.agent.id;
+  const agentId = invokable[0]?.agent.id;
+  return agentId ? { agentId } : { refusal: "permanent" };
 }
 
 function normalizeEmail(email: string): string {
diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
index 05e0d8b59000..760bfcb65cb6 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
@@ -26,6 +26,7 @@ import {
 } from "./issue-mapping.js";
 import { resolveIssueRoute } from "./issue-route-resolver.js";
 import { resolveAssigneeUserId, resolveFallbackAgentId } from "./owner-resolver.js";
+import type { FallbackOwnerResolution } from "./owner-resolver.js";
 import { aggregateKeyForAlert } from "./aggregate-key.js";
 import { escalationDeadlineMs, recordSourceResolvedAndCloseCovers } from "./escalation.js";
 import {
@@ -1095,14 +1096,14 @@ function suppressionExpiryLabel(
  * delivery (rather than the module) is what keeps it correct: a config edit or
  * an agent being paused takes effect on the very next delivery.
  */
-export type FallbackOwnerMemo = Map<string, Promise<string | undefined>>;
+export type FallbackOwnerMemo = Map<string, Promise<FallbackOwnerResolution>>;
 
 function resolveFallbackAgentIdMemoized(
   ctx: Pick<PluginContext, "agents" | "logger">,
   companyId: string,
   fallbackAgentName: string | undefined,
   memo: FallbackOwnerMemo | undefined,
-): Promise<string | undefined> {
+): Promise<FallbackOwnerResolution> {
   if (!memo) return resolveFallbackAgentId(ctx, companyId, fallbackAgentName);
   // JSON-encoded pair rather than a naive `a + sep + b`: agent names are
   // operator-supplied config, so any single-character separator could be
@@ -1115,11 +1116,12 @@ function resolveFallbackAgentIdMemoized(
     companyId,
     fallbackAgentName,
   ).catch((err: unknown) => {
-    // Evict on failure. A refusal (bad name / paused / ambiguous) resolves to
-    // `undefined` and IS cached — it is a config fact, stable for the delivery.
-    // A *throw* is a transient host fault, and caching it would let one failed
-    // `agents.list` poison every remaining alert in the batch, converting a
-    // blip that previously cost one alert into a whole-delivery failure.
+    // Evict on failure. A refusal (bad name / paused / ambiguous) resolves to a
+    // `refusal` value and IS cached — the underlying condition is stable for the
+    // delivery, whether or not it is permanent beyond it. A *throw* is a
+    // transient host fault, and caching it would let one failed `agents.list`
+    // poison every remaining alert in the batch, converting a blip that
+    // previously cost one alert into a whole-delivery failure.
     memo.delete(key);
     throw err;
   });
@@ -1552,7 +1554,7 @@ export async function handleFiring(
         ? `agent:${resolution.agentId}`
         : resolution.email ?? "(none)";
   }
-  const fallbackAssigneeAgentId =
+  const fallbackResolution =
     retainedIssue || createAssigneeAgentId || createAssigneeUserId
       ? undefined
       : await resolveFallbackAgentIdMemoized(
@@ -1561,18 +1563,40 @@ export async function handleFiring(
           config.fallbackAgentName,
           fallbackOwnerMemo,
         );
+  const fallbackAssigneeAgentId = fallbackResolution?.agentId;
   const finalAssigneeAgentId = createAssigneeAgentId ?? fallbackAssigneeAgentId;
   if (!retainedIssue && !finalAssigneeAgentId && !createAssigneeUserId) {
+    // Only `terminated` / wrong-name / genuinely-ambiguous is unfixable by
+    // retrying. A `paused` or `pending_approval` fallback owner becomes
+    // invokable without anyone editing config, and Alertmanager's retry window
+    // is the only thing that lets the alert land within minutes of that rather
+    // than waiting out a whole `repeat_interval`. Absent a classification we
+    // take the transient branch: a needless retry burst is survivable, a
+    // wrongly-dropped alert is not.
+    const isPermanent = fallbackResolution?.refusal === "permanent";
     ctx.logger.warn(
-      `Cannot create issue for ${alertname}: fallbackAgentName is missing, invalid, or ambiguous`,
-    );
-    await ctx.metrics.write("alertmanager.owner.fallback_failed", 1, {
-      alertname,
-      severity,
-    });
-    throw new PermanentAlertError(
-      `Fallback owner resolution failed for ${alertname}; refusing ownerless issue creation`,
+      `Cannot create issue for ${alertname}: fallbackAgentName is missing, invalid, or ambiguous (${
+        isPermanent ? "permanent" : "transient"
+      })`,
     );
+    try {
+      await ctx.metrics.write("alertmanager.owner.fallback_failed", 1, {
+        alertname,
+        severity,
+      });
+    } catch (metricErr) {
+      // Best-effort, matching the severity-floor and opt-out drops above. On the
+      // permanent branch this is load-bearing: letting a metrics outage throw
+      // would surface a *metrics* error instead of `PermanentAlertError`, the
+      // per-alert catch would push the fingerprint, and the delivery would 502
+      // — reinstating exactly the doomed retry burst this path removes, and
+      // taking the rest of the batch down with it.
+      ctx.logger.error(
+        `paperclip-plugin-alertmanager: failed to record fallback owner metric for ${alert.fingerprint}: ${String(metricErr)}`,
+      );
+    }
+    const message = `Fallback owner resolution failed for ${alertname}; refusing ownerless issue creation`;
+    throw isPermanent ? new PermanentAlertError(message) : new Error(message);
   }
   const routeProjectId = nonEmptyString(issueRoute?.projectId);
   const routeGoalId = nonEmptyString(issueRoute?.goalId);

From d2393fb35afe5fd699a7ed5f63ca45ea5f5a2c35 Mon Sep 17 00:00:00 2001
From: Cto <cto@paperclip.blockcast.net>
Date: Fri, 4 Sep 2026 09:17:53 +0000
Subject: [PATCH 3/5] fix(alertmanager): correct the batch-isolation claim and
 harden the refusal default
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The batch-isolation test's comment claimed that before this change "every
other alert in the batch was lost too". That is wrong, and it was the
stated justification for widening a taxonomy a prior silent-loss outage
(BLO-20467) deliberately narrowed — so a future maintainer extending the
`PermanentAlertError` carve-out would have been building on a mechanism
the code does not have.

The per-alert catch in `handleWebhook` already isolated the batch: the
catch is inside the loop, `AlertDeliveryIncompleteError` is thrown only
after it completes, so a sibling alert's issue was created on the first
attempt even when the delivery reported 502. Confirmed empirically by
neutralising the carve-out — the only assertion that fails is
`resolves.toBeUndefined()`; the `issues.create` assertions pass on the
pre-change source. What actually changed is the delivery's reported
outcome, which is now also what the test is named for.

The production incident did dark-tier every alert, but for an
incident-specific reason: with `fallbackAgentName` unset, every unmapped
alert took the same throw, so there were no healthy siblings to survive.
The comment now says that rather than generalising it.

Also, from the same review:

- `resolveFallbackAgentId` tests the refusal class with `!== "permanent"`
  rather than `=== "transient"`, so a reason missing from the map at
  runtime lands on the survivable branch instead of dropping the alert.
  The exhaustive `Record` makes that unreachable in-repo, but the plugin
  resolves `getAgentWorkEligibility` from `@paperclipai/shared` at
  runtime, so a built plugin against a newer host could see a reason its
  own map never had. Matches the documented intent at zero cost.
- `alertmanager.owner.fallback_failed` carries a `refusal` label, so the
  drop-vs-retry split is queryable without joining against
  `alertmanager.alert.permanent_error`.
- The batch-isolation test now pins the other half of BLO-26613: the
  ownerless alert leaves no `alert:` state row. Filtered by key rather
  than counting `state.set`, because the healthy alert's owner lookup
  also memoises `owner-by-email:…` — a raw count would be 2 and would
  couple a fail-closed assertion to an unrelated cache.

Refs PEN-2581.

Signed-off-by: Cto <cto@paperclip.blockcast.net>
---
 .../src/__tests__/worker.test.ts              | 55 ++++++++++++++++---
 .../src/owner-resolver.ts                     | 12 +++-
 .../src/webhook-handler.ts                    |  8 +++
 3 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
index cd5bf39bf3dc..cb1069aa191d 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
@@ -681,7 +681,11 @@ describe("handleWebhook — firing first time", () => {
     expect(mocks.metrics.write).toHaveBeenCalledWith(
       "alertmanager.owner.fallback_failed",
       1,
-      { alertname: "CiliumPolicyDropsHigh", severity: "critical" },
+      {
+        alertname: "CiliumPolicyDropsHigh",
+        severity: "critical",
+        refusal: "permanent",
+      },
     );
     expect(mocks.metrics.write).toHaveBeenCalledWith(
       "alertmanager.alert.permanent_error",
@@ -690,11 +694,28 @@ describe("handleWebhook — firing first time", () => {
     );
   });
 
-  it("does not let one ownerless alert dark-tier the rest of its batch", async () => {
-    // The PEN-2581 outage in one test: a single alert with no resolvable owner
-    // threw, the throw was accumulated into AlertDeliveryIncompleteError, and
-    // the whole delivery 502'd — so every *other* alert in the batch was lost
-    // too, and Alertmanager retried the doomed batch 15-17× before dropping it.
+  it("drops one ownerless alert without aborting the loop or failing the delivery", async () => {
+    // What this pins, stated precisely, because the obvious reading is wrong:
+    // the per-alert catch in `handleWebhook` ALREADY kept the rest of the batch
+    // processing before PEN-2581. The catch is inside the loop and
+    // `AlertDeliveryIncompleteError` is thrown only after it completes, so a
+    // sibling alert's issue was created on the first attempt even when the
+    // delivery reported 502. Verified, not assumed: neutralise the carve-out
+    // and the only assertion that fails is `resolves.toBeUndefined()` below —
+    // the `issues.create` assertions pass on the pre-change source.
+    //
+    // What PEN-2581 actually changed is the delivery's reported *outcome*: the
+    // ownerless fingerprint is no longer accumulated, so Alertmanager is no
+    // longer told to retry a batch 15-17× for a fault no retry can fix, and the
+    // resulting failure storm no longer masks genuinely-transient failures that
+    // retrying would have fixed.
+    //
+    // The production incident did dark-tier every alert, but for an
+    // incident-specific reason rather than a structural one: with
+    // `fallbackAgentName` unset, *every* unmapped alert in the batch took this
+    // same throw, so there were no healthy siblings left to survive. That case
+    // is real and reachable — it is just not what "one ownerless alert" does to
+    // a mixed batch, which is what this test covers.
     const { ctx, mocks } = mkCtx();
     const config = baseConfig({ fallbackAgentName: undefined });
     // team=platform resolves through ownerMap; team=storage is unmapped, and
@@ -734,13 +755,29 @@ describe("handleWebhook — firing first time", () => {
       ),
     ).resolves.toBeUndefined();
 
-    // The healthy alert still became tracked work despite sharing a batch with
-    // the ownerless one — and it is ordered FIRST in the payload, so this also
-    // pins that the drop does not abort the remainder of the loop.
+    // The healthy alert still became tracked work — and it is ordered SECOND in
+    // the payload, behind the ownerless one, so this pins that the permanent
+    // drop does not abort the remainder of the loop.
     expect(mocks.issues.create).toHaveBeenCalledTimes(1);
     expect(mocks.issues.create.mock.calls[0][0].title).toBe(
       "[critical] CiliumPolicyDropsHigh · platform",
     );
+    // The other half of the BLO-26613 fail-closed guarantee, asserted here and
+    // not only in the single-alert tests: the ownerless alert must leave no
+    // state row behind, and a multi-alert batch is where that is easiest to
+    // regress.
+    //
+    // Filtered to `alert:` keys rather than counting `state.set` calls
+    // outright: the healthy alert's owner lookup also memoises
+    // `owner-by-email:…` on the instance scope, so a raw count would be 2 and
+    // would couple this fail-closed assertion to an unrelated cache. Keying on
+    // the fingerprint says the thing we actually mean — one alert row, and it
+    // belongs to the alert that got an issue.
+    const alertStateWrites = mocks.state.set.mock.calls.filter((call) =>
+      String(call[0].stateKey).startsWith("alert:"),
+    );
+    expect(alertStateWrites).toHaveLength(1);
+    expect(alertStateWrites[0][0].stateKey).toBe(`alert:${owned.fingerprint}`);
     expect(mocks.metrics.write).toHaveBeenCalledWith(
       "alertmanager.alert.permanent_error",
       1,
diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts b/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts
index 5302df4001d5..06d07c943de4 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts
@@ -288,11 +288,21 @@ export async function resolveFallbackAgentId(
       // Any one self-clearing candidate makes the whole refusal transient: a
       // paused duplicate alongside a terminated one still becomes resolvable
       // the moment the pause lifts, with no config edit.
+      //
+      // Tested as `!== "permanent"` rather than `=== "transient"` so that the
+      // *runtime* default matches the documented policy above: a reason missing
+      // from the map indexes to `undefined`, and only this direction lands that
+      // on the survivable branch. The exhaustive `Record` makes a gap
+      // unreachable within the monorepo, but the plugin resolves
+      // `getAgentWorkEligibility` from `@paperclipai/shared` at runtime, so a
+      // built plugin running against a newer host could see a reason its own
+      // copy of the map never had. That skew must not silently start dropping
+      // alerts.
       const refusal: FallbackOwnerRefusal = evaluated.some(
         (entry) =>
           REFUSAL_CLASS_BY_INVOKABILITY_REASON[
             entry.eligibility.invokabilityReason
-          ] === "transient",
+          ] !== "permanent",
       )
         ? "transient"
         : "permanent";
diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
index 760bfcb65cb6..2e8aeaa3cc1e 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
@@ -1580,9 +1580,17 @@ export async function handleFiring(
       })`,
     );
     try {
+      // `refusal` splits the two outcomes this metric otherwise conflates: a
+      // permanent refusal is dropped at 200 and will not be retried, a
+      // transient one keeps Alertmanager's retry window. Without the label an
+      // operator has to join this series against
+      // `alertmanager.alert.permanent_error` to tell "gone until someone edits
+      // config" from "retrying, may still land". Two values, so no meaningful
+      // cardinality cost.
       await ctx.metrics.write("alertmanager.owner.fallback_failed", 1, {
         alertname,
         severity,
+        refusal: isPermanent ? "permanent" : "transient",
       });
     } catch (metricErr) {
       // Best-effort, matching the severity-floor and opt-out drops above. On the

From e3ca49486914bbaf4657092afbbc3279bd6e32cc Mon Sep 17 00:00:00 2001
From: Cto <cto@paperclip.blockcast.net>
Date: Fri, 4 Sep 2026 20:54:43 +0000
Subject: [PATCH 4/5] test(alertmanager): pin the transient refusal value and
 unknown_status
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Addresses three suggestions from Ally's review of #1621 (0 Critical, 0
Important). All three are test/comment-only; no source behaviour changes.

- Pin `refusal: "transient"` on `alertmanager.owner.fallback_failed` in the
  paused test. The label was asserted only on its "permanent" value, so a
  regression that hardcoded `"permanent"` at the write site passed the whole
  suite — splitting drop-from-retry within one series is the label's entire
  purpose. Mutation-checked: hardcoding `"permanent"` now fails this test.

- Add the `unknown_status` case to owner-resolver.test.ts. It was the only
  `REFUSAL_CLASS_BY_INVOKABILITY_REASON` entry with no coverage, and it is the
  one encoding the transient-default asymmetry policy, so it is the most
  exposed to a future tidy-up reading it as an arbitrary default.
  Mutation-checked: flipping it to `"permanent"` now fails.

- Correct the batch-isolation comment's account of how it was checked. The
  previous wording described an experiment Vitest cannot run — it aborts a test
  at the first failing assertion, so a run tripping the outcome assertion never
  reaches the `issues.create` assertions and cannot observe them passing, and
  the `permanent_error` assertion would also fail on pre-change source. The
  conclusion was right; only the method was misstated. Replaced with the
  experiment actually run, named precisely enough to reproduce.

Verified: pnpm typecheck clean; full plugin suite 287/287 green.
Signed-off-by: Cto <cto@paperclip.blockcast.net>
---
 .../src/__tests__/owner-resolver.test.ts      | 19 ++++++++++++++
 .../src/__tests__/worker.test.ts              | 26 ++++++++++++++++---
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts
index 016203b04b7c..9b9374c1f595 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts
@@ -599,6 +599,25 @@ describe("resolveFallbackAgentId", () => {
     );
   });
 
+  it("refuses a status it does not recognise as transient", async () => {
+    // The one `REFUSAL_CLASS_BY_INVOKABILITY_REASON` entry with no other
+    // coverage, and the one that encodes the asymmetry policy stated at
+    // `owner-resolver.ts:226-229`: an unrecognised status means this resolver
+    // cannot claim the condition is unfixable, so it takes the survivable
+    // branch. Pinned so a later tidy-up does not read it as an arbitrary
+    // default and flip it — misclassifying transient-as-permanent drops an
+    // alert, which is the single outcome this whole path exists to prevent.
+    const { ctx, logger } = mkAgentCtx([
+      { id: "agent-1", name: "Ops Triage", status: "hibernating" },
+    ]);
+    await expect(
+      resolveFallbackAgentId(ctx, "company-1", "Ops Triage"),
+    ).resolves.toEqual({ refusal: "transient" });
+    expect(logger.warn).toHaveBeenCalledWith(
+      expect.stringContaining("agent-1=unknown_status"),
+    );
+  });
+
   it("names the blocking reason rather than reporting an unmatched name", async () => {
     // "paused" and "name is wrong" need different fixes, so an operator must
     // not be sent hunting for a typo that isn't there.
diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
index cb1069aa191d..178a3d12bfa8 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
@@ -700,9 +700,15 @@ describe("handleWebhook — firing first time", () => {
     // processing before PEN-2581. The catch is inside the loop and
     // `AlertDeliveryIncompleteError` is thrown only after it completes, so a
     // sibling alert's issue was created on the first attempt even when the
-    // delivery reported 502. Verified, not assumed: neutralise the carve-out
-    // and the only assertion that fails is `resolves.toBeUndefined()` below —
-    // the `issues.create` assertions pass on the pre-change source.
+    // delivery reported 502. Verified by running it, not assumed, and
+    // reproducible as written: neutralise the carve-out (`const permanent =
+    // false` at the per-alert catch in `webhook-handler.ts`), then comment out
+    // BOTH the outcome assertion below and the `permanent_error` assertion at
+    // the end — on the pre-change source that alert reports through
+    // `alertmanager.alert.error`. The `issues.create` and `alert:` state
+    // assertions still pass. Both have to be neutralised first because Vitest
+    // aborts a test at its first failing assertion, so a run that trips the
+    // outcome assertion never reaches the ones that carry the point.
     //
     // What PEN-2581 actually changed is the delivery's reported *outcome*: the
     // ownerless fingerprint is no longer accumulated, so Alertmanager is no
@@ -817,6 +823,20 @@ describe("handleWebhook — firing first time", () => {
       1,
       expect.anything(),
     );
+    // The `refusal` label's other value, pinned here because the permanent test
+    // above pins only `"permanent"`. Splitting drop-from-retry within this one
+    // series is the label's entire purpose, so a regression that hardcoded
+    // `"permanent"` at the write site would otherwise pass the whole suite —
+    // the alert-level metric split asserted just above would still be correct.
+    expect(mocks.metrics.write).toHaveBeenCalledWith(
+      "alertmanager.owner.fallback_failed",
+      1,
+      {
+        alertname: "CiliumPolicyDropsHigh",
+        severity: "critical",
+        refusal: "transient",
+      },
+    );
     // The operator-facing warning names the blocking reason, so the next
     // occurrence is diagnosable from the log alone (PEN-2581).
     expect(mocks.logger.warn).toHaveBeenCalledWith(

From 8d223352ec340030af477942b098f158dd0f11d1 Mon Sep 17 00:00:00 2001
From: Cto <cto@paperclip.blockcast.net>
Date: Sat, 5 Sep 2026 06:12:48 +0000
Subject: [PATCH 5/5] fix(alertmanager): keep the retry window when the roster
 comes back empty
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Ally's review of e3ca4948 found that the zero-name-match branch returned
`permanent` unconditionally, so an `agents.list` that succeeds but comes
back empty permanently dropped the alert at 200. Before this PR that same
condition retried, so it was a regression this PR would have introduced —
and it sits on the path where every roster-derived permanent refusal is
actually decided, since `agents.list` filters terminated agents out and a
terminated fallback owner therefore lands here as an unmatched name.

The asymmetry is the bug: an `agents.list` that *throws* is handled
correctly (memo evicts, plain Error, Alertmanager retries), while the same
class of host degradation that returns `[]` without throwing was dropped.

An empty roster now takes the survivable branch — the same rule the PR
already applies to `unknown_status`, applied to the one input the
classified branch cannot see. Deliberately narrow: zero matches against a
non-empty roster stays permanent, because that really is a wrong name.

Also from the same review:
- `alertmanager.alert.permanent_error` carries `severity`. That drop is
  acknowledged at 200 and so is invisible in Alertmanager's own failure
  metrics, making this series the entire detection surface for it; it
  could not answer "did we drop a critical?" without a join.
- Corrected the `invalid_org_chain` comment: it is transient under the
  survivable-direction rule, not because it self-clears —
  `terminated_ancestor` and `cycle` need a human roster edit.
- Noted that the `terminated` map entry is unreachable from this resolver
  in production, so its test row pins the map in isolation rather than the
  live route.

Tests: empty-roster cases at both the resolver and worker layers, plus a
roster-driven permanent drop (previously every permanent-drop test used
unset config, which returns before `agents.list` is called). The
roster-absent test is a control — it passes before and after, pinning that
the carve-out did not widen into "every zero-match is transient";
mutation-checked with `agents.length >= 0`, which fails 12 tests.

Verification: 290/290 plugin tests, `tsc --noEmit` clean. Both new
behavioural tests confirmed to fail with the source change reverted.

Refs PEN-2581

Signed-off-by: Cto <cto@paperclip.blockcast.net>
---
 .../src/__tests__/owner-resolver.test.ts      | 30 +++++++
 .../src/__tests__/worker.test.ts              | 78 +++++++++++++++++--
 .../src/owner-resolver.ts                     | 38 ++++++++-
 .../src/webhook-handler.ts                    |  7 ++
 4 files changed, 145 insertions(+), 8 deletions(-)

diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts
index 9b9374c1f595..5481e7cdb040 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts
@@ -551,6 +551,27 @@ describe("resolveFallbackAgentId", () => {
     );
   });
 
+  // An empty roster is the shape a degraded-but-non-throwing `agents.list`
+  // takes, and it is the one input the unmatched-name branch cannot tell from
+  // a genuine typo. A throwing list already retries (the caller's memo evicts
+  // and a plain Error propagates); this pins that the silent-failure variant of
+  // the *same* host fault gets the same survivable treatment rather than a
+  // permanent 200 drop.
+  //
+  // Paired deliberately with the wrong-name case directly above: same call,
+  // same "no match" outcome, opposite refusal class. Asserting only the empty
+  // case would not catch a change that made *both* transient and reinstated
+  // the retry burst on genuinely wrong config.
+  it("refuses transiently when the roster itself comes back empty", async () => {
+    const { ctx, logger } = mkAgentCtx([]);
+    await expect(
+      resolveFallbackAgentId(ctx, "company-1", "Ops Triage"),
+    ).resolves.toEqual({ refusal: "transient" });
+    expect(logger.warn).toHaveBeenCalledWith(
+      expect.stringContaining("roster came back empty"),
+    );
+  });
+
   it("refuses permanently and warns when the name is ambiguous", async () => {
     const { ctx, logger } = mkAgentCtx([
       { id: "agent-1", name: "Ops Triage" },
@@ -583,6 +604,15 @@ describe("resolveFallbackAgentId", () => {
   // paused owner becomes invokable on its own and must keep Alertmanager's
   // retry window, while a terminated one needs a roster edit and must not.
   // Bundling them would assert only "refuses" and lose the distinction.
+  //
+  // ⚠️ The `terminated` row pins the map entry in isolation, not the
+  // production route. It injects a terminated agent into the mocked list, but
+  // the real `agents.list` filters those out (see the comment above), so in
+  // production a terminated owner is absent from the roster entirely and is
+  // refused by the unmatched-name branch instead — same `permanent` outcome,
+  // different code path. Keep the row as a guard on the map; do not read it as
+  // evidence that the classification ladder handles `terminated` live. The
+  // empty-roster case below covers the branch that does the real deciding.
   it.each([
     ["paused", "transient"],
     ["pending_approval", "transient"],
diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
index 178a3d12bfa8..efb359b6f191 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts
@@ -690,7 +690,7 @@ describe("handleWebhook — firing first time", () => {
     expect(mocks.metrics.write).toHaveBeenCalledWith(
       "alertmanager.alert.permanent_error",
       1,
-      { alertname: "CiliumPolicyDropsHigh" },
+      { alertname: "CiliumPolicyDropsHigh", severity: "critical" },
     );
   });
 
@@ -787,7 +787,75 @@ describe("handleWebhook — firing first time", () => {
     expect(mocks.metrics.write).toHaveBeenCalledWith(
       "alertmanager.alert.permanent_error",
       1,
-      { alertname: "CephOsdNearFull" },
+      { alertname: "CephOsdNearFull", severity: "critical" },
+    );
+  });
+
+  // The permanent drop above is driven by *unset config*, which returns before
+  // `agents.list` is ever called. This one is driven by roster contents: the
+  // name is configured and correct-looking, and the refusal is decided by what
+  // the list came back with. That is the branch every roster-derived permanent
+  // refusal actually takes in production — `agents.list` filters terminated
+  // agents out, so a terminated fallback owner never reaches the eligibility
+  // ladder and lands here as an unmatched name instead.
+  it("permanently drops when the configured name is absent from the roster", async () => {
+    const { ctx, mocks } = mkCtx();
+    const config = baseConfig({ ownerMap: {} });
+    // A non-empty roster that simply does not contain the configured name —
+    // indistinguishable from a typo, and correctly permanent.
+    mocks.agents.list.mockResolvedValue([
+      { id: "agent-other", name: "Someone Else", status: "idle" },
+    ]);
+
+    await expect(
+      handleWebhook(ctx, config, true, baseInput()),
+    ).resolves.toBeUndefined();
+
+    expect(mocks.issues.create).not.toHaveBeenCalled();
+    expect(mocks.state.set).not.toHaveBeenCalled();
+    expect(mocks.metrics.write).toHaveBeenCalledWith(
+      "alertmanager.owner.fallback_failed",
+      1,
+      {
+        alertname: "CiliumPolicyDropsHigh",
+        severity: "critical",
+        refusal: "permanent",
+      },
+    );
+  });
+
+  // The guard that separates a degraded host from a wrong name. Same "no
+  // match" outcome as the test directly above, opposite refusal class, and the
+  // only difference in the input is that the roster is empty rather than merely
+  // lacking the name. An `agents.list` that fails by *returning* `[]` instead
+  // of throwing would otherwise be dropped at 200 and never retried, while the
+  // throwing variant of the identical fault keeps its retry window.
+  it("keeps the retry window when the roster comes back empty", async () => {
+    const { ctx, mocks } = mkCtx();
+    const config = baseConfig({ ownerMap: {} });
+    mocks.agents.list.mockResolvedValue([]);
+
+    await expect(
+      handleWebhook(ctx, config, true, baseInput()),
+    ).rejects.toBeInstanceOf(AlertDeliveryIncompleteError);
+
+    // Fail-closed is intact either way — the class change is about the
+    // reporting channel, never about creating an ownerless issue.
+    expect(mocks.issues.create).not.toHaveBeenCalled();
+    expect(mocks.state.set).not.toHaveBeenCalled();
+    expect(mocks.metrics.write).toHaveBeenCalledWith(
+      "alertmanager.owner.fallback_failed",
+      1,
+      {
+        alertname: "CiliumPolicyDropsHigh",
+        severity: "critical",
+        refusal: "transient",
+      },
+    );
+    expect(mocks.metrics.write).not.toHaveBeenCalledWith(
+      "alertmanager.alert.permanent_error",
+      1,
+      expect.anything(),
     );
   });
 
@@ -816,7 +884,7 @@ describe("handleWebhook — firing first time", () => {
     expect(mocks.metrics.write).toHaveBeenCalledWith(
       "alertmanager.alert.error",
       1,
-      { alertname: "CiliumPolicyDropsHigh" },
+      { alertname: "CiliumPolicyDropsHigh", severity: "critical" },
     );
     expect(mocks.metrics.write).not.toHaveBeenCalledWith(
       "alertmanager.alert.permanent_error",
@@ -869,7 +937,7 @@ describe("handleWebhook — firing first time", () => {
     expect(mocks.metrics.write).toHaveBeenCalledWith(
       "alertmanager.alert.permanent_error",
       1,
-      { alertname: "CiliumPolicyDropsHigh" },
+      { alertname: "CiliumPolicyDropsHigh", severity: "critical" },
     );
     // And the swallowed metrics failure is still audible in the log.
     expect(mocks.logger.error).toHaveBeenCalledWith(
@@ -892,7 +960,7 @@ describe("handleWebhook — firing first time", () => {
     expect(mocks.metrics.write).toHaveBeenCalledWith(
       "alertmanager.alert.error",
       1,
-      { alertname: "CiliumPolicyDropsHigh" },
+      { alertname: "CiliumPolicyDropsHigh", severity: "critical" },
     );
     expect(mocks.metrics.write).not.toHaveBeenCalledWith(
       "alertmanager.alert.permanent_error",
diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts b/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts
index 06d07c943de4..714985d6067c 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts
@@ -219,9 +219,20 @@ export type FallbackOwnerResolution =
  * change config or the roster.
  *
  * `paused` and `pending_approval` are process state: an unpause or a board
- * approval flips them with nobody editing config, and `invalid_org_chain`
- * clears when the manager upstream is restored. Only `terminated` is the
- * roster fact that no amount of waiting fixes.
+ * approval flips them with nobody editing config. `invalid_org_chain` is
+ * mixed — `getAgentOrgChainHealth` returns it for `missing_manager`, which is
+ * genuinely self-clearing, but also for `cycle` and `terminated_ancestor`,
+ * which need a human roster edit exactly as `terminated` does. It is
+ * classified `transient` under the survivable-direction rule below, *not*
+ * because it always self-clears. `terminated` is the one reason here that is
+ * both unambiguous and unfixable by waiting.
+ *
+ * Note that `terminated` is unreachable from this resolver in production, and
+ * the entry is a guard on the map rather than a live classification: the host
+ * calls `agents.list` with no options (`plugin-host-services.ts`), which
+ * filters `ne(status, "terminated")`, so a terminated fallback owner is never
+ * in the list to be evaluated. It resolves as zero name matches instead, and
+ * is refused permanently by the unmatched-name branch below.
  *
  * `unknown_status` is deliberately transient: it means this resolver does not
  * recognise the status, so it cannot claim the condition is unfixable.
@@ -265,6 +276,27 @@ export async function resolveFallbackAgentId(
   // list is unordered, so paging could drift a match across page boundaries
   // and turn a stable config into an intermittent ownerless-issue bug.
   const agents = await ctx.agents.list({ companyId });
+  // A host fault that *throws* is already handled correctly downstream: the
+  // caller's memo evicts and a plain `Error` keeps Alertmanager's retry window.
+  // This guard covers the same class of degradation arriving by a quieter
+  // route — a lagging read replica, a company-scoping regression, a partial
+  // read — where the list resolves to `[]` instead. Without it that lands as
+  // zero name matches below and drops the alert permanently at 200, which is
+  // strictly worse than the throwing case for an identical underlying fault.
+  //
+  // A company with a configured Alertmanager plugin and zero non-terminated
+  // agents is not a legitimate steady state, so treating it as a host fault
+  // costs nothing real. Deliberately narrow: zero matches against a *non-empty*
+  // roster stays permanent, because nothing distinguishes it from the wrong
+  // name it usually is. Only the empty roster is separable, and it takes the
+  // survivable direction under the same asymmetry stated above for
+  // `unknown_status` — a retry burst, not a dropped alert.
+  if (agents.length === 0) {
+    ctx.logger.warn(
+      `Fallback agent "${fallbackAgentName}" could not be resolved: the company roster came back empty; refusing ownerless issue creation (transient)`,
+    );
+    return { refusal: "transient" };
+  }
   const nameMatches = agents.filter(
     (agent) => agent.name.trim().toLowerCase() === target,
   );
diff --git a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
index 2e8aeaa3cc1e..1c91bf2671d0 100644
--- a/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
+++ b/packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts
@@ -2306,6 +2306,12 @@ export async function handleWebhook(
         failedFingerprints.push(alert.fingerprint);
       }
       try {
+        // `severity` is carried on both branches for the same reason the
+        // `refusal` label exists on `alertmanager.owner.fallback_failed`:
+        // without it, "did we drop a critical?" needs a join against another
+        // series. It matters most on the permanent branch — that drop returns
+        // 200, so it is by design invisible in Alertmanager's own failure
+        // metrics and this series is the entire detection surface for it.
         await ctx.metrics.write(
           permanent
             ? "alertmanager.alert.permanent_error"
@@ -2313,6 +2319,7 @@ export async function handleWebhook(
           1,
           {
             alertname: alert.labels.alertname ?? "unknown",
+            severity: alert.labels.severity ?? "unknown",
           },
         );
       } catch (metricErr) {