Repository navigation
feat: config fallback, slack webhook, unwatch, pause/resume monitoring - #309
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughSlack alerting is migrated from a token-based API call to a webhook-based ChangesSlack webhook migration
Contract active status and pause/resume/unwatch CLI
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/daemon.ts (1)
53-56: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDisplayed network still uses stale
options.network, will print "undefined" when omitted.The resolved
networkvariable (Line 23) correctly falls back toconfig.network, but the console output at Line 55 still readsoptions.networkdirectly instead of the resolvednetwork. Since--networkno longer has a hard-coded default, omitting it will printundefinedto the console even though the daemon actually starts against the config-derived network.🐛 Proposed fix
- console.log(` Network: ${chalk.cyan(options.network)}`); + console.log(` Network: ${chalk.cyan(network)}`);🤖 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/daemon.ts` around lines 53 - 56, The daemon status output is still reading the raw options value instead of the resolved network, so it can print undefined when --network is omitted. Update the console logging in daemon.ts to use the already-resolved network variable (the one derived from options.network with the config.network fallback) rather than options.network, so the displayed network always matches what the daemon actually uses.
🤖 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/alerts/slack.ts`:
- Around line 96-127: The Slack alert send path in send() currently lets
AbortError bubble up on timeout, which makes retry logs in dispatcher.ts hard to
understand. Update the fetch timeout handling around controller.abort()/fetch()
so the timeout case is caught and rethrown with a clear message like a webhook
timeout, while preserving the existing non-OK HTTP response handling and the
logger.debug context.
- Line 14: The Block Kit payload typing in buildBlocks has been loosened to
any[], which removes compile-time checks for header, details, and footer.
Restore the previous SlackBlock-based typing in buildBlocks and explicitly type
the local block variables so field names and structure are validated by the
compiler. Make sure the returned array is typed with the existing SlackBlock
union or equivalent block-specific types rather than any.
- Around line 89-94: The SlackChannel constructor is validating the wrong kind
of input: `SlackChannel` is being constructed with channel names like `#alerts`,
so the `startsWith("http")` webhook check rejects valid Slack targets. Update
`SlackChannel` to accept channel names as provided by `--type slack`, and move
any webhook URL validation to the webhook transport or the code that actually
handles webhooks, keeping the constructor aligned with its real input.
In `@src/commands/pause.ts`:
- Around line 19-22: The guard clause in pause.ts for the missing contract case
should stop execution explicitly after process.exit(1). Update the contract
lookup block in the pause command so that the !contract branch includes a return
after logging and exiting, matching the pattern used in watch.ts and preventing
fallthrough into setContractActiveStatus when process.exit is stubbed.
In `@src/commands/watch.ts`:
- Around line 178-181: The `watch.ts` guard that checks `!contract` should
follow the same early-exit pattern as the existing `!contractId` branch and
`pause.ts`: after calling `process.exit(1)` in the `watch` command, add an
immediate `return` so the function does not continue past the failure path. Use
the `watch` command’s `!contract` block and the `process.exit(1)` call as the
reference point.
- Around line 188-207: The async work inside `rl.question` in `watch.ts`
bypasses the surrounding `try/catch`, so errors from
`proceedWithDeletion`/`deleteContract` are not handled. Update the `rl.question`
callback to catch and route failures through the same error handling path used
by the outer `watch` flow, ensuring the `chalk.red("Failed to unwatch
contract...")` message is printed instead of an unhandled rejection. Use the
`rl.question`, `proceedWithDeletion`, and `deleteContract` logic as the
touchpoints when fixing this.
In `@src/db/repositories.ts`:
- Around line 98-117: The upsert in insertContract is forcing active to 1 in
JavaScript, so the ON CONFLICT path in the insertContract repository method
never preserves a paused contract. Move the defaulting logic into the SQL
statement so `@active` can remain NULL when omitted, allowing
COALESCE(excluded.active, contracts.active) to keep the existing value on
re-insert while still defaulting new rows to active. Use the insertContract
function and its ON CONFLICT(id) DO UPDATE clause to make the change.
In `@tests/alerts/slack.test.ts`:
- Around line 104-114: Add a test for the timeout/abort path in the Slack alert
sender so `channel.send` is covered when `TIMEOUT_MS` elapses. In
`tests/alerts/slack.test.ts`, extend the existing `Error handling` suite around
`channel.send` by using fake timers and a fetch mock that never resolves, then
advance time past `TIMEOUT_MS` and assert the promise rejects with the
abort/timeout error. Make sure the test targets the `send` method and its
`AbortController` timeout behavior.
In `@tests/alerts/state_change_alerts.test.ts`:
- Around line 208-209: The Slack alert flow is still passing channel names
through as webhook targets, so `SlackChannel` rejects valid existing Slack
configs. Update the contract between `src/commands/alerts.ts` and
`src/alerts/slack.ts` so `--channel` can remain a channel name like `#alerts`,
or add a migration/translation step before `new SlackChannel(...)` is created.
Make sure the affected alert creation and send path use the same target format
consistently across stored configs, fixtures, and `SlackChannel`.
In `@tests/commands/daemon.test.ts`:
- Around line 98-115: Add test coverage in daemon.test.ts for the CLI output
when the network option is omitted, since the current assertion only checks
startDaemon arguments. Use actionFn and the daemon command’s printed status
lines to verify the console includes the config-derived network value in the
“Network:” display, which will catch the options.network versus network bug in
src/commands/daemon.ts.
In `@tests/commands/pause.test.ts`:
- Around line 42-46: The missing-contract pause/resume tests only verify that
mockExit was called, but they do not ensure repositories.setContractActiveStatus
is skipped when repositories.getContract returns undefined. Update the
pauseActionFn and resumeActionFn missing-contract cases in the pause/resume
tests to also assert that the status setter was never called, so the tests catch
the missing return after process.exit(1) in the corresponding actions.
---
Outside diff comments:
In `@src/commands/daemon.ts`:
- Around line 53-56: The daemon status output is still reading the raw options
value instead of the resolved network, so it can print undefined when --network
is omitted. Update the console logging in daemon.ts to use the already-resolved
network variable (the one derived from options.network with the config.network
fallback) rather than options.network, so the displayed network always matches
what the daemon actually uses.
🪄 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: dc7a1ab2-e10d-47de-89d8-0d2b1657385a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
src/alerts/dispatcher.tssrc/alerts/slack.test.tssrc/alerts/slack.tssrc/commands/daemon.tssrc/commands/pause.tssrc/commands/resume.tssrc/commands/watch.tssrc/core/monitor.tssrc/db/database.tssrc/db/repositories.tssrc/db/schema.sqlsrc/index.tstests/alerts/slack.test.tstests/alerts/state_change_alerts.test.tstests/commands/daemon.test.tstests/commands/pause.test.tstests/commands/watch.test.ts
💤 Files with no reviewable changes (1)
- src/alerts/slack.test.ts
📜 Review details
🔇 Additional comments (21)
src/commands/daemon.ts (2)
12-27: Option defaults correctly moved to config-driven resolution.
networkandintervalMsresolution logic looks correct, falling back toconfig.network/config.pollingIntervalSeconds * 1000when CLI flags are omitted.
78-78: LGTM!tests/commands/daemon.test.ts (2)
6-10: LGTM!
57-60: LGTM!Also applies to: 88-88
src/alerts/dispatcher.ts (1)
11-11: LGTM!Also applies to: 46-46, 105-105
tests/alerts/slack.test.ts (1)
1-102: LGTM!tests/alerts/state_change_alerts.test.ts (1)
22-29: LGTM!Also applies to: 186-187
src/db/schema.sql (1)
8-8: LGTM!src/db/database.ts (1)
66-66: LGTM!tests/commands/watch.test.ts (5)
288-292: 📐 Maintainability & Code Quality | 💤 Low valueTest name doesn't match what's actually verified.
This relies on the default auto-mock returning
undefinedfromgetContract, not on any explicit "no contract id" guard in the handler — it's effectively a duplicate of the "contract is not found" case rather than testing missing-id behavior.
11-21: 📐 Maintainability & Code Quality | 💤 Low valueDead scaffolding left in the original describe block.
vi.mock("../../src/db/repositories")/vi.mock("node:readline"),mockReadline,unwatchActionFn, and the"unwatch"branch (line 31) added to the "Watch Command CLI" suite are never exercised by any test there — the actual unwatch coverage was added in a fully separate "Unwatch Command CLI" describe block below with its own independent setup. Consider removing this now-redundant scaffolding from the original describe block.Also applies to: 30-31
249-252: 📐 Maintainability & Code Quality | 💤 Low valueNew imports placed mid-file instead of at the top.
import * as repositories ...andimport readline from "node:readline"are declared after the closing});of the first describe block. ES module imports are hoisted so this works, but it's unconventional and likely to trip import-ordering lint rules; consider moving these next to the other top-of-file imports.
301-329: LGTM!
294-299: 🎯 Functional CorrectnessAssert the prompt path is skipped here.
deleteContractwon’t be reached in this branch anyway; the real gap is that a mockedprocess.exit(1)still lets execution continue intoreadline.createInterface/question. Assert that path is not hit, or add an earlyreturnafterprocess.exit(1).> Likely an incorrect or invalid review comment.src/commands/resume.ts (1)
19-25: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMissing
returnafterprocess.exit(1)lets resume proceed on a missing contract.
process.exit()schedules termination but does not stop synchronous execution of the current function. Without areturn,setContractActiveStatus(db, contractId, true)at line 24 still runs even whencontractisundefined, silently writing anactiverow update for a non-existent/unwatched contract. The same pattern exists in the siblingpause.tscommand shown in the review context.🐛 Proposed fix
if (!contract) { console.log(chalk.red(`Contract ${formatContractID(contractId)} is not being watched.`)); process.exit(1); + return; }> Likely an incorrect or invalid review comment.src/db/repositories.ts (2)
3-15: LGTM!
152-159: LGTM!src/core/monitor.ts (1)
88-89: LGTM!src/commands/pause.ts (1)
1-18: LGTM!Also applies to: 23-34
src/index.ts (1)
19-20: LGTM!Also applies to: 48-49
src/commands/watch.ts (1)
3-6: LGTM!Also applies to: 168-177, 182-187, 208-214
| } | ||
|
|
||
| function buildBlocks(event: AlertEvent): SlackBlock[] { | ||
| function buildBlocks(event: AlertEvent): any[] { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Block Kit payload types loosened to any.
buildBlocks now returns any[], and header/details/footer are all implicitly any. This drops the compile-time guarantees the previous SlackBlock typing provided, so a typo in a field name (e.g. mrkdwn vs mrkdown) or a structural mistake would silently pass type-checking.
Also applies to: 31-31, 40-87
🤖 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/alerts/slack.ts` at line 14, The Block Kit payload typing in buildBlocks
has been loosened to any[], which removes compile-time checks for header,
details, and footer. Restore the previous SlackBlock-based typing in buildBlocks
and explicitly type the local block variables so field names and structure are
validated by the compiler. Make sure the returned array is typed with the
existing SlackBlock union or equivalent block-specific types rather than any.
| export class SlackChannel { | ||
| constructor(private readonly webhookUrl: string) { | ||
| if (!webhookUrl || !webhookUrl.startsWith("http")) { | ||
| throw new Error("Invalid Slack webhook URL"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File map\n'
git ls-files | rg '^src/alerts/|^src/' | sed -n '1,200p'
printf '\n## Slack alert file outline\n'
ast-grep outline src/alerts/slack.ts --view expanded || true
printf '\n## Relevant usages of SlackChannel and webhook URL config\n'
rg -n "SlackChannel|webhookUrl|hooks\.slack\.com|slack" src test . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | sed -n '1,220p'Repository: AbdulmalikAlayande/sorokeep
Length of output: 45491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Show src/alerts/slack.ts with line numbers\n'
cat -n src/alerts/slack.ts | sed -n '1,220p'
printf '\n## Search for any URL validation helpers\n'
rg -n "new URL\(|startsWith\\(\"http\"\\)|Invalid Slack webhook URL|hooks\\.slack\\.com" src test . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | sed -n '1,200p'Repository: AbdulmalikAlayande/sorokeep
Length of output: 6617
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/alerts/slack.ts')
text = p.read_text()
print("Lines:", len(text.splitlines()))
print(text)
PYRepository: AbdulmalikAlayande/sorokeep
Length of output: 5114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/alerts/slack.ts ---'
nl -ba src/alerts/slack.ts | sed -n '1,180p'
echo
echo '--- direct references to SlackChannel and webhookUrl ---'
rg -n "new SlackChannel|SlackChannel|webhookUrl|Invalid Slack webhook URL|hooks\.slack\.com" src test . \
--glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | sed -n '1,240p'Repository: AbdulmalikAlayande/sorokeep
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for docs or config that constrain the expected webhook host.
rg -n "hooks\.slack\.com|hooks\.slack-gov\.com|Slack webhook|incoming webhook|webhook URL|SlackChannel" . \
--glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | sed -n '1,240p'Repository: AbdulmalikAlayande/sorokeep
Length of output: 6984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/alerts/dispatcher.ts ---'
python3 - <<'PY'
from pathlib import Path
p = Path('src/alerts/dispatcher.ts')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 1 <= i <= 180:
print(f"{i:4d}: {line}")
PY
echo
echo '--- src/alerts/webhook.ts ---'
python3 - <<'PY'
from pathlib import Path
p = Path('src/alerts/webhook.ts')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 1 <= i <= 220:
print(f"{i:4d}: {line}")
PY
echo
echo '--- relevant README sections around Slack/webhook ---'
python3 - <<'PY'
from pathlib import Path
p = Path('README.md')
lines = p.read_text().splitlines()
for start in (180, 420, 520):
print(f"\n## README lines {start}-{start+80}")
for i in range(start, min(start+80, len(lines))+1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: AbdulmalikAlayande/sorokeep
Length of output: 19361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests/alerts/slack.test.ts ---'
python3 - <<'PY'
from pathlib import Path
p = Path('tests/alerts/slack.test.ts')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 1 <= i <= 220:
print(f"{i:4d}: {line}")
PY
echo
echo '--- src/commands/alerts.ts relevant slack handling ---'
python3 - <<'PY'
from pathlib import Path
p = Path('src/commands/alerts.ts')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 1 <= i <= 130:
print(f"{i:4d}: {line}")
PY
echo
echo '--- tests/alerts/state_change_alerts.test.ts relevant slack bits ---'
python3 - <<'PY'
from pathlib import Path
p = Path('tests/alerts/state_change_alerts.test.ts')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 150 <= i <= 220:
print(f"{i:4d}: {line}")
PYRepository: AbdulmalikAlayande/sorokeep
Length of output: 15094
Slack alert targets are channel names, not webhook URLs. --type slack passes values like #alerts into SlackChannel, so this startsWith("http") check will reject valid configs at runtime. Either accept the actual Slack channel input here or move URL validation to the webhook transport.
🤖 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/alerts/slack.ts` around lines 89 - 94, The SlackChannel constructor is
validating the wrong kind of input: `SlackChannel` is being constructed with
channel names like `#alerts`, so the `startsWith("http")` webhook check rejects
valid Slack targets. Update `SlackChannel` to accept channel names as provided
by `--type slack`, and move any webhook URL validation to the webhook transport
or the code that actually handles webhooks, keeping the constructor aligned with
its real input.
| async send(event: AlertEvent): Promise<void> { | ||
| logger.debug(`Sending Slack alert to webhook`, { type: event.type, contractId: event.contractId }); | ||
|
|
||
| logger.debug(`Sending Slack alert to ${channel}`, { type: event.type, contractId: event.contractId }); | ||
|
|
||
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), TIMEOUT_MS); | ||
|
|
||
| let response: Response; | ||
| try { | ||
| response = await fetch(SLACK_API_URL, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| "Authorization": `Bearer ${token}`, | ||
| }, | ||
| body: JSON.stringify({ | ||
| channel, | ||
| text: buildFallbackText(event), | ||
| blocks: buildBlocks(event), | ||
| }), | ||
| signal: controller.signal, | ||
| }); | ||
| } finally { | ||
| clearTimeout(timeout); | ||
| } | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error( | ||
| `Slack API request failed: HTTP ${response.status}`, | ||
| ); | ||
| } | ||
| const payload = { | ||
| blocks: buildBlocks(event), | ||
| }; | ||
|
|
||
| // Slack always returns HTTP 200, but errors are in the body as ok: false | ||
| const body = await response.json() as { ok: boolean; error?: string }; | ||
| if (!body.ok) { | ||
| throw new Error( | ||
| `Slack API error: ${body.error ?? "unknown error"}`, | ||
| ); | ||
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), TIMEOUT_MS); | ||
|
|
||
| let response: Response; | ||
| try { | ||
| response = await fetch(this.webhookUrl, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(payload), | ||
| signal: controller.signal, | ||
| }); | ||
| } finally { | ||
| clearTimeout(timeout); | ||
| } | ||
|
|
||
| if (!response.ok) { | ||
| let errorMsg = `HTTP ${response.status}`; | ||
| try { | ||
| const text = await response.text(); | ||
| if (text) errorMsg += `: ${text}`; | ||
| } catch { | ||
| // ignore | ||
| } | ||
| throw new Error(errorMsg); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Timeout errors surface as generic abort errors.
On timeout, controller.abort() causes fetch to reject with an AbortError, which propagates without a clearer message (e.g., "Slack webhook timed out after 10000ms"), making delivery failures in dispatcher.ts's retry logs harder to diagnose.
🤖 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/alerts/slack.ts` around lines 96 - 127, The Slack alert send path in
send() currently lets AbortError bubble up on timeout, which makes retry logs in
dispatcher.ts hard to understand. Update the fetch timeout handling around
controller.abort()/fetch() so the timeout case is caught and rethrown with a
clear message like a webhook timeout, while preserving the existing non-OK HTTP
response handling and the logger.debug context.
| if (!contract) { | ||
| console.log(chalk.red(`Contract ${formatContractID(contractId)} is not being watched.`)); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing return after process.exit(1).
The sibling guard clause in src/commands/watch.ts (unchanged !contractId check) follows process.exit(1) with an explicit return;. This file omits it, so if process.exit is ever stubbed (e.g. in tests) execution falls through to setContractActiveStatus(db, contractId, false) for a contract that doesn't exist.
🔧 Proposed fix
if (!contract) {
console.log(chalk.red(`Contract ${formatContractID(contractId)} is not being watched.`));
process.exit(1);
+ return;
}📝 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.
| if (!contract) { | |
| console.log(chalk.red(`Contract ${formatContractID(contractId)} is not being watched.`)); | |
| process.exit(1); | |
| } | |
| if (!contract) { | |
| console.log(chalk.red(`Contract ${formatContractID(contractId)} is not being watched.`)); | |
| process.exit(1); | |
| return; | |
| } |
🤖 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/pause.ts` around lines 19 - 22, The guard clause in pause.ts for
the missing contract case should stop execution explicitly after
process.exit(1). Update the contract lookup block in the pause command so that
the !contract branch includes a return after logging and exiting, matching the
pattern used in watch.ts and preventing fallthrough into setContractActiveStatus
when process.exit is stubbed.
| if (!contract) { | ||
| console.log(chalk.red(`Contract ${formatContractID(contractId)} is not being watched.`)); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Same missing-return pattern as pause.ts.
process.exit(1) isn't followed by return, unlike the existing !contractId guard earlier in this same file (lines 85-93) which uses process.exit(1); return;.
🔧 Proposed fix
if (!contract) {
console.log(chalk.red(`Contract ${formatContractID(contractId)} is not being watched.`));
process.exit(1);
+ return;
}📝 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.
| if (!contract) { | |
| console.log(chalk.red(`Contract ${formatContractID(contractId)} is not being watched.`)); | |
| process.exit(1); | |
| } | |
| if (!contract) { | |
| console.log(chalk.red(`Contract ${formatContractID(contractId)} is not being watched.`)); | |
| process.exit(1); | |
| return; | |
| } |
🤖 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/watch.ts` around lines 178 - 181, The `watch.ts` guard that
checks `!contract` should follow the same early-exit pattern as the existing
`!contractId` branch and `pause.ts`: after calling `process.exit(1)` in the
`watch` command, add an immediate `return` so the function does not continue
past the failure path. Use the `watch` command’s `!contract` block and the
`process.exit(1)` call as the reference point.
| export function insertContract(db: Database.Database, contract: {id: string; name?: string; network: string; wasm_hash?: string; tags?: string; poll_interval_seconds?: number | null; active?: number}): void { | ||
| db.prepare(` | ||
| INSERT INTO contracts (id, name, network, wasm_hash, tags, poll_interval_seconds) | ||
| VALUES (@id, @name, @network, @wasm_hash, @tags, @poll_interval_seconds) | ||
| INSERT INTO contracts (id, name, network, wasm_hash, tags, poll_interval_seconds, active) | ||
| VALUES (@id, @name, @network, @wasm_hash, @tags, @poll_interval_seconds, @active) | ||
| ON CONFLICT(id) DO UPDATE SET | ||
| name = excluded.name, | ||
| network = excluded.network, | ||
| wasm_hash = excluded.wasm_hash, | ||
| tags = excluded.tags, | ||
| poll_interval_seconds = COALESCE(excluded.poll_interval_seconds, contracts.poll_interval_seconds) | ||
| poll_interval_seconds = COALESCE(excluded.poll_interval_seconds, contracts.poll_interval_seconds), | ||
| active = COALESCE(excluded.active, contracts.active) | ||
| `).run({ | ||
| id: contract.id, | ||
| name: contract.name ?? null, | ||
| network: contract.network, | ||
| wasm_hash: contract.wasm_hash ?? null, | ||
| tags: contract.tags ?? null, | ||
| poll_interval_seconds: contract.poll_interval_seconds ?? null, | ||
| active: contract.active ?? 1, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Upsert silently resets active on every re-insert, un-pausing monitoring.
active: contract.active ?? 1 (line 116) converts undefined to 1 before it ever reaches SQL, so @active is never NULL. That means active = COALESCE(excluded.active, contracts.active) (line 108) always evaluates to 1 whenever the caller omits active — the fallback-to-existing-value branch is unreachable.
src/core/watch.ts calls insertContract on every watch (including re-watching an already-registered contract) without an active field. Since insertContract uses ON CONFLICT(id) DO UPDATE, re-running watch on a contract that was previously paused will silently flip it back to active = 1, defeating the pause feature.
🐛 Proposed fix — push the default into SQL so COALESCE actually works
export function insertContract(db: Database.Database, contract: {id: string; name?: string; network: string; wasm_hash?: string; tags?: string; poll_interval_seconds?: number | null; active?: number}): void {
db.prepare(`
INSERT INTO contracts (id, name, network, wasm_hash, tags, poll_interval_seconds, active)
- VALUES (`@id`, `@name`, `@network`, `@wasm_hash`, `@tags`, `@poll_interval_seconds`, `@active`)
+ VALUES (`@id`, `@name`, `@network`, `@wasm_hash`, `@tags`, `@poll_interval_seconds`, COALESCE(`@active`, 1))
ON CONFLICT(id) DO UPDATE SET
name = excluded.name,
network = excluded.network,
wasm_hash = excluded.wasm_hash,
tags = excluded.tags,
poll_interval_seconds = COALESCE(excluded.poll_interval_seconds, contracts.poll_interval_seconds),
- active = COALESCE(excluded.active, contracts.active)
+ active = COALESCE(`@active`, contracts.active)
`).run({
id: contract.id,
name: contract.name ?? null,
network: contract.network,
wasm_hash: contract.wasm_hash ?? null,
tags: contract.tags ?? null,
poll_interval_seconds: contract.poll_interval_seconds ?? null,
- active: contract.active ?? 1,
+ active: contract.active ?? null,
});
}better-sqlite3 supports reusing the same named parameter (@active) multiple times in a statement, so this keeps the fresh-insert default (1) while genuinely preserving the existing active value on conflict.
📝 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.
| export function insertContract(db: Database.Database, contract: {id: string; name?: string; network: string; wasm_hash?: string; tags?: string; poll_interval_seconds?: number | null; active?: number}): void { | |
| db.prepare(` | |
| INSERT INTO contracts (id, name, network, wasm_hash, tags, poll_interval_seconds) | |
| VALUES (@id, @name, @network, @wasm_hash, @tags, @poll_interval_seconds) | |
| INSERT INTO contracts (id, name, network, wasm_hash, tags, poll_interval_seconds, active) | |
| VALUES (@id, @name, @network, @wasm_hash, @tags, @poll_interval_seconds, @active) | |
| ON CONFLICT(id) DO UPDATE SET | |
| name = excluded.name, | |
| network = excluded.network, | |
| wasm_hash = excluded.wasm_hash, | |
| tags = excluded.tags, | |
| poll_interval_seconds = COALESCE(excluded.poll_interval_seconds, contracts.poll_interval_seconds) | |
| poll_interval_seconds = COALESCE(excluded.poll_interval_seconds, contracts.poll_interval_seconds), | |
| active = COALESCE(excluded.active, contracts.active) | |
| `).run({ | |
| id: contract.id, | |
| name: contract.name ?? null, | |
| network: contract.network, | |
| wasm_hash: contract.wasm_hash ?? null, | |
| tags: contract.tags ?? null, | |
| poll_interval_seconds: contract.poll_interval_seconds ?? null, | |
| active: contract.active ?? 1, | |
| }); | |
| export function insertContract(db: Database.Database, contract: {id: string; name?: string; network: string; wasm_hash?: string; tags?: string; poll_interval_seconds?: number | null; active?: number}): void { | |
| db.prepare(` | |
| INSERT INTO contracts (id, name, network, wasm_hash, tags, poll_interval_seconds, active) | |
| VALUES (`@id`, `@name`, `@network`, `@wasm_hash`, `@tags`, `@poll_interval_seconds`, COALESCE(`@active`, 1)) | |
| ON CONFLICT(id) DO UPDATE SET | |
| name = excluded.name, | |
| network = excluded.network, | |
| wasm_hash = excluded.wasm_hash, | |
| tags = excluded.tags, | |
| poll_interval_seconds = COALESCE(excluded.poll_interval_seconds, contracts.poll_interval_seconds), | |
| active = COALESCE(`@active`, contracts.active) | |
| `).run({ | |
| id: contract.id, | |
| name: contract.name ?? null, | |
| network: contract.network, | |
| wasm_hash: contract.wasm_hash ?? null, | |
| tags: contract.tags ?? null, | |
| poll_interval_seconds: contract.poll_interval_seconds ?? null, | |
| active: contract.active ?? null, | |
| }); |
🤖 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/db/repositories.ts` around lines 98 - 117, The upsert in insertContract
is forcing active to 1 in JavaScript, so the ON CONFLICT path in the
insertContract repository method never preserves a paused contract. Move the
defaulting logic into the SQL statement so `@active` can remain NULL when omitted,
allowing COALESCE(excluded.active, contracts.active) to keep the existing value
on re-insert while still defaulting new rows to active. Use the insertContract
function and its ON CONFLICT(id) DO UPDATE clause to make the change.
| describe("Error handling", () => { | ||
| it("throws when API returns non-2xx", async () => { | ||
| mockFetch.mockResolvedValue(makeSlackErrorResponse("invalid_payload")); | ||
| await expect(channel.send(makeAlertEvent())).rejects.toThrow("invalid_payload"); | ||
| }); | ||
|
|
||
| it("throws when fetch itself rejects (network error)", async () => { | ||
| mockFetch.mockRejectedValue(new Error("ECONNREFUSED")); | ||
| await expect(channel.send(makeAlertEvent())).rejects.toThrow("ECONNREFUSED"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
No coverage for the timeout/abort path.
Consider a test using fake timers to assert send rejects once TIMEOUT_MS elapses, given AbortController-based timeout is a key reliability feature of this class.
🤖 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/alerts/slack.test.ts` around lines 104 - 114, Add a test for the
timeout/abort path in the Slack alert sender so `channel.send` is covered when
`TIMEOUT_MS` elapses. In `tests/alerts/slack.test.ts`, extend the existing
`Error handling` suite around `channel.send` by using fake timers and a fetch
mock that never resolves, then advance time past `TIMEOUT_MS` and assert the
promise rejects with the abort/timeout error. Make sure the test targets the
`send` method and its `AbortController` timeout behavior.
| expect(SlackChannel).toHaveBeenCalledWith("#alerts"); | ||
| expect(mockSlackSend).toHaveBeenCalledWith( |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any migration or validation of pre-existing slack channel_target values
rg -nP -C3 'channel_target' --type=ts
rg -nP -C3 'channelTarget' --type=ts
fd -e sql | xargs rg -n 'channel_target'Repository: AbdulmalikAlayande/sorokeep
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
git ls-files | rg '^(tests/alerts/state_change_alerts\.test\.ts|src/alerts/slack\.ts|src/alerts/|migrations/|.*\.sql|.*schema.*)$'
# Show compact structure for the likely implementation file(s)
for f in src/alerts/slack.ts tests/alerts/state_change_alerts.test.ts; do
if [ -f "$f" ]; then
echo "===== $f ====="
ast-grep outline "$f" --view expanded
fi
done
# Search for any validation or migration logic around Slack targets
rg -n -C 3 'channel_target|channelTarget|Slack webhook URL|startsWith\("http"\)|startsWith\(' src tests migrations . -g '!node_modules' -g '!dist' -g '!build'
# Read the relevant source slices with line numbers
for f in src/alerts/slack.ts tests/alerts/state_change_alerts.test.ts; do
if [ -f "$f" ]; then
echo "===== $f (numbered) ====="
wc -l "$f"
fi
doneRepository: AbdulmalikAlayande/sorokeep
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact relevant slices
sed -n '180,230p' tests/alerts/state_change_alerts.test.ts
printf '\n===== src/alerts/slack.ts =====\n'
sed -n '80,115p' src/alerts/slack.ts
printf '\n===== src/alerts/dispatcher.ts =====\n'
sed -n '35,120p' src/alerts/dispatcher.ts
# Check whether this test file is the only one using "`#alerts`" for Slack delivery
rg -n '"`#alerts`"|"`#my-alerts`"|"`#ops`"|SlackChannel\(' tests src/alerts -g '!node_modules' -g '!dist' -g '!build'Repository: AbdulmalikAlayande/sorokeep
Length of output: 7750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '===== src/commands/alerts.ts (relevant slices) =====\n'
sed -n '1,220p' src/commands/alerts.ts
printf '\n===== tests/commands/alerts.test.ts (slack-related slices) =====\n'
sed -n '90,120p' tests/commands/alerts.test.ts
sed -n '195,220p' tests/commands/alerts.test.ts
sed -n '490,520p' tests/commands/alerts.test.ts
sed -n '550,570p' tests/commands/alerts.test.ts
sed -n '730,765p' tests/commands/alerts.test.ts
printf '\n===== docs / README mentions of slack target =====\n'
rg -n -C 2 'slack|webhook|channel_target|`#alerts`|`#ops`' README.md docs src tests -g '!node_modules' -g '!dist' -g '!build'Repository: AbdulmalikAlayande/sorokeep
Length of output: 50386
Slack alerts still use channel names, not webhook URLs. In src/commands/alerts.ts, --channel is stored as-is for slack, and existing configs/fixtures use values like #alerts, so new SlackChannel(channelTarget) in src/alerts/slack.ts will reject real Slack configs. Align the delivery contract or add a migration for stored Slack targets.
🤖 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/alerts/state_change_alerts.test.ts` around lines 208 - 209, The Slack
alert flow is still passing channel names through as webhook targets, so
`SlackChannel` rejects valid existing Slack configs. Update the contract between
`src/commands/alerts.ts` and `src/alerts/slack.ts` so `--channel` can remain a
channel name like `#alerts`, or add a migration/translation step before `new
SlackChannel(...)` is created. Make sure the affected alert creation and send
path use the same target format consistently across stored configs, fixtures,
and `SlackChannel`.
| }); | ||
|
|
||
| it("falls back to config values when interval and network are not provided", async () => { | ||
| vi.spyOn(configLib, "loadConfig").mockReturnValue({ | ||
| network: "futurenet", | ||
| pollingIntervalSeconds: 45 | ||
| } as any); | ||
|
|
||
| await actionFn({ logFormat: "pretty" }); | ||
|
|
||
| expect(daemonLib.startDaemon).toHaveBeenCalledWith( | ||
| expect.anything(), | ||
| "futurenet", | ||
| expect.objectContaining({ | ||
| intervalMs: 45000 | ||
| }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Missing coverage for the display-string bug (options.network undefined).
This test correctly verifies startDaemon receives the config-derived network/interval, but nothing asserts the console output when network is omitted. Adding an assertion on the printed "Network:" line would have caught the options.network vs network display bug flagged in src/commands/daemon.ts.
🤖 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/daemon.test.ts` around lines 98 - 115, Add test coverage in
daemon.test.ts for the CLI output when the network option is omitted, since the
current assertion only checks startDaemon arguments. Use actionFn and the daemon
command’s printed status lines to verify the console includes the config-derived
network value in the “Network:” display, which will catch the options.network
versus network bug in src/commands/daemon.ts.
| it("pause exits with 1 if contract is not found", async () => { | ||
| vi.mocked(repositories.getContract).mockReturnValue(undefined); | ||
| await pauseActionFn("CDEF1234"); | ||
| expect(mockExit).toHaveBeenCalledWith(1); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing-contract tests don't assert the status setter was skipped.
These tests only check mockExit was called with 1, not that repositories.setContractActiveStatus was never invoked. Since process.exit() doesn't halt synchronous execution and pause.ts/resume.ts are missing a return after process.exit(1) (see companion source review), the status-toggle call actually still fires in this scenario — but these tests can't catch it.
✅ Suggested strengthened assertions
it("pause exits with 1 if contract is not found", async () => {
vi.mocked(repositories.getContract).mockReturnValue(undefined);
await pauseActionFn("CDEF1234");
expect(mockExit).toHaveBeenCalledWith(1);
+ expect(repositories.setContractActiveStatus).not.toHaveBeenCalled();
});Also applies to: 55-59
🤖 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/pause.test.ts` around lines 42 - 46, The missing-contract
pause/resume tests only verify that mockExit was called, but they do not ensure
repositories.setContractActiveStatus is skipped when repositories.getContract
returns undefined. Update the pauseActionFn and resumeActionFn missing-contract
cases in the pause/resume tests to also assert that the status setter was never
called, so the tests catch the missing return after process.exit(1) in the
corresponding actions.
2 participants
Overview
This PR implements multiple API enhancements, security validations, and compliance improvements across the application as per the required acceptance criteria.
Changes Included:
Access-Control-Allow-Origin: *header in the/.well-known/stellar.tomlcontroller to ensure wallets across all domains can seamlessly read federation settings.app.all('/register', ...)handler directly beneath thePOST /registerroute that returns a405 Method Not Allowedwhen clients send arbitrary incorrect HTTP verbs to the endpoint.POST /registerrequests against a static array of reserved system keywords, immediately returning a403 Forbiddenif a match is found to protect platform integrity.connect-timeoutto abort requests exceeding a 10-second lifetime, returning a503 Service UnavailableJSON response to prevent connection hangs and memory leaks.Related Issues