From 62ea2f3fcaf66459cf500d1ab4cf79c51d69b174 Mon Sep 17 00:00:00 2001
From: Tym Rabchuk <tymrabchuk@gmail.com>
Date: Fri, 10 Apr 2026 00:54:27 -0400
Subject: [PATCH 1/6] fix: bootstrap engine binary on first run via committed
 wrapper
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
---
 .claude-plugin/plugin.json |   2 +-
 .github/workflows/ci.yml   |  55 ++++++++++++++++
 .gitignore                 |   7 +-
 bin/devkit                 | 129 +++++++++++++++++++++++++++++++++++++
 presets/.gitkeep           |   0
 src/Makefile               |  18 +++---
 6 files changed, 200 insertions(+), 11 deletions(-)
 create mode 100755 bin/devkit
 delete mode 100644 presets/.gitkeep

diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json
index 01d23ac..10c3b20 100644
--- a/.claude-plugin/plugin.json
+++ b/.claude-plugin/plugin.json
@@ -1,6 +1,6 @@
 {
   "name": "devkit",
-  "version": "2.1.1",
+  "version": "2.1.2",
   "description": "A deterministic development harness for AI agents — YAML workflow engine, self-learning hooks, and multi-agent consensus",
   "author": {
     "name": "5uck1ess"
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 58c03d2..912647b 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -47,6 +47,61 @@ jobs:
       - name: Run hook smoke tests
         run: bash hooks/hooks_test.sh
 
+  # Catches the class of bug where the shipped marketplace tree is
+  # missing something the plugin needs at runtime. PR #52 shipped a
+  # plugin.json pointing at bin/devkit, but bin/ was gitignored — the
+  # plugin installed but the MCP server silently failed to start.
+  # This job runs in a clean dir with no local build artifacts.
+  fresh-install-smoke:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Bootstrap wrapper must exist and be executable
+        run: |
+          test -f bin/devkit || { echo "FATAL: bin/devkit wrapper missing from repo"; exit 1; }
+          test -x bin/devkit || { echo "FATAL: bin/devkit not executable"; exit 1; }
+
+      - name: Wrapper must not contain devkit-engine binary (that is gitignored)
+        run: |
+          if [ -e bin/devkit-engine ]; then
+            echo "FATAL: bin/devkit-engine should not be tracked in git (it is the cached binary)"
+            exit 1
+          fi
+
+      - name: plugin.json MCP command must match wrapper path
+        run: |
+          cmd=$(jq -r '.mcpServers["devkit-engine"].command' .claude-plugin/plugin.json)
+          expected='${CLAUDE_PLUGIN_ROOT}/bin/devkit'
+          if [ "$cmd" != "$expected" ]; then
+            echo "FATAL: plugin.json mcpServers.devkit-engine.command is '$cmd', expected '$expected'"
+            exit 1
+          fi
+
+      - name: shellcheck wrapper
+        run: |
+          sudo apt-get update -qq && sudo apt-get install -y -qq shellcheck
+          shellcheck bin/devkit
+
+      - name: Wrapper --help must exit with a download attempt (no local engine)
+        run: |
+          # With no local bin/devkit-engine and no network cache, the wrapper
+          # should try to download and either succeed or fail loudly. It must
+          # NOT silently pass with exit 0 and no output.
+          set +e
+          output=$(./bin/devkit --version 2>&1)
+          exit_code=$?
+          set -e
+          echo "wrapper exit=$exit_code"
+          echo "wrapper output: $output"
+          # The wrapper must either succeed (downloaded + ran --version) or
+          # emit a clear error. An empty output with exit 0 would be the silent
+          # failure class we're guarding against.
+          if [ $exit_code -eq 0 ] && [ -z "$output" ]; then
+            echo "FATAL: wrapper exited 0 with no output — silent failure class bug"
+            exit 1
+          fi
+
   validate-counts:
     runs-on: ubuntu-latest
     steps:
diff --git a/.gitignore b/.gitignore
index d63e9c6..cc2ad9b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,5 +1,8 @@
-# Compiled binaries
-bin/
+# Compiled engine binaries (wrapper bin/devkit is committed)
+bin/devkit-engine
+bin/devkit-engine-*
+bin/.checksums.*
+src/bin/
 
 # Credentials — never commit
 *.token
diff --git a/bin/devkit b/bin/devkit
new file mode 100755
index 0000000..b7a9f07
--- /dev/null
+++ b/bin/devkit
@@ -0,0 +1,129 @@
+#!/bin/sh
+# devkit wrapper — resolves and execs the platform engine binary.
+#
+# The real Go binary is not committed to git (platform-specific, ~10MB).
+# This wrapper downloads the matching release asset from GitHub on first
+# run, verifies its SHA256, caches it alongside this script, and execs it.
+#
+# All log output goes to stderr. Stdout stays clean because the MCP stdio
+# protocol runs over it when Claude Code invokes `devkit mcp`.
+
+set -eu
+
+SCRIPT_DIR=$(CDPATH='' cd -- "$(dirname -- "$0")" && pwd)
+PLUGIN_ROOT=$(CDPATH='' cd -- "$SCRIPT_DIR/.." && pwd)
+PLUGIN_JSON="$PLUGIN_ROOT/.claude-plugin/plugin.json"
+RELEASE_OWNER="5uck1ess"
+RELEASE_REPO="devkit"
+
+log() { printf 'devkit: %s\n' "$*" >&2; }
+die() { log "$*"; exit 1; }
+
+# Fast path: developer-built binary next to this script (from `make install-plugin`).
+LOCAL_DEV="$SCRIPT_DIR/devkit-engine"
+if [ -x "$LOCAL_DEV" ]; then
+    exec "$LOCAL_DEV" "$@"
+fi
+
+# Sets VERSION (global) so die() kills the parent shell on failure.
+read_version() {
+    [ -f "$PLUGIN_JSON" ] || die "plugin.json not found at $PLUGIN_JSON"
+    VERSION=$(sed -n 's/.*"version"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' "$PLUGIN_JSON" | head -1)
+    [ -n "$VERSION" ] || die "could not read version from $PLUGIN_JSON"
+}
+
+# Sets PLATFORM (global) so die() kills the parent shell on failure.
+detect_platform() {
+    uname_s=$(uname -s 2>/dev/null || echo unknown)
+    uname_m=$(uname -m 2>/dev/null || echo unknown)
+    case "$uname_s" in
+        Linux)  os=linux ;;
+        Darwin) os=darwin ;;
+        MINGW*|MSYS*|CYGWIN*) os=windows ;;
+        *) die "unsupported OS: $uname_s" ;;
+    esac
+    case "$uname_m" in
+        x86_64|amd64)  arch=amd64 ;;
+        arm64|aarch64) arch=arm64 ;;
+        *) die "unsupported arch: $uname_m" ;;
+    esac
+    PLATFORM="${os}-${arch}"
+}
+
+sha256_of() {
+    f=$1
+    if command -v sha256sum >/dev/null 2>&1; then
+        sha256sum "$f" | awk '{print $1}'
+    elif command -v shasum >/dev/null 2>&1; then
+        shasum -a 256 "$f" | awk '{print $1}'
+    else
+        die "need sha256sum or shasum to verify downloads"
+    fi
+}
+
+download() {
+    url=$1
+    out=$2
+    if command -v curl >/dev/null 2>&1; then
+        curl -fsSL --retry 2 --retry-delay 2 -o "$out" "$url"
+    elif command -v wget >/dev/null 2>&1; then
+        wget -q -O "$out" "$url"
+    else
+        die "need curl or wget to download engine binary"
+    fi
+}
+
+# ensure_engine sets ENGINE_PATH as a side effect rather than echoing it,
+# so `die` inside this function can `exit 1` on the parent shell. Command
+# substitution would run it in a subshell, hiding the failure exit code.
+ensure_engine() {
+    read_version
+    detect_platform
+    ext=""
+    case "$PLATFORM" in windows-*) ext=".exe" ;; esac
+
+    engine_name="devkit-engine-v${VERSION}-${PLATFORM}${ext}"
+    ENGINE_PATH="$SCRIPT_DIR/$engine_name"
+
+    if [ -x "$ENGINE_PATH" ]; then
+        return 0
+    fi
+
+    # Remove stale cached engines from old versions in the same directory.
+    find "$SCRIPT_DIR" -maxdepth 1 -name 'devkit-engine-v*' ! -name "$engine_name" \
+        -type f -exec rm -f {} + 2>/dev/null || true
+
+    tag="v${VERSION}"
+    asset="devkit-${PLATFORM}${ext}"
+    base_url="https://github.com/${RELEASE_OWNER}/${RELEASE_REPO}/releases/download/${tag}"
+
+    log "first-run: downloading engine ${tag} (${PLATFORM})…"
+
+    tmp_bin="${ENGINE_PATH}.tmp"
+    tmp_sums="${SCRIPT_DIR}/.checksums.${VERSION}.tmp"
+    trap 'rm -f "$tmp_bin" "$tmp_sums"' EXIT INT TERM
+
+    download "${base_url}/${asset}" "$tmp_bin" \
+        || die "download failed: ${base_url}/${asset}"
+    download "${base_url}/checksums.txt" "$tmp_sums" \
+        || die "checksum file download failed from ${base_url}/checksums.txt"
+
+    expected=$(awk -v name="$asset" '$2 == name || $2 == "*"name { print $1; exit }' "$tmp_sums")
+    [ -n "$expected" ] || die "no checksum entry for $asset in release $tag"
+
+    actual=$(sha256_of "$tmp_bin")
+    [ "$actual" = "$expected" ] \
+        || die "checksum mismatch for $asset (expected $expected, got $actual)"
+
+    chmod +x "$tmp_bin"
+    mv -f "$tmp_bin" "$ENGINE_PATH"
+    rm -f "$tmp_sums"
+    trap - EXIT INT TERM
+
+    log "installed engine at $ENGINE_PATH"
+}
+
+ENGINE_PATH=""
+ensure_engine
+[ -n "$ENGINE_PATH" ] && [ -x "$ENGINE_PATH" ] || die "engine path not resolved"
+exec "$ENGINE_PATH" "$@"
diff --git a/presets/.gitkeep b/presets/.gitkeep
deleted file mode 100644
index e69de29..0000000
diff --git a/src/Makefile b/src/Makefile
index fede679..b792838 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -1,4 +1,5 @@
-BINARY  := devkit
+BINARY         := devkit-engine
+RELEASE_BINARY := devkit
 VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev")
 # Clean semver for plugin.json (strip leading v; falls back to 0.0.0 when untagged)
 SEMVER  := $(shell V=$$(git describe --tags --abbrev=0 2>/dev/null | sed 's/^v//'); echo "$${V:-0.0.0}")
@@ -22,7 +23,8 @@ sync-version:
 	else \
 		CURRENT=$$(jq -r '.version' "$(PLUGIN_JSON)" 2>/dev/null) || \
 			{ echo "ERROR: plugin.json is not valid JSON"; exit 1; }; \
-		if [ "$$CURRENT" != "$(SEMVER)" ]; then \
+		HIGHER=$$(printf '%s\n%s\n' "$$CURRENT" "$(SEMVER)" | sort -V | tail -1); \
+		if [ "$$HIGHER" = "$(SEMVER)" ] && [ "$$CURRENT" != "$(SEMVER)" ]; then \
 			jq --arg v "$(SEMVER)" '.version = $$v' "$(PLUGIN_JSON)" > "$(PLUGIN_JSON).tmp" \
 				&& mv "$(PLUGIN_JSON).tmp" "$(PLUGIN_JSON)" \
 				|| { rm -f "$(PLUGIN_JSON).tmp"; echo "ERROR: failed to sync plugin.json"; exit 1; }; \
@@ -33,10 +35,10 @@ sync-version:
 build: | sync-version
 	go build $(GOFLAGS) -ldflags '$(LDFLAGS)' -o bin/$(BINARY) .
 
-build-for: | sync-version ## Cross-compile for specified GOOS/GOARCH
+build-for: | sync-version ## Cross-compile release asset for specified GOOS/GOARCH (outputs bin/devkit-<os>-<arch>)
 	@EXT=""; [ "$(GOOS)" = "windows" ] && EXT=".exe"; \
-	CGO_ENABLED=0 GOOS=$(GOOS) GOARCH=$(GOARCH) go build $(GOFLAGS) -ldflags '$(LDFLAGS)' -o bin/$(BINARY)-$(GOOS)-$(GOARCH)$${EXT} . && \
-	echo "Built: bin/$(BINARY)-$(GOOS)-$(GOARCH)$${EXT}"
+	CGO_ENABLED=0 GOOS=$(GOOS) GOARCH=$(GOARCH) go build $(GOFLAGS) -ldflags '$(LDFLAGS)' -o bin/$(RELEASE_BINARY)-$(GOOS)-$(GOARCH)$${EXT} . && \
+	echo "Built: bin/$(RELEASE_BINARY)-$(GOOS)-$(GOARCH)$${EXT}"
 
 build-all: ## Build all platform binaries
 	@$(MAKE) build-for GOOS=linux GOARCH=amd64
@@ -53,12 +55,12 @@ install-plugin: | sync-version ## Build and copy binary to plugin bin/ for curre
 
 install: | sync-version
 	go install $(GOFLAGS) -ldflags '$(LDFLAGS)' .
-	@echo "Installed to $$(go env GOPATH)/bin/$(BINARY)"
+	@echo "Installed to $$(go env GOPATH)/bin/$(RELEASE_BINARY)"
 	@echo "Ensure $$(go env GOPATH)/bin is in your PATH"
 
 link: build
-	ln -sf $(CURDIR)/bin/$(BINARY) /usr/local/bin/$(BINARY)
-	@echo "Linked bin/$(BINARY) → /usr/local/bin/$(BINARY)"
+	ln -sf $(CURDIR)/bin/$(BINARY) /usr/local/bin/$(RELEASE_BINARY)
+	@echo "Linked bin/$(BINARY) → /usr/local/bin/$(RELEASE_BINARY)"
 
 clean:
 	rm -rf bin/

From 6ed6e79a9bc75d327d6e9554bf485fc416daf847 Mon Sep 17 00:00:00 2001
From: Tym Rabchuk <tymrabchuk@gmail.com>
Date: Fri, 10 Apr 2026 01:15:43 -0400
Subject: [PATCH 2/6] fix: make MCP engine enforcement actually work (PR #54
 expanded)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 CHANGELOG.md               |  37 ++++++++++++--
 CONTRIBUTING.md            |   2 +-
 README.md                  |  13 +++--
 ROADMAP.md                 |   2 +-
 hooks/devkit-guard.sh      |  42 ++++++++++++---
 hooks/devkit-stop-guard.sh |  28 +++++++---
 hooks/hooks.json           |   2 +-
 hooks/pr-gate.sh           |  35 +++++++------
 src/cmd/status.go          |   2 +-
 src/engine/engine.go       |  53 +++++++++++++++----
 src/engine/engine_test.go  |  64 +++++++++++++++++++++--
 src/engine/workflow.go     |  85 +++++++++++++++++++++++++++++--
 src/mcp/tools.go           | 102 ++++++++++++++++++++++++++++++-------
 src/mcp/tools_test.go      |  44 +++++++++++++++-
 workflows/pr-ready.yml     |   2 +-
 workflows/self-improve.yml |   6 +--
 workflows/self-lint.yml    |   6 +--
 workflows/self-migrate.yml |   6 +--
 workflows/self-perf.yml    |   6 +--
 workflows/self-test.yml    |   9 ++--
 20 files changed, 454 insertions(+), 92 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c490ff0..f390531 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,36 @@
 # Changelog
 
+## 2.1.2
+
+### Make the MCP engine actually work + tighten enforcement (PR #54)
+
+Ships the critical fix that 2.1.0 missed, plus the determinism gaps that a full mega-pr review on #52 uncovered. Restores the engine to a state where step enforcement is not bypassable and command steps are not shell-injectable.
+
+#### Fixed (CRITICAL)
+- **Engine binary now actually ships with the plugin.** 2.1.0/2.1.1 set `mcpServers.devkit-engine.command` to `${CLAUDE_PLUGIN_ROOT}/bin/devkit` but `bin/` was gitignored, so the marketplace install had no binary. MCP server silently failed to start and every workflow silently fell back to direct agent dispatch. Fixed by committing a POSIX shell wrapper at `bin/devkit` that downloads the matching release asset on first run, verifies SHA256, caches it next to itself, and execs it. Local dev builds (`make install-plugin`) are used directly via a fast path.
+- **Shell injection in command steps.** `runCommand` interpolated `{{input}}` and `{{step-id}}` into `sh -c <command>`. LLM-chosen input or contaminated prior-step output could execute arbitrary shell. Fixed: command and gate strings are now literal YAML; values are passed via `$DEVKIT_INPUT` and `$DEVKIT_OUT_<step_id>` env vars. Workflow validator rejects `{{...}}` in command/gate strings at parse time with an explicit "shell injection mitigation" error. The 5 self-* workflows were updated to use `$DEVKIT_INPUT`.
+- **PreToolUse guard was trivially bypassable.** The blocklist only matched 11 hardcoded tool names (Bash, Edit, Write, ...) and missed Task, SlashCommand, TodoWrite, mcp__*, and any new tools Claude Code adds. Fixed: guard now uses an allowlist — during command steps, only `mcp__devkit*` tools and `TodoWrite` are permitted. Everything else returns exit 2.
+- **Stop guard failed open on corrupt session.json, guard failed closed.** Opposite policies on the same condition meant workflow completion could be silently approved on a corrupt state file. Fixed: both hooks now fail closed with a clear diagnostic when JSON parsing fails.
+- **Loop `until` matched substrings.** `strings.Contains("no failures found", "fail")` returned true, silently terminating loops. Same bug in branch `when:` matching. Fixed: both now use word-boundary matching (grep -w semantics) — `fail` won't match `failures`.
+
+#### Fixed (HIGH)
+- `completeWorkflow` ignored `git.CommitAll` errors — users saw "WORKFLOW COMPLETE" while their branch work was unpersisted. Now surfaces warnings in the result text and on stderr.
+- `runCommand` threw away command output on non-`*exec.ExitError` failures. Now returns the combined stream alongside the error, and distinguishes `context.DeadlineExceeded` (5-minute timeout) from "command crashed" with exit 124.
+- Hook fail-silent when `CLAUDE_PLUGIN_DATA` is unset. Now emits a stderr warning so the degraded state is observable.
+- TOCTOU race between hook `[[ -f session.json ]]` check and python `open()`: if the engine cleared the file between those two, hooks spuriously failed closed. Python code now handles `FileNotFoundError` explicitly as "no session".
+
+#### Fixed (MEDIUM)
+- YAML workflow parsing now uses `KnownFields(true)` — typos like `commnd:` fail loudly instead of silently dropping the field.
+- `make sync-version` no longer downgrades `plugin.json`. It only writes when the git tag is strictly higher than the current version, which prevents local dev builds on feature branches from clobbering manual version bumps.
+- `bin/devkit` wrapper emits `devkit:` diagnostics to stderr on download, checksum, install, and failure paths. Stdout stays clean so it doesn't corrupt the MCP stdio protocol.
+
+#### Added
+- **CI `fresh-install-smoke` job.** Runs on every PR in a clean checkout. Asserts `bin/devkit` exists and is executable, that `bin/devkit-engine` is not tracked, that `plugin.json`'s MCP command matches the wrapper path, runs `shellcheck bin/devkit`, and runs `./bin/devkit --version` to confirm it either succeeds or fails loudly — never exits 0 with empty output. This is the check that would have caught the v2.1.0 binary-shipping bug before merge.
+- Regression tests: `TestRunWorkflowCommandRejectsInterpolation`, `TestRunWorkflowCommandEnvInput`, `TestRunWorkflowCommandEnvPriorOutput`, `TestLoopUntilRejectsSubstring`.
+
+#### Removed
+- `presets/` — empty v1 directory with only `.gitkeep`, no references anywhere.
+
 ## 2.1.0
 
 ### MCP Engine — Deterministic Workflow Enforcement (PR #52)
@@ -21,10 +52,10 @@ Replaces the broken subprocess-spawning engine with an MCP server that runs insi
 - **Enforcement** — None (markdown honor system) → MCP tool scoping + PreToolUse exit 2
 - **Principle skills** — Loaded if Claude decided to → injected by engine per step
 - **Token usage** — ~50k+ for 8-step workflow → ~17k (~65% reduction)
-- **Skills and commands** — All 8 entry points (research, deep-research, autoloop, tri-review, tri-debug, tri-security, pr-ready, status) now use MCP tools instead of `ensure-engine.sh` + `devkit workflow run`
+- **Skills and commands** — All 8 entry points (research, deep-research, autoloop, tri-review, tri-debug, tri-security, pr-ready, status) now use MCP tools instead of `ensure-engine.sh` + `devkit workflow`
 
 #### Removed
-- `scripts/ensure-engine.sh` — no longer needed (binary ships in `bin/`, auto-PATH)
+- `scripts/ensure-engine.sh` — no longer needed; replaced by the committed `bin/devkit` wrapper in 2.1.2 (see below), which downloads the engine binary on first run and caches it next to itself. In 2.1.0 the wrapper was missing entirely, which shipped a broken plugin — 2.1.2 fixes that and adds CI smoke tests to prevent regression.
 - `scripts/install-engine.sh` — installed by plugin manifest
 
 #### Fixed
@@ -37,7 +68,7 @@ Replaces the broken subprocess-spawning engine with an MCP server that runs insi
 Major architectural shift: all command logic moved from LLM-interpreted markdown to Go-engine-driven YAML workflows.
 
 #### Changed
-- **24 → 8 slash commands** — 16 commands deleted, logic now in YAML workflows invoked via `devkit workflow run <name>` or context-activated skills
+- **24 → 8 slash commands** — 16 commands deleted, logic now in YAML workflows invoked via `devkit workflow <name> "<description>"` or context-activated skills
 - **4 ultra-thin wrappers** — tri-review, tri-debug, tri-security, pr-ready (one-liner pointing to workflow)
 - **4 kept as-is** — pr-monitor, status, setup-rules, workflow
 - **~3,600 lines removed** across PRs 1–5
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 8b67248..bdaaef9 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -5,7 +5,7 @@
 Most command logic lives in YAML workflows executed by the Go engine. Only 6 slash commands remain as tab-completable entry points — everything else is context-activated via skills.
 
 1. Create `workflows/my-workflow.yml` with steps, model assignments, and loop/gate definitions
-2. Test with `devkit_start` MCP tool or `devkit workflow run my-workflow "input"` from terminal
+2. Test with `devkit_start` MCP tool or `devkit workflow my-workflow "input"` from terminal
 3. Optionally add a context-activated skill in `skills/` to auto-trigger it
 
 See `skills/creating-workflows/SKILL.md` for YAML schema reference.
diff --git a/README.md b/README.md
index 966c0fe..1c47758 100644
--- a/README.md
+++ b/README.md
@@ -244,14 +244,21 @@ Language-specific rules that auto-activate when Claude reads matching files. Ins
 
 ```
 MCP Server (bin/devkit mcp — auto-started by plugin)
+  ├── bin/devkit = POSIX shell wrapper (committed to git)
+  │   └── On first run, downloads matching release asset from GitHub,
+  │       verifies SHA256, caches as bin/devkit-engine-v<ver>-<os>-<arch>,
+  │       then execs it. Local dev builds (make install-plugin) are used
+  │       directly via the fast path.
   ├── Tools: devkit_start, devkit_advance, devkit_status, devkit_list
   ├── State: session.json (hot, <50ms reads) + SQLite (cold history)
   ├── Parse YAML → validate steps, branches, budget
   ├── Walk steps:
   │   ├── Command steps → engine executes shell directly ($0 cost)
+  │   │   Values passed via $DEVKIT_INPUT / $DEVKIT_OUT_<step_id>
+  │   │   env vars — never interpolated into the command string.
   │   ├── Prompt steps → Claude works, calls devkit_advance when done
   │   ├── Loop with gate → run, verify, keep or revert
-  │   ├── Branch → case-insensitive substring match → goto
+  │   ├── Branch → case-insensitive word-boundary match → goto
   │   └── Parallel → Agent tool dispatch (Claude/Codex/Gemini)
   └── Principles injected per step (~120 tokens, not full skill files)
 
@@ -260,7 +267,7 @@ Enforcement:
   ├── PreToolUse hook — exit 2 blocks tools during command steps
   └── Stop hook — blocks session end during active workflows
 
-Terminal fallback (devkit workflow run <name>):
+Terminal usage (devkit workflow <name> "<description>"):
   └── Subprocess runners for Codex/Gemini CLI usage
 ```
 
@@ -282,6 +289,6 @@ devkit/
 │   ├── runners/       # Codex, Gemini interfaces (terminal fallback)
 │   ├── lib/           # DB, git, metrics, session state, reporting
 │   └── cmd/           # CLI entry points (including `devkit mcp`)
-├── bin/               # Auto-PATH binary (built by make install-plugin)
+├── bin/               # devkit wrapper (committed) + downloaded engine binaries (gitignored)
 └── .github/workflows/ # CI (build+test+vet) + auto-release (6 platforms)
 ```
diff --git a/ROADMAP.md b/ROADMAP.md
index ef58b0e..76ee1dc 100644
--- a/ROADMAP.md
+++ b/ROADMAP.md
@@ -36,6 +36,6 @@ Items below were on the roadmap but determined to be unnecessary — either alre
 | Stop hook redesign | Still fires every turn, but exits early with `approve` when no files are changed — near-instant on clean trees, so the performance concern is moot. Revisit only if it causes measurable latency. |
 | Cost event hooks | Budget enforcement already exists in the Go engine via `overBudget()` + `addCost()` callbacks with hard limits |
 | Execution registry | Step tracking already handled by SQLite via `lib.DB` with status, cost, and timing per step |
-| Preset library | The 18 YAML workflows and 19 skills already serve this purpose |
+| Preset library | The 18 YAML workflows and 22 skills already serve this purpose |
 | Framework-specific review checklists | `lang-review.sh` covers language-level patterns; framework-specific rules are better added per-project via hookify |
 | Conditional hook firing | Hooks already self-filter internally (extension checks, changed-file checks); a generic condition system adds complexity for no current need |
diff --git a/hooks/devkit-guard.sh b/hooks/devkit-guard.sh
index 8f53a2e..5419f6e 100755
--- a/hooks/devkit-guard.sh
+++ b/hooks/devkit-guard.sh
@@ -4,9 +4,21 @@ set -euo pipefail
 # 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.
+#
+# Policy: during a command step (workflow.yml `command:`), the engine —
+# not Claude — executes the shell. The only tool Claude is allowed to
+# call is devkit_advance (which triggers execution and returns the next
+# step). Everything else is blocked so Claude cannot observe or
+# interfere with the state of the step.
+#
+# 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.
 
 DATA_DIR="${CLAUDE_PLUGIN_DATA:-}"
 if [[ -z "$DATA_DIR" ]]; then
+  printf 'devkit-guard: CLAUDE_PLUGIN_DATA unset — enforcement disabled\n' >&2
   exit 0  # not in plugin context
 fi
 
@@ -17,10 +29,16 @@ fi
 
 # Parse all session fields in a single python3 call (no jq dependency).
 # Outputs tab-separated: status, step_type, enforce, current_step.
-# Passes file path via sys.argv to prevent shell injection.
+# Passes file path via sys.argv to prevent shell injection. Handles
+# FileNotFoundError so a TOCTOU race (file cleared between -f and open)
+# is treated as "no session," matching the pre-check intent.
 SESSION_DATA=$(python3 -c "
 import json, sys
-d = json.load(open(sys.argv[1]))
+try:
+    d = json.load(open(sys.argv[1]))
+except FileNotFoundError:
+    print('\t'.join(['', '', '', '']))
+    sys.exit(0)
 print('\t'.join([
     d.get('status', ''),
     d.get('step_type', ''),
@@ -29,7 +47,7 @@ print('\t'.join([
 ]))
 " "$SESSION_FILE" 2>/dev/null) || {
   # python3 unavailable or JSON corrupt — fail closed if session file exists
-  printf 'BLOCKED: Cannot parse session state (python3 required). Remove %s to clear.\n' "$SESSION_FILE" >&2
+  printf 'BLOCKED: Cannot parse session state (python3 required or JSON corrupt). Remove %s to clear.\n' "$SESSION_FILE" >&2
   exit 2
 }
 
@@ -43,15 +61,25 @@ fi
 INPUT=$(cat)
 TOOL_NAME=$(printf '%s' "$INPUT" | python3 -c "import json,sys; print(json.load(sys.stdin).get('tool_name',''))" 2>/dev/null || echo "")
 
-# Command steps: block all standard tools — only MCP tools (devkit_advance) allowed
+# Command steps: allow ONLY the MCP tools needed to progress the
+# workflow. Everything else is blocked, including future tools the
+# hook author hasn't heard of.
 if [[ "$STEP_TYPE" == "command" && "$ENFORCE" == "hard" ]]; then
   case "$TOOL_NAME" in
-    Bash|Edit|Write|Read|Glob|Grep|Agent|WebFetch|WebSearch|NotebookEdit|Skill)
-      printf 'BLOCKED: Command step "%s" in progress. Call devkit_advance to execute it and proceed.\n' "$CURRENT_STEP" >&2
+    # MCP devkit tools — Claude uses these to drive the engine.
+    mcp__*devkit*|devkit_advance|devkit_status|devkit_list|devkit_start)
+      exit 0
+      ;;
+    # TodoWrite is a pure in-memory tracker with no side effects, allowed.
+    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' "$CURRENT_STEP" "$TOOL_NAME" >&2
       exit 2
       ;;
   esac
 fi
 
-# All other cases: allow
+# Prompt/parallel steps: allow everything (Claude needs full tool access).
 exit 0
diff --git a/hooks/devkit-stop-guard.sh b/hooks/devkit-stop-guard.sh
index 1368822..c632bfc 100755
--- a/hooks/devkit-stop-guard.sh
+++ b/hooks/devkit-stop-guard.sh
@@ -3,9 +3,15 @@ set -euo pipefail
 
 # devkit-stop-guard: Stop hook that blocks session end during active workflows.
 # Outputs JSON: {"decision":"approve"} or {"decision":"block","reason":"..."}.
+#
+# Policy: symmetric with devkit-guard.sh. If the session file exists but
+# cannot be parsed, we fail CLOSED (block with a clear reason) rather
+# than silently approve — a corrupted state file during an active
+# workflow is a bug the user needs to see, not something to paper over.
 
 DATA_DIR="${CLAUDE_PLUGIN_DATA:-}"
 if [[ -z "$DATA_DIR" ]]; then
+  printf 'devkit-stop-guard: CLAUDE_PLUGIN_DATA unset — enforcement disabled\n' >&2
   printf '{"decision":"approve"}'
   exit 0
 fi
@@ -17,10 +23,16 @@ if [[ ! -f "$SESSION_FILE" ]]; then
 fi
 
 # Parse all fields in a single python3 call. Passes path via sys.argv
-# to prevent shell injection. Outputs valid JSON directly.
-python3 -c "
+# to prevent shell injection. Outputs valid JSON directly. Handles the
+# TOCTOU race (file cleared between -f test and open) as "no session"
+# so we don't spuriously block a completed workflow.
+PARSED=$(python3 -c "
 import json, sys
-d = json.load(open(sys.argv[1]))
+try:
+    d = json.load(open(sys.argv[1]))
+except FileNotFoundError:
+    print(json.dumps({'decision': 'approve'}))
+    sys.exit(0)
 if d.get('status') == 'running':
     remaining = d.get('total_steps', 0) - d.get('current_index', 0)
     wf = d.get('workflow', 'unknown')
@@ -30,9 +42,13 @@ if d.get('status') == 'running':
     }))
 else:
     print(json.dumps({'decision': 'approve'}))
-" "$SESSION_FILE" 2>/dev/null || {
-  # Cannot parse — approve to avoid trapping the user
-  printf '{"decision":"approve"}'
+" "$SESSION_FILE" 2>/dev/null) || {
+  # Cannot parse — fail closed with diagnostic. User must either
+  # complete the workflow or remove the stale file manually.
+  printf 'devkit-stop-guard: cannot parse session state (python3 missing or JSON corrupt); blocking Stop\n' >&2
+  printf '{"decision":"block","reason":"devkit session state corrupted — remove %s to clear"}' "$SESSION_FILE"
+  exit 0
 }
 
+printf '%s' "$PARSED"
 exit 0
diff --git a/hooks/hooks.json b/hooks/hooks.json
index c423aa4..82be6c0 100644
--- a/hooks/hooks.json
+++ b/hooks/hooks.json
@@ -1,5 +1,5 @@
 {
-  "description": "Consolidated enforcement stack: PreToolUse safety + security, PostToolUse validation + language review, SubagentStop verification, Stop quality gate",
+  "description": "Consolidated enforcement stack: PreToolUse safety + security + workflow guard, PostToolUse validation + language review, SubagentStop verification, Stop quality gate + workflow completion guard",
   "hooks": {
     "PreToolUse": [
       {
diff --git a/hooks/pr-gate.sh b/hooks/pr-gate.sh
index ff79a0f..ee3987d 100755
--- a/hooks/pr-gate.sh
+++ b/hooks/pr-gate.sh
@@ -1,4 +1,5 @@
-#!/bin/bash
+#!/usr/bin/env bash
+set -euo pipefail
 # devkit PR gate hook — prompts to run pr-ready pipeline before creating a PR
 # Runs on PreToolUse for Bash tool
 #
@@ -6,27 +7,29 @@
 # the full pr-ready pipeline first.
 
 INPUT=$(cat)
-COMMAND=$(echo "$INPUT" | jq -r '.tool_input.command // empty')
+COMMAND=$(printf '%s' "$INPUT" | jq -r '.tool_input.command // empty')
 
 # Only trigger on gh pr create
-echo "$COMMAND" | grep -qE 'gh\s+pr\s+create' || exit 0
+printf '%s' "$COMMAND" | grep -qE 'gh[[:space:]]+pr[[:space:]]+create' || exit 0
 
-# Check if pr-ready already ran this session (cooldown file)
-PR_GATE_FILE="/tmp/devkit-pr-gate-done"
-if [ -f "$PR_GATE_FILE" ]; then
-  LAST=$(cat "$PR_GATE_FILE" 2>/dev/null)
-  NOW=$(date +%s 2>/dev/null)
-  if [ -n "$LAST" ] && [ -n "$NOW" ]; then
-    ELAPSED=$(( NOW - LAST )) 2>/dev/null || ELAPSED=0
-    if [ "$ELAPSED" -lt 600 ] 2>/dev/null; then
-      # Pipeline already ran recently, allow the PR creation
-      exit 0
-    fi
+# Check if pr-ready already ran recently (cooldown file). Per-user
+# rather than /tmp-global so concurrent projects don't share state.
+COOLDOWN_DIR="${XDG_CACHE_HOME:-$HOME/.cache}/devkit"
+PR_GATE_FILE="${COOLDOWN_DIR}/pr-gate-done"
+mkdir -p "$COOLDOWN_DIR" 2>/dev/null || true
+
+if [[ -f "$PR_GATE_FILE" ]]; then
+  LAST=$(cat "$PR_GATE_FILE" 2>/dev/null || printf '0')
+  NOW=$(date +%s)
+  # Only arithmetic-compare if both values are numeric, else treat as expired.
+  if [[ "$LAST" =~ ^[0-9]+$ ]] && (( NOW - LAST < 600 )); then
+    # Pipeline already ran recently; skip the prompt.
+    exit 0
   fi
 fi
 
-# Set cooldown so this only fires once
-date +%s > "$PR_GATE_FILE" 2>/dev/null
+# Set cooldown so this only fires once per 10 minutes.
+date +%s > "$PR_GATE_FILE" 2>/dev/null || true
 
 # Ask the user
 jq -n '{
diff --git a/src/cmd/status.go b/src/cmd/status.go
index e870600..5bb39c8 100644
--- a/src/cmd/status.go
+++ b/src/cmd/status.go
@@ -31,7 +31,7 @@ func showAllSessions() error {
 		return fmt.Errorf("list sessions: %w", err)
 	}
 	if len(sessions) == 0 {
-		fmt.Println("No sessions found. Run `devkit workflow run <name>` to start one, or use the MCP tools inside Claude Code.")
+		fmt.Println("No sessions found. Run `devkit workflow <name> \"<description>\"` to start one, or use the MCP tools inside Claude Code.")
 		return nil
 	}
 
diff --git a/src/engine/engine.go b/src/engine/engine.go
index 56e6231..a7afed0 100644
--- a/src/engine/engine.go
+++ b/src/engine/engine.go
@@ -251,9 +251,14 @@ func (e *Engine) RunWorkflow(ctx context.Context, wf *Workflow, cfg RunConfig) (
 }
 
 // runCommand executes a shell command and returns its combined output.
-func (e *Engine) runCommand(ctx context.Context, command string) (string, int, error) {
+// The caller passes input and outputs through env vars (DEVKIT_INPUT and
+// DEVKIT_OUT_<id>) rather than interpolating them into the command string,
+// to eliminate shell injection via LLM-chosen input or contaminated
+// prior-step output.
+func (e *Engine) runCommand(ctx context.Context, command, input string, outputs map[string]string) (string, int, error) {
 	cmd := exec.CommandContext(ctx, "sh", "-c", command)
 	cmd.Dir = e.repoRoot
+	cmd.Env = append(os.Environ(), buildCommandEnv(input, outputs)...)
 
 	var out bytes.Buffer
 	cmd.Stdout = &out
@@ -266,19 +271,45 @@ func (e *Engine) runCommand(ctx context.Context, command string) (string, int, e
 		if errors.As(err, &exitErr) {
 			exitCode = exitErr.ExitCode()
 		} else {
-			return "", 1, fmt.Errorf("command execution failed: %w", err)
+			return out.String(), 1, fmt.Errorf("command execution failed: %w", err)
 		}
 	}
 	return out.String(), exitCode, nil
 }
 
+// buildCommandEnv returns DEVKIT_INPUT and DEVKIT_OUT_<step_id> env vars
+// for use by command/gate steps.
+func buildCommandEnv(input string, outputs map[string]string) []string {
+	env := []string{"DEVKIT_INPUT=" + input}
+	for id, out := range outputs {
+		env = append(env, "DEVKIT_OUT_"+envKey(id)+"="+out)
+	}
+	return env
+}
+
+// envKey maps a step ID (may contain hyphens) to a POSIX env var suffix.
+func envKey(id string) string {
+	b := make([]byte, 0, len(id))
+	for i := 0; i < len(id); i++ {
+		c := id[i]
+		switch {
+		case c >= 'a' && c <= 'z':
+			b = append(b, c-32)
+		case c >= 'A' && c <= 'Z', c >= '0' && c <= '9', c == '_':
+			b = append(b, c)
+		default:
+			b = append(b, '_')
+		}
+	}
+	return string(b)
+}
+
 // runStep executes a single step and records it in the database.
 func (e *Engine) runStep(ctx context.Context, step *WfStep, session *lib.Session, input string, outputs map[string]string, opts runners.RunOpts, iterNum *int) (float64, string, error) {
 	*iterNum++
 
 	// Command step: run shell command directly, no LLM cost.
 	if step.Command != "" {
-		command := Interpolate(step.Command, input, outputs)
 		fmt.Printf("--- %s (step %d, command) ---\n", step.ID, *iterNum)
 
 		dbStep := &lib.Step{
@@ -289,7 +320,9 @@ func (e *Engine) runStep(ctx context.Context, step *WfStep, session *lib.Session
 		}
 		e.db.CreateStep(dbStep)
 
-		output, exitCode, err := e.runCommand(ctx, command)
+		// Command string is literal — no {{...}} expansion. Values
+		// come via env vars ($DEVKIT_INPUT, $DEVKIT_OUT_<step_id>).
+		output, exitCode, err := e.runCommand(ctx, step.Command, input, outputs)
 		if err != nil {
 			dbStep.Status = "failed"
 			dbStep.ChangeSummary = err.Error()
@@ -405,10 +438,10 @@ func (e *Engine) runLoop(ctx context.Context, step *WfStep, session *lib.Session
 		iterCost := result.CostUSD
 
 		// Gate check: run shell command, revert if non-zero exit.
+		// Gate string is literal — values come via env vars.
 		if step.Loop.Gate != "" {
-			gateCmd := Interpolate(step.Loop.Gate, input, outputs)
-			fmt.Printf("  gate: %s\n", runners.TruncStr(gateCmd, 80))
-			_, exitCode, gateErr := e.runCommand(ctx, gateCmd)
+			fmt.Printf("  gate: %s\n", runners.TruncStr(step.Loop.Gate, 80))
+			_, exitCode, gateErr := e.runCommand(ctx, step.Loop.Gate, input, outputs)
 
 			// Distinguish "gate couldn't execute" from "gate ran and returned non-zero".
 			// A startup/context error is fatal — the gate never validated anything.
@@ -474,8 +507,10 @@ func (e *Engine) runLoop(ctx context.Context, step *WfStep, session *lib.Session
 			fmt.Printf("  → commit failed: %s\n", commitErr)
 		}
 
-		// Check until condition
-		if step.Loop.Until != "" && strings.Contains(strings.ToUpper(result.Output), strings.ToUpper(step.Loop.Until)) {
+		// Check until condition. Line-anchored (see MatchUntil) —
+		// sentinel must appear on its own line to avoid matching
+		// conversational text.
+		if step.Loop.Until != "" && MatchUntil(result.Output, step.Loop.Until) {
 			fmt.Printf("  → loop complete (%s found)\n\n", step.Loop.Until)
 			return totalCost, nil
 		}
diff --git a/src/engine/engine_test.go b/src/engine/engine_test.go
index 1502a41..513d2d2 100644
--- a/src/engine/engine_test.go
+++ b/src/engine/engine_test.go
@@ -845,7 +845,9 @@ func TestRunWorkflowCommandStep(t *testing.T) {
 	}
 }
 
-func TestRunWorkflowCommandInterpolation(t *testing.T) {
+func TestRunWorkflowCommandEnvInput(t *testing.T) {
+	// Regression: command steps must NOT interpolate {{input}} — values
+	// come via $DEVKIT_INPUT to prevent shell injection.
 	db := tempDB(t)
 	dir, git := initGitRepo(t)
 	runner := newMockRunner(nil, nil)
@@ -854,7 +856,7 @@ func TestRunWorkflowCommandInterpolation(t *testing.T) {
 	wf := &Workflow{
 		Name: "test",
 		Steps: []WfStep{
-			{ID: "greet", Command: "echo {{input}}"},
+			{ID: "greet", Command: `printf '%s' "$DEVKIT_INPUT"`},
 		},
 	}
 
@@ -863,7 +865,58 @@ func TestRunWorkflowCommandInterpolation(t *testing.T) {
 		t.Fatalf("RunWorkflow: %v", err)
 	}
 	if !strings.Contains(res.Outputs["greet"], "howdy") {
-		t.Errorf("input not interpolated in command output: %q", res.Outputs["greet"])
+		t.Errorf("input not passed via env: %q", res.Outputs["greet"])
+	}
+}
+
+func TestRunWorkflowCommandRejectsInterpolation(t *testing.T) {
+	// Validation must reject {{...}} in command strings (shell injection
+	// mitigation). Authors must use $DEVKIT_INPUT / $DEVKIT_OUT_<id>.
+	db := tempDB(t)
+	dir, git := initGitRepo(t)
+	runner := newMockRunner(nil, nil)
+	eng := mustEngine(t, db, git, runner, dir)
+
+	wf := &Workflow{
+		Name: "test",
+		Steps: []WfStep{
+			{ID: "greet", Command: "echo {{input}}"},
+		},
+	}
+
+	_, err := eng.RunWorkflow(context.Background(), wf, RunConfig{Input: "howdy"})
+	if err == nil {
+		t.Fatal("expected validation error for {{...}} in command string, got nil")
+	}
+	if !strings.Contains(err.Error(), "shell injection mitigation") {
+		t.Errorf("error = %q, want it to mention shell injection mitigation", err.Error())
+	}
+}
+
+func TestRunWorkflowCommandEnvPriorOutput(t *testing.T) {
+	// $DEVKIT_OUT_<step_id> exposes prior step outputs to later command
+	// steps without interpolation.
+	db := tempDB(t)
+	dir, git := initGitRepo(t)
+	runner := newMockRunner([]runners.RunResult{
+		result("review-body: approved"),
+	}, nil)
+	eng := mustEngine(t, db, git, runner, dir)
+
+	wf := &Workflow{
+		Name: "test",
+		Steps: []WfStep{
+			{ID: "review", Prompt: "review the code"},
+			{ID: "publish", Command: `printf '%s' "$DEVKIT_OUT_REVIEW"`},
+		},
+	}
+
+	res, err := eng.RunWorkflow(context.Background(), wf, RunConfig{Input: "x"})
+	if err != nil {
+		t.Fatalf("RunWorkflow: %v", err)
+	}
+	if !strings.Contains(res.Outputs["publish"], "approved") {
+		t.Errorf("prior step output not passed via env: %q", res.Outputs["publish"])
 	}
 }
 
@@ -1192,10 +1245,11 @@ func TestRunWorkflowLoopGateRecovery(t *testing.T) {
 	db := tempDB(t)
 	dir, git := initGitRepo(t)
 
-	// First attempt fails gate, second passes and hits until
+	// First attempt fails gate, second passes gate and its output has
+	// ALL_DONE on its own line so the line-anchored until matches.
 	runner := newMockRunner([]runners.RunResult{
 		result("attempt 1"),
-		result("attempt 2 ALL_DONE"),
+		result("attempt 2\nALL_DONE"),
 	}, nil)
 	eng := mustEngine(t, db, git, runner, dir)
 
diff --git a/src/engine/workflow.go b/src/engine/workflow.go
index 4a55a58..2f56dc3 100644
--- a/src/engine/workflow.go
+++ b/src/engine/workflow.go
@@ -4,6 +4,7 @@
 package engine
 
 import (
+	"bytes"
 	"fmt"
 	"os"
 	"strings"
@@ -63,10 +64,14 @@ func ParseFile(path string) (*Workflow, error) {
 	return Parse(data)
 }
 
-// Parse parses workflow YAML bytes.
+// Parse parses workflow YAML bytes with strict field checking so typos
+// like "commnd:" fail loudly instead of silently producing a step that
+// never runs the intended command.
 func Parse(data []byte) (*Workflow, error) {
 	var wf Workflow
-	if err := yaml.Unmarshal(data, &wf); err != nil {
+	dec := yaml.NewDecoder(bytes.NewReader(data))
+	dec.KnownFields(true)
+	if err := dec.Decode(&wf); err != nil {
 		return nil, fmt.Errorf("parse yaml: %w", err)
 	}
 	if err := validate(&wf); err != nil {
@@ -127,6 +132,17 @@ func validate(wf *Workflow) error {
 		if len(s.Parallel) > 0 && s.Loop != nil {
 			return fmt.Errorf("step %q has both parallel and loop — these are mutually exclusive", s.ID)
 		}
+		// Command strings are never interpolated — values are passed
+		// through env vars ($DEVKIT_INPUT, $DEVKIT_OUT_<id>) to avoid
+		// shell injection. Reject {{...}} in command/gate strings so
+		// the author gets a clear error instead of a silently broken
+		// step or, worse, a shell injection.
+		if s.Command != "" && strings.Contains(s.Command, "{{") {
+			return fmt.Errorf("step %q command must not use {{...}} — pass values via $DEVKIT_INPUT or $DEVKIT_OUT_<step_id> instead (shell injection mitigation)", s.ID)
+		}
+		if s.Loop != nil && s.Loop.Gate != "" && strings.Contains(s.Loop.Gate, "{{") {
+			return fmt.Errorf("step %q loop.gate must not use {{...}} — pass values via $DEVKIT_INPUT or $DEVKIT_OUT_<step_id> instead (shell injection mitigation)", s.ID)
+		}
 	}
 
 	// Validate branch targets exist
@@ -164,12 +180,73 @@ func Interpolate(prompt string, input string, outputs map[string]string) string
 
 // EvalBranch checks step output against branch conditions.
 // Returns the goto target step ID, or "" if no match.
+//
+// Matching is word-boundary (case-insensitive): the sentinel must
+// appear as a whole word in the output, bounded on both sides by a
+// non-alphanumeric character or string edge. This is the same
+// semantics as grep -w. It accepts idiomatic patterns like "TINY: short
+// fix" and "attempt 2: ALL_PASSING" while rejecting accidental
+// substrings — `fail` won't match inside `failures`, and `small` won't
+// match inside `smaller`.
+//
+// Note: workflow authors should still pick distinctive sentinels.
+// `until: done` will match any sentence containing the standalone word
+// "done" (e.g. a prose reply "I'm done reviewing"). Prefer sentinels
+// like `ALL_DONE`, `DONE_FIXING`, or `===DONE===` for robustness.
 func EvalBranch(output string, branches []Branch) string {
-	lower := strings.ToLower(output)
 	for _, b := range branches {
-		if strings.Contains(lower, strings.ToLower(b.When)) {
+		want := strings.ToLower(strings.TrimSpace(b.When))
+		if want == "" {
+			continue
+		}
+		if containsWord(output, want) {
 			return b.Goto
 		}
 	}
 	return ""
 }
+
+// MatchUntil checks whether a step's output satisfies its loop `until`
+// sentinel. Same word-boundary semantics as EvalBranch.
+func MatchUntil(output, sentinel string) bool {
+	want := strings.ToLower(strings.TrimSpace(sentinel))
+	if want == "" {
+		return false
+	}
+	return containsWord(output, want)
+}
+
+// containsWord returns true when `want` (already lowercased and
+// trimmed) appears in `text` as a whole word — bounded on both sides
+// by a non-alphanumeric character, underscore, or string edge. Matches
+// grep -w semantics.
+func containsWord(text, want string) bool {
+	lower := strings.ToLower(text)
+	for i := 0; ; {
+		idx := strings.Index(lower[i:], want)
+		if idx < 0 {
+			return false
+		}
+		start := i + idx
+		end := start + len(want)
+		if isWordBoundary(lower, start, end) {
+			return true
+		}
+		i = start + 1
+		if i >= len(lower) {
+			return false
+		}
+	}
+}
+
+// isWordBoundary returns true when the characters just outside [start,
+// end) in s are not alphanumeric/underscore (or the edge of the string).
+func isWordBoundary(s string, start, end int) bool {
+	leftOK := start == 0 || !isWordChar(s[start-1])
+	rightOK := end == len(s) || !isWordChar(s[end])
+	return leftOK && rightOK
+}
+
+func isWordChar(c byte) bool {
+	return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_'
+}
diff --git a/src/mcp/tools.go b/src/mcp/tools.go
index f5498e0..d45f397 100644
--- a/src/mcp/tools.go
+++ b/src/mcp/tools.go
@@ -201,9 +201,13 @@ func (s *Server) formatStepResponse(wf *engine.Workflow, state *lib.SessionState
 	fmt.Fprintf(&b, "=== STEP %d/%d: %s ===\n", state.CurrentIndex+1, state.TotalSteps, step.ID)
 
 	if step.Command != "" {
-		cmd := engine.Interpolate(step.Command, input, state.Outputs)
 		fmt.Fprintf(&b, "TYPE: command (engine will execute automatically on devkit_advance)\n")
-		fmt.Fprintf(&b, "COMMAND: %s\n", cmd)
+		fmt.Fprintf(&b, "COMMAND: %s\n", step.Command)
+		fmt.Fprintf(&b, "ENV: DEVKIT_INPUT=%q", input)
+		for id, out := range state.Outputs {
+			fmt.Fprintf(&b, ", DEVKIT_OUT_%s=<%d bytes>", sanitizeEnvKey(id), len(out))
+		}
+		fmt.Fprintln(&b)
 		if step.Expect != "" {
 			fmt.Fprintf(&b, "EXPECT: %s\n", step.Expect)
 		}
@@ -291,10 +295,13 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 
 		currentStep := wf.Steps[state.CurrentIndex]
 
-		// Handle command steps — engine executes them
+		// Handle command steps — engine executes them. Command strings
+		// are run literally (no {{...}} expansion); values are passed
+		// through env vars DEVKIT_INPUT and DEVKIT_OUT_<step_id> to
+		// avoid shell injection via LLM-chosen input or contaminated
+		// prior-step output.
 		if currentStep.Command != "" {
-			cmd := engine.Interpolate(currentStep.Command, state.Input, state.Outputs)
-			output, exitCode, cmdErr := s.runCommand(ctx, cmd)
+			output, exitCode, cmdErr := s.runCommand(ctx, currentStep.Command, state)
 			if cmdErr != nil {
 				return mcpmcp.NewToolResultError(fmt.Sprintf("command failed: %v", cmdErr)), nil
 			}
@@ -360,33 +367,58 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 }
 
 // completeWorkflow marks a session as done, updates DB, and clears hot state.
+// Any warnings (DB update failure, commit failure, state clear failure)
+// are collected and surfaced in the user-visible response so silent
+// post-completion failures are observable.
 func (s *Server) completeWorkflow(state *lib.SessionState) (*mcpmcp.CallToolResult, error) {
 	state.Status = "done"
 	if err := lib.WriteSessionJSON(s.dataDir, state); err != nil {
 		return mcpmcp.NewToolResultError(fmt.Sprintf("write final state: %v", err)), nil
 	}
+
+	var warnings []string
 	if s.db != nil {
 		if err := s.db.UpdateSessionStatus(state.ID, "done"); err != nil {
-			fmt.Fprintf(os.Stderr, "warning: db update session: %v\n", err)
+			warnings = append(warnings, fmt.Sprintf("db session status update failed: %v", err))
 		}
 	}
 
 	if state.Branch && s.git != nil {
-		s.git.CommitAll(fmt.Sprintf("%s(%s): complete", state.Workflow, state.ID))
+		if err := s.git.CommitAll(fmt.Sprintf("%s(%s): complete", state.Workflow, state.ID)); err != nil {
+			// Don't swallow — the user's branch work may not be
+			// persisted. Report prominently.
+			warnings = append(warnings, fmt.Sprintf("final git commit failed: %v (your working tree may have uncommitted changes)", err))
+		}
 	}
 
 	if err := lib.ClearSessionJSON(s.dataDir); err != nil {
-		fmt.Fprintf(os.Stderr, "warning: clear session: %v\n", err)
+		warnings = append(warnings, fmt.Sprintf("clear session state failed: %v (the hot state file at %s may be stale)", err, s.dataDir))
+	}
+
+	var b strings.Builder
+	fmt.Fprintf(&b, "=== WORKFLOW COMPLETE ===\nSession: %s\nSteps completed: %d", state.ID, state.TotalSteps)
+	if len(warnings) > 0 {
+		fmt.Fprintf(&b, "\n\n=== WARNINGS (non-fatal) ===")
+		for _, w := range warnings {
+			fmt.Fprintf(&b, "\n- %s", w)
+			fmt.Fprintf(os.Stderr, "devkit completeWorkflow: %s\n", w)
+		}
 	}
-	return mcpmcp.NewToolResultText(fmt.Sprintf("=== WORKFLOW COMPLETE ===\nSession: %s\nSteps completed: %d", state.ID, state.TotalSteps)), nil
+	return mcpmcp.NewToolResultText(b.String()), nil
 }
 
-func (s *Server) runCommand(ctx context.Context, command string) (string, int, error) {
+// runCommand executes a workflow command string under sh -c, passing the
+// session's Input and prior step Outputs as environment variables rather
+// than interpolating them into the shell string. This eliminates shell
+// injection via LLM-chosen input or contaminated prior-step output — the
+// command text is always the literal YAML value.
+func (s *Server) runCommand(ctx context.Context, command string, state *lib.SessionState) (string, int, error) {
 	ctx, cancel := context.WithTimeout(ctx, commandTimeout)
 	defer cancel()
 
 	cmd := exec.CommandContext(ctx, "sh", "-c", command)
 	cmd.Dir = s.repoRoot
+	cmd.Env = append(os.Environ(), commandEnv(state)...)
 	var out bytes.Buffer
 	cmd.Stdout = &out
 	cmd.Stderr = &out
@@ -397,12 +429,47 @@ func (s *Server) runCommand(ctx context.Context, command string) (string, int, e
 		if errors.As(err, &exitErr) {
 			exitCode = exitErr.ExitCode()
 		} else {
-			return "", 1, fmt.Errorf("command execution failed: %w", err)
+			// Surface whatever the command produced on its combined
+			// stream so the user can see the real cause (missing
+			// binary, permission denied, timeout), not just the Go
+			// error wrapper.
+			if ctx.Err() == context.DeadlineExceeded {
+				return out.String(), 124, fmt.Errorf("command timed out after %s: %w", commandTimeout, err)
+			}
+			return out.String(), 1, fmt.Errorf("command execution failed: %w", err)
 		}
 	}
 	return out.String(), exitCode, nil
 }
 
+// commandEnv returns the DEVKIT_INPUT and DEVKIT_OUT_<id> env vars that
+// command steps can read via $DEVKIT_INPUT / $DEVKIT_OUT_<id>.
+func commandEnv(state *lib.SessionState) []string {
+	env := []string{"DEVKIT_INPUT=" + state.Input}
+	for id, out := range state.Outputs {
+		env = append(env, "DEVKIT_OUT_"+sanitizeEnvKey(id)+"="+out)
+	}
+	return env
+}
+
+// sanitizeEnvKey maps a workflow step ID to a valid env var suffix.
+// POSIX allows [A-Za-z_][A-Za-z0-9_]*; step IDs may contain hyphens.
+func sanitizeEnvKey(id string) string {
+	b := make([]byte, 0, len(id))
+	for i := 0; i < len(id); i++ {
+		c := id[i]
+		switch {
+		case c >= 'a' && c <= 'z':
+			b = append(b, c-32) // upper
+		case c >= 'A' && c <= 'Z', c >= '0' && c <= '9', c == '_':
+			b = append(b, c)
+		default:
+			b = append(b, '_')
+		}
+	}
+	return string(b)
+}
+
 func (s *Server) handleLoopAdvance(ctx context.Context, wf *engine.Workflow, state *lib.SessionState, step *engine.WfStep, req mcpmcp.CallToolRequest) (*mcpmcp.CallToolResult, error) {
 	// Initialize loop tracking on first call
 	if state.LoopMax == 0 {
@@ -413,12 +480,12 @@ func (s *Server) handleLoopAdvance(ctx context.Context, wf *engine.Workflow, sta
 	}
 	state.LoopIteration++
 
-	// Check gate command if present
+	// Check gate command if present. Gate strings are also literal —
+	// values come via env vars DEVKIT_INPUT / DEVKIT_OUT_<step_id>.
 	if step.Loop.Gate != "" {
-		gateCmd := engine.Interpolate(step.Loop.Gate, state.Input, state.Outputs)
-		_, exitCode, err := s.runCommand(ctx, gateCmd)
+		gateOut, exitCode, err := s.runCommand(ctx, step.Loop.Gate, state)
 		if err != nil {
-			return mcpmcp.NewToolResultError(fmt.Sprintf("gate command failed: %v", err)), nil
+			return mcpmcp.NewToolResultError(fmt.Sprintf("gate command failed: %v\n%s", err, gateOut)), nil
 		}
 		if exitCode == 0 {
 			// Gate passed — advance past loop
@@ -427,10 +494,11 @@ func (s *Server) handleLoopAdvance(ctx context.Context, wf *engine.Workflow, sta
 		// Gate failed — continue loop
 	}
 
-	// Check "until" condition
+	// Check "until" condition. Line-anchored match (see engine.MatchUntil)
+	// so sentinels like "DONE" do not match prose mentions.
 	if step.Loop.Until != "" {
 		if output, ok := state.Outputs[step.ID]; ok {
-			if strings.Contains(strings.ToLower(output), strings.ToLower(step.Loop.Until)) {
+			if engine.MatchUntil(output, step.Loop.Until) {
 				return s.advancePastLoop(wf, state)
 			}
 		}
diff --git a/src/mcp/tools_test.go b/src/mcp/tools_test.go
index faa2ff0..63a4b2f 100644
--- a/src/mcp/tools_test.go
+++ b/src/mcp/tools_test.go
@@ -934,11 +934,12 @@ steps:
 		t.Errorf("expected loop iteration, got:\n%s", tc.Text)
 	}
 
-	// Second advance — output contains "all clear", should exit loop
+	// Second advance — output has "all clear" as its own trimmed line,
+	// which matches the line-anchored until sentinel.
 	advReq2 := mcpmcp.CallToolRequest{}
 	advReq2.Params.Arguments = map[string]interface{}{
 		"session": state.ID,
-		"output":  "All Clear now",
+		"output":  "fixed the issue\nall clear\n",
 	}
 	result2, _ := advHandler(context.Background(), advReq2)
 	tc2, _ := result2.Content[0].(mcpmcp.TextContent)
@@ -947,6 +948,45 @@ steps:
 	}
 }
 
+func TestLoopUntilRejectsSubstring(t *testing.T) {
+	// Regression: an until sentinel must NOT match when it appears only
+	// inside another word. Prior behavior used strings.Contains which
+	// made "fail" match "no failures found". Word-boundary match now.
+	wfDir := t.TempDir()
+	dataDir := t.TempDir()
+
+	writeFile(t, filepath.Join(wfDir, "test.yml"), `name: test
+steps:
+  - id: fix
+    prompt: "Fix the issue"
+    loop:
+      max: 3
+      until: "FAIL"
+  - id: end
+    prompt: end
+`)
+
+	srv := newTestServer(t, dataDir, wfDir)
+
+	_, startHandler := srv.startTool()
+	startReq := mcpmcp.CallToolRequest{}
+	startReq.Params.Arguments = map[string]interface{}{"workflow": "test", "input": "x"}
+	startHandler(context.Background(), startReq)
+	state, _ := lib.ReadSessionJSON(dataDir)
+
+	_, advHandler := srv.advanceTool()
+	advReq := mcpmcp.CallToolRequest{}
+	advReq.Params.Arguments = map[string]interface{}{
+		"session": state.ID,
+		"output":  "no failures found; classification succeeded",
+	}
+	result, _ := advHandler(context.Background(), advReq)
+	tc, _ := result.Content[0].(mcpmcp.TextContent)
+	if !strings.Contains(tc.Text, "LOOP ITERATION") {
+		t.Errorf("expected to remain in loop (FAIL must not match inside 'failures'), got:\n%s", tc.Text)
+	}
+}
+
 // writeFile is a test helper that creates a file with the given content.
 func writeFile(t *testing.T, path, content string) {
 	t.Helper()
diff --git a/workflows/pr-ready.yml b/workflows/pr-ready.yml
index b9cb517..e994a60 100644
--- a/workflows/pr-ready.yml
+++ b/workflows/pr-ready.yml
@@ -1,5 +1,5 @@
 name: PR Ready
-description: Full PR preparation pipeline — lint, test, security, changelog, create PR
+description: Full PR preparation pipeline — validate, drop unrelated changes, lint, test, security, changelog, create PR, monitor CI and reviews until resolved
 
 steps:
   - id: validate
diff --git a/workflows/self-improve.yml b/workflows/self-improve.yml
index 79e8643..6e5817d 100644
--- a/workflows/self-improve.yml
+++ b/workflows/self-improve.yml
@@ -3,7 +3,7 @@ description: Metric-gated improvement loop — run command, fix issues, repeat u
 
 steps:
   - id: baseline
-    command: "{{input}} 2>&1 || true"
+    command: '$DEVKIT_INPUT 2>&1 || true'
 
   - id: improve
     model: smart
@@ -20,10 +20,10 @@ steps:
     loop:
       max: 10
       until: DONE
-      gate: "{{input}}"
+      gate: '$DEVKIT_INPUT'
 
   - id: verify
-    command: "{{input}} 2>&1 || true"
+    command: '$DEVKIT_INPUT 2>&1 || true'
 
   - id: summary
     model: fast
diff --git a/workflows/self-lint.yml b/workflows/self-lint.yml
index 557fd23..d9ef97c 100644
--- a/workflows/self-lint.yml
+++ b/workflows/self-lint.yml
@@ -3,7 +3,7 @@ description: Run linter, fix violations deterministically, repeat until clean
 
 steps:
   - id: baseline
-    command: "{{input}} 2>&1 || true"
+    command: '$DEVKIT_INPUT 2>&1 || true'
 
   - id: fix
     model: smart
@@ -21,10 +21,10 @@ steps:
     loop:
       max: 20
       until: DONE
-      gate: "{{input}}"
+      gate: '$DEVKIT_INPUT'
 
   - id: verify
-    command: "{{input}} 2>&1 || true"
+    command: '$DEVKIT_INPUT 2>&1 || true'
 
   - id: summary
     model: fast
diff --git a/workflows/self-migrate.yml b/workflows/self-migrate.yml
index d584090..679fecc 100644
--- a/workflows/self-migrate.yml
+++ b/workflows/self-migrate.yml
@@ -3,7 +3,7 @@ description: Incremental migration loop — migrate code one piece at a time wit
 
 steps:
   - id: baseline
-    command: "{{input}} 2>&1 || true"
+    command: '$DEVKIT_INPUT 2>&1 || true'
 
   - id: migrate
     model: smart
@@ -21,10 +21,10 @@ steps:
     loop:
       max: 20
       until: DONE
-      gate: "{{input}}"
+      gate: '$DEVKIT_INPUT'
 
   - id: verify
-    command: "{{input}} 2>&1 || true"
+    command: '$DEVKIT_INPUT 2>&1 || true'
 
   - id: summary
     model: fast
diff --git a/workflows/self-perf.yml b/workflows/self-perf.yml
index df7096b..be3a765 100644
--- a/workflows/self-perf.yml
+++ b/workflows/self-perf.yml
@@ -3,7 +3,7 @@ description: Profile performance, optimize hot paths deterministically, verify i
 
 steps:
   - id: baseline
-    command: "{{input}} 2>&1 || true"
+    command: '$DEVKIT_INPUT 2>&1 || true'
 
   - id: optimize
     model: smart
@@ -20,10 +20,10 @@ steps:
     loop:
       max: 5
       until: DONE
-      gate: "{{input}}"
+      gate: '$DEVKIT_INPUT'
 
   - id: verify
-    command: "{{input}} 2>&1 || true"
+    command: '$DEVKIT_INPUT 2>&1 || true'
 
   - id: summary
     model: fast
diff --git a/workflows/self-test.yml b/workflows/self-test.yml
index ecf94ad..76f23af 100644
--- a/workflows/self-test.yml
+++ b/workflows/self-test.yml
@@ -2,8 +2,11 @@ name: Self-Test
 description: Run tests, fix failures deterministically, repeat until all pass
 
 steps:
+  # $DEVKIT_INPUT is the shell command provided by the caller (e.g.
+  # "npm test"). The outer sh expands it, running it as-is. Inherently
+  # trusts whatever the caller passes — document in the workflow README.
   - id: baseline
-    command: "{{input}} 2>&1 || true"
+    command: '$DEVKIT_INPUT 2>&1 || true'
 
   - id: fix
     model: smart
@@ -20,10 +23,10 @@ steps:
     loop:
       max: 8
       until: DONE
-      gate: "{{input}}"
+      gate: '$DEVKIT_INPUT'
 
   - id: verify
-    command: "{{input}} 2>&1 || true"
+    command: '$DEVKIT_INPUT 2>&1 || true'
 
   - id: summary
     model: fast

From 2c13ea66631264be09f1aae00b5ac3059d22e339 Mon Sep 17 00:00:00 2001
From: Tym Rabchuk <tymrabchuk@gmail.com>
Date: Fri, 10 Apr 2026 01:53:38 -0400
Subject: [PATCH 3/6] fix(engine): address remaining PR #54 review findings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 hooks/hooks_test.sh       | 148 +++++++++++++++++++++++++++++
 src/engine/engine_test.go |  29 ++++++
 src/engine/workflow.go    |  41 ++++++--
 src/go.mod                |   4 +-
 src/go.sum                |  16 ++++
 src/lib/state_json.go     | 191 ++++++++++++++++++++++++++++++--------
 src/mcp/tools.go          | 103 +++++++++++++++++---
 src/mcp/tools_test.go     | 134 ++++++++++++++++++++++++++
 8 files changed, 603 insertions(+), 63 deletions(-)

diff --git a/hooks/hooks_test.sh b/hooks/hooks_test.sh
index d65d068..efcdc7d 100644
--- a/hooks/hooks_test.sh
+++ b/hooks/hooks_test.sh
@@ -150,6 +150,154 @@ run_hook "stop-gate.sh" \
 # stop-gate.sh — empty input
 run_hook "stop-gate.sh" "" "stop-gate: empty input" true
 
+echo ""
+echo "=== devkit-guard.sh (command-step enforcement) ==="
+
+# Helper: run guard with a session.json containing specific fields.
+# Args: session_json_body tool_input expected_exit label
+run_guard() {
+  local body="$1" tool_input="$2" want_exit="$3" label="$4"
+  local tmp
+  tmp=$(mktemp -d)
+  printf '%s' "$body" > "$tmp/session.json"
+  local exit_code=0
+  printf '%s' "$tool_input" | CLAUDE_PLUGIN_DATA="$tmp" bash "$HOOK_DIR/devkit-guard.sh" >/dev/null 2>&1 || exit_code=$?
+  if [[ "$exit_code" -eq "$want_exit" ]]; then
+    pass "devkit-guard: $label"
+  else
+    fail "devkit-guard: $label (exit $exit_code, want $want_exit)"
+  fi
+  rm -rf "$tmp"
+}
+
+# No CLAUDE_PLUGIN_DATA — disabled (exit 0)
+printf '{"tool_name":"Bash"}' | CLAUDE_PLUGIN_DATA="" bash "$HOOK_DIR/devkit-guard.sh" >/dev/null 2>&1
+if [[ $? -eq 0 ]]; then pass "devkit-guard: no CLAUDE_PLUGIN_DATA → allow"; else fail "devkit-guard: no CLAUDE_PLUGIN_DATA"; fi
+
+# Empty data dir — no session file → allow
+guard_tmp=$(mktemp -d)
+printf '{"tool_name":"Bash"}' | CLAUDE_PLUGIN_DATA="$guard_tmp" bash "$HOOK_DIR/devkit-guard.sh" >/dev/null 2>&1
+if [[ $? -eq 0 ]]; then pass "devkit-guard: no session file → allow"; else fail "devkit-guard: no session file"; fi
+rm -rf "$guard_tmp"
+
+# status != running → allow everything
+run_guard '{"status":"done","step_type":"command","enforce":"hard","current_step":"build"}' \
+  '{"tool_name":"Bash","tool_input":{"command":"ls"}}' \
+  0 "status=done → allow"
+
+# Prompt step (any enforce) → allow everything
+run_guard '{"status":"running","step_type":"prompt","enforce":"hard","current_step":"analyse"}' \
+  '{"tool_name":"Bash","tool_input":{"command":"ls"}}' \
+  0 "prompt step hard enforce → allow Bash"
+
+# Command step + hard enforce + Bash → block (exit 2)
+run_guard '{"status":"running","step_type":"command","enforce":"hard","current_step":"build"}' \
+  '{"tool_name":"Bash","tool_input":{"command":"make"}}' \
+  2 "command+hard+Bash → block"
+
+# Command step + hard enforce + Write → block
+run_guard '{"status":"running","step_type":"command","enforce":"hard","current_step":"build"}' \
+  '{"tool_name":"Write","tool_input":{"file_path":"x.go","content":"package x"}}' \
+  2 "command+hard+Write → block"
+
+# Command step + hard enforce + devkit_advance → allow
+run_guard '{"status":"running","step_type":"command","enforce":"hard","current_step":"build"}' \
+  '{"tool_name":"devkit_advance"}' \
+  0 "command+hard+devkit_advance → allow"
+
+# Command step + hard enforce + mcp__devkit__advance → allow (MCP namespaced)
+run_guard '{"status":"running","step_type":"command","enforce":"hard","current_step":"build"}' \
+  '{"tool_name":"mcp__devkit__advance"}' \
+  0 "command+hard+mcp__devkit__advance → allow"
+
+# Command step + hard enforce + TodoWrite → allow (pure in-memory)
+run_guard '{"status":"running","step_type":"command","enforce":"hard","current_step":"build"}' \
+  '{"tool_name":"TodoWrite","tool_input":{}}' \
+  0 "command+hard+TodoWrite → allow"
+
+# Command step + soft enforce → allow everything
+run_guard '{"status":"running","step_type":"command","enforce":"soft","current_step":"build"}' \
+  '{"tool_name":"Bash","tool_input":{"command":"ls"}}' \
+  0 "command+soft → allow"
+
+# Corrupt JSON session file → fail closed (exit 2)
+corrupt_tmp=$(mktemp -d)
+printf '{not valid json' > "$corrupt_tmp/session.json"
+printf '{"tool_name":"Bash"}' | CLAUDE_PLUGIN_DATA="$corrupt_tmp" bash "$HOOK_DIR/devkit-guard.sh" >/dev/null 2>&1
+corrupt_exit=$?
+if [[ $corrupt_exit -eq 2 ]]; then
+  pass "devkit-guard: corrupt JSON → block"
+else
+  fail "devkit-guard: corrupt JSON (exit $corrupt_exit, want 2)"
+fi
+rm -rf "$corrupt_tmp"
+
+echo ""
+echo "=== devkit-stop-guard.sh (stop-hook enforcement) ==="
+
+# Helper: run stop-guard and capture JSON output
+run_stop_guard() {
+  local body="$1" want_decision="$2" label="$3"
+  local tmp
+  tmp=$(mktemp -d)
+  printf '%s' "$body" > "$tmp/session.json"
+  local out
+  out=$(printf '{}' | CLAUDE_PLUGIN_DATA="$tmp" bash "$HOOK_DIR/devkit-stop-guard.sh" 2>/dev/null || true)
+  rm -rf "$tmp"
+  if ! printf '%s' "$out" | jq . >/dev/null 2>&1; then
+    fail "devkit-stop-guard: $label (invalid JSON: $out)"
+    return
+  fi
+  local decision
+  decision=$(printf '%s' "$out" | jq -r '.decision')
+  if [[ "$decision" == "$want_decision" ]]; then
+    pass "devkit-stop-guard: $label"
+  else
+    fail "devkit-stop-guard: $label (decision=$decision want=$want_decision)"
+  fi
+}
+
+# No CLAUDE_PLUGIN_DATA → approve
+out=$(printf '{}' | CLAUDE_PLUGIN_DATA="" bash "$HOOK_DIR/devkit-stop-guard.sh" 2>/dev/null)
+if printf '%s' "$out" | jq -e '.decision=="approve"' >/dev/null 2>&1; then
+  pass "devkit-stop-guard: no CLAUDE_PLUGIN_DATA → approve"
+else
+  fail "devkit-stop-guard: no CLAUDE_PLUGIN_DATA (got: $out)"
+fi
+
+# No session file → approve
+sg_tmp=$(mktemp -d)
+out=$(printf '{}' | CLAUDE_PLUGIN_DATA="$sg_tmp" bash "$HOOK_DIR/devkit-stop-guard.sh" 2>/dev/null)
+if printf '%s' "$out" | jq -e '.decision=="approve"' >/dev/null 2>&1; then
+  pass "devkit-stop-guard: no session file → approve"
+else
+  fail "devkit-stop-guard: no session file (got: $out)"
+fi
+rm -rf "$sg_tmp"
+
+# Running workflow → block
+run_stop_guard '{"status":"running","workflow":"test","total_steps":5,"current_index":2}' \
+  "block" "running workflow → block"
+
+# Done workflow → approve
+run_stop_guard '{"status":"done","workflow":"test","total_steps":5,"current_index":4}' \
+  "approve" "done workflow → approve"
+
+# Failed workflow → approve (user should see the failure, not be stuck in a loop)
+run_stop_guard '{"status":"failed","workflow":"test","total_steps":5,"current_index":2}' \
+  "approve" "failed workflow → approve"
+
+# Corrupt JSON → block (fail closed)
+corrupt_sg_tmp=$(mktemp -d)
+printf 'not json' > "$corrupt_sg_tmp/session.json"
+out=$(printf '{}' | CLAUDE_PLUGIN_DATA="$corrupt_sg_tmp" bash "$HOOK_DIR/devkit-stop-guard.sh" 2>/dev/null)
+if printf '%s' "$out" | jq -e '.decision=="block"' >/dev/null 2>&1; then
+  pass "devkit-stop-guard: corrupt JSON → block (fail closed)"
+else
+  fail "devkit-stop-guard: corrupt JSON (got: $out)"
+fi
+rm -rf "$corrupt_sg_tmp"
+
 echo ""
 echo "========================================="
 echo "Results: $PASS passed, $FAIL failed"
diff --git a/src/engine/engine_test.go b/src/engine/engine_test.go
index 513d2d2..bc5d27d 100644
--- a/src/engine/engine_test.go
+++ b/src/engine/engine_test.go
@@ -243,6 +243,17 @@ steps:
   - id: a
     command: "echo hi"
     loop: {max: 3, until: DONE}`, "mutually exclusive"},
+		{"empty branch when", `name: T
+steps:
+  - id: a
+    prompt: x
+    branch: [{when: "", goto: b}]
+  - id: b
+    prompt: y`, "empty when"},
+		{"step with no body", `name: T
+steps:
+  - id: a
+    model: fast`, "no body"},
 	}
 
 	for _, tt := range tests {
@@ -1397,3 +1408,21 @@ steps:
 		t.Errorf("step principles = %v, want [clean-code]", wf.Steps[0].Principles)
 	}
 }
+
+func TestInterpolateDeterministic(t *testing.T) {
+	// Regression: map iteration order is randomized in Go; Interpolate
+	// must sort keys so a step output containing {{another-id}} renders
+	// the same way on every call.
+	outputs := map[string]string{
+		"a": "[AA {{b}}]",
+		"b": "BB",
+		"c": "CC",
+	}
+	first := Interpolate("{{a}} {{c}}", "", outputs)
+	for i := 0; i < 100; i++ {
+		got := Interpolate("{{a}} {{c}}", "", outputs)
+		if got != first {
+			t.Fatalf("nondeterministic interpolate: %q vs %q", first, got)
+		}
+	}
+}
diff --git a/src/engine/workflow.go b/src/engine/workflow.go
index 2f56dc3..b48c2e1 100644
--- a/src/engine/workflow.go
+++ b/src/engine/workflow.go
@@ -7,6 +7,7 @@ import (
 	"bytes"
 	"fmt"
 	"os"
+	"sort"
 	"strings"
 
 	"gopkg.in/yaml.v3"
@@ -113,12 +114,25 @@ func validate(wf *Workflow) error {
 		}
 		ids[s.ID] = true
 
-		// Validate step mode mutual exclusion
-		if s.Command != "" && s.Prompt != "" {
-			return fmt.Errorf("step %q has both command and prompt — these are mutually exclusive", s.ID)
+		// Validate step mode mutual exclusion — exactly one of
+		// prompt | command | parallel must be set. A step with only
+		// metadata (id/model/principles but no executable body) would
+		// otherwise parse fine and silently do nothing.
+		modes := 0
+		if s.Prompt != "" {
+			modes++
 		}
-		if len(s.Parallel) > 0 && (s.Prompt != "" || s.Command != "") {
-			return fmt.Errorf("step %q has both parallel and prompt/command — these are mutually exclusive", s.ID)
+		if s.Command != "" {
+			modes++
+		}
+		if len(s.Parallel) > 0 {
+			modes++
+		}
+		if modes == 0 {
+			return fmt.Errorf("step %q has no body — set exactly one of prompt, command, or parallel", s.ID)
+		}
+		if modes > 1 {
+			return fmt.Errorf("step %q has multiple bodies — prompt, command, and parallel are mutually exclusive, set exactly one", s.ID)
 		}
 		if s.Expect != "" && s.Command == "" {
 			return fmt.Errorf("step %q has expect without command — expect only applies to command steps", s.ID)
@@ -148,6 +162,12 @@ func validate(wf *Workflow) error {
 	// Validate branch targets exist
 	for _, s := range wf.Steps {
 		for _, b := range s.Branch {
+			// Reject empty when — strings.Contains(x, "") is
+			// always true, so an empty when: matches every step
+			// and silently hijacks execution.
+			if strings.TrimSpace(b.When) == "" {
+				return fmt.Errorf("branch in step %q has empty when: — use a non-empty sentinel", s.ID)
+			}
 			if !ids[b.Goto] {
 				return fmt.Errorf("branch target %q not found (step %q)", b.Goto, s.ID)
 			}
@@ -170,10 +190,17 @@ func (wf *Workflow) Validate() error {
 }
 
 // Interpolate replaces {{step-id}} and {{input}} placeholders in a prompt.
+// Keys are iterated in sorted order so rendering is deterministic when one
+// step's output itself contains a {{another-id}} placeholder.
 func Interpolate(prompt string, input string, outputs map[string]string) string {
 	result := strings.ReplaceAll(prompt, "{{input}}", input)
-	for id, output := range outputs {
-		result = strings.ReplaceAll(result, "{{"+id+"}}", output)
+	ids := make([]string, 0, len(outputs))
+	for id := range outputs {
+		ids = append(ids, id)
+	}
+	sort.Strings(ids)
+	for _, id := range ids {
+		result = strings.ReplaceAll(result, "{{"+id+"}}", outputs[id])
 	}
 	return result
 }
diff --git a/src/go.mod b/src/go.mod
index 7947529..9fc0e7a 100644
--- a/src/go.mod
+++ b/src/go.mod
@@ -3,7 +3,9 @@ module github.com/5uck1ess/devkit
 go 1.26.1
 
 require (
+	github.com/mark3labs/mcp-go v0.47.1
 	github.com/spf13/cobra v1.10.2
+	golang.org/x/sys v0.42.0
 	gopkg.in/yaml.v3 v3.0.1
 	modernc.org/sqlite v1.48.0
 )
@@ -13,14 +15,12 @@ require (
 	github.com/google/jsonschema-go v0.4.2 // indirect
 	github.com/google/uuid v1.6.0 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
-	github.com/mark3labs/mcp-go v0.47.1 // indirect
 	github.com/mattn/go-isatty v0.0.20 // indirect
 	github.com/ncruces/go-strftime v1.0.0 // indirect
 	github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect
 	github.com/spf13/cast v1.7.1 // indirect
 	github.com/spf13/pflag v1.0.9 // indirect
 	github.com/yosida95/uritemplate/v3 v3.0.2 // indirect
-	golang.org/x/sys v0.42.0 // indirect
 	modernc.org/libc v1.70.0 // indirect
 	modernc.org/mathutil v1.7.1 // indirect
 	modernc.org/memory v1.11.0 // indirect
diff --git a/src/go.sum b/src/go.sum
index 27b3eb1..0278968 100644
--- a/src/go.sum
+++ b/src/go.sum
@@ -1,6 +1,12 @@
 github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g=
+github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
+github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY=
 github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto=
+github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8=
+github.com/frankban/quicktest v1.14.6/go.mod h1:4ptaffx2x8+WTWXmUCuVU6aPUX1/Mz7zb5vbUoiM6w0=
+github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8=
+github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU=
 github.com/google/jsonschema-go v0.4.2 h1:tmrUohrwoLZZS/P3x7ex0WAVknEkBZM46iALbcqoRA8=
 github.com/google/jsonschema-go v0.4.2/go.mod h1:r5quNTdLOYEz95Ru18zA0ydNbBuYoo9tgaYcxEYhJVE=
 github.com/google/pprof v0.0.0-20250317173921-a4b03ec1a45e h1:ijClszYn+mADRFY17kjQEVQ1XRhq2/JR1M3sGqeJoxs=
@@ -11,14 +17,22 @@ github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs
 github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM=
 github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8=
 github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw=
+github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
+github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk=
+github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
+github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
 github.com/mark3labs/mcp-go v0.47.1 h1:A9sJJ20mscl/ssLYHjodfaoBmq6uuhMG7pAPNYaQymQ=
 github.com/mark3labs/mcp-go v0.47.1/go.mod h1:JKTC7R2LLVagkEWK7Kwu7DbmA6iIvnNAod6yrHiQMag=
 github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY=
 github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
 github.com/ncruces/go-strftime v1.0.0 h1:HMFp8mLCTPp341M/ZnA4qaf7ZlsbTc+miZjCLOFAw7w=
 github.com/ncruces/go-strftime v1.0.0/go.mod h1:Fwc5htZGVVkseilnfgOVb9mKy6w1naJmn9CehxcKcls=
+github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
+github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec h1:W09IVJc94icq4NjY3clb7Lk8O1qJ8BdBEF8z0ibU0rE=
 github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo=
+github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8=
+github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs=
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
 github.com/spf13/cast v1.7.1 h1:cuNEagBQEHWN1FnbGEjCXL2szYEXqfJPbP2HNUaca9Y=
 github.com/spf13/cast v1.7.1/go.mod h1:ancEpBxwJDODSW/UG4rDrAqiKolqNNh2DX3mk86cAdo=
@@ -26,6 +40,8 @@ github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU=
 github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4=
 github.com/spf13/pflag v1.0.9 h1:9exaQaMOCwffKiiiYk6/BndUBv+iRViNW+4lEMi0PvY=
 github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
+github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
+github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
 github.com/yosida95/uritemplate/v3 v3.0.2 h1:Ed3Oyj9yrmi9087+NczuL5BwkIc4wvTb5zIM+UJPGz4=
 github.com/yosida95/uritemplate/v3 v3.0.2/go.mod h1:ILOh0sOhIJR3+L/8afwt/kE++YT040gmv5BQTMR2HP4=
 go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg=
diff --git a/src/lib/state_json.go b/src/lib/state_json.go
index ca55754..43e3bee 100644
--- a/src/lib/state_json.go
+++ b/src/lib/state_json.go
@@ -6,26 +6,33 @@ import (
 	"os"
 	"path/filepath"
 	"time"
+
+	"golang.org/x/sys/unix"
 )
 
 // SessionState is the hot-path state file read by hooks on every tool call.
 type SessionState struct {
-	ID            string            `json:"id"`
-	Workflow      string            `json:"workflow"`
-	Input         string            `json:"input"`
-	CurrentStep   string            `json:"current_step"`
-	CurrentIndex  int               `json:"current_index"`
-	TotalSteps    int               `json:"total_steps"`
-	StepType      string            `json:"step_type"` // "prompt" | "command" | "parallel"
-	Enforce       string            `json:"enforce"`
-	Branch        bool              `json:"branch"`
-	BudgetUSD     float64           `json:"budget_usd"`
-	SpentUSD      float64           `json:"spent_usd"`
-	StartedAt     time.Time         `json:"started_at"`
-	Outputs       map[string]string `json:"outputs"`
-	Status        string            `json:"status"`                   // "running" | "done" | "failed"
-	LoopIteration int               `json:"loop_iteration,omitempty"` // current loop count for loop steps
-	LoopMax       int               `json:"loop_max,omitempty"`       // max iterations for current loop
+	ID           string            `json:"id"`
+	Workflow     string            `json:"workflow"`
+	Input        string            `json:"input"`
+	CurrentStep  string            `json:"current_step"`
+	CurrentIndex int               `json:"current_index"`
+	TotalSteps   int               `json:"total_steps"`
+	StepType     string            `json:"step_type"` // "prompt" | "command" | "parallel"
+	Enforce      string            `json:"enforce"`
+	Branch       bool              `json:"branch"`
+	BudgetUSD    float64           `json:"budget_usd"`
+	SpentUSD     float64           `json:"spent_usd"`
+	StartedAt    time.Time         `json:"started_at"`
+	Outputs      map[string]string `json:"outputs"`
+	Status       string            `json:"status"` // "running" | "done" | "failed"
+	// Busy is a claim flag set by devkit_advance while it is executing.
+	// A second concurrent devkit_advance seeing Busy=true rejects with a
+	// "step already in progress" error rather than racing the first
+	// writer. Written under the cross-process session.json.lock.
+	Busy          bool `json:"busy,omitempty"`
+	LoopIteration int  `json:"loop_iteration,omitempty"` // current loop count for loop steps
+	LoopMax       int  `json:"loop_max,omitempty"`       // max iterations for current loop
 }
 
 // SessionJSONPath returns the path to the hot-state session file.
@@ -33,45 +40,151 @@ func SessionJSONPath(dataDir string) string {
 	return filepath.Join(dataDir, "session.json")
 }
 
-// WriteSessionJSON atomically writes session state to the hot-path JSON file.
-func WriteSessionJSON(dataDir string, state *SessionState) error {
+// sessionLockPath is the sibling lock file for cross-process serialization.
+func sessionLockPath(dataDir string) string {
+	return filepath.Join(dataDir, "session.json.lock")
+}
+
+// withSessionLock acquires an exclusive advisory lock on a sibling .lock
+// file for the duration of fn. The lock is cross-process (flock) so two
+// MCP server instances or a racing hook cannot observe torn
+// read-modify-write sequences on session.json. The lock file itself is
+// created on first use and never removed; the lock is released by
+// closing the descriptor.
+func withSessionLock(dataDir string, fn func() error) error {
 	if err := os.MkdirAll(dataDir, 0o755); err != nil {
 		return fmt.Errorf("create data dir: %w", err)
 	}
+	lockPath := sessionLockPath(dataDir)
+	f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600)
+	if err != nil {
+		return fmt.Errorf("open session lock: %w", err)
+	}
+	defer f.Close()
+	if err := unix.Flock(int(f.Fd()), unix.LOCK_EX); err != nil {
+		return fmt.Errorf("acquire session lock: %w", err)
+	}
+	defer unix.Flock(int(f.Fd()), unix.LOCK_UN)
+	return fn()
+}
+
+// WriteSessionJSON atomically writes session state to the hot-path JSON file.
+// Serialized across processes via a sibling .lock file to prevent two
+// concurrent devkit_advance calls from clobbering each other's updates.
+// Uses a per-writer temp file name so temp collisions between racing
+// writers are impossible.
+func WriteSessionJSON(dataDir string, state *SessionState) error {
+	return withSessionLock(dataDir, func() error {
+		return writeSessionJSONLocked(dataDir, state)
+	})
+}
+
+func writeSessionJSONLocked(dataDir string, state *SessionState) error {
 	data, err := json.MarshalIndent(state, "", "  ")
 	if err != nil {
 		return fmt.Errorf("marshal session: %w", err)
 	}
 	path := SessionJSONPath(dataDir)
-	tmp := path + ".tmp"
-	if err := os.WriteFile(tmp, data, 0o644); err != nil {
+	// Per-writer temp name so two concurrent writers within the same
+	// process (or across processes between flock grants) never target
+	// the same temp path. CreateTemp uses O_EXCL so it is collision-free.
+	tmp, err := os.CreateTemp(dataDir, "session.json.tmp-*")
+	if err != nil {
+		return fmt.Errorf("create session tmp: %w", err)
+	}
+	tmpName := tmp.Name()
+	// Best-effort cleanup if we bail before rename.
+	committed := false
+	defer func() {
+		if !committed {
+			os.Remove(tmpName)
+		}
+	}()
+	if _, err := tmp.Write(data); err != nil {
+		tmp.Close()
 		return fmt.Errorf("write session tmp: %w", err)
 	}
-	return os.Rename(tmp, path)
+	if err := tmp.Chmod(0o600); err != nil {
+		tmp.Close()
+		return fmt.Errorf("chmod session tmp: %w", err)
+	}
+	if err := tmp.Close(); err != nil {
+		return fmt.Errorf("close session tmp: %w", err)
+	}
+	if err := os.Rename(tmpName, path); err != nil {
+		return fmt.Errorf("rename session: %w", err)
+	}
+	committed = true
+	return nil
 }
 
 // ReadSessionJSON reads the hot-path session state. Returns nil if no session file exists.
+// Takes a shared lock so a concurrent writer cannot be observed mid-rename.
 func ReadSessionJSON(dataDir string) (*SessionState, error) {
-	path := SessionJSONPath(dataDir)
-	data, err := os.ReadFile(path)
-	if os.IsNotExist(err) {
-		return nil, nil
-	}
-	if err != nil {
-		return nil, fmt.Errorf("read session: %w", err)
-	}
-	var state SessionState
-	if err := json.Unmarshal(data, &state); err != nil {
-		return nil, fmt.Errorf("parse session: %w", err)
-	}
-	return &state, nil
+	var state *SessionState
+	err := withSessionLock(dataDir, func() error {
+		path := SessionJSONPath(dataDir)
+		data, err := os.ReadFile(path)
+		if os.IsNotExist(err) {
+			return nil
+		}
+		if err != nil {
+			return fmt.Errorf("read session: %w", err)
+		}
+		var s SessionState
+		if err := json.Unmarshal(data, &s); err != nil {
+			return fmt.Errorf("parse session: %w", err)
+		}
+		state = &s
+		return nil
+	})
+	return state, err
 }
 
 // ClearSessionJSON removes the hot-path session file.
 func ClearSessionJSON(dataDir string) error {
-	path := SessionJSONPath(dataDir)
-	if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
-		return fmt.Errorf("clear session: %w", err)
-	}
-	return nil
+	return withSessionLock(dataDir, func() error {
+		path := SessionJSONPath(dataDir)
+		if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
+			return fmt.Errorf("clear session: %w", err)
+		}
+		return nil
+	})
+}
+
+// UpdateSessionJSON runs fn under an exclusive lock with the current
+// state, then writes whatever fn returns (unless fn returns nil, which
+// means "no change"). This gives callers a true read-modify-write
+// primitive so multiple devkit_advance calls never race on the same
+// session index.
+func UpdateSessionJSON(dataDir string, fn func(*SessionState) (*SessionState, error)) (*SessionState, error) {
+	var result *SessionState
+	err := withSessionLock(dataDir, func() error {
+		path := SessionJSONPath(dataDir)
+		data, err := os.ReadFile(path)
+		var cur *SessionState
+		if err == nil {
+			var s SessionState
+			if jerr := json.Unmarshal(data, &s); jerr != nil {
+				return fmt.Errorf("parse session: %w", jerr)
+			}
+			cur = &s
+		} else if !os.IsNotExist(err) {
+			return fmt.Errorf("read session: %w", err)
+		}
+		next, fnErr := fn(cur)
+		if fnErr != nil {
+			return fnErr
+		}
+		if next == nil {
+			result = cur
+			return nil
+		}
+		if werr := writeSessionJSONLocked(dataDir, next); werr != nil {
+			return werr
+		}
+		result = next
+		return nil
+	})
+	return result, err
 }
diff --git a/src/mcp/tools.go b/src/mcp/tools.go
index d45f397..0d36bfa 100644
--- a/src/mcp/tools.go
+++ b/src/mcp/tools.go
@@ -20,6 +20,12 @@ import (
 // commandTimeout is the maximum duration for workflow command execution.
 const commandTimeout = 5 * time.Minute
 
+// gateTimeout is shorter than commandTimeout because loop gates should
+// be fast checks (lint, test, build) — a gate that takes longer than
+// this is almost certainly wedged and should fail the loop fast rather
+// than eat the full command budget.
+const gateTimeout = 60 * time.Second
+
 func (s *Server) listTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 	tool := mcpmcp.NewTool("devkit_list",
 		mcpmcp.WithDescription("List available workflows"),
@@ -147,11 +153,26 @@ func (s *Server) startTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			}
 		}
 
-		// Git branch if configured
-		if wf.BranchMode && s.git != nil {
+		// Git branch if configured. Must be a hard error — if a
+		// workflow declares branch: true and we silently fall through,
+		// completeWorkflow will later commit onto the user's current
+		// branch (often main). Roll back session state and DB record
+		// before returning.
+		if wf.BranchMode {
+			if s.git == nil {
+				_ = lib.ClearSessionJSON(s.dataDir)
+				if s.db != nil {
+					_ = s.db.UpdateSessionStatus(sessionID, "failed")
+				}
+				return mcpmcp.NewToolResultError(fmt.Sprintf("workflow %q requires branch mode but git is not available", wfName)), nil
+			}
 			branchName := fmt.Sprintf("%s/%s", wf.Name, sessionID)
 			if err := s.git.CreateBranch(branchName); err != nil {
-				fmt.Fprintf(os.Stderr, "warning: branch creation failed: %v\n", err)
+				_ = lib.ClearSessionJSON(s.dataDir)
+				if s.db != nil {
+					_ = s.db.UpdateSessionStatus(sessionID, "failed")
+				}
+				return mcpmcp.NewToolResultError(fmt.Sprintf("create branch %q: %v (workflow declares branch mode and cannot proceed on the current branch)", branchName, err)), nil
 			}
 		}
 
@@ -267,16 +288,46 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			return mcpmcp.NewToolResultError("missing session argument"), nil
 		}
 
-		state, err := lib.ReadSessionJSON(s.dataDir)
+		// Claim the advance slot atomically under the session lock. If
+		// another advance is already in progress we reject — letting
+		// both proceed would race on the current step index and could
+		// execute the same command step twice or skip one.
+		state, err := lib.UpdateSessionJSON(s.dataDir, func(cur *lib.SessionState) (*lib.SessionState, error) {
+			if cur == nil {
+				return nil, fmt.Errorf("no active session")
+			}
+			if cur.ID != sessionID {
+				return nil, fmt.Errorf("session mismatch: active is %s", cur.ID)
+			}
+			if cur.Busy {
+				return nil, fmt.Errorf("step %s already in progress (another devkit_advance call holds the claim)", cur.CurrentStep)
+			}
+			cur.Busy = true
+			return cur, nil
+		})
 		if err != nil {
-			return mcpmcp.NewToolResultError(fmt.Sprintf("read session: %v", err)), nil
-		}
-		if state == nil {
-			return mcpmcp.NewToolResultError("no active session"), nil
+			return mcpmcp.NewToolResultError(err.Error()), nil
 		}
-		if state.ID != sessionID {
-			return mcpmcp.NewToolResultError(fmt.Sprintf("session mismatch: active is %s", state.ID)), nil
+
+		// Ensure the claim is released no matter how we exit. On the
+		// success path we explicitly clear Busy before the final write;
+		// this deferred clear is the safety net for panics, errors, or
+		// early returns in the handlers below.
+		claimReleased := false
+		releaseClaim := func() {
+			if claimReleased {
+				return
+			}
+			claimReleased = true
+			_, _ = lib.UpdateSessionJSON(s.dataDir, func(cur *lib.SessionState) (*lib.SessionState, error) {
+				if cur == nil || cur.ID != sessionID {
+					return nil, nil
+				}
+				cur.Busy = false
+				return cur, nil
+			})
 		}
+		defer releaseClaim()
 
 		// Re-parse workflow using validated filename stored in state.Workflow
 		wfPath, err := s.resolveWorkflowPath(state.Workflow)
@@ -325,8 +376,11 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			}
 		}
 
-		// Handle loop steps
+		// Handle loop steps. handleLoopAdvance writes state (clearing
+		// Busy) on all its return paths, so mark the claim released
+		// here so the deferred release does not double-write.
 		if currentStep.Loop != nil {
+			claimReleased = true
 			return s.handleLoopAdvance(ctx, wf, state, &currentStep, req)
 		}
 
@@ -349,17 +403,22 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 		}
 
 		if nextIndex >= len(wf.Steps) {
+			claimReleased = true // completeWorkflow clears the whole state
 			return s.completeWorkflow(state)
 		}
 
-		// Write next step state
+		// Write next step state. Clear the claim as part of this same
+		// write so hooks observing session.json mid-transition never
+		// see Busy=true with a stale step index.
 		nextStep := wf.Steps[nextIndex]
 		state.CurrentStep = nextStep.ID
 		state.CurrentIndex = nextIndex
 		state.StepType = stepType(nextStep)
+		state.Busy = false
 		if err := lib.WriteSessionJSON(s.dataDir, state); err != nil {
 			return mcpmcp.NewToolResultError(fmt.Sprintf("write state: %v", err)), nil
 		}
+		claimReleased = true
 
 		response := s.formatStepResponse(wf, state, &nextStep, state.Input)
 		return mcpmcp.NewToolResultText(response), nil
@@ -413,7 +472,16 @@ func (s *Server) completeWorkflow(state *lib.SessionState) (*mcpmcp.CallToolResu
 // injection via LLM-chosen input or contaminated prior-step output — the
 // command text is always the literal YAML value.
 func (s *Server) runCommand(ctx context.Context, command string, state *lib.SessionState) (string, int, error) {
-	ctx, cancel := context.WithTimeout(ctx, commandTimeout)
+	return s.runCommandWithTimeout(ctx, command, state, commandTimeout)
+}
+
+// runCommandWithTimeout is the general form — gate commands call this
+// with gateTimeout so a stuck gate does not eat the full command budget.
+func (s *Server) runCommandWithTimeout(ctx context.Context, command string, state *lib.SessionState, timeout time.Duration) (string, int, error) {
+	// Nest a fresh deadline on top of the parent. Parent cancellation
+	// still propagates (e.g. MCP request abort), but the effective
+	// deadline is now min(parent deadline, now+timeout).
+	ctx, cancel := context.WithTimeout(ctx, timeout)
 	defer cancel()
 
 	cmd := exec.CommandContext(ctx, "sh", "-c", command)
@@ -482,8 +550,10 @@ func (s *Server) handleLoopAdvance(ctx context.Context, wf *engine.Workflow, sta
 
 	// Check gate command if present. Gate strings are also literal —
 	// values come via env vars DEVKIT_INPUT / DEVKIT_OUT_<step_id>.
+	// Gates get a shorter independent timeout so a wedged gate cannot
+	// eat the full command budget.
 	if step.Loop.Gate != "" {
-		gateOut, exitCode, err := s.runCommand(ctx, step.Loop.Gate, state)
+		gateOut, exitCode, err := s.runCommandWithTimeout(ctx, step.Loop.Gate, state, gateTimeout)
 		if err != nil {
 			return mcpmcp.NewToolResultError(fmt.Sprintf("gate command failed: %v\n%s", err, gateOut)), nil
 		}
@@ -509,7 +579,9 @@ func (s *Server) handleLoopAdvance(ctx context.Context, wf *engine.Workflow, sta
 		return s.advancePastLoop(wf, state)
 	}
 
-	// Continue loop — return same step for another iteration
+	// Continue loop — return same step for another iteration.
+	// Clear the advance claim as part of this write (see advanceTool).
+	state.Busy = false
 	if err := lib.WriteSessionJSON(s.dataDir, state); err != nil {
 		return mcpmcp.NewToolResultError(fmt.Sprintf("write loop state: %v", err)), nil
 	}
@@ -532,6 +604,7 @@ func (s *Server) advancePastLoop(wf *engine.Workflow, state *lib.SessionState) (
 	state.CurrentStep = nextStep.ID
 	state.CurrentIndex = nextIndex
 	state.StepType = stepType(nextStep)
+	state.Busy = false
 	if err := lib.WriteSessionJSON(s.dataDir, state); err != nil {
 		return mcpmcp.NewToolResultError(fmt.Sprintf("write state: %v", err)), nil
 	}
diff --git a/src/mcp/tools_test.go b/src/mcp/tools_test.go
index 63a4b2f..5507590 100644
--- a/src/mcp/tools_test.go
+++ b/src/mcp/tools_test.go
@@ -987,6 +987,140 @@ steps:
 	}
 }
 
+// TestAdvanceConcurrentClaim verifies the cross-process Busy claim: when
+// one devkit_advance is mid-flight, a racing second call must be rejected
+// rather than both advancing and clobbering the session index.
+func TestAdvanceConcurrentClaim(t *testing.T) {
+	dataDir := t.TempDir()
+
+	// Seed state directly as if an advance is already in progress.
+	seeded := &lib.SessionState{
+		ID:           "race-sess",
+		Workflow:     "race",
+		CurrentStep:  "one",
+		CurrentIndex: 0,
+		TotalSteps:   2,
+		StepType:     "prompt",
+		Enforce:      "hard",
+		Status:       "running",
+		Busy:         true,
+		StartedAt:    time.Now(),
+		Outputs:      map[string]string{},
+	}
+	if err := lib.WriteSessionJSON(dataDir, seeded); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	wfDir := t.TempDir()
+	writeFile(t, filepath.Join(wfDir, "race.yml"), `name: race
+description: race test
+steps:
+  - id: one
+    prompt: first
+  - id: two
+    prompt: second
+`)
+
+	srv := newTestServer(t, dataDir, wfDir)
+	_, advHandler := srv.advanceTool()
+
+	req := mcpmcp.CallToolRequest{}
+	req.Params.Arguments = map[string]interface{}{
+		"session": "race-sess",
+		"output":  "x",
+	}
+	result, err := advHandler(context.Background(), req)
+	if err != nil {
+		t.Fatalf("advance: %v", err)
+	}
+	if !result.IsError {
+		t.Fatal("expected IsError=true when Busy claim is held")
+	}
+	tc, _ := result.Content[0].(mcpmcp.TextContent)
+	if !strings.Contains(tc.Text, "already in progress") {
+		t.Errorf("expected 'already in progress' message, got: %s", tc.Text)
+	}
+
+	// Busy must still be true — the rejected caller must not clear it.
+	state, err := lib.ReadSessionJSON(dataDir)
+	if err != nil || state == nil {
+		t.Fatalf("read state: %v", err)
+	}
+	if !state.Busy {
+		t.Error("rejected advance should not have cleared Busy")
+	}
+}
+
+// TestAdvanceClearsBusy verifies that a successful advance clears the
+// Busy claim so the next call can proceed.
+func TestAdvanceClearsBusy(t *testing.T) {
+	wfDir := t.TempDir()
+	dataDir := t.TempDir()
+
+	writeFile(t, filepath.Join(wfDir, "clear.yml"), `name: clear
+description: clear busy test
+steps:
+  - id: a
+    prompt: first
+  - id: b
+    prompt: second
+`)
+
+	srv := newTestServer(t, dataDir, wfDir)
+
+	_, startHandler := srv.startTool()
+	startReq := mcpmcp.CallToolRequest{}
+	startReq.Params.Arguments = map[string]interface{}{"workflow": "clear", "input": "x"}
+	if _, err := startHandler(context.Background(), startReq); err != nil {
+		t.Fatalf("start: %v", err)
+	}
+
+	state, _ := lib.ReadSessionJSON(dataDir)
+	if state.Busy {
+		t.Error("start should not leave Busy=true")
+	}
+
+	_, advHandler := srv.advanceTool()
+	advReq := mcpmcp.CallToolRequest{}
+	advReq.Params.Arguments = map[string]interface{}{
+		"session": state.ID,
+		"output":  "done with a",
+	}
+	if _, err := advHandler(context.Background(), advReq); err != nil {
+		t.Fatalf("advance: %v", err)
+	}
+
+	state2, _ := lib.ReadSessionJSON(dataDir)
+	if state2.Busy {
+		t.Error("advance should clear Busy before returning")
+	}
+	if state2.CurrentStep != "b" {
+		t.Errorf("expected CurrentStep=b, got %q", state2.CurrentStep)
+	}
+}
+
+// TestSessionFileMode verifies session.json is chmod 0600 (may contain
+// pasted secrets via workflow input/outputs).
+func TestSessionFileMode(t *testing.T) {
+	dir := t.TempDir()
+	state := &lib.SessionState{
+		ID:      "mode-test",
+		Status:  "running",
+		Outputs: map[string]string{},
+	}
+	if err := lib.WriteSessionJSON(dir, state); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+	info, err := os.Stat(lib.SessionJSONPath(dir))
+	if err != nil {
+		t.Fatalf("stat: %v", err)
+	}
+	got := info.Mode().Perm()
+	if got != 0o600 {
+		t.Errorf("session.json mode = %o, want 0600", got)
+	}
+}
+
 // writeFile is a test helper that creates a file with the given content.
 func writeFile(t *testing.T, path, content string) {
 	t.Helper()

From e487c4e6ceac8b1c97a39633a244c0f4d01370a3 Mon Sep 17 00:00:00 2001
From: Tym Rabchuk <tymrabchuk@gmail.com>
Date: Fri, 10 Apr 2026 02:04:18 -0400
Subject: [PATCH 4/6] fix(engine): address mega-pr review findings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 hooks/hooks_test.sh   |  40 +++++++--
 src/lib/state_json.go |  38 ++++----
 src/mcp/tools.go      | 105 +++++++++++-----------
 src/mcp/tools_test.go | 197 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 300 insertions(+), 80 deletions(-)

diff --git a/hooks/hooks_test.sh b/hooks/hooks_test.sh
index efcdc7d..06774b4 100644
--- a/hooks/hooks_test.sh
+++ b/hooks/hooks_test.sh
@@ -153,12 +153,27 @@ run_hook "stop-gate.sh" "" "stop-gate: empty input" true
 echo ""
 echo "=== devkit-guard.sh (command-step enforcement) ==="
 
+# Collect tmp dirs created by the guard/stop-guard tests for a single
+# trap-driven cleanup on exit (including early exit, SIGINT, or ERR).
+GUARD_TMPS=()
+cleanup_guard_tmps() {
+  for d in "${GUARD_TMPS[@]}"; do
+    [[ -n "$d" && -d "$d" ]] && rm -rf "$d"
+  done
+}
+trap cleanup_guard_tmps EXIT INT TERM
+track_tmp() {
+  GUARD_TMPS+=("$1")
+}
+
 # Helper: run guard with a session.json containing specific fields.
 # Args: session_json_body tool_input expected_exit label
+# Tmp dir is tracked by the EXIT trap so early-exit/SIGINT also cleans up.
 run_guard() {
   local body="$1" tool_input="$2" want_exit="$3" label="$4"
   local tmp
   tmp=$(mktemp -d)
+  track_tmp "$tmp"
   printf '%s' "$body" > "$tmp/session.json"
   local exit_code=0
   printf '%s' "$tool_input" | CLAUDE_PLUGIN_DATA="$tmp" bash "$HOOK_DIR/devkit-guard.sh" >/dev/null 2>&1 || exit_code=$?
@@ -167,7 +182,6 @@ run_guard() {
   else
     fail "devkit-guard: $label (exit $exit_code, want $want_exit)"
   fi
-  rm -rf "$tmp"
 }
 
 # No CLAUDE_PLUGIN_DATA — disabled (exit 0)
@@ -176,9 +190,9 @@ if [[ $? -eq 0 ]]; then pass "devkit-guard: no CLAUDE_PLUGIN_DATA → allow"; el
 
 # Empty data dir — no session file → allow
 guard_tmp=$(mktemp -d)
+track_tmp "$guard_tmp"
 printf '{"tool_name":"Bash"}' | CLAUDE_PLUGIN_DATA="$guard_tmp" bash "$HOOK_DIR/devkit-guard.sh" >/dev/null 2>&1
 if [[ $? -eq 0 ]]; then pass "devkit-guard: no session file → allow"; else fail "devkit-guard: no session file"; fi
-rm -rf "$guard_tmp"
 
 # status != running → allow everything
 run_guard '{"status":"done","step_type":"command","enforce":"hard","current_step":"build"}' \
@@ -222,6 +236,7 @@ run_guard '{"status":"running","step_type":"command","enforce":"soft","current_s
 
 # Corrupt JSON session file → fail closed (exit 2)
 corrupt_tmp=$(mktemp -d)
+track_tmp "$corrupt_tmp"
 printf '{not valid json' > "$corrupt_tmp/session.json"
 printf '{"tool_name":"Bash"}' | CLAUDE_PLUGIN_DATA="$corrupt_tmp" bash "$HOOK_DIR/devkit-guard.sh" >/dev/null 2>&1
 corrupt_exit=$?
@@ -230,7 +245,20 @@ if [[ $corrupt_exit -eq 2 ]]; then
 else
   fail "devkit-guard: corrupt JSON (exit $corrupt_exit, want 2)"
 fi
-rm -rf "$corrupt_tmp"
+
+# Valid JSON but missing enforce field — Python .get() returns default
+# "hard", so a command step with no enforce must still block like hard.
+# This catches the "schema drift silently degrades enforcement" class.
+run_guard '{"status":"running","step_type":"command","current_step":"build"}' \
+  '{"tool_name":"Bash","tool_input":{"command":"ls"}}' \
+  2 "command step with missing enforce field → block (default hard)"
+
+# Valid JSON missing step_type — should default to empty string, which
+# is NOT "command", so fall through to allow. This verifies the guard
+# does not accidentally block prompt-like steps because of schema drift.
+run_guard '{"status":"running","enforce":"hard","current_step":"analyse"}' \
+  '{"tool_name":"Bash","tool_input":{"command":"ls"}}' \
+  0 "missing step_type treated as non-command → allow"
 
 echo ""
 echo "=== devkit-stop-guard.sh (stop-hook enforcement) ==="
@@ -240,10 +268,10 @@ run_stop_guard() {
   local body="$1" want_decision="$2" label="$3"
   local tmp
   tmp=$(mktemp -d)
+  track_tmp "$tmp"
   printf '%s' "$body" > "$tmp/session.json"
   local out
   out=$(printf '{}' | CLAUDE_PLUGIN_DATA="$tmp" bash "$HOOK_DIR/devkit-stop-guard.sh" 2>/dev/null || true)
-  rm -rf "$tmp"
   if ! printf '%s' "$out" | jq . >/dev/null 2>&1; then
     fail "devkit-stop-guard: $label (invalid JSON: $out)"
     return
@@ -267,13 +295,13 @@ fi
 
 # No session file → approve
 sg_tmp=$(mktemp -d)
+track_tmp "$sg_tmp"
 out=$(printf '{}' | CLAUDE_PLUGIN_DATA="$sg_tmp" bash "$HOOK_DIR/devkit-stop-guard.sh" 2>/dev/null)
 if printf '%s' "$out" | jq -e '.decision=="approve"' >/dev/null 2>&1; then
   pass "devkit-stop-guard: no session file → approve"
 else
   fail "devkit-stop-guard: no session file (got: $out)"
 fi
-rm -rf "$sg_tmp"
 
 # Running workflow → block
 run_stop_guard '{"status":"running","workflow":"test","total_steps":5,"current_index":2}' \
@@ -289,6 +317,7 @@ run_stop_guard '{"status":"failed","workflow":"test","total_steps":5,"current_in
 
 # Corrupt JSON → block (fail closed)
 corrupt_sg_tmp=$(mktemp -d)
+track_tmp "$corrupt_sg_tmp"
 printf 'not json' > "$corrupt_sg_tmp/session.json"
 out=$(printf '{}' | CLAUDE_PLUGIN_DATA="$corrupt_sg_tmp" bash "$HOOK_DIR/devkit-stop-guard.sh" 2>/dev/null)
 if printf '%s' "$out" | jq -e '.decision=="block"' >/dev/null 2>&1; then
@@ -296,7 +325,6 @@ if printf '%s' "$out" | jq -e '.decision=="block"' >/dev/null 2>&1; then
 else
   fail "devkit-stop-guard: corrupt JSON (got: $out)"
 fi
-rm -rf "$corrupt_sg_tmp"
 
 echo ""
 echo "========================================="
diff --git a/src/lib/state_json.go b/src/lib/state_json.go
index 43e3bee..da4f979 100644
--- a/src/lib/state_json.go
+++ b/src/lib/state_json.go
@@ -26,10 +26,9 @@ type SessionState struct {
 	StartedAt    time.Time         `json:"started_at"`
 	Outputs      map[string]string `json:"outputs"`
 	Status       string            `json:"status"` // "running" | "done" | "failed"
-	// Busy is a claim flag set by devkit_advance while it is executing.
-	// A second concurrent devkit_advance seeing Busy=true rejects with a
-	// "step already in progress" error rather than racing the first
-	// writer. Written under the cross-process session.json.lock.
+	// Busy is a claim flag held for the duration of an in-flight step
+	// advance. Written under the session lock; a concurrent claimant
+	// seeing it set must abort instead of racing the current writer.
 	Busy          bool `json:"busy,omitempty"`
 	LoopIteration int  `json:"loop_iteration,omitempty"` // current loop count for loop steps
 	LoopMax       int  `json:"loop_max,omitempty"`       // max iterations for current loop
@@ -46,11 +45,8 @@ func sessionLockPath(dataDir string) string {
 }
 
 // withSessionLock acquires an exclusive advisory lock on a sibling .lock
-// file for the duration of fn. The lock is cross-process (flock) so two
-// MCP server instances or a racing hook cannot observe torn
-// read-modify-write sequences on session.json. The lock file itself is
-// created on first use and never removed; the lock is released by
-// closing the descriptor.
+// file for the duration of fn. Cross-process (flock); the lock file
+// persists and is released on fd close.
 func withSessionLock(dataDir string, fn func() error) error {
 	if err := os.MkdirAll(dataDir, 0o755); err != nil {
 		return fmt.Errorf("create data dir: %w", err)
@@ -64,15 +60,14 @@ func withSessionLock(dataDir string, fn func() error) error {
 	if err := unix.Flock(int(f.Fd()), unix.LOCK_EX); err != nil {
 		return fmt.Errorf("acquire session lock: %w", err)
 	}
+	// Register LOCK_UN only after LOCK_EX succeeds. If the lock
+	// acquisition failed we already returned above, so this defer
+	// never runs on an unlocked fd.
 	defer unix.Flock(int(f.Fd()), unix.LOCK_UN)
 	return fn()
 }
 
-// WriteSessionJSON atomically writes session state to the hot-path JSON file.
-// Serialized across processes via a sibling .lock file to prevent two
-// concurrent devkit_advance calls from clobbering each other's updates.
-// Uses a per-writer temp file name so temp collisions between racing
-// writers are impossible.
+// WriteSessionJSON atomically writes session state under the session lock.
 func WriteSessionJSON(dataDir string, state *SessionState) error {
 	return withSessionLock(dataDir, func() error {
 		return writeSessionJSONLocked(dataDir, state)
@@ -85,9 +80,8 @@ func writeSessionJSONLocked(dataDir string, state *SessionState) error {
 		return fmt.Errorf("marshal session: %w", err)
 	}
 	path := SessionJSONPath(dataDir)
-	// Per-writer temp name so two concurrent writers within the same
-	// process (or across processes between flock grants) never target
-	// the same temp path. CreateTemp uses O_EXCL so it is collision-free.
+	// CreateTemp uses O_EXCL so the tmp name is collision-free even if
+	// a caller ever writes without holding the session lock.
 	tmp, err := os.CreateTemp(dataDir, "session.json.tmp-*")
 	if err != nil {
 		return fmt.Errorf("create session tmp: %w", err)
@@ -152,11 +146,11 @@ func ClearSessionJSON(dataDir string) error {
 	})
 }
 
-// UpdateSessionJSON runs fn under an exclusive lock with the current
-// state, then writes whatever fn returns (unless fn returns nil, which
-// means "no change"). This gives callers a true read-modify-write
-// primitive so multiple devkit_advance calls never race on the same
-// session index.
+// UpdateSessionJSON runs fn under the session lock with the current
+// state. fn returning a non-nil state saves it; fn returning nil means
+// "no change" and the on-disk state is left untouched. This is a true
+// read-modify-write primitive — use it for any state mutation that
+// must be atomic against concurrent readers or writers.
 func UpdateSessionJSON(dataDir string, fn func(*SessionState) (*SessionState, error)) (*SessionState, error) {
 	var result *SessionState
 	err := withSessionLock(dataDir, func() error {
diff --git a/src/mcp/tools.go b/src/mcp/tools.go
index 0d36bfa..66a6407 100644
--- a/src/mcp/tools.go
+++ b/src/mcp/tools.go
@@ -20,10 +20,8 @@ import (
 // commandTimeout is the maximum duration for workflow command execution.
 const commandTimeout = 5 * time.Minute
 
-// gateTimeout is shorter than commandTimeout because loop gates should
-// be fast checks (lint, test, build) — a gate that takes longer than
-// this is almost certainly wedged and should fail the loop fast rather
-// than eat the full command budget.
+// gateTimeout bounds each loop gate independently so a wedged gate
+// cannot consume the full commandTimeout budget.
 const gateTimeout = 60 * time.Second
 
 func (s *Server) listTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
@@ -119,7 +117,11 @@ func (s *Server) startTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			return mcpmcp.NewToolResultError(fmt.Sprintf("workflow %q has no steps", wfName)), nil
 		}
 
-		// Create session — store the validated filename for safe re-parsing in advance
+		// Create session in memory only — store the validated filename
+		// for safe re-parsing in advance. We publish session.json ONLY
+		// after all pre-flight side effects (branch creation) succeed,
+		// so a concurrent devkit_advance can never observe a
+		// half-initialized session and race the start itself.
 		sessionID := lib.NewSessionID()
 		firstStep := wf.Steps[0]
 
@@ -137,11 +139,27 @@ func (s *Server) startTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			StartedAt:    time.Now(),
 			Outputs:      map[string]string{},
 		}
+
+		// Hard error on branch-mode failure: silent fallthrough would
+		// later commit onto the caller's current branch. Done BEFORE
+		// the session.json write so there is nothing to roll back.
+		if wf.BranchMode {
+			if s.git == nil {
+				return mcpmcp.NewToolResultError(fmt.Sprintf("workflow %q requires branch mode but git is not available", wfName)), nil
+			}
+			branchName := fmt.Sprintf("%s/%s", wf.Name, sessionID)
+			if err := s.git.CreateBranch(branchName); err != nil {
+				return mcpmcp.NewToolResultError(fmt.Sprintf("create branch %q: %v (workflow declares branch mode and cannot proceed on the current branch)", branchName, err)), nil
+			}
+		}
+
+		// Publish session state now that all pre-flight succeeded.
 		if err := lib.WriteSessionJSON(s.dataDir, state); err != nil {
 			return mcpmcp.NewToolResultError(fmt.Sprintf("write state: %v", err)), nil
 		}
 
-		// SQLite record
+		// SQLite record (best-effort; session.json is the source of
+		// truth for the hot path).
 		if s.db != nil {
 			if err := s.db.CreateSession(&lib.Session{
 				ID:       sessionID,
@@ -153,29 +171,6 @@ func (s *Server) startTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			}
 		}
 
-		// Git branch if configured. Must be a hard error — if a
-		// workflow declares branch: true and we silently fall through,
-		// completeWorkflow will later commit onto the user's current
-		// branch (often main). Roll back session state and DB record
-		// before returning.
-		if wf.BranchMode {
-			if s.git == nil {
-				_ = lib.ClearSessionJSON(s.dataDir)
-				if s.db != nil {
-					_ = s.db.UpdateSessionStatus(sessionID, "failed")
-				}
-				return mcpmcp.NewToolResultError(fmt.Sprintf("workflow %q requires branch mode but git is not available", wfName)), nil
-			}
-			branchName := fmt.Sprintf("%s/%s", wf.Name, sessionID)
-			if err := s.git.CreateBranch(branchName); err != nil {
-				_ = lib.ClearSessionJSON(s.dataDir)
-				if s.db != nil {
-					_ = s.db.UpdateSessionStatus(sessionID, "failed")
-				}
-				return mcpmcp.NewToolResultError(fmt.Sprintf("create branch %q: %v (workflow declares branch mode and cannot proceed on the current branch)", branchName, err)), nil
-			}
-		}
-
 		// Build response with first step + principles
 		response := s.formatStepResponse(wf, state, &firstStep, input)
 		return mcpmcp.NewToolResultText(response), nil
@@ -309,25 +304,23 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			return mcpmcp.NewToolResultError(err.Error()), nil
 		}
 
-		// Ensure the claim is released no matter how we exit. On the
-		// success path we explicitly clear Busy before the final write;
-		// this deferred clear is the safety net for panics, errors, or
-		// early returns in the handlers below.
-		claimReleased := false
-		releaseClaim := func() {
-			if claimReleased {
-				return
-			}
-			claimReleased = true
-			_, _ = lib.UpdateSessionJSON(s.dataDir, func(cur *lib.SessionState) (*lib.SessionState, error) {
-				if cur == nil || cur.ID != sessionID {
+		// Ensure the claim is released no matter how we exit. On
+		// success paths the handlers below have already written
+		// Busy=false, so this is a no-op. On error/panic paths this
+		// is the only thing that clears Busy — without it a failing
+		// handler would leak the claim and brick every subsequent
+		// advance. Release failure is logged, never swallowed.
+		defer func() {
+			if _, relErr := lib.UpdateSessionJSON(s.dataDir, func(cur *lib.SessionState) (*lib.SessionState, error) {
+				if cur == nil || cur.ID != sessionID || !cur.Busy {
 					return nil, nil
 				}
 				cur.Busy = false
 				return cur, nil
-			})
-		}
-		defer releaseClaim()
+			}); relErr != nil {
+				fmt.Fprintf(os.Stderr, "devkit advance: release claim failed for session %s: %v (run `devkit clear` if advance calls start rejecting with 'already in progress')\n", sessionID, relErr)
+			}
+		}()
 
 		// Re-parse workflow using validated filename stored in state.Workflow
 		wfPath, err := s.resolveWorkflowPath(state.Workflow)
@@ -376,11 +369,13 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			}
 		}
 
-		// Handle loop steps. handleLoopAdvance writes state (clearing
-		// Busy) on all its return paths, so mark the claim released
-		// here so the deferred release does not double-write.
+		// Handle loop steps. Do NOT pre-release the claim here — if
+		// handleLoopAdvance errors out before writing state (e.g.
+		// gate command failure), the deferred releaseClaim must still
+		// run to clear Busy on disk. On the happy path the helper
+		// writes with Busy=false, and the defer's follow-up write is
+		// a harmless no-op.
 		if currentStep.Loop != nil {
-			claimReleased = true
 			return s.handleLoopAdvance(ctx, wf, state, &currentStep, req)
 		}
 
@@ -403,13 +398,20 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 		}
 
 		if nextIndex >= len(wf.Steps) {
-			claimReleased = true // completeWorkflow clears the whole state
+			// Do NOT pre-release the claim — if completeWorkflow's
+			// initial WriteSessionJSON fails the session file still
+			// has Busy=true on disk and we need the deferred release
+			// to clean it up. On the success path completeWorkflow
+			// removes session.json via ClearSessionJSON, so the
+			// defer's UpdateSessionJSON finds nil and no-ops.
 			return s.completeWorkflow(state)
 		}
 
 		// Write next step state. Clear the claim as part of this same
-		// write so hooks observing session.json mid-transition never
-		// see Busy=true with a stale step index.
+		// write so the common case is a single atomic transition; the
+		// deferred releaseClaim will then observe Busy=false and
+		// no-op. We do NOT set claimReleased=true here so the defer
+		// still runs on subsequent panics or added return paths.
 		nextStep := wf.Steps[nextIndex]
 		state.CurrentStep = nextStep.ID
 		state.CurrentIndex = nextIndex
@@ -418,7 +420,6 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 		if err := lib.WriteSessionJSON(s.dataDir, state); err != nil {
 			return mcpmcp.NewToolResultError(fmt.Sprintf("write state: %v", err)), nil
 		}
-		claimReleased = true
 
 		response := s.formatStepResponse(wf, state, &nextStep, state.Input)
 		return mcpmcp.NewToolResultText(response), nil
diff --git a/src/mcp/tools_test.go b/src/mcp/tools_test.go
index 5507590..e0fb796 100644
--- a/src/mcp/tools_test.go
+++ b/src/mcp/tools_test.go
@@ -5,6 +5,8 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -1099,6 +1101,201 @@ steps:
 	}
 }
 
+// TestAdvanceRealRace launches N concurrent devkit_advance calls while
+// the first step (a sleeping command) holds the Busy claim. Under the
+// flock + Busy claim, exactly one must get through; every other racer
+// fired while Busy=true must be rejected with "already in progress".
+// Removing the lock or the Busy check should make this test flaky.
+func TestAdvanceRealRace(t *testing.T) {
+	wfDir := t.TempDir()
+	dataDir := t.TempDir()
+
+	// Command step that sleeps long enough for racers to pile up while
+	// the winner is inside runCommand (and therefore holding Busy).
+	writeFile(t, filepath.Join(wfDir, "race.yml"), `name: race
+description: concurrent race test
+steps:
+  - id: one
+    command: "sleep 0.3"
+    expect: success
+  - id: two
+    prompt: done
+`)
+
+	srv := newTestServer(t, dataDir, wfDir)
+
+	_, startHandler := srv.startTool()
+	startReq := mcpmcp.CallToolRequest{}
+	startReq.Params.Arguments = map[string]interface{}{"workflow": "race", "input": "x"}
+	if _, err := startHandler(context.Background(), startReq); err != nil {
+		t.Fatalf("start: %v", err)
+	}
+	state, _ := lib.ReadSessionJSON(dataDir)
+	sessionID := state.ID
+
+	_, advHandler := srv.advanceTool()
+
+	const N = 8
+	var wg sync.WaitGroup
+	var successes, rejections int64
+	start := make(chan struct{})
+	wg.Add(N)
+	for i := 0; i < N; i++ {
+		go func() {
+			defer wg.Done()
+			<-start // all goroutines released together
+			req := mcpmcp.CallToolRequest{}
+			req.Params.Arguments = map[string]interface{}{"session": sessionID}
+			result, err := advHandler(context.Background(), req)
+			if err != nil {
+				return
+			}
+			if result.IsError {
+				tc, _ := result.Content[0].(mcpmcp.TextContent)
+				if strings.Contains(tc.Text, "already in progress") {
+					atomic.AddInt64(&rejections, 1)
+				}
+				return
+			}
+			atomic.AddInt64(&successes, 1)
+		}()
+	}
+	close(start)
+	wg.Wait()
+
+	if successes != 1 {
+		t.Errorf("successes = %d, want 1 (Busy claim is not mutually exclusive)", successes)
+	}
+	if rejections != N-1 {
+		t.Errorf("rejections = %d, want %d (losing racers should see 'already in progress')", rejections, N-1)
+	}
+
+	final, _ := lib.ReadSessionJSON(dataDir)
+	if final == nil {
+		t.Fatal("session unexpectedly cleared")
+	}
+	if final.CurrentIndex != 1 {
+		t.Errorf("CurrentIndex = %d, want 1 (exactly one advance)", final.CurrentIndex)
+	}
+	if final.Busy {
+		t.Error("Busy should be cleared after all goroutines return")
+	}
+}
+
+// TestAdvanceCommandFailClearsBusy verifies the defer releaseClaim runs
+// and clears Busy when a command step errors out (expect mismatch). A
+// regression that leaked the claim would wedge the session forever.
+func TestAdvanceCommandFailClearsBusy(t *testing.T) {
+	wfDir := t.TempDir()
+	dataDir := t.TempDir()
+
+	writeFile(t, filepath.Join(wfDir, "f.yml"), `name: f
+steps:
+  - id: boom
+    command: "exit 1"
+    expect: success
+  - id: done
+    prompt: done
+`)
+
+	srv := newTestServer(t, dataDir, wfDir)
+	_, startHandler := srv.startTool()
+	startReq := mcpmcp.CallToolRequest{}
+	startReq.Params.Arguments = map[string]interface{}{"workflow": "f", "input": "x"}
+	startHandler(context.Background(), startReq)
+	state, _ := lib.ReadSessionJSON(dataDir)
+
+	_, advHandler := srv.advanceTool()
+	advReq := mcpmcp.CallToolRequest{}
+	advReq.Params.Arguments = map[string]interface{}{"session": state.ID}
+	result, _ := advHandler(context.Background(), advReq)
+	if !result.IsError {
+		t.Fatal("expected expect:success mismatch to return IsError")
+	}
+
+	after, err := lib.ReadSessionJSON(dataDir)
+	if err != nil {
+		t.Fatalf("read: %v", err)
+	}
+	if after == nil {
+		t.Fatal("session should still exist after failed advance")
+	}
+	if after.Busy {
+		t.Error("Busy leaked after command failure — defer releaseClaim did not run")
+	}
+}
+
+// TestStartBranchModeRollback verifies branch-mode failure does not
+// publish a session.json. Uses git=nil to force the "not available"
+// branch, which is the simplest injectable failure.
+func TestStartBranchModeRollback(t *testing.T) {
+	wfDir := t.TempDir()
+	dataDir := t.TempDir()
+	writeFile(t, filepath.Join(wfDir, "b.yml"), `name: b
+branch: true
+steps:
+  - id: one
+    prompt: first
+`)
+
+	srv := newTestServer(t, dataDir, wfDir) // git: nil
+	_, handler := srv.startTool()
+	req := mcpmcp.CallToolRequest{}
+	req.Params.Arguments = map[string]interface{}{"workflow": "b", "input": "x"}
+	result, err := handler(context.Background(), req)
+	if err != nil {
+		t.Fatalf("handler: %v", err)
+	}
+	if !result.IsError {
+		t.Fatal("expected IsError=true when git is nil and branch: true")
+	}
+
+	state, _ := lib.ReadSessionJSON(dataDir)
+	if state != nil {
+		t.Errorf("session.json should not exist after branch-mode failure, got %+v", state)
+	}
+}
+
+// TestUpdateSessionJSONNoChange verifies fn returning nil leaves the
+// on-disk state untouched (mtime unchanged, content unchanged).
+func TestUpdateSessionJSONNoChange(t *testing.T) {
+	dir := t.TempDir()
+	seed := &lib.SessionState{
+		ID:      "nochange",
+		Status:  "running",
+		Outputs: map[string]string{"a": "b"},
+	}
+	if err := lib.WriteSessionJSON(dir, seed); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+
+	path := lib.SessionJSONPath(dir)
+	before, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("stat: %v", err)
+	}
+	// Ensure mtime resolution is crossed.
+	time.Sleep(10 * time.Millisecond)
+
+	got, err := lib.UpdateSessionJSON(dir, func(cur *lib.SessionState) (*lib.SessionState, error) {
+		return nil, nil // no change
+	})
+	if err != nil {
+		t.Fatalf("update: %v", err)
+	}
+	if got == nil || got.ID != "nochange" {
+		t.Errorf("got %+v, want seeded state", got)
+	}
+
+	after, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("stat after: %v", err)
+	}
+	if !after.ModTime().Equal(before.ModTime()) {
+		t.Errorf("mtime changed (%v → %v) — no-change path must not write", before.ModTime(), after.ModTime())
+	}
+}
+
 // TestSessionFileMode verifies session.json is chmod 0600 (may contain
 // pasted secrets via workflow input/outputs).
 func TestSessionFileMode(t *testing.T) {

From 0002b4750a9f9a87082b840b4536d8035e52b3b9 Mon Sep 17 00:00:00 2001
From: Tym Rabchuk <tymrabchuk@gmail.com>
Date: Fri, 10 Apr 2026 02:13:33 -0400
Subject: [PATCH 5/6] fix(engine): address tri-review findings from mega-pr
 review
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 bin/devkit                    |  14 +++-
 src/engine/engine.go          |  18 +----
 src/engine/engine_test.go     |   6 ++
 src/engine/workflow.go        |  34 ++++++++
 src/lib/state_json.go         |  13 ++-
 src/lib/state_lock_unix.go    |  23 ++++++
 src/lib/state_lock_windows.go |  31 +++++++
 src/mcp/tools.go              | 148 ++++++++++++++++++++--------------
 src/mcp/tools_test.go         |  55 +++++++++++++
 9 files changed, 255 insertions(+), 87 deletions(-)
 create mode 100644 src/lib/state_lock_unix.go
 create mode 100644 src/lib/state_lock_windows.go

diff --git a/bin/devkit b/bin/devkit
index b7a9f07..e551d60 100755
--- a/bin/devkit
+++ b/bin/devkit
@@ -99,9 +99,15 @@ ensure_engine() {
 
     log "first-run: downloading engine ${tag} (${PLATFORM})…"
 
-    tmp_bin="${ENGINE_PATH}.tmp"
-    tmp_sums="${SCRIPT_DIR}/.checksums.${VERSION}.tmp"
-    trap 'rm -f "$tmp_bin" "$tmp_sums"' EXIT INT TERM
+    # Per-invocation temp dir so two simultaneous first runs never
+    # share tmp paths. Each process cleans up its own dir on exit;
+    # the final mv(2) is atomic within a filesystem, so whichever
+    # writer lands last produces the correct (bit-identical) binary.
+    tmp_dir=$(mktemp -d "${TMPDIR:-/tmp}/devkit-engine-XXXXXX") \
+        || die "mktemp -d failed"
+    tmp_bin="$tmp_dir/${engine_name}"
+    tmp_sums="$tmp_dir/checksums.txt"
+    trap 'rm -rf "$tmp_dir"' EXIT INT TERM
 
     download "${base_url}/${asset}" "$tmp_bin" \
         || die "download failed: ${base_url}/${asset}"
@@ -117,7 +123,7 @@ ensure_engine() {
 
     chmod +x "$tmp_bin"
     mv -f "$tmp_bin" "$ENGINE_PATH"
-    rm -f "$tmp_sums"
+    rm -rf "$tmp_dir"
     trap - EXIT INT TERM
 
     log "installed engine at $ENGINE_PATH"
diff --git a/src/engine/engine.go b/src/engine/engine.go
index a7afed0..8eec7c0 100644
--- a/src/engine/engine.go
+++ b/src/engine/engine.go
@@ -287,22 +287,8 @@ func buildCommandEnv(input string, outputs map[string]string) []string {
 	return env
 }
 
-// envKey maps a step ID (may contain hyphens) to a POSIX env var suffix.
-func envKey(id string) string {
-	b := make([]byte, 0, len(id))
-	for i := 0; i < len(id); i++ {
-		c := id[i]
-		switch {
-		case c >= 'a' && c <= 'z':
-			b = append(b, c-32)
-		case c >= 'A' && c <= 'Z', c >= '0' && c <= '9', c == '_':
-			b = append(b, c)
-		default:
-			b = append(b, '_')
-		}
-	}
-	return string(b)
-}
+// envKey is a package-private alias to EnvKey for call-site brevity.
+func envKey(id string) string { return EnvKey(id) }
 
 // runStep executes a single step and records it in the database.
 func (e *Engine) runStep(ctx context.Context, step *WfStep, session *lib.Session, input string, outputs map[string]string, opts runners.RunOpts, iterNum *int) (float64, string, error) {
diff --git a/src/engine/engine_test.go b/src/engine/engine_test.go
index bc5d27d..87950a5 100644
--- a/src/engine/engine_test.go
+++ b/src/engine/engine_test.go
@@ -254,6 +254,12 @@ steps:
 steps:
   - id: a
     model: fast`, "no body"},
+		{"env key collision", `name: T
+steps:
+  - id: fetch-data
+    prompt: x
+  - id: fetch_data
+    prompt: y`, "collide under env key"},
 	}
 
 	for _, tt := range tests {
diff --git a/src/engine/workflow.go b/src/engine/workflow.go
index b48c2e1..33c1bbe 100644
--- a/src/engine/workflow.go
+++ b/src/engine/workflow.go
@@ -105,6 +105,7 @@ func validate(wf *Workflow) error {
 	}
 
 	ids := make(map[string]bool)
+	envKeys := make(map[string]string) // canonical env key → first step id that produced it
 	for _, s := range wf.Steps {
 		if s.ID == "" {
 			return fmt.Errorf("step missing id in workflow %q", wf.Name)
@@ -114,6 +115,16 @@ func validate(wf *Workflow) error {
 		}
 		ids[s.ID] = true
 
+		// Reject step IDs whose canonical env-key collides with an
+		// earlier step. Without this, "fetch-data" and "fetch_data"
+		// would both map to DEVKIT_OUT_FETCH_DATA and silently
+		// overwrite each other depending on map iteration order.
+		key := EnvKey(s.ID)
+		if prior, clash := envKeys[key]; clash {
+			return fmt.Errorf("step ids %q and %q collide under env key %q in workflow %q — rename one", prior, s.ID, key, wf.Name)
+		}
+		envKeys[key] = s.ID
+
 		// Validate step mode mutual exclusion — exactly one of
 		// prompt | command | parallel must be set. A step with only
 		// metadata (id/model/principles but no executable body) would
@@ -189,6 +200,29 @@ func (wf *Workflow) Validate() error {
 	return validate(wf)
 }
 
+// EnvKey maps a workflow step ID to a POSIX env var suffix used in
+// DEVKIT_OUT_<key>. POSIX allows [A-Za-z_][A-Za-z0-9_]*, so any
+// non-alphanumeric byte is mapped to underscore and lowercase is
+// upcased. Note the collision risk: "a-b", "a_b", "a.b", "A B" all
+// produce "A_B". The validator rejects workflows whose step IDs
+// collide under this mapping so two outputs can never silently shadow
+// each other in the env.
+func EnvKey(id string) string {
+	b := make([]byte, 0, len(id))
+	for i := 0; i < len(id); i++ {
+		c := id[i]
+		switch {
+		case c >= 'a' && c <= 'z':
+			b = append(b, c-32)
+		case c >= 'A' && c <= 'Z', c >= '0' && c <= '9', c == '_':
+			b = append(b, c)
+		default:
+			b = append(b, '_')
+		}
+	}
+	return string(b)
+}
+
 // Interpolate replaces {{step-id}} and {{input}} placeholders in a prompt.
 // Keys are iterated in sorted order so rendering is deterministic when one
 // step's output itself contains a {{another-id}} placeholder.
diff --git a/src/lib/state_json.go b/src/lib/state_json.go
index da4f979..f954b81 100644
--- a/src/lib/state_json.go
+++ b/src/lib/state_json.go
@@ -6,8 +6,6 @@ import (
 	"os"
 	"path/filepath"
 	"time"
-
-	"golang.org/x/sys/unix"
 )
 
 // SessionState is the hot-path state file read by hooks on every tool call.
@@ -57,13 +55,14 @@ func withSessionLock(dataDir string, fn func() error) error {
 		return fmt.Errorf("open session lock: %w", err)
 	}
 	defer f.Close()
-	if err := unix.Flock(int(f.Fd()), unix.LOCK_EX); err != nil {
+	// Acquire platform-specific exclusive advisory lock. Implementations
+	// live in state_lock_unix.go / state_lock_windows.go behind build
+	// tags so the package compiles on all GOOS targets the release
+	// Makefile ships (linux, darwin, windows).
+	if err := lockFile(f); err != nil {
 		return fmt.Errorf("acquire session lock: %w", err)
 	}
-	// Register LOCK_UN only after LOCK_EX succeeds. If the lock
-	// acquisition failed we already returned above, so this defer
-	// never runs on an unlocked fd.
-	defer unix.Flock(int(f.Fd()), unix.LOCK_UN)
+	defer unlockFile(f)
 	return fn()
 }
 
diff --git a/src/lib/state_lock_unix.go b/src/lib/state_lock_unix.go
new file mode 100644
index 0000000..f09d8cd
--- /dev/null
+++ b/src/lib/state_lock_unix.go
@@ -0,0 +1,23 @@
+//go:build !windows
+
+package lib
+
+import (
+	"os"
+
+	"golang.org/x/sys/unix"
+)
+
+// lockFile takes an exclusive advisory flock on f. Blocks until granted.
+// Released automatically when the fd is closed, but callers should pair
+// with unlockFile on exit to be explicit.
+func lockFile(f *os.File) error {
+	return unix.Flock(int(f.Fd()), unix.LOCK_EX)
+}
+
+// unlockFile releases an advisory flock held on f. Errors are ignored
+// by callers because Close() would release the lock anyway; returning
+// the error lets tests assert on it.
+func unlockFile(f *os.File) error {
+	return unix.Flock(int(f.Fd()), unix.LOCK_UN)
+}
diff --git a/src/lib/state_lock_windows.go b/src/lib/state_lock_windows.go
new file mode 100644
index 0000000..363295b
--- /dev/null
+++ b/src/lib/state_lock_windows.go
@@ -0,0 +1,31 @@
+//go:build windows
+
+package lib
+
+import (
+	"os"
+
+	"golang.org/x/sys/windows"
+)
+
+// lockFile takes an exclusive lock on the first byte of f via
+// LockFileEx, which is Windows' mandatory equivalent of Unix advisory
+// flock for the purposes we use it (serializing session.json writers
+// across processes). Blocks until granted.
+func lockFile(f *os.File) error {
+	ol := new(windows.Overlapped)
+	return windows.LockFileEx(
+		windows.Handle(f.Fd()),
+		windows.LOCKFILE_EXCLUSIVE_LOCK,
+		0, 1, 0, ol,
+	)
+}
+
+// unlockFile releases a lock previously taken by lockFile.
+func unlockFile(f *os.File) error {
+	ol := new(windows.Overlapped)
+	return windows.UnlockFileEx(
+		windows.Handle(f.Fd()),
+		0, 1, 0, ol,
+	)
+}
diff --git a/src/mcp/tools.go b/src/mcp/tools.go
index 66a6407..f2b60af 100644
--- a/src/mcp/tools.go
+++ b/src/mcp/tools.go
@@ -80,15 +80,6 @@ func (s *Server) startTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 		mcpmcp.WithString("input", mcpmcp.Required(), mcpmcp.Description("Workflow input/description")),
 	)
 	return tool, func(ctx context.Context, req mcpmcp.CallToolRequest) (*mcpmcp.CallToolResult, error) {
-		// Check no active session — propagate read errors
-		existing, err := lib.ReadSessionJSON(s.dataDir)
-		if err != nil {
-			return mcpmcp.NewToolResultError(fmt.Sprintf("read session state: %v", err)), nil
-		}
-		if existing != nil && existing.Status == "running" {
-			return mcpmcp.NewToolResultError(fmt.Sprintf("workflow %s already running (session %s). Call devkit_advance to continue or devkit_status to check.", existing.Workflow, existing.ID)), nil
-		}
-
 		wfName, err := req.RequireString("workflow")
 		if err != nil {
 			return mcpmcp.NewToolResultError(fmt.Sprintf("missing argument: %v", err)), nil
@@ -117,32 +108,56 @@ func (s *Server) startTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			return mcpmcp.NewToolResultError(fmt.Sprintf("workflow %q has no steps", wfName)), nil
 		}
 
-		// Create session in memory only — store the validated filename
-		// for safe re-parsing in advance. We publish session.json ONLY
-		// after all pre-flight side effects (branch creation) succeed,
-		// so a concurrent devkit_advance can never observe a
-		// half-initialized session and race the start itself.
+		// Atomically CLAIM the session slot under the session lock with
+		// Status="starting". This closes the start/start race: two
+		// concurrent devkit_start calls both observing "no session"
+		// cannot both proceed — the second one sees Status="starting"
+		// or "running" and rejects. The "starting" status is distinct
+		// from "running" so a concurrent devkit_advance firing in this
+		// window will also correctly report "no active session" until
+		// we publish the transition to "running" below.
 		sessionID := lib.NewSessionID()
 		firstStep := wf.Steps[0]
-
-		state := &lib.SessionState{
-			ID:           sessionID,
-			Workflow:     wfName, // store filename, not wf.Name, to prevent traversal in advance
-			Input:        input,
-			CurrentStep:  firstStep.ID,
-			CurrentIndex: 0,
-			TotalSteps:   len(wf.Steps),
-			StepType:     stepType(firstStep),
-			Enforce:      wf.Enforce,
-			Branch:       wf.BranchMode,
-			Status:       "running",
-			StartedAt:    time.Now(),
-			Outputs:      map[string]string{},
+		state, err := lib.UpdateSessionJSON(s.dataDir, func(cur *lib.SessionState) (*lib.SessionState, error) {
+			if cur != nil && (cur.Status == "running" || cur.Status == "starting") {
+				return nil, fmt.Errorf("workflow %s already %s (session %s). Call devkit_advance to continue or devkit_status to check", cur.Workflow, cur.Status, cur.ID)
+			}
+			return &lib.SessionState{
+				ID:           sessionID,
+				Workflow:     wfName, // store filename, not wf.Name, to prevent traversal in advance
+				Input:        input,
+				CurrentStep:  firstStep.ID,
+				CurrentIndex: 0,
+				TotalSteps:   len(wf.Steps),
+				StepType:     stepType(firstStep),
+				Enforce:      wf.Enforce,
+				Branch:       wf.BranchMode,
+				Status:       "starting",
+				StartedAt:    time.Now(),
+				Outputs:      map[string]string{},
+			}, nil
+		})
+		if err != nil {
+			return mcpmcp.NewToolResultError(err.Error()), nil
 		}
 
+		// From here on, any failure must roll back the claim or the
+		// slot stays wedged. Track cleanup with a deferred rollback
+		// that only fires if we never reach the final "running"
+		// transition.
+		committed := false
+		defer func() {
+			if committed {
+				return
+			}
+			_ = lib.ClearSessionJSON(s.dataDir)
+			if s.db != nil {
+				_ = s.db.UpdateSessionStatus(sessionID, "failed")
+			}
+		}()
+
 		// Hard error on branch-mode failure: silent fallthrough would
-		// later commit onto the caller's current branch. Done BEFORE
-		// the session.json write so there is nothing to roll back.
+		// later commit onto the caller's current branch.
 		if wf.BranchMode {
 			if s.git == nil {
 				return mcpmcp.NewToolResultError(fmt.Sprintf("workflow %q requires branch mode but git is not available", wfName)), nil
@@ -153,10 +168,19 @@ func (s *Server) startTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			}
 		}
 
-		// Publish session state now that all pre-flight succeeded.
-		if err := lib.WriteSessionJSON(s.dataDir, state); err != nil {
-			return mcpmcp.NewToolResultError(fmt.Sprintf("write state: %v", err)), nil
+		// Transition starting → running. After this, devkit_advance
+		// will accept the session.
+		state, err = lib.UpdateSessionJSON(s.dataDir, func(cur *lib.SessionState) (*lib.SessionState, error) {
+			if cur == nil || cur.ID != sessionID {
+				return nil, fmt.Errorf("session %s disappeared during start", sessionID)
+			}
+			cur.Status = "running"
+			return cur, nil
+		})
+		if err != nil {
+			return mcpmcp.NewToolResultError(err.Error()), nil
 		}
+		committed = true
 
 		// SQLite record (best-effort; session.json is the source of
 		// truth for the hot path).
@@ -286,7 +310,10 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 		// Claim the advance slot atomically under the session lock. If
 		// another advance is already in progress we reject — letting
 		// both proceed would race on the current step index and could
-		// execute the same command step twice or skip one.
+		// execute the same command step twice or skip one. Also
+		// reject if the session is still in the "starting" state,
+		// which means devkit_start has not finished its pre-flight
+		// (branch creation) yet and there is no valid step to run.
 		state, err := lib.UpdateSessionJSON(s.dataDir, func(cur *lib.SessionState) (*lib.SessionState, error) {
 			if cur == nil {
 				return nil, fmt.Errorf("no active session")
@@ -294,6 +321,9 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) {
 			if cur.ID != sessionID {
 				return nil, fmt.Errorf("session mismatch: active is %s", cur.ID)
 			}
+			if cur.Status != "running" {
+				return nil, fmt.Errorf("session %s is %s, not running — wait for devkit_start to finish", cur.ID, cur.Status)
+			}
 			if cur.Busy {
 				return nil, fmt.Errorf("step %s already in progress (another devkit_advance call holds the claim)", cur.CurrentStep)
 			}
@@ -492,19 +522,26 @@ func (s *Server) runCommandWithTimeout(ctx context.Context, command string, stat
 	cmd.Stdout = &out
 	cmd.Stderr = &out
 	err := cmd.Run()
+	// Timeout must be checked BEFORE the exec.ExitError branch —
+	// on Unix, CommandContext kills the process with SIGKILL when
+	// the deadline fires, and that surfaces as an *exec.ExitError
+	// with ExitCode() == -1, NOT a non-ExitError. Previous code
+	// put the ctx.Err() check only in the non-ExitError branch,
+	// so timeouts were reported as "exit code -1" instead of the
+	// promised exit 124 with a clear timeout message.
+	if ctx.Err() == context.DeadlineExceeded {
+		return out.String(), 124, fmt.Errorf("command timed out after %s", timeout)
+	}
 	exitCode := 0
 	if err != nil {
 		var exitErr *exec.ExitError
 		if errors.As(err, &exitErr) {
 			exitCode = exitErr.ExitCode()
 		} else {
-			// Surface whatever the command produced on its combined
-			// stream so the user can see the real cause (missing
-			// binary, permission denied, timeout), not just the Go
-			// error wrapper.
-			if ctx.Err() == context.DeadlineExceeded {
-				return out.String(), 124, fmt.Errorf("command timed out after %s: %w", commandTimeout, err)
-			}
+			// Non-ExitError: startup failure (missing binary,
+			// permission denied). Surface whatever the command
+			// produced on its combined stream so the user sees
+			// the real cause, not just the Go wrapper.
 			return out.String(), 1, fmt.Errorf("command execution failed: %w", err)
 		}
 	}
@@ -512,32 +549,23 @@ func (s *Server) runCommandWithTimeout(ctx context.Context, command string, stat
 }
 
 // commandEnv returns the DEVKIT_INPUT and DEVKIT_OUT_<id> env vars that
-// command steps can read via $DEVKIT_INPUT / $DEVKIT_OUT_<id>.
+// command steps can read via $DEVKIT_INPUT / $DEVKIT_OUT_<id>. Keys are
+// canonicalized via engine.EnvKey; the validator rejects workflows
+// whose IDs would collide under that mapping, so there is no
+// ambiguity about which output wins.
 func commandEnv(state *lib.SessionState) []string {
 	env := []string{"DEVKIT_INPUT=" + state.Input}
 	for id, out := range state.Outputs {
-		env = append(env, "DEVKIT_OUT_"+sanitizeEnvKey(id)+"="+out)
+		env = append(env, "DEVKIT_OUT_"+engine.EnvKey(id)+"="+out)
 	}
 	return env
 }
 
-// sanitizeEnvKey maps a workflow step ID to a valid env var suffix.
-// POSIX allows [A-Za-z_][A-Za-z0-9_]*; step IDs may contain hyphens.
-func sanitizeEnvKey(id string) string {
-	b := make([]byte, 0, len(id))
-	for i := 0; i < len(id); i++ {
-		c := id[i]
-		switch {
-		case c >= 'a' && c <= 'z':
-			b = append(b, c-32) // upper
-		case c >= 'A' && c <= 'Z', c >= '0' && c <= '9', c == '_':
-			b = append(b, c)
-		default:
-			b = append(b, '_')
-		}
-	}
-	return string(b)
-}
+// sanitizeEnvKey is a thin alias to engine.EnvKey for call sites in
+// this package that already reference it by the old name. Both
+// callers (formatStepResponse and commandEnv above) now canonicalize
+// through the same function the validator uses.
+func sanitizeEnvKey(id string) string { return engine.EnvKey(id) }
 
 func (s *Server) handleLoopAdvance(ctx context.Context, wf *engine.Workflow, state *lib.SessionState, step *engine.WfStep, req mcpmcp.CallToolRequest) (*mcpmcp.CallToolResult, error) {
 	// Initialize loop tracking on first call
diff --git a/src/mcp/tools_test.go b/src/mcp/tools_test.go
index e0fb796..101a4f2 100644
--- a/src/mcp/tools_test.go
+++ b/src/mcp/tools_test.go
@@ -1101,6 +1101,61 @@ steps:
 	}
 }
 
+// TestStartConcurrentRace verifies two simultaneous devkit_start calls
+// cannot both succeed. The previous read-then-write pattern let them
+// both see "no session" and both write, silently clobbering.
+func TestStartConcurrentRace(t *testing.T) {
+	wfDir := t.TempDir()
+	dataDir := t.TempDir()
+	writeFile(t, filepath.Join(wfDir, "r.yml"), `name: r
+steps:
+  - id: one
+    prompt: first
+`)
+
+	srv := newTestServer(t, dataDir, wfDir)
+	_, startHandler := srv.startTool()
+
+	const N = 8
+	var wg sync.WaitGroup
+	var successes, alreadyRunning int64
+	start := make(chan struct{})
+	wg.Add(N)
+	for i := 0; i < N; i++ {
+		go func() {
+			defer wg.Done()
+			<-start
+			req := mcpmcp.CallToolRequest{}
+			req.Params.Arguments = map[string]interface{}{"workflow": "r", "input": "x"}
+			result, err := startHandler(context.Background(), req)
+			if err != nil {
+				return
+			}
+			if result.IsError {
+				tc, _ := result.Content[0].(mcpmcp.TextContent)
+				if strings.Contains(tc.Text, "already") {
+					atomic.AddInt64(&alreadyRunning, 1)
+				}
+				return
+			}
+			atomic.AddInt64(&successes, 1)
+		}()
+	}
+	close(start)
+	wg.Wait()
+
+	if successes != 1 {
+		t.Errorf("successes = %d, want 1 (start/start race: claim is not atomic)", successes)
+	}
+	if alreadyRunning != N-1 {
+		t.Errorf("alreadyRunning = %d, want %d", alreadyRunning, N-1)
+	}
+	state, _ := lib.ReadSessionJSON(dataDir)
+	if state == nil || state.Status != "running" {
+		t.Errorf("final state should be running, got %+v", state)
+	}
+}
+
 // TestAdvanceRealRace launches N concurrent devkit_advance calls while
 // the first step (a sleeping command) holds the Busy claim. Under the
 // flock + Busy claim, exactly one must get through; every other racer

From 401906344b1643f3b7ba23ac5c07356f44d6dccf Mon Sep 17 00:00:00 2001
From: Tym Rabchuk <tymrabchuk@gmail.com>
Date: Fri, 10 Apr 2026 10:19:46 -0400
Subject: [PATCH 6/6] fix(ci): silence shellcheck SC2015 in bin/devkit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 bin/devkit | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/bin/devkit b/bin/devkit
index e551d60..9b83266 100755
--- a/bin/devkit
+++ b/bin/devkit
@@ -131,5 +131,7 @@ ensure_engine() {
 
 ENGINE_PATH=""
 ensure_engine
-[ -n "$ENGINE_PATH" ] && [ -x "$ENGINE_PATH" ] || die "engine path not resolved"
+if [ -z "$ENGINE_PATH" ] || [ ! -x "$ENGINE_PATH" ]; then
+    die "engine path not resolved"
+fi
 exec "$ENGINE_PATH" "$@"