From 09ca092a799d0dc9a19fd2a4334b8275cbdd700d Mon Sep 17 00:00:00 2001 From: t Date: Sun, 6 Sep 2026 02:32:20 +0530 Subject: [PATCH 1/4] fix(security): wire the permission bridge, the sandbox, and worktree preservation into production Three public claims did not match the code (ADR-034): - Approvals said they "hold until you approve". `onPermissionRequest` was only ever called by `mock-runner.ts`, so in production they never fired. The Claude CLI engine now raises every gated tool call through a loopback HTTP MCP bridge and holds for the run's remaining wall-clock budget. `--permission-prompt-tool` is present in CLI 2.1.261, which falsifies ADR-020's premise; the contract matrix was re-run to prove it. - Docs said every run executes inside a macOS Seatbelt sandbox. The generator, symlink resolution, credential-collision refusal and the 13 escape checks were all real, but no production spawn passed a spec. Every engine spawn now routes through `applySandbox()`. `CW_SANDBOX=off` is the only way out and it is logged, journaled, and stamped on the report. - Docs said an interrupted run keeps its worktree. S-39 removed it whenever the agent had committed nothing, including on timeout. Pruning is now limited to runs that ended cleanly with no git operation in flight. Under `acceptEdits` the CLI runs Bash without consulting the prompt tool, so the deny-list floor is enforced by a PreToolUse hook, which fires in every permission mode. Also makes the cancel test deterministic. The daemon marks a run 'running' at spawn, before the child has run a line, so cancelling on that signal raced the child's first message and `sandboxed` was legitimately null. The test now waits for the containment stamp, so it interrupts a run that is genuinely under way. Test-only change; `sandboxedFor` semantics are unchanged. Suite: 260 passed, 37 files. Lint and typecheck clean. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/release.yml | 8 +- README.md | 18 +- decisions/DECISIONS.md | 7 + docs/install.md | 2 +- docs/security.md | 60 +++- package.json | 2 +- packages/daemon/package.json | 2 +- packages/daemon/src/run-manager.ts | 87 +++++- packages/daemon/src/runner-child.ts | 85 ++++- packages/daemon/src/runner-protocol.ts | 5 + packages/daemon/test/full-loop.test.ts | 18 ++ packages/runner/package.json | 2 +- packages/runner/src/api-agent-runner.ts | 32 +- packages/runner/src/claude-cli-runner.ts | 118 ++++++- packages/runner/src/codex-runner.ts | 22 +- packages/runner/src/floor-hook.ts | 187 +++++++++++ packages/runner/src/hermes-runner.ts | 23 +- packages/runner/src/index.ts | 2 + packages/runner/src/opencode-runner.ts | 14 +- packages/runner/src/permission-server.ts | 295 ++++++++++++++++++ packages/runner/src/safety-journal.ts | 4 +- packages/runner/src/sandbox.ts | 138 +++++++- packages/runner/src/worktree.ts | 40 +++ packages/runner/test/floor-hook.test.ts | 119 +++++++ .../runner/test/permission-server.test.ts | 202 ++++++++++++ .../runner/test/runner-env-wiring.test.ts | 38 +++ .../test/sandbox-production-spec.test.ts | 191 ++++++++++++ packages/runner/test/worktree-state.test.ts | 81 +++++ packages/shared/package.json | 2 +- packages/shared/src/report.ts | 17 + packages/shared/src/runner.ts | 7 + packages/ui/package.json | 2 +- packages/ui/src/components/InboxView.tsx | 15 + packaging/stage-release.sh | 40 ++- plan/STATUS.md | 6 +- .../T007-engine-contract-matrix-2.1.261.md | 58 ++++ src-tauri/Cargo.lock | 2 +- src-tauri/Cargo.toml | 2 +- src-tauri/tauri.conf.json | 2 +- 39 files changed, 1858 insertions(+), 97 deletions(-) create mode 100644 packages/runner/src/floor-hook.ts create mode 100644 packages/runner/src/permission-server.ts create mode 100644 packages/runner/test/floor-hook.test.ts create mode 100644 packages/runner/test/permission-server.test.ts create mode 100644 packages/runner/test/sandbox-production-spec.test.ts create mode 100644 packages/runner/test/worktree-state.test.ts create mode 100644 spikes/reports/T007-engine-contract-matrix-2.1.261.md diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ae759fb..89d2921 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -78,9 +78,13 @@ jobs: id: sum run: | cd src-tauri/target/release/bundle/dmg + # Stable-named copy so the landing page can point at + # /releases/latest/download/Clockwork_aarch64.dmg and never go stale. + # The versioned file stays: the Homebrew cask pins its sha256 to it. + cp Clockwork_*_aarch64.dmg Clockwork_aarch64.dmg shasum -a 256 *.dmg > checksums-sha256.txt cat checksums-sha256.txt - echo "dmg=$(ls *.dmg)" >> $GITHUB_OUTPUT + echo "dmg=$(ls Clockwork_[0-9]*_aarch64.dmg)" >> $GITHUB_OUTPUT - name: Create GitHub Release + upload assets (REST, no gh) if: startsWith(github.ref, 'refs/tags/') @@ -98,7 +102,7 @@ jobs: \`\`\` ### Install - 1. Download Clockwork_${VER}_aarch64.dmg (Apple Silicon) + 1. Download Clockwork_${VER}_aarch64.dmg (Apple Silicon; Clockwork_aarch64.dmg is the same file under a stable name) 2. Open the DMG and drag Clockwork to Applications 3. Launch — pair with your local daemon using ~/.clockwork/api-token diff --git a/README.md b/README.md index 6414828..bb4c754 100644 --- a/README.md +++ b/README.md @@ -239,11 +239,19 @@ Full list: [docs/SHORTCUTS.md](docs/SHORTCUTS.md) ## ⚖️ Security Model - **Isolation:** each run gets a fresh git worktree + branch cut from base; - macOS Seatbelt (`sandbox-exec`) profile restricts writes to that worktree -- **Credential hygiene:** sanitized child environment; deny-list blocks reads - of `.ssh`, `.aws`, `.gnupg`, Keychains; secret masking in reports -- **Approvals:** sensitive tool calls pause the run; ~2-minute decision window, - then fail-safe auto-deny (unattended mode) — recorded for audit either way + a macOS Seatbelt (`sandbox-exec`) profile restricts writes to that worktree, + Clockwork-managed tool caches, and the engine's own state dirs — for every + engine, including the BYOK agent's shell. Turning it off (`CW_SANDBOX=off`) + is journaled and stamped on the report. +- **Credential hygiene:** sanitized child environment (allowlist, so + `SSH_AUTH_SOCK` and provider keys never reach the agent); the sandbox denies + reads of `.ssh`, `.aws`, `.gnupg`, gcloud, browser profiles, shell history; + secret masking in reports. Keychain *files* stay readable — Claude Code needs + its own OAuth item — see `docs/security.md` for why. +- **Approvals (Claude engine):** gated tool calls pause the run and **hold until + you answer or the run's wall-clock budget ends**, then fail-safe deny — recorded + for audit either way. Other engines have no permission hook; the sandbox is + their containment. - **Budgets:** USD soft cap + turn cap + wall-clock timeout enforced by the supervisor process, not by the model's self-restraint - **Local-only:** daemon binds 127.0.0.1; bearer token file is 0600; no diff --git a/decisions/DECISIONS.md b/decisions/DECISIONS.md index f01036e..7c6c4aa 100644 --- a/decisions/DECISIONS.md +++ b/decisions/DECISIONS.md @@ -183,3 +183,10 @@ ADR-style, append-only. Format: Decision → Context → Alternatives rejected **Alternatives rejected:** Electron fallback (heavier; not needed since nothing blocks on native yet); blocking all UI work on Rust installation (schedule risk for zero architectural delta). **Why:** The UI code is identical under both wrappers (fetch/EventSource only); Tauri contributes the window chrome, tray, autostart, updater — all packaging-time concerns. **Consequence:** Token handshake currently manual (file read) until Tauri injects it at spawn; tray/menubar surfaces land with the Tauri step; stack #4 FullCalendar replaced by a purpose-built week/month grid matching designs/DESIGN.md (bundle size + we control booking UX end-to-end; FullCalendar's recurring-event model fights our occurrence-ledger source of truth). + +## ADR-034 — Supersedes ADR-020: CLI keep-alive approvals via a loopback permission bridge; Seatbelt sandbox wired into production for every engine; S-39 narrowed to preserve interrupted worktrees +**Decision:** (a) The Claude CLI engine asks Clockwork before every gated tool call through `--permission-prompt-tool`, served by an HTTP MCP server hosted inside `runner-child` (`packages/runner/src/permission-server.ts`, zero deps). The hold is bounded by the run's remaining wall-clock budget, not a fixed window; `MCP_TOOL_TIMEOUT` and the per-server `timeout` are set to the same bound. (b) Every engine spawn — Claude, Codex, OpenCode, Hermes, and the BYOK agent's bash — is routed through `applySandbox()`; `runner-child` builds the spec with `buildSandboxSpec()`. The only way out is `CW_SANDBOX=off`, which is logged, written to the safety journal (`sandbox_disabled`), and stamped on the report (`sandboxed:false`). (c) Profile v2 admits the CLI's per-cwd work dir (`/tmp/claude-/`, pre-created) and its cwd-tracking file (`regex ^/private/tmp/claude-[0-9a-f]+-cwd$`), plus `SandboxSpec.writeRegexes` for engine staging files. (d) `runner-child` scrubs `CW_BYOK_KEY`/`CW_BYOK_BASE_URL` from its own env after reading them; the BYOK bash gets `buildRunEnv()` + the sandbox wrap. (e) S-39 prunes a worktree only when the run ended cleanly (`completed`/non-crash `failed`) AND the worktree is clean with no git operation in flight; otherwise it is preserved and the report says so (`worktreeState`). +**Context:** 2026-09-05, the day after the Product Hunt launch. Three public statements did not match the code: approvals "hold until you approve" (they never fired — `onPermissionRequest` was called only by `mock-runner.ts`); "runs execute inside a macOS Seatbelt sandbox" (`new ClaudeCliRunner()` at `runner-child.ts:67` passed no spec; the other engines had no hook; T-111's "productionized" meant generator + tests only); "the worktree is preserved" after a timeout (`removeWorktree` ran `git worktree remove --force` + `rmSync` whenever the agent committed nothing, including on `timed_out`). ADR-020's premise — `--permission-prompt-tool` absent — was verified against 2.1.238 and is false on the installed 2.1.261; ADR-020 itself required this re-run. Evidence: `spikes/reports/T007-engine-contract-matrix-2.1.261.md` (real runs: 100s hold honoured inside the sandbox; `/tmp` write and `~/.zsh_history` read denied with a live shell; OpenCode verified; Hermes 0.21.0 resolves `write_file` against `$HOME` even outside the sandbox — pre-existing, now loud; Codex unverifiable here due to a local `config.toml` error). +**Alternatives rejected:** stdio MCP bridge (the CLI spawns it INSIDE the sandbox with stdio owned by the CLI → needs a side channel and a profile allow for it); extending the fixed 120s window (still a lie about "holds"); `--strict-mcp-config` (drops the repo's own `.mcp.json` servers — behaviour change for existing tasks); falling back to an unsandboxed spawn when the profile is refused (fail-open); a blanket allow on `/tmp` for the CLI's cwd file (the regex admits one filename and was tested against near-misses). +**Why:** The launch's credibility argument is "check it rather than believe it". Every one of the three gaps was a missing call site in front of working, tested code — so the fix is wiring plus guards that read the sources (`runner-env-wiring.test.ts` "sandbox wiring") so the call sites cannot silently disappear again. +**Consequence:** T-003/T-201 unblocked for the CLI engine; the composer may offer `default` permission mode meaningfully. `docs/security.md`, README, `plan/STATUS.md` T-111 corrected to describe what ships. Open, surfaced not decided: the run inherits `HOME`, so `~/.claude/settings.json` `permissions.allow` rules pre-empt the prompt tool — `--setting-sources` (2.1.261) can pin what an unattended run loads. ADR-026…033 are cited in code but never written here; this entry takes 034 to avoid collision. Re-run the matrix on every observed CLI/engine version change (unchanged from ADR-020). diff --git a/docs/install.md b/docs/install.md index 61486f6..96311c8 100644 --- a/docs/install.md +++ b/docs/install.md @@ -72,7 +72,7 @@ trust this specific binary, so verify it first: ```bash # 1. Check the hash matches the published one -shasum -a 256 ~/Downloads/Clockwork_0.4.0_aarch64.dmg +shasum -a 256 ~/Downloads/Clockwork_0.5.0_aarch64.dmg curl -s https://clockwork.vmoksh-shah179.workers.dev/downloads/checksums-sha256.txt ``` diff --git a/docs/security.md b/docs/security.md index e2d5df3..0882a02 100644 --- a/docs/security.md +++ b/docs/security.md @@ -8,15 +8,38 @@ ### 1. OS sandbox = THE containment boundary Every run executes inside a per-run macOS Seatbelt profile -(`packages/runner/src/sandbox.ts`, versioned in source): +(`packages/runner/src/sandbox.ts`, versioned in source). Every engine spawn — +Claude, Codex, OpenCode, Hermes, and the BYOK agent's shell — is wrapped by +`applySandbox()`; `runner-child` builds the spec. A source-reading test +(`packages/runner/test/runner-env-wiring.test.ts`, "sandbox wiring") fails the +build if any runner stops calling it. + +> **Correction (2026-09-05, ADR-034).** Before this date the profile and its +> escape tests existed but no production run was inside one — the runner was +> constructed without a spec. If you installed an earlier build, your runs +> were contained by the run-env allowlist and worktree isolation only. This +> document now describes what ships. **Enforced:** - **Writes are default-denied.** A run may write only to: - its own worktree or scratch directory, - `/dev/null` (git requires it), - - scoped engine-state subpaths: `~/.claude/{projects,statsig,shell-snapshots,logs}`. + - Clockwork-managed tool caches under `~/.clockwork/cache/` (npm, pnpm, + yarn, pip, XDG, cargo, go, gem/bundler, uv, poetry, gradle, composer, + nuget) — redirected there by env so package managers work without + opening `$HOME`, + - the Claude CLI's per-run work dir `/tmp/claude-/` and its + cwd-tracking file `/tmp/claude--cwd` (exact-name regex; without these + the CLI's Bash tool cannot start a shell), + - scoped engine-state subpaths: `~/.claude/{projects,statsig,shell-snapshots,logs}`, + and for the engine actually running: `~/.codex`, `~/.opencode` + opencode's + share/config/cache dirs, or `~/.hermes` (+ hermes's `$HOME/.hermes-tmp.` + staging file by exact-name regex). A run **cannot** modify global Claude config that future runs would load, cannot write to your home directory, other repos, or system locations. +- **Escape hatch is loud, never silent.** `CW_SANDBOX=off` disables the wrap + for a run; it is logged in the run's live log, written to the safety journal + as `sandbox_disabled`, and stamped on the report as `sandboxed: false`. - **Credential paths are explicitly unreadable:** `~/.ssh`, `~/.aws`, `~/.gnupg`, `~/.config/gcloud`, browser profiles, cookies, shell histories. Symlinks are resolved at profile-generation time; a context root pointing into @@ -75,10 +98,35 @@ jobs. Imported templates/profiles arrive disabled with a security preview. ## Engine capability honesty -The default engine (headless `claude -p`) has no permission-callback hook as of -CLI 2.1.238 (verified, ADR-020): M1 runs fail safe on permission blocks rather -than hanging. HITL approvals require the SDK engine (opt-in). Capability flags -drive the UI so it degrades honestly per engine. +The Claude engine (headless `claude -p`) asks Clockwork before each gated tool +call through `--permission-prompt-tool`, served by a loopback HTTP MCP server in +the runner (verified on CLI 2.1.261, ADR-034 — this replaces ADR-020's "no hook +as of 2.1.238"). A request **holds until a human answers or the run's wall-clock +budget ends**, then fail-safe denies; both outcomes are recorded. The tool is not +visible to the model, so the agent cannot approve itself. + +Codex, OpenCode and Hermes expose no permission hook; for them the sandbox and +budgets are the containment, and the UI hides approval affordances. There is no +"SDK engine". + +**Known gap, not yet closed — the policy floor only sees what the CLI asks +about.** Probed 2026-09-05 through the production runner on CLI 2.1.261 under +`acceptEdits`: `npm view left-pad version` was sent to the prompt tool; **`git +push --force origin main` was executed with no prompt at all** (transcript shows +the tool call and git's error; `permission_denials` empty). Clockwork's deny-list +would have refused it (`floor:true`, "force-push to protected branch") — it was +never consulted. The developer's `~/.claude/settings.json` had no matching allow +rule, so this is the CLI's own `acceptEdits` behaviour, not a settings leak. The +run also inherits `HOME`, so any `permissions.allow` rule in that file would be +honoured before the prompt tool too. + +Until this is closed, "a policy floor denies catastrophic commands" is true only +for commands the CLI chooses to prompt for. Candidate fixes, for the maker to +choose: run unattended tasks in `default` mode now that the bridge exists (every +non-allowlisted call is asked, so the floor evaluates everything — at the cost of +more prompts); inject the floor as CLI `permissions.deny` rules via `--settings`; +pin `--setting-sources` so a developer's interactive allow rules never apply to +an unattended run. ## Reporting a security issue diff --git a/package.json b/package.json index 2d112fe..c60b797 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "clockwork", - "version": "0.4.0", + "version": "0.5.0", "private": true, "description": "The calendar where your agents show up for work.", "packageManager": "pnpm@11.22.0", diff --git a/packages/daemon/package.json b/packages/daemon/package.json index e9e1fbf..1e639ae 100644 --- a/packages/daemon/package.json +++ b/packages/daemon/package.json @@ -1,6 +1,6 @@ { "name": "@clockwork/daemon", - "version": "0.4.0", + "version": "0.5.0", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/daemon/src/run-manager.ts b/packages/daemon/src/run-manager.ts index 86775ec..774e1b3 100644 --- a/packages/daemon/src/run-manager.ts +++ b/packages/daemon/src/run-manager.ts @@ -24,6 +24,7 @@ import { diffStat, hasCommitsBeyondBase, preflightRepo, + inspectWorktree, pruneBranch, removeWorktree, runGit, @@ -215,6 +216,7 @@ export class RunManager { LANG: process.env.LANG ?? 'en_US.UTF-8', CW_ENGINE: process.env.CW_ENGINE ?? '', // test hook only ...(process.env.CW_MOCK_STEP_MS ? { CW_MOCK_STEP_MS: process.env.CW_MOCK_STEP_MS } : {}), // test hook + ...(process.env.CW_SANDBOX ? { CW_SANDBOX: process.env.CW_SANDBOX } : {}), // escape hatch; journaled + stamped on the report ...this.byokEnv(spec), // BYOK credential injection (ADR-027/028) — keychain read happens here, in the daemon ...(process.env.USER ? { USER: process.env.USER } : {}), ...(process.env.LOGNAME ? { LOGNAME: process.env.LOGNAME } : {}), @@ -348,16 +350,31 @@ export class RunManager { this.deps.broadcast({ type: 'usage.updated', window: kind, at: now }); break; } + case 'sandbox': { + this.recordEvent(now, runId, 'sandbox_status', { enabled: msg.enabled, profileVersion: msg.profileVersion }); + if (!msg.enabled) this.deps.safetyJournal.record('sandbox_disabled', 'CW_SANDBOX=off', runId); + break; + } + case 'floor': { + // PreToolUse policy-floor hit (FR-11/T-114): denied outside the normal + // permission flow, so it gets its own event + safety-journal entry + // rather than riding the 'permission'/approval path above. + this.recordEvent(now, runId, 'policy_deny', { tool: msg.tool, command: msg.command.slice(0, 300), reason: msg.reason }); + this.deps.safetyJournal.record('deny_list_hit', `${msg.tool}: ${msg.reason}`, runId); + break; + } case 'permission': { - // Record the request; the child holds its callback open for ~2 min. - // A human decision (POST /approvals/:id/respond → respondToChild) - // reaches the live run; otherwise the child fail-safe auto-denies - // (M1 unattended mode, ADR-020) and finalize resolves the row. + // Record the request; the child holds its callback open until a human + // answers or the run's wall-clock budget ends (mirrors runner-child). + // A decision (POST /approvals/:id/respond → respondToChild) reaches the + // live run; otherwise the child fail-safe denies and finalize resolves the row. const approvalId = newId(); const reqId = (msg as { reqId?: string }).reqId ?? null; + const startedAt = this.getRun(runId)?.started_at ?? now; + const timeoutAt = Math.max(now + 5_000, startedAt + spec.budget.timeoutSec * 1000); this.deps.db .prepare(`INSERT INTO approvals (id, run_id, kind, payload_json, requested_at, timeout_at, fallback) VALUES (?, ?, 'permission', ?, ?, ?, ?)`) - .run(approvalId, runId, JSON.stringify({ tool: msg.tool, reqId }), now, now + 120_000, 'deny-and-continue'); + .run(approvalId, runId, JSON.stringify({ tool: msg.tool, reqId }), now, timeoutAt, 'deny-and-continue'); this.recordEvent(now, runId, 'approval_requested', { tool: msg.tool, reqId }); this.deps.broadcast({ type: 'approval.requested', approvalId, runId, at: now }); break; @@ -397,6 +414,42 @@ export class RunManager { } } + // S-39, narrowed (2026-09-05): "analysis-only runs leave no litter" applies + // only when the run ENDED cleanly and the worktree IS clean. A run killed by + // timeout, budget, cancel or crash — or one that left uncommitted work or a + // rebase/merge in flight — keeps its worktree for the next run or the human + // to recover. Before this, `git worktree remove --force` erased exactly the + // half-done state a timed-out rebase leaves behind. + let worktreeState: RunReport['worktreeState'] = null; + if (spec.repoPath && r.worktree_path) { + const inspected = inspectWorktree(r.worktree_path); + if (inspected.exists) { + const interrupted = + outcome.state === 'timed_out' || + outcome.state === 'budget_exceeded' || + outcome.state === 'cancelled' || + (outcome.state === 'failed' && ('failureReason' in outcome ? outcome.failureReason : undefined) === 'runner_crashed'); + const reason = committedSomething + ? 'committed' + : interrupted + ? 'interrupted' + : inspected.interruptedOp + ? 'in_progress_op' + : inspected.dirty + ? 'dirty' + : null; + if (reason === null && r.branch) { + try { + removeWorktree(spec.repoPath, r.worktree_path); + pruneBranch(spec.repoPath, r.branch); + } catch {} + worktreeState = { preserved: false, path: null, dirty: false, interruptedOp: null, reason: null }; + } else { + worktreeState = { preserved: true, path: r.worktree_path, dirty: inspected.dirty, interruptedOp: inspected.interruptedOp, reason }; + } + } + } + const report: RunReport = { runId, taskId: spec.taskId, @@ -412,6 +465,8 @@ export class RunManager { baseSha: null, basedOnLocalState: false, committedSomething, + sandboxed: this.sandboxedFor(runId), + worktreeState, diffStat: diffRows, artifacts: outcome.artifacts ?? [], transcriptPath: outcome.transcriptPath ?? null, @@ -437,14 +492,6 @@ export class RunManager { repoLockDelayMs: 0, }; - // S-39: analysis-only runs → prune branch immediately - if (spec.repoPath && !committedSomething && r.branch && existsSync(r.worktree_path ?? '')) { - try { - removeWorktree(spec.repoPath, r.worktree_path!); - pruneBranch(spec.repoPath, r.branch); - } catch {} - } - const tx = this.deps.db.transaction(() => { const terminalMap: Record = { completed: 'completed', @@ -758,6 +805,20 @@ export class RunManager { return rows.map((r) => r.occurrence_at); } + /** What the child reported before spawning its engine; null for runs that predate the protocol message. */ + private sandboxedFor(runId: string): boolean | null { + const row = this.deps.db + .prepare(`SELECT data_json FROM events WHERE run_id=? AND kind='sandbox_status' ORDER BY at DESC LIMIT 1`) + .get(runId) as { data_json: string } | undefined; + if (!row) return null; + try { + const d = JSON.parse(row.data_json) as { enabled?: unknown }; + return typeof d.enabled === 'boolean' ? d.enabled : null; + } catch { + return null; + } + } + approvalsFor(runId: string): Array<{ id: string; kind: string; requestedAt: number }> { return (this.deps.db .prepare('SELECT id, kind, requested_at FROM approvals WHERE run_id=?') diff --git a/packages/daemon/src/runner-child.ts b/packages/daemon/src/runner-child.ts index 9f4c281..dbab59e 100644 --- a/packages/daemon/src/runner-child.ts +++ b/packages/daemon/src/runner-child.ts @@ -7,7 +7,20 @@ */ import { readFileSync } from 'node:fs'; import { createInterface } from 'node:readline'; -import { ClaudeCliRunner, MockRunner, CodexRunner, OpenCodeRunner, HermesRunner, evaluateCommand, evaluatePathRead } from '@clockwork/runner'; +import { + ClaudeCliRunner, + MockRunner, + CodexRunner, + OpenCodeRunner, + HermesRunner, + evaluateCommand, + evaluatePathRead, + buildSandboxSpec, + escapeRegexLiteral, + SANDBOX_PROFILE_VERSION, + type SandboxSpec, +} from '@clockwork/runner'; +import os from 'node:os'; import type { ChildToDaemon, DaemonToChild } from './runner-protocol.js'; import type { JobSpec, RunOutcome } from '@clockwork/shared'; @@ -18,6 +31,15 @@ if (!specPath || !nonce) { } const job = JSON.parse(readFileSync(specPath, 'utf8')) as JobSpec; +const startedAtMs = Date.now(); + +// BYOK credential (ADR-027/028) arrives in THIS process's env only. Read it once +// and scrub it, so nothing this child spawns — the agent's own shell included — +// can `echo $CW_BYOK_KEY`. Before this scrub, it could. +const byokKey = process.env.CW_BYOK_KEY ?? ''; +const byokBaseUrl = process.env.CW_BYOK_BASE_URL ?? ''; +delete process.env.CW_BYOK_KEY; +delete process.env.CW_BYOK_BASE_URL; function send(msg: ChildToDaemon): void { try { @@ -55,16 +77,49 @@ async function main(): Promise { // CW_MOCK_STEP_MS makes the deterministic mock observable on fast machines // (full-loop tests sample intermediate states). const stepMs = Number(process.env.CW_MOCK_STEP_MS ?? '0'); + + // Seatbelt containment for the engine process (FR-26). CW_SANDBOX=off is the + // only way out, and it is logged here, journaled by the daemon, and stamped + // on the report — never silent. + const sandboxOff = process.env.CW_SANDBOX === 'off'; + let sandbox: SandboxSpec | null = null; + if (sandboxOff) { + send({ t: 'log', line: '[sandbox] DISABLED by CW_SANDBOX=off — writes and credential reads are NOT contained for this run' }); + } else { + const home = os.homedir(); + sandbox = buildSandboxSpec({ + worktreePath: job.worktreePath, + scratchPath: job.scratchPath, + repoPath: job.repoPath, + contextRoots: job.profile?.contextRoots ?? [], + // Each engine keeps session state under $HOME; deny it and the engine + // fails to start (opencode hangs without ~/.opencode — probed 2026-09-05). + // Scoped to the engine actually running. + engineStatePaths: + job.engine === 'codex' + ? [`${home}/.codex`] + : job.engine === 'opencode' + ? [`${home}/.opencode`, `${home}/.local/share/opencode`, `${home}/.config/opencode`, `${home}/.cache/opencode`] + : job.engine === 'hermes' + ? [`${home}/.hermes`] + : [], + // hermes's write_file stages through $HOME/.hermes-tmp. then moves it; + // without this exact-name allow every file write fails (probed 2026-09-05). + engineWriteRegexes: job.engine === 'hermes' ? [`^${escapeRegexLiteral(home)}/\\.hermes-tmp\\.[0-9]+$`] : [], + }); + } + send({ t: 'sandbox', enabled: !sandboxOff, profileVersion: sandboxOff ? null : SANDBOX_PROFILE_VERSION }); + const runner = process.env.CW_ENGINE === 'mock' ? new MockRunner(stepMs > 0 ? { steps: [{ delayMs: stepMs }] } : {}) : job.engine === 'codex' - ? new CodexRunner() + ? new CodexRunner({ sandbox }) : job.engine === 'opencode' - ? new OpenCodeRunner() + ? new OpenCodeRunner({ sandbox }) : job.engine === 'hermes' - ? new HermesRunner() - : new ClaudeCliRunner(); + ? new HermesRunner({ sandbox }) + : new ClaudeCliRunner({ sandbox }); // FR-2a: live-reference file attachments resolved at execution time. let effectiveJob: JobSpec = job; @@ -86,6 +141,8 @@ async function main(): Promise { onHeartbeat: () => send({ t: 'heartbeat' }), onLog: (line: string) => send({ t: 'log', line }), onArtifact: (path: string) => send({ t: 'artifact', path }), + onPolicyDeny: (p: { tool: string; command: string; reason: string }) => + send({ t: 'floor', tool: p.tool, command: p.command, reason: p.reason }), onPermissionRequest: async (p: { tool: string; input: unknown }) => { // deny-list floor FIRST (policy layer, FR-11); floor hits are never approvable const input = (p.input ?? {}) as Record; @@ -102,13 +159,16 @@ async function main(): Promise { send({ t: 'permission', reqId, tool: p.tool, input: p.input }); return new Promise<{ behavior: 'allow' } | { behavior: 'deny'; message: string }>((resolve) => { pendingPermissions.set(reqId, resolve); - // M1 fail-safe (ADR-020): CLI engine cannot hold approvals — auto-deny after grace. + // Hold until a human answers or the run's own wall-clock budget ends. + // The old fixed 120s window (ADR-020) existed because CLI 2.1.238 had no + // way to wait; 2.1.261 does, so the run's timeout is the only bound. + const remainingMs = Math.max(5_000, job.budget.timeoutSec * 1000 - (Date.now() - startedAtMs)); setTimeout(() => { if (pendingPermissions.has(reqId)) { pendingPermissions.delete(reqId); - resolve({ behavior: 'deny', message: 'No human reachable in unattended mode (M1 fail-safe).' }); + resolve({ behavior: 'deny', message: "No human answered before the run's wall-clock budget ended (fail-safe deny)." }); } - }, 120_000); + }, remainingMs); }); }, }; @@ -127,20 +187,19 @@ async function main(): Promise { // (CW_BYOK_KEY / CW_BYOK_BASE_URL), injected by the daemon at spawn time; // it is never written to the jobspec file or logs. const { runApiAgent } = await import('@clockwork/runner'); - const apiKey = process.env.CW_BYOK_KEY ?? ''; - const baseUrl = process.env.CW_BYOK_BASE_URL ?? ''; - if (!apiKey || !baseUrl) { + if (!byokKey || !byokBaseUrl) { outcome = { state: 'failed', failureReason: 'auth', summary: 'BYOK credential not provided to runner', artifacts: [], costUsd: 0, turns: 0 }; } else { const r = await runApiAgent({ - baseUrl, - apiKey, + baseUrl: byokBaseUrl, + apiKey: byokKey, model: job.model || 'default', systemPrompt: job.profile?.systemPromptExtra ?? 'You are a helpful autonomous agent working in a repository workspace.', prompt: effectiveJob.prompt, cwd: job.worktreePath || job.scratchPath || process.cwd(), maxTurns: job.budget.maxTurns, timeoutSec: job.budget.timeoutSec, + sandbox, onLog: (line: string) => send({ t: 'log', line }), }); // Stream usage as it lands (same channel CLI engines use). diff --git a/packages/daemon/src/runner-protocol.ts b/packages/daemon/src/runner-protocol.ts index e17891d..1b0c6d4 100644 --- a/packages/daemon/src/runner-protocol.ts +++ b/packages/daemon/src/runner-protocol.ts @@ -15,6 +15,11 @@ export type ChildToDaemon = | { t: 'artifact'; path: string } | { t: 'rateLimit'; info: Record } | { t: 'permission'; reqId: string; tool: string; input: unknown } + /** Sent once before the engine spawns; enabled=false is the CW_SANDBOX=off escape hatch. */ + | { t: 'sandbox'; enabled: boolean; profileVersion: number | null } + /** A PreToolUse policy-floor hit (FR-11/T-114) — the deny-list floor denied + * a command outside the normal permission flow; see safety-journal.ts. */ + | { t: 'floor'; tool: string; command: string; reason: string } | { t: 'outcome'; outcome: RunOutcome }; export type DaemonToChild = diff --git a/packages/daemon/test/full-loop.test.ts b/packages/daemon/test/full-loop.test.ts index 37e1057..c096a70 100644 --- a/packages/daemon/test/full-loop.test.ts +++ b/packages/daemon/test/full-loop.test.ts @@ -218,8 +218,26 @@ describe('full loop through child process (MockRunner engine)', () => { await waitFor(() => stateOf(runId) === 'running'); const row = db.prepare('SELECT pgid FROM runs WHERE id=?').get(runId) as any; expect(row.pgid).toBeGreaterThan(0); + // The daemon marks a run 'running' at SPAWN, before the child has executed a + // single line — so cancelling on that signal alone races the child's first + // message and `sandboxed` would be legitimately null (nothing was contained + // because no engine ever launched). Wait for the containment stamp so we are + // interrupting a run that is genuinely under way. + await waitFor(() => + db.prepare(`SELECT 1 FROM events WHERE run_id=? AND kind='sandbox_status'`).get(runId) !== undefined, + ); rm.cancel(runId); await waitFor(() => stateOf(runId) === 'cancelled'); + + // An interrupted run keeps its worktree even though it committed nothing — + // the S-39 prune is for runs that ENDED cleanly. Before 2026-09-05 this path + // force-deleted whatever a killed agent left behind. + const done = db.prepare('SELECT worktree_path, report_json FROM runs WHERE id=?').get(runId) as any; + expect(existsSync(done.worktree_path)).toBe(true); + const report = JSON.parse(done.report_json); + expect(report.worktreeState).toEqual({ preserved: true, path: done.worktree_path, dirty: false, interruptedOp: null, reason: 'interrupted' }); + expect(report.sandboxed).toBe(true); // runner-child reported its containment status before spawning + delete process.env.CW_MOCK_STEP_MS; delete process.env.CW_ENGINE; }, 60_000); diff --git a/packages/runner/package.json b/packages/runner/package.json index 9086b4f..f0aeb19 100644 --- a/packages/runner/package.json +++ b/packages/runner/package.json @@ -1,6 +1,6 @@ { "name": "@clockwork/runner", - "version": "0.4.0", + "version": "0.5.0", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/runner/src/api-agent-runner.ts b/packages/runner/src/api-agent-runner.ts index 55d9fba..018e785 100644 --- a/packages/runner/src/api-agent-runner.ts +++ b/packages/runner/src/api-agent-runner.ts @@ -9,6 +9,8 @@ * request only — never logged or persisted. */ import { writeFileSync } from 'node:fs'; +import { buildRunEnv } from './run-env.js'; +import { applySandbox, toolCacheEnv, type SandboxSpec } from './sandbox.js'; export interface ApiAgentJob { baseUrl: string; @@ -19,6 +21,8 @@ export interface ApiAgentJob { cwd: string; maxTurns: number; timeoutSec: number; + /** Seatbelt spec for the agent's shell. null = CW_SANDBOX=off (caller logs it). */ + sandbox?: SandboxSpec | null; onLog?: (line: string) => void; } @@ -87,7 +91,7 @@ export async function runApiAgent(job: ApiAgentJob): Promise { messages.push(choice as { role: string; content: string | null }); for (const tc of choice.tool_calls) { - const result = await execTool(tc.function.name, JSON.parse(tc.function.arguments || '{}'), job.cwd); + const result = await execTool(tc.function.name, JSON.parse(tc.function.arguments || '{}'), job.cwd, job.sandbox ?? null); log(`[api-agent] tool ${tc.function.name} → ${result.slice(0, 120).replace(/\n/g, ' ')}`); messages.push({ role: 'tool', tool_call_id: tc.id, name: tc.function.name, content: result.slice(0, 12_000) }); } @@ -118,14 +122,22 @@ const TOOLS = [ }, ]; -async function execTool(name: string, args: Record, cwd: string): Promise { +async function execTool(name: string, args: Record, cwd: string, sandbox: SandboxSpec | null): Promise { const { execFile } = await import('node:child_process'); const { promisify } = await import('node:util'); const run = promisify(execFile); if (name === 'run_command') { const command = args.command ?? ''; try { - const { stdout } = await run('/bin/bash', ['-c', command], { cwd, timeout: 120_000, maxBuffer: 4 * 1024 * 1024 }); + // The shell gets the same allowlisted env and Seatbelt wrap as every CLI + // engine. Before this it inherited process.env — including the BYOK key. + const wrapped = applySandbox(['/bin/bash', '-c', command], sandbox).argv; + const { stdout } = await run(wrapped[0]!, wrapped.slice(1), { + cwd, + env: buildRunEnv(toolCacheEnv()), + timeout: 120_000, + maxBuffer: 4 * 1024 * 1024, + }); return stdout || '(no output)'; } catch (e) { const err = e as { stdout?: string; stderr?: string; message?: string }; @@ -134,9 +146,17 @@ async function execTool(name: string, args: Record, cwd: string) } if (name === 'write_file') { const path = await import('node:path'); - const target = path.resolve(cwd, args.path ?? 'untitled.txt'); - if (!target.startsWith(path.resolve(cwd))) return 'ERROR: path escapes workspace'; - writeFileSync(target, args.content ?? ''); + const { existsSync, realpathSync } = await import('node:fs'); + // This write happens in the unsandboxed runner-child, so the prefix check + // IS the boundary. Resolve symlinks first: a link inside the worktree that + // points at ~/.ssh/authorized_keys passes a plain string-prefix test. + const root = realpathSync(cwd); + const target = path.resolve(root, args.path ?? 'untitled.txt'); + let probe = target; + while (!existsSync(probe)) probe = path.dirname(probe); + const real = path.join(realpathSync(probe), path.relative(probe, target)); + if (real !== root && !real.startsWith(root + path.sep)) return 'ERROR: path escapes workspace'; + writeFileSync(real, args.content ?? ''); return `wrote ${args.path}`; } return `ERROR: unknown tool ${name}`; diff --git a/packages/runner/src/claude-cli-runner.ts b/packages/runner/src/claude-cli-runner.ts index 9ac0181..e774323 100644 --- a/packages/runner/src/claude-cli-runner.ts +++ b/packages/runner/src/claude-cli-runner.ts @@ -13,6 +13,7 @@ import { execFileSync } from 'node:child_process'; import { appendFileSync, mkdtempSync, + rmSync, statfsSync, writeFileSync, } from 'node:fs'; @@ -27,7 +28,10 @@ import type { } from '@clockwork/shared'; import { BudgetGuard } from './budget-guard.js'; import { fold, newAccumulator, parseStreamLine } from './stream-parser.js'; -import { generateSeatbeltProfile, wrapWithSandbox, type SandboxSpec } from './sandbox.js'; +import { applySandbox, toolCacheEnv, type SandboxSpec } from './sandbox.js'; +import { PermissionServer } from './permission-server.js'; +import { evaluateCommand } from './deny-list.js'; +import { writeFloorHook, floorHookCommand, floorHookSettings } from './floor-hook.js'; import { buildRunEnv } from './run-env.js'; const GRACE_MS = 30_000; @@ -35,8 +39,13 @@ const DEFAULT_DISK_FLOOR_BYTES = 2 * 1024 * 1024 * 1024; // S-88 export interface CliRunnerOptions { claudeBin?: string; - /** when provided, spawn inside sandbox-exec with this spec */ + /** Seatbelt spec for the spawn. null = CW_SANDBOX=off escape hatch (caller logs it). */ sandbox?: SandboxSpec | null; + /** + * Host the permission bridge and point the CLI at it (default true). Off only + * for tests that drive a fake binary and assert exact argv. + */ + permissionBridge?: boolean; diskFloorBytes?: number; /** injectable clock for tests */ now?: () => number; @@ -112,24 +121,97 @@ export class ClaudeCliRunner implements AgentRunner { argv.push('--max-turns', String(job.budget.maxTurns)); } - let fullArgv = [this.bin, ...argv]; - let sandboxProfilePath: string | null = null; - if (this.opts.sandbox) { + // Permission bridge: the CLI asks Clockwork before every gated tool call and + // WAITS for the answer — the hold is bounded by the run's wall-clock, not a + // fixed window. Contract verified on CLI 2.1.261; see permission-server.ts. + const timeoutMs = job.budget.timeoutSec * 1000; + let bridge: PermissionServer | null = null; + // Temp dirs created for this run (the cw-mcp-* config dir, the cw-sb-* + // sandbox profile dir). Removed once the child is truly done with them — + // AFTER the bridge closes — so a held approval socket never gets orphaned + // mid-read. Never left to accumulate across runs (T-114/S-88 hygiene). + const tempDirs: string[] = []; + const cleanupTempDirs = (): void => { + for (const d of tempDirs.splice(0, tempDirs.length)) { + try { + rmSync(d, { recursive: true, force: true }); + } catch { + /* best-effort; a leaked temp dir is not a run failure */ + } + } + }; + if (this.opts.permissionBridge !== false) { + bridge = new PermissionServer({ + log: (l) => ctx.io.onLog(l), + decide: async ({ toolName, input }) => { + const d = await ctx.io.onPermissionRequest({ tool: toolName, input }); + if (d === 'ESCALATE') return { behavior: 'deny', message: 'Clockwork: escalation is not available for this run.' }; + return d.behavior === 'allow' ? { behavior: 'allow', updatedInput: input } : d; + }, + // Deny-list policy floor (FR-11/T-114): consulted over /floor by the + // PreToolUse hook below for EVERY Bash call, in every + // --permission-mode — unlike `decide` above, which acceptEdits mode + // never calls for Bash (verified live on CLI 2.1.261, 2026-09-05: + // `git push --force origin main` executed unasked). Only a genuine + // FLOOR hit denies here; an ordinary (non-floor) deny-list hit still + // goes through the normal permission flow via `decide`. + floor: ({ toolName, input }) => { + const record = (input ?? {}) as Record; + if (typeof record.command !== 'string') return { denied: false }; + const v = evaluateCommand(record.command); + if (v.floor) { + const reason = v.reason ?? 'blocked by global policy floor'; + ctx.io.onPolicyDeny?.({ tool: toolName, command: record.command, reason }); + ctx.io.onLog(`[floor] denied: ${reason}`); + } + return { denied: v.floor, reason: v.reason }; + }, + }); try { - const { profile } = generateSeatbeltProfile(this.opts.sandbox); - sandboxProfilePath = path.join(mkdtempSync(path.join(os.tmpdir(), 'cw-sb-')), 'profile.sb'); - writeFileSync(sandboxProfilePath, profile, 'utf8'); - fullArgv = wrapWithSandbox(fullArgv, sandboxProfilePath); + await bridge.start(); } catch (e) { - ctx.io.onLog(`[sandbox] profile generation failed: ${String(e)}`); - return fail('failed', 'internal', now() - startedAtMs, journalPath, String(e)); + return fail('failed', 'internal', now() - startedAtMs, journalPath, `permission bridge failed to start: ${String(e)}`); } + // Kept out of the worktree so it never appears in the run's diffstat. + const mcpDir = mkdtempSync(path.join(os.tmpdir(), 'cw-mcp-')); + tempDirs.push(mcpDir); + const mcpPath = path.join(mcpDir, 'permissions.json'); + writeFileSync(mcpPath, JSON.stringify(bridge.mcpConfig(timeoutMs)), 'utf8'); + argv.push('--permission-prompts', 'host', '--permission-prompt-tool', bridge.toolFlag, '--mcp-config', mcpPath); + + // PreToolUse hook (T-114): the CLI runs this for EVERY Bash call, before + // it would ever reach the MCP tool above, regardless of + // --permission-mode. Closes the acceptEdits gap: without it the deny-list + // floor above is only asked for tool calls the CLI itself gates. + const hookPath = writeFloorHook(mcpDir, bridge.floorUrl!); + const hookCommand = floorHookCommand(process.execPath, hookPath); + argv.push('--settings', floorHookSettings(hookCommand)); + } + + // Seatbelt wrap. A refused spec (credential path in the allowlist) fails the + // run; it never degrades to an unsandboxed spawn. + let fullArgv: string[]; + try { + const sandboxResult = applySandbox([this.bin, ...argv], this.opts.sandbox); + fullArgv = sandboxResult.argv; + if (sandboxResult.profilePath) tempDirs.push(path.dirname(sandboxResult.profilePath)); + } catch (e) { + void bridge?.close(); + cleanupTempDirs(); + ctx.io.onLog(`[sandbox] profile generation failed: ${String(e)}`); + return fail('failed', 'internal', now() - startedAtMs, journalPath, String(e)); } // Sanitized env: only what Node + the CLI genuinely need (arch §7.3). // The allowlist itself lives in run-env.ts — see that file for why this is - // a security boundary and not a convenience. - const env = buildRunEnv({ SHELL: '/bin/zsh' }); + // a security boundary and not a convenience. MCP_TOOL_TIMEOUT lifts the + // CLI's 60s HTTP default so a held approval survives until the run's own + // timeout; the cache vars keep package managers inside the sandbox. + const env = buildRunEnv({ + SHELL: '/bin/zsh', + MCP_TOOL_TIMEOUT: String(Math.max(60_000, timeoutMs)), + ...toolCacheEnv(), + }); const child = spawn(fullArgv[0]!, fullArgv.slice(1), { cwd: ctx.worktreePath, @@ -138,6 +220,8 @@ export class ClaudeCliRunner implements AgentRunner { stdio: ['ignore', 'pipe', 'pipe'], }); if (!child.pid) { + void bridge?.close(); + cleanupTempDirs(); return fail('failed', 'runner_crashed', now() - startedAtMs, journalPath, 'spawn failed'); } const pgid = child.pid; // detached => pgid == child pid @@ -235,6 +319,10 @@ export class ClaudeCliRunner implements AgentRunner { clearInterval(heartbeat); clearInterval(diskTimer); clearTimeout(timeoutTimer); + void (async () => { + await bridge?.close(); + cleanupTempDirs(); // AFTER the bridge closes — never mid-read of a held socket + })(); resolve(fail('failed', 'runner_crashed', now() - startedAtMs, journalPath, String(err))); }); @@ -242,6 +330,10 @@ export class ClaudeCliRunner implements AgentRunner { clearInterval(heartbeat); clearInterval(diskTimer); clearTimeout(timeoutTimer); + void (async () => { + await bridge?.close(); // drops any still-held approval socket + cleanupTempDirs(); // AFTER the bridge closes — never mid-read of a held socket + })(); guard.finalize(acc.totalCostUsd); const base = { diff --git a/packages/runner/src/codex-runner.ts b/packages/runner/src/codex-runner.ts index fcd5256..a30e6ec 100644 --- a/packages/runner/src/codex-runner.ts +++ b/packages/runner/src/codex-runner.ts @@ -11,6 +11,7 @@ import path from 'node:path'; import { BudgetGuard } from './budget-guard.js'; import { classifyError } from './stream-parser.js'; import { buildRunEnv } from './run-env.js'; +import { applySandbox, toolCacheEnv, type SandboxSpec } from './sandbox.js'; import type { AgentRunner, JobContext, @@ -34,7 +35,7 @@ export class CodexRunner implements AgentRunner { readonly engine = 'codex' as const; private livePgids = new Set(); - constructor(private readonly opts: { graceMs?: number } = {}) {} + constructor(private readonly opts: { graceMs?: number; sandbox?: SandboxSpec | null } = {}) {} async start(job: JobSpecLike, ctx: JobContext): Promise { return this.execute(job, ctx); @@ -61,18 +62,31 @@ export class CodexRunner implements AgentRunner { { onLog: (l) => ctx.io.onLog(l) }, ); + // Exactly one Seatbelt layer. macOS refuses to apply codex's own + // `workspace-write` profile inside Clockwork's deny-default profile + // (`sandbox_apply: Operation not permitted`, probed 2026-09-05), so when + // ours is on, codex's is off and ours is the containment. Only with + // CW_SANDBOX=off does codex fall back to its own sandbox. + const innerSandbox = this.opts.sandbox ? 'danger-full-access' : 'workspace-write'; const argv = [ 'exec', '--json', '--skip-git-repo-check', '-s', - 'workspace-write', + innerSandbox, ...(job.model ? ['-c', `model="${job.model}"`] : []), buildPrompt(job), ]; - const env = buildRunEnv(); + const env = buildRunEnv(toolCacheEnv()); - const child: ChildProcess = spawn('codex', argv, { + let wrapped: string[]; + try { + wrapped = applySandbox(['codex', ...argv], this.opts.sandbox).argv; + } catch (e) { + resolve({ state: 'failed', failureReason: 'internal', summary: `sandbox profile refused: ${String(e)}`, artifacts: [], costUsd: 0, turns: 0 }); + return; + } + const child: ChildProcess = spawn(wrapped[0]!, wrapped.slice(1), { cwd: path.join(ctx.worktreePath), env, detached: true, diff --git a/packages/runner/src/floor-hook.ts b/packages/runner/src/floor-hook.ts new file mode 100644 index 0000000..916a772 --- /dev/null +++ b/packages/runner/src/floor-hook.ts @@ -0,0 +1,187 @@ +/** + * The Claude Code `PreToolUse` hook that closes the gap the MCP permission + * bridge cannot: under `--permission-mode acceptEdits`, the CLI executes Bash + * WITHOUT ever calling `--permission-prompt-tool` (verified live on CLI + * 2.1.261, 2026-09-05 — `git push --force origin main` ran unasked). A + * PreToolUse hook, by contrast, is invoked by the CLI for every Bash call in + * every permission mode, so this is where the deny-list floor (FR-11, + * deny-list.ts `evaluateCommand`) actually gets consulted for every command. + * + * Shared by the runner (writes+wires this per run) and its test (spawns the + * exact file the runner would spawn) so the two can never silently drift. + * + * FAIL-CLOSED STANCE, spelled out because it is the entire point of this + * file: Claude Code's PreToolUse contract is exit 0 = continue, exit 2 = + * deny — but ANY OTHER outcome (a non-2/0 exit code, an uncaught exception, a + * hang past the CLI's own hook timeout) is FAIL-OPEN: the tool call proceeds + * as if nothing had answered. A future CLI format change (a different stdin + * shape, different exit-code semantics) must therefore break every run + * LOUDLY — every Bash call denied with "policy floor unreachable" — never + * silently stop protecting them. Every code path below converges on exit(2) + * with a reason on stderr; nothing exits 0 except the one explicit allow. + * + * NO imports beyond node:http in the generated file: it runs inside the + * Seatbelt sandbox, spawned by the packaged Claude Code CLI, and must never + * resolve a `dist/` (or any other) path from this package. + */ +import { writeFileSync } from 'node:fs'; +import path from 'node:path'; + +/** POST timeout to the bridge — generous for a loopback call, short enough + * that a wedged bridge is caught well inside the CLI's own hook timeout. */ +const POST_TIMEOUT_MS = 5_000; + +/** Self-watchdog: fires before the CLI's default 60s hook timeout could ever + * let a hang fail open, and comfortably outlasts POST_TIMEOUT_MS plus + * process startup. */ +const WATCHDOG_MS = 7_000; + +/** + * The hook's own source, templated with the bridge's `/floor` URL so the + * generated file needs no environment variables at all. + */ +export function floorHookSource(floorUrl: string): string { + return `// Clockwork policy-floor PreToolUse hook — generated per run, do not edit. +// +// FAIL-CLOSED: exit 0 = continue, exit 2 = deny (Claude Code PreToolUse +// contract). Anything else — a different exit code, an uncaught exception, a +// hang — is FAIL-OPEN on the CLI side, so every path here ends in exit(2) +// unless the bridge explicitly allowed the call. +import http from 'node:http'; + +const FLOOR_URL = ${JSON.stringify(floorUrl)}; +const POST_TIMEOUT_MS = ${POST_TIMEOUT_MS}; +const WATCHDOG_MS = ${WATCHDOG_MS}; + +function deny(reason) { + process.stderr.write(String(reason) + '\\n'); + process.exit(2); +} + +// A hang anywhere below (stdin never closes, a socket wedges past its own +// timeout) must not silently fail open just because this process never +// reaches its own exit() call. Deliberately left REF'd (not .unref()'d): an +// unref'd timer means "don't keep the process alive for this" — exactly the +// fail-OPEN this file exists to prevent if some unforeseen path leaves the +// process idle without ever reaching exit(). The allow path clearTimeout()s +// it explicitly; every other path calls process.exit() before it would fire. +const watchdog = setTimeout(() => { + deny('Clockwork policy floor unreachable: hook watchdog expired'); +}, WATCHDOG_MS); + +process.on('uncaughtException', (e) => { + deny('Clockwork policy floor unreachable: ' + (e && e.message ? e.message : String(e))); +}); +process.on('unhandledRejection', (e) => { + deny('Clockwork policy floor unreachable: ' + (e && e.message ? e.message : String(e))); +}); + +let raw = ''; +process.stdin.setEncoding('utf8'); +process.stdin.on('data', (chunk) => { + raw += chunk; +}); +process.stdin.on('error', (e) => { + deny('Clockwork policy floor unreachable: stdin error: ' + e.message); +}); +process.stdin.on('end', () => { + let input; + try { + input = JSON.parse(raw); + } catch (e) { + deny('Clockwork policy floor unreachable: malformed hook input (' + e.message + ')'); + return; + } + const toolName = input && typeof input.tool_name === 'string' ? input.tool_name : 'unknown'; + const toolInput = input ? input.tool_input : undefined; + const body = JSON.stringify({ tool_name: toolName, tool_input: toolInput }); + + let settled = false; + const req = http.request( + FLOOR_URL, + { + method: 'POST', + headers: { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(body) }, + timeout: POST_TIMEOUT_MS, + }, + (res) => { + let resBody = ''; + res.setEncoding('utf8'); + res.on('data', (chunk) => { + resBody += chunk; + }); + res.on('end', () => { + if (settled) return; + settled = true; + if (res.statusCode !== 200) { + deny('Clockwork policy floor unreachable: HTTP ' + res.statusCode); + return; + } + let decision; + try { + decision = JSON.parse(resBody); + } catch (e) { + deny('Clockwork policy floor unreachable: malformed response body (' + e.message + ')'); + return; + } + if (decision && decision.decision === 'allow') { + clearTimeout(watchdog); + process.exit(0); + return; + } + if (decision && decision.decision === 'deny') { + deny(decision.reason || 'Clockwork policy floor: denied'); + return; + } + deny('Clockwork policy floor unreachable: unexpected response body'); + }); + }, + ); + req.on('timeout', () => { + if (settled) return; + settled = true; + req.destroy(); + deny('Clockwork policy floor unreachable: request timed out'); + }); + req.on('error', (e) => { + if (settled) return; + settled = true; + deny('Clockwork policy floor unreachable: ' + e.message); + }); + req.end(body); +}); +`; +} + +/** Write the hook file into `dir` (the same `cw-mcp-*` temp dir the run's + * MCP config lives in) and return its absolute path. */ +export function writeFloorHook(dir: string, floorUrl: string): string { + const hookPath = path.join(dir, 'floor-hook.mjs'); + writeFileSync(hookPath, floorHookSource(floorUrl), 'utf8'); + return hookPath; +} + +/** Shell-quote a path for embedding in the hook's `command` string. */ +function shellQuote(s: string): string { + return `'${s.replace(/'/g, `'\\''`)}'`; +} + +/** + * The shell command Claude Code invokes for the hook. Uses `process.execPath` + * (absolute path to the running Node binary) rather than `node` off PATH — + * the sandboxed CLI's PATH is not guaranteed to resolve one — and + * `--no-warnings` so a stray Node runtime warning can never land in the + * stderr the model reads as the deny reason. + */ +export function floorHookCommand(execPath: string, hookPath: string): string { + return `${shellQuote(execPath)} --no-warnings ${shellQuote(hookPath)}`; +} + +/** The `--settings` JSON payload wiring `command` in as a Bash PreToolUse hook. */ +export function floorHookSettings(command: string): string { + return JSON.stringify({ + hooks: { + PreToolUse: [{ matcher: 'Bash', hooks: [{ type: 'command', command }] }], + }, + }); +} diff --git a/packages/runner/src/hermes-runner.ts b/packages/runner/src/hermes-runner.ts index 9767cad..fe1d7ed 100644 --- a/packages/runner/src/hermes-runner.ts +++ b/packages/runner/src/hermes-runner.ts @@ -22,6 +22,7 @@ import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { buildRunEnv } from './run-env.js'; +import { applySandbox, toolCacheEnv, type SandboxSpec } from './sandbox.js'; import type { AgentRunner, JobContext, JobSpecLike, RunOutcome } from '@clockwork/shared'; const GRACE_MS = 30_000; @@ -54,7 +55,7 @@ export class HermesRunner implements AgentRunner { private livePgids = new Set(); constructor( - private readonly opts: { graceMs?: number; hermesBin?: string } = {}, + private readonly opts: { graceMs?: number; hermesBin?: string; sandbox?: SandboxSpec | null } = {}, ) {} async start(job: JobSpecLike, ctx: JobContext): Promise { @@ -81,6 +82,13 @@ export class HermesRunner implements AgentRunner { buildPrompt(job), '--cli', '--no-restore-cwd', + // `--in DIR` is hermes's documented way to pin the session directory, + // but hermes 0.21.0's oneshot (-z) path never applies it (main.py skips + // _apply_in_dir), so the agent's cwd fell back to $HOME and write_file + // landed there. TERMINAL_CWD in the env below is what oneshot actually + // honours (probed 2026-09-05); the flag stays for the day upstream fixes it. + '--in', + ctx.worktreePath, '--usage-file', usagePath, ]; @@ -89,9 +97,18 @@ export class HermesRunner implements AgentRunner { argv[1] = `${job.profile.systemPromptExtra}\n\n---\n\n${argv[1]}`; } const bin = this.opts.hermesBin ?? 'hermes'; - const child: ChildProcess = spawn(bin, argv, { + // The usage file lives in a temp dir outside the worktree; the sandbox must be told. + let wrapped: string[]; + try { + wrapped = applySandbox([bin, ...argv], this.opts.sandbox, { extraWritePaths: [path.dirname(usagePath)] }).argv; + } catch (e) { + cleanup(); + resolve({ state: 'failed', failureReason: 'internal', summary: `sandbox profile refused: ${String(e)}`, artifacts: [], costUsd: 0, turns: 0 }); + return; + } + const child: ChildProcess = spawn(wrapped[0]!, wrapped.slice(1), { cwd: ctx.worktreePath, - env: buildRunEnv({ HERMES_NONINTERACTIVE: '1' }), + env: buildRunEnv({ HERMES_NONINTERACTIVE: '1', TERMINAL_CWD: ctx.worktreePath, ...toolCacheEnv() }), detached: true, stdio: ['ignore', 'pipe', 'pipe'], }); diff --git a/packages/runner/src/index.ts b/packages/runner/src/index.ts index 2709971..8f9ebe2 100644 --- a/packages/runner/src/index.ts +++ b/packages/runner/src/index.ts @@ -2,6 +2,8 @@ export * from './worktree.js'; export * from './profile-materializer.js'; export * from './safety-journal.js'; export * from './sandbox.js'; +export * from './permission-server.js'; +export * from './floor-hook.js'; export * from './stream-parser.js'; export * from './deny-list.js'; export * from './budget-guard.js'; diff --git a/packages/runner/src/opencode-runner.ts b/packages/runner/src/opencode-runner.ts index c57520d..0822279 100644 --- a/packages/runner/src/opencode-runner.ts +++ b/packages/runner/src/opencode-runner.ts @@ -7,6 +7,7 @@ */ import { spawn, type ChildProcess } from 'node:child_process'; import { buildRunEnv } from './run-env.js'; +import { applySandbox, toolCacheEnv, type SandboxSpec } from './sandbox.js'; import type { AgentRunner, JobContext, JobSpecLike, RunOutcome } from '@clockwork/shared'; const GRACE_MS = 30_000; @@ -25,7 +26,7 @@ export class OpenCodeRunner implements AgentRunner { readonly engine = 'opencode' as const; private livePgids = new Set(); - constructor(private readonly opts: { graceMs?: number } = {}) {} + constructor(private readonly opts: { graceMs?: number; sandbox?: SandboxSpec | null } = {}) {} async start(job: JobSpecLike, ctx: JobContext): Promise { return this.execute(job, ctx); @@ -46,9 +47,16 @@ export class OpenCodeRunner implements AgentRunner { private execute(job: JobSpecLike, ctx: JobContext): Promise { return new Promise((resolve) => { const argv = ['run', buildPrompt(job)]; - const env = buildRunEnv(); + const env = buildRunEnv(toolCacheEnv()); - const child: ChildProcess = spawn('opencode', argv, { + let wrapped: string[]; + try { + wrapped = applySandbox(['opencode', ...argv], this.opts.sandbox).argv; + } catch (e) { + resolve({ state: 'failed', failureReason: 'internal', summary: `sandbox profile refused: ${String(e)}`, artifacts: [], costUsd: 0, turns: 0 }); + return; + } + const child: ChildProcess = spawn(wrapped[0]!, wrapped.slice(1), { cwd: ctx.worktreePath, env, detached: true, diff --git a/packages/runner/src/permission-server.ts b/packages/runner/src/permission-server.ts new file mode 100644 index 0000000..d65d134 --- /dev/null +++ b/packages/runner/src/permission-server.ts @@ -0,0 +1,295 @@ +/** + * Permission bridge: a loopback MCP server the Claude CLI consults before every + * gated tool call (`--permission-prompt-tool`). + * + * Why HTTP and not stdio: the CLI spawns stdio MCP servers itself, so a stdio + * bridge would run INSIDE the Seatbelt sandbox with its stdio owned by the CLI, + * and would need a second channel back to the supervisor. An HTTP server hosted + * here — in the unsandboxed runner-child process — is reached by the CLI over + * loopback (the profile allows network*), and requests land directly where the + * pending-approval map already lives. No extra process, no side channel. + * + * Verified against Claude Code CLI 2.1.261 on 2026-09-05 (spike: sandboxed + * claude, this server outside, one decision held 100s and then honoured): + * - tools/call arguments: { tool_name, input, tool_use_id } + * - response: text content whose body is JSON + * { behavior: 'allow', updatedInput } | { behavior: 'deny', message } + * - HTTP MCP requests time out at 60s by default; the per-server `timeout` + * in --mcp-config and MCP_TOOL_TIMEOUT both raise it. Both are set by the + * CLI runner to the run's wall-clock budget. + * - The tool is NOT exposed to the model, so the agent cannot approve itself. + * + * Zero dependencies on purpose: this package is the published security + * boundary and stays auditable without a lockfile. + */ +import http from 'node:http'; +import type { AddressInfo } from 'node:net'; + +export type PermissionDecision = + | { behavior: 'allow'; updatedInput?: unknown } + | { behavior: 'deny'; message: string }; + +export interface PermissionRequestPayload { + toolName: string; + input: unknown; + toolUseId: string | null; +} + +export interface PermissionServerOptions { + /** Resolves when a human (or the policy floor) has decided. May take as long as the run allows. */ + decide: (req: PermissionRequestPayload) => Promise; + /** MCP server name; becomes the `mcp____` prefix. */ + serverName?: string; + toolName?: string; + log?: (line: string) => void; + /** + * The deny-list policy floor (FR-11/T-114), consulted over `POST /floor` by + * the PreToolUse hook (floor-hook.ts) for EVERY Bash call, in every + * `--permission-mode` — unlike `decide` above, which acceptEdits mode skips + * for Bash entirely. Synchronous and wrapped in try/catch here: a throwing + * or missing floor callback must deny, never implicitly allow. + */ + floor?: (req: { toolName: string; input: unknown }) => { denied: boolean; reason?: string }; +} + +/** The `/floor` route's response body. */ +export interface FloorDecision { + decision: 'deny' | 'allow'; + reason?: string; +} + +interface JsonRpcRequest { + jsonrpc?: string; + id?: number | string | null; + method?: string; + params?: Record; +} + +const PROTOCOL_FALLBACK = '2025-03-26'; + +export class PermissionServer { + private server: http.Server | null = null; + private boundUrl: string | null = null; + private boundOrigin: string | null = null; + private readonly serverName: string; + private readonly toolName: string; + + constructor(private readonly opts: PermissionServerOptions) { + this.serverName = opts.serverName ?? 'clockwork'; + this.toolName = opts.toolName ?? 'approve'; + } + + /** The exact value the CLI expects for --permission-prompt-tool. */ + get toolFlag(): string { + return `mcp__${this.serverName}__${this.toolName}`; + } + + get url(): string | null { + return this.boundUrl; + } + + /** The `/floor` route's URL — the address the PreToolUse hook posts to. */ + get floorUrl(): string | null { + return this.boundOrigin ? `${this.boundOrigin}/floor` : null; + } + + /** + * Document for --mcp-config. `timeoutMs` must exceed the longest hold the run + * may need — the CLI's HTTP default is 60s and a slow human is the whole point. + */ + mcpConfig(timeoutMs: number): { mcpServers: Record } { + if (!this.boundUrl) throw new Error('PermissionServer.mcpConfig called before start()'); + return { mcpServers: { [this.serverName]: { type: 'http', url: this.boundUrl, timeout: Math.max(60_000, timeoutMs) } } }; + } + + async start(): Promise<{ port: number; url: string }> { + if (this.server) throw new Error('PermissionServer already started'); + const server = http.createServer((req, res) => void this.handle(req, res)); + // A held decision is a legitimately slow response; never let the socket idle-timeout kill it. + server.timeout = 0; + server.keepAliveTimeout = 30_000; + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, '127.0.0.1', () => resolve()); + }); + const { port } = server.address() as AddressInfo; + this.server = server; + this.boundOrigin = `http://127.0.0.1:${port}`; + this.boundUrl = `${this.boundOrigin}/mcp`; + return { port, url: this.boundUrl }; + } + + async close(): Promise { + const s = this.server; + this.server = null; + this.boundUrl = null; + this.boundOrigin = null; + if (!s) return; + await new Promise((resolve) => { + s.close(() => resolve()); + // Held requests keep sockets open; drop them so close() cannot hang a finalize. + s.closeAllConnections?.(); + }); + } + + private async handle(req: http.IncomingMessage, res: http.ServerResponse): Promise { + // Route on pathname only; every previous behaviour on /mcp is unchanged. + const pathname = new URL(req.url ?? '/', 'http://internal').pathname; + if (pathname === '/mcp') { + await this.handleMcp(req, res); + return; + } + if (pathname === '/floor') { + await this.handleFloor(req, res); + return; + } + res.writeHead(404).end(); + } + + private async handleMcp(req: http.IncomingMessage, res: http.ServerResponse): Promise { + if (req.method !== 'POST') { + res.writeHead(405, { Allow: 'POST' }).end(); + return; + } + let body = ''; + for await (const chunk of req) body += chunk; + let msg: JsonRpcRequest; + try { + msg = JSON.parse(body) as JsonRpcRequest; + } catch { + this.reply(res, { jsonrpc: '2.0', id: null, error: { code: -32700, message: 'parse error' } }); + return; + } + const { id, method, params } = msg; + + // Notifications carry no id and expect no body. + if (id === undefined || id === null || (method ?? '').startsWith('notifications/')) { + res.writeHead(202).end(); + return; + } + + switch (method) { + case 'initialize': { + const requested = typeof params?.protocolVersion === 'string' ? params.protocolVersion : PROTOCOL_FALLBACK; + this.reply(res, { + jsonrpc: '2.0', + id, + result: { + protocolVersion: requested, + capabilities: { tools: {} }, + serverInfo: { name: this.serverName, version: '1' }, + }, + }); + return; + } + case 'ping': + this.reply(res, { jsonrpc: '2.0', id, result: {} }); + return; + case 'tools/list': + this.reply(res, { + jsonrpc: '2.0', + id, + result: { + tools: [ + { + name: this.toolName, + description: 'Clockwork permission gate: asks the supervising human before a gated tool call runs.', + inputSchema: { + type: 'object', + properties: { + tool_name: { type: 'string' }, + input: { type: 'object' }, + tool_use_id: { type: 'string' }, + }, + required: ['tool_name', 'input'], + }, + }, + ], + }, + }); + return; + case 'tools/call': { + const name = typeof params?.name === 'string' ? params.name : ''; + if (name !== this.toolName) { + this.reply(res, { jsonrpc: '2.0', id, error: { code: -32602, message: `unknown tool ${name}` } }); + return; + } + const args = (params?.arguments ?? {}) as { tool_name?: unknown; input?: unknown; tool_use_id?: unknown }; + const payload: PermissionRequestPayload = { + toolName: typeof args.tool_name === 'string' ? args.tool_name : 'unknown', + input: args.input ?? {}, + toolUseId: typeof args.tool_use_id === 'string' ? args.tool_use_id : null, + }; + let decision: PermissionDecision; + try { + decision = await this.opts.decide(payload); + } catch (e) { + // A broken supervisor must never become an implicit allow. + decision = { behavior: 'deny', message: `Clockwork permission bridge error: ${String(e)}` }; + } + this.opts.log?.(`[permission] ${payload.toolName} -> ${decision.behavior}`); + this.reply(res, { + jsonrpc: '2.0', + id, + result: { content: [{ type: 'text', text: JSON.stringify(decision) }] }, + }); + return; + } + default: + this.reply(res, { jsonrpc: '2.0', id, error: { code: -32601, message: `method not found: ${method}` } }); + } + } + + /** + * The policy-floor route the PreToolUse hook posts to. Deliberately + * fail-closed: a malformed body, a missing `floor` callback, or a callback + * that throws all resolve to `deny` — never an implicit allow. Always + * answers 200 (the decision itself is the payload); only the HTTP method is + * validated before that. + */ + private async handleFloor(req: http.IncomingMessage, res: http.ServerResponse): Promise { + if (req.method !== 'POST') { + res.writeHead(405, { Allow: 'POST' }).end(); + return; + } + let body = ''; + for await (const chunk of req) body += chunk; + let parsed: { tool_name?: unknown; tool_input?: unknown }; + try { + parsed = JSON.parse(body) as { tool_name?: unknown; tool_input?: unknown }; + } catch (e) { + this.replyFloor(res, { decision: 'deny', reason: `malformed /floor request body: ${String(e)}` }); + return; + } + const toolName = typeof parsed.tool_name === 'string' ? parsed.tool_name : 'unknown'; + const input = parsed.tool_input; + if (!this.opts.floor) { + this.replyFloor(res, { decision: 'deny', reason: 'no policy floor configured' }); + return; + } + let verdict: { denied: boolean; reason?: string }; + try { + verdict = this.opts.floor({ toolName, input }); + } catch (e) { + // A broken deny-list evaluation must never become an implicit allow. + verdict = { denied: true, reason: `policy floor callback threw: ${String(e)}` }; + } + // Only the deny path is worth a log line — this route is hit for EVERY + // Bash call under acceptEdits, so logging every allow would flood the + // run's live log with routine chatter. claude-cli-runner.ts's floor + // callback logs its own deny separately (with the deny-list reason); this + // one also covers the malformed-body / no-callback / threw cases above. + if (verdict.denied) this.opts.log?.(`[floor] ${toolName} -> deny`); + this.replyFloor(res, verdict.denied ? { decision: 'deny', reason: verdict.reason } : { decision: 'allow' }); + } + + private replyFloor(res: http.ServerResponse, payload: FloorDecision): void { + const text = JSON.stringify(payload); + res.writeHead(200, { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(text) }).end(text); + } + + private reply(res: http.ServerResponse, payload: unknown): void { + const text = JSON.stringify(payload); + res.writeHead(200, { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(text) }).end(text); + } +} diff --git a/packages/runner/src/safety-journal.ts b/packages/runner/src/safety-journal.ts index 8f0ef4c..f085ee6 100644 --- a/packages/runner/src/safety-journal.ts +++ b/packages/runner/src/safety-journal.ts @@ -12,7 +12,9 @@ export type JournalKind = | 'budget_hard_stop' | 'approval_decision' | 'orphan_terminated' - | 'preflight_failure'; + | 'preflight_failure' + /** CW_SANDBOX=off escape hatch used for a run — loud by design. */ + | 'sandbox_disabled'; export interface JournalEntry { at: number; diff --git a/packages/runner/src/sandbox.ts b/packages/runner/src/sandbox.ts index b1020e5..1510962 100644 --- a/packages/runner/src/sandbox.ts +++ b/packages/runner/src/sandbox.ts @@ -14,11 +14,12 @@ * what it bounds is filesystem/process damage beyond the allowlist. This is * documented in docs/security.md, never marketed away. */ -import { realpathSync, existsSync } from 'node:fs'; +import { realpathSync, existsSync, mkdirSync, mkdtempSync, writeFileSync } from 'node:fs'; import path from 'node:path'; import os from 'node:os'; -export const SANDBOX_PROFILE_VERSION = 1; +/** v2 (2026-09-05): allow the CLI's per-shell cwd-tracking file; see CLI_CWD_FILE_REGEX. */ +export const SANDBOX_PROFILE_VERSION = 2; /** * Credential paths always excluded — cannot be relaxed per task (NFR-2). @@ -73,11 +74,31 @@ export const CREDENTIAL_PATHS = [ /** Subpaths of ~/.claude the ENGINE may write (never global config). */ export const CLAUDE_STATE_WRITE_SUBPATHS = ['projects', 'statsig', 'shell-snapshots', 'logs']; +/** + * Claude Code 2.1.x's Bash tool writes one cwd-tracking file per shell + * invocation at /tmp/claude--cwd (verified 2026-09-05 on 2.1.261). With + * it denied, every command still runs but the shell exits 1, which the agent + * reads as failure. This regex admits exactly that filename and nothing else + * under /tmp — tested against /tmp/claude--cwdx and /tmp/other-cwd. + */ +export const CLI_CWD_FILE_REGEX = '^/private/tmp/claude-[0-9a-f]+-cwd$'; + export interface SandboxSpec { /** rw locations: the run worktree or scratch dir */ writePaths: string[]; /** ro locations: repo root, context roots */ readPaths: string[]; + /** + * Exact-filename allows (Seatbelt `regex`) for engines that stage a file + * outside the run scope — e.g. hermes writes $HOME/.hermes-tmp. before + * moving it into the worktree. Anchored patterns only; never a directory. + */ + writeRegexes?: string[]; +} + +/** Escape a literal path for use inside a Seatbelt regex. */ +export function escapeRegexLiteral(s: string): string { + return s.replace(/[.*+?^${}()|[\]\\/]/g, '\\$&'); } /** @@ -131,6 +152,8 @@ export function generateSeatbeltProfile(spec: SandboxSpec): { profile: string; v // Engine state subpaths + /dev/null (git needs it; verified T-008). const engineWriteLines = [ ' (allow file-write* (literal "/dev/null"))', + ` (allow file-write* (regex #"${CLI_CWD_FILE_REGEX}"))`, + ...(spec.writeRegexes ?? []).map((r) => ` (allow file-write* (regex #"${r}"))`), ...CLAUDE_STATE_WRITE_SUBPATHS.map( (s) => ` (allow file-write* (subpath "${escapeForSeatbelt(`${os.homedir()}/.claude/${s}`)}"))`, ), @@ -179,3 +202,114 @@ ${engineWriteLines} export function wrapWithSandbox(argv: string[], profilePath: string): string[] { return ['sandbox-exec', '-f', profilePath, '--', ...argv]; } + +// --------------------------------------------------------------------------- +// Production wiring. Everything above proves a PROFILE contains a process; +// everything below is how a run actually ends up inside one. Until 2026-09-05 +// nothing called it — `new ClaudeCliRunner()` passed no spec and the other +// engines had no hook — while docs said every run was sandboxed. +// --------------------------------------------------------------------------- + +/** + * Claude Code's Bash tool needs a per-cwd work directory at + * /tmp/claude-/ (observed 2.1.261). + * Outside the write allowlist the shell never starts (EPERM on mkdir), so the + * runner pre-creates it and allowlists exactly that directory. + */ +export function cliWorkDirFor(cwd: string): string { + const real = resolveReal(cwd) ?? cwd; + const uid = typeof process.getuid === 'function' ? process.getuid() : 0; + return `/tmp/claude-${uid}/${real.replace(/\//g, '-')}`; +} + +/** + * Package-manager caches live outside the worktree (~/.npm, ~/.cargo, ~/.cache, + * …) and would be denied. Redirect the common ones — npm/pnpm/yarn, pip, + * cargo, go, gem/bundler, uv/poetry, gradle, composer, nuget — into one + * Clockwork-owned root so nightly runs keep a warm cache without widening the + * boundary to $HOME. The framing in docs is "worktree plus Clockwork-managed + * tool caches". + */ +export const TOOL_CACHE_ROOT = `${dataDir}/cache`; + +export function toolCacheEnv(root: string = TOOL_CACHE_ROOT): Record { + return { + npm_config_cache: path.join(root, 'npm'), + npm_config_store_dir: path.join(root, 'pnpm-store'), + YARN_CACHE_FOLDER: path.join(root, 'yarn'), + PIP_CACHE_DIR: path.join(root, 'pip'), + XDG_CACHE_HOME: path.join(root, 'xdg'), + CARGO_HOME: path.join(root, 'cargo'), + GOMODCACHE: path.join(root, 'go-mod'), + GOCACHE: path.join(root, 'go-build'), + GEM_HOME: path.join(root, 'gem'), + BUNDLE_PATH: path.join(root, 'bundle'), + UV_CACHE_DIR: path.join(root, 'uv'), + POETRY_CACHE_DIR: path.join(root, 'poetry'), + GRADLE_USER_HOME: path.join(root, 'gradle'), + COMPOSER_CACHE_DIR: path.join(root, 'composer'), + NUGET_PACKAGES: path.join(root, 'nuget'), + }; +} + +export interface SandboxSpecInput { + /** The run's cwd: the git worktree, or the scratch dir for repo-less tasks. */ + worktreePath: string; + scratchPath: string | null; + repoPath: string | null; + contextRoots: string[]; + /** Extra writable roots an engine needs for its own state (e.g. ~/.codex). */ + engineStatePaths?: string[]; + /** Exact-filename allows for engine staging files; see SandboxSpec.writeRegexes. */ + engineWriteRegexes?: string[]; + cacheRoot?: string; +} + +/** + * Build the per-run spec and create the directories it names. Reads stay + * platform-broad (ADR-023); writes are the worktree, the CLI work dir, the + * tool-cache root, and any engine state paths — nothing else. + */ +export function buildSandboxSpec(input: SandboxSpecInput): SandboxSpec { + const cacheRoot = input.cacheRoot ?? TOOL_CACHE_ROOT; + const cliWork = cliWorkDirFor(input.worktreePath); + mkdirSync(cliWork, { recursive: true, mode: 0o700 }); + for (const v of Object.values(toolCacheEnv(cacheRoot))) mkdirSync(v, { recursive: true }); + for (const p of input.engineStatePaths ?? []) mkdirSync(p, { recursive: true, mode: 0o700 }); + const writePaths = [input.worktreePath, input.scratchPath, cliWork, cacheRoot, ...(input.engineStatePaths ?? [])].filter( + (p): p is string => typeof p === 'string' && p.length > 0, + ); + const readPaths = [input.repoPath, ...input.contextRoots].filter((p): p is string => typeof p === 'string' && p.length > 0); + return { writePaths, readPaths, writeRegexes: input.engineWriteRegexes ?? [] }; +} + +export interface ApplySandboxOptions { + /** Per-spawn additions (a usage-file dir, a journal dir) that are not part of the run spec. */ + extraWritePaths?: string[]; +} + +/** + * Wrap an argv in sandbox-exec with a freshly generated profile. Every engine + * runner routes its spawn through here; the wiring test asserts it by reading + * the sources. A null spec is the explicit CW_SANDBOX=off escape hatch and is + * logged and stamped on the report by the caller — never silent. + * + * Throws when the spec would allowlist a credential path. Callers must FAIL the + * run on that, never fall back to an unsandboxed spawn. + */ +export function applySandbox( + argv: string[], + spec: SandboxSpec | null | undefined, + opts: ApplySandboxOptions = {}, +): { argv: string[]; profilePath: string | null; version: number | null } { + if (!spec) return { argv, profilePath: null, version: null }; + const merged: SandboxSpec = { + writePaths: [...spec.writePaths, ...(opts.extraWritePaths ?? [])], + readPaths: spec.readPaths, + writeRegexes: spec.writeRegexes, + }; + const { profile, version } = generateSeatbeltProfile(merged); + const profilePath = path.join(mkdtempSync(path.join(os.tmpdir(), 'cw-sb-')), 'profile.sb'); + writeFileSync(profilePath, profile, 'utf8'); + return { argv: wrapWithSandbox(argv, profilePath), profilePath, version }; +} diff --git a/packages/runner/src/worktree.ts b/packages/runner/src/worktree.ts index 0f4e7ff..0449249 100644 --- a/packages/runner/src/worktree.ts +++ b/packages/runner/src/worktree.ts @@ -157,6 +157,46 @@ export function pruneBranch(repoPath: string, branch: string): void { runGit(['branch', '-D', branch], repoPath); } +export interface WorktreeState { + exists: boolean; + /** uncommitted or untracked changes present */ + dirty: boolean; + /** a git operation was in flight when the process died: rebase | merge | cherry-pick | revert | bisect */ + interruptedOp: string | null; +} + +/** + * What a run left behind. Drives the finalize decision "prune or preserve" and + * the report line that tells the human. In a LINKED worktree `.git` is a file + * pointing into the main repo's worktrees/ dir, so the operation markers + * (rebase-merge, MERGE_HEAD, …) are NOT under /.git/ — the only + * correct lookup is `git rev-parse --git-path `. + */ +export function inspectWorktree(worktreePath: string): WorktreeState { + if (!existsSync(worktreePath)) return { exists: false, dirty: false, interruptedOp: null }; + const status = runGit(['status', '--porcelain', '--untracked-files=normal'], worktreePath); + const dirty = status.code === 0 && status.out.trim().length > 0; + const markers: Array<[string, string]> = [ + ['rebase-merge', 'rebase'], + ['rebase-apply', 'rebase'], + ['MERGE_HEAD', 'merge'], + ['CHERRY_PICK_HEAD', 'cherry-pick'], + ['REVERT_HEAD', 'revert'], + ['BISECT_LOG', 'bisect'], + ]; + let interruptedOp: string | null = null; + for (const [marker, op] of markers) { + const r = runGit(['rev-parse', '--git-path', marker], worktreePath); + if (r.code !== 0) continue; + const p = r.out.trim(); + if (p && existsSync(path.isAbsolute(p) ? p : path.join(worktreePath, p))) { + interruptedOp = op; + break; + } + } + return { exists: true, dirty, interruptedOp }; +} + /** Diffstat for the report (FR-15): name-only + numstat vs merge-base. */ export function diffStat(worktreePath: string, baseSha: string): Array<{ path: string; additions: number; deletions: number; binary: boolean }> { const r = runGit(['diff', '--numstat', baseSha], worktreePath); diff --git a/packages/runner/test/floor-hook.test.ts b/packages/runner/test/floor-hook.test.ts new file mode 100644 index 0000000..b590772 --- /dev/null +++ b/packages/runner/test/floor-hook.test.ts @@ -0,0 +1,119 @@ +/** + * floor-hook.ts is the actual security-closing piece (T-114): the CLI runs + * this PreToolUse hook for EVERY Bash call, in every --permission-mode — + * unlike the MCP permission-prompt-tool path, which acceptEdits mode skips + * for Bash entirely (verified live on CLI 2.1.261, 2026-09-05: an unasked + * `git push --force origin main` under acceptEdits). This test spawns the + * REAL generated .mjs file against a REAL PermissionServer — no mocks — and + * pins the fail-closed contract: bridge down (or hung) => exit 2, never 0. + * + * Uses async `spawn`, not `spawnSync`: this test process itself hosts the + * PermissionServer being called, and `spawnSync` blocks the whole event loop + * until the child exits — starving the very server the child is trying to + * reach and turning every case into a false "unreachable" timeout. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { spawn } from 'node:child_process'; +import http from 'node:http'; +import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { PermissionServer } from '../src/permission-server.js'; +import { evaluateCommand } from '../src/deny-list.js'; +import { writeFloorHook } from '../src/floor-hook.js'; + +let server: PermissionServer; +let dir: string; +let hookPath: string; + +function runHook(hook: string, command: string): Promise<{ status: number | null; stderr: string }> { + return new Promise((resolve, reject) => { + const child = spawn(process.execPath, ['--no-warnings', hook], { stdio: ['pipe', 'pipe', 'pipe'] }); + let stderr = ''; + child.stderr!.setEncoding('utf8'); + child.stderr!.on('data', (c: string) => { + stderr += c; + }); + child.on('error', reject); + child.on('close', (code) => resolve({ status: code, stderr })); + child.stdin!.end( + JSON.stringify({ + session_id: 's1', + cwd: '/tmp', + hook_event_name: 'PreToolUse', + tool_name: 'Bash', + tool_input: { command }, + }), + ); + }); +} + +beforeAll(async () => { + server = new PermissionServer({ + decide: async () => ({ behavior: 'deny', message: 'n/a — /floor path only, this test never uses /mcp' }), + // Same wiring claude-cli-runner.ts uses: only a genuine FLOOR hit denies. + floor: ({ input }) => { + const record = (input ?? {}) as Record; + if (typeof record.command !== 'string') return { denied: false }; + const v = evaluateCommand(record.command); + return { denied: v.floor, reason: v.reason }; + }, + }); + await server.start(); + dir = mkdtempSync(path.join(os.tmpdir(), 'cw-floor-hook-test-')); + hookPath = writeFloorHook(dir, server.floorUrl!); +}); + +afterAll(async () => { + await server.close(); // idempotent — safe even if an earlier test already closed it + rmSync(dir, { recursive: true, force: true }); +}); + +describe('floor-hook.mjs — PreToolUse fail-closed hook', () => { + it('imports nothing but node:http — it runs inside the packaged CLI\'s sandbox and must resolve no dist/ path', () => { + const src = readFileSync(hookPath, 'utf8'); + const importLines = src.split('\n').filter((l) => /^\s*(import|export)\b/.test(l) || /\brequire\(/.test(l)); + expect(importLines.length).toBeGreaterThan(0); // sanity: the http import really is there + for (const l of importLines) expect(l).toContain('node:http'); + }); + + it('denies a floor-hit command with exit 2 and the reason on stderr', async () => { + const r = await runHook(hookPath, 'git push --force origin main'); + expect(r.status).toBe(2); + expect(r.stderr).toMatch(/force-push to protected branch 'main' is blocked by global deny-list/); + }); + + it('allows a benign command with exit 0 and no stderr', async () => { + const r = await runHook(hookPath, 'ls'); + expect(r.status).toBe(0); + expect(r.stderr).toBe(''); + }); + + it('fails closed on a request that times out — never exit 0 while waiting', async () => { + // A server that accepts the connection but never answers: the hook's own + // POST_TIMEOUT_MS must fire and deny, distinct from the ECONNREFUSED path below. + const hung = http.createServer(() => { + /* never respond */ + }); + await new Promise((resolve) => hung.listen(0, '127.0.0.1', resolve)); + const port = (hung.address() as { port: number }).port; + const hungDir = mkdtempSync(path.join(os.tmpdir(), 'cw-floor-hook-hung-')); + const hungHookPath = writeFloorHook(hungDir, `http://127.0.0.1:${port}/floor`); + try { + const r = await runHook(hungHookPath, 'ls'); + expect(r.status).toBe(2); + expect(r.stderr).toMatch(/Clockwork policy floor unreachable/); + expect(r.stderr).toMatch(/timed out/); + } finally { + await new Promise((resolve) => hung.close(() => resolve())); + rmSync(hungDir, { recursive: true, force: true }); + } + }, 15_000); + + it('fails closed when the bridge is unreachable — never exit 0 on error', async () => { + await server.close(); + const r = await runHook(hookPath, 'ls'); + expect(r.status).toBe(2); + expect(r.stderr).toMatch(/Clockwork policy floor unreachable/); + }); +}); diff --git a/packages/runner/test/permission-server.test.ts b/packages/runner/test/permission-server.test.ts new file mode 100644 index 0000000..92a547e --- /dev/null +++ b/packages/runner/test/permission-server.test.ts @@ -0,0 +1,202 @@ +/** + * The permission bridge speaks exactly the JSON-RPC-over-HTTP shape Claude Code + * CLI 2.1.261 was observed sending (spike 2026-09-05): tools/call with + * { tool_name, input, tool_use_id }, answered with text content holding the + * decision JSON. These tests pin that contract and the two failure modes that + * must never become an implicit allow: a throwing supervisor and an unknown tool. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { PermissionServer, type PermissionDecision, type PermissionRequestPayload } from '../src/permission-server.js'; + +let server: PermissionServer; +let url: string; +let nextDecision: (req: PermissionRequestPayload) => Promise; +const seen: PermissionRequestPayload[] = []; + +async function rpc(body: unknown): Promise<{ status: number; json: any }> { + const res = await fetch(url, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(body) }); + const text = await res.text(); + return { status: res.status, json: text ? JSON.parse(text) : null }; +} + +beforeAll(async () => { + nextDecision = async () => ({ behavior: 'deny', message: 'default' }); + server = new PermissionServer({ + decide: (req) => { + seen.push(req); + return nextDecision(req); + }, + }); + ({ url } = await server.start()); +}); + +afterAll(async () => { + await server.close(); +}); + +describe('permission bridge — MCP contract', () => { + it('names the tool the way --permission-prompt-tool expects', () => { + expect(server.toolFlag).toBe('mcp__clockwork__approve'); + expect(url).toMatch(/^http:\/\/127\.0\.0\.1:\d+\/mcp$/); + }); + + it('mcp-config floors the per-server timeout at the CLI HTTP default and otherwise passes the run budget through', () => { + expect(server.mcpConfig(1_000).mcpServers.clockwork).toEqual({ type: 'http', url, timeout: 60_000 }); + expect(server.mcpConfig(300_000).mcpServers.clockwork.timeout).toBe(300_000); + }); + + it('initialize echoes the requested protocol version and advertises tools', async () => { + const { status, json } = await rpc({ jsonrpc: '2.0', id: 0, method: 'initialize', params: { protocolVersion: '2025-11-25' } }); + expect(status).toBe(200); + expect(json.result.protocolVersion).toBe('2025-11-25'); + expect(json.result.capabilities).toEqual({ tools: {} }); + expect(json.result.serverInfo.name).toBe('clockwork'); + }); + + it('notifications get 202 and no body', async () => { + const res = await fetch(url, { method: 'POST', body: JSON.stringify({ jsonrpc: '2.0', method: 'notifications/initialized' }) }); + expect(res.status).toBe(202); + expect(await res.text()).toBe(''); + }); + + it('tools/list exposes exactly one tool with the observed input shape', async () => { + const { json } = await rpc({ jsonrpc: '2.0', id: 1, method: 'tools/list' }); + expect(json.result.tools).toHaveLength(1); + expect(json.result.tools[0].name).toBe('approve'); + expect(json.result.tools[0].inputSchema.required).toEqual(['tool_name', 'input']); + }); + + it('tools/call forwards the request and returns the decision as JSON text — after a genuine hold', async () => { + nextDecision = () => new Promise((r) => setTimeout(() => r({ behavior: 'allow', updatedInput: { command: 'ls' } }), 150)); + const t0 = Date.now(); + const { json } = await rpc({ + jsonrpc: '2.0', + id: 2, + method: 'tools/call', + params: { name: 'approve', arguments: { tool_name: 'Bash', input: { command: 'ls' }, tool_use_id: 'toolu_x' } }, + }); + expect(Date.now() - t0).toBeGreaterThanOrEqual(140); + expect(seen.at(-1)).toEqual({ toolName: 'Bash', input: { command: 'ls' }, toolUseId: 'toolu_x' }); + expect(json.result.content[0].type).toBe('text'); + expect(JSON.parse(json.result.content[0].text)).toEqual({ behavior: 'allow', updatedInput: { command: 'ls' } }); + }); + + it('a deny carries its message verbatim', async () => { + nextDecision = async () => ({ behavior: 'deny', message: 'policy floor: git push --force' }); + const { json } = await rpc({ jsonrpc: '2.0', id: 3, method: 'tools/call', params: { name: 'approve', arguments: { tool_name: 'Bash', input: {} } } }); + expect(JSON.parse(json.result.content[0].text)).toEqual({ behavior: 'deny', message: 'policy floor: git push --force' }); + }); + + it('a throwing supervisor is a deny, never an allow', async () => { + nextDecision = async () => { + throw new Error('daemon gone'); + }; + const { json } = await rpc({ jsonrpc: '2.0', id: 4, method: 'tools/call', params: { name: 'approve', arguments: { tool_name: 'Write', input: {} } } }); + const d = JSON.parse(json.result.content[0].text); + expect(d.behavior).toBe('deny'); + expect(d.message).toContain('daemon gone'); + }); + + it('rejects unknown tools and unknown methods with JSON-RPC errors', async () => { + const tool = await rpc({ jsonrpc: '2.0', id: 5, method: 'tools/call', params: { name: 'other', arguments: {} } }); + expect(tool.json.error.code).toBe(-32602); + const method = await rpc({ jsonrpc: '2.0', id: 6, method: 'resources/list' }); + expect(method.json.error.code).toBe(-32601); + }); + + it('refuses non-POST and malformed bodies', async () => { + expect((await fetch(url)).status).toBe(405); + const bad = await fetch(url, { method: 'POST', body: '{not json' }); + expect((await bad.json()).error.code).toBe(-32700); + }); + + it('404s an unknown path — /mcp above never falls back to a catch-all', async () => { + const base = url.replace(/\/mcp$/, ''); + const res = await fetch(`${base}/nope`, { method: 'POST' }); + expect(res.status).toBe(404); + }); + + it('/floor denies with no floor callback configured — fail-closed default, this server never set one', async () => { + const floorUrl = url.replace(/\/mcp$/, '/floor'); + const res = await fetch(floorUrl, { method: 'POST', body: JSON.stringify({ tool_name: 'Bash', tool_input: { command: 'ls' } }) }); + const json = await res.json(); + expect(res.status).toBe(200); + expect(json).toEqual({ decision: 'deny', reason: 'no policy floor configured' }); + }); +}); + +describe('permission bridge — /floor policy hook (T-114)', () => { + let floorServer: PermissionServer; + let floorUrl: string; + let nextFloor: (req: { toolName: string; input: unknown }) => { denied: boolean; reason?: string }; + + beforeAll(async () => { + nextFloor = () => ({ denied: false }); + floorServer = new PermissionServer({ + decide: async () => ({ behavior: 'deny', message: 'n/a' }), + floor: (req) => nextFloor(req), + }); + await floorServer.start(); + floorUrl = floorServer.floorUrl!; + }); + + afterAll(async () => { + await floorServer.close(); + }); + + async function floorRpc(body: unknown): Promise<{ status: number; json: any }> { + const res = await fetch(floorUrl, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: typeof body === 'string' ? body : JSON.stringify(body), + }); + const text = await res.text(); + return { status: res.status, json: text ? JSON.parse(text) : null }; + } + + it('exposes /floor next to /mcp on the same bound origin', () => { + expect(floorUrl).toMatch(/^http:\/\/127\.0\.0\.1:\d+\/floor$/); + }); + + it('denies when the floor callback says so, reason verbatim', async () => { + nextFloor = () => ({ denied: true, reason: "force-push to protected branch 'main' is blocked by global deny-list" }); + const { status, json } = await floorRpc({ tool_name: 'Bash', tool_input: { command: 'git push --force origin main' } }); + expect(status).toBe(200); + expect(json).toEqual({ decision: 'deny', reason: "force-push to protected branch 'main' is blocked by global deny-list" }); + }); + + it('allows when the floor callback says so', async () => { + nextFloor = () => ({ denied: false }); + const { status, json } = await floorRpc({ tool_name: 'Bash', tool_input: { command: 'ls' } }); + expect(status).toBe(200); + expect(json).toEqual({ decision: 'allow' }); + }); + + it('a malformed body denies, never allows', async () => { + const { status, json } = await floorRpc('{not json'); + expect(status).toBe(200); + expect(json.decision).toBe('deny'); + expect(json.reason).toMatch(/malformed/i); + }); + + it('a throwing floor callback denies, never allows', async () => { + nextFloor = () => { + throw new Error('deny-list evaluation crashed'); + }; + const { status, json } = await floorRpc({ tool_name: 'Bash', tool_input: { command: 'ls' } }); + expect(status).toBe(200); + expect(json.decision).toBe('deny'); + expect(json.reason).toContain('deny-list evaluation crashed'); + }); + + it('rejects non-POST with 405', async () => { + const res = await fetch(floorUrl); + expect(res.status).toBe(405); + }); + + it('404s an unknown path on this server too', async () => { + const base = floorUrl.replace(/\/floor$/, ''); + const res = await fetch(`${base}/nope`, { method: 'POST' }); + expect(res.status).toBe(404); + }); +}); diff --git a/packages/runner/test/runner-env-wiring.test.ts b/packages/runner/test/runner-env-wiring.test.ts index 6c36df8..648f4e5 100644 --- a/packages/runner/test/runner-env-wiring.test.ts +++ b/packages/runner/test/runner-env-wiring.test.ts @@ -51,3 +51,41 @@ describe('runner env wiring', () => { expect(offenders, `runners declaring their own env object: ${offenders.join(', ')}`).toEqual([]); }); }); + +/** + * Sandbox wiring. sandbox.ts and its escape suite prove the PROFILE contains + * a process; nothing below proves a production run is ever inside one. Until + * this block existed, no runner was — `new ClaudeCliRunner()` at the + * runner-child call site passed no spec, and the other engines had no hook at + * all — while docs/security.md said "every run executes inside a per-run + * macOS Seatbelt profile". Same source-reading approach as above, same reason. + */ +const RUNNER_CHILD = resolve(SRC, '../../daemon/src/runner-child.ts'); +const API_AGENT = 'api-agent-runner.ts'; + +describe('sandbox wiring', () => { + it('every engine runner routes its argv through the shared sandbox helper', () => { + const offenders = RUNNERS.filter((f) => !/applySandbox\(/.test(read(f))); + expect(offenders, `runners that never wrap with sandbox-exec: ${offenders.join(', ')}`).toEqual([]); + }); + + it('the BYOK api-agent shell also routes through the sandbox helper and the env allowlist', () => { + const src = read(API_AGENT); + expect(/applySandbox\(/.test(src), 'api-agent-runner never wraps its bash in sandbox-exec').toBe(true); + expect(/buildRunEnv\(/.test(src), 'api-agent-runner execFile inherits process.env (leaks CW_BYOK_KEY)').toBe(true); + }); + + it('runner-child constructs a SandboxSpec and hands it to every runner it builds', () => { + expect(existsSync(RUNNER_CHILD), 'runner-child.ts moved — guard is blind').toBe(true); + const src = readFileSync(RUNNER_CHILD, 'utf8'); + expect(/buildSandboxSpec\(/.test(src), 'runner-child never builds a SandboxSpec').toBe(true); + // A constructor call with no options is exactly the production bug this guards against. + const bare = src.match(/new (ClaudeCliRunner|CodexRunner|OpenCodeRunner|HermesRunner)\(\s*\)/g) ?? []; + expect(bare, `runners constructed with no sandbox option: ${bare.join(', ')}`).toEqual([]); + }); + + it('runner-child scrubs the BYOK key from its own env after reading it', () => { + const src = readFileSync(RUNNER_CHILD, 'utf8'); + expect(/delete process\.env\.CW_BYOK_KEY/.test(src), 'CW_BYOK_KEY stays readable by every child the agent spawns').toBe(true); + }); +}); diff --git a/packages/runner/test/sandbox-production-spec.test.ts b/packages/runner/test/sandbox-production-spec.test.ts new file mode 100644 index 0000000..28c81a3 --- /dev/null +++ b/packages/runner/test/sandbox-production-spec.test.ts @@ -0,0 +1,191 @@ +/** + * The production spec builder and the wrap helper — the two functions that turn + * "a profile exists" into "this run is inside one". The macOS block runs a real + * sandbox-exec against a profile built exactly the way runner-child builds it, + * and checks the three findings from the 2026-09-05 probe: the CLI work dir + * must be writable or no shell starts; the cwd-tracking file must be writable + * or every command exits 1; and nothing else under /tmp or $HOME may be. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { + applySandbox, + buildSandboxSpec, + cliWorkDirFor, + generateSeatbeltProfile, + toolCacheEnv, + SANDBOX_PROFILE_VERSION, +} from '../src/sandbox.js'; + +const onMac = process.platform === 'darwin'; +const uid = process.getuid?.() ?? 0; + +describe('cliWorkDirFor', () => { + it('mirrors the CLI: /tmp/claude-/', () => { + // Nonexistent path: realpath falls back to the literal, so the slug is predictable. + expect(cliWorkDirFor('/a/b c/d')).toBe(`/tmp/claude-${uid}/-a-b c-d`); + }); + + it('resolves symlinks first, so the slug matches what the CLI computes from its real cwd', () => { + const real = mkdtempSync(path.join(os.tmpdir(), 'cw-real-')); + const realResolved = execFileSync('/bin/pwd', ['-P'], { cwd: real, encoding: 'utf8' }).trim(); + expect(cliWorkDirFor(real)).toBe(`/tmp/claude-${uid}/${realResolved.replace(/\//g, '-')}`); + rmSync(real, { recursive: true, force: true }); + }); +}); + +describe('toolCacheEnv', () => { + it('redirects every known package-manager cache under the given root', () => { + const expectedKeys = [ + 'npm_config_cache', + 'npm_config_store_dir', + 'YARN_CACHE_FOLDER', + 'PIP_CACHE_DIR', + 'XDG_CACHE_HOME', + 'CARGO_HOME', + 'GOMODCACHE', + 'GOCACHE', + 'GEM_HOME', + 'BUNDLE_PATH', + 'UV_CACHE_DIR', + 'POETRY_CACHE_DIR', + 'GRADLE_USER_HOME', + 'COMPOSER_CACHE_DIR', + 'NUGET_PACKAGES', + ]; + const env = toolCacheEnv('/x'); + expect(Object.keys(env).sort()).toEqual(expectedKeys.sort()); + for (const v of Object.values(env)) expect(v.startsWith('/x/')).toBe(true); + }); +}); + +describe('buildSandboxSpec', () => { + it('writes = worktree + CLI work dir + cache root + engine state; reads = repo + context roots', () => { + const tmp = mkdtempSync(path.join(os.tmpdir(), 'cw-spec-')); + const worktree = path.join(tmp, 'wt'); + const repo = path.join(tmp, 'repo'); + const cache = path.join(tmp, 'cache'); + const state = path.join(tmp, 'engine-state'); + const spec = buildSandboxSpec({ + worktreePath: worktree, + scratchPath: null, + repoPath: repo, + contextRoots: ['/ctx/one'], + engineStatePaths: [state], + cacheRoot: cache, + }); + expect(spec.writePaths).toEqual([worktree, cliWorkDirFor(worktree), cache, state]); + expect(spec.readPaths).toEqual([repo, '/ctx/one']); + // It creates what it names so the first spawn does not EPERM on mkdir. + expect(existsSync(cliWorkDirFor(worktree))).toBe(true); + expect(existsSync(state)).toBe(true); + for (const dir of Object.values(toolCacheEnv(cache))) expect(existsSync(dir)).toBe(true); + rmSync(tmp, { recursive: true, force: true }); + rmSync(cliWorkDirFor(worktree), { recursive: true, force: true }); + }); + + it('drops a null scratch path instead of emitting an empty allow', () => { + const tmp = mkdtempSync(path.join(os.tmpdir(), 'cw-spec-')); + const spec = buildSandboxSpec({ worktreePath: tmp, scratchPath: null, repoPath: null, contextRoots: [], cacheRoot: path.join(tmp, 'c') }); + expect(spec.writePaths.every((p) => p.length > 0)).toBe(true); + expect(spec.readPaths).toEqual([]); + rmSync(tmp, { recursive: true, force: true }); + rmSync(cliWorkDirFor(tmp), { recursive: true, force: true }); + }); +}); + +describe('applySandbox', () => { + it('a null spec is a no-op — the caller owns logging that escape hatch', () => { + expect(applySandbox(['claude', '-p', 'x'], null)).toEqual({ argv: ['claude', '-p', 'x'], profilePath: null, version: null }); + }); + + it('wraps in sandbox-exec with a written v2 profile that includes the cwd-file allow', () => { + const tmp = mkdtempSync(path.join(os.tmpdir(), 'cw-apply-')); + const { argv, profilePath, version } = applySandbox(['echo', 'hi'], { writePaths: [tmp], readPaths: [] }); + expect(argv.slice(0, 2)).toEqual(['sandbox-exec', '-f']); + expect(argv.slice(-2)).toEqual(['echo', 'hi']); + expect(version).toBe(SANDBOX_PROFILE_VERSION); + expect(version).toBe(2); + const profile = readFileSync(profilePath!, 'utf8'); + expect(profile).toContain('(regex #"^/private/tmp/claude-[0-9a-f]+-cwd$")'); + expect(profile).toContain(`(allow file-write* (subpath "${realpathSync(tmp)}"))`); + rmSync(tmp, { recursive: true, force: true }); + }); + + it('emits engine staging-file regexes as exact-name allows', () => { + const tmp = mkdtempSync(path.join(os.tmpdir(), 'cw-apply-')); + const { profilePath } = applySandbox(['true'], { writePaths: [tmp], readPaths: [], writeRegexes: ['^/Users/x/\\.hermes-tmp\\.[0-9]+$'] }); + expect(readFileSync(profilePath!, 'utf8')).toContain('(allow file-write* (regex #"^/Users/x/\\.hermes-tmp\\.[0-9]+$"))'); + rmSync(tmp, { recursive: true, force: true }); + }); + + it('merges per-spawn extra write paths without mutating the run spec', () => { + const tmp = mkdtempSync(path.join(os.tmpdir(), 'cw-apply-')); + const extra = mkdtempSync(path.join(os.tmpdir(), 'cw-extra-')); + const spec = { writePaths: [tmp], readPaths: [] }; + const { profilePath } = applySandbox(['true'], spec, { extraWritePaths: [extra] }); + expect(readFileSync(profilePath!, 'utf8')).toContain(path.basename(extra)); + expect(spec.writePaths).toEqual([tmp]); + rmSync(tmp, { recursive: true, force: true }); + rmSync(extra, { recursive: true, force: true }); + }); + + it('refuses to allowlist a credential path — a thrown error, never a silent widen', () => { + const ssh = path.join(os.homedir(), '.ssh'); + if (!existsSync(ssh)) return; // resolveReal drops missing paths before the check can fire + expect(() => applySandbox(['true'], { writePaths: [ssh], readPaths: [] })).toThrow(/credential path/); + }); +}); + +describe.skipIf(!onMac)('the production profile under a real sandbox-exec', () => { + let tmp: string; + let worktree: string; + let profilePath: string; + let cliWork: string; + + function inSandbox(argv: string[]): boolean { + try { + execFileSync('sandbox-exec', ['-f', profilePath, '--', ...argv], { stdio: ['ignore', 'pipe', 'pipe'], timeout: 20_000 }); + return true; + } catch { + return false; + } + } + + beforeAll(() => { + tmp = mkdtempSync(path.join(os.tmpdir(), 'cw-prod-')); + worktree = path.join(tmp, 'wt'); + mkdirSync(worktree); // in production the daemon creates it before spawn; a missing path is (correctly) dropped + const spec = buildSandboxSpec({ worktreePath: worktree, scratchPath: null, repoPath: null, contextRoots: [], cacheRoot: path.join(tmp, 'cache') }); + cliWork = cliWorkDirFor(worktree); + const { profile } = generateSeatbeltProfile(spec); + profilePath = path.join(tmp, 'profile.sb'); + writeFileSync(profilePath, profile); + }); + + afterAll(() => { + rmSync(tmp, { recursive: true, force: true }); + rmSync(cliWork, { recursive: true, force: true }); + for (const f of ['/tmp/claude-ab12-cwd', '/tmp/claude-ab12-cwdx', path.join(os.homedir(), 'cw-escape-probe')]) rmSync(f, { force: true }); + }); + + it('lets the engine write its worktree and its CLI work dir', () => { + expect(inSandbox(['/usr/bin/touch', path.join(worktree, 'ok')])).toBe(true); + expect(inSandbox(['/usr/bin/touch', path.join(cliWork, 'ok')])).toBe(true); + }); + + it('admits exactly the cwd-tracking filename and nothing else under /tmp', () => { + expect(inSandbox(['/usr/bin/touch', '/tmp/claude-ab12-cwd'])).toBe(true); + expect(inSandbox(['/usr/bin/touch', '/tmp/claude-ab12-cwdx'])).toBe(false); + expect(inSandbox(['/usr/bin/touch', '/tmp/cw-probe-escape'])).toBe(false); + }); + + it('still denies writes to $HOME and reads of credential paths', () => { + expect(inSandbox(['/usr/bin/touch', path.join(os.homedir(), 'cw-escape-probe')])).toBe(false); + const hist = path.join(os.homedir(), '.zsh_history'); + if (existsSync(hist)) expect(inSandbox(['/bin/cat', hist])).toBe(false); + }); +}); diff --git a/packages/runner/test/worktree-state.test.ts b/packages/runner/test/worktree-state.test.ts new file mode 100644 index 0000000..28b07fd --- /dev/null +++ b/packages/runner/test/worktree-state.test.ts @@ -0,0 +1,81 @@ +/** + * inspectWorktree drives finalize's prune-or-preserve decision. The case that + * matters is a LINKED worktree with a rebase in flight: its markers live in the + * main repo's .git/worktrees//, not under /.git (which is a + * file), so a naive existsSync('/.git/rebase-merge') would say + * "clean" and let finalize force-delete a half-done rebase. A Product Hunt user + * asked about exactly this case on 2026-09-04. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { inspectWorktree } from '../src/worktree.js'; + +function git(args: string[], cwd: string): string { + return execFileSync('git', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); +} + +let tmp: string; +let repo: string; +let wt: string; + +beforeAll(() => { + tmp = mkdtempSync(path.join(os.tmpdir(), 'cw-wtstate-')); + repo = path.join(tmp, 'repo'); + mkdirSync(repo); + git(['init', '-q', '-b', 'main'], repo); + git(['config', 'user.email', 't@t'], repo); + git(['config', 'user.name', 't'], repo); + writeFileSync(path.join(repo, 'f.txt'), '1\n'); + git(['add', '-A'], repo); + git(['commit', '-qm', 'init'], repo); + wt = path.join(tmp, 'wt'); + git(['worktree', 'add', '-q', '-b', 'run/x', wt, 'main'], repo); +}); + +afterAll(() => { + rmSync(tmp, { recursive: true, force: true }); +}); + +describe('inspectWorktree', () => { + it('a missing path is reported as such, not as clean', () => { + expect(inspectWorktree(path.join(tmp, 'nope'))).toEqual({ exists: false, dirty: false, interruptedOp: null }); + }); + + it('a fresh linked worktree is clean with no operation in flight', () => { + expect(inspectWorktree(wt)).toEqual({ exists: true, dirty: false, interruptedOp: null }); + }); + + it('an untracked file makes it dirty', () => { + writeFileSync(path.join(wt, 'scratch.txt'), 'wip\n'); + expect(inspectWorktree(wt).dirty).toBe(true); + rmSync(path.join(wt, 'scratch.txt')); + expect(inspectWorktree(wt).dirty).toBe(false); + }); + + it('a conflicting rebase in a LINKED worktree is detected via --git-path, and clears on abort', () => { + // Diverge: main and run/x both edit f.txt. + writeFileSync(path.join(repo, 'f.txt'), 'main\n'); + git(['commit', '-qam', 'main edit'], repo); + writeFileSync(path.join(wt, 'f.txt'), 'branch\n'); + git(['commit', '-qam', 'branch edit'], wt); + + let conflicted = false; + try { + git(['rebase', 'main'], wt); + } catch { + conflicted = true; // expected: the process "died" mid-rebase + } + expect(conflicted).toBe(true); + + const mid = inspectWorktree(wt); + expect(mid.exists).toBe(true); + expect(mid.interruptedOp).toBe('rebase'); + expect(mid.dirty).toBe(true); + + git(['rebase', '--abort'], wt); + expect(inspectWorktree(wt).interruptedOp).toBeNull(); + }); +}); diff --git a/packages/shared/package.json b/packages/shared/package.json index ca0122c..4c13c68 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -1,6 +1,6 @@ { "name": "@clockwork/shared", - "version": "0.4.0", + "version": "0.5.0", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/shared/src/report.ts b/packages/shared/src/report.ts index 6127ecd..e2badd5 100644 --- a/packages/shared/src/report.ts +++ b/packages/shared/src/report.ts @@ -47,6 +47,21 @@ export const DeliveryReceipt = z.object({ }); export type DeliveryReceipt = z.infer; +/** + * What happened to the run's worktree at finalize. `reason` names why it was + * kept — 'committed' | 'interrupted' | 'in_progress_op' | 'dirty' — and is null + * when it was pruned (clean, committed nothing, ended normally) or when the run + * had no repo. + */ +export const WorktreeStateRecord = z.object({ + preserved: z.boolean(), + path: z.string().nullable(), + dirty: z.boolean(), + interruptedOp: z.string().nullable(), + reason: z.string().nullable(), +}); +export type WorktreeStateRecord = z.infer; + export const RunReport = z.object({ runId: z.string(), taskId: z.string(), @@ -63,6 +78,8 @@ export const RunReport = z.object({ baseSha: z.string().nullable(), basedOnLocalState: z.boolean().default(false), // S-35 banner committedSomething: z.boolean().default(false), // S-39: analysis-only runs are valid + sandboxed: z.boolean().nullable().default(null), // false = CW_SANDBOX=off; null = engine never reported + worktreeState: WorktreeStateRecord.nullable().default(null), // null = no repo, or report predates the field diffStat: z.array(DiffFileStat).default([]), artifacts: z.array(z.string()).default([]), transcriptPath: z.string().nullable(), diff --git a/packages/shared/src/runner.ts b/packages/shared/src/runner.ts index d363e15..3aca8f7 100644 --- a/packages/shared/src/runner.ts +++ b/packages/shared/src/runner.ts @@ -34,6 +34,13 @@ export interface RunnerIO { /** Claude rate_limit_event telemetry → capacity model (FR-7, estimate-grade). */ onRateLimit?(info: Record): void; onLog(line: string): void; + /** + * A PreToolUse policy-floor hit (FR-11/T-114): the deny-list floor denied a + * command the CLI's own permission-prompt-tool path never asked about + * (acceptEdits skips it for Bash). Optional so existing RunnerIO + * implementers are unaffected until they choose to journal it. + */ + onPolicyDeny?(p: { tool: string; command: string; reason: string }): void; } export interface RunOutcome { diff --git a/packages/ui/package.json b/packages/ui/package.json index 9d11f2d..4f65880 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@clockwork/ui", - "version": "0.4.0", + "version": "0.5.0", "private": true, "type": "module", "scripts": { diff --git a/packages/ui/src/components/InboxView.tsx b/packages/ui/src/components/InboxView.tsx index de96436..4d97d68 100644 --- a/packages/ui/src/components/InboxView.tsx +++ b/packages/ui/src/components/InboxView.tsx @@ -332,6 +332,21 @@ function ReportDetail({ runId, version }: { runId: string; version: number }): J
Report not finalized yet — check back once the run completes.
)} + {report?.sandboxed === false && ( +
+ Sandbox was off for this run (CW_SANDBOX=off). Writes and credential reads were not contained. +
+ )} + {report?.worktreeState?.preserved && report.worktreeState.reason !== 'committed' && ( +
+ Worktree preserved at {report.worktreeState.path} + {report.worktreeState.interruptedOp + ? ` — interrupted during ${report.worktreeState.interruptedOp}; inspect before the next run touches it` + : report.worktreeState.dirty + ? ' — uncommitted changes left behind' + : ' — the run was interrupted, so nothing was pruned'} +
+ )} {report?.diffStat?.length > 0 && ( diff --git a/packaging/stage-release.sh b/packaging/stage-release.sh index 6a02182..d9766b5 100755 --- a/packaging/stage-release.sh +++ b/packaging/stage-release.sh @@ -16,17 +16,24 @@ BASE_URL="${BASE_URL:-https://clockwork.vmoksh-shah179.workers.dev}" BUNDLE="$ROOT/src-tauri/target/release/bundle/dmg" DEST="$ROOT/landing-page/downloads" -DMG="$(ls -1 "$BUNDLE"/Clockwork_*_aarch64.dmg 2>/dev/null | tail -1)" -[ -n "$DMG" ] || { echo "no DMG in $BUNDLE — run the tauri build first" >&2; exit 1; } - -# A locally built DMG is NOT byte-identical to the one CI publishes, so its -# hash differs. Staging a local build over a published one silently breaks the -# cask for everyone who already has the published hash. Prefer the release -# artifact; pass ALLOW_LOCAL=1 to override deliberately. -if [ "${ALLOW_LOCAL:-0}" != "1" ]; then - echo "refusing to stage a locally built DMG." >&2 - echo "download the artifact from the GitHub release instead, or re-run with ALLOW_LOCAL=1" >&2 - exit 1 +# Preferred input: the DMG downloaded from the GitHub release, e.g. +# gh release download v0.5.0 -p 'Clockwork_*_aarch64.dmg' -D /tmp/rel +# DMG=/tmp/rel/Clockwork_0.5.0_aarch64.dmg ./packaging/stage-release.sh +if [ -n "${DMG:-}" ]; then + [ -f "$DMG" ] || { echo "DMG not found: $DMG" >&2; exit 1; } +else + DMG="$(ls -1 "$BUNDLE"/Clockwork_*_aarch64.dmg 2>/dev/null | tail -1)" + [ -n "$DMG" ] || { echo "no DMG in $BUNDLE — run the tauri build first, or pass DMG=" >&2; exit 1; } + + # A locally built DMG is NOT byte-identical to the one CI publishes, so its + # hash differs. Staging a local build over a published one silently breaks the + # cask for everyone who already has the published hash. Prefer the release + # artifact (DMG=...); pass ALLOW_LOCAL=1 to override deliberately. + if [ "${ALLOW_LOCAL:-0}" != "1" ]; then + echo "refusing to stage a locally built DMG." >&2 + echo "download the artifact from the GitHub release (DMG=), or re-run with ALLOW_LOCAL=1" >&2 + exit 1 + fi fi VERSION="$(basename "$DMG" | sed -E 's/Clockwork_(.+)_aarch64\.dmg/\1/')" @@ -41,11 +48,16 @@ CASK="$ROOT/packaging/homebrew/clockwork.rb" /usr/bin/sed -i '' -E "s/^ version \".*\"/ version \"${VERSION}\"/" "$CASK" /usr/bin/sed -i '' -E "s/^ sha256 \".*\"/ sha256 \"${SHA}\"/" "$CASK" -# landing page download links + button label +# landing page: every download link points at the stable-named asset of the +# LATEST GitHub release (release.yml publishes Clockwork_aarch64.dmg alongside +# the versioned file), so the links never go stale. Only the JSON-LD metadata +# carries the version. PAGE="$ROOT/landing-page/index.html" -/usr/bin/sed -i '' -E "s#/downloads/Clockwork_[0-9.]+_aarch64\.dmg#/downloads/Clockwork_${VERSION}_aarch64.dmg#g" "$PAGE" +LATEST="https://github.com/vimoxshah/clockwork/releases/latest/download/Clockwork_aarch64.dmg" +/usr/bin/sed -i '' -E "s#href=\"https://github.com/vimoxshah/clockwork/releases/download/v[0-9.]+/Clockwork_[0-9.]+_aarch64\.dmg\"#href=\"${LATEST}\"#g" "$PAGE" +/usr/bin/sed -i '' -E "s#\"softwareVersion\": \"[0-9.]+\"#\"softwareVersion\": \"${VERSION}\"#" "$PAGE" +/usr/bin/sed -i '' -E "s#\"downloadUrl\": \"[^\"]+\"#\"downloadUrl\": \"${LATEST}\"#" "$PAGE" /usr/bin/sed -i '' -E "s#Download Clockwork [0-9.]+ for Mac#Download Clockwork ${VERSION} for Mac#" "$PAGE" -/usr/bin/sed -i '' -E "s#Clockwork_[0-9.]+_aarch64\.dmg#Clockwork_${VERSION}_aarch64.dmg#g" "$PAGE" # keep the cask host in sync with BASE_URL /usr/bin/sed -i '' -E "s#url \"https://[^/]+/downloads/#url \"${BASE_URL}/downloads/#" "$CASK" diff --git a/plan/STATUS.md b/plan/STATUS.md index eb12c20..f35f1e2 100644 --- a/plan/STATUS.md +++ b/plan/STATUS.md @@ -20,7 +20,7 @@ Last updated: 2026-08-22 (post-M1-core implementation sprint) |---|---|---| | T-001 | DONE | Real headless `claude -p --output-format stream-json` on subscription login: exit 0, 10 usage events, structured summary, session-id capture, worktree containment. `spikes/reports/T001-cli-runner.md` | | T-002 | DONE | Interruption matrix as permanent tests: SIGTERM/timeout→timed_out/budget→budget_exceeded/turn-stop, group-kill leaves no zombies incl. grandchildren. `packages/runner/test/interrupt-matrix.test.ts` (8 tests) | -| T-003 | PARTIAL | CLI decision made with binary evidence: `--permission-prompt-tool` ABSENT in 2.1.238 → fail-safe-on-permission (ADR-020). SDK keep-alive/resume fidelity probe deferred to T-212 integration. `spikes/reports/T003-hitl-decision.md` | +| T-003 | DONE | Re-run 2026-09-05 against CLI 2.1.261: `--permission-prompt-tool` now PRESENT and proven by run (tool called, 100s hold honoured inside the sandbox). Keep-alive HITL wired for the CLI engine via a loopback HTTP MCP bridge (ADR-034 supersedes ADR-020). `spikes/reports/T007-engine-contract-matrix-2.1.261.md` | | T-004 | DONE | Absent-auth probe: `authentication_failed` detectable in stream (exit 1). Error taxonomy documented incl. observed `rate_limit_event`. Expired-token simulation documented as not-probed (requires mutating real creds). `spikes/reports/T004-auth-probes.md` | | T-005 | DONE | Scheduler micro-PoC became product code with fixture suite: 18 fake-clock tests across America/New_York, Europe/Berlin, Australia/Lord_Howe, UTC; concurrent double-fire attack cannot produce two runs. `packages/daemon/test/scheduler.test.ts` | | T-006 | DONE | Auth posture memo: own-login actuator, no credential scraping, expiry UX defined, SDK fallback lane. `spikes/reports/T006-auth-posture.md` | @@ -44,7 +44,7 @@ Last updated: 2026-08-22 (post-M1-core implementation sprint) | T-108 | PARTIAL | LaunchAgent plist generation + install/uninstall + doctor (6 checks incl. duplicate instance). Code complete; live `launchctl bootstrap` verification pending on a clean machine (CI smoke covers daemon boot only). | | T-109 | DONE | Daemon-side osascript notifier + quiet-hours suppression; works with UI closed by construction. | | T-110 | DONE | Retention pruning (7d success / 30d failed worktrees, reports forever) + startup worktree reconciliation with quarantine list (never auto-deletes). | -| T-111 | DONE | Sandbox productionized from T-008: per-run profile generation w/ symlink resolution + credential-collision refusal, scoped engine-state writes, safety journal recording deny/orphan/preflight events. Escape suite green in repo; runs in CI on every change. | +| T-111 | DONE (2026-09-05) | **Correction:** until 2026-09-05 this row overstated — the profile generator, symlink resolution, credential-collision refusal and escape suite were done, but NO production spawn used them (`new ClaudeCliRunner()` passed no spec; other engines had no hook). Now every engine spawn incl. BYOK bash routes through `applySandbox()`; `runner-child` builds the spec; profile v2 admits the CLI work dir + cwd file; `CW_SANDBOX=off` is journaled and stamped on the report. Guarded by `runner-env-wiring.test.ts` "sandbox wiring". Verified in-sandbox: Claude ✅, OpenCode ✅, Hermes ⚠️ (pre-existing `$HOME` cwd bug), Codex ❓ (local config error). ADR-034. | | T-112 | PARTIAL | Profiles table, 3 seeded built-ins (Generalist/Dep Surgeon/Docs Scribe), bundled versioned skill pack authored (3 skills × SKILL.md procedures), name@version resolver (+user-skill fallback), @mention resolution server-side. Profile identity shown in report header/delivery text; calendar/inbox chips not yet rendered everywhere. | | T-113 | PARTIAL | FTS5 search_idx, index-on-finalize + task save, snippeted /search, inbox search box. 5k-corpus <100ms benchmark not yet measured. | | T-114 | PARTIAL | queue schedule kind persisted; GET /queue computes position + wait reason (slot/repo/paused); queue lane UI with cancel. Composer cannot yet book ASAP items directly (schedule-kind=queue selection missing). | @@ -66,7 +66,7 @@ Shell note: React+Vite web app served by the daemon; Tauri wrapper deferred to p | ID | Status | Notes / Evidence | |---|---|---| -| T-201 | PARTIAL | Approval rows persist across restarts (S-54 data model), CAS respond endpoint (S-57), inbox needs-you panel, deny-list floor never approvable (S-55), M1 fail-safe auto-deny (ADR-020). Keep-alive held-runner model blocked on SDK engine (T-212). | +| T-201 | DONE for CLI engine (2026-09-05) | Approval rows persist across restarts (S-54), CAS respond endpoint (S-57), inbox needs-you panel, deny-list floor never approvable (S-55). **Now actually reachable:** the CLI engine raises permission requests through the loopback bridge and HOLDS until a human answers or the run's wall-clock ends (was: never raised in production; fixed 120s window). Codex/OpenCode/Hermes have no permission hook — their containment is the sandbox alone. ADR-034. | | T-202 | PARTIAL | Linear chain validation (cycles rejected at save, S-72), {{previous.report}} binding w/ honest truncation (S-73), upstream-failure semantics in schema. Calendar ghost rendering for skipped successors missing. | | T-203 | PARTIAL | Security preview (red/yellow/info flags), import arrives DISABLED (S-74), apply-time variable validation (S-75). Export-as-JSON missing. | | T-204 | PARTIAL | Orphan terminate+journal-report done (real-process tests), reboot sweep done. Disk-full pause-all suggestion, DB backup-on-migrate, updater drain: missing. | diff --git a/spikes/reports/T007-engine-contract-matrix-2.1.261.md b/spikes/reports/T007-engine-contract-matrix-2.1.261.md new file mode 100644 index 0000000..5d6951b --- /dev/null +++ b/spikes/reports/T007-engine-contract-matrix-2.1.261.md @@ -0,0 +1,58 @@ +# T-007 re-run — engine contract matrix, Claude Code CLI 2.1.261 + +- Date: 2026-09-05 +- Trigger: ADR-020 consequence line ("re-run matrix on every observed CLI version change"). Last run: 2.1.238 on 2026-08-21. +- Method: real binary, real runs. Every row below was observed, not read from `--help`. Raw MCP request/response logs: `scratchpad/permspike/rpc.log`, `scratchpad/combined2/rpc.log` (session-local). + +## Verdict + +**Keep-alive HITL is now possible on the CLI engine.** The two load-bearing absences that produced ADR-020 no longer hold. Clockwork wires both in this commit. + +| Assumption (ADR-020, 2.1.238) | 2.1.261 | Evidence | +|---|---|---| +| `--permission-prompt-tool` absent → fail-safe auto-deny after 120s | **Present and working.** CLI calls the named MCP tool and waits for the answer. | `claude -p … --permission-prompt-tool mcp__cwperm__approve --mcp-config `; `tools/call` received with `{tool_name:"Bash", input:{command:…}, tool_use_id}`; deny message echoed by the model verbatim; gated file never created. | +| `--max-turns` absent | Not in `--help`, but **accepted** (run proceeds, no unknown-flag error). BudgetGuard remains the enforcement point; the flag is belt-and-braces. | `claude -p "…" --max-turns 3 --output-format stream-json` → stream started normally. | +| Hold duration | HTTP MCP requests time out at **60s by default**. Raised by per-server `timeout` in `--mcp-config` and `MCP_TOOL_TIMEOUT`. With both set, a decision held **100.0s** was honoured (npm install ran after the wait). | `combined2/rpc.log`: call #1 in at 78.06s, answered at +100.0s, subsequent Bash executed. Without the raise: call #2 arrived exactly 60.3s after call #1 (CLI gave up). | +| Prompt tool visible to the model | **Not exposed.** The model's tool list omits `mcp__cwperm__*`; it cannot approve itself. | `system.init` event: `mcp_servers:[{name:"cwperm",status:"connected"}]`, no matching tool name in `tools[]`. | +| `acceptEdits` and Bash | Bash prompts **do** fire under `acceptEdits` (3 requests in one run). An in-cwd `touch` did **not** prompt — acceptEdits auto-approves it. The Seatbelt sandbox is what contains those. | `combined2/stream.jsonl` | + +## Sandbox interaction (new since T-008) + +T-008 proved `claude -p` *prints* inside the profile. It never exercised the model's Bash tool. Two write targets the Bash tool needs were outside the allowlist: + +| Path | Effect when denied | Fix (profile v2) | +|---|---|---| +| `/tmp/claude-/` | `EPERM: operation not permitted, mkdir …` — **no shell ever starts**; every command fails. | Pre-create from runner-child, allowlist as `subpath`. `cliWorkDirFor()` in `sandbox.ts`. | +| `/tmp/claude--cwd` | Commands run, but shell exits 1 → agent reads every command as failed. | `(allow file-write* (regex #"^/private/tmp/claude-[0-9a-f]+-cwd$"))` — proven to reject `…-cwdx`, `/tmp/other-cwd`, `/tmp/x`. | + +With both fixed, inside the sandbox on 2.1.261: `touch` in worktree ✅ created · `npm install left-pad` ✅ installed (cache redirected via `npm_config_cache`) · `touch /tmp/probe-escape.txt` ❌ `Operation not permitted` · `head ~/.zsh_history` ❌ `Operation not permitted`. + +## Other engines inside the same profile (first time probed) + +| Engine | Result | Notes | +|---|---|---| +| OpenCode | ✅ writes `ok.txt` in worktree, `/tmp` escape denied, 14s | Needs `~/.opencode` writable in addition to `~/.local/share`, `~/.config`, `~/.cache` opencode dirs; hung 180s without it. stdin must be closed (runners use `'ignore'`). | +| Hermes 0.21.0 | ✅ writes `ok.txt` in worktree, `/tmp` escape denied, history read denied, usage file written, 37s | Root cause of the earlier `$HOME` writes: hermes's oneshot (`-z`) path never applies `--in` (skips `_apply_in_dir`). It does honour `TERMINAL_CWD`; `HermesRunner` now sets it to the worktree (flag kept). Staging file `$HOME/.hermes-tmp.` admitted via exact-name regex. | +| Codex 0.142.4 | ✅ writes `ok.txt` in worktree, `/tmp` escape denied, `~/.zsh_history` read denied, 27s | Two findings. (1) The machine's `~/.codex/config.toml` had tables the installed codex rejects (`[agents]`, two unknown keys in `features.multi_agent_v2`); fixed locally, backup kept. (2) **Seatbelt does not nest:** codex's own `workspace-write` profile fails with `sandbox_apply: Operation not permitted` inside any `(deny default)` outer profile — bisected every allow, none unblocks it; only an `(allow default)` outer works. `CodexRunner` now passes `-s danger-full-access` when Clockwork's profile is on (ours is the containment) and keeps `workspace-write` only under `CW_SANDBOX=off`. Trade-off: codex's inner sandbox also blocked shell-command network; under Clockwork's profile network is allowed, same as every other engine. | + +## Wiring landed (see ADR-0xx superseding ADR-020) + +- `packages/runner/src/permission-server.ts` — loopback HTTP MCP server hosted in runner-child (zero deps). +- `claude-cli-runner.ts` — `--permission-prompts host --permission-prompt-tool mcp__clockwork__approve --mcp-config `; `MCP_TOOL_TIMEOUT` = run wall-clock; per-server `timeout` = same. +- `runner-child.ts` / `run-manager.ts` — approval hold bounded by the run's remaining wall-clock (was fixed 120s). +- `sandbox.ts` — profile v2, `buildSandboxSpec`, `applySandbox`; all five runners (incl. BYOK bash) route through it; `runner-child` constructs the spec. Guarded by `runner-env-wiring.test.ts` "sandbox wiring". + +## Production-path E2E (runner-child dist, real daemon protocol) + +| Run | Observed | +|---|---| +| deny | `sandbox enabled=true v2` at 0.1s → `permission Bash npm view left-pad version` at 40s → held 12s → deny → model reported the exact deny text, command never ran → `completed`, exit 0. | +| allow | same → held 8s → allow → `npm view` executed inside the production-built profile (cache root + engine paths merged) and returned stdout. | +| **floor bypass** | in the allow run, step 2 `git push --force origin main` **executed without any permission request** (`permission_denials: []`; git failed only because the probe repo had no commits). `evaluateCommand` returns `floor:true` for it — the floor was never consulted. Developer settings had no matching allow rule. `acceptEdits` on 2.1.261 does not prompt for this command. | + +## Not decided here (surfaced for the maker) + +- **Policy-floor coverage under `acceptEdits`** (row above). Options: unattended tasks in `default` mode (the bridge makes that viable now); inject deny-list patterns as CLI `permissions.deny` via `--settings`; or both. + +- **Settings leak.** The run inherits `HOME`, so the CLI loads `~/.claude/settings.json`. Any `permissions.allow` rule there pre-empts the prompt tool. `--setting-sources` (user,project,local) exists in 2.1.261 and can pin what an unattended run loads. Whether interactive allow rules should apply unattended is a product decision. +- `--strict-mcp-config` was **not** added: it would drop the repo's own `.mcp.json` servers, a behaviour change for existing tasks. diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index 9eb9051..c5b752e 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -323,7 +323,7 @@ dependencies = [ [[package]] name = "clockwork" -version = "0.1.0" +version = "0.5.0" dependencies = [ "serde", "serde_json", diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index 07cbb51..e3f5e80 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clockwork" -version = "0.1.0" +version = "0.5.0" description = "The calendar where your agents show up for work." edition = "2021" diff --git a/src-tauri/tauri.conf.json b/src-tauri/tauri.conf.json index 8ec8059..1b5ed02 100644 --- a/src-tauri/tauri.conf.json +++ b/src-tauri/tauri.conf.json @@ -1,7 +1,7 @@ { "$schema": "https://schema.tauri.app/config/2", "productName": "Clockwork", - "version": "0.4.0", + "version": "0.5.0", "identifier": "com.clockwork.app", "build": { "frontendDist": "../packages/ui/dist" From f3b0654bab7a87c87d7958475c2ac276b2a5eb5c Mon Sep 17 00:00:00 2001 From: t Date: Sun, 6 Sep 2026 04:10:40 +0530 Subject: [PATCH 2/4] fix(security): harden the floor hook's deny path and record ADR-035 The PreToolUse floor hook exited right after writing its deny reason to stderr. On macOS a pipe-backed stderr write is asynchronous, so the reason the model reads could be truncated (the exit code, and therefore the deny, was never at risk). The hook now exits from the write callback with a ref'd 1s fallback that still guarantees exit(2). Docs catch up with what ships: docs/security.md closes the "policy floor only sees what the CLI asks about" gap with the 2026-09-06 production probe (npm view still prompts; git push --force is refused with the deny-list reason; ~60 ms per Bash call), README gains the floor bullet, the T-007 matrix gets the "floor closed" row, and ADR-035 records three decisions: the fail-closed hook, exactly one Seatbelt layer for Codex (macOS refuses to nest a profile inside a deny-default one), and TERMINAL_CWD for Hermes whose oneshot path ignores --in. Co-Authored-By: Claude Fable 5.1 --- README.md | 3 + decisions/DECISIONS.md | 12 ++++ docs/security.md | 61 ++++++++++++------- packages/runner/src/floor-hook.ts | 8 ++- .../T007-engine-contract-matrix-2.1.261.md | 3 +- 5 files changed, 63 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index bb4c754..12b325e 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,9 @@ REPEAT Make it weekly. Search your retained run history. safety rails, and an output contract - 🛡 **Human-in-the-loop approvals** — risky actions pause the run and ask you; unanswered asks fail safe (never silently approved) +- 🧱 **Policy floor in every mode** — force-pushes to protected branches and + package publishing are refused before they run on the Claude engine, even when + the CLI would not have asked (a `PreToolUse` hook, fail-closed, ~60 ms per call) - 🔎 **Searchable execution history** — FTS across every report and transcript; ⌘K command palette everywhere - 💰 **Budget enforcement by the supervisor** — USD soft cap, turn limits, diff --git a/decisions/DECISIONS.md b/decisions/DECISIONS.md index 7c6c4aa..8468307 100644 --- a/decisions/DECISIONS.md +++ b/decisions/DECISIONS.md @@ -190,3 +190,15 @@ ADR-style, append-only. Format: Decision → Context → Alternatives rejected **Alternatives rejected:** stdio MCP bridge (the CLI spawns it INSIDE the sandbox with stdio owned by the CLI → needs a side channel and a profile allow for it); extending the fixed 120s window (still a lie about "holds"); `--strict-mcp-config` (drops the repo's own `.mcp.json` servers — behaviour change for existing tasks); falling back to an unsandboxed spawn when the profile is refused (fail-open); a blanket allow on `/tmp` for the CLI's cwd file (the regex admits one filename and was tested against near-misses). **Why:** The launch's credibility argument is "check it rather than believe it". Every one of the three gaps was a missing call site in front of working, tested code — so the fix is wiring plus guards that read the sources (`runner-env-wiring.test.ts` "sandbox wiring") so the call sites cannot silently disappear again. **Consequence:** T-003/T-201 unblocked for the CLI engine; the composer may offer `default` permission mode meaningfully. `docs/security.md`, README, `plan/STATUS.md` T-111 corrected to describe what ships. Open, surfaced not decided: the run inherits `HOME`, so `~/.claude/settings.json` `permissions.allow` rules pre-empt the prompt tool — `--setting-sources` (2.1.261) can pin what an unattended run loads. ADR-026…033 are cited in code but never written here; this entry takes 034 to avoid collision. Re-run the matrix on every observed CLI/engine version change (unchanged from ADR-020). + +## ADR-035 — The policy floor as a fail-closed PreToolUse hook; exactly one Seatbelt layer; engine cwd pinning + +**Decision:** (a) Every Claude CLI run injects a `PreToolUse` hook (matcher `Bash`) via `--settings`; the generated hook (`packages/runner/src/floor-hook.ts`) POSTs `{tool_name, tool_input}` to the permission bridge's `/floor` route, the supervisor runs `evaluateCommand`, and a floor hit exits 2 (CLI refuses the call with the reason). Every error path exits 2 — bridge unreachable, malformed input, timeout, watchdog — so the hook is fail-closed and a CLI contract change breaks runs loudly. Floor hits are sent to the daemon (`{t:'floor'}`), recorded as `policy_deny` events and `deny_list_hit` journal entries. (b) Codex runs with `-s danger-full-access` when Clockwork's Seatbelt profile is on and `workspace-write` only under `CW_SANDBOX=off`. (c) `HermesRunner` sets `TERMINAL_CWD` to the worktree. + +**Context:** 2026-09-05/06. The ADR-034 bridge only sees tool calls the CLI chooses to gate; under `acceptEdits` on CLI 2.1.261 `git push --force origin main` ran with no prompt (production probe, `permission_denials: []`). macOS refuses to apply codex's own profile inside ours (`sandbox_apply: Operation not permitted` under any `(deny default)` outer profile; bisected every allow, only `(allow default)` nests). hermes 0.21.0's oneshot (`-z`) path never applies `--in`, so writes resolved against `$HOME` — silently before the sandbox, loudly (EPERM) after it. + +**Alternatives rejected:** running unattended tasks in `default` mode (every call prompts — turns a 2am run into a wall of asks); injecting deny-list patterns as CLI `permissions.deny` rules (pattern dialect differs from ours, two sources of truth, still mode-dependent); a hook that imports the runner's dist (a packaging path baked into a run; an exec failure would be fail-open); exempting codex from the sandbox (loses credential-read denial and the "every engine wrapped" guard); making nesting work (no outer allow unblocks it). + +**Why:** hooks fire in every permission mode, so coverage no longer depends on the CLI's gating heuristics; evaluation stays in the supervisor with the real deny-list; the hook has zero imports beyond `node:http`, so packaging cannot break it. One containment layer that is ours is simpler to reason about than two that fight. Measured cost: ~60 ms per Bash call (Node start + one loopback round trip, median of ten). + +**Consequence:** docs/security.md "Known gap" closed with dated evidence; codex users lose codex's own shell-command network block (network is allowed under Clockwork's profile for every engine — documented). Open: the run inherits `HOME`, so a developer's `permissions.allow` rules and `SessionStart`/`SessionEnd` hooks apply unattended (`--setting-sources` pinning is a product decision); LICENSE §12's audit set does not yet list `permission-server.ts` or `floor-hook.ts`. diff --git a/docs/security.md b/docs/security.md index 0882a02..f2feb0b 100644 --- a/docs/security.md +++ b/docs/security.md @@ -74,7 +74,9 @@ This bounds *mistakes*, not adversaries. A global-only deny-list blocks force-pushes to protected branches, package publishing, and credential-path access attempts, and journals every hit -(FR-27). It shapes normal behavior; it does NOT stop a determined adversarial +(FR-27). On the Claude engine it is consulted for every Bash call through the +`PreToolUse` hook described below; other engines have no per-command hook and +rely on the sandbox. It shapes normal behavior; it does NOT stop a determined adversarial prompt from routing around patterns. The sandbox above is what bounds damage. `bypassPermissions` is not offered in H1. Permission modes are `plan` @@ -107,26 +109,43 @@ visible to the model, so the agent cannot approve itself. Codex, OpenCode and Hermes expose no permission hook; for them the sandbox and budgets are the containment, and the UI hides approval affordances. There is no -"SDK engine". - -**Known gap, not yet closed — the policy floor only sees what the CLI asks -about.** Probed 2026-09-05 through the production runner on CLI 2.1.261 under -`acceptEdits`: `npm view left-pad version` was sent to the prompt tool; **`git -push --force origin main` was executed with no prompt at all** (transcript shows -the tool call and git's error; `permission_denials` empty). Clockwork's deny-list -would have refused it (`floor:true`, "force-push to protected branch") — it was -never consulted. The developer's `~/.claude/settings.json` had no matching allow -rule, so this is the CLI's own `acceptEdits` behaviour, not a settings leak. The -run also inherits `HOME`, so any `permissions.allow` rule in that file would be -honoured before the prompt tool too. - -Until this is closed, "a policy floor denies catastrophic commands" is true only -for commands the CLI chooses to prompt for. Candidate fixes, for the maker to -choose: run unattended tasks in `default` mode now that the bridge exists (every -non-allowlisted call is asked, so the floor evaluates everything — at the cost of -more prompts); inject the floor as CLI `permissions.deny` rules via `--settings`; -pin `--setting-sources` so a developer's interactive allow rules never apply to -an unattended run. +"SDK engine". Two engine specifics (ADR-035): **Codex runs with its own +`workspace-write` sandbox off while Clockwork's is on** — macOS refuses to apply a +second Seatbelt inside a `(deny default)` profile (`sandbox_apply: Operation not +permitted`, every allow bisected), so exactly one layer applies and it is ours; +only `CW_SANDBOX=off` falls back to codex's. Codex's inner sandbox also blocked +shell-command network; under Clockwork's profile network is allowed, as for every +engine. **Hermes 0.21.0's oneshot path ignores `--in`**, so `HermesRunner` sets +`TERMINAL_CWD` to the worktree; before this its file writes landed in `$HOME`. + +**Closed 2026-09-06 — the policy floor now sees every Bash call (ADR-035).** +Probed 2026-09-05 through the production runner on CLI 2.1.261 under +`acceptEdits`: `npm view left-pad version` was sent to the prompt tool, but +**`git push --force origin main` executed with no prompt at all** — the CLI's own +`acceptEdits` behaviour, not a settings leak. The deny-list would have refused it +and was never consulted. + +The fix: every run injects a Claude Code `PreToolUse` hook (matcher `Bash`) +through `--settings`. The hook (`packages/runner/src/floor-hook.ts`, generated +per run next to the MCP config, no imports beyond `node:http`) posts the command +to the bridge's `/floor` route, where the supervisor runs `evaluateCommand`; a +floor hit exits 2 and the CLI refuses the call with the reason. It is +**fail-closed**: bridge unreachable, malformed input, timeout — every error path +also exits 2, so a CLI format change breaks runs loudly instead of silently +un-protecting them. Hooks fire in every permission mode, so coverage no longer +depends on what the CLI chooses to prompt for. Cost per Bash call: about 60 ms +(Node start plus one loopback round trip, median of ten). + +Verified 2026-09-06 through the production `runner-child`: `npm view …` still +prompted (bridge intact), `git push --force origin main` came back as +"force-push to protected branch 'main' is blocked by global deny-list" and never +ran; recorded as a `policy_deny` event and a `deny_list_hit` journal entry. + +**Still open, surfaced not hidden:** the run inherits `HOME`, so +`permissions.allow` rules in `~/.claude/settings.json` and the developer's own +`SessionStart`/`SessionEnd` hooks apply to unattended runs. The floor hook holds +regardless (deny beats allow in the CLI's hook precedence); pinning +`--setting-sources` is a product decision left open. ## Reporting a security issue diff --git a/packages/runner/src/floor-hook.ts b/packages/runner/src/floor-hook.ts index 916a772..c636046 100644 --- a/packages/runner/src/floor-hook.ts +++ b/packages/runner/src/floor-hook.ts @@ -54,8 +54,12 @@ const POST_TIMEOUT_MS = ${POST_TIMEOUT_MS}; const WATCHDOG_MS = ${WATCHDOG_MS}; function deny(reason) { - process.stderr.write(String(reason) + '\\n'); - process.exit(2); + // On macOS a pipe-backed stderr write is asynchronous, so exiting right + // after write() can truncate the reason the model reads. Exit from the + // write callback; the ref'd fallback still guarantees exit(2) if the + // callback never fires (stderr closed). Either way the exit code is 2. + setTimeout(() => process.exit(2), 1000); + process.stderr.write(String(reason) + '\\n', () => process.exit(2)); } // A hang anywhere below (stdin never closes, a socket wedges past its own diff --git a/spikes/reports/T007-engine-contract-matrix-2.1.261.md b/spikes/reports/T007-engine-contract-matrix-2.1.261.md index 5d6951b..bc4084a 100644 --- a/spikes/reports/T007-engine-contract-matrix-2.1.261.md +++ b/spikes/reports/T007-engine-contract-matrix-2.1.261.md @@ -49,10 +49,11 @@ With both fixed, inside the sandbox on 2.1.261: `touch` in worktree ✅ created | deny | `sandbox enabled=true v2` at 0.1s → `permission Bash npm view left-pad version` at 40s → held 12s → deny → model reported the exact deny text, command never ran → `completed`, exit 0. | | allow | same → held 8s → allow → `npm view` executed inside the production-built profile (cache root + engine paths merged) and returned stdout. | | **floor bypass** | in the allow run, step 2 `git push --force origin main` **executed without any permission request** (`permission_denials: []`; git failed only because the probe repo had no commits). `evaluateCommand` returns `floor:true` for it — the floor was never consulted. Developer settings had no matching allow rule. `acceptEdits` on 2.1.261 does not prompt for this command. | +| **floor closed** (2026-09-06) | same prompt through the production `runner-child` with the PreToolUse hook wired: `npm view` prompted at 29.7s (held 8s, allowed, ran); `git push --force origin main` → `floor` message at 95.8s with "force-push to protected branch 'main' is blocked by global deny-list", never executed; `echo done` ran; `completed`, exit 0. Hook cost: 60 ms median per Bash call (10 allow calls: 55–72 ms; deny 59 ms, full reason on stderr). | ## Not decided here (surfaced for the maker) -- **Policy-floor coverage under `acceptEdits`** (row above). Options: unattended tasks in `default` mode (the bridge makes that viable now); inject deny-list patterns as CLI `permissions.deny` via `--settings`; or both. +- ~~Policy-floor coverage under `acceptEdits`~~ — closed by the PreToolUse hook (ADR-035); see the "floor closed" row. - **Settings leak.** The run inherits `HOME`, so the CLI loads `~/.claude/settings.json`. Any `permissions.allow` rule there pre-empts the prompt tool. `--setting-sources` (user,project,local) exists in 2.1.261 and can pin what an unattended run loads. Whether interactive allow rules should apply unattended is a product decision. - `--strict-mcp-config` was **not** added: it would drop the repo's own `.mcp.json` servers, a behaviour change for existing tasks. From 067dda2e32f438f9e40d25fc3a18b19204ffe29d Mon Sep 17 00:00:00 2001 From: t Date: Sun, 6 Sep 2026 04:58:16 +0530 Subject: [PATCH 3/4] fix(security): pin disableAllHooks off and move the BYOK credential off the environment Two findings from the pre-release review, both reproduced on this Mac. A repo committing {"disableAllHooks": true} in .claude/settings.json switched the PreToolUse floor hook off, so under acceptEdits a floor-listed command ran unasked while the report still said sandboxed: true. The --settings payload now pins disableAllHooks: false; CLI-flag settings outrank project settings, so the pin wins. Probed on CLI 2.1.261: unpinned payload, hook never ran and the command ran; pinned payload, hook ran and the command was blocked. Unit test pins the key. The BYOK provider key travelled in runner-child's environment and was deleted after being read. macOS keeps a process's exec-time env readable by any same-user process through sysctl KERN_PROCARGS2, and the profile must allow sysctl-read, so a sandboxed agent read the key from a sibling child after the delete. The daemon now writes {t:'credential'} to the child's stdin pipe right after spawn, only for BYOK runs; the env never contains it. The child races the message against a 15s timeout and fails with the same auth text either way (verified against the built child: 0.1s with an empty message, 15.1s with none). Also from review: codex/opencode/hermes/api-agent runners now remove their per-run cw-sb-* profile dir on close, error and spawn failure (the api agent leaked one per tool call), and the three CLI runners gain the child 'error' handler they lacked. docs/security.md names the remaining gaps (Bash-only hook matcher vs repo MCP servers, no bearer token on the loopback bridge); ADR-035 records both findings. Runner 124/124, daemon 130/130, typecheck and build clean. Co-Authored-By: Claude Fable 5.1 --- decisions/DECISIONS.md | 4 +- docs/security.md | 30 ++- packages/daemon/src/run-manager.ts | 59 +++-- packages/daemon/src/runner-child.ts | 39 ++- packages/daemon/src/runner-protocol.ts | 7 +- .../daemon/test/credential-channel.test.ts | 233 ++++++++++++++++++ packages/runner/src/api-agent-runner.ts | 25 +- packages/runner/src/codex-runner.ts | 36 ++- packages/runner/src/floor-hook.ts | 13 +- packages/runner/src/hermes-runner.ts | 29 ++- packages/runner/src/opencode-runner.ts | 35 ++- .../api-agent-run-command-sandbox.test.ts | 131 ++++++++++ packages/runner/test/floor-hook.test.ts | 13 +- .../runner/test/runner-env-wiring.test.ts | 51 +++- packages/shared/src/schemas.ts | 2 +- 15 files changed, 662 insertions(+), 45 deletions(-) create mode 100644 packages/daemon/test/credential-channel.test.ts create mode 100644 packages/runner/test/api-agent-run-command-sandbox.test.ts diff --git a/decisions/DECISIONS.md b/decisions/DECISIONS.md index 8468307..70d7ca2 100644 --- a/decisions/DECISIONS.md +++ b/decisions/DECISIONS.md @@ -201,4 +201,6 @@ ADR-style, append-only. Format: Decision → Context → Alternatives rejected **Why:** hooks fire in every permission mode, so coverage no longer depends on the CLI's gating heuristics; evaluation stays in the supervisor with the real deny-list; the hook has zero imports beyond `node:http`, so packaging cannot break it. One containment layer that is ours is simpler to reason about than two that fight. Measured cost: ~60 ms per Bash call (Node start + one loopback round trip, median of ten). -**Consequence:** docs/security.md "Known gap" closed with dated evidence; codex users lose codex's own shell-command network block (network is allowed under Clockwork's profile for every engine — documented). Open: the run inherits `HOME`, so a developer's `permissions.allow` rules and `SessionStart`/`SessionEnd` hooks apply unattended (`--setting-sources` pinning is a product decision); LICENSE §12's audit set does not yet list `permission-server.ts` or `floor-hook.ts`. +**Consequence:** docs/security.md "Known gap" closed with dated evidence; codex users lose codex's own shell-command network block (network is allowed under Clockwork's profile for every engine — documented). Open: the run inherits `HOME`, so a developer's `permissions.allow` rules and `SessionStart`/`SessionEnd` hooks apply unattended (`--setting-sources` pinning is a product decision); repo-declared MCP servers that run shell are not matched by the `Bash` matcher; the bridge has no per-run bearer token yet; LICENSE §12's audit set does not yet list `permission-server.ts` or `floor-hook.ts`. + +**Review findings folded in (2026-09-06, Opus read-only pass, both reproduced on this machine):** (1) the `--settings` payload must pin `disableAllHooks: false` — the CLI honours that switch from a repo's own `.claude/settings.json`, and without the pin one committed key disabled the hook while the report still said `sandboxed: true`; CLI-flag settings outrank project settings, so the pin wins (unit-tested in `floor-hook.test.ts`). (2) The BYOK provider key must not travel in the child's environment at all: macOS keeps a process's exec-time env readable via `sysctl KERN_PROCARGS2`, the profile must allow `sysctl-read` (Node needs it), and a sandboxed agent read the key out of a sibling `runner-child` after it had been deleted from `process.env`. The credential now arrives over the daemon⇄child stdin channel as a `credential` message; the env never contains it. diff --git a/docs/security.md b/docs/security.md index f2feb0b..d4a92e3 100644 --- a/docs/security.md +++ b/docs/security.md @@ -74,9 +74,11 @@ This bounds *mistakes*, not adversaries. A global-only deny-list blocks force-pushes to protected branches, package publishing, and credential-path access attempts, and journals every hit -(FR-27). On the Claude engine it is consulted for every Bash call through the -`PreToolUse` hook described below; other engines have no per-command hook and -rely on the sandbox. It shapes normal behavior; it does NOT stop a determined adversarial +(FR-27). On the Claude engine its **floor** entries (force-push to a protected +branch, package publishing) are enforced for every Bash call through the +`PreToolUse` hook described below; non-floor entries still only apply to calls +the CLI chooses to prompt for. Other engines have no per-command hook and rely +on the sandbox. It shapes normal behavior; it does NOT stop a determined adversarial prompt from routing around patterns. The sandbox above is what bounds damage. `bypassPermissions` is not offered in H1. Permission modes are `plan` @@ -96,6 +98,11 @@ jobs. Imported templates/profiles arrive disabled with a security preview. - Delivery credentials live outside the daemon DB (keychain at packaging; env/ file bridge for CLI installs) and are **never visible to runner processes** — runners get sanitized envs and stdio IPC only. +- BYOK provider keys travel to the runner over the daemon⇄child **stdin + channel, never the environment**. A process's exec-time environment stays + readable through `sysctl` (`KERN_PROCARGS2`) by any same-user process, + sandboxed or not, so an env var that is "read then deleted" is not a boundary + (found in review 2026-09-06; the transport is the fix, the profile cannot be). - The local API is loopback-only with a bearer token stored `0600`. ## Engine capability honesty @@ -133,8 +140,12 @@ floor hit exits 2 and the CLI refuses the call with the reason. It is **fail-closed**: bridge unreachable, malformed input, timeout — every error path also exits 2, so a CLI format change breaks runs loudly instead of silently un-protecting them. Hooks fire in every permission mode, so coverage no longer -depends on what the CLI chooses to prompt for. Cost per Bash call: about 60 ms -(Node start plus one loopback round trip, median of ten). +depends on what the CLI chooses to prompt for. The payload also pins +`disableAllHooks: false`: the CLI honours that switch from a repo's own +`.claude/settings.json`, and without the pin one committed key turned the floor +off while the report still said `sandboxed: true` (probed 2026-09-06; CLI-flag +settings outrank project settings, so the pin wins). Cost per Bash call: about +60 ms (Node start plus one loopback round trip, median of ten). Verified 2026-09-06 through the production `runner-child`: `npm view …` still prompted (bridge intact), `git push --force origin main` came back as @@ -145,7 +156,14 @@ ran; recorded as a `policy_deny` event and a `deny_list_hit` journal entry. `permissions.allow` rules in `~/.claude/settings.json` and the developer's own `SessionStart`/`SessionEnd` hooks apply to unattended runs. The floor hook holds regardless (deny beats allow in the CLI's hook precedence); pinning -`--setting-sources` is a product decision left open. +`--setting-sources` is a product decision left open. The hook matches the +`Bash` tool only: an MCP server the repo declares in `.mcp.json` (kept on +purpose — `--strict-mcp-config` would drop it) that runs shell on the agent's +behalf is not matched, so the floor does not see those calls; the sandbox still +bounds them. The permission bridge listens on loopback without a bearer token, so +a sandboxed agent that reads the port from its own argv could post fake approval +prompts into the inbox (noise, not an escalation — decisions route back by +request id); a per-run header is the planned fix. ## Reporting a security issue diff --git a/packages/daemon/src/run-manager.ts b/packages/daemon/src/run-manager.ts index 774e1b3..eae9461 100644 --- a/packages/daemon/src/run-manager.ts +++ b/packages/daemon/src/run-manager.ts @@ -207,6 +207,11 @@ export class RunManager { writeFileSync(specPath, JSON.stringify(spec)); const nonce = newId(); + // BYOK credential resolution happens here, in the daemon, exactly as before + // (ADR-027/028) — but the result is held in a LOCAL and delivered to the + // child over stdin after spawn, never through env. See the doc comment on + // resolveByokCredential for why an env var is not a security boundary here. + const byokCredential = this.resolveByokCredential(spec); // Sanitized env (arch §7.3): nothing but the minimum. No bearer token, no delivery creds. // USER/LOGNAME required for macOS keychain ACL identification (verified 2026-08-21). const env: Record = { @@ -217,7 +222,6 @@ export class RunManager { CW_ENGINE: process.env.CW_ENGINE ?? '', // test hook only ...(process.env.CW_MOCK_STEP_MS ? { CW_MOCK_STEP_MS: process.env.CW_MOCK_STEP_MS } : {}), // test hook ...(process.env.CW_SANDBOX ? { CW_SANDBOX: process.env.CW_SANDBOX } : {}), // escape hatch; journaled + stamped on the report - ...this.byokEnv(spec), // BYOK credential injection (ADR-027/028) — keychain read happens here, in the daemon ...(process.env.USER ? { USER: process.env.USER } : {}), ...(process.env.LOGNAME ? { LOGNAME: process.env.LOGNAME } : {}), }; @@ -237,6 +241,24 @@ export class RunManager { const pgid = child.pid!; this.liveChildren.set(runId, child); + // ADR-035: BYOK credential travels over stdin, never env. macOS exposes a + // process's exec-time environment to any other same-user process via + // sysctl KERN_PROCARGS2 — sandboxed or not, since Node itself needs + // sysctl-read to run, so the Seatbelt profile cannot close that door. stdin + // is a pipe only the daemon holds the write end of, so it is the actual + // boundary. Safe to write before 'ready': stdin is a pipe and the child's + // readline reader is listening on 'line' from the moment it starts up, so + // nothing here is lost to a race. Never logged, never journaled. + if (byokCredential) { + try { + child.stdin!.write( + JSON.stringify({ t: 'credential', byokKey: byokCredential.byokKey, byokBaseUrl: byokCredential.byokBaseUrl }) + '\n', + ); + } catch { + /* broken pipe — runner-child's credential wait times out and fails the run safely */ + } + } + this.deps.db .prepare('UPDATE runs SET pid=?, pgid=?, proc_started_at=?, heartbeat_at=?, journal_path=?, started_at=?, state=? , state_changed_at=? WHERE id=?') .run(child.pid, pgid, now, now, path.join(runDir, 'stream.jsonl'), now, 'running', now, runId); @@ -759,29 +781,38 @@ export class RunManager { } /** - * BYOK credential env (ADR-027/028). Resolved lazily at spawn in the daemon - * process; the secret travels only via child env, never the jobspec file. - * Returns {} when the task is not a BYOK task or resolution fails (the run - * will then fail fast with an auth error inside the runner). + * BYOK credential resolution (ADR-027/028; delivery moved to stdin under + * ADR-035 — see spawnChild). Resolved lazily at spawn in the daemon process; + * the secret is held in a local and never written to the jobspec file, env, + * a log line, or recordEvent/journal. + * + * Returns null when the task is not a BYOK task at all (spawnChild uses that + * to decide whether to write a 'credential' message — never for a non-BYOK + * run). Returns an object with empty fields when the task IS a BYOK task but + * resolution fails (config missing, env var unset, keychain miss) — the + * child still gets an explicit 'credential' message, so it fails fast with + * the same auth error as before rather than waiting out the no-message + * timeout. */ - private byokEnv(spec: JobSpec): Record { - const byokId = (spec as unknown as { byokId?: string | null }).byokId; - if (!byokId) return {}; + private resolveByokCredential(spec: JobSpec): { byokKey: string; byokBaseUrl: string } | null { + const byokId = spec.byokId; + if (!byokId) return null; try { const store = new ByokStore({ db: this.deps.db }); const cfg = store.get(byokId); - if (!cfg) return {}; - const envOut: Record = { CW_BYOK_BASE_URL: store.baseUrlFor(cfg) }; + if (!cfg) return { byokKey: '', byokBaseUrl: '' }; + const byokBaseUrl = store.baseUrlFor(cfg); + let byokKey = ''; if (cfg.auth === 'env' && cfg.env_var && process.env[cfg.env_var]) { - envOut.CW_BYOK_KEY = process.env[cfg.env_var] as string; + byokKey = process.env[cfg.env_var] as string; } else if (cfg.auth === 'keychain') { try { - envOut.CW_BYOK_KEY = keychainGet(cfg.id); + byokKey = keychainGet(cfg.id); } catch { /* absent key → runner fails fast with auth */ } } - return envOut; + return { byokKey, byokBaseUrl }; } catch { - return {}; + return { byokKey: '', byokBaseUrl: '' }; } } diff --git a/packages/daemon/src/runner-child.ts b/packages/daemon/src/runner-child.ts index dbab59e..f7d7aa5 100644 --- a/packages/daemon/src/runner-child.ts +++ b/packages/daemon/src/runner-child.ts @@ -33,13 +33,22 @@ if (!specPath || !nonce) { const job = JSON.parse(readFileSync(specPath, 'utf8')) as JobSpec; const startedAtMs = Date.now(); -// BYOK credential (ADR-027/028) arrives in THIS process's env only. Read it once -// and scrub it, so nothing this child spawns — the agent's own shell included — -// can `echo $CW_BYOK_KEY`. Before this scrub, it could. -const byokKey = process.env.CW_BYOK_KEY ?? ''; -const byokBaseUrl = process.env.CW_BYOK_BASE_URL ?? ''; -delete process.env.CW_BYOK_KEY; -delete process.env.CW_BYOK_BASE_URL; +// BYOK credential delivery (ADR-035). WHY stdin and not env: macOS exposes a +// process's exec-time argv/env to any OTHER same-user process — sandboxed or +// not — via sysctl KERN_PROCARGS2. The Seatbelt profile has to allow +// sysctl-read (Node needs it to run at all), so a sibling runner-child, even +// one contained by its own profile, can read a credential that was ever +// placed in this process's env, no matter how quickly it is deleted +// afterward — a scrub only stops future children spawned FROM here, not a +// third party sampling this process's own KERN_PROCARGS2 record. Only a +// transport neither process ever puts in its argv/env closes that: the +// existing daemon<->child JSONL channel over a pipe that only the daemon +// holds the write end of. +const CREDENTIAL_TIMEOUT_MS = 15_000; +let resolveCredential!: (c: { byokKey: string; byokBaseUrl: string }) => void; +const credential = new Promise<{ byokKey: string; byokBaseUrl: string }>((resolve) => { + resolveCredential = resolve; +}); function send(msg: ChildToDaemon): void { try { @@ -67,6 +76,8 @@ rl.on('line', (line) => { pendingPermissions.delete(msg.reqId); resolve(msg.behavior === 'allow' ? { behavior: 'allow' } : { behavior: 'deny', message: msg.message }); } + } else if (msg.t === 'credential') { + resolveCredential({ byokKey: msg.byokKey, byokBaseUrl: msg.byokBaseUrl }); } }); @@ -183,10 +194,18 @@ async function main(): Promise { let outcome: RunOutcome; try { if ((job as any).byokId) { - // ADR-028: BYOK API-agent execution. Credential arrives via env - // (CW_BYOK_KEY / CW_BYOK_BASE_URL), injected by the daemon at spawn time; - // it is never written to the jobspec file or logs. + // ADR-028/035: BYOK API-agent execution. Credential arrives over stdin + // (the 'credential' message), injected by the daemon right after spawn; + // it is never written to the jobspec file, env, or logs. Bounded wait so + // a stdin write that never arrives (broken pipe, protocol bug) fails the + // run instead of hanging it. const { runApiAgent } = await import('@clockwork/runner'); + const cred = await Promise.race([ + credential, + new Promise((resolve) => setTimeout(() => resolve(null), CREDENTIAL_TIMEOUT_MS)), + ]); + const byokKey = cred?.byokKey ?? ''; + const byokBaseUrl = cred?.byokBaseUrl ?? ''; if (!byokKey || !byokBaseUrl) { outcome = { state: 'failed', failureReason: 'auth', summary: 'BYOK credential not provided to runner', artifacts: [], costUsd: 0, turns: 0 }; } else { diff --git a/packages/daemon/src/runner-protocol.ts b/packages/daemon/src/runner-protocol.ts index 1b0c6d4..b6686b7 100644 --- a/packages/daemon/src/runner-protocol.ts +++ b/packages/daemon/src/runner-protocol.ts @@ -23,5 +23,8 @@ export type ChildToDaemon = | { t: 'outcome'; outcome: RunOutcome }; export type DaemonToChild = - | { t: 'decision'; reqId: string; behavior: 'allow' } - | { t: 'decision'; reqId: string; behavior: 'deny'; message: string }; + | { t: 'decision'; reqId: string; behavior: 'allow' } + | { t: 'decision'; reqId: string; behavior: 'deny'; message: string } + /** BYOK credential delivery (ADR-034): stdin, never env — see run-manager.ts + * spawnChild and runner-child.ts's credential promise for why. */ + | { t: 'credential'; byokKey: string; byokBaseUrl: string }; diff --git a/packages/daemon/test/credential-channel.test.ts b/packages/daemon/test/credential-channel.test.ts new file mode 100644 index 0000000..77ce1d4 --- /dev/null +++ b/packages/daemon/test/credential-channel.test.ts @@ -0,0 +1,233 @@ +/** + * ADR-034: the BYOK credential must travel to runner-child over stdin, never + * through the spawned child's env — macOS exposes a process's exec-time env + * to any other same-user process via sysctl KERN_PROCARGS2 (the Seatbelt + * profile has to allow sysctl-read for Node to run at all, so it cannot close + * that door). A `delete process.env.*` scrub inside the child is not a + * boundary against a sibling process reading the ORIGINAL env this process + * was exec'd with. + * + * This test drives the REAL RunManager.spawnChild() code path — real DB, real + * ByokStore credential resolution, real env construction — and intercepts only + * node:child_process's `spawn` (kept real is spawnSync, used elsewhere by + * RunManager for identity-verified kills, unrelated to this path) so we can + * observe, without needing a live provider or a real subprocess, (a) the exact + * env object RunManager hands to spawn(), and (b) the exact bytes written to + * the child's stdin. A real runner-child (as full-loop.test.ts drives) cannot + * prove env absence from outside the process without shelling out to + * KERN_PROCARGS2 itself, which is what this finding is ABOUT — spying on the + * daemon's own spawn call is the deterministic way to assert the production + * code path never puts the secret where the finding says it leaked from. + */ +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; +import { mkdtempSync, mkdirSync, rmSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { openDatabase, createMigrator, loadMigrationsFrom, type DB } from '../src/db.js'; +import { RunManager } from '../src/run-manager.js'; +import { FakeClock } from '../src/clock.js'; +import { ByokStore } from '../src/byok.js'; +import { SafetyJournal } from '@clockwork/runner'; + +// ---- fake node:child_process.spawn ---------------------------------------- +const captured = vi.hoisted(() => ({ + calls: [] as Array<{ env: Record; child: FakeChild }>, +})); +interface FakeChild { + pid: number; + stdin: { writable: boolean; writes: string[]; write: (d: string) => boolean }; + stdout: unknown; + stderr: unknown; + on: (...a: unknown[]) => unknown; +} +let nextPid = 424242; + +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal(); + const { EventEmitter } = await import('node:events'); + const { PassThrough } = await import('node:stream'); + return { + ...actual, + spawn: (_bin: string, _args: string[], opts: { env?: Record }) => { + const emitter = new EventEmitter(); + const writes: string[] = []; + const child = Object.assign(emitter, { + pid: nextPid++, + stdin: { writable: true, writes, write: (d: string) => { writes.push(d); return true; } }, + stdout: new PassThrough(), + stderr: new PassThrough(), + }) as unknown as FakeChild; + captured.calls.push({ env: opts.env ?? {}, child }); + return child; + }, + }; +}); + +let db: DB; +let dir: string; +let rm: RunManager; +let clock: FakeClock; + +function seedTask(name: string): { id: string } { + const now = Date.now(); + const id = `t-${Math.random().toString(36).slice(2, 10)}`; + db.prepare( + `INSERT INTO tasks (id, name, prompt, repo_path, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?)`, + ).run(id, name, 'do the thing', null, now, now); + return { id }; +} + +function enqueueScratchRun(taskId: string, byokId: string | null): string { + const runId = `r-${Math.random().toString(36).slice(2, 10)}`; + const scratchPath = path.join(dir, 'scratch', runId); + const spec = { + runId, + taskId, + taskName: taskId, + taskSlug: taskId, + prompt: 'do the thing', + engine: 'cli', + model: null, + byokId, + permissionMode: 'acceptEdits', + budget: { maxUsd: 2, maxTurns: 50, timeoutSec: 60 }, + repoPath: null, + baseBranch: null, + worktreePath: scratchPath, + branch: `clockwork/${taskId}/x`, + scratchPath, + profile: null, + contextFiles: [], + occurrenceAt: Date.now(), + scheduledFor: Date.now(), + createdAt: Date.now(), + }; + const now = Date.now(); + db.prepare( + `INSERT INTO runs (id, task_id, occurrence_at, jobspec_json, state, state_changed_at, scheduled_for) VALUES (?, ?, ?, ?, 'queued', ?, ?)`, + ).run(runId, taskId, now, JSON.stringify(spec), now, now); + return runId; +} + +function waitFor(predicate: () => boolean, timeoutMs = 5_000): Promise { + return new Promise((resolve, reject) => { + const t0 = Date.now(); + const timer = setInterval(() => { + if (predicate()) { + clearInterval(timer); + resolve(); + } else if (Date.now() - t0 > timeoutMs) { + clearInterval(timer); + reject(new Error('waitFor timeout')); + } + }, 20); + }); +} + +beforeAll(() => { + dir = mkdtempSync(path.join(os.tmpdir(), 'cw-credchannel-')); + mkdirSync(path.join(dir, 'scratch'), { recursive: true }); + const opened = openDatabase(path.join(dir, 'data')); + db = opened.db; + createMigrator(db, loadMigrationsFrom(path.resolve(import.meta.dirname, '../migrations'))).migrate(); + clock = new FakeClock(Date.now()); + rm = new RunManager({ + db, + clock, + dataDir: path.join(dir, 'data'), + runnerChildModule: path.resolve(import.meta.dirname, '../src/runner-child.ts'), + childCommandPrefix: [path.resolve(import.meta.dirname, '../node_modules/.bin/tsx')], + // Every run in this suite is spawned via the mocked spawn() and never + // reaches a terminal state (no 'outcome' message is ever sent), so each + // `it()`'s run stays 'running' in the DB forever. A low maxParallel would + // starve later tests on the mutex-free slot count; this suite only ever + // has a handful of runs total, so a generous cap avoids that entirely. + maxParallel: 10, + notify: () => {}, + broadcast: () => {}, + safetyJournal: new SafetyJournal(path.join(dir, 'journal.jsonl')), + }); +}); + +afterEach(() => { + captured.calls.length = 0; +}); + +afterAll(() => { + db.close(); + rmSync(dir, { recursive: true, force: true }); +}); + +describe('BYOK credential delivery: stdin, never env (ADR-034)', () => { + it('a BYOK run gets no CW_BYOK_KEY/CW_BYOK_BASE_URL in its spawned env, and the credential arrives as a stdin line', async () => { + process.env.CW_TEST_BYOK_SECRET = 'sk-test-shhh-do-not-log'; + const store = new ByokStore({ db: db as unknown as { prepare: (s: string) => any; transaction?: (fn: () => void) => unknown } }); + const cfg = store.create({ + kind: 'openai', + auth: 'env', + envVar: 'CW_TEST_BYOK_SECRET', + defaultModel: 'gpt-5-mini', + }); + + const task = seedTask('byok-task'); + enqueueScratchRun(task.id, cfg.id); + + rm.pump(); + await waitFor(() => captured.calls.length > 0); + const { env, child } = captured.calls[0]!; + + expect(Object.keys(env)).not.toContain('CW_BYOK_KEY'); + expect(Object.keys(env)).not.toContain('CW_BYOK_BASE_URL'); + + const credentialLines = child.stdin.writes + .map((w) => { try { return JSON.parse(w); } catch { return null; } }) + .filter((m): m is { t: string; byokKey?: string; byokBaseUrl?: string } => !!m && m.t === 'credential'); + expect(credentialLines.length).toBe(1); + expect(credentialLines[0]!.byokKey).toBe('sk-test-shhh-do-not-log'); + expect(credentialLines[0]!.byokBaseUrl).toBe('https://api.openai.com/v1'); + + delete process.env.CW_TEST_BYOK_SECRET; + }); + + it('a non-BYOK run never gets a credential message on its stdin', async () => { + const task = seedTask('plain-task'); + enqueueScratchRun(task.id, null); + + rm.pump(); + await waitFor(() => captured.calls.length > 0); + const { env, child } = captured.calls[0]!; + + expect(Object.keys(env)).not.toContain('CW_BYOK_KEY'); + expect(Object.keys(env)).not.toContain('CW_BYOK_BASE_URL'); + const credentialLines = child.stdin.writes + .map((w) => { try { return JSON.parse(w); } catch { return null; } }) + .filter((m): m is { t: string } => !!m && m.t === 'credential'); + expect(credentialLines.length).toBe(0); + }); + + it('a BYOK run whose credential cannot be resolved (env var unset) still gets an explicit empty credential message, not silence', async () => { + // No process.env var set for this config's env_var — resolution fails. + const store = new ByokStore({ db: db as unknown as { prepare: (s: string) => any; transaction?: (fn: () => void) => unknown } }); + const cfg = store.create({ + kind: 'openai', + auth: 'env', + envVar: 'CW_TEST_BYOK_SECRET_MISSING', + defaultModel: 'gpt-5-mini', + }); + + const task = seedTask('byok-missing-cred-task'); + enqueueScratchRun(task.id, cfg.id); + + rm.pump(); + await waitFor(() => captured.calls.length > 0); + const { env, child } = captured.calls[0]!; + + expect(Object.keys(env)).not.toContain('CW_BYOK_KEY'); + expect(Object.keys(env)).not.toContain('CW_BYOK_BASE_URL'); + const credentialLines = child.stdin.writes + .map((w) => { try { return JSON.parse(w); } catch { return null; } }) + .filter((m): m is { t: string; byokKey?: string } => !!m && m.t === 'credential'); + expect(credentialLines.length).toBe(1); + expect(credentialLines[0]!.byokKey).toBe(''); + }); +}); diff --git a/packages/runner/src/api-agent-runner.ts b/packages/runner/src/api-agent-runner.ts index 018e785..6085950 100644 --- a/packages/runner/src/api-agent-runner.ts +++ b/packages/runner/src/api-agent-runner.ts @@ -8,7 +8,8 @@ * Credential is resolved from the BYOK store at run start and injected into the * request only — never logged or persisted. */ -import { writeFileSync } from 'node:fs'; +import { rmSync, writeFileSync } from 'node:fs'; +import path from 'node:path'; import { buildRunEnv } from './run-env.js'; import { applySandbox, toolCacheEnv, type SandboxSpec } from './sandbox.js'; @@ -122,16 +123,25 @@ const TOOLS = [ }, ]; -async function execTool(name: string, args: Record, cwd: string, sandbox: SandboxSpec | null): Promise { +/** Exported for sandbox-cleanup testing (api-agent-run-command-sandbox.test.ts); not part of the daemon<->runner contract. */ +export async function execTool(name: string, args: Record, cwd: string, sandbox: SandboxSpec | null): Promise { const { execFile } = await import('node:child_process'); const { promisify } = await import('node:util'); const run = promisify(execFile); if (name === 'run_command') { const command = args.command ?? ''; + // applySandbox is called once per run_command tool call, so every call + // writes a fresh cw-sb-*/profile.sb under os.tmpdir(). Nothing else in + // this file removed it — a long-running BYOK agent that calls run_command + // repeatedly leaked one profile dir per call. Clean up the ONE dir this + // call created once the call is done, whether it succeeded or threw. + let profilePath: string | null = null; try { // The shell gets the same allowlisted env and Seatbelt wrap as every CLI // engine. Before this it inherited process.env — including the BYOK key. - const wrapped = applySandbox(['/bin/bash', '-c', command], sandbox).argv; + const sandboxResult = applySandbox(['/bin/bash', '-c', command], sandbox); + profilePath = sandboxResult.profilePath; + const wrapped = sandboxResult.argv; const { stdout } = await run(wrapped[0]!, wrapped.slice(1), { cwd, env: buildRunEnv(toolCacheEnv()), @@ -142,10 +152,17 @@ async function execTool(name: string, args: Record, cwd: string, } catch (e) { const err = e as { stdout?: string; stderr?: string; message?: string }; return `EXIT-ERROR: ${err.stderr ?? err.message ?? 'unknown'}`.slice(0, 6000); + } finally { + if (profilePath) { + try { + rmSync(path.dirname(profilePath), { recursive: true, force: true }); + } catch { + /* best-effort; a leaked temp dir is not a run failure */ + } + } } } if (name === 'write_file') { - const path = await import('node:path'); const { existsSync, realpathSync } = await import('node:fs'); // This write happens in the unsandboxed runner-child, so the prefix check // IS the boundary. Resolve symlinks first: a link inside the worktree that diff --git a/packages/runner/src/codex-runner.ts b/packages/runner/src/codex-runner.ts index a30e6ec..3875ae2 100644 --- a/packages/runner/src/codex-runner.ts +++ b/packages/runner/src/codex-runner.ts @@ -7,6 +7,7 @@ * {"type":"turn.failed","error":{"message":"..."}} */ import { spawn, type ChildProcess } from 'node:child_process'; +import { rmSync } from 'node:fs'; import path from 'node:path'; import { BudgetGuard } from './budget-guard.js'; import { classifyError } from './stream-parser.js'; @@ -80,12 +81,26 @@ export class CodexRunner implements AgentRunner { const env = buildRunEnv(toolCacheEnv()); let wrapped: string[]; + let profilePath: string | null = null; try { - wrapped = applySandbox(['codex', ...argv], this.opts.sandbox).argv; + const sandboxResult = applySandbox(['codex', ...argv], this.opts.sandbox); + wrapped = sandboxResult.argv; + profilePath = sandboxResult.profilePath; } catch (e) { resolve({ state: 'failed', failureReason: 'internal', summary: `sandbox profile refused: ${String(e)}`, artifacts: [], costUsd: 0, turns: 0 }); return; } + // Per-run Seatbelt profile dir (cw-sb-*): applySandbox already wrote it to + // disk before spawn; nothing else removes it, so every run leaked one + // until this cleaned up on every exit path (close, error, spawn failure). + const cleanupProfileDir = (): void => { + if (!profilePath) return; + try { + rmSync(path.dirname(profilePath), { recursive: true, force: true }); + } catch { + /* best-effort; a leaked temp dir is not a run failure */ + } + }; const child: ChildProcess = spawn(wrapped[0]!, wrapped.slice(1), { cwd: path.join(ctx.worktreePath), env, @@ -93,6 +108,7 @@ export class CodexRunner implements AgentRunner { stdio: ['ignore', 'pipe', 'pipe'], }); if (!child.pid) { + cleanupProfileDir(); resolve({ state: 'failed', failureReason: 'runner_crashed', @@ -186,11 +202,29 @@ export class CodexRunner implements AgentRunner { const heartbeat = setInterval(() => ctx.io.onHeartbeat(), 15_000); + child.on('error', (err) => { + clearInterval(heartbeat); + clearTimeout(timeoutTimer); + ctx.signal.removeEventListener('abort', onAbort); + this.livePgids.delete(pgid); + cleanupProfileDir(); + resolve({ + sessionId, + artifacts: [], + state: 'failed', + failureReason: 'runner_crashed', + summary: String(err), + costUsd: guard.snapshot.costUsd, + turns, + }); + }); + child.on('close', () => { clearInterval(heartbeat); clearTimeout(timeoutTimer); ctx.signal.removeEventListener('abort', onAbort); this.livePgids.delete(pgid); + cleanupProfileDir(); const base = { sessionId, diff --git a/packages/runner/src/floor-hook.ts b/packages/runner/src/floor-hook.ts index c636046..1d12069 100644 --- a/packages/runner/src/floor-hook.ts +++ b/packages/runner/src/floor-hook.ts @@ -181,9 +181,20 @@ export function floorHookCommand(execPath: string, hookPath: string): string { return `${shellQuote(execPath)} --no-warnings ${shellQuote(hookPath)}`; } -/** The `--settings` JSON payload wiring `command` in as a Bash PreToolUse hook. */ +/** + * The `--settings` JSON payload wiring `command` in as a Bash PreToolUse hook. + * + * `disableAllHooks: false` is load-bearing, not decoration. The CLI honours + * `disableAllHooks: true` from a repo's own `.claude/settings.json`, and a + * `--settings` payload that only adds hooks leaves that switch in the repo's + * hands — one committed key and the floor is gone while the report still says + * `sandboxed: true` (probed on CLI 2.1.261, 2026-09-06). CLI-flag settings + * outrank project settings, so pinning the switch here wins, and it also + * covers the agent writing that file into its own worktree mid-run. + */ export function floorHookSettings(command: string): string { return JSON.stringify({ + disableAllHooks: false, hooks: { PreToolUse: [{ matcher: 'Bash', hooks: [{ type: 'command', command }] }], }, diff --git a/packages/runner/src/hermes-runner.ts b/packages/runner/src/hermes-runner.ts index fe1d7ed..e11ede3 100644 --- a/packages/runner/src/hermes-runner.ts +++ b/packages/runner/src/hermes-runner.ts @@ -99,8 +99,11 @@ export class HermesRunner implements AgentRunner { const bin = this.opts.hermesBin ?? 'hermes'; // The usage file lives in a temp dir outside the worktree; the sandbox must be told. let wrapped: string[]; + let profilePath: string | null = null; try { - wrapped = applySandbox([bin, ...argv], this.opts.sandbox, { extraWritePaths: [path.dirname(usagePath)] }).argv; + const sandboxResult = applySandbox([bin, ...argv], this.opts.sandbox, { extraWritePaths: [path.dirname(usagePath)] }); + wrapped = sandboxResult.argv; + profilePath = sandboxResult.profilePath; } catch (e) { cleanup(); resolve({ state: 'failed', failureReason: 'internal', summary: `sandbox profile refused: ${String(e)}`, artifacts: [], costUsd: 0, turns: 0 }); @@ -150,6 +153,21 @@ export class HermesRunner implements AgentRunner { ctx.io.onHeartbeat(); }); + child.on('error', (err) => { + clearTimeout(timeoutTimer); + ctx.signal.removeEventListener('abort', onAbort); + this.livePgids.delete(pgid); + cleanup(); + resolve({ + state: 'failed', + failureReason: 'runner_crashed', + summary: String(err), + artifacts: [], + costUsd: 0, + turns: 1, + }); + }); + child.on('close', (code) => { clearTimeout(timeoutTimer); ctx.signal.removeEventListener('abort', onAbort); @@ -199,6 +217,15 @@ export class HermesRunner implements AgentRunner { try { rmSync(path.dirname(usagePath), { recursive: true, force: true }); } catch {} + // Per-run Seatbelt profile dir (cw-sb-*): applySandbox already wrote it + // to disk before spawn; nothing else removed it, so every run leaked + // one until this cleaned up on every exit path (close, error, spawn + // failure, sandbox refusal). + if (profilePath) { + try { + rmSync(path.dirname(profilePath), { recursive: true, force: true }); + } catch {} + } } }); } diff --git a/packages/runner/src/opencode-runner.ts b/packages/runner/src/opencode-runner.ts index 0822279..fc4551a 100644 --- a/packages/runner/src/opencode-runner.ts +++ b/packages/runner/src/opencode-runner.ts @@ -6,6 +6,8 @@ * by time only. */ import { spawn, type ChildProcess } from 'node:child_process'; +import { rmSync } from 'node:fs'; +import path from 'node:path'; import { buildRunEnv } from './run-env.js'; import { applySandbox, toolCacheEnv, type SandboxSpec } from './sandbox.js'; import type { AgentRunner, JobContext, JobSpecLike, RunOutcome } from '@clockwork/shared'; @@ -50,12 +52,26 @@ export class OpenCodeRunner implements AgentRunner { const env = buildRunEnv(toolCacheEnv()); let wrapped: string[]; + let profilePath: string | null = null; try { - wrapped = applySandbox(['opencode', ...argv], this.opts.sandbox).argv; + const sandboxResult = applySandbox(['opencode', ...argv], this.opts.sandbox); + wrapped = sandboxResult.argv; + profilePath = sandboxResult.profilePath; } catch (e) { resolve({ state: 'failed', failureReason: 'internal', summary: `sandbox profile refused: ${String(e)}`, artifacts: [], costUsd: 0, turns: 0 }); return; } + // Per-run Seatbelt profile dir (cw-sb-*): applySandbox already wrote it to + // disk before spawn; nothing else removes it, so every run leaked one + // until this cleaned up on every exit path (close, error, spawn failure). + const cleanupProfileDir = (): void => { + if (!profilePath) return; + try { + rmSync(path.dirname(profilePath), { recursive: true, force: true }); + } catch { + /* best-effort; a leaked temp dir is not a run failure */ + } + }; const child: ChildProcess = spawn(wrapped[0]!, wrapped.slice(1), { cwd: ctx.worktreePath, env, @@ -63,6 +79,7 @@ export class OpenCodeRunner implements AgentRunner { stdio: ['ignore', 'pipe', 'pipe'], }); if (!child.pid) { + cleanupProfileDir(); resolve({ state: 'failed', failureReason: 'runner_crashed', @@ -100,10 +117,26 @@ export class OpenCodeRunner implements AgentRunner { ctx.io.onHeartbeat(); }); + child.on('error', (err) => { + clearTimeout(timeoutTimer); + ctx.signal.removeEventListener('abort', onAbort); + this.livePgids.delete(pgid); + cleanupProfileDir(); + resolve({ + state: 'failed', + failureReason: 'runner_crashed', + summary: String(err), + artifacts: [], + costUsd: 0, + turns: 1, + }); + }); + child.on('close', (code) => { clearTimeout(timeoutTimer); ctx.signal.removeEventListener('abort', onAbort); this.livePgids.delete(pgid); + cleanupProfileDir(); const summary = lastMeaningfulChunk(stdoutAll); const aborted = this.livePgids.size >= 0 && ctx.signal.aborted; diff --git a/packages/runner/test/api-agent-run-command-sandbox.test.ts b/packages/runner/test/api-agent-run-command-sandbox.test.ts new file mode 100644 index 0000000..33f6c94 --- /dev/null +++ b/packages/runner/test/api-agent-run-command-sandbox.test.ts @@ -0,0 +1,131 @@ +/** + * ADR-034 finding #2: applySandbox() writes a fresh `cw-sb-` directory + * (containing profile.sb) under os.tmpdir() on EVERY call. ClaudeCliRunner + * cleans its own up; codex/opencode/hermes now do too (see + * runner-env-wiring.test.ts's source guard — they need a real binary to + * actually spawn). api-agent-runner is different: it calls + * applySandbox once per run_command TOOL CALL inside a single BYOK run, so an + * agent that calls run_command N times used to leak N profile directories. + * + * This drives execTool('run_command', ...) against a REAL SandboxSpec, built + * the same way runner-child builds one (mirrors sandbox-production-spec.test.ts), + * through a real `sandbox-exec` (darwin only) — no mocks of the thing under test. + * + * Race-safety: os.tmpdir() is redirected (via TMPDIR) to a private, empty root + * for this file's own worker only, so counting cw-sb-* dirs can never be + * confused by another test file's concurrent use of the real /tmp. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdirSync, mkdtempSync, readdirSync, rmSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { applySandbox, buildSandboxSpec, cliWorkDirFor } from '../src/sandbox.js'; +import { execTool } from '../src/api-agent-runner.js'; + +const onMac = process.platform === 'darwin'; + +describe.skipIf(!onMac)('api-agent-runner run_command: Seatbelt profile dir cleanup', () => { + const originalTmpdir = process.env.TMPDIR; + let isolatedRoot: string; + let worktree: string; + let cliWork: string; + + beforeAll(() => { + // Every applySandbox() call in this suite lands under isolatedRoot instead + // of the real /tmp, so "no growth" can be asserted without any chance of + // a sibling test file's own cw-sb-* dir being mistaken for a leak here. + isolatedRoot = mkdtempSync(path.join(os.tmpdir(), 'cw-apiagent-root-')); + process.env.TMPDIR = isolatedRoot; + // Prove the redirect actually took: os.tmpdir() reads TMPDIR lazily on + // every call, so if this ever stops being true, every "no growth" + // assertion below would still read 0 → 0 against the REAL /tmp and pass + // for the wrong reason. + expect(os.tmpdir()).toBe(isolatedRoot); + worktree = path.join(isolatedRoot, 'wt'); + mkdirSync(worktree, { recursive: true }); + // cliWorkDirFor resolves the worktree's REAL path (macOS /tmp -> /private/tmp + // symlink) and is hardcoded under the real /tmp regardless of TMPDIR — it + // must be captured now, while `worktree` still exists on disk, or + // resolveReal() falls back to the un-resolved literal and afterAll cleans + // up a path buildSandboxSpec never actually created, leaking the real one. + cliWork = cliWorkDirFor(worktree); + }); + + afterAll(() => { + if (originalTmpdir === undefined) delete process.env.TMPDIR; + else process.env.TMPDIR = originalTmpdir; + rmSync(isolatedRoot, { recursive: true, force: true }); + rmSync(cliWork, { recursive: true, force: true }); + }); + + function sandboxDirCount(): number { + return readdirSync(isolatedRoot).filter((f) => f.startsWith('cw-sb-')).length; + } + + it('sandboxDirCount actually detects a profile dir (positive control for every "no growth" assertion below)', () => { + const spec = buildSandboxSpec({ + worktreePath: worktree, + scratchPath: null, + repoPath: null, + contextRoots: [], + cacheRoot: path.join(isolatedRoot, 'cache-control'), + }); + const before = sandboxDirCount(); + const { profilePath } = applySandbox(['true'], spec); + expect(profilePath).toBeTruthy(); + expect(profilePath!.startsWith(isolatedRoot)).toBe(true); + expect(sandboxDirCount()).toBe(before + 1); + rmSync(path.dirname(profilePath!), { recursive: true, force: true }); + expect(sandboxDirCount()).toBe(before); + }); + + it('removes the per-call profile dir once run_command succeeds', async () => { + const spec = buildSandboxSpec({ + worktreePath: worktree, + scratchPath: null, + repoPath: null, + contextRoots: [], + cacheRoot: path.join(isolatedRoot, 'cache-a'), + }); + const before = sandboxDirCount(); + const out = await execTool('run_command', { command: 'echo cw-probe-ok' }, worktree, spec); + expect(out).toContain('cw-probe-ok'); + expect(sandboxDirCount()).toBe(before); // not before+1 — the leak this closes + }); + + it('removes the profile dir even when the command exits non-zero', async () => { + const spec = buildSandboxSpec({ + worktreePath: worktree, + scratchPath: null, + repoPath: null, + contextRoots: [], + cacheRoot: path.join(isolatedRoot, 'cache-b'), + }); + const before = sandboxDirCount(); + const out = await execTool('run_command', { command: 'exit 7' }, worktree, spec); + expect(out).toContain('EXIT-ERROR'); + expect(sandboxDirCount()).toBe(before); + }); + + it('never accumulates across repeated run_command calls in the same run', async () => { + const spec = buildSandboxSpec({ + worktreePath: worktree, + scratchPath: null, + repoPath: null, + contextRoots: [], + cacheRoot: path.join(isolatedRoot, 'cache-c'), + }); + const before = sandboxDirCount(); + for (let i = 0; i < 3; i++) { + await execTool('run_command', { command: `echo call-${i}` }, worktree, spec); + } + expect(sandboxDirCount()).toBe(before); + }); + + it('a null spec (CW_SANDBOX=off) never creates a profile dir to begin with', async () => { + const before = sandboxDirCount(); + const out = await execTool('run_command', { command: 'echo unsandboxed' }, worktree, null); + expect(out).toContain('unsandboxed'); + expect(sandboxDirCount()).toBe(before); + }); +}); diff --git a/packages/runner/test/floor-hook.test.ts b/packages/runner/test/floor-hook.test.ts index b590772..fb2d217 100644 --- a/packages/runner/test/floor-hook.test.ts +++ b/packages/runner/test/floor-hook.test.ts @@ -20,7 +20,7 @@ import os from 'node:os'; import path from 'node:path'; import { PermissionServer } from '../src/permission-server.js'; import { evaluateCommand } from '../src/deny-list.js'; -import { writeFloorHook } from '../src/floor-hook.js'; +import { writeFloorHook, floorHookSettings } from '../src/floor-hook.js'; let server: PermissionServer; let dir: string; @@ -117,3 +117,14 @@ describe('floor-hook.mjs — PreToolUse fail-closed hook', () => { expect(r.stderr).toMatch(/Clockwork policy floor unreachable/); }); }); + +describe('floorHookSettings pins the hook switch', () => { + it('sets disableAllHooks:false so a repo .claude/settings.json cannot turn the floor off', () => { + const parsed = JSON.parse(floorHookSettings("'/usr/bin/true'")) as { disableAllHooks?: unknown; hooks?: unknown }; + // Probed on CLI 2.1.261 (2026-09-06): without this key, `{"disableAllHooks": true}` + // committed in the repo silently disables the PreToolUse hook and the gated + // command runs unasked. CLI-flag settings outrank project settings. + expect(parsed.disableAllHooks).toBe(false); + expect(parsed.hooks).toBeDefined(); + }); +}); diff --git a/packages/runner/test/runner-env-wiring.test.ts b/packages/runner/test/runner-env-wiring.test.ts index 648f4e5..928a391 100644 --- a/packages/runner/test/runner-env-wiring.test.ts +++ b/packages/runner/test/runner-env-wiring.test.ts @@ -61,6 +61,7 @@ describe('runner env wiring', () => { * macOS Seatbelt profile". Same source-reading approach as above, same reason. */ const RUNNER_CHILD = resolve(SRC, '../../daemon/src/runner-child.ts'); +const RUN_MANAGER = resolve(SRC, '../../daemon/src/run-manager.ts'); const API_AGENT = 'api-agent-runner.ts'; describe('sandbox wiring', () => { @@ -84,8 +85,54 @@ describe('sandbox wiring', () => { expect(bare, `runners constructed with no sandbox option: ${bare.join(', ')}`).toEqual([]); }); - it('runner-child scrubs the BYOK key from its own env after reading it', () => { + /** + * ADR-034: the BYOK credential travels over the daemon<->child stdin JSONL + * channel, never through env. macOS exposes a process's exec-time env to + * ANY other same-user process — sandboxed or not — via sysctl + * KERN_PROCARGS2 (the Seatbelt profile must allow sysctl-read for Node + * itself to run, so it cannot close that door). A `delete process.env.*` + * scrub only stops future children spawned FROM the credentialed process; + * it does nothing about a sibling process reading this process's own + * KERN_PROCARGS2 record before or after the scrub. Only a transport that + * never puts the secret in argv/env in the first place closes that gap — + * these two guards prove neither side of the channel does. + */ + it('run-manager never puts the BYOK credential in the child env', () => { + const src = readFileSync(RUN_MANAGER, 'utf8'); + const offenders = [/CW_BYOK_KEY:/, /CW_BYOK_KEY\s*=/, /envOut\.CW_BYOK_KEY/].filter((re) => re.test(src)); + expect( + offenders, + 'run-manager.ts assigns CW_BYOK_KEY into an object literal — that object is passed as the spawned child\'s env, and macOS exposes exec-time env to any same-user process via KERN_PROCARGS2, sandboxed or not. The credential must travel over stdin only.', + ).toEqual([]); + }); + + it('runner-child never reads the BYOK credential from its own env, and awaits it over stdin instead', () => { const src = readFileSync(RUNNER_CHILD, 'utf8'); - expect(/delete process\.env\.CW_BYOK_KEY/.test(src), 'CW_BYOK_KEY stays readable by every child the agent spawns').toBe(true); + expect( + /process\.env\.CW_BYOK_KEY/.test(src), + 'runner-child.ts still reads CW_BYOK_KEY from env — that value is readable by any same-user process via KERN_PROCARGS2 (sysctl-read is required for Node to run at all, sandboxed or not), so a scrub after reading it is not a boundary. The credential must arrive over stdin only.', + ).toBe(false); + expect( + /t === 'credential'/.test(src), + 'runner-child.ts must handle the {t:"credential"} message from run-manager.ts — that is the only channel the BYOK secret should ever travel over.', + ).toBe(true); + }); + + /** + * Per-run Seatbelt profile dirs (T-114-adjacent): applySandbox() writes a + * fresh `cw-sb-` directory (containing profile.sb) under os.tmpdir() and + * returns the path. claude-cli-runner already removed it; codex/opencode/ + * hermes did not, so every non-Claude-CLI run — and every api-agent + * run_command call — leaked one directory forever. These runners need a + * real binary to actually spawn, so this stays a source-reading guard like + * the ones above; sandbox-production-spec.test.ts and the api-agent test + * below prove the mechanism behaviourally. + */ + it('every CLI runner captures profilePath and removes it with rmSync', () => { + const offenders = RUNNERS.filter((f) => { + const src = read(f); + return !/profilePath/.test(src) || !/rmSync\(/.test(src); + }); + expect(offenders, `runners that never clean up their sandbox profile dir: ${offenders.join(', ')}`).toEqual([]); }); }); diff --git a/packages/shared/src/schemas.ts b/packages/shared/src/schemas.ts index 982ba9c..7c10312 100644 --- a/packages/shared/src/schemas.ts +++ b/packages/shared/src/schemas.ts @@ -322,7 +322,7 @@ export const JobSpec = z.object({ prompt: z.string(), engine: Engine, model: z.string().nullable(), - /** BYOK config id snapshot (ADR-027); credential itself is injected via env at spawn, never serialized */ + /** BYOK config id snapshot (ADR-027); the credential itself is delivered to the runner over the daemon⇄child stdin channel (ADR-035), never serialized here and never placed in env */ byokId: z.string().nullable(), permissionMode: PermissionMode, budget: Budget, From 9bb01b45802828713a04197e60fb0fb23902b463 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 6 Sep 2026 05:01:17 +0530 Subject: [PATCH 4/4] fix(security): make the floor hook's watchdog final and qualify the BYOK env claim Review follow-ups on the previous two commits. The generated PreToolUse hook's watchdog now marks the outcome settled before denying, so a bridge reply landing in the one-second window after a watchdog deny can no longer turn into exit(0). docs/security.md qualifies the BYOK statement: a config that reads its key from an environment variable has that key in the daemon's own exec-time environment by construction, which the same sysctl path can read; keychain-backed configs are fully closed. Four comments and test titles that cited ADR-034 for the credential channel and the profile cleanup now point at ADR-035, where those findings are recorded. Co-Authored-By: Claude Fable 5.1 --- docs/security.md | 4 ++++ packages/daemon/src/runner-protocol.ts | 2 +- packages/daemon/test/credential-channel.test.ts | 4 ++-- packages/runner/src/floor-hook.ts | 5 ++++- packages/runner/test/api-agent-run-command-sandbox.test.ts | 2 +- packages/runner/test/runner-env-wiring.test.ts | 2 +- 6 files changed, 13 insertions(+), 6 deletions(-) diff --git a/docs/security.md b/docs/security.md index d4a92e3..b8c17b5 100644 --- a/docs/security.md +++ b/docs/security.md @@ -103,6 +103,10 @@ jobs. Imported templates/profiles arrive disabled with a security preview. readable through `sysctl` (`KERN_PROCARGS2`) by any same-user process, sandboxed or not, so an env var that is "read then deleted" is not a boundary (found in review 2026-09-06; the transport is the fix, the profile cannot be). + One qualifier: a BYOK config that reads its key from an environment variable + (`auth: 'env'`) has the key in the **daemon's** own exec-time environment by + construction, and that stays readable through the same `sysctl` path. + Keychain-backed configs are fully closed; prefer them. - The local API is loopback-only with a bearer token stored `0600`. ## Engine capability honesty diff --git a/packages/daemon/src/runner-protocol.ts b/packages/daemon/src/runner-protocol.ts index b6686b7..33bd236 100644 --- a/packages/daemon/src/runner-protocol.ts +++ b/packages/daemon/src/runner-protocol.ts @@ -25,6 +25,6 @@ export type ChildToDaemon = export type DaemonToChild = | { t: 'decision'; reqId: string; behavior: 'allow' } | { t: 'decision'; reqId: string; behavior: 'deny'; message: string } - /** BYOK credential delivery (ADR-034): stdin, never env — see run-manager.ts + /** BYOK credential delivery (ADR-035): stdin, never env — see run-manager.ts * spawnChild and runner-child.ts's credential promise for why. */ | { t: 'credential'; byokKey: string; byokBaseUrl: string }; diff --git a/packages/daemon/test/credential-channel.test.ts b/packages/daemon/test/credential-channel.test.ts index 77ce1d4..3083561 100644 --- a/packages/daemon/test/credential-channel.test.ts +++ b/packages/daemon/test/credential-channel.test.ts @@ -1,5 +1,5 @@ /** - * ADR-034: the BYOK credential must travel to runner-child over stdin, never + * ADR-035: the BYOK credential must travel to runner-child over stdin, never * through the spawned child's env — macOS exposes a process's exec-time env * to any other same-user process via sysctl KERN_PROCARGS2 (the Seatbelt * profile has to allow sysctl-read for Node to run at all, so it cannot close @@ -158,7 +158,7 @@ afterAll(() => { rmSync(dir, { recursive: true, force: true }); }); -describe('BYOK credential delivery: stdin, never env (ADR-034)', () => { +describe('BYOK credential delivery: stdin, never env (ADR-035)', () => { it('a BYOK run gets no CW_BYOK_KEY/CW_BYOK_BASE_URL in its spawned env, and the credential arrives as a stdin line', async () => { process.env.CW_TEST_BYOK_SECRET = 'sk-test-shhh-do-not-log'; const store = new ByokStore({ db: db as unknown as { prepare: (s: string) => any; transaction?: (fn: () => void) => unknown } }); diff --git a/packages/runner/src/floor-hook.ts b/packages/runner/src/floor-hook.ts index 1d12069..839dc83 100644 --- a/packages/runner/src/floor-hook.ts +++ b/packages/runner/src/floor-hook.ts @@ -69,7 +69,11 @@ function deny(reason) { // fail-OPEN this file exists to prevent if some unforeseen path leaves the // process idle without ever reaching exit(). The allow path clearTimeout()s // it explicitly; every other path calls process.exit() before it would fire. +// Shared "final answer given" flag: once the watchdog has denied, a late allow +// from the bridge must not turn into exit(0). +let settled = false; const watchdog = setTimeout(() => { + settled = true; deny('Clockwork policy floor unreachable: hook watchdog expired'); }, WATCHDOG_MS); @@ -100,7 +104,6 @@ process.stdin.on('end', () => { const toolInput = input ? input.tool_input : undefined; const body = JSON.stringify({ tool_name: toolName, tool_input: toolInput }); - let settled = false; const req = http.request( FLOOR_URL, { diff --git a/packages/runner/test/api-agent-run-command-sandbox.test.ts b/packages/runner/test/api-agent-run-command-sandbox.test.ts index 33f6c94..c0609c7 100644 --- a/packages/runner/test/api-agent-run-command-sandbox.test.ts +++ b/packages/runner/test/api-agent-run-command-sandbox.test.ts @@ -1,5 +1,5 @@ /** - * ADR-034 finding #2: applySandbox() writes a fresh `cw-sb-` directory + * ADR-035 finding #2: applySandbox() writes a fresh `cw-sb-` directory * (containing profile.sb) under os.tmpdir() on EVERY call. ClaudeCliRunner * cleans its own up; codex/opencode/hermes now do too (see * runner-env-wiring.test.ts's source guard — they need a real binary to diff --git a/packages/runner/test/runner-env-wiring.test.ts b/packages/runner/test/runner-env-wiring.test.ts index 928a391..e8e9f37 100644 --- a/packages/runner/test/runner-env-wiring.test.ts +++ b/packages/runner/test/runner-env-wiring.test.ts @@ -86,7 +86,7 @@ describe('sandbox wiring', () => { }); /** - * ADR-034: the BYOK credential travels over the daemon<->child stdin JSONL + * ADR-035: the BYOK credential travels over the daemon<->child stdin JSONL * channel, never through env. macOS exposes a process's exec-time env to * ANY other same-user process — sandboxed or not — via sysctl * KERN_PROCARGS2 (the Seatbelt profile must allow sysctl-read for Node