add channel connectivity and command - #562
Conversation
|
@codefather2026 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
WalkthroughThe alerts CLI now reuses a shared synthetic event builder, lists registered channel plugins, and tests delivery across all configured channels for a contract with summarized results and failure exit status. ChangesAlert CLI expansion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 2
🤖 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 `@src/commands/alerts.ts`:
- Around line 305-321: Update the channel-test loop around deliverSingleAlert to
start all channel checks concurrently rather than awaiting each one
sequentially, then await the combined results before populating results.
Preserve each result’s channelType, target, and success mapping, and avoid
introducing timeout behavior outside the scope of this change.
In `@tests/commands/alerts.test.ts`:
- Around line 839-854: Update the test containing registerAlertChannel and the
alerts channels output assertions to call _resetRegistryForTesting() during
cleanup after verifying the output, ensuring the dynamically registered
"matrix-listing-test" channel is removed before subsequent tests run.
🪄 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: 01ed2d23-2066-4bca-9bba-7e659bc55e43
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/commands/alerts.tstests/commands/alerts.test.ts
📜 Review details
🔇 Additional comments (6)
src/commands/alerts.ts (4)
202-226: LGTM!
263-263: LGTM!
386-386: LGTM!
22-36: 🎯 Functional CorrectnessNo change needed.
buildTestEventonly receives rows fromalert_configs, which are TTL-based and enforcethreshold_ledgers INTEGER NOT NULL; resource alert configs are stored in the separateresource_alert_configstable.> Likely an incorrect or invalid review comment.tests/commands/alerts.test.ts (2)
6-6: LGTM!
765-825: LGTM!
| for (const config of configs) { | ||
| const testEvent = buildTestEvent(config.contract_id, config.threshold_ledgers); | ||
| console.log(`Sending test alert to ${config.channel_type}:${config.channel_target}...`); | ||
|
|
||
| const success = await deliverSingleAlert( | ||
| config.channel_type, | ||
| config.channel_target, | ||
| testEvent, | ||
| config.webhook_secret, | ||
| ); | ||
|
|
||
| results.push({ | ||
| channelType: config.channel_type, | ||
| target: config.channel_target, | ||
| success, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider parallelizing channel tests and guarding against a hanging channel.
Channels are tested one at a time with a blocking await inside the loop. deliverSingleAlert/the underlying channel.send() don't appear to enforce a timeout, so one slow or unresponsive channel delays reporting the status of every subsequent channel — for a contract with several channels this can make test-all hang far longer than necessary, undermining the "quick connectivity check" purpose of the command.
♻️ Suggested parallelization
- const results: Array<{ channelType: string; target: string; success: boolean; }> = [];
-
- for (const config of configs) {
- const testEvent = buildTestEvent(config.contract_id, config.threshold_ledgers);
- console.log(`Sending test alert to ${config.channel_type}:${config.channel_target}...`);
-
- const success = await deliverSingleAlert(
- config.channel_type,
- config.channel_target,
- testEvent,
- config.webhook_secret,
- );
-
- results.push({
- channelType: config.channel_type,
- target: config.channel_target,
- success,
- });
- }
+ const results = await Promise.all(configs.map(async (config) => {
+ const testEvent = buildTestEvent(config.contract_id, config.threshold_ledgers);
+ console.log(`Sending test alert to ${config.channel_type}:${config.channel_target}...`);
+
+ const success = await deliverSingleAlert(
+ config.channel_type,
+ config.channel_target,
+ testEvent,
+ config.webhook_secret,
+ );
+
+ return { channelType: config.channel_type, target: config.channel_target, success };
+ }));This still doesn't cap the worst-case wall-clock time if a channel truly hangs — pairing it with a per-call timeout would fully close the gap, but that likely needs a change in deliverSingleAlert/channel implementations outside this file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const config of configs) { | |
| const testEvent = buildTestEvent(config.contract_id, config.threshold_ledgers); | |
| console.log(`Sending test alert to ${config.channel_type}:${config.channel_target}...`); | |
| const success = await deliverSingleAlert( | |
| config.channel_type, | |
| config.channel_target, | |
| testEvent, | |
| config.webhook_secret, | |
| ); | |
| results.push({ | |
| channelType: config.channel_type, | |
| target: config.channel_target, | |
| success, | |
| }); | |
| } | |
| const results = await Promise.all(configs.map(async (config) => { | |
| const testEvent = buildTestEvent(config.contract_id, config.threshold_ledgers); | |
| console.log(`Sending test alert to ${config.channel_type}:${config.channel_target}...`); | |
| const success = await deliverSingleAlert( | |
| config.channel_type, | |
| config.channel_target, | |
| testEvent, | |
| config.webhook_secret, | |
| ); | |
| return { channelType: config.channel_type, target: config.channel_target, success }; | |
| })); |
🤖 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 `@src/commands/alerts.ts` around lines 305 - 321, Update the channel-test loop
around deliverSingleAlert to start all channel checks concurrently rather than
awaiting each one sequentially, then await the combined results before
populating results. Preserve each result’s channelType, target, and success
mapping, and avoid introducing timeout behavior outside the scope of this
change.
| it("includes dynamically registered plugin channels in the output", () => { | ||
| registerAlertChannel({ | ||
| name: "matrix-listing-test", | ||
| channel: { send: vi.fn().mockResolvedValue(undefined) }, | ||
| targetOption: "url", | ||
| missingTargetError: "Error: --url is required when --type is matrix-listing-test.", | ||
| supportsSigning: false, | ||
| }); | ||
|
|
||
| parse(["alerts", "channels"]); | ||
|
|
||
| const output = consoleLogSpy.mock.calls.flat().join("\n"); | ||
| expect(output).toContain("matrix-listing-test"); | ||
| expect(output).toContain("url"); | ||
| expect(output).toContain("no"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the registry exposes any reset/unregister capability for tests.
rg -n 'export function' src/alerts/registry.tsRepository: AbdulmalikAlayande/sorokeep
Length of output: 442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== registry.ts =="
sed -n '1,90p' src/alerts/registry.ts
echo
echo "== imports and setup in alerts.test.ts around registry/cleanup =="
rg -n 'registerAlertChannel|_resetRegistryForTesting|afterEach|afterAll|beforeEach|beforeAll|alerts channels|consoleLogSpy' tests/commands/alerts.test.ts
echo
echo "== lines 800-870 =="
sed -n '800,870p' tests/commands/alerts.test.tsRepository: AbdulmalikAlayande/sorokeep
Length of output: 5577
Reset the alert channel registry after registering the plugin channel.
registerAlertChannel adds "matrix-listing-test" to the module-level registry and alerts channels reads from that same registry, so this expectation can leak into later tests in the same test worker. Use _resetRegistryForTesting() in this test’s cleanup after verifying output.
🤖 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 `@tests/commands/alerts.test.ts` around lines 839 - 854, Update the test
containing registerAlertChannel and the alerts channels output assertions to
call _resetRegistryForTesting() during cleanup after verifying the output,
ensuring the dynamically registered "matrix-listing-test" channel is removed
before subsequent tests run.
|
| 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.
|
Verified locally (tsc, lint, full test suite 1178/1178, npm audit, build, manual CLI smoke test of both new commands) and merged into |
3 participants
Summary
Closes #320
Closes #317
Adds two new
alertssubcommands:sorokeep alerts test-all --contract <id>to send a synthetic connectivity check to every configured alert channel for a contractsorokeep alerts channelsto list all registered alert channel plugins, including dynamically registered onesWhat Changed
alerts test-allinsrc/commands/alerts.tsalerts testtest-alltest-allexit non-zero when any channel delivery failsalerts channelsinsrc/commands/alerts.tstargetOption, and signing supportTests
alerts test-allsends a test event to every configured channel for a contractalerts test-allexits non-zero if any delivery failsalerts channelsprints all five built-in channelsalerts channelsoutput