From ef971d81b1429400aade7493bf9e6e9b303aca55 Mon Sep 17 00:00:00 2001 From: Steve Krenzel Date: Tue, 28 Oct 2025 01:10:05 -0700 Subject: [PATCH] [NO-TICKET] Centralize environment variable access through CONFIG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem The codebase had inconsistent environment variable access patterns: - Direct `process.env` access in multiple files (llm_client.ts, groq tests, vitest.config.ts) - Tests manipulating `process.env` directly, causing side effects between tests - No single source of truth for environment configuration - Vitest config importing dotenv separately from the rest of the application This made the code harder to test, maintain, and reason about. Mock strategies were fragile and required manual process.env manipulation. ## Solution Centralized all environment variable access through the CONFIG object in config.ts: 1. **Added TEST.SCOPE configuration**: New `TEST_SCOPE` environment variable in CONFIG.TEST for controlling test execution scope 2. **Updated llm_client.ts**: Changed from `process.env.GROQ_API_KEY` to `CONFIG.GROQ.API_KEY` 3. **Updated groq integration tests**: Use CONFIG instead of direct process.env access 4. **Updated vitest.config.ts**: Import CONFIG instead of dotenv, use `CONFIG.TEST.SCOPE` instead of `process.env.TEST_SCOPE` 5. **Improved unit test mocking**: - groq-default unit test now mocks CONFIG module instead of manipulating process.env - reranker unit test uses vi.doMock to properly isolate CONFIG changes 6. **Updated groq provider JSDoc**: Example now shows CONFIG usage instead of process.env 7. **Removed obsolete comment**: Deleted incorrect comment about RerankerConfig in types.ts ## Benefits - **Single source of truth**: All environment variables accessed through CONFIG - **Better testability**: Tests mock CONFIG module instead of global process.env - **Type safety**: CONFIG provides typed access to all configuration - **Cleaner tests**: No more side effects from process.env manipulation - **Consistent patterns**: All files follow the same configuration access pattern - **Better documentation**: Examples and tests demonstrate the correct usage pattern ## Implementation Details - Used Vitest's `vi.doMock` for isolating CONFIG changes in tests that need different values - Maintained backward compatibility - all existing environment variables work the same way - Integration tests properly skip when GROQ_API_KEY is not available using CONFIG check - vitest.config.ts now imports CONFIG directly, eliminating duplicate dotenv loading ## Edge Cases Handled - Empty string API keys are treated as missing (falsy check remains) - Test scope defaults to "all" if not specified - Module isolation in unit tests prevents cross-test pollution 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/config.ts | 3 ++ src/llm_client.ts | 3 +- src/providers/groq.int.test.ts | 7 ++-- src/providers/groq.ts | 3 +- src/reranker.groq-default.unit.test.ts | 45 +++++++++++++++----------- src/reranker.int.test.ts | 3 +- src/reranker.unit.test.ts | 28 ++++++++++++---- src/types.ts | 2 -- vitest.config.ts | 5 +-- 9 files changed, 65 insertions(+), 34 deletions(-) diff --git a/src/config.ts b/src/config.ts index 0f02f96..15395a1 100644 --- a/src/config.ts +++ b/src/config.ts @@ -17,4 +17,7 @@ export const CONFIG = { BATCH_SIZE: int("INTENT_BATCH_SIZE", { default: 20, min: 1 }), TINY_BATCH_FRACTION: number("INTENT_TINY_BATCH_FRACTION", { default: 0.2, min: 0, max: 1 }), }, + TEST: { + SCOPE: string("TEST_SCOPE", { default: "all" }), + }, } as const; diff --git a/src/llm_client.ts b/src/llm_client.ts index 39b9592..2300e70 100644 --- a/src/llm_client.ts +++ b/src/llm_client.ts @@ -1,3 +1,4 @@ +import { CONFIG } from "./config"; import { createDefaultGroqClient } from "./providers/groq"; import type { LlmClient, IntentContext } from "./types"; @@ -17,7 +18,7 @@ export function selectLlmClient(ctx: IntentContext): LlmClient | undefined { if (ctx.llm) { return ctx.llm; } - const groqKey = process.env.GROQ_API_KEY; + const groqKey = CONFIG.GROQ.API_KEY; if (groqKey && groqKey !== "") { return createDefaultGroqClient(groqKey); } diff --git a/src/providers/groq.int.test.ts b/src/providers/groq.int.test.ts index 3ebb9bd..cb20bd6 100644 --- a/src/providers/groq.int.test.ts +++ b/src/providers/groq.int.test.ts @@ -1,15 +1,16 @@ import { describe, expect, test } from "vitest"; +import { CONFIG } from "../config"; import { buildMessages } from "../messages"; import { buildRelevancySchema } from "../schema"; import { createDefaultGroqClient } from "./groq"; -const hasKey = Boolean(process.env.GROQ_API_KEY); +const hasKey = Boolean(CONFIG.GROQ.API_KEY); describe.skipIf(!hasKey)("groq provider integration", () => { test.concurrent("provider returns scores for all schema keys", async () => { - const client = createDefaultGroqClient(process.env.GROQ_API_KEY!); + const client = createDefaultGroqClient(CONFIG.GROQ.API_KEY); const candidates = [ { key: "A", summary: "first" }, { key: "B", summary: "second" }, @@ -28,7 +29,7 @@ describe.skipIf(!hasKey)("groq provider integration", () => { }); test.concurrent("assigns 0 to unrelated and >0 to related", async () => { - const client = createDefaultGroqClient(process.env.GROQ_API_KEY!); + const client = createDefaultGroqClient(CONFIG.GROQ.API_KEY); const candidates = [ { key: "JS Arrays", summary: "Guide to sorting arrays in JavaScript" }, { key: "Banana Bread Recipe", summary: "How to bake banana bread" }, diff --git a/src/providers/groq.ts b/src/providers/groq.ts index 52a2b39..e4229a7 100644 --- a/src/providers/groq.ts +++ b/src/providers/groq.ts @@ -146,7 +146,8 @@ function shouldRetry(err: any, remaining: number): boolean { * * @example * ```typescript - * const client = createDefaultGroqClient(process.env.GROQ_API_KEY!); + * import { CONFIG } from "../config"; + * const client = createDefaultGroqClient(CONFIG.GROQ.API_KEY); * const result = await client.call(messages, schema, { model: "llama-3.3-70b" }); * ``` */ diff --git a/src/reranker.groq-default.unit.test.ts b/src/reranker.groq-default.unit.test.ts index b62417e..0aece9c 100644 --- a/src/reranker.groq-default.unit.test.ts +++ b/src/reranker.groq-default.unit.test.ts @@ -11,27 +11,36 @@ vi.mock("groq-sdk", () => ({ }, })); +// Mock CONFIG to have a test key +vi.mock("./config", async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + CONFIG: { + ...original.CONFIG, + GROQ: { + ...original.CONFIG.GROQ, + API_KEY: "test-key", + }, + }, + }; +}); + const { Reranker } = await import("./reranker"); describe("Reranker (default Groq) ", () => { test("uses groq-sdk when GROQ_API_KEY is set", async () => { - const oldKey = process.env.GROQ_API_KEY; - process.env.GROQ_API_KEY = "test-key"; - try { - const reranker = new Reranker<{ key: string; summary: string }>( - { - /* no llm */ - }, - { key: (x) => x.key, summary: (x) => x.summary }, - ); - const out = await reranker.rerank("q", [ - { key: "A", summary: "" }, - { key: "B", summary: "" }, - ]); - expect(out.map((c) => c.key)).toEqual(["A"]); - expect(callMock.mock.calls.length).toBe(1); - } finally { - process.env.GROQ_API_KEY = oldKey; - } + const reranker = new Reranker<{ key: string; summary: string }>( + { + /* no llm */ + }, + { key: (x) => x.key, summary: (x) => x.summary }, + ); + const out = await reranker.rerank("q", [ + { key: "A", summary: "" }, + { key: "B", summary: "" }, + ]); + expect(out.map((c) => c.key)).toEqual(["A"]); + expect(callMock.mock.calls.length).toBe(1); }); }); diff --git a/src/reranker.int.test.ts b/src/reranker.int.test.ts index 44832d4..84a73df 100644 --- a/src/reranker.int.test.ts +++ b/src/reranker.int.test.ts @@ -1,8 +1,9 @@ import { describe, expect, test } from "vitest"; +import { CONFIG } from "./config"; import { Reranker } from "./reranker"; -const hasKey = Boolean(process.env.GROQ_API_KEY); +const hasKey = Boolean(CONFIG.GROQ.API_KEY); describe.skipIf(!hasKey)("reranker integration", () => { test.concurrent( diff --git a/src/reranker.unit.test.ts b/src/reranker.unit.test.ts index 4f56680..8f63c8c 100644 --- a/src/reranker.unit.test.ts +++ b/src/reranker.unit.test.ts @@ -26,14 +26,30 @@ function makeCtx(overrides: Partial = {}): IntentContext & { describe("Reranker.rerank", () => { test("throws when no llm and no GROQ_API_KEY", async () => { - const oldKey = process.env.GROQ_API_KEY; - delete (process.env as any).GROQ_API_KEY; + const { CONFIG } = await import("./config"); + try { - expect(() => new Reranker({} as any, { key: (c) => c.key })).toThrow( - /No LLM client provided/, - ); + // Mock CONFIG to return empty API key + vi.doMock("./config", () => ({ + CONFIG: { + ...CONFIG, + GROQ: { + ...CONFIG.GROQ, + API_KEY: "", + }, + }, + })); + + // Re-import modules to get mocked config + vi.resetModules(); + const { Reranker: RerankerWithMock } = await import("./reranker"); + + expect( + () => new RerankerWithMock({} as any, { key: (c) => c.key }), + ).toThrow(/No LLM client provided/); } finally { - process.env.GROQ_API_KEY = oldKey; + vi.doUnmock("./config"); + vi.resetModules(); } }); diff --git a/src/types.ts b/src/types.ts index 0a12c65..ea86ad8 100644 --- a/src/types.ts +++ b/src/types.ts @@ -36,8 +36,6 @@ export type RerankerCandidate = { summary: string; }; -// RerankerConfig moved to reranker_config.ts to centralize defaults/env/validation - export type RerankerExtractors = { key: (item: T) => string; summary?: (item: T) => string; diff --git a/vitest.config.ts b/vitest.config.ts index e9cf4cf..e8cd821 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,7 +1,8 @@ -import "dotenv/config"; import { defineConfig } from "vitest/config"; -const scope = process.env.TEST_SCOPE ?? "all"; // unit | int | all +import { CONFIG } from "./src/config"; + +const scope = CONFIG.TEST.SCOPE; const includePatterns = scope === "unit" ? ["src/**/*.unit.test.ts"]