Prove the value by hand before installing anything: review PR #995 with /code-review

[simonoppowa]

Part of #1023

Question

Is AI first-pass review on an outside fork PR worth automating at all?

Nothing gets installed until this is answered. No GitHub App, no contents: write grant to a third party, no config committed. The local /code-review skill runs against a real fork PR by hand, and the output decides whether #1034 is worth doing.

The subject: #995 (ibrahim-iqbal, feat: add scan and create-custom actions to no-results search state). It is the right test rather than merely an available one:

• A genuine fork PR (isCrossRepository: true) — the axis on which every candidate tool differs.
• 12 files, +213/-39, touching lib/features/add_meal/presentation/add_meal_screen.dart and no_results_widget.dart — new interactive widgets, so it should trip the Semantics(identifier:) convention if anything does.
• Eight ARB files — so it should trip the nine-ARBs-at-equal-key-count rule.
• Has an originating issue (Add next-step actions to empty food search results #578), so the skill's Spec axis has something real to check against.
• It targets main. Feature work here goes to develop. Whether the review notices is a free extra signal.

Run it without disturbing the working tree — no checkout, no stash:

git fetch origin pull/995/head:pr-995

then review main...pr-995, passing AGENTS.md as the standards source. (The skill's step 3 discovers standards files explicitly and hands them to its sub-agents, so the CLAUDE.md-is-a-stub problem does not block this — but pass AGENTS.md by name rather than trusting discovery to find a file called AGENTS.md when the documented examples are CODING_STANDARDS.md and CONTRIBUTING.md.)

Judge, and write the verdict down:

• Did it catch the repo-specific conventions — the Semantics(identifier:) gap and the ARB key parity? These are the half no generic reviewer can do, and the entire case for automating rests on them. A miss here is the finding.
• How much was noise? Count findings worth acting on against the total. Read it as ibrahim-iqbal would, not as its author.
• Did the Spec axis add anything over the Standards axis, given Add next-step actions to empty food search results #578 exists?
• Would this have helped or discouraged the contributor?
• How long did it take, and how much attention did it cost you? That is the number that decides whether unattended is worth paying a third party in contents: write for.

Verdict is one of: worth automating — proceed to #1034 and pick a tool; worth doing by hand — keep it manual, close #1034 and #1028 as unneeded; not worth doing — the whole map closes, which is a real outcome and not a failure.

Do not clean up pr-995 until the verdict is recorded — a second opinion may want the same diff.

[simonoppowa]

Verdict: worth automating

Ran /code-review against origin/main...pr-995 with AGENTS.md + CONTRIBUTING.md as standards sources and issue #578 as the spec. Both axes returned in under about two minutes. Every load-bearing finding below was re-verified by hand before recording it.

It caught a real violation that CI cannot

Three of the four new strings are verbatim English in all eight non-English locales — 24 strings. noResultsFound is translated (de "Keine Ergebnisse gefunden", zh "未找到结果"), but noResultsSubtitle, noResultsScanBarcode and noResultsCreateCustomFood are English everywhere.

CONTRIBUTING.md:54 is explicit: "Provide a real translation for each locale — do not leave the English string in as a placeholder."

And it is structurally invisible to CI: just check_l10n fails on missing keys via l10n_untranslated.json (CONTRIBUTING.md:60). A key that is present but still English passes. This PR is green and violates a documented standard. That is the single most valuable thing the review produced.

It correctly passed what the contributor got right

This matters more than the catches — a reviewer that manufactures findings on a first-timer's PR is worse than none.

• ARB key parity: verified correct. All nine locales at 946 real keys. Raw JSON counts differ (en 1011, de 986, cs 982) purely because @key placeholder metadata lives only in the template — exactly as AGENTS.md prescribes. A naive reviewer would have flagged this as a violation. It counted properly and passed it.
• Semantics(identifier:): verified correct. search-no-results-scan-barcode and search-no-results-create-custom-food — kebab-case, locale-independent, identifier: only with no doubled-up button: true, and container: true correctly absent because the parent is a Wrap, not a layout-greedy flex. It reasoned about the gotcha rather than pattern-matching it.

The Spec axis earned its separate existence

It found two behavioural regressions a standards pass cannot see, both verified against the bloc rather than the widget:

1. Recent tab, empty query, no history (add_meal_screen.dart:497) now renders the actioned widget asserting "No foods matched that search" — when no search ran. Directly against criterion 1.
2. All-tab provider outage: both blocs failing yields merged.isEmpty with neither Initial, so during an outage the user is told no foods matched, with no retry. Against criterion 6. The same subtitle also now prints underneath real results at the remoteSourceEmpty trailing item.

Also: widget tests cover 1 of the 5 states the spec named — three tests exist but two are callback-nullability permutations, not states. And ~40 of the 105 changed lines in add_meal_screen.dart are unrelated dart format churn (the known SDK skew), not requested.

It was also fair where the spec was at fault: criterion 9 asks for "manually maintained generated localization files", but lib/generated/ is gitignored and regenerated by just gen_l10n. It called that an issue defect rather than marking the contributor down.

Judged against the ticket's criteria

Caught repo-specific conventions?	Checked both, correctly found them satisfied, and caught a different documented violation CI misses
Noise	Low. 1 hard violation + 4 grounded judgement calls; 9 criteria each with evidence
Spec axis add value over Standards?	Yes — both regressions are invisible to a standards pass
Fair to the contributor?	Yes — credits what they got right, blames the spec where the spec is wrong
Attention cost	~2 min wall clock, unattended

One factual slip: the Spec agent reported ARB counts as "1031 per locale" (the real figure is 946, or 1011 raw for en). It did not change any verdict, but it is a reminder that a review needs the maintainer's eye on any number it asserts.

Consequence

Proceed to #1034 and choose a tool. The bar this sets is concrete: a candidate must read AGENTS.md/CONTRIBUTING.md well enough to reason about the @key metadata exception and the container: true gotcha, not merely grep for Semantics.

Note also that #995 has genuine defects and was sitting unreviewed — the pile this map was drawn against is real.

Local ref pr-995 is left in place for a second opinion. The review has not been posted to #995; that is an outward-facing action awaiting the maintainer's say-so.