mosaicstack/stack
1
0
Fork 1
Code Issues 207 Pull Requests 14 Actions Packages Projects Releases 15 Wiki Activity

codex review wrappers crash on binary/non-UTF-8 diffs (invalid utf-8 byte) — blocks the independent-review gate on any PR with binary artifacts #1046

New Issue
Open
opened 2026-08-04 21:49:44 +00:00 by Mos · 0 comments
Mos commented 2026-08-04 21:49:44 +00:00
Contributor
Copy Link
Copy Source

Problem

codex-code-review.sh (and the security-review sibling) fail hard when the diff under review contains binary / non-UTF-8 content. The wrapper embeds staged bytes directly into the review prompt, so any non-text blob aborts the run:

invalid utf-8 sequence ... invalid byte at offset 197720

This is not a one-off. It blocks independent review of any change carrying binary artifacts — and at least one delivery lane produces them by design (gzipped checker fixtures/artifacts committed intentionally). Every future revision of such a PR hits it again.

Impact — it silently removes a merge gate

Independent code review (author≠reviewer) is a required gate. When the review tool cannot run, the honest outcome is "gate not satisfied → do not merge" — which is what happened here (correct, but it stalls delivery). The dangerous failure mode is the other one: a lane that reads the crash as an infrastructure hiccup and proceeds, merging with a required review that never actually ran. A gate that cannot execute must never be indistinguishable from a gate that passed.

Expected behavior

The reviewer should review the reviewable content and be explicit about what it excluded:

  1. Detect non-UTF-8 / binary blobs before prompt assembly (git already classifies these — git diff --numstat reports - for binary).
  2. Exclude them from the prompt body, substituting a placeholder: <binary: path, N bytes, sha …>.
  3. Review the text portion normally.
  4. State the exclusion in the verdict — the review must declare "N binary files excluded from textual review: …". An excluded file silently dropped is an unstated population; the verdict must carry its own scope.
  5. Exit non-zero with a distinct code only when there is nothing reviewable at all — never crash mid-assembly.

Why the exclusion must be stated, not silent

If binary blobs are dropped without being named, the verdict reads as a full-coverage review of a partial population — the "clean result over an unenumerated population" failure. The reviewer's own output has to state its scope so a reader can tell a full review from a partial one.

Repro

Stage any commit containing a gzipped artifact (or any binary file) and run codex-code-review.sh --uncommitted. Observed on a PR whose checker fixtures are gzipped by design.

Suggested scope

~/.config/mosaic/tools/codex/codex-code-review.sh + codex-security-review.sh — shared prompt-assembly path. Add binary detection + placeholder substitution + a scope line in the verdict.

## Problem `codex-code-review.sh` (and the security-review sibling) fail hard when the diff under review contains **binary / non-UTF-8 content**. The wrapper embeds staged bytes directly into the review prompt, so any non-text blob aborts the run: ``` invalid utf-8 sequence ... invalid byte at offset 197720 ``` **This is not a one-off.** It blocks independent review of any change carrying binary artifacts — and at least one delivery lane produces them **by design** (gzipped checker fixtures/artifacts committed intentionally). Every future revision of such a PR hits it again. ## Impact — it silently removes a merge gate Independent code review (author≠reviewer) is a **required gate**. When the review tool cannot run, the honest outcome is "gate not satisfied → do not merge" — which is what happened here (correct, but it stalls delivery). The dangerous failure mode is the other one: a lane that reads the crash as an infrastructure hiccup and proceeds, merging with a **required review that never actually ran**. A gate that cannot execute must never be indistinguishable from a gate that passed. ## Expected behavior The reviewer should review the **reviewable** content and be explicit about what it excluded: 1. **Detect non-UTF-8 / binary blobs** before prompt assembly (git already classifies these — `git diff --numstat` reports `-` for binary). 2. **Exclude them from the prompt body**, substituting a placeholder: `<binary: path, N bytes, sha …>`. 3. **Review the text portion normally.** 4. **State the exclusion in the verdict** — the review must declare "N binary files excluded from textual review: …". An excluded file silently dropped is an unstated population; the verdict must carry its own scope. 5. **Exit non-zero with a distinct code** only when there is nothing reviewable at all — never crash mid-assembly. ## Why the exclusion must be stated, not silent If binary blobs are dropped without being named, the verdict reads as a full-coverage review of a partial population — the "clean result over an unenumerated population" failure. The reviewer's own output has to state its scope so a reader can tell a full review from a partial one. ## Repro Stage any commit containing a gzipped artifact (or any binary file) and run `codex-code-review.sh --uncommitted`. Observed on a PR whose checker fixtures are gzipped by design. ## Suggested scope `~/.config/mosaic/tools/codex/codex-code-review.sh` + `codex-security-review.sh` — shared prompt-assembly path. Add binary detection + placeholder substitution + a scope line in the verdict.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Defect in framework tooling or delivered code
 fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
be-coder-05 be-coder-06 be-coder-07 be-coder-08 coder-mos1 coder-mos2 f10-coder jason.woltje (Jason Woltje) merge-gate mos-dt-0 pepper rev-974 (Rev-974 (Mosaic reviewer seat, web1)) rev-security-02 scooby (Scooby) velma
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#1046