What the review checks, how loud it is, and what fits in 3,087 bytes

[simonoppowa]

Part of #1023

Question

What does the review check, how loud is it — and what fits in 3,087 bytes?

Largely tool-independent as originally written, and #1035 proved the substance is right. What #1034 added is a hard byte budget, which turns this from an editorial question into an engineering one.

The budget

Codex reads AGENTS.md on the review path, wants a ## Code Review Rules heading, and truncates at 32 KiB silently (openai/codex#13386, open).

AGENTS.md   29,681 bytes
cap         32,768 bytes  (32 KiB)
headroom     3,087 bytes  — 90.6% consumed

There is no ## Code Review Rules heading today, and adding one costs from that 3,087. The file has grown steadily — docs-correction PRs land most weeks — so this will breach on its own, quietly, with no error and no signal beyond reviews mysteriously getting worse.

Decide:

• Does the review section fit in the headroom, or does AGENTS.md get trimmed / split? Codex merges instruction files root→leaf, so a nested AGENTS.md may carry per-directory rules — worth establishing whether that relieves the root file.
• Ordering as insurance. If truncation cuts the tail, then position is protection: the non-negotiables belong early in the file, not wherever they landed historically. Cheap, and it survives the file growing.
• A guard against silent breach. A CI check failing when AGENTS.md exceeds ~31 KiB would turn a silent degradation into a visible one. This repo already runs just check_l10n for an analogous quiet failure, so the pattern exists.

What the rules should say

From #1035, the conventions a review must enforce — with the evidence that they matter:

• Real translations, not English placeholders. The single most valuable catch in the trial: three of four new strings left verbatim English across eight locales, against CONTRIBUTING.md:54, and structurally invisible to CI because just check_l10n only fails on missing keys.
• ARB key parity — nine locales at equal count, discounting template-only @key metadata. State the exception explicitly or a reviewer will report a false violation (raw counts legitimately differ: en 1011, de 986, cs 982, all at 946 real keys).
• Semantics(identifier:) on new interactive widgets, kebab-case, with the container: true rule stated as conditional on a layout-greedy parent — the trial showed a good reviewer reasons about this rather than pattern-matching it.
• No Co-authored-by: trailer naming an AI; conventional-commit PR titles; lib/generated/ never hand-edited; Row title overflow rules.

How loud

Advisory by permission — Codex holds checks: read/statuses: read and cannot block. The remaining question is volume and tone on a first-time contributor's PR, since general code review is the noisiest kind and this repo has 19 outside contributors to keep. Decide whether the rules cap findings, set a confidence bar, or say nothing about volume at all.

[simonoppowa]

Decision

The section below goes into AGENTS.md, immediately after ## Project Overview (before ## Commits, around byte 650), and both trims are taken. Execution belongs to #1031; nothing is edited here.

The mechanics, established from Codex source rather than docs

Three facts decided the whole shape, and two of them killed remedies that looked obvious:

1. Truncation drops the TAIL. codex-rs/core/src/agents_md.rs:153 — data.truncate(remaining) on a Vec<u8>, pinned by the repo's own test doc_larger_than_limit_is_truncated asserting res == huge[..LIMIT]. So front-loading genuinely protects, and costs nothing. Placing the rules near the top also demotes ## Naming Conventions (884 B) into the at-risk tail — a far cheaper thing to lose.
2. The nested-AGENTS.md remedy is dead. The 32 KiB budget is cumulative across every discovered file (config_toml.rs:301: "Maximum total bytes of project instruction content across all selected environments"), and the walk is root-first (dirs.reverse()). An oversized root eats the budget and the nested file is truncated or skipped. Moving the accessibility section to lib/features/AGENTS.md buys nothing.
3. ## Code Review Rules is a prompt convention, not a parsed section. The literal string appears nowhere in the Codex loader. Behavioural proof: on astral-sh/ruff PRs #28232 and #28238 the bot cited AGENTS.md:L180-L183, ~175 lines below that repo's heading at line 5. Every byte of AGENTS.md still competes — the heading does not exempt the rest of the file.

The cut is a raw byte truncate with no marker, decoded from_utf8_lossy, so a rule straddling the boundary survives as a half-sentence that reads complete. Never plan to land just under the cap.

Byte plan — both cuts

29,681  current
-2,783  AGENTS.md:224-277  ADB/device prose  → tools/adb/README.md
-3,496  AGENTS.md:35-102   demo-data section → demo_seeder.dart doc comments
+3,935  ## Code Review Rules (3,934 + separator)
———
27,337  bytes · 5,431 under the 32,768 cap · 3,663 under a 31,000 CI guard

All four figures measured locally, not estimated. The second cut is not optional: one cut alone lands at 30,833, leaving 167 bytes under the guard — a tripwire that fires on the next docs-correction PR, and this repo merged 15 of those in the week to 2026-09-01.

Neither cut removes anything a reviewer can act on. Lines 224-277 are device-driver operating instructions — how uiautomator reports Flutter text under content-desc, why TextField bounds drift when the IME opens, the tap_field_by_hint helpers, DEVICE=... bash tools/adb/walk-onboarding.sh. A PR reviewer has no device. tools/adb/ already exists with no README; this text is that README. Lines 35-102 are authoring orientation for the demo seeder, with a natural home in the doc comments already on lib/core/utils/demo/demo_seeder.dart.

Architecture (8,462 B) is deliberately NOT cut despite being the largest section — roughly half is diff-checkable and feeds these rules directly: the DI registration split the registerFactory rule leans on, the Hive typeId uniqueness warning, the layer structure. Cutting it to buy bytes would degrade the review. The container: true gotcha (lines 204-223) also stays; rule 3 depends on it.

The section

## Code Review Rules

Reviews are advisory and cannot block a merge. Most PRs come from outside contributors, often their first — write for that person. Cite `file:line`, phrase findings as observations, and raise at most five. Silence is a valid review.

Never assert a count, or an "every locale" claim, you have not computed from this PR's own diff — not from the PR description, which has been wrong before. Don't restate what CI already failed on: `linux-checks` runs `just check_l10n`, `just build`, `flutter analyze`, `just test` — it deliberately does **not** run the format check. Don't comment on the target branch; a maintainer repoints those.

### 1. Untranslated ARB values — the one check CI cannot do

`just check_l10n` fails only on *missing* keys, so a key present in every locale with the English text still in it ships green. For each key **this PR adds or changes** under `lib/l10n/`, compare its value across the locale files.

Report a key left byte-identical to English in every, or nearly every, non-English ARB. `CONTRIBUTING.md` forbids leaving English in as a placeholder, machine translation is the accepted floor, and no Weblate/Crowdin pipeline exists here — so "translations land later" is not an exemption. Name the key and its locales once.

Identical is legitimate, and silent, for brand and platform names, units and symbols, acronyms, and placeholder-only strings (`{hour}:00`). A value identical in only one or two locales is usually a real cognate (German `Protein`, Italian `golf`) — leave those. Never audit keys the PR did not touch; that backlog is not this contributor's debt.

### 2. Never report an ARB key-count difference

Every ARB carries its own `@key` metadata, in differing amounts, so raw JSON entry counts legitimately differ between *any* two locales. Parity means keys not starting with `@`, and those are equal. Strip them before comparing, or say nothing.

### 3. Semantics identifiers

New interactive widgets take `Semantics(identifier: 'kebab-case')`, locale-independent. Exempt: children of a `ListView`/`GridView.builder` (the parent surface is labelled instead), and OK/Cancel inside a Material dialog. Don't ask for `button:`/`textField:` when the child already publishes that role — unless `excludeSemantics: true` is set, where the flag is required.

`container: true` is **conditional**: needed only when the immediate parent is layout-greedy (`Expanded`, a filling `Container`). Under a `Wrap`, or any parent sized by its content, its absence is correct — do not ask for it. `Align` is the documented alternative to the flag, not a trigger for it. Bounds are a device fact (`uiautomator dump`), not a diff fact: ask, never assert.

### 4. Everything else, in one line each

- Row titles: `Expanded` on the title plus `maxLines` + ellipsis, `AutoSizeText` for prominent headers. Never `Flexible(title)` beside a `Spacer()` — that starves the title. Wrapping body copy, and Rows sized by their content, are exempt.
- `lib/generated/` and `env.g.dart` are gitignored — their absence from a diff is correct, never ask for them. Every other `*.g.dart` is committed: a changed `@HiveType` / `@HiveField` / `@JsonSerializable` without its regenerated file is a real finding.
- `locator<T>()` on a `registerFactory` type returns a **fresh** instance in its initial state, not the screen's — reading `.state` off one is a bug `flutter analyze` cannot see.
- The same constructor call with the same arguments at three or more sites: point at it once.
- Reformatted lines the change did not need: mention once, ask for those hunks back. The cause is pinned-SDK format drift plus the `just format` that `CONTRIBUTING.md` tells contributors to run — never blame the contributor, never suggest a repo-wide reformat.
- If you can read the linked issue, name any state it asked for that no test covers. If you cannot, do not characterise what it asked for.

The CI guard

Nothing in Codex will ever tell you the file was cut: the warning is a tracing::warn! to a log file, openai/codex#13386 is open, and #7138 — which asked for a visible warning — was closed not_planned. So the guard is the only signal. Modelled on check_l10n, which exists in this repo for exactly the same reason: a green check that would otherwise lie.

# Guard AGENTS.md against Codex's silent instruction-file truncation.
#
# Codex reads AGENTS.md on the PR review path and truncates instruction
# content at 32 KiB (32768 bytes) — keeping the head, dropping the tail, with
# no warning anywhere a human looks (openai/codex#13386, still open; #7138
# asked for a warning and was closed not_planned). Nothing else catches it:
# the review still runs, still posts findings, and simply never sees the rules
# past the cut. The budget is also cumulative across every AGENTS.md in the
# tree, so a nested file does not buy headroom.
check_agents_md:
  #!/usr/bin/env bash
  set -euo pipefail
  limit=31000          # 1768 bytes of deliberate margin under 32768
  head_limit=12000     # Code Review Rules must sit well inside the head
  size=$(wc -c < AGENTS.md)
  if [ "$size" -gt "$limit" ]; then
    echo "AGENTS.md is ${size} bytes, over the ${limit}-byte guard." >&2
    echo "Codex truncates at 32768 and says nothing. Trim a section or move" >&2
    echo "device/authoring prose out (e.g. to tools/adb/README.md)." >&2
    exit 1
  fi
  offset=$(grep -b -m1 '^## Code Review Rules$' AGENTS.md | cut -d: -f1) || true
  if [ -z "${offset:-}" ]; then
    echo "AGENTS.md has no '## Code Review Rules' heading — Codex review reads it." >&2
    exit 1
  fi
  if [ "$offset" -gt "$head_limit" ]; then
    echo "'## Code Review Rules' starts at byte ${offset}; keep it under ${head_limit}." >&2
    echo "Truncation drops the tail, so the review rules must stay near the top." >&2
    exit 1
  fi
  echo "AGENTS.md ${size}/${limit} bytes; Code Review Rules at byte ${offset}."

Wired into justfile's ci: recipe and as a linux-checks step before just install — it is pure bash, needs no Flutter, and so costs nothing against the macOS-queueing long tail that #1016 is mapping. Note that nothing on main or develop is a required check here, so this shows as a red X you choose to read.

What was rejected, and why

The judging phase killed more than it kept. The most consequential:

• "Latin-script prose in intl_zh.arb is the tell" — wrong roughly 10:1. A judge measured 155 zh values containing a 3+ letter Latin run against 15 actually identical to English. The dominant cause is ICU plural syntax, which is ASCII in every locale by construction ({count, plural, one{пляшка} ...}). Any PR adding a plural string would draw a false accusation on a correctly translated line.
• Unconditional "no button: true / textField: true" — restored as conditional. Twelve committed sites set it deliberately; highlight_button.dart:82 sets it precisely because excludeSemantics: true strips the child's node. The unconditional form would have Codex asking first-time contributors to delete working accessibility code.
• Align as a container: true trigger — inverts the rule. AGENTS.md:220 offers Align as the documented alternative to the flag.
• "Must target develop" — 17 of the last 40 PRs legitimately target something else (main for release/hotfix, release/2.2.0, feature/ai-assisted-meal-logging, the last documented at CONTRIBUTING.md:18). Since Codex has no author filter this would fire on your own release PRs, the most sensitive in the repo.
• "Skip anything just ci enforces" as the silence predicate — every draft used a variant, and all were self-defeating: just ci includes format --set-exit-if-changed, but default_workflow.yml:133-137 deliberately omits it ("the codebase currently has accumulated pre-existing format drift"). The drafts silenced the reviewer on the one check CI does not perform. Replaced with the four steps linux-checks actually runs.
• Reporting format churn as carelessness — measured: PR feat: add scan and create-custom actions to no-results search state (#578) #995's add_meal_screen.dart is byte-perfect under the pinned formatter while main's version is 62 lines off. The churn is the contributor obeying CONTRIBUTING.md:71. The rule now names the cause and forbids suggesting a repo-wide reformat.
• Severity labels / P0-P1-P2 — gate grammar misrepresents a reviewer holding checks: read that cannot block.
• A mandatory opening compliment — with no author filter, ~40 manufactured compliments a year trains you to skim.

Corrections to this ticket's own brief

Both measured, both wrong as I stated them:

• @key metadata is not template-only. At HEAD: en 79, de/sk/tr 42, zh 39, cs/it/pl/uk 38. Raw counts therefore differ between two non-English locales (de 1,073 vs cs 1,069), so a reviewer given my explanation could still manufacture the false positive. Rule 2 is written as a mechanical instruction — strip @-prefixed entries before comparing anything — and names no locale as special.
• 946 is stale. Correct at PR feat: add scan and create-custom actions to no-results search state (#578) #995's merge base; HEAD sits at 1,031 real keys in all nine. This is precisely the failure the section's "never assert a count you have not computed from this PR's own diff" clause exists to stop, so no number appears anywhere in the final section.

The measurement that justifies rule 1's exact wording

Across the whole current tree, only seven keys are byte-identical to English in all eight non-English locales: appTitle and notificationsDailyReminderTitle ("OpenNutriTracker"), csvImportContributeOffAndroidLink ("Android"), csvImportContributeOffIosLink ("iOS"), settingsAccentSubtitleMaterialYou ("Material You"), and the placeholder-only {hour}:00 and {hour}:{minute}. Every one is already excused by rule 1's exception list.

So "identical in every non-English locale, minus brands/units/acronyms/placeholder-only" has a historical false-positive rate of zero on this codebase, while per-locale identity has a base rate of 15–75 per locale and is mostly cognates. PR #995's three keys were identical in all eight. That asymmetry is the single most load-bearing choice in the section — a future edit must not soften "every, or nearly every" into "any".