diff --git a/src/notifications/ams-events.ts b/src/notifications/ams-events.ts index 594465577f..536f0feff8 100644 --- a/src/notifications/ams-events.ts +++ b/src/notifications/ams-events.ts @@ -103,6 +103,27 @@ export function buildAmsGovernorPausedEvent(input: { }; } +// The `ams_pr_outcome` dedupKey is the ONLY carrier of the merged/closed decision (DetectedNotificationEvent +// has no `decision` field), so its exact format is load-bearing. Both the producer below and the reader +// (parseAmsPrOutcomeDecision) derive from this one prefix so the format is spelled once, not twice (#9703). +const AMS_PR_OUTCOME_DEDUP_KEY_PREFIX = "ams_pr_outcome"; + +/** Build the decision-carrying dedupKey. The single source of the `ams_pr_outcome:...:decision:closedAt` shape. */ +function amsPrOutcomeDedupKey(repoFullName: string, pullNumber: number, decision: "merged" | "closed", closedAt: string): string { + return `${AMS_PR_OUTCOME_DEDUP_KEY_PREFIX}:${repoFullName}#${pullNumber}:${decision}:${closedAt}`; +} + +/** Anchored matcher for a well-formed `ams_pr_outcome` dedupKey, with the decision as capture group 1. Derives + * its prefix + decision segment from the same constants the producer uses, so the two cannot drift (#9703). */ +export const AMS_PR_OUTCOME_DEDUP_KEY_PATTERN = new RegExp(`^${AMS_PR_OUTCOME_DEDUP_KEY_PREFIX}:.+#\\d+:(merged|closed):.+$`); + +/** The single reader of AMS_PR_OUTCOME_DEDUP_KEY_PATTERN: returns the embedded decision, or null when the key is + * malformed (missing/unknown decision segment). A null result must never be rendered as an unproven merge. */ +export function parseAmsPrOutcomeDecision(dedupKey: string): "merged" | "closed" | null { + const match = AMS_PR_OUTCOME_DEDUP_KEY_PATTERN.exec(dedupKey); + return match ? (match[1] as "merged" | "closed") : null; +} + /** Miner-local PR outcome change (merged or closed). */ export function buildAmsPrOutcomeEvent(input: { recipientLogin: string; @@ -120,7 +141,7 @@ export function buildAmsPrOutcomeEvent(input: { recipientLogin, repoFullName: input.repoFullName, pullNumber: input.pullNumber, - dedupKey: `ams_pr_outcome:${input.repoFullName}#${input.pullNumber}:${input.decision}:${closedAt}`, + dedupKey: amsPrOutcomeDedupKey(input.repoFullName, input.pullNumber, input.decision, closedAt), deeplink: githubPullDeeplink(input.repoFullName, input.pullNumber), actorLogin: recipientLogin, detectedAt, @@ -140,6 +161,11 @@ export function normalizeAmsNotificationEventInput( if (!isAmsNotificationEventType(record.eventType)) return null; if (typeof record.repoFullName !== "string" || !record.repoFullName.trim()) return null; if (typeof record.dedupKey !== "string" || !record.dedupKey.trim()) return null; + // ams_pr_outcome-only: its dedupKey is the sole carrier of the merged/closed decision, so a key that does not + // parse would silently render as "closed without merge" (including for a real merge). Reject it at ingest -- + // the MCP/self-host path reaches this normalizer directly, so validating only in the HTTP zod schema is not + // enough. Every other AMS kind keeps its existing non-empty-string check unchanged (#9703). + if (record.eventType === "ams_pr_outcome" && parseAmsPrOutcomeDecision(record.dedupKey.trim()) === null) return null; if (typeof record.deeplink !== "string" || !record.deeplink.trim()) return null; if (typeof record.actorLogin !== "string" || !record.actorLogin.trim()) return null; if (typeof record.detectedAt !== "string" || !record.detectedAt.trim()) return null; diff --git a/src/notifications/service.ts b/src/notifications/service.ts index 3f0a60e433..58d4cad800 100644 --- a/src/notifications/service.ts +++ b/src/notifications/service.ts @@ -1,4 +1,5 @@ import { sanitizePublicComment } from "../github/commands"; +import { parseAmsPrOutcomeDecision } from "./ams-events"; import { countRecentNotificationDeliveries, getNotificationDeliveryById, @@ -88,8 +89,10 @@ export function buildAmsGovernorPausedNotification(_event: DetectedNotificationE export function buildAmsPrOutcomeNotification(event: DetectedNotificationEvent): { title: string; body: string } { const ref = `${event.repoFullName}#${event.pullNumber}`; - // buildAmsPrOutcomeEvent embeds `:merged:` or `:closed:` in the dedupKey after the PR number. - const merged = event.dedupKey.includes(":merged:"); + // The decision is carried in the dedupKey; read it through the single anchored parser rather than a loose + // substring scan. A null parse renders the CLOSED copy -- never claim an unproven merge -- though the ingest + // normalizer now rejects a malformed ams_pr_outcome key upstream, so that arm is unreachable from ingest (#9703). + const merged = parseAmsPrOutcomeDecision(event.dedupKey) === "merged"; if (merged) { return { title: sanitizePublicComment(`AMS recorded merge: ${ref}`), diff --git a/test/unit/miner-ams-notifications.test.ts b/test/unit/miner-ams-notifications.test.ts index 83431b9ce3..b3ea099ee6 100644 --- a/test/unit/miner-ams-notifications.test.ts +++ b/test/unit/miner-ams-notifications.test.ts @@ -10,6 +10,8 @@ import { publishAmsNotificationEvents, scheduleAmsNotificationEvents, } from "../../packages/loopover-miner/lib/ams-notifications"; +// #9703: the SERVER's anchored decision-parser -- the consumer this producer must stay format-compatible with. +import { parseAmsPrOutcomeDecision } from "../../src/notifications/ams-events"; const roots: string[] = []; @@ -240,4 +242,14 @@ describe("ams-notifications (#7657)", () => { it("returns sent 0 for an empty event list", async () => { await expect(publishAmsNotificationEvents([])).resolves.toEqual({ sent: 0 }); }); + + // #9703: the miner producer here is what actually runs; the server consumer recovers the merged/closed + // decision by parsing this dedupKey. Pin the producer's output against the server's anchored pattern so a + // reorder of decision/closedAt (which would silently flip every merge to "closed without merge") fails loudly. + it("buildAmsPrOutcomePayload's dedupKey satisfies the server's AMS_PR_OUTCOME_DEDUP_KEY_PATTERN (drift guard)", () => { + for (const decision of ["merged", "closed"] as const) { + const payload = buildAmsPrOutcomePayload({ recipientLogin: "miner", repoFullName: "acme/widgets", pullNumber: 7, decision, closedAt: "2026-07-21T00:00:00.000Z" }); + expect(parseAmsPrOutcomeDecision(payload.dedupKey)).toBe(decision); + } + }); }); diff --git a/test/unit/notifications-ams-events.test.ts b/test/unit/notifications-ams-events.test.ts index 14edf5f328..d25f492f15 100644 --- a/test/unit/notifications-ams-events.test.ts +++ b/test/unit/notifications-ams-events.test.ts @@ -1,11 +1,13 @@ import { describe, expect, it } from "vitest"; import { + AMS_PR_OUTCOME_DEDUP_KEY_PATTERN, buildAmsAttemptFailedEvent, buildAmsAttemptStartedEvent, buildAmsGovernorPausedEvent, buildAmsPrOutcomeEvent, isAmsNotificationEventType, normalizeAmsNotificationEventInput, + parseAmsPrOutcomeDecision, } from "../../src/notifications/ams-events"; describe("AMS notification event builders (#7657)", () => { @@ -100,6 +102,47 @@ describe("AMS notification event builders (#7657)", () => { expect(closed.dedupKey).toContain(":closed:"); }); + it("#9703: parseAmsPrOutcomeDecision reads the decision from a well-formed dedupKey and null otherwise", () => { + // The builder's own key round-trips through the parser for both decisions. + const merged = buildAmsPrOutcomeEvent({ recipientLogin: "miner", repoFullName: "acme/widgets", pullNumber: 7, decision: "merged" }); + const closed = buildAmsPrOutcomeEvent({ recipientLogin: "miner", repoFullName: "acme/widgets", pullNumber: 7, decision: "closed" }); + expect(parseAmsPrOutcomeDecision(merged.dedupKey)).toBe("merged"); + expect(parseAmsPrOutcomeDecision(closed.dedupKey)).toBe("closed"); + expect(AMS_PR_OUTCOME_DEDUP_KEY_PATTERN.test(merged.dedupKey)).toBe(true); + + // Malformed keys: wrong prefix, no decision segment, an unknown decision, and the loose-substring trap where + // ":merged:" appears somewhere other than the decision segment. All must parse to null, never a false merge. + for (const bad of [ + "k", + "ams_pr_outcome:acme/widgets#7", // truncated, no decision/closedAt + "ams_pr_outcome:acme/widgets#7:reopened:2026-07-21T00:00:00.000Z", // unknown decision + "other:acme/widgets#7:merged:2026-07-21T00:00:00.000Z", // wrong prefix + "ams_pr_outcome:acme/widgets:merged:#7:2026", // no # before the decision + ]) { + expect(parseAmsPrOutcomeDecision(bad)).toBeNull(); + } + }); + + it("#9703: normalizeAmsNotificationEventInput rejects an ams_pr_outcome whose dedupKey has no valid decision", () => { + const base = { + repoFullName: "acme/widgets", + pullNumber: 7, + deeplink: "https://example.com", + actorLogin: "miner", + detectedAt: "2026-07-21T00:00:00.000Z", + }; + // A malformed ams_pr_outcome key would silently render as "closed without merge" -- rejected at ingest. + expect(normalizeAmsNotificationEventInput({ ...base, eventType: "ams_pr_outcome", dedupKey: "ams_pr_outcome:no-decision" }, "miner")).toBeNull(); + // A well-formed one is accepted. + const good = normalizeAmsNotificationEventInput( + { ...base, eventType: "ams_pr_outcome", dedupKey: "ams_pr_outcome:acme/widgets#7:merged:2026-07-21T00:00:00.000Z" }, + "miner", + ); + expect(good).not.toBeNull(); + // The ams_pr_outcome-only rule does not touch other AMS kinds: a loose dedupKey still passes for them. + expect(normalizeAmsNotificationEventInput({ ...base, eventType: "ams_attempt_started", dedupKey: "k" }, "miner")).not.toBeNull(); + }); + it("normalizes ingest payloads and rejects non-AMS kinds", () => { expect(isAmsNotificationEventType("ams_attempt_started")).toBe(true); expect(isAmsNotificationEventType("pull_request_merged")).toBe(false); diff --git a/test/unit/notifications-service.test.ts b/test/unit/notifications-service.test.ts index 00bf94a944..4f9ffee4aa 100644 --- a/test/unit/notifications-service.test.ts +++ b/test/unit/notifications-service.test.ts @@ -156,6 +156,15 @@ describe("AMS notification event kinds (#7657)", () => { ); expect(closed.title.toLowerCase()).toContain("close"); expect(closed.body.toLowerCase()).toContain("without merge"); + + // #9703: a malformed dedupKey (no valid decision) renders the CLOSED copy -- never a false merge. This arm + // is unreachable from the ingest route (normalizeAmsNotificationEventInput rejects such a key upstream), but + // buildAmsPrOutcomeNotification stays fail-safe on its own regardless of how the event was constructed. + const malformed = buildNotificationContent( + event({ eventType: "ams_pr_outcome", dedupKey: "ams_pr_outcome:owner/repo#7:no-decision-here" }), + ); + expect(malformed.title.toLowerCase()).toContain("close"); + expect(malformed.body.toLowerCase()).toContain("without merge"); }); it("evaluates AMS events and enqueues notify-deliver jobs (job-dispatch handoff shape)", async () => {