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/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 `. 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_` 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 ` or context-activated skills +- **24 → 8 slash commands** — 16 commands deleted, logic now in YAML workflows invoked via `devkit workflow ""` 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--, + │ 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_ + │ │ 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 ): +Terminal usage (devkit workflow ""): └── 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/bin/devkit b/bin/devkit new file mode 100755 index 0000000..9b83266 --- /dev/null +++ b/bin/devkit @@ -0,0 +1,137 @@ +#!/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})…" + + # 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}" + 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 -rf "$tmp_dir" + trap - EXIT INT TERM + + log "installed engine at $ENGINE_PATH" +} + +ENGINE_PATH="" +ensure_engine +if [ -z "$ENGINE_PATH" ] || [ ! -x "$ENGINE_PATH" ]; then + die "engine path not resolved" +fi +exec "$ENGINE_PATH" "$@" 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/hooks_test.sh b/hooks/hooks_test.sh index d65d068..06774b4 100644 --- a/hooks/hooks_test.sh +++ b/hooks/hooks_test.sh @@ -150,6 +150,182 @@ 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) ===" + +# 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=$? + if [[ "$exit_code" -eq "$want_exit" ]]; then + pass "devkit-guard: $label" + else + fail "devkit-guard: $label (exit $exit_code, want $want_exit)" + fi +} + +# 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) +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 + +# 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) +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=$? +if [[ $corrupt_exit -eq 2 ]]; then + pass "devkit-guard: corrupt JSON → block" +else + fail "devkit-guard: corrupt JSON (exit $corrupt_exit, want 2)" +fi + +# 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) ===" + +# Helper: run stop-guard and capture JSON output +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) + 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) +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 + +# 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) +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 + pass "devkit-stop-guard: corrupt JSON → block (fail closed)" +else + fail "devkit-stop-guard: corrupt JSON (got: $out)" +fi + echo "" echo "=========================================" echo "Results: $PASS passed, $FAIL failed" 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/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--) @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/ 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 ` to start one, or use the MCP tools inside Claude Code.") + fmt.Println("No sessions found. Run `devkit workflow \"\"` 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..8eec7c0 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_) 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,31 @@ 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_ 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 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) { *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 +306,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_). + output, exitCode, err := e.runCommand(ctx, step.Command, input, outputs) if err != nil { dbStep.Status = "failed" dbStep.ChangeSummary = err.Error() @@ -405,10 +424,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 +493,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..87950a5 100644 --- a/src/engine/engine_test.go +++ b/src/engine/engine_test.go @@ -243,6 +243,23 @@ 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"}, + {"env key collision", `name: T +steps: + - id: fetch-data + prompt: x + - id: fetch_data + prompt: y`, "collide under env key"}, } for _, tt := range tests { @@ -845,7 +862,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 +873,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 +882,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_. + 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_ 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 +1262,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) @@ -1343,3 +1414,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 4a55a58..33c1bbe 100644 --- a/src/engine/workflow.go +++ b/src/engine/workflow.go @@ -4,8 +4,10 @@ package engine import ( + "bytes" "fmt" "os" + "sort" "strings" "gopkg.in/yaml.v3" @@ -63,10 +65,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 { @@ -99,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) @@ -108,12 +115,35 @@ 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) + // 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) } - 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) + 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 + // otherwise parse fine and silently do nothing. + modes := 0 + if s.Prompt != "" { + modes++ + } + 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) @@ -127,11 +157,28 @@ 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_) 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_ 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_ instead (shell injection mitigation)", s.ID) + } } // 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) } @@ -153,23 +200,114 @@ func (wf *Workflow) Validate() error { return validate(wf) } +// EnvKey maps a workflow step ID to a POSIX env var suffix used in +// DEVKIT_OUT_. 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. 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 } // 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/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..f954b81 100644 --- a/src/lib/state_json.go +++ b/src/lib/state_json.go @@ -10,22 +10,26 @@ import ( // 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 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 } // SessionJSONPath returns the path to the hot-state session file. @@ -33,45 +37,147 @@ 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. 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) } + 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() + // 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) + } + defer unlockFile(f) + return fn() +} + +// 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) + }) +} + +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 { + // 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) + } + 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 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 { + 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/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 f5498e0..f2b60af 100644 --- a/src/mcp/tools.go +++ b/src/mcp/tools.go @@ -20,6 +20,10 @@ import ( // commandTimeout is the maximum duration for workflow command execution. const commandTimeout = 5 * time.Minute +// 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) { tool := mcpmcp.NewTool("devkit_list", mcpmcp.WithDescription("List available workflows"), @@ -76,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 @@ -113,29 +108,82 @@ 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 + // 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, 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 + } - 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{}, + // 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. + 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 + } } - 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 + // 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, @@ -147,14 +195,6 @@ func (s *Server) startTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) { } } - // Git branch if configured - if wf.BranchMode && s.git != 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) - } - } - // Build response with first step + principles response := s.formatStepResponse(wf, state, &firstStep, input) return mcpmcp.NewToolResultText(response), nil @@ -201,9 +241,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) } @@ -263,17 +307,51 @@ 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. 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") + } + 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) + } + 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 - } - if state.ID != sessionID { - return mcpmcp.NewToolResultError(fmt.Sprintf("session mismatch: active is %s", state.ID)), nil + return mcpmcp.NewToolResultError(err.Error()), nil } + // 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 + }); 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) if err != nil { @@ -291,10 +369,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_ 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 } @@ -318,7 +399,12 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) { } } - // Handle loop steps + // 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 { return s.handleLoopAdvance(ctx, wf, state, ¤tStep, req) } @@ -342,14 +428,25 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) { } if nextIndex >= len(wf.Steps) { + // 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 + // Write next step state. Clear the claim as part of this same + // 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 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 } @@ -360,49 +457,116 @@ 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)) } - return mcpmcp.NewToolResultText(fmt.Sprintf("=== WORKFLOW COMPLETE ===\nSession: %s\nSteps completed: %d", state.ID, state.TotalSteps)), nil + + 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(b.String()), nil } -func (s *Server) runCommand(ctx context.Context, command string) (string, int, error) { - ctx, cancel := context.WithTimeout(ctx, commandTimeout) +// 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) { + 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) cmd.Dir = s.repoRoot + cmd.Env = append(os.Environ(), commandEnv(state)...) var out bytes.Buffer 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 { - return "", 1, fmt.Errorf("command execution failed: %w", 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) } } return out.String(), exitCode, nil } +// commandEnv returns the DEVKIT_INPUT and DEVKIT_OUT_ env vars that +// command steps can read via $DEVKIT_INPUT / $DEVKIT_OUT_. 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_"+engine.EnvKey(id)+"="+out) + } + return env +} + +// 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 if state.LoopMax == 0 { @@ -413,12 +577,14 @@ 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_. + // Gates get a shorter independent timeout so a wedged gate cannot + // eat the full command budget. if step.Loop.Gate != "" { - gateCmd := engine.Interpolate(step.Loop.Gate, state.Input, state.Outputs) - _, exitCode, err := s.runCommand(ctx, gateCmd) + gateOut, exitCode, err := s.runCommandWithTimeout(ctx, step.Loop.Gate, state, gateTimeout) 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 +593,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) } } @@ -441,7 +608,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 } @@ -464,6 +633,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 faa2ff0..101a4f2 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" @@ -934,11 +936,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 +950,429 @@ 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) + } +} + +// 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) + } +} + +// 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 +// 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) { + 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() 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