feat(workflows): add continue_on_error step field for non-halting failures

[doquanghuy]

Description

Closes #2591.

Adds an optional continue_on_error: bool field on every step.
When set to true and the step fails, the engine records the
result (exit_code, stderr, status) into steps.<id>.output and
continues to the next sibling step instead of halting the run.
Downstream if, switch, or gate steps can then branch on
{{ steps.<id>.output.exit_code }} to route the recovery path.

This is the shape @mnriem proposed in the issue discussion —
it composes with primitives that already exist (the exit code
is already captured, the expression engine already resolves it,
and if/switch/gate are already available). The only gap
was that a non-zero exit hard-stopped the pipeline before any
downstream step could evaluate it.

Canonical usage

- id: heavy-thing
  type: command
  integration: claude
  command: speckit.heavy-thing
  continue_on_error: true

- id: check-result
  type: if
  condition: "{{ steps.heavy-thing.output.exit_code != 0 }}"
  then:
    - id: review
      type: gate
      message: "Step failed (exit {{ steps.heavy-thing.output.exit_code }}). Retry or skip?"
      on_reject: skip
  else:
    - id: next-thing
      command: speckit.next-thing

Engine

WorkflowEngine._execute_steps now consults the step config when
a step returns StepStatus.FAILED:

• Gate aborts (output.aborted) always halt the run — operator
  decisions take precedence over the flag.
• Otherwise, if continue_on_error: true, log a
  step_continue_on_error event and proceed to the next sibling.
• Otherwise, behave as before: log step_failed, set
  RunStatus.FAILED, and return.

Exactly one event per failure-resolution path is logged so the
log timeline is unambiguous: either the run continued past the
failure or it halted.

Validation

_validate_steps rejects non-bool values for continue_on_error.
Coerced strings like "true" are not accepted so authoring
mistakes surface at validation time rather than silently
changing run semantics.

Default behaviour preserved

When continue_on_error is omitted, every code path is
byte-equivalent to before this change. Existing workflows see no
difference.

Verdict coverage (from the issue discussion)

Scenario	How
Skip	continue_on_error: true + if branches around the failure
Abort	Omit the flag — today's default halts the run
Retry	continue_on_error: true + gate → operator approves → resume re-runs from gate

Fully unattended retry-on-transient (e.g. retry a 429 at 3 AM
without operator attendance) is intentionally out of scope here.
The skip and abort verdicts work without a human; the
retry verdict still pauses for one at the gate. A future
loop/retry-count primitive or an auto-approving gate type could
close that gap on top of this mechanism without further engine
changes — happy to follow up on that in a separate issue if
useful.

Testing

• Tested locally with uv run specify --help
• Ran existing tests with uv sync && uv run pytest
  → 2967 passed, 35 skipped (was 2960 before; +7 new
  tests added in this PR).
• Tested with a sample project: ran a 3-step workflow where
  the middle step exits non-zero. Without
  continue_on_error, run halts at the failing step (as
  before). With continue_on_error: true, the failing step
  records exit_code and the third step executes. A
  downstream if branching on
  {{ steps.flaky.output.exit_code != 0 }} routes into a
  recovery gate cleanly.

New test coverage

TestContinueOnError in tests/test_workflows.py:

Test	What it locks
test_undeclared_failure_halts_run	Default behaviour byte-equivalent — no flag → run halts on first non-zero exit.
test_declared_and_fired_continues_run	Flag set + step fails → run continues, exit_code recorded.
test_declared_but_step_succeeded_is_noop	Flag set + step succeeds → no behaviour change.
test_if_branch_routes_around_failure	End-to-end recovery pattern from the issue discussion.
test_gate_abort_still_halts_with_continue_on_error	Operator-driven gate abort always halts, even with the flag set.
test_validation_rejects_non_bool_continue_on_error	"true" (string) fails validation.
test_validation_accepts_bool_continue_on_error	true and false pass validation cleanly.

AI Disclosure

• I did not use AI assistance for this contribution
• I did use AI assistance (described below)

Used Claude Opus to draft the engine change, the test suite, the
docs section, and this PR body. The shape (continue_on_error

• exit-code-as-API + branch on it via existing primitives) was
  proposed by @mnriem on the issue thread; this PR implements that
  proposal. Code, tests, and design decisions were human-reviewed
  before submission.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 3

[doquanghuy]

@mnriem — addressed all three Copilot findings in e9a4871, and rebased the branch onto current main (clean rebase, no conflicts).

1. README wording. Rewrote the "Error Handling" intro in terms of StepStatus.FAILED halting by default, with non-zero shell / command exit as one common cause. Avoids implying only exit codes can halt a run (gate aborts and validation failures also do, just via different mechanisms).

2. Validation test contract tightened. test_validation_accepts_bool_continue_on_error now asserts errors == [] instead of "no error mentions continue_on_error". Unrelated validation regressions on the same minimal YAML can no longer slip past this test.

3. Gate stdin patching made runner-robust. In test_gate_abort_still_halts_with_continue_on_error, swapped sys.stdin itself for a stub _TTYStdin object instead of patching sys.stdin.isatty. Method-on-instance assignment is unreliable on real io.TextIOWrapper objects (e.g. under pytest with capture disabled), so replacing the whole stdin object is more robust across runners.

Full suite still passes (continue_on_error: 7/7), no regressions. Branch is MERGEABLE and ready for another look whenever you have a moment.

AI disclosure: drafted with Claude Opus, human-reviewed.

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[doquanghuy]

@mnriem — addressed both new Copilot findings in c827f4f.

1. README runtime-vs-validation framing. Dropped the "step type that fails validation" example from the "Error Handling" intro — structural validation runs at specify workflow run startup and exits before any run is created, so it never produces a StepStatus.FAILED mid-run and was misleading as an example for continue_on_error. Reworded to "any other runtime error raised during step execution" with a parenthetical clarifying that invalid workflows are rejected up-front.

2. Test-block comment about status location. Fixed the misleading "(exit_code, stderr, status)" wording in the section header above TestContinueOnError. status is stored as steps.<id>.status (sibling of output), not nested under steps.<id>.output. Clarified that output carries the exit_code / stderr from the failure and that status is a separate sibling key, so future readers writing expressions against the run state aren't led astray.

Continue_on_error suite still 7/7, no regressions.

AI disclosure: drafted with Claude Opus, human-reviewed.

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 1

[doquanghuy]

@mnriem — addressed the new Copilot finding in 838e7e4.

The gap. Copilot caught a real defense-in-depth hole: _validate_steps rejects non-bool continue_on_error values at validation time, but the engine itself was checking the flag via truthiness (if step_config.get("continue_on_error"):) at runtime. Since WorkflowEngine.execute() does NOT auto-validate the definition (the load_definition docstring explicitly says "not yet validated; call validate_workflow() or engine.validate() separately"), a caller that bypasses validation and feeds the engine a definition with continue_on_error: "true" (string) could silently activate the flag — the YAML string "true" is truthy in Python — which contradicts the documented "literal boolean only" contract.

The fix. Tightened the runtime check at engine.py:661 to identity comparison:

- if step_config.get("continue_on_error"):
+ if step_config.get("continue_on_error") is True:

Only a literal True now enables the behaviour. Validation continues to catch the authoring mistake at parse time; this is the belt-and-braces guard for the bypass-validation path. Inline comment expanded to explain why identity beats truthiness here, with a pointer to the load_definition docstring so future readers don't "simplify" it back to a truthy check.

New test. test_engine_ignores_truthy_non_bool_continue_on_error locks the contract. It bypasses validate_workflow() entirely, feeds continue_on_error: "true" (string) straight through engine.execute(), and asserts the run halts with RunStatus.FAILED and the sibling step does NOT run. Would have failed under the old truthy check.

Continue_on_error suite: 8/8 pass (was 7/7). Full suite: 3002 passed, 40 skipped, no regressions.

AI disclosure: drafted with Claude Opus, human-reviewed.

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[mnriem]

Please address Copilot feedback and resolve conflicts

[doquanghuy]

@mnriem — addressed both Copilot findings and resolved the conflicts in d0b9e00.

1. Wrong method name reference. Both the engine inline comment (engine.py:667) and the test docstring referenced load_definition, but the actual engine API is WorkflowEngine.load_workflow() / WorkflowEngine.execute() — there is no load_definition. Fixed both to reference WorkflowEngine.load_workflow and quote the exact docstring line ("not yet validated; call validate_workflow() or engine.validate() separately") so the pointer is self-checking.

2. Conflict resolution after PR #2664 landed. Upstream main advanced past my branch — notably #2664 merged in c6afe4c, which inserted a new TestContextRunId class at the same anchor point (just before # ===== State Persistence Tests =====) where my TestContinueOnError was sitting. Both classes are independent feature suites and need to coexist.

I rebased the branch onto current main (c6afe4c) and squashed my four iteration commits into a single feat(workflows): add continue_on_error step field commit on top — cleaner history, and it sidesteps the messy three-way conflict that came from re-applying each iteration commit through the same conflict region. New class ordering in tests/test_workflows.py:

TestContextRunId        (upstream, from #2664)
TestContinueOnError     (this PR, 8 tests)
TestRunState            (upstream, unchanged)

Full suite: 3013 passed (was 3009 pre-rebase; +4 from upstream's TestContextRunId + same 8 from TestContinueOnError), 40 skipped, no regressions. Branch is MERGEABLE again.

AI disclosure: drafted with Claude Opus, human-reviewed.

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[mnriem]

Please address Copilot feedback

[doquanghuy]

@mnriem — addressed both Copilot findings in 22c5184 (docs-only, no engine/test changes).

1. Runtime Context section restored. The ### Runtime Context block for {{ context.* }} (added upstream in #2664) was lost during my rebase onto current main — the squash dropped the anchor where it sat. Restored under ## Expressions so context.run_id semantics and the three usage examples are discoverable again.

2. Gate prompt clarified. Reworded the example's message: from "Retry or skip?" to "Approve to continue, or reject to skip the rest of this branch." — the default options: [approve, reject] never offered a retry path, and gates do not auto re-run the failed step. Added an explicit note below the example clarifying that retry requires either custom gate options with downstream branching, or a wrapper loop.

CI was green on the prior commit (11/11 passing). This change is README-only — no Python tests touched.

AI disclosure: drafted with Claude Opus, human-reviewed.

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 1

[doquanghuy]

@mnriem — addressed the new Copilot finding in b8982a7 (docs-only, no engine/test code changes).

The finding. Copilot caught a real overclaim in the README "Error Handling" intro: the text implied continue_on_error covers "any other runtime error raised during step execution", but the engine only consults the flag when a step returns StepResult(status=FAILED, ...). Exceptions raised out of step_impl.execute() propagate to WorkflowEngine.execute(), where the catch-all logs workflow_failed and re-raises — the step result is never recorded, and the flag is never consulted.

Audit (per "fix all similar issues"). Grepped the entire PR diff for the same pattern:

1. workflows/README.md — main fix.

   • Reworded the Error Handling intro to "any step that returns StepResult(status=FAILED, ...)" (removed the "any other runtime error raised during step execution" clause).
   • Added a new "Scope: returned failures only" bullet in the Notes block that names the exception path explicitly: unhandled exceptions are caught by WorkflowEngine.execute(), logged as workflow_failed, and abort the run regardless of continue_on_error. Tells step authors how to bring the flag into scope for exceptional paths — catch the exception internally and return StepResult(status=FAILED, ...) with the failure encoded in output.
   • Promoted the parenthetical structural-validation note ("Invalid workflow definitions are rejected up-front...") out of the main paragraph into the Notes block for parallelism with the other "what the flag does NOT cover" notes.

2. tests/test_workflows.py — the ===== continue_on_error Tests ===== section header read "when an executable step fails and continue_on_error: true is declared", same ambiguity (could be misread to include exception paths). Tightened to "when a step returns StepResult(status=FAILED, ...)" and added a sentence calling out that unhandled exceptions are out of scope.

3. src/specify_cli/workflows/engine.py — audited the inline comments added in d0b9e00: the validator block already says "any step that returns FAILED" (line 235) and the execute path says "lets the pipeline route around the failure" (line 654). Both correctly use returned, not raised. No code-side change needed.

Engine semantics, validation, and test bodies are unchanged. CI: 161/161 tests/test_workflows.py pass locally.

Branch is clean against upstream/main. Ready when you are.

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 1

[doquanghuy]

@mnriem — addressed the new Copilot finding in $(git -C /tmp/spec-kit-pr-2663 rev-parse --short HEAD) (docs-only, no engine/test code changes).

The finding. Copilot caught a real overclaim in the example: the gate message said "reject to skip the rest of this branch" and the explanatory paragraph claimed [approve, reject] map to "continue" vs "skip the rest of this branch". The engine does not implement automatic branch-skipping. Per gate/__init__.py:65-66, on_reject: skip returns StepStatus.COMPLETED; the next sibling step in the then: list runs unconditionally unless the workflow author wires a downstream if reading {{ steps.<gate-id>.output.choice }}.

Two fixes:

1. Restructured the YAML example to actually demonstrate the manual-branching pattern. Added a recover if-step after the gate that conditions on steps.review.output.choice == 'approve'. Now the example shows what the workflow author has to do, instead of implying the engine does it for them.

2. Replaced the trailing paragraph with three precise notes:

   • both gate options return COMPLETED; on_reject: skip controls abort behaviour only, not sibling-skipping
   • all three on_reject values enumerated with their actual engine semantics (abort → FAILED+output.aborted, skip → COMPLETED, retry → PAUSED)
   • the original retry-loop guidance retained as the third bullet

Updated the example gate message to match — "reject to leave the failure recorded and move on" instead of the old overclaim.

Audit (per "fix all similar issues"). Grepped the entire PR diff for the same pattern. No other instance — tests don't carry similar phrasing, and engine.py comments accurately describe the gate semantics already.

Engine semantics, validation, and test bodies are unchanged. CI: 161/161 tests/test_workflows.py pass locally.

[doquanghuy]

@mnriem — proactive audit follow-up in 03454dd (docs + inline-comment only).

After fixing the Copilot finding in 393ac6b I audited the rest of the PR diff for the same class of issue (docs claiming engine behaviour that the code doesn't actually implement). Found three sites with the same minor overclaim:

1. workflows/README.md — intro sentence to the example said "downstream if, switch, or gate steps can branch on it".
2. src/specify_cli/workflows/engine.py:236 — validator inline comment said the same.
3. src/specify_cli/workflows/engine.py:657 — execute-path inline comment said the same.

A gate step doesn't have a condition or expression field. Looking at gate/__init__.py:29,36, the only expressions a gate evaluates are template interpolations in message and show_file for human display. Programmatic branching is if/switch only; a gate's role is to surface a value to the operator, and the operator's output.choice is what a subsequent if/switch then routes on.

Reworded all three sites consistently to: "a downstream if or switch can branch on it (or a gate can surface it to the operator via message interpolation)". The README example already demonstrates this distinction — the gate carries {{ }} template variables in its message, and the recover if-step downstream is what actually branches on the choice.

Engine semantics, validation, and test bodies are unchanged. CI: 161/161 tests/test_workflows.py pass locally.

Branch is clean against upstream/main.