Conversation
Dual Postgres/SQLite migrations for the Crons feature (issue #143): - monitors: scheduled jobs, upserted per (project_id, slug), holding schedule config, derived status, last_check_in and next_expected_at. - check_ins: one row per reported execution, linked to its monitor.
Add the check-in ingestion pipeline (issue #143), mirroring Relay's relay-monitors CheckIn schema and process_check_in normalization: - models::check_in: CheckInPayload + normalize() (slug<=50, env<=64, server-only 'missed' coerced to 'unknown'), schedule/config models, and Monitor/CheckIn response shapes. - EnvelopeItemKind::CheckIn + parser 'check_in' arm + Route::CheckIn. - CheckInProcessor: upsert monitor by (project,slug), COALESCE schedule config, in_progress->ok lifecycle upsert, derived monitor status, and next_expected_at from the schedule. - services::monitor: schedule math (crontab via cron + interval + chrono-tz) and process_overdue() (missed + max_runtime timeout). Deps: cron, chrono-tz.
- workers::monitor_worker: background loop running process_overdue every
MONITOR_TICK_INTERVAL_SECS (default 60); wired in main.rs.
- GET /api/projects/{id}/monitors and
GET /api/projects/{id}/monitors/{slug}/checkins (offset paginated),
behind ViewProject access control.
- config: MONITOR_TICK_INTERVAL_SECS.
- Regenerated openapi.json with the monitor paths and schemas.
checkin_margin and max_runtime are read as Option<i64> but were declared INTEGER (INT4) in the Postgres migration, which panics on decode under real Postgres (INT8 vs INT4) while passing on SQLite's loose typing. Declare them BIGINT to match. Also switch the monitor test verification queries from '?' to '$N' placeholders so the suite runs green under both SQLite and Postgres (cargo test --features postgres), catching this class of dialect bug.
client.monitors.list(projectId) and listCheckIns(projectId, slug, options), with Zod schemas (Monitor, CheckIn) and types. MSW handlers + integration tests for both endpoints.
list_monitors and list_monitor_check_ins MCP tools wrapping the client, registered in the server factory. Tool tests + updated integration tool count.
New 'Crons' entry in the project sidebar and a /crons page listing monitors (status badge, schedule, last/next check-in) with lazily-loaded check-in history per monitor. Server actions wrap the client's monitors resource.
demo/src/crons.ts (pnpm demo:crons) exercises Sentry.withMonitor and captureCheckIn against a local server, the true wire-format compatibility test for monitor check-ins.
📝 WalkthroughWalkthroughImplements full Sentry Crons (monitors/check-ins) support: SQLite and Postgres migrations create ChangesSentry Crons — Monitors and Check-ins
Sequence Diagram(s)sequenceDiagram
participant SDK as Sentry SDK
participant Server as Ingest Server
participant CheckInProcessor
participant DB as Database
participant MonitorWorker
SDK->>Server: POST /api/{project}/envelope (check_in item)
Server->>CheckInProcessor: process(payload, ctx)
CheckInProcessor->>DB: UPSERT monitors(project_id, slug)
CheckInProcessor->>CheckInProcessor: next_expected_after(schedule, now)
CheckInProcessor->>DB: INSERT or UPDATE check_ins row
CheckInProcessor->>DB: UPDATE monitors status/next_expected_at
loop every monitor_tick_interval_secs
MonitorWorker->>MonitorWorker: Utc::now()
MonitorWorker->>DB: SELECT overdue monitors
MonitorWorker->>DB: UPDATE status=missed, INSERT missed check_in
MonitorWorker->>DB: SELECT in_progress exceeding max_runtime
MonitorWorker->>DB: UPDATE status=timeout
end
SDK->>Server: GET /api/projects/{id}/monitors
Server->>DB: SELECT monitors ORDER BY last_check_in_at
Server-->>SDK: MonitorsListResponse
SDK->>Server: GET /api/projects/{id}/monitors/{slug}/checkins
Server->>DB: SELECT check_ins LIMIT/OFFSET + COUNT
Server-->>SDK: OffsetPaginatedResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. 🔧 oasdiff (1.20.0)apps/server/openapi.jsonError: failed to load base spec from "/tmp/coderabbit-oasdiff-base.gyoX6l": map key "SortOrder" not found 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 |
Greptile SummaryThis PR implements Sentry Crons (monitor check-in ingestion) end-to-end: envelope item parsing, a
Confidence Score: 3/5Mostly solid implementation, but the missed-detection worker can overwrite a The background worker's
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant SDK as Sentry SDK
participant Ingest as Ingest Route
participant Proc as CheckInProcessor
participant DB as Database
participant Worker as MonitorWorker
SDK->>Ingest: "POST /api/{dsn}/envelope (check_in item)"
Ingest->>Proc: process(payload, ctx)
Proc->>DB: UPSERT monitors (project_id, slug) COALESCE schedule config
Proc->>DB: SELECT monitor id + schedule
Proc->>Proc: next_expected_after(schedule, ingested_at)
alt closing check-in (ok/error) with check_in_id
Proc->>DB: UPDATE check_ins SET status WHERE check_in_id
else new check-in
Proc->>DB: INSERT check_ins
end
Proc->>DB: UPDATE monitors SET status/last_check_in/next_expected_at
DB-->>Proc: ok
Proc-->>Ingest: ok
loop every MONITOR_TICK_INTERVAL_SECS
Worker->>Worker: process_timeouts(now)
Worker->>DB: SELECT in_progress check_ins with max_runtime
DB-->>Worker: open rows
Worker->>DB: UPDATE check_ins SET timeout / UPDATE monitors SET timeout
Worker->>Worker: process_overdue(now)
Worker->>DB: SELECT monitors WHERE next_expected_at NOT NULL AND status NOT IN (missed, disabled)
DB-->>Worker: candidates
Worker->>DB: UPDATE monitors SET missed / INSERT synthetic missed check_in
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant SDK as Sentry SDK
participant Ingest as Ingest Route
participant Proc as CheckInProcessor
participant DB as Database
participant Worker as MonitorWorker
SDK->>Ingest: "POST /api/{dsn}/envelope (check_in item)"
Ingest->>Proc: process(payload, ctx)
Proc->>DB: UPSERT monitors (project_id, slug) COALESCE schedule config
Proc->>DB: SELECT monitor id + schedule
Proc->>Proc: next_expected_after(schedule, ingested_at)
alt closing check-in (ok/error) with check_in_id
Proc->>DB: UPDATE check_ins SET status WHERE check_in_id
else new check-in
Proc->>DB: INSERT check_ins
end
Proc->>DB: UPDATE monitors SET status/last_check_in/next_expected_at
DB-->>Proc: ok
Proc-->>Ingest: ok
loop every MONITOR_TICK_INTERVAL_SECS
Worker->>Worker: process_timeouts(now)
Worker->>DB: SELECT in_progress check_ins with max_runtime
DB-->>Worker: open rows
Worker->>DB: UPDATE check_ins SET timeout / UPDATE monitors SET timeout
Worker->>Worker: process_overdue(now)
Worker->>DB: SELECT monitors WHERE next_expected_at NOT NULL AND status NOT IN (missed, disabled)
DB-->>Worker: candidates
Worker->>DB: UPDATE monitors SET missed / INSERT synthetic missed check_in
end
Reviews (1): Last reviewed commit: "docs(memory): record Crons feature + #14..." | Re-trigger Greptile |
| let candidates = sqlx::query( | ||
| r#" | ||
| SELECT id, schedule_type, schedule_value, schedule_unit, timezone, | ||
| checkin_margin, next_expected_at | ||
| FROM monitors | ||
| WHERE next_expected_at IS NOT NULL | ||
| AND status NOT IN ('missed', 'disabled') | ||
| "#, | ||
| ) | ||
| .fetch_all(pool) | ||
| .await?; |
There was a problem hiding this comment.
timeout status not excluded from missed-detection candidates
The candidates query filters status NOT IN ('missed', 'disabled') but does not exclude 'timeout'. Because process_timeouts marks a monitor as timeout without advancing next_expected_at, the very next worker tick (once next_expected_at + margin < now) will re-enter the monitor into the missed loop and overwrite its status to missed. A concrete failure path: a job's check-in times out, its next_expected_at is still in the past (since no new check-in arrived), the worker next tick enters the monitor as a candidate, and it is flipped from timeout to missed. Users see the wrong terminal state. The fix is to add 'timeout' to the exclusion list, or to advance next_expected_at in process_timeouts.
| const toggle = useCallback( | ||
| async (slug: string) => { | ||
| if (expanded === slug) { | ||
| setExpanded(null); | ||
| return; | ||
| } | ||
| setExpanded(slug); | ||
| if (!checkInsBySlug[slug]) { | ||
| setLoadingSlug(slug); | ||
| try { | ||
| const page = await listCheckIns(projectId, slug, { per_page: 10 }); | ||
| setCheckInsBySlug((prev) => ({ ...prev, [slug]: page.items })); | ||
| } finally { | ||
| setLoadingSlug(null); | ||
| } | ||
| } | ||
| }, | ||
| [expanded, checkInsBySlug, projectId], | ||
| ); |
There was a problem hiding this comment.
The toggle handler uses try-finally without a catch block. If listCheckIns throws (network error, server 5xx, auth expiry), setLoadingSlug(null) fires via finally, but checkInsBySlug[slug] remains undefined. The expanded panel then renders "No check-ins recorded yet." — indistinguishable from a monitor that genuinely has no history — giving users no indication that the load failed. Consider adding an error state (e.g. a errorBySlug map) and rendering an error message instead.
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (6)
_bmad/_memory/agent-rusty/sessions/2026-06-29.md (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRephrase to avoid markdownlint false positive.
Line 26 starts with
#143which markdownlint interprets as an ATX heading lacking a space. Rephrase to start with text, e.g., "Issue#143only covers performance items."🤖 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 `@_bmad/_memory/agent-rusty/sessions/2026-06-29.md` at line 26, The line in the session note starts with “#143”, which markdownlint reads as an unintended heading; update the text in the affected markdown entry to begin with normal prose instead, such as referencing the issue within a sentence. Keep the meaning the same while rephrasing the sentence in the session file so it no longer starts with a hash token and avoids the ATX heading false positive._bmad/_memory/agent-rusty/BOND.md (1)
38-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
EnvelopeItemKind::Logline reference may be stale.The comment cites
envelope.rs:60, but the enum variant appears around line 52 in the current file (perapps/server/src/ingest/envelope.rs). If line numbers are meant to be precise references, consider updating or using a symbol search instead of hardcoded lines.🤖 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 `@_bmad/_memory/agent-rusty/BOND.md` around lines 38 - 40, The line reference for EnvelopeItemKind::Log in BOND.md appears stale, so update the note to use the current location or replace the hardcoded line number with a symbol-based reference. Adjust the documentation entry that mentions envelope.rs and digest/processors/logs.rs so it points to the correct EnvelopeItemKind::Log definition and remains accurate if the file shifts again.apps/server/tests/integration/monitors_api_test.rs (2)
112-116: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the schedule payload too.
This seeds
monitor_config.schedule, but the test only checksslugandstatus. A regression in schedule persistence/serialization would still pass here while breaking the Crons UI.🤖 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 `@apps/server/tests/integration/monitors_api_test.rs` around lines 112 - 116, The integration test for monitors only verifies slug and status, so it can miss regressions in schedule serialization. Update the assertions in the monitors API test to also validate the schedule value returned in the response for the seeded monitor_config.schedule, using the existing body["monitors"] item checks so the test covers the full payload.
167-170: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the pagination contract in this assertion block.
Right now this would still pass if
has_more/next_cursordisappeared or changed shape, even though the new client/MCP layers depend on list metadata being stable.🤖 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 `@apps/server/tests/integration/monitors_api_test.rs` around lines 167 - 170, The current assertions in the monitors API integration test only validate total_count and items, so they would miss regressions in the pagination metadata contract. Update the assertion block in the test that reads the JSON response to also verify the presence and expected shape/value of has_more and next_cursor, using the same response body and items array checks already in place. Keep the assertions tied to the list response contract so future changes in the API are caught by this test.packages/client/src/schemas/monitor.ts (1)
10-24: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTighten the monitor/check-in schema domain constraints.
These schemas currently accept values the API should never emit, like arbitrary status strings or negative runtimes/durations. That weakens the Zod boundary and lets server/client contract drift pass validation unnoticed. Please model the known enum fields explicitly and make the runtime fields non-negative. As per coding guidelines, "Use Zod schemas as the single source of truth for both runtime validation and TypeScript type definitions."
Also applies to: 36-44
🤖 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 `@packages/client/src/schemas/monitor.ts` around lines 10 - 24, The monitorSchema currently allows overly broad values for API-controlled fields, so tighten the Zod contract by replacing free-form strings with explicit enums for known status/check-in/schedule fields and constraining runtime/duration integers to non-negative values. Update the relevant schema definitions in monitorSchema (and the related schema block referenced by the same comment) so the inferred TypeScript types stay aligned with the runtime validation and cannot accept invalid server/client contract values.Source: Coding guidelines
packages/mcp/src/tools/monitors.ts (1)
15-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap these MCP tool inputs in
z.object(...).inputSchemais still relying on raw-shape normalization, but the Standard Schema form here should be explicit so these tools stay on the supported path and avoid future breakage. Also applies to the second tool registration.🤖 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 `@packages/mcp/src/tools/monitors.ts` around lines 15 - 17, Wrap the MCP tool input definitions in an explicit z.object(...) schema instead of passing a raw shape to inputSchema. Update the tool registration in monitors.ts where project_id is defined, and apply the same change to the second tool registration so both use the supported Standard Schema form and avoid relying on shape normalization.Source: Coding guidelines
🤖 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 `@_bmad/_memory/agent-rusty/sessions/2026-06-29.md`:
- Around line 8-10: The `EnvelopeItemKind` inventory in the session note is
missing `CheckIn`, so update the list to match the current enum state reflected
by `EnvelopeItemKind` and the related digest processor modules. Edit the session
entry that mentions Event, Transaction, Session, Sessions, Log, and Other to
also include CheckIn, keeping the BOND/status note consistent with the
implemented item.
In `@apps/server/migrations/postgres/20260629000000_create_monitors.up.sql`:
- Around line 41-56: The check-in schema currently allows duplicate lifecycle
rows because `idx_check_ins_lifecycle` is only a non-unique index on
`(monitor_id, check_in_id)`, which can make terminal upserts hit multiple rows.
Update the `check_ins` table definition in the migration to enforce a unique
lifecycle key for the open/closing check-in pair, using the existing
`monitor_id` and `check_in_id` columns. If needed, adjust the lifecycle index
name or add a unique constraint/index so each check-in lifecycle can only exist
once and terminal updates remain deterministic.
In `@apps/server/migrations/sqlite/20260629000000_create_monitors.up.sql`:
- Around line 31-49: The lifecycle index on check_ins is currently non-unique,
so duplicate rows can still be inserted for the same monitor_id and check_in_id.
Update the sqlite migration that creates check_ins to make
idx_check_ins_lifecycle a UNIQUE index, using the existing check_ins table
definition and the lifecycle columns monitor_id and check_in_id so each run
identity can only appear once.
In `@apps/server/src/config.rs`:
- Around line 107-110: The MONITOR_TICK_INTERVAL_SECS parsing in Config
currently accepts 0 and can later trigger a tokio::time::interval panic. Update
the monitor_tick_interval_secs initialization in config.rs to reject zero by
validating the parsed value and returning a config load error, or by clamping it
to at least 1 before storing it in the Config struct.
In `@apps/server/src/digest/processors/check_in.rs`:
- Around line 109-129: The check-in update in `check_in` is too broad because it
matches any row with the same `check_in_id`, which can overwrite already
finalized runs. Tighten the `UPDATE` in
`apps/server/src/digest/processors/check_in.rs` so it only targets the open
`in_progress` row for the given `monitor_id` and `check_in_id`, and then adjust
the `rows_affected()` handling to distinguish “already finalized” from “missing”
instead of falling through to an insert. Use the existing `check_in_id`,
`monitor_id`, and `check_in.status` logic in `check_in` to keep retried or
out-of-order terminal events from rewriting completed rows.
In `@apps/server/src/main.rs`:
- Around line 82-85: The MonitorWorker loop currently only runs overdue/missed
detection via MonitorService::process_overdue, so timeout handling is missing.
Update MonitorWorker::run to also invoke MonitorService::process_timeouts on
each tick (using the same db_pool/config context as the overdue path), so
in-progress check-ins that exceed max_runtime are cleared. Keep the change
localized to the monitor worker path in main.rs/MonitorWorker so both missed and
timeout processing happen together.
In `@apps/server/src/models/check_in.rs`:
- Around line 131-151: Reject negative timing values during check-in
normalization by updating CheckIn::normalize() to validate
monitor_config.checkin_margin and monitor_config.max_runtime before persistence.
If either value is negative, return an AppError::Validation instead of allowing
the record to be saved unchanged. Use the existing normalize() flow in
check_in.rs so CheckInProcessor::process() cannot persist invalid timing values,
and keep the validation close to the current monitor_slug and environment
checks.
In `@apps/server/src/routes/ingest.rs`:
- Around line 116-123: The inline check-in handling in the check-in branch of
the ingest route is swallowing write failures by logging and continuing, which
allows a successful ACK even when the store/update failed. Update the logic
around processors.check_ins.process in ingest to propagate the error as a Result
instead of returning 200 OK on failure, following the existing idiomatic Rust
error-handling style used in apps/server/src/**/*.rs. Make sure the
EnvelopeItemKind::CheckIn path returns an error from the route when processing
fails so the caller can retry and the monitor/run-state updates are not silently
lost.
In `@apps/server/src/services/monitor.rs`:
- Around line 120-195: The monitor transition logic in the worker can overwrite
a real check-in because `process_missed_monitors` reads candidates first and
then performs unconditional writes later. Update the `UPDATE monitors`
statements in the missed/timeout transition paths to include the evaluated state
guards (such as `status`, `next_expected_at`, or the relevant check-in
identifier), or lock the row before calling `next_expected_after`, so only
still-stale rows transition. Make the synthetic `check_ins` insert run only when
the guarded update actually affects a row, and keep the transaction flow in
`process_missed_monitors` and the timeout path consistent.
In `@apps/server/tests/unit/check_in_test.rs`:
- Around line 406-413: The newest-first check in MonitorService::list_check_ins
is only verifying that both statuses exist, so it can pass even if the results
are not actually sorted by timestamp descending. Update the test in
check_in_test to assert the exact order of the returned check_ins using their
timestamps or another stable ordering field, alongside the existing status
checks, so the test explicitly proves newest-first behavior.
In `@apps/webview-ui/src/app/`(main)/projects/[id]/crons/monitors-list.tsx:
- Around line 64-80: Track loading state per monitor in monitors-list.tsx
instead of using a single loadingSlug that gets overwritten by overlapping
requests. Update the toggle callback and the row rendering logic to key loading
by slug (for example via a per-slug map or Set) so one monitor’s request
finishing does not clear another monitor’s spinner. Use the existing toggle,
loadingSlug, setLoadingSlug, and listCheckIns logic as the main places to
update, and apply the same change to the duplicated rendering path referenced in
the later section.
- Around line 107-123: The expand/collapse behavior on TableRow in
monitors-list.tsx is mouse-only, so make that row keyboard-accessible. Update
the row interaction in the TableRow/toggle(monitor.slug) block so it can be
focused and activated with keyboard controls (for example Enter/Space), and keep
aria-expanded in sync with isOpen. Use the existing toggle helper and isOpen
state to wire the accessible behavior without changing the panel logic.
In `@packages/client/src/resources/monitors.ts`:
- Around line 40-45: The query param handling in monitors.ts is incorrectly
gated on truthiness, so numeric values like page=0 or per_page=0 are skipped
instead of being serialized or explicitly rejected. Update the parameter checks
in the monitor request builder to test for options.page and options.per_page
being !== undefined, and keep any 1-based/positive validation separate from
serialization so the behavior is explicit and consistent.
In `@packages/client/tests/integration/monitors.test.ts`:
- Around line 23-25: The test for an unknown project currently uses a generic
rejection assertion, which does not verify the intended error mapping. Update
the `client.monitors.list(999)` expectation in `monitors.test.ts` to assert the
specific `NotFoundError` class explicitly, using the same test case name and
`rejects` chain so the check confirms the correct error type rather than any
thrown failure.
In `@packages/client/tests/mocks/handlers.ts`:
- Around line 1226-1248: The mock fixture for the monitors check-ins list
endpoint is using offset pagination fields, which conflicts with the repo’s
cursor-based list contract. Update the response shape in the relevant handlers
fixture to use the same cursor pagination fields as other list endpoints,
specifically `next_cursor` and `has_more`, and remove the
`page`/`per_page`/`total_pages` assumptions so tests and the API mock stay
aligned.
In `@packages/test-sentry/demo/src/crons.ts`:
- Around line 110-113: The error path in main().catch currently exits
immediately after logging, which can drop pending Sentry events. Update the
catch handler in crons.ts so it awaits Sentry.flush(2000) before calling
process.exit(1), matching the flush behavior used on the success path and
preserving any final batched check-ins.
---
Nitpick comments:
In `@_bmad/_memory/agent-rusty/BOND.md`:
- Around line 38-40: The line reference for EnvelopeItemKind::Log in BOND.md
appears stale, so update the note to use the current location or replace the
hardcoded line number with a symbol-based reference. Adjust the documentation
entry that mentions envelope.rs and digest/processors/logs.rs so it points to
the correct EnvelopeItemKind::Log definition and remains accurate if the file
shifts again.
In `@_bmad/_memory/agent-rusty/sessions/2026-06-29.md`:
- Line 26: The line in the session note starts with “#143”, which markdownlint
reads as an unintended heading; update the text in the affected markdown entry
to begin with normal prose instead, such as referencing the issue within a
sentence. Keep the meaning the same while rephrasing the sentence in the session
file so it no longer starts with a hash token and avoids the ATX heading false
positive.
In `@apps/server/tests/integration/monitors_api_test.rs`:
- Around line 112-116: The integration test for monitors only verifies slug and
status, so it can miss regressions in schedule serialization. Update the
assertions in the monitors API test to also validate the schedule value returned
in the response for the seeded monitor_config.schedule, using the existing
body["monitors"] item checks so the test covers the full payload.
- Around line 167-170: The current assertions in the monitors API integration
test only validate total_count and items, so they would miss regressions in the
pagination metadata contract. Update the assertion block in the test that reads
the JSON response to also verify the presence and expected shape/value of
has_more and next_cursor, using the same response body and items array checks
already in place. Keep the assertions tied to the list response contract so
future changes in the API are caught by this test.
In `@packages/client/src/schemas/monitor.ts`:
- Around line 10-24: The monitorSchema currently allows overly broad values for
API-controlled fields, so tighten the Zod contract by replacing free-form
strings with explicit enums for known status/check-in/schedule fields and
constraining runtime/duration integers to non-negative values. Update the
relevant schema definitions in monitorSchema (and the related schema block
referenced by the same comment) so the inferred TypeScript types stay aligned
with the runtime validation and cannot accept invalid server/client contract
values.
In `@packages/mcp/src/tools/monitors.ts`:
- Around line 15-17: Wrap the MCP tool input definitions in an explicit
z.object(...) schema instead of passing a raw shape to inputSchema. Update the
tool registration in monitors.ts where project_id is defined, and apply the same
change to the second tool registration so both use the supported Standard Schema
form and avoid relying on shape normalization.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c3f4b92-c5a3-428f-b1ab-aa748d3369c7
⛔ Files ignored due to path filters (1)
apps/server/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (68)
_bmad/_memory/agent-rusty/BOND.md_bmad/_memory/agent-rusty/sessions/2026-06-29.mdapps/server/Cargo.tomlapps/server/migrations/postgres/20260629000000_create_monitors.down.sqlapps/server/migrations/postgres/20260629000000_create_monitors.up.sqlapps/server/migrations/sqlite/20260629000000_create_monitors.down.sqlapps/server/migrations/sqlite/20260629000000_create_monitors.up.sqlapps/server/openapi.jsonapps/server/src/config.rsapps/server/src/digest/processors/check_in.rsapps/server/src/digest/processors/mod.rsapps/server/src/ingest/envelope.rsapps/server/src/main.rsapps/server/src/models/check_in.rsapps/server/src/models/mod.rsapps/server/src/openapi.rsapps/server/src/pagination/mod.rsapps/server/src/routes/ingest.rsapps/server/src/routes/mod.rsapps/server/src/routes/monitors.rsapps/server/src/routes/projects.rsapps/server/src/services/mod.rsapps/server/src/services/monitor.rsapps/server/src/workers/mod.rsapps/server/src/workers/monitor_worker.rsapps/server/tests/e2e/sentry_sdk_test.rsapps/server/tests/integration/alerts_api_test.rsapps/server/tests/integration/auth_test.rsapps/server/tests/integration/envelope_v2_test.rsapps/server/tests/integration/events_api_test.rsapps/server/tests/integration/ingest_test.rsapps/server/tests/integration/issues_api_test.rsapps/server/tests/integration/logs_api_test.rsapps/server/tests/integration/mod.rsapps/server/tests/integration/monitors_api_test.rsapps/server/tests/integration/projects_api_test.rsapps/server/tests/integration/rate_limit_test.rsapps/server/tests/integration/sourcemaps_api_test.rsapps/server/tests/integration/storage_api_test.rsapps/server/tests/integration/team_rbac_test.rsapps/server/tests/integration/tokens_api_test.rsapps/server/tests/integration/transactions_api_test.rsapps/server/tests/unit/check_in_test.rsapps/server/tests/unit/envelope_parser_test.rsapps/server/tests/unit/mod.rsapps/server/tests/unit/monitor_schedule_test.rsapps/server/tests/unit/monitor_worker_test.rsapps/webview-ui/src/actions/monitors.tsapps/webview-ui/src/app/(main)/projects/[id]/crons/monitors-list.tsxapps/webview-ui/src/app/(main)/projects/[id]/crons/page.tsxapps/webview-ui/src/app/(main)/projects/[id]/project-sidebar.tsxpackages/client/src/client.tspackages/client/src/index.tspackages/client/src/resources/index.tspackages/client/src/resources/monitors.tspackages/client/src/schemas/index.tspackages/client/src/schemas/monitor.tspackages/client/src/types/common.tspackages/client/src/types/index.tspackages/client/src/types/monitor.tspackages/client/tests/integration/monitors.test.tspackages/client/tests/mocks/handlers.tspackages/mcp/src/server.tspackages/mcp/src/tools/monitors.tspackages/mcp/tests/integration/server.test.tspackages/mcp/tests/tools/monitors.test.tspackages/test-sentry/demo/src/crons.tspackages/test-sentry/package.json
| - Rustrak `EnvelopeItemKind` (envelope.rs:52): Event, Transaction, Session, Sessions, **Log**, Other. | ||
| - My BOND was stale: **Log is now implemented** (v0.8.0, `digest/processors/logs.rs`). Issue #143 | ||
| item #7 is DONE. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing CheckIn in the EnvelopeItemKind variant list.
Line 8 lists: Event, Transaction, Session, Sessions, Log, Other. But CheckIn was just implemented and is present in the enum (per apps/server/src/ingest/envelope.rs and apps/server/src/digest/processors/mod.rs). The session note should include CheckIn in this inventory to accurately reflect the current state and be consistent with the BOND update.
🤖 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 `@_bmad/_memory/agent-rusty/sessions/2026-06-29.md` around lines 8 - 10, The
`EnvelopeItemKind` inventory in the session note is missing `CheckIn`, so update
the list to match the current enum state reflected by `EnvelopeItemKind` and the
related digest processor modules. Edit the session entry that mentions Event,
Transaction, Session, Sessions, Log, and Other to also include CheckIn, keeping
the BOND/status note consistent with the implemented item.
| -- SDK-provided id. Shared between the in_progress and the closing check-in, | ||
| -- so the closing status/duration upserts onto the open row. NULL when absent. | ||
| check_in_id UUID, | ||
|
|
||
| status VARCHAR(16) NOT NULL, | ||
| duration DOUBLE PRECISION, -- seconds | ||
| environment VARCHAR(64), | ||
| trace_id VARCHAR(64), | ||
|
|
||
| timestamp TIMESTAMPTZ NOT NULL, | ||
| ingested_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | ||
| ); | ||
|
|
||
| CREATE INDEX idx_check_ins_monitor ON check_ins(monitor_id, timestamp DESC); | ||
| CREATE INDEX idx_check_ins_project_ingested ON check_ins(project_id, ingested_at DESC); | ||
| CREATE INDEX idx_check_ins_lifecycle ON check_ins(monitor_id, check_in_id); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce one row per check-in lifecycle.
The comment here says the terminal check-in upserts onto the open row, but the schema only adds a non-unique index on (monitor_id, check_in_id). That lets duplicate lifecycle rows exist and makes terminal updates non-deterministic.
Suggested migration change
-CREATE INDEX idx_check_ins_lifecycle ON check_ins(monitor_id, check_in_id);
+CREATE UNIQUE INDEX idx_check_ins_lifecycle ON check_ins(monitor_id, check_in_id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- SDK-provided id. Shared between the in_progress and the closing check-in, | |
| -- so the closing status/duration upserts onto the open row. NULL when absent. | |
| check_in_id UUID, | |
| status VARCHAR(16) NOT NULL, | |
| duration DOUBLE PRECISION, -- seconds | |
| environment VARCHAR(64), | |
| trace_id VARCHAR(64), | |
| timestamp TIMESTAMPTZ NOT NULL, | |
| ingested_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | |
| ); | |
| CREATE INDEX idx_check_ins_monitor ON check_ins(monitor_id, timestamp DESC); | |
| CREATE INDEX idx_check_ins_project_ingested ON check_ins(project_id, ingested_at DESC); | |
| CREATE INDEX idx_check_ins_lifecycle ON check_ins(monitor_id, check_in_id); | |
| -- SDK-provided id. Shared between the in_progress and the closing check-in, | |
| -- so the closing status/duration upserts onto the open row. NULL when absent. | |
| check_in_id UUID, | |
| status VARCHAR(16) NOT NULL, | |
| duration DOUBLE PRECISION, -- seconds | |
| environment VARCHAR(64), | |
| trace_id VARCHAR(64), | |
| timestamp TIMESTAMPTZ NOT NULL, | |
| ingested_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | |
| ); | |
| CREATE INDEX idx_check_ins_monitor ON check_ins(monitor_id, timestamp DESC); | |
| CREATE INDEX idx_check_ins_project_ingested ON check_ins(project_id, ingested_at DESC); | |
| CREATE UNIQUE INDEX idx_check_ins_lifecycle ON check_ins(monitor_id, check_in_id); |
🤖 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 `@apps/server/migrations/postgres/20260629000000_create_monitors.up.sql` around
lines 41 - 56, The check-in schema currently allows duplicate lifecycle rows
because `idx_check_ins_lifecycle` is only a non-unique index on `(monitor_id,
check_in_id)`, which can make terminal upserts hit multiple rows. Update the
`check_ins` table definition in the migration to enforce a unique lifecycle key
for the open/closing check-in pair, using the existing `monitor_id` and
`check_in_id` columns. If needed, adjust the lifecycle index name or add a
unique constraint/index so each check-in lifecycle can only exist once and
terminal updates remain deterministic.
| CREATE TABLE check_ins ( | ||
| id TEXT PRIMARY KEY, | ||
| monitor_id TEXT NOT NULL REFERENCES monitors(id) ON DELETE CASCADE, | ||
| project_id INTEGER NOT NULL REFERENCES projects(id) ON DELETE CASCADE, | ||
|
|
||
| check_in_id TEXT, | ||
|
|
||
| status VARCHAR(16) NOT NULL, | ||
| duration REAL, | ||
| environment VARCHAR(64), | ||
| trace_id VARCHAR(64), | ||
|
|
||
| timestamp TEXT NOT NULL, | ||
| ingested_at TEXT NOT NULL DEFAULT (datetime('now')) | ||
| ); | ||
|
|
||
| CREATE INDEX idx_check_ins_monitor ON check_ins(monitor_id, timestamp DESC); | ||
| CREATE INDEX idx_check_ins_project_ingested ON check_ins(project_id, ingested_at DESC); | ||
| CREATE INDEX idx_check_ins_lifecycle ON check_ins(monitor_id, check_in_id); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the lifecycle index unique.
This schema treats check_in_id as the run identity, but idx_check_ins_lifecycle is only a plain index. That still allows duplicate rows for the same (monitor_id, check_in_id), so a later ok/error close can become ambiguous and check-in history can overcount executions.
Suggested migration change
-CREATE INDEX idx_check_ins_lifecycle ON check_ins(monitor_id, check_in_id);
+CREATE UNIQUE INDEX idx_check_ins_lifecycle ON check_ins(monitor_id, check_in_id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE check_ins ( | |
| id TEXT PRIMARY KEY, | |
| monitor_id TEXT NOT NULL REFERENCES monitors(id) ON DELETE CASCADE, | |
| project_id INTEGER NOT NULL REFERENCES projects(id) ON DELETE CASCADE, | |
| check_in_id TEXT, | |
| status VARCHAR(16) NOT NULL, | |
| duration REAL, | |
| environment VARCHAR(64), | |
| trace_id VARCHAR(64), | |
| timestamp TEXT NOT NULL, | |
| ingested_at TEXT NOT NULL DEFAULT (datetime('now')) | |
| ); | |
| CREATE INDEX idx_check_ins_monitor ON check_ins(monitor_id, timestamp DESC); | |
| CREATE INDEX idx_check_ins_project_ingested ON check_ins(project_id, ingested_at DESC); | |
| CREATE INDEX idx_check_ins_lifecycle ON check_ins(monitor_id, check_in_id); | |
| CREATE TABLE check_ins ( | |
| id TEXT PRIMARY KEY, | |
| monitor_id TEXT NOT NULL REFERENCES monitors(id) ON DELETE CASCADE, | |
| project_id INTEGER NOT NULL REFERENCES projects(id) ON DELETE CASCADE, | |
| check_in_id TEXT, | |
| status VARCHAR(16) NOT NULL, | |
| duration REAL, | |
| environment VARCHAR(64), | |
| trace_id VARCHAR(64), | |
| timestamp TEXT NOT NULL, | |
| ingested_at TEXT NOT NULL DEFAULT (datetime('now')) | |
| ); | |
| CREATE INDEX idx_check_ins_monitor ON check_ins(monitor_id, timestamp DESC); | |
| CREATE INDEX idx_check_ins_project_ingested ON check_ins(project_id, ingested_at DESC); | |
| CREATE UNIQUE INDEX idx_check_ins_lifecycle ON check_ins(monitor_id, check_in_id); |
🤖 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 `@apps/server/migrations/sqlite/20260629000000_create_monitors.up.sql` around
lines 31 - 49, The lifecycle index on check_ins is currently non-unique, so
duplicate rows can still be inserted for the same monitor_id and check_in_id.
Update the sqlite migration that creates check_ins to make
idx_check_ins_lifecycle a UNIQUE index, using the existing check_ins table
definition and the lifecycle columns monitor_id and check_in_id so each run
identity can only appear once.
| monitor_tick_interval_secs: env::var("MONITOR_TICK_INTERVAL_SECS") | ||
| .unwrap_or_else(|_| "60".to_string()) | ||
| .parse() | ||
| .unwrap_or(60), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the config and worker definitions, then inspect the relevant slices.
sed -n '1,220p' apps/server/src/config.rs
printf '\n--- monitor worker ---\n'
sed -n '1,160p' apps/server/src/workers/monitor_worker.rsRepository: rustrak/rustrak
Length of output: 10160
🌐 Web query:
tokio time interval zero duration panic docs
💡 Result:
The Tokio interval function will panic if the provided period is a zero duration [1][2]. This is an intentional design choice enforced by an assertion within the interval function, which requires the period to be strictly greater than a zero-duration Duration [2]. The official documentation explicitly notes this behavior under the Panics section [1][3], and the source code uses assert!(period > Duration::new(0, 0), "period must be non-zero."); to trigger this panic if the condition is not met [2].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/time/fn.interval.html
- 2: https://github.com/tokio-rs/tokio/blob/5030b300/tokio/src/time/interval.rs
- 3: https://docs.rs/tokio/1.50.0/tokio/time/fn.interval.html
Reject MONITOR_TICK_INTERVAL_SECS=0 in apps/server/src/config.rs:107-110. tokio::time::interval(Duration::from_secs(0)) panics, and this path currently accepts 0 and passes it through unchanged. Fail config loading or clamp the value to >= 1.
🤖 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 `@apps/server/src/config.rs` around lines 107 - 110, The
MONITOR_TICK_INTERVAL_SECS parsing in Config currently accepts 0 and can later
trigger a tokio::time::interval panic. Update the monitor_tick_interval_secs
initialization in config.rs to reject zero by validating the parsed value and
returning a config load error, or by clamping it to at least 1 before storing it
in the Config struct.
| let updated = if let Some(cid) = check_in_id { | ||
| sqlx::query( | ||
| r#" | ||
| UPDATE check_ins SET | ||
| status = $1, | ||
| duration = COALESCE($2, duration), | ||
| environment = COALESCE($3, environment), | ||
| timestamp = $4 | ||
| WHERE monitor_id = $5 AND check_in_id = $6 | ||
| "#, | ||
| ) | ||
| .bind(check_in.status.as_str()) | ||
| .bind(check_in.duration) | ||
| .bind(check_in.environment.as_deref()) | ||
| .bind(ctx.ingested_at) | ||
| .bind(monitor_id) | ||
| .bind(cid) | ||
| .execute(&mut *tx) | ||
| .await? | ||
| .rows_affected() | ||
| > 0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only close the open check-in row.
This UPDATE matches any row with the same check_in_id, not just the active in_progress one. A retried or out-of-order terminal check-in can therefore rewrite a completed run from ok to error (or the reverse) because rows_affected() > 0 suppresses the insert path.
Suggested direction
UPDATE check_ins SET
status = $1,
duration = COALESCE($2, duration),
environment = COALESCE($3, environment),
timestamp = $4
- WHERE monitor_id = $5 AND check_in_id = $6
+ WHERE monitor_id = $5
+ AND check_in_id = $6
+ AND status = 'in_progress'After that, if no row was updated, treat an existing non-in_progress row with the same check_in_id as already finalized instead of inserting a duplicate.
🤖 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 `@apps/server/src/digest/processors/check_in.rs` around lines 109 - 129, The
check-in update in `check_in` is too broad because it matches any row with the
same `check_in_id`, which can overwrite already finalized runs. Tighten the
`UPDATE` in `apps/server/src/digest/processors/check_in.rs` so it only targets
the open `in_progress` row for the given `monitor_id` and `check_in_id`, and
then adjust the `rows_affected()` handling to distinguish “already finalized”
from “missing” instead of falling through to an insert. Use the existing
`check_in_id`, `monitor_id`, and `check_in.status` logic in `check_in` to keep
retried or out-of-order terminal events from rewriting completed rows.
| <TableRow | ||
| onClick={() => toggle(monitor.slug)} | ||
| aria-expanded={isOpen} | ||
| className={cn( | ||
| 'cursor-pointer', | ||
| isOpen && 'bg-muted/40 hover:bg-muted/40', | ||
| )} | ||
| > | ||
| <TableCell className="py-2 text-muted-foreground"> | ||
| <ChevronRight | ||
| className={cn( | ||
| 'size-4 transition-transform', | ||
| isOpen && 'rotate-90', | ||
| )} | ||
| aria-hidden="true" | ||
| /> | ||
| </TableCell> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the row toggle keyboard-accessible.
Line 108 puts the expand/collapse action on TableRow, which leaves the recent check-ins panel unreachable from the keyboard. That blocks a core task for non-pointer users.
Proposed fix
- <TableRow
- onClick={() => toggle(monitor.slug)}
- aria-expanded={isOpen}
+ <TableRow
className={cn(
'cursor-pointer',
isOpen && 'bg-muted/40 hover:bg-muted/40',
)}
>
<TableCell className="py-2 text-muted-foreground">
- <ChevronRight
- className={cn(
- 'size-4 transition-transform',
- isOpen && 'rotate-90',
- )}
- aria-hidden="true"
- />
+ <button
+ type="button"
+ onClick={() => toggle(monitor.slug)}
+ aria-expanded={isOpen}
+ aria-controls={`monitor-${monitor.id}-checkins`}
+ className="inline-flex items-center rounded-sm focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring"
+ >
+ <ChevronRight
+ className={cn(
+ 'size-4 transition-transform',
+ isOpen && 'rotate-90',
+ )}
+ aria-hidden="true"
+ />
+ <span className="sr-only">
+ {isOpen ? 'Collapse' : 'Expand'} recent check-ins for {monitor.slug}
+ </span>
+ </button>
</TableCell>
@@
- <TableRow className="hover:bg-transparent">
+ <TableRow
+ id={`monitor-${monitor.id}-checkins`}
+ className="hover:bg-transparent"
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <TableRow | |
| onClick={() => toggle(monitor.slug)} | |
| aria-expanded={isOpen} | |
| className={cn( | |
| 'cursor-pointer', | |
| isOpen && 'bg-muted/40 hover:bg-muted/40', | |
| )} | |
| > | |
| <TableCell className="py-2 text-muted-foreground"> | |
| <ChevronRight | |
| className={cn( | |
| 'size-4 transition-transform', | |
| isOpen && 'rotate-90', | |
| )} | |
| aria-hidden="true" | |
| /> | |
| </TableCell> | |
| <TableRow | |
| className={cn( | |
| 'cursor-pointer', | |
| isOpen && 'bg-muted/40 hover:bg-muted/40', | |
| )} | |
| > | |
| <TableCell className="py-2 text-muted-foreground"> | |
| <button | |
| type="button" | |
| onClick={() => toggle(monitor.slug)} | |
| aria-expanded={isOpen} | |
| aria-controls={`monitor-${monitor.id}-checkins`} | |
| className="inline-flex items-center rounded-sm focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" | |
| > | |
| <ChevronRight | |
| className={cn( | |
| 'size-4 transition-transform', | |
| isOpen && 'rotate-90', | |
| )} | |
| aria-hidden="true" | |
| /> | |
| <span className="sr-only"> | |
| {isOpen ? 'Collapse' : 'Expand'} recent check-ins for {monitor.slug} | |
| </span> | |
| </button> | |
| </TableCell> |
🤖 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 `@apps/webview-ui/src/app/`(main)/projects/[id]/crons/monitors-list.tsx around
lines 107 - 123, The expand/collapse behavior on TableRow in monitors-list.tsx
is mouse-only, so make that row keyboard-accessible. Update the row interaction
in the TableRow/toggle(monitor.slug) block so it can be focused and activated
with keyboard controls (for example Enter/Space), and keep aria-expanded in sync
with isOpen. Use the existing toggle helper and isOpen state to wire the
accessible behavior without changing the panel logic.
| if (options?.page) { | ||
| searchParams.page = String(options.page); | ||
| } | ||
| if (options?.per_page) { | ||
| searchParams.per_page = String(options.per_page); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't gate numeric query params on truthiness.
page=0 or per_page=0 gets dropped here instead of being forwarded or rejected, which silently changes the request semantics. Check for !== undefined before serializing, and validate positivity separately if the API requires 1-based paging.
Suggested fix
- if (options?.page) {
+ if (options?.page !== undefined) {
searchParams.page = String(options.page);
}
- if (options?.per_page) {
+ if (options?.per_page !== undefined) {
searchParams.per_page = String(options.per_page);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (options?.page) { | |
| searchParams.page = String(options.page); | |
| } | |
| if (options?.per_page) { | |
| searchParams.per_page = String(options.per_page); | |
| } | |
| if (options?.page !== undefined) { | |
| searchParams.page = String(options.page); | |
| } | |
| if (options?.per_page !== undefined) { | |
| searchParams.per_page = String(options.per_page); | |
| } |
🤖 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 `@packages/client/src/resources/monitors.ts` around lines 40 - 45, The query
param handling in monitors.ts is incorrectly gated on truthiness, so numeric
values like page=0 or per_page=0 are skipped instead of being serialized or
explicitly rejected. Update the parameter checks in the monitor request builder
to test for options.page and options.per_page being !== undefined, and keep any
1-based/positive validation separate from serialization so the behavior is
explicit and consistent.
| it('throws NotFoundError for an unknown project', async () => { | ||
| await expect(client.monitors.list(999)).rejects.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the specific error type here.
rejects.toThrow() will pass for any failure path, including transport or schema errors. If this is meant to verify NotFoundError mapping, assert that class explicitly.
🤖 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 `@packages/client/tests/integration/monitors.test.ts` around lines 23 - 25, The
test for an unknown project currently uses a generic rejection assertion, which
does not verify the intended error mapping. Update the
`client.monitors.list(999)` expectation in `monitors.test.ts` to assert the
specific `NotFoundError` class explicitly, using the same test case name and
`rejects` chain so the check confirms the correct error type rather than any
thrown failure.
| return HttpResponse.json({ | ||
| items: [ | ||
| { | ||
| id: 'c1c2c3c4-e89b-12d3-a456-426614174000', | ||
| status: 'ok', | ||
| duration: 12.5, | ||
| environment: 'production', | ||
| trace_id: null, | ||
| timestamp: '2026-06-18T00:00:01.000Z', | ||
| }, | ||
| { | ||
| id: 'd1d2d3d4-e89b-12d3-a456-426614174000', | ||
| status: 'error', | ||
| duration: null, | ||
| environment: null, | ||
| trace_id: null, | ||
| timestamp: '2026-06-17T00:00:01.000Z', | ||
| }, | ||
| ], | ||
| total_count: 2, | ||
| page: 1, | ||
| per_page: 20, | ||
| total_pages: 1, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
This fixture locks the new endpoint into offset pagination.
The mock response uses page/per_page/total_pages, which bakes the new monitors check-ins API into an offset-based contract. If this endpoint is meant to follow the repo standard, it should expose cursor pagination (next_cursor, has_more) instead, and the fixture/tests should match that before the contract spreads further.
As per coding guidelines, packages/client/src/**/*.ts must “Implement cursor-based pagination in all list endpoints using next_cursor and has_more fields.”
🤖 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 `@packages/client/tests/mocks/handlers.ts` around lines 1226 - 1248, The mock
fixture for the monitors check-ins list endpoint is using offset pagination
fields, which conflicts with the repo’s cursor-based list contract. Update the
response shape in the relevant handlers fixture to use the same cursor
pagination fields as other list endpoints, specifically `next_cursor` and
`has_more`, and remove the `page`/`per_page`/`total_pages` assumptions so tests
and the API mock stay aligned.
Source: Coding guidelines
| main().catch((err) => { | ||
| console.error(err); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For @sentry/node 10.59.0, what is the recommended way to flush pending events or check-ins before terminating a Node.js process after an uncaught error?
💡 Result:
In @sentry/node 10.59.0, flushing pending events before terminating a Node.js process after an uncaught error is handled by calling Sentry.flush(timeout) [1][2]. For reliable flushing when your application encounters a fatal error, you should invoke Sentry.flush(timeout) manually before exiting the process [3]. The flush method returns a Promise that resolves when all pending events have been sent or the specified timeout (in milliseconds) has elapsed [1]. Important Considerations: 1. Manual Flushing: When handling uncaught exceptions, you are responsible for calling Sentry.flush before your application calls process.exit [3]. Without this, because process.exit terminates the process synchronously, any ongoing network requests used to send events will be aborted, causing data loss [3]. 2. Default Integration: The OnUncaughtException integration, which is enabled by default, captures these errors and handles the process exit automatically [4]. If you wish to perform custom cleanup, you can configure this integration (using the onFatalError callback) or disable the default behavior and implement your own handler to ensure Sentry.flush is called appropriately [4][5]. 3. Flush vs. Close: Use Sentry.flush(timeout) if you intend to continue using the client momentarily. Use Sentry.close(timeout) if you are permanently shutting down the application, as it flushes all pending events and disables the SDK [1]. 4. Node.js Behavior: Node.js documentation advises that after an uncaughtException, the process is in an undefined state and should be terminated [6][7]. Performing asynchronous operations like flushing Sentry events after an uncaught error carries inherent risks, but Sentry.flush is the intended mechanism to attempt this reliably before termination [3]. For a timeout, it is recommended to provide a reasonable duration (e.g., 2000 ms) to balance between waiting for network requests and ensuring the process eventually terminates [8]. Note that in recent versions of @sentry/node, internal improvements have been made to ensure flushing is more robust [8][9], but manually awaiting the flush remains the standard practice for guaranteed delivery before exit.
Citations:
- 1: https://docs.sentry.io/platforms/javascript/guides/node/configuration/apis/
- 2: https://www.jsdocs.io/package/@sentry/core
- 3: Force Sentry to wait for every captureException call getsentry/sentry-javascript#15058
- 4: https://docs.sentry.io/platforms/javascript/guides/node/configuration/integrations/onuncaughtexception/
- 5: https://docs.sentry.io/platforms/javascript/guides/node/troubleshooting/
- 6: feat(node): Add option to
OnUncaughtExceptionintegration that allows mimicking native uncaught error exit behaviour getsentry/sentry-javascript#6137 - 7: Can't prevent Sentry SDK from exiting the process even with onFatalError getsentry/sentry-javascript#1661
- 8: client.flush() blocks process exit for full timeout duration due to non-unref'd timers getsentry/sentry-javascript#18996
- 9: fix(node): Make log flushing logic more robust getsentry/sentry-javascript#15991
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first, then inspect the nearby lines.
ast-grep outline packages/test-sentry/demo/src/crons.ts --view expanded || true
wc -l packages/test-sentry/demo/src/crons.ts
sed -n '1,180p' packages/test-sentry/demo/src/crons.ts
# Find any existing flush/close handling in this demo package.
rg -n "Sentry\.(flush|close)|flush\(" packages/test-sentry/demo -SRepository: rustrak/rustrak
Length of output: 4415
Flush before exiting on the error path. The success path already awaits Sentry.flush(2000), but main().catch(...) exits immediately, so the last batched check-ins can be dropped if main() rejects. Await a flush here before process.exit(1).
🤖 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 `@packages/test-sentry/demo/src/crons.ts` around lines 110 - 113, The error
path in main().catch currently exits immediately after logging, which can drop
pending Sentry events. Update the catch handler in crons.ts so it awaits
Sentry.flush(2000) before calling process.exit(1), matching the flush behavior
used on the success path and preserving any final batched check-ins.
Overview
Implements Sentry Crons (cron/scheduled-job monitoring) end-to-end — item type #4 of #143. Ground truth for the protocol is
relay-monitors/src/lib.rs(CheckIn,process_check_in); behaviour mirrors Relay so a Sentry SDK can't tell the difference.How it works (SDK-driven, like Sentry)
Monitors are not created in the dashboard — they're auto-upserted from SDK check-ins. A check-in carrying
monitor_configprovisions the monitor's schedule;in_progressthenok/error(sharing acheck_in_id) record the run.missed/timeoutare computed server-side (Relay rejects an inboundmissed).Layers
EnvelopeItemKind::CheckIn+CheckInProcessor(upsert monitor by(project, slug), schedule config viaCOALESCE,in_progress→oklifecycle upsert, derived status,next_expected_atfrom crontab/interval + timezone).monitors+check_instables (dual Postgres/SQLite).MonitorWorkerbackground loop marksmissed(overdue pastcheckin_margin) andtimeout(in-progress pastmax_runtime). Read API:GET /api/projects/{id}/monitorsand/monitors/{slug}/checkins. OpenAPI regenerated.@rustrak/client) —client.monitors.list()/listCheckIns()with Zod schemas.@rustrak/mcp) —list_monitors+list_monitor_check_instools.pnpm demo:cronsexercises the real@sentry/nodeSDK (withMonitor/captureCheckIn).New deps (server)
cron(crontab parsing) +chrono-tz(timezone-aware next-occurrence).Testing
cargo test --features postgres, testcontainers). Client 357 tests, MCP 78 tests, webview-uitsc/biome/next buildclean.0 0 * * *America/New_York → 04:00 UTC),error/lifecycle/idempotent upsert all correct.BIGINTon Postgres, notINTEGER) was caught during live testing and fixed; test placeholders switched to$Nso the suite runs under both engines.Config
MONITOR_TICK_INTERVAL_SECS(default 60) — how often the missed/timeout worker scans.Not included (open question)
Write operations (delete / mute-disable / manual create-edit) — Sentry offers these via UI/API, but the SDK-upsert read-only flow is the canonical path. Can follow up if wanted.
Summary by CodeRabbit
New Features
Bug Fixes
Tests