verify._run_git does not translate ValueError (embedded NUL in argv), so it bypasses every except-GitError guard

[pbean]

What

verify._run_git translates three failures that subprocess.run raises before any return code exists — TimeoutExpired (#156), OSError → GitSpawnError (#343), and a strict-decode UnicodeDecodeError (#377). Its docstring states that rationale explicitly: left uncaught, such a failure "would bypass every except GitError guard and crash the run."

There is a fourth in that class it does not translate. If any argv element contains a NUL, subprocess.run raises a plain ValueError: embedded null byte — not an OSError, and not a UnicodeDecodeError (which is a ValueError, but the reverse does not hold, so except UnicodeDecodeError does not catch it). It escapes as itself.

Why it matters

An escaping ValueError behaves worse than a git failure, because it bypasses the graceful arm. In engine._finalize_commit_phase, except verify.GitError escalates the story, but a ValueError falls to except BaseException, which restores the ledger and park record and then re-raises (engine.py:2304-2314). The task is already persisted as COMMITTING by then, so the run crashes and every later bmad-loop resume re-enters the same window and re-crashes identically.

Reachability

The one live path was fixed under #475: {story_title} in scm.commit_message_template put spec-frontmatter text into git commit -m argv, and title: "\0" is an ordinary double-quoted YAML scalar — no exotic file bytes required. That PR now neutralizes C0+DEL at the title chokepoint, with an ablation showing the unpatched path yielding crashed=True, crash_error='ValueError: embedded null byte'.

So this issue is not about a currently-reachable crash. It is that the chokepoint's stated contract — "all pre-return-code failures are translated into the GitError taxonomy" — is incomplete, so the protection depends on every present and future caller sanitizing its own strings. Callers that interpolate agent- or spec-authored text into argv are the risk surface as they accumulate.

Suggested shape

Add a fourth arm to _run_git, alongside the existing three:

except ValueError as exc:  # embedded NUL in an argv element
    raise GitError(f"git {cmd[3]} refused an invalid argument in {repo}: {exc}") from exc

Ordering matters: it must sit after the UnicodeDecodeError arm, which is a ValueError subclass and carries its own #377 message.

Worth deciding as part of this: whether a distinct type (à la GitSpawnError) is warranted, since "we handed git an unspawnable argument" is a caller bug rather than an environment fault, and the two want different remedies.

Deliberately not folded into #475 — that PR is a feature from an outside contributor, and this touches the shared git chokepoint that every caller funnels through, so it deserves its own review and its own ablation.

[coderabbitai]

🔗 Related PRs

#237 - fix(verify): force LC_ALL=C at the git chokepoint so rollback stops matching English (#236) [merged]
#344 - fix(verify): translate spawn OSError to GitSpawnError in _run_git (#343) [merged]
#382 - fix(verify): decode verify-command output with errors="replace" (#378) [merged]
#387 - fix(verify): translate git's decode fault and add a bytes mode (#377) [merged]
#441 - fix(verify): force literal git pathspecs, and read status stdout alone (#433, 6D) [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[pbean]

There is a second untranslated shape in this same class, found by the Codex review on #475 and confirmed there by reproduction: an argv element containing an unpaired surrogate.

UnicodeEncodeError: 'utf-8' codec can't encode character '\ud800'
in position 17: surrogates not allowed

UnicodeEncodeError and UnicodeDecodeError are siblings, not parent and child — both derive from UnicodeError → ValueError. So the existing except UnicodeDecodeError arm added by #377 does not catch the encode direction, exactly as it does not catch the NUL ValueError described above. Same escape path, same consequence: it bypasses except verify.GitError, hits the re-raising catch-all in _finalize_commit_phase, and leaves the task persisted as COMMITTING so every resume re-crashes.

It is reachable without exotic file bytes, on the same route as the NUL: title: "\uD800" is an ordinary double-quoted YAML scalar and PyYAML hands the code point back verbatim.

Both shapes are neutralized at the caller in #475 (the {story_title} sanitizer now strips C0/DEL and surrogates), so the story-title route is closed. This issue remains the general fix, and the second shape strengthens the case for it: two members of "characters a POSIX argv cannot carry" have now been found one after the other at the same caller, which suggests the set should be closed at the chokepoint rather than enumerated at each producer.

Implementation note for whoever picks this up: the new arm must be ordered after the UnicodeDecodeError arm, since that is a ValueError subclass and a ValueError arm placed first would shadow it — swallowing #377's specific message. A single except ValueError arm placed last covers both the NUL and the surrogate case.