Repository navigation
fix(governance): broken featureFlags imports + 3 silent fail-opens + extras test #507
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
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 |
|---|---|---|
|
|
@@ -125,7 +125,7 @@ async function startServer() { | |
| // first request after restart. Gated on the feature flag so the | ||
| // default-off behaviour is unchanged. | ||
| try { | ||
| const { isFeatureFlagEnabled } = await import("./lib/featureFlags"); | ||
| const { isFeatureFlagEnabled } = await import("./shared/utils/featureFlags"); | ||
| const { getSelfHealingManager } = await import("./lib/resilience/anomalyHook"); | ||
| if (isFeatureFlagEnabled("OMNIROUTE_SELF_HEALING_ENABLED")) { | ||
| const mgr = getSelfHealingManager(); | ||
|
|
@@ -135,7 +135,7 @@ async function startServer() { | |
| void mgr; // referenced to ensure the singleton is constructed | ||
| } | ||
| } catch (err) { | ||
| startupLog.warn({ err }, "Self-healing hydration skipped (non-fatal)"); | ||
| startupLog.error({ err }, "Self-healing hydration skipped (non-fatal)"); | ||
|
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. WARNING: Log severity says "error" but message says "non-fatal" The Reply with |
||
| } | ||
|
|
||
| startupLog.info("Server started with cloud sync initialized"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,9 @@ import { WebSocketServer, WebSocket } from "ws"; | |
| import { jwtVerify } from "jose"; | ||
| import { createServer, type IncomingMessage, type ServerResponse } from "http"; | ||
| import { randomUUID } from "crypto"; | ||
| import { createLogger } from "@/shared/utils/logger"; | ||
|
|
||
| const log = createLogger("ws:live-server"); | ||
|
|
||
| // ── Types ───────────────────────────────────────────────────────────────── | ||
|
|
||
|
|
@@ -476,7 +479,12 @@ export async function startLiveDashboardServer( | |
| // does not block the event loop on a cold import — which would starve concurrent | ||
| // WebSocket handshakes (see loadAuthModule). A failed warm is non-fatal: the | ||
| // handler retries the import lazily. | ||
| await loadAuthModule().catch(() => {}); | ||
| await loadAuthModule().catch((err) => | ||
| log.error( | ||
| { err }, | ||
| "liveServer: failed to warm auth module — clients may experience cold-import latency" | ||
| ) | ||
| ); | ||
|
Comment on lines
+482
to
+487
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. Suggestion: The warm-up catches the rejected dynamic import, but Severity Level: Major
|
||
|
|
||
| wss.on("connection", async (ws, request) => { | ||
| const pendingMessages: string[] = []; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| // @vitest-environment node | ||
| import { describe, it, expect, beforeEach, afterEach } from "vitest"; | ||
| import { | ||
| getKeyvQuotaStore, | ||
| __resetKeyvQuotaStoreForTests, | ||
| } from "../../../src/lib/quota/keyvQuotaStore"; | ||
| import type { ProviderPlan, QuotaPool } from "../../../src/lib/quota/dimensions"; | ||
|
|
||
| /** | ||
| * Sanity tests for the "extras" surface of KeyvQuotaStore: | ||
| * recordPlanUsage, upsertProviderPlan, listProviderPlans, setPools, getPool. | ||
| * | ||
| * These methods were originally scoped for a KeyvQuotaStoreExtras class per | ||
| * plans/quota-keystore-type-drift-spec.md §8.2, but were folded directly into | ||
| * KeyvQuotaStore before the spec landed. We exercise them here against the | ||
| * in-memory backing to guarantee the surface stays wired correctly. | ||
| */ | ||
| describe("KeyvQuotaStoreExtras", () => { | ||
| let store: ReturnType<typeof getKeyvQuotaStore>; | ||
|
|
||
| beforeEach(() => { | ||
| __resetKeyvQuotaStoreForTests(); | ||
| store = getKeyvQuotaStore({ uri: "memory://" }); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await store.dispose(); | ||
| __resetKeyvQuotaStoreForTests(); | ||
| }); | ||
|
|
||
| it("recordPlanUsage returns a PlanPoolUsage shape with totalConsumed and lastUpdatedAt populated", async () => { | ||
| const rollup = await store.recordPlanUsage( | ||
| "conn-1", | ||
| "openai", | ||
| "pool-1", | ||
| [{ unit: "tokens", window: "hourly" }], | ||
| 42, | ||
| ); | ||
| expect(rollup).toBeDefined(); | ||
| expect(rollup.totalConsumed).toBe(42); | ||
| expect(typeof rollup.lastUpdatedAt).toBe("number"); | ||
| expect(rollup.lastUpdatedAt).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it("upsertProviderPlan writes the plan without throwing", async () => { | ||
| const plan: ProviderPlan = { | ||
| connectionId: "conn-1", | ||
| provider: "openai", | ||
| dimensions: [{ unit: "tokens", window: "hourly", limit: 1000 }], | ||
| source: "manual", | ||
| }; | ||
| await expect(store.upsertProviderPlan(plan)).resolves.toBeUndefined(); | ||
| }); | ||
|
|
||
| it("listProviderPlans returns [] even after an upsert (independent surface)", async () => { | ||
| // upsertProviderPlan and listProviderPlans are independent: the in-memory | ||
| // store does not enumerate provider plans, so the list stays empty. | ||
| await store.upsertProviderPlan({ | ||
| connectionId: "conn-1", | ||
| provider: "openai", | ||
| dimensions: [{ unit: "tokens", window: "hourly", limit: 1000 }], | ||
| source: "manual", | ||
| }); | ||
| const plans = await store.listProviderPlans(); | ||
| expect(plans).toEqual([]); | ||
| }); | ||
|
|
||
| it("setPools persists a QuotaPool retrievable via getPool", async () => { | ||
| const pool: QuotaPool = { | ||
| id: "pool-1", | ||
| connectionId: "conn-1", | ||
| name: "Primary", | ||
| createdAt: new Date().toISOString(), | ||
| allocations: [], | ||
| }; | ||
| await store.setPools([pool]); | ||
| const retrieved = await store.getPool("pool-1"); | ||
| expect(retrieved).toBeDefined(); | ||
| expect(retrieved?.id).toBe("pool-1"); | ||
| expect(retrieved?.name).toBe("Primary"); | ||
| expect(retrieved?.allocations).toEqual([]); | ||
| }); | ||
|
|
||
| it("getPool returns undefined for an unknown poolId", async () => { | ||
| const result = await store.getPool("nonexistent-pool"); | ||
| expect(result).toBeUndefined(); | ||
| }); | ||
| }); |
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.
SUGGESTION:
alive: falseconflates "dead" with "unreadable"The catch now returns
{ pid, alive: false }on any read failure. This is correct for the documented race-condition case (process died betweenisProcessRunningand the/proc/psread), but it also maps transient failures (e.g. permissions, platform quirks) toalive: false. If future callers usegetProcessInfoto drive restart decisions, a temporarily unreadable alive process could be restarted unnecessarily. Consider adding anunknownstate or a separatereadableflag so callers can distinguish "confirmed dead" from "state unreadable".Reply with
@kilocode-bot fix itto have Kilo Code address this issue.