fix(kpi): reject corrupt event metric evidence - #441
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (typeof record.event !== "string" || record.event.trim().length === 0) { | ||
| console.error("KPI exchange record is missing canonical http_request event identity."); | ||
| process.exit(1); | ||
| } | ||
| if (record.event !== "http_request") continue; |
There was a problem hiding this comment.
🟡 Behavior change without a CHANGELOG entry
This PR changes the fail-closed exit behavior of the KPI check script but adds no ## Unreleased entry to CHANGELOG.md, which CONTRIBUTING.md and CLAUDE.md require for every behavior change. The diff touches only the script and tests.
Prompt for agents
CONTRIBUTING.md and CLAUDE.md require that CHANGELOG.md's `## Unreleased` section be updated with every behavior change. This PR hardens scripts/check-kpi.mjs to fail closed on corrupt/incomplete exchange evidence and invalid threshold/window configuration, which is a behavior change, but CHANGELOG.md is not modified. Add a short `## Unreleased` bullet (Korean, matching the existing changelog style) describing the new KPI evidence integrity rejections.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (ts > Date.now()) { | ||
| console.error("Invalid exchange timestamp in KPI log; timestamp cannot be in the future."); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
📝 Info: Future-timestamp rejection extends to non-windowed runs
The ts > Date.now() check runs for every exchange record, not only under a window requirement, so the default kpi:check now hard-fails on any timestamp ahead of the checker's clock. Edge-vs-runner clock skew could in principle reject valid evidence, though checks normally run well after collection.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (typeof candidate === "string") { | ||
| const calendarMatch = calendarDatePrefixPattern.exec(candidate); | ||
| if (calendarMatch) { | ||
| const datePart = calendarMatch[1]; | ||
| const calendarDate = new Date(`${datePart}T00:00:00.000Z`); | ||
| if (Number.isNaN(calendarDate.getTime()) || calendarDate.toISOString().slice(0, 10) !== datePart) { | ||
| return Number.NaN; | ||
| } | ||
| } | ||
| const parsed = Date.parse(candidate); | ||
| if (!Number.isNaN(parsed)) return parsed; | ||
| const numeric = Number(candidate); |
There was a problem hiding this comment.
📝 Info: Some string timestamp formats silently ignored
resolveTimestampMs returns null for string formats that miss the calendar prefix and fail Date.parse/Number (e.g. 2026-02-01Z), silently excluding the record from window evidence rather than rejecting it. Pre-existing and unaffected by this PR.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| const status = record.status_code ?? record.status ?? record.response?.status; | ||
| if (typeof status !== "number" || !Number.isInteger(status) || status < 100 || status > 599) { | ||
| console.error("Invalid exchange HTTP status in KPI log; expected an integer from 100 through 599."); | ||
| process.exit(1); | ||
| } | ||
| if (status >= 400) failures += 1; | ||
|
|
||
| const latency = Number(record.latency_ms || record.latencyMs || record.duration_ms); | ||
| if (!Number.isNaN(latency)) latencies.push(latency); | ||
| const latency = record.latency_ms ?? record.latencyMs ?? record.duration_ms; | ||
| if (latency === undefined || latency === null) { | ||
| console.error("KPI exchange latency is required for every canonical http_request event."); | ||
| process.exit(1); | ||
| } | ||
| if (typeof latency !== "number" || !Number.isFinite(latency) || latency < 0) { | ||
| console.error("Invalid exchange latency in KPI log; expected a finite non-negative number."); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
📝 Info: Missing status/latency now hard-fails the gate
An /exchange http_request record lacking a valid integer status or finite non-negative latency now exits the whole check, where before a missing status counted as a failure and a missing latency was just dropped from the p95 sample. Any legacy log missing these fields would now fail the KPI gate.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Scope
Harden
scripts/check-kpi.mjsso corrupt or incomplete exchange-event evidence and invalid threshold/window configuration cannot improve or manufacture KPI results. This is repository-owned KPI evidence integrity under issue #3; it does not fabricate production logs or relax KPI targets.Verified defects and TDD lineage
0 mslatency.c39f310c...→ candidate16496e93...: failure-rate threshold must be finite and in[0,1]; p95 threshold must be finite and non-negative.943c43dc...:/exchangeevidence must carry explicit canonicalevent: "http_request"; missing/empty event identity fails closed while explicitly different event types remain excluded.79f25466...→ candidateb1eab749...: every canonical exchange event must contain latency evidence; missing/null latency cannot silently shrink the p95 sample.cbf524805e33754139fe8f3c12f0041aca3f5cdb: a whitespace-only event identity on/exchangemust fail as missing authority rather than being treated as an explicitly different event type and excluded from KPI observations.ed8bdd4557a86361ea8537f26be7991c58550d27: event authority is nonempty after trimming before non-http_requestevents can be excluded. RED→candidate production delta is exactly one predicate change inscripts/check-kpi.mjs(+1/-1).Existing route selection, status/latency numeric validation, timestamp-unit handling, duplicate-key/UTF-8 defenses and non-JSON Wrangler diagnostic tolerance remain intact.
Current exact-head evidence
c746aafc24fe1ea25f74fa4da9fc2c355f90e2a7ed8bdd4557a86361ea8537f26be7991c58550d2732493734330: queued32493734370: pending32493734531: queuedQueued/pending evidence is non-passing; predecessor runs are not reused.
Merge boundary
Keep Draft. Do not merge until application CI, reviewer-ci, eligible central Security Scan, exact review/thread state, live protected base and central scanner authority are freshly terminal-clean on this unchanged exact head. Production KPI evidence remains external and fail-closed.