From a0813177c2546addf0c8688e42b2cfe89ca5386b Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Sat, 11 Apr 2026 13:51:59 -0400 Subject: [PATCH 1/3] refactor(engine): enforce type design for EnforceMode (closes #81) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces a named EnforceMode string type (hard | soft | inherit) in lib so Workflow, WfStep, and SessionState all carry the invariant at the type level instead of checking at runtime. SessionState.Enforce is renamed to StepEnforce to reflect the post-#80 semantics (current step's effective enforce, re-derived on every transition). A new SessionState.UnmarshalJSON rejects stale or corrupt session.json with a missing/invalid enforce value at read time — this closes the latent silent-soft fall-through in guard.go's switch and lets cmd/guard.go drop its now-dead effectiveEnforce helper. - EnforceMode + constants live in lib (engine imports lib, so the type must live below engine to avoid a cycle); engine re-exports via type alias so engine call sites stay ergonomic. - JSON on-disk tag stays "enforce" — no session.json migration. - YAML tag unchanged — workflow authors unaffected. - New test: invalid/missing/bogus enforce in session.json → parse error at ReadSessionJSON time. --- src/cmd/guard.go | 20 +++----- src/cmd/guard_test.go | 105 ++++++++++++++++++++------------------ src/engine/engine_test.go | 20 ++++---- src/engine/workflow.go | 54 +++++++++++++------- src/lib/state_json.go | 70 ++++++++++++++++++++----- src/lib/state_test.go | 49 +++++++++++++++--- src/mcp/tools.go | 30 +++++------ src/mcp/tools_test.go | 70 +++++++++++++------------ 8 files changed, 258 insertions(+), 160 deletions(-) diff --git a/src/cmd/guard.go b/src/cmd/guard.go index 21b15b8..08a74f0 100644 --- a/src/cmd/guard.go +++ b/src/cmd/guard.go @@ -201,17 +201,6 @@ func sessionFileExists(dataDir string) bool { return err == nil } -// effectiveEnforce defaults an empty Enforce field to "hard". The shell -// version relied on python3 .get('enforce','hard'); the fixture matrix -// explicitly asserts that a command step with no enforce field still -// blocks. See hooks_test.sh "missing enforce field" case. -func effectiveEnforce(s *lib.SessionState) string { - if s.Enforce == "" { - return "hard" - } - return s.Enforce -} - // isDevkitMCPTool identifies tools that are part of the devkit MCP // server's own surface — and therefore safe to allow during a command // or prompt step because they drive the engine, not the agent. @@ -319,7 +308,10 @@ func runPreToolGuard() { } tool = t } - enforce := effectiveEnforce(state) + // state.StepEnforce is guaranteed valid ("hard" or "soft") by + // SessionState.UnmarshalJSON — ReadSessionJSON would have rejected + // a stale/corrupt session with a missing or invalid enforce field. + enforce := state.StepEnforce // attemptedTool is what we print in veto diagnostics. An empty // tool name means we failed to parse it from stdin (or got an @@ -332,7 +324,7 @@ func runPreToolGuard() { switch state.StepType { case "command": - if enforce != "hard" { + if enforce != lib.EnforceHard { guardExit(0) return } @@ -347,7 +339,7 @@ func runPreToolGuard() { return case "prompt": - if enforce == "hard" { + if enforce == lib.EnforceHard { if isDevkitMCPTool(tool) { guardExit(0) return diff --git a/src/cmd/guard_test.go b/src/cmd/guard_test.go index 70baab9..a968d49 100644 --- a/src/cmd/guard_test.go +++ b/src/cmd/guard_test.go @@ -146,7 +146,7 @@ func TestGuardPreToolUse(t *testing.T) { session: lib.SessionState{ Status: "done", StepType: "command", - Enforce: "hard", + StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"Bash"}`, @@ -159,7 +159,7 @@ func TestGuardPreToolUse(t *testing.T) { session: lib.SessionState{ Status: "running", StepType: "command", - Enforce: "hard", + StepEnforce: lib.EnforceHard, CurrentStep: "build", Workflow: "feature", TotalSteps: 3, @@ -173,7 +173,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"Write"}`, wantExit: 2, @@ -184,7 +184,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"devkit_advance"}`, wantExit: 0, @@ -194,7 +194,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"mcp__devkit__advance"}`, wantExit: 0, @@ -204,7 +204,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"TodoWrite"}`, wantExit: 0, @@ -218,7 +218,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"Skill"}`, wantExit: 0, @@ -228,7 +228,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "soft", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceSoft, CurrentStep: "build", }, stdin: `{"tool_name":"Bash"}`, wantExit: 0, @@ -238,7 +238,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", }, stdin: `{"tool_name":"Read"}`, wantExit: 0, @@ -248,7 +248,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", }, stdin: `{"tool_name":"Grep"}`, wantExit: 0, @@ -258,7 +258,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", Workflow: "tri-review", TotalSteps: 6, }, stdin: `{"tool_name":"Bash"}`, @@ -270,7 +270,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", Workflow: "tri-review", TotalSteps: 6, }, stdin: `{"tool_name":"Write"}`, @@ -282,7 +282,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", }, stdin: `{"tool_name":"Task"}`, wantExit: 2, @@ -293,7 +293,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", }, stdin: `{"tool_name":"devkit_advance"}`, wantExit: 0, @@ -303,7 +303,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "soft", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceSoft, CurrentStep: "analyse", }, stdin: `{"tool_name":"Bash"}`, wantExit: 0, @@ -313,23 +313,26 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "parallel", Enforce: "hard", CurrentStep: "fanout", + Status: "running", StepType: "parallel", StepEnforce: lib.EnforceHard, CurrentStep: "fanout", }, stdin: `{"tool_name":"Task"}`, wantExit: 0, }, { - // The shell hook relied on python .get('enforce','hard') - // to catch this silent-degrade case. Mirrored via - // effectiveEnforce(). - name: "command step missing enforce → block (default hard)", + // Session files with a missing/empty enforce field are now + // rejected at ReadSessionJSON time by SessionState.UnmarshalJSON + // — the guard never gets a chance to fall through to a + // silent default. Exits with a "cannot read session state" + // error rather than the command-step "BLOCKED" message. + name: "session missing enforce field → parse-reject", dataDir: true, hasSession: true, session: lib.SessionState{ Status: "running", StepType: "command", CurrentStep: "build", }, - stdin: `{"tool_name":"Bash"}`, - wantExit: 2, + stdin: `{"tool_name":"Bash"}`, + wantExit: 2, + wantStderrSubstr: "cannot read session state", }, { // Empty step_type is non-command and non-prompt, so the @@ -339,7 +342,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", }, stdin: `{"tool_name":"Bash"}`, wantExit: 0, @@ -350,7 +353,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: "", toolFlag: "Bash", @@ -364,7 +367,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"mcp__plugin_devkit_devkit-engine__devkit_advance"}`, wantExit: 0, @@ -374,7 +377,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", }, stdin: `{"tool_name":"Glob"}`, wantExit: 0, @@ -384,7 +387,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", }, stdin: `{"tool_name":"Edit"}`, wantExit: 2, @@ -394,7 +397,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", }, stdin: `{"tool_name":"WebFetch"}`, wantExit: 2, @@ -404,7 +407,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "parallel", Enforce: "hard", CurrentStep: "fanout", + Status: "running", StepType: "parallel", StepEnforce: lib.EnforceHard, CurrentStep: "fanout", }, stdin: `{"tool_name":"Write"}`, wantExit: 0, @@ -417,7 +420,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "parallel", Enforce: "soft", CurrentStep: "fanout", + Status: "running", StepType: "parallel", StepEnforce: lib.EnforceSoft, CurrentStep: "fanout", }, stdin: `{"tool_name":"Bash"}`, wantExit: 0, @@ -430,7 +433,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", }, stdin: `{"tool_name":"TodoWrite"}`, wantExit: 0, @@ -443,7 +446,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "analyse", }, stdin: `{"tool_name":"Skill"}`, wantExit: 0, @@ -455,7 +458,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "soft", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceSoft, CurrentStep: "analyse", Workflow: "feature", TotalSteps: 4, }, stdin: `{"tool_name":"Write"}`, @@ -470,7 +473,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "hard", CurrentStep: "unknown", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "unknown", Workflow: "mystery", TotalSteps: 0, }, stdin: `{"tool_name":"Bash"}`, @@ -487,7 +490,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"mcp__plugin_evil_server__devkit_masquerade"}`, wantExit: 2, @@ -500,7 +503,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"mcp__devkit__advance"}`, wantExit: 0, @@ -515,7 +518,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"mcp__plugin_devkit_other_server__probe"}`, wantExit: 2, @@ -529,7 +532,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "RUNNING", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "RUNNING", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"Bash"}`, wantExit: 0, @@ -542,7 +545,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{"tool_name":"devkit_advance"}`, toolFlag: "Bash", @@ -557,7 +560,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: ``, wantExit: 2, @@ -572,7 +575,7 @@ func TestGuardPreToolUse(t *testing.T) { dataDir: true, hasSession: true, session: lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", }, stdin: `{not json`, wantExit: 2, @@ -663,7 +666,7 @@ func TestGuardStaleTTLHonourLongerOverride(t *testing.T) { dir := t.TempDir() t.Setenv("DEVKIT_SESSION_STALE_TTL_SECONDS", "7200") // 2h writeSession(t, dir, lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", Workflow: "feature", TotalSteps: 2, UpdatedAt: time.Now().Add(-45 * time.Minute), }) @@ -734,7 +737,7 @@ func TestGuardStaleSessionPromptSoft(t *testing.T) { dir := t.TempDir() old := time.Now().Add(-2 * time.Hour) writeSession(t, dir, lib.SessionState{ - Status: "running", StepType: "prompt", Enforce: "soft", CurrentStep: "analyse", + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceSoft, CurrentStep: "analyse", Workflow: "tri-review", TotalSteps: 6, UpdatedAt: old, StartedAt: old, }) @@ -770,7 +773,7 @@ func TestGuardPreToolUseStaleSession(t *testing.T) { writeSession(t, dir, lib.SessionState{ Status: "running", StepType: "command", - Enforce: "hard", + StepEnforce: lib.EnforceHard, CurrentStep: "build", Workflow: "feature", UpdatedAt: old, @@ -792,7 +795,7 @@ func TestGuardPreToolUseEnvTTLOverride(t *testing.T) { // Mirrors the DEVKIT_SESSION_STALE_TTL_SECONDS hook-era knob. t.Setenv("DEVKIT_SESSION_STALE_TTL_SECONDS", "1") writeSession(t, dir, lib.SessionState{ - Status: "running", StepType: "command", Enforce: "hard", CurrentStep: "build", + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", UpdatedAt: time.Now().Add(-2 * time.Second), }) env := newGuardTestEnv(t, `{"tool_name":"Bash"}`, "", false, dir) @@ -828,6 +831,7 @@ func TestGuardStopHook(t *testing.T) { hasSession: true, session: lib.SessionState{ Status: "running", Workflow: "test", TotalSteps: 5, CurrentIndex: 2, + StepEnforce: lib.EnforceHard, }, wantDecision: "block", wantReason: "3 steps remaining", @@ -838,6 +842,7 @@ func TestGuardStopHook(t *testing.T) { hasSession: true, session: lib.SessionState{ Status: "done", Workflow: "test", TotalSteps: 5, CurrentIndex: 4, + StepEnforce: lib.EnforceHard, }, wantDecision: "approve", }, @@ -847,6 +852,7 @@ func TestGuardStopHook(t *testing.T) { hasSession: true, session: lib.SessionState{ Status: "failed", Workflow: "test", TotalSteps: 5, CurrentIndex: 2, + StepEnforce: lib.EnforceHard, }, wantDecision: "approve", }, @@ -899,10 +905,11 @@ func TestGuardStopHookStaleSession(t *testing.T) { dir := t.TempDir() old := time.Now().Add(-2 * time.Hour) writeSession(t, dir, lib.SessionState{ - Status: "running", - Workflow: "test", - UpdatedAt: old, - StartedAt: old, + Status: "running", + Workflow: "test", + StepEnforce: lib.EnforceHard, + UpdatedAt: old, + StartedAt: old, }) env := newGuardTestEnv(t, "", "", true, dir) runGuard(guardCmd, nil) diff --git a/src/engine/engine_test.go b/src/engine/engine_test.go index 0e5eafd..54b9871 100644 --- a/src/engine/engine_test.go +++ b/src/engine/engine_test.go @@ -1459,17 +1459,17 @@ steps: func TestEffectiveEnforce(t *testing.T) { tests := []struct { name string - wfField string - stepField string - want string + wfField EnforceMode + stepField EnforceMode + want EnforceMode }{ - {"step soft overrides wf hard", "hard", "soft", "soft"}, - {"step hard overrides wf soft", "soft", "hard", "hard"}, - {"empty step inherits wf soft", "soft", "", "soft"}, - {"empty step inherits wf hard", "hard", "", "hard"}, - {"both zero → default hard", "", "", "hard"}, - {"zero wf + soft step → soft", "", "soft", "soft"}, - {"zero wf + hard step → hard", "", "hard", "hard"}, + {"step soft overrides wf hard", EnforceHard, EnforceSoft, EnforceSoft}, + {"step hard overrides wf soft", EnforceSoft, EnforceHard, EnforceHard}, + {"empty step inherits wf soft", EnforceSoft, EnforceInherit, EnforceSoft}, + {"empty step inherits wf hard", EnforceHard, EnforceInherit, EnforceHard}, + {"both zero → default hard", EnforceInherit, EnforceInherit, EnforceHard}, + {"zero wf + soft step → soft", EnforceInherit, EnforceSoft, EnforceSoft}, + {"zero wf + hard step → hard", EnforceInherit, EnforceHard, EnforceHard}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/src/engine/workflow.go b/src/engine/workflow.go index 5202660..ec3832a 100644 --- a/src/engine/workflow.go +++ b/src/engine/workflow.go @@ -10,18 +10,31 @@ import ( "sort" "strings" + "github.com/5uck1ess/devkit/lib" "gopkg.in/yaml.v3" ) +// EnforceMode aliases lib.EnforceMode so call sites in this package +// don't need a second import. The canonical definition lives in lib +// because SessionState (in lib) also needs it and lib cannot import +// engine (engine already imports lib). +type EnforceMode = lib.EnforceMode + +const ( + EnforceInherit = lib.EnforceInherit + EnforceHard = lib.EnforceHard + EnforceSoft = lib.EnforceSoft +) + // Workflow is the top-level YAML structure. type Workflow struct { - Name string `yaml:"name"` - Description string `yaml:"description"` - Budget Budget `yaml:"budget"` - Steps []WfStep `yaml:"steps"` - Enforce string `yaml:"enforce"` // "hard" (default) | "soft" - BranchMode bool `yaml:"branch"` // create git branch per session - Principles []string `yaml:"principles"` // principle keys to inject + Name string `yaml:"name"` + Description string `yaml:"description"` + Budget Budget `yaml:"budget"` + Steps []WfStep `yaml:"steps"` + Enforce EnforceMode `yaml:"enforce"` // "hard" (default) | "soft" + BranchMode bool `yaml:"branch"` // create git branch per session + Principles []string `yaml:"principles"` // principle keys to inject } // Budget controls token spending limits. @@ -47,24 +60,27 @@ type WfStep struct { // specific steps whose body needs tools the hard mode blocks to // run under soft. The Stop-hook still blocks session end on soft // steps, so end-of-turn drift is still caught. - Enforce string `yaml:"enforce,omitempty"` + Enforce EnforceMode `yaml:"enforce,omitempty"` } // EffectiveEnforce returns the enforcement mode for a step, falling back // to the workflow-level setting when the step does not override it, and -// to "hard" when neither is set. Callers must use this instead of +// to EnforceHard when neither is set. Callers must use this instead of // reading step.Enforce directly so the fall-through is consistent at // every state transition. Takes values (not pointers) so the compiler // enforces that both fields exist at the call site — every current // caller owns concrete structs by the time they reach a transition. -func EffectiveEnforce(wf Workflow, step WfStep) string { - if step.Enforce != "" { +// The return type is guaranteed concrete (never EnforceInherit), so +// callers storing the result into SessionState.StepEnforce can rely on +// the type-level invariant. +func EffectiveEnforce(wf Workflow, step WfStep) EnforceMode { + if step.Enforce != EnforceInherit { return step.Enforce } - if wf.Enforce != "" { + if wf.Enforce != EnforceInherit { return wf.Enforce } - return "hard" + return EnforceHard } // Loop controls step repetition. @@ -109,14 +125,14 @@ func Parse(data []byte) (*Workflow, error) { func validate(wf *Workflow) error { // Apply defaults before validation so directly-constructed Workflow values // (not via Parse) also get sensible defaults. - if wf.Enforce == "" { - wf.Enforce = "hard" + if wf.Enforce == EnforceInherit { + wf.Enforce = EnforceHard } if wf.Name == "" { return fmt.Errorf("workflow missing name") } - if wf.Enforce != "hard" && wf.Enforce != "soft" { + if !wf.Enforce.IsValid() { return fmt.Errorf("workflow %q has invalid enforce %q — must be \"hard\" or \"soft\"", wf.Name, wf.Enforce) } if len(wf.Steps) == 0 { @@ -177,14 +193,14 @@ func validate(wf *Workflow) error { } // Step-level enforce override: empty inherits from workflow, // otherwise must be hard|soft. Reject on command steps — the - // guard honors SessionState.Enforce uniformly (see guard.go's + // guard honors SessionState.StepEnforce uniformly (see guard.go's // command branch), so marking a command step `soft` would let // arbitrary agent tool calls slip through while the engine is // executing that step. Since command steps are engine-owned // and never need per-step overrides, fail loudly at parse time // instead of producing a sharp edge at runtime. - if s.Enforce != "" { - if s.Enforce != "hard" && s.Enforce != "soft" { + if s.Enforce != EnforceInherit { + if !s.Enforce.IsValid() { return fmt.Errorf("step %q has invalid enforce %q — must be \"hard\" or \"soft\"", s.ID, s.Enforce) } if s.Command != "" { diff --git a/src/lib/state_json.go b/src/lib/state_json.go index 25d36de..9b7c54e 100644 --- a/src/lib/state_json.go +++ b/src/lib/state_json.go @@ -8,20 +8,46 @@ import ( "time" ) +// EnforceMode is a typed enum for workflow enforcement mode. Bare string +// was previously checked only in engine.validate(), meaning a stray +// assignment from any package writing to SessionState could silently +// fall through to the "soft" branch of guard.go's switch. The named +// type concentrates the valid-value set in IsValid() and lets writers +// at every layer signal intent at the type level. The YAML and JSON +// wire format is unchanged — gopkg.in/yaml.v3 and encoding/json both +// handle string-aliased types transparently. Defined here (not in +// engine) because engine already imports lib; inverting that would +// create a cycle. engine re-exports this type as a type alias for +// ergonomic call sites. +type EnforceMode string + +const ( + EnforceInherit EnforceMode = "" // step-level only — inherits workflow + EnforceHard EnforceMode = "hard" // default — guard blocks tools mid-step + EnforceSoft EnforceMode = "soft" // allow + nudge; Stop-hook still blocks +) + +// IsValid reports whether m is a concrete enforcement mode. The empty +// value is valid on a step override (inherit) but not on a resolved +// SessionState.StepEnforce — callers in that context should reject "". +func (m EnforceMode) IsValid() bool { + return m == EnforceHard || m == EnforceSoft +} + // 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"` + 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" + StepEnforce EnforceMode `json:"enforce"` + Branch bool `json:"branch"` + BudgetUSD float64 `json:"budget_usd"` + SpentUSD float64 `json:"spent_usd"` + StartedAt time.Time `json:"started_at"` // UpdatedAt is bumped on every WriteSessionJSON. Hooks read this to // detect orphaned sessions (engine crash leaves Status=running but // no process is advancing) and refuse to enforce against them. @@ -36,6 +62,26 @@ type SessionState struct { LoopMax int `json:"loop_max,omitempty"` // max iterations for current loop } +// UnmarshalJSON validates StepEnforce at read time so a stale or +// hand-edited session.json with an invalid/missing enforce value can +// never reach guard.go's switch. Every current writer goes through +// engine.EffectiveEnforce which returns a concrete mode, so this only +// triggers on corrupt or pre-#80 session files — in which case we'd +// rather fail loudly than silently fall through to "soft". Uses a +// type alias to avoid infinite recursion. +func (s *SessionState) UnmarshalJSON(data []byte) error { + type alias SessionState + var a alias + if err := json.Unmarshal(data, &a); err != nil { + return err + } + if !a.StepEnforce.IsValid() { + return fmt.Errorf("session state has invalid enforce %q — must be \"hard\" or \"soft\"", a.StepEnforce) + } + *s = SessionState(a) + return nil +} + // SessionJSONPath returns the path to the hot-state session file. func SessionJSONPath(dataDir string) string { return filepath.Join(dataDir, "session.json") diff --git a/src/lib/state_test.go b/src/lib/state_test.go index cc4eaba..8b9c80e 100644 --- a/src/lib/state_test.go +++ b/src/lib/state_test.go @@ -2,6 +2,8 @@ package lib import ( "os" + "path/filepath" + "strings" "testing" "time" ) @@ -51,7 +53,7 @@ func TestSessionJSON(t *testing.T) { Workflow: "research", CurrentStep: "clarify", StepType: "prompt", - Enforce: "hard", + StepEnforce: EnforceHard, Status: "running", Outputs: map[string]string{}, } @@ -87,12 +89,12 @@ func TestSessionJSON(t *testing.T) { func TestSessionJSONUpdatedAtBumps(t *testing.T) { dir := t.TempDir() state := &SessionState{ - ID: "abc123", - Workflow: "research", - StepType: "prompt", - Enforce: "hard", - Status: "running", - Outputs: map[string]string{}, + ID: "abc123", + Workflow: "research", + StepType: "prompt", + StepEnforce: EnforceHard, + Status: "running", + Outputs: map[string]string{}, } if err := WriteSessionJSON(dir, state); err != nil { @@ -124,3 +126,36 @@ func TestSessionJSONUpdatedAtBumps(t *testing.T) { t.Errorf("UpdatedAt did not advance: first=%v second=%v", first.UpdatedAt, second.UpdatedAt) } } + +// TestSessionJSONRejectsInvalidEnforce verifies that a stale or +// hand-edited session.json with a missing or bogus enforce value is +// rejected at ReadSessionJSON time by SessionState.UnmarshalJSON. This +// is the type-level replacement for guard.go's old effectiveEnforce +// empty-default: rather than silently coercing to "hard" we fail fast +// so the caller can see the corruption. +func TestSessionJSONRejectsInvalidEnforce(t *testing.T) { + cases := []struct { + name string + raw string + }{ + {"missing enforce field", `{"id":"x","status":"running"}`}, + {"empty enforce", `{"id":"x","status":"running","enforce":""}`}, + {"bogus enforce", `{"id":"x","status":"running","enforce":"medium"}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "session.json") + if err := os.WriteFile(path, []byte(tc.raw), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + _, err := ReadSessionJSON(dir) + if err == nil { + t.Fatalf("expected parse error, got nil") + } + if !strings.Contains(err.Error(), "invalid enforce") { + t.Errorf("error = %q, want substring %q", err.Error(), "invalid enforce") + } + }) + } +} diff --git a/src/mcp/tools.go b/src/mcp/tools.go index 157b31b..a5a5f95 100644 --- a/src/mcp/tools.go +++ b/src/mcp/tools.go @@ -76,7 +76,7 @@ func (s *Server) statusTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) { msg := fmt.Sprintf("Workflow: %s\nSession: %s\nStep: %s (%d/%d)\nEnforce: %s\nStatus: %s", state.Workflow, state.ID, state.CurrentStep, state.CurrentIndex+1, state.TotalSteps, - state.Enforce, state.Status) + state.StepEnforce, state.Status) return mcpmcp.NewToolResultText(msg), nil } } @@ -160,14 +160,14 @@ func (s *Server) startTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) { CurrentIndex: 0, TotalSteps: len(wf.Steps), StepType: stepType(firstStep), - // SessionState.Enforce is the current step's effective - // enforce, re-derived on every transition so the hook - // always reads the mode that matches state.CurrentStep. - Enforce: engine.EffectiveEnforce(*wf, firstStep), - Branch: wf.BranchMode, - Status: "starting", - StartedAt: time.Now(), - Outputs: map[string]string{}, + // StepEnforce is the current step's effective enforce, + // re-derived on every transition so the hook always + // reads the mode that matches state.CurrentStep. + StepEnforce: engine.EffectiveEnforce(*wf, firstStep), + Branch: wf.BranchMode, + Status: "starting", + StartedAt: time.Now(), + Outputs: map[string]string{}, }, nil }) if err != nil { @@ -481,7 +481,7 @@ func (s *Server) advanceTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) { state.CurrentStep = nextStep.ID state.CurrentIndex = nextIndex state.StepType = stepType(nextStep) - state.Enforce = engine.EffectiveEnforce(*wf, nextStep) + state.StepEnforce = engine.EffectiveEnforce(*wf, nextStep) state.Busy = false if err := lib.WriteSessionJSON(s.dataDir, state); err != nil { return mcpmcp.NewToolResultError(fmt.Sprintf("write state: %v", err)), nil @@ -646,10 +646,10 @@ func (s *Server) handleLoopAdvance(ctx context.Context, wf *engine.Workflow, sta // Continue loop — return same step for another iteration. // Clear the advance claim as part of this write (see advanceTool). - // state.Enforce is NOT re-derived: loop iterations stay on the same - // step, so the step's effective enforce does not change. If that - // invariant ever breaks (e.g. enforce becomes iteration-dependent), - // recompute here like the other transition sites do. + // state.StepEnforce is NOT re-derived: loop iterations stay on the + // same step, so the step's effective enforce does not change. If + // that invariant ever breaks (e.g. enforce becomes iteration- + // dependent), recompute here like the other transition sites do. state.Busy = false if err := lib.WriteSessionJSON(s.dataDir, state); err != nil { return mcpmcp.NewToolResultError(fmt.Sprintf("write loop state: %v", err)), nil @@ -673,7 +673,7 @@ func (s *Server) advancePastLoop(wf *engine.Workflow, state *lib.SessionState) ( state.CurrentStep = nextStep.ID state.CurrentIndex = nextIndex state.StepType = stepType(nextStep) - state.Enforce = engine.EffectiveEnforce(*wf, nextStep) + state.StepEnforce = engine.EffectiveEnforce(*wf, 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 f543e22..1f7006f 100644 --- a/src/mcp/tools_test.go +++ b/src/mcp/tools_test.go @@ -128,7 +128,7 @@ func TestStatusWithSession(t *testing.T) { CurrentStep: "step-review", CurrentIndex: 2, TotalSteps: 5, - Enforce: "hard", + StepEnforce: lib.EnforceHard, Status: "running", StartedAt: time.Now(), Outputs: map[string]string{}, @@ -257,11 +257,12 @@ steps: // Pre-seed a running session existing := &lib.SessionState{ - ID: "abc123", - Workflow: "review", - Status: "running", - StartedAt: time.Now(), - Outputs: map[string]string{}, + ID: "abc123", + Workflow: "review", + Status: "running", + StepEnforce: lib.EnforceHard, + StartedAt: time.Now(), + Outputs: map[string]string{}, } if err := lib.WriteSessionJSON(dataDir, existing); err != nil { t.Fatalf("write session: %v", err) @@ -563,7 +564,7 @@ steps: } } -// TestAdvancePropagatesStepEnforce verifies that SessionState.Enforce is +// TestAdvancePropagatesStepEnforce verifies that SessionState.StepEnforce is // re-derived from the *current* step on every transition so that a // workflow with mixed per-step enforce correctly flips the hook's // enforcement mode as the workflow walks from step to step. @@ -609,8 +610,8 @@ steps: if state.CurrentStep != "review" { t.Fatalf("expected starting step review, got %s", state.CurrentStep) } - if state.Enforce != "hard" { - t.Errorf("step 1 (review) enforce = %q, want hard (inherited from workflow default)", state.Enforce) + if state.StepEnforce != "hard" { + t.Errorf("step 1 (review) enforce = %q, want hard (inherited from workflow default)", state.StepEnforce) } sessionID := state.ID @@ -640,8 +641,8 @@ steps: if state.CurrentStep != "apply" { t.Fatalf("expected step apply, got %s", state.CurrentStep) } - if state.Enforce != "soft" { - t.Errorf("step 2 (apply) enforce = %q, want soft (per-step override)", state.Enforce) + if state.StepEnforce != "soft" { + t.Errorf("step 2 (apply) enforce = %q, want soft (per-step override)", state.StepEnforce) } advance("apply output") @@ -652,8 +653,8 @@ steps: if state.CurrentStep != "summarize" { t.Fatalf("expected step summarize, got %s", state.CurrentStep) } - if state.Enforce != "hard" { - t.Errorf("step 3 (summarize) enforce = %q, want hard (back to inherited)", state.Enforce) + if state.StepEnforce != "hard" { + t.Errorf("step 3 (summarize) enforce = %q, want hard (back to inherited)", state.StepEnforce) } } @@ -694,8 +695,8 @@ steps: if state == nil { t.Fatal("no session after start") } - if state.Enforce != "soft" { - t.Errorf("step 1 (collect) enforce = %q, want soft (inherited from soft default)", state.Enforce) + if state.StepEnforce != "soft" { + t.Errorf("step 1 (collect) enforce = %q, want soft (inherited from soft default)", state.StepEnforce) } sessionID := state.ID @@ -715,8 +716,8 @@ steps: if state.CurrentStep != "review" { t.Fatalf("expected review, got %s", state.CurrentStep) } - if state.Enforce != "hard" { - t.Errorf("step 2 (review) enforce = %q, want hard (per-step override flips soft→hard)", state.Enforce) + if state.StepEnforce != "hard" { + t.Errorf("step 2 (review) enforce = %q, want hard (per-step override flips soft→hard)", state.StepEnforce) } advance("reviewed") @@ -724,15 +725,15 @@ steps: if state.CurrentStep != "write" { t.Fatalf("expected write, got %s", state.CurrentStep) } - if state.Enforce != "soft" { - t.Errorf("step 3 (write) enforce = %q, want soft (back to inherited default)", state.Enforce) + if state.StepEnforce != "soft" { + t.Errorf("step 3 (write) enforce = %q, want soft (back to inherited default)", state.StepEnforce) } } // TestAdvancePropagatesStepEnforceAfterLoop exercises the // advancePastLoop path: a loop step exits (via max iterations), and the // following step has an explicit per-step enforce override. Without -// state.Enforce re-derivation in advancePastLoop, the post-loop step +// state.StepEnforce re-derivation in advancePastLoop, the post-loop step // would carry the loop step's enforce and the hook would apply the // wrong mode. func TestAdvancePropagatesStepEnforceAfterLoop(t *testing.T) { @@ -766,8 +767,8 @@ steps: if state == nil { t.Fatal("no session after start") } - if state.CurrentStep != "iterate" || state.Enforce != "soft" { - t.Errorf("expected iterate/soft, got %s/%s", state.CurrentStep, state.Enforce) + if state.CurrentStep != "iterate" || state.StepEnforce != "soft" { + t.Errorf("expected iterate/soft, got %s/%s", state.CurrentStep, state.StepEnforce) } sessionID := state.ID @@ -783,8 +784,8 @@ steps: advance() // iteration 1/2 state, _ = lib.ReadSessionJSON(dataDir) - if state.CurrentStep != "iterate" || state.Enforce != "soft" { - t.Errorf("mid-loop iter1: got %s/%s, want iterate/soft", state.CurrentStep, state.Enforce) + if state.CurrentStep != "iterate" || state.StepEnforce != "soft" { + t.Errorf("mid-loop iter1: got %s/%s, want iterate/soft", state.CurrentStep, state.StepEnforce) } advance() // iteration 2/2 — loop hits max, advancePastLoop fires @@ -795,8 +796,8 @@ steps: if state.CurrentStep != "wrapup" { t.Fatalf("expected wrapup after loop exit, got %s", state.CurrentStep) } - if state.Enforce != "hard" { - t.Errorf("wrapup enforce = %q, want hard (wrapup has no override, workflow default is hard — advancePastLoop must re-derive)", state.Enforce) + if state.StepEnforce != "hard" { + t.Errorf("wrapup enforce = %q, want hard (wrapup has no override, workflow default is hard — advancePastLoop must re-derive)", state.StepEnforce) } } @@ -837,8 +838,8 @@ steps: } state, _ := lib.ReadSessionJSON(dataDir) - if state.Enforce != "hard" { - t.Errorf("classify enforce = %q, want hard (inherited)", state.Enforce) + if state.StepEnforce != "hard" { + t.Errorf("classify enforce = %q, want hard (inherited)", state.StepEnforce) } sessionID := state.ID @@ -853,8 +854,8 @@ steps: if state.CurrentStep != "jump-target" { t.Fatalf("expected branch to jump-target, got %s", state.CurrentStep) } - if state.Enforce != "soft" { - t.Errorf("jump-target enforce = %q, want soft (per-step override on branch target, not on skipped step)", state.Enforce) + if state.StepEnforce != "soft" { + t.Errorf("jump-target enforce = %q, want soft (per-step override on branch target, not on skipped step)", state.StepEnforce) } } @@ -1452,7 +1453,7 @@ func TestAdvanceConcurrentClaim(t *testing.T) { CurrentIndex: 0, TotalSteps: 2, StepType: "prompt", - Enforce: "hard", + StepEnforce: lib.EnforceHard, Status: "running", Busy: true, StartedAt: time.Now(), @@ -1765,9 +1766,10 @@ steps: func TestUpdateSessionJSONNoChange(t *testing.T) { dir := t.TempDir() seed := &lib.SessionState{ - ID: "nochange", - Status: "running", - Outputs: map[string]string{"a": "b"}, + ID: "nochange", + Status: "running", + StepEnforce: lib.EnforceHard, + Outputs: map[string]string{"a": "b"}, } if err := lib.WriteSessionJSON(dir, seed); err != nil { t.Fatalf("write: %v", err) From 25d285d8f89fd19e598441bfba89f2f8ad23bffd Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Sat, 11 Apr 2026 14:00:52 -0400 Subject: [PATCH 2/3] =?UTF-8?q?refactor(engine):=20address=20#83=20review?= =?UTF-8?q?=20=E2=80=94=20IsValidOverride,=20doc,=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for PR #83: - Fix stale `effectiveEnforce` comment in guard.go stop-hook region that referenced the helper deleted in the parent commit. - Add `IsValidOverride()` method on EnforceMode collapsing the `!= EnforceInherit && !IsValid()` split-brain check at authoring-time call sites. engine.validate() now uses it. - Expand EnforceInherit doc comment to explicitly state it is an authoring-time sentinel that IsValid() rejects — prevents reader confusion given the dual-use semantics. - Expand TestSessionJSONRejectsInvalidEnforce matrix with case variants (HARD, Hard), whitespace (" hard", "hard "), and non-string (42) to pin the strict-parsing contract. - Add TestEnforceModeIsValid table test directly exercising IsValid and IsValidOverride on boundary inputs — documents the contract so a future "be lenient" refactor must be deliberate. - Add TestGuardPreToolUseMissingEnforceField: end-to-end guard test that a session.json without the enforce field fails closed through UnmarshalJSON rejection in the PreToolUse path. - Add stop-hook parse-reject case to TestGuardStopHook matrix covering the same corruption through the Stop path. --- src/cmd/guard.go | 2 +- src/cmd/guard_test.go | 35 ++++++++++++++++++++++++++ src/engine/workflow.go | 12 ++++----- src/lib/state_json.go | 23 ++++++++++++++--- src/lib/state_test.go | 57 ++++++++++++++++++++++++++++++++++++------ 5 files changed, 110 insertions(+), 19 deletions(-) diff --git a/src/cmd/guard.go b/src/cmd/guard.go index 08a74f0..6950b53 100644 --- a/src/cmd/guard.go +++ b/src/cmd/guard.go @@ -436,7 +436,7 @@ func runStopGuard() { // state == nil handles the TOCTOU where the file was removed // between sessionFileExists and ReadSessionJSON. // Stop is enforce-agnostic — any running workflow blocks Stop - // regardless of soft/hard — so we don't consult effectiveEnforce. + // regardless of soft/hard — so we don't branch on state.StepEnforce. writeStopVerdict(stopVerdict{Decision: "approve"}) guardExit(0) return diff --git a/src/cmd/guard_test.go b/src/cmd/guard_test.go index a968d49..d6677e1 100644 --- a/src/cmd/guard_test.go +++ b/src/cmd/guard_test.go @@ -764,6 +764,28 @@ func TestGuardPreToolUseCorruptSession(t *testing.T) { } } +// TestGuardPreToolUseMissingEnforceField locks in the #81 fix end-to-end: +// a stale session.json missing the enforce field must fail closed through +// SessionState.UnmarshalJSON's rejection, not silently fall through to the +// pre-PR guard.go `effectiveEnforce` empty-default. Without this test, a +// refactor that swallowed the parse error between ReadSessionJSON and +// runPreToolGuard would silently disarm enforcement on stale sessions. +func TestGuardPreToolUseMissingEnforceField(t *testing.T) { + dir := t.TempDir() + writeSessionRaw(t, dir, []byte(`{"id":"x","status":"running","step_type":"command","workflow":"test","current_step":"build"}`)) + env := newGuardTestEnv(t, `{"tool_name":"Bash"}`, "", false, dir) + runGuard(guardCmd, nil) + if env.exit != 2 { + t.Fatalf("missing enforce should fail closed: exit=%d stderr=%s", env.exit, env.stderr.String()) + } + if !strings.Contains(env.stderr.String(), "BLOCKED") { + t.Fatalf("expected BLOCKED diagnostic, got: %s", env.stderr.String()) + } + if !strings.Contains(env.stderr.String(), "cannot read session state") { + t.Fatalf("expected parse-reject path (cannot read session state), got: %s", env.stderr.String()) + } +} + func TestGuardPreToolUseStaleSession(t *testing.T) { dir := t.TempDir() // Stale session: UpdatedAt older than TTL. This is the orphan @@ -863,6 +885,19 @@ func TestGuardStopHook(t *testing.T) { wantDecision: "block", wantReason: "unreadable", }, + { + // A stale session.json without an enforce field must flow + // through SessionState.UnmarshalJSON's rejection and fail + // closed in the stop hook too — not just the pre-tool guard. + // Without this test, a refactor that swallowed the parse + // error anywhere between ReadSessionJSON and runStopGuard + // would silently disarm enforcement on stale sessions. + name: "missing enforce field → block (parse-reject through stop hook)", + dataDir: true, + rawSession: []byte(`{"id":"x","status":"running","workflow":"test"}`), + wantDecision: "block", + wantReason: "unreadable", + }, } for _, tc := range tests { diff --git a/src/engine/workflow.go b/src/engine/workflow.go index ec3832a..05f50d1 100644 --- a/src/engine/workflow.go +++ b/src/engine/workflow.go @@ -199,13 +199,11 @@ func validate(wf *Workflow) error { // executing that step. Since command steps are engine-owned // and never need per-step overrides, fail loudly at parse time // instead of producing a sharp edge at runtime. - if s.Enforce != EnforceInherit { - if !s.Enforce.IsValid() { - return fmt.Errorf("step %q has invalid enforce %q — must be \"hard\" or \"soft\"", s.ID, s.Enforce) - } - if s.Command != "" { - return fmt.Errorf("step %q has enforce on a command step — enforce is only meaningful for prompt steps (the engine executes command steps directly)", s.ID) - } + if !s.Enforce.IsValidOverride() { + return fmt.Errorf("step %q has invalid enforce %q — must be \"hard\" or \"soft\"", s.ID, s.Enforce) + } + if s.Enforce != EnforceInherit && s.Command != "" { + return fmt.Errorf("step %q has enforce on a command step — enforce is only meaningful for prompt steps (the engine executes command steps directly)", s.ID) } if s.Command != "" && s.Loop != nil { return fmt.Errorf("step %q has both command and loop — these are mutually exclusive", s.ID) diff --git a/src/lib/state_json.go b/src/lib/state_json.go index 9b7c54e..2cd4b3f 100644 --- a/src/lib/state_json.go +++ b/src/lib/state_json.go @@ -22,18 +22,33 @@ import ( type EnforceMode string const ( - EnforceInherit EnforceMode = "" // step-level only — inherits workflow + // EnforceInherit is an authoring-time sentinel meaning "no explicit + // mode set at this level." On WfStep.Enforce it inherits from the + // enclosing workflow; on Workflow.Enforce it triggers the default + // (hard) in validate(). It is NEVER a resolved mode — IsValid() + // returns false for it, and SessionState.StepEnforce must never + // hold it once a session is running (enforced by UnmarshalJSON). + EnforceInherit EnforceMode = "" EnforceHard EnforceMode = "hard" // default — guard blocks tools mid-step EnforceSoft EnforceMode = "soft" // allow + nudge; Stop-hook still blocks ) -// IsValid reports whether m is a concrete enforcement mode. The empty -// value is valid on a step override (inherit) but not on a resolved -// SessionState.StepEnforce — callers in that context should reject "". +// IsValid reports whether m is a concrete (resolved) enforcement mode. +// Returns false for EnforceInherit — use IsValidOverride for the +// authoring-time contract that accepts inherit. func (m EnforceMode) IsValid() bool { return m == EnforceHard || m == EnforceSoft } +// IsValidOverride reports whether m is legal as a WfStep or Workflow +// enforce override at authoring time: either an explicit resolved +// mode, or EnforceInherit to fall back to the enclosing level. +// Collapses the "empty-ok" carveout that validate() previously +// spelled out at every call site as `m != EnforceInherit && !m.IsValid()`. +func (m EnforceMode) IsValidOverride() bool { + return m == EnforceInherit || m.IsValid() +} + // SessionState is the hot-path state file read by hooks on every tool call. type SessionState struct { ID string `json:"id"` diff --git a/src/lib/state_test.go b/src/lib/state_test.go index 8b9c80e..db858a9 100644 --- a/src/lib/state_test.go +++ b/src/lib/state_test.go @@ -135,12 +135,25 @@ func TestSessionJSONUpdatedAtBumps(t *testing.T) { // so the caller can see the corruption. func TestSessionJSONRejectsInvalidEnforce(t *testing.T) { cases := []struct { - name string - raw string + name string + raw string + errSubstr string // expected substring; "" means any error is fine }{ - {"missing enforce field", `{"id":"x","status":"running"}`}, - {"empty enforce", `{"id":"x","status":"running","enforce":""}`}, - {"bogus enforce", `{"id":"x","status":"running","enforce":"medium"}`}, + // The type-level gate — these all hit UnmarshalJSON's IsValid() + // check and surface with "invalid enforce" in the error string. + {"missing enforce field", `{"id":"x","status":"running"}`, "invalid enforce"}, + {"empty enforce", `{"id":"x","status":"running","enforce":""}`, "invalid enforce"}, + {"bogus enforce", `{"id":"x","status":"running","enforce":"medium"}`, "invalid enforce"}, + // Case variants — IsValid is strict, no folding. + {"uppercase HARD", `{"id":"x","status":"running","enforce":"HARD"}`, "invalid enforce"}, + {"titlecase Hard", `{"id":"x","status":"running","enforce":"Hard"}`, "invalid enforce"}, + // Whitespace variants — IsValid does not trim. + {"leading space", `{"id":"x","status":"running","enforce":" hard"}`, "invalid enforce"}, + {"trailing space", `{"id":"x","status":"running","enforce":"hard "}`, "invalid enforce"}, + // Non-string — encoding/json surfaces a type mismatch from the + // underlying string alias before our UnmarshalJSON ever runs. + // Still must reject, but the error substring differs. + {"non-string enforce", `{"id":"x","status":"running","enforce":42}`, ""}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -153,8 +166,38 @@ func TestSessionJSONRejectsInvalidEnforce(t *testing.T) { if err == nil { t.Fatalf("expected parse error, got nil") } - if !strings.Contains(err.Error(), "invalid enforce") { - t.Errorf("error = %q, want substring %q", err.Error(), "invalid enforce") + if tc.errSubstr != "" && !strings.Contains(err.Error(), tc.errSubstr) { + t.Errorf("error = %q, want substring %q", err.Error(), tc.errSubstr) + } + }) + } +} + +// TestEnforceModeIsValid pins the IsValid / IsValidOverride contract +// so a future "be lenient" refactor (case folding, trimming, adding a +// third mode) must be a deliberate change rather than an accident. +func TestEnforceModeIsValid(t *testing.T) { + cases := []struct { + mode EnforceMode + valid bool + validOverride bool + }{ + {EnforceHard, true, true}, + {EnforceSoft, true, true}, + {EnforceInherit, false, true}, + {"HARD", false, false}, + {"Hard", false, false}, + {" hard", false, false}, + {"hard ", false, false}, + {"medium", false, false}, + } + for _, tc := range cases { + t.Run(string(tc.mode), func(t *testing.T) { + if got := tc.mode.IsValid(); got != tc.valid { + t.Errorf("IsValid(%q) = %v, want %v", tc.mode, got, tc.valid) + } + if got := tc.mode.IsValidOverride(); got != tc.validOverride { + t.Errorf("IsValidOverride(%q) = %v, want %v", tc.mode, got, tc.validOverride) } }) } From d396ff7961e40a110fd6ec41b3880135702e7a0f Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Sat, 11 Apr 2026 14:05:23 -0400 Subject: [PATCH 3/3] test(hooks): fix shell smoke fixtures for strict enforce parsing The hook_test.sh stop-guard fixtures for done / failed / running workflows were missing the enforce field. Pre-#81 the Python .get('enforce','hard') fallback masked this; post-#81 the Go binary's SessionState.UnmarshalJSON rejects the missing field at parse time and the stop hook fails closed before reaching the status check. - Add enforce:"hard" to the three stop-guard running/done/failed fixtures so the parse succeeds and the status-based routing runs. - Update the PreToolUse "missing enforce field" test comment: it still passes (exits 2), but via the parse-reject fail-closed path rather than the old default-hard coercion. Retitle to reflect the new semantics. --- hooks/hooks_test.sh | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/hooks/hooks_test.sh b/hooks/hooks_test.sh index 08db33f..fd929cb 100644 --- a/hooks/hooks_test.sh +++ b/hooks/hooks_test.sh @@ -551,12 +551,15 @@ 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. +# Valid JSON but missing enforce field — used to silently default to +# "hard" via Python .get() / effectiveEnforce(). Post-#81 the Go binary +# rejects the parse at SessionState.UnmarshalJSON and the guard fails +# closed via the "cannot read session state" path. Exit is still 2 so +# the contract (missing enforce → block) is preserved, but via type- +# level rejection instead of silent coercion. 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)" + 2 "command step with missing enforce field → block (parse-reject)" # Valid JSON missing step_type — should default to empty string, which # is NOT "command", so fall through to allow. This verifies the guard @@ -608,16 +611,19 @@ 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}' \ +# Running workflow → block. Fixtures must include enforce: the Go +# binary's SessionState.UnmarshalJSON rejects session.json with a +# missing/empty enforce at read time (closes #81 silent-soft fall- +# through), so the stop hook fails closed before even reading status. +run_stop_guard '{"status":"running","workflow":"test","enforce":"hard","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}' \ +run_stop_guard '{"status":"done","workflow":"test","enforce":"hard","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}' \ +run_stop_guard '{"status":"failed","workflow":"test","enforce":"hard","total_steps":5,"current_index":2}' \ "approve" "failed workflow → approve" # Corrupt JSON → block (fail closed)