fix(search): address coderabbitai findings on PR #414

src/cache/store.ts

@@ -2,6 +2,7 @@ import { createHash } from 'node:crypto';
 import { getDatabase } from './db.js';
 import { getConfig } from '../config.js';
 import { createLogger } from '../logger.js';
+import { normaliseEngineList } from '../util/engine-list.js';
 import type { RawFetchResult, ExtractionResult, CachedContent, SearchResultItem, CacheStats, ContentCompleteness } from '../types.js';
 
 const log = createLogger('cache');
@@ -353,6 +354,7 @@ export interface SearchCacheFilters {
   exact_match?: boolean | null;
   search_depth?: string | null;
   reranker?: string | null;
+  search_engines?: string[] | null;
 }
 
 function normaliseDomainList(list?: string[] | null): string[] | null {
@@ -362,6 +364,10 @@ function normaliseDomainList(list?: string[] | null): string[] | null {
   return [...new Set(lower)].sort();
 }
 
+// Shared with the orchestrator's allowlist gates (src/util/engine-list.ts)
+// so the cache-key fingerprint and dispatch matching can never drift apart.
+export { normaliseEngineList } from '../util/engine-list.js';
+
 function hasAnyFilter(filters?: SearchCacheFilters): boolean {
   if (!filters) return false;
   return (
@@ -375,7 +381,8 @@ function hasAnyFilter(filters?: SearchCacheFilters): boolean {
     filters.time_range != null ||
     filters.exact_match != null ||
     filters.search_depth != null ||
-    filters.reranker != null
+    filters.reranker != null ||
+    normaliseEngineList(filters.search_engines) != null
   );
 }
 
@@ -400,6 +407,7 @@ export function buildSearchCacheKey(
     exact_match: filters!.exact_match ?? null,
     search_depth: filters!.search_depth ?? null,
     reranker: filters!.reranker ?? null,
+    search_engines: normaliseEngineList(filters!.search_engines),
   };
   return `${query}${JSON.stringify(fingerprint)}`;
 }

src/search/core/core-provider.ts

@@ -203,6 +203,7 @@ export class CoreSearchProvider implements SearchProvider {
       exact_match: input.exact_match,
       search_depth: depth,
       reranker: getConfig().reranker,
+      search_engines: input.search_engines,
     });
 
     let items: SearchResultItem[] = [];
@@ -395,6 +396,7 @@ export class CoreSearchProvider implements SearchProvider {
             country: input.country,
             timeRange: input.time_range,
             exactMatch: input.exact_match,
+            engineFilter: input.search_engines,
           }),
         ),
       );
@@ -452,6 +454,7 @@ export class CoreSearchProvider implements SearchProvider {
             country: input.country,
             timeRange: input.time_range,
             exactMatch: input.exact_match,
+            engineFilter: input.search_engines,
           });
           // RRF-merge the retry results on top of the initial dispatch so
           // we keep ranking signal from both passes.

src/search/core/orchestrator.ts

@@ -6,6 +6,7 @@ import type {
   SearchEngineOptions,
 } from '../../types.js';
 import { createLogger } from '../../logger.js';
+import { normaliseEngineList } from '../../util/engine-list.js';
 import {
   classifyIntentDetailed,
   extractErrorTokens,
@@ -211,6 +212,10 @@ export interface OrchestratorInput {
    * result whose title+snippet does not contain the unquoted query as a
    * case-insensitive substring is dropped post-rerank. */
   exactMatch?: boolean;
+  /** Caller-supplied engine allowlist. When non-empty, only engines whose
+   * name matches an entry (case-insensitive) are dispatched. Wired from
+   * SearchInput.search_engines via the MCP schema and CLI --search-engines. */
+  engineFilter?: string[];
 }
 
 export interface OrchestratorOutput {
@@ -303,6 +308,12 @@ interface RunV1SearchOptions {
   _isFallback?: boolean;
 }
 
+function applyEngineAllowlist(entries: EngineEntry[], allowlist: string[]): EngineEntry[] {
+  const lowered = allowlist.map((n) => n.toLowerCase());
+  const filtered = entries.filter((e) => lowered.includes(e.engine.name.toLowerCase()));
+  return filtered;
+}
+
 export async function runV1Search(
   input: OrchestratorInput,
   opts: RunV1SearchOptions = {},
@@ -365,9 +376,43 @@ export async function runV1Search(
   // Probe-only engines are held back from the primary wave: they are a
   // per-call latency/failure tax on the happy path but still an independent
   // signal the degraded-recovery wave can pull in when the pool collapses.
-  const entries = allEntries.filter((e) => e.probeOnly !== true);
+  let entries = allEntries.filter((e) => e.probeOnly !== true);
   const probeEntries = allEntries.filter((e) => e.probeOnly === true);
 
+  // Apply caller-supplied engine allowlist (SearchInput.search_engines).
+  // Normalise ONCE up front (trim, lowercase, dedupe, sort) and use that
+  // normalised list at every gate below — primary, probe fallback, recovery,
+  // and starvation backfill. Normalising at the gates (rather than raw-matching)
+  // keeps the orchestrator consistent with the cache-key fingerprint in
+  // cache/store.ts, which trims the same value: a whitespace-padded valid name
+  // like [' duckduckgo '] must dispatch ONLY that engine, not miss the
+  // allowlist and dispatch the full roster (which would then be cached under
+  // the trimmed single-engine key). An all-blank list normalises to null,
+  // i.e. treated as "no filter". Case-insensitive match against engine name.
+  // For the primary wave, if no entries match the allowlist, fall back to the
+  // full roster (the caller likely made a typo or passed an unknown engine
+  // name). For recovery and backfill waves, an empty result means those waves
+  // run nothing — which is correct: if the caller explicitly filtered out all
+  // probe/backfill engines, we don't secretly re-introduce them.
+  const engineAllowlist = normaliseEngineList(input.engineFilter);
+  if (engineAllowlist && engineAllowlist.length > 0) {
+    const allowlisted = applyEngineAllowlist(entries, engineAllowlist);
+    if (allowlisted.length > 0) {
+      entries = allowlisted;
+    } else {
+      // No NON-probe entry matched. Distinguish a caller typo from an explicit
+      // probe-only selection: if the filter names a configured probe-only
+      // engine (e.g. Mojeek with searchMojeekProbeOnly enabled), dispatch those
+      // probe-only engines rather than silently restoring the full primary
+      // roster and dispatching unselected engines. Fall back to the full roster
+      // ONLY when the filter matches no configured engine at all.
+      const probeAllowlisted = applyEngineAllowlist(probeEntries, engineAllowlist);
+      if (probeAllowlisted.length > 0) {
+        entries = probeAllowlisted;
+      }
+    }
+  }
+
   const options: SearchEngineOptions = {
     maxResults: input.maxResults ?? DEFAULT_MAX_RESULTS,
     timeoutMs: input.timeoutMs ?? DEFAULT_TIMEOUT_MS,
@@ -623,10 +668,30 @@ export async function runV1Search(
   const skippedPrimary = outcomes
     .filter((o) => o.skipped)
     .map((o) => o.engine);
-  const recoveryEntries = [
-    ...probeEntries,
+  // Engines that received an ATTEMPTED (non-skipped) primary dispatch. A
+  // probe-only engine selected via engineFilter and dispatched as the primary
+  // wave that returns zero results must NOT re-enter the recovery roster via
+  // probeEntries — that would fire a second external request and recovery wait
+  // against the same engine without probing a new one. Engines SKIPPED in the
+  // primary wave (breaker open) stay eligible: recovery is their retry path.
+  const attemptedPrimary = new Set(
+    outcomes.filter((o) => !o.skipped).map((o) => o.engine),
+  );
+  let recoveryEntries = [
+    ...probeEntries.filter((e) => !attemptedPrimary.has(e.engine.name)),
     ...entries.filter((e) => skippedPrimary.includes(e.engine.name)),
   ];
+  // Dedupe by engine name: a probe-only engine selected via engineFilter and
+  // skipped (breaker open) appears in both lists above.
+  const seenRecovery = new Set<string>();
+  recoveryEntries = recoveryEntries.filter((e) => {
+    if (seenRecovery.has(e.engine.name)) return false;
+    seenRecovery.add(e.engine.name);
+    return true;
+  });
+  if (engineAllowlist && engineAllowlist.length > 0) {
+    recoveryEntries = applyEngineAllowlist(recoveryEntries, engineAllowlist);
+  }
   if (
     outcomes.length > 0 &&
     primaryHealthy < poolHealthFloor(outcomes.length) &&
@@ -684,7 +749,10 @@ export async function runV1Search(
     vertical !== 'images' &&
     !opts._isFallback
   ) {
-    const generalEntries = getGeneralEngines();
+    let generalEntries = getGeneralEngines();
+    if (engineAllowlist && engineAllowlist.length > 0) {
+      generalEntries = applyEngineAllowlist(generalEntries, engineAllowlist);
+    }
     if (generalEntries.length > 0) {
       log.info('vertical starved below floor, backfilling from general', {
         from: vertical,

src/util/engine-list.ts

@@ -0,0 +1,16 @@
+/** Normalise a caller-supplied engine list: trim, lowercase, dedupe, sort.
+ * Mirrors the orchestrator's case-insensitive engine matching, so
+ * `['DuckDuckGo']`, `['duckduckgo']`, whitespace-padded, reordered, or
+ * duplicate-name lists — all of which dispatch the same engine set — hit
+ * the same allowlist gates AND produce identical cache keys. Shared by the
+ * cache-key fingerprint (cache/store.ts) and every engineFilter gate in the
+ * orchestrator so the two can never drift apart (a padded value that misses
+ * the dispatch allowlist but trims into the cache key would file a
+ * full-roster response under a single-engine key). An all-blank list
+ * normalises to null, i.e. "no filter". */
+export function normaliseEngineList(list?: string[] | null): string[] | null {
+  if (!list || list.length === 0) return null;
+  const lower = list.map((e) => e.toLowerCase().trim()).filter((e) => e.length > 0);
+  if (lower.length === 0) return null;
+  return [...new Set(lower)].sort();
+}

tests/unit/cache/store-search-key.test.ts

@@ -62,6 +62,26 @@ describe('buildSearchCacheKey', () => {
     expect(balanced).not.toBe(noRerank);
     expect(balanced).not.toBe('q'); // depth always present -> always fingerprinted
   });
+
+  it('normalises search_engines: casing, order, and duplicates share one key', () => {
+    const a = buildSearchCacheKey('q', { search_engines: ['DuckDuckGo'] });
+    const b = buildSearchCacheKey('q', { search_engines: ['duckduckgo'] });
+    const c = buildSearchCacheKey('q', { search_engines: ['brave', 'duckduckgo'] });
+    const d = buildSearchCacheKey('q', { search_engines: ['DuckDuckGo', 'brave'] });
+    const e = buildSearchCacheKey('q', { search_engines: ['duckduckgo', 'duckduckgo'] });
+    expect(a).toBe(b); // case-insensitive
+    expect(c).toBe(d); // order- and case-insensitive
+    expect(a).toBe(e); // duplicates collapse
+    expect(a).not.toBe(c); // different engine sets stay distinct
+  });
+
+  it('treats whitespace-only or empty search_engines as no filter', () => {
+    const bare = buildSearchCacheKey('q');
+    const empty = buildSearchCacheKey('q', { search_engines: [] });
+    const blanks = buildSearchCacheKey('q', { search_engines: ['  ', ''] });
+    expect(bare).toBe(empty);
+    expect(bare).toBe(blanks);
+  });
 });
 
 describe('cache miss on filter mismatch', () => {