fix(hooks): native devkit-engine guard subcommand (closes #65) - #66
Merged
Conversation
5uck1ess
added a commit
that referenced
this pull request
Apr 11, 2026
Applies the must-fix + strong-recommend findings from tri-review + pr-review-toolkit on PR #66. Policy is unchanged; correctness, safety, and diagnostics improve across the board. Must-fix (N1, N2, B1, B2, B3): - N1: lib.ReadSessionJSON no longer runs on the no-workflow hot path. runPreToolGuard + runStopGuard now short-circuit on sessionFileExists BEFORE acquiring the session lock, eliminating the mkdir + session.json.lock side effects that the old code created on every tool call. Any error from ReadSessionJSON after the file is confirmed present now unconditionally fails closed — previously the post-hoc sessionFileExists fallback could fail-OPEN on permission errors or TOCTOU races, silently disarming the guard. - N2: isDevkitMCPTool is tightened from the unanchored mcp__*devkit* substring match (ported from the shell glob) to exact prefix matching on mcp__plugin_devkit_ and mcp__devkit__. The old loose match would have allowed a third-party MCP server with "devkit" anywhere in its tool name to bypass the command-step allowlist (e.g. mcp__plugin_evil_server__devkit_masquerade). Negative test row added to pin the tightened behaviour. - B1: shell wrappers now shopt -s nullglob and pick the highest-sorted versioned binary instead of executing the first glob-expansion match. The old for-loop picked v2.1.0 over v2.1.10 lexicographically. Still imperfect under 9→10 semver transitions; header comment documents the limitation and points at the long-term fix (a devkit-engine-latest symlink maintained by the installer). - B2: BLOCKED stderr diagnostic no longer embeds the raw Go error. Wording is pinned to a stable string so minor encoding/json version changes don't break downstream log parsers. - B3: hook timeout in hooks.json raised from 2s to 10s to give room for cold-start on macOS Gatekeeper, Windows Defender, and network mounts. The bin/devkit first-run-download fallback is REMOVED from the hook path entirely — downloading release assets from a time-limited hook is unsafe, and a fresh install should fail closed with a diagnostic pointing at `devkit install` rather than silently blocking on a network call. The loud "no binary" stderr makes a broken install visible on the user's first tool call. Strong-recommends (M1, M2, M3, M4, M5, M6, plus test coverage): - M1: sessionIsStale now emits a one-line stderr warning when both UpdatedAt and StartedAt are zero, so a schema-drift wedge leaves a debuggable trail instead of silently disabling orphan recovery. - M2: staleTTL now strings.TrimSpace the env var (handles copy-paste trailing whitespace), and logs a stderr warning when the value is non-numeric or non-positive. Silent degradation to default was untestable by operators tuning the knob. - M3: readToolNameFromStdin returns (string, error) instead of collapsing three failure modes (read error, empty, parse error) into "". The call site logs the error distinctly, and the empty tool name still falls through to default-deny under hard enforcement — security posture unchanged, debuggability improved. Block diagnostics now print "<unknown>" instead of a dangling paren when the tool name is empty. - M4: guardCmd now declares Args: cobra.NoArgs so `devkit-engine guard extra_positional` fails loudly in development instead of silently ignoring the extra arg. - M5: top-of-file comment in guard_test.go documents the t.Parallel() prohibition and why (six package-level globals + t.Setenv). Follow- up PR should refactor to a guardContext struct. - M6: the TestGuardPreToolUse table now carries a wantStderrSubstr field so block-message wording is pinned. Previously any regression in veto text would silently pass the suite. - writeStopVerdict fallback branch deleted (dead code — json.Marshal on stopVerdict cannot fail). Panics on unreachable instead, so any future field addition that breaks marshalling trips CI. - Broken stdout write in writeStopVerdict now logs to stderr so a post-mortem trail exists if Claude Code's stdout pipe breaks. - sessionFileExists now inlines filepath.Join(dataDir, "session.json") for readability, no package hop. New test coverage (Go): - Fixture parity gaps from the deleted hooks/devkit-guard_test.sh: prompt+hard+TodoWrite, prompt+soft+Write, parallel+soft+Bash. - Schema-drift pins: Status="RUNNING" uppercase treated as not-running; TotalSteps=0 exercises stepLabel() no-index branch; --tool-name flag vs stdin precedence; empty stdin under command+hard blocks; malformed stdin JSON blocks with error log. - Allowlist bypass negative: mcp__plugin_evil_server__devkit_masquerade must block; mcp__devkit__advance short-form must allow. - DEVKIT_SESSION_STALE_TTL_SECONDS garbage matrix (6 subtests): non-numeric, negative, zero, trailing-space (trimmed), empty, overflow. - Longer TTL override: 2h env + 45min-old session stays fresh. - Zero-timestamp warning: session with no UpdatedAt/StartedAt logs the anomaly and still enforces. - Unreadable session file (mode 0o000) fails closed. Unix-only. - Stale session under prompt+soft (previously only command+hard was covered). Pins the stale check's precedence over the step-type switch. New test coverage (shell): - CLAUDE_PLUGIN_ROOT unset → disabled + exit 0 (guard) / approve (stop-guard). Previously completely untested. - Empty bin/ directory → loud warning + allow (guard) / approve (stop-guard). The "fresh clone before first build" scenario. - Versioned binary only (no local-dev symlink) → exec with guard arg. - Multiple versioned binaries coexisting → pick the highest-sorted executable. Pins B1's behaviour so any future refactor of the selection strategy must update the test. Verification: - go test ./... — all packages green. - hooks/hooks_test.sh — 52/52 pass (up from 46), including the 6 new shell wrapper binary-resolution cases. - Latency on the no-workflow hot path: still ~8.5ms per call, and verified empty $CLAUDE_PLUGIN_DATA after 20 runs — no lock file leaked as a side effect (N1 regression fixed). - Cross-compile clean for linux/amd64 and windows/amd64.
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 "<unknown>" 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.
5uck1ess
force-pushed
the
fix/65-native-guard
branch
from
April 11, 2026 04:35
c175ee8 to
a152211
Compare
…ference Addresses two CI failures on a152211 plus two blockers surfaced by the second-pass pr-review-toolkit code review. CI failures ----------- - build-and-test "Check formatting": gofmt -d flagged two docstring comment-list indentations and one test struct alignment. `gofmt -w` applied; no semantic change. - hook-smoke-tests "Run hook smoke tests": the CI job runs `bash hooks/hooks_test.sh` from the repo root with no Go toolchain set up and no CLAUDE_PLUGIN_ROOT exported. Pre-refactor, that was fine because the shell hooks had all their logic inline. After the python3 → Go rewrite the wrappers exec a binary that doesn't exist on a fresh CI checkout, so every "expected exit 2" fixture silently fell through the no-binary fail-open path and looked like a regression. Two-layer fix: 1. hooks/hooks_test.sh now auto-detects the repo root, defaults CLAUDE_PLUGIN_ROOT to it when unset, and auto-builds the engine binary from src/ if it's missing. This lets `bash hooks/hooks_test.sh` just work from a fresh clone, for local dev and CI alike. 2. .github/workflows/ci.yml hook-smoke-tests job now includes actions/setup-go@v5 so the auto-build path is reachable. Second-pass review blockers --------------------------- - C1 (review): the version-picker string-comparison loop in hooks/devkit-guard.sh and devkit-stop-guard.sh was the exact bug the comment above it warned about. `v2.1.9 > v2.1.10` is the lexicographic ordering ("9" > "1"), so the loop would have silently run a stale engine after the 10th patch release. Replaced with `sort -V` (GNU coreutils, available on Ubuntu and recent macOS). The hooks_test.sh multi-version fixture is tightened from v2.1.6/v2.1.7 (both lex-ordered correctly) to v2.1.9/v2.1.10 (the digit-count boundary that actually exercises the bug). - C2 (review): three stderr diagnostics directed the user to `devkit install`, which is not a real subcommand. `src/cmd/` has guard, mcp, status, workflow, and root — no install. The actual self-downloader is `$CLAUDE_PLUGIN_ROOT/bin/devkit` (the committed wrapper that handles download + verify + cache on first run). Updated all three diagnostics to point at `$BIN_DIR/devkit --version` (a side-effect-free invocation that triggers the cache-if-missing path). Verification ------------ - gofmt -l . is clean - go test ./... -race -count=1 — all packages green - hooks/hooks_test.sh — 52/52 pass on a fresh-clone simulation (deleted bin/devkit-engine, unset CLAUDE_PLUGIN_ROOT). The script auto-built the engine and all guard fixtures exercised the real binary. - sort -V version picker verified end-to-end: v2.1.9 + v2.1.10 side by side → wrapper execs v2.1.10 (the new test pins this).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #65
Replaces python3-based parsing in
devkit-guard.sh/devkit-stop-guard.shwith a newdevkit-engine guard [--tool-name] [--stop]Cobra subcommand. Policy is unchanged; the substrate moves to Go for portability, speed, and testability.Why
python3unavailable) or silently skipped enforcement on timeout.SessionStateparser (lib.ReadSessionJSON), eliminating drift.What
Native subcommand (
src/cmd/guard.go)rootCmd.PersistentPreRunEto a no-op so the guard never requires a git repo or opens the SQLite DB — critical because it runs on everyPreToolUse.sessionFileExists(pureos.Stat) runs BEFORElib.ReadSessionJSONso the no-workflow path skips themkdir+session.json.lockcreate side effects. Verified: 20 no-workflow invocations leave an empty$CLAUDE_PLUGIN_DATA.isDevkitMCPToolis anchored on the full plugin+server prefix (mcp__plugin_devkit_devkit-engine__+mcp__devkit__short-form). This is tighter than the pre-fix(hooks): enforce workflow progression on prompt steps + orphan recovery #64mcp__*devkit*substring AND tighter than fix(hooks): enforce workflow progression on prompt steps + orphan recovery #64'smcp__*devkit-engine*glob — a hypothetical second MCP server under the devkit plugin cannot silently inherit command-step permissions.--stopmode emits the Stop-hook JSON verdict byte-for-byte matching the old shell output.writeStopVerdictpanics on the unreachablejson.Marshalfailure path (dead code removed).readToolNameFromStdinreturns(string, error)so three failure modes (read / empty / parse) log distinctly. Empty tool still default-denies under hard enforcement — security posture unchanged, debuggability improved.staleTTLhonoursDEVKIT_SESSION_STALE_TTL_SECONDSwithTrimSpace+ warning on garbage values.sessionIsStalelogs a one-line WARNING when both timestamps are zero so a wedged session leaves a debuggable trail.Args: cobra.NoArgsso extra positional args fail loudly during dev.Thin shell wrappers (
hooks/devkit-guard.sh,hooks/devkit-stop-guard.sh)shopt -s nullglob+ highest-sorted versioned binary selection (the naive "first glob match" would pickv2.1.0overv2.1.10).bin/devkitfirst-run-download fallback is deliberately removed from the hook path. Downloading release assets from a time-limited hook is unsafe — a fresh install should fail closed with a diagnostic pointing atdevkit installrather than silently blocking on a network call.hooks/hooks.jsonDeleted
hooks/lib/read-session.sh(python3 session parser, superseded)hooks/devkit-guard_test.sh(shell fixture matrix, ported to Go as 30+ table-driven rows)Test coverage
Go (
src/cmd/guard_test.go, ~800 lines):wantStderrSubstrpinning veto-message wordingmcp__plugin_evil_server__devkit_masquerade→ block,mcp__plugin_devkit_other_server__probe→ blockprompt+hard+TodoWrite,prompt+soft+Write,parallel+soft+BashRUNNING,TotalSteps=0label fallback,--tool-nameflag vs stdin precedence, empty / malformed stdinDEVKIT_SESSION_STALE_TTL_SECONDSgarbage matrix (6 subtests): non-numeric, negative, zero, trailing-space, empty, overflowt.Parallel()prohibition comment documenting the package-level IO hook globalsShell (
hooks/hooks_test.sh, +105 lines):CLAUDE_PLUGIN_ROOTunset → disabled + exit 0 / approvebin/directory → loud warning + allow / approve (the "fresh clone before first build" scenario)guardargVerification
go test ./...— all packages greenhooks/hooks_test.sh— 52/52 pass (up from 46), including all 6 new shell wrapper binary-resolution cases$CLAUDE_PLUGIN_DATAverified after 20 runs — nosession.json.lockleaked as a side effect on the no-workflow hot pathlinux/amd64,darwin/arm64,windows/amd64History
This PR went through a full mega-review (tri-review + pr-review-toolkit, 6 agents). The review found 2 critical issues my first pass missed:
lib.ReadSessionJSONerror path was silently failing OPEN on permission errors (sessionFileExistspost-hoc check could return false on broken data dirs)isDevkitMCPToolsubstring match would have allowed any third-party MCP server with "devkit" anywhere in its tool nameBoth plus 3 other HIGH-priority findings and 8 MEDIUM recommendations are fixed in this single-commit rebase onto main. See the commit message for the full catalogue.
Test plan
featureworkflow end-to-end on a machine without python3; confirm enforcement worksdevkit-engine-v*binaries