[#198] NL search — keyring-unavailable distinct banner

BACKLOG.md

@@ -196,7 +196,7 @@ Description formats:
 | ~~201~~ | ~~Tech Debt~~ | ~~[Consolidate ResolvedPositionStyle and align with schema](docs/ideas/201-resolvedpositionstyle-consolidation.md) — two `ResolvedPositionStyle` interfaces with drifted shapes (`symbol: 'circle'\|'square'\|'triangle'` + `label` vs the 5-symbol components version + `labelText`). Canonicalise in `@debrief/utils`; symbol field uses `PositionStyleSymbolEnum` from `@debrief/schemas` (not a hand-typed union); field name `labelText`.~~ | ~~3~~ | ~~1~~ | ~~5~~ | ~~9~~ | ~~Low~~ | ~~complete~~ |
 | ~~200~~ | ~~Tech Debt~~ | ~~[Consolidate bounds utilities into @debrief/utils](specs/200-bounds-consolidation/spec.md) — `apps/vscode/src/utils/bounds.ts` is a ~116-line 95%-identical copy of `shared/utils/src/bounds.ts`; vscode version has a null-guard the shared version is missing. Lift null-guard into utils, reconcile `SafeFeature`/`GeoJSONFeature` at the call site, delete the vscode copy + duplicate test, switch `mapPanel.ts` to import from `@debrief/utils`.~~ | ~~3~~ | ~~1~~ | ~~5~~ | ~~9~~ | ~~Low~~ | ~~complete~~ |
 | ~~199~~ | ~~Tech Debt~~ | ~~[Code-quality cleanup: small-bucket consolidation](docs/ideas/199-code-quality-small-bucket.md) — combined single-PR package of five low-risk follow-ups from PR #465: (a) document residual vscode view↔service type-only cycles in `decisions.md`; (b) merge `LogTimelineProps` + `LogByFeatureProps` into single `LogPanelProps`; (c) delete orphaned `shared/components/diff/` sub-package; (d) add `specs/**` to knip ignore; (e) fix `plotName` placeholder in loader's useLoadWorkflow + promote 3 remaining TODOs to GitHub issues.~~ | ~~3~~ | ~~1~~ | ~~5~~ | ~~9~~ | ~~Low~~ | ~~complete~~ |
-| 198 | Enhancement | [[E10] NL search — keyring-unavailable distinct banner](specs/191-vscode-nl-search/spec.md) — when `context.secrets.get()` throws (locked/missing OS keyring on Linux), surface a specific `keyring-unavailable` outcome + banner rather than folding it into the generic `not-configured` path; different diagnosis (unlock keyring vs re-enter key) (requires #191) | 3 | 1 | 4 | 8 | Low | approved |
+| ~~198~~ | ~~Enhancement~~ | ~~[[E10] NL search — keyring-unavailable distinct banner](specs/198-nl-keyring-banner/spec.md) — when `context.secrets.get()` throws (locked/missing OS keyring on Linux), surface a specific `keyring-unavailable` outcome + banner rather than folding it into the generic `not-configured` path; different diagnosis (unlock keyring vs re-enter key) (requires #191)~~ | ~~3~~ | ~~1~~ | ~~4~~ | ~~8~~ | ~~Low~~ | ~~complete~~ |
 | 197 | Feature | [[E10] NL search — per-prompt audit trail (opt-in)](specs/191-vscode-nl-search/spec.md) — optional verbose log capturing prompts + responses for forensic review; separate setting + separate log channel; off by default; structured for SIEM ingest (requires #191 structured telemetry) | 3 | 2 | 3 | 8 | Medium | approved |
 | 196 | Feature | [[E10] NL search — non-Anthropic providers](specs/191-vscode-nl-search/spec.md) — pluggable provider choice via `debrief.nlSearch.provider` setting (Claude / OpenAI / ollama); `LLMClient` abstraction already supports new factories, main work is per-provider prompt adaptation + error-class mapping (requires #191, provider-neutral prompt validation harness) | 4 | 3 | 3 | 10 | High | approved |
 | 195 | Feature | [[E10] NL search in Layers & Tools panels](specs/191-vscode-nl-search/spec.md) — extend NL-mode to other VS Code webview surfaces once #191 proves out in the Catalog Overview; FilterBar `llmClient` prop carries over, wiring is presentational (requires #191) | 4 | 3 | 4 | 11 | Medium | approved |

apps/vscode/src/panels/catalogOverviewPanel.ts

@@ -84,10 +84,10 @@ interface NlAbortMessage {
   requestId: string;
 }
 
-/** NL-search banner-action request from the webview (#191 T082). */
+/** NL-search banner-action request from the webview (#191 T082, #198). */
 interface NlBannerActionMessage {
   type: 'nlBannerAction';
-  action: 'open-settings' | 'retry' | 'reload';
+  action: 'open-settings' | 'retry' | 'reload' | 'help';
 }
 
 type OverviewToExtensionMessage =
@@ -339,6 +339,15 @@ export class CatalogOverviewPanel {
       case 'retry':
         // FilterBar handles retry locally; no host action required.
         break;
+      case 'help':
+        // #198 Decision 4 — open the static troubleshooting docs page.
+        // We never shell out to keyring daemons; help is documentation only.
+        void vscode.env.openExternal(
+          vscode.Uri.parse(
+            'https://debrief.github.io/docs/nl-search-troubleshooting#keyring-unavailable',
+          ),
+        );
+        break;
     }
   }
 

apps/vscode/src/services/llmProxy.ts

@@ -114,15 +114,32 @@
   let cachedKey: string | null | undefined = undefined; // undefined = not yet read; null = read, not present
   let callsUsed = 0;
 
-  // Key-cache invalidation: clear the cache on every secrets change so the
-  // next `nlGenerate` re-reads. (Review Decision 14.)
+  // Key-cache invalidation on secrets change (review Decision 14).
+  //
+  // #198 FR-008 — a throw during the cache-refresh re-read MUST NOT evict
+  // a previously-working `cachedKey`. We re-read eagerly inside the
+  // listener so the next `nlGenerate` already has the fresh value, and
+  // we wrap the re-read in its own try/catch:
+  //   * resolved with value     → replace `cachedKey`
+  //   * resolved with undefined → evict (key was deleted)
+  //   * rejected / threw        → leave `cachedKey` UNCHANGED (the
+  //     previously-working key remains usable; the next `nlGenerate`
+  //     will surface `LiveKeyringUnavailable` only if the cache has no
+  //     usable value).
   const secretsChangeListener = vscodeApi.secrets.onDidChange?.((e) => {
-    if (e.key === SECRET_KEY) {
-      cachedKey = undefined;
+    if (e.key !== SECRET_KEY) return;
+    void (async () => {
+      try {
+        const value = await vscodeApi.secrets.get(SECRET_KEY);
+        cachedKey = value ?? null;
+      } catch {
+        // FR-008 — preserve the previously-working cached key on throw.
+        // Intentionally NO `cachedKey = ...` assignment here.
+      }
       fireConfigChange();
-    }
+    })();
   });
   if (secretsChangeListener) {
     disposables.push(secretsChangeListener);
   }
 
@@ -145,14 +162,35 @@
     };
   }
 
-  async function readApiKey(): Promise<string | null> {
+  /**
+   * Three-way result for `readApiKey()`:
+   *   - `{ ok: true, value }` — key is present (or genuinely absent
+   *     `null`); proceed using `value` (or short-circuit on `null` to
+   *     `not-configured`).
+   *   - `{ ok: false }`       — `secrets.get` rejected/threw on first
+   *     read AND we have no usable cached value; surface
+   *     `keyring-unavailable` (#198 FR-002).
+   */
+  type ReadApiKeyResult =
+    | { readonly ok: true; readonly value: string | null }
+    | { readonly ok: false };
+
+  async function readApiKey(): Promise<ReadApiKeyResult> {
     if (cachedKey !== undefined) {
-      return cachedKey;
+      return { ok: true, value: cachedKey };
+    }
+    try {
+      const value = await vscodeApi.secrets.get(SECRET_KEY);
+      const resolved: string | null = value ?? null;
+      cachedKey = resolved;
+      return { ok: true, value: resolved };
+    } catch {
+      // #198 — any rejection (Error, string, undefined, DOMException…)
+      // classifies as `keyring-unavailable`. We do NOT inspect error
+      // shapes (Decision 1). We do NOT cache the failure — the next
+      // submission re-attempts `secrets.get` (FR-007).
+      return { ok: false };
     }
-    const value = await vscodeApi.secrets.get(SECRET_KEY);
-    const resolved: string | null = value ?? null;
-    cachedKey = resolved;
-    return resolved;
   }
 
   function readConfigSync(): HostLiveConfig {
@@ -171,8 +209,8 @@
   }
 
   function emitRecord(record: TransportCallRecord): void {
     if (typeof console !== 'undefined' && typeof console.info === 'function') {
       console.info('[nl-search/live]', record);
     }
   }
 
@@ -189,8 +227,19 @@
       return outcome;
     }
 
-    const apiKey = await readApiKey();
-    if (apiKey === null || apiKey === undefined || apiKey === '') {
+    const keyResult = await readApiKey();
+    if (!keyResult.ok) {
+      // #198 — `secrets.get` rejected; classify as keyring-unavailable.
+      const outcome: LiveOutcome = {
+        kind: 'keyring-unavailable',
+        platformHint: detectPlatformHint(),
+        durationMs: 0,
+      };
+      emitRecord(makeRecord(outcome, settings.model, 0));
+      return outcome;
+    }
+    const apiKey = keyResult.value;
+    if (apiKey === null || apiKey === '') {
       const outcome: LiveOutcome = { kind: 'not-configured', reason: 'no-key', durationMs: 0 };
       emitRecord(makeRecord(outcome, settings.model, 0));
       fireConfigChange(); // hasApiKey boolean changed if webview was out of sync
@@ -254,7 +303,7 @@
     listener: (snapshot: HostLiveConfig) => void,
   ): vscode.Disposable {
     configListeners.add(listener);
     return { dispose: () => configListeners.delete(listener) } as vscode.Disposable;
   }
 
   function dispose(): void {
@@ -273,6 +322,28 @@
   return { handleGenerate, handleAbort, readConfig, onConfigChange, dispose };
 }
 
+/**
+ * Map `process.platform` to the optional `platformHint` carried by a
+ * `keyring-unavailable` outcome (#198 Decision 3). Pure; no side effects.
+ *
+ * Only Linux/macOS/Windows have OS credential keyrings worth naming;
+ * everything else gets `"unknown"` and the banner suppresses the
+ * platform-specific hint sentence (FR-010 — headline stays OS-neutral).
+ */
+export function detectPlatformHint(): 'linux' | 'macos' | 'windows' | 'unknown' {
+  if (typeof process === 'undefined' || !process.platform) return 'unknown';
+  switch (process.platform) {
+    case 'linux':
+      return 'linux';
+    case 'darwin':
+      return 'macos';
+    case 'win32':
+      return 'windows';
+    default:
+      return 'unknown';
+  }
+}
+
 function makeRecord(
   outcome: LiveOutcome,
   model: string,

apps/vscode/src/webview/messages.ts

@@ -367,6 +367,7 @@ export type NlLiveOutcome =
   | { readonly kind: 'timeout'; readonly durationMs: number }
   | { readonly kind: 'malformed-response'; readonly reason: 'non-json' | 'oversize' | 'truncated'; readonly durationMs: number; readonly responseBytes: number }
   | { readonly kind: 'not-configured'; readonly reason: 'disabled' | 'no-key'; readonly durationMs: 0 }
+  | { readonly kind: 'keyring-unavailable'; readonly platformHint?: 'linux' | 'macos' | 'windows' | 'unknown'; readonly durationMs: 0 }
   | { readonly kind: 'ceiling-reached'; readonly ceiling: number; readonly durationMs: 0 };
 
 /**

apps/vscode/src/webview/web/catalogOverview.tsx

@@ -168,11 +168,12 @@ function CatalogOverviewApp(): React.ReactElement {
 
   // #191 T082 — host wiring for the NL failure-banner recovery buttons.
   // `retry` is handled inside FilterBar (re-submits the last phrase); here
-  // we route `open-settings` and `reload` through VS Code commands by
-  // posting a message back to the extension host, which maps them to the
-  // matching `vscode.commands.executeCommand`.
+  // we route `open-settings`, `reload`, and `help` through VS Code commands
+  // by posting a message back to the extension host, which maps them to
+  // the matching `vscode.commands.executeCommand` (`help` resolves via
+  // `vscode.env.openExternal` — #198 Decision 4).
   const handleNlBannerAction = useCallback(
-    (action: 'open-settings' | 'retry' | 'reload') => {
+    (action: 'open-settings' | 'retry' | 'reload' | 'help') => {
       vscode.postMessage({ type: 'nlBannerAction', action });
     },
     [],

apps/vscode/tests/unit/llmProxy.test.ts

@@ -321,6 +321,184 @@ describe('llmProxy — controller-map cleanup (T039)', () => {
   });
 });
 
+// ---------------------------------------------------------------------------
+// #198 — keyring-unavailable classification
+// ---------------------------------------------------------------------------
+
+describe('llmProxy — keyring-unavailable classification (#198 T010-T013)', () => {
+  it('classifies a rejected secrets.get as keyring-unavailable (not not-configured)', async () => {
+    const f = makeFakeApi({ storedKey: undefined });
+    f.secretsGet.mockReset().mockRejectedValueOnce(new Error('keyring locked'));
+    const proxy = createLlmProxy(f.api);
+    const outcome = await proxy.handleGenerate({ requestId: 'r1', prompt: 'x' });
+    expect(outcome.kind).toBe('keyring-unavailable');
+    if (outcome.kind === 'keyring-unavailable') {
+      // platformHint is derived from process.platform — assert it's one of
+      // the expected literals (the actual value depends on the test host).
+      expect(['linux', 'macos', 'windows', 'unknown']).toContain(outcome.platformHint);
+      expect(outcome.durationMs).toBe(0);
+    }
+    // Provider call MUST NOT have been attempted — the failure is purely
+    // host-side.
+    expect(providerCallMock).not.toHaveBeenCalled();
+  });
+
+  it('keeps not-configured/no-key when secrets.get resolves to undefined (regression)', async () => {
+    // FR-003 — the no-key-ever-saved path must NOT regress.
+    const f = makeFakeApi({ storedKey: undefined });
+    const proxy = createLlmProxy(f.api);
+    const outcome = await proxy.handleGenerate({ requestId: 'r1', prompt: 'x' });
+    expect(outcome.kind).toBe('not-configured');
+    if (outcome.kind === 'not-configured') {
+      expect(outcome.reason).toBe('no-key');
+    }
+    expect(providerCallMock).not.toHaveBeenCalled();
+  });
+
+  it('non-Error rejection (string) still classifies as keyring-unavailable', async () => {
+    // Edge case — `secrets.get` may reject with non-Error values
+    // (string, undefined, DOMException). The discriminator is "was the
+    // promise rejected", not "what was thrown".
+    const f = makeFakeApi({ storedKey: undefined });
+    f.secretsGet.mockReset().mockRejectedValueOnce('locked');
+    const proxy = createLlmProxy(f.api);
+    const outcome = await proxy.handleGenerate({ requestId: 'r1', prompt: 'x' });
+    expect(outcome.kind).toBe('keyring-unavailable');
+  });
+
+  it('non-Error rejection (undefined) still classifies as keyring-unavailable', async () => {
+    const f = makeFakeApi({ storedKey: undefined });
+    f.secretsGet.mockReset().mockRejectedValueOnce(undefined);
+    const proxy = createLlmProxy(f.api);
+    const outcome = await proxy.handleGenerate({ requestId: 'r1', prompt: 'x' });
+    expect(outcome.kind).toBe('keyring-unavailable');
+  });
+
+  it('second submission after keyring-unavailable re-reads the secret (FR-007)', async () => {
+    // First read rejects, second resolves with a real key — the second
+    // submission must succeed without any extension reload. Classification
+    // is NOT cached/sticky.
+    providerCallMock.mockResolvedValueOnce(SUCCESS_OUTCOME);
+    const f = makeFakeApi({ storedKey: undefined });
+    f.secretsGet
+      .mockReset()
+      .mockRejectedValueOnce(new Error('locked'))
+      .mockResolvedValueOnce('sk-recovered');
+    const proxy = createLlmProxy(f.api);
+
+    const first = await proxy.handleGenerate({ requestId: 'r1', prompt: 'a' });
+    expect(first.kind).toBe('keyring-unavailable');
+    expect(f.secretsGet).toHaveBeenCalledTimes(1);
+
+    // Second submission re-attempts the read.
+    const second = await proxy.handleGenerate({ requestId: 'r2', prompt: 'b' });
+    expect(second.kind).toBe('success');
+    expect(f.secretsGet).toHaveBeenCalledTimes(2);
+  });
+
+  it('cache-refresh throw preserves the previously-working cachedKey (FR-008)', async () => {
+    // First read succeeds → cache populated. onDidChange fires; the re-read
+    // throws. The next submission must succeed using the *previously
+    // cached* key — eviction must NOT occur on a throw.
+    providerCallMock.mockResolvedValueOnce(SUCCESS_OUTCOME);
+    providerCallMock.mockResolvedValueOnce(SUCCESS_OUTCOME);
+    const f = makeFakeApi({ storedKey: 'sk-original' });
+    const proxy = createLlmProxy(f.api);
+
+    // Hydrate the cache.
+    const first = await proxy.handleGenerate({ requestId: 'r1', prompt: 'a' });
+    expect(first.kind).toBe('success');
+    expect(f.secretsGet).toHaveBeenCalledTimes(1);
+
+    // Configure the next read (driven by onDidChange) to reject.
+    f.secretsGet.mockRejectedValueOnce(new Error('keyring went down'));
+    f.fireSecretsChange('debrief.nlSearch.anthropicApiKey');
+    // Allow the async re-read inside the listener to settle.
+    await new Promise((r) => setTimeout(r, 5));
+
+    // Cache should still hold the original key — the next handleGenerate
+    // must NOT trigger a fresh read (cache hit) AND must succeed.
+    const second = await proxy.handleGenerate({ requestId: 'r2', prompt: 'b' });
+    expect(second.kind).toBe('success');
+    // 1 call for hydration + 1 call from onDidChange (which threw) = 2.
+    expect(f.secretsGet).toHaveBeenCalledTimes(2);
+    // The provider call must have received the original key.
+    const lastArgs = providerCallMock.mock.calls.at(-1)![0] as { apiKey: string };
+    expect(lastArgs.apiKey).toBe('sk-original');
+  });
+
+  it('cache-refresh resolves with undefined → cache evicted (no false retention)', async () => {
+    // The complement of the previous test: a *successful* re-read that
+    // resolves to undefined IS an eviction. The next submission should
+    // surface not-configured.
+    providerCallMock.mockResolvedValueOnce(SUCCESS_OUTCOME);
+    const f = makeFakeApi({ storedKey: 'sk-original' });
+    const proxy = createLlmProxy(f.api);
+
+    await proxy.handleGenerate({ requestId: 'r1', prompt: 'a' });
+    expect(f.secretsGet).toHaveBeenCalledTimes(1);
+
+    f.secretsGet.mockResolvedValueOnce(undefined);
+    f.fireSecretsChange('debrief.nlSearch.anthropicApiKey');
+    await new Promise((r) => setTimeout(r, 5));
+
+    const second = await proxy.handleGenerate({ requestId: 'r2', prompt: 'b' });
+    expect(second.kind).toBe('not-configured');
+    if (second.kind === 'not-configured') {
+      expect(second.reason).toBe('no-key');
+    }
+  });
+
+  it('telemetry record carries outcome="keyring-unavailable" distinctly (FR-009)', async () => {
+    const records: unknown[] = [];
+    const consoleInfoSpy = vi
+      .spyOn(console, 'info')
+      .mockImplementation((tag: unknown, record: unknown) => {
+        if (tag === '[nl-search/live]') records.push(record);
+      });
+
+    try {
+      const f = makeFakeApi({ storedKey: undefined });
+      f.secretsGet.mockReset().mockRejectedValueOnce(new Error('locked'));
+      const proxy = createLlmProxy(f.api);
+      await proxy.handleGenerate({ requestId: 'r1', prompt: 'x' });
+
+      expect(records).toHaveLength(1);
+      expect((records[0] as { outcome: string }).outcome).toBe('keyring-unavailable');
+    } finally {
+      consoleInfoSpy.mockRestore();
+    }
+  });
+});
+
+describe('llmProxy — detectPlatformHint (#198 T012)', () => {
+  it('returns one of the documented literals on this host', async () => {
+    const { detectPlatformHint } = await import('../../src/services/llmProxy');
+    const hint = detectPlatformHint();
+    expect(['linux', 'macos', 'windows', 'unknown']).toContain(hint);
+  });
+
+  it('maps process.platform values to the documented hints', async () => {
+    const { detectPlatformHint } = await import('../../src/services/llmProxy');
+    const original = process.platform;
+    const cases: Array<[NodeJS.Platform, ReturnType<typeof detectPlatformHint>]> = [
+      ['linux', 'linux'],
+      ['darwin', 'macos'],
+      ['win32', 'windows'],
+      ['freebsd', 'unknown'],
+      ['aix', 'unknown'],
+    ];
+    try {
+      for (const [platform, expected] of cases) {
+        Object.defineProperty(process, 'platform', { value: platform, configurable: true });
+        expect(detectPlatformHint()).toBe(expected);
+      }
+    } finally {
+      Object.defineProperty(process, 'platform', { value: original, configurable: true });
+    }
+  });
+});
+
 describe('llmProxy — config snapshot + change events', () => {
   it('readConfig reflects hasApiKey after the first generate() resolves', async () => {
     providerCallMock.mockResolvedValueOnce(SUCCESS_OUTCOME);