From 27d6e2e30a9f80823274e594feedcdca93c8be3b Mon Sep 17 00:00:00 2001 From: timkjr Date: Tue, 10 Nov 2026 09:59:22 -0600 Subject: [PATCH] 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 }); + } + }); + }); +});