-
Notifications
You must be signed in to change notification settings - Fork 0
fix(kpi): reject corrupt event metric evidence #441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ef02d67
9835130
84c785f
e4cbac0
e6db2ed
c06675c
a681b68
6bb224f
c5a7ee6
12a2b80
c39f310
16496e9
ddafb4d
943c43d
e4f578c
6bb1c04
79f2546
b1eab74
cbf5248
ed8bdd4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,14 @@ import { hasDuplicateJsonObjectKeys } from "./normalize-commercial-readiness-evi | |
|
|
||
| const fs = await import("node:fs/promises"); | ||
| const { existsSync } = await import("node:fs"); | ||
| const calendarDatePrefixPattern = /^(\d{4}-\d{2}-\d{2})(?:$|[T ])/; | ||
|
|
||
| const inputPath = process.argv[2] ?? "exchange-30d.ndjson"; | ||
| const failureThreshold = Number(process.argv[3] ?? "0.02"); | ||
| const p95Threshold = Number(process.argv[4] ?? "300"); | ||
| const requireWindowDays = Number(process.env.NOEMA_KPI_REQUIRE_WINDOW_DAYS); | ||
| const requireWindowDaysRaw = process.env.NOEMA_KPI_REQUIRE_WINDOW_DAYS; | ||
| const hasWindowRequirement = requireWindowDaysRaw !== undefined; | ||
| const requireWindowDays = hasWindowRequirement ? Number(requireWindowDaysRaw) : Number.NaN; | ||
|
|
||
| if (!inputPath) { | ||
| console.error("Usage: node scripts/check-kpi.mjs [wrangler-tail-ndjson] [failureThreshold] [p95ThresholdMs]"); | ||
|
|
@@ -20,13 +23,18 @@ if (!existsSync(inputPath)) { | |
| process.exit(1); | ||
| } | ||
|
|
||
| if (!Number.isFinite(failureThreshold) || !Number.isFinite(p95Threshold)) { | ||
| console.error("Invalid threshold values."); | ||
| if (!Number.isFinite(failureThreshold) || failureThreshold < 0 || failureThreshold > 1) { | ||
| console.error("KPI failure threshold must be between 0 and 1."); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (Number.isFinite(requireWindowDays) && requireWindowDays <= 0) { | ||
| console.error("NOEMA_KPI_REQUIRE_WINDOW_DAYS must be a positive number."); | ||
| if (!Number.isFinite(p95Threshold) || p95Threshold < 0) { | ||
| console.error("KPI p95 threshold must be non-negative and finite."); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (hasWindowRequirement && (!Number.isFinite(requireWindowDays) || requireWindowDays <= 0)) { | ||
| console.error("NOEMA_KPI_REQUIRE_WINDOW_DAYS must be a positive finite number."); | ||
| process.exit(1); | ||
| } | ||
|
|
||
|
|
@@ -71,19 +79,47 @@ for (const line of lines) { | |
| } | ||
|
|
||
| const route = resolveRoute(record); | ||
| const event = record.event || "http_request"; | ||
| if (route !== "/exchange" || event !== "http_request") continue; | ||
| if (route !== "/exchange") continue; | ||
| 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; | ||
|
|
||
| exchanges += 1; | ||
|
|
||
| const status = Number(record.status_code || record.status || record.response?.status); | ||
| if (Number.isNaN(status) || status >= 400) failures += 1; | ||
| 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); | ||
| } | ||
|
Comment on lines
+91
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Info: Missing status/latency now hard-fails the gate An Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| latencies.push(latency); | ||
|
|
||
| const ts = resolveTimestampMs(record); | ||
| if (Number.isNaN(ts)) { | ||
| console.error("Invalid exchange timestamp in KPI log; refusing normalized calendar evidence."); | ||
| process.exit(1); | ||
| } | ||
| if (ts != null) { | ||
| if (!Number.isFinite(ts) || ts < 0) { | ||
| console.error("Invalid exchange timestamp in KPI log; expected a finite non-negative timestamp."); | ||
| process.exit(1); | ||
| } | ||
| if (ts > Date.now()) { | ||
| console.error("Invalid exchange timestamp in KPI log; timestamp cannot be in the future."); | ||
| process.exit(1); | ||
| } | ||
|
Comment on lines
+119
to
+122
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Info: Future-timestamp rejection extends to non-windowed runs The Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| exchangesWithTimestamp += 1; | ||
| minTimestampMs = Math.min(minTimestampMs, ts); | ||
| maxTimestampMs = Math.max(maxTimestampMs, ts); | ||
|
|
@@ -99,7 +135,6 @@ latencies.sort((a, b) => a - b); | |
| const p95Index = Math.max(0, Math.ceil((0.95 * latencies.length) - 1)); | ||
| const p95 = latencies.length ? latencies[p95Index] : null; | ||
| const failureRate = failures / exchanges; | ||
| const hasWindowRequirement = Number.isFinite(requireWindowDays); | ||
| const requiredWindowMs = hasWindowRequirement ? requireWindowDays * 24 * 60 * 60 * 1000 : null; | ||
| const exchangeWindowMs = Number.isFinite(minTimestampMs) && Number.isFinite(maxTimestampMs) | ||
| ? maxTimestampMs - minTimestampMs | ||
|
|
@@ -172,6 +207,14 @@ function resolveTimestampMs(record) { | |
| return candidate; | ||
| } | ||
| 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); | ||
|
Comment on lines
209
to
220
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Info: Some string timestamp formats silently ignored
(Refers to this code) Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; | ||
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { spawnSync } from "node:child_process"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| function runCheckKpi(logPath: string) { | ||
| const env = { ...process.env }; | ||
| delete env.NOEMA_KPI_REQUIRE_WINDOW_DAYS; | ||
| return spawnSync(process.execPath, ["scripts/check-kpi.mjs", logPath, "0.02", "300"], { | ||
| cwd: process.cwd(), | ||
| encoding: "utf8", | ||
| env, | ||
| }); | ||
| } | ||
|
|
||
| function expectInvalidEventIdentity(record: Record<string, unknown>) { | ||
| const dir = mkdtempSync(join(tmpdir(), "noema-check-kpi-event-")); | ||
| try { | ||
| const logPath = join(dir, "exchange-30d.ndjson"); | ||
| writeFileSync(logPath, `${JSON.stringify(record)}\n`); | ||
|
|
||
| const result = runCheckKpi(logPath); | ||
|
|
||
| expect(result.status).toBe(1); | ||
| expect(result.stderr).toContain("exchange record is missing canonical http_request event identity"); | ||
| expect(result.stdout).toBe(""); | ||
| } finally { | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| } | ||
| } | ||
|
|
||
| describe("KPI event authority", () => { | ||
| it("rejects an exchange-shaped record that omits the canonical http_request event identity", () => { | ||
| expectInvalidEventIdentity({ | ||
| route: "/exchange", | ||
| status_code: 200, | ||
| latency_ms: 10, | ||
| }); | ||
| }); | ||
|
|
||
| it("rejects an empty event identity instead of treating it as a canonical http_request", () => { | ||
| expectInvalidEventIdentity({ | ||
| event: "", | ||
| route: "/exchange", | ||
| status_code: 200, | ||
| latency_ms: 10, | ||
| }); | ||
| }); | ||
|
|
||
| it("rejects a whitespace-only event identity instead of excluding an exchange observation", () => { | ||
| expectInvalidEventIdentity({ | ||
| event: " \t", | ||
| route: "/exchange", | ||
| status_code: 500, | ||
| latency_ms: 10, | ||
| }); | ||
| }); | ||
|
|
||
| it("continues to ignore explicitly non-http_request events on the exchange route", () => { | ||
| const dir = mkdtempSync(join(tmpdir(), "noema-check-kpi-event-ignore-")); | ||
| try { | ||
| const logPath = join(dir, "exchange-30d.ndjson"); | ||
| writeFileSync(logPath, [ | ||
| JSON.stringify({ | ||
| event: "workflow_trust", | ||
| route: "/exchange", | ||
| status_code: 403, | ||
| latency_ms: 1, | ||
| }), | ||
| JSON.stringify({ | ||
| event: "http_request", | ||
| route: "/exchange", | ||
| status_code: 200, | ||
| latency_ms: 10, | ||
| }), | ||
| "", | ||
| ].join("\n")); | ||
|
|
||
| const result = runCheckKpi(logPath); | ||
|
|
||
| expect(result.status).toBe(0); | ||
| expect(result.stderr).toBe(""); | ||
| expect(JSON.parse(result.stdout)).toMatchObject({ | ||
| exchange_requests: 1, | ||
| exchange_failures: 0, | ||
| pass: true, | ||
| }); | ||
| } finally { | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; | ||
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { spawnSync } from "node:child_process"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| function runCheckKpi(logPath: string) { | ||
| const env = { ...process.env }; | ||
| delete env.NOEMA_KPI_REQUIRE_WINDOW_DAYS; | ||
| return spawnSync(process.execPath, ["scripts/check-kpi.mjs", logPath, "0.02", "300"], { | ||
| cwd: process.cwd(), | ||
| encoding: "utf8", | ||
| env, | ||
| }); | ||
| } | ||
|
|
||
| describe("KPI latency evidence completeness", () => { | ||
| it("rejects a canonical exchange event that omits latency instead of shrinking the p95 sample", () => { | ||
| const dir = mkdtempSync(join(tmpdir(), "noema-check-kpi-latency-")); | ||
| try { | ||
| const logPath = join(dir, "exchange-30d.ndjson"); | ||
| writeFileSync(logPath, [ | ||
| JSON.stringify({ | ||
| event: "http_request", | ||
| route: "/exchange", | ||
| status_code: 200, | ||
| latency_ms: 10, | ||
| }), | ||
| JSON.stringify({ | ||
| event: "http_request", | ||
| route: "/exchange", | ||
| status_code: 200, | ||
| }), | ||
| "", | ||
| ].join("\n")); | ||
|
|
||
| const result = runCheckKpi(logPath); | ||
|
|
||
| expect(result.status).toBe(1); | ||
| expect(result.stderr).toContain("exchange latency is required"); | ||
| expect(result.stdout).toBe(""); | ||
| } finally { | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Behavior change without a CHANGELOG entry
This PR changes the fail-closed exit behavior of the KPI check script but adds no
## Unreleasedentry toCHANGELOG.md, which CONTRIBUTING.md and CLAUDE.md require for every behavior change. The diff touches only the script and tests.Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.