roborev can report a clean verdict without having reviewed the requested content (4 issues confirmed on v0.61.2)

[pmcfadin]

Context

We drive roborev from automated agents as a merge gate — a review must be confirmed clean before a PR
is allowed to land. That use case makes us unusually sensitive to one failure mode: roborev returning a
plausible-looking clean verdict when it did not actually review the requested content. A vacuous
No issues found. is textually identical to a genuine clean pass, so an automated consumer cannot tell them
apart without out-of-band evidence.

This is not hypothetical for us. On one of our changes, two runs reported "No issues found"; re-run correctly
against the right commit, the same diff produced two genuine blocking findings. Accepting the first
verdict would have merged unreviewed code.

We ended up building a defensive wrapper that verifies roborev's output against independently-computed facts.
In doing so we found the following. Everything in §1 I reproduced directly on v0.61.2; §2 is honestly
flagged as older evidence I have not re-verified; §3 is a doc/UX note, not a bug.

Happy to split this into separate issues if you'd prefer — each section is independently actionable. Also
happy to test patches; we exercise this heavily.

Environment: roborev v0.61.2 (static release), Linux x86_64, --agent codex --model gpt-5.6-sol,
daemon healthy, 4 workers. Clean database (roborev repo list was empty at session start), so the job ids
below are a fresh 1 and 2.

---

1. Confirmed on v0.61.2

1a. --wait returns before the job record is durable

roborev review <sha> --wait returns, but the job record is not yet readable. Immediately after the
command exited, roborev list --json for job 2 had no git_ref, no status, no model, and no
token_usage. Queried again moments later, all four were populated:

id=2 status='done' git_ref='155e12c8e8d1af9f75e47ca854e87f489b818fe6' model='gpt-5.6-sol' token_usage=<present>

Repro: run roborev review <sha> --repo <abs> --agent codex --model gpt-5.6-sol --wait, then immediately
roborev list --json (or roborev show <job> --json) in the same shell. Re-query a few seconds later.

Impact: a consumer that reads the record right after --wait — the natural thing to do — silently gets a
degraded view. In our case four independent verification signals fell back to weaker sources simultaneously,
with no error and no indication anything was missing. We now poll with a bounded retry, but ideally --wait
would not return until the record it produced is readable (or would expose an explicit "record durable"
signal).

1b. token_usage is a JSON-encoded string nested inside JSON

"token_usage": "{\"input_tokens\":67387,\"cached_input_tokens\":43520,\"total_output_tokens\":2232,\"usage_source\":\"job_log_turn_completed\",...}"

It's a string field whose contents are JSON, so every consumer must decode twice:
json.loads(json.loads(record)["token_usage"]).

Impact — this one cost us a real, silent defect. Our first implementation parsed one level and read the
field names we'd inferred from the binary's struct tags. Result: our token-based check reported
UNAVAILABLE on every single run — a verification step that appeared to exist but never once executed.
We only caught it by noticing the field was never populated on a real review. Emitting token_usage as a
nested object would remove this whole class of consumer error.

1c. Asymmetric token field naming

Input is input_tokens and cached_input_tokens, but output is total_output_tokens. Minor on its own,
but it's the specific inconsistency that made 1b's silent failure easy to write and hard to spot — we
guessed output_tokens and got a plausible-looking "no data" result rather than an error.

1d. roborev repo has no add subcommand, so a git worktree cannot be registered

Available subcommands are list, show, rename, move, delete, merge. Registration only happens
implicitly on first use.

We use git worktree extensively (one worktree per in-flight change), and an unregistered worktree appears
to be the root cause of the --branch misresolution in §2a below: with no registered repo matching the cwd,
--branch has no correct repo to resolve against. A roborev repo add <path> would make the worktree case
expressible, and would also let a consumer verify registration rather than hoping first-use did the right
thing.

---

2. Older evidence, NOT re-verified on v0.61.2

Flagging the verification gap rather than presenting these as current facts — they may already be fixed. I
can re-test and follow up if useful.

2a. --branch from inside a git worktree resolves against the ROOT checkout and reviews the BASE commit

Observed enqueueing 39900e4db (= origin/main, the base) when the worktree branch HEAD was 4e7ab591e.
Five consecutive jobs all enqueued the base commit. The reviewer then receives an empty diff and replies
No issues found. Summary: The provided combined diff contains no code changes to review.

Likely narrower framing: this may be entirely about the absence of an explicit --repo. If --branch
resolves correctly when given --repo <abs-worktree>, then the precise bug is "--branch silently resolves
against a different repo when the cwd is an unregistered worktree" — which would be both clearer and more
tractable. Either way, silently resolving to a different commit than the user asked for seems worth an
error rather than a review.

2b. The two-positional commit-range form enqueued a commit that is neither endpoint

roborev review 89fdbb895 989d7d2c3 enqueued 90a17d376 — neither the start nor the end of the range.

2c. A code-free (docs-only) diff is silently discarded on a correctly-targeted run

roborev review <sha> --repo <abs> --agent codex --model gpt-5.6-sol --wait on a 5-file, +167/−63,
all-markdown diff: correct SHA enqueued, correct repo, and yet
No issues found. Summary: The provided diff contains no code changes to review. Reproducible across two
runs.

Token accounting was the only way to tell: 18,700 input / 0 cached / 53 output / 8s, versus 398k–649k
input with 314k–554k cached and 5.0k–6.3k output over ~2.5 minutes for genuine reviews of comparable diffs.
(A known-empty diff measured 17,333 input / 21 output — i.e. the docs-only run sat right on the empty-diff
baseline.)

This is the most consequential one, because it passes an enqueued-SHA check — right SHA, right repo,
plausible clean verdict — so verifying the reviewed commit cannot catch it. If a code-free diff is
deliberately out of scope, an explicit non-zero refusal would be enormously better than a verdict that is
byte-identical to a clean pass.

---

3. Doc/UX note (not a bug)

3a. roborev review <sha> reviews one commit, which is easy to misread as "the branch"

Confirmed on v0.61.2: commit 155e12c touches 3 files, while the branch's diff against its base is 27
files / +3603/−74. Only the single commit's diff reached the reviewer (confirmed via the prompt length and
content).

This is presumably intended semantics. But an automated caller told to "review this branch" that reaches for
the single-SHA form gets a partial review reported as a complete one — for any multi-commit branch,
"clean" silently means "the last commit was clean." A line in review --help distinguishing the single-commit
and branch-range forms, and/or echoing the reviewed file count in the output, would make this
self-evident. Echoing the file count would incidentally make 2c self-diagnosing too.

---

The one change that would matter most

The unifying theme across 1a, 2a, 2b and 2c is that each fails by producing a plausible success rather than
an error:

When roborev does not actually review the requested content — wrong repo, wrong or partial range,
unsupported/code-free diff, or a record that isn't readable yet — it would be far safer to fail loudly and
non-zero than to return a clean verdict.

For interactive use a vacuous "No issues found." is a minor annoyance. For anything gating a merge, it's
indistinguishable from success, and that's the difference between a review tool and a review guarantee.

Thanks for roborev — it's genuinely useful, and the findings it produces when correctly targeted have caught
real bugs for us. These reports are all in service of trusting it more, not less.

[pmcfadin]

Correction and update after further testing

I have a retraction on §1a, plus verified results that upgrade three of the §2 items from "not
re-verified" to confirmed on v0.61.2 with mechanisms. Correcting the record first, since §1a was my
highest-confidence claim and it was wrong.

❌ RETRACTED — §1a "--wait returns before the job record is durable"

This is not a roborev bug. It was a parsing error on my side, and I misdiagnosed it.

roborev show <id> --json returns a review row whose top level is
[agent, closed, created_at, id, job, job_id, output, prompt, uuid, verdict_bool] — and the job fields are
nested under the job key:

job keys: [agent, agentic, branch, commit_id, commit_subject, enqueued_at, finished_at, git_ref, id,
           job_type, model, patch_id, prompt_prebuilt, reasoning, repo_id, repo_name, repo_path,
           requested_model, retry_count, review_type, session_id, started_at, status, token_usage,
           verdict, worker_id]

git_ref, status, model and token_usage are all present there, immediately. My code read the top
level of show --json, found them missing, and I inferred a write-durability race from the fact that a
later list --json query showed them. The data was available the whole time. There is no race. Apologies for
the noise.

Residual, and it is minor: show --json and list --json return different entity shapes for the same
integer id — list returns job rows with those fields at the top level, show returns a review row with
them one level down. That is a legitimate design choice, but it is an easy consumer footgun (it fooled me),
and a line in show --help noting that the job fields live under job would have saved this entirely. Feel
free to treat that as a docs nit or close it outright — it is not a bug.

I have left §1b, §1c and §1d as filed; those are unaffected and independently verified. §1b in particular
still stands on its own: token_usage is a JSON-encoded string requiring a double decode, wherever you
read it from.

---

✅ CONFIRMED on v0.61.2, with mechanisms — the §2 items

I re-tested all four invocation forms against a real 17-commit branch whose diff is 27 files (22 markdown +
5 code), measuring both the enqueued git_ref and which files actually appear as diff --git headers in
the prompt sent to the reviewer:

form	enqueued git_ref	code files present in the prompt
--branch --base <base> --repo <abs>	<base40>..<head40>	5/5 ✅
--since <base> --repo <abs>	<base40>..<head40>	5/5 ✅ (byte-identical prompt)
<base> <head> (two positionals)	4b825dc6…(EMPTY TREE)..<head40>	3/5 ❌
<sha> (single commit)	<head40>	3/5 ⚠️ one commit only

§2a — narrowed and confirmed. --branch is correct when given an explicit --repo. So the bug is
precisely: --branch without an explicit --repo, run from an unregistered git worktree, silently
resolves against a different repo (the root checkout, which typically sits on the base branch) and reviews
the base commit. With --repo it resolves correctly and reviews the full range. This is the narrow framing I
suggested in the original report, now measured — and it strengthens the §1d ask, since a registered worktree
would presumably also resolve correctly.

§2b — confirmed, with a sharper root cause. The two-positional range form enqueues
4b825dc6…..<head>, where 4b825dc6ddd64… is git's empty-tree hash. So it does not diff the
requested range at all — it diffs the head against the empty tree. (My original report said it enqueued "a
commit that is neither endpoint"; the empty-tree hash explains that observation precisely.) It still yielded
only 3/5 code files, so the resulting prompt is also not a faithful whole-tree diff.

§2c — confirmed, and I found the mechanism. roborev excludes non-code paths from the diff it builds.
All 22 markdown files in my census are absent from the prompt; only the 5 code files appear. So for a
markdown-only diff the constructed diff is genuinely empty, and No issues found. Summary: The provided diff contains no code changes to review. is a truthful report of an empty input rather than a reviewer
malfunction. That reframes the ask and makes it much easier to act on:

The exclusion itself is defensible. The problem is purely that the outcome is indistinguishable from a
clean pass. A docs-only diff should exit non-zero with something like "no reviewable files after
filtering (22 files excluded)", not return a clean verdict. Echoing the included/excluded file counts would
make this self-diagnosing — and would also make §2b and §3a self-diagnosing.

§3a — confirmed. The single-SHA form reviews one commit (3/5 code files here, being the last commit's
subset), not the branch range.

---

What this changes about the original ask

The unifying request stands, and §2c sharpens it: the highest-value fix is echoing what was actually
reviewed — the resolved range and the count of files included versus filtered out. Every one of §2a, §2b,
§2c and §3a would become immediately self-diagnosing, without any change to the filtering or resolution
behaviour itself. That seems like a much smaller change than fixing each path individually, and it is the one
I would prioritise if you only wanted to do one.

For our part we now invoke --branch --base <base> --repo <abs-path> exclusively and verify the reviewed
range and the per-file prompt coverage independently before trusting a verdict. Glad to test any patch.