fix(coder): address #825 + #829 + #832 auto-review findings (final review pass)

[kovtcharov]

Summary

Final cleanup pass to complete the coder branch for EM testing. Five Important + three Minor findings across the Phase 5/6/11 auto-reviews. All 395 tests pass.

Changes

• test_self_fix/test_cli.py — pytest.raises so a silent-pass regression in argparse can't pass the test. [feat(coder): self-correction loop (§7.3-§7.9 + continuous critique) #825, feat(coder): Phase 11 — CLI unification + end-to-end integration tests #829]
• test_integration_e2e.py — real PATH prepend via monkeypatch.setenv instead of a no-op assignment that leaked env. [feat(coder): Phase 11 — CLI unification + end-to-end integration tests #829]
• test_fixes_827_828.py — drop unused Path import. [fix(coder): address #827 + #828 auto-review findings (security + runtime) #832]
• loop_driver.py — narrow broad except Exception around review_gate and notify_em to (RuntimeError, CalledProcessError, OSError). Programming errors now surface per CLAUDE.md fail-loudly. [feat(coder): self-correction loop (§7.3-§7.9 + continuous critique) #825]
• loop_driver.py + verifier.py — _append_notes / _append_note raise ValueError on corrupted or wrong-type notes_json instead of silently replacing with []. [feat(coder): self-correction loop (§7.3-§7.9 + continuous critique) #825]

Test plan

• pytest tests/coder/ tests/eval/ — 395/395 pass

[github-actions]

Summary

Tight, well-scoped cleanup PR addressing auto-review findings from #825, #829, and #832. The two structural changes — narrowing except Exception to (RuntimeError, CalledProcessError, OSError) in loop_driver.py and raising ValueError from _append_notes / _append_note on corrupted notes_json — correctly align the coder self-fix loop with GAIA's fail-loudly rule in CLAUDE.md. Test changes convert a silently-passing argparse assertion into a real pytest.raises and fix a PATH leak via monkeypatch. All touched code reads cleanly; my single suggestion is that the new ValueError branches lack unit coverage.

Issues Found

🟢 Minor — no unit tests for the new ValueError paths (src/gaia/coder/self_fix/loop_driver.py:106, src/gaia/coder/self_fix/verifier.py:99)

The fail-loudly rewrite of _append_notes / _append_note adds two new raise paths (corrupted JSON → ValueError, non-list JSON → ValueError) that aren't exercised by tests/coder/test_self_fix/. Per CLAUDE.md "every new feature requires tests," and since the explicit motivation is hiding-regression prevention, a regression here would be invisible. A tiny parametrized test against the module-level helpers would lock the contract in:

# tests/coder/test_self_fix/test_notes_json.py
import pytest
from gaia.coder.self_fix.loop_driver import _append_notes
from gaia.coder.self_fix.verifier import _append_note

@pytest.mark.parametrize("fn", [_append_notes, _append_note])
def test_raises_on_corrupted_json(fn):
    with pytest.raises(ValueError, match="corrupted"):
        fn("{not-json", {"at": "t"})

@pytest.mark.parametrize("fn", [_append_notes, _append_note])
def test_raises_on_non_list_json(fn):
    with pytest.raises(ValueError, match="must be a JSON array"):
        fn('{"already": "an object"}', {"at": "t"})

Non-blocking — can land as a follow-up.

🟢 Minor — ValueError absent from narrowed except tuple (src/gaia/coder/self_fix/loop_driver.py:381, loop_driver.py:435)

The narrowed except (RuntimeError, subprocess.CalledProcessError, OSError) covers the documented runtime/subprocess/IO failure modes, but notify_em (publisher.py:322) and the review-gate runner both pass user-derived context_url/pr_url strings to gh — misformed input surfaces as ValueError from the URL parsers or from argparse-style checks deep in a runner. Programming-error surfacing is the goal, so this is defensible as-is, but worth deciding consciously: is a malformed URL at step 6/8 a "programming error the EM must see immediately," or a "loose-coupling note like a failed review gate"? If the latter, consider adding ValueError to the tuple. No change required — flag for the author to confirm intent.

Strengths

• Narrowing is exemplary fail-loudly practice. The before/after diff at loop_driver.py:378-390 is a textbook example of how CLAUDE.md's "No Silent Fallbacks" rule should be applied: the specific failure modes for a loose-coupled step are caught and logged; everything else (AttributeError, TypeError, KeyError) surfaces. The inline comments cite CLAUDE.md and issue feat(coder): self-correction loop (§7.3-§7.9 + continuous critique) #825, so future readers know why without git-blame archaeology.
• _append_notes docstring earns its words. Explaining that notes_json is the canonical audit trail and that silent [] replacement would hide a regression is exactly the "non-obvious WHY" that CLAUDE.md encourages.
• monkeypatch.setenv fix is the right call. Replacing the no-op os.environ["PATH"] = os.environ.get("PATH", "") with a real prepend through monkeypatch both (a) actually makes the Windows-CI resolution work and (b) auto-reverts, so the test no longer leaks env state into siblings. This is a meaningful correctness fix disguised as a cleanup.
• pytest.raises over bare try/except SystemExit. The old pattern was a classic silent-pass trap — if argparse ever stopped exiting, the test would pass with zero assertions. The new version makes the contract explicit.

Verdict

Approve — this is a clean finishing pass on the coder branch. The two minor notes above are follow-up material, not merge blockers. Nothing structurally concerning; the changes move the coder module closer to the project's fail-loudly contract rather than further from it.