From 0bbf483a35b1589f7b9ce22908abd5ed89ac8260 Mon Sep 17 00:00:00 2001 From: timkjr Date: Mon, 9 Nov 2026 11:14:18 -0600 Subject: [PATCH 1/2] fix(omp): report tool approvals to Codeman so an omp pane alerts An omp pane asking for tool approval raised no tab alert, no desktop notification and no Approvals Inbox item. omp inherited hooks:'none' from agentDefaults(), so hooksAvailableForMode() answered false and hook-event-routes never opened an item; there was nothing to resolve either. omp is one of the few non-claude CLIs with real approval events (tool_approval_requested / tool_approval_resolved), so Codeman can install a hook module into /.claude/hooks/pre/ - the directory omp scans - on session create. It forwards them as permission_prompt and elicitation_complete, the names the server already routes on, so the existing alert, notification and inbox pipeline works unchanged. The file name comes from the new capabilities.approvalHookFile field rather than a branch on the CLI id (cli-registry-no-id-branching). stop/idle_prompt are deliberately NOT reported: omp's idle stays screen-read via workDetect, which the agent skill's verbs.md now says explicitly. --- .../codeman/skills/codeman/reference/verbs.md | 7 +- skills/codeman/reference/verbs.md | 7 +- src/config/cli-registry/schema.ts | 8 + src/config/cli-registry/stock.ts | 15 ++ src/config/cli-registry/types.ts | 18 ++- src/hooks-config.ts | 121 +++++++++++++++ src/web/routes/session-routes.ts | 13 ++ test/cli-capability-predicates.test.ts | 2 +- test/omp-approval-hook.test.ts | 138 ++++++++++++++++++ 9 files changed, 325 insertions(+), 4 deletions(-) create mode 100644 test/omp-approval-hook.test.ts diff --git a/plugins/codeman/skills/codeman/reference/verbs.md b/plugins/codeman/skills/codeman/reference/verbs.md index fc656281..a6c1c531 100644 --- a/plugins/codeman/skills/codeman/reference/verbs.md +++ b/plugins/codeman/skills/codeman/reference/verbs.md @@ -383,12 +383,17 @@ recovered by submitting it with `{"input":"\r"}`. only when the workspace actually has them, see [§5.1](#51-where-to-spawn)) **and for `deepseek`** — the one external CLI that reports its own lifecycle, so its `stop` is a real end-of-turn signal rather than a guess. On -`shell`/`opencode`/`codex`/`gemini`/`antigravity`/`pi`/`grok`/`omp`/`copilot`, requesting them explicitly is a +`shell`/`opencode`/`codex`/`gemini`/`antigravity`/`pi`/`grok`/`copilot`, requesting them explicitly is a 400 (except `blocked` on `codex`, which Codeman reads off its screen when an approval, question or trust dialog is up; a wait for it answers at once if one already is), and lifecycle transitions there are coarse (a short shell command may emit **no** `idle` transition at all, verified live), so synchronize those with markers. +`omp` sits in between: its approval dialogs DO reach Codeman (Codeman installs a hook into +the workspace that reports them, so the tab alert, notification and Approvals Inbox all +work), but `stop`/`idle_prompt` are still read off its screen — an explicit wait for either +is still a 400. Treat an omp pane's end-of-turn as guessed, same as the list above. + ⚠️ A dsh session can still refuse them for a per-SESSION reason: `statusReporting: false` at create time disarms the bridge, and an explicit `until=stop` is then a 400 naming that setting. And a `stop` that is *accepted* is not proof it will ever fire — diff --git a/skills/codeman/reference/verbs.md b/skills/codeman/reference/verbs.md index fc656281..a6c1c531 100644 --- a/skills/codeman/reference/verbs.md +++ b/skills/codeman/reference/verbs.md @@ -383,12 +383,17 @@ recovered by submitting it with `{"input":"\r"}`. only when the workspace actually has them, see [§5.1](#51-where-to-spawn)) **and for `deepseek`** — the one external CLI that reports its own lifecycle, so its `stop` is a real end-of-turn signal rather than a guess. On -`shell`/`opencode`/`codex`/`gemini`/`antigravity`/`pi`/`grok`/`omp`/`copilot`, requesting them explicitly is a +`shell`/`opencode`/`codex`/`gemini`/`antigravity`/`pi`/`grok`/`copilot`, requesting them explicitly is a 400 (except `blocked` on `codex`, which Codeman reads off its screen when an approval, question or trust dialog is up; a wait for it answers at once if one already is), and lifecycle transitions there are coarse (a short shell command may emit **no** `idle` transition at all, verified live), so synchronize those with markers. +`omp` sits in between: its approval dialogs DO reach Codeman (Codeman installs a hook into +the workspace that reports them, so the tab alert, notification and Approvals Inbox all +work), but `stop`/`idle_prompt` are still read off its screen — an explicit wait for either +is still a 400. Treat an omp pane's end-of-turn as guessed, same as the list above. + ⚠️ A dsh session can still refuse them for a per-SESSION reason: `statusReporting: false` at create time disarms the bridge, and an explicit `until=stop` is then a 400 naming that setting. And a `stop` that is *accepted* is not proof it will ever fire — diff --git a/src/config/cli-registry/schema.ts b/src/config/cli-registry/schema.ts index ff3f40d5..c4673274 100644 --- a/src/config/cli-registry/schema.ts +++ b/src/config/cli-registry/schema.ts @@ -325,6 +325,14 @@ const capabilitiesSchema = z // A closed list on purpose: it decides where the server WRITES, and this file is // user-editable, so it can never be a free path (`..`, absolute). agentSkillDir: z.enum(AGENT_SKILL_DIRS).optional(), + // The hook module Codeman installs for a CLI whose hooks are files, not a settings + // block (omp). Same closed-list reasoning as `agentSkillDir`, stricter still: this + // name is written into the user's workspace, so a value like `../../evil.ts` must be + // refused by the schema rather than by whatever calls the writer. + approvalHookFile: z + .string() + .regex(/^[A-Za-z0-9._-]+\.ts$/) + .optional(), statusLineTelemetry: z.boolean(), // How many columns this CLI indents its transcript body by, so a copy can take // that much off the clipboard. Bounded, because it is the whole strip: a copy diff --git a/src/config/cli-registry/stock.ts b/src/config/cli-registry/stock.ts index c1634144..657d97e2 100644 --- a/src/config/cli-registry/stock.ts +++ b/src/config/cli-registry/stock.ts @@ -1742,6 +1742,21 @@ const OMP: CliEntry = { capabilities: { ...agentDefaults(), ...AGENTS_DIR_SKILL, + // omp emits REAL approval events (`tool_approval_requested` / `tool_approval_resolved`), + // so an omp pane can report a pending dialog first-hand instead of by inference. + // Codeman installs the hook module named by `approvalHookFile` into + // `/.claude/hooks/pre/` when a session is created; it forwards those two + // events to /api/hook-event as `permission_prompt` / `elicitation_complete`, the same + // names claude's settings block sends, so the tab alert, the desktop notification and + // the Approvals Inbox work unchanged. + // ⚠️ 'always' rather than 'supervised': the report comes from a hook Codeman owns and + // installs (like claude's), not from a supervised-CLI status bridge. + // ⚠️ Only approvals are reported. `stop`/`idle_prompt` still come from `workDetect` + // below, which stays the source of truth for whether an omp pane is working or idle. + hooks: 'always', + // The hook module Codeman writes for this CLI. Declared as data so the session-create + // path asks "does this CLI declare a hook file?" instead of branching on an id. + approvalHookFile: 'codeman-approval.ts', // Fullscreen alt-screen TUI, same shape as antigravity/grok: only the // tmux-attach-time smcup strip, not Ink's full erase-scrollback+DECSET strip. altScreen: 'strip-mux-only', diff --git a/src/config/cli-registry/types.ts b/src/config/cli-registry/types.ts index 094242ff..be9361a2 100644 --- a/src/config/cli-registry/types.ts +++ b/src/config/cli-registry/types.ts @@ -509,7 +509,9 @@ export interface CliCapabilities { * * ⚠️ A TRI-STATE, not a boolean, because for one CLI this is a per-SESSION question: * 'none' — no hook signals, ever (every external CLI, and `shell`). - * 'always' — the CLI installs Codeman's hooks (claude). + * 'always' — the CLI's signals are installed by Codeman and are unconditional: + * claude via a `.claude/settings.local.json` block, omp via the hook + * MODULE named by `approvalHookFile` below. * 'supervised' — the CLI REPORTS its own idle/working/blocked state to a supervisor * over a generic env-gated contract, and Codeman is that supervisor * (deepseek, via deepseek-status-shim.ts). Definitive rather than @@ -523,6 +525,20 @@ export interface CliCapabilities { * timeout the predicate exists to prevent. */ hooks: 'none' | 'always' | 'supervised'; + /** + * A CLI whose hooks are MODULES rather than a settings-file block names the file + * Codeman installs into its project hook directory. + * + * omp is the case that exists today: it loads `/.claude/hooks/pre/*.ts`, and the + * installed module forwards `tool_approval_requested`/`tool_approval_resolved` to + * `/api/hook-event` as `permission_prompt`/`elicitation_complete`, so an omp pane asking + * for approval raises the same tab alert, notification and Approvals Inbox item a claude + * pane does. Absent with `hooks: 'always'` means Claude's settings-file block. + * + * Declared as data, not a branch on the id, so the session-create path can ask + * "does this CLI need a hook file installed?" (see cli-registry-no-id-branching). + */ + approvalHookFile?: string; /** * Which transcript reader, if any, understands this CLI's on-disk history. * diff --git a/src/hooks-config.ts b/src/hooks-config.ts index 9708b5f1..1c59133a 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -1669,6 +1669,127 @@ export async function removeAgentSkillFrom(skillDir: string): Promise/.claude/hooks/pre` + * and `/.claude/hooks/post` (`projectDir` for the `claude` persona) plus a + * user-level `/hooks`, and loads each `.ts`/`.js` file's default export as + * `(pi: HookAPI) => void`. A project-level `pre` hook is deliberately the target: it + * travels with the workspace the session actually runs in, so no per-machine or + * per-user install is needed and removing the workspace removes the hook. + */ +export const OMP_APPROVAL_HOOK_DIR = '.claude/hooks/pre'; +export const OMP_APPROVAL_HOOK_FILE = 'codeman-approval.ts'; + +/** + * Source of the hook that turns omp's own approval events into Codeman hook events. + * + * WHY this exists: omp declares `capabilities.hooks: 'always'` (stock registry) but + * installs no Claude-style `settings.local.json` hook block — its hooks are modules. + * Without this file `hooksAvailableForMode()` is true while nothing ever POSTs, so + * `/api/approvals` stays empty, the tab never goes red and no notification fires: an + * omp pane asking for tool approval is silent. Claude solves the same problem by + * writing a `Notification: permission_prompt` block into `settings.local.json`. + * + * The mapping mirrors Claude's names exactly, because the server routes on them + * (`APPROVAL_KIND_BY_EVENT` in web/routes/hook-event-routes.ts): + * - `tool_approval_requested` -> `permission_prompt` (opens the inbox item) + * - `tool_approval_resolved` -> `elicitation_complete` (resolves it, whether the user + * approved or denied, so a denied dialog does not leave a red tab forever) + * + * ⚠️ Fails OPEN and SILENT, like the memory-gate hook: a hook that throws would break + * the approval flow it exists to report. Every path catches, and nothing is awaited + * unbounded — the fetch is fire-and-forget with a short AbortSignal. + */ +export function generateOmpApprovalHook(): string { + return `// ${OMP_APPROVAL_HOOK_MARKER} — auto-generated by Codeman; safe to delete, rewritten when a session is created. +// +// Reports omp's tool-approval dialogs to Codeman's /api/hook-event so an omp pane +// raises a tab alert and a desktop notification exactly like a claude pane does. +// Codeman owns this file: a foreign file of the same name is never overwritten. +import { readFile } from "node:fs/promises"; +import type { HookAPI } from "@oh-my-pi/pi-coding-agent"; + +const API_URL = process.env.CODEMAN_API_URL; +const SESSION_ID = process.env.CODEMAN_SESSION_ID; +const SECRET_FILE = process.env.CODEMAN_HOOK_SECRET_FILE; + +/** POST one Codeman hook event. Never throws, never awaited past its timeout. */ +async function report(event: string, data: Record): Promise { + if (!API_URL || !SESSION_ID) return; + try { + const secret = SECRET_FILE ? (await readFile(SECRET_FILE, "utf8")).trim() : ""; + await fetch(\`\${API_URL}/api/hook-event\`, { + method: "POST", + headers: { "Content-Type": "application/json", "X-Codeman-Hook-Secret": secret }, + body: JSON.stringify({ event, sessionId: SESSION_ID, data }), + signal: AbortSignal.timeout(5000), + }); + } catch { + // Best effort by contract: a failed report must never disturb the dialog itself. + } +} + +export default function codemanApprovalHook(pi: HookAPI): void { + pi.on("tool_approval_requested", (event) => { + void report("permission_prompt", { + tool_name: event.toolName, + tool_input: event.reason ? { description: event.reason } : undefined, + }); + }); + + pi.on("tool_approval_resolved", (event) => { + void report("elicitation_complete", { + tool_name: event.toolName, + approved: event.approved, + }); + }); +} +`; +} + +export type ApprovalHookApplyResult = 'installed' | 'refreshed' | 'unchanged' | 'foreign' | 'symlink'; + +/** + * Install or refresh the omp approval hook in `/.claude/hooks/pre/`. + * + * Ownership: the file carries `OMP_APPROVAL_HOOK_MARKER` in its first line. A file + * WITHOUT the marker is somebody else's hook and is left untouched (`foreign`) — the + * same posture `installAgentSkillInto` takes, so Codeman never clobbers user work. + * A symlinked directory is refused rather than written through. + * + * Idempotent: an unchanged file is not rewritten (`unchanged`), so this is safe to call + * on every omp session create. + */ +export async function applyApprovalHookFile(workspace: string, fileName: string): Promise { + const hookDir = join(workspace, OMP_APPROVAL_HOOK_DIR); + const target = join(hookDir, fileName); + return withSettingsLock(target, async () => { + if ((await isSymlink(hookDir)) || (await isSymlink(join(workspace, '.claude')))) return 'symlink'; + + let existing: string | null = null; + try { + existing = await readFile(target, 'utf-8'); + } catch { + // absent: fresh install + } + if (existing !== null && !existing.includes(OMP_APPROVAL_HOOK_MARKER)) return 'foreign'; + + const content = generateOmpApprovalHook(); + if (existing === content) return 'unchanged'; + + await mkdir(hookDir, { recursive: true }); + await writeSkillFileAtomic(target, content); + return existing === null ? 'installed' : 'refreshed'; + }); +} + /** * Claude Code's project skills directory, and the meaning `agentSkillInjection: true` * had before `capabilities.agentSkillDir` existed. diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index bef9e927..5f2bb8cc 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -156,6 +156,7 @@ import { seedAgentSessionPreamble, applyWorkspaceHooks, refreshStaleCodemanHooks, + applyApprovalHookFile, } from '../../hooks-config.js'; import { generateClaudeMd } from '../../templates/claude-md.js'; import { imageWatcher } from '../../image-watcher.js'; @@ -1331,6 +1332,18 @@ export function registerSessionRoutes( await applyWorkspaceHooks(workingDir, await ctx.getWorkspaceHooksEnabled()); } + // A CLI whose hooks are MODULES rather than a settings.local.json block (omp) needs + // its hook file installed into the workspace, so its approval dialogs raise the same + // alert a claude pane does. Registry-driven: `approvalHookFile` names the file, so no + // call site branches on a CLI id. Same local-workspace and `workspaceHooksEnabled` + // gate as claude's block; best-effort, because a failure here must not block a create. + const approvalHookFile = getCli(body.mode ?? 'claude')?.capabilities.approvalHookFile; + if (!remote && body.workingDir && approvalHookFile) { + if (await ctx.getWorkspaceHooksEnabled()) { + await applyApprovalHookFile(workingDir, approvalHookFile).catch(() => {}); + } + } + // Refuse up front if the requested CLI cannot start, rather than spawning a pane that // dies on `command not found`. The message comes from the resolver, so it names where // resolution actually looked (server PATH, login shell, the entry's search dirs); a diff --git a/test/cli-capability-predicates.test.ts b/test/cli-capability-predicates.test.ts index 0302ef3a..f78276e3 100644 --- a/test/cli-capability-predicates.test.ts +++ b/test/cli-capability-predicates.test.ts @@ -35,7 +35,7 @@ describe('per-mode capability predicates', () => { ['pi', true, false, false], ['grok', true, false, false], ['deepseek', true, false, true], - ['omp', true, false, false], + ['omp', true, false, true], ['copilot', true, false, false], ] as Array<[SessionMode, boolean, boolean, boolean]>)( '%s: external=%s altScreenStrip=%s hooks=%s', diff --git a/test/omp-approval-hook.test.ts b/test/omp-approval-hook.test.ts new file mode 100644 index 00000000..a6595540 --- /dev/null +++ b/test/omp-approval-hook.test.ts @@ -0,0 +1,138 @@ +/** + * @fileoverview omp approval-hook wiring. + * + * Pins the three things that make an omp pane able to report a pending tool approval + * at all: the registry says omp HAS hooks, Codeman installs the module that emits them, + * and the module maps omp's events onto the names the server already routes on. + * + * Without these, `hooksAvailableForMode('omp')` is false, /api/approvals never receives + * an item, and a dialog on screen raises no tab alert and no notification. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, mkdirSync, readFileSync, rmSync, symlinkSync, writeFileSync, existsSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + applyApprovalHookFile, + generateOmpApprovalHook, + OMP_APPROVAL_HOOK_DIR, + OMP_APPROVAL_HOOK_FILE, +} from '../src/hooks-config.js'; +import { getCli } from '../src/config/cli-registry/registry.js'; +import { hooksAvailableForMode } from '../src/web/session-wait-registry.js'; + +const MARKER = 'CODEMAN_OMP_APPROVAL_HOOK_V1'; + +describe('omp approval hook — registry', () => { + it('declares hooks: always, which is what lets approval items be opened at all', () => { + expect(getCli('omp')?.capabilities.hooks).toBe('always'); + expect(hooksAvailableForMode('omp')).toBe(true); + }); + + it('leaves every other hook-less CLI failing the ask', () => { + // The rule that keeps this honest: 'always' is earned by a real signal, not by + // wanting alerts. opencode/gemini/pi/grok/antigravity have no approval events. + for (const mode of ['opencode', 'gemini', 'pi', 'grok', 'antigravity', 'shell'] as const) { + expect(hooksAvailableForMode(mode), mode).toBe(false); + } + }); + + it('names the hook module as registry data, so no call site branches on the id', () => { + // The session-create path asks "does this CLI declare approvalHookFile?"; if the + // field is absent the hook is never installed and the pane stays silent. + expect(getCli('omp')?.capabilities.approvalHookFile).toBe(OMP_APPROVAL_HOOK_FILE); + // claude installs a settings.local.json block instead and must NOT be given a file. + expect(getCli('claude')?.capabilities.approvalHookFile).toBeUndefined(); + }); +}); + +describe('omp approval hook — generated module', () => { + const src = generateOmpApprovalHook(); + + it('carries the ownership marker on its first line', () => { + expect(src.split('\n')[0]).toContain(MARKER); + }); + + it('maps omp approval events onto the names the server routes on', () => { + // APPROVAL_KIND_BY_EVENT opens an item for permission_prompt and resolves it on + // elicitation_complete; using any other name makes the hook a silent no-op. + expect(src).toContain('"tool_approval_requested"'); + expect(src).toContain('"tool_approval_resolved"'); + expect(src).toContain('"permission_prompt"'); + expect(src).toContain('"elicitation_complete"'); + }); + + it('posts to the hook-event route with the secret header', () => { + expect(src).toContain('/api/hook-event'); + expect(src).toContain('X-Codeman-Hook-Secret'); + expect(src).toContain('CODEMAN_SESSION_ID'); + expect(src).toContain('CODEMAN_API_URL'); + }); + + it('fails open: both handlers catch and nothing is awaited unbounded', () => { + expect(src).toContain('catch'); + expect(src).toContain('AbortSignal.timeout'); + // `void report(...)` — a rejected report must never surface to the dialog. + expect(src).toMatch(/void report\(/); + }); + + it('uses a static import, so the module parses without a dynamic loader', () => { + expect(src).toContain('import { readFile } from "node:fs/promises";'); + expect(src).not.toContain('await import('); + }); +}); + +describe('omp approval hook — install', () => { + let workspace: string; + + beforeEach(() => { + workspace = mkdtempSync(join(tmpdir(), 'codeman-omp-hook-')); + }); + + afterEach(() => { + rmSync(workspace, { recursive: true, force: true }); + }); + + it('installs into .claude/hooks/pre, where omp scans for project hooks', async () => { + expect(await applyApprovalHookFile(workspace, OMP_APPROVAL_HOOK_FILE)).toBe('installed'); + expect(existsSync(join(workspace, OMP_APPROVAL_HOOK_DIR, OMP_APPROVAL_HOOK_FILE))).toBe(true); + expect(readFileSync(join(workspace, OMP_APPROVAL_HOOK_DIR, OMP_APPROVAL_HOOK_FILE), 'utf-8')).toBe( + generateOmpApprovalHook() + ); + }); + + it('is idempotent — a second call rewrites nothing', async () => { + await applyApprovalHookFile(workspace, OMP_APPROVAL_HOOK_FILE); + expect(await applyApprovalHookFile(workspace, OMP_APPROVAL_HOOK_FILE)).toBe('unchanged'); + }); + + it('refreshes a stale Codeman copy', async () => { + await applyApprovalHookFile(workspace, OMP_APPROVAL_HOOK_FILE); + writeFileSync(join(workspace, OMP_APPROVAL_HOOK_DIR, OMP_APPROVAL_HOOK_FILE), `// ${MARKER}\n// stale old shape\n`); + expect(await applyApprovalHookFile(workspace, OMP_APPROVAL_HOOK_FILE)).toBe('refreshed'); + expect(readFileSync(join(workspace, OMP_APPROVAL_HOOK_DIR, OMP_APPROVAL_HOOK_FILE), 'utf-8')).toBe( + generateOmpApprovalHook() + ); + }); + + it("never overwrites another project's hook of the same name", async () => { + mkdirSync(join(workspace, OMP_APPROVAL_HOOK_DIR), { recursive: true }); + const foreign = 'export default function mine() {}\n'; + writeFileSync(join(workspace, OMP_APPROVAL_HOOK_DIR, OMP_APPROVAL_HOOK_FILE), foreign); + expect(await applyApprovalHookFile(workspace, OMP_APPROVAL_HOOK_FILE)).toBe('foreign'); + expect(readFileSync(join(workspace, OMP_APPROVAL_HOOK_DIR, OMP_APPROVAL_HOOK_FILE), 'utf-8')).toBe(foreign); + }); + + it('refuses to write through a symlinked hooks directory', async () => { + const elsewhere = mkdtempSync(join(tmpdir(), 'codeman-omp-hook-target-')); + try { + mkdirSync(join(workspace, '.claude', 'hooks'), { recursive: true }); + symlinkSync(elsewhere, join(workspace, OMP_APPROVAL_HOOK_DIR)); + expect(await applyApprovalHookFile(workspace, OMP_APPROVAL_HOOK_FILE)).toBe('symlink'); + expect(existsSync(join(elsewhere, OMP_APPROVAL_HOOK_FILE))).toBe(false); + } finally { + rmSync(elsewhere, { recursive: true, force: true }); + } + }); +}); From 27d6e2e30a9f80823274e594feedcdca93c8be3b Mon Sep 17 00:00:00 2001 From: timkjr Date: Tue, 10 Nov 2026 09:59:22 -0600 Subject: [PATCH 2/2] fix(omp): report approvals without claiming Claude's hook set Review follow-up. omp declared `hooks: 'always'`, which every consumer reads as Claude's full hook set, so an omp session picked up behaviour it cannot back. omp goes back to `hooks: 'none'`; `capabilities.approvalHookFile` is now a separate, narrower capability with its own predicate, `approvalHookAvailableForMode()` (local, non-adopted sessions only). - An explicit `stop` wait on omp is a 400 again; `blocked` still works. - omp inbox items are alert-only (`answerInTerminal`, no `approvalId` on the push), and only `permission_prompt` / `elicitation_complete` are accepted for an approval-only session. - The module is installed from POST /api/quick-start too (new and existing cases, not Docker or remote) and on reboot-restore, after the launch pre-flight on both create paths. Claude's settings.local.json block and the Docker CLAUDE.md no longer land in omp workspaces. - The generated module passes Bun's `tls: { rejectUnauthorized: false }` for an https CODEMAN_API_URL (checked against a `--https` server), clears the alert only when the last pending toolCallId resolves, sends reports in order, carries omp's policy `reason` as `message`, and imports HookAPI from `@oh-my-pi/pi-coding-agent/extensibility/hooks`. - Every path component and the target file are lstat-checked. - docs/omp-integration.md, verbs.md and the stock.ts comments updated. Co-Authored-By: Claude Opus 5.5 --- docs/omp-integration.md | 21 +- .../codeman/skills/codeman/reference/verbs.md | 9 +- skills/codeman/reference/verbs.md | 9 +- src/config/cli-registry/stock.ts | 31 +- src/config/cli-registry/types.ts | 22 +- src/hooks-config.ts | 76 +++-- src/web/routes/hook-event-routes.ts | 45 ++- src/web/routes/reboot-restore-routes.ts | 18 +- src/web/routes/session-routes.ts | 39 ++- src/web/session-wait-registry.ts | 33 ++- test/cli-capability-predicates.test.ts | 2 +- test/omp-approval-hook.test.ts | 264 +++++++++++++++--- test/routes/approval-routes.test.ts | 80 ++++++ test/routes/omp-approval-hook-routes.test.ts | 177 ++++++++++++ 14 files changed, 701 insertions(+), 125 deletions(-) create mode 100644 test/routes/omp-approval-hook-routes.test.ts diff --git a/docs/omp-integration.md b/docs/omp-integration.md index bb5dea7b..80bf4bc7 100644 --- a/docs/omp-integration.md +++ b/docs/omp-integration.md @@ -55,6 +55,19 @@ documented default `tools.approvalMode` is `yolo`, so an omp pane auto-approves with no flag from Codeman — the CLI's own config, not Codeman, is what would need to change that. +**Approval alerts.** When a session is created (`POST /api/sessions` or +`POST /api/quick-start`, local workspaces only, and only while `workspaceHooksEnabled` +is on), Codeman writes a hook module, `.claude/hooks/pre/codeman-approval.ts`, into the +workspace; omp loads project hooks from that folder. It forwards omp's +`tool_approval_requested` / `tool_approval_resolved` events to `/api/hook-event`, so a +pending approval raises a tab alert, a notification and an Approvals Inbox item. The +item is **alert-only**: answer the dialog in the omp pane (omp's composer keeps taking +typing while the prompt is docked, so Codeman never types an answer for you). The +registry declares this as `capabilities.approvalHookFile`, separate from `hooks`, which +stays `'none'`: the `blocked` wait signal works for omp, `stop` is still a 400. ⚠️ With +omp's default `tools.approvalMode: yolo` omp never asks, so the module only does +anything once that setting is changed. + Env overrides: the `OMP_*` prefix is allowlisted, and per omp's own `docs/environment-variables.md` it is not the narrow surface it looks like. omp reads roughly 40 provider keys from the environment (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, @@ -163,13 +176,13 @@ remote user's home, so resolving locally would pin a stranger's id. See ## Known gaps -- **No idle/completion hook.** Idle detection reads the screen instead: the +- **No idle/completion hook.** The approval hook module (see Approval alerts + above) reports approvals only, so idle detection reads the screen instead: the registry entry's `workDetect` names omp's `╰─` input row as the glyph that arms the idle check, and the status bar's spinner plus elapsed time (` ⠼ 14s > ⬢ …`) or the `⎋ Working…` row as the working line, measured on omp 18.8.6 and 18.0.11. - Without it an omp session that had started a turn never left `busy`. If omp ever - ships a hooks system, a Codeman hook POSTing to `/api/hook-event` would still be - the highest-value follow-up. + Without it an omp session that had started a turn never left `busy`. Reporting + end-of-turn from the same module would be the follow-up that earns omp `stop`. - **Killing a pane mid-turn loses the conversation for real.** `tmux kill-session` before an in-TUI `/exit` beats omp's own session-file flush — confirmed by direct testing (kill after a clean `/exit` resumes correctly; kill without `/exit` first diff --git a/plugins/codeman/skills/codeman/reference/verbs.md b/plugins/codeman/skills/codeman/reference/verbs.md index a6c1c531..9b229bcb 100644 --- a/plugins/codeman/skills/codeman/reference/verbs.md +++ b/plugins/codeman/skills/codeman/reference/verbs.md @@ -383,16 +383,15 @@ recovered by submitting it with `{"input":"\r"}`. only when the workspace actually has them, see [§5.1](#51-where-to-spawn)) **and for `deepseek`** — the one external CLI that reports its own lifecycle, so its `stop` is a real end-of-turn signal rather than a guess. On -`shell`/`opencode`/`codex`/`gemini`/`antigravity`/`pi`/`grok`/`copilot`, requesting them explicitly is a +`shell`/`opencode`/`codex`/`gemini`/`antigravity`/`pi`/`grok`/`omp`/`copilot`, requesting them explicitly is a 400 (except `blocked` on `codex`, which Codeman reads off its screen when an approval, question or trust dialog is up; a wait for it answers at once if one already is), and lifecycle transitions there are coarse (a short shell command may emit **no** `idle` transition at all, verified live), so synchronize those with markers. -`omp` sits in between: its approval dialogs DO reach Codeman (Codeman installs a hook into -the workspace that reports them, so the tab alert, notification and Approvals Inbox all -work), but `stop`/`idle_prompt` are still read off its screen — an explicit wait for either -is still a 400. Treat an omp pane's end-of-turn as guessed, same as the list above. +`blocked` also works on a local `omp` session: Codeman installs a hook module into its +workspace that reports omp's approval dialogs (the inbox item is alert-only, so answer in +the pane). `stop` stays a 400 there, so wait on `idle` or a marker for end-of-turn. ⚠️ A dsh session can still refuse them for a per-SESSION reason: `statusReporting: false` at create time disarms the bridge, and an explicit `until=stop` is then a 400 diff --git a/skills/codeman/reference/verbs.md b/skills/codeman/reference/verbs.md index a6c1c531..9b229bcb 100644 --- a/skills/codeman/reference/verbs.md +++ b/skills/codeman/reference/verbs.md @@ -383,16 +383,15 @@ recovered by submitting it with `{"input":"\r"}`. only when the workspace actually has them, see [§5.1](#51-where-to-spawn)) **and for `deepseek`** — the one external CLI that reports its own lifecycle, so its `stop` is a real end-of-turn signal rather than a guess. On -`shell`/`opencode`/`codex`/`gemini`/`antigravity`/`pi`/`grok`/`copilot`, requesting them explicitly is a +`shell`/`opencode`/`codex`/`gemini`/`antigravity`/`pi`/`grok`/`omp`/`copilot`, requesting them explicitly is a 400 (except `blocked` on `codex`, which Codeman reads off its screen when an approval, question or trust dialog is up; a wait for it answers at once if one already is), and lifecycle transitions there are coarse (a short shell command may emit **no** `idle` transition at all, verified live), so synchronize those with markers. -`omp` sits in between: its approval dialogs DO reach Codeman (Codeman installs a hook into -the workspace that reports them, so the tab alert, notification and Approvals Inbox all -work), but `stop`/`idle_prompt` are still read off its screen — an explicit wait for either -is still a 400. Treat an omp pane's end-of-turn as guessed, same as the list above. +`blocked` also works on a local `omp` session: Codeman installs a hook module into its +workspace that reports omp's approval dialogs (the inbox item is alert-only, so answer in +the pane). `stop` stays a 400 there, so wait on `idle` or a marker for end-of-turn. ⚠️ A dsh session can still refuse them for a per-SESSION reason: `statusReporting: false` at create time disarms the bridge, and an explicit `until=stop` is then a 400 diff --git a/src/config/cli-registry/stock.ts b/src/config/cli-registry/stock.ts index 657d97e2..a3703d78 100644 --- a/src/config/cli-registry/stock.ts +++ b/src/config/cli-registry/stock.ts @@ -1661,9 +1661,11 @@ const DEEPSEEK: CliEntry = { }; // OMP (`omp`, omp.sh). The plainest entry in the catalog after opencode: no permission -// flags at all — omp reads its model routing and hooks from `~/.omp/agent`, so the CLI's -// own config governs and there is deliberately nothing bypass-shaped to clamp. Its only -// privileged surface is a pair of ENV keys (see privilegedEnvKeys below). +// flags at all — omp reads its model routing, approval mode and user hooks from +// `~/.omp/agent`, so the CLI's own config governs and there is deliberately nothing +// bypass-shaped to clamp. The one thing Codeman adds is a project hook module that +// reports approval dialogs (`approvalHookFile` below). Its only privileged surface is a +// pair of ENV keys (see privilegedEnvKeys below). const OMP: CliEntry = { id: 'omp' as CliEntry['id'], label: 'OMP', @@ -1745,17 +1747,15 @@ const OMP: CliEntry = { // omp emits REAL approval events (`tool_approval_requested` / `tool_approval_resolved`), // so an omp pane can report a pending dialog first-hand instead of by inference. // Codeman installs the hook module named by `approvalHookFile` into - // `/.claude/hooks/pre/` when a session is created; it forwards those two - // events to /api/hook-event as `permission_prompt` / `elicitation_complete`, the same - // names claude's settings block sends, so the tab alert, the desktop notification and - // the Approvals Inbox work unchanged. - // ⚠️ 'always' rather than 'supervised': the report comes from a hook Codeman owns and - // installs (like claude's), not from a supervised-CLI status bridge. - // ⚠️ Only approvals are reported. `stop`/`idle_prompt` still come from `workDetect` - // below, which stays the source of truth for whether an omp pane is working or idle. - hooks: 'always', - // The hook module Codeman writes for this CLI. Declared as data so the session-create - // path asks "does this CLI declare a hook file?" instead of branching on an id. + // `/.claude/hooks/pre/` when a session is created (both create paths); it + // forwards those two events to /api/hook-event as `permission_prompt` / + // `elicitation_complete`, so the tab alert, the desktop notification and an ALERT-ONLY + // Approvals Inbox item (answer in the terminal) work. + // ⚠️ `hooks` stays 'none' (inherited from agentDefaults): that value means Claude's full + // hook set (`stop`, `idle_prompt`, answerable dialogs, `.claude/settings.local.json`), + // and omp has none of it. `approvalHookFile` is the separate, narrower capability; + // `approvalHookAvailableForMode()` is its predicate. Idle stays screen-read via + // `workDetect` below. approvalHookFile: 'codeman-approval.ts', // Fullscreen alt-screen TUI, same shape as antigravity/grok: only the // tmux-attach-time smcup strip, not Ink's full erase-scrollback+DECSET strip. @@ -1778,7 +1778,8 @@ const OMP: CliEntry = { promptGlyph: '╰─', workingLine: '[⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏] [0-9hms ]+> |⎋ Working', }, - // No permission prompts and no bypass flag, so nothing config-shaped to clamp — the + // No permission FLAGS (omp's approval mode lives in its own config, `tools.approvalMode`) + // and no bypass flag, so nothing config-shaped to clamp — the // whole privileged surface here is env-shaped. privilegedParams: [], // Where omp resolves its auth from. No known concrete exfiltration path today (omp diff --git a/src/config/cli-registry/types.ts b/src/config/cli-registry/types.ts index be9361a2..448449a3 100644 --- a/src/config/cli-registry/types.ts +++ b/src/config/cli-registry/types.ts @@ -508,10 +508,11 @@ export interface CliCapabilities { * Whether `stop`/`blocked` wait signals can ever fire for this CLI. * * ⚠️ A TRI-STATE, not a boolean, because for one CLI this is a per-SESSION question: - * 'none' — no hook signals, ever (every external CLI, and `shell`). - * 'always' — the CLI's signals are installed by Codeman and are unconditional: - * claude via a `.claude/settings.local.json` block, omp via the hook - * MODULE named by `approvalHookFile` below. + * 'none' — no hook signals, ever (every external CLI, and `shell`). A CLI can + * still report approval dialogs alone through `approvalHookFile` below. + * 'always' — the CLI installs Codeman's hooks (claude). Every consumer reads this + * as Claude's FULL hook set: `stop`, `idle_prompt`, answerable inbox + * items, and a `.claude/settings.local.json` block in the workspace. * 'supervised' — the CLI REPORTS its own idle/working/blocked state to a supervisor * over a generic env-gated contract, and Codeman is that supervisor * (deepseek, via deepseek-status-shim.ts). Definitive rather than @@ -526,16 +527,17 @@ export interface CliCapabilities { */ hooks: 'none' | 'always' | 'supervised'; /** - * A CLI whose hooks are MODULES rather than a settings-file block names the file - * Codeman installs into its project hook directory. + * The hook MODULE Codeman installs into the workspace of a CLI that reports its + * approval dialogs, and nothing else, through hooks. Independent of `hooks` above. * * omp is the case that exists today: it loads `/.claude/hooks/pre/*.ts`, and the * installed module forwards `tool_approval_requested`/`tool_approval_resolved` to - * `/api/hook-event` as `permission_prompt`/`elicitation_complete`, so an omp pane asking - * for approval raises the same tab alert, notification and Approvals Inbox item a claude - * pane does. Absent with `hooks: 'always'` means Claude's settings-file block. + * `/api/hook-event` as `permission_prompt`/`elicitation_complete`. What that earns + * (`approvalHookAvailableForMode()`): the `blocked` wait signal, a tab alert, a + * notification and an ALERT-ONLY inbox item (`answerInTerminal`). Never `stop`, never + * `idle_prompt`, never Approve/Deny keystrokes. * - * Declared as data, not a branch on the id, so the session-create path can ask + * Declared as data, not a branch on the id, so both create paths can ask * "does this CLI need a hook file installed?" (see cli-registry-no-id-branching). */ approvalHookFile?: string; diff --git a/src/hooks-config.ts b/src/hooks-config.ts index 1c59133a..ded5cba4 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -1690,38 +1690,56 @@ export const OMP_APPROVAL_HOOK_FILE = 'codeman-approval.ts'; /** * Source of the hook that turns omp's own approval events into Codeman hook events. * - * WHY this exists: omp declares `capabilities.hooks: 'always'` (stock registry) but - * installs no Claude-style `settings.local.json` hook block — its hooks are modules. - * Without this file `hooksAvailableForMode()` is true while nothing ever POSTs, so - * `/api/approvals` stays empty, the tab never goes red and no notification fires: an - * omp pane asking for tool approval is silent. Claude solves the same problem by - * writing a `Notification: permission_prompt` block into `settings.local.json`. + * WHY this exists: omp installs no Claude-style `settings.local.json` hook block — its + * hooks are modules — so without this file an omp pane asking for tool approval is + * silent: `/api/approvals` stays empty, the tab never goes red and no notification + * fires. The registry declares it through `capabilities.approvalHookFile`, NOT + * `hooks: 'always'` (which would also promise `stop` and answerable dialogs). * - * The mapping mirrors Claude's names exactly, because the server routes on them + * The mapping uses Claude's event names, because the server routes on them * (`APPROVAL_KIND_BY_EVENT` in web/routes/hook-event-routes.ts): - * - `tool_approval_requested` -> `permission_prompt` (opens the inbox item) + * - `tool_approval_requested` -> `permission_prompt` (opens an alert-only inbox item) * - `tool_approval_resolved` -> `elicitation_complete` (resolves it, whether the user * approved or denied, so a denied dialog does not leave a red tab forever) * + * Several calls in one batch can each wait on their own prompt, and the inbox keeps one + * item per session, so the module tracks pending `toolCallId`s and reports the close only + * when the last one resolves. That set drains because omp pairs every request with a + * resolve, an aborted prompt included (its approval wrapper emits `approved: false` from + * the catch, omp 18.18.6 `extensibility/extensions/wrapper.ts`); only omp dying mid-prompt + * skips it, which leaves the alert up exactly as it would without the set. Reports go out + * in order (one chain), so a fast approve cannot overtake the prompt that opened it. + * + * `reason` is omp's POLICY note (a restricted folder, a critical-pattern match), not the + * command, and is absent on an ordinary mode-raised prompt, so it rides `message`. + * + * HTTPS: `CODEMAN_API_URL` is loopback HTTPS with a self-signed certificate on `--https` + * and tailscale installs (Codeman's own hook curl passes `-k`, the DeepSeek shim sets + * `rejectUnauthorized: false`). omp runs on Bun, whose fetch verifies certificates by + * default, so an `https:` URL gets Bun's `tls: { rejectUnauthorized: false }`. + * * ⚠️ Fails OPEN and SILENT, like the memory-gate hook: a hook that throws would break * the approval flow it exists to report. Every path catches, and nothing is awaited - * unbounded — the fetch is fire-and-forget with a short AbortSignal. + * unbounded — each POST carries a short AbortSignal and the handlers never await it. */ export function generateOmpApprovalHook(): string { return `// ${OMP_APPROVAL_HOOK_MARKER} — auto-generated by Codeman; safe to delete, rewritten when a session is created. // // Reports omp's tool-approval dialogs to Codeman's /api/hook-event so an omp pane -// raises a tab alert and a desktop notification exactly like a claude pane does. +// raises a tab alert and a desktop notification. Answer the dialog in the terminal. // Codeman owns this file: a foreign file of the same name is never overwritten. import { readFile } from "node:fs/promises"; -import type { HookAPI } from "@oh-my-pi/pi-coding-agent"; +import type { HookAPI } from "@oh-my-pi/pi-coding-agent/extensibility/hooks"; const API_URL = process.env.CODEMAN_API_URL; const SESSION_ID = process.env.CODEMAN_SESSION_ID; const SECRET_FILE = process.env.CODEMAN_HOOK_SECRET_FILE; +// Loopback HTTPS (--https, tailscale) uses a self-signed certificate, which Bun's fetch +// refuses by default; Codeman's own hook curl passes -k for the same reason. +const TLS = API_URL?.startsWith("https:") ? { tls: { rejectUnauthorized: false } } : {}; -/** POST one Codeman hook event. Never throws, never awaited past its timeout. */ -async function report(event: string, data: Record): Promise { +/** POST one Codeman hook event. Never throws; bounded by its timeout. */ +async function send(event: string, data: Record): Promise { if (!API_URL || !SESSION_ID) return; try { const secret = SECRET_FILE ? (await readFile(SECRET_FILE, "utf8")).trim() : ""; @@ -1730,22 +1748,38 @@ async function report(event: string, data: Record): Promise = Promise.resolve(); +function report(event: string, data: Record): void { + chain = chain.then(() => send(event, data)); +} + export default function codemanApprovalHook(pi: HookAPI): void { + // Calls in one batch can each wait on their own prompt, and Codeman keeps one alert per + // session, so it is cleared only when the LAST pending approval resolves. + const pending = new Set(); + pi.on("tool_approval_requested", (event) => { - void report("permission_prompt", { + pending.add(event.toolCallId); + // \`reason\` is omp's policy note (restricted folder, critical pattern), not the command. + report("permission_prompt", { tool_name: event.toolName, - tool_input: event.reason ? { description: event.reason } : undefined, + ...(event.reason ? { message: event.reason } : {}), }); }); pi.on("tool_approval_resolved", (event) => { - void report("elicitation_complete", { + pending.delete(event.toolCallId); + if (pending.size > 0) return; + report("elicitation_complete", { tool_name: event.toolName, approved: event.approved, }); @@ -1762,7 +1796,9 @@ export type ApprovalHookApplyResult = 'installed' | 'refreshed' | 'unchanged' | * Ownership: the file carries `OMP_APPROVAL_HOOK_MARKER` in its first line. A file * WITHOUT the marker is somebody else's hook and is left untouched (`foreign`) — the * same posture `installAgentSkillInto` takes, so Codeman never clobbers user work. - * A symlinked directory is refused rather than written through. + * EVERY component below the workspace (`.claude`, `.claude/hooks`, `.claude/hooks/pre`) + * and the target file itself is `lstat`ed, and a symlink anywhere is refused rather than + * written through: a linked `.claude/hooks` once put `pre/` in the link's target. * * Idempotent: an unchanged file is not rewritten (`unchanged`), so this is safe to call * on every omp session create. @@ -1771,7 +1807,11 @@ export async function applyApprovalHookFile(workspace: string, fileName: string) const hookDir = join(workspace, OMP_APPROVAL_HOOK_DIR); const target = join(hookDir, fileName); return withSettingsLock(target, async () => { - if ((await isSymlink(hookDir)) || (await isSymlink(join(workspace, '.claude')))) return 'symlink'; + let component = workspace; + for (const part of [...OMP_APPROVAL_HOOK_DIR.split('/'), fileName]) { + component = join(component, part); + if (await isSymlink(component)) return 'symlink'; + } let existing: string | null = null; try { diff --git a/src/web/routes/hook-event-routes.ts b/src/web/routes/hook-event-routes.ts index e05d0448..7417a4db 100644 --- a/src/web/routes/hook-event-routes.ts +++ b/src/web/routes/hook-event-routes.ts @@ -13,11 +13,24 @@ import { HookEventSchema, isValidWorkingDir } from '../schemas.js'; import { sanitizeHookData, parseBody } from '../route-helpers.js'; import { persistDockerCaseClaudeSessionId } from '../../docker-hosts.js'; import { getDataDir } from '../../config/instance.js'; -import { sessionWaits, hooksAvailableForMode, sessionHookOptions } from '../session-wait-registry.js'; +import { + sessionWaits, + hooksAvailableForMode, + approvalHookAvailableForMode, + sessionHookOptions, +} from '../session-wait-registry.js'; import { getCli } from '../../config/cli-registry/registry.js'; import { approvalInbox, type ApprovalKind } from '../approval-inbox.js'; import type { SessionPort, EventPort, RespawnPort, ConfigPort, InfraPort } from '../ports/index.js'; +/** + * The only events an approval-only hook module (`capabilities.approvalHookFile`, omp) + * sends: a dialog opened, and a dialog closed. Anything else claimed for such a session + * (`stop`, `idle_prompt`) is a signal it can never legitimately emit, so it is dropped + * like any other mode-gated forgery. + */ +const APPROVAL_HOOK_EVENTS = new Set(['permission_prompt', 'elicitation_complete']); + /** Hook events that open an Approvals Inbox item. */ const APPROVAL_KIND_BY_EVENT: Record = { permission_prompt: 'permission', @@ -93,12 +106,21 @@ export function registerHookEventRoutes( // could never legitimately emit one is now dropped instead of steering another // agent's control flow. const waitSession = ctx.sessions.get(sessionId); + // An approval-only session (omp) reports dialogs and nothing else, so it reaches the + // `blocked` half of the gate and never the `stop` half. + const approvalOnly = + !!waitSession && + !hooksAvailableForMode(waitSession.mode, sessionHookOptions(waitSession)) && + approvalHookAvailableForMode(waitSession.mode, sessionHookOptions(waitSession)) && + APPROVAL_HOOK_EVENTS.has(event); if (waitSession && hooksAvailableForMode(waitSession.mode, sessionHookOptions(waitSession))) { if (event === 'stop') { sessionWaits.notifySignal(sessionId, 'stop'); } else if (event === 'permission_prompt' || event === 'elicitation_dialog') { sessionWaits.notifySignal(sessionId, 'blocked'); } + } else if (approvalOnly && event === 'permission_prompt') { + sessionWaits.notifySignal(sessionId, 'blocked'); } // Signal the respawn controller based on hook event type @@ -195,7 +217,7 @@ export function registerHookEventRoutes( // replacement would forget the item was unseen, or that a person had seen it). With no idle // item pending, the keep-alive's idle prompt opens a pre-acknowledged one. const keepsPendingIdle = keepAliveReason !== undefined && approvalInbox.getForSession(sessionId)?.kind === 'idle'; - if (session && hooksAvailableForMode(session.mode, sessionHookOptions(session))) { + if (session && (hooksAvailableForMode(session.mode, sessionHookOptions(session)) || approvalOnly)) { if (approvalKind === 'idle' && keepsPendingIdle) { // Left in place: no new item, and the broadcast below carries the keep-alive reason. } else if (approvalKind) { @@ -214,10 +236,12 @@ export function registerHookEventRoutes( toolSummary: typeof toolSummary === 'string' ? toolSummary : undefined, message: typeof safeData.message === 'string' ? safeData.message : undefined, cwd: typeof safeData.cwd === 'string' ? safeData.cwd : undefined, - // Supervised-hook CLIs (DeepSeek Harness): dialog keystrokes are unverifiable, - // so the item is an alert — answer in the terminal. Capability-gated, not by - // mode id, so a future supervised CLI inherits the same shape. - answerInTerminal: getCli(session.mode)?.capabilities.hooks === 'supervised', + // Supervised-hook CLIs (DeepSeek Harness) and approval-only hook modules (omp): + // dialog keystrokes are unverifiable (the inbox's re-capture parses Claude's + // numbered dialog only, and omp's composer keeps taking typing while its prompt + // is docked, so a `1` would land there), so the item is an alert — answer in + // the terminal. Capability-gated, not by mode id. + answerInTerminal: getCli(session.mode)?.capabilities.hooks === 'supervised' || approvalOnly, // What the pane says is still running in the background. An idle prompt from a // session that is watching its own work opens acknowledged, so it never arms an // alert nobody can answer; notePrompt() carries the whole reasoning. @@ -268,9 +292,12 @@ export function registerHookEventRoutes( sessionId, sessionName, ...safeData, - // Supervised-hook CLIs answer in the terminal: omit approvalId so push - // cannot offer Approve/Deny that the answer route would refuse. - ...(approvalId && session && getCli(session.mode)?.capabilities.hooks !== 'supervised' && { approvalId }), + // Supervised-hook CLIs and approval-only modules answer in the terminal: omit + // approvalId so push cannot offer Approve/Deny that the answer route would refuse. + ...(approvalId && + session && + getCli(session.mode)?.capabilities.hooks !== 'supervised' && + !approvalOnly && { approvalId }), }); } diff --git a/src/web/routes/reboot-restore-routes.ts b/src/web/routes/reboot-restore-routes.ts index 9cffe9f5..e9a2a663 100644 --- a/src/web/routes/reboot-restore-routes.ts +++ b/src/web/routes/reboot-restore-routes.ts @@ -42,7 +42,12 @@ import { clampEnvOverridesForOwner } from '../../session-env-clamp.js'; import { Session } from '../../session.js'; import { resolveClaudeModeForUsername } from '../../user-store.js'; import { getCli } from '../../config/cli-registry/registry.js'; -import { agentSkillDir, applyWorkspaceHooks, seedAgentSessionPreamble } from '../../hooks-config.js'; +import { + agentSkillDir, + applyApprovalHookFile, + applyWorkspaceHooks, + seedAgentSessionPreamble, +} from '../../hooks-config.js'; import { getLifecycleLog } from '../../session-lifecycle-log.js'; import { STATS_COLLECTION_INTERVAL_MS } from '../../config/server-timing.js'; import { SseEvent } from '../sse-events.js'; @@ -260,6 +265,17 @@ export function registerRebootRestoreRoutes(app: FastifyInstance, ctx: RebootRes console.warn(`[reboot-restore] hook install failed for ${session.workingDir}: ${getErrorMessage(err)}`) ); } + // Same reasoning for a CLI that reports approvals through a hook MODULE (omp, + // `approvalHookFile`): a deleted or recreated workspace would leave the restored + // pane's approval dialogs silent. Local-only is structural here (see below). + const approvalHookFile = getCli(session.mode)?.capabilities.approvalHookFile; + if (workspaceHooksEnabled && approvalHookFile) { + await applyApprovalHookFile(session.workingDir, approvalHookFile).catch((err: unknown) => + console.warn( + `[reboot-restore] approval hook install failed for ${session.workingDir}: ${getErrorMessage(err)}` + ) + ); + } // Both create paths seed this; without it a restored claude session's agent // skill falls back to writing out the whole ~150-line §0 preamble. Remote and diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 5f2bb8cc..a5e7f668 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -1090,6 +1090,19 @@ export function registerSessionRoutes( /** Test seam: inject a registry with fake IO instead of the real TCP/WoL probes. */ options: { remoteWake?: RemoteWakeRegistry } = {} ): RemoteWakeRegistry { + /** + * Install the approval hook MODULE a CLI declares (`capabilities.approvalHookFile`, + * omp) into a LOCAL workspace, so its approval dialogs raise an alert. Shared by + * POST /api/sessions and POST /api/quick-start so the two cannot drift; callers own + * the remote/Docker/named-workspace conditions and call it only after the launch + * pre-flight. Gated on `workspaceHooksEnabled` like Claude's block. Best-effort: a + * failure here must not fail the create. + */ + const installApprovalHook = async (mode: SessionMode, workspace: string): Promise => { + const fileName = getCli(mode)?.capabilities.approvalHookFile; + if (!fileName || !(await ctx.getWorkspaceHooksEnabled())) return; + await applyApprovalHookFile(workspace, fileName).catch(() => {}); + }; // Wake-on-LAN for sleeping remote hosts (see remote-wake.ts). One registry per // route registration (= one web server) — the same shape as the process-wide // `sessionWaits` singleton, but without the global. @@ -1332,18 +1345,6 @@ export function registerSessionRoutes( await applyWorkspaceHooks(workingDir, await ctx.getWorkspaceHooksEnabled()); } - // A CLI whose hooks are MODULES rather than a settings.local.json block (omp) needs - // its hook file installed into the workspace, so its approval dialogs raise the same - // alert a claude pane does. Registry-driven: `approvalHookFile` names the file, so no - // call site branches on a CLI id. Same local-workspace and `workspaceHooksEnabled` - // gate as claude's block; best-effort, because a failure here must not block a create. - const approvalHookFile = getCli(body.mode ?? 'claude')?.capabilities.approvalHookFile; - if (!remote && body.workingDir && approvalHookFile) { - if (await ctx.getWorkspaceHooksEnabled()) { - await applyApprovalHookFile(workingDir, approvalHookFile).catch(() => {}); - } - } - // Refuse up front if the requested CLI cannot start, rather than spawning a pane that // dies on `command not found`. The message comes from the resolver, so it names where // resolution actually looked (server PATH, login shell, the entry's search dirs); a @@ -1365,6 +1366,13 @@ export function registerSessionRoutes( } } + // Approval hook module (omp): after the pre-flight, like the agent skill below, so a + // CLI that cannot start leaves nothing in the workspace. Same remote and + // named-workingDir conditions as the hooks above. + if (!remote && body.workingDir) { + await installApprovalHook(body.mode ?? 'claude', workingDir); + } + // Agent skill (docs/agent-control-plan.md §2): ADD-ONLY on create, same shared- // workspace rationale as the statusLine above: a create must never remove the // skill from under other live sessions in the repo. Marker-guarded, so a user's @@ -4643,6 +4651,13 @@ export function registerSessionRoutes( } } + // Approval hook module (omp), for a new AND an existing case. The launch pre-flight + // above has passed. Local only: a remote case path lives on another host, and a Docker + // pane cannot reach the loopback API (`approvalHookAvailableForMode()` answers no there). + if (!remote && !docker) { + await installApprovalHook(mode, workspacePath); + } + // Agent skill injection (docs/agent-control-plan.md §2): ADD-ONLY on create, // marker-guarded (a user's own copy is never touched). Each CLI that takes the // skill gets it in the directory it scans (`capabilities.agentSkillDir`: diff --git a/src/web/session-wait-registry.ts b/src/web/session-wait-registry.ts index fb1acda1..bfc007b1 100644 --- a/src/web/session-wait-registry.ts +++ b/src/web/session-wait-registry.ts @@ -295,15 +295,38 @@ export function sessionHookOptions(session: { /** * Whether `blocked` can ever fire for this session: its CLI reports it through hooks - * (`hooksAvailableForMode()`), or Codeman reads the dialogs it blocks on off its screen - * (`capabilities.workDetect.blockedLine`, codex). The screen half needs no per-session - * facts: a remote or Docker pane is still a local tmux pane showing the CLI's screen - * (their nested tmux runs with `status off`, so nothing is drawn below the CLI). + * (`hooksAvailableForMode()`), through an approval-only hook module + * (`approvalHookAvailableForMode()`, omp), or Codeman reads the dialogs it blocks on off + * its screen (`capabilities.workDetect.blockedLine`, codex). The screen half needs no + * per-session facts: a remote or Docker pane is still a local tmux pane showing the CLI's + * screen (their nested tmux runs with `status off`, so nothing is drawn below the CLI). * * ⚠️ Says nothing about `stop`. A screen shows a dialog; it never says a turn ended. */ export function blockedSignalAvailable(mode: SessionMode, options: HookCapabilityOptions = {}): boolean { - return hooksAvailableForMode(mode, options) || !!getCli(mode)?.capabilities.workDetect?.blockedLine; + return ( + hooksAvailableForMode(mode, options) || + approvalHookAvailableForMode(mode, options) || + !!getCli(mode)?.capabilities.workDetect?.blockedLine + ); +} + +/** + * Whether this session reports its approval dialogs, and ONLY those, through a hook + * module Codeman installed into its workspace (`capabilities.approvalHookFile`, omp). + * + * Deliberately separate from `hooksAvailableForMode()`: its consumers read a yes as + * "this CLI has Claude's full hook set", so `stop` would be accepted and never fire, and + * the inbox would offer Approve/Deny keystrokes it cannot verify. A yes HERE means + * `blocked` can fire and a dialog opens an alert-only inbox item, nothing more. + * + * Per-session like the supervised bridge: the module is installed only into a LOCAL + * workspace Codeman created the session in, so an adopted pane, a remote attach and a + * Docker case (`deepSeekBridgeUnreachable`, set for both) never have one. + */ +export function approvalHookAvailableForMode(mode: SessionMode, options: HookCapabilityOptions = {}): boolean { + if (options.adopted || options.deepSeekBridgeUnreachable) return false; + return !!getCli(mode)?.capabilities.approvalHookFile; } /** Outcome of resolving a caller-supplied wait target against a session's mode. */ diff --git a/test/cli-capability-predicates.test.ts b/test/cli-capability-predicates.test.ts index f78276e3..0302ef3a 100644 --- a/test/cli-capability-predicates.test.ts +++ b/test/cli-capability-predicates.test.ts @@ -35,7 +35,7 @@ describe('per-mode capability predicates', () => { ['pi', true, false, false], ['grok', true, false, false], ['deepseek', true, false, true], - ['omp', true, false, true], + ['omp', true, false, false], ['copilot', true, false, false], ] as Array<[SessionMode, boolean, boolean, boolean]>)( '%s: external=%s altScreenStrip=%s hooks=%s', diff --git a/test/omp-approval-hook.test.ts b/test/omp-approval-hook.test.ts index a6595540..3e9c0670 100644 --- a/test/omp-approval-hook.test.ts +++ b/test/omp-approval-hook.test.ts @@ -1,18 +1,19 @@ /** * @fileoverview omp approval-hook wiring. * - * Pins the three things that make an omp pane able to report a pending tool approval - * at all: the registry says omp HAS hooks, Codeman installs the module that emits them, - * and the module maps omp's events onto the names the server already routes on. - * - * Without these, `hooksAvailableForMode('omp')` is false, /api/approvals never receives - * an item, and a dialog on screen raises no tab alert and no notification. + * Pins what makes an omp pane able to report a pending tool approval, and what it must + * NOT earn by doing so: the registry declares the narrow `approvalHookFile` capability + * while `hooks` stays 'none' (so `stop` is still a 400), Codeman installs the module + * that emits the events, and the module maps omp's events onto the names the server + * already routes on. The route side (both create paths, alert-only inbox items) lives in + * test/routes/omp-approval-hook-routes.test.ts and test/routes/approval-routes.test.ts. */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { mkdtempSync, mkdirSync, readFileSync, rmSync, symlinkSync, writeFileSync, existsSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { pathToFileURL } from 'node:url'; import { applyApprovalHookFile, generateOmpApprovalHook, @@ -20,61 +21,76 @@ import { OMP_APPROVAL_HOOK_FILE, } from '../src/hooks-config.js'; import { getCli } from '../src/config/cli-registry/registry.js'; -import { hooksAvailableForMode } from '../src/web/session-wait-registry.js'; +import { + hooksAvailableForMode, + approvalHookAvailableForMode, + blockedSignalAvailable, + resolveWaitSignals, +} from '../src/web/session-wait-registry.js'; const MARKER = 'CODEMAN_OMP_APPROVAL_HOOK_V1'; describe('omp approval hook — registry', () => { - it('declares hooks: always, which is what lets approval items be opened at all', () => { - expect(getCli('omp')?.capabilities.hooks).toBe('always'); - expect(hooksAvailableForMode('omp')).toBe(true); + it('keeps hooks: none, because every consumer reads "always" as Claude\'s full hook set', () => { + expect(getCli('omp')?.capabilities.hooks).toBe('none'); + expect(hooksAvailableForMode('omp')).toBe(false); + }); + + it('reports approvals through the separate approval-hook capability', () => { + expect(approvalHookAvailableForMode('omp')).toBe(true); + expect(blockedSignalAvailable('omp')).toBe(true); + expect(approvalHookAvailableForMode('claude')).toBe(false); + }); + + it('answers no where Codeman never installed the module (adopted, remote, Docker)', () => { + // sessionHookOptions() sets deepSeekBridgeUnreachable for remote and Docker sessions. + expect(approvalHookAvailableForMode('omp', { adopted: true })).toBe(false); + expect(approvalHookAvailableForMode('omp', { deepSeekBridgeUnreachable: true })).toBe(false); + expect(blockedSignalAvailable('omp', { deepSeekBridgeUnreachable: true })).toBe(false); + }); + + it('refuses an explicit stop wait for omp (the 400), and accepts blocked', () => { + const stop = resolveWaitSignals('stop', { mode: 'omp' }); + expect(stop.error).toMatch(/stop/); + expect(stop.until).toEqual([]); + const blocked = resolveWaitSignals('blocked', { mode: 'omp' }); + expect(blocked.error).toBeNull(); + expect(blocked.until).toEqual(['blocked']); + // The default set must never hold stop for omp, or a bare wait sits out its timeout. + expect(resolveWaitSignals(undefined, { mode: 'omp' }).until).not.toContain('stop'); + }); + + it('refuses blocked for a remote omp session, where no module reports', () => { + expect(resolveWaitSignals('blocked', { mode: 'omp', deepSeekBridgeUnreachable: true }).error).toMatch(/blocked/); }); it('leaves every other hook-less CLI failing the ask', () => { - // The rule that keeps this honest: 'always' is earned by a real signal, not by - // wanting alerts. opencode/gemini/pi/grok/antigravity have no approval events. + // opencode/gemini/pi/grok/antigravity have no approval events. for (const mode of ['opencode', 'gemini', 'pi', 'grok', 'antigravity', 'shell'] as const) { expect(hooksAvailableForMode(mode), mode).toBe(false); + expect(approvalHookAvailableForMode(mode), mode).toBe(false); } }); it('names the hook module as registry data, so no call site branches on the id', () => { - // The session-create path asks "does this CLI declare approvalHookFile?"; if the - // field is absent the hook is never installed and the pane stays silent. + // Both create paths ask "does this CLI declare approvalHookFile?"; if the field is + // absent the hook is never installed and the pane stays silent. expect(getCli('omp')?.capabilities.approvalHookFile).toBe(OMP_APPROVAL_HOOK_FILE); // claude installs a settings.local.json block instead and must NOT be given a file. expect(getCli('claude')?.capabilities.approvalHookFile).toBeUndefined(); }); }); -describe('omp approval hook — generated module', () => { +describe('omp approval hook — generated module (source)', () => { const src = generateOmpApprovalHook(); it('carries the ownership marker on its first line', () => { expect(src.split('\n')[0]).toContain(MARKER); }); - it('maps omp approval events onto the names the server routes on', () => { - // APPROVAL_KIND_BY_EVENT opens an item for permission_prompt and resolves it on - // elicitation_complete; using any other name makes the hook a silent no-op. - expect(src).toContain('"tool_approval_requested"'); - expect(src).toContain('"tool_approval_resolved"'); - expect(src).toContain('"permission_prompt"'); - expect(src).toContain('"elicitation_complete"'); - }); - - it('posts to the hook-event route with the secret header', () => { - expect(src).toContain('/api/hook-event'); - expect(src).toContain('X-Codeman-Hook-Secret'); - expect(src).toContain('CODEMAN_SESSION_ID'); - expect(src).toContain('CODEMAN_API_URL'); - }); - - it('fails open: both handlers catch and nothing is awaited unbounded', () => { - expect(src).toContain('catch'); - expect(src).toContain('AbortSignal.timeout'); - // `void report(...)` — a rejected report must never surface to the dialog. - expect(src).toMatch(/void report\(/); + it('imports HookAPI from the subpath omp exports it from, type-only', () => { + // omp's package root does not re-export it; erased at runtime either way. + expect(src).toContain('import type { HookAPI } from "@oh-my-pi/pi-coding-agent/extensibility/hooks";'); }); it('uses a static import, so the module parses without a dynamic loader', () => { @@ -83,6 +99,150 @@ describe('omp approval hook — generated module', () => { }); }); +/** A fake omp HookAPI: records the handlers the module registers. */ +function fakeHookApi() { + const handlers = new Map) => void>(); + return { + api: { on: (name: string, fn: (event: Record) => void) => handlers.set(name, fn) }, + emit: (name: string, event: Record) => handlers.get(name)?.(event), + }; +} + +const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +describe('omp approval hook — generated module (runtime)', () => { + let dir: string; + let fetchMock: ReturnType; + let counter = 0; + const savedEnv = { ...process.env }; + + /** Write the module to disk and import a FRESH copy (env is read at load time). */ + async function loadModule(env: Record) { + Object.assign(process.env, env); + const file = join(dir, `codeman-approval-${counter++}.ts`); + writeFileSync(file, generateOmpApprovalHook()); + const mod = (await import(pathToFileURL(file).href)) as { default: (pi: unknown) => void }; + const pi = fakeHookApi(); + mod.default(pi.api); + return pi; + } + + /** The `{ event, sessionId, data }` bodies posted, in order. */ + const posted = () => + fetchMock.mock.calls.map( + ([, init]) => JSON.parse((init as { body: string }).body) as { event: string; data: Record } + ); + + /** + * Wait until the module's report chain has made exactly `calls` POSTs, then a little + * longer so a surplus report would show up here instead of leaking into the next test. + */ + async function settle(calls: number) { + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(calls)); + await sleep(50); + expect(fetchMock).toHaveBeenCalledTimes(calls); + } + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'codeman-omp-hook-mod-')); + writeFileSync(join(dir, 'secret'), 's3cret\n'); + fetchMock = vi.fn(async () => new Response('{}')); + vi.stubGlobal('fetch', fetchMock); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + process.env = { ...savedEnv }; + rmSync(dir, { recursive: true, force: true }); + }); + + const baseEnv = () => ({ + CODEMAN_API_URL: 'http://127.0.0.1:3000', + CODEMAN_SESSION_ID: 'sess-1', + CODEMAN_HOOK_SECRET_FILE: join(dir, 'secret'), + }); + + it('maps a request to permission_prompt and a resolve to elicitation_complete, with the secret', async () => { + const pi = await loadModule(baseEnv()); + pi.emit('tool_approval_requested', { toolCallId: 'a', toolName: 'bash' }); + pi.emit('tool_approval_resolved', { toolCallId: 'a', toolName: 'bash', approved: false }); + await settle(2); + + expect(posted().map((b) => b.event)).toEqual(['permission_prompt', 'elicitation_complete']); + const [url, init] = fetchMock.mock.calls[0] as [string, { headers: Record }]; + expect(url).toBe('http://127.0.0.1:3000/api/hook-event'); + expect(init.headers['X-Codeman-Hook-Secret']).toBe('s3cret'); + expect(init).not.toHaveProperty('tls'); + }); + + it('clears the alert only when the LAST pending approval in a batch resolves', async () => { + const pi = await loadModule(baseEnv()); + pi.emit('tool_approval_requested', { toolCallId: 'a', toolName: 'bash' }); + pi.emit('tool_approval_requested', { toolCallId: 'b', toolName: 'write' }); + pi.emit('tool_approval_resolved', { toolCallId: 'a', toolName: 'bash', approved: true }); + await settle(2); + expect(posted().map((b) => b.event)).toEqual(['permission_prompt', 'permission_prompt']); + + pi.emit('tool_approval_resolved', { toolCallId: 'b', toolName: 'write', approved: true }); + await settle(3); + expect(posted().map((b) => b.event)).toEqual(['permission_prompt', 'permission_prompt', 'elicitation_complete']); + }); + + it('keeps request-then-resolve order when the first POST is slow', async () => { + // Without the chain, the resolve's POST would start while the prompt's is still in + // flight and could land first, leaving the server's alert up. + const started: string[] = []; + fetchMock.mockImplementation(async (_url: string, init: { body: string }) => { + const { event } = JSON.parse(init.body) as { event: string }; + started.push(event); + if (event === 'permission_prompt') await sleep(150); + return new Response('{}'); + }); + const pi = await loadModule(baseEnv()); + pi.emit('tool_approval_requested', { toolCallId: 'a', toolName: 'bash' }); + pi.emit('tool_approval_resolved', { toolCallId: 'a', toolName: 'bash', approved: true }); + await sleep(60); + // The resolve has not even started while the prompt is still in flight. + expect(started).toEqual(['permission_prompt']); + await settle(2); + expect(started).toEqual(['permission_prompt', 'elicitation_complete']); + }); + + it("sends omp's policy reason as message, and nothing summary-shaped without one", async () => { + const pi = await loadModule(baseEnv()); + pi.emit('tool_approval_requested', { toolCallId: 'a', toolName: 'bash', reason: 'restricted folder' }); + pi.emit('tool_approval_requested', { toolCallId: 'b', toolName: 'bash' }); + await settle(2); + const [withReason, without] = posted(); + expect(withReason.data).toEqual({ tool_name: 'bash', message: 'restricted folder' }); + expect(without.data).toEqual({ tool_name: 'bash' }); + }); + + it('skips certificate verification for an https URL (self-signed loopback)', async () => { + const pi = await loadModule({ ...baseEnv(), CODEMAN_API_URL: 'https://127.0.0.1:3000' }); + pi.emit('tool_approval_requested', { toolCallId: 'a', toolName: 'bash' }); + await settle(1); + expect(fetchMock.mock.calls[0][1]).toMatchObject({ tls: { rejectUnauthorized: false } }); + }); + + it('fails open: a rejected fetch never throws out of a handler, and later reports still go', async () => { + fetchMock.mockRejectedValueOnce(new Error('ECONNREFUSED')); + const pi = await loadModule(baseEnv()); + expect(() => pi.emit('tool_approval_requested', { toolCallId: 'a', toolName: 'bash' })).not.toThrow(); + pi.emit('tool_approval_resolved', { toolCallId: 'a', toolName: 'bash', approved: true }); + await settle(2); + }); + + it('posts nothing outside a Codeman session', async () => { + delete process.env.CODEMAN_API_URL; + delete process.env.CODEMAN_SESSION_ID; + const pi = await loadModule({}); + pi.emit('tool_approval_requested', { toolCallId: 'a', toolName: 'bash' }); + await sleep(100); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); + describe('omp approval hook — install', () => { let workspace: string; @@ -124,15 +284,39 @@ describe('omp approval hook — install', () => { expect(readFileSync(join(workspace, OMP_APPROVAL_HOOK_DIR, OMP_APPROVAL_HOOK_FILE), 'utf-8')).toBe(foreign); }); - it('refuses to write through a symlinked hooks directory', async () => { + // Every component below the workspace, and the file itself: a link at any level + // would put the module wherever it points. + it.each([ + ['.claude', '.claude'], + ['.claude/hooks', join('.claude', 'hooks')], + ['.claude/hooks/pre', OMP_APPROVAL_HOOK_DIR], + ])('refuses to write through a symlinked %s', async (_label, linkRel) => { const elsewhere = mkdtempSync(join(tmpdir(), 'codeman-omp-hook-target-')); try { - mkdirSync(join(workspace, '.claude', 'hooks'), { recursive: true }); - symlinkSync(elsewhere, join(workspace, OMP_APPROVAL_HOOK_DIR)); + const link = join(workspace, linkRel); + mkdirSync(join(link, '..'), { recursive: true }); + symlinkSync(elsewhere, link); expect(await applyApprovalHookFile(workspace, OMP_APPROVAL_HOOK_FILE)).toBe('symlink'); + // Nothing written anywhere under the link target. + expect(existsSync(join(elsewhere, 'pre'))).toBe(false); + expect(existsSync(join(elsewhere, 'hooks'))).toBe(false); expect(existsSync(join(elsewhere, OMP_APPROVAL_HOOK_FILE))).toBe(false); } finally { rmSync(elsewhere, { recursive: true, force: true }); } }); + + it('refuses a symlinked target file', async () => { + const elsewhere = mkdtempSync(join(tmpdir(), 'codeman-omp-hook-target-')); + try { + const outside = join(elsewhere, 'victim.ts'); + writeFileSync(outside, `// ${MARKER}\n`); + mkdirSync(join(workspace, OMP_APPROVAL_HOOK_DIR), { recursive: true }); + symlinkSync(outside, join(workspace, OMP_APPROVAL_HOOK_DIR, OMP_APPROVAL_HOOK_FILE)); + expect(await applyApprovalHookFile(workspace, OMP_APPROVAL_HOOK_FILE)).toBe('symlink'); + expect(readFileSync(outside, 'utf-8')).toBe(`// ${MARKER}\n`); + } finally { + rmSync(elsewhere, { recursive: true, force: true }); + } + }); }); diff --git a/test/routes/approval-routes.test.ts b/test/routes/approval-routes.test.ts index 4bfc4417..703368c6 100644 --- a/test/routes/approval-routes.test.ts +++ b/test/routes/approval-routes.test.ts @@ -13,6 +13,7 @@ import fastifyCookie from '@fastify/cookie'; import { registerApprovalRoutes } from '../../src/web/routes/approval-routes.js'; import { registerHookEventRoutes } from '../../src/web/routes/hook-event-routes.js'; import { approvalInbox } from '../../src/web/approval-inbox.js'; +import { sessionWaits } from '../../src/web/session-wait-registry.js'; import { raiseScreenBlocked } from '../../src/web/screen-blocked.js'; import { installRouteErrorHandler } from '../../src/web/route-error-handler.js'; import { httpStatusForErrorCode, type ApiErrorCode } from '../../src/types.js'; @@ -522,6 +523,85 @@ describe('approval routes: supervised-hook (DeepSeek) alert-only items', () => { }); }); +describe('approval routes: approval-only hook module (omp) alert-only items', () => { + let harness: RouteTestHarness; + let session: MockSession; + + beforeEach(async () => { + harness = await createEnvelopeHarness(); + session = harness.ctx.sessions.get(SESSION_ID)!; + session.mode = 'omp'; + // A Claude-shaped frame on purpose: even when the capture parses into options, an + // omp item must stay alert-only, because omp's composer takes the keystroke. + session.terminalBuffer = PERMISSION_DIALOG; + }); + + afterEach(async () => { + vi.restoreAllMocks(); + for (const item of approvalInbox.listPending()) { + approvalInbox.resolveForSession(item.sessionId, 'dismissed'); + } + approvalInbox.onPending = approvalInbox.onUpdated = approvalInbox.onResolved = undefined; + await harness.app.close(); + }); + + it('creates an answerInTerminal item and omits approvalId from push', async () => { + await postHook(harness, 'permission_prompt', { tool_name: 'bash', message: 'restricted folder' }); + const approvals = await listApprovals(harness); + expect(approvals).toHaveLength(1); + expect(approvals[0]).toMatchObject({ + sessionId: SESSION_ID, + kind: 'permission', + toolName: 'bash', + message: 'restricted folder', + answerInTerminal: true, + }); + const push = harness.ctx.sendPushNotifications.mock.calls.find((c) => c[0] === 'hook:permission_prompt'); + expect(push?.[1]).toBeDefined(); + expect(push?.[1]).not.toHaveProperty('approvalId'); + }); + + it('refuses Approve with a terminal-only error and writes nothing into the pane', async () => { + await postHook(harness, 'permission_prompt', { tool_name: 'bash' }); + const [item] = await listApprovals(harness); + for (const payload of [{ action: 'approve' }, { action: 'deny' }, { action: 'option', option: 1 }]) { + const res = await harness.app.inject({ method: 'POST', url: `/api/approvals/${item.id}/answer`, payload }); + expect(res.statusCode).toBe(400); + expect(res.json().error).toMatch(/answered in the terminal/i); + } + expect(session.writeBuffer).toEqual([]); + expect(await listApprovals(harness)).toHaveLength(1); + }); + + it('resolves the item on elicitation_complete', async () => { + await postHook(harness, 'permission_prompt', { tool_name: 'bash' }); + await postHook(harness, 'elicitation_complete', { tool_name: 'bash', approved: true }); + expect(await listApprovals(harness)).toHaveLength(0); + }); + + it('wakes a blocked waiter, and never a stop waiter', async () => { + const notify = vi.spyOn(sessionWaits, 'notifySignal'); + await postHook(harness, 'permission_prompt', { tool_name: 'bash' }); + await postHook(harness, 'stop', {}); + expect(notify.mock.calls).toEqual([[SESSION_ID, 'blocked']]); + }); + + it('drops events the module never sends (stop, idle_prompt) instead of opening items', async () => { + await postHook(harness, 'idle_prompt', {}); + expect(await listApprovals(harness)).toHaveLength(0); + await postHook(harness, 'permission_prompt', { tool_name: 'bash' }); + // A forged stop must not clear a real pending dialog either. + await postHook(harness, 'stop', {}); + expect(await listApprovals(harness)).toHaveLength(1); + }); + + it('opens nothing for a remote omp session, where no module was installed', async () => { + (session as unknown as { remote: unknown }).remote = { host: 'box' }; + await postHook(harness, 'permission_prompt', { tool_name: 'bash' }); + expect(await listApprovals(harness)).toHaveLength(0); + }); +}); + describe('approval routes: dialogs read off a hook-less screen (codex) are alert-only', () => { let harness: RouteTestHarness; let session: MockSession; diff --git a/test/routes/omp-approval-hook-routes.test.ts b/test/routes/omp-approval-hook-routes.test.ts new file mode 100644 index 00000000..1e680cbf --- /dev/null +++ b/test/routes/omp-approval-hook-routes.test.ts @@ -0,0 +1,177 @@ +/** + * @fileoverview The omp approval hook module on BOTH session create paths. + * + * `POST /api/quick-start` is how the Run button, the agent skill's `spawn_worker` and + * `codeman agent spawn` start omp, so installing only from `POST /api/sessions` left the + * usual omp launch silent. These tests pin, on disk: + * - both paths write `.claude/hooks/pre/codeman-approval.ts` into a local workspace; + * - neither writes Claude's `.claude/settings.local.json` hook block for omp (only + * claude reads it) or, for a Docker case, a CLAUDE.md; + * - `workspaceHooksEnabled: false` writes nothing; + * - a create refused by the launch pre-flight leaves nothing behind. + * + * The launch pre-flight is mocked so the result does not depend on whether omp is + * installed on the machine running the suite. Uses app.inject(), no port. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import Fastify, { type FastifyInstance } from 'fastify'; +import fastifyCookie from '@fastify/cookie'; +import { mkdtemp, rm, mkdir, writeFile } from 'node:fs/promises'; +import { existsSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { createMockRouteContext, safeRmHomeTree } from '../mocks/index.js'; +import { installRouteErrorHandler } from '../../src/web/route-error-handler.js'; +import { registerSessionRoutes } from '../../src/web/routes/session-routes.js'; +import { OMP_APPROVAL_HOOK_DIR, OMP_APPROVAL_HOOK_FILE } from '../../src/hooks-config.js'; +import { getDataDir } from '../../src/config/instance.js'; +import { CASES_DIR } from '../../src/web/route-helpers.js'; +import { Session } from '../../src/session.js'; + +const launch = vi.hoisted(() => ({ error: null as string | null })); +vi.mock('../../src/utils/cli-launcher.js', async (importOriginal) => ({ + ...(await importOriginal()), + resolveCliLaunchError: vi.fn(async () => launch.error), +})); + +const hookFileIn = (dir: string) => join(dir, OMP_APPROVAL_HOOK_DIR, OMP_APPROVAL_HOOK_FILE); +const claudeSettingsIn = (dir: string) => join(dir, '.claude', 'settings.local.json'); + +/** + * A create that SUCCEEDED. The status alone cannot say so: `createErrorResponse()` + * envelopes come back as 200 here (no preSerialization hook), so an early refusal would + * make every "wrote nothing" assertion below pass vacuously. A success body is the raw + * payload (no envelope), so the proof is a real session id in it. + */ +function expectCreated(res: { statusCode: number; body: string }) { + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.body) as { success?: boolean; session?: { id?: string }; sessionId?: string }; + expect(body.success, res.body).not.toBe(false); + expect(body.session?.id ?? body.sessionId, res.body).toBeTruthy(); +} + +describe('omp approval hook on the create paths', () => { + let app: FastifyInstance; + + const useApp = async (workspaceHooksEnabled = true) => { + await app?.close(); + app = Fastify({ logger: false }); + await app.register(fastifyCookie); + registerSessionRoutes(app, createMockRouteContext({ workspaceHooksEnabled })); + installRouteErrorHandler(app); + await app.ready(); + }; + + beforeEach(async () => { + launch.error = null; + vi.spyOn(Session.prototype, 'startInteractive').mockResolvedValue(undefined); + vi.spyOn(Session.prototype, 'startShell').mockResolvedValue(undefined); + await useApp(); + }); + + afterEach(async () => { + await app.close(); + vi.restoreAllMocks(); + await rm(join(getDataDir(), 'docker-hosts.json'), { force: true }); + await rm(join(getDataDir(), 'docker-cases.json'), { force: true }); + // CASES_DIR sits under the (redirected) test HOME; safeRmHomeTree refuses anything else. + safeRmHomeTree(CASES_DIR); + }); + + describe('POST /api/sessions', () => { + let workingDir: string; + const createSession = (payload: Record) => + app.inject({ method: 'POST', url: '/api/sessions', payload }); + + beforeEach(async () => { + workingDir = await mkdtemp(join(tmpdir(), 'codeman-omp-create-')); + }); + + afterEach(async () => { + await rm(workingDir, { recursive: true, force: true }); + }); + + it('installs the module, and no Claude hook block', async () => { + expectCreated(await createSession({ name: 'omp-create', mode: 'omp', workingDir })); + expect(existsSync(hookFileIn(workingDir))).toBe(true); + expect(existsSync(claudeSettingsIn(workingDir))).toBe(false); + }); + + it('writes nothing when workspaceHooksEnabled is off', async () => { + await useApp(false); + expectCreated(await createSession({ name: 'omp-off', mode: 'omp', workingDir })); + expect(existsSync(join(workingDir, '.claude'))).toBe(false); + }); + + it('leaves nothing behind when the launch pre-flight refuses the create', async () => { + launch.error = 'omp: command not found'; + const res = await createSession({ name: 'omp-missing', mode: 'omp', workingDir }); + expect(res.json().success).toBe(false); + expect(existsSync(join(workingDir, '.claude'))).toBe(false); + }); + + it('gives claude no module', async () => { + expectCreated(await createSession({ name: 'claude-create', mode: 'claude', workingDir })); + expect(existsSync(hookFileIn(workingDir))).toBe(false); + }); + }); + + describe('POST /api/quick-start', () => { + const quickStart = (payload: Record) => + app.inject({ method: 'POST', url: '/api/quick-start', payload }); + + it('installs the module into an EXISTING case, and no Claude hook block', async () => { + const casePath = join(CASES_DIR, 'ompexisting'); + await mkdir(casePath, { recursive: true }); + expectCreated(await quickStart({ caseName: 'ompexisting', mode: 'omp' })); + expect(existsSync(hookFileIn(casePath))).toBe(true); + expect(existsSync(claudeSettingsIn(casePath))).toBe(false); + }); + + it('installs the module into a case it scaffolds, and no Claude hook block', async () => { + expectCreated(await quickStart({ caseName: 'ompnew', mode: 'omp' })); + const casePath = join(CASES_DIR, 'ompnew'); + expect(existsSync(hookFileIn(casePath))).toBe(true); + expect(existsSync(claudeSettingsIn(casePath))).toBe(false); + }); + + it('writes nothing into an existing case when workspaceHooksEnabled is off', async () => { + await useApp(false); + const casePath = join(CASES_DIR, 'ompoff'); + await mkdir(casePath, { recursive: true }); + expectCreated(await quickStart({ caseName: 'ompoff', mode: 'omp' })); + expect(existsSync(join(casePath, '.claude'))).toBe(false); + }); + + it('leaves nothing behind when the launch pre-flight refuses', async () => { + launch.error = 'omp: command not found'; + const casePath = join(CASES_DIR, 'ompmissing'); + await mkdir(casePath, { recursive: true }); + const res = await quickStart({ caseName: 'ompmissing', mode: 'omp' }); + expect(res.json().success).toBe(false); + expect(existsSync(join(casePath, '.claude'))).toBe(false); + }); + + it('writes neither the module nor a Claude block (or CLAUDE.md) into a Docker case', async () => { + const ws = await mkdtemp(join(tmpdir(), 'codeman-docker-omp-')); + try { + await mkdir(getDataDir(), { recursive: true }); + await writeFile( + join(getDataDir(), 'docker-hosts.json'), + JSON.stringify([{ id: 'd1', label: 'box', image: 'codeman/agent:base' }]) + ); + await writeFile( + join(getDataDir(), 'docker-cases.json'), + JSON.stringify([{ name: 'dockomp', type: 'docker', hostId: 'd1', hostWorkspacePath: ws }]) + ); + expectCreated(await quickStart({ caseName: 'dockomp', mode: 'omp' })); + expect(existsSync(hookFileIn(ws))).toBe(false); + expect(existsSync(claudeSettingsIn(ws))).toBe(false); + expect(existsSync(join(ws, 'CLAUDE.md'))).toBe(false); + } finally { + await rm(ws, { recursive: true, force: true }); + } + }); + }); +});