fix(verify): classify environment faults per shell so win32 stops burning attempts (#302)

[dracic]

Closes #302

Problem

ENV_FAULT_RCS = frozenset({126, 127}) is sh's launcher convention. Verify commands run shell=True, so on Windows the launcher is cmd — and cmd has no equivalent. Measured on Windows 11 (the issue's premise was corrected in a comment):

subprocess.run("no-such-tool", shell=True)   -> rc 1   "'no-such-tool' is not recognized …"
cmd /c "no-such-tool && echo x"              -> rc 1
probe.cmd calling the same                   -> ERRORLEVEL 9009, only *inside* the batch file
cmd /c "C:\tmp\check.sh"                     -> rc 0, no output — handed to the file association

So 9009 never reaches the orchestrator unless the verify command is itself a .cmd/.bat that propagates it, and the common "pytest is not on PATH" fault arrives as rc 1 — the same code an ordinary test failure uses. The env-fault arm was therefore unreachable on win32: a broken environment burned dev attempts on repairs no session can make (the regression #130 fixed on POSIX), and the three engine tests that pin the behavior were skipif(win32), so nothing caught it.

The rc 0 line above is the second half: a verify command naming a file cmd cannot execute reports passed for a check that never ran.

Fix

The exit-code test becomes a per-shell classifier (verify.env_fault_reason). POSIX is unchanged. The win32 arm takes three independent signals:

1. rc 9009 — the batch-wrapper case.
2. cmd's own message — is not recognized / access is denied, matched against the closing lines of the output (cmd's message wraps onto two), so a subprocess that merely prints the phrase mid-run stays an ordinary failure.
3. A resolvability probe of the leading token — covers localized Windows, where neither phrase is printed. Its file branch also fires at rc 0, closing the silent false pass.

Four guards keep the arm from escalating a healthy run — each one a false positive found while building it:

Guard	Without it
A token the shell still acts on (%VAR%, !VAR!, `&	; < > ^`) skips the probe
Executability decided by PATHEXT (appended, as cmd does), not shutil.which	which resolves a relative path against the process cwd, not the run's — under worktree isolation a green run escalates
The not-found-on-PATH branch is skipped when the token names a runnable file in the run cwd	cmd searches the run's own directory before PATH; check.cmd failing there → env fault instead of a fix session
A timed-out command (rc sentinel -1) is never classified	it ran and hung, so it was found and runnable

Parens and a leading @ are stripped from both ends ((pytest) tokenized to pytest)), and is_file() is wrapped for OSError.

Tests

• The three skipif(sys.platform == "win32") engine tests now run on both platforms; their assertions moved off rc=126 onto the classification itself.
• conftest gains the host-shell env-fault builders (MISSING_TOOL_CMD, _self_disarming_cmd, _write_check_script / _disarm_check_script) in the file's existing single platform-detection spot.
• The classifier's matrix is unit-tested per signal, including every guard above. The win32 rows drive env_fault_reason with synthetic results rather than executing an unrunnable path — doing that for real pops the file-association picker mid-suite (Windows: env-fault engine test replays check.sh through cmd — pops the 'How do you want to open this file?' dialog on every local full-suite run #292).
• A batch file cannot disarm itself (cmd re-reads it per line, so deleting or renaming it kills the current run with "The batch file cannot be found."), so the win32 twin of _self_disarming_cmd burns a flag file and reaches for a missing tool on the second run.
• Three tests carried POSIX-only verify commands (true, false, a bare .sh) that cmd cannot run. test_verify_review_happy_and_commands was already failing on win32 before this change; the other two passed for the wrong reason — a missing command reading as an ordinary failure. All three now use host-shell verbs.

Verification

• Windows 11: pytest tests/test_engine.py tests/test_verify.py → 395 passed, 5 skipped (the skips are the POSIX-only classifier test + upstream's POSIX symlink tests)
• Linux (WSL): same suites → 380 passed, 21 skipped (all skips are the cmd-specific tests)
• ruff check + ruff format --check clean; prettier@3.8.4 clean on the touched markdown; pyright reports nothing new in verify.py

Scope

plugins/bus.py is the other shell=True site but has no env-fault classification at all, so it is deliberately untouched. Rebased on a82be01.

Summary by CodeRabbit

• Bug Fixes
  • Improved Windows detection for verify command environment faults, including missing/unrecognized/non-runnable commands, avoiding incorrect “passed” or burn-attempt routing.
  • Verify environment faults now pause the run with a more specific, human-readable fault reason.
  • Kept POSIX behavior consistent while tightening cross-platform classification logic.
• Documentation
  • Updated failure-handling docs to describe the refined, platform-specific environment-fault detection and budget-preservation behavior.
• Tests
  • Expanded and adjusted Windows and cross-shell test coverage to validate correct env-fault precedence and avoid regressions.

[coderabbitai]

Warning

Review limit reached

@pbean, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 46 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e518274e-aac4-47c5-a5d6-ea7782dfb78f

📥 Commits

Reviewing files that changed from the base of the PR and between 74b210d and ed77c6b.

📒 Files selected for processing (6)

• CHANGELOG.md
• docs/FEATURES.md
• src/bmad_loop/verify.py
• tests/conftest.py
• tests/test_engine.py
• tests/test_verify.py

Walkthrough

Verify environment-fault detection now distinguishes POSIX and Windows shell behavior. Windows classification uses command output, return codes, and executable resolution, with expanded engine and unit-test coverage plus updated documentation.

Changes

Verify environment fault handling

Layer / File(s)	Summary
Shell-aware fault classifier src/bmad_loop/verify.py	Adds Windows command parsing, executable resolution, shell-signal detection, and env_fault_reason(...) integration into verify outcome escalation.
Cross-platform verify fixtures and engine scenarios tests/conftest.py, tests/test_engine.py	Adds host-shell verify helpers and updates dev, review, and fix-phase environment-fault scenarios.
Classification coverage and behavior documentation tests/test_verify.py, docs/FEATURES.md, CHANGELOG.md	Covers Windows cmd signals and POSIX behavior while documenting the updated environment-fault rules.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant VerifyCommandsOutcome
  participant EnvFaultReason
  participant CmdExe
  participant Engine
  VerifyCommandsOutcome->>EnvFaultReason: classify verify result
  EnvFaultReason->>CmdExe: probe command token
  CmdExe-->>EnvFaultReason: return code and command resolution
  EnvFaultReason-->>VerifyCommandsOutcome: fault reason or no fault
  VerifyCommandsOutcome->>Engine: escalate environment fault

Loading

Possibly related PRs

• bmad-code-org/bmad-loop#130: Introduced the earlier POSIX environment-fault classification and engine escalation path.
• bmad-code-org/bmad-loop#301: Updated host-shell environment-fault test setup for Windows-compatible verify scripts.

Suggested reviewers: pbean

Poem

I’m a bunny with a shell-safe tune,
Sorting faults beneath the moon.
Windows speaks, and POSIX too,
Tests hop along with paths made true.
No broken command burns the day—
We pause, then safely re-arm and play.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately describes the Windows verify classification fix and its goal.
Linked Issues check	✅ Passed	The code and tests implement the per-shell verifier classification and lift the Windows test gaps required by #302.
Out of Scope Changes check	✅ Passed	The changes stay focused on verify classification, related tests, and docs/changelog updates with no clear unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_verify.py (1)

427-445: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

These "ordinary failure" cases silently depend on pytest being resolvable via shutil.which.

Every pytest … row reaches the token probe (rc≠0, no cmd message) and only returns None because shutil.which("pytest") succeeds. Under a python -m pytest invocation from an env whose Scripts\ dir is not on PATH, which returns None and the classifier reports pytest not found on PATH, failing the assertion for reasons unrelated to the behavior under test. Pinning resolution makes the matrix hermetic:

♻️ Make token resolution explicit

 def test_win32_ordinary_failures_are_not_env_faults(tmp_path, command, rc, tail):
+    # the probe's PATH lookup is not the subject here: pin it so the matrix
+    # asserts classification, not this interpreter's PATH layout.
+    monkeypatch.setattr(verify.shutil, "which", lambda name: rf"C:\tools\{name}.exe")
     assert verify.env_fault_reason(verify.CommandResult(command, rc, tail), tmp_path) is None

(add monkeypatch to the signature). Note the same dependency applies to ruff check rows.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_verify.py` around lines 427 - 445, Make
test_win32_ordinary_failures_are_not_env_faults independent of the host PATH by
adding monkeypatch to its signature and explicitly patching the token-resolution
lookup used by verify.env_fault_reason so pytest and ruff resolve successfully.
Preserve the existing command matrix and assertions while ensuring the cmd
builtin, malformed, and other non-tool cases retain their current behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_verify.py`:
- Around line 427-445: Make test_win32_ordinary_failures_are_not_env_faults
independent of the host PATH by adding monkeypatch to its signature and
explicitly patching the token-resolution lookup used by verify.env_fault_reason
so pytest and ruff resolve successfully. Preserve the existing command matrix
and assertions while ensuring the cmd builtin, malformed, and other non-tool
cases retain their current behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c1c30c33-a1bc-4f55-9caf-f86beee339fd

📥 Commits

Reviewing files that changed from the base of the PR and between a82be01 and 1dfdbff.

📒 Files selected for processing (6)

• CHANGELOG.md
• docs/FEATURES.md
• src/bmad_loop/verify.py
• tests/conftest.py
• tests/test_engine.py
• tests/test_verify.py

[dracic]

@coderabbitai The nitpick is valid and is now addressed in 06b7c4f.

Confirmed the dependency: every pytest … / ruff check row reaches the token probe (rc≠0, no cmd message on the closing lines) and returned None only because shutil.which("pytest") happens to resolve on the host that ran it. Under a python -m pytest invocation from an env whose Scripts\ dir is off PATH, the classifier reports pytest not found on PATH and the row fails for a reason unrelated to what it asserts.

I took the fix with one deliberate change from the suggested diff. Resolving every name:

monkeypatch.setattr(verify.shutil, "which", lambda name: rf"C:\tools\{name}.exe")

would make the matrix hermetic but strip several rows of their evidence: ("(pytest)", 1, …) exists to prove the grouping parens are stripped from both ends (before that fix the token was pytest)), and ('if exist "x" (exit 1)', 1, "") exists to prove the cmd builtin allowlist is consulted. With a universally-resolving which, both pass no matter what the classifier does.

So the fake pins a host that has exactly the two tools the matrix names, and nothing else:

monkeypatch.setattr(
    verify.shutil,
    "which",
    lambda name: rf"C:\tools\{name}.exe" if name in {"pytest", "ruff"} else None,
)

Hermetic, and every row stays load-bearing.

Two related rows do not share the dependency, so they were left alone: test_win32_passing_command_is_never_an_env_fault is rc 0, which only reaches the file branch (pytest -q names no file, so which is never consulted), and test_win32_shell_signals_are_env_faults returns on rc 9009 / the cmd message before the probe.

Verified after the change: Windows 11 pytest tests/test_verify.py → 173 passed, 3 skipped; Linux → 155 passed, 21 skipped; ruff check / ruff format --check clean.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@dracic Thanks for the detailed validation. The selective which stub makes the matrix independent of the runner’s PATH while preserving the negative cases that exercise parenthesis normalization and cmd builtin handling. The rc-0 and early-shell-signal cases are correctly excluded from that dependency.

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

[pbean]

Reviewed and rebased onto main (5432c81). The approach holds — it implements what your own analysis on #302 converged on, the scope traces entirely to that issue, and POSIX is genuinely untouched (the rc arm returns the same rc=126 string, everything else is behind the platform guard; I ablated both arms to confirm — see below). Pushed 74b210d with the rebase plus five changes.

Rebase: one conflict that needed a decision, not a merge

main now has #301, which fixed test_fix_phase_session_env_fault_escalates by subtraction — _OK instead of a script file, with the rationale in-line ("Only the marker command carries signal here — the escalation is the session's"). This PR re-added a script file to that same test via _write_check_script. Taking this branch's side would have silently reverted that.

Kept main's _OK there and dropped the hunk. _write_check_script / _disarm_check_script stay in conftest — test_fix_phase_env_fault_escalates_instead_of_looping genuinely needs a check it can disarm mid-run, which _OK can't do.

Removed the skipif(win32) on test_verify_commands_env_fault_rc_escalates

@pytest.mark.skipif(sys.platform == "win32", reason="sh-specific exit codes")

This contradicts the code it guards: env_fault_reason checks ENV_FAULT_RCS before the platform branch, so cmd /c "exit 126" still classifies. The proof is already in the tree — test_verify_env_fault_pauses_dev_without_burning_budget runs exit 127, asserts rc=127, is not win32-skipped, and the Windows job is green. So the skip was pure Windows coverage loss on a path the PR deliberately keeps platform-neutral. Dropped, with the reason recorded in the docstring.

Two comments that claimed more than was measured

The unrunnable-file branch. "cmd hands this to the file association and returns 0 without running it" was measured for .sh / .txt — extensions with no executing association. An extension that does have one (.rb, .pl on a host that registered them) runs and propagates an exit code, and the branch fires at any rc, so such a host escalates every run. Left the behavior alone — escalating is right, since what an association returns is the app's exit convention, not the script's, and PATHEXT is cmd's actual contract for what it runs — but the comment now says that instead of the universal claim.

The closing-lines window. The stated reason ("a subprocess that merely prints the phrase mid-run stays an ordinary failure") doesn't follow from the code: run_verify_commands builds the tail as stdout + stderr, so all stderr lands at the end regardless of chronology. The window holds for a better reason — cmd writes to stderr and stderr is appended last — and the edge it doesn't cover (a command whose own stderr ends with the phrase) is now named as accepted. Two test consequences:

• added the shape production actually builds — (1, f"1 failed, 3 passed\n{NOT_RECOGNIZED}") — which nothing covered;
• relabelled ("ruff check", 1, f"{NOT_RECOGNIZED}\n1 file reformatted") as pinning the window, since a real cmd message can't land in that position.

_FAIL beside _OK

conftest's "single platform-detection spot" already names _OK so nobody reaches for true. This PR exists because someone reached for false, so it now names _FAIL = "exit 1" with the same rationale, and the exit 0 / exit 1 literals in that block use both.

Commit subject

82 chars → 57 (CONTRIBUTING.md caps at 72). Reworded to fix(verify): classify verify env faults per shell (#302); the detail was already in the body. Authorship preserved.

Verification

• Full suite on Linux: 3095 passed, 23 skipped. (Two test_module_skills_sync failures are pre-existing local seeded-fork drift — reproduced on main at 5432c81, and that test skips in CI.)
• trunk check clean, uvx pyright@1.1.411 → 0 errors.
• Ablations, since the win32 matrix is all negative assertions:
  • deleting the ENV_FAULT_RCS arm → 8 red, including the now-unskipped [126] / [127] rows;
  • deleting if sys.platform != "win32": return None → test_posix_ignores_the_cmd_signals red plus three engine tests, which is the sharpest evidence that guard is what keeps POSIX byte-identical.
  • The win32 rows can't be ablated from here — stating plainly that those are CI-verified only.

One note, no action

_win32_env_fault_reason + helpers put ~90 lines of cmd-shell semantics into verify.py (now ~1700 lines), at roughly a third comment. A small dedicated module would sit better with the project's platform-quarantine doctrine — but each guard traces to a measured false positive, so the density is earned, and AGENTS.md says not to initiate refactors of untouched code. Recording it rather than acting on it.

[greptile-apps]

Greptile Summary

This PR fixes a Windows-specific regression where verify commands that fail due to a broken environment (missing tool, unrunnable file) were never classified as environment faults — burning dev-session budget on repairs that can't succeed. The fix replaces the single returncode in {126, 127} check with a per-shell env_fault_reason function that adds three independent win32 signals: rc=9009, cmd's English-language error messages in the output tail, and a PATHEXT/PATH resolvability probe of the command's leading token.

• verify.py: Adds _leading_token, _cmd_executable, _win32_env_fault_reason, and the public env_fault_reason dispatcher; verify_commands_outcome now inspects rc=0 results too.
• tests/test_verify.py: Replaces the POSIX-only skip with a cross-platform test; adds a dense matrix of win32-specific unit tests covering every signal and guard.
• tests/conftest.py / tests/test_engine.py: Removes three skipif(win32) guards and replaces POSIX-only shell verbs with cross-platform equivalents.

Confidence Score: 5/5

Safe to merge — the change is additive on POSIX and the win32 arm is guarded by four independently-verified false-positive checks that are each unit-tested.

The classifier logic is tightly scoped to a single public function that replaces a one-liner rc-set check. Each win32 signal and each guard has a dedicated unit test, including all edge cases enumerated in the PR description. The three engine-level integration tests now run on both platforms. No production path is altered on POSIX.

Files Needing Attention: No files require special attention. The most complex logic is in _win32_env_fault_reason in verify.py, and its decision tree is fully exercised by the new test matrix.

Important Files Changed

Filename	Overview
src/bmad_loop/verify.py	Core classifier refactor: adds per-shell env_fault_reason with four guards against false positives; logic is well-documented and matches the test matrix exactly
tests/test_verify.py	Comprehensive unit tests for every signal and guard in the win32 arm; win32 rows drive env_fault_reason with synthetic CommandResults
tests/conftest.py	Adds cross-platform verify-script helpers and MISSING_TOOL_CMD constant; centralises OS branching in one place
tests/test_engine.py	Removes three skipif(win32) guards, replaces POSIX-only shell verbs with cross-platform helpers

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[env_fault_reason called] --> B{rc in 126 / 127?}
    B -->|yes| C[return rc=N POSIX env fault]
    B -->|no| D{sys.platform == win32?}
    D -->|no| E[return None]
    D -->|yes| F[_win32_env_fault_reason]
    F --> G{rc less than 0?}
    G -->|yes| H[return None timeout]
    G -->|no| I{rc == 9009?}
    I -->|yes| J[return rc=9009]
    I -->|no| K{rc != 0 AND tail non-empty?}
    K -->|yes| L{is not recognized in last 2 lines?}
    L -->|yes| M[return cmd message]
    L -->|no| N{access is denied in last line?}
    N -->|yes| O[return cmd message]
    K -->|no| P[token probe]
    N -->|no| P
    P --> Q{_leading_token is None?}
    Q -->|yes| R[return None]
    Q -->|no| S{token names a file in cwd?}
    S -->|yes| T{not executable by PATHEXT AND not builtin?}
    T -->|yes| U[return not executable by cmd]
    T -->|no| V[return None]
    S -->|no| W{rc != 0 AND not local_executable AND not builtin AND which is None?}
    W -->|yes| X[return not found on PATH]
    W -->|no| Y[return None]

Loading

_{Reviews (2): Last reviewed commit: "fix(verify): classify verify env faults ..." | Re-trigger Greptile}