Skip to content

fix(hooks): enforce workflow progression on prompt steps + orphan recovery - #64

Merged
5uck1ess merged 3 commits into
mainfrom
fix/63-workflow-guard-prompt-steps
Apr 11, 2026
Merged

fix(hooks): enforce workflow progression on prompt steps + orphan recovery#64
5uck1ess merged 3 commits into
mainfrom
fix/63-workflow-guard-prompt-steps

Conversation

@5uck1ess

Copy link
Copy Markdown
Owner

Closes #63.

Summary

  • Prompt-step guard: devkit-guard.sh now enforces step progression on prompt steps under enforce: hard, not just command steps. Allows Read/Grep/Glob/TodoWrite/NotebookRead + devkit MCP tools; blocks Write/Edit/Bash/Task/WebFetch/other MCP with a veto naming workflow + step N/M. Soft enforce gets an idempotent stderr nudge instead of a block.
  • Orphan recovery: SessionState.UpdatedAt bumps on every write; sessions idle past 30min are treated as orphaned — hooks stop enforcing against them, and devkit_start reclaims the slot instead of rejecting. No more wedging when the engine crashes mid-workflow.
  • Shared hook helper: new hooks/lib/read-session.sh sourced by both devkit-guard and devkit-stop-guard. Eliminates the duplicate python3 parsing logic and the drift risk that came with it.
  • Timeout hedge: devkit-guard timeout 2s → 5s so python3 cold start on Windows can't silently skip the hook. A proper fix (native Go guard to drop the python3 dep) is tracked in the follow-up issue.

Why

Discovered during PR #62's mega-review when tri-review step 1 returned a prompt and 5/6 steps got silently skipped. The engine behaved correctly (returned the step, waited for advance); the hook layer was the only thing that could actually veto the next drifted tool call, and it only enforced against command steps. This closes that outer loop.

Stop-guard already catches drift at end-of-turn, so the correctness safety net was intact — this PR prevents the token waste that happens when the agent burns thousands of tokens on unrelated work before Stop fires.

Blast radius

1 new bash helper + 2 guard rewrites to use it + 1 hooks.json line + 3 Go file touches + 2 test files + 1 README line. Engine schema gains one field (UpdatedAt), additive. No breaking changes.

Test plan

  • go test ./... — all packages green (new TestSessionJSONUpdatedAtBumps, TestStartReclaimsStaleSession, TestStartRejectsFreshSession)
  • bash hooks/devkit-guard_test.sh — 21/21 fixture cases pass (command/prompt × hard/soft × fresh/stale)
  • Manual: fresh CC window, start tri-review workflow, verify Write/Bash blocked mid-prompt until devkit_advance is called
  • Manual: backdate session.json updated_at past 30min, confirm hook stops enforcing + next devkit_start reclaims cleanly

Closes the drift hole where a workflow prompt step returns its prompt
and the agent can run arbitrary unrelated tools without ever calling
devkit_advance. The engine correctly holds step ordering inside a
workflow; this closes the outer "force the agent to actually use it"
loop at the hook layer.

- devkit-guard.sh: under enforce=hard, prompt steps now allow only
  Read/Grep/Glob/TodoWrite/NotebookRead + devkit MCP tools. Write,
  Edit, Bash, Task, WebFetch, and other MCP tools are blocked with a
  veto naming workflow + step N/M. Soft enforce emits a stderr nudge
  instead of blocking.
- hooks/lib/read-session.sh: shared python3 parser sourced by both
  devkit-guard and devkit-stop-guard (eliminates drift risk from
  duplicate parsing).
- SessionState.UpdatedAt: bumped on every WriteSessionJSON; hooks
  treat sessions idle past 30min TTL as orphaned and stop enforcing.
- devkit_start: reclaims stale sessions instead of rejecting, so a
  crashed engine no longer wedges the slot forever.
- hooks.json: devkit-guard timeout 2s → 5s to hedge python3 cold-start
  on Windows (proper fix is a native Go guard — follow-up issue).
- Tests: Go coverage for UpdatedAt bump + stale/fresh reclaim cutoff;
  new hooks/devkit-guard_test.sh fixture matrix (21 cases) covering
  command/prompt × hard/soft × fresh/stale.
The CI smoke suite asserted prompt+hard+Bash → allow, which is exactly
the drift hole this branch closes. Replace the single old case with
explicit coverage of the new allowlist:

- prompt+hard: Read/Grep/devkit_advance allow; Bash/Write/Task block
- prompt+soft: Bash allowed (nudge-only)
- parallel+hard: Task allowed (engine dispatches)
5uck1ess added a commit that referenced this pull request Apr 11, 2026
Closes #65. The PreToolUse and Stop workflow guards no longer shell out
to python3 on every tool call — all policy lives in a new `guard`
subcommand on the Go engine, and the hooks are thin exec wrappers.

- src/cmd/guard.go: new `devkit-engine guard [--tool-name] [--stop]`
  subcommand. Overrides root PersistentPreRunE so it never needs a git
  repo or the SQLite DB. Reads session.json via lib.ReadSessionJSON
  (flock-safe), mirrors the full policy matrix from PR #64 exactly
  (command/prompt x hard/soft, stale-TTL orphan recovery, corrupt
  session = fail closed), honours DEVKIT_SESSION_STALE_TTL_SECONDS for
  operators who tuned it during the #64 rollout.
- src/cmd/guard_test.go: 27 table-driven cases covering every branch
  the shell fixtures covered, plus explicit corrupt-JSON, stale-TTL,
  and env-override paths. IO and os.Exit are stubbed via package-level
  hooks so tests run in-process.
- hooks/devkit-guard.sh + devkit-stop-guard.sh: reduced to thin
  wrappers that resolve the engine binary (local-dev symlink -> release
  asset -> bin/devkit fallback) and exec it. No python3, no jq, no
  stdin parsing in shell.
- hooks/lib/read-session.sh: deleted. No longer needed.
- hooks/devkit-guard_test.sh: deleted. All cases ported to Go.
- hooks/hooks.json: devkit-guard timeout returned from 5 to 2 — the
  Go path has an order-of-magnitude margin.

Wins verified:
- hooks/hooks_test.sh: 46/46 pass against the Go-backed wrappers.
- go test ./...: all packages green.
- Latency: ~8.5ms per guard invocation (20-run wall clock), vs. the
  50-150ms python3 warm-up the issue cited.
- Cross-compile clean for linux/amd64 and windows/amd64.
Addresses critical findings from tri-review + pr-review-toolkit
mega-review before merge.

Critical fixes:
- hooks/devkit-guard.sh: narrow MCP allowlist from `mcp__*devkit*` to
  `mcp__*devkit-engine*|mcp__devkit__*` so a third-party tool with
  "devkit" in its name cannot sneak through hard enforcement.
- hooks/lib/read-session.sh: align TTL boundary with Go side (>= not >)
  so a session at exactly 30min has the same verdict in hook + start.
- hooks/lib/read-session.sh: catch TypeError (naive datetime) in addition
  to ValueError, so a session file without tzinfo doesn't crash the
  parser and flip the guard into fail-closed mode.
- hooks/lib/read-session.sh: require exactly 8 tab-separated fields from
  the python3 parser — a future refactor dropping a field would have
  silently defaulted SESSION_STALE to empty and started enforcing
  against orphaned sessions.
- hooks/devkit-stop-guard.sh: wrap the final python3 verdict emit with
  a manual-JSON fail-closed fallback. Under `set -e` an unguarded python3
  crash would exit non-zero with no Stop decision on stdout, leaving
  the behaviour undefined.
- hooks/devkit-guard.sh: stderr diagnostic on tool-name parse failure,
  so the BLOCKED message doesn't just say "(attempted tool: )" with
  no hint.
- src/mcp/tools.go: surface stale-session reclaim in the
  NewToolResultText response so the agent immediately sees that the
  previous session's outputs were discarded. Also refuse to reclaim
  a session with BOTH timestamps zero (malformed state file).
- src/lib/state_json.go: document the UpdatedAt side-effect on
  WriteSessionJSON so future callers don't get bitten by the mutation.

Test coverage additions:
- devkit-guard_test.sh: fail-closed corrupt-JSON fixture on the new
  code path.
- devkit-guard_test.sh: stale-command-hard, stale-prompt-soft, and
  legacy-session-without-updated_at fixtures.
- devkit-guard_test.sh: regression guards for narrowed MCP glob
  (blocks foreign devkit-like, allows real devkit-engine).
- src/mcp/tools_test.go: assert agent-visible reclaim notice in
  TestStartReclaimsStaleSession; assert specific error string in
  TestStartRejectsFreshSession so an unrelated error can't pass
  silently.

Test matrix: 27 devkit-guard fixtures + 46 hooks_test.sh smoke +
Go unit tests all green.
@5uck1ess
5uck1ess merged commit 5be0143 into main Apr 11, 2026
6 checks passed
@5uck1ess
5uck1ess deleted the fix/63-workflow-guard-prompt-steps branch April 11, 2026 04:28
5uck1ess added a commit that referenced this pull request Apr 11, 2026
Replaces python3-based parsing in devkit-guard.sh / devkit-stop-guard.sh
with a new `devkit-engine guard [--tool-name] [--stop]` Cobra subcommand.
Policy is unchanged; the substrate moves to Go for portability, speed,
and testability.

Why
---
- Windows / minimal containers often lack python3. The old hooks hard-
  blocked every tool call on those hosts (fail-closed on python3
  unavailable) or silently skipped enforcement.
- Python3 cold-start was 50-150 ms per hook invocation on every tool
  call, compounding in high-churn workflow steps.
- Bash + python3 + jq split policy across three languages. A Go
  subcommand lets the engine and the guard share the exact same
  SessionState parser (lib.ReadSessionJSON), eliminating drift.

Native subcommand (src/cmd/guard.go, +398 lines)
-------------------------------------------------
- New `devkit-engine guard` command. Overrides rootCmd.PersistentPreRunE
  to a no-op so the guard never requires a git repo, never opens the
  SQLite DB, and never fails on hosts without .git. Cobra lets a child
  command shadow the parent's persistent pre-run entirely.
- Hot path (no active workflow): single read-only os.Stat, zero writes.
  sessionFileExists runs BEFORE lib.ReadSessionJSON so we skip the
  withSessionLock mkdir + session.json.lock create side effects on
  every PreToolUse call where the user has no running session.
- Any error after a positive sessionFileExists result fails CLOSED
  unconditionally — permission errors, quota, lock-acquire failures,
  parse errors all return a BLOCKED diagnostic pointing at the file.
  Silently fail-open on permission errors would let a broken plugin
  data dir disarm the guard with zero user-visible signal.
- Policy matrix mirrors PR #64 exactly:
    command + hard → only devkit MCP + TodoWrite
    prompt  + hard → read-only evidence tools + devkit MCP
    prompt  + soft → allow with stderr nudge
    parallel       → allow (engine is dispatching)
    stale session  → allow with stderr warning (orphan recovery)
- isDevkitMCPTool is anchored on the full plugin+server prefix
  (mcp__plugin_devkit_devkit-engine__) plus the short-form
  mcp__devkit__ namespace. This is tighter than PR #64's shell glob
  (mcp__*devkit-engine*) and much tighter than the original
  mcp__*devkit* substring — even a hypothetical second MCP server
  under the devkit plugin cannot silently inherit command-step
  permissions.
- effectiveEnforce defaults empty Enforce to "hard", mirroring the
  shell hook's python .get('enforce','hard') so a schema-drift gap
  can't silently disarm enforcement.
- sessionIsStale falls back UpdatedAt → StartedAt → "fresh", but also
  logs a one-line WARNING when both timestamps are zero so a wedged
  session leaves a debuggable trail.
- staleTTL honours DEVKIT_SESSION_STALE_TTL_SECONDS. Whitespace is
  trimmed (TrimSpace) so copy-paste trailing-space doesn't bite;
  non-numeric / non-positive values log a warning and fall back to
  default rather than silently degrading.
- readToolNameFromStdin returns (string, error) so the three failure
  modes (read error / empty / parse error) can be logged distinctly.
  Empty tool name still falls through to default-deny under hard
  enforcement, so the security posture is unchanged; only the
  diagnostic improves.
- Block diagnostics substitute "<unknown>" when the tool name is
  empty, so log readers don't see a dangling "(attempted tool: )".
- --stop mode emits Stop-hook JSON verdict on stdout (no trailing
  newline, matching the shell printf '%s' output byte-for-byte).
  writeStopVerdict panics on the unreachable json.Marshal failure
  path instead of silently writing a hardcoded fallback — any future
  field addition that breaks marshalling trips CI.
- Broken stdout in writeStopVerdict now logs to stderr so a pipe
  failure leaves some post-mortem trail.
- guardCmd declares Args: cobra.NoArgs so extra positional args fail
  loudly in development rather than being silently dropped.

Thin shell wrappers (hooks/devkit-guard.sh, hooks/devkit-stop-guard.sh)
----------------------------------------------------------------------
Both scripts reduce to binary-resolution + exec:
  1. $CLAUDE_PLUGIN_ROOT/bin/devkit-engine (local-dev symlink)
  2. $CLAUDE_PLUGIN_ROOT/bin/devkit-engine-v* (shipped release asset)
- shopt -s nullglob so an unmatched glob expands to nothing instead
  of iterating once with the literal pattern string.
- When multiple versioned binaries coexist, pick the highest-sorted
  executable match. The naive "first glob match" would pick v2.1.0
  over v2.1.10 lexicographically.
- The bin/devkit first-run-download fallback has been DELIBERATELY
  removed from the hook path. Downloading release assets from a
  time-limited hook is unsafe (fail-open on timeout), and a fresh
  install should fail closed with a diagnostic pointing at
  `devkit install` rather than silently blocking on a network call.
- When no binary is found, emit a LOUD stderr diagnostic naming the
  search path and instructing `devkit install` — then allow (guard)
  or approve (stop-guard). A broken install should trip the user's
  attention on first tool call rather than silently disarming
  enforcement.
- No python3, no jq, no subshell parsing. Stdin (the PreToolUse JSON
  payload) passes straight through exec to the Go binary.

hooks/hooks.json
----------------
Timeout for devkit-guard.sh and devkit-stop-guard.sh raised from 2s
to 10s. The Go binary clears the old 8.5ms budget by three orders of
magnitude under warm conditions, but 10s gives room for:
- macOS Gatekeeper quarantine scan on first exec
- Windows Defender cold scan
- Network filesystem exec stall
- Cold Go runtime init on large binaries

Deleted
-------
- hooks/lib/read-session.sh — python3 session parser, superseded.
- hooks/devkit-guard_test.sh — shell fixture matrix, ported to
  src/cmd/guard_test.go as 40+ table-driven cases.

Test coverage (src/cmd/guard_test.go, +800 lines)
-------------------------------------------------
- 30+ table-driven rows covering the full policy matrix including
  fixture-parity gaps from the deleted shell test: prompt+hard+TodoWrite,
  prompt+soft+Write, parallel+soft+Bash.
- Schema-drift pins: Status="RUNNING" uppercase case-sensitivity,
  TotalSteps=0 label fallback, --tool-name flag vs stdin precedence,
  empty stdin / malformed stdin under command+hard.
- wantStderrSubstr field on the table pins veto-message wording so
  any regression in the block diagnostic fails the suite.
- Allowlist bypass negatives:
    mcp__plugin_evil_server__devkit_masquerade → block
    mcp__plugin_devkit_other_server__probe    → block (tightening
      beyond PR #64's shell glob)
  Positive cases:
    mcp__plugin_devkit_devkit-engine__devkit_advance → allow
    mcp__devkit__advance → allow (short-form)
- DEVKIT_SESSION_STALE_TTL_SECONDS garbage matrix (6 subtests):
  non-numeric, negative, zero, trailing-space (trimmed), empty,
  integer overflow.
- Longer TTL override (7200s + 45min-old session stays fresh).
- Zero-timestamp warning pinned (session with no UpdatedAt/StartedAt
  logs anomaly and still enforces).
- Unreadable session file (mode 0o000, Unix-only) fails closed with
  BLOCKED diagnostic.
- Stale session under prompt+soft (previously only command+hard was
  covered).
- Top-of-file comment documents the t.Parallel() prohibition: the
  test helper mutates package-level IO globals, and parallelism
  would race. Follow-up refactor to a guardContext struct would
  enable parallel tests.

Test coverage (hooks/hooks_test.sh, +105 lines)
-----------------------------------------------
- CLAUDE_PLUGIN_ROOT unset → disabled + exit 0 / approve.
- Empty bin/ directory → loud warning + allow / approve.
  (The "fresh clone before first build" scenario — previously
  completely untested.)
- Versioned binary only (no local-dev symlink) → exec with guard arg.
- Multiple versioned binaries coexisting → pick the highest-sorted
  executable. Pins B1's contract against future refactors.

Verification
------------
- go test ./... — all packages green (40+ guard cases, 6 stale-TTL
  subtests, 5 dedicated scenario tests).
- hooks/hooks_test.sh — 52/52 pass, up from 46.
- Latency: ~8.5 ms per guard invocation over a 20-run wall clock
  (vs. 50-150 ms python3 cold start), with empty $CLAUDE_PLUGIN_DATA
  verified after 20 runs — no session.json.lock leaked as a side
  effect on the no-workflow hot path.
- Cross-compile clean: linux/amd64, darwin/arm64, windows/amd64.

Rebased onto main after PR #64 + 2.1.8 version bump. PR #64's engine-
side changes (stale-session reclaim notice in tools.go, UpdatedAt
side-effect doc in state_json.go) are inherited from main unchanged.
5uck1ess added a commit that referenced this pull request Apr 11, 2026
Closes the "all-or-nothing" problem introduced by PR #64's prompt+hard
block. Each workflow step can now declare `enforce: soft` individually
so mid-step hard-block stays where it's doing useful work (pure
reasoning steps) and only relaxes where the step body explicitly needs
Bash/Edit/Write/WebFetch/Task.

Engine:
  - WfStep.Enforce (`yaml:"enforce"`) — empty inherits workflow default
  - engine.EffectiveEnforce(wf, step) — fall-through helper, nil-safe
  - validate() rejects step-level enforce values other than
    hard|soft|"" with a clear error

MCP state propagation:
  - SessionState.Enforce is now re-derived on every transition:
      1. devkit_start uses EffectiveEnforce(wf, &firstStep)
      2. advanceTool normal transition recomputes for nextStep
      3. advancePastLoop recomputes after loop exit
  - Loop iterations stay on the same step, so intra-loop state is
    unchanged (already correct).

Workflows — reverted blanket `enforce: soft` from PR #80's first round
and applied per-step overrides only where the step body requires a
blocked tool. Pure-reasoning steps now go back to hard:

  pr-ready: 8 soft, security stays hard (read-only review)
  feature: 8 soft, triage/brainstorm/plan/review-smart/review-fast hard
  bugfix: 8 soft, triage hard
  refactor: 3 soft, analyze/plan/comparison hard
  self-lint/test/perf/improve/migrate: 1 soft each (fix step), summary hard
  self-audit: 3 soft (measure-*), detect/analyze/synthesize hard
  audit: 2 soft (deps/lint), detect (command)/report hard
  autoloop: 6 soft (baseline/fix/measure/keep/revert/report), audit/compare hard
  onboard: 2 soft (architect/guide), analyze hard
  doc-gen: 2 soft (generate/write), analyze hard
  test-gen: 2 soft (generate/run-fix), analyze/report hard
  research: 6 soft (clarify/search-*/summarize/follow-up), decompose/synthesize hard
  deep-research: 8 soft (clarify/perspectives/search-*/extract-claims/disconfirm/self-critique),
                 decompose/hypotheses/evidence-matrix/synthesize hard
  tri-review: 1 soft (gather), 3 reviews + consolidate hard
  tri-security: 1 soft (gather), 3 audits + consolidate hard
  tri-debug, tri-dispatch: fully hard (already correct)

Net effect: PR #64's mid-step tool block retains coverage on 30+
pure-reasoning steps across the workflow surface; soft only applies to
steps whose body explicitly instructs the agent to run a blocked tool.

Tests:
  - TestParseStepLevelEnforceOverride — parser accepts step.enforce
  - TestParseValidation/invalid_step_enforce — rejects bad values
  - TestEffectiveEnforce — nil-safe fall-through matrix
  - TestAdvancePropagatesStepEnforce — MCP state transitions flip
    enforce as the workflow walks mixed steps
  - TestParseAllShippedWorkflows — all 21 YAMLs parse + validate
  - `go test -count=1 ./...` green (cmd/engine/lib/mcp/runners)

Refs #78
5uck1ess added a commit that referenced this pull request Apr 11, 2026
…mmand steps, close test gaps

PR #80 review round. Aggregated feedback from code-reviewer,
pr-test-analyzer, silent-failure-hunter, comment-analyzer, and
type-design-analyzer. Addressed the high-severity items in-scope for
this PR; deferred the named-type / guard-helper-dedup / state-field-
rename refactors to follow-ups since they touch on-disk format or
cross PR #64's territory.

Engine:
  - EffectiveEnforce now takes values (Workflow, WfStep) instead of
    pointers. All current callers own concrete structs by the time
    they reach a transition, so the nil-safety branches were
    aspirational — dropping them makes the compiler enforce presence
    and removes a silent "nil,nil → hard" fall-through that would
    hide programming errors.
  - validate() now rejects `enforce:` on command steps. The previous
    comment claimed it was a no-op on command steps, but guard.go's
    command branch uniformly consults SessionState.Enforce, so
    marking a command step `soft` would let agent tools slip through
    while the engine is executing it. Fail loudly at parse time.
  - WfStep.Enforce YAML tag now `omitempty` so round-trips don't
    serialize empty strings.
  - Comment on WfStep.Enforce no longer hardcodes the guard tool
    list (would rot if guard.go gains or drops a tool).

MCP state propagation:
  - All three call sites (startTool, advanceTool, advancePastLoop)
    updated to pass *wf / step by value. No behavior change.
  - Added a comment at handleLoopAdvance's continue path explaining
    why state.Enforce is NOT re-derived mid-loop (iterations stay
    on the same step, so effective enforce is stable).

Tests:
  - TestEffectiveEnforce: reworked for value-based signature; nil
    edge cases replaced with zero-value cases (which the compiler
    now constructs safely).
  - TestParseValidation: new case `enforce on command step` asserts
    parse-time rejection.
  - TestAdvancePropagatesStepEnforceReverseOverride: NEW.
    Soft-default workflow with a hard-override step; asserts the
    state flips soft→hard→soft as the workflow walks. This is the
    symmetric case to the original test and was the biggest missing
    end-to-end gap (previously only unit-tested via EffectiveEnforce).
  - TestAdvancePropagatesStepEnforceAfterLoop: NEW. A loop step
    with `enforce: soft` hits its max, and advancePastLoop must
    re-derive enforce for the post-loop step (which inherits the
    workflow default hard). Before this test, advancePastLoop's
    state.Enforce write had zero coverage — a regression on
    tools.go:674 would not have been caught by the suite.
  - TestAdvancePropagatesStepEnforceAcrossBranch: NEW. A step
    with a `branch:` clause jumps past a sequential step with a
    different enforce to the target. Guards against a future
    refactor computing enforce from CurrentIndex+1 instead of the
    branch target.

YAML comment cleanup (comment-analyzer feedback):
  - pr-ready.yml validate: "runs git status / git log" → "inspects
    branch state via git" (was over-claiming specific subcommands).
  - pr-ready.yml doc-check: "commits docs edits when mechanical" →
    "edits doc files (and commits them)" (the blocker is Edit, not
    the commit).
  - research.yml / deep-research.yml search-*: removed redundant
    "# WebSearch" annotations (the step body "Execute web search
    for..." is self-evident; the comment was pure WHAT).
  - audit.yml deps/lint: removed tool-list enumerations (rot-prone
    when new ecosystems added; body already lists them).
  - autoloop.yml measure: removed "re-runs the metric command"
    (restated the first line of the prompt body verbatim).
  - bugfix.yml reproduce: fake-quoted "run the failing case" was
    not in the step body; rewrote as "may run the failing case to
    confirm the bug".

Deferred to follow-up PRs (tracked in PR body):
  - Named EnforceMode type across Workflow / WfStep / SessionState
    (touches src/lib/state_json.go JSON on-disk format).
  - Collapse guard.go's parallel effectiveEnforce helper now that
    state.Enforce is always concrete post-transition (PR #64
    territory — warrants its own review).
  - Rename SessionState.Enforce → StepEnforce to signal the
    semantic shift from workflow-scoped to step-scoped.

Refs #78
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PreToolUse hook to enforce devkit workflow step progression

1 participant