Cli fix - #240
Merged
AbdulmalikAlayande merged 3 commits intoJun 25, 2026
Merged
Cli fix#240
Conversation
Add --cpu-limit/--mem-limit options, resource alert detection, quoting for SQL 'limit' column, improved deduplication allowing new alerts on increased usage, and adjust percent rounding to floor. All tests updated and passing.
|
@nextgenuniversity1-oss 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 CPU/memory resource alerting end-to-end: two new DB tables, repository CRUD/query/delivery functions, alert event types ( ChangesResource Alerting & Resources CLI
Sequence Diagram(s)sequenceDiagram
participant MonitorDaemon
participant checkResourceLimitsAndAlert
participant DB
participant dispatchResourceAlert
participant SlackOrWebhook
MonitorDaemon->>checkResourceLimitsAndAlert: contractId, cpuInstructions, memoryBytes
checkResourceLimitsAndAlert->>DB: getResourceAlertConfigsForContract(contractId)
checkResourceLimitsAndAlert->>DB: getContractById(contractId)
loop per config × per resource (cpu, memory)
checkResourceLimitsAndAlert->>DB: hasUnresolvedResourceAlert(configId, resourceType, usagePercent)
alt usagePercent >= 80% and no duplicate
checkResourceLimitsAndAlert->>DB: recordResourceAlertFired(...)
checkResourceLimitsAndAlert->>dispatchResourceAlert: ResourceAlertEvent
dispatchResourceAlert->>SlackOrWebhook: sendAlert(event)
alt delivered
dispatchResourceAlert->>DB: markResourceAlertDelivered(alertFiredId)
else failed
dispatchResourceAlert->>DB: incrementResourceAlertRetryCount(alertFiredId)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 10
🤖 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 10-12: The alerts CLI is only wired to the TTL alert-config
repository, so resource alert configs added via insertResourceAlertConfig are
not visible or manageable through the same command surface. Update the alerts
commands in alerts.ts, especially alerts list, alerts remove, and alerts test,
to query and operate on both getResourceAlertConfigsForContract and the existing
TTL repository, and route removals/tests through the correct resource-aware
paths so resource configs can be listed, tested, and deleted.
- Around line 34-36: The alert limit parsing in the options for alerts.ts is too
permissive, so invalid or truncated values can slip through. Update the parsing
and validation around the alert limit options and the related validation branch
in the alerts command to use strict numeric parsing for `--cpu-limit`,
`--mem-limit`, and the other limit options, and reject any non-integer, partial,
NaN, zero, or negative values before persisting. Keep the fix localized to the
option handlers and the validation logic in the alerts command so the checks are
enforced consistently.
In `@src/commands/resources.ts`:
- Around line 60-61: The resource summary in the reduce and render path is
keeping `minPercent` and `maxPercent` without using them, while the table only
prints `Avg %` plus min/max usage values from the `resources.ts` command flow.
Update the `resources` aggregation/rendering logic so `minPercent`/`maxPercent`
are either removed from the accumulator or actually surfaced in the output, and
make the `Min`/`Max` column labels match the values they display to avoid
confusion.
- Line 40: The usage trend data in getResourceUsageHistory is biased because it
is sourced only from resource_alerts_fired, which captures threshold breaches
rather than continuous consumption. Update the resource usage history flow in
getResourceUsageHistory (and the call site in resources.ts if needed) to query a
continuous usage source instead of only fired alerts, and adjust any downstream
aggregation so averages/minimums reflect full historical load rather than
alert-only events.
In `@src/db/repositories.ts`:
- Around line 601-609: The dedupe check in the repository method that reads from
resource_alerts_fired is using the most recent unresolved row via ORDER BY
fired_at DESC LIMIT 1, which can miss a higher unresolved usage_percent when
multiple alerts share the same timestamp bucket. Update this query to compare
against the maximum unresolved usage_percent for the given
resource_alert_config_id and resource_type, and keep the existing return-boolean
logic in the same helper so the dedupe decision is based on the highest
outstanding alert.
In `@src/db/schema.sql`:
- Around line 78-91: The resource alert schema currently allows invalid zero or
negative limits, which can break percentage calculations later. Update the
CREATE TABLE definitions for resource_alert_configs and resource_alerts_fired in
schema.sql to add CHECK constraints on cpu_limit, mem_limit, and the fired
"limit" column so they must be greater than zero. Make sure the constraints are
applied at the database boundary so inserts through repositories or manual SQL
cannot persist invalid alert config or history rows.
In `@stdout`:
- Around line 53-78: The tracked stdout artifact should be removed from version
control and kept out of the repo going forward; delete it from the index so the
existing .gitignore can prevent re-tracking. Use the stdout artifact as the
target to untrack, and then verify the logging setup around MonitorCycle in
tests/core/monitor.test.ts to eliminate the duplicated component field caused by
binding component in both the root logger and a child logger.
In `@tests/alerts/resource.test.ts`:
- Around line 506-522: The warning-severity test in resource.test.ts is too weak
because it only checks severity when mockSendWebhookAlert was called, so missing
alerts can pass unnoticed. In the loop around checkResourceLimitsAndAlert and
mockSendWebhookAlert, assert that exactly one webhook alert was sent before
inspecting the event, then verify the severity value directly for the 80–94.9%
cases to match the critical-severity pattern used elsewhere in the test.
In `@tests/commands/resources.test.ts`:
- Around line 92-118: The test for registerResourcesCommand only checks that the
Resource Usage Trends header is printed, so it does not prove that --period
filtering works. Update the resources command test to include an additional
older history record outside the requested window using
recordResourceAlertFired, then assert that the output excludes that record while
still showing the in-range one. Keep the focus on the command flow in
registerResourcesCommand and the fired_at_ledger-based history data so the test
genuinely verifies --period behavior.
- Around line 52-56: The test setup is accessing `.id` on the result of
`mockDb.prepare(...).get()` without a type assertion, which fails under strict
typing. In `resources.test.ts`, update the `config` lookup used by
`recordResourceAlertFired` (and the similar lookup near the other referenced
spot) to cast the `get()` result as `{ id: number }`, matching the pattern
already used elsewhere in the suite.
🪄 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: 5291a1db-e715-4a63-9859-d6f6a3a59b98
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
src/alerts/resource.tssrc/alerts/slack.tssrc/alerts/types.tssrc/commands/alerts.tssrc/commands/resources.tssrc/db/repositories.tssrc/db/schema.sqlsrc/index.tsstdouttests/alerts/resource.test.tstests/commands/resources.test.ts
📜 Review details
🔇 Additional comments (1)
src/index.ts (1)
10-10: LGTM!Also applies to: 28-28
Comment on lines
+10
to
+12
| insertResourceAlertConfig, | ||
| getResourceAlertConfigsForContract, | ||
| deleteResourceAlertConfig, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Wire resource configs into the rest of the alerts management CLI.
This branch inserts resource_alert_configs, but alerts list, alerts remove, and alerts test still use only the TTL alert-config repository. A resource alert added here will be invisible to list/test and cannot be removed through this command surface.
Also applies to: 121-128
🤖 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 10 - 12, The alerts CLI is only wired to
the TTL alert-config repository, so resource alert configs added via
insertResourceAlertConfig are not visible or manageable through the same command
surface. Update the alerts commands in alerts.ts, especially alerts list, alerts
remove, and alerts test, to query and operate on both
getResourceAlertConfigsForContract and the existing TTL repository, and route
removals/tests through the correct resource-aware paths so resource configs can
be listed, tested, and deleted.
Comment on lines
+34
to
+36
| .option("--threshold <ledgers>", "Threshold in number of ledgers (for TTL-based alerts)", (val) => parseInt(val, 10)) | ||
| .option("--cpu-limit <instructions>", "CPU instruction limit for resource alerts (default: 100,000,000)", (val) => parseInt(val, 10)) | ||
| .option("--mem-limit <bytes>", "Memory byte limit for resource alerts (default: 50,000,000)", (val) => parseInt(val, 10)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject invalid or truncated resource limits before persisting.
parseInt accepts partial values like 1e8 as 1, and NaN <= 0 is false, so invalid --cpu-limit/--mem-limit input can pass this branch. Use strict numeric parsing and validate safe positive integers for all alert limits.
Proposed validation tightening
- .option("--threshold <ledgers>", "Threshold in number of ledgers (for TTL-based alerts)", (val) => parseInt(val, 10))
- .option("--cpu-limit <instructions>", "CPU instruction limit for resource alerts (default: 100,000,000)", (val) => parseInt(val, 10))
- .option("--mem-limit <bytes>", "Memory byte limit for resource alerts (default: 50,000,000)", (val) => parseInt(val, 10))
+ .option("--threshold <ledgers>", "Threshold in number of ledgers (for TTL-based alerts)", Number)
+ .option("--cpu-limit <instructions>", "CPU instruction limit for resource alerts (default: 100,000,000)", Number)
+ .option("--mem-limit <bytes>", "Memory byte limit for resource alerts (default: 50,000,000)", Number)
@@
- if (cpuLimit <= 0 || memLimit <= 0) {
+ if (
+ !Number.isSafeInteger(cpuLimit) ||
+ !Number.isSafeInteger(memLimit) ||
+ cpuLimit <= 0 ||
+ memLimit <= 0
+ ) {Also applies to: 113-118
🤖 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 34 - 36, The alert limit parsing in the
options for alerts.ts is too permissive, so invalid or truncated values can slip
through. Update the parsing and validation around the alert limit options and
the related validation branch in the alerts command to use strict numeric
parsing for `--cpu-limit`, `--mem-limit`, and the other limit options, and
reject any non-integer, partial, NaN, zero, or negative values before
persisting. Keep the fix localized to the option handlers and the validation
logic in the alerts command so the checks are enforced consistently.
| process.exit(1); | ||
| } | ||
|
|
||
| const history = getResourceUsageHistory(db, contractId, days); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any full resource-usage table/source beyond fired alerts.
rg -nP --type=ts -C2 '\b(getResourceUsageHistory|resource_alerts_fired|resource_usage|usage_history)\b' src
fd -e sql | xargs rg -n 'CREATE TABLE' 2>/dev/nullRepository: AbdulmalikAlayande/sorokeep
Length of output: 4749
Data bias: "Usage trends" computed exclusively from fired alerts.
The getResourceUsageHistory function in src/db/repositories.ts:490-515 queries only the resource_alerts_fired table. This table records instances solely when usage crossed a threshold (≈80%), omitting all periods of low-to-moderate consumption.
Aggregating this sparse data results in statistically misleading metrics: reported averages overstate typical usage, and minimum values never reflect baseline load. A continuous usage table is required to accurately show historical consumption trends.
Current Data Source (src/db/repositories.ts)
SELECT ...
FROM resource_alerts_fired raf
...🤖 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/resources.ts` at line 40, The usage trend data in
getResourceUsageHistory is biased because it is sourced only from
resource_alerts_fired, which captures threshold breaches rather than continuous
consumption. Update the resource usage history flow in getResourceUsageHistory
(and the call site in resources.ts if needed) to query a continuous usage source
instead of only fired alerts, and adjust any downstream aggregation so
averages/minimums reflect full historical load rather than alert-only events.
Comment on lines
+60
to
+61
| minPercent: Number.POSITIVE_INFINITY, | ||
| maxPercent: Number.NEGATIVE_INFINITY, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Dead fields: minPercent/maxPercent are accumulated but never rendered.
The reduce maintains minPercent/maxPercent (and totalPercent is used for avg), but only Avg % is printed. Either drop these fields or surface them. Note also the Min/Max columns render usage values while there is no min/max percentage column, which is easy to misread.
Also applies to: 68-70
🤖 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/resources.ts` around lines 60 - 61, The resource summary in the
reduce and render path is keeping `minPercent` and `maxPercent` without using
them, while the table only prints `Avg %` plus min/max usage values from the
`resources.ts` command flow. Update the `resources` aggregation/rendering logic
so `minPercent`/`maxPercent` are either removed from the accumulator or actually
surfaced in the output, and make the `Min`/`Max` column labels match the values
they display to avoid confusion.
Comment on lines
+601
to
+609
| const row = db.prepare(` | ||
| SELECT usage_percent FROM resource_alerts_fired | ||
| WHERE resource_alert_config_id = ? AND resource_type = ? AND resolved = 0 | ||
| ORDER BY fired_at DESC | ||
| LIMIT 1 | ||
| `).get(configId, resourceType) as { usage_percent?: number } | undefined; | ||
|
|
||
| if (!row || typeof row.usage_percent === "undefined") return false; | ||
| return row.usage_percent >= currentUsagePercent; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare against the highest unresolved usage, not an arbitrary latest row.
ORDER BY fired_at DESC LIMIT 1 is nondeterministic when alerts fire in the same timestamp bucket, so dedupe can read a lower unresolved percent and emit another alert even though a higher unresolved alert exists. Query the max unresolved usage_percent instead.
Proposed dedupe query fix
- const row = db.prepare(`
- SELECT usage_percent FROM resource_alerts_fired
+ const row = db.prepare(`
+ SELECT MAX(usage_percent) AS usage_percent FROM resource_alerts_fired
WHERE resource_alert_config_id = ? AND resource_type = ? AND resolved = 0
- ORDER BY fired_at DESC
- LIMIT 1
- `).get(configId, resourceType) as { usage_percent?: number } | undefined;
+ `).get(configId, resourceType) as { usage_percent: number | null } | undefined;
- if (!row || typeof row.usage_percent === "undefined") return false;
+ if (!row || typeof row.usage_percent !== "number") return false;
return row.usage_percent >= currentUsagePercent;📝 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.
Suggested change
| const row = db.prepare(` | |
| SELECT usage_percent FROM resource_alerts_fired | |
| WHERE resource_alert_config_id = ? AND resource_type = ? AND resolved = 0 | |
| ORDER BY fired_at DESC | |
| LIMIT 1 | |
| `).get(configId, resourceType) as { usage_percent?: number } | undefined; | |
| if (!row || typeof row.usage_percent === "undefined") return false; | |
| return row.usage_percent >= currentUsagePercent; | |
| const row = db.prepare(` | |
| SELECT MAX(usage_percent) AS usage_percent FROM resource_alerts_fired | |
| WHERE resource_alert_config_id = ? AND resource_type = ? AND resolved = 0 | |
| `).get(configId, resourceType) as { usage_percent: number | null } | undefined; | |
| if (!row || typeof row.usage_percent !== "number") return false; | |
| return row.usage_percent >= currentUsagePercent; |
🤖 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 601 - 609, The dedupe check in the
repository method that reads from resource_alerts_fired is using the most recent
unresolved row via ORDER BY fired_at DESC LIMIT 1, which can miss a higher
unresolved usage_percent when multiple alerts share the same timestamp bucket.
Update this query to compare against the maximum unresolved usage_percent for
the given resource_alert_config_id and resource_type, and keep the existing
return-boolean logic in the same helper so the dedupe decision is based on the
highest outstanding alert.
Comment on lines
+78
to
+91
| cpu_limit INTEGER NOT NULL, | ||
| mem_limit INTEGER NOT NULL, | ||
| webhook_secret TEXT, | ||
| created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| UNIQUE(contract_id, channel_type, channel_target) | ||
| ); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS resource_alerts_fired ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| resource_alert_config_id INTEGER NOT NULL REFERENCES resource_alert_configs(id) ON DELETE CASCADE, | ||
| resource_type TEXT NOT NULL CHECK(resource_type IN ('cpu', 'memory')), | ||
| usage INTEGER NOT NULL, | ||
| "limit" INTEGER NOT NULL, | ||
| usage_percent INTEGER NOT NULL, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce resource metric invariants at the database boundary.
cpu_limit, mem_limit, and fired "limit" currently allow 0/negative values, but dispatch computes percentages by dividing by the configured limit. Add CHECK constraints so repository/manual inserts cannot persist invalid alert configs or history rows.
Proposed schema hardening
- cpu_limit INTEGER NOT NULL,
- mem_limit INTEGER NOT NULL,
+ cpu_limit INTEGER NOT NULL CHECK(cpu_limit > 0),
+ mem_limit INTEGER NOT NULL CHECK(mem_limit > 0),
@@
- usage INTEGER NOT NULL,
- "limit" INTEGER NOT NULL,
- usage_percent INTEGER NOT NULL,
+ usage INTEGER NOT NULL CHECK(usage >= 0),
+ "limit" INTEGER NOT NULL CHECK("limit" > 0),
+ usage_percent INTEGER NOT NULL CHECK(usage_percent >= 0),📝 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.
Suggested change
| cpu_limit INTEGER NOT NULL, | |
| mem_limit INTEGER NOT NULL, | |
| webhook_secret TEXT, | |
| created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| UNIQUE(contract_id, channel_type, channel_target) | |
| ); | |
| CREATE TABLE IF NOT EXISTS resource_alerts_fired ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| resource_alert_config_id INTEGER NOT NULL REFERENCES resource_alert_configs(id) ON DELETE CASCADE, | |
| resource_type TEXT NOT NULL CHECK(resource_type IN ('cpu', 'memory')), | |
| usage INTEGER NOT NULL, | |
| "limit" INTEGER NOT NULL, | |
| usage_percent INTEGER NOT NULL, | |
| cpu_limit INTEGER NOT NULL CHECK(cpu_limit > 0), | |
| mem_limit INTEGER NOT NULL CHECK(mem_limit > 0), | |
| webhook_secret TEXT, | |
| created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| UNIQUE(contract_id, channel_type, channel_target) | |
| ); | |
| CREATE TABLE IF NOT EXISTS resource_alerts_fired ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| resource_alert_config_id INTEGER NOT NULL REFERENCES resource_alert_configs(id) ON DELETE CASCADE, | |
| resource_type TEXT NOT NULL CHECK(resource_type IN ('cpu', 'memory')), | |
| usage INTEGER NOT NULL CHECK(usage >= 0), | |
| "limit" INTEGER NOT NULL CHECK("limit" > 0), | |
| usage_percent INTEGER NOT NULL CHECK(usage_percent >= 0), |
🤖 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/schema.sql` around lines 78 - 91, The resource alert schema currently
allows invalid zero or negative limits, which can break percentage calculations
later. Update the CREATE TABLE definitions for resource_alert_configs and
resource_alerts_fired in schema.sql to add CHECK constraints on cpu_limit,
mem_limit, and the fired "limit" column so they must be greater than zero. Make
sure the constraints are applied at the database boundary so inserts through
repositories or manual SQL cannot persist invalid alert config or history rows.
Comment on lines
+53
to
+78
| {"level":40,"time":1782369448298,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_ALERT, entry: alert-key, remainingTTL: 8000, threshold: 15000"} | ||
| {"level":40,"time":1782369448302,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_NEAR, entry: near-key, remainingTTL: 9999, threshold: 10000"} | ||
| {"level":40,"time":1782369448303,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_MULTI_ALERT, entry: ma-key, remainingTTL: 8000, threshold: 15000"} | ||
| {"level":40,"time":1782369448303,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_MULTI_ALERT, entry: ma-key, remainingTTL: 8000, threshold: 12000"} | ||
| {"level":40,"time":1782369448304,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_ENTRIES_ALERT, entry: e-instance, remainingTTL: 5000, threshold: 10000"} | ||
| {"level":40,"time":1782369448304,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_ENTRIES_ALERT, entry: e-wasm, remainingTTL: 5000, threshold: 10000"} | ||
| {"level":40,"time":1782369448306,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_EXPIRED, entry: exp-key, remainingTTL: -1000, threshold: 10000"} | ||
| {"level":40,"time":1782369448307,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_DEDUP, entry: dedup-key, remainingTTL: 5000, threshold: 10000"} | ||
| {"level":40,"time":1782369448308,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_3CYCLE, entry: c3-key, remainingTTL: 3000, threshold: 10000"} | ||
| {"level":40,"time":1782369448310,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_REFIRE, entry: rf-key, remainingTTL: 5000, threshold: 10000"} | ||
| {"level":40,"time":1782369448310,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_REFIRE, entry: rf-key, remainingTTL: 5000, threshold: 10000"} | ||
| {"level":40,"time":1782369448311,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_RESOLVE, entry: resolve-key, remainingTTL: 5000, threshold: 10000"} | ||
| {"level":30,"time":1782369448312,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Alert resolved — contract: CONTRACT_RESOLVE, entry: resolve-key, remainingTTL: 120000, threshold: 10000"} | ||
| {"level":40,"time":1782369448351,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_PARTIAL_RECOVER, entry: pr-key, remainingTTL: 5000, threshold: 20000"} | ||
| {"level":40,"time":1782369448352,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_SELECTIVE_RESOLVE, entry: sr-instance, remainingTTL: 5000, threshold: 10000"} | ||
| {"level":40,"time":1782369448352,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_SELECTIVE_RESOLVE, entry: sr-wasm, remainingTTL: 5000, threshold: 10000"} | ||
| {"level":30,"time":1782369448352,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Alert resolved — contract: CONTRACT_SELECTIVE_RESOLVE, entry: sr-instance, remainingTTL: 100000, threshold: 10000"} | ||
| {"level":50,"time":1782369448362,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","meta":[{}],"msg":"Error processing contract CONTRACT_FAIL: RPC timeout Error: RPC timeout\n at /workspaces/sorokeep/tests/core/monitor.test.ts:598:40\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:302:11\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:1903:26\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2326:20\n at new Promise (<anonymous>)\n at runWithCancel (file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2323:10)\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2305:20\n at new Promise (<anonymous>)\n at runWithTimeout (file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2272:10)\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2955:64"} | ||
| {"level":50,"time":1782369448366,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","meta":[{}],"msg":"Error processing contract CONTRACT_ERR_ID: Connection refused Error: Connection refused\n at /workspaces/sorokeep/tests/core/monitor.test.ts:612:48\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:302:11\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:1903:26\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2326:20\n at new Promise (<anonymous>)\n at runWithCancel (file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2323:10)\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2305:20\n at new Promise (<anonymous>)\n at runWithTimeout (file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2272:10)\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2955:64"} | ||
| {"level":50,"time":1782369448368,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","meta":[{}],"msg":"Error processing contract C_FAIL_1: Network down Error: Network down\n at /workspaces/sorokeep/tests/core/monitor.test.ts:624:48\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:302:11\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:1903:26\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2326:20\n at new Promise (<anonymous>)\n at runWithCancel (file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2323:10)\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2305:20\n at new Promise (<anonymous>)\n at runWithTimeout (file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2272:10)\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2955:64"} | ||
| {"level":50,"time":1782369448368,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","meta":[{}],"msg":"Error processing contract C_FAIL_2: Network down Error: Network down\n at /workspaces/sorokeep/tests/core/monitor.test.ts:624:48\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:302:11\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:1903:26\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2326:20\n at new Promise (<anonymous>)\n at runWithCancel (file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2323:10)\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2305:20\n at new Promise (<anonymous>)\n at runWithTimeout (file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2272:10)\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2955:64"} | ||
| {"level":50,"time":1782369448370,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","meta":[{}],"msg":"Error processing contract CONTRACT_NO_UPDATE: Timeout Error: Timeout\n at /workspaces/sorokeep/tests/core/monitor.test.ts:639:48\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:302:11\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:1903:26\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2326:20\n at new Promise (<anonymous>)\n at runWithCancel (file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2323:10)\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2305:20\n at new Promise (<anonymous>)\n at runWithTimeout (file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2272:10)\n at file:///workspaces/sorokeep/node_modules/@vitest/runner/dist/chunk-artifact.js:2955:64"} | ||
| {"level":40,"time":1782369448372,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_TIERED, entry: tier-key, remainingTTL: 18000, threshold: 20000"} | ||
| {"level":40,"time":1782369448373,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_ESCALATE, entry: esc-key, remainingTTL: 18000, threshold: 20000"} | ||
| {"level":40,"time":1782369448373,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: CONTRACT_ESCALATE, entry: esc-key, remainingTTL: 3000, threshold: 5000"} | ||
| {"level":40,"time":1782369448374,"pid":14634,"hostname":"codespaces-d2f649","component":"MonitorCycle","component":"MonitorCycle","msg":"Threshold crossed — contract: C_CRITICAL, entry: crit-key, remainingTTL: 3000, threshold: 10000"} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect logger child setup for the duplicated "component" field.
rg -nP --type=ts -C3 "\bchild\s*\(|component" src/logging
fd -t f '.gitignore' -x rg -n 'stdout' {} 2>/dev/nullRepository: AbdulmalikAlayande/sorokeep
Length of output: 880
🏁 Script executed:
cat .gitignore
git ls-files stdoutRepository: AbdulmalikAlayande/sorokeep
Length of output: 500
🏁 Script executed:
git status
git status stdoutRepository: AbdulmalikAlayande/sorokeep
Length of output: 297
🏁 Script executed:
# Check if stdout is tracked in the current HEAD
git ls-files --error-unmatch stdout 2>&1 || echo "File not tracked"
# Check if file exists in filesystem
ls -la stdout 2>&1 || echo "File not found in fs"
# Try to look at the file content in HEAD if tracked
if git show HEAD:stdout | head -5; then echo "File is tracked and has content"; fiRepository: AbdulmalikAlayande/sorokeep
Length of output: 1439
🏁 Script executed:
# Inspect logger.ts usage to confirm duplicate binding source
rg -n "component" src/logging/logger.ts -B5 -A5Repository: AbdulmalikAlayande/sorokeep
Length of output: 165
Remove the stdout log file from the repository immediately.
The file stdout is a raw artifact containing machine-specific PII (PIDs, hostnames, local absolute paths) and raw pino lines. It adds significant noise and churn to the repo. Since it is already in .gitignore but still tracked by git, you must remove it from the index using git rm --cached stdout to stop versioning it. No new local files named stdout will be tracked moving forward.
Additionally, verify logger usage (e.g., in tests/core/monitor.test.ts) as individual log lines exhibit a duplicate "component" key ("component":"MonitorCycle","component":"MonitorCycle"), likely caused by passing component to both the root logger and a child logger binding.
🤖 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 `@stdout` around lines 53 - 78, The tracked stdout artifact should be removed
from version control and kept out of the repo going forward; delete it from the
index so the existing .gitignore can prevent re-tracking. Use the stdout
artifact as the target to untrack, and then verify the logging setup around
MonitorCycle in tests/core/monitor.test.ts to eliminate the duplicated component
field caused by binding component in both the root logger and a child logger.
Comment on lines
+506
to
+522
| for (const percent of [80, 85, 90, 94.9]) { | ||
| mockSendWebhookAlert.mockClear(); | ||
| const resourceData = { | ||
| cpuInstructions: Math.floor(100_000_000 * (percent / 100)), | ||
| memoryBytes: 25_000_000, | ||
| }; | ||
|
|
||
| checkResourceLimitsAndAlert(db, "CTEST1234", resourceData); | ||
|
|
||
| if (mockSendWebhookAlert.mock.calls.length > 0) { | ||
| const [, event] = mockSendWebhookAlert.mock.calls[0]!; | ||
| if (percent >= 80) { | ||
| expect(event.severity).toBe(percent >= 95 ? "critical" : "warning"); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Conditional assertions weaken the warning-severity test.
Because the assertions are guarded by if (mockSendWebhookAlert.mock.calls.length > 0), a regression where no alert fires for 80–94.9% would silently pass. Assert that a call was made first (expect(mockSendWebhookAlert).toHaveBeenCalledTimes(1)), then check severity, mirroring the critical-severity test below.
🤖 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/resource.test.ts` around lines 506 - 522, The warning-severity
test in resource.test.ts is too weak because it only checks severity when
mockSendWebhookAlert was called, so missing alerts can pass unnoticed. In the
loop around checkResourceLimitsAndAlert and mockSendWebhookAlert, assert that
exactly one webhook alert was sent before inspecting the event, then verify the
severity value directly for the 80–94.9% cases to match the critical-severity
pattern used elsewhere in the test.
Comment on lines
+52
to
+56
| const config = mockDb.prepare("SELECT id FROM resource_alert_configs WHERE contract_id = ?").get(contractID); | ||
| expect(config).toBeDefined(); | ||
|
|
||
| recordResourceAlertFired(mockDb, { | ||
| resource_alert_config_id: config.id, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm tests are type-checked and check tsconfig strictness.
fd -t f 'tsconfig*.json' | xargs -I{} sh -c 'echo "== {} =="; cat {}'
fd -t f 'vitest.config*' -x cat {}Repository: AbdulmalikAlayande/sorokeep
Length of output: 1102
🏁 Script executed:
cat node_modules/better-sqlite3/package.json 2>/dev/null || cat package.json | jq .dependencies.better-sqlite3Repository: AbdulmalikAlayande/sorokeep
Length of output: 295
🏁 Script executed:
cat tsconfig.test.jsonRepository: AbdulmalikAlayande/sorokeep
Length of output: 328
🏁 Script executed:
cat tests/commands/resources.test.ts | head -n 110 | tail -n 70Repository: AbdulmalikAlayande/sorokeep
Length of output: 2584
🏁 Script executed:
npx tsc --noEmit --project tsconfig.test.json 2>&1 | head -n 50Repository: AbdulmalikAlayande/sorokeep
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Check for any existing type errors involving config.id or better-sqlite3 usage
# First, let's check if better-sqlite3 types are present
cat package.json | grep -A 20 '"dependencies"'Repository: AbdulmalikAlayande/sorokeep
Length of output: 752
🏁 Script executed:
#!/bin/bash
# Check node_modules to see if typescript is installed
ls -la node_modules/.bin/tsc 2>/dev/null && echo "TSC found" || echo "TSC not found in node_modules"
# Check `@types/better-sqlite3` to see its definition for Statement.get()
head -n 100 node_modules/@types/better-sqlite3/lite-buffer.d.ts 2>/dev/null || head -n 100 node_modules/@types/better-sqlite3/index.d.ts 2>/dev/null || echo "No types found"Repository: AbdulmalikAlayande/sorokeep
Length of output: 210
Missing type assertion for mockDb.prepare(...).get() result.
get() returns unknown in strict mode; accessing .id on this value is a type error. Apply the as { id: number } cast here as done elsewhere in the suite.
Also applies to line 103.
Proposed fix
- const config = mockDb.prepare("SELECT id FROM resource_alert_configs WHERE contract_id = ?").get(contractID);
+ const config = mockDb.prepare("SELECT id FROM resource_alert_configs WHERE contract_id = ?").get(contractID) as { id: number };📝 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.
Suggested change
| const config = mockDb.prepare("SELECT id FROM resource_alert_configs WHERE contract_id = ?").get(contractID); | |
| expect(config).toBeDefined(); | |
| recordResourceAlertFired(mockDb, { | |
| resource_alert_config_id: config.id, | |
| const config = mockDb.prepare("SELECT id FROM resource_alert_configs WHERE contract_id = ?").get(contractID) as { id: number }; | |
| expect(config).toBeDefined(); | |
| recordResourceAlertFired(mockDb, { | |
| resource_alert_config_id: config.id, |
🤖 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/resources.test.ts` around lines 52 - 56, The test setup is
accessing `.id` on the result of `mockDb.prepare(...).get()` without a type
assertion, which fails under strict typing. In `resources.test.ts`, update the
`config` lookup used by `recordResourceAlertFired` (and the similar lookup near
the other referenced spot) to cast the `get()` result as `{ id: number }`,
matching the pattern already used elsewhere in the suite.
Comment on lines
+92
to
+118
| it("accepts --period and filters history records", () => { | ||
| insertResourceAlertConfig(mockDb, { | ||
| contract_id: contractID, | ||
| channel_type: "webhook", | ||
| channel_target: "https://example.com", | ||
| cpu_limit: 100000000, | ||
| mem_limit: 50000000, | ||
| }); | ||
|
|
||
| const config = mockDb.prepare("SELECT id FROM resource_alert_configs WHERE contract_id = ?").get(contractID); | ||
|
|
||
| recordResourceAlertFired(mockDb, { | ||
| resource_alert_config_id: config.id, | ||
| resource_type: "cpu", | ||
| usage: 50_000_000, | ||
| limit: 100_000_000, | ||
| usage_percent: 50, | ||
| fired_at_ledger: 100, | ||
| }); | ||
|
|
||
| const program = new Command(); | ||
| registerResourcesCommand(program); | ||
|
|
||
| program.parse(["node", "sorokeep", "resources", contractID, "--period", "30"]); | ||
|
|
||
| expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining("Resource Usage Trends")); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Test does not actually verify --period filtering.
It only asserts the header line is printed, which would pass even if --period were ignored. Add a record outside the window (e.g. via an older fired_at) and assert it is excluded, so the filter behavior is genuinely covered as the issue's TDD requirement intends.
🤖 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/resources.test.ts` around lines 92 - 118, The test for
registerResourcesCommand only checks that the Resource Usage Trends header is
printed, so it does not prove that --period filtering works. Update the
resources command test to include an additional older history record outside the
requested window using recordResourceAlertFired, then assert that the output
excludes that record while still showing the in-range one. Keep the focus on the
command flow in registerResourcesCommand and the fired_at_ledger-based history
data so the test genuinely verifies --period behavior.
This was referenced Jun 26, 2026
AbdulmalikAlayande
added a commit
that referenced
this pull request
Aug 2, 2026
AbdulmalikAlayande
added a commit
that referenced
this pull request
Aug 2, 2026
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
2 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.
Closes #165