Feature/shared budget pools - #635
TheWeirdDee wants to merge 842 commits into
Conversation
…des and acceptance criteria
…estimates - Add countExtensionsInLastHour() to repositories.ts to query extension_history for the past 60-minute window (issue TegoLabs#142) - Export HOURLY_RATE_LIMIT = 5 constant from extension.ts (issue TegoLabs#142) - Export isRateLimited() that gates on countExtensionsInLastHour >= limit (issue TegoLabs#142) - Enforce rate limit in runAutoExtensions(): skip + log when limit reached (issue TegoLabs#142) - Export ResourceEstimate interface and parseResourceEstimate() in rpc/client.ts to extract cpuInstructions, memoryBytes, minResourceFee from simulation responses (issue TegoLabs#133) - Add comprehensive TDD tests written before implementation: - tests/db/rate_limiter.test.ts: countExtensionsInLastHour edge cases - tests/core/rate_limiter.test.ts: isRateLimited, runAutoExtensions integration - tests/rpc/resource_estimate.test.ts: parseResourceEstimate + failure edge cases Closes TegoLabs#133 Closes TegoLabs#137 Closes TegoLabs#142
Repo Avatar
…goLabs#246) * feat(db): add resource_usage_logs table with repository functions * fix(db): add type assertions for snapshotRow and currentDayRow in repositories.ts * fix(core): replace empty RentWindowProjection interface with type alias * fix: resolve schema corruption and linting bypasses --------- Co-authored-by: AbdulmalikAlayande <114596864+AbdulmalikAlayande@users.noreply.github.com>
…goLabs#246) * feat(db): add resource_usage_logs table with repository functions * fix(db): add type assertions for snapshotRow and currentDayRow in repositories.ts * fix(core): replace empty RentWindowProjection interface with type alias * fix: resolve schema corruption and linting bypasses --------- Co-authored-by: AbdulmalikAlayande <114596864+AbdulmalikAlayande@users.noreply.github.com>
Cover the 'alerts test' subcommand with unit tests: - dispatcher is invoked with correct channel type, target, and event - test message delivered to webhook and Slack channels - webhook secret passed through to dispatcher - exits with code 1 on delivery failure - exits with code 1 when config ID is not found or not numeric
Cover the 'alerts test' subcommand with unit tests: - dispatcher is invoked with correct channel type, target, and event - test message delivered to webhook and Slack channels - webhook secret passed through to dispatcher - exits with code 1 on delivery failure - exits with code 1 when config ID is not found or not numeric
…oLabs#325) Adds opt-in per-alert-config quiet-hours windows so operators can suppress alert delivery during planned maintenance without deleting and re-creating alert configs. ## Changes ### Schema & migration - src/db/schema.sql: add nullable quiet_hours_start, quiet_hours_end, quiet_hours_timezone TEXT columns to alert_configs (fresh installs) - src/db/migrations/002_quiet_hours.sql: ALTER TABLE migration for existing on-disk databases - src/db/migrator.ts: handle 'duplicate column name' errors gracefully for ADD COLUMN migrations (columns already present via schema.sql on fresh/test DBs) — marks migration applied rather than failing - src/db/database.ts: add quiet-hours columns to live migrations array (try/catch pattern); include new columns in table-rebuild functions migrateAlertConfigsChannelTypeCheck() and relaxChannelTypeChecks() ### Repository layer - src/db/repositories.ts: extend AlertConfig interface with quiet_hours_start/end/timezone; update insertAlertConfig() to accept and persist them; extend UndeliveredAlert interface and getUndeliveredAlerts() SQL to carry the fields to the dispatcher ### Dispatcher - src/alerts/dispatcher.ts: export isInQuietHours() and currentHHMMInTz() helpers using Intl.DateTimeFormat (no new dep); add quiet-hours skip check in deliverPendingAlerts() — skipped alerts remain pending (delivered=0, retry_count unchanged) for the next cycle ### CLI - src/commands/alerts.ts: add --quiet-hours <HH:MM-HH:MM> and --timezone <IANA-tz> flags to 'alerts add'; validates both are provided together, enforces HH:MM-HH:MM format, validates IANA tz via Intl; success message includes quiet window when set ### Tests (TDD) - tests/alerts/dispatcher.test.ts: extend seedAlert() helper to accept quiet-hours opts; add 'Quiet hours' describe block with 7 tests: no-quiet-hours delivery, inside-window skip (not delivered, not marked delivered, retry_count not incremented), outside-window delivery, overnight windows (start > end), pending-after-skip then delivery once window cleared, retry_count isolation, multi-alert isolation Closes TegoLabs#325
…r PR CI Every open PR's CI Pipeline was failing at the "Audit Dependencies" step on the exact same findings, since actions/checkout defaults to the merge-with-main ref for pull_request events - meaning every PR inherited main's vulnerable dependency tree regardless of its own diff. - Bump @stellar/stellar-sdk ^16.0.1 -> ^16.2.0, which resolves to a patched axios (>=1.18.0), fixing the high-severity DoS/prototype- pollution advisories in the previously-resolved 1.16.1. - npm audit fix for @hono/node-server, fast-uri, and postcss. - Scope the CI audit step to --omit=dev: the one remaining high-severity finding (brace-expansion, via @vitest/coverage-v8's glob chain) only exists in devDependencies, is never shipped to npm install'ers, and fixing it requires a vitest 3->4 major bump that needs its own deliberate verification, not a blind force-fix to unblock CI. Verified locally: lint, tsc --noEmit, full test suite (995/995), build, and npm audit --omit=dev all clean before pushing.
Closes TegoLabs#314 - Add src/alerts/opsgenie.ts with OpsgenieChannel class and sendOpsgenieAlert function following the AlertChannel interface - POST to /v2/alerts with GenieKey authorization header for threshold_crossed, resource_alert, and state_changed events - Map alert_resolved to POST /v2/alerts/{alias}/close endpoint - buildAlias() mirrors pagerduty.ts buildDedupKey() exactly so repeated threshold crossings for the same entry deduplicate - Validate API key before any network call with a clear error message - Use AbortController/timeout pattern consistent with other channels - Register in builtins.ts with targetOption: routingKey - Add 32 tests in tests/alerts/opsgenie.test.ts (TDD, written first) - Update tests/alerts/builtins.test.ts for the new 6-channel count
- Add ?identifierType=alias to the close URL so Opsgenie looks up the alert by alias rather than ID (critical: without this, close silently finds nothing since the default identifierType is 'id') - Surface Opsgenie response body in error messages for faster debugging - Test that identifierType=alias is present in the close URL - Test that error body message is included in the thrown error - Test AbortController timeout fires after 10 seconds - Assert opsgenie missingTargetError wording in builtins test
Ensures vi.useRealTimers() is always called even if the test assertion fails, preventing timer pollution affecting subsequent tests.
Distinguish AbortError (our own 10-second timeout) from other fetch failures by catching it explicitly and rethrowing with a message that names the configured timeout duration: 'Opsgenie API request timed out after 10 seconds' Other network errors (ECONNREFUSED etc.) are rethrown unchanged. Update the timeout test assertion to match the new message.
- Store trimmed API key in constructor so leading/trailing whitespace is not forwarded in the GenieKey Authorization header - Simplify 'sets priority P3 for info severity' test: remove the redundant alert_resolved call and mock reset; test directly with a resource event at info severity
…egoLabs#550) Small maintainer fixup on top of TegoLabs#550's Opsgenie channel: the timeout error was thrown without a cause chain, tripping preserve-caught-error.
… rebuild (fixup for TegoLabs#528) migrateAlertConfigsChannelTypeCheck's and relaxChannelTypeChecks's table-rebuild INSERT/SELECT lists didn't include quiet_hours_start/end/timezone, so any existing database still carrying the legacy channel_type CHECK enum would silently drop configured quiet-hours windows the first time it opened after upgrading. Caught by CodeRabbit on TegoLabs#528; fixing directly since it touches the same migration functions from earlier this session.
…e.test.ts (TegoLabs#537) Verified locally: lint, typecheck, full suite (1039/1039), build, and audit all clean. Closes TegoLabs#367.
|
@TheWeirdDee 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 shared monthly budget pools, CLI commands for pool creation and contract assignment, and pool-aware auto-extension enforcement. Shared spending uses cycle resets, fee reservations, reconciliation, and rollback. Existing per-contract budget enforcement remains the fallback. ChangesShared budget pools
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant runAutoExtensions
participant shared_budget_pools
participant extendEntries
runAutoExtensions->>shared_budget_pools: Read assigned pool and billing cycle
runAutoExtensions->>shared_budget_pools: Reserve estimated fee
runAutoExtensions->>extendEntries: Submit extension
extendEntries-->>runAutoExtensions: Return actual charged fee or failure
runAutoExtensions->>shared_budget_pools: Reconcile actual fee or release reservation
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.6)src/core/extension.tsFile contains syntax errors that prevent linting: Line 521: Expected a statement but instead found 'finally'.; Line 529: Expected a catch clause but instead found ')'.; Line 534: expected 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/budget.ts`:
- Around line 28-51: Wrap the INSERT in the pool create action handler with
duplicate-name error handling so a UNIQUE constraint failure for
shared_budget_pools.name follows the existing chalk.red message and
process.exit(1) failure pattern. Keep successful creation behavior unchanged,
and add coverage in budget.test.ts for attempting to create a pool with an
existing name.
In `@tests/core/budget_enforcement.test.ts`:
- Around line 142-172: Add a success-path test around runAutoExtensions that
mocks extendEntries so feeCharged differs from the reserved estimate, then
assert the shared pool’s spent_xlm reflects the actual fee by reconciling the
reservation delta. Cover either an upward or downward difference and retain the
contract budget assertion that shared-pool spending is recorded correctly.
🪄 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: 5abbd5bb-d853-4d79-b132-fb25b6c271b2
📒 Files selected for processing (6)
src/commands/budget.tssrc/core/extension.tssrc/db/migrations/003_shared_budget_pools.sqlsrc/db/schema.sqltests/commands/budget.test.tstests/core/budget_enforcement.test.ts
📜 Review details
🔇 Additional comments (6)
src/db/migrations/003_shared_budget_pools.sql (1)
1-19: LGTM!src/db/schema.sql (1)
220-238: LGTM!src/commands/budget.ts (1)
53-82: LGTM!tests/commands/budget.test.ts (1)
7-7: LGTM!Also applies to: 56-103
src/core/extension.ts (1)
384-438: LGTM!Also applies to: 450-460, 478-484
tests/core/budget_enforcement.test.ts (1)
159-172: 🎯 Functional CorrectnessNo duplicated statement.
The line is not duplicated in the actual test file.
> Likely an incorrect or invalid review comment.
| it('records successful extension spend against the shared pool only', async () => { | ||
| const currentCycle = new Date().toISOString().slice(0, 7); | ||
| const pool = db.prepare(`INSERT INTO shared_budget_pools (name, monthly_limit_xlm, billing_cycle) VALUES (?, ?, ?)`) | ||
| .run('product-line', 100, currentCycle); | ||
| db.prepare(`INSERT INTO shared_budget_pool_contracts (pool_id, contract_id) VALUES (?, ?)`) | ||
| .run(pool.lastInsertRowid, 'contract_1'); | ||
| upsertBudget(db, { contract_id: 'contract_1', limit_xlm: 1000, billing_cycle: currentCycle }); | ||
|
|
||
| const result = await runAutoExtensions(db, 'testnet'); | ||
|
|
||
| expect(result.contractsExtended).toBe(1); | ||
| const sharedPool = db.prepare(`SELECT spent_xlm FROM shared_budget_pools WHERE id = ?`) | ||
| .get(pool.lastInsertRowid) as { spent_xlm: number }; | ||
| expect(sharedPool.spent_xlm).toBe(1.5); | ||
| expect(getBudget(db, 'contract_1', currentCycle)?.spent_xlm).toBe(0); | ||
| }); | ||
|
|
||
| it('resets stale pool spend when a new billing cycle starts', async () => { | ||
| const currentCycle = new Date().toISOString().slice(0, 7); | ||
| const pool = db.prepare(`INSERT INTO shared_budget_pools (name, monthly_limit_xlm, billing_cycle, spent_xlm) VALUES (?, ?, ?, ?)`) | ||
| .run('product-line', 2, '2000-01', 2); | ||
| db.prepare(`INSERT INTO shared_budget_pool_contracts (pool_id, contract_id) VALUES (?, ?)`) | ||
| .run(pool.lastInsertRowid, 'contract_1'); | ||
|
|
||
| const result = await runAutoExtensions(db, 'testnet'); | ||
|
|
||
| expect(result.contractsExtended).toBe(1); | ||
| const sharedPool = db.prepare(`SELECT billing_cycle, spent_xlm FROM shared_budget_pools WHERE id = ?`) | ||
| .get(pool.lastInsertRowid) as { billing_cycle: string; spent_xlm: number }; | ||
| expect(sharedPool).toEqual({ billing_cycle: currentCycle, spent_xlm: 1.5 }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add a test where the actual fee differs from the reservation estimate.
Both new success-path tests settle at the same figure (1.5) as the reservation estimate, so the delta-reconciliation math in extension.ts (spent_xlm += actualFeeXlm - reservedPoolSpend) is never exercised with actual != estimated. A mock where extendEntries's feeCharged diverges from the simulated fee would validate that reconciliation correctly adjusts spent_xlm up or down.
🤖 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/core/budget_enforcement.test.ts` around lines 142 - 172, Add a
success-path test around runAutoExtensions that mocks extendEntries so
feeCharged differs from the reserved estimate, then assert the shared pool’s
spent_xlm reflects the actual fee by reconciling the reservation delta. Cover
either an upward or downward difference and retain the contract budget assertion
that shared-pool spending is recorded correctly.
|
| 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.
43ad363 to
8692701
Compare
Resolves all conflicts from upstream main's independent progress (new alert channels, RPC client typing, OpenTelemetry tracing, metrics/audit-log commands, contract groups, quiet-hours CHECK rebuild, etc.) while preserving the shared budget pools feature. Notable fixes made during resolution: - Renumbered migration 003_shared_budget_pools.sql to 006_shared_budget_pools.sql to avoid a version collision with upstream's 003_contract_groups.sql (both would have hit the schema_migrations PRIMARY KEY on a fresh install). - Dropped a duplicate inline daily-cost-aggregation call in src/daemon/loop.ts left over from a stale merge of the daemon cycle steps. - Regenerated package-lock.json and man/sorokeep.1 against the merged package.json and CLI surface. Verified: tsc --noEmit clean, npm run build succeeds, full test suite passes (100 files, 1317 tests, 1 pre-existing skip).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
src/core/extension.ts (3)
394-438: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRevalidate pool assignment during reservation.
Lines 394-417 read the pool before Line 427 awaits RPC simulation.
budget pool assigncan reassign the contract during that gap. Line 438 can then reserve spend from the cached pool aftershared_budget_pool_contractspoints to another pool.Make the reservation conditional on the current
contract_idassociation. If the association changed, reload the pool and retry the reservation.🤖 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/core/extension.ts` around lines 394 - 438, The shared-budget reservation flow around sharedBudget and the UPDATE of shared_budget_pools must revalidate that contract.id is still associated with sharedBudget.id after simulateExtension awaits RPC. Make the reservation conditional on the current shared_budget_pool_contracts association; if it no longer matches, reload the contract’s current pool and retry reservation using that pool, preserving billing-cycle and limit checks.
424-507: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSettle reservations when
extendEntriesthrows.After Lines 433-444 reserve pool spend, Lines 450-458 call
extendEntries.extendEntriescan throw after a successful submission whengetEntryTTLsor local persistence fails. Control then reaches Lines 503-507 and skips the release at Lines 489-495. The reservation remains inspent_xlmindefinitely.Track reservation settlement explicitly. Release it only when submission did not occur. Persist a pending reconciliation when submission may have succeeded.
🤖 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/core/extension.ts` around lines 424 - 507, Track whether extendEntries submitted a transaction and whether the shared-budget reservation has been settled across the extendEntries call and surrounding catch/finally flow. On thrown errors, release the reservation only when submission did not occur; when submission may have succeeded, persist a pending reconciliation instead of leaving spent_xlm reserved indefinitely. Update the shared-budget handling around extendEntries and the outer catch so every reservation reaches one explicit settlement path.
461-467: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReserve an upper fee bound before submission.
Line 467 adds a positive actual-fee delta without checking
monthly_limit_xlm. IfactualFeeXlmexceeds the estimate, the extension can exceed the shared-pool cap. If the estimate is zero, Line 462 also skips actual-fee accounting.Reserve a conservative maximum fee before submission. Reject the extension when that bound exceeds the remaining allowance. Reconcile only downward after the transaction completes.
🤖 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/core/extension.ts` around lines 461 - 467, Update the extension fee flow around actualFeeXlm and the shared_budget_pools update to reserve a conservative maximum fee before submission, validate it against the pool’s remaining monthly_limit_xlm allowance, and reject the extension when insufficient. Ensure zero estimates still receive actual-fee accounting, and after completion reconcile the reservation only downward rather than adding a positive actual-fee delta.src/commands/alerts.ts (4)
401-432: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExercise every configured target in the connectivity commands.
alerts addpersists additional targets, butalerts testandalerts test-allcalldeliverSingleAlertonly with the primary configuration fields. A successful primary delivery can hide failures on additional targets. LoadgetAlertConfigTargetsand test each target.Also applies to: 465-481
🤖 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 401 - 432, Update the alert test flows around deliverSingleAlert in alerts test and alerts test-all to load targets via getAlertConfigTargets and attempt delivery for every configured target, not only the primary channel_target. Preserve dry-run output behavior, while ensuring success or failure reporting reflects all configured targets and a primary success cannot hide additional-target failures.
26-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the contract network in test events.
buildTestEventalways setsnetworkto"testnet". A test for a contract on another network sends an incorrect network value. Pass the registered contract network into this helper and update both callers.Proposed fix
-function buildTestEvent(contractId: string, thresholdLedgers: number) { +function buildTestEvent(contractId: string, network: string, thresholdLedgers: number) { return buildAlertEvent({ type: "threshold_crossed", contractId, - network: "testnet", + 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/alerts.ts` around lines 26 - 38, Update buildTestEvent to accept the registered contract network as an argument and use it for the network field instead of the hardcoded "testnet"; update both callers to pass each contract’s network value.
117-146: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGenerate the webhook secret for signing-capable additional targets.
alert_config_targetshas no per-target secret, and delivery passes oneconfig.webhook_secretto every target. If the primary target is Slack but an--target webhook:...is added,webhookSecretstays undefined and the webhook payload is sent withoutX-Sorokeep-Signature. Use one shared secret when any target supports signing, or reject combination commands containing both non-signed primary and signing additional 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 `@src/commands/alerts.ts` around lines 117 - 146, Update the target initialization flow around primaryType and additionalTargets so webhookSecret is generated or supplied whenever any configured target supports signing, including signing-capable additional targets. Reuse one shared secret across all targets, and ensure combinations with a non-signing primary and signing additional target are either given that secret or explicitly rejected before delivery.
97-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject empty additional targets before persisting them.
--target slack:passes the current validation.ctargetbecomes empty, and the command stores an unusable target while reporting success. Reject empty or whitespace-only targets beforeadditionalTargets.push.Proposed fix
const parts = t.split(":"); - if (parts.length < 2) { + const ctarget = parts.slice(1).join(":"); + if (parts.length < 2 || ctarget.trim().length === 0) { console.error(chalk.red(`Error: --target must be formatted as <type>:<target> (e.g. webhook:https://...). Got: ${t}`)); process.exit(1); } const ctype = parts[0]!; - const ctarget = parts.slice(1).join(":");🤖 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 97 - 113, Update the validation in the --target parsing loop before additionalTargets.push so ctarget is rejected when empty or whitespace-only, reporting the same invalid-target error and exiting without persisting it. Preserve the existing channel-type validation and valid target handling.tests/commands/alerts.test.ts (2)
898-920: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the complete dry-run event payload.
The test name promises the exact JSON payload, but the assertion checks only
type. A regression can remove or changecontractIdor other required fields and still pass. Assert every field required by the alert-event contract and its expected value.🤖 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 898 - 920, Update the dry-run test around registerAlertsCommand and consoleLogSpy to parse the printed JSON payload and assert the complete alert-event contract, including contractId and every other required field with its expected value, rather than checking only type. Keep the existing assertions that deliverSingleAlert is not called and that output is produced.
923-950: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAssert the HMAC value, not only its prefix.
The test does not verify that the digest matches
dry-run-secretand the serialized event. Compute an expected digest independently and compare the completeX-Sorokeep-Signaturevalue. Keep the expected calculation independent from the production signing helper.🤖 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 923 - 950, Update the signed webhook test around the dry-run invocation to independently compute the expected HMAC-SHA256 digest from `dry-run-secret` and the serialized event payload, then assert the complete `X-Sorokeep-Signature: sha256=...` output equals that value. Replace the current prefix-only assertion while keeping the existing `mockDeliverSingleAlert` check and avoiding reuse of the production signing helper.src/utils/config.ts (1)
154-164: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate the SMTP port range and integer form.
The current check accepts fractional ports, values above
65535, andInfinity. The loader returns these values as valid, so email delivery fails later. RequireNumber.isInteger(port) && port >= 1 && port <= 65535.🤖 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/utils/config.ts` around lines 154 - 164, Update the port validation in parseSmtpConfig to require Number.isInteger(port), port >= 1, and port <= 65535, replacing the current !isNaN(port) and port > 0 checks while preserving the existing SMTP field validation and returned configuration.src/commands/db.ts (2)
41-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject conflicting import modes.
When both
--forceand--mergeare supplied,modeis always"merge"becauseoptions.mergetakes precedence. The command silently selects a different mode from the--forcedescription. Reject the combination before importing.🤖 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/db.ts` around lines 41 - 54, In the import command’s action handler, validate that options.force and options.merge are not both enabled before determining mode or calling importDatabase. Reject the conflicting combination with a clear error, while preserving the existing replace and merge behavior for each individual option.
41-54: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve shared-budget state in database backups.
DatabaseBackup,exportDatabase(),isDatabaseEmpty(),importDatabase(), and the backup metadata all omitshared_budget_poolsandshared_budget_pool_contracts. Add both shared-budget tables to the typed backup, export list, import loop, validate, column maps, and clear-table list so imports restore pool limits, billing cycles, spend, and contract assignments.🤖 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/db.ts` around lines 41 - 54, Extend the database backup flow to include shared_budget_pools and shared_budget_pool_contracts everywhere backup state is represented: DatabaseBackup, backup metadata, exportDatabase(), isDatabaseEmpty(), importDatabase() validation and import loops, column maps, and clear-table handling. Preserve the existing import modes while ensuring pool limits, billing cycles, spend, and contract assignments are exported and restored.src/db/repositories.ts (2)
919-927: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter delivery statistics by the recorded delivery channel.
recordAlertFiredstores delivery-time channel metadata, and the history queries already prefer it withCOALESCE(af.channel_type, ac.channel_type). This query still filters onlyac.channel_type. A webhook target recorded under a Slack primary configuration is omitted fromalerts stats --type webhook. Use the sameCOALESCEexpression in the filter and add a regression case.Proposed fix
- WHERE ac.channel_type = ? + WHERE COALESCE(af.channel_type, ac.channel_type) = ?🤖 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 919 - 927, Update the alerts statistics query near the totalAttempts and deliveredCount aggregates to filter by the recorded delivery channel using COALESCE(af.channel_type, ac.channel_type), matching the existing history-query behavior. Add a regression case covering a webhook delivery recorded under a Slack primary configuration and verify it is included when requesting webhook statistics.
55-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign
AlertConfig.created_atwith the SQLite runtime value.
alert_configs.created_atis stored asTEXT, and the raw SQLite rows return it as a string. Map or select it asstring, or convert it explicitly; usingDatefor the current type still masks the runtime gap.🤖 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 55 - 57, Update the AlertConfig created_at field in the repository model to match SQLite’s TEXT runtime value by representing it as a string, or explicitly converting the selected value to Date before assignment. Ensure the type and returned row value remain consistent throughout the alert configuration repository flow.tests/core/extension.test.ts (1)
837-842: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace the wall-clock assertions with deterministic timer checks.
runAutoExtensionsreturns before scheduled delay timers fire, so the second test can advance ~270 ms before theDate.now()read becomes stale. Use Vitest fake timers or assert the injected delay schedule directly. Both tests can still be flaky under CI load or low timer resolution.🤖 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/core/extension.test.ts` around lines 837 - 842, Replace the Date.now-based duration assertion around runAutoExtensions with deterministic Vitest fake-timer checks, and verify the injected delay schedule directly where applicable. Ensure timers are advanced or inspected explicitly so both tests validate immediate return and scheduled delays without relying on wall-clock timing.
🤖 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.
Outside diff comments:
In `@src/commands/alerts.ts`:
- Around line 401-432: Update the alert test flows around deliverSingleAlert in
alerts test and alerts test-all to load targets via getAlertConfigTargets and
attempt delivery for every configured target, not only the primary
channel_target. Preserve dry-run output behavior, while ensuring success or
failure reporting reflects all configured targets and a primary success cannot
hide additional-target failures.
- Around line 26-38: Update buildTestEvent to accept the registered contract
network as an argument and use it for the network field instead of the hardcoded
"testnet"; update both callers to pass each contract’s network value.
- Around line 117-146: Update the target initialization flow around primaryType
and additionalTargets so webhookSecret is generated or supplied whenever any
configured target supports signing, including signing-capable additional
targets. Reuse one shared secret across all targets, and ensure combinations
with a non-signing primary and signing additional target are either given that
secret or explicitly rejected before delivery.
- Around line 97-113: Update the validation in the --target parsing loop before
additionalTargets.push so ctarget is rejected when empty or whitespace-only,
reporting the same invalid-target error and exiting without persisting it.
Preserve the existing channel-type validation and valid target handling.
In `@src/commands/db.ts`:
- Around line 41-54: In the import command’s action handler, validate that
options.force and options.merge are not both enabled before determining mode or
calling importDatabase. Reject the conflicting combination with a clear error,
while preserving the existing replace and merge behavior for each individual
option.
- Around line 41-54: Extend the database backup flow to include
shared_budget_pools and shared_budget_pool_contracts everywhere backup state is
represented: DatabaseBackup, backup metadata, exportDatabase(),
isDatabaseEmpty(), importDatabase() validation and import loops, column maps,
and clear-table handling. Preserve the existing import modes while ensuring pool
limits, billing cycles, spend, and contract assignments are exported and
restored.
In `@src/core/extension.ts`:
- Around line 394-438: The shared-budget reservation flow around sharedBudget
and the UPDATE of shared_budget_pools must revalidate that contract.id is still
associated with sharedBudget.id after simulateExtension awaits RPC. Make the
reservation conditional on the current shared_budget_pool_contracts association;
if it no longer matches, reload the contract’s current pool and retry
reservation using that pool, preserving billing-cycle and limit checks.
- Around line 424-507: Track whether extendEntries submitted a transaction and
whether the shared-budget reservation has been settled across the extendEntries
call and surrounding catch/finally flow. On thrown errors, release the
reservation only when submission did not occur; when submission may have
succeeded, persist a pending reconciliation instead of leaving spent_xlm
reserved indefinitely. Update the shared-budget handling around extendEntries
and the outer catch so every reservation reaches one explicit settlement path.
- Around line 461-467: Update the extension fee flow around actualFeeXlm and the
shared_budget_pools update to reserve a conservative maximum fee before
submission, validate it against the pool’s remaining monthly_limit_xlm
allowance, and reject the extension when insufficient. Ensure zero estimates
still receive actual-fee accounting, and after completion reconcile the
reservation only downward rather than adding a positive actual-fee delta.
In `@src/db/repositories.ts`:
- Around line 919-927: Update the alerts statistics query near the totalAttempts
and deliveredCount aggregates to filter by the recorded delivery channel using
COALESCE(af.channel_type, ac.channel_type), matching the existing history-query
behavior. Add a regression case covering a webhook delivery recorded under a
Slack primary configuration and verify it is included when requesting webhook
statistics.
- Around line 55-57: Update the AlertConfig created_at field in the repository
model to match SQLite’s TEXT runtime value by representing it as a string, or
explicitly converting the selected value to Date before assignment. Ensure the
type and returned row value remain consistent throughout the alert configuration
repository flow.
In `@src/utils/config.ts`:
- Around line 154-164: Update the port validation in parseSmtpConfig to require
Number.isInteger(port), port >= 1, and port <= 65535, replacing the current
!isNaN(port) and port > 0 checks while preserving the existing SMTP field
validation and returned configuration.
In `@tests/commands/alerts.test.ts`:
- Around line 898-920: Update the dry-run test around registerAlertsCommand and
consoleLogSpy to parse the printed JSON payload and assert the complete
alert-event contract, including contractId and every other required field with
its expected value, rather than checking only type. Keep the existing assertions
that deliverSingleAlert is not called and that output is produced.
- Around line 923-950: Update the signed webhook test around the dry-run
invocation to independently compute the expected HMAC-SHA256 digest from
`dry-run-secret` and the serialized event payload, then assert the complete
`X-Sorokeep-Signature: sha256=...` output equals that value. Replace the current
prefix-only assertion while keeping the existing `mockDeliverSingleAlert` check
and avoiding reuse of the production signing helper.
In `@tests/core/extension.test.ts`:
- Around line 837-842: Replace the Date.now-based duration assertion around
runAutoExtensions with deterministic Vitest fake-timer checks, and verify the
injected delay schedule directly where applicable. Ensure timers are advanced or
inspected explicitly so both tests validate immediate return and scheduled
delays without relying on wall-clock timing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c1f513ad-9c27-4506-b066-dd7873612e7c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
man/sorokeep.1src/commands/alerts.tssrc/commands/budget.tssrc/commands/db.tssrc/core/extension.tssrc/db/migrations/006_shared_budget_pools.sqlsrc/db/repositories.tssrc/db/schema.sqlsrc/utils/config.tstests/commands/alerts.test.tstests/commands/budget.test.tstests/core/budget_enforcement.test.tstests/core/extension.test.tstests/db/repositories.test.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.0)
src/commands/db.ts
[warning] 45-45: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(file, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔇 Additional comments (18)
src/commands/budget.ts (2)
46-49: Handle duplicate pool names.
shared_budget_pools.nameis unique. Lines 46-49 still let a duplicate-name constraint error escape instead of using the command failure path.
53-82: LGTM!src/db/migrations/006_shared_budget_pools.sql (1)
1-19: LGTM!tests/commands/budget.test.ts (1)
7-7: LGTM!Also applies to: 56-104
tests/commands/alerts.test.ts (8)
6-12: LGTM!
96-116: LGTM!
118-140: LGTM!
331-358: LGTM!
649-718: LGTM!
954-1013: LGTM!
1015-1025: LGTM!
1027-1042: 📐 Maintainability & Code QualityNo change is needed.
Vitest isolates test files by default, and
_resetRegistryForTestingalready clears the shared registry between affected tests in the relevant alert tests.> Likely an incorrect or invalid review comment.man/sorokeep.1 (1)
1-52: LGTM!src/commands/alerts.ts (1)
10-24: LGTM!Also applies to: 51-55, 190-207, 230-241, 293-316, 335-379, 442-464, 483-500, 547-572
src/commands/db.ts (1)
8-8: LGTM!src/db/repositories.ts (1)
60-65: LGTM!Also applies to: 297-371, 467-506, 767-903, 1492-1570
src/utils/config.ts (1)
20-56: LGTM!Also applies to: 99-115
tests/db/repositories.test.ts (1)
241-291: LGTM!Also applies to: 537-690
|
Hi, please can you merge this pr before the deadline? |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/extension.ts (2)
397-447: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReserve against the current pool assignment.
At Line 397, the code reads the pool before
simulateExtensionawaits. A CLI reassignment can complete before Line 436. The reservation then debits the old pool because the update checks onlysharedBudget.id.Move the pool lookup, cycle reset, and reservation into one transaction after simulation. Verify the current
shared_budget_pool_contractsmapping in the reservation. If the contract is no longer assigned, apply the individual-budget path.🤖 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/core/extension.ts` around lines 397 - 447, The shared budget pool is read before simulateExtension and may be stale when reserving spend. Move the pool lookup, billing-cycle reset, and reservation logic into one database transaction after simulation; have the reservation revalidate the current shared_budget_pool_contracts mapping for contract.id, and fall back to the individual getBudget path when no assignment remains. Update the surrounding sharedBudget/budget flow so only the currently assigned pool is debited.
482-489: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard shared-pool spend against actual fees above the reservation.
The simulation only reserves
minResourceFee / 10000000; the submit path can return afeeChargedXDR value different from that. Since the final UPDATE recordsactualFeeXlm - reservedPoolSpendwithout a cap check, a successful shared-budget extension can pushspent_xlmovermonthly_limit_xlm. Use a conservative upper bound for the reservation/reconciliation or enforce the pooled limit again during reconciliation.🤖 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/core/extension.ts` around lines 482 - 489, Update the shared-budget reconciliation in the extension submit path to prevent actual fees from exceeding the pool’s remaining monthly limit when applying actualFeeXlm - reservedPoolSpend. Use a conservative reservation upper bound or revalidate the pooled limit atomically in the shared_budget_pools UPDATE, while preserving the existing fee reconciliation behavior when sufficient budget remains.
🤖 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.
Outside diff comments:
In `@src/core/extension.ts`:
- Around line 397-447: The shared budget pool is read before simulateExtension
and may be stale when reserving spend. Move the pool lookup, billing-cycle
reset, and reservation logic into one database transaction after simulation;
have the reservation revalidate the current shared_budget_pool_contracts mapping
for contract.id, and fall back to the individual getBudget path when no
assignment remains. Update the surrounding sharedBudget/budget flow so only the
currently assigned pool is debited.
- Around line 482-489: Update the shared-budget reconciliation in the extension
submit path to prevent actual fees from exceeding the pool’s remaining monthly
limit when applying actualFeeXlm - reservedPoolSpend. Use a conservative
reservation upper bound or revalidate the pooled limit atomically in the
shared_budget_pools UPDATE, while preserving the existing fee reconciliation
behavior when sufficient budget remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3dff4624-c952-466c-a463-c004f22cb711
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/core/extension.ts
📜 Review details
🔇 Additional comments (3)
src/core/extension.ts (3)
16-25: LGTM!
449-466: LGTM!
510-516: 🩺 Stability & AvailabilityNo reservation rollback is needed here.
… PR #635) Adds shared_budget_pools / shared_budget_pool_contracts and a budget pool create/assign CLI surface. A contract assigned to a pool is enforced against the pool's combined monthly cap instead of its individual contract_budgets row — pool membership takes precedence, unassigned contracts keep working exactly as before. Spend is reserved atomically (a single UPDATE with a WHERE clause re-checking spent_xlm against monthly_limit_xlm) before the extension transaction runs, then trued up to the actual charged fee on success or rolled back on failure — safe against two contracts in the same pool racing past the cap when runAutoExtensions processes contracts concurrently. Renumbered the PR's migration to 008 (006/007 were taken by digest configs and fleet indexes, merged earlier in this batch). The PR's own diff for the budget-check branch in extension.ts had a botched hunk that would have left a duplicated, dead nested `if` condition in the individual-budget path — rewrote that section cleanly instead of applying it as-is. Also added alert delivery on pool exhaustion (reusing buildBudgetExhaustedAlertEvent, same as the per-contract path) for UX parity, since the PR's version silently threw without notifying configured alert channels. Dropped the PR's unrelated whitespace-only diffs in commands/alerts.ts, commands/db.ts, and utils/config.ts, three stray src/alerts/*.test.ts files (same simulation_cache-referencing pollution seen in #406/#409), and a pure reordering of an unrelated test block in tests/db/repositories.test.ts — none in scope for this issue.
|
Merged via b34acb7 on main. The core design — shared_budget_pools + a junction table, pool membership taking precedence over a contract's individual budget, and a reserve-then-true-up spend pattern in runAutoExtensions — was correct and is exactly what shipped. A few adjustments before merging:
The atomic reserve-if-under-limit UPDATE pattern (checking Thanks for a well-designed, well-tested PR on real spend-limiting logic. |
18 participants
Summary
Closes #407.
Implements fleet-wide budget aggregation through shared monthly budget pools.
Contracts assigned to the same pool now draw from one combined XLM allowance. Pool membership takes precedence over an individual contract budget, while contracts without a pool retain the existing per-contract budget behavior.
Changes
shared_budget_poolsandshared_budget_pool_contracts003_shared_budget_pools.sqlrunAutoExtensionsto:sorokeep budget pool create --name <name> --limit <xlm>sorokeep budget pool assign --pool <name> --contract <contractId>Budget Precedence
A contract assigned to a shared pool uses the pool limit instead of its individual limit.
A contract that is not assigned to a pool continues using its individual budget exactly as before.
Testing
Tests were written before the implementation.
Acceptance Criteria