feat(notifications): extend badge notifications to AMS attempt, governor-pause, and PR-outcome events - #8055
Conversation
…nor-pause, and PR-outcome events (JSONbored#7657)
loopover-orb
Bot
added
the
gittensor:feature
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-22 16:48:25 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. Visual preview
Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy · Diff highlights exactly what changed. Scroll preview
A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show. 🟩 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.
|
|
This pull request changes UI/visual code but its screenshot evidence is incomplete. Every required viewport × theme combination needs its own before/after image pair in a labeled table row (e.g. "Desktop · Light | before | after"). Still missing: Desktop · Dark, Tablet · Dark, Mobile · Dark. Please resubmit with the remaining rows filled in. See https://github.com/JSONbored/loopover/blob/main/.claude/skills/contributing-to-loopover/SKILL.md for the exact format and examples. This is an automated maintenance action. |
…nor-pause, and PR-outcome events (#8057) 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 (#7691, #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 #7657
1 participant
Summary
Closes #7657.
Extends the existing notification-subscription infrastructure to AMS-relevant events, exactly along the issue's required pattern — every new kind fires through the existing
evaluateNotificationEvent→notify-deliverpath thatsrc/queue/job-dispatch.tsuses for webhook-detected kinds; no parallel notification mechanism is introduced.ams_attempt_started,ams_attempt_failed,ams_governor_paused,ams_pr_outcomeadded to the existingNotificationEventTypeunion (src/types.ts), with public-safe content builders and an exhaustivebuildNotificationContentswitch insrc/notifications/service.ts.src/notifications/ams-events.ts— pure event builders plus the ingest-side validator. Recipient AND actor are always re-stamped from the authenticated login (a payload can never notify or impersonate someone else), and non-AMS kinds are rejected so the ingest cannot forge webhook notification types.POST /v1/contributors/:login/ams-notifications, self-scoped viarequireContributorAccess(the same guard the sibling notifications routes use), evaluating through a newevaluateAndEnqueueNotificationDeliverieshelper inservice.tsthat mirrorsjob-dispatch.ts's notify-evaluate → notify-deliver handoff shape.canSessionAccessPath's allowlist following that function's documented convention ("a path added here MUST be scoped by a per-route guard in its handler") — without this, a miner's own loopover-mcp session bearer getsinsufficient_rolefrom the coarse middleware and the ingest is unreachable for the exact client it exists for. Covered by a test using a real created session.packages/loopover-miner/lib/): a new fail-softams-notifications.tsclient (payload builders mirroring the hosted dedupKey/deeplink layouts + a session-authenticated POST that never throws into miner work), wired at the points the events actually occur:attempt-cli.ts(attempt start, crash, and non-submitted outcomes),governor-pause-cli.ts(pause, after the persisted write), andpr-outcome.ts/loop-cli.ts(recorded PR outcome, after the ledger write).No DB schema change was needed —
notification_deliveries.event_typealready stores the kind as free text (documented insrc/db/schema.ts).Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
test:coverage(30+ min on this machine), I ran every touched test surface with coverage —notifications-ams-events,notifications-service,routes-ams-notifications(new),routes-notifications,access-boundary,miner-ams-notifications(new),miner-pr-outcome,miner-governor-pause-cli,miner-attempt-cli,miner-loop-cli,openapi,write-cloudflare-schema— all green (300+ tests), and measured patch coverage empirically by intersectingcoverage/lcov.infoDA/BRDArecords withgit diff -U0againstupstream/main: 100% of changed lines and 100% of changed branches in every coverage-scoped file (src/**andpackages/loopover-miner/lib/**).test:workers,build:mcp,test:mcp-pack,ui:lint,ui:typecheck,ui:build: untouched surfaces (notest/workers/**,packages/loopover-mcp/**, or UI-source change; the onlyapps/change is the regeneratedopenapi.json, andui:openapi:checkpasses). I additionally rannpm run build:minerandnpm run test:miner-pack(both green) since the miner package is touched, even though the template does not list them.npm audit --audit-level=moderatecurrently fails onupstream/mainitself (pre-existing wrangler/miniflare/nitro advisories). This PR adds zero dependencies, so the dependency-review gate (which checks newly added deps) is unaffected.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.The two unchecked Safety boxes are N/A: there is no visible UI change in this PR (the only
apps/diff is the regeneratedopenapi.json).Auth negative paths covered: a session for a different login is rejected 403
forbidden_contributor; the shared MCP token is rejected 403 unless fully unscoped (mirroring the #2455 parity tests on the sibling notifications routes); the miner's own session succeeds end-to-end. All notification copy passes throughsanitizePublicComment, and tests assert the absence of forbidden terms.UI Evidence
N/A — no visible UI change (backend + miner CLI only; the
apps/diff is the regeneratedopenapi.json).Notes
src/notifications/ams-events.ts↔ minerpackages/loopover-miner/lib/ams-notifications.ts) keep dedupKey/deeplink layouts in lockstep, with a header comment in each pointing at the other.components.schemas$ref), matching every other request body in the generator so Cloudflare schema pruning stays response-only (write-cloudflare-schemainvariant test run green).