Skip to content

orb(notifications): stop deriving the AMS PR outcome by substring-scanning a client dedupKey #9703

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

ams_pr_outcome is the only notification kind whose rendered content depends on a field that is not on the event. buildAmsPrOutcomeNotification (src/notifications/service.ts:89-107) recovers the decision by substring-scanning the dedup key:

// buildAmsPrOutcomeEvent embeds `:merged:` or `:closed:` in the dedupKey after the PR number.
const merged = event.dedupKey.includes(":merged:");

DetectedNotificationEvent has no decision field, so that string is the entire carrier. Three things make it unsafe:

  1. The validator does not check the shape. normalizeAmsNotificationEventInput (src/notifications/ams-events.ts:134-156) type-checks every other field but for dedupKey only tests non-empty string (:142). The HTTP schema is equally permissive: dedupKey: z.string().min(1).max(500) (src/api/routes.ts:487). Any ams_pr_outcome whose key lacks :merged: renders as "AMS recorded close: ... closed without merge" — including for a merge.
  2. The format has two hand-duplicated producers. buildAmsPrOutcomeEvent (ams-events.ts:107-128) and buildAmsPrOutcomePayload (packages/loopover-miner/lib/ams-notifications.ts:118-139) each independently write ams_pr_outcome:${repoFullName}#${pullNumber}:${decision}:${closedAt}. Nothing pins them together.
  3. The server-side builders are dead. buildAmsPrOutcomeEvent, buildAmsAttemptStartedEvent, buildAmsAttemptFailedEvent and buildAmsGovernorPausedEvent have zero non-test callers (grep across src/, apps/, packages/); the miner's copies are what actually run. So the format the consumer documents itself against is produced by a file this repo never exercises — if the miner reorders decision and closedAt, every merge notification silently flips to "closed without merge" and nothing fails.

The repo has a convention for exactly this drift class — see the source-pinning tests at test/unit/advisory-spend-gate.test.ts:49-71 and the REVIEWER_VOTE_EVENT_TYPE note at src/services/reviewer-routing.ts:102-104.

Requirements

  • Add to src/notifications/ams-events.ts:
    • export const AMS_PR_OUTCOME_DEDUP_KEY_PATTERN — an anchored RegExp matching exactly ams_pr_outcome:<repoFullName>#<pullNumber>:(merged|closed):<closedAt>, with the decision as a capture group.
    • export function parseAmsPrOutcomeDecision(dedupKey: string): "merged" | "closed" | null — the single reader of that pattern.
  • buildAmsPrOutcomeEvent keeps its output, but the file must contain no second literal spelling of the format: the builder and the pattern must derive from one shared prefix/segment constant.
  • normalizeAmsNotificationEventInput returns null for an ams_pr_outcome event whose dedupKey does not satisfy parseAmsPrOutcomeDecision. Every other AMS_NOTIFICATION_EVENT_TYPES kind keeps its current validation unchanged — this rule is ams_pr_outcome-only.
  • buildAmsPrOutcomeNotification uses parseAmsPrOutcomeDecision instead of .includes(":merged:"). A null parse renders the closed copy (never claim an unproven merge); the tests below must show that arm is unreachable from the ingest route once the validator rejects it.
  • Add a producer-parity test pinning packages/loopover-miner/lib/ams-notifications.ts's buildAmsPrOutcomePayload output against AMS_PR_OUTCOME_DEDUP_KEY_PATTERN for both merged and closed.

⚠️ Required pattern: the source-pinned drift guard at test/unit/advisory-spend-gate.test.ts:49-71 ("adoption is real, not aspirational") is the convention for the parity test. It does NOT satisfy this issue to add a decision field to DetectedNotificationEvent / the deliveries schema (a schema migration, out of scope); to validate only in src/api/routes.ts's zod schema while normalizeAmsNotificationEventInput stays permissive (the MCP/self-host path reaches the normalizer directly); or to keep .includes(":merged:") and merely add tests around it.

Deliverables

  • parseAmsPrOutcomeDecision("ams_pr_outcome:owner/repo#7:merged:2026-05-28T12:00:00.000Z") returns "merged", the :closed: twin returns "closed", and "custom-key" returns null — asserted in test/unit/notifications-ams-events.test.ts.
  • normalizeAmsNotificationEventInput({ ...validAmsPrOutcome, dedupKey: "custom-key" }, "miner") returns null, while the same malformed key on an ams_attempt_started event is still accepted — two cases.
  • buildAmsPrOutcomeNotification renders the merge copy for a :merged: key and the close copy for both a :closed: key and an unparseable key — extending test/unit/notifications-service.test.ts:144-155.
  • A parity test asserts buildAmsPrOutcomePayload produces a dedupKey matching AMS_PR_OUTCOME_DEDUP_KEY_PATTERN for both decisions.
  • src/notifications/service.ts no longer contains the string ":merged:" (grep-verifiable).

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example swapping the consumer without tightening normalizeAmsNotificationEventInput — does not resolve this issue.

Test Coverage Requirements

99%+ Codecov patch coverage, branch-counted, on src/**; src/notifications/ams-events.ts and src/notifications/service.ts are inside coverage.include, so both arms of every changed conditional need a test. packages/loopover-miner/lib/** is ALSO inside coverage.include and gated, so the parity test is both mandatory and measured.

Expected Outcome

A merged/closed AMS notification's wording is decided by a validated, single-sourced key format instead of a substring search over arbitrary client input. A miner-side edit that reorders the dedup key's segments fails CI instead of silently reporting every merged PR as "closed without merge".

Links & Resources

src/notifications/service.ts:89-107; src/notifications/ams-events.ts:107-128, :134-156; packages/loopover-miner/lib/ams-notifications.ts:118-139; src/api/routes.ts:479-493, :3842; test/unit/advisory-spend-gate.test.ts:49-71.

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions