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