Integration batch I (#1423, #1424)

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).

docs/plans/2026-09-27-state-file-reveal-failure.md

@@ -0,0 +1,43 @@
+# Recovery-state actions say when they did not work (#1250 row 14)
+
+Short plan: a bug with a known root cause. The row comes from `temp/quality-loop/hunt-c3.md` (row 14, P3). #1250 is a batch issue, so this PR is `Refs #1250`.
+
+## Outcome
+When an Agent Code-managed skill state file is unreadable, three surfaces show a recovery panel with **Reveal State File** and **Reset State**: Settings → Skills (`SkillsGrid`), Conventions, and Custom Skills. A reveal that main refuses (`{ ok: false, message }`, e.g. "No managed skill recovery file exists." once the file is gone), or an IPC that rejects, now shows a message in the panel's row. Today the result is dropped. Reset in Conventions and Custom Skills also gains a rejection handler.
+
+## Root cause (verified in source, origin/main)
+- All three Reveal buttons: `void window.api.reveal…RecoveryFile()`, with the `{ ok, message }` answer ignored.
+- Conventions / Custom Skills Reset: `void window.api.reset…Recovery().then(applyResult)`. A rejection is unhandled. (`SkillsGrid`'s reset goes through its `run`, which already catches.)
+- Main's refusal messages are fixed, curated strings (`src/main/ipc/agentCode{Conventions,CustomSkills,InstalledSkills}.ts`).
+
+## Design (contract)
+- **Reveal:** on `{ ok: false }`, the row's existing error state shows main's `message`, or "Couldn't reveal the state file." when it has none. On a rejection, it shows the same fixed sentence. On success, the error is cleared.
+- **Reset** (Conventions, Custom Skills): on a rejection, "Couldn't reset the state. Try again." (fixed words; the rejection text is IPC text, q22).
+- Each row already renders its `error` as an alert; no new surface.
+
+## Tests (red on main)
+- **`AgentCodeConventionsRow.renderer.test.tsx`:** with a recovery snapshot, a refused reveal shows main's message; a rejected reset shows the fixed sentence.
+- **`AgentCodeCustomSkillsRow.renderer.test.tsx`:** a refused reveal shows main's message.
+- **`Skills.renderer.test.tsx`:** a refused installed-skills reveal shows main's message.
+
+## Out of scope
+- #1250's other rows.
+- The existing `cause.message` displays elsewhere in these rows (a separate q22 sweep).
+
+## Review round 1 (a, b, c: FIX-BEFORE-MERGE)
+- **a1 (Major): a reset whose unlink failed answered `io-error` with the raw Node message** (an absolute path and an OS code), and the panel showed it. All three resets now answer "Couldn't remove the unreadable state file. Check that Agent Code's data folder is writable, then try again." and warn the raw error in main.
+  - The plan's claim that main's messages are "fixed, curated strings" was true for reveal, not for reset; corrected here.
+  - The same raw pass-through elsewhere in the service is split into #1427.
+  - Real-filesystem system test (a read-only state directory), red on the previous head.
+- **a2 (Major): reveal answered ok for a file removed since load.** `showItemInFolder` answers nothing, so all three handlers now `lstat` first and refuse a missing file. Test covering all three channels, red on the previous head.
+- **a3 (Minor): the Conventions TARGET reveal dropped its answer.** It uses the same helper, with "Couldn't reveal that folder." as its fallback.
+- **b1 (Major): the Skills grid showed a reveal failure below every skill row.** It now shows inside the recovery panel, beside its buttons. The test asserts the placement.
+- **b2 (Major): a late reveal success cleared a newer reset failure.** A success clears only the reveal's own earlier message. Unit-tested.
+- **c (test gaps):** the Custom Skills reset rejection, the helper's reject/fallback/success branches (unit test), and the ordering rule.
+
+## Verification (a, b: FIX-BEFORE-MERGE; c: MERGE-READY) and steering q111
+- **b1 / q111.1: a late reveal FAILURE still replaced a newer reset failure.** The WeakMap compared message text on success only. It is replaced by a per-panel action gate (`useRecoveryActionGate`): every reveal or reset takes a generation, and only the latest completion may speak, for a success and a failure alike. An older reset still applies its snapshot (`onStale`), never its message.
+  - Pinned with deferred real actions through the real Conventions row, both orders. The failure order is red on the previous head.
+  - The gate is unit-tested: both orders, and `invalidate`.
+- **b2 / q111.2: the Skills grid carried a reveal refusal into a later recovery episode.** `useRecoveryEpisode` clears the grid's recovery message and retires in-flight actions when the recovery identity (state path plus message) changes or disappears. Test: refuse, recovery clears, a new recovery shows no old message.
+- **a1 / q111.3: reveal accepted a DIRECTORY at the state-file path.** All three handlers now require a regular file (`lstat().isFile()`). The test covers all three channels with a real directory. The race between that `lstat` and the OS call remains the honest Electron limit: `showItemInFolder` returns nothing.

src/main/agentCodeConventions/AgentCodeConventionsService.system.test.ts

@@ -1,7 +1,7 @@
-import { mkdir, mkdtemp, readFile, realpath, rename, rm, stat, symlink, writeFile } from 'fs/promises'
+import { chmod, mkdir, mkdtemp, readFile, realpath, rename, rm, stat, symlink, writeFile } from 'fs/promises'
 import { tmpdir } from 'os'
 import { join } from 'path'
-import { afterEach, describe, expect, it } from 'vitest'
+import { afterEach, describe, expect, it, vi } from 'vitest'
 
 import {
   createEmptyAgentCodeConventionsDocument,
@@ -692,3 +692,39 @@ async function writeFileWithParents(path: string, contents: string): Promise<voi
   await mkdir(dirname(path), { recursive: true })
   await writeFile(path, contents)
 }
+
+// #1424 review a: the recovery panel shows a reset's result message as it is,
+// and a failed unlink's message carried the state file's absolute path and
+// the OS code. All three resets now answer in fixed words (q22). Real
+// filesystem: an unreadable state file inside a read-only directory.
+describe('recovery reset that cannot remove the state file', () => {
+  it('answers in fixed words, never the path or the OS error', async () => {
+    const root = await temporaryDirectory()
+    const stateDirectory = join(root, 'state')
+    const stateFilePath = join(stateDirectory, 'conventions.json')
+    await writeFileWithParents(stateFilePath, 'not json at all')
+    const service = new AgentCodeConventionsService({
+      stateFilePath,
+      homeDirectory: root,
+      resolveTargets: async () => ({ targets: [], unsupportedProviders: [] }),
+    })
+    await service.initialize()
+    expect(await service.getSnapshot()).toMatchObject({ health: 'recovery-required' })
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
+    await chmod(stateDirectory, 0o500)
+    try {
+      const results = [await service.resetRecovery(), await service.resetCustomSkillsRecovery(), await service.resetInstalledSkillsRecovery()]
+      for (const result of results) {
+        expect(result).toMatchObject({ ok: false, code: 'io-error' })
+        const message = (result as { message: string }).message
+        expect(message).toBe("Couldn't remove the unreadable state file. Check that Agent Code's data folder is writable, then try again.")
+        expect(message).not.toContain(root)
+      }
+      // The raw error is kept for diagnosis.
+      expect(warn).toHaveBeenCalled()
+    } finally {
+      await chmod(stateDirectory, 0o700)
+      warn.mockRestore()
+    }
+  })
+})

src/main/agentCodeConventions/AgentCodeConventionsService.ts

@@ -1061,7 +1061,7 @@ export class AgentCodeManagedSkillsService {
       try {
         await resetAgentCodeConventionsState(this.stateFilePath)
       } catch (error) {
-        return this.installedIoError(error)
+        return this.installedIoError(recoveryResetError(error))
       }
       this.document = createEmptyAgentCodeConventionsDocument()
       this.recovery = undefined
@@ -1479,7 +1479,7 @@ export class AgentCodeManagedSkillsService {
       try {
         await resetAgentCodeConventionsState(this.stateFilePath)
       } catch (error) {
-        return this.customIoError(error)
+        return this.customIoError(recoveryResetError(error))
       }
       this.document = createEmptyAgentCodeConventionsDocument()
       this.recovery = undefined
@@ -1688,7 +1688,7 @@ export class AgentCodeManagedSkillsService {
       try {
         await resetAgentCodeConventionsState(this.stateFilePath)
       } catch (error) {
-        return this.ioError(error)
+        return this.ioError(recoveryResetError(error))
       }
       this.document = createEmptyAgentCodeConventionsDocument()
       this.recovery = undefined
@@ -3693,6 +3693,16 @@ export class AgentCodeManagedSkillsService {
 // consumers to migrate in the same feature diff.
 export { AgentCodeManagedSkillsService as AgentCodeConventionsService }
 
+// WHY a fixed sentence for a failed recovery reset (#1424 review a): the
+// unlink error carries the state file's absolute path and the OS code, and
+// the recovery panel shows the result's message as it is. The raw error stays
+// in the main log for diagnosis (q22). The rest of this service still passes
+// error.message through; that wider sweep is a separate issue.
+function recoveryResetError(error: unknown): Error {
+  console.warn('[agent-code-conventions] recovery reset failed:', error)
+  return new Error("Couldn't remove the unreadable state file. Check that Agent Code's data folder is writable, then try again.")
+}
+
 function safeErrorMessage(error: unknown): string {
   if (error instanceof Error && error.message) return error.message
   return 'Unknown filesystem error'

src/main/ipc/agentCodeConventions.ts

@@ -83,6 +83,14 @@ export function registerAgentCodeConventionsIpc(
   ipcMain.handle('agent-code-conventions:reveal-recovery', async () => {
     const path = await service.resolveRecoveryFile()
     if (!path) return { ok: false, message: 'No conventions recovery file exists.' }
+    // WHY checked (#1424 review a, steering q111): the recovery snapshot keeps
+    // the path it found at load, and showItemInFolder answers nothing, so a
+    // file removed since then (or replaced by a directory) used to report ok
+    // with no state file revealed. A regular file is required. What remains
+    // is the honest Electron limit: the file can still change between this
+    // lstat and the OS call, and the OS call cannot report it.
+    const file = await lstat(path).catch(() => null)
+    if (!file?.isFile()) return { ok: false, message: 'The state file is no longer there. Refresh to check again.' }
     shell.showItemInFolder(path)
     return { ok: true }
   })

src/main/ipc/agentCodeCustomSkills.ts

@@ -126,6 +126,14 @@ export function registerAgentCodeCustomSkillsIpc(service: AgentCodeConventionsSe
   ipcMain.handle('agent-code-custom-skills:reveal-recovery', async () => {
     const path = await service.resolveRecoveryFile()
     if (!path) return { ok: false, message: 'No managed skill recovery file exists.' }
+    // WHY checked (#1424 review a, steering q111): the recovery snapshot keeps
+    // the path it found at load, and showItemInFolder answers nothing, so a
+    // file removed since then (or replaced by a directory) used to report ok
+    // with no state file revealed. A regular file is required. What remains
+    // is the honest Electron limit: the file can still change between this
+    // lstat and the OS call, and the OS call cannot report it.
+    const file = await lstat(path).catch(() => null)
+    if (!file?.isFile()) return { ok: false, message: 'The state file is no longer there. Refresh to check again.' }
     shell.showItemInFolder(path)
     return { ok: true }
   })

src/main/ipc/agentCodeInstalledSkills.ts

@@ -140,6 +140,14 @@ export function registerAgentCodeInstalledSkillsIpc(service: AgentCodeConvention
   ipcMain.handle('agent-code-installed-skills:reveal-recovery', async () => {
     const path = await service.resolveRecoveryFile()
     if (!path) return { ok: false, message: 'No managed skill recovery file exists.' }
+    // WHY checked (#1424 review a, steering q111): the recovery snapshot keeps
+    // the path it found at load, and showItemInFolder answers nothing, so a
+    // file removed since then (or replaced by a directory) used to report ok
+    // with no state file revealed. A regular file is required. What remains
+    // is the honest Electron limit: the file can still change between this
+    // lstat and the OS call, and the OS call cannot report it.
+    const file = await lstat(path).catch(() => null)
+    if (!file?.isFile()) return { ok: false, message: 'The state file is no longer there. Refresh to check again.' }
     shell.showItemInFolder(path)
     return { ok: true }
   })

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()
+})