From 0bbf483a35b1589f7b9ce22908abd5ed89ac8260 Mon Sep 17 00:00:00 2001 From: timkjr Date: Mon, 9 Nov 2026 11:14:18 -0600 Subject: [PATCH] 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 }); + } + }); +});