fix(verify): translate git's decode fault and add a bytes mode (#377) - #387
Conversation
`_run_git` decoded git's output strictly and translated only a timeout and a spawn OSError. A UnicodeDecodeError is a third fault raised before any return code exists, and being a ValueError it matched neither arm — so it escaped untyped past every `except GitError` guard, which is the one thing the chokepoint's docstring says must not happen. Reachable wherever git's own quoting is off: `-z` disables core.quotePath (dirty_paths, branch_incoming_paths, commit_paths), `worktree list --porcelain` never applied it, and `git diff` emits content verbatim. The test uses a real file with real bytes and real git: monkeypatching subprocess.run hands the code str objects and never runs the stdlib's decoding, so that version passes with the fix ablated. The #378 codec guard moves to the top of the file, since it now serves both.
AGENTS.md requires every git subprocess to go through `_run_git`, but two properties kept a caller out: it hands back CompletedProcess[str], i.e. the strict decode, and it raises rather than returning a return code the caller reads as an answer — `git config --get` of an unset key exits 1, and that is the reply, not a fault. A keyword-only `binary` flag with @overload declarations gives the mode a type, and `git_bytes` is the public accessor so no other module reaches for the private name. Bytes callers keep what the chokepoint is for: the LC_ALL=C pin (#236) and the limits.git_timeout_s bound (#156), with the two faults that have no rc to return still raising. No caller on main yet — install._shield_git is the intended customer and lands with #385, whose docstring names this issue as its blocker.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe shared git subprocess chokepoint now supports text and raw-bytes execution, converts decode failures into ChangesGit output taxonomy
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What
verify._run_gitnow translates a strict-decode fault intoGitError, and gained a bytes mode(
binary=, plus the publicgit_bytesaccessor).Why
Fixes #377
The chokepoint exists to give git's pre-returncode faults a type its ~50
except GitErrorguardscan catch. It translated a timeout (#156) and a spawn
OSError(#343) — butUnicodeDecodeErrorisa third such fault, and being a
ValueErrorit matched neither arm, so it escaped untyped: theone thing its own docstring says must not happen. One POSIX filename that is not valid in the run's
encoding is enough, wherever git's own quoting is off — the
-zcallers (merge pre-flight),worktree list --porcelain(crashes the unguarded stale-run reconcile at every run/sweep start),and
git diff, whose content is verbatim (a failed unit's forensic capture bypasses the valve thatpreserves its uncaptured work).
The bytes mode is the second half #377 asks for:
install._shield_gitstands outside the chokepointAGENTS.md requires because it needs raw bytes and needs a non-zero rc as an answer (
config --getof an unset key exits 1), and its docstring names this issue as the blocker.How
except UnicodeDecodeError→GitError, verbatim as the issue sanctions. Noterrors="surrogateescape"— _run_git's decode fault escapes the GitError taxonomy on -z output #377 defers that, since surrogates would flow into UTF-8 journal/JSONwrites.
binary: bool = Falsewith@overloads so the return type isCompletedProcess[bytes]when set; the only body change istext=not binary. Bytes callers keepthe
LC_ALL=Cpin (safe_rollback matches git's English "did not match" — benign restore no-op raises GitError under a localized git #236) and the_git_timeout_sbound (Uncaught GIT_TIMEOUT in verify.attempt_dirty crashes the whole run from the rollback/pause path #156), and the two faults with no rc toreturn still raise.
git_bytes(repo, *args)so no other module reaches for the private name. Argv shape isunchanged —
_run_git's messages indexcmd[3].No caller on
mainyet: #385/#386 are still open, soinstall._shield_gitdoes not exist here.This is the API half; the rewiring stays with #384's stack.
Testing
5 tests in
tests/test_verify.py, all on the conftestprojectfixture. The decode test uses areal bad-byte filename and real git — monkeypatching
subprocess.runhands the codestrandnever runs the stdlib's decoding, so that version passes with the fix ablated
(
tests/test_install.py:1996documents the same trap). It reuses theneeds_strict_codecguardfrom #378 (hoisted to the top of the file, since it now serves both), because 0xff decodes fine
under ISO-8859-x/cp125x.
Full suite
3703 passed, 24 skipped(main: 3698 + 24, i.e. exactly the 5 new tests);pyright@1.1.4110 errors;trunk fmt+ fulltrunk checkclean.Ablations run (AGENTS.md rule)
except UnicodeDecodeErrorarmtest_z_output_undecodable_path_becomes_git_errorFAILS with rawUnicodeDecodeError✅binary=Trueingit_bytes..._returns_bytes_and_reads_rc_as_an_answer+..._inherits_locale_pin_and_timeoutFAIL ✅except subprocess.TimeoutExpiredarmtest_git_bytes_timeout_still_becomes_git_errorFAILS with rawTimeoutExpired✅except OSErrorarmtest_git_bytes_spawn_oserror_still_becomes_git_spawn_errorFAILS with rawOSError✅Plus the vacuity check the #378 follow-up exists for: under
LC_ALL=et_EE.iso885915the decode testskips with its stated reason rather than falsely passing.
Summary by CodeRabbit
Bug Fixes
Improvements