feat(engine): per-step enforce override + surgical soft-flips (#78) - #80
Merged
Conversation
… steps Without an explicit `enforce:` field, the engine defaults to `hard` (src/engine/workflow.go:88-90), and every prompt-only step is classified as "prompt" (src/mcp/tools.go:260-268). Under prompt+hard the guard (src/cmd/guard.go:349-364) only permits Read/Grep/Glob/TodoWrite/ NotebookRead/Skill + devkit MCP tools, which blocks the 7 pr-ready steps whose bodies explicitly require Bash or Edit (validate, necessity, lint, test, doc-check, create-pr, monitor). Soft enforcement still routes through the guard — stderr nudges remain and the Stop-hook still blocks session end until advance is called — it just stops hard-blocking tool calls the step needs. Matches the pattern already used by tri-review.yml and tri-security.yml. Fixes #78
…lows Extends PR #78's quick fix beyond pr-ready.yml after auditing every workflow step body against the prompt+hard allowlist (Read/Grep/Glob/TodoWrite/NotebookRead/Skill + devkit MCP). Flipped to `enforce: soft` — step bodies explicitly require Bash, Edit, Write, WebFetch, WebSearch, AskUserQuestion, or Task: feature implement/gen-tests/run-tests/fix-tests/lint/fix-lint/quick-fix bugfix diagnose/fix/regression-test/run-tests/fix-tests/quick-fix refactor refactor/run-tests/fix-tests self-lint fix (Edit) self-test fix (Edit) self-perf optimize (Edit) self-improve improve (Edit) self-migrate migrate (Edit) self-audit measure-quality/measure-security/measure-git (Bash) audit deps/lint (Bash) autoloop baseline/fix/measure/keep/revert (Bash + Edit) onboard architect (Task) + guide (Write) doc-gen generate (Task) + write (Write) test-gen generate (Task) + run-fix (Bash) research clarify (ask_user) + search-* (WebSearch) + summarize (WebFetch) deep-research same pattern — WebFetch/WebSearch/ask_user throughout Left at default (hard) after verification: tri-debug Read-only diagnosis, no edits/shells tri-dispatch pure reasoning, no tool use (tri-review, tri-security already soft) Verification path: MCP devkit_advance does NOT skip parallel children (src/mcp/tools.go advances CurrentIndex linearly), so each search-* and measure-* prompt body lands as a main-line step and hits the guard. This is unlike engine.go which builds parallelChildren and skips them. That confirms research/deep-research/self-audit need soft. Tests: `go test -count=1 ./...` green. TestParseAllShippedWorkflows already exercises every workflow YAML through the engine's parser and validate path. Refs #78
5uck1ess
changed the title
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
changed the title
…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
Owner
Author
Review round addressedRan Fixed in this commitEngine / correctness
Test coverage (pr-test-analyzer ratings 7-9)
Comment cleanup (comment-analyzer)
Deferred to follow-up PRsThese are valid type-design-analyzer suggestions but touch on-disk format or PR #64 territory, so they warrant separate focused reviews:
Verification
|
6 tasks
5uck1ess
added a commit
that referenced
this pull request
Apr 11, 2026
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.
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #78 surfaced that pr-ready and other workflows can't complete because PR #64's mid-step hard-block applies at workflow granularity. A blanket
enforce: softfix (first round on this branch) works but throws away PR #64's coverage on pure-reasoning steps across the whole surface. This revision adds per-stepenforce:override so the block stays where it's catching drift and only relaxes where the step body literally needs Bash/Edit/Write/WebFetch/Task.Engine changes
engine.WfStep.Enforce— newyaml:"enforce"field, empty inherits workflow defaultengine.EffectiveEnforce(wf, step)— fall-through helper, nil-safevalidate()rejects step-level enforce values other thanhard|soft|""MCP state propagation
SessionState.Enforceis now re-derived on every transition, not just at start. All three transition paths covered:devkit_startusesEffectiveEnforce(wf, &firstStep)advanceToolnormal transition recomputes for the next stepadvancePastLooprecomputes after loop exitWorkflow changes — reverted blanket soft, applied per-step
Counts of soft-flagged prompt steps per workflow (everything else stays hard and retains PR #64's mid-step block):
Net effect: 30+ pure-reasoning steps retain PR #64's mid-step tool block; soft only applies to steps whose body explicitly instructs the agent to run a blocked tool.
Tests added
TestParseStepLevelEnforceOverride— parser acceptsstep.enforceTestParseValidation/invalid_step_enforce— rejects bad values with clear errorTestEffectiveEnforce— nil-safe fall-through matrix (6 table cases + 3 nil edges)TestAdvancePropagatesStepEnforce— MCP state transitions across a workflow with mixed per-step enforce; verifiesSessionState.Enforceflips on each step boundaryTestParseAllShippedWorkflows— all 21 YAMLs parse + validate with the new field (pre-existing test, now exercises per-step enforce across the whole shipped surface)Verification
engine.ParseFile+ValidateTest plan
Closes #78