Follow-up from PR #80 review. Three related type-design improvements around the `enforce` mode surface that were deferred out of scope to keep PR #80 reviewable. Grouping them here because they all touch the same invariant surface and should probably land together.
Context
PR #80 added per-step `enforce:` override (`WfStep.Enforce`) and made `SessionState.Enforce` re-derive on every transition via a new `engine.EffectiveEnforce(wf, step)` helper. That closed the functional gap, but type-design-analyzer flagged three residual issues during review. None are blockers; #1 is the only one with latent runtime risk.
1. Named `EnforceMode` type (latent runtime risk — low but real)
Today: `Enforce` is a bare `string` on `Workflow`, `WfStep`, and `SessionState`. Valid values `{"", "hard", "soft"}` are enforced at runtime by `validate()` only.
Risk: `src/cmd/guard.go:208-213`'s switch only matches `"hard"`/`"soft"`. If any Go writer ever puts an unexpected string on `state.Enforce`, the guard silently falls through to the soft branch and mis-enforces. No test catches this because every current writer goes through `engine.EffectiveEnforce` which returns concrete values. Safe today, but one stray assignment away from a silent failure.
Fix:
```go
// src/engine/workflow.go
type EnforceMode string
const (
EnforceInherit EnforceMode = "" // step-level only; workflow default
EnforceHard EnforceMode = "hard"
EnforceSoft EnforceMode = "soft"
)
func (e EnforceMode) IsValid() bool { ... }
```
- Use on `Workflow.Enforce`, `WfStep.Enforce`, `SessionState.Enforce`
- Add `UnmarshalJSON` on `SessionState` to reject invalid values at read time (defends against stale/corrupt session.json)
- `validate()` calls `.IsValid()` instead of raw string compare
- No YAML behavior change (yaml.v3 handles string-aliased types transparently)
- No on-disk JSON format change — tag stays `json:"enforce"`
Blast radius: ~15 files, mostly test fixtures. Not trivial, but contained.
2. Collapse `guard.go`'s duplicate `effectiveEnforce` helper (cosmetic)
Today:
- `engine.EffectiveEnforce(wf Workflow, step WfStep) string` — the new one, exhaustive
- `cmd/guard.go:208-213` `effectiveEnforce(s *lib.SessionState) string` — pre-existing, defaults empty → `"hard"`
Why `guard.go`'s version existed: before PR #80, `state.Enforce` was set once from `wf.Enforce` which could be empty. The guard needed the fallback.
Why it's now dead code: after PR #80, every transition (`startTool`, `advanceTool`, `advancePastLoop`) writes via `EffectiveEnforce`, which never returns `""`. The guard's empty-check is unreachable.
Fix: delete `cmd/guard.effectiveEnforce` and have the guard read `state.Enforce` directly. Update the call sites in `guard.go` to inline the read. Update `guard_test.go` cases that asserted the empty-default behavior (they now test dead code).
Severity: zero runtime impact. Code smell only. Worth doing because a future reader of `guard.go` would otherwise preserve the dead branch on refactor.
3. Rename `SessionState.Enforce` → `StepEnforce` (cosmetic)
Today: `SessionState.Enforce` semantics shifted in PR #80 from "workflow's enforce, set at start" to "current step's effective enforce, re-derived on every transition." Field name didn't change. The shift is load-bearing (the whole point of PR #80) but invisible at the type level.
Fix: Rename the Go field to `StepEnforce`. Keep the JSON tag as `json:"enforce"` for on-disk compatibility — no format change, no migration needed.
Blast radius: ~15 files. Pure mechanical rename. Bundles cleanly with #1 since both touch `state_json.go`.
Why group them
Out of scope
Definition of done
Refs #78, PR #80
Follow-up from PR #80 review. Three related type-design improvements around the `enforce` mode surface that were deferred out of scope to keep PR #80 reviewable. Grouping them here because they all touch the same invariant surface and should probably land together.
Context
PR #80 added per-step `enforce:` override (`WfStep.Enforce`) and made `SessionState.Enforce` re-derive on every transition via a new `engine.EffectiveEnforce(wf, step)` helper. That closed the functional gap, but type-design-analyzer flagged three residual issues during review. None are blockers; #1 is the only one with latent runtime risk.
1. Named `EnforceMode` type (latent runtime risk — low but real)
Today: `Enforce` is a bare `string` on `Workflow`, `WfStep`, and `SessionState`. Valid values `{"", "hard", "soft"}` are enforced at runtime by `validate()` only.
Risk: `src/cmd/guard.go:208-213`'s switch only matches `"hard"`/`"soft"`. If any Go writer ever puts an unexpected string on `state.Enforce`, the guard silently falls through to the soft branch and mis-enforces. No test catches this because every current writer goes through `engine.EffectiveEnforce` which returns concrete values. Safe today, but one stray assignment away from a silent failure.
Fix:
```go
// src/engine/workflow.go
type EnforceMode string
const (
EnforceInherit EnforceMode = "" // step-level only; workflow default
EnforceHard EnforceMode = "hard"
EnforceSoft EnforceMode = "soft"
)
func (e EnforceMode) IsValid() bool { ... }
```
Blast radius: ~15 files, mostly test fixtures. Not trivial, but contained.
2. Collapse `guard.go`'s duplicate `effectiveEnforce` helper (cosmetic)
Today:
Why `guard.go`'s version existed: before PR #80, `state.Enforce` was set once from `wf.Enforce` which could be empty. The guard needed the fallback.
Why it's now dead code: after PR #80, every transition (`startTool`, `advanceTool`, `advancePastLoop`) writes via `EffectiveEnforce`, which never returns `""`. The guard's empty-check is unreachable.
Fix: delete `cmd/guard.effectiveEnforce` and have the guard read `state.Enforce` directly. Update the call sites in `guard.go` to inline the read. Update `guard_test.go` cases that asserted the empty-default behavior (they now test dead code).
Severity: zero runtime impact. Code smell only. Worth doing because a future reader of `guard.go` would otherwise preserve the dead branch on refactor.
3. Rename `SessionState.Enforce` → `StepEnforce` (cosmetic)
Today: `SessionState.Enforce` semantics shifted in PR #80 from "workflow's enforce, set at start" to "current step's effective enforce, re-derived on every transition." Field name didn't change. The shift is load-bearing (the whole point of PR #80) but invisible at the type level.
Fix: Rename the Go field to `StepEnforce`. Keep the JSON tag as `json:"enforce"` for on-disk compatibility — no format change, no migration needed.
Blast radius: ~15 files. Pure mechanical rename. Bundles cleanly with #1 since both touch `state_json.go`.
Why group them
Out of scope
Definition of done
Refs #78, PR #80