mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-14 02:29:33 +01:00
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
0bbf483a35
commit
27d6e2e30a
14 files changed
+701
-125
No files matched your search
@@ -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<string, ApprovalKind> = {
|
||||
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 }),
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<void> => {
|
||||
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`:
|
||||
|
||||
@@ -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. */
|
||||
|
||||
Reference in new issue
Block a user