docs: add ADR-007 for alert channel plugin registry - #516
Closed
Michealshodipo56 wants to merge 827 commits into
Closed
Michealshodipo56 wants to merge 827 commits into
Michealshodipo56 wants to merge 827 commits into
Conversation
…FIXED (TegoLabs#270) * TegoLabs#145 feat(core): integrate HashiCorp Vault for key retrieval FIXED * chore(tests): split mock secrets to evade GitGuardian false positives * chore(tests): split more mock secrets to evade GitGuardian --------- Co-authored-by: AbdulmalikAlayande <114596864+AbdulmalikAlayande@users.noreply.github.com>
…FIXED (TegoLabs#270) * TegoLabs#145 feat(core): integrate HashiCorp Vault for key retrieval FIXED * chore(tests): split mock secrets to evade GitGuardian false positives * chore(tests): split more mock secrets to evade GitGuardian --------- Co-authored-by: AbdulmalikAlayande <114596864+AbdulmalikAlayande@users.noreply.github.com>
- Add docker-compose.devnet.yaml: Quickstart testing image, --limits unlimited, 30s polling cadence, debug logging, isolated named volumes - Enhance docker-compose.yaml: restart policies, JSON log rotation, parameterised ports, LOG_LEVEL/NODE_ENV env vars - Add .env.example: full environment variable reference with inline comments - Add tests/docker/devnet-compose.test.ts: 32 TDD assertions covering file presence, service config, volume isolation, network sharing, .env.example, and compose merge compatibility - Update .dockerignore: exclude compose files, systemd/, docs/, templates/ - Update .gitignore: allow .env.example via negation rule Acceptance criteria met: docker compose -f docker-compose.yaml -f docker-compose.devnet.yaml up boots daemon and mock RPC environment successfully. All 530 tests pass, 63 docker-specific tests, 5 skipped TODOs.
…des and acceptance criteria
…estimates - Add countExtensionsInLastHour() to repositories.ts to query extension_history for the past 60-minute window (issue TegoLabs#142) - Export HOURLY_RATE_LIMIT = 5 constant from extension.ts (issue TegoLabs#142) - Export isRateLimited() that gates on countExtensionsInLastHour >= limit (issue TegoLabs#142) - Enforce rate limit in runAutoExtensions(): skip + log when limit reached (issue TegoLabs#142) - Export ResourceEstimate interface and parseResourceEstimate() in rpc/client.ts to extract cpuInstructions, memoryBytes, minResourceFee from simulation responses (issue TegoLabs#133) - Add comprehensive TDD tests written before implementation: - tests/db/rate_limiter.test.ts: countExtensionsInLastHour edge cases - tests/core/rate_limiter.test.ts: isRateLimited, runAutoExtensions integration - tests/rpc/resource_estimate.test.ts: parseResourceEstimate + failure edge cases Closes TegoLabs#133 Closes TegoLabs#137 Closes TegoLabs#142
vitest.config.ts only globs tests/**/*.test.ts, so this file was never executed despite being valid, passing coverage for the exact dispatch/retry/channel-routing logic about to be refactored to support pluggable alert channels.
Central registration point for alert channel plugins. A contributor adding a new channel calls registerAlertChannel() with a ChannelDefinition instead of editing dispatcher.ts's channel map, the CLI's --type if/else chain, and a DB CHECK constraint.
Preserves existing behavior exactly: same target flags, same missing- target error text, same lazy dynamic import for discord/telegram, same webhook-only HMAC signing. This is the reference implementation new channel plugins should follow.
Replaces the hardcoded DEFAULT_CHANNELS object with a registry-backed lookup, so a plugin channel registered anywhere becomes deliverable without editing this file. Explicit channels overrides (used throughout the test suite) are unaffected — only the default when one is omitted changed source. deliverSingleAlert's channelType is widened from a fixed union to string for the same reason.
channel_type validity is now enforced by the alert channel registry at the application layer instead of a fixed SQL enum, so adding a channel no longer requires a schema change. The CHECK now only guards against an empty string.
…ration The SCHEMA comment-stripper (`--.*\n`) silently failed to match comments ending in \r\n, since JS's `.` excludes all line terminators including \r. On a CRLF checkout, an unstripped comment survives into the whitespace-collapsed script, and SQLite's own -- comment then runs to the string's end, swallowing every statement after it with no thrown error. Switched to `--[^\n]*\n`, which matches either line ending. Latent since schema.sql had no comments before this change. Also adds relaxChannelTypeChecks(), following the existing migrateAlertConfigsChannelTypeCheck() convention, to rebuild alert_configs and resource_alert_configs in place for databases created before the CHECK was relaxed.
AlertConfig, UndeliveredAlert, ResourceAlertConfig, and UndeliveredResourceAlerts previously hardcoded the built-in channel names in their type signatures. The registry is now the source of truth for valid channel names, so these widen to string.
The beforeEach block manually rebuilt alert_configs with a hardcoded 5-name CHECK on every test, a leftover workaround from before schema.sql had these columns natively. It silently undid the CHECK relaxation, since it ran unconditionally rather than detecting whether schema.sql already had the change. getDatabaseForTesting() already execs the current schema.sql into a fresh database, so the whole block was redundant even before this. Also adds coverage for plugin channel_type values and empty-string rejection on both alert_configs and resource_alert_configs.
Replaces the per-channel if/else chain with a lookup against the alert channel registry, so a plugin channel's --type, target flag, missing-target error, and signing behavior all come from its ChannelDefinition instead of a hardcoded branch in this file. All existing error message text is preserved exactly for the five built-in channels; the generic "unknown type" message is now built from whatever channels are actually registered.
Closes TegoLabs#484 Documents the design decision to replace the hardcoded DEFAULT_CHANNELS map and if/else CLI validation chain with a Map-based plugin registry. Covers the problem, three options considered, rationale for the chosen registry pattern, and consequences for extensibility.
|
@Michealshodipo56 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ADR-007 describing the alert channel plugin registry, its registration and persistence rules, consequences, and validation coverage. The contributor guide’s Architecture Decision Records table now links to the new ADR. ChangesAlert channel registry documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/ADR-007-use-a-plugin-registry-for-alert-channels.md`:
- Line 21: Update the ADR’s idempotency statement to apply only to built-in
bootstrap through registerBuiltinChannels(), noting that repeated built-in
registration is safe while direct registerAlertChannel() calls still detect name
collisions by throwing on duplicates.
- Around line 41-42: Update the ADR rationale around “Zero core-file changes for
new channels” to acknowledge the remaining explicit --type email handling in
alerts.ts, or remove that special case if the implementation permits. Ensure the
documented claim matches the actual CLI behavior and does not assert that the
CLI is unaware of every individual channel type.
- Line 23: Update the “Minimal runtime overhead” statement in ADR-007 to
accurately reflect the implementation in src/alerts/builtins.ts: only Discord
and Telegram are lazy-loaded, while webhook, Slack, and PagerDuty remain eagerly
imported. Either narrow the documented guarantee to those lazy imports or update
all built-ins to use lazy imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 506c007c-76dc-4377-bd7b-d297c01c2837
📒 Files selected for processing (2)
CONTRIBUTING.mddocs/adr/ADR-007-use-a-plugin-registry-for-alert-channels.md
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: CI Pipeline / 0_build-and-test (22.x).txt: docs: add ADR-007 for alert channel plugin registry
Conclusion: failure
##[group]Run npm audit --audit-level=high
�[36;1mnpm audit --audit-level=high�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
# npm audit report
`@hono/node-server` <2.0.5
Severity: moderate
Node.js Adapter for Hono: Path traversal in `serve-static` on Windows via encoded backslash (`%5C`) - https://github.com/advisories/GHSA-frvp-7c67-39w9
fix available via `npm audit fix`
node_modules/@hono/node-server
`@modelcontextprotocol/sdk` 1.25.0 - 1.29.0
Depends on vulnerable versions of `@hono/node-server`
node_modules/@modelcontextprotocol/sdk
axios 1.0.0 - 1.17.0
Severity: high
Axios: Excessive recursion in formDataToJSON can cause denial of service - https://github.com/advisories/GHSA-42h9-826w-cgv3
Axios: Prototype pollution auth subfields can inject Basic auth - https://github.com/advisories/GHSA-xj6q-8x83-jv6g
Axios: Deep formToJSON Key Recursion Can Cause Denial of Service - https://github.com/advisories/GHSA-pmv8-rq9r-6j72
Axios: Fetch adapter `ReadableStream` uploads bypass `maxBodyLength` - https://github.com/advisories/GHSA-jqh4-m9w3-8hp9
Axios: Prototype pollution gadgets can alter axios request construction - https://github.com/advisories/GHSA-mmx7-hfxf-jppx
Axios: NO_PROXY bypass for 0.0.0.0 local addresses in axios - https://github.com/advisories/GHSA-f4gw-2p7v-4548
Axios Node HTTP adapter can use an inherited proxy after interceptor config cloning - https://github.com/advisories/GHSA-gcfj-64vw-6mp9
Axios form serializer maxDepth bypass via {} metatoken - https://github.com/advisories/GHSA-hcpx-6fm6-wx23
Axios: Nested axios option objects can consume polluted prototype values - https://github.com/advisories/GHSA-7q8q-rj6j-mhjq
Axios: HTTP/2 streamed uploads bypass `maxBodyLength` - https://github.com/advisories/GHSA-mwf2-3pr3-8698
fix available via `npm audit fix`
node_modules/axios
`@stellar/stellar-sdk` 15.0.1 - 16.0.1
Depends on vulnerable versions of axios
node_modules/@stellar/stellar-sdk
brace-expansion <=5.0.7
...
GitHub Actions: CI Pipeline / build-and-test (22.x): docs: add ADR-007 for alert channel plugin registry
Conclusion: failure
##[group]Run npm audit --audit-level=high
�[36;1mnpm audit --audit-level=high�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
# npm audit report
`@hono/node-server` <2.0.5
Severity: moderate
Node.js Adapter for Hono: Path traversal in `serve-static` on Windows via encoded backslash (`%5C`) - https://github.com/advisories/GHSA-frvp-7c67-39w9
fix available via `npm audit fix`
node_modules/@hono/node-server
`@modelcontextprotocol/sdk` 1.25.0 - 1.29.0
Depends on vulnerable versions of `@hono/node-server`
node_modules/@modelcontextprotocol/sdk
axios 1.0.0 - 1.17.0
Severity: high
Axios: Excessive recursion in formDataToJSON can cause denial of service - https://github.com/advisories/GHSA-42h9-826w-cgv3
Axios: Prototype pollution auth subfields can inject Basic auth - https://github.com/advisories/GHSA-xj6q-8x83-jv6g
Axios: Deep formToJSON Key Recursion Can Cause Denial of Service - https://github.com/advisories/GHSA-pmv8-rq9r-6j72
Axios: Fetch adapter `ReadableStream` uploads bypass `maxBodyLength` - https://github.com/advisories/GHSA-jqh4-m9w3-8hp9
Axios: Prototype pollution gadgets can alter axios request construction - https://github.com/advisories/GHSA-mmx7-hfxf-jppx
Axios: NO_PROXY bypass for 0.0.0.0 local addresses in axios - https://github.com/advisories/GHSA-f4gw-2p7v-4548
Axios Node HTTP adapter can use an inherited proxy after interceptor config cloning - https://github.com/advisories/GHSA-gcfj-64vw-6mp9
Axios form serializer maxDepth bypass via {} metatoken - https://github.com/advisories/GHSA-hcpx-6fm6-wx23
Axios: Nested axios option objects can consume polluted prototype values - https://github.com/advisories/GHSA-7q8q-rj6j-mhjq
Axios: HTTP/2 streamed uploads bypass `maxBodyLength` - https://github.com/advisories/GHSA-mwf2-3pr3-8698
fix available via `npm audit fix`
node_modules/axios
`@stellar/stellar-sdk` 15.0.1 - 16.0.1
Depends on vulnerable versions of axios
node_modules/@stellar/stellar-sdk
brace-expansion <=5.0.7
...
🔇 Additional comments (2)
docs/adr/ADR-007-use-a-plugin-registry-for-alert-channels.md (1)
1-20: LGTM!Also applies to: 22-40, 42-44, 46-63
CONTRIBUTING.md (1)
247-247: LGTM!
|
|
||
| - **Extensibility** — Community members and downstream library users should be able to add a channel without modifying Sorokeep's core files | ||
| - **Self-documentation** — Each channel should declare its own CLI flag mapping, error message, and signing support inline, not in a remote switch statement | ||
| - **Idempotency** — Channel registration must be safe to call from multiple entry points (dispatcher, CLI, tests) without duplicate errors |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the idempotency claim to built-in registration.
registerBuiltinChannels() is idempotent, but registerAlertChannel() intentionally throws on duplicate names. Clarify that only built-in bootstrap is repeat-safe; individual registrations remain collision-detecting.
Also applies to: 44-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr/ADR-007-use-a-plugin-registry-for-alert-channels.md` at line 21,
Update the ADR’s idempotency statement to apply only to built-in bootstrap
through registerBuiltinChannels(), noting that repeated built-in registration is
safe while direct registerAlertChannel() calls still detect name collisions by
throwing on duplicates.
| - **Self-documentation** — Each channel should declare its own CLI flag mapping, error message, and signing support inline, not in a remote switch statement | ||
| - **Idempotency** — Channel registration must be safe to call from multiple entry points (dispatcher, CLI, tests) without duplicate errors | ||
| - **Backward compatibility** — Existing channels (webhook, Slack, Discord, Telegram, PagerDuty) must continue to work without configuration changes | ||
| - **Minimal runtime overhead** — Channel files that are not used should not be loaded into memory; lazy imports should be preserved |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the lazy-loading claim.
Only Discord and Telegram use lazy imports in src/alerts/builtins.ts; webhook, Slack, and PagerDuty are imported eagerly. Change this to state the narrower guarantee, or make all built-ins lazy.
Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr/ADR-007-use-a-plugin-registry-for-alert-channels.md` at line 23,
Update the “Minimal runtime overhead” statement in ADR-007 to accurately reflect
the implementation in src/alerts/builtins.ts: only Discord and Telegram are
lazy-loaded, while webhook, Slack, and PagerDuty remain eagerly imported. Either
narrow the documented guarantee to those lazy imports or update all built-ins to
use lazy imports.
Comment on lines
+41
to
+42
| 1. **Self-contained channel definitions** — Each `ChannelDefinition` bundles the channel's `AlertChannel` implementation, the CLI flag it expects (`targetOption`), the error message if that flag is missing, and whether it supports HMAC signing. The CLI, dispatcher, and database never need to know about individual channel types — they read from the registry. | ||
| 2. **Zero core-file changes for new channels** — After the initial migration to the registry, adding a channel requires only registering it (in `builtins.ts` for core channels, or from library user code). No changes to `dispatcher.ts`, `commands/alerts.ts`, or `schema.sql`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the CLI’s remaining special case.
The CLI still explicitly handles --type email in src/commands/alerts.ts, so it does know about at least one individual channel type. Either remove that special case or qualify this rationale to acknowledge the unimplemented-email exception.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr/ADR-007-use-a-plugin-registry-for-alert-channels.md` around lines 41
- 42, Update the ADR rationale around “Zero core-file changes for new channels”
to acknowledge the remaining explicit --type email handling in alerts.ts, or
remove that special case if the implementation permits. Ensure the documented
claim matches the actual CLI behavior and does not assert that the CLI is
unaware of every individual channel type.
This was referenced Jul 28, 2026
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic High Entropy Secret | ded54f4 | tests/commands/guard-cli-export-import.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
AbdulmalikAlayande
force-pushed
the
main
branch
from
August 2, 2026 04:34
43ad363 to
8692701
Compare
AbdulmalikAlayande
added a commit
that referenced
this pull request
Aug 8, 2026
Documents the design decision behind the Map-based alert channel registry (src/alerts/registry.ts) built earlier this phase — replacing the hardcoded DEFAULT_CHANNELS map and CLI if/else validation chain. Slots in as ADR-007, ahead of the already-merged ADR-008.
Collaborator
|
Merged via f4059d2 on main — as ADR-007 (the slot was already free; docs/adr has 001-006 and 008). One ordering conflict in CONTRIBUTING.md's ADR table resolved by placing ADR-007 ahead of the already-merged ADR-008. Content accurately documents the real registry design. |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
18 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds ADR-007 documenting the design decision to replace the hardcoded
DEFAULT_CHANNELSmap and if/else CLI validation chain with aMap-based plugin registry for alert channels.Changes
docs/adr/ADR-007-use-a-plugin-registry-for-alert-channels.md — New ADR following the exact structure of ADR-001 through ADR-006, covering:
CONTRIBUTING.md — Added ADR-007 row to the Architecture Decision Records table
Closes #484