Repository navigation
build: pin postcss to resolve high-severity path-traversal advisory - #655
francisprecious061 wants to merge 940 commits into
Conversation
# Conflicts: # src/core/extension.ts # src/rpc/client.ts # tests/core/extension.test.ts
…alone extension run
…missing commitBudget rename
…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
TegoLabs#546, TegoLabs#328) PR TegoLabs#546 implemented this correctly in spirit (repeatable --channel-plugin flag, dynamic import() of a package's default-exported registration function, clear startup error on failure) but the branch bundled 60+ unrelated files — stray coverage/lint-report artifacts, eslint/vitest config changes, package.json/package-lock changes, and edits across dozens of unrelated core/utils/rpc test files — and its src/index.ts diff targeted a version of the CLI entry point that predates the createProgram()/src/cli/program.ts refactor (TegoLabs#622), so it couldn't be merged as-is. Reimplemented the same feature directly against the current CLI structure, giving credit to TegoLabs#546 for the design (the plugin convention — default-export a function receiving registerAlertChannel — and the disk-based integration test approach that writes a real package into node_modules and dynamically imports it): - src/cli/program.ts: --channel-plugin <package> (repeatable), loaded via a preAction hook before any command's action runs, scoped per createProgram() call (not module-level state) so it stays correctly isolated across multiple program instances in tests. - src/index.ts: switched to parseAsync since the hook is now async. - src/lib.ts: export registerAlertChannel/ChannelDefinition so plugin packages can type against sorokeep's public API. - docs/adding-an-alert-channel.md: new "External plugin package convention" section. - tests/commands/channel_plugin_flag.test.ts: writes a real plugin package to node_modules, loads it via createProgram().parseAsync(), and asserts the channel becomes available — plus repeated-flag and missing-package-error cases. Verified: tsc clean, full suite 1248/1248, npm audit clean, build succeeds, and manually smoke-tested against the compiled CLI binary in an isolated directory — both the success path (plugin channel appears in `alerts channels`) and the failure path (clear error, exit 1) work end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…egoLabs#329) PR TegoLabs#576 implemented the right table for the README's Alerting section but the branch also touched src/commands/guard.ts, src/core/fleet.ts, src/observability/server.ts and their tests — the issue explicitly scoped this as README-only ("do not touch any src/ files"), and those changes were unrelated to this issue, so I didn't bring the branch in. Applied just the table (updated to reflect all 11 channels currently registered — the PR's version only listed 5, written before opsgenie, teams, matrix, email, googlechat, and webhook2 landed), added the matching section to README.pt.md to keep the Portuguese-parity test passing, and added a test that fails if the table and the registry ever drift apart again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TegoLabs#330) Wired up the Hono-based /metrics HTTP server this PR added, reconciled against the prom-client-based registry.ts that a separately-merged budget-gauge PR had already introduced (that PR only did the "collect into a registry" half; this scaffold was still needed for the "expose over HTTP" half): - Kept registry.ts's real prom-client Registry (with the budget gauge already registered) instead of the PR's own placeholder registry.ts, which would have silently discarded the working metric. - server.ts's /metrics route now calls register.metrics() instead of the placeholder renderMetrics() the PR wrote before real metrics existed. - Combined package.json (hono + the already-merged nodemailer/ @types/nodemailer entries). - Minor lint fix (unused catch binding). Verified: tsc clean, full suite 1257/1257, npm audit clean, build succeeds, and manually smoke-tested `sorokeep daemon --metrics-port` end-to-end — confirmed the real sorokeep_budget_remaining_xlm metric is served correctly through the new HTTP endpoint. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…est/sorokeep into verify-527 # Conflicts: # package-lock.json # package.json # src/observability/registry.ts
…TegoLabs#527, TegoLabs#331) PR TegoLabs#527 correctly implemented the TTL gauge and its computation function (setEntryTtlGaugeSamples), but nothing ever called it in real daemon operation — a manual smoke test against the compiled CLI showed the endpoint always returned only HELP/TYPE comments, never actual samples. Tracing it further, the same gap already existed for the budget gauge from an earlier-merged PR (collectBudgetRemaining was never called anywhere outside its own tests either) — this is a systemic issue, not specific to this PR. Also resolved a design conflict in registry.ts: the PR's own version re-exported prom-client's ambient global default register, replacing the dedicated Registry instance the budget-gauge PR had already established. Kept the dedicated instance (safer — isolates sorokeep's metrics from any dependency's own default-register usage) and added the TTL gauge's registration to it, matching the issue's own scope ("one import + one registration line"). Fixed the collection gap generically so both existing gauges (and any future ones) actually work: - registry.ts: added collectAllMetrics(db), calling every DB-backed metric's collect function. - server.ts: createMetricsServer now takes an optional db handle; the /metrics route recomputes all DB-backed metrics from it before serving, exactly once per scrape, matching issue TegoLabs#331's own "recompute on every scrape rather than caching" requirement. - daemon/loop.ts: passes its db handle through to createMetricsServer. Verified: tsc clean, full suite 1265/1265, npm audit clean, build succeeds, and manually smoke-tested `sorokeep daemon --metrics-port` end-to-end — confirmed both sorokeep_budget_remaining_xlm and sorokeep_entry_ttl_remaining_ledgers now serve real, live-computed values through the HTTP endpoint (previously neither did). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…TegoLabs#543, TegoLabs#332) PR TegoLabs#543's query/aggregation logic (zero-baseline per contract_entries row, COALESCE for NULL cost_xlm) was solid, but its registry.ts diff replaced the dedicated prom-client Registry + collectAllMetrics wiring that TegoLabs#527 had just established with a completely different architecture — a custom MetricSample[] type and a hand-rolled collector-registry, bypassing prom-client entirely ("deliberately avoids importing prom-client"). Taking that diff as-is would have silently deleted the working budget and TTL gauges. Reimplemented as prom-client Counters instead, keeping the same query logic and zero-baseline behavior, integrated into the existing registry.ts pattern: - src/observability/metrics/cost.ts: sorokeep_extension_cost_xlm_total and sorokeep_extensions_total as prom-client Counters, recomputed via reset() + inc(labels, value) each scrape (mirrors budget.ts/ttl.ts). - registry.ts: two registration lines + added to collectAllMetrics so both counters get live data on every /metrics scrape automatically, same as the other two metrics. - Rewrote the test suite to assert against the counters' own .get() and against register.metrics() exposition output (the original tests only checked a custom sample-array shape that's no longer produced), preserving every original test scenario. Verified: tsc clean, full suite 1277/1277, npm audit clean, build succeeds, and manually smoke-tested end-to-end against an isolated temp database — confirmed both counters serve correct live values (0.25 XLM / 1 extension for a seeded record) through the real HTTP endpoint. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Labs#566, TegoLabs#334) PR TegoLabs#566's registry.ts diff replaced the established prom-client Registry pattern (and would have deleted the budget/TTL/cost metrics already wired into it) with an unrelated namespace-object design, and its metrics/daemon.ts was a non-functional stub (observe/inc were both no-ops). Its daemon/loop.ts diff was based on a very old snapshot of the file and, beyond the instrumentation, would have reintroduced a real bug: resetting cycleInFlight inside stopDaemon() — exactly the re-entrance-guard race the current code deliberately avoids (see the "Do NOT reset cycleInFlight here" comment in stopDaemon) — plus it silently deleted the metrics-port wiring from TegoLabs#569, the feeSponsorSecret option, and the per-network poll-interval override. Implemented the same two metrics narrowly against the current loop.ts instead: - src/observability/metrics/daemon.ts: sorokeep_daemon_cycle_duration_seconds (Histogram) and sorokeep_daemon_cycles_skipped_total (Counter), both labeled by network to match the codebase's existing per-metric labeling convention (enforced by an existing regression test in budget.test.ts). - registry.ts: two registration lines, no changes to collectAllMetrics needed — these are event-driven (observed at the point of occurrence), not recomputed from the DB on scrape. - daemon/loop.ts: exactly the two instrumentation calls the issue asked for (histogram.observe() using the cycle's own cycleStartedAt/ cycleFinishedAt, and counter.inc() at the existing "Skipping tick" line) — no other changes to executeCycle, scheduledTick, or the re-entrance guard. - Added two tests to the existing (not replaced) loop.test.ts suite, using the real prom-client objects rather than mocks. Verified: tsc clean, full suite 1279/1279, npm audit clean, build succeeds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ges (TegoLabs#591, TegoLabs#335) PR TegoLabs#591's registry.ts used a factory-function pattern (createFleetGauges(registry), a separately-exported registry object, collectAll()) incompatible with the established register/collectAllMetrics singleton pattern other merged metrics already use — taking it as-is would have replaced the shared registry and dropped every other metric. Kept the PR's gauge logic exactly (per-network counting via getAllContracts + getEntriesForContract, reset-then-set to drop stale network labels), adapted to the existing module shape: - src/observability/metrics/fleet.ts: sorokeep_contracts_tracked and sorokeep_entries_tracked as plain exported Gauge objects + collectFleetMetrics(db). - registry.ts: two registration lines + added to collectAllMetrics. - Rewrote the tests against the gauges' own .get() (matching the pattern used by budget/ttl/cost tests) instead of a per-test fresh Registry instance, preserving every original test scenario. Verified: tsc clean, full suite 1287/1287, npm audit clean, build succeeds, and manually smoke-tested end-to-end against an isolated temp database — confirmed correct per-network counts through the real HTTP endpoint (2 testnet / 1 mainnet contracts, 2 testnet / 1 mainnet entries). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…oint (TegoLabs#573, TegoLabs#344) PR TegoLabs#573's server.ts was a brand-new file (its branch predates TegoLabs#569's scaffold) implementing its own createObservabilityServer with plain Node http, hardcoded fake /metrics output, and stub /healthz/readyz routes, binding 0.0.0.0 by default — this would have completely replaced the real Hono-based server.ts that daemon/loop.ts already depends on (createMetricsServer/stopMetricsServer), and reintroduced a less-safe default bind address in the process. The PR also carried unrelated changes to src/commands/guard.ts and src/core/fleet.ts, outside this issue's explicit "only the auth middleware" scope. Ported just the auth concept — optional SOROKEEP_METRICS_TOKEN env var, Bearer-token check, bare 401 with no body leakage on mismatch/missing, fully backward-compatible when unset — as Hono middleware applied to all observability routes (app.use("*", ...)), so it automatically covers /healthz and /readyz once those land from their own issues, without another PR needing to touch this file again. Added 5 tests to the existing server.test.ts covering both the default (open) and token-configured (missing header / wrong token / wrong scheme / correct token) cases. Verified: tsc clean, full suite 1292/1292, npm audit clean, build succeeds, and manually smoke-tested all three cases (no token configured, wrong token, correct token) against the compiled server. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change documents and declares a Prometheus ChangesApplication runtime and metrics
Dependency resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes 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.5)src/observability/server.tsFile contains syntax errors that prevent linting: Line 120: Expected a statement but instead found '* metric is recomputed from the live database on each scrape, and
src/daemon/loop.tsFile contains syntax errors that prevent linting: Line 117: Expected a catch clause but instead found 'export'.; Line 309: 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 Warning |
43ad363 to
8692701
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@README.md`:
- Line 698: Remove the duplicate Prometheus /metrics roadmap entry from the
README, keeping the corresponding entry that links to the Observability Setup
Guide.
In `@src/core/monitor.ts`:
- Around line 140-161: Remove the duplicate auto-extension invocation block
around runAutoExtensions, including its result assignments and logging, so each
monitor cycle calls runAutoExtensions only once. Preserve the earlier invocation
and its extension state, counts, errors, and anomaly handling.
In `@src/daemon/loop.ts`:
- Around line 203-208: Remove the first try/catch block invoking
aggregateDailyCostSnapshots in the daemon cycle, while retaining the later
aggregation block so the operation runs exactly once per cycle.
- Around line 297-308: Remove the duplicate resolvePollIntervalMs declaration
from the daemon loop, retaining only one implementation with the existing
contract filtering and fallback behavior so TypeScript compilation succeeds.
- Line 162: In the monitor loop around runMonitorCycle, remove the outer
tracing-first result declaration and retain only the non-traced monitor-cycle
invocation so result is declared once in the same scope.
- Around line 81-88: Remove the incomplete metrics-server fragment beginning at
the options.metricsPort check, including its unmatched try and
createMetricsServer call, while preserving the complete metrics-server block
later in the daemon startup flow. Ensure the surrounding initialization in the
daemon loop remains syntactically valid.
In `@src/observability/server.ts`:
- Around line 117-119: Remove the stale bodyless createMetricsServer declaration
immediately before the implementation, leaving the valid three-argument overload
and its implementation intact.
🪄 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: e44acaf5-c4b8-4bd3-b8a5-23f38adf5387
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
README.mdpackage.jsonsrc/core/monitor.tssrc/daemon/loop.tssrc/observability/server.tstests/observability/server.test.ts
📜 Review details
🔇 Additional comments (2)
package.json (1)
85-87: LGTM!tests/observability/server.test.ts (1)
1-1: 🎯 Functional CorrectnessNo duplicate Vitest import is present.
tests/observability/server.test.tshas a single Vitest bindings import, so this comment does not apply.> Likely an incorrect or invalid review comment.
|
Thanks for flagging this, but closing without merging: |
closes #364