Skip to content

fix: route sync output_invalid through _fail_run - #13

Open
rickgorman wants to merge 1 commit into
zackham:masterfrom
rickgorman:fix/sync-output-invalid-uses-fail-run
Open

fix: route sync output_invalid through _fail_run#13
rickgorman wants to merge 1 commit into
zackham:masterfrom
rickgorman:fix/sync-output-invalid-uses-fail-run

Conversation

@rickgorman

Copy link
Copy Markdown

Summary

Sync executor.start() results that fail declared-output validation (artifact is empty / missing fields) currently mark the run FAILED and call _halt_job immediately. The async tick path already routes the same failure through _fail_run(..., error_category=\"output_invalid\"), which is what lets exit rules retry.

This made a loop on output_invalid fire for async executors and never fire for sync ones (script / callable). Arena hits this on stepwise-run --local --wait with a script invoke that must honor output contracts.

Reproduce

git clone https://github.com/zackham/stepwise.git
cd stepwise
uv sync
uv run pytest tests/test_engine.py::TestSyncOutputInvalidExitRules -q

On current master (this PR's parent):

FAILED tests/test_engine.py::TestSyncOutputInvalidExitRules::test_missing_output_retries_via_exit_rule
AssertionError: expected retry-then-complete, got failed; calls=1 runs=['failed']

The test is a callable step that returns {} on attempt 1 (declared output status missing) and {\"status\": \"ok\"} on retry, with an exit rule:

ExitRule("retry_invalid", "field_match", {
    "field": "error_category",
    "value": "output_invalid",
    "action": "loop",
    "target": "emit",
    "max_iterations": 5,
}, priority=10)

With this change the same test completes after two runs: FAILED (output_invalid) then COMPLETED.

Minimal CLI equivalent (same engine path as --local --wait):

# /tmp/invalid-then-ok.yaml
name: invalid-then-ok
steps:
  emit:
    executor: script
    run: |
      python3 -c 'import os,json,pathlib; p=pathlib.Path("/tmp/sw-attempt"); n=int(p.read_text()) if p.exists() else 0; p.write_text(str(n+1)); print("{}" if n==0 else json.dumps({"status":"ok"}))'
    outputs: [status]
    exit:
      - name: retry_invalid
        type: field_match
        priority: 10
        config:
          field: error_category
          value: output_invalid
          action: loop
          target: emit
          max_iterations: 5
rm -f /tmp/sw-attempt
uv run stepwise run --local --wait /tmp/invalid-then-ok.yaml; echo exit=$?

Unpatched: job fails after one empty artifact. Patched: retries and completes.

Change

In _process_launch_result for result.type == "data", when _validate_artifact / _check_artifact_size fails, call _fail_run with error_category="output_invalid" instead of _halt_job. Same as the async poll path a few hundred lines above.

No exit-rule change; jobs without a matching rule still halt via _fail_run's existing fallthrough.

The async tick path already retries missing declared outputs via
_fail_run + exit rules. The sync _process_launch_result path marked
FAILED and halted the job immediately, so a loop-on-output_invalid
rule never fired. Match the async path.

Reproduce: uv run pytest tests/test_engine.py::TestSyncOutputInvalidExitRules -q
On unpatched master that test fails with job FAILED after one empty artifact.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant