From 319ee9e238ed06dbee4ae0c2c3bb75ec4ed7b7f3 Mon Sep 17 00:00:00 2001 From: pbean Date: Tue, 21 Jul 2026 10:35:16 -0700 Subject: [PATCH] fix(verify): force LC_ALL=C at the git chokepoint so rollback stops matching English (#236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit safe_rollback tolerated a benign `git checkout -- ` no-op (a preserve dir holding only untracked files) by substring-matching git's English "pathspec ... did not match" message. `_run_git` ran git with the inherited environment, so under a localized git (e.g. LANG=it_IT.UTF-8) the message is translated, the substring never matches, and the benign no-op raises GitError — turning a resolvable re-drive into the rollback-pause path. Force LC_ALL=C at the sole git spawn point (_run_git) so every message the orchestrator inspects stays stable English, making the whole class of locale-fragile parsing impossible rather than patching the one instance. The locale is merged last so it wins over both the inherited environment and the explicit env the two _git_env callers pass (their throwaway GIT_INDEX_FILE / synthetic commit identity vars are preserved by the spread). Regression coverage spies on the actual subprocess env rather than the parent LANG, so the guarantee holds even on a CI box with no non-English locale catalogs installed (where a plain LANG=it_IT test would emit English anyway and pass without the fix). Refs #233. --- CHANGELOG.md | 5 +++++ src/bmad_loop/verify.py | 13 +++++++++++-- tests/test_verify.py | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 520ead17..2067d35f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -319,6 +319,11 @@ PATH)`, the TUI notifies `multiplexer backend unavailable — launch/attach disa ### Fixed +- **Locale-stable rollback (#236).** Git subprocesses now run with `LC_ALL=C`, so `safe_rollback`'s + benign "pathspec did not match" no-op is no longer misread as a hard failure under a localized git + (e.g. `LANG=it_IT.UTF-8`) — which had turned a resolvable re-drive into a rollback pause. Forced at + the single `_run_git` spawn point, so every git message the orchestrator inspects stays English. + - **The parked-window return target is now backend-composed (#221).** An interactive attach recorded the client's origin as a bare pane id (`%N`) and replayed it as `switch-client -t %N` from inside the control session — sound under tmux's one-server model, but on psmux (one diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 504ba15e..3017dfac 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -105,14 +105,21 @@ def _run_git( `subprocess.run` *before* any return code exists, so left uncaught it would bypass every `except GitError` guard and crash the run (#156); translating it here puts timeouts in the same taxonomy as any other git failure — - observation guards degrade, unguarded paths fail typed.""" + observation guards degrade, unguarded paths fail typed. + + Every git child runs with `LC_ALL=C` so messages stay stable English: the one + place that inspects git *text* rather than a return code — `safe_rollback`'s + benign "pathspec did not match" tolerance — must not misread a translated + message under a localized git (#236). Merged last so it wins over both the + inherited environment and any explicit `env` (the `_git_env` callers' throwaway + `GIT_INDEX_FILE` / synthetic identity vars are preserved by the spread).""" try: return subprocess.run( cmd, capture_output=True, text=True, timeout=_git_timeout_s, - env=env, + env={**(env if env is not None else os.environ), "LC_ALL": "C"}, ) except subprocess.TimeoutExpired as exc: raise GitError(f"git {cmd[3]} timed out after {_git_timeout_s}s in {repo}") from exc @@ -582,6 +589,8 @@ def safe_rollback( # only untracked files). Any other failure means a protected path wasn't # restored: raise instead of silently dropping a resolved re-drive's # corrected spec (which would regress the re-drive into a recovery loop). + # `_run_git` pins LC_ALL=C, so this English substring is stable under a + # localized git (#236) — never translated out from under the match. for d in preserve: rc, out = _git(repo, "checkout", snapshot, "--", d) if rc != 0 and "did not match" not in out: diff --git a/tests/test_verify.py b/tests/test_verify.py index 751bd0c8..0919c958 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -132,6 +132,45 @@ def spying_run(cmd, **kwargs): assert seen["timeout"] == 7 +def test_run_git_forces_c_locale(project, monkeypatch): + """Every git child runs with LC_ALL=C so message text stays stable English — + the chokepoint fix for #236 (safe_rollback's "did not match" tolerance must not + be translated out from under it by a localized parent git). Spy on the actual + subprocess env rather than the parent LANG, so the guarantee is pinned even on a + CI box with no non-English locale catalogs installed (where a plain LANG=it_IT + test would emit English anyway and pass without the fix).""" + seen: dict[str, object] = {} + real_run = subprocess.run + + def spying_run(cmd, **kwargs): + seen["env"] = kwargs.get("env") + return real_run(cmd, **kwargs) + + monkeypatch.setattr(verify.subprocess, "run", spying_run) + verify.rev_parse_head(project.project) + assert seen["env"] is not None + assert seen["env"]["LC_ALL"] == "C" + + +def test_run_git_locale_merge_preserves_explicit_env(project, monkeypatch): + """The LC_ALL=C merge must not clobber a caller-supplied env — the `_git_env` + callers pass a throwaway GIT_INDEX_FILE / synthetic commit identity that has to + survive (else snapshot_worktree breaks). Assert both the forced LC_ALL and the + caller's own key reach subprocess.run together.""" + seen: dict[str, object] = {} + real_run = subprocess.run + + def spying_run(cmd, **kwargs): + seen["env"] = kwargs.get("env") + return real_run(cmd, **kwargs) + + monkeypatch.setattr(verify.subprocess, "run", spying_run) + verify._git_env(project.project, "status", env={**os.environ, "SENTINEL_X": "1"}) + assert seen["env"] is not None + assert seen["env"]["LC_ALL"] == "C" + assert seen["env"]["SENTINEL_X"] == "1" # caller's env preserved + + @pytest.mark.parametrize( "raw,expected", [