feat: add public event validator [agent:clawd]#10
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 56 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds SignalOps event normalization and validation, an API endpoint, a browser-based public validator, and deterministic checks for event identity, redaction, diagnostics, request limits, and validation responses. ChangesSignalOps validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ValidatorPage
participant ValidateRoute
participant EventValidator
ValidatorPage->>ValidateRoute: POST event payload
ValidateRoute->>EventValidator: validateSignalEventRequest(request)
EventValidator-->>ValidateRoute: status and validation response
ValidateRoute-->>ValidatorPage: JSON validation result
ValidatorPage-->>ValidatorPage: render counts, diagnostics, and previews
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
scripts/test-signalops-events.mjs (2)
160-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test doesn't isolate the early Content-Length short-circuit.
tooLargePayloadplus its wrapping quotes already exceedsmaxBodyBytes, so the fallback UTF-8 byte-length check (independent of thecontent-lengthheader) would also yield 413 here. This test can't distinguish whetherrequestContentLengthExceedsitself is actually triggering the rejection. Consider a dedicated case with a small real body and an inflatedcontent-lengthheader to verify the early-rejection path specifically.🤖 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 `@scripts/test-signalops-events.mjs` around lines 160 - 167, The test around validateSignalEventRequest does not isolate the requestContentLengthExceeds early-rejection path. Replace or supplement the oversized-body case with a small valid JSON body whose inflated content-length header exceeds SIGNALOPS_EVENT_LIMITS.maxBodyBytes, and assert status 413; keep the actual body below the limit so fallback UTF-8 validation cannot cause the rejection.
30-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
cost.recordedvalidation branch.The suite exercises
generation.started/completed/retryingandprovider.health, but nevercost.recorded, even thoughnormalizeSignalEventhas a dedicated required-field check for it (providerId+cost). Consider adding a positive case and a negative case (missingcost/providerId) to lock in this behavior.🤖 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 `@scripts/test-signalops-events.mjs` around lines 30 - 263, Add positive and negative validation coverage near the existing event normalization and batch assertions for the `cost.recorded` type. Use `normalizeSignalEvent` or `normalizeSignalEventBatch` to verify a valid event containing both required `providerId` and `cost` fields succeeds, and verify cases missing either field are rejected with the expected validation error.src/app/validate/page.tsx (3)
143-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOverly complex type derivation for
readinessTone's parameter.This nested
extends infer T ? T extends {...} ? R : never : neveridiom is used to force distribution over the optionaldiagnosticsunion, but it's needlessly hard to read for whatNonNullable<...>["readiness"]already achieves directly.♻️ Proposed simplification
-function readinessTone(readiness: ValidationResponse["diagnostics"] extends infer T - ? T extends { readiness: infer R } - ? R - : never - : never) { +function readinessTone(readiness: NonNullable<ValidationResponse["diagnostics"]>["readiness"]) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/validate/page.tsx` around lines 143 - 166, Update the readinessTone parameter type to use NonNullable<ValidationResponse["diagnostics"]>["readiness"] instead of the nested conditional infer expression. Preserve the existing readinessTone behavior and return values unchanged.
364-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
aria-live="polite"scope is too broad.The live region wraps the entire result card — icon, heading, accepted/rejected counts, coverage chips, gaps, next actions, rejected list, and accepted preview (through line 519). Every validation run causes screen readers to re-announce this whole block, which is excessive; scope
aria-liveto just the status heading/summary text instead.♻️ Proposed refactor
- <div - aria-live="polite" - className="rounded-[28px] border border-white/75 bg-white/90 p-5 shadow-[var(--shadow-2)] backdrop-blur sm:p-6" - > - <div className="flex items-center gap-3"> + <div className="rounded-[28px] border border-white/75 bg-white/90 p-5 shadow-[var(--shadow-2)] backdrop-blur sm:p-6"> + <div aria-live="polite" className="flex items-center gap-3">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/validate/page.tsx` around lines 364 - 368, Move aria-live="polite" from the outer result-card container to the status heading/summary element within the validation result markup, keeping the live region limited to the validation status text and counts. Leave the icon, coverage details, actions, rejected list, and accepted preview outside the live region.
6-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate hand-rolled response type instead of the shared contract type.
ValidationResponsere-declares the shape already defined server-side asSignalEventValidationSummary/SignalEventValidationResponseinsrc/lib/signalops/events.ts. Since this is a type-only need, it can be imported withimport type(erased at compile time, safe for a"use client"file) instead of hand-copied, removing drift risk if the server contract changes.♻️ Proposed refactor
-type ValidationResponse = { - ok: boolean; - code?: string; - error?: string; - partial?: boolean; - verificationOnly?: boolean; - storedEvents?: number; - validEvents?: number; - rejectedEvents?: number; - eventTypes?: string[]; - providerIds?: string[]; - modelIds?: string[]; - requestId?: string; - limits?: { - maxBodyBytes: number; - maxBatchEvents: number; - }; - diagnostics?: { - readiness: "insufficient" | "partial" | "pilot_ready"; - coverage: { - generationLifecycle: { - started: boolean; - completed: boolean; - failed: boolean; - retrying: boolean; - }; - providerHealth: boolean; - latency: boolean; - cost: boolean; - retries: boolean; - providers: number; - models: number; - }; - gaps: string[]; - nextActions: string[]; - }; - rejected?: Array<{ index: number; error: string }>; - acceptedPreview?: Array<Record<string, unknown>>; -}; +import type { SignalEventValidationResponse } from "`@/lib/signalops/events`"; + +// Keep fields optional here since we're parsing an untrusted network response. +type ValidationResponse = Partial<SignalEventValidationResponse>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/validate/page.tsx` around lines 6 - 44, Replace the hand-rolled ValidationResponse type with the shared SignalEventValidationSummary or SignalEventValidationResponse contract from the signal events module. Add it as a type-only import so the "use client" file has no runtime dependency, and update local references to use the imported contract while removing the duplicate declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/api/events/validate/route.ts`:
- Around line 9-12: Wrap the POST handler’s validateSignalEventRequest call in a
defensive try/catch so unexpected failures still return the documented JSON
error shape, including ok, code, error, and requestId fields, with an
appropriate server-error status. Preserve the existing response.body and
response.status behavior for successful validation and handled errors.
- Line 1: Update the NextResponse import in the route to use the exported
`next/server` subpath instead of `next/server.js`, preserving the existing
NextResponse usage.
In `@src/app/validate/page.tsx`:
- Line 338: Update the payload textarea’s onChange handler to clear the existing
copied, error, and result state before or alongside setPayload, matching the
reset behavior in loadSample. Ensure editing the JSON immediately removes stale
validation feedback and previous results.
- Around line 203-221: Update the validation request flow around the fetch call
to use an AbortController with a finite timeout, pass its signal to fetch, and
abort the request when the timeout expires. Preserve the existing setError
handling and ensure setIsValidating(false) still runs through the existing
finally block after timeout or other request failures.
- Line 340: Replace the textarea’s outline-none utility in the className at the
validation page textarea with outline-hidden, preserving all other styling
classes unchanged.
In `@src/lib/signalops/events.ts`:
- Around line 509-535: The validateSignalEventRequest function currently buffers
the entire request via request.text() before enforcing the body limit. Replace
that read with incremental streaming from request.body, tracking UTF-8 byte
length and stopping as soon as SIGNALOPS_EVENT_LIMITS.maxBodyBytes is exceeded,
while preserving the existing 413 error response and downstream body parsing for
valid payloads.
---
Nitpick comments:
In `@scripts/test-signalops-events.mjs`:
- Around line 160-167: The test around validateSignalEventRequest does not
isolate the requestContentLengthExceeds early-rejection path. Replace or
supplement the oversized-body case with a small valid JSON body whose inflated
content-length header exceeds SIGNALOPS_EVENT_LIMITS.maxBodyBytes, and assert
status 413; keep the actual body below the limit so fallback UTF-8 validation
cannot cause the rejection.
- Around line 30-263: Add positive and negative validation coverage near the
existing event normalization and batch assertions for the `cost.recorded` type.
Use `normalizeSignalEvent` or `normalizeSignalEventBatch` to verify a valid
event containing both required `providerId` and `cost` fields succeeds, and
verify cases missing either field are rejected with the expected validation
error.
In `@src/app/validate/page.tsx`:
- Around line 143-166: Update the readinessTone parameter type to use
NonNullable<ValidationResponse["diagnostics"]>["readiness"] instead of the
nested conditional infer expression. Preserve the existing readinessTone
behavior and return values unchanged.
- Around line 364-368: Move aria-live="polite" from the outer result-card
container to the status heading/summary element within the validation result
markup, keeping the live region limited to the validation status text and
counts. Leave the icon, coverage details, actions, rejected list, and accepted
preview outside the live region.
- Around line 6-44: Replace the hand-rolled ValidationResponse type with the
shared SignalEventValidationSummary or SignalEventValidationResponse contract
from the signal events module. Add it as a type-only import so the "use client"
file has no runtime dependency, and update local references to use the imported
contract while removing the duplicate declaration.
🪄 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
Run ID: 9ac9d0d5-9549-4738-aaeb-35cbde7af412
📒 Files selected for processing (4)
scripts/test-signalops-events.mjssrc/app/api/events/validate/route.tssrc/app/validate/page.tsxsrc/lib/signalops/events.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/test-signalops-events.mjs (1)
288-304: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the negative-duration test isolate the telemetry rule.
The test only asserts that the event is rejected, so it can remain green if rejection is caused by another field instead of
durationMs: -1. Use a known-valid timestamp and assert the rejection diagnostic specifically mentionsdurationMs.🐛 Proposed fix
{ type: "generation.completed", - occurredAt: "3026-07-12T12:00:00.000Z", + occurredAt: isoOffset(-60_000), generationId: "gen_invalid_001", providerId: "fal", modelId: "flux-kontext-pro", durationMs: -1, },Also assert the rejected item's existing diagnostic field contains
durationMs.🤖 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 `@scripts/test-signalops-events.mjs` around lines 288 - 304, Update the negative-duration test event in the noValidResponse setup to use a known-valid timestamp, ensuring rejection is isolated to durationMs: -1. Extend the assertions for noValidJson.rejectedEvents to verify the rejected item’s existing diagnostic field specifically mentions durationMs, while preserving the current response-count assertions.
🤖 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.
Outside diff comments:
In `@scripts/test-signalops-events.mjs`:
- Around line 288-304: Update the negative-duration test event in the
noValidResponse setup to use a known-valid timestamp, ensuring rejection is
isolated to durationMs: -1. Extend the assertions for noValidJson.rejectedEvents
to verify the rejected item’s existing diagnostic field specifically mentions
durationMs, while preserving the current response-count assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f1b129f-0595-4017-9dbf-3d2936229b16
📒 Files selected for processing (4)
scripts/test-signalops-events.mjssrc/app/api/events/validate/route.tssrc/app/validate/page.tsxsrc/lib/signalops/events.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/validate/page.tsx
- src/lib/signalops/events.ts
1 participant
Why now
SignalOps already has a strong guided cockpit replay, but prospective integrations had no safe way to test the event contract. This recovers one bounded, useful slice from the broader uncommitted productization experiment without bringing in auth, storage, SDK, or pilot-workflow scope.
What changed
POST /api/events/validateendpointuserandpromptin accepted previews/validateplayground with editable samples, readiness coverage, gaps, and next actionsVerification
node --experimental-strip-types --disable-warning=MODULE_TYPELESS_PACKAGE_JSON scripts/test-signalops-events.mjs./node_modules/.bin/eslint ../node_modules/.bin/tsc --noEmit --pretty false./node_modules/.bin/next buildgpt-5.6-solmodel; manual Codex review usedgpt-5.4insteadagent:clawd
Ready for Ilya's manual review and merge. No auto-merge enabled.
Summary by CodeRabbit
New Features
Tests