Skip to content

fix(cli,minimize): validate --mode upfront and reject invalid modes (exit 2) - #19

Merged
utsab345 merged 6 commits into
mainfrom
fix/minimize-mode-issue17
Oct 19, 2026
Merged

utsab345 merged 6 commits into
mainfrom
fix/minimize-mode-issue17

Conversation

@utsab345

Copy link
Copy Markdown
Owner

Fix minimize --mode validation to reject invalid modes (hybrid/bogus) before minimization with exit 2. Add mode validation tests and replay contract tests; document frozen-replay contract and update 0.2.0 docs. All tests pass.

@utsab345
utsab345 force-pushed the fix/minimize-mode-issue17 branch from 7a017c1 to 1d3b0db Compare October 18, 2026 17:07
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Read through the fix on ffb31ac and ran it: --mode hybrid and --mode bogus now exit 2 with Invalid replay mode 'hybrid'., and the five valid modes are accepted. Good.

What I want to flag is that the mode check sits in front of a wider opening, and two arms still walk through it on that same commit. Measured with the repo's own tests/golden/failed_tool_run.sftrace (it records error_type: ToolTimeout, "flight_search timed out") and three throwaway entrypoints:

entrypoint what minimize reported exit
--mode bogus on your branch Invalid replay mode 'bogus' 2
raises ModuleNotFoundError at import Failure Signature: Exception: ModuleNotFoundError, Minimized Events 5 0
raises an unrelated RuntimeError Failure Signature: Exception: RuntimeError 0
returns normally (no report: "no failure to minimize") 1

The last row is the interesting one: the refusal path already exists and works. The first two rows are not failures being minimized — they are the attempt failing to get off the ground, and they still produce a report and a written bundle.

The mechanism is one line in minimize/signature.py: the whole attempt sits inside

try:
    with ReplaySession.from_trace(trace, mode=mode) as replay:
        actual_output = entrypoint_fn()
        divergence_error = replay.divergence
...
except Exception as exc:
    # Application exception during replay
    exception = exc

ReplaySession.from_trace(...) is inside that try, which is why a mode ValueError could become a signature in the first place — and why validating the mode at the CLI closes the symptom rather than the door. Moving the construction outside the try (or catching its errors into FailureDetectionError) makes it structurally impossible for a harness error to be minimizable, including the ones nobody has hit yet. That is cheap and I'd suggest it alongside the CLI check, since the engine is also reachable from the Python API where the CLI guard does not apply.

The second half is the agent-side one and it needs a decision, not a patch: a missing dependency genuinely can be the recorded bug, so the exception type alone cannot separate "this is what we are minimizing" from "this check never ran". The trace does record its own failure (ErrorEvent.error_type / message), and the session knows whether anything was consumed (_cursor, private) — a signature produced before any recorded call was consumed, matching neither the recorded error type nor a divergence, is a strong candidate for refusal-with-explanation. A strict type match alone would be too narrow, because a correct fix can legitimately replace the old failure with a new one.

Same distinction I tried to draw in #20, applied one layer earlier: could_not_execute is not failed, and here it is not "minimized" either.

@utsab345

Copy link
Copy Markdown
Owner Author

Hoisted ReplaySession construction out of the try-block in signature.py (a5806f1). Harness errors now raise FailureDetectionError and cannot become a minimizable signature, including via the Python API path. Lint and mypy failures also fixed, CI is green.

Your second point (refusal when a signature is produced before any recorded call is consumed) is tracked in #22. Needs a design decision as you said.

@gary1381

Copy link
Copy Markdown
Contributor

Following up on #17. I ran my triage and crash agents against ffb31ac (offline fake LLM, Python 3.13). Mode validation matches what @how2how2how2-arch reported, so just the parts not covered yet:

  • Root cause: +1 to moving ReplaySession.from_trace out of the try. On my triage trace the other advertised modes (live/forbidden/manual/derived) all exit 0 with Failure Signature: Exception: ReplayPolicyError, same hole. test_minimize_accepts_valid_modes only checks != 2, so it won't catch it.
  • Replay-contract docs match what I saw (changed args → ReplayMismatchError at call ci: add trusted-publishing workflow for PyPI #4; extra call → ReplayExhaustedError: unexpected tool call 'escalate'). One gap: the docs suggest stepfork score for the record-the-fixed-run workaround, but score rates exactly that test run-pinned (exit 1). Worth a sentence, or a flag for fix-side bundles.
  • CI: full suite passes locally (584 passed), but 3.11–3.13 are red. Locally: ruff SIM117 at tests/unit/test_replay_contract.py:104 and 15 mypy errors in the four new test files (main is clean). Also ReplayExhaustedError subclasses ReplayMismatchError, so the extra-call tests would pass on an args divergence too; pytest.raises(ReplayExhaustedError, match="unexpected tool call") would pin it.
  • Docs nit: README and getting-started.md / early-adopter-guide.md still mention --pre.

Happy to contribute the two agents (~80 lines with the fake LLM) as end-to-end fixtures for the export → pytest path.

@utsab345

Copy link
Copy Markdown
Owner Author

Thanks, all five points are handled now.

  • Policy error hole: fixed in 7dfa510. ReplayPolicyError from dispatch is refused as a failure signature, so live/forbidden/manual/derived now exit 1 with replay policy refused the attempt and write no bundle. test_minimize_accepts_valid_modes now asserts exit 1 for every valid mode on a non-failing trace, and a new test checks frozen succeeds (exit 0) where forbidden refuses (exit 1) on the same failing trace.
  • CI: your run was on ffb31ac. The ruff SIM117 and 15 mypy errors were fixed in a5806f1 and 665ec3f, green on 3.11/3.12/3.13 since, and green again on this push.
  • Extra-call tests: pinned to pytest.raises(ReplayExhaustedError, match="unexpected tool call") in both files (279bacc), as you suggested.
  • --pre: removed from README, getting-started, and early-adopter-guide (0793272).
  • score sentence: added in pytest-export docs: a fix-side bundle reads run-pinned by construction, that verdict describes where the bundle came from, not whether the test is worth keeping (0793272).

The two agents as fixtures sound good, please send them as a separate PR.

@utsab345
utsab345 merged commit 919d390 into main Oct 19, 2026
6 checks passed
@utsab345
utsab345 deleted the fix/minimize-mode-issue17 branch October 19, 2026 04:16
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.

3 participants