Let a grouping failure stop the review instead of falling back to per-file dispatch

[ThomasCarca]

Problem Statement

When the grouping LLM call fails, OCR falls back to one review subtask per file and reports it with a single stdout line (internal/agent/grouping.go:95-99):

groups, usage, err := callGroupingLLM(...)
if err != nil {
    fmt.Fprintf(stdout.Writer(), "[ocr] LLM grouping failed (%v), falling back to per-file dispatch\n", err)
    return groupDiffsResult{groups: toSingleFileGroups(diffs), usage: usage}
}

No run through the GitHub Action can see that line. action.yml:668 always passes --audience agent, which routes [ocr] progress to io.Discard through stdout.Quiet() (shared.go:338-346, internal/stdout/stdout.go:33-37). The flag is not an action input, so no user can change it.

Nothing else states what happened:

• There is no grouping.failed telemetry event to pair with the existing grouping.skipped (grouping.go:158-165).
• The --json result has no field naming the strategy that produced groups.
• The groups array narrows it and cannot name it. A grouping failure, a GroupingPerFile plan, and a template with no GROUPING_TASK all emit one group per file, labelled with its path (grouping.go:373-382). An enforceGroupTokenBudget split is the one case the labels do separate, because it appends " (split: <path>)" (grouping.go:351).
• retry_report does carry the failed grouping request. Its listing rule lists any logical request that retried, saw an error, or did not succeed, whatever the task type (internal/llm/retry_report.go:535-542). In our incident it was the only trace that named the failure. It proves the call failed, and it does not say what OCR then decided.

The fallback is expensive, and a caller pays for all of it before it can react. Our run turned a 188-file change set into 188 review conversations, and each one is entitled to --timeout minutes and MAX_REVIEW_ROUNDS rounds. The ocr review call alone took 47m40s inside a timeout-minutes: 50 job, so under 2m20s was left for every other step, and it still exited 0. The result JSON arrives too late to help, because shared.go:696-708 writes it after the agent run returns.

Proposed Solution

1. Add a policy flag, --on-grouping-failure=fallback|abort. With abort, review stops before it dispatches any subtask, and it exits non-zero. Keep fallback as the default so current behaviour does not change. --max-tokens-budget shows that a flag can already carry an exit-code contract in its help text (cmd/opencodereview/shared_flags.go:58), so the surface exists. Its own rule points the other way: it exits 0 on a partial run.
2. Report the partition in the result JSON. A grouping block would carry the strategy behind groups (llm, bundle_all, per_file) and, when the LLM call failed, the reason. GroupingStrategy.String() already produces those names. This one is for diagnosis after a run, not for prevention.
3. Emit a grouping.failed telemetry event next to grouping.skipped, with the same attribute set plus the error class.

Item 1 is the one we need. A CI job cannot stop a fan-out it never learns about, and a non-zero exit is all our workflow needs to fail fast.

Alternatives Considered

• Read the stdout line. It does not survive --audience agent, which the action always passes, and a log string carries no stability contract.
• Write the grouping-failure line to stderr. OCR already sends [ocr] diagnostics there whatever the audience (internal/telemetry/events.go:125, cmd/opencodereview/output.go:88), and the action captures stderr to a file and prints it, so a one-line change at grouping.go:97 would surface it. It still leaves a workflow gate parsing a log line, and it still pays for the whole fan-out first.
• Infer the failure from retry_report. It reports the request, not the decision: a failed grouping_task entry proves the call failed, and it still leaves the resulting partition unstated. It also arrives only after the run, which is the same problem the result JSON has.
• Cancel the CI run from the workflow. The workflow cannot see the fallback, so there is nothing to trigger the cancel on. With a non-zero exit there is also nothing left to cancel.
• Set --max-tokens-budget low enough to cut a per-file fan-out short. That truncates coverage instead of preventing a partition we did not ask for. The undispatched files land in coverage.failed as budget, so status reads partial and our gate does catch it, but only after we pay for the part that ran. It is also out of reach from the action today, since feat(action): expose --effort and --max-tokens-budget as action inputs #1147 is still open on exposing it as an input.

Affected Area

• Review Agent / LLM interaction
• CLI / Commands
• Telemetry / Observability

Additional Context

Read on v1.11.1, the version this run used: the workflow pins alibaba/open-code-review@v1.11.1 and passes ocr_version: "1.11.1". Commit 8d023aa is the same commit as that tag, so every line number above matches the version that produced the failure. We run OCR through the GitHub Action on a label, with a gate step that reads the result JSON and fails the job unless the review finished. That gate is why the fallback matters to us: today it can turn a degraded review into a green check.

The run this comes from

The run is on a private GitHub organisation, so there is no shareable URL. The uploaded ocr-result.json holds the whole story. OCR reviewed 188 files as 188 groups of one file each, which is the per-file fallback. It cost 21.4M tokens and 47m40s and produced 5 comments, and status reads complete, so our gate step passed a degraded review. The retry_report records exactly one failure out of 1357 requests:

{
  "model": "qwen/qwen3.8-max",
  "file_path": "__grouping__",
  "task_type": "grouping_task",
  "request_no": 1,
  "outcome": "failed",
  "attempts": [
    { "attempt": 1, "outcome": "error", "error_class": "timeout",
      "failure_phase": "context", "status_code": 200,
      "duration_to_headers_ms": 1308 }
  ]
}

That block is the only trace that names the failure. The 188 single-file groups show the shape and not the cause. It proves the grouping call failed, and it does not say that the partition became per-file. Whether the grouping call should also retry or carry its own deadline is a separate question, and we are not raising it here.

[lizhengfeng101]

Thanks for the unusually precise report — every line reference checks out, and internal/agent/grouping.go is unchanged from v1.11.1 through v1.11.5, so what you hit is still current.

complete is a coverage verdict, not a grouping verdict.

computeTerminal (internal/session/manifest.go:941) derives the state from the coverage sets alone — failed == 0 → StateComplete — and coverage is registered per file (registerCoverage → RegisterSelected(a.coverageItem(d))). TerminalState's doc comment is explicit that it is "computed only from the coverage sets plus run_failure, never from comment count or warnings."

All 188 files were reviewed and none failed, so complete is accurate. It means "nothing was left uncovered"; it never claimed "partitioned the way you expected." That separation is deliberate — agent.go:674-679 declines SetRunFailure for budget truncation precisely so the state stays coverage-derived, and a grouping fallback sits on the same side of that line: the denominator is intact and every file still gets reviewed.

So this isn't a status defect, and we'd rather not add --on-grouping-failure for it.

The real gap is in grouping, and your retry_report names it.

error_class: "timeout" + failure_phase: "context" maps to context.DeadlineExceeded (internal/llm/retry_boundary.go:52). With status_code: 200 and duration_to_headers_ms: 1308, the provider was healthy — the deadline expired during generation, while the model was still writing a grouping JSON for 188 files.

And that deadline isn't --timeout. groupDiffs receives a bare ctx (agent.go:630); the --timeout budget is computed afterwards at agent.go:647 and only wraps review subtasks (agent.go:725). What actually bounded the call is the client's per-request timeout, defaulting to 5 minutes (internal/llm/client.go:503-504) — the same number a single ordinary completion gets.

That's the design gap: the grouping call has no deadline of its own and doesn't scale with the change set it has to partition. Retrying wouldn't have saved it either — a retry hits the same 5 minutes. The right half was already in your own wording: "or carry its own deadline."

We'll treat this as a grouping problem and work on that side.

Your points 2 and 3 stand independently — a grouping failure, a GroupingPerFile plan, and a template with no GROUPING_TASK all pass through toSingleFileGroups with path labels, so they really are indistinguishable in groups today. Cheap to fix and worth doing regardless.