refactor(engine): enforce type design for EnforceMode (closes #81) - #83
Merged
Conversation
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.
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.
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.
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.
Closes #81. Follow-up to PR #80. Bundles the three type-design improvements flagged during review because they all touch the same invariant surface.
Summary
EnforceModetype (lib.EnforceMode=hard | soft | "").Workflow,WfStep, andSessionStateall carry the invariant at the type level.IsValid()concentrates the valid-value check;engine.validate()now uses it instead of raw string compares.SessionState.UnmarshalJSONrejects invalid enforce values atReadSessionJSONtime — closes the latent silent-soft fall-through inguard.go's switch from stale/corruptsession.json.cmd/guard.effectiveEnforcedeleted. Every writer now goes throughengine.EffectiveEnforce(PR feat(engine): per-step enforce override + surgical soft-flips (#78) #80) which returns concrete values, andUnmarshalJSONdefends the read path, so the empty-default helper is dead code. Guard readsstate.StepEnforcedirectly.SessionState.Enforce→StepEnforce. Semantics shifted in feat(engine): per-step enforce override + surgical soft-flips (#78) #80 from "workflow's enforce, set at start" to "current step's effective enforce, re-derived on every transition"; the field name now reflects that. JSON tag unchanged (json:"enforce") — no on-disk migration.Layer decision
EnforceModeis defined inlib, notengine, becauseenginealready importslib; inverting that would create a cycle.enginere-exports the type via a type alias and the three constants so call sites inengine/workflow.gostay ergonomic.Definition of done
EnforceModenamed type withIsValid()onWorkflow.Enforce,WfStep.Enforce,SessionState.StepEnforceUnmarshalJSONonSessionStaterejects invalid/missing enforce valuescmd/guard.effectiveEnforcehelper deleted; guard readsstate.StepEnforcedirectlySessionState.Enforcerenamed toStepEnforce(JSON tag unchanged)TestSessionJSONRejectsInvalidEnforcecovers missing/empty/bogus casesgo test -count=1 ./...greengo vet ./...cleanTest plan
go build ./...go test -count=1 ./...— all packages pass (cmd, engine, lib, mcp, runners)go vet ./...ReadSessionJSONrejects{},{"enforce":""},{"enforce":"medium"}with aninvalid enforceerrorStepEnforce: lib.EnforceHardcannot read session statepath instead of the old empty-default fall-throughRefs #78, PR #80.