fix(cli-updates): View Log says when the log did not open (#1250 row 10)

docs/plans/2026-09-27-cli-update-log-open-failure.md

@@ -0,0 +1,43 @@
+# "View Log" says when the log did not open (#1250 row 10)
+
+Short plan: a bug with a known root cause. The row comes from `temp/quality-loop/hunt-c3.md` (row 10, P3). #1250 is a batch issue, so this PR is `Refs #1250`.
+
+## Outcome
+A failed CLI auto-update shows a **View Log** button. When the log cannot be opened (removed by hand, or no handler for the file type; nothing prunes this directory automatically), the banner now says so. Today the click does nothing visible.
+
+## Root cause (verified in source, origin/main)
+- `src/main/ipc/cliUpdates.ts` `cli-updates:open-log`: `shell.openPath` RESOLVES with an error string on failure, and the handler discards it. Its comment says "the OS shell surfaces its own error dialog", but Electron's `openPath` on a missing file returns `Failed to open path` and shows nothing.
+- The preload types the call `Promise<void>`, and the banner calls it `void`.
+
+## Design (contract)
+- **`cli-updates:open-log` returns `boolean`:** `true` only when `openPath` returned `''`. An error string, or a throw, is `false` and is warned to the console (diagnostic only).
+- **Preload:** `cliUpdatesOpenLog(logPath): Promise<boolean>`.
+- **`BannerEntry.action`** gains optional `failureText`. `BannerRow` awaits `onClick`; when it resolves `false`, it shows `failureText` in a `role="alert"` line on that row. A later click clears it.
+- The failed state's View Log action gets `failureText`: "Couldn't open the update log. It may have been cleaned up; the next failed update writes a new one."
+  - Fixed words (q22): no path or OS text.
+
+## Tests
+- **New `src/main/ipc/cliUpdates.test.ts`:** Electron's `ipcMain` is captured and `shell.openPath` stubbed (the edge). `''` answers `true`; `'Failed to open path'` answers `false`; a throw answers `false`.
+- **`CliUpdateBanner.renderer.test.tsx`:** a failed state whose `cliUpdatesOpenLog` resolves `false` shows the fixed sentence after clicking View Log; one resolving `true` shows none.
+- Red on main.
+
+## Out of scope
+- #1250's other rows.
+
+## Review round 1 (a: FIX-BEFORE-MERGE)
+- **a1 (Major, pre-existing but in the touched handler): the renderer could make main open ANY path.** `shell.openPath` opens applications too.
+  - **Ruling:** `cli-updates:open-log` takes the CLI kind. Main opens that CLI's current `failed` state's `logPath`, which it wrote itself; anything else opens nothing.
+  - Test: a path, an unknown CLI, or a CLI whose state is not failed opens nothing, and `openPath` is never called.
+- **a2 (Minor): an older, slower answer could re-show the alert after a newer successful click.** Only the latest click sets the result. Test added.
+- **a3 (Minor): the alert survived a new failed run with a new log.** The result is keyed by the action's `resultKey` (the log path). Test added.
+- **a4:** the plan and comment claimed the debug-retention prune removes these logs. It does not (`cliUpdateOrchestrator.ts` says retention is not automated for this directory); both are corrected.
+
+## Verification a, round 1 b and c
+- **a, c: the two NEW test comments repeated the false retention claim.** Corrected.
+- **c (test gaps, each pinned):**
+  - a later click clears the alert; the stale-answer test now shows the alert first, so it is no longer vacuous on main;
+  - a rejected request is said the same way;
+  - the diagnostic warn is asserted.
+  The "no clear" and "no catch" mutations now fail.
+- **b1 (pre-existing): Update Now that cannot start leaves an undismissable "Updating" row.** Split into #1425. It needs a `failed` shape without a log, which touches the shared types, the orchestrator and the banner.
+- **b2: the behaviour setting ignored a write failure.** Already fixed by #1403 (rows 6 and 13).

src/main/ipc/cliUpdates.test.ts

@@ -0,0 +1,61 @@
+import { beforeEach, expect, it, vi } from 'vitest'
+
+// #1250 row 10: `shell.openPath` RESOLVES with an error string when it cannot
+// open a file (a log that is gone gives "Failed to open path"; nothing prunes
+// this directory automatically, so that means removed by hand or by another
+// tool), and the handler used to discard it, believing the OS would
+// show its own dialog. It does not, so View Log did nothing visible. The
+// handler now answers whether the log opened.
+//
+// The REAL handler; Electron's ipcMain is captured and shell.openPath is the
+// replaced edge (it is the OS).
+
+const handlers = vi.hoisted(() => new Map<string, (...args: unknown[]) => Promise<unknown>>())
+const shell = vi.hoisted(() => ({ openPath: vi.fn<(path: string) => Promise<string>>() }))
+vi.mock('electron', () => ({
+  ipcMain: { handle: (channel: string, handler: (...args: unknown[]) => Promise<unknown>) => handlers.set(channel, handler) },
+  shell,
+}))
+vi.mock('@main/window/windowRegistry.js', () => ({ broadcastToWindows: vi.fn() }))
+vi.mock('@main/setup/setupState.js', () => ({ setCliUpdateBehavior: vi.fn() }))
+
+import { registerCliUpdatesIpc } from './cliUpdates'
+
+const failed = { kind: 'failed', cli: 'claude', from: '2.1.281', wantedLatest: '2.1.282', installMethod: 'npm', reason: 'command-failed', logPath: '/state/cli-update-logs/claude-1.log', finishedAt: 1 }
+const snapshot = { claude: failed as Record<string, unknown>, codex: { kind: 'idle' } as Record<string, unknown> }
+
+beforeEach(() => {
+  handlers.clear()
+  shell.openPath.mockReset()
+  vi.spyOn(console, 'warn').mockImplementation(() => {})
+  registerCliUpdatesIpc({ on: vi.fn(), getSnapshot: () => snapshot, refresh: vi.fn(), setBehavior: vi.fn(), updateNow: vi.fn() } as never)
+})
+
+const openLog = (cli: unknown) => handlers.get('cli-updates:open-log')!({}, cli)
+
+it('opens the log main wrote for that CLI\'s failure, and answers true', async () => {
+  shell.openPath.mockResolvedValue('')
+  expect(await openLog('claude')).toBe(true)
+  expect(shell.openPath).toHaveBeenCalledWith('/state/cli-update-logs/claude-1.log')
+})
+
+it('answers false, and warns the diagnostic, when the OS could not open it or openPath throws', async () => {
+  const warn = vi.mocked(console.warn)
+  shell.openPath.mockResolvedValue('Failed to open path')
+  expect(await openLog('claude')).toBe(false)
+  expect(warn).toHaveBeenLastCalledWith('[cli-updates] failed to open log:', 'Failed to open path')
+  const boom = new Error('boom')
+  shell.openPath.mockRejectedValue(boom)
+  expect(await openLog('claude')).toBe(false)
+  expect(warn).toHaveBeenLastCalledWith('[cli-updates] failed to open log:', boom)
+})
+
+// #1423 review a: the renderer used to send a PATH, and any string reached
+// shell.openPath, which opens applications too. Nothing but the failed
+// state's own log is ever opened now.
+it('opens nothing for a path, an unknown CLI, or a CLI whose state is not failed', async () => {
+  shell.openPath.mockResolvedValue('')
+  expect(await openLog('/Applications/Calculator.app')).toBe(false)
+  expect(await openLog('codex')).toBe(false)
+  expect(shell.openPath).not.toHaveBeenCalled()
+})

src/main/ipc/cliUpdates.ts

@@ -66,15 +66,30 @@ export function registerCliUpdatesIpc(orchestrator: CliUpdateOrchestrator): void
     return orchestrator.getSnapshot()
   })
 
-  ipcMain.handle('cli-updates:open-log', async (_evt, logPath: string) => {
-    // shell.openPath returns '' on success, an error string on failure.
-    // We don't propagate the string — the caller only needs to know the
-    // click "did something". If the file is missing (log path predates a
-    // debug-retention prune) the OS shell surfaces its own error dialog.
+  ipcMain.handle('cli-updates:open-log', async (_evt, cli: CliUpdateKind): Promise<boolean> => {
+    // WHY the renderer names the CLI and not a path (#1423 review a): the
+    // renderer used to send the path, and this handler handed any string to
+    // shell.openPath, which opens files AND applications. Main already holds
+    // the one path it wrote: the current failed state's log. Anything else
+    // (an unknown CLI, a state that is not `failed`) opens nothing.
+    const state = cli === 'claude' || cli === 'codex' ? orchestrator.getSnapshot()[cli] : undefined
+    if (state?.kind !== 'failed') return false
+    // shell.openPath RESOLVES with '' on success and an error string on
+    // failure. WHY the answer is returned (#1250 row 10): this used to discard
+    // the string on the belief that "the OS shell surfaces its own error
+    // dialog". It does not: a log that is gone (removed by hand; nothing
+    // prunes this directory automatically) gives back "Failed to open path"
+    // and nothing appears, so View Log looked broken. Only a boolean crosses
+    // to the renderer, which shows its own fixed words; the OS text stays in
+    // this diagnostic log (q22).
     try {
-      await shell.openPath(logPath)
+      const error = await shell.openPath(state.logPath)
+      if (error === '') return true
+      console.warn('[cli-updates] failed to open log:', error)
+      return false
     } catch (err) {
       console.warn('[cli-updates] failed to open log:', err)
+      return false
     }
   })
 }

src/preload/api/cliUpdates.ts

@@ -34,12 +34,12 @@ export const cliUpdatesApi = {
    *  for the state push. */
   cliUpdatesSetBehavior: (behavior: CliUpdateBehavior): Promise<CliUpdateSnapshot> =>
     ipcRenderer.invoke('cli-updates:set-behavior', behavior),
-  /** Open a failure log file in the OS default handler. No return value
-   *  — success is a spawned OS window, failure is silent (the log path
-   *  can be stale after retention pruning; a hard error case is
-   *  logged in main). */
-  cliUpdatesOpenLog: (logPath: string): Promise<void> =>
-    ipcRenderer.invoke('cli-updates:open-log', logPath),
+  /** Open this CLI's current failure log in the OS default handler. Main
+   *  picks the path from its own failed state (#1423 review a: a
+   *  renderer-supplied path could open anything). `false` when it did not
+   *  open; the caller says so in its own words (#1250 row 10). */
+  cliUpdatesOpenLog: (cli: CliUpdateKind): Promise<boolean> =>
+    ipcRenderer.invoke('cli-updates:open-log', cli),
   /** Subscribe to state changes. Returns an Unsub. The subscriber gets
    *  every transition — updating→updated, updating→failed, notify
    *  transitions on manual refresh, and behavior changes. */

src/renderer/src/features/cli-updates/CliUpdateBanner.renderer.test.tsx

@@ -1,4 +1,4 @@
-import { fireEvent, render, screen } from '@testing-library/react'
+import { act, fireEvent, render, screen } from '@testing-library/react'
 import { describe, expect, it, vi } from 'vitest'
 
 import { CliUpdateBanner, describeState } from './CliUpdateBanner'
@@ -35,3 +35,72 @@ describe('CliUpdateBanner deferred state (#1243)', () => {
     expect(dismissKey('claude', first)).toBe(dismissKey('claude', { ...first }))
   })
 })
+
+// #1250 row 10: View Log did nothing visible when the log could not be opened
+// (a log removed by hand or another tool; the OS shows no dialog, and nothing
+// prunes this directory automatically). The row says so now,
+// in fixed words, and only when main answers that it did not open.
+describe('CliUpdateBanner View Log', () => {
+  const failed = { kind: 'failed' as const, cli: 'claude' as const, from: '2.1.281', wantedLatest: '2.1.282', installMethod: 'npm' as const, reason: 'command-failed' as const, logPath: '/logs/claude-update.log', finishedAt: 1 }
+
+  it.each([
+    [false, true],
+    [true, false],
+  ])('when opening the log answers %s, the row shows the failure: %s', async (opened, shown) => {
+    const openLog = vi.fn(async () => opened)
+    Object.defineProperty(window, 'api', { configurable: true, value: { ...(window as { api?: object }).api, cliUpdatesOpenLog: openLog } })
+    useCliUpdateStore.setState({ snapshot: { ...DEFAULT_CLI_UPDATE_SNAPSHOT, claude: failed }, dismissed: new Set() })
+    render(<CliUpdateBanner />)
+    await act(async () => { fireEvent.click(screen.getByRole('button', { name: 'View Log' })) })
+    // The CLI, never a path: main picks the log it wrote (#1423 review a).
+    expect(openLog).toHaveBeenCalledWith('claude')
+    const alert = screen.queryByRole('alert')
+    if (shown) expect(alert).toHaveTextContent("Couldn't open the update log. It may have been cleaned up; the next failed update writes a new one.")
+    else expect(alert).toBeNull()
+  })
+
+  // #1423 review a and c: a later click clears the alert, and an older,
+  // slower answer must not overwrite a newer one. The alert is shown FIRST,
+  // so this cannot pass on a banner that never shows one.
+  it('clears on a later click, and keeps the latest click\'s answer when an older one arrives last', async () => {
+    const answers: Array<(opened: boolean) => void> = []
+    const openLog = vi.fn(() => new Promise<boolean>(resolve => { answers.push(resolve) }))
+    Object.defineProperty(window, 'api', { configurable: true, value: { ...(window as { api?: object }).api, cliUpdatesOpenLog: openLog } })
+    useCliUpdateStore.setState({ snapshot: { ...DEFAULT_CLI_UPDATE_SNAPSHOT, claude: failed }, dismissed: new Set() })
+    render(<CliUpdateBanner />)
+    const button = screen.getByRole('button', { name: 'View Log' })
+    await act(async () => { fireEvent.click(button) })
+    await act(async () => { answers[0]!(false) })
+    expect(screen.getByRole('alert')).toBeTruthy()
+    // A later click clears it at once, before its answer arrives.
+    await act(async () => { fireEvent.click(button) })
+    expect(screen.queryByRole('alert')).toBeNull()
+    await act(async () => { fireEvent.click(button) })
+    await act(async () => { answers[2]!(true) })
+    await act(async () => { answers[1]!(false) })
+    expect(screen.queryByRole('alert')).toBeNull()
+  })
+
+  // #1423 review c: a REJECTED request (the IPC itself failed) is a failed
+  // open too, said the same way, never an unhandled rejection.
+  it('says a rejected request the same way', async () => {
+    Object.defineProperty(window, 'api', { configurable: true, value: { ...(window as { api?: object }).api, cliUpdatesOpenLog: vi.fn(async () => { throw new Error('IPC unavailable') }) } })
+    useCliUpdateStore.setState({ snapshot: { ...DEFAULT_CLI_UPDATE_SNAPSHOT, claude: failed }, dismissed: new Set() })
+    render(<CliUpdateBanner />)
+    await act(async () => { fireEvent.click(screen.getByRole('button', { name: 'View Log' })) })
+    expect(screen.getByRole('alert')).toHaveTextContent("Couldn't open the update log.")
+    expect(document.body.textContent).not.toContain('IPC unavailable')
+  })
+
+  // #1423 review a: a new failed run writes a new log; the old "couldn't
+  // open" says nothing about it.
+  it('clears the alert when a new failure brings a new log', async () => {
+    Object.defineProperty(window, 'api', { configurable: true, value: { ...(window as { api?: object }).api, cliUpdatesOpenLog: vi.fn(async () => false) } })
+    useCliUpdateStore.setState({ snapshot: { ...DEFAULT_CLI_UPDATE_SNAPSHOT, claude: failed }, dismissed: new Set() })
+    render(<CliUpdateBanner />)
+    await act(async () => { fireEvent.click(screen.getByRole('button', { name: 'View Log' })) })
+    expect(screen.getByRole('alert')).toBeTruthy()
+    act(() => { useCliUpdateStore.setState({ snapshot: { ...DEFAULT_CLI_UPDATE_SNAPSHOT, claude: { ...failed, logPath: '/logs/claude-update-2.log', finishedAt: 2 } } }) })
+    expect(screen.queryByRole('alert')).toBeNull()
+  })
+})

src/renderer/src/features/cli-updates/CliUpdateBanner.tsx

@@ -1,4 +1,4 @@
-import { useMemo, useState } from 'react'
+import { useMemo, useRef, useState } from 'react'
 
 import type { CliUpdateKind, CliUpdateState } from '@shared/types/cliUpdate.js'
 import { dismissKey, useCliUpdateStore } from '@renderer/features/cli-updates/store'
@@ -32,7 +32,9 @@ import { dismissKey, useCliUpdateStore } from '@renderer/features/cli-updates/st
 type BannerEntry = {
   tone: 'info' | 'warning' | 'success'
   text: string
-  action?: { label: string; onClick: () => void }
+  /** `onClick` may answer whether it worked; a `false` shows `failureText`
+   *  on the row (#1250 row 10: View Log failed with nothing on screen). */
+  action?: { label: string; onClick: () => void | Promise<boolean | void>; failureText?: string; resultKey?: string }
   // Rendered next to the action as a tiny info button. When present,
   // the banner shows an expandable diagnostic hint. Only failed states
   // set this today; other states don't need the extra context.
@@ -44,6 +46,9 @@ type BannerEntry = {
   undismissable?: boolean
 }
 
+// Fixed words (q22): the OS failure text stays in main's diagnostic log.
+export const CLI_UPDATE_LOG_NOT_OPENED = "Couldn't open the update log. It may have been cleaned up; the next failed update writes a new one."
+
 export function describeState(cli: CliUpdateKind, state: CliUpdateState): BannerEntry | null {
   const label = cli === 'claude' ? 'Claude Code' : 'Codex'
   switch (state.kind) {
@@ -101,9 +106,11 @@ export function describeState(cli: CliUpdateKind, state: CliUpdateState): Banner
         text: `${label} auto-update failed${methodHint} — ${reasonHint}. Wanted ${state.wantedLatest}, still at ${state.from}.`,
         action: {
           label: 'View Log',
-          onClick: () => {
-            void window.api.cliUpdatesOpenLog(state.logPath)
-          },
+          onClick: () => window.api.cliUpdatesOpenLog(cli),
+          failureText: CLI_UPDATE_LOG_NOT_OPENED,
+          // A new failed run writes a new log: an earlier "couldn't open"
+          // says nothing about it (#1423 review a).
+          resultKey: state.logPath,
         },
         hint,
       }
@@ -190,6 +197,22 @@ function BannerRow({
   onDismiss: (key: string) => void
 }) {
   const [hintOpen, setHintOpen] = useState(false)
+  // The failure belongs to one action target; a new target clears it.
+  const [failedFor, setFailedFor] = useState<string | null>(null)
+  const resultKey = entry.action?.resultKey ?? ''
+  const actionFailed = failedFor === resultKey
+  // Only the LATEST click may set the result (#1423 review a): an older,
+  // slower answer must not overwrite a newer one.
+  const latestRun = useRef(0)
+  const runAction = async () => {
+    const action = entry.action
+    if (!action) return
+    const run = ++latestRun.current
+    setFailedFor(null)
+    const worked = await Promise.resolve(action.onClick()).catch(() => false as const)
+    if (run !== latestRun.current) return
+    if (worked === false && action.failureText) setFailedFor(action.resultKey ?? '')
+  }
   const toneClasses =
     // Uses the semantic tokens from #520 (warning-*, info-*, success-*)
     // so custom-appearance JSON overrides theme correctly.
@@ -223,7 +246,7 @@ function BannerRow({
         {entry.action && (
           <button
             type="button"
-            onClick={entry.action.onClick}
+            onClick={() => void runAction()}
             className="rounded-control border border-current px-2 py-0.5 text-[10px] uppercase tracking-wide hover:bg-current/10"
           >
             {entry.action.label}
@@ -255,6 +278,11 @@ function BannerRow({
           {entry.hint}
         </div>
       )}
+      {actionFailed && entry.action?.failureText && (
+        <div role="alert" className="border-t border-current/30 px-3 py-2 text-[10px] leading-relaxed text-danger">
+          {entry.action.failureText}
+        </div>
+      )}
     </div>
   )
 }