fix(memory): widen the #13003 skip guard, narrow selector_skipped

Two review findings on the opt-in skip-selector guard, each reproduced at
b4e0bba5b8 before fixing.

The uniqueness test counted only the published fast list -- the top
MAX_RELEVANT_DOCS of the lexical ranking -- while the selector it suppresses
is handed modelCandidates, whose lexical half is the top
MAX_MODEL_CANDIDATE_DOCS - RECENT_MODEL_CANDIDATE_RESERVE, roughly 36x wider.
A second strong title/keyword match ranked 6th or lower was therefore
invisible to the guard: an ambiguous recall skipped the selector and still
reported selector_skipped: true, landing in the treatment arm as a "unique"
strong hit. selectModelCandidateDocuments now also returns the strong-match
count over the lexical array it already scored. The count is computed only
while the guard is live, so knob-off recalls pay nothing on a path that
carries a latency budget, and the flag is now read once per recall so the
count and the guard cannot disagree.

logRecallResult also stamped selector_skipped: false on the empty-query /
empty-corpus / non-positive-limit short circuit, where the selector is never
called. Those trivially fast recalls joined the ablation's control arm, which
the treatment arm structurally cannot contain, and recordMemoryRecallMetrics
puts only strategy and selector_skipped on the counter and histogram, so the
samples could not be sliced back out. logRecallResult now takes the caller's
decision state and omits the field when no skip decision was made; the
heuristic fallback keys off selectorStartedAt so a selector failure still
reports false as a genuine control sample.

Both fixes are mutation-checked: reverting either one reds exactly the tests
added for it (1 failure and 2 failures respectively).

Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmurnh6s41i

docs/developers/development/telemetry.md

@@ -752,7 +752,7 @@ The following events are logged:
   - **Attributes**: `trigger` ("auto"/"manual"), `status` ("updated"/"noop"/"failed"/"cancelled"), `deduped_entries` (int), `touched_topics_count` (int), `touched_topics` (string), `duration_ms` (int)
 
 - `qwen-code.memory.recall`: Memory recall operation completed.
-  - **Attributes**: `query_length` (int), `docs_scanned` (int), `docs_selected` (int), `strategy` ("none"/"heuristic"/"model"), `duration_ms` (int), `selector_skipped` (bool, optional — set for structured-mode recalls; false when the selector runs, including when the #13003 knob is off)
+  - **Attributes**: `query_length` (int), `docs_scanned` (int), `docs_selected` (int), `strategy` ("none"/"heuristic"/"model"), `duration_ms` (int), `selector_skipped` (bool, optional — set for structured-mode recalls that made a skip decision, so not on the empty-query/empty-corpus short circuit; false when the selector runs, including when the #13003 knob is off)
 
 #### Prompt Suggestion & Speculation Events
 

packages/core/src/memory/recall.test.ts

@@ -1033,6 +1033,45 @@ describe('auto-memory relevant recall', () => {
       expect(event.selector_skipped).toBeUndefined();
     });
 
+    it('leaves selector_skipped unset when an empty corpus short-circuits the recall', async () => {
+      // The short circuit at the top of the entry point never reaches the
+      // selector, so the recall made no skip decision. `false` here would read
+      // as "the selector ran and was not skipped" and would put a trivially
+      // fast recall into the ablation's control arm, which the treatment arm
+      // structurally cannot contain — a skip requires exactly one candidate.
+      mockSnapshot([]);
+
+      const result = await resolveRelevantAutoMemoryPromptForQuery(
+        '/tmp/project',
+        'We hit provider fallback again.',
+        { config, onFastResult: vi.fn() },
+      );
+
+      expect(selectRelevantAutoMemoryDocumentsByModel).not.toHaveBeenCalled();
+      expect(result.strategy).toBe('none');
+      const event = vi.mocked(logMemoryRecall).mock.calls.at(-1)?.[1] as {
+        selector_skipped?: boolean;
+      };
+      expect(event.selector_skipped).toBeUndefined();
+    });
+
+    it('leaves selector_skipped unset for a whitespace-only query', async () => {
+      mockSnapshot([exact]);
+
+      const result = await resolveRelevantAutoMemoryPromptForQuery(
+        '/tmp/project',
+        '   ',
+        { config, onFastResult: vi.fn() },
+      );
+
+      expect(selectRelevantAutoMemoryDocumentsByModel).not.toHaveBeenCalled();
+      expect(result.strategy).toBe('none');
+      const event = vi.mocked(logMemoryRecall).mock.calls.at(-1)?.[1] as {
+        selector_skipped?: boolean;
+      };
+      expect(event.selector_skipped).toBeUndefined();
+    });
+
     it('keeps the selector when the unique strong fast document has a stale body', async () => {
       const stale = {
         ...exact,
@@ -1105,6 +1144,64 @@ describe('auto-memory relevant recall', () => {
       expect(selectRelevantAutoMemoryDocumentsByModel).toHaveBeenCalledOnce();
     });
 
+    it('keeps the selector when a second strong match sits below the published window', async () => {
+      // Uniqueness has to be judged against the pool the suppressed selector
+      // would have been handed, not just the published fast list. Doc B is a
+      // title match ranked 7th lexically, so `fallbackDocs` (top
+      // MAX_RELEVANT_DOCS) never contains it while `modelCandidates` does.
+      const ranked = 'deploy timeout provider fallback';
+      const strongTop = {
+        ...memoryDoc(
+          'a-deploy-timeout.md',
+          'reference',
+          'deploy timeout',
+          'runbook alpha',
+          'deploy timeout provider fallback',
+        ),
+        mtimeMs: 100,
+      };
+      // Each filler outscores B (one title token plus four body tokens) while
+      // carrying exactly one query token in its metadata, so no filler is a
+      // strong match and B stays below the published window.
+      const fillers = [1, 2, 3, 4, 5].map((n) => ({
+        ...memoryDoc(
+          `filler-${n}.md`,
+          'reference',
+          `deploy notes ${n}`,
+          'alpha group checklist',
+          'deploy timeout provider fallback',
+        ),
+        mtimeMs: 90 - n,
+      }));
+      const strongBelowWindow = {
+        ...memoryDoc(
+          'b-provider-glossary.md',
+          'reference',
+          'provider',
+          'glossary entry alpha',
+          'unrelated alpha text',
+        ),
+        mtimeMs: 1,
+      };
+      mockSnapshot([strongTop, ...fillers, strongBelowWindow]);
+      const onFastResult = vi.fn();
+
+      const result = await resolveRelevantAutoMemoryPromptForQuery(
+        '/tmp/project',
+        ranked,
+        { config, onFastResult },
+      );
+
+      expect(onFastResult.mock.calls[0]?.[0].selectedDocs).toEqual([strongTop]);
+      expect(selectRelevantAutoMemoryDocumentsByModel).toHaveBeenCalledOnce();
+      // The suppressed selector would have seen B, so the recall was ambiguous.
+      expect(
+        vi.mocked(selectRelevantAutoMemoryDocumentsByModel).mock.calls[0]?.[2] ??
+          [],
+      ).toContainEqual(strongBelowWindow);
+      expect(result.selectorSkipped).toBeUndefined();
+    });
+
     it('keeps the selector when nothing matches strongly', async () => {
       mockSnapshot(docs);
 

packages/core/src/memory/recall.ts

@@ -390,9 +390,28 @@ function selectModelCandidateDocuments(
   recentTools: readonly string[],
   fallbackLimit: number,
   useStructuredMetadata = true,
+  countStrongMatches = false,
 ): {
   modelCandidates: ScannedAutoMemoryDocument[];
   fallbackDocs: ScannedAutoMemoryDocument[];
+  /**
+   * How many of the scored `lexical` pool match a title or keyword, or
+   * `undefined` when `countStrongMatches` was off.
+   *
+   * The #13003 skip guard needs this because the selector it suppresses is
+   * handed `modelCandidates`, whose lexical half is the top
+   * `MAX_MODEL_CANDIDATE_DOCS - RECENT_MODEL_CANDIDATE_RESERVE` of `lexical`,
+   * while `fallbackDocs` is only the top `fallbackLimit`
+   * (`<= MAX_RELEVANT_DOCS`). Counting the published list alone would call a
+   * recall "unique" while a second strong match still sat in the suppressed
+   * selector's pool at ranks `fallbackLimit + 1` and beyond.
+   *
+   * Computed only while the guard is live: `matchesTitleOrKeyword` builds a
+   * RegExp per short keyword per document, and this pool is roughly 36x wider
+   * than the fast list, on a path that carries a latency budget for every
+   * structured recall including knob-off ones.
+   */
+  lexicalStrongMatchCount: number | undefined;
 } {
   const isActiveToolNoise = createActiveToolUsageFilter(
     recentTools,
@@ -408,6 +427,12 @@ function selectModelCandidateDocuments(
     ),
     useStructuredMetadata,
   );
+  // Reuse the already-scored `lexical` array rather than re-scoring the
+  // corpus: the guard only needs a count, and re-scoring would double the
+  // cost of the widest pool on this path.
+  const lexicalStrongMatchCount = countStrongMatches
+    ? lexical.filter((doc) => matchesTitleOrKeyword(query, doc)).length
+    : undefined;
   const modelLexical = lexical.slice(
     0,
     MAX_MODEL_CANDIDATE_DOCS - RECENT_MODEL_CANDIDATE_RESERVE,
@@ -425,6 +450,7 @@ function selectModelCandidateDocuments(
   return {
     modelCandidates,
     fallbackDocs: lexical.slice(0, fallbackLimit),
+    lexicalStrongMatchCount,
   };
 }
 
@@ -567,12 +593,22 @@ function logRecallResult(
     fastDurationMs: number;
     selectorDurationMs: number;
   },
+  selectorDecided: boolean,
 ): void {
   if (!config || abortSignal?.aborted) return;
   // The skip guard only exists in structured mode, so legacy recalls never
   // have a selector-skip decision to report: stamping `false` there would
   // mix a constant into the experiment's control series. Leave the field
   // unset and let the logger drop the dimension.
+  //
+  // A structured recall that returned before the selector was ever reached —
+  // the empty-query / empty-corpus / non-positive-limit short circuit — is in
+  // the same position: it made no skip decision, and `false` there would read
+  // as "the selector ran and was not skipped". That would put trivially fast
+  // recalls into the ablation's control arm, which the treatment arm
+  // structurally cannot contain (a skip requires exactly one candidate), and
+  // `recordMemoryRecallMetrics` carries only `strategy` and `selector_skipped`
+  // on the counter and histogram, so the samples could not be sliced back out.
   const legacy = (config.getMemoryRecallMode?.() ?? 'legacy') === 'legacy';
   logMemoryRecall(
     config,
@@ -585,7 +621,9 @@ function logRecallResult(
       scan_duration_ms: timings.scanDurationMs,
       fast_duration_ms: timings.fastDurationMs,
       selector_duration_ms: timings.selectorDurationMs,
-      ...(legacy ? {} : { selector_skipped: result.selectorSkipped === true }),
+      ...(legacy || !selectorDecided
+        ? {}
+        : { selector_skipped: result.selectorSkipped === true }),
     }),
   );
 }
@@ -683,6 +721,9 @@ export async function resolveRelevantAutoMemoryPromptForQuery(
       result,
       t0,
       timings(),
+      // Short circuit: the selector was never reached, so this recall made no
+      // skip decision and must not enter the control series as `false`.
+      false,
     );
     return result;
   }
@@ -691,12 +732,18 @@ export async function resolveRelevantAutoMemoryPromptForQuery(
   if (options.config) {
     try {
       const fastStartedAt = Date.now();
+      // Evaluate once, before the candidate pass: the same value decides
+      // whether the widened count is paid for and whether the guard below may
+      // fire, so the two cannot disagree if the environment changes mid-call.
+      const skipGateEnabled =
+        !legacy && isSkipSelectorOnUniqueStrongHitEnabled();
       const candidates = selectModelCandidateDocuments(
         query,
         docs,
         options.recentTools ?? [],
         limit,
         !legacy,
+        skipGateEnabled,
       );
       fallbackDocs = candidates.fallbackDocs;
       // Publish the deterministic candidates before blocking on the selector
@@ -732,11 +779,21 @@ export async function resolveRelevantAutoMemoryPromptForQuery(
       fastDurationMs = Date.now() - fastStartedAt;
       // Count before rendering: a second candidate trimmed by the prompt
       // budget must not turn an ambiguous recall into a selector skip.
+      //
+      // Count the selector's own pool for the same reason. `fastCandidateCount`
+      // only sees `fallbackDocs`, the top `limit` (<= MAX_RELEVANT_DOCS) of the
+      // lexical ranking, while the selector this guard suppresses would have
+      // been handed `candidates.modelCandidates` — the top
+      // MAX_MODEL_CANDIDATE_DOCS - RECENT_MODEL_CANDIDATE_RESERVE, roughly 36x
+      // wider. A second strong match ranked below `limit` is invisible to the
+      // published count, so without `lexicalStrongMatchCount` the guard would
+      // fire on an ambiguous recall and still report `selector_skipped: true`,
+      // counting it in the treatment arm as a *unique* strong hit.
       const uniqueStrongHit = publishedFast?.selectedDocs[0];
       if (
-        !legacy &&
-        isSkipSelectorOnUniqueStrongHitEnabled() &&
+        skipGateEnabled &&
         fastCandidateCount === 1 &&
+        (candidates.lexicalStrongMatchCount ?? 0) === 1 &&
         publishedFast?.selectedDocs.length === 1 &&
         uniqueStrongHit !== undefined &&
         matchesTitleOrKeyword(query, uniqueStrongHit) &&
@@ -755,6 +812,8 @@ export async function resolveRelevantAutoMemoryPromptForQuery(
           result,
           t0,
           timings(),
+          // This is the deliberate skip the field exists to report.
+          true,
         );
         return result;
       }
@@ -788,6 +847,8 @@ export async function resolveRelevantAutoMemoryPromptForQuery(
         result,
         t0,
         timings(),
+        // The selector ran, so `false` is a genuine control sample.
+        true,
       );
       return result;
     } catch (error) {
@@ -853,6 +914,11 @@ export async function resolveRelevantAutoMemoryPromptForQuery(
     result,
     t0,
     timings(),
+    // Heuristic fallback. `selectorStartedAt` is set the instant the selector
+    // is entered, so it separates "the selector ran and then failed" — a real
+    // control sample, `false` is correct — from a throw before it was reached,
+    // which made no skip decision and must stay off the series.
+    selectorStartedAt !== undefined,
   );
   return result;
 }

packages/core/src/telemetry/types.ts

@@ -1756,9 +1756,12 @@ export class MemoryRecallEvent implements BaseTelemetryEvent {
    * True only when the model selector was skipped because the deterministic
    * fast result matched a title/keyword and its body was absent (#13003). Keeps a
    * deliberate skip apart from a selector failure, which also reports
-   * `strategy: 'heuristic'`. Undefined when the recall mode had no skip
-   * decision to make (legacy mode), so the metric dimension stays off a
-   * series the experiment cannot move.
+   * `strategy: 'heuristic'`. Undefined when the recall had no skip decision to
+   * make — legacy mode, or a structured recall that returned before the
+   * selector was reached (empty query, empty corpus, non-positive limit) — so
+   * the metric dimension stays off a series the experiment cannot move, and
+   * `false` keeps meaning "the selector ran and was not skipped" instead of
+   * absorbing trivially fast recalls into the ablation's control arm.
    */
   selector_skipped: boolean | undefined;