alibaba / alibaba/open-code-review

code_comment failure still yields exit 0 and complete coverage; clarify automated admission semantics

Open
#1,196 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
24.4k
Forks
1.8k
Avg merge
2d 4m
Merged PRs (30d)
102

Description

## Environment

- open-code-review v1.11.6 (`04284b5dc`), Darwin/arm64, built 2026-09-07T07:09:04Z.
- Provider `zai-anthropic`; actual review manifest model `glm-5.2`.
- `ocr review --audience agent --format json --concurrency 1 --from --to --rule --background-file `.
- Three changed Rust files grouped into one review group. Compact business context, no context-length warning.

## Observed result

Run `c82286dc-5885-4100-a219-4ac05a470941` printed:

```text
[ocr] ✘ code_comment failed: Error: 'comments' array is required. Got args: {}
```

The retained JSON reports:

```json
{
"status": "complete",
"tool_calls": {
"total": 15,
"failure": 1,
"failure_by_tool": {"code_comment": 1},
"failure_details": [{
"tool_call_number": 5,
"tool_name": "code_comment",
"file_path": ",,",
"arguments": "{}",
"error": "Error: 'comments' array is required. Got args: {}"
}]
}
}
```

The process exited 0. The manifest's `terminal_state` is `complete`, all three selected files are completed, and `failed`, `reused`, and `waived` are empty. The run has two final findings. No `warnings` field is present. Tool failure is available in the JSON counters and stderr, but not reflected in the top-level completion or coverage outcome.

Our review operator interpreted complete coverage and exit0 as sufficient despite the tool error. The supervising admission check caught the raw error and blocked the delivery. This makes the distinction between completed execution and successfully submitted review findings important for automated consumers.

## Requested clarification or change

Please document whether `complete` intentionally permits rejected comment submissions, and how a machine consumer should establish that such a failure was recovered without losing a finding. An explicit strict-admission outcome or a reconciled failure record would help distinguish recovered exploration errors from potentially lost comment submissions. This report does not assume every recoverable tool error must terminate a review.

## Related grouped-path observation

A separate run of the same range, `04eaf880-f652-4df1-a827-77505ac79435`, completed 3/3 with zero tool failures and empty stderr, but emitted five JSON `comment_refiled` warnings:

```text
comment filed against ,, describes code in ; re-filed
```

That run had two `code_comment` calls and seven findings. Please clarify the contract for grouped-path submissions and whether automatic re-filing can provide explicit attribution proof rather than leaving consumers to infer it from warnings.

## Evidence limits

Both are retained real-run observations, not a deterministic reproduction. We have OCR result JSON, stderr, exit markers and manifests, but not the raw provider tool-call payload. We cannot establish whether `{}` came from the model or OCR argument parsing, and are not claiming a parsing root cause. File names and local paths above are redacted. No credentials, reviewed source, or speculative source defects are included.

Contributor guide

Open the contributing guide

Research direction

Start with the `ocr review` execution path and inspect how retained result JSON, stderr, exit status, and the manifest's `terminal_state` are reconciled after tool failures. Compare the failed `code_comment` run with the zero-failure grouped-path run and trace `comment_refiled` handling. Done means the consumer-facing contract clearly distinguishes complete execution from successful admission and exposes grouped-path attribution evidence.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
devtools
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.