mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-14 02:29:33 +01:00
fix(omp): an omp alert stays up until omp says the dialog closed (#798)
The four edits promised in the review of #798: - An omp alert could clear while omp's dialog was still up. omp items got the same pane `capture` as Claude's, which the inbox re-reads with Claude's numbered-dialog parser. omp's dialog has no numbered rows, but an agent's plan above it does, so the list was parsed as the item's options, and once it scrolled out of the frame the item was resolved as answered in the terminal. An approval-only item now takes the frame once as `context` and gets no `capture`, like a screen-read item (web/screen-blocked.ts), so the module's `elicitation_complete` is the only thing that clears it. A route test puts a numbered list above an omp-shaped dialog and fails on the old code at both steps (options parsed, alert cleared). - The reboot-restore install is dropped: the restore plan skips every CLI whose transcript is not `claude-jsonl` (omp's is `omp-jsonl`), so it never ran, and it sat after the start, when omp has already scanned its hooks. - The docs that say which CLIs raise `blocked` and open alert-only items now name omp: api-reference, CLAUDE.md, architecture-invariants, an entry for `approvalHookFile` in cli-registry, and the skill with its plugin mirror (verbs.md listed omp among the modes where `blocked` is a 400, one line above the paragraph that says it works). - Two comments: the schema's file-name rule is a pattern, not a closed list, and the marker check takes the marker anywhere in the file. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
This commit is contained in:
1 parent
13f1b986af
commit
3eb0689f6d
15 files changed
+79
-46
No files matched your search
@@ -326,9 +326,10 @@ const capabilitiesSchema = z
|
||||
// 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.
|
||||
// block (omp). Not a closed list like `agentSkillDir`: any bare `.ts` file name is
|
||||
// taken, and nothing else. The name is written into the user's workspace, so a path
|
||||
// (`../../evil.ts`, an absolute one) must be refused by the schema rather than by
|
||||
// whatever calls the writer.
|
||||
approvalHookFile: z
|
||||
.string()
|
||||
.regex(/^[A-Za-z0-9._-]+\.ts$/)
|
||||
|
||||
+2
-1
@@ -1793,7 +1793,8 @@ export type ApprovalHookApplyResult = 'installed' | 'refreshed' | 'unchanged' |
|
||||
/**
|
||||
* Install or refresh the omp approval hook in `<workspace>/.claude/hooks/pre/`.
|
||||
*
|
||||
* Ownership: the file carries `OMP_APPROVAL_HOOK_MARKER` in its first line. A file
|
||||
* Ownership: the file carries `OMP_APPROVAL_HOOK_MARKER` (the generated module has it
|
||||
* in its header, and the check takes it anywhere in the file). 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.
|
||||
* EVERY component below the workspace (`.claude`, `.claude/hooks`, `.claude/hooks/pre`)
|
||||
|
||||
@@ -228,6 +228,13 @@ export function registerHookEventRoutes(
|
||||
const toolSummary = toolInput
|
||||
? [toolInput.command, toolInput.file_path, toolInput.description].find((v) => typeof v === 'string')
|
||||
: undefined;
|
||||
// Visible tmux frame first (it IS the dialog); raw byte-buffer tail as
|
||||
// the fallback for direct-PTY sessions and the no-op test mux.
|
||||
const readFrame = (): string | null => {
|
||||
const muxName = session.muxName;
|
||||
const frame = muxName ? (ctx.mux.capturePaneBuffer?.(muxName) ?? null) : null;
|
||||
return frame ?? session.terminalBuffer.slice(-8192) ?? null;
|
||||
};
|
||||
const item = approvalInbox.notePrompt({
|
||||
sessionId,
|
||||
sessionName,
|
||||
@@ -247,13 +254,14 @@ export function registerHookEventRoutes(
|
||||
// alert nobody can answer; notePrompt() carries the whole reasoning.
|
||||
watching: session.watching,
|
||||
preAcknowledged: keepAliveReason,
|
||||
// Visible tmux frame first (it IS the dialog); raw byte-buffer tail as
|
||||
// the fallback for direct-PTY sessions and the no-op test mux.
|
||||
capture: () => {
|
||||
const muxName = session.muxName;
|
||||
const frame = muxName ? (ctx.mux.capturePaneBuffer?.(muxName) ?? null) : null;
|
||||
return frame ?? session.terminalBuffer.slice(-8192) ?? null;
|
||||
},
|
||||
// An approval-only module (omp) clears its own item with `elicitation_complete`,
|
||||
// so its frame is taken ONCE, as the card's context, and never read again, like
|
||||
// an item raised from the screen (web/screen-blocked.ts). The inbox's re-read is
|
||||
// shaped for Claude's numbered dialog: omp's has no numbered rows, but an agent's
|
||||
// plan above it does, so the list was parsed as the item's options, and once it
|
||||
// scrolled out of the frame the item was resolved as answered in the terminal
|
||||
// while omp still waited.
|
||||
...(approvalOnly ? { context: readFrame() ?? undefined } : { capture: readFrame }),
|
||||
});
|
||||
approvalId = item.id;
|
||||
acknowledgedReason = item.acknowledgedReason;
|
||||
|
||||
@@ -42,12 +42,7 @@ 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,
|
||||
applyApprovalHookFile,
|
||||
applyWorkspaceHooks,
|
||||
seedAgentSessionPreamble,
|
||||
} from '../../hooks-config.js';
|
||||
import { agentSkillDir, 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';
|
||||
@@ -265,17 +260,6 @@ 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
|
||||
|
||||
Reference in new issue
Block a user