2 Commits

Author	SHA1	Message	Date
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
Dragan Spiridonov	94033e3500	feat(llm-router): Phase 1 — kernel singleton + persistence + Gemini key alias ... Wires the foundation for ADR-043's HybridRouter into the kernel boot sequence so domain plugins can finally receive an llmRouter dependency. Phase 1 lands the kernel/CLI/provider changes; Phase 2 will plumb the router through all 13 domains. Background: the LLM-enhancement code paths added in ADR-051 (isLLMAnalysisAvailable() across 14 services) were unreachable because no construction site ever passed an llmRouter — services accepted the optional dependency but no caller injected it. ProviderManager + HybridRouter were only instantiated from the `aqe llm advise` CLI subcommand. Changes - src/shared/llm/providers/gemini.ts: read GOOGLE_API_KEY as a third fallback after GOOGLE_AI_API_KEY and GEMINI_API_KEY, so users with the conventional Google key name don't need to alias it themselves. - src/shared/llm/router/config-store.ts (new): load/save .agentic-qe/llm-config.json with env-detection layering (defaults < disk < env < explicit override). Strips apiKey fields defensively before persisting. Filters out providers the ProviderManager doesn't yet construct (onnx) to keep boot logs clean. - src/shared/llm/llm-router-service.ts (new): builds a HybridRouter from the loaded config, picks primary/fallbacks from the available set honoring the user's defaultProvider when possible. Accepts an injected ProviderManager for tests. - src/kernel/interfaces.ts: extends QEKernel with llmRouter? and KernelConfig with llmRouter.enabled ('auto'|true|false) + configOverride + providerManager test-injection. - src/kernel/kernel.ts: kernel-singleton llmRouter built during initialize(), exposed via kernel.llmRouter, forwarded to all 13 DOMAIN_FACTORIES as a 4th arg. Phase 2 wires each factory through to its coordinator/services; for now the wrappers accept-and-discard. - src/cli/commands/llm-router.ts: replaces the `// would be persisted in real implementation` stub with real read/write through the config-store. `aqe llm config --set` now survives a process restart. Adds AQE_CONFIG_ROOT env var as a test seam so tests don't mutate the real .agentic-qe/. Tests (110 total, all passing) - gemini.test.ts: +3 new tests for each env-var alias, existing tests updated to clear all three names - config-store.test.ts (new, 29 tests): load/save/merge precedence, env detection, secret stripping, atomicity - llm-router-service.test.ts (new, 10 tests): pickEnabledProviders, pickPrimaryAndFallbacks, end-to-end with injected mock provider - kernel-llm-router.test.ts (new, 4 tests): kernel boot with router enabled=false / auto-no-providers / injected mock, chat() round-trip - llm-router.test.ts (CLI): +2 persistence-to-disk tests, env-isolated - tests/mocks/index.ts: shared createMockLLMProvider() helper Verification - npx tsc --noEmit: clean - 110 tests pass; no regressions in existing kernel.test.ts (34/34) - Stderr noise from PersistentSONAEngine init is pre-existing and unrelated to this change. What this commit does NOT do - Does NOT change behavior for any user yet — DOMAIN_FACTORIES wrappers still discard the llmRouter param. Phase 2 lights up the domain services. Refs ADR-043 (vendor-independent LLM), ADR-051 (LLM-enhanced services) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>	2026-05-21 09:38:40 +00:00