Repository navigation
Feat/495 ttl drift alerting v2 - #664
Gabugo-tech wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 30 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe extension workflow now computes signed TTL drift after successful extensions, stores the value in TTL drift alerting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant runAutoExtensions
participant extendEntries
participant ExtensionHistory
participant AlertDispatcher
participant Webhook
runAutoExtensions->>extendEntries: execute extension and fetch fresh TTL
extendEntries->>extendEntries: calculate signed TTL drift
extendEntries->>ExtensionHistory: persist drift_ledgers
extendEntries-->>runAutoExtensions: return driftLedgers
runAutoExtensions->>runAutoExtensions: compare drift with tolerance
runAutoExtensions->>AlertDispatcher: dispatch ttl_drift event when excessive
AlertDispatcher->>Webhook: send configured alert
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…m policy target (TegoLabs#495) - Add TTLDriftAlertEvent interface and buildTTLDriftAlertEvent builder to alerts/types.ts - Extend AlertEvent union and AlertEventType with 'ttl_drift' - Add DEFAULT_DRIFT_TOLERANCE_LEDGERS = 100 constant to extension.ts - Add driftToleranceLedgers + channels params to runAutoExtensions - Add driftLedgers field to ExtensionResult and AutoExtensionResult - Compute and persist drift_ledgers in extendEntries alongside extension record - Add drift_ledgers to recordExtension in repositories.ts - Post-extension drift check fires TTLDriftAlertEvent when abs(drift) > tolerance - Migration 002: ALTER TABLE extension_history ADD COLUMN drift_ledgers INTEGER - Tests: within-tolerance no alert; outside-tolerance exactly one alert per config
2cb4e66 to
444c2b9
Compare
|
@AbdulmalikAlayande please come and review this pr and merge it |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/alerts/types.ts`:
- Line 4: Update the alert routing and rendering logic for the "ttl_drift"
AlertEventType, especially deliverSingleAlert and the relevant helpers in
types.ts and templates.ts, so TTLDriftAlertEvent uses its drift-specific entry
label, template flags, isTTLAlert classification, and custom/template fields
instead of generic fallbacks. Preserve existing behavior for all other alert
event types and ensure every configured channel receives the populated TTL-drift
data.
In `@src/core/extension.ts`:
- Around line 254-260: Update the drift handling in extendEntries: calculate and
persist each entry’s signed drift against extendToLedgers in its corresponding
history row instead of using the aggregate Math.max result. Track the per-entry
drift with the largest absolute value solely for the extension-level alert,
preserving signed values in storage. Add a mixed-entry regression test verifying
both per-entry stored drifts and that the alert is emitted.
In `@tests/core/extension.test.ts`:
- Around line 887-1003: The drift-alert tests only cover successful delivery;
add a rejected-delivery case around runAutoExtensions using the existing setup
and mockChannel, with send configured to reject. Assert the completed extension
remains successful by checking contractsExtended, entriesExtended, and
extensions still report the extension, while preserving the existing drift
behavior.
🪄 Autofix
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: d2c1c3ed-6972-4231-999d-f532c4f3e79e
📒 Files selected for processing (5)
src/alerts/types.tssrc/core/extension.tssrc/db/migrations/002_extension_history_drift_ledgers.sqlsrc/db/repositories.tstests/core/extension.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: GitGuardian Security Checks
🧰 Additional context used
🪛 Squawk (2.61.0)
src/db/migrations/002_extension_history_drift_ledgers.sql
[warning] 7-7: Using 32-bit integer fields can result in hitting the max int limit. Use 64-bit integer values instead to prevent hitting this limit.
(prefer-bigint-over-int)
🔇 Additional comments (7)
src/alerts/types.ts (1)
199-256: LGTM!src/core/extension.ts (3)
16-34: LGTM!Also applies to: 90-91, 106-106
318-319: 🗄️ Data Integrity & IntegrationNo change needed.
Current callers do not pass a channel map in the
driftToleranceLedgersposition; channel maps are passed after that argument, so existing call sites do not have this drift-alert suppression path.> Likely an incorrect or invalid review comment.
479-489: 🔒 Security & PrivacySensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability path
● Entry tests/core/extension.test.ts │ ▼ ● Sink src/core/extension.tsVerify secret-safe delivery for the new alert path.
cfg.channel_targetandcfg.webhook_secretflow fromalert_configsthroughdeliverSingleAlerttoAlertChannel.send. If an attacker can configure an alert and a channel permits HTTP or forwards the secret after a redirect, that endpoint receives the secret. The only supplied control resolves a channel type. Verify destination authorization, HTTPS enforcement, redirect handling, and secret forwarding in each channel implementation.#!/bin/bash set -euo pipefail ast-grep outline src/alerts --items all --type class,function rg -nP -C 8 \ '\b(send|channel_target|webhook_secret|redirect|Authorization|signature)\b|https?://' \ src/alerts rg -nP -C 8 \ '\b(insertAlertConfig|updateAlertConfig|deleteAlertConfig|channel_target|webhook_secret)\b' \ srctests/core/extension.test.ts (1)
6-6: LGTM!src/db/migrations/002_extension_history_drift_ledgers.sql (1)
7-7: 🩺 Stability & AvailabilityNo change needed.
Fresh and test databases apply
getDatabaseForTesting()and migrate through the migrations directory beforerecordExtensioncan write. Existing databases also run migrations from that directory before auto-extension writes.src/db/repositories.ts (1)
410-421: 🗄️ Data Integrity & IntegrationNo read-contract change needed.
getExtensionHistory()returns raw extension-history rows viaSELECT *, andExtensionRecordalready contains all existing columns, includingdrift_ledgers.
| // Compute drift here so it can be persisted alongside the extension record. | ||
| let driftLedgers: number | undefined; | ||
| if (freshTTLs.entries.length > 0) { | ||
| const maxActualTTL = Math.max(...freshTTLs.entries.map(e => e.remainingTTL)); | ||
| driftLedgers = maxActualTTL - extendToLedgers; | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Compute and persist drift per entry before aggregate alerting.
extendEntries can process multiple entries. Math.max selects the healthy entry TTL and writes its aggregate drift to every history row. If one entry is 1,000 ledgers short and another reaches the target, this code stores 0 for both and sends no alert. Calculate each row’s signed drift. Use the largest absolute per-entry drift only for the one extension-level alert. Add a mixed-entry regression test that checks both stored values and the alert.
Proposed fix
- let driftLedgers: number | undefined;
- if (freshTTLs.entries.length > 0) {
- const maxActualTTL = Math.max(...freshTTLs.entries.map(e => e.remainingTTL));
- driftLedgers = maxActualTTL - extendToLedgers;
- }
+ const driftLedgers = freshTTLs.entries.reduce<number | undefined>(
+ (worst, freshEntry) => {
+ const candidate = freshEntry.remainingTTL - extendToLedgers;
+ return worst === undefined || Math.abs(candidate) > Math.abs(worst)
+ ? candidate
+ : worst;
+ },
+ undefined,
+ );
@@
- drift_ledgers: driftLedgers ?? null,
+ drift_ledgers: freshEntry.remainingTTL - extendToLedgers,Also applies to: 280-280, 309-309
🤖 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 254 - 260, Update the drift handling in
extendEntries: calculate and persist each entry’s signed drift against
extendToLedgers in its corresponding history row instead of using the aggregate
Math.max result. Track the per-entry drift with the largest absolute value
solely for the extension-level alert, preserving signed values in storage. Add a
mixed-entry regression test verifying both per-entry stored drifts and that the
alert is emitted.
) - templates.ts: add ttl_drift branch for entryLabel, dedupKey, customDetails, and isDriftAlert template flag; guard entryLabel assignment with type check - extension.ts: compute per-entry drift in recordExtension rows instead of aggregate Math.max; track largest-absolute-value drift for the alert - tests: add mixed-entry regression test verifying per-entry stored drifts and correct alert drift value; add rejected-delivery test verifying that a failed alert dispatch does not surface as an extension failure
1 participant
feat(core): implement TTL-drift alerting when actual TTL diverges from policy target
Closes #495
Summary
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 one ttl_drift alert is fired per extension through the existing dispatcher — no new delivery infrastructure required.
Changes
types.ts
Added TTLDriftAlertEvent interface (type: "ttl_drift") with fields: targetTTLLedgers, actualTTLLedgers, driftLedgers, toleranceLedgers, txHash, detectedAtLedger
Added buildTTLDriftAlertEvent builder function
Extended AlertEvent union and AlertEventType to include "ttl_drift"
discord.ts
/ slack.ts / telegram.ts / pagerduty.ts / opsgenie.ts / templates.ts
Added explicit ttl_drift branches in all channel formatters so the new event type renders correctly and TypeScript does not error on missing entry/threshold fields
extension.ts
Added DEFAULT_DRIFT_TOLERANCE_LEDGERS = 100 constant (exported)
Added driftToleranceLedgers param to runAutoExtensions (default: 100)
Added optional channels param to runAutoExtensions for test injection
Added optional driftLedgers?: number to ExtensionResult and AutoExtensionResult
Drift is computed inside extendEntries right after getEntryTTLs and persisted with the extension record
Post-extension drift-check in runAutoExtensions fires TTLDriftAlertEvent via deliverSingleAlert when |drift| > tolerance, isolated in its own try/catch so delivery failures never roll back a completed extension
repositories.ts
Added drift_ledgers?: number | null to the recordExtension contract and INSERT statement
002_extension_history_drift_ledgers.sql
ALTER TABLE extension_history ADD COLUMN drift_ledgers INTEGER
extension.test.ts
Added insertAlertConfig import
Two new tests in the runAutoExtensions describe block:
TTL within tolerance (drift=50, tolerance=100) → no send call
TTL outside tolerance (drift=−2000, tolerance=100) → exactly one send call with correct ttl_drift payload
Acceptance Criteria
A resulting TTL within tolerance of the target does not fire a drift alert
A resulting TTL outside tolerance fires exactly one drift alert per extension
Scope
Only the files described in issue #495 were touched.
dispatcher.ts
and
registry.ts
were not modified — drift alerts flow through the existing dispatcher unchanged.
closes #495