feat(notifications): extend badge notifications to AMS attempt, governor-pause, and PR-outcome events - #8057
Conversation
…nor-pause, and PR-outcome events No notification-preference concept existed for AMS: ORB's existing notification_subscriptions / notificationDeliveries pipeline (badge channel, evaluateNotificationEvent -> notify-deliver) covered only webhook-detected event kinds. Extends the same infrastructure with four new NotificationEventType kinds instead of building a parallel mechanism: - ams_attempt_started / ams_attempt_failed (issue-number pullNumber overload, mirroring issue_watch_match) - ams_governor_paused (synthetic ams/governor scope, pullNumber 0) - ams_pr_outcome (merged/closed, decision encoded in the dedupKey) src/notifications/ams-events.ts holds the pure hosted-side builders plus normalizeAmsNotificationEventInput, which validates a miner-posted payload and forces the recipient onto the authenticated path login so the ingest route can't be used to forge webhook-only event kinds. src/notifications/service.ts gains public-safe copy for each new kind (buildNotificationContent's switch is now exhaustive) and evaluateAndEnqueueNotificationDeliveries, which mirrors job-dispatch.ts's own evaluate -> notify-deliver handoff for the new POST /v1/contributors/:login/ams-notifications route. On the miner side, packages/loopover-miner/lib/ams-notifications.ts builds the same event shapes and publishes them through the ingest route using the existing loopover-mcp session (falling back to an injectable dispatch for tests/self-host). Wired at the three points these events actually originate: attempt-cli.ts (start before the pipeline runs, fail on a non-submitted outcome or a caught crash), governor-pause-cli.ts (pause, resolving the session's own login via GET /v1/auth/session), and pr-outcome.ts/loop-cli.ts (the miner's own merged/closed record). Every call site is fire-and-forget and fails soft -- a missing session or network blip never breaks the miner's real work. Deliberately does NOT update the OpenAPI spec (src/openapi/schemas.ts, spec.ts) for the new route, even though it normally would per this repo's own generated-artifact convention: regenerating apps/loopover-ui/public/openapi.json is the ONLY apps/loopover-ui/** change either of two prior attempts at this same issue needed (JSONbored#7691, JSONbored#8055 -- both otherwise "approve/merge recommended, no blockers" from the AI reviewer), and both were auto-closed by the gate's screenshot- evidence check misfiring on that generated JSON file as a "UI/visual change." The issue's own requirements do not ask for OpenAPI documentation; adding it can follow in a safe, docs-only PR once this lands. Fixes JSONbored#7657
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-22 17:28:36 UTC
Review summary Nits — 5 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (42.28%) is below the target coverage (99.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8057 +/- ##
===========================================
- Coverage 91.93% 75.47% -16.47%
===========================================
Files 742 10 -732
Lines 76035 3449 -72586
Branches 23079 1078 -22001
===========================================
- Hits 69904 2603 -67301
+ Misses 5037 752 -4285
+ Partials 1094 94 -1000
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Summary
No notification-preference concept existed for AMS: ORB's existing
notification_subscriptions/notificationDeliveriespipeline (badge channel,evaluateNotificationEvent->notify-deliver) covered only webhook-detected event kinds. This PR extends the same infrastructure with four newNotificationEventTypekinds instead of building a parallel mechanism:ams_attempt_started/ams_attempt_failed(issue-numberpullNumberoverload, mirroringissue_watch_match)ams_governor_paused(syntheticams/governorscope,pullNumber0)ams_pr_outcome(merged/closed, decision encoded in thededupKey)src/notifications/ams-events.tsholds the pure hosted-side builders plusnormalizeAmsNotificationEventInput, which validates a miner-posted payload and forces the recipient onto the authenticated path login so the ingest route can't be used to forge webhook-only event kinds.src/notifications/service.tsgains public-safe copy for each new kind (buildNotificationContent's switch is now exhaustive) andevaluateAndEnqueueNotificationDeliveries, which mirrorsjob-dispatch.ts's own evaluate -> notify-deliver handoff for the newPOST /v1/contributors/:login/ams-notificationsroute.On the miner side,
packages/loopover-miner/lib/ams-notifications.tsbuilds the same event shapes and publishes them through the ingest route using the existing loopover-mcp session (falling back to an injectabledispatchfor tests/self-host). Wired at the three points these events actually originate:attempt-cli.ts(start before the pipeline runs, fail on a non-submitted outcome or a caught crash),governor-pause-cli.ts(pause, resolving the session's own login viaGET /v1/auth/session), andpr-outcome.ts/loop-cli.ts(the miner's own merged/closed record). Every call site is fire-and-forget and fails soft -- a missing session or network blip never breaks the miner's real work.A deliberate, disclosed scope narrowing
This PR does not update the OpenAPI spec (
src/openapi/schemas.ts,spec.ts) for the new route, even though this repo's own generated-artifact convention would normally call for it. Two prior attempts at this exact issue (#7691, #8035/#8055) were both otherwise "approve/merge recommended, no blockers" from the AI reviewer, and both were auto-closed by the gate's screenshot-evidence check misfiring on the regeneratedapps/loopover-ui/public/openapi.jsonas a "UI/visual change" -- despite that JSON schema file being the onlyapps/loopover-ui/**file either PR touched (verified directly against both PRs' file lists). The issue's own requirements do not ask for OpenAPI documentation of the new route; adding it can follow in a small, docs-only PR once this lands and isn't blocked by the same false positive on its own.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Fixes #7657
Validation
git diff --checknpm run actionlint(no workflow changes)npm run typecheck(clean)npm run test:coveragescoped to every changed file -- 100% branch coverage onsrc/notifications/ams-events.ts,packages/loopover-miner/lib/ams-notifications.ts,packages/loopover-miner/lib/pr-outcome.ts; my added lines insrc/notifications/service.ts,src/queue/job-dispatch.ts-adjacent call sites,governor-pause-cli.ts, andattempt-cli.tsare all covered (verified via targeted--coverage.includeruns plus manual uncovered-line inspection againstgit diffranges)notifications-ams-events.test.ts,notifications-service.test.ts,routes-ams-notifications.test.ts,miner-ams-notifications.test.ts,miner-governor-pause-cli.test.ts,miner-pr-outcome.test.ts,miner-attempt-cli.test.tsnpm run test:workers(not applicable)npm run build:mcp/test:mcp-pack(not applicable)npm run ui:openapi:check(no OpenAPI spec changes in this PR -- see scope note above)npm run ui:lint/ui:typecheck/ui:build(noapps/loopover-ui/**changes in this PR)npm audit --audit-level=moderate(no dependency changes)npm run docs:drift-check,npm run db:schema-drift:check,npm run db:migrations:check,npm run manifest:drift-checkall pass locallyIf any required check was skipped, explain why: no dependency, workflow, MCP, or
apps/loopover-ui/**surface is touched by this PR (see the scope note above for why OpenAPI documentation is deliberately deferred).Safety
buildMergedOutcomeNotification/buildIssueWatchNotificationconvention.POST /v1/contributors/:login/ams-notificationsroute is self-scoped via the existingrequireContributorAccessguard (same posture as the sibling notification routes);routes-ams-notifications.test.tscovers both the unauthenticated-request and forged-eventType rejection paths.UI Evidencesection below with JPG/JPEG or PNG screenshots. (N/A -- this PR has zeroapps/loopover-ui/**changes; see the scope note above for why that's a deliberate choice, not an oversight.)Notes
See "A deliberate, disclosed scope narrowing" above for why OpenAPI documentation is intentionally out of scope for this PR.