diff --git a/BACKLOG.md b/BACKLOG.md index 2e942145c..bea587a7c 100644 --- a/BACKLOG.md +++ b/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 | diff --git a/apps/vscode/src/panels/catalogOverviewPanel.ts b/apps/vscode/src/panels/catalogOverviewPanel.ts index 47a051eb6..50ef2797f 100644 --- a/apps/vscode/src/panels/catalogOverviewPanel.ts +++ b/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; } } diff --git a/apps/vscode/src/services/llmProxy.ts b/apps/vscode/src/services/llmProxy.ts index cf2f1fb89..60eda17b4 100644 --- a/apps/vscode/src/services/llmProxy.ts +++ b/apps/vscode/src/services/llmProxy.ts @@ -114,13 +114,30 @@ export function createLlmProxy(vscodeApi: LlmProxyVsCodeApi): LlmProxy { 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 @@ export function createLlmProxy(vscodeApi: LlmProxyVsCodeApi): LlmProxy { }; } - async function readApiKey(): Promise { + /** + * 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 { 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 { @@ -189,8 +227,19 @@ export function createLlmProxy(vscodeApi: LlmProxyVsCodeApi): LlmProxy { 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 @@ -273,6 +322,28 @@ export function createLlmProxy(vscodeApi: LlmProxyVsCodeApi): LlmProxy { 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, diff --git a/apps/vscode/src/webview/messages.ts b/apps/vscode/src/webview/messages.ts index 0dc5d6880..f69f884f2 100644 --- a/apps/vscode/src/webview/messages.ts +++ b/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 }; /** diff --git a/apps/vscode/src/webview/web/catalogOverview.tsx b/apps/vscode/src/webview/web/catalogOverview.tsx index cbec13ff2..d0d03cf77 100644 --- a/apps/vscode/src/webview/web/catalogOverview.tsx +++ b/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 }); }, [], diff --git a/apps/vscode/tests/unit/llmProxy.test.ts b/apps/vscode/tests/unit/llmProxy.test.ts index ab1bc5036..a348c3f80 100644 --- a/apps/vscode/tests/unit/llmProxy.test.ts +++ b/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]> = [ + ['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); diff --git a/docs/project_notes/cloud-sandbox-pnpm-investigation.md b/docs/project_notes/cloud-sandbox-pnpm-investigation.md new file mode 100644 index 000000000..ee4d53c91 --- /dev/null +++ b/docs/project_notes/cloud-sandbox-pnpm-investigation.md @@ -0,0 +1,92 @@ +# Investigate: pnpm Broken in Claude Code Cloud Sandbox + +## Problem + +In Claude Code cloud sessions on this repo, `pnpm` is broken. Every invocation fails with HTTP 403 from `registry.npmjs.org`, blocking all `task verify` / typecheck / lint / vitest / Storybook E2E runs. The 403 originates from the sandbox network policy, not from npm: + +``` +$ curl -sS -i https://registry.npmjs.org/pnpm +HTTP/2 403 +x-deny-reason: host_not_allowed +Host not in allowlist +``` + +`registry.npmjs.org` is not on the cloud sandbox's allowlist. `corepack` hits the same wall (it also fetches from the registry to satisfy the `packageManager: "pnpm@9.15.5"` pin in `package.json`). There is no `.npmrc` redirecting traffic, and `node_modules/` does not exist in the cloud image, so the `pnpm install` step has nothing to fall back on. + +Net effect: a Claude Code session can edit code, run git, run Python (uv works), and reach the GitHub MCP — but cannot run any TypeScript test, lint, or build. PRs from cloud sessions ship unverified locally. + +## What to investigate + +### 1. Is this a regression? + +Check whether earlier cloud sessions had a working pnpm. Likely candidates: + +- someone tightened the sandbox allowlist +- a pre-baked `node_modules/` was removed from the image +- a `.npmrc` redirecting to an internal mirror was deleted + +Confirm timestamps against any recent infra changes to the cloud sandbox image: + +- `ANT_IMAGE_REPOSITORY=sandbox-ccr-default` +- `CLAUDE_CODE_REMOTE_ENVIRONMENT_TYPE=cloud_default` + +### 2. Find the allowlist owner + +The sandbox is administered by Anthropic Claude Code infra (the same plumbing that runs the git proxy at `127.0.0.1:46101`). Identify who maintains the host allowlist for `cloud_default` sessions and whether the allowlist is per-account, per-repo, or global. Reach out via the standard internal Claude Code support channel. + +### 3. Pick the cheapest viable fix and prototype it + +Three candidates, in order of expected effort: + +#### Option (a) — Add `registry.npmjs.org` to the allowlist + +Lowest effort if Anthropic will allow it. Probably also needs: + +- `*.npmmirror.com` +- `registry.yarnpkg.com` +- Playwright CDN (`playwright.azureedge.net`) for full parity with CI + +#### Option (b) — Pre-warm `node_modules/` in the sandbox image + +Bake `pnpm install --frozen-lockfile` output into the image at build time. Sessions start with deps available; only changes to `pnpm-lock.yaml` would need network. Cheaper than maintaining a mirror but couples the image to the lockfile and needs rebuilds when deps change. + +#### Option (c) — Stand up a pull-through cache on an allowlisted host + +Verdaccio or Sonatype Nexus running on infra that IS allowlisted, with a checked-in `.npmrc` pointing every repo at it. Most flexible long-term — survives lockfile churn and works for new repos automatically — but most setup work and a new service to operate. + +### 4. Test the chosen fix end-to-end + +Run against a fresh cloud session in this repo. Verification steps: + +- [ ] `pnpm --version` resolves without 403 +- [ ] `pnpm install` from a clean clone succeeds +- [ ] `task verify` (lint + typecheck + test) passes +- [ ] `pnpm --filter @debrief/components test:e2e FilterBar-nl` produces PNGs in `specs/198-nl-keyring-banner/evidence/screenshots/` + +> The last bullet is the immediate use case — PR #551 is waiting on those captures. + +### 5. Document it in the repo + +Add a note to `CLAUDE.md` and `docs/project_notes/key_facts.md` describing: + +- the cloud-sandbox network constraint +- the chosen fix +- how to recover if the registry is unreachable again + +Cross-reference from `docs/project_notes/playwright-installation-research.md` since Playwright/`@sparticuz/chromium` shares the same plumbing. + +## Constraints + +- Don't disable the `packageManager` pin — CI relies on it. +- Don't commit credentials in `.npmrc` if option (c) is chosen — use environment variable interpolation: + ``` + registry=https://npm.internal/... + //npm.internal/:_authToken=${NPM_TOKEN} + ``` +- Solution must work for both Claude Code cloud sessions (where this hurts) and local dev (where it currently works fine via the public registry). A `.npmrc` change must be a no-op or strict improvement for local dev. + +## Deliverable + +A PR that lands the fix, plus a written confirmation in the PR description that a fresh cloud session can run `task verify` and `test:e2e` end-to-end. + +**Time-box**: 1 day for investigation + chosen-fix prototype; up to 3 days if option (c) is picked. diff --git a/docs/project_notes/issues.md b/docs/project_notes/issues.md index b0f4c007d..4c1296cc5 100644 --- a/docs/project_notes/issues.md +++ b/docs/project_notes/issues.md @@ -72,3 +72,9 @@ Each entry should include: - **Description**: Make every Debrief webview reflect VS Code's active colour theme on load and update within 1s on switch. Replace muddled `'vscode'` variant with a flat first-class enum that distinguishes high-contrast variants. Wire every webview through a shared `` wrapper. Inject `--vscode-*` token map per variant in Storybook so the toolbar actually drives component colours. - **Spec**: `specs/220-fix-theme-responsiveness/spec.md` - **ADR**: ADR-025 in `docs/project_notes/decisions.md` + +### 2026-04-26 - #198: NL Search — Keyring-Unavailable Distinct Banner +- **Status**: Implementation complete (PR pending) +- **Description**: Split a new `keyring-unavailable` outcome out of `not-configured` so VS Code analysts whose OS keyring is locked/missing see "unlock the keyring" guidance instead of the misleading "set your API key" banner. Wraps every `context.secrets.get()` call in a try/catch (first-read + cache-refresh in `onDidChange`); a throw classifies as `keyring-unavailable`, undefined still classifies as `not-configured`. Cache-refresh throws preserve the previously-working cached key (FR-008). New banner has an OS-neutral headline + optional platform hint paragraph + primary "Help: unlock your keyring" + secondary "Open settings". +- **Spec**: `specs/198-nl-keyring-banner/spec.md` +- **Evidence**: `specs/198-nl-keyring-banner/evidence/` diff --git a/shared/components/e2e/FilterBar-nl.spec.ts b/shared/components/e2e/FilterBar-nl.spec.ts index 1031d426d..7e26fc2d1 100644 --- a/shared/components/e2e/FilterBar-nl.spec.ts +++ b/shared/components/e2e/FilterBar-nl.spec.ts @@ -96,6 +96,116 @@ test.describe('FilterBar NL mode — banner per failure class', () => { } }); +// --------------------------------------------------------------------------- +// #198 T031 — keyring-unavailable banner per platformHint × theme +// --------------------------------------------------------------------------- + +const KEYRING_EVIDENCE_DIR = resolve( + HERE, + '..', + '..', + '..', + 'specs', + '198-nl-keyring-banner', + 'evidence', + 'screenshots', +); +mkdirSync(KEYRING_EVIDENCE_DIR, { recursive: true }); + +const KEYRING_STORY_BY_HINT: Record< + 'linux' | 'macos' | 'windows' | 'unknown', + string +> = { + linux: 'filterbar--nl-mode-keyring-unavailable', + macos: 'filterbar--nl-mode-keyring-unavailable-macos', + windows: 'filterbar--nl-mode-keyring-unavailable-windows', + unknown: 'filterbar--nl-mode-keyring-unavailable-unknown', +}; + +function keyringUrl( + hint: 'linux' | 'macos' | 'windows' | 'unknown', + theme: 'light' | 'dark' | 'vscode', +): string { + return `/iframe.html?id=${KEYRING_STORY_BY_HINT[hint]}&globals=theme:${theme}`; +} + +test.describe('FilterBar NL mode — keyring-unavailable banner (#198 T031)', () => { + for (const hint of ['linux', 'macos', 'windows', 'unknown'] as const) { + test(`renders ${hint} variant with correct reason + actions`, async ({ page }) => { + await page.goto(keyringUrl(hint, 'light')); + await page.getByTestId('nl-search-indicator').waitFor({ timeout: 10_000 }); + + const input = page.getByTestId('quick-search-input'); + await input.fill('any phrase'); + await input.press('Enter'); + + const banner = page.getByTestId('live-transport-banner'); + await banner.waitFor({ timeout: 10_000 }); + await expect(banner).toHaveAttribute( + 'data-transport-reason', + 'keyring-unavailable', + ); + + // Hint paragraph: present for known platforms, absent for unknown. + const hintEl = banner.locator('[data-testid="live-transport-banner-hint"]'); + if (hint === 'unknown') { + await expect(hintEl).toHaveCount(0); + } else { + await expect(hintEl).toHaveCount(1); + await expect(hintEl).toHaveAttribute('data-platform-hint', hint); + } + + // Both action buttons should render. + await expect(page.getByTestId('live-transport-banner-help')).toBeVisible(); + await expect( + page.getByTestId('live-transport-banner-open-settings'), + ).toBeVisible(); + + // Banner copy must NOT instruct re-entering the key (FR-004). + const text = (await banner.textContent()) ?? ''; + expect(text).not.toMatch(/re-?enter/i); + expect(text).toMatch(/keyring/i); + + // Capture for evidence — light theme only here; dark/vscode below. + if (hint !== 'unknown') { + await page.screenshot({ + path: resolve( + KEYRING_EVIDENCE_DIR, + `banner-keyring-unavailable-${hint}.png`, + ), + fullPage: false, + }); + } + }); + } + + // T032 — dark + vscode theme captures (Linux variant covers all three). + for (const theme of ['dark', 'vscode'] as const) { + test(`linux variant renders in ${theme} theme`, async ({ page }) => { + await page.goto(keyringUrl('linux', theme)); + await page.getByTestId('nl-search-indicator').waitFor({ timeout: 10_000 }); + + const input = page.getByTestId('quick-search-input'); + await input.fill('any phrase'); + await input.press('Enter'); + + const banner = page.getByTestId('live-transport-banner'); + await banner.waitFor({ timeout: 10_000 }); + await expect(banner).toHaveAttribute( + 'data-transport-reason', + 'keyring-unavailable', + ); + await page.screenshot({ + path: resolve( + KEYRING_EVIDENCE_DIR, + `banner-keyring-unavailable-linux-${theme}.png`, + ), + fullPage: false, + }); + }); + } +}); + test.describe('FilterBar NL mode — lozenges survive failure (T046)', () => { test('chips persist across a follow-up auth-failure', async ({ page }) => { await page.goto(themeUrl('light')); diff --git a/shared/components/src/FilterBar/FilterBar.stories.tsx b/shared/components/src/FilterBar/FilterBar.stories.tsx index 4598a1869..275a4a1d4 100644 --- a/shared/components/src/FilterBar/FilterBar.stories.tsx +++ b/shared/components/src/FilterBar/FilterBar.stories.tsx @@ -703,3 +703,117 @@ export const NlModeWithStubClient: Story = { }, }, }; + +// --------------------------------------------------------------------------- +// #198 T030 — keyring-unavailable banner variants (one per platformHint) +// --------------------------------------------------------------------------- + +function createKeyringStubClient( + platformHint: 'linux' | 'macos' | 'windows' | 'unknown', +): LLMClient { + return { + async generate(): Promise { + // Tiny latency so the pending-state transition is observable. + await new Promise((resolve) => setTimeout(resolve, 100)); + return { kind: 'keyring-unavailable', platformHint, durationMs: 0 }; + }, + abort() { + // No-op — there is nothing in-flight worth aborting. + }, + }; +} + +function NlKeyringUnavailableWrapper({ + platformHint, +}: { + platformHint: 'linux' | 'macos' | 'windows' | 'unknown'; +}): ReactElement { + const [client] = useState(() => createKeyringStubClient(platformHint)); + const handleBannerAction = useCallback( + (action: 'open-settings' | 'retry' | 'reload' | 'help') => { + // Storybook-only — log so reviewers can confirm the wiring without + // a real VS Code host. + // eslint-disable-next-line no-console + console.info(`[storybook] banner action dispatched: ${action}`); + }, + [], + ); + return ( +
+ undefined} + llmClient={client} + nlEnums={NL_STUB_ENUMS} + liveModeLabel="Live · Anthropic · claude-haiku-4-5-20251001 (stub)" + onBannerAction={handleBannerAction} + /> +
+ Submit any phrase — the stub returns + kind: "keyring-unavailable", platformHint: "{platformHint}" + once. The banner's headline stays OS-neutral; the platform hint + paragraph (when not unknown) names the appropriate + OS keyring tool. +
+
+ ); +} + +export const NlModeKeyringUnavailable: Story = { + name: 'NL Mode — keyring-unavailable (Linux)', + render: () => , + parameters: { + docs: { + description: { + story: + '#198 — Linux keyring-unavailable banner. Headline names the OS keyring (not the API key); body has a Linux-specific hint paragraph; primary action opens troubleshooting help; secondary action opens settings (deliberately NOT primary, to avoid misdirecting the analyst into re-entering a key that is already saved).', + }, + }, + }, +}; + +export const NlModeKeyringUnavailableMacos: Story = { + name: 'NL Mode — keyring-unavailable (macOS)', + render: () => , + parameters: { + docs: { + description: { + story: + '#198 — macOS variant. Same banner, hint paragraph names Keychain Access.', + }, + }, + }, +}; + +export const NlModeKeyringUnavailableWindows: Story = { + name: 'NL Mode — keyring-unavailable (Windows)', + render: () => , + parameters: { + docs: { + description: { + story: + '#198 — Windows variant. Same banner, hint paragraph names Credential Manager.', + }, + }, + }, +}; + +export const NlModeKeyringUnavailableUnknown: Story = { + name: 'NL Mode — keyring-unavailable (unknown OS)', + render: () => , + parameters: { + docs: { + description: { + story: + '#198 — fallback variant for unrecognised platforms. The platform-specific hint paragraph is suppressed (no placeholder text), but the banner remains usable with the same primary/secondary actions.', + }, + }, + }, +}; diff --git a/shared/components/src/FilterBar/FilterBar.tsx b/shared/components/src/FilterBar/FilterBar.tsx index e70a502d6..fb74bf4b8 100644 --- a/shared/components/src/FilterBar/FilterBar.tsx +++ b/shared/components/src/FilterBar/FilterBar.tsx @@ -419,7 +419,10 @@ export const FilterBar: React.FC = ({ )} {/* #191 T044 / T082 — NL failure banner with per-class recovery - affordance. Keyed by `data-transport-reason` for E2E selectors. */} + affordance. Keyed by `data-transport-reason` for E2E selectors. + #198 — `keyring-unavailable` renders a primary "Help: unlock + your keyring" + secondary "Open settings"; the headline stays + OS-neutral with an optional platform hint paragraph. */} {nlBanner && (
= ({ {nlBannerMessage(nlBanner)} - {(() => { - const action = nlBannerAction(nlBanner); - if (!action) return null; - return ( - - ); - })()} + {keyringPlatformHint(nlBanner.platformHint)} + + )} + {nlBannerActions(nlBanner).map((action) => ( + + ))}