fix: bootstrap engine binary on first run via committed wrapper - #54
Merged
Conversation
Fixes the critical silent failure shipped in v2.1.0/v2.1.1: the plugin
ships via git, but `bin/` was gitignored, so plugin.json's MCP server
command pointed at a file that didn't exist. The MCP server silently
failed to start, hooks had no session state to read, and enforcement
silently did nothing.
Changes
- `bin/devkit` — new committed POSIX shell wrapper. On invocation:
1. If `bin/devkit-engine` exists (dev build), exec it directly.
2. Otherwise read version from plugin.json, detect OS/arch, download
the matching `devkit-<os>-<arch>` release asset from GitHub,
verify its SHA256 against checksums.txt, cache it next to the
wrapper as `devkit-engine-v<ver>-<os>-<arch>`, and exec it.
3. All log output goes to stderr so stdout stays clean for MCP stdio.
4. Failure paths (missing curl/sha256sum, bad checksum, 404, wrong
platform) are loud — no silent fallback.
- `.gitignore` — allow `bin/devkit` (the wrapper) while still ignoring
`bin/devkit-engine*` (engine binaries) and `bin/.checksums.*` (tmp).
- `.claude-plugin/plugin.json` — bump to 2.1.2. MCP command unchanged
(still `${CLAUDE_PLUGIN_ROOT}/bin/devkit`) because that now resolves
to the wrapper.
- `src/Makefile` — local build now produces `bin/devkit-engine`
(the wrapper's dev fast-path name). Release cross-compile still
outputs `bin/devkit-<os>-<arch>` so release assets keep their names.
`sync-version` now refuses to downgrade plugin.json — it only
overwrites when the git tag is strictly higher than the current
plugin.json version, which prevents `make build` from clobbering
manual version bumps on feature branches.
- `.github/workflows/ci.yml` — new `fresh-install-smoke` job that
catches the class of mistake that caused the v2.1.0 shipping bug:
- asserts `bin/devkit` exists and is executable
- asserts `bin/devkit-engine` is NOT committed (should be cached,
never tracked)
- asserts plugin.json's MCP command matches the wrapper path
- runs shellcheck on the wrapper
- runs `./bin/devkit --version` from a clean checkout and confirms
it either succeeds (downloaded + ran) or fails loudly with output,
never exits 0 with empty output
Without this job, the same class of bug can silently reappear.
- `presets/` — removed. Empty v1 directory with only `.gitkeep`, zero
references, dead weight.
Tested
- `shellcheck bin/devkit` → clean
- `./bin/devkit --version` with local engine present → fast path, exit 0
- `./bin/devkit --version` with no local engine, valid version →
downloads, verifies checksum, installs, execs, exit 0
- `./bin/devkit --version` with invalid version (99.99.99) → 404, loud
error to stderr, exit 1
- `go build ./... && go test ./... -race` → all pass
Ships the critical fixes a full mega-pr review on #52 uncovered, beyond the binary-shipping bug already addressed in the first commit on this branch. All findings from silent-failure-hunter and code-reviewer marked CRITICAL or HIGH are addressed here. CRITICAL — shell injection in command steps runCommand interpolated {{input}} and {{step-id}} into `sh -c <cmd>`. Any LLM-chosen input or contaminated prior-step output executed shell. Command and gate strings are now literal YAML; values are passed via $DEVKIT_INPUT and $DEVKIT_OUT_<step_id> env vars. The engine validator rejects {{...}} in command/gate strings at parse time with an explicit "shell injection mitigation" error. Both engine.runCommand and mcp.runCommand were affected; both fixed. The 5 self-* workflows (self-test, self-lint, self-improve, self-perf, self-migrate) were updated to use $DEVKIT_INPUT. Regression tests: TestRunWorkflowCommandRejectsInterpolation, TestRunWorkflowCommandEnvInput, TestRunWorkflowCommandEnvPriorOutput. CRITICAL — PreToolUse guard bypassable hooks/devkit-guard.sh blocklist matched 11 hardcoded tool names and missed Task, SlashCommand, TodoWrite, BashOutput, any mcp__* tool, and every future tool Claude Code adds. Claude could trivially drive a command step by calling Task (subagent) or any non-listed tool. Fixed by inverting to an allowlist: during command steps only mcp__devkit* and TodoWrite are permitted; everything else returns exit 2 with a clear diagnostic including the attempted tool name. CRITICAL — stop-guard and guard had opposite fail-closed policies devkit-stop-guard approved on corrupt session.json; devkit-guard blocked. A corrupt state file mid-workflow would silently end the session. Both now fail closed with a diagnostic and handle the TOCTOU race between the -f test and python open() by treating FileNotFoundError as "no session" (matching pre-check intent). HIGH — CLAUDE_PLUGIN_DATA unset silent no-op Both guard hooks exited 0 with no output when the env var was missing. Combined with the binary-shipping bug, enforcement was an invisible no-op. Added stderr warnings so the degraded state is observable. HIGH — loop until / branch when substring false-positives strings.Contains("no failures found", "fail") returned true, silently terminating loops or routing branches incorrectly. Same bug in EvalBranch. Both now use word-boundary matching (grep -w semantics) via a new containsWord helper: the sentinel must be bounded on both sides by a non-alphanumeric/underscore character. Accepts idiomatic forms like "TINY: short fix" and "attempt 2: ALL_PASSING" while rejecting accidental substrings. Regression tests cover both directions. HIGH — completeWorkflow swallowed git.CommitAll errors A failed final commit was logged to stderr only, so the user saw "WORKFLOW COMPLETE" with uncommitted branch work. Now collected into a warnings list and surfaced in the MCP response body, alongside any DB update or state-clear failures. HIGH — runCommand discarded stdout/stderr on non-ExitError failures When the error wasn't an *exec.ExitError (missing binary, permission denied, ctx cancelled), the captured combined stream was thrown away. Now returned alongside the wrapped error so users see the actual cause. Also distinguishes context.DeadlineExceeded (returns exit 124 + "timed out after 5m") from generic execution failure. MEDIUM — YAML typos silently ignored yaml.Unmarshal accepted unknown fields, so "commnd: foo" (missing e) parsed as a no-op step. Now uses NewDecoder(bytes).KnownFields(true); typos fail loudly at parse time. MEDIUM — make sync-version clobbered manual bumps Triggering any make target on a feature branch rewrote plugin.json to the latest git tag, silently undoing manual version bumps. Now only writes when the git tag is strictly higher than the current plugin.json version (semver ordering via sort -V). MEDIUM — pr-gate.sh violated project shell rules Missing `set -euo pipefail`, used /tmp/devkit-pr-gate-done shared across concurrent projects, and arithmetic fallback that silently skipped the gate on any failure. Now strict mode, per-user cooldown file under $XDG_CACHE_HOME/devkit, and explicit numeric-regex guard on the timestamp comparison. Stale docs - src/cmd/status.go, README.md, CONTRIBUTING.md, CHANGELOG.md — replaced `devkit workflow run <name>` with the correct `devkit workflow <name> "<description>"`. There is no `run` subcommand; src/cmd/workflow.go defines `Use: "workflow [name] ..."`. - README.md architecture block — reframed the "binary ships in bin/" claim to describe the wrapper + download flow introduced by this PR. - workflows/pr-ready.yml description — updated to reflect all 8 steps (validate, necessity, lint, test, security, changelog, create-pr, monitor) rather than the stale 5-step list from before pr-monitor was folded in. - hooks/hooks.json top-level description — now mentions the workflow guard and completion guard hooks that were silently added in 2.1.0. - ROADMAP.md — 19 → 22 skills (stale count from before the Playwright skills were added in PR #53). - CHANGELOG.md — new 2.1.2 section documenting every fix in this PR.
Concurrent safety
- Cross-process flock on session.json via sibling .lock file. Two
simultaneous devkit_advance calls can no longer both read at index N
and both write at index N+1 — a Busy claim flag under the lock makes
the losing caller report "step already in progress".
- UpdateSessionJSON helper provides a true read-modify-write primitive.
- Per-writer CreateTemp names avoid collisions between racing writers.
- session.json chmod 0o600 (may contain pasted secrets).
Validator hardening
- Reject empty branch when: — strings.Contains(x, "") is always true.
- Require exactly one of prompt | command | parallel per step.
- Deterministic Interpolate: sort map keys so nested {{id}} refs render
identically on every call.
Fail-fast on branch mode
- devkit_start now errors out if branch: true is set and CreateBranch
fails (or git is unavailable), rather than silently committing onto
the user's current branch in completeWorkflow.
Independent gate timeout
- Loop gates use a 60s timeout instead of inheriting the 5-minute
command timeout.
Hook enforcement tests
- 16 new tests for devkit-guard.sh and devkit-stop-guard.sh covering
command/prompt, hard/soft enforce, allowed/blocked tool names,
corrupt JSON fail-closed, and missing env var.
37 hook smoke tests and full Go test suite pass.
silent-failure-hunter (HIGH) - Remove premature claimReleased=true at loop-dispatch and completeWorkflow-dispatch sites. The deferred release is now the sole authority for clearing Busy and runs on every exit path, including gate-command errors and mid-helper panics that would otherwise leak the claim forever. - Move session.json write to AFTER CreateBranch in devkit_start so branch-mode failure cannot leave a half-initialized session that a concurrent devkit_advance could race. code-reviewer - Log release-claim failures to stderr with recovery hint instead of silently dropping the error; a stuck Busy=true is now observable. pr-test-analyzer - TestAdvanceRealRace: N concurrent goroutines racing advance on a sleeping command step. Asserts exactly one succeeds and N-1 are rejected with "already in progress". Passes under -race. - TestAdvanceCommandFailClearsBusy: expect:success mismatch must not leak the Busy claim. - TestStartBranchModeRollback: branch: true with nil git leaves no session.json behind. - TestUpdateSessionJSONNoChange: fn returning nil leaves mtime and contents untouched. - hooks_test.sh: two new cases for schema drift — missing enforce field defaults to hard, missing step_type treated as non-command. comment-analyzer - Trim stale caller-referencing comments in state_json.go and tools.go that would rot if call sites move. hooks_test.sh hygiene - EXIT/INT/TERM trap with tracked tmp dirs so Ctrl-C cleans up. 39 hook tests, Go suite passes under -race.
Windows build (Codex HIGH)
- Split session lock into state_lock_unix.go (flock via unix.Flock)
and state_lock_windows.go (LockFileEx via golang.org/x/sys/windows)
behind //go:build tags. The previous unconditional unix.Flock import
broke cross-compilation for GOOS=windows which the release Makefile
still ships. Verified with GOOS=windows GOARCH=amd64 go build ./...
devkit_start start/start race (Codex HIGH)
- Previous code did ReadSessionJSON then WriteSessionJSON non-atomically;
two concurrent devkit_start calls both observed "no session" and both
clobbered. Now: UpdateSessionJSON claims the slot with Status="starting"
under the lock, side effects (branch creation) run, then another
UpdateSessionJSON transitions to Status="running". Rollback on failure
via deferred committed flag. Added TestStartConcurrentRace with 8
goroutines — exactly one succeeds, seven report "already".
- advanceTool rejects sessions in Status="starting" so Claude cannot
observe a half-initialized session between the two transitions.
runCommandWithTimeout exit 124 (Codex MED)
- On Unix, exec.CommandContext timeout kills via SIGKILL which surfaces
as *exec.ExitError with ExitCode() == -1, NOT a non-ExitError.
Previous ctx.Err() check lived in the non-ExitError branch and never
ran. Check now comes FIRST so timeouts correctly return exit 124 with
a clear "command timed out after N" message.
envKey collision validator (Codex MED)
- sanitizeEnvKey collapsed "a-b", "a_b", "a.b" to the same DEVKIT_OUT_A_B
suffix, silently shadowing outputs. Consolidated into engine.EnvKey
(exported), added validator rejection when two step IDs collide under
the mapping, and pointed both mcp package sites at engine.EnvKey.
bin/devkit wrapper concurrency (Codex MED)
- Previous wrapper used a fixed "${ENGINE_PATH}.tmp" and shared checksum
tmp path. Two concurrent first runs could race. Now uses mktemp -d
for a per-invocation temp directory with trap cleanup on exit.
All platforms compile, full Go suite passes under -race, 39 hook tests
pass.
Replace A && B || C with an explicit if block at the ENGINE_PATH validation site. The old form is flagged by shellcheck because it is not equivalent to if-then-else — C can run even when A is true. In our case die() is a terminal call so the semantics were fine, but the CI shellcheck job fails the build regardless.
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
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.
Problem
v2.1.0/v2.1.1 shipped a critical silent failure.
plugin.jsonpointed the MCP server at\${CLAUDE_PLUGIN_ROOT}/bin/devkit, butbin/was in.gitignore, so the marketplace-installed plugin contained no binary. The MCP server silently failed to start. Hooks loaded but had no session state to read. Enforcement silently did nothing. Every user who installed 2.1.0 got a broken engine, and workflows fell back to direct agent dispatch without anyone noticing.The real cause wasn't just a missing file — it was the absence of a CI check that would have caught the shipping tree not matching runtime expectations.
Fix
1.
bin/devkitwrapper (committed POSIX sh)bin/devkit-engineexists (dev build frommake install-plugin), exec it directly — fast path, no network.plugin.json, detect OS/arch, download the matchingdevkit-<os>-<arch>asset from the GitHub release for that version, verify its SHA256 against the release'schecksums.txt, cache the binary alongside the wrapper asdevkit-engine-v<ver>-<os>-<arch>, then exec it.2.
.gitignore— commit the wrapper, ignore the binaries```
bin/devkit-engine
bin/devkit-engine-*
bin/.checksums.*
src/bin/
```
3. Makefile — split dev build from release cross-compile
4. CI `fresh-install-smoke` job — the class-of-bug guard
This is the thing that would have caught the v2.1.0 bug before merge. On every PR, in a clean checkout:
5. Removed `presets/` — empty v1 directory with only `.gitkeep`, zero references.
Tested locally
Why this is the right shape
The wrapper pattern is how npm, rustup, and homebrew handle platform-specific binaries. Shipping all 6 platforms in git would be ~60MB; committing a shell script that downloads the right one on first run is ~30 lines and auto-heals on version bumps.
More importantly: the CI smoke test is the real prevention. A future change could reintroduce the same class of bug (wrong path, missing file, broken plugin.json), and the smoke test catches it the same way every time.
Test plan