Skip to content

[Spec 13] feat: filter notifications by conclusion (v0.6.0)#14

Merged
waleedkadous merged 9 commits into
mainfrom
builder/aspir-13-feat-filter-notifications-by-c
Apr 18, 2026
Merged

[Spec 13] feat: filter notifications by conclusion (v0.6.0)#14
waleedkadous merged 9 commits into
mainfrom
builder/aspir-13-feat-filter-notifications-by-c

Conversation

@waleedkadous

Copy link
Copy Markdown
Contributor

Summary

Adds a --conclusions CLI flag (with matching CONCLUSIONS env var) that filters ci-channel notifications by CI run outcome. The channel is marketed as a CI failure alerting tool, but it previously forwarded every workflow_run / pipeline event regardless of outcome. This PR aligns the implementation with the stated intent.

Closes #13. Ships as v0.6.0 (minor — user-visible behavior change).

Behavior change

  • By default: only failure-like outcomes are forwarded. success, skipped, neutral, manual, stale, plus in-progress states (requested, in_progress, completed, running, pending, queued, waiting, preparing) are dropped. Unknown strings are forwarded (fail-open for novel forge outcomes).
  • Opt out: --conclusions all restores pre-0.6.0 behavior (forward everything).
  • Custom: e.g. --conclusions failure,success forwards just those two.

Values are case-insensitive and normalized for cross-forge terminology (failedfailure, canceledcancelled).

Changes

Production

  • lib/webhook.ts — new normalizeConclusion + isConclusionAllowed helpers (forge-agnostic)
  • lib/config.ts — new conclusions: string[] | null field, --conclusions flag, CONCLUSIONS env var, normalization + mixed-all rejection at load time
  • lib/handler.ts — new filter step 6 (between workflow filter and notification)
  • lib/bootstrap.tsformatConclusionsSummary helper; startup banner includes the active filter description
  • Docs: README.md, INSTALL.md, CLAUDE.md, AGENTS.md updated with flag docs + v0.6.0 upgrade note
  • package.json / package-lock.json0.6.0

Tests (237 total, +36 new, 0 failures)

  • tests/webhook.test.ts — 16 pure-function tests covering normalizeConclusion + all default / explicit / all / cross-forge / unknown-value cases
  • tests/config.test.ts — 8 config-layer integration tests (precedence, normalization, all-sentinel validation, env-over-.env)
  • tests/integration.test.ts — 4 handler-integration tests covering filter on/off for success + failure events
  • tests/integration-gitlab.test.ts — 1 cross-forge test (GitLab's failed/running normalizes through filter correctly)
  • tests/bootstrap.test.ts — 4 tests covering formatConclusionsSummary + banner integration
  • Mechanical: conclusions field added to 7 existing Config fixtures

Testing

  • npm run build — passes (TypeScript compilation clean)
  • npm test — 237 passed, 0 failed, 2 pre-existing skipped
  • Manual smoke: npx tsx server.ts --conclusions failure,all errors with the documented message; --conclusions failure loads cleanly

Docs

  • Spec: codev/specs/13-feat-filter-notifications-by-c.md
  • Plan: codev/plans/13-feat-filter-notifications-by-c.md
  • Review (with consultation feedback, architecture + lessons-learned updates): codev/reviews/13-feat-filter-notifications-by-c.md

Upgrade note

Pre-0.6.0 users who relied on the prior "every event forwarded" behavior: add --conclusions all to your .mcp.json args. This is the documented escape hatch and is the only backwards-compatibility knob.

@waleedkadous

Copy link
Copy Markdown
Contributor Author

Architect Integration Review

Verdict: APPROVE (medium risk, single-model review)

Summary

Clean, well-patterned feature that mirrors existing codebase conventions (isConclusionAllowed mirrors isWorkflowAllowed, config parsing follows workflowFilter exactly, handler pipeline insertion is a clean step 6). ~91 lines of actual lib code. 238 tests pass.

Architectural fit

  • No new files, no new dependencies
  • The exclusion-based default (vs. inclusion-based explicit) is well-justified by the competing requirements (fail-open for unknowns + strict-by-default) and documented via JSDoc
  • setup.ts correctly untouched — new installs get failures-only by default via the library default
  • reconcile.ts scoped out with a documented audit (all three forges already filter to failures there)

Migration

  • Breaking change is appropriately communicated: README upgrade note, INSTALL.md callout, startup banner shows active filter mode
  • Escape hatch is clean: --conclusions all restores pre-0.6.0 behavior
  • Version bump 0.5.5 → 0.6.0 is appropriate

Follow-ups (non-blocking)

  • If reconciliation is ever changed to emit non-failure events, the conclusion filter should be applied there too
  • Consider a future --log-filtered flag for observability

Architect integration review

@waleedkadous waleedkadous merged commit 4cf6b6b into main Apr 18, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: filter notifications by conclusion (default: failures only)

1 participant