Centralize environment variable access through CONFIG

[stevekrenzel]

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

[claude]

Claude finished @stevekrenzel's task —— View job

---

Code Review: Centralize environment variable access through CONFIG

Review Tasks

• Read all 9 changed files
• Review config.ts changes
• Review llm_client.ts changes
• Review groq.int.test.ts changes
• Review groq.ts changes
• Review reranker.groq-default.unit.test.ts changes
• Review reranker.int.test.ts changes
• Review reranker.unit.test.ts changes
• Review types.ts changes
• Review vitest.config.ts changes
• Compile final review feedback

---

Summary

This PR successfully centralizes environment variable access through the CONFIG object. The implementation is solid and improves testability. However, there are a few areas where the code doesn't fully comply with the Intent library's strict engineering standards.

🟢 Strengths

1. Excellent Architecture: Centralizing env vars through CONFIG is the right pattern - improves testability and type safety
2. Clean Migration: All files consistently use CONFIG.X.Y instead of process.env.X
3. Better Test Isolation: Using vi.doMock and module mocking instead of mutating process.env is much cleaner
4. Good Comment Cleanup: Removing the obsolete comment from types.ts (line 48) shows attention to detail

🟡 Issues Found

1. Missing Braces on If Statements (vitest.config.ts:33-42)

Severity: High - Violates mandatory code style

The ternary operator on lines 33-42 creates an implicit single-branch structure that should use braces:

// Current (lines 33-42)
...(scope === "int"
  ? {}
  : {
      thresholds: {
        lines: 100,
        statements: 100,
        functions: 100,
        branches: 100,
      },
    }),

Why this matters: While this is a ternary (not an if statement), the CLAUDE.md guidelines emphasize "Always use braces" to avoid single-line conditionals. This structure is functionally equivalent to a conditional branch and should be refactored for consistency.

Suggested fix: Extract this logic into a helper function with explicit early returns:

function getCoverageThresholds(scope: string) {
  if (scope === "int") {
    return {};
  }
  
  return {
    thresholds: {
      lines: 100,
      statements: 100,
      functions: 100,
      branches: 100,
    },
  };
}

// Then in config:
coverage: {
  ...baseCoverageConfig,
  ...getCoverageThresholds(scope),
}

2. Missing JSDoc Documentation (config.ts:7-23)

Severity: Medium - Violates "Document Everything" principle

The CONFIG object lacks JSDoc documentation explaining:

• What the config object contains
• When values are loaded (module initialization)
• How to override values in tests

Suggested addition (before line 7):

/**
 * Global configuration object for Intent library.
 * 
 * Centralizes all environment variable access with type-safe defaults.
 * Values are loaded once at module initialization via dotenv/config.
 * 
 * In tests, mock this module using vi.doMock() to override values:
 * 
 * @example
 * ```typescript
 * vi.doMock("./config", () => ({
 *   CONFIG: {
 *     GROQ: { API_KEY: "test-key" },
 *     // ... rest of config
 *   },
 * }));
 * ```
 * 
 * @see lib/config for the underlying config helpers
 */
export const CONFIG = {

3. Missing JSDoc on TEST.SCOPE (config.ts:20-22)

Severity: Low - New config property should be documented

The new TEST.SCOPE property is undocumented. While the other properties have JSDoc in CLAUDE.md, this new addition should be documented inline.

Suggested fix (before line 20):

  TEST: {
    /** Test execution scope: "unit", "int", or "all" (default) */
    SCOPE: string("TEST_SCOPE", { default: "all" }),
  },

4. Ternary Chain Could Be Simplified (vitest.config.ts:6-11)

Severity: Low - Minor readability issue

The nested ternary for includePatterns could be more readable with early returns.

Current (lines 6-11):

const includePatterns =
  scope === "unit"
    ? ["src/**/*.unit.test.ts"]
    : scope === "int"
      ? ["src/**/*.int.test.ts"]
      : ["src/**/*.unit.test.ts", "src/**/*.int.test.ts"];

Suggested refactor:

/**
 * Get test file patterns based on scope.
 * 
 * @param scope - Test scope from CONFIG
 * @returns Array of glob patterns for test files
 * @private
 */
function getTestPatterns(scope: string): string[] {
  if (scope === "unit") {
    return ["src/**/*.unit.test.ts"];
  }
  if (scope === "int") {
    return ["src/**/*.int.test.ts"];
  }
  return ["src/**/*.unit.test.ts", "src/**/*.int.test.ts"];
}

const includePatterns = getTestPatterns(scope);

This follows the "early returns with braces" pattern and makes the logic more testable.

✅ Code Quality Checklist

• No functions over 50 lines
• TypeScript strict mode compliance
• No any types without justification
• No hardcoded values (all use CONFIG)
• No commented-out code
• No console.log statements
• Files under 300 lines
• Proper import organization
• All if statements use braces (vitest.config.ts has nested ternaries)
• All functions have JSDoc (missing on CONFIG object and helper opportunities)
• Test isolation (excellent use of vi.doMock)
• Type safety maintained

📝 Additional Observations

1. Test Quality: The test changes are exemplary - proper use of vi.doMock for module isolation instead of global process.env mutation
2. Backward Compatibility: All existing environment variables work exactly as before
3. Integration Test Skipping: Proper use of describe.skipIf(!hasKey) pattern is clean and idiomatic
4. No Breaking Changes: This is purely an internal refactor with zero API surface changes

🎯 Recommendation

Approve with minor revisions. The architectural change is excellent and the implementation is solid. The issues found are style/documentation violations that should be addressed to maintain the library's strict quality standards, but they don't affect functionality.

Priority fixes:

1. Add JSDoc to CONFIG object (medium priority)
2. Extract vitest.config.ts ternaries into early-return functions (low-medium priority)
3. Add inline docs for TEST.SCOPE (low priority)

The core change - centralizing CONFIG access - is implemented correctly and significantly improves the codebase.

---