Files
Dragan Spiridonov 3bc09cda6d fix(llm-router): address 10 findings from qe-devils-advocate audit
A post-Phase-4 devil's-advocate audit identified 10 issues. All addressed
with code fixes + tests. Some were real bugs (silent billing regression,
2 unwired MCP tools, 2 HybridRouter routing bugs); others were robustness
improvements (split singletons, silent errors, decorative type fields).

Findings → fixes (severity tagged in audit report):

1. CRITICAL — silent billing regression on upgrade
   Users with ANTHROPIC_API_KEY in env would silently start paying for
   Claude API calls on every Fleet operation. The CHANGELOG's "opt out
   via code changes" wasn't sufficient — needed an env-only escape.
   Fix: AQE_LLM_ROUTER_DISABLED=1 env kill-switch honored in both
   QEKernelImpl._initializeLLMRouter and src/mcp/tools/base.ts
   getSharedLLMRouter. Truthy values disable; 'false'/'0'/'no'/'off'/''
   pass. Unified parse rule between kernel and MCP base.

2. HIGH — 2 unwired MCP standalone tools (audit said 3; QualityCriteria
   isn't LLM-aware so excluded)
   src/mcp/tools/test-generation/generate.ts:82 was calling
   createTestGeneratorService(memory) without router. Same for
   visual-accessibility/index.ts:177. Both now use the
   createXxxServiceWithDependencies factories with { memory, llmRouter }.

3. CRITICAL — E2E only tested manual routing mode, not production
   Production default is rule-based. Adding two new E2E tests for
   rule-based mode SURFACED two real bugs in HybridRouter that I fixed:

   3a. executeWithFallback hardcoded ['claude','openai','ollama'] as the
       only fallback providers; gemini/openrouter/azure-openai/bedrock
       users had no fallback even though ProviderManager can construct
       them. Plus the user's defaultProvider wasn't auto-included in the
       chain. Both fixed at src/shared/llm/router/hybrid-router.ts:612.

   3b. A non-retryable error from one provider (e.g. Claude "invalid
       x-api-key") aborted the entire fallback loop via `throw error`,
       defeating the whole point of a fallback chain. Fixed: skip the
       inter-attempt delay for non-retryable but continue to the next
       fallback entry.

4. HIGH — MCPToolContext.llmRouter field was decorative
   Added the field but no code populated it. Every getLLMRouter(context)
   call fell through to getSharedLLMRouter().
   Fix: added llmRouter and memory to BaseMCPTool.invoke() options;
   qe-tool-bridge.ts and registry.ts now explicitly forward the shared
   singleton via { llmRouter } at invoke time.

5. HIGH — 14 of 15 wired services had only STRUCTURAL verification, not
   behavioral (only TestExecutor had a chat() round-trip test)
   Fix: new tests/integration/llm-router-behavioral.test.ts asserts
   isLLMAnalysisAvailable()===true (the exact gate the audit named) for
   all 14 services + 2 full behavioral round-trips. 16 tests total.

6. HIGH — MCP and kernel built SEPARATE HybridRouter instances
   Different cost trackers, caches, circuit breakers per side.
   routing_economics/routing_metrics would only see half the traffic.
   Fix: kernel.ts._initializeLLMRouter calls setSharedLLMRouter(router)
   on init so both paths share one instance. kernel.dispose() calls
   resetSharedLLMRouter() so the next kernel doesn't inherit the
   previous instance.

7. HIGH — Silent error swallowing in _initializeLLMRouter
   console.warn'd then continued without LLM, with no signal for
   monitoring. A misconfigured provider would silently degrade with no
   alert.
   Fix: now publishes kernel.llm-router.init-failed (caught error) and
   kernel.llm-router.init-no-provider (enabled=true but nothing
   available) events on the event bus. Test confirms event delivery.

8. MEDIUM — Service count mismatch in docs (ADR/CHANGELOG said "14
   services / 12 domains"; test enumerates 15 services / 11
   LLM-enhanced domains)
   Fix: ADR-043 addendum + CHANGELOG corrected to 15/11. Added
   explicit service list and noted the 2 non-LLM domains
   (enterprise-integration, coordination).

9. MEDIUM — aqe llm config --set apiKey=... silently dropped the key
   stripApiKeys() returned a successfully-saved file without the key,
   user thought it was saved.
   Fix: CLI updateConfig() now throws a clear error pointing to env
   vars BEFORE calling save. stripApiKeys also now logs a console.warn
   when it strips anything, so programmatic callers see the strip.

10. LOW — Env vs disk precedence was counterintuitive and undocumented
    Force-enable-on-env overrides explicit `enabled: false` on disk.
    Fix: doc comment in config-store.applyEnvProviderDetection +
    ADR-043 "Precedence & env-var conventions" section explains.

Verification
- npx tsc --noEmit: clean
- 191 unit + integration tests pass (16 new behavioral tests for #5,
  17 wiring tests, gemini provider, config-store, llm-router-service,
  kernel-llm-router with 17 tests including all kill-switch cases,
  CLI persistence with 28 tests)
- 5 real-provider E2E tests pass (Gemini 2.5 Flash, OpenAI gpt-4o-mini,
  OpenRouter, plus 2 rule-based-mode tests that previously failed and
  now pass with the HybridRouter fallback fixes)

Refs ADR-043 audit findings (2026-05-21)
Builds on 94033e3 (Phase 1), be1e60d (Phase 2), c206c81 (Phase 3+4)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-21 12:09:18 +00:00

313 lines
11 KiB
TypeScript

/**
* Tests for ADR-043 kernel ↔ LLM router wiring.
*
* Covers:
* - kernel.llmRouter is undefined when llmRouter.enabled = false
* - kernel.llmRouter is undefined when no provider is available (auto)
* - kernel.llmRouter is built when an injected ProviderManager is provided
* - the router is forwarded to plugin factories as the 4th arg
* - a chat() call round-trips through the mock provider
*/
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
import * as fs from 'fs';
import * as os from 'os';
import * as path from 'path';
import { QEKernelImpl } from '../../../src/kernel/kernel';
import { resetUnifiedMemory } from '../../../src/kernel/unified-memory';
import { ProviderManager } from '../../../src/shared/llm/provider-manager';
import { createMockLLMProvider } from '../../mocks';
import {
getSharedLLMRouter,
resetSharedLLMRouter,
} from '../../../src/mcp/tools/base';
let tmpRoot: string;
beforeEach(() => {
tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'aqe-kernel-llm-'));
});
afterEach(async () => {
resetUnifiedMemory();
// ADR-043: clear shared singleton between tests so each test gets a
// fresh router. Kernel.dispose() does this when a router was built,
// but tests that fail mid-setup might not reach dispose.
resetSharedLLMRouter();
// Defensive: also clear the env kill-switch tests may have set.
delete process.env.AQE_LLM_ROUTER_DISABLED;
if (fs.existsSync(tmpRoot)) {
fs.rmSync(tmpRoot, { recursive: true, force: true });
}
});
/** Build a ProviderManager pre-loaded with a mock provider — bypasses
* network on initialize(). Same trick as in llm-router-service.test.ts. */
function makeMockProviderManager(): { pm: ProviderManager; stats: ReturnType<typeof createMockLLMProvider>['stats'] } {
const { provider, stats } = createMockLLMProvider({ type: 'claude' });
const pm = new ProviderManager({
primary: 'claude',
fallbacks: [],
providers: { claude: { model: 'mock-model' } as any },
});
(pm as any).providers.set('claude', provider);
(pm as any).initialized = true;
(pm as any).initializeMetrics('claude');
return { pm, stats };
}
describe('Kernel ↔ LLM router wiring (ADR-043)', () => {
it('does not build a router when llmRouter.enabled is false', async () => {
const kernel = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: false },
});
await kernel.initialize();
expect(kernel.llmRouter).toBeUndefined();
await kernel.dispose();
});
it('does not build a router in auto mode when no provider is available', async () => {
// Save and clear any provider keys from the host env so this test
// is hermetic (the dev shell may have keys exported from .env).
const saved = {
ANTHROPIC_API_KEY: process.env.ANTHROPIC_API_KEY,
OPENAI_API_KEY: process.env.OPENAI_API_KEY,
OPENROUTER_API_KEY: process.env.OPENROUTER_API_KEY,
GEMINI_API_KEY: process.env.GEMINI_API_KEY,
GOOGLE_AI_API_KEY: process.env.GOOGLE_AI_API_KEY,
GOOGLE_API_KEY: process.env.GOOGLE_API_KEY,
};
for (const k of Object.keys(saved)) delete process.env[k];
try {
const kernel = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: 'auto' },
});
await kernel.initialize();
expect(kernel.llmRouter).toBeUndefined();
await kernel.dispose();
} finally {
for (const [k, v] of Object.entries(saved)) {
if (v !== undefined) process.env[k] = v;
}
}
});
it('builds a router when an injected ProviderManager is supplied', async () => {
const { pm } = makeMockProviderManager();
const kernel = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: true, providerManager: pm },
});
await kernel.initialize();
expect(kernel.llmRouter).toBeDefined();
await kernel.dispose();
});
it('round-trips a chat() call through the injected mock provider', async () => {
const { pm, stats } = makeMockProviderManager();
const kernel = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: true, providerManager: pm },
});
await kernel.initialize();
const router = kernel.llmRouter as any;
const response = await router.chat({
messages: [{ role: 'user', content: 'hello from a domain plugin' }],
model: 'mock-model',
});
expect(response.content).toBe('mock response');
expect(stats.generateCalls).toBe(1);
await kernel.dispose();
});
describe('AQE_LLM_ROUTER_DISABLED env kill-switch', () => {
it('AQE_LLM_ROUTER_DISABLED=1 prevents the router from being built even with enabled=true', async () => {
process.env.AQE_LLM_ROUTER_DISABLED = '1';
const { pm } = makeMockProviderManager();
const kernel = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: true, providerManager: pm },
});
await kernel.initialize();
expect(kernel.llmRouter).toBeUndefined();
await kernel.dispose();
});
it.each(['true', 'yes', 'on'])(
'AQE_LLM_ROUTER_DISABLED=%s also disables the router',
async (value) => {
process.env.AQE_LLM_ROUTER_DISABLED = value;
const { pm } = makeMockProviderManager();
const kernel = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: true, providerManager: pm },
});
await kernel.initialize();
expect(kernel.llmRouter).toBeUndefined();
await kernel.dispose();
}
);
it.each(['', '0', 'false', 'no', 'off'])(
'AQE_LLM_ROUTER_DISABLED=%s does NOT disable the router (router still builds)',
async (value) => {
process.env.AQE_LLM_ROUTER_DISABLED = value;
const { pm } = makeMockProviderManager();
const kernel = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: true, providerManager: pm },
});
await kernel.initialize();
expect(kernel.llmRouter).toBeDefined();
await kernel.dispose();
}
);
it('kill-switch also short-circuits the MCP getSharedLLMRouter() path', async () => {
process.env.AQE_LLM_ROUTER_DISABLED = '1';
const shared = await getSharedLLMRouter();
expect(shared).toBeNull();
});
});
describe('Shared singleton unification (Fix #6)', () => {
it('kernel registers its router as the shared singleton so MCP sees the same instance', async () => {
const { pm } = makeMockProviderManager();
const kernel = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: true, providerManager: pm },
});
await kernel.initialize();
const shared = await getSharedLLMRouter();
expect(shared).toBe(kernel.llmRouter);
await kernel.dispose();
});
it('kernel.dispose() clears the shared singleton so the next kernel does not inherit it', async () => {
const { pm: pm1 } = makeMockProviderManager();
const kernel1 = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: true, providerManager: pm1 },
});
await kernel1.initialize();
const router1 = kernel1.llmRouter;
await kernel1.dispose();
// After dispose, the shared singleton is cleared; building a fresh
// kernel with a fresh provider produces a DIFFERENT router instance.
const { pm: pm2 } = makeMockProviderManager();
const kernel2 = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: true, providerManager: pm2 },
});
await kernel2.initialize();
expect(kernel2.llmRouter).not.toBe(router1);
await kernel2.dispose();
});
});
describe('Init failure observability (Fix #7)', () => {
it('publishes kernel.llm-router.init-no-provider event when enabled=true but no provider available', async () => {
// Hermetic: clear all provider keys
const saved: Record<string, string | undefined> = {
ANTHROPIC_API_KEY: process.env.ANTHROPIC_API_KEY,
OPENAI_API_KEY: process.env.OPENAI_API_KEY,
OPENROUTER_API_KEY: process.env.OPENROUTER_API_KEY,
GEMINI_API_KEY: process.env.GEMINI_API_KEY,
GOOGLE_AI_API_KEY: process.env.GOOGLE_AI_API_KEY,
GOOGLE_API_KEY: process.env.GOOGLE_API_KEY,
};
for (const k of Object.keys(saved)) delete process.env[k];
try {
const kernel = new QEKernelImpl({
memoryBackend: 'memory',
enabledDomains: [],
enableExperienceBridge: false,
enableDreamScheduler: false,
dataDir: tmpRoot,
llmRouter: { enabled: true },
});
const captured: Array<{ type: string; payload: unknown }> = [];
kernel.eventBus.subscribe('kernel.llm-router.init-no-provider', async (e: any) => {
captured.push({ type: e.type, payload: e.payload });
});
await kernel.initialize();
// Wait a tick for the event to propagate.
await new Promise((r) => setImmediate(r));
expect(captured.length).toBe(1);
expect(captured[0].type).toBe('kernel.llm-router.init-no-provider');
expect((captured[0].payload as any).reason).toContain('no provider available');
await kernel.dispose();
} finally {
for (const [k, v] of Object.entries(saved)) {
if (v !== undefined) process.env[k] = v;
}
}
});
});
});