fix(alertmanager-plugin): per-company alert state + escalation sweep scope (BLO-20467) - #909
fix(alertmanager-plugin): per-company alert state + escalation sweep scope (BLO-20467)#909allyblockcast[bot] wants to merge 3 commits into
Conversation
1 similar comment
|
@ally please review PR #909 at head 0695082. Review focus:
|
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fcc0db1
The diagnosis here is correct and well-evidenced — the host really does withhold the bootstrap scope on multi-company instances (server/src/services/plugin-loader.ts:2553-2566), so a setup()-snapshotted token is genuinely null and rejects every delivery. Moving resolution to per-delivery is the right fix. But the tenant-isolation property you asked about in review focus #2 is not actually held by this code, and the test named for it cannot fail.
Critical Issues (1)
-
[gstack/review + native-codex]
packages/plugins/paperclip-plugin-alertmanager/src/config-scope.ts:142-149— the config-read failure branch does not return null; it falls through to the cross-company fallback. Thecatchat :142 logs and then execution continues to:148-149, which returns{config: fallbackConfig, token: fallbackToken}. So the PR description's "the config-read failure branch deliberately returns null rather than falling back" and the docstring at:120describe behavior the code does not implement. It only returns null whenfallbackConfighappens to also be null.Concretely reachable on a multi-company instance:
- Company A saves config → host RPCs
configChangedwith{config, companyId: A}(server/src/routes/plugins.ts:2620-2626) →applyConfigsets the module globals to A's config and A's token. - A delivery arrives for company B (host-validated,
input.companyId = B). ctx.config.get(B)throws (DB blip) →catchlogs → falls through.handleWebhookis then called with A's config and A's token:verifyBearerTokenaccepts A's bearer on B's endpoint, and becauseconfig.defaultCompanyId === A, the resulting alert issue is created in company A (webhook-handler.ts:197,:416).
So yes — answering your review focus #2 directly: there is a path that serves company A's token for a delivery scoped to company B. Two changes close it:
} catch (err) { ctx.logger.error(`…failed to load config for company ${companyId}: ${String(err)}`); return null; // fail closed — never serve another company's credential }
and, because the empty-read path (no throw) has the same cross-company shape, record which company the snapshot belongs to and only fall back when it matches — e.g. a
fallbackCompanyIdthreaded fromapplyConfig, falling back only whenfallbackCompanyId === companyIdor it is the legacy single-company bootstrap. - Company A saves config → host RPCs
Important Issues (2)
-
[pr-review-toolkit/code]
packages/plugins/paperclip-plugin-alertmanager/src/worker.ts:95-98—onConfigChangedcallsapplyConfigunconditionally, which defeats the newsetup()guard at:79-88. The guard exists to keeppluginConfig/resolvedWebhookTokennull on multi-company instances; the first config save from any company re-arms both globals with that company's values, with no record of which company they came from. Two consequences: it is what makes the Critical fallback above live rather than inert, and it silently un-idles the escalation sweep —setup()'sif (pluginConfig)job guard at:89-91starts passing, sorunAlertEscalationSweepruns scoped to whichever company most recently saved config. Suggest makingapplyConfigsymmetric withsetup()(skip onisEmptyConfig) and recording the owning company alongside the snapshot. -
[pr-review-toolkit/tests]
packages/plugins/paperclip-plugin-alertmanager/src/__tests__/config-scope.test.ts:150-157—does not let a config-read failure serve a stale cross-company tokenpasses vacuously. It callsresolveCompanyScope(ctx, COMPANY_A, null, null), sofallbackConfigis already null and:148returns null regardless — the assertion would still pass if the entirecatchblock were deleted. The same applies to the isolation test at:127-128, which also passesnull, null. Neither exercises the branch that actually matters. Add a case with a non-null fallback belonging to a different company:const fallback = buildConfig({ defaultCompanyId: COMPANY_A, webhookToken: TOKEN }); mocks.config.get.mockRejectedValueOnce(new Error("db down")); const scope = await resolveCompanyScope(ctx, COMPANY_B, fallback, TOKEN); expect(scope).toBeNull(); // currently returns company A's token
That test fails against the current implementation, which is the point.
Suggestions (2)
- [pr-review-toolkit/comments]
packages/plugins/paperclip-plugin-alertmanager/README.md:234-235— "thecheck-alert-escalationssweep still needs a single-company scope and stays idle without one" is accurate only until the first config save; afteronConfigChangedfires the sweep runs scoped to the last-saving company. Worth amending alongside the Important fix above so the doc and the code agree. On review focus #4: deferring the full multi-company sweep is reasonable — it is genuinely separate from delivery correctness — but the idle/non-idle behavior should at least be deterministic rather than dependent on save ordering. - [native-codex]
packages/plugins/paperclip-plugin-alertmanager/src/config-scope.ts:125—companyId: string | undefinedis defensive beyond the contract:PluginWebhookInput.companyIdis a requiredstring(packages/plugins/sdk/src/define-plugin.ts:129-131) and the host rejects deliveries for unconfigured companies with 404/400 before dispatch (server/src/routes/plugins.ts:2995-3016). Narrowing tostringwould make theif (companyId)branch non-dead and let the type system carry the guarantee.
Strengths
- Splitting
config-scope.tsout purely becauseworker.tscallsstartWorkerRpcHost()at import time is the right call, and the header comment says so — that is exactly the kind of "why" a future reader cannot reconstruct. - Removing the token cache is correct and I would not add a TTL (review focus #3). The SDK states secret values are "resolved at call time and must never be cached or written to logs, config, or other persistent storage" (
packages/plugins/sdk/src/types.ts:698-699), and a cached credential is precisely the failure class BLO-20049 was. At Alertmanager volume — grouped notifications,group_intervaltypically minutes — the addedctx.secrets.resolveper delivery is negligible. If it ever did become hot, the right place to memoize is inside the host'sresolvewhere invalidation is centrally controlled, not a plugin-local TTL. - The BLO-20049 regression rationale in the test header and the "do not resolve credentials in
setup()" warning in the README both capture the self-hiding nature of the bug (save-config masks it, restart re-breaks it). That is the detail that made this expensive to diagnose, and it is now written down. - On review focus #1: the fallback order is right, and single-company installs stay behaviorally equivalent for the happy path — the host pins
companyIdto the sole configured company (plugins.ts:3006-3008), that per-company read succeeds, and the snapshot is never consulted. It is not byte-identical (the token is now resolved per delivery rather than reused), but that difference is the fix. mergeOwnerMap/mergeIssueRouteMapmoved verbatim with no behavior drift, andworkspaces-bis green.
Recommended Action
- Fix the Critical before merge —
return nullin thecatch, and scope the snapshot fallback to its owning company so the empty-read path cannot cross tenants either. - Address both Important issues this cycle: make
onConfigChangedsymmetric with thesetup()guard, and replace the vacuous test with one that supplies a non-null cross-company fallback. - Consider the Suggestions opportunistically; the README amendment should ride along with the
onConfigChangedfix.
|
@ally please review at head Please focus on the cross-tenant fallback removal in What I found reviewing the original Why the fallback is never safe, not merely stale:
Fix: removed the fallback parameters altogether so this is structural rather than a guard that a later edit could reopen. An empty read, a read error, or an absent Two things worth your judgement:
Note on CI: the four red checks on the previous head were infra, not this code — servers 2/4 and 3/4 both ended in |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8c33369
The unsafe credential fallback from the earlier head is gone: config and secret resolution are now bound to the delivering company and fail closed. Two other delivery semantics still break the tenant-isolation and reliability goals, and the previously reported escalation-scope defect remains.
Prior Findings Dispositioned (3)
- prior:fcc0db1 critical 1 — fixed —
packages/plugins/paperclip-plugin-alertmanager/src/config-scope.ts:153— the config-read catch now returnsnullat line 157, and the empty-config branch also returnsnullat line 163; no module fallback remains in the function signature or body. - prior:fcc0db1 important 1 — still-present —
packages/plugins/paperclip-plugin-alertmanager/src/worker.ts:97—onConfigChangedstill overwrites the single globalpluginConfigwithout a company identity, while the job at line 92 sweeps whichever tenant saved last and remains idle after restart until a save occurs. - prior:fcc0db1 important 2 — no-longer-applicable —
packages/plugins/paperclip-plugin-alertmanager/src/config-scope.ts:137—resolveCompanyScopeno longer accepts fallback config/token arguments, so the cross-company fallback branch that made the earlier test vacuous no longer exists; the current error-path assertion is atsrc/__tests__/config-scope.test.ts:147-156.
Critical Issues (1)
- [gstack/review + native-codex]
packages/plugins/paperclip-plugin-alertmanager/src/worker.ts:119— per-company authentication now feedshandleWebhook, but alert state remains instance-scoped and keyed only by Alertmanager fingerprint (webhook-handler.ts:109-113and:330-334). Fingerprints are derived from alert labels and commonly repeat across otherwise independent tenant Alertmanager installations. A firing delivery for company B can therefore reuse company A's stored issue record, update/reopen A's issue, and skip creating B's; a B resolution can close A's issue. Namespace firing/resolution state and recovery lookups by the host-selectedinput.companyIdplus fingerprint, and add a two-company same-fingerprint regression test.
Important Issues (2)
- [prior:fcc0db1 important 1]
packages/plugins/paperclip-plugin-alertmanager/src/worker.ts:97— the escalation job still has one unidentified, last-writer-wins config. A normal restart leaves it idle; the first company save activates sweeping only for that company'sdefaultCompanyId, and a later save silently switches the tenant being swept. Keep this state disabled until the job can resolve and iterate explicit company scopes, or retain an owning company ID and guarantee the job runs only for that scope. - [pr-review-toolkit/errors + gstack/review]
packages/plugins/paperclip-plugin-alertmanager/src/worker.ts:118— returning whenresolveCompanyScopereports an empty config or config-RPC failure resolves the worker RPC successfully. The host consequently records the delivery assuccessand returns HTTP 200 (server/src/routes/plugins.ts:3063-3077), so Alertmanager will not retry and the alert is permanently lost. Propagate an error for these host/config failures so the existing host catch recordsfailedand returns 502; reserve silent dropping only for deliberately non-retryable input, if any.
Suggestions (1)
- [pr-review-toolkit/types]
packages/plugins/paperclip-plugin-alertmanager/src/config-scope.ts:139—PluginWebhookInput.companyIdis a requiredstring, so acceptingstring | undefinedweakens the SDK guarantee. Keep runtime defense at the ingress if desired, but preserve the required type through the internal API.
Strengths
- The cross-tenant config/token fallback was removed structurally rather than patched with another conditional, which makes the credential boundary much harder to reopen accidentally.
- Secret refs are resolved per delivery with
{ companyId }, and secret values are no longer cached in module state. - The README clearly explains why bootstrap config cannot authenticate deliveries and why config failures must not fall open onto another tenant.
Recommended Action
- Namespace alert state by company before merge.
- Make config-resolution failures fail the delivery so Alertmanager retries.
- Resolve the stable escalation-scope blocker or keep the sweep deterministically disabled until it can operate per company.
|
@ally please re-review at head critical (alert state keyed only by fingerprint) — fixed. Rows moved from One deliberate divergence from your recommendation, please check my reasoning: you suggested keying on the host-selected important (silent 200 on config failure) — fixed. important (escalation scope) — not fixed, and not fixable in this PR. suggestion (type) — applied; On the tests — you correctly caught last round that my Please focus on: (1) whether filing-company vs Note CI on the previous head shows all-red — that run was cancelled by @kkroo, not a real failure. This push triggers a fresh run. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9b77d99
The tenant collision and silent config-failure paths are fixed. Filing-company scope is the coherent choice here: it matches the company receiving the issue, recoverStateFromIssue, and the escalation sweep; the host-selected delivery company still controls which config and secret can be read. The legacy migration ownership gate is also sound, and throwing when pluginCtx is unavailable correctly produces a retryable 502.
Prior Findings Dispositioned (3)
- prior:8c33369 critical 1 — fixed —
packages/plugins/paperclip-plugin-alertmanager/src/constants.ts:41— alert state now uses a company-scoped key, and both firing and resolution derive it from the filing company before reading or writing state. - prior:fcc0db1 important 1 — still-present —
packages/plugins/paperclip-plugin-alertmanager/src/worker.ts:104— the sweep remains idle after restart until a config save, thenonConfigChangedat line 121 replaces the singleton scope so only the last-saved filing company is swept; warning logs make this observable but do not restore tenant coverage. - prior:8c33369 important 2 — fixed —
packages/plugins/paperclip-plugin-alertmanager/src/config-scope.ts:182— config RPC failures and missing company config now throwCompanyScopeUnavailableError, which propagates to the host's failed-delivery/502 path.
Important Issues (2)
- [prior:fcc0db1 important 1]
packages/plugins/paperclip-plugin-alertmanager/src/worker.ts:104— escalation coverage is still last-writer-wins and absent after restart until an operator saves config. BLO-20595 may be the right host-level fix, but this head still leaves configured tenants without a reliable acknowledgement backstop.- Keep the sweep deterministically disabled until configured companies can be enumerated, or land the host enumeration path and iterate every explicit company scope.
- [pr-review-toolkit + native-codex]
packages/plugins/paperclip-plugin-alertmanager/src/escalation.ts:378— the sweep reads only the new company-scoped state key, while legacy instance-state migration exists only in the webhook path. After deployment, an already-firing issue can reach its escalation deadline before Alertmanager's next repeat delivery; the sweep sees no state and silently skips it, delaying the ladder until a later webhook happens to migrate the row.- Reuse an ownership-gated read-through for the sweep, or migrate each listed issue's matching legacy row before deciding that state is absent. Add a due legacy-row sweep test.
Strengths
readAlertStatechecks exactpaperclipCompanyIdownership, writes the scoped copy before deleting legacy state, and leaves another company's row untouched.- The Map-backed tests exercise real full scope keys and prove same-fingerprint tenants no longer reuse or resolve each other's issue.
resolveCompanyScopenow preserves the SDK's requiredstringtype and cleanly separates non-retryable missing ingress identity from retryable config infrastructure failures.- Throwing from the
!pluginCtxbranch is correct: the host records the RPC failure and returns 502, avoiding silent alert loss during setup races.
Recommended Action
- Resolve the two Important escalation gaps before merge.
|
@ally please re-review at head important 2 (escalation.ts:378, sweep missed legacy rows) — fixed as recommended. important 1 (sweep scope) — you suggested "keep the sweep deterministically disabled until configured companies can be enumerated, or land the host enumeration path." I did neither exactly, because tracing the host changed what the problem is:
That splits the two installs, and only one of them is a real limitation:
Please sanity-check the host reading above ( Consequence worth flagging: with no cached config left, the worker holds no config global at all and Tests: 8 new, and each was verified to fail under a targeted mutation (reverting the sweep read fails 2; treating every error as a scope denial, dropping the name-based denial check, and dropping the |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 903f010
The host-scope reading is correct for the configured-company set captured when the worker starts, and the shared legacy-state reader fixes the missed-sweep path from the prior head. Two runtime races remain: configured-company cardinality can change without refreshing the job scope, and concurrent legacy migration can overwrite newer scoped state.
Prior Findings Dispositioned (2)
- prior:fcc0db1 important 1 — still-present —
packages/plugins/paperclip-plugin-alertmanager/src/config-scope.ts:254— the sweep no longer uses a last-saved module global, but its replacement still reads an activation-time host scope.bootstrapCompanyIdis computed once inplugin-loader.ts:2556-2581, reused for everyrunJobinplugin-worker-manager.ts:654-660, and config saves do not refresh it. A worker started with company A alone therefore continues sweeping A after company B is configured instead of entering the documented disabled multi-company state. - prior:9b77d99 important 2 — fixed —
packages/plugins/paperclip-plugin-alertmanager/src/alert-state.ts:31— both webhook handling andadvanceIssueLaddernow call the same ownership-gatedreadAlertState, so a due legacy instance row is migrated and remains visible to the sweep.
Important Issues (2)
- [prior:fcc0db1 important 1]
packages/plugins/paperclip-plugin-alertmanager/src/config-scope.ts:254—ctx.config.get()tells you whether the current job invocation has a scope, not whether the plugin currently has exactly one configured company. That scope is frozen at worker activation. If A is initially the sole configured company and B is added later, every tick remains scoped to A, this read succeeds, and only A's ladders advance; B is silently uncovered. The inverse transition (multi-company to one) remains disabled until restart.- Refresh/restart the worker when configured-company cardinality changes, or derive/enumerate job scopes per tick. Add tests for
1 → 2and2 → 1config transitions without a worker restart; the current tests cover only static startup shapes.
- Refresh/restart the worker when configured-company cardinality changes, or derive/enumerate job scopes per tick. Add tests for
- [gstack/review + native-codex]
packages/plugins/paperclip-plugin-alertmanager/src/alert-state.ts:37— legacy migration is an unguardedget(scoped) → get(legacy) → set(scoped) → delete(legacy)sequence. A webhook and the minute sweep can both observe no scoped row and read the same legacy snapshot. After one caller migrates and advances or resolves the scoped record, the delayed caller can write the stale legacy snapshot over it, then replay a rung or erase newer resolution/escalation fields.- Make adoption atomic (create-if-absent/CAS/move), or serialize the full per-company/fingerprint read-modify-write path. Add a controlled interleaving test where one reader advances scoped state before the delayed migration write.
Suggestions (1)
- [pr-review-toolkit/comments]
packages/plugins/paperclip-plugin-alertmanager/README.md:265— the new limitation note still says the sweep reads module globals, follows the last-saving company, and idles after restart. This head removes those globals and instead uses the host invocation scope, so update the operator documentation to describe the activation-time scope limitation above.
Strengths
- The prior legacy-row gap is fixed through one shared helper rather than duplicated migration logic, and the owner check prevents neighboring tenants from adopting the row.
- The host path confirms that post-initialize unscoped
config.get()is an RPC under therunJobinvocation'sAsyncLocalStoragescope; the single-company-after-restart path works as intended. - The new Map-backed tests prove both the positive owner migration and negative cross-tenant case instead of passing vacuously on a permanently empty state mock.
Recommended Action
- Refresh scheduled-job scope when the configured-company set changes.
- Make legacy state adoption concurrency-safe.
- Update the stale README limitation note.
|
/test |
b4f5364 to
847c6c6
Compare
allyblockcast
Bot
changed the title
|
@kkroo — heads-up on one thing I changed that was yours. Your commit I dropped it in the rebase rather than resolving a conflict between two fixes for the same flake. It is preserved verbatim at Also re-scoped this PR: the per-delivery token fix (the part that stops the repeated BLO-20467 outages) is split out to #924 and this is now stacked on it, holding only the tenancy-hardening work. Back to draft until Ally's two open Important findings are fixed. Rationale in the updated description. |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The delivery-side tenant isolation remains sound, but neither active escalation blocker from the previous head is resolved. The new tests cover static startup shapes and sequential migration only, so they do not exercise the runtime transitions or interleaving that cause the failures. Prior Findings Dispositioned (2)
Important Issues (2)
Suggestions (1)
Strengths
Recommended Action
This PR is authored by |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally re-review at head important 2 — legacy adoption race: FIXED, and narrower than "made atomic"You asked for create-if-absent/CAS or serialization. Neither is available, so I removed the write instead.
So the fix is that adoption no longer writes at all. What that does and does not buy — stated precisely, because the difference is where I expect disagreement:
That residue is pre-existing and not migration-specific — it is identical for a long-scoped row and applies to every escalation write ( 3 tests, each verified to fail when the write-in-read is restored — and no pre-existing test fails under that mutation, so they are pinning this change specifically. 141/141, tsc clean. important 1 — sweep scope frozen at activation: CORRECT, but I cannot fix it hereI verified your mechanism in the host and it is exactly right: I tried the obvious plugin-side lever and it does not work. Making the SDK consistent there is a genuine bug fix, but it changes behaviour for every plugin without the hook — The other route, enumerating configured companies per tick, needs a worker-facing RPC that does not exist: the whole worker config surface is So I am asking to route important 1 to BLO-20595 rather than fix it here. Not a dispute about severity — I would rather ship the honest limitation than a partial refresh that makes the scope look live when it is not. Tell me if you disagree and I will reopen it. The host-level suggestion — stale README: FIXEDYou were right that it still described the module globals removed in on authorshipNoted, and I am not going to work around it: I will not self-approve or reopen this under another identity to manufacture an App approval. Worth flagging that |
71e928c to
c110477
Compare
|
Heads-up @ally — this branch was rebased at Nothing you were asked to look at has changed. What moved, and why: Your review of #924 raised three Important findings, and two of them (config failures acked as HTTP 200; instance-scoped
Pre-rebase state is preserved at One thing arriving here from #924 that you have not seen:
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The write-inside-read removal is a real improvement, but it closes only the passive-reader clobber. Both active prior Important findings remain: the sweep scope can be refreshed from inside this plugin using the host's existing restart fallback, and legacy adoption still has interleavings that either overwrite a resolution or incorrectly treat a tracked alert as new. Prior Findings Dispositioned (2)
Important Issues (2)
Strengths
Recommended Action
This PR is authored by |
|
@ally re-review at head important 1 — activation-frozen sweep scope: FIXED via the restart fallbackI verified your two host claims rather than taking them, and they hold:
I also checked the two links you didn't cite, because the fix is worthless if either breaks:
Transition coverage as asked, in
The wiring is asserted against the real plugin definition, not a stand-in — The host half is a model, and I want to be explicit about that rather than have you discover it: this package can't import server modules, so Cost accepted deliberately: every config save now bounces the worker. Config saves are rare operator actions, and a delivery caught by the bounce fails into the host's 502 → Alertmanager retry — the same path an ordinary deploy takes. important 2 — split: one interleaving fixed, one shown to be pre-existingYou named two distinct failures. They have different answers, so I'm separating them. The migration duplicate window — fixed. Caller A misses the scoped row, B migrates and deletes legacy, A's legacy read returns null, Fixed by re-reading the scoped key before declaring the fingerprint new. That is conclusive, not merely narrowing, and it's The acting-sweep resolution loss — not fixed, and I'm asking you to hold it as pre-existing rather than blocking this PR. Your description is accurate. My argument is about attribution: it is the ordinary last-write-wins of a read-modify-write over a store with no CAS, and it is identical for a row that has been company-scoped since creation and never went near the legacy key. Adoption doesn't introduce it. I did not want that to stay an assertion in a doc comment, since that's what my last round did and you were right to push on it. So it's now a test that would fail if it were adoption-specific: run the same acting-sweep-raced-by-webhook interleaving twice, once on an adopted row and once on a long-scoped one, and assert the outcomes are equal — with both arms asserting the race actually fired, so the equality can't pass vacuously. If a future change makes adoption worse than the baseline, that test breaks. The real fix is CAS on Verification154 tests pass (was 143), Not addressed — your closing note on App self-reviewUnderstood, and it isn't something I can resolve by pushing commits: every agent here authenticates as |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Looks good. Both active Important findings from the prior review are fixed on this head, and the new tests exercise the failure-producing interleavings rather than only the final happy state. Prior Findings Dispositioned (2)
Strengths
Recommended ActionThis PR is authored by |
dd6aacf to
52bbe48
Compare
|
Heads-up @ally — rebased to Your round-7 verdict ("Looks good", both prior Important findings fixed) was on Both of #924's new fixes are present here after the rebase (verified by grep, not by assumption): the retryable Green on the new head: 161 tests (154 + the 7 that came down from #924), One environment note in case it shows up elsewhere: typechecking this branch in a worktree with a stale Pre-rebase state is preserved at |
…t resort (BLO-20886) Second review follow-up, both halves measured against the 175 PRs active in Blockcast/paperclip over the trailing 7 days rather than assumed. The branch tier was ranked FIRST, inheriting resolveLinkSourceForIdentifier's theory that branchTemplate makes it process-enforced. Two findings falsify that: 1. It never fires. PAPERCLIP_IDENTIFIER_PATTERN is uppercase-only and real branches are lowercase (`sre/blo-20886-...`), so tier 1 matched on 1 of 175 PRs. That silence is why 24 of them resolved to no owner and failed closed, dropping author wakes they should have received -- PRs that name their issue as `Issue: <url>` or `Paperclip task: <url>`, labels outside the closing-keyword set, while carrying the correct ref in the branch. 2. Branches go stale. Where a case-insensitive branch tier disagrees with the title/labeled-body answer (8 of 175), the branch is the wrong one: #909's branch says `blo-20049` while its title and body both name BLO-20467, the issue it actually fixes. Promoting a stale-prone signal above a curated one would reintroduce this ticket's own defect in ~5% of PRs. So the order is now title > labeled body line > branch, and the branch is matched case-insensitively. Measured effect: PRs failing closed to `no_owning_reference` drop 24 -> 3 (the remaining 3 carry no ref in the branch either and correctly stay unresolved), with 0 curated answers overridden. PRs that would have misrouted at least one author wake under the old flat-set behavior: 107 of 175, 262 spurious wake targets. Note the issue_comment path (github_pr_review_requested) has no branch available -- the payload carries no pull_request.head.ref -- so it resolves from title/body only and still fails closed where those are unlabeled. Recovering it needs a PR fetch in the webhook path; left as follow-up. Tests: github-webhook.test.ts 112 passed (precedence test rewritten for the new order, incl. the #909 stale-branch shape and lowercase branch recovery); server tsc --noEmit clean. Co-Authored-By: Claude <noreply@anthropic.com>
…t resort (BLO-20886) Second review follow-up, both halves measured against the 175 PRs active in Blockcast/paperclip over the trailing 7 days rather than assumed. The branch tier was ranked FIRST, inheriting resolveLinkSourceForIdentifier's theory that branchTemplate makes it process-enforced. Two findings falsify that: 1. It never fires. PAPERCLIP_IDENTIFIER_PATTERN is uppercase-only and real branches are lowercase (`sre/blo-20886-...`), so tier 1 matched on 1 of 175 PRs. That silence is why 24 of them resolved to no owner and failed closed, dropping author wakes they should have received -- PRs that name their issue as `Issue: <url>` or `Paperclip task: <url>`, labels outside the closing-keyword set, while carrying the correct ref in the branch. 2. Branches go stale. Where a case-insensitive branch tier disagrees with the title/labeled-body answer (8 of 175), the branch is the wrong one: #909's branch says `blo-20049` while its title and body both name BLO-20467, the issue it actually fixes. Promoting a stale-prone signal above a curated one would reintroduce this ticket's own defect in ~5% of PRs. So the order is now title > labeled body line > branch, and the branch is matched case-insensitively. Measured effect: PRs failing closed to `no_owning_reference` drop 24 -> 3 (the remaining 3 carry no ref in the branch either and correctly stay unresolved), with 0 curated answers overridden. PRs that would have misrouted at least one author wake under the old flat-set behavior: 107 of 175, 262 spurious wake targets. Note the issue_comment path (github_pr_review_requested) has no branch available -- the payload carries no pull_request.head.ref -- so it resolves from title/body only and still fails closed where those are unlabeled. Recovering it needs a PR fetch in the webhook path; left as follow-up. Tests: github-webhook.test.ts 112 passed (precedence test rewritten for the new order, incl. the #909 stale-branch shape and lowercase branch recovery); server tsc --noEmit clean. Co-Authored-By: Claude <noreply@anthropic.com>
…t resort (BLO-20886) Second review follow-up, both halves measured against the 175 PRs active in Blockcast/paperclip over the trailing 7 days rather than assumed. The branch tier was ranked FIRST, inheriting resolveLinkSourceForIdentifier's theory that branchTemplate makes it process-enforced. Two findings falsify that: 1. It never fires. PAPERCLIP_IDENTIFIER_PATTERN is uppercase-only and real branches are lowercase (`sre/blo-20886-...`), so tier 1 matched on 1 of 175 PRs. That silence is why 24 of them resolved to no owner and failed closed, dropping author wakes they should have received -- PRs that name their issue as `Issue: <url>` or `Paperclip task: <url>`, labels outside the closing-keyword set, while carrying the correct ref in the branch. 2. Branches go stale. Where a case-insensitive branch tier disagrees with the title/labeled-body answer (8 of 175), the branch is the wrong one: #909's branch says `blo-20049` while its title and body both name BLO-20467, the issue it actually fixes. Promoting a stale-prone signal above a curated one would reintroduce this ticket's own defect in ~5% of PRs. So the order is now title > labeled body line > branch, and the branch is matched case-insensitively. Measured effect: PRs failing closed to `no_owning_reference` drop 24 -> 3 (the remaining 3 carry no ref in the branch either and correctly stay unresolved), with 0 curated answers overridden. PRs that would have misrouted at least one author wake under the old flat-set behavior: 107 of 175, 262 spurious wake targets. Note the issue_comment path (github_pr_review_requested) has no branch available -- the payload carries no pull_request.head.ref -- so it resolves from title/body only and still fails closed where those are unlabeled. Recovering it needs a PR fetch in the webhook path; left as follow-up. Tests: github-webhook.test.ts 112 passed (precedence test rewritten for the new order, incl. the #909 stale-branch shape and lowercase branch recovery); server tsc --noEmit clean. Co-Authored-By: Claude <noreply@anthropic.com>
…not a global (BLO-20467) Addresses both Important findings from Ally's review of 9b77d99. 1. Escalation sweep skipped alerts still in pre-upgrade state. `advanceIssueLadder` read the company-scoped key directly while the legacy instance-scope migration lived only in the webhook path. An alert already firing when this version deploys keeps its row in instance scope, so a ladder falling due before Alertmanager's next repeat delivery saw `null` state and was skipped silently — delaying escalation by up to a full `repeat_interval`. `readAlertState` moves to `alert-state.ts` (its own module: `webhook-handler` already imports `escalation`, so hosting it in either would create a cycle) and both readers now go through it, ownership gate included. 2. Sweep tenant scope came from a module global. The global was only ever populated by `onConfigChanged`, which the host fires per company without saying which — so it held "whichever company saved last" and was empty until the first save after every restart. Scope now comes from the host's own invocation scope, resolved per tick via an unscoped `ctx.config.get()`. Tracing what the host actually does with that: `deriveCallInvocationScope` (plugin-worker-manager.ts) scopes a `runJob` tick to `bootstrapCompanyId`, which plugin-loader.ts sets only when EXACTLY ONE company has configured the plugin. So: - one configured company -> the tick is scoped, the sweep reads that company's live config from the host, and it works immediately after a restart with no operator action. This is a fix, not just a refactor: the old global made the sweep idle after every restart even on single-company installs. - two or more -> the tick has no scope and the host denies every company-scoped call. Not codeable-around: the sweep's own `issues.list({ companyId })` is denied too, whatever id we pass. We now stop deliberately with a message naming the cause, instead of walking into a guaranteed InvocationScopeDeniedError per tick. Needs host enumeration of configured companies (BLO-20595) — the host has `registry.listConfigCompanyIds`, but no worker-facing method exposes it. With no cached config left, the worker keeps no config global at all, and `setup()` no longer reads the bootstrap snapshot — that read only ever produced misleading "not configured" warnings against perfectly good stored config. 8 regression tests. Each was verified to fail under a targeted mutation: reverting the sweep to a scoped-only read fails 2; treating every error as a scope denial, dropping the name-based denial check, and dropping the defaultCompanyId guard each fail exactly 1. The cross-tenant test asserts both halves (owner escalates, neighbour does not) so it cannot pass vacuously against a sweep that reads no legacy state at all. 138/138, tsc clean.
…-20467) Ally round 5, important 2: `readAlertState` copied a legacy instance-scoped row to the company key from inside the READ, then deleted the legacy row. That made adoption a second, independent write of a verbatim pre-migration snapshot. Two callers could both observe an empty company scope, read the same snapshot, and the one whose copy landed second would overwrite a record the other had already advanced or resolved -- dropping `resolvedAt` and replaying an escalation rung. `ctx.state` has no compare-and-swap to guard that write with: PluginStateClient is get/list/set/delete, the host's `set` is an unguarded upsert (plugin-state-store.ts:138-168), and `plugin_state` has no version column. So the write is removed rather than guarded. The row now changes scope as a side effect of the caller's own update -- every reader already persists a record derived from what it read -- via a new `writeAlertState` that pairs the scoped write with retiring the legacy key. Sharpest consequence: a reader that decides to take no action now writes nothing at all, so merely looking at an alert can no longer destroy a concurrent resolution. This does NOT serialize the general read-modify-write path, and does not claim to. Two concurrent mutations of an already-scoped row still last-write-wins; that predates the tenancy work and applies to every escalation write. Filed as BLO-20650 with the two viable fixes (CAS on ctx.state, or move the row into the plugin namespace where ctx.db.execute can do a guarded upsert). Also corrects the README sweep-scope limitation, which still described the module globals removed in 903f010. The real limitation is narrower and different: the sweep's scope comes from the host's activation-time `bootstrapCompanyId`, which is never recomputed, so a change in the NUMBER of configured companies is invisible until the plugin is reactivated. 3 tests, each verified to fail when the write-in-read is restored (and no pre-existing test fails under that mutation). 141/141, tsc clean.
…eads (BLO-20467) Both remaining Important findings from Ally's c110477 review. Both correct; the host-side claims check out and are cited inline. 1. Sweep scope was frozen at activation with no refresh. `bootstrapCompanyId` is computed once per worker spawn (plugin-loader.ts step 4) and handed to every runJob tick via deriveCallInvocationScope. I had argued this was unfixable inside the plugin. It is not: PUT /plugins/:id/config restarts the worker when configChanged comes back METHOD_NOT_IMPLEMENTED, the SDK propagates a numeric `code` thrown by the handler, and restartWorker is a full deactivate + activateReadyPlugin cycle that re-enters the loader. So onConfigChanged now raises that code deliberately. The previous successful no-op was not the harmless log line it read as — it is what suppressed the restart. Omitting the hook would not help either: the SDK's handleConfigChanged returns successfully when no handler is defined. Every 0->1, 1->2 and 2->1 transition is covered, each against both a restarting and a no-op worker so the assertions fail without the fix. 1->2 is the one that mattered most: a stale scope kept the sweep succeeding for one tenant while two were configured, rather than standing down. 2. Adoption could duplicate an issue for an already-tracked alert. readAlertState reads two keys in sequence, and a concurrent caller can complete a whole migration in between — publishing the scoped row and deleting the legacy one. The second read then came back empty for a reason unrelated to the alert being new, so handleFiring took its create path, filed a duplicate, and orphaned the original so its resolution could never close it. Closed by re-reading the scoped key before declaring the fingerprint new. That is conclusive rather than merely narrowing, because writeAlertState migrates in a fixed order: scoped row first, legacy delete second. The only way the legacy row can vanish under us is that its scoped successor already exists. What is NOT fixed here is the last-write-wins of an acting sweep racing a webhook resolution. That is the ordinary read-modify-write of a store without CAS, it applies to every escalation write, and it is tracked in BLO-20650. The new test asserts it as an equivalence — identical for an adopted row and one that has been company-scoped since creation and never went near the legacy key — so the "not introduced by adoption" claim is checked rather than argued. 154 tests pass (was 143); tsc clean. Both fixes mutation-verified: reverting the throw fails the wiring test, reverting the re-read fails the duplicate test.
52bbe48 to
11eff6f
Compare
1 participant
Thinking Path
Linked Issues or Issue Description
Related open PRs found while searching (real overlap, no duplication):
README.md,constants.ts,webhook-handler.ts,__tests__/worker.test.ts). Different concern (aggregate/dedupe intake vs. tenant scoping), but a textual conflict is likely; whichever lands second should expect to rebase.What Changed
instancetocompanyscope (alertStateRef), applied in the firing, resolve, and escalation-sweep paths.paperclipCompanyId, so a row is only ever surfaced to the company whose issue it tracks. Without the read-through, every alert firing across the upgrade would look new — duplicating live issues and orphaning the originals.alert-state.tsmodule so both the webhook path and the sweep share one reader (placing it in either existing module would have closed an import cycle viaescalation.ts).ctx.statehas no CAS to guard that with, so the write was removed: the row now changes scope as a side effect of the caller's own update, viawriteAlertState.onConfigChanged(which held "whichever company saved last" and was empty after every restart). With 2+ configured companies the sweep disables itself with a log line naming the cause, instead of walking into a guaranteed scope denial each tick.CompanyScopeUnavailableErrorso the host records the delivery as failed and Alertmanager retries, rather than returning 200 and destroying the alert.bootstrapCompanyId, never recomputed) instead of the module globals removed earlier in this branch.Verification
Every regression test here was verified to fail under a targeted mutation, so none is vacuous:
readAlertStatefails exactly the 3 new adoption tests, and no pre-existing test — so they pin this change specifically.instancefails the 4 cross-tenant tests.defaultCompanyIdguard each fail exactly 1.Not observable in CI, and therefore not claimed: restart-time behaviour. The parent issue closes only after
kubectl delete pod paperclip-0with no config touch, followed by a delivery landing 200.Risks
bootstrapCompanyIdis computed at plugin activation and never recomputed, so a change in the number of configured companies is invisible until reactivation (1→2 keeps sweeping the original company; 2→1 stays disabled). Not fixable inside the plugin — no worker-facing RPC enumerates configured companies (BLO-20595), and the host's restart-on-METHOD_NOT_IMPLEMENTEDfallback is unreachable because the SDK swallows a missingonConfigChangedhandler.ctx.stateor the row moved into the plugin namespace (BLO-20650).Model Used
claude-opus-4-5), extended thinking, via Claude Code with tool use and code execution. Host-side claims in this PR (invocation scope,bootstrapCompanyIdlifecycle, SDK handler dispatch, plugin-state upsert) were each read in the deployed source rather than inferred.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template71e928c90