feat(core): implement TTL-drift alerting when actual TTL diverges fro… - #520
Gabugo-tech wants to merge 884 commits into
Conversation
* feat(core): build ledger entry key decoder * fix: use valid base32 string for mocked contract ID * fix: resolve eslint errors in decoder.ts --------- Co-authored-by: Stephan-Thomas <xevermontes8@gmail.com> Co-authored-by: AbdulmalikAlayande <114596864+AbdulmalikAlayande@users.noreply.github.com>
* feat(core): build ledger entry key decoder * fix: use valid base32 string for mocked contract ID * fix: resolve eslint errors in decoder.ts --------- Co-authored-by: Stephan-Thomas <xevermontes8@gmail.com> Co-authored-by: AbdulmalikAlayande <114596864+AbdulmalikAlayande@users.noreply.github.com>
Co-authored-by: Abdulazeem-code <olamilekanabdulazeem@gmail.com>
Co-authored-by: Abdulazeem-code <olamilekanabdulazeem@gmail.com>
…egoLabs#307) - Add unit tests covering all validation paths (invalid TTL, threshold >= target, missing keypair-env for --auto-extend, dry-run edge cases) - Add integration tests verifying acceptance criteria: - --auto-extend registers extension policy in SQLite - Only public key (not secret) is stored in DB; keypair_source stores env var reference
…egoLabs#307) - Add unit tests covering all validation paths (invalid TTL, threshold >= target, missing keypair-env for --auto-extend, dry-run edge cases) - Add integration tests verifying acceptance criteria: - --auto-extend registers extension policy in SQLite - Only public key (not secret) is stored in DB; keypair_source stores env var reference
….test.ts (TegoLabs#575) Verified locally: lint, typecheck, full suite (1039/1039), build, and audit all clean. Closes TegoLabs#368.
… tests (TegoLabs#553) Type-only changes, no behavior change. Verified locally: lint, typecheck, full suite (1039/1039), build, and audit all clean. Closes TegoLabs#362.
Add two tests in the daemon loop Maintenance section: - Verify vacuum skipped during active transaction does NOT update lastVacuumAt, so the next tick retries immediately after rollback - Verify successful vacuum DOES update lastVacuumAt, preventing premature re-vacuum within the interval Closes TegoLabs#466
…imulation_cache.test (TegoLabs#551) Relocates src/alerts/keys.test.ts (misplaced outside the vitest.config.ts include glob) to tests/alerts/keys.test.ts, and deletes the orphaned src/alerts/simulation_cache.test.ts (a self-contained duplicate reimplementation of a SimulationCacheManager, never wired to the real module). Verified locally: lint, typecheck, full suite (1042/1042), build, and audit all clean. Closes TegoLabs#357, TegoLabs#359.
Every `any` cast replaced with a precisely-named interface (ErrorLike, SorobanMetaLike, SimulateWithCost, SendTransactionErrorResult, GetTransactionRawFields); reviewed the full diff line-by-line, every runtime property access/behavior is preserved exactly. Verified locally: lint, typecheck, full suite (1042/1042, including all 66 rpc tests), build, and audit all clean. Closes TegoLabs#360.
…med to scope) Adds README.pt.md and a language-switcher link in README.md, per TegoLabs#476. Trimmed from the original PR before merging: - src/core/costs.ts / tests/core/costs.test.ts (an unrelated fee-trend cost-forecasting feature — that's issue TegoLabs#498's scope, not TegoLabs#476's) - README/README.pt.md badge blocks and tests/docs/readme-badges.test.ts (badges are issue TegoLabs#488's scope; the URLs also pointed at the contributor's own fork - github.com/OlaBakare/sorokeep - not this repo, which would have shown broken/wrong CI and license badges on the real README) The translation itself (README.pt.md, tests/docs/readme-portuguese.test.ts) is unchanged and verified: 56/56 tests pass.
…abs#600, trimmed to scope) Adds schema-version validation, --merge mode, and isDatabaseEmpty guard to the existing export/import utility (from TegoLabs#125), per TegoLabs#387. Trimmed from the original PR before merging: - --group filtering across status/costs/alerts commands, and the getContractsInGroup stub in repositories.ts — that's issue TegoLabs#397's scope (a separate PR, TegoLabs#602, already covers it), and the stub always returns an empty array since the contract_groups table (TegoLabs#394) hasn't landed yet. Merging it as-is would have shipped a --group flag that silently does nothing. Also fixed two real test bugs found during verification: - tests/db/backup.test.ts's atomic-rollback test corrupted a row with `id: null` expecting a constraint violation, but `contracts.id` is `TEXT PRIMARY KEY` without an explicit `NOT NULL` - SQLite allows a NULL non-INTEGER primary key, so nothing ever threw. Changed the corruption to a duplicate id within the batch, which genuinely violates PRIMARY KEY uniqueness. - tests/commands/db.test.ts's existing import test didn't expect the new third `{ mode }` argument now passed to importDatabase.
…n (fixup for TegoLabs#518) 002_contract_groups.sql (this PR) and 002_quiet_hours.sql (merged separately as part of TegoLabs#528) both claimed migration version 2 - schema_migrations.version is an INTEGER PRIMARY KEY, so only one file can ever legitimately own that slot. It happens not to crash today only by accident: quiet_hours_start/end/ timezone are also declared directly in schema.sql (not just the migration file), so 002_quiet_hours.sql's ADD COLUMN statements fail with "duplicate column name" regardless of which file ran first, which the Migrator's existing idempotency catch already swallows. That's a coincidence, not a guarantee - the next migration to collide on a shared version number without that same redundant schema.sql declaration would crash getDatabase() for every fresh install. Renumbered to 003_contract_groups.sql.
Extracts CLI construction into createProgram() (src/cli/program.ts) so scripts/generate-man.ts can introspect the Command tree without running the CLI, generates man/sorokeep.1, and wires build:man into npm run build. Per TegoLabs#384. Trimmed from the original PR before merging: - A full set of unrelated ".claude/agents/kfc/*", ".claude/settings/ kfc-settings.json", and ".claude/system-prompts/spec-workflow- starter.md" files - generic AI-agent workflow scaffolding with no connection to sorokeep or this issue. - getContractsByTag() and related changes to src/commands/status.ts / src/db/repositories.ts - that's issue TegoLabs#379's scope (--tag filtering), a different, unrelated feature. The PR's branch was also quite stale (predated the quiet-hours, contract_groups, rpc/client.ts any-type, and CI audit-scope fixes already on main), which produced a large raw diff and one real merge conflict in src/index.ts (this PR's createProgram() refactor vs. the already-merged --extension-jitter-ms option). Resolved by moving the jitter option registration into createProgram() so both are preserved.
…s#548) Relocates the misplaced test to tests/alerts/ (outside vitest.config.ts's include glob otherwise) and rewrites it against the current SlackChannel class / sendPagerDutyAlert function API - the old copy called a removed sendSlackAlert function.
|
hey maintainer, whats delaying you from merging this pr so that i can earn my points |
…erting # Conflicts: # src/core/extension.ts # tests/core/extension.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/db/repositories.ts (1)
372-383: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExpose
drift_ledgersinExtensionRecord.
recordExtensionnow persists this value, andgetExtensionHistoryreturnsExtensionRecord[]fromSELECT *. Adddrift_ledgers: number | nulltoExtensionRecord. Otherwise, typed callers cannot read the drift history required by this feature.🤖 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 372 - 383, Update the ExtensionRecord type to include drift_ledgers as a nullable number, matching the value persisted by recordExtension and returned by getExtensionHistory. Keep the existing record shape and nullability conventions unchanged.
🤖 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 `@tests/core/extension.test.ts`:
- Line 814: Resolve the Git conflict markers in the test source around the
affected sections, including the blocks near the referenced locations. Choose
and merge the required test changes, remove all<<<<<<<, =======, and >>>>>>>
markers, and preserve a valid test structure that parses and runs.
---
Outside diff comments:
In `@src/db/repositories.ts`:
- Around line 372-383: Update the ExtensionRecord type to include drift_ledgers
as a nullable number, matching the value persisted by recordExtension and
returned by getExtensionHistory. Keep the existing record shape and nullability
conventions unchanged.
🪄 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: 4cc264f7-9ae1-41f1-bd33-70f06aba0558
📒 Files selected for processing (3)
src/core/extension.tssrc/db/repositories.tstests/core/extension.test.ts
📜 Review details
🧰 Additional context used
🪛 Biome (2.5.5)
tests/core/extension.test.ts
[error] 814-820: Expected a statement but instead found '<<<<<<< HEAD
// =========================================================================
// TTL Drift Alerting (issue `#495`)
// =========================================================================
it("does not fire a drift alert when actual TTL is within tolerance", async () =>'.
(parse)
[error] 944-945: Expected a statement but instead found '=======
it("with jitter disabled (default), submission timing is unchanged from current behavior", async () =>'.
(parse)
[error] 974-974: Expected a statement but instead found ')'.
(parse)
[error] 1012-1012: Expected a statement but instead found '>>>>>>> origin/main'.
(parse)
🔇 Additional comments (1)
src/core/extension.ts (1)
213-219: Compute and persist drift for each entry.The aggregate maximum TTL still hides an under-extended entry. For example, entry drifts of
-2000and+50report+50and store that value for both history rows. ComputefreshEntry.remainingTTL - extendToLedgersfor each entry. Use the signed value with the largest absolute value for the extension result and alert.Also applies to: 239-239
| const anomaly = history.find(h => h.tx_hash === "anomaly-tx"); | ||
| expect(anomaly!.is_anomaly).toBe(1); | ||
| }); | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Resolve the Git merge conflict.
<<<<<<< HEAD, =======, and >>>>>>> origin/main are present in the test source. The file cannot parse, so the test suite cannot run. Select the required test blocks, remove all conflict markers, and keep the resulting test structure valid.
Also applies to: 944-944, 1012-1012
🧰 Tools
🪛 Biome (2.5.5)
[error] 814-820: Expected a statement but instead found '<<<<<<< HEAD
// =========================================================================
// TTL Drift Alerting (issue `#495`)
// =========================================================================
it("does not fire a drift alert when actual TTL is within tolerance", async () =>'.
(parse)
🤖 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` at line 814, Resolve the Git conflict markers
in the test source around the affected sections, including the blocks near the
referenced locations. Choose and merge the required test changes, remove
all<<<<<<<, =======, and >>>>>>> markers, and preserve a valid test structure
that parses and runs.
Source: Linters/SAST tools
43ad363 to
8692701
Compare
Closes #495
Summary
Implements TTL-drift alerting as described in issue #495. After every successful auto-extension, the actual post-extension TTL is compared against the policy's
target_ttl_ledgers. When the absolute delta exceeds a configurable tolerance, exactly onettl_driftalert is fired per extension through the existing dispatcher — no new delivery infrastructure needed.Changes
src/alerts/types.tsTTLDriftAlertEventinterface (type: "ttl_drift") with fields:targetTTLLedgers,actualTTLLedgers,driftLedgers,toleranceLedgers,txHash,detectedAtLedgerbuildTTLDriftAlertEventbuilder functionAlertEventunion andAlertEventTypeto include"ttl_drift"src/core/extension.tsDEFAULT_DRIFT_TOLERANCE_LEDGERS = 100constant (exported)driftToleranceLedgersparameter torunAutoExtensions(default:DEFAULT_DRIFT_TOLERANCE_LEDGERS)driftLedgers?: numberfield toExtensionResultandAutoExtensionResultmaxActualTTL − target_ttl_ledgers, and firesbuildTTLDriftAlertEvent+deliverSingleAlertfor each alert config when|drift| > driftToleranceLedgersbuildTTLDriftAlertEventfromalerts/types.tsanddeliverSingleAlertfromalerts/dispatcher.tsgetAlertConfigsForContractfromdb/repositories.tssrc/db/migrations/002_extension_history_drift_ledgers.sqlALTER TABLE extension_history ADD COLUMN drift_ledgers INTEGER— stores the signed ledger delta for each extension record (NULL for pre-migration rows)tests/core/extension.test.tsrunAutoExtensionsdescribe block covering the acceptance criteria:deliverSingleAlertcall|driftLedgers|correctly exceeds toleranceTest Results
All pre-existing tests continue to pass.
Acceptance Criteria
Scope
Only the files listed in the issue were touched:
src/alerts/types.ts— extended, not redesignedsrc/core/extension.ts— post-extension comparison onlysrc/db/migrations/— new migration file addedsrc/alerts/dispatcher.tsandsrc/alerts/registry.tswere not modified; drift alerts flow through the existing dispatcher unchanged.cloese #495