Skip to content

Keep command diagnostics from changing breaker categories - #128

Merged
swerner merged 2 commits into
mainfrom
codex/command-failure-classifier
Oct 19, 2026
Merged

swerner merged 2 commits into
mainfrom
codex/command-failure-classifier

Conversation

@swerner

@swerner swerner commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Command diagnostics such as order.py:502:5, lib.rs:500:13, test_timeout, and budget could make a normal nonzero command exit look transient or budget-exhausted to the circuit breaker. That bypassed deterministic signature counting and could admit a transient-only restart edge.

Make ReferenceClassifier treat the structured exit_status:<code> class as deterministic before inspecting message hints. Keep the full failure reason, stdout, and command.output intact. Structured timeout/cancellation handling, fallback infrastructure hints, and hash/signature normalization are unchanged. The Attractor format documentation records this boundary.

Refs #126 and fabro-sh/fabro#876.

Reproduction and coverage

Confirmed on main at 50809f9bd2847d20f047c3d5ffc6692fbc2f803d: CommandStep appends output to its reason and sets exit_status:<code>; Stage::into_outcome preserves both; the host installs the reference circuit breaker, which previously scanned the whole message.

Before the fix, the new classifier test classified a line-500 diagnostic as transient. Real shell commands emitting a line-502 diagnostic bypassed the three-repeat breaker and took restart edges until max_visits=4 stopped them. After the fix, repeated failures stop at three, restart edges are blocked on the first failure, and captured diagnostics remain available. These exercise representative compiler/linter output through real commands, without requiring Ruff or Clippy as the failing command.

Validation

  • cargo nextest run --locked -p petri-execution -p petri --lib --test controls, with the pinned host/Docker plugin paths: 45 passed.
  • CARGO_PROFILE_DEV_DEBUG=0 CARGO_INCREMENTAL=0 mise run check: passed. Includes formatting, workspace Clippy with warnings denied, 1,765 tests, 3 doctests, the release build, workflow/whitespace/bundle checks, and the isolated remote-Docker artifact test.
  • The workspace run reported 9 skipped tests and one LEAK warning for the existing petri-driver::resume::resume_reacquires_held_scopes test. A targeted rerun with the same workspace/all-targets/all-features configuration passed without that warning.
  • External GitHub/Fabro corpora and the live Daytona tier were not provisioned; their normal local gates remain in effect. The extended nightly gate was not run.

Boundary and downstream adoption

An exit-status failure now stays deterministic even when its captured output quotes a real-looking network error. Infrastructure failures without that class still use the existing hints; explicit timeouts/cancellation retain priority. This does not change public failure-class vocabulary, retry eligibility, timeout retry allowance, or sandbox architecture.

Fabro main currently locks Petri to 56ec3849ca693974ff778aa5ad4f9f9410d47d29. After this lands, Fabro must update its Petri Git dependency lock to a revision containing the fix, rebuild, and verify its own fabro-petri command workflow path: ordinary versus 50x-line diagnostics, test_timeout/budget, three-repeat signature blocking, refusal of transient-only restart edges, preserved output, and timeout/cancellation/infrastructure controls. Leave Fabro #876 open until adoption and that downstream verification are complete; this PR does not modify or validate the Fabro application itself.

swerner and others added 2 commits October 16, 2026 16:09
Add FailureClass::is_exit_status so the breaker no longer copies the
"exit_status:" prefix, and note the departure from the reference
heuristics in the breaker's module docs.

Trim the unit tests to the cases the change adds, run one workflow per
integration test instead of five, replace the restart flag with two
direct tests, and share a blocked_reason helper across the breaker tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@swerner

swerner commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Simplify pass

Cleanup pass for reuse, simplification, efficiency, and fix depth, applied in 3188c94. The behavior is unchanged.

Fixed

  • Typed class check. Added FailureClass::is_exit_status() next to the exit_status(code) constructor, and the classifier calls it instead of starts_with("exit_status:"). The prefix now lives in one place, so a format change can't silently bring command output back as category hints.

  • Docs. Cut the classifier comment to one line. The breaker module doc now says that exit_status: classes are a deliberate departure from the reference heuristics, so a later sync doesn't undo it. The FORMAT.md paragraph is down from four sentences to one.

  • Unit tests. Removed the cases that only repeat other cases or existing tests:

    • the order.py lines that differ only in their numbers
    • an is_signature_tracked() assert that repeats the Deterministic check
    • the TimedOut/Cancelled early returns and the unclassed-message cases already covered by reasons_classify_as_the_reference_does

    The cases that add coverage remain: a class outranks a hint, and spawn_failed falls through to hints.

  • Integration tests.

    • One workflow run per test instead of five in a row. The one diagnostic left still carries both the timeout and 502 hints.
    • The restart: bool helper is replaced by two direct tests that share run_failing_command.
    • Removed the stdout assert, which duplicated the command.output check, and the inspect_run_dir read.
  • Shared helper. Added blocked_reason(&EngineState) in controls.rs. It replaces five copies of the find_map(RoutingBlocked) block.

Left for follow-up

  • A command killed by a signal (signal:<n>) carries the same captured output and still goes through message hints. Making it deterministic is a behavior change: a SIGKILL from running out of memory may fairly count as transient. It needs its own decision.
  • The root fix is to carry diagnostics in their own field instead of the failure message. That changes the replay record and the signatures, so it's out of scope here.
  • crates/github/acceptance/src/runs.rs:955 could use the new predicate.

Verification

  • cargo nextest: petri-ir and petri-execution (220 passed) and the controls suite (16 passed), run with the sandbox plugin env from mise run test.
  • cargo clippy --all-targets --all-features is clean on the changed crates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant