MCP engine: deterministic workflow enforcement via tool scoping + hooks - #52
Merged
Conversation
Extends Workflow struct with Enforce (hard/soft), BranchMode, and Principles fields; adds per-step Principles override to WfStep. Default for Enforce is "hard", applied in validate() so directly- constructed Workflow values also normalize correctly.
Introduces SessionState struct with atomic write (tmp+rename) for race-free reads by PreToolUse hooks on every tool call.
Move all tool methods from server.go stubs into tools.go. Replace workflow_list/workflow_status stubs with real devkit_list and devkit_status implementations. Add tests for both tools covering happy path, missing dir, parse errors, and no-session state.
Replaces startTool stub with full implementation: checks for running session, finds/parses workflow YAML, writes session.json, creates SQLite record, optionally creates git branch, and returns the first step prompt with interpolated input and injected principles. Adds path traversal guard on workflow name. Adds TestStart and TestStartAlreadyRunning.
Replaces handleLoopAdvance stub with full loop tracking: per-iteration state (LoopIteration/LoopMax on SessionState), gate command evaluation, until-string detection, and max-iteration enforcement. Fixes path traversal gap in advanceTool where state.Workflow (from YAML name field) was used in filepath.Join without the abs-path guard already applied at start time. Three new tests cover max-iteration rollover, gate pass, and gate fail cases.
Add devkit-guard.sh that reads session.json on every tool call and hard-blocks Bash/Edit/Write/Read/Glob/Grep/Agent during active command steps (exit 2). Register as matcher:"*" PreToolUse hook with 2s timeout in hooks.json.
Adds make install-plugin to build natively and place the binary in bin/ (plugin root), so Claude Code auto-adds it to PATH. Also adds bin/ to .gitignore to prevent committing platform binaries.
Replace ensure-engine.sh bootstrap + devkit workflow run with devkit_start/devkit_advance MCP tool calls in all 8 skill/command files. Remove scripts/ensure-engine.sh and scripts/install-engine.sh — binary now ships in bin/ via plugin.
Critical fixes: - Shell hooks: use sys.argv[1] instead of string interpolation (injection fix) - Shell hooks: fail closed (exit 2) when python3 unavailable - Shell hooks: single python3 call instead of 4 per invocation - Guard hook: block all standard tools during command steps (not just 7) - Check all WriteSessionJSON/ClearSessionJSON errors (5 unchecked calls) - Check ReadSessionJSON error in startTool (was swallowed with _) - Check DB CreateSession/UpdateSessionStatus errors Important fixes: - Add Server.Close() for DB resource cleanup, defer in cmd/mcp.go - Use StdioServer.Listen(ctx) for graceful shutdown (was ignoring ctx) - Store validated filename in state.Workflow (not YAML name field) - Add 5-minute command timeout via context.WithTimeout - Add bounds check on state.CurrentIndex before array access - Extract completeWorkflow() to deduplicate advance/advancePastLoop - Validate non-empty paths in NewServer - Fix stale CONTRIBUTING.md references - Stop hook: output valid JSON via python3 json.dumps New tests: - Path traversal rejection (4 cases) - Nonexistent workflow name - Advance with no active session - expect:success with failing command - expect:failure with passing command - Loop until condition (stay + exit)
- README: rewrite hook table (10→12), architecture diagram, add How It Works section - ROADMAP: add MCP engine entry at top of implemented list - CHANGELOG: full 2.1.0 entry covering PR #52 architectural shift - GitHub about section: updated description, added mcp/mcp-server topics
Removes pre-MCP-conversion duplicate code. The YAML engine + MCP server
already implement all loop capabilities (max, gate, until) via the engine
and mcp.handleLoopAdvance. These Go implementations were unreachable
legacy from before the deterministic workflow conversion.
Deleted:
- src/loops/ entire package (improve, feature, bugfix, refactor, review,
testgen, dispatch) — 2,651 lines
- src/cmd/{improve,feature,bugfix,refactor,review,testgen,dispatch,resume}.go
— 631 lines of CLI shims that called loops.*
- Total: 3,282 lines
Kept:
- src/engine/ — YAML workflow engine (loops live here)
- src/mcp/ — MCP server with handleLoopAdvance
- src/runners/ — still used by engine for Codex/Gemini terminal fallback
- 'devkit workflow run <name>' — single entry point for all workflows
- 'devkit mcp' — MCP server mode
- 'devkit status' — session history
The old command described a pattern where Claude manually walked YAML
files step-by-step. Now the devkit engine controls execution via MCP
tools (devkit_start, devkit_advance, devkit_list). The command is now
the generic entry point for all 18 YAML workflows that don't have
dedicated skills (feature, bugfix, refactor, self-*, audit, etc.).
Verified all commands still work:
- /tri:{review,debug,security} → devkit_start(tri-*)
- /devkit:pr-ready → devkit_start(pr-ready)
- /devkit:pr-monitor → standalone (no engine dep)
- /devkit:status → devkit_status + shell
- /devkit:setup-rules → pure shell
- /devkit:workflow → generic entry for any YAML workflow
- research, deep-research, autoloop skills → devkit_start
- test-gen, doc-gen, changelog, onboard, scrape, adr → self-contained
pr-monitor was a manual-only command — nothing chained to it after pr-ready, so users had to explicitly invoke it (and rarely did). Changes: - Add 'monitor' step to workflows/pr-ready.yml as the final loop step (max 10 iterations, until 'all resolved'). Handles CI waiting, comment classification, fix/reply cycles, re-review requests. - Delete commands/pr-monitor.md (standalone command removed) - Delete commands/pr-ready.md (command form removed) - Add skills/pr-ready/SKILL.md — auto-activates on 'submit a PR', 'create a pull request', 'ship this', 'open a PR', etc. - Update README: 6 commands (was 7), 20 skills (was 19) - Update ROADMAP to reflect new counts and rationale Commands are now reserved for things needing explicit invocation: tri-* (CLI detection), setup-rules (one-time), status (manual check), workflow (generic runner). Everything else is a skill.
- README: remove /devkit:pr-ready slash command reference in Quick Start (contradicted line 134 which says it's now a skill), fix ast-grep comment (no such workflow 'repo-map'), update pr-gate hook description, remove stale src/loops/ entry in repo tree - CONTRIBUTING.md: 8→6 command count - commands/status.md: update example table row to say 'pr-ready skill' - hooks/pr-gate.sh: update prompt text to reference pr-ready skill (not slash command) since /devkit:pr-ready no longer exists - Delete docs/superpowers/specs/ — policy violation (specs belong in homebase, not public repo); the file is already in homebase
- Delete orphan lib files (similarity, metric) — only used by removed
loops/ package. Remove HandoffPath/WriteHandoff from state.go and their
tests for the same reason.
- Delete stale src/TODO.md — described architecture from before MCP engine
rewrite (loops/, devkit improve/review/dispatch).
- Update cmd/status.go wording to match current command surface.
- pr-ready.yml monitor step — fix silent failures from review:
- create-pr step now emits `PR: <number>` or `PR: FAILED <reason>` on
last line; monitor parses it in STEP 0 and bails cleanly if missing.
- gh API failures retry once, then halt with a terminal `all resolved
(gh api unreachable ...)` message instead of misreading empty fetches
as success.
- Check state classification is explicit: PENDING/QUEUED is never
treated as resolved; completion requires PENDING_CHECKS=0 AND
FAILED_CHECKS=0 AND REMAINING=0.
- Stuck detection now emits the engine's terminal `all resolved`
string (so loop actually stops) with a diagnostic suffix.
- Loop-exhausted observability note — engine halts with last iter
output preserved in session log.
# Conflicts: # .claude-plugin/plugin.json
5uck1ess
added a commit
that referenced
this pull request
Apr 10, 2026
Ships the critical fixes a full mega-pr review on #52 uncovered, beyond the binary-shipping bug already addressed in the first commit on this branch. All findings from silent-failure-hunter and code-reviewer marked CRITICAL or HIGH are addressed here. CRITICAL — shell injection in command steps runCommand interpolated {{input}} and {{step-id}} into `sh -c <cmd>`. Any LLM-chosen input or contaminated prior-step output executed shell. Command and gate strings are now literal YAML; values are passed via $DEVKIT_INPUT and $DEVKIT_OUT_<step_id> env vars. The engine validator rejects {{...}} in command/gate strings at parse time with an explicit "shell injection mitigation" error. Both engine.runCommand and mcp.runCommand were affected; both fixed. The 5 self-* workflows (self-test, self-lint, self-improve, self-perf, self-migrate) were updated to use $DEVKIT_INPUT. Regression tests: TestRunWorkflowCommandRejectsInterpolation, TestRunWorkflowCommandEnvInput, TestRunWorkflowCommandEnvPriorOutput. CRITICAL — PreToolUse guard bypassable hooks/devkit-guard.sh blocklist matched 11 hardcoded tool names and missed Task, SlashCommand, TodoWrite, BashOutput, any mcp__* tool, and every future tool Claude Code adds. Claude could trivially drive a command step by calling Task (subagent) or any non-listed tool. Fixed by inverting to an allowlist: during command steps only mcp__devkit* and TodoWrite are permitted; everything else returns exit 2 with a clear diagnostic including the attempted tool name. CRITICAL — stop-guard and guard had opposite fail-closed policies devkit-stop-guard approved on corrupt session.json; devkit-guard blocked. A corrupt state file mid-workflow would silently end the session. Both now fail closed with a diagnostic and handle the TOCTOU race between the -f test and python open() by treating FileNotFoundError as "no session" (matching pre-check intent). HIGH — CLAUDE_PLUGIN_DATA unset silent no-op Both guard hooks exited 0 with no output when the env var was missing. Combined with the binary-shipping bug, enforcement was an invisible no-op. Added stderr warnings so the degraded state is observable. HIGH — loop until / branch when substring false-positives strings.Contains("no failures found", "fail") returned true, silently terminating loops or routing branches incorrectly. Same bug in EvalBranch. Both now use word-boundary matching (grep -w semantics) via a new containsWord helper: the sentinel must be bounded on both sides by a non-alphanumeric/underscore character. Accepts idiomatic forms like "TINY: short fix" and "attempt 2: ALL_PASSING" while rejecting accidental substrings. Regression tests cover both directions. HIGH — completeWorkflow swallowed git.CommitAll errors A failed final commit was logged to stderr only, so the user saw "WORKFLOW COMPLETE" with uncommitted branch work. Now collected into a warnings list and surfaced in the MCP response body, alongside any DB update or state-clear failures. HIGH — runCommand discarded stdout/stderr on non-ExitError failures When the error wasn't an *exec.ExitError (missing binary, permission denied, ctx cancelled), the captured combined stream was thrown away. Now returned alongside the wrapped error so users see the actual cause. Also distinguishes context.DeadlineExceeded (returns exit 124 + "timed out after 5m") from generic execution failure. MEDIUM — YAML typos silently ignored yaml.Unmarshal accepted unknown fields, so "commnd: foo" (missing e) parsed as a no-op step. Now uses NewDecoder(bytes).KnownFields(true); typos fail loudly at parse time. MEDIUM — make sync-version clobbered manual bumps Triggering any make target on a feature branch rewrote plugin.json to the latest git tag, silently undoing manual version bumps. Now only writes when the git tag is strictly higher than the current plugin.json version (semver ordering via sort -V). MEDIUM — pr-gate.sh violated project shell rules Missing `set -euo pipefail`, used /tmp/devkit-pr-gate-done shared across concurrent projects, and arithmetic fallback that silently skipped the gate on any failure. Now strict mode, per-user cooldown file under $XDG_CACHE_HOME/devkit, and explicit numeric-regex guard on the timestamp comparison. Stale docs - src/cmd/status.go, README.md, CONTRIBUTING.md, CHANGELOG.md — replaced `devkit workflow run <name>` with the correct `devkit workflow <name> "<description>"`. There is no `run` subcommand; src/cmd/workflow.go defines `Use: "workflow [name] ..."`. - README.md architecture block — reframed the "binary ships in bin/" claim to describe the wrapper + download flow introduced by this PR. - workflows/pr-ready.yml description — updated to reflect all 8 steps (validate, necessity, lint, test, security, changelog, create-pr, monitor) rather than the stale 5-step list from before pr-monitor was folded in. - hooks/hooks.json top-level description — now mentions the workflow guard and completion guard hooks that were silently added in 2.1.0. - ROADMAP.md — 19 → 22 skills (stale count from before the Playwright skills were added in PR #53). - CHANGELOG.md — new 2.1.2 section documenting every fix in this PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the broken subprocess-spawning engine with an MCP server that enforces deterministic workflow execution inside Claude Code.
src/mcp/) — Go binary speaks JSON-RPC over stdio, exposes 4 tools:devkit_start,devkit_advance,devkit_status,devkit_list. Registered inplugin.jsonviamcpServers.session.json, hard-blocks (exit 2) out-of-step actions during command stepsskills/_principles.yml) — ~120 tokens of DRY/YAGNI/clean-code rules injected per step instead of full ~800 token skill filesWhy
The old engine spawned
claude -pas a subprocess. Claude Code's OAuth token doesn't work for subprocess calls, so the primary runner was broken. All "deterministic" workflows were running via markdown fallback — Claude followed them voluntarily. This PR makes step skipping structurally impossible.Enforcement layers
Token budget
~17k tokens for an 8-step workflow vs ~50k+ with the old monolithic approach (~65% reduction).
Stats
src/mcp/package with full test coverage (integration + unit)scripts/ensure-engine.shandscripts/install-engine.sh(binary ships inbin/, auto-PATH)Test plan
cd src && go test ./... -race -count=1— all passingmake install-pluginbuilds binary tobin/devkitecho '{"jsonrpc":"2.0","method":"initialize","id":1,"params":{"capabilities":{}}}' | bin/devkit mcpreturns valid JSON-RPCecho '{"tool_name":"Bash"}' | CLAUDE_PLUGIN_DATA=/tmp/test bash hooks/devkit-guard.shexits 0 (no session)