mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-02 13:39:41 +02:00
review fixes: every claude create path routes through the workspace-hooks decision
Post-#304 follow-ups. The install-vs-refresh decision (workspaceHooksEnabled, default ON) moved from a session-routes-local helper into hooks-config.ts as applyWorkspaceHooks(workspace, install?), and the claude session-create sites that bypassed it now go through it: cron job fires (cron-service), legacy scheduled-run iterations (runScheduledLoop), and the plan-orchestrator research and planner one-shots. A cron or scheduled run firing in a linked case that never had an interactive session ran hook-blind (no stop for completion detection, no tab alert on a blocking dialog). The shared core also carries the two guards every caller needs: a workspace that no longer exists is skipped (ensureCodemanHooks mkdir -p's, so the boot recovery sweep used to resurrect a deleted repo as an empty tree holding only .claude/settings.local.json), and all errors are swallowed since a create must never fail on hooks. Route handlers keep resolving the setting through their ConfigPort and pass it in; non-route callers omit it and the core reads settings.json itself (absent key or unreadable file = ON). Two adjacent gates tightened in session-routes: - the docker quick-start hooks branch excluded the five external CLIs but let `shell` through, contradicting its own rule that only claude reads .claude hooks; it is now gated on mode === 'claude' - the statusLine exporter call in POST /api/sessions got the same !remote && body.workingDir guard the hooks call got in499d355(it mkdirs the same way, so a remote attach created a junk user@host:session dir locally and a cwd-fallback create wrote into $HOME) plan-routes' one-shot deliberately stays out: its workingDir is process.cwd(), exactly the target499d355forbids writing into. restoreMuxSessions stays out too: the boot sweep already covers recovered workspaces. Tests: quick-start existing-case install, docker claude-installs/shell-does-not, and the core directly (default-ON install, OFF add-nothing, OFF still heals a stale block, malformed file untouched, vanished workspace skipped); the remote and cwd-fallback regressions now also send statusLineTelemetry:true to pin the statusLine guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -11,6 +11,7 @@ import { v4 as uuidv4 } from 'uuid';
|
||||
import { readFile } from 'node:fs/promises';
|
||||
import { statSync, realpathSync } from 'node:fs';
|
||||
import { Session } from '../session.js';
|
||||
import { applyWorkspaceHooks } from '../hooks-config.js';
|
||||
import { SseEvent } from '../web/sse-events.js';
|
||||
import { CronJobSchema } from '../web/schemas.js';
|
||||
import { getErrorMessage, createErrorResponse, ApiErrorCode } from '../types/api.js';
|
||||
@@ -401,6 +402,15 @@ export class CronService {
|
||||
// clampCronExternalCliConfigs — cron sends no per-CLI config, so the CLI's own
|
||||
// spawn default is what would otherwise apply).
|
||||
const { geminiConfig, piConfig } = clampCronExternalCliConfigs(mode, ownerGranted);
|
||||
// Workspace hooks (see applyWorkspaceHooks in hooks-config): cron jobs are
|
||||
// always local (workingDir was stat-validated above) but used to bypass the
|
||||
// shared install-vs-refresh decision, so a job firing in a linked case that
|
||||
// never had an interactive session ran hook-blind — no `stop` for the
|
||||
// completion detection, no tab alert on a blocking dialog. Claude mode only
|
||||
// (nothing else reads `.claude` hooks); best-effort inside the helper.
|
||||
if (mode === 'claude') {
|
||||
await applyWorkspaceHooks(job.workingDir);
|
||||
}
|
||||
session = new Session({
|
||||
workingDir: job.workingDir,
|
||||
mode,
|
||||
|
||||
+61
-1
@@ -10,8 +10,9 @@
|
||||
* Key exports:
|
||||
* - `generateHooksConfig()` — returns hooks object for settings.local.json
|
||||
* - `writeHooksConfig(casePath)` — writes hooks + env config to disk
|
||||
* - `applyWorkspaceHooks(workspace, install?)` — the ONE install-vs-refresh decision
|
||||
* point every claude-session create path routes through (see its doc comment)
|
||||
* - `ensureCodemanHooks(casePath)` — safely installs/updates hooks for a managed case
|
||||
* (no production call site yet; see its doc comment before wiring one)
|
||||
* - `updateCaseEnvVars(casePath, envVars)` — merges env vars into settings
|
||||
*
|
||||
* Hook events generated: `idle_prompt`, `permission_prompt`, `elicitation_dialog`,
|
||||
@@ -37,6 +38,7 @@ import { fileURLToPath } from 'node:url';
|
||||
|
||||
import type { HookEventType } from './types.js';
|
||||
import { HOOK_TIMEOUT_SECONDS } from './config/auth-config.js';
|
||||
import { dataPath } from './config/instance.js';
|
||||
|
||||
/**
|
||||
* Serializes read-modify-write access to a `settings.local.json` path. Every
|
||||
@@ -747,6 +749,64 @@ export async function refreshStaleCodemanHooks(casePath: string): Promise<void>
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Hooks for the workspace a Claude session is about to run in. ONE decision point,
|
||||
* shared by every claude-session create path — the interactive routes, quick-start,
|
||||
* cron fires, legacy scheduled runs, the plan-orchestrator one-shots, and the boot
|
||||
* recovery sweep — so the `workspaceHooksEnabled` setting cannot apply to some of
|
||||
* them only.
|
||||
*
|
||||
* ON (the default): INSTALL Codeman's hooks block (`ensureCodemanHooks`), merging so
|
||||
* a user's own hook entries and every other settings key survive. Hooks used to be
|
||||
* written only when Codeman CREATED the case DIRECTORY, so a linked case or any
|
||||
* pre-existing repo — where most sessions actually run — had none, and every
|
||||
* hook-driven surface was silently dead there (full history on `ensureCodemanHooks`).
|
||||
*
|
||||
* OFF: the older, narrower behavior. A Codeman block that is already there is still
|
||||
* refreshed when stale (COD-91: a pre-secret block 401s once the hook-secret gate
|
||||
* went unconditional), but one is never added, so Codeman leaves the repo alone.
|
||||
*
|
||||
* `install` overrides the setting read: route handlers resolve it through their
|
||||
* ConfigPort (`ctx.getWorkspaceHooksEnabled()`, which tests stub), and the boot sweep
|
||||
* passes `true` after checking the setting once for its whole batch. Every other
|
||||
* caller omits it and the synced setting is read from settings.json here — default ON
|
||||
* when the key is absent or the file unreadable, matching the server's resolver.
|
||||
*
|
||||
* Callers gate on their own context (claude mode only; local — never a remote
|
||||
* workingDir, which is a path on ANOTHER host, and never a docker case that opted
|
||||
* out of hooks). The guards EVERY caller needs live here instead:
|
||||
* - a workspace that does not exist is skipped — `ensureCodemanHooks` mkdir -p's,
|
||||
* so a deleted repo whose tmux session survived would otherwise be resurrected
|
||||
* as an empty directory tree holding only `.claude/settings.local.json`;
|
||||
* - errors are swallowed — a session create must never fail on hooks.
|
||||
*/
|
||||
export async function applyWorkspaceHooks(workspace: string, install?: boolean): Promise<void> {
|
||||
try {
|
||||
if (!existsSync(workspace)) return;
|
||||
const shouldInstall = install ?? (await readWorkspaceHooksEnabled());
|
||||
await (shouldInstall ? ensureCodemanHooks(workspace) : refreshStaleCodemanHooks(workspace));
|
||||
} catch {
|
||||
// Best-effort by contract (see doc comment): hooks degrade to output-based
|
||||
// idle detection; the create goes ahead.
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The synced `workspaceHooksEnabled` app setting, read straight from settings.json
|
||||
* for callers that live outside the web layer (cron, scheduled runs, the plan
|
||||
* orchestrator). Default ON: an absent key means a user who has never seen the
|
||||
* setting, and OFF for them would mean no tab alerts, no Approvals Inbox and no
|
||||
* respawn idle signals in every workspace Codeman did not scaffold itself.
|
||||
*/
|
||||
async function readWorkspaceHooksEnabled(): Promise<boolean> {
|
||||
try {
|
||||
const parsed = JSON.parse(await readFile(dataPath('settings.json'), 'utf-8')) as Record<string, unknown>;
|
||||
return parsed.workspaceHooksEnabled !== false;
|
||||
} catch {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
/** Unique marker identifying Codeman's own statusLine command (vs a user's). */
|
||||
const STATUSLINE_MARKER = '/api/status-telemetry';
|
||||
|
||||
|
||||
@@ -20,6 +20,7 @@ import type { TerminalMultiplexer } from './mux-interface.js';
|
||||
import { existsSync, mkdirSync, writeFileSync } from 'node:fs';
|
||||
import { join } from 'node:path';
|
||||
import { RESEARCH_AGENT_PROMPT, PLANNER_PROMPT } from './prompts/index.js';
|
||||
import { applyWorkspaceHooks } from './hooks-config.js';
|
||||
import { getErrorMessage, type PlanItem, type ClaudeMode } from './types.js';
|
||||
|
||||
// Re-export for backward compatibility
|
||||
@@ -429,6 +430,11 @@ export class PlanOrchestrator {
|
||||
detail: 'Researching...',
|
||||
});
|
||||
|
||||
// Workspace hooks for the case this plan targets (see applyWorkspaceHooks in
|
||||
// hooks-config): claude-mode, local workingDir, and the helper itself skips a
|
||||
// vanished dir + swallows failures — the plan run must never fail on hooks.
|
||||
await applyWorkspaceHooks(this.workingDir);
|
||||
|
||||
const session = new Session({
|
||||
workingDir: this.workingDir,
|
||||
mux: this.mux,
|
||||
@@ -591,6 +597,10 @@ export class PlanOrchestrator {
|
||||
detail: 'Generating plan...',
|
||||
});
|
||||
|
||||
// Workspace hooks: same rationale as the research one-shot above (idempotent —
|
||||
// the helper short-circuits when the hooks block is already current).
|
||||
await applyWorkspaceHooks(this.workingDir);
|
||||
|
||||
const session = new Session({
|
||||
workingDir: this.workingDir,
|
||||
mux: this.mux,
|
||||
|
||||
@@ -84,7 +84,7 @@ import {
|
||||
applyAgentSkill,
|
||||
refreshUserAgentSkill,
|
||||
seedAgentSessionPreamble,
|
||||
ensureCodemanHooks,
|
||||
applyWorkspaceHooks,
|
||||
refreshStaleCodemanHooks,
|
||||
} from '../../hooks-config.js';
|
||||
import { generateClaudeMd } from '../../templates/claude-md.js';
|
||||
@@ -626,31 +626,12 @@ async function injectAgentSkill(casePath: string): Promise<void> {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Hooks for the workspace a Claude session is about to run in. ONE decision point,
|
||||
* shared by every create path, so the setting cannot apply to some of them only.
|
||||
*
|
||||
* ON (`workspaceHooksEnabled`, the default): INSTALL Codeman's hooks block, merging
|
||||
* so a user's own hook entries and every other settings key survive. Hooks were
|
||||
* previously written only when Codeman CREATED the case DIRECTORY, so a linked case
|
||||
* or any pre-existing repo — where most sessions actually run — had none, and every
|
||||
* hook-driven surface was silently dead there: no tab alert or phone-overview row
|
||||
* when a dialog blocks the pane, no Approvals Inbox item, no push, no definitive
|
||||
* `stop`/`idle_prompt` for respawn, and no `stop`/`blocked` for the wait endpoints.
|
||||
* Measured 2026-08-15 in a linked case: an AskUserQuestion dialog on screen with the
|
||||
* tab reporting a calm `idle`. Claude Code re-reads the file, so a session already
|
||||
* running in that workspace starts firing hooks without a restart (verified live).
|
||||
*
|
||||
* OFF: the older, narrower behavior. A Codeman block that is already there is still
|
||||
* refreshed when stale (COD-91: a pre-secret block 401s once the hook-secret gate
|
||||
* went unconditional), but one is never added, so Codeman leaves the repo alone.
|
||||
*
|
||||
* Best-effort either way: a refusal or a thrown error must never fail the create.
|
||||
*/
|
||||
async function applyWorkspaceHooks(ctx: ConfigPort, workspace: string): Promise<void> {
|
||||
const install = await ctx.getWorkspaceHooksEnabled();
|
||||
await (install ? ensureCodemanHooks(workspace) : refreshStaleCodemanHooks(workspace)).catch(() => {});
|
||||
}
|
||||
// Workspace hooks: the install-vs-refresh decision core moved to
|
||||
// `applyWorkspaceHooks` in hooks-config.ts (imported above) so the non-route
|
||||
// claude create paths — cron fires, legacy scheduled runs, the plan-orchestrator
|
||||
// one-shots, the boot recovery sweep — share the SAME decision instead of
|
||||
// bypassing the `workspaceHooksEnabled` setting. Route handlers here resolve the
|
||||
// setting through the ConfigPort (tests stub it) and pass it as the second arg.
|
||||
|
||||
export function registerSessionRoutes(
|
||||
app: FastifyInstance,
|
||||
@@ -788,7 +769,14 @@ export function registerSessionRoutes(
|
||||
// chip's data feed for everyone. The exporter is benign when the chip is off
|
||||
// (the footer just shows session status). isOurs-guarded so a user's own
|
||||
// statusLine is never touched.
|
||||
if ((body.mode ?? 'claude') === 'claude' && body.statusLineTelemetry === true) {
|
||||
//
|
||||
// Same guard as the hooks call below (499d355): never for a remote attach
|
||||
// (workingDir is a user@host:session pseudo-path — the mkdir inside
|
||||
// applyStatusLineConfig would create it as a junk local dir), and only when
|
||||
// the caller named a workingDir — the process-cwd fallback is $HOME under
|
||||
// installer-created services, and a statusLine materializing in
|
||||
// ~/.claude/settings.local.json was never asked for.
|
||||
if (!remote && body.workingDir && (body.mode ?? 'claude') === 'claude' && body.statusLineTelemetry === true) {
|
||||
await applyStatusLineConfig(workingDir, true);
|
||||
}
|
||||
|
||||
@@ -799,7 +787,7 @@ export function registerSessionRoutes(
|
||||
// process-cwd fallback is $HOME under installer-created services, and hooks
|
||||
// materializing in ~/.claude/settings.local.json was never asked for.
|
||||
if (!remote && body.workingDir && (body.mode ?? 'claude') === 'claude') {
|
||||
await applyWorkspaceHooks(ctx, workingDir);
|
||||
await applyWorkspaceHooks(workingDir, await ctx.getWorkspaceHooksEnabled());
|
||||
// Agent skill (docs/agent-control-plan.md §2): ADD-ONLY on create, same shared-
|
||||
// .claude rationale as the statusLine above: a create must never remove the
|
||||
// skill from under other live sessions in the repo. Marker-guarded, so a
|
||||
@@ -2936,7 +2924,7 @@ export function registerSessionRoutes(
|
||||
// of its own. Skipped for remote cases — resolvedCasePath is a REMOTE path that
|
||||
// doesn't exist on the local filesystem.
|
||||
if (mode === 'claude') {
|
||||
await applyWorkspaceHooks(ctx, resolvedCasePath);
|
||||
await applyWorkspaceHooks(resolvedCasePath, await ctx.getWorkspaceHooksEnabled());
|
||||
} else {
|
||||
await refreshStaleCodemanHooks(resolvedCasePath).catch(() => {});
|
||||
}
|
||||
@@ -2954,16 +2942,11 @@ export function registerSessionRoutes(
|
||||
// Docker cases: the workspace is a REAL host dir bind-mounted into the container.
|
||||
// Scaffold hooks (+ a CLAUDE.md) if MISSING so in-container permission prompts and
|
||||
// hook-idle detection fire (decision: wire hooks now). Never clobbers an existing
|
||||
// configured project. Skipped for external CLIs (they use their own systems).
|
||||
if (
|
||||
docker &&
|
||||
docker.hooksEnabled &&
|
||||
mode !== 'opencode' &&
|
||||
mode !== 'codex' &&
|
||||
mode !== 'gemini' &&
|
||||
mode !== 'antigravity' &&
|
||||
mode !== 'pi'
|
||||
) {
|
||||
// configured project. Claude mode ONLY — only claude reads `.claude` hooks, so a
|
||||
// shell or external-CLI quick-start must not author a block of its own (the same
|
||||
// rule the existing-case branch above states; this branch used to exclude just
|
||||
// the five external CLIs and let `shell` through).
|
||||
if (docker && docker.hooksEnabled && mode === 'claude') {
|
||||
try {
|
||||
if (!existsSync(join(resolvedCasePath, 'CLAUDE.md'))) {
|
||||
const templatePath = await ctx.getDefaultClaudeMdPath();
|
||||
@@ -2975,7 +2958,7 @@ export function registerSessionRoutes(
|
||||
// A settings file with no hooks in it is the same dead-surface case as a
|
||||
// linked case. This branch is already gated on `docker.hooksEnabled`, and
|
||||
// applyWorkspaceHooks adds the user-level gate on top.
|
||||
await applyWorkspaceHooks(ctx, resolvedCasePath);
|
||||
await applyWorkspaceHooks(resolvedCasePath, await ctx.getWorkspaceHooksEnabled());
|
||||
}
|
||||
} catch {
|
||||
/* non-fatal — the session still runs, hooks may be degraded */
|
||||
|
||||
+17
-2
@@ -76,7 +76,7 @@ import { RunSummaryTracker } from '../run-summary.js';
|
||||
import { PlanOrchestrator } from '../plan-orchestrator.js';
|
||||
import { OrchestratorLoop } from '../orchestrator-loop.js';
|
||||
import { getLifecycleLog } from '../session-lifecycle-log.js';
|
||||
import { ensureCodemanHooks } from '../hooks-config.js';
|
||||
import { applyWorkspaceHooks } from '../hooks-config.js';
|
||||
import { PushSubscriptionStore } from '../push-store.js';
|
||||
import webpush from 'web-push';
|
||||
import { SseStreamManager } from './sse-stream-manager.js';
|
||||
@@ -1819,6 +1819,14 @@ export class WebServer extends EventEmitter {
|
||||
|
||||
let session: Session | null = null;
|
||||
try {
|
||||
// Workspace hooks for this iteration's session — legacy scheduled runs are
|
||||
// always claude-mode and always local, and used to bypass the shared decision
|
||||
// entirely: a scheduled run firing in a linked case that never had an
|
||||
// interactive session ran hook-blind (see applyWorkspaceHooks in hooks-config;
|
||||
// it reads the `workspaceHooksEnabled` setting itself, skips a vanished
|
||||
// workingDir, and swallows failures — a run must never fail on hooks).
|
||||
await applyWorkspaceHooks(run.workingDir);
|
||||
|
||||
// Create a session for this iteration.
|
||||
if (isMultiUserMode()) {
|
||||
// §6.3: resolve the permission mode with the RUN OWNER (a non-granted user
|
||||
@@ -2881,6 +2889,11 @@ export class WebServer extends EventEmitter {
|
||||
* Skipped entirely when `workspaceHooksEnabled` is OFF: that setting exists so a
|
||||
* user can keep Codeman out of their repos, and a boot-time sweep is the last
|
||||
* place that should ignore it.
|
||||
*
|
||||
* A workspace that no longer EXISTS is skipped by applyWorkspaceHooks: a tmux
|
||||
* session can outlive its deleted repo, and `ensureCodemanHooks` mkdir -p's, so
|
||||
* the sweep used to resurrect the directory as an empty tree holding only
|
||||
* `.claude/settings.local.json`.
|
||||
*/
|
||||
private async ensureHooksForRecoveredWorkspaces(): Promise<void> {
|
||||
if (!(await this.getWorkspaceHooksEnabled())) return;
|
||||
@@ -2891,7 +2904,9 @@ export class WebServer extends EventEmitter {
|
||||
if (session.workingDir) workspaces.add(session.workingDir);
|
||||
}
|
||||
for (const workspace of workspaces) {
|
||||
await ensureCodemanHooks(workspace).catch(() => {});
|
||||
// install=true: the setting was already resolved ON above for the whole batch
|
||||
// (OFF skips the sweep wholesale, keeping its documented semantics).
|
||||
await applyWorkspaceHooks(workspace, true);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user