Repository navigation
Conversation
# Conflicts: # src/core/extension.ts # src/rpc/client.ts # tests/core/extension.test.ts
…alone extension run
…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.
…ted) Adds src/alerts/matrix.ts (real Matrix Client-Server API, token auth, room-scoped delivery) and its tests, per TegoLabs#310. The PR never registered the channel in builtins.ts, so `--type matrix` was unreachable from the CLI despite the sender being fully implemented and tested. Completed the registration (targetOption: "channel", since the sender's target is a Matrix room ID) plus the matching builtins.test.ts coverage, following the exact pattern used by every other lazily-imported channel (discord/telegram/opsgenie). Verified `alerts add --type matrix` end-to-end at the CLI.
… fixed + completed) Adds src/alerts/teams.ts (Adaptive Card payload, severity coloring per event type) and its tests, per TegoLabs#311. Two things fixed before merging: - CodeRabbit correctly flagged validateWebhookUrl()'s hostname check: `hostname.includes("webhook.office.com")` accepts any hostname that merely contains that substring, e.g. an attacker-controlled "x.webhook.office.com.evil.com" would pass, silently sending real alert content (contract IDs, TTL data) to an attacker-controlled server. Changed to an exact/suffix match on the real hostname, plus an explicit https-only check. Added regression tests for both. - The PR never registered the channel in builtins.ts, so `--type teams` was unreachable from the CLI. Completed the registration (targetOption: "url", matching webhook/discord's pattern) and the matching builtins.test.ts coverage.
…#585, fixed + completed) Adds src/alerts/email.ts (nodemailer SMTP transport, env/config token resolution, password redaction on error) and its tests, per TegoLabs#312. Fixed before merging: - Bumped nodemailer ^7.0.7 -> ^9.0.3 (major version, verified tsc still compiles clean and all tests pass unchanged): the pinned 7.x range had six high-severity advisories, including SMTP/CRLF command injection and an SSRF via the raw-message option. This is a production dependency, so `npm audit --omit=dev` would have failed on it. - The channel was registered in builtins.ts, but src/commands/alerts.ts still had a special-cased `--type email` branch printing "Email alerting is not yet implemented" *before* the registry was ever consulted - making the new channel completely unreachable from the CLI. Removed the special case. - Updated the one existing test that asserted the old "not implemented" behavior, and added a real success-path test (registers a config with --channel <email>). - Fixed a lint error (preserve-caught-error) the right way: the error handler already redacts the SMTP password from the thrown message, but attaching the raw caught error as `cause` would have smuggled the unredacted password back in via the cause chain. Redact the caught error's own message in place before using it as cause, so the guarantee holds through the whole chain.
…s#597, trimmed to scope) Adds src/alerts/googlechat.ts and registers it in builtins.ts, per TegoLabs#313. Registration and the sender itself were already correct. Trimmed from the original PR before merging: a bundled Grafana/Prometheus observability stack (devops/grafana/*, devops/prometheus/*, docker-compose. observability.yml, docs/observability.md) - unrelated to this issue, shared branch lineage with several other open PRs (TegoLabs#594, TegoLabs#595, TegoLabs#596) that also carry the identical bundle. Left tests/docker/docker-compose.test.ts untouched by reverting to main's version. Updated tests/alerts/builtins.test.ts for the 10th channel (the PR's branch predated matrix/teams/email, so its own copy of this file didn't know about them). Note for a follow-up: src/alerts/discord.ts has the same hostname- validation weakness fixed in TegoLabs#557 (`hostname.includes("discord")`, even looser than Teams's check) - pre-existing, not part of this PR, flagging separately.
…egoLabs#318) PR TegoLabs#568 referenced ChannelDefinition.maxRetries in dispatcher.ts but never added the field to the interface, so the branch failed to compile (TS2339). It also shipped a new dispatcher test that called registerAlertChannel("webhook", { maxRetries: 2 }) — a two-argument overload that doesn't exist on the real one-argument, throw-on-duplicate registerAlertChannel API, so the test could never have run against this codebase. - Add optional maxRetries/retryBackoffMs to ChannelDefinition (registry.ts); dispatcher.ts's lookup already existed from the merge and needed no further changes. - Give telegram a maxRetries: 3 default, matching the issue's own rationale (Telegram's Bot API rate limits are stricter than a generic webhook's) — every other channel keeps the global MAX_RETRY_COUNT default, preserving existing behavior exactly. - Rewrite the broken test to swap in a real ChannelDefinition override via the actual registry API, and restore the registry to normal built-in state afterward so it doesn't leak into other tests. Added a companion test asserting webhook has no override by default. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # package-lock.json
…nnels' into verify-555
…egoLabs#555) Issue TegoLabs#319's scope explicitly excludes the orphaned src/alerts/*.test.ts files ("reconciling those is phase-8's job, not this issue's") — they aren't in vitest.config.ts's include glob (tests/**/*.test.ts), so any tests added there never actually run. Keeping the docs update and the contract suite applied to webhook/slack, whose real test files live under tests/alerts/. Applying the contract suite to pagerduty/discord/ telegram is deferred to their dedicated orphan-reconciliation issues (TegoLabs#354-TegoLabs#356).
…to-preview-payload-without-sending-FIX' of https://github.com/veloura-dev/sorokeep into verify-582
…b/sorokeep into verify-536 # Conflicts: # src/commands/alerts.ts
…yStats (TegoLabs#536) The PR's src/db/repositories.ts shipped literal backslash-escaped backticks (\`...\`) instead of real template literals in the days-filter branch of getChannelDeliveryStats — invalid TypeScript syntax that made the entire file fail to parse (tsc reported ~30 cascading errors past this point, and CI's build-and-test check was correctly failing). Replaced with proper template literals. Also tightened the query params array from any[] to Array<number | string>. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…bs#541, TegoLabs#323) PR TegoLabs#541 added fan-out alert delivery (multiple targets per alert_config) but shipped several real bugs that would have made it silently broken in production: - src/db/migrations/002_alert_config_targets.sql collided with the already-merged 002_quiet_hours.sql. Since the Migrator tracks applied migrations by numeric version only, this table (and the new alerts_fired.channel_type/channel_target columns) would never have been created on any real install — renamed to 004. - insertAlertConfig's .run() result was never captured (referenced an undefined `info`) and its return type was left as `void`, despite the new code needing the inserted row's id to attach additional targets — this didn't even compile. - The CLI's `alerts add` action collected --target flags into additionalTargets but never called addTargetToAlertConfig for them — the entire fan-out feature was unreachable from the CLI; only the first target (used as primary) ever got persisted. - The success message referenced undefined variables (`target`, `options.type` instead of `primaryTarget`/`primaryType`) — TS2304. - The import block dropped `deleteAlertConfig`, breaking `alerts remove` — TS2304. - A stale "email not implemented" special-case (from before TegoLabs#585) resurfaced from the PR's outdated base and would have made the now fully-supported email channel unreachable via --type email again. Fixed all of the above, tightened insertAlertConfig's webhook_secret type to allow null (matching how the new tests call it), added CLI-level tests proving --target actually persists additional targets end-to-end, and added a test for the previously-untested removeTargetFromAlertConfig. Verified: tsc clean, full suite 1199/1199 passing, npm audit clean, build succeeds, and manually smoke-tested `alerts add --target ...` against the compiled CLI — confirmed both additional targets are correctly persisted to alert_config_targets. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…bs#326) Merged PR TegoLabs#580's alert_configs.enabled column and alerts enable/disable commands, with fixes: - src/db/migrations/002_add_enabled_to_alert_configs.sql collided with the already-merged 002_quiet_hours.sql — renamed to 005 (numbers 001-004 are now taken). Same landmine class as TegoLabs#518/TegoLabs#541: the Migrator tracks applied migrations by numeric version only, so this would have silently never run on any real install. - tests/commands/alerts.test.ts had a corrupted "alerts remove" test: a full "alerts enable / disable" describe block had been pasted into the middle of "deletes the alert config from the DB", orphaning the rest of that test's body outside any it() block. Restored the original test and kept the separate, correctly-scoped "alerts enable / disable" describe block that already existed later in the file. - Fixed monitor.ts merge indentation (tabs/spaces mixed from the 3-way merge). - Added monitor-level tests for the actual acceptance criteria — "a disabled alert_config does not fire even when its threshold is crossed" and "re-enabling resumes normal detection" — which had no coverage; the PR's own tests only checked the CLI toggle in isolation, not the monitor.ts filtering behavior it exists to drive. Verified: tsc clean, full suite 1210/1210, npm audit clean, build succeeds, and manually smoke-tested `alerts add` → `alerts disable` → `alerts enable` end-to-end against the compiled CLI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eremiah24/sorokeep into verify-535 # Conflicts: # tests/alerts/builtins.test.ts
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (2)
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 |
- Add getCurrentSchemaVersion() that reads MAX(version) from schema_migrations - Expand DatabaseBackup interface to cover all 15 schema tables: alerts_fired, extension_history, cost_daily_snapshots, state_snapshots, state_changes, budgets, resource_alerts_fired, contract_budgets, resource_usage_logs (in addition to the existing 6) - Add schema_version stamp to exportDatabase() output - Add version mismatch guard in importDatabase() to prevent restoring a newer backup onto an older schema - Expand EXPORT_TABLES, CLEAR_TABLES, and TABLE_COLUMNS accordingly - Use backtick-quoted column names to handle reserved words (limit) - Expand backup.test.ts from 2 to 15 tests covering every table with round-trip assertions Fixes: ReferenceError: getCurrentSchemaVersion is not defined (12 tests in backup.test.ts + 1 test in commands/db.test.ts = 13 tests total)
temi-Dee
force-pushed
the
fix/backup-schema-version-all-tables
branch
from
August 1, 2026 10:11
0fda752 to
28f0290
Compare
AbdulmalikAlayande
force-pushed
the
main
branch
from
August 2, 2026 04:34
43ad363 to
8692701
Compare
AbdulmalikAlayande
added a commit
that referenced
this pull request
Aug 7, 2026
) src/db/backup.ts previously exported only 6 of the schema's tables. Extends EXPORT_TABLES/CLEAR_TABLES/TABLE_COLUMNS to cover every restorable table, in FK-safe insert/clear order: contracts, channel_accounts, contract_groups, contract_entries, extension_policies, alert_configs, resource_alert_configs, cost_daily_snapshots, budgets, contract_budgets, resource_usage_logs, contract_group_members, alerts_fired, extension_history, state_snapshots, state_changes, resource_alerts_fired. Large tables (state_snapshots, state_changes, resource_usage_logs) are read in 1,000-row pages via selectTable() rather than loaded whole, so export doesn't materialize an entire large table in memory. INSERT column lists are now double-quote-identifier-escaped uniformly, which also fixes resource_alerts_fired's "limit" column (a SQLite reserved word) without a special case. Also fixed a pre-existing gap unrelated to the 6-table scope: the `contracts` row's TABLE_COLUMNS list was missing poll_interval_seconds and active — both real columns in schema.sql that were silently dropped by every prior export. Reimplemented from PR #621 (temi-Dee) rather than merged directly: both #621 and the same author's follow-up PR #653 were 1100+ files behind current main, based on a version of backup.ts that predates this session's getCurrentSchemaVersion()/schema_version work (already on main) — so neither patch applied cleanly. Also, the issue's own "15 tables" count is stale: main now has 17 real data tables, since contract_groups/contract_group_members were added by separate, already-merged work. Rebuilt the table/column lists directly against current schema.sql (17 tables, not 15) rather than the PR's list. Verified with 18 tests in tests/db/backup.test.ts (one per new table plus a full 17-table round-trip and a 1,500-row pagination test), full suite (116 files / 1500 tests), tsc --noEmit, lint, build, npm audit, and a manual smoke test confirming `sorokeep db export` includes all 17 tables via the built CLI.
Collaborator
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
17 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.
Summary
Fixes 13 failing tests caused by
getCurrentSchemaVersion is not definedinsrc/db/backup.ts.Root cause
importDatabasewas callinggetCurrentSchemaVersion(db)at runtime but the function had never been defined inbackup.ts. This caused:tests/db/backup.test.tsto fail withReferenceError: getCurrentSchemaVersion is not definedtests/commands/db.test.ts(db import reads a JSON file and restores it) to fail because the error propagated through toprocess.exit(1)Changes
src/db/backup.tsgetCurrentSchemaVersion(db)— readsMAX(version)fromschema_migrations, returns 0 if the table doesn't exist yetschema_versionto theDatabaseBackupinterface andexportDatabase()outputimportDatabase(): throws if the backup'sschema_versionis newer than the current DB (prompts the user to runsorokeep db migratefirst)EXPORT_TABLES,CLEAR_TABLES, andTABLE_COLUMNSto cover all 15 schema tables (previously only 6 were exported)alerts_fired,extension_history,cost_daily_snapshots,state_snapshots,state_changes,budgets,resource_alerts_fired,contract_budgets,resource_usage_logslimitinresource_alerts_fired)tests/db/backup.test.tsschema_versionmarker presence, no raw secret key columns, full 15-table round-tripTesting
Full suite: 1008 passed, 9 pre-existing
getContractsByTagfailures (unrelated, present before this change).