diff --git a/CLAUDE.md b/CLAUDE.md index a65f2017..2093820f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -208,7 +208,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Unified session list**: `GET /api/sessions/unified` merges live sessions, persisted state, lifecycle-log history, and Claude transcript files into one deduped list (pure core in `src/services/unified-session-service.ts`). Transcript rows fold into their owning session via a `claudeSessionId → Codeman id` alias map, so resumed and `/clear`-respawned sessions do not appear twice. No terminal buffers in the response, unlike `/api/sessions`. Backs the Cmd+K Session Manager, plus pinning and cross-device tab order (`PUT /api/session-order`; pure merge helpers in `src/session-order.ts`, pushing device wins and server-only ids are never dropped). → [architecture-invariants#unified-session-list-and-session-manager](docs/architecture-invariants.md#unified-session-list-and-session-manager) -**Hook events**: Claude Code hooks trigger via `/api/hook-event`. Key events: `permission_prompt`, `elicitation_dialog`, `elicitation_complete`, `elicitation_response`, `idle_prompt`, `stop`, `teammate_idle`, `task_completed`. See `src/hooks-config.ts`; upstream hook semantics mirrored in `docs/claude-code-hooks-reference.md`. +**Hook events**: Claude Code hooks trigger via `/api/hook-event`. Key events: `permission_prompt`, `elicitation_dialog`, `elicitation_complete`, `elicitation_response`, `idle_prompt`, `stop`, `teammate_idle`, `task_completed`. See `src/hooks-config.ts`; upstream hook semantics mirrored in `docs/claude-code-hooks-reference.md`. ⚠️ **Every claude session INSTALLS the hooks block into its workspace** (`ensureCodemanHooks`, add-only merge that keeps a user's own handlers), from both create paths and from `restoreMuxSessions()` for sessions recovered on server start. Before 2026-08-15 hooks were written ONLY when Codeman created the case DIRECTORY, so a linked case / cloned repo — where most sessions actually run — had no hooks at all and every hook-driven surface was silently dead there: an AskUserQuestion dialog blocked the pane while the tab and the phone overview both read a calm `idle`, with no Approvals Inbox item, no push, no definitive `stop`/`idle_prompt` for respawn and no `stop`/`blocked` for the wait endpoints. Do not restore the old "never add" policy. ⚠️ Claude Code RE-READS `settings.local.json`, so an already-running session starts firing hooks without a restart (measured 2026-08-15) — and the notification for a blocking dialog is delayed by Claude Code (~30s), so the alert trails the dialog. ⚠️ An AskUserQuestion / plan-selection dialog arrives as **`permission_prompt`**, not `elicitation_dialog` (that one is MCP elicitation), so it renders as the RED "needs you" alert, not the yellow idle one. **Approvals Inbox** (cross-session queue of prompts waiting on a human; `approvalsInboxEnabled`, SYNCED, default OFF: every surface is opt-in; only the store and answer endpoints run regardless, so flipping it ON shows anything already pending): `web/approval-inbox.ts` is a `sessionWaits`-style singleton fed by `/api/hook-event`, holding at most ONE item per session (a new prompt supersedes), claude-mode only, in-memory. Cards are answered via `POST /api/approvals/:id/answer`, which sends a digit / Esc / idle-prompt text through `writeViaMux` (menu answers never carry `\r`). ⚠️ `option` digits are accepted ONLY when they match options parsed from the captured pane frame, and the answer path RE-CAPTURES the pane first (a dialog that no longer parses on screen means the keystroke would land in the composer, so refuse with 409). ⚠️ Resolution on the heuristic `working` signal is restricted to `idle` items; permission/question items clear only on definitive signals (`stop`, `elicitation_complete`/`elicitation_response`, exit/delete, answer, supersede, 12h TTL). The frontend seeds from `GET /api/approvals` in `handleInit` **regardless of the setting**: the seed re-arms the tab-alert state machine (`setPendingHook`) unconditionally, and only populating `this.approvals` (the inbox surfaces) is gated — seeding used to be gated wholesale, which left a reloaded page with NO red tab while a permission dialog sat blocking a session (2026-08-15); `_onApprovalResolved` clears the pending-hook alert unconditionally for the same reason. ⚠️ The red/yellow tab alert itself is a STEADY border/background/dot with a pulse on top: the original keyframes swung to transparent at 0%/100%, so half of every cycle looked like a normal tab. Push Approve/Deny buttons stay gated on the setting (`sendPushNotifications` strips `actions`/`approvalId` when OFF) and are answered from `sw.js` directly so they work with no tab open. Surfaces (all gated on the setting): header bell (marker-hidden until count > 0, phones never show it) + drawer (`approvals-ui.js`), phone overview NEEDS YOU answer strips (`mobile-overview.js`). Design: `docs/approvals-inbox-plan.md`. diff --git a/src/hooks-config.ts b/src/hooks-config.ts index c9365de6..39164046 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -646,22 +646,28 @@ export async function writeHooksConfig(casePath: string): Promise { } /** - * Ensures an explicitly managed case has the current Codeman hooks. + * Ensures a workspace Codeman is about to run Claude in has the current Codeman hooks. * - * Unlike `refreshStaleCodemanHooks`, this may add Codeman handlers to a valid - * user-owned settings file. It is therefore reserved for case quick-starts, - * where the user has explicitly asked Codeman to manage that workspace. A - * malformed existing file is left untouched rather than replaced. + * Unlike `refreshStaleCodemanHooks`, this may ADD Codeman handlers to a settings + * file that has none (a linked case, a cloned repo, any directory Codeman did not + * scaffold). It merges rather than replaces, so a user's own hook entries survive, + * and a malformed existing file is left untouched rather than replaced. * - * ⚠️ It has NO production call site: PR #233 landed it with the hook scripts and never - * wired it up, and knip can't flag it (`test/**` are entry points, so its tests count as - * a use). Kept anyway, because it is redundant with neither sibling: `writeHooksConfig` - * REPLACES a malformed settings file and rewrites unconditionally, and - * `refreshStaleCodemanHooks` deliberately never adds hooks to a case that has none. The - * one place it fits is quick-start's existing-case branch in session-routes.ts, and - * moving that branch onto this function is a POLICY change (hooks would come back for a - * user who deleted them from their case, and linked cases would start getting a hooks - * block they have never had), so that call is left to the owner rather than made here. + * ⚠️ That "may add" is a deliberate POLICY, adopted 2026-08-15 after the symptom it + * causes was reported: hooks were only ever written when Codeman CREATED a case + * directory, so every session in a linked case ran with no hooks at all and each + * hook-driven surface was silently dead there — an AskUserQuestion dialog blocking + * the pane while the tab and the phone overview both read a calm `idle`, no + * Approvals Inbox item, no push, no definitive `stop`/`idle_prompt` for respawn, and + * no `stop`/`blocked` for the agent wait endpoints. The cost of the policy is the + * other direction: a user who DELETES Codeman's hooks from a workspace gets them + * back on the next session create there, because nothing on disk distinguishes + * "removed on purpose" from "never had any". + * + * Called from both session-create paths (`POST /api/sessions`, `POST /api/quick-start`) + * for claude mode, and from `restoreMuxSessions()` so sessions that predate this heal + * on the next server start. Claude Code re-reads the file, so a session ALREADY running + * in the workspace picks the hooks up without a restart (verified live, 2026-08-15). */ export async function ensureCodemanHooks(casePath: string): Promise { await withSafeSettingsWrite(casePath, 'hooks (ensure)', async (claudeDir, settingsPath) => { diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index cb5ccb2e..89a1f5e3 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -84,6 +84,7 @@ import { applyAgentSkill, refreshUserAgentSkill, seedAgentSessionPreamble, + ensureCodemanHooks, refreshStaleCodemanHooks, } from '../../hooks-config.js'; import { generateClaudeMd } from '../../templates/claude-md.js'; @@ -765,11 +766,21 @@ export function registerSessionRoutes( await applyStatusLineConfig(workingDir, true); } - // COD-91 self-heal: refresh a pre-secret hooks block in an existing case so the now - // unconditional hook-secret gate keeps accepting its hook events. No-op for fresh - // cases (writeHooksConfig already wrote the secret) and for non-Codeman/absent hooks. + // Hooks for the workspace this session runs in. ADD-ONLY and merge-based: + // Codeman's own handlers are (re)written, a user's own hook entries are kept. + // + // This used to be `refreshStaleCodemanHooks`, which deliberately never ADDS — + // and `writeHooksConfig` only runs when Codeman CREATES a case directory. So a + // session in a LINKED case or any pre-existing repo (where most sessions live) + // got no hooks block at all, and every hook-driven surface was silently dead + // there: no permission/question tab alert, no Approvals Inbox item, no push, + // no definitive `stop`/`idle_prompt` for respawn, and no `stop`/`blocked` for + // the agent wait endpoints. Measured 2026-08-15 on a linked case: an + // AskUserQuestion dialog sat on screen with the tab showing plain `idle`. + // Claude Code re-reads the file, so a session already running in that + // workspace starts firing hooks too (verified live, same day). if ((body.mode ?? 'claude') === 'claude') { - await refreshStaleCodemanHooks(workingDir).catch(() => {}); + await ensureCodemanHooks(workingDir).catch(() => {}); // Agent skill (docs/agent-control-plan.md §2): ADD-ONLY on create, same shared- // .claude rationale as the statusLine above: a create must never remove the // skill from under other live sessions in the repo. Marker-guarded, so a @@ -2899,11 +2910,19 @@ export function registerSessionRoutes( return createErrorResponse(ApiErrorCode.OPERATION_FAILED, `Failed to create case: ${getErrorMessage(err)}`); } } else if (!remote && !docker && mode !== 'opencode') { - // COD-91 self-heal for an EXISTING case: refresh a pre-secret hooks block so the - // now-unconditional hook-secret gate keeps accepting its hook events. No-op when - // the hooks aren't ours or already carry the secret. Skipped for remote cases — - // resolvedCasePath is a REMOTE path that doesn't exist on the local filesystem. - await refreshStaleCodemanHooks(resolvedCasePath).catch(() => {}); + // EXISTING case directory (a linked case, a cloned repo, anything Codeman did + // not scaffold). Claude mode INSTALLS the hooks block when it is missing and + // refreshes ours when it is stale — a linked case never got one otherwise, which + // left every hook-driven surface dead there (see POST /api/sessions above). + // Other modes keep the narrower COD-91 self-heal: only claude reads `.claude` + // hooks, so a shell/codex quick-start should not author a block of its own. + // Skipped for remote cases — resolvedCasePath is a REMOTE path that doesn't + // exist on the local filesystem. + if (mode === 'claude') { + await ensureCodemanHooks(resolvedCasePath).catch(() => {}); + } else { + await refreshStaleCodemanHooks(resolvedCasePath).catch(() => {}); + } } // Agent skill injection (docs/agent-control-plan.md §2): ADD-ONLY on create, @@ -2936,7 +2955,10 @@ export function registerSessionRoutes( if (!existsSync(join(resolvedCasePath, '.claude', 'settings.local.json'))) { await writeHooksConfig(resolvedCasePath); } else { - await refreshStaleCodemanHooks(resolvedCasePath).catch(() => {}); + // A settings file with no hooks in it is the same dead-surface case as a + // linked case: this branch is already gated on `docker.hooksEnabled`, so + // install ours rather than only refreshing an existing block. + await ensureCodemanHooks(resolvedCasePath).catch(() => {}); } } catch { /* non-fatal — the session still runs, hooks may be degraded */ diff --git a/src/web/server.ts b/src/web/server.ts index 4231b162..189d4db9 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -76,6 +76,7 @@ import { RunSummaryTracker } from '../run-summary.js'; import { PlanOrchestrator } from '../plan-orchestrator.js'; import { OrchestratorLoop } from '../orchestrator-loop.js'; import { getLifecycleLog } from '../session-lifecycle-log.js'; +import { ensureCodemanHooks } from '../hooks-config.js'; import { PushSubscriptionStore } from '../push-store.js'; import webpush from 'web-push'; import { SseStreamManager } from './sse-stream-manager.js'; @@ -2819,6 +2820,13 @@ export class WebServer extends EventEmitter { } } + // Sessions recovered from a previous run predate the create-path hook + // install, and these are long-lived: by the time a server restart comes + // round a session may be days old and has been running hook-blind the + // whole time. Claude Code re-reads settings.local.json, so writing the + // block now arms the RUNNING CLI, no session restart needed. + await this.ensureHooksForRecoveredWorkspaces(); + // Start stats collection for mux sessions this.mux.startStatsCollection(STATS_COLLECTION_INTERVAL_MS); } @@ -2845,6 +2853,32 @@ export class WebServer extends EventEmitter { } } + /** + * Install Codeman's hooks into the workspaces of the sessions just recovered. + * + * Deduped by workspace, because sessions in one repo share a single + * `.claude/settings.local.json` and the write is otherwise repeated per tab. + * Claude mode only (nothing else reads `.claude` hooks), never for remote + * sessions (their `workingDir` is a path on ANOTHER host, so writing it here + * would scaffold a stray directory locally), and never for a docker case that + * opted out of hooks. + * + * Failures are swallowed per workspace: `ensureCodemanHooks` already refuses + * unsafe targets with a warning, and a workspace we cannot write to must not + * stop the rest of recovery. + */ + private async ensureHooksForRecoveredWorkspaces(): Promise { + const workspaces = new Set(); + for (const session of this.sessions.values()) { + if (session.mode !== 'claude' || session.remote) continue; + if (session.docker && !session.docker.hooksEnabled) continue; + if (session.workingDir) workspaces.add(session.workingDir); + } + for (const workspace of workspaces) { + await ensureCodemanHooks(workspace).catch(() => {}); + } + } + /** * COD-108 — handle a `remoteSessionDropped` emit from the watcher: reattach * the dropped remote session and report the outcome back to the watcher so it diff --git a/test/routes/session-routes-workspace-hooks.test.ts b/test/routes/session-routes-workspace-hooks.test.ts new file mode 100644 index 00000000..c1960cf8 --- /dev/null +++ b/test/routes/session-routes-workspace-hooks.test.ts @@ -0,0 +1,111 @@ +/** + * @fileoverview Hooks are installed into the workspace a claude session starts in. + * + * Regression cover for the 2026-08-15 report: a session in a LINKED case (the user's + * own repo, where most sessions live) ran with no hooks block at all, because + * `writeHooksConfig` only fires when Codeman CREATES a case directory and the old + * self-heal call deliberately never ADDED one. The visible symptom was an + * AskUserQuestion dialog blocking the pane while the tab and the phone overview both + * showed a calm `idle` — no hook event, so no pending-hook state, so no alert. + * + * Asserts bytes on disk (the real `ensureCodemanHooks`), not a spy call. + * Uses app.inject(), so no real HTTP port is needed. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import Fastify, { type FastifyInstance } from 'fastify'; +import fastifyCookie from '@fastify/cookie'; +import { mkdtemp, rm, readFile, mkdir, writeFile } from 'node:fs/promises'; +import { existsSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { createMockRouteContext } from '../mocks/index.js'; +import { installRouteErrorHandler } from '../../src/web/route-error-handler.js'; +import { registerSessionRoutes } from '../../src/web/routes/session-routes.js'; + +interface HooksFile { + hooks?: Record }>>; + permissions?: unknown; + model?: unknown; +} + +describe('POST /api/sessions workspace hooks', () => { + let app: FastifyInstance; + let workingDir: string; + + const settingsPath = () => join(workingDir, '.claude', 'settings.local.json'); + const readSettings = async (): Promise => JSON.parse(await readFile(settingsPath(), 'utf-8')); + + const createSession = (payload: Record) => + app.inject({ method: 'POST', url: '/api/sessions', payload }); + + beforeEach(async () => { + workingDir = await mkdtemp(join(tmpdir(), 'codeman-workspace-hooks-')); + app = Fastify({ logger: false }); + await app.register(fastifyCookie); + registerSessionRoutes(app, createMockRouteContext()); + installRouteErrorHandler(app); + await app.ready(); + }); + + afterEach(async () => { + await app.close(); + await rm(workingDir, { recursive: true, force: true }); + }); + + it('installs hooks in a workspace that has none (the linked-case bug)', async () => { + const res = await createSession({ name: 'hooks-fresh', mode: 'claude', workingDir }); + expect(res.statusCode).toBe(200); + + const settings = await readSettings(); + const matchers = (settings.hooks?.Notification ?? []).map((entry) => entry.matcher); + // permission_prompt is the one an AskUserQuestion dialog raises; the + // elicitation pair is what CLOSES the resulting Approvals Inbox item. + expect(matchers).toEqual( + expect.arrayContaining([ + 'idle_prompt', + 'permission_prompt', + 'elicitation_dialog', + 'elicitation_complete', + 'elicitation_response', + ]) + ); + expect(settings.hooks?.Stop?.length).toBeGreaterThan(0); + + const serialized = JSON.stringify(settings.hooks); + // The two shapes that have historically shipped dead hooks: no secret header + // (401 once the gate went unconditional) and no -k (exit 60 on HTTPS installs). + expect(serialized).toContain('X-Codeman-Hook-Secret'); + expect(serialized).toContain('curl -sk -X POST'); + }); + + it('merges into a user-owned settings file without disturbing it', async () => { + await mkdir(join(workingDir, '.claude'), { recursive: true }); + const userHook = { matcher: 'Write', hooks: [{ type: 'command', command: './my-formatter.sh' }] }; + await writeFile( + settingsPath(), + JSON.stringify({ model: 'opus[1m]', permissions: { allow: ['Read'] }, hooks: { PostToolUse: [userHook] } }) + ); + + expect((await createSession({ name: 'hooks-merge', mode: 'claude', workingDir })).statusCode).toBe(200); + + const settings = await readSettings(); + expect(settings.model).toBe('opus[1m]'); + expect(settings.permissions).toEqual({ allow: ['Read'] }); + expect(JSON.stringify(settings.hooks)).toContain('./my-formatter.sh'); + expect((settings.hooks?.Notification ?? []).length).toBeGreaterThan(0); + }); + + it('leaves a non-claude session alone (only claude reads .claude hooks)', async () => { + expect((await createSession({ name: 'hooks-shell', mode: 'shell', workingDir })).statusCode).toBe(200); + expect(existsSync(settingsPath())).toBe(false); + }); + + it('leaves a malformed settings file untouched rather than replacing it', async () => { + await mkdir(join(workingDir, '.claude'), { recursive: true }); + await writeFile(settingsPath(), '{ not json'); + + expect((await createSession({ name: 'hooks-malformed', mode: 'claude', workingDir })).statusCode).toBe(200); + expect(await readFile(settingsPath(), 'utf-8')).toBe('{ not json'); + }); +});