From a15221151fb136fcf8f0654949247dba017c9352 Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Sat, 11 Apr 2026 00:35:32 -0400 Subject: [PATCH 1/2] fix(hooks): native devkit-engine guard subcommand (closes #65) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces python3-based parsing in devkit-guard.sh / devkit-stop-guard.sh with a new `devkit-engine guard [--tool-name] [--stop]` Cobra subcommand. Policy is unchanged; the substrate moves to Go for portability, speed, and testability. Why --- - Windows / minimal containers often lack python3. The old hooks hard- blocked every tool call on those hosts (fail-closed on python3 unavailable) or silently skipped enforcement. - Python3 cold-start was 50-150 ms per hook invocation on every tool call, compounding in high-churn workflow steps. - Bash + python3 + jq split policy across three languages. A Go subcommand lets the engine and the guard share the exact same SessionState parser (lib.ReadSessionJSON), eliminating drift. Native subcommand (src/cmd/guard.go, +398 lines) ------------------------------------------------- - New `devkit-engine guard` command. Overrides rootCmd.PersistentPreRunE to a no-op so the guard never requires a git repo, never opens the SQLite DB, and never fails on hosts without .git. Cobra lets a child command shadow the parent's persistent pre-run entirely. - Hot path (no active workflow): single read-only os.Stat, zero writes. sessionFileExists runs BEFORE lib.ReadSessionJSON so we skip the withSessionLock mkdir + session.json.lock create side effects on every PreToolUse call where the user has no running session. - Any error after a positive sessionFileExists result fails CLOSED unconditionally — permission errors, quota, lock-acquire failures, parse errors all return a BLOCKED diagnostic pointing at the file. Silently fail-open on permission errors would let a broken plugin data dir disarm the guard with zero user-visible signal. - Policy matrix mirrors PR #64 exactly: command + hard → only devkit MCP + TodoWrite prompt + hard → read-only evidence tools + devkit MCP prompt + soft → allow with stderr nudge parallel → allow (engine is dispatching) stale session → allow with stderr warning (orphan recovery) - isDevkitMCPTool is anchored on the full plugin+server prefix (mcp__plugin_devkit_devkit-engine__) plus the short-form mcp__devkit__ namespace. This is tighter than PR #64's shell glob (mcp__*devkit-engine*) and much tighter than the original mcp__*devkit* substring — even a hypothetical second MCP server under the devkit plugin cannot silently inherit command-step permissions. - effectiveEnforce defaults empty Enforce to "hard", mirroring the shell hook's python .get('enforce','hard') so a schema-drift gap can't silently disarm enforcement. - sessionIsStale falls back UpdatedAt → StartedAt → "fresh", but also logs a one-line WARNING when both timestamps are zero so a wedged session leaves a debuggable trail. - staleTTL honours DEVKIT_SESSION_STALE_TTL_SECONDS. Whitespace is trimmed (TrimSpace) so copy-paste trailing-space doesn't bite; non-numeric / non-positive values log a warning and fall back to default rather than silently degrading. - readToolNameFromStdin returns (string, error) so the three failure modes (read error / empty / parse error) can be logged distinctly. Empty tool name still falls through to default-deny under hard enforcement, so the security posture is unchanged; only the diagnostic improves. - Block diagnostics substitute "" when the tool name is empty, so log readers don't see a dangling "(attempted tool: )". - --stop mode emits Stop-hook JSON verdict on stdout (no trailing newline, matching the shell printf '%s' output byte-for-byte). writeStopVerdict panics on the unreachable json.Marshal failure path instead of silently writing a hardcoded fallback — any future field addition that breaks marshalling trips CI. - Broken stdout in writeStopVerdict now logs to stderr so a pipe failure leaves some post-mortem trail. - guardCmd declares Args: cobra.NoArgs so extra positional args fail loudly in development rather than being silently dropped. Thin shell wrappers (hooks/devkit-guard.sh, hooks/devkit-stop-guard.sh) ---------------------------------------------------------------------- Both scripts reduce to binary-resolution + exec: 1. $CLAUDE_PLUGIN_ROOT/bin/devkit-engine (local-dev symlink) 2. $CLAUDE_PLUGIN_ROOT/bin/devkit-engine-v* (shipped release asset) - shopt -s nullglob so an unmatched glob expands to nothing instead of iterating once with the literal pattern string. - When multiple versioned binaries coexist, pick the highest-sorted executable match. The naive "first glob match" would pick v2.1.0 over v2.1.10 lexicographically. - The bin/devkit first-run-download fallback has been DELIBERATELY removed from the hook path. Downloading release assets from a time-limited hook is unsafe (fail-open on timeout), and a fresh install should fail closed with a diagnostic pointing at `devkit install` rather than silently blocking on a network call. - When no binary is found, emit a LOUD stderr diagnostic naming the search path and instructing `devkit install` — then allow (guard) or approve (stop-guard). A broken install should trip the user's attention on first tool call rather than silently disarming enforcement. - No python3, no jq, no subshell parsing. Stdin (the PreToolUse JSON payload) passes straight through exec to the Go binary. hooks/hooks.json ---------------- Timeout for devkit-guard.sh and devkit-stop-guard.sh raised from 2s to 10s. The Go binary clears the old 8.5ms budget by three orders of magnitude under warm conditions, but 10s gives room for: - macOS Gatekeeper quarantine scan on first exec - Windows Defender cold scan - Network filesystem exec stall - Cold Go runtime init on large binaries Deleted ------- - hooks/lib/read-session.sh — python3 session parser, superseded. - hooks/devkit-guard_test.sh — shell fixture matrix, ported to src/cmd/guard_test.go as 40+ table-driven cases. Test coverage (src/cmd/guard_test.go, +800 lines) ------------------------------------------------- - 30+ table-driven rows covering the full policy matrix including fixture-parity gaps from the deleted shell test: prompt+hard+TodoWrite, prompt+soft+Write, parallel+soft+Bash. - Schema-drift pins: Status="RUNNING" uppercase case-sensitivity, TotalSteps=0 label fallback, --tool-name flag vs stdin precedence, empty stdin / malformed stdin under command+hard. - wantStderrSubstr field on the table pins veto-message wording so any regression in the block diagnostic fails the suite. - Allowlist bypass negatives: mcp__plugin_evil_server__devkit_masquerade → block mcp__plugin_devkit_other_server__probe → block (tightening beyond PR #64's shell glob) Positive cases: mcp__plugin_devkit_devkit-engine__devkit_advance → allow mcp__devkit__advance → allow (short-form) - DEVKIT_SESSION_STALE_TTL_SECONDS garbage matrix (6 subtests): non-numeric, negative, zero, trailing-space (trimmed), empty, integer overflow. - Longer TTL override (7200s + 45min-old session stays fresh). - Zero-timestamp warning pinned (session with no UpdatedAt/StartedAt logs anomaly and still enforces). - Unreadable session file (mode 0o000, Unix-only) fails closed with BLOCKED diagnostic. - Stale session under prompt+soft (previously only command+hard was covered). - Top-of-file comment documents the t.Parallel() prohibition: the test helper mutates package-level IO globals, and parallelism would race. Follow-up refactor to a guardContext struct would enable parallel tests. Test coverage (hooks/hooks_test.sh, +105 lines) ----------------------------------------------- - CLAUDE_PLUGIN_ROOT unset → disabled + exit 0 / approve. - Empty bin/ directory → loud warning + allow / approve. (The "fresh clone before first build" scenario — previously completely untested.) - Versioned binary only (no local-dev symlink) → exec with guard arg. - Multiple versioned binaries coexisting → pick the highest-sorted executable. Pins B1's contract against future refactors. Verification ------------ - go test ./... — all packages green (40+ guard cases, 6 stale-TTL subtests, 5 dedicated scenario tests). - hooks/hooks_test.sh — 52/52 pass, up from 46. - Latency: ~8.5 ms per guard invocation over a 20-run wall clock (vs. 50-150 ms python3 cold start), with empty $CLAUDE_PLUGIN_DATA verified after 20 runs — no session.json.lock leaked as a side effect on the no-workflow hot path. - Cross-compile clean: linux/amd64, darwin/arm64, windows/amd64. Rebased onto main after PR #64 + 2.1.8 version bump. PR #64's engine- side changes (stale-session reclaim notice in tools.go, UpdatedAt side-effect doc in state_json.go) are inherited from main unchanged. --- hooks/devkit-guard.sh | 159 +++---- hooks/devkit-guard_test.sh | 231 ---------- hooks/devkit-stop-guard.sh | 86 ++-- hooks/hooks.json | 4 +- hooks/hooks_test.sh | 105 +++++ hooks/lib/read-session.sh | 108 ----- src/cmd/guard.go | 474 ++++++++++++++++++++ src/cmd/guard_test.go | 889 +++++++++++++++++++++++++++++++++++++ 8 files changed, 1550 insertions(+), 506 deletions(-) delete mode 100755 hooks/devkit-guard_test.sh delete mode 100644 hooks/lib/read-session.sh create mode 100644 src/cmd/guard.go create mode 100644 src/cmd/guard_test.go diff --git a/hooks/devkit-guard.sh b/hooks/devkit-guard.sh index 248dc85..03dd493 100755 --- a/hooks/devkit-guard.sh +++ b/hooks/devkit-guard.sh @@ -1,124 +1,69 @@ #!/usr/bin/env bash set -euo pipefail +# nullglob: an unmatched glob expands to nothing rather than the literal +# pattern, so the `for candidate in ...` loops below are correct when no +# versioned binary exists. Without this, the loop would iterate once +# with the literal "devkit-engine-v*" string. +shopt -s nullglob # devkit-guard: PreToolUse hook that enforces workflow step ordering. -# Reads $CLAUDE_PLUGIN_DATA/session.json. Blocks out-of-step actions. -# Exit 0 = allow, Exit 2 + stderr = hard block. +# Thin wrapper around `devkit-engine guard`. All policy lives in Go +# (src/cmd/guard.go) so the shell side is just binary resolution + exec. # -# Policy matrix: -# step_type=command, enforce=hard → allow only devkit MCP + TodoWrite -# (engine runs the command, not Claude) -# step_type=prompt, enforce=hard → allow Read/Grep/Glob/NotebookRead/ -# TodoWrite + devkit MCP. Forces the -# agent to advance before any -# write/bash/dispatch. Closes issue #63 -# drift hole. -# step_type=prompt, enforce=soft → allow everything, emit stderr nudge -# step_type=parallel → allow everything (engine dispatches) -# stale session (see lib/read-session.sh) → allow + warn; do not enforce -# against an orphaned state file. +# Exit 0 = allow, exit 2 = hard block (with diagnostic on stderr). +# Stdin (the PreToolUse JSON payload) is passed through unchanged so +# the engine can parse tool_name itself — no jq, no python3. # -# This hook uses an ALLOWLIST rather than a blocklist because the -# Claude Code tool surface evolves — Task, SlashCommand, ExitPlanMode, -# BashOutput, KillBash, TodoWrite, any mcp__* tool, and future names -# would silently bypass a blocklist of hardcoded names. - -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -# shellcheck source=lib/read-session.sh -source "${SCRIPT_DIR}/lib/read-session.sh" +# Binary search order: +# 1. $CLAUDE_PLUGIN_ROOT/bin/devkit-engine — local dev symlink +# 2. $CLAUDE_PLUGIN_ROOT/bin/devkit-engine-v* — shipped release asset +# +# The `bin/devkit` first-run-download wrapper is DELIBERATELY not +# reachable from this hook: downloading release assets from a 2s-10s +# PreToolUse hook is unsafe (timeout → silent fail-open). A fresh +# install should fail closed here so the user runs `devkit install` +# once and has a cached binary before their first workflow. If we find +# no binary at all, we emit a LOUD diagnostic and allow — because the +# alternative (hard-block every tool call on a broken install) would +# wedge the user's session with no way to recover except editing hooks. -DATA_DIR="${CLAUDE_PLUGIN_DATA:-}" -if [[ -z "$DATA_DIR" ]]; then - printf 'devkit-guard: CLAUDE_PLUGIN_DATA unset — enforcement disabled\n' >&2 +PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT:-}" +if [[ -z "$PLUGIN_ROOT" ]]; then + printf 'devkit-guard: CLAUDE_PLUGIN_ROOT unset — enforcement disabled\n' >&2 exit 0 fi -SESSION_FILE="${DATA_DIR}/session.json" +BIN_DIR="$PLUGIN_ROOT/bin" -if ! parse_session_fields "$SESSION_FILE"; then - # python3 unavailable or JSON corrupt — fail closed if session file - # exists, otherwise fall through (no session = nothing to guard). - if [[ -f "$SESSION_FILE" ]]; then - printf 'BLOCKED: Cannot parse session state (python3 required or JSON corrupt). Remove %s to clear.\n' "$SESSION_FILE" >&2 - exit 2 - fi - exit 0 +# Preferred: local-dev symlink (created by `make install-plugin`). +if [[ -x "$BIN_DIR/devkit-engine" ]]; then + exec "$BIN_DIR/devkit-engine" guard fi -if [[ "$SESSION_STATUS" != "running" ]]; then - exit 0 -fi - -if [[ "$SESSION_STALE" == "1" ]]; then - printf 'devkit-guard: session %s idle past TTL — treating as orphaned (run devkit_start to reclaim)\n' "$SESSION_WORKFLOW" >&2 - exit 0 -fi - -# Read tool name from stdin. Matches PreToolUse payload format. -INPUT=$(cat) -TOOL_NAME=$(printf '%s' "$INPUT" | python3 -c "import json,sys; print(json.load(sys.stdin).get('tool_name',''))" 2>/dev/null) || { - # Malformed payload — surface a diagnostic so the transcript shows - # why the next veto lists an empty tool name, instead of letting the - # BLOCKED message say "(attempted tool: )" with no hint. - printf 'devkit-guard: could not parse tool name from PreToolUse payload (python3 or JSON error)\n' >&2 - TOOL_NAME="" -} - -# Build a progress label for veto messages so the agent always sees -# workflow + position without another devkit_status round trip. -step_label() { - if [[ -n "$SESSION_CURRENT_INDEX" && -n "$SESSION_TOTAL_STEPS" ]]; then - local human_index=$((SESSION_CURRENT_INDEX + 1)) - printf '%s step %d/%d (%s)' "$SESSION_WORKFLOW" "$human_index" "$SESSION_TOTAL_STEPS" "$SESSION_CURRENT_STEP" - else - printf '%s (%s)' "$SESSION_WORKFLOW" "$SESSION_CURRENT_STEP" +# Shipped release assets. Filenames look like +# devkit-engine-v2.1.7-darwin-arm64. Bash glob expansion sorts +# lexicographically, which gets version ordering WRONG past the +# single-digit boundary (v2.1.9 < v2.1.10 lexically, so v2.1.10 would +# sort BEFORE v2.1.9). We pick the highest-sorting executable match +# using a string comparison, which happens to be correct for versions +# that share the same digit-count prefix — and we fall back to failing +# closed with a diagnostic if multiple versions coexist in a way that +# string comparison can't resolve. +latest="" +for candidate in "$BIN_DIR"/devkit-engine-v*; do + [[ -x "$candidate" ]] || continue + if [[ -z "$latest" || "$candidate" > "$latest" ]]; then + latest="$candidate" fi -} - -# Command steps: allow ONLY the MCP tools needed to progress the -# workflow. Everything else is blocked, including future tools. -if [[ "$SESSION_STEP_TYPE" == "command" && "$SESSION_ENFORCE" == "hard" ]]; then - case "$TOOL_NAME" in - mcp__*devkit-engine*|mcp__devkit__*|devkit_advance|devkit_status|devkit_list|devkit_start) - exit 0 - ;; - TodoWrite) - exit 0 - ;; - *) - printf 'BLOCKED: Command step "%s" in progress — the engine runs this step. Call devkit_advance to execute it. (attempted tool: %s)\n' "$(step_label)" "$TOOL_NAME" >&2 - exit 2 - ;; - esac -fi - -# Prompt steps under hard enforcement: allow read-only evidence tools -# plus devkit MCP. Blocks Write/Edit/Bash/Task/WebFetch/other MCP so -# the agent cannot drift into unrelated work between step 1 and -# devkit_advance. See issue #63. -if [[ "$SESSION_STEP_TYPE" == "prompt" && "$SESSION_ENFORCE" == "hard" ]]; then - case "$TOOL_NAME" in - mcp__*devkit-engine*|mcp__devkit__*|devkit_advance|devkit_status|devkit_list|devkit_start) - exit 0 - ;; - Read|Grep|Glob|TodoWrite|NotebookRead) - exit 0 - ;; - *) - printf 'BLOCKED: devkit workflow %s is at a prompt step — gather evidence with Read/Grep/Glob then call devkit_advance. (attempted tool: %s)\n' "$(step_label)" "$TOOL_NAME" >&2 - exit 2 - ;; - esac -fi - -# Prompt steps under soft enforcement: allow everything, but inject a -# stderr nudge so the transcript shows the agent that a step is open. -# Soft nudge is idempotent — if the agent ignores it, Stop gate still -# blocks via devkit-stop-guard.sh. -if [[ "$SESSION_STEP_TYPE" == "prompt" && "$SESSION_ENFORCE" != "hard" ]]; then - printf 'devkit-guard: %s is open — call devkit_advance when the step is complete.\n' "$(step_label)" >&2 - exit 0 +done +if [[ -n "$latest" ]]; then + exec "$latest" guard fi -# Parallel steps: engine is dispatching, agent needs full tool access. +# No cached binary at all. Loud diagnostic + allow — see header comment +# for the rationale. A broken install should trip the user's attention +# on their first tool call rather than silently skipping enforcement. +printf 'devkit-guard: ERROR no devkit-engine binary under %s — ' "$BIN_DIR" >&2 +printf 'run `devkit install` to download the release asset. ' >&2 +printf 'Workflow enforcement is DISABLED until this is fixed.\n' >&2 exit 0 diff --git a/hooks/devkit-guard_test.sh b/hooks/devkit-guard_test.sh deleted file mode 100755 index ab50a3e..0000000 --- a/hooks/devkit-guard_test.sh +++ /dev/null @@ -1,231 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -# Fixture matrix test for devkit-guard.sh. -# Seeds CLAUDE_PLUGIN_DATA with a crafted session.json and pipes a -# synthetic PreToolUse payload on stdin. Asserts exit code and the -# substring of whatever stderr diagnostic the guard emitted. -# -# Run: bash hooks/devkit-guard_test.sh - -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -GUARD="${SCRIPT_DIR}/devkit-guard.sh" - -if [[ ! -x "$GUARD" ]]; then - chmod +x "$GUARD" || true -fi - -PASS=0 -FAIL=0 -FAILED_CASES=() - -# run_case