mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-08-27 09:32:45 +00:00
a66240084b
* feat(agent): group semantically related files for multi-file review
Introduce LLM-based semantic file grouping so that related files (e.g.
implementation + test, i18n variants, interface + impl) are reviewed
together in a single LLM call instead of individually. This enables
cross-file consistency checks and reduces total LLM calls.
Key changes:
- Add grouping.go with LLM-based file grouping (GROUPING_TASK template)
that clusters changed files by semantic relationship, with fallback
to per-file dispatch on any error
- Refactor dispatchSubtasks to iterate over FileGroup instead of
individual Diff, updating budget estimation, error handling, panic
recovery, and session recording to work at group granularity
- Move the 'path' field from tool-level to per-comment level in
code_comment tool schema, since one review call now covers multiple
files — remove the forced path override in loop.go
- Rewrite plan phase output from JSON to a structured Review Directive
with MUST/SHOULD/MAY severity tiers and concrete verification actions
- Update main_task prompts to accept {{diffs}} (multi-file XML) instead
of {{diff}} + {{current_file_path}}, with cross-file review guidance
- Add enforceGroupTokenBudget and enforceMaxFilesPerGroup safety limits
- Treat partial completion (comments produced before round exhaustion)
as partial success instead of hard error
* test(agent): improve grouping test coverage
* fix(llmloop): add per-item path to thinking backfill tests
ParseComments skips comments with empty path. After removing the
top-level args path override, these tests need path in each comment
object to match the new per-item path contract.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(llmloop): backfill comment thinking from turn output (#773)
* feat(llmloop): backfill comment thinking from the turn's reasoning or message
Expose ChatResponse.ReasoningContent and backfill per-comment thinking
with the current turn's reasoning content, falling back to the assistant
message for models that do not expose reasoning, so --format json output
carries thinking even when the model omits it.
* fix(llmloop): drop content fallback for comment thinking backfill
The turn's assistant message is usually a short user-facing preamble
rather than real reasoning, so backfill per-comment thinking only from
the model's native reasoning_content and leave it empty otherwise.
Add a full-wiring RunPerFile test for the reasoning backfill and a
regression test that fails if the content fallback returns. Sync the
thinking docs across en/zh/ja/ru.
* docs(llmloop): note that turn-level thinking is shared by design
Document in the main loop and at the code_comment backfill site that
the model emits reasoning once per turn, so every tool call and
comment in the same turn intentionally shares the same thinking.
* fix(tool): fall back to newPath when comment omits path field
The multi-file grouping change moved path from a top-level arg
to per-comment objects and removed the args["path"] = newPath
injection. This broke single-file RunPerFile calls where the model
does not emit path per comment.
Add ParseCommentsWithPath that applies newPath as a default when
comments lack an explicit path, restoring the previous behavior.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(prompt): remove redundant Comment Attribution section
The path field is already marked required in the tool JSON schema
with a clear description. Repeating it in the system prompt wastes
tokens without adding signal.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(agent): rename file_metadata_table to file_list
The placeholder renders a plain one-per-line list, not a table.
The old name misleadingly suggests markdown table formatting.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(prompt): reference actual <file> elements instead of nonexistent <review_files>
The system prompt mentioned <review_files> but no such tag exists in
the rendered user message. The files are wrapped in <file path="...">
elements by buildConcatenatedDiffs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(prompt): wrap diffs in <review_files> container
Add <review_files> wrapper in the user prompt template around {{diffs}}
so the system prompt can reference it as the explicit review scope,
consistent with <other_changed_files> and <user_task> containers.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(prompt): clean up system/user prompts for multi-file review
- Consolidate scope constraints into Strict Focus Rules; remove
duplicated 'don't' items from Capabilities
- Remove redundant path instruction from user prompt (already in
tool schema)
- Fix末尾 instruction to reference <review_files> instead of <file>
- Fix plan output format: each category numbers independently
- Replace // comments with plain text in user prompts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* docs(agent): restore design-decision comments lost during refactor
The grouping refactor deleted explanatory comments about budget
look-ahead semantics, why SetRunFailure is NOT used, and the
panic-isolation contract. These explain non-obvious WHY decisions
and are restored in condensed form.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(prompt): restore review recall lost to plan-phase suppression
The semantic-grouping branch cut comment recall from 20.0% to 12.7%
(F1 25.2% -> 19.1%) on the 194-PR benchmark. Ground truth shows the
396 dropped comments had a 27.8% hit rate — only 14% better than a
random cut of the same size, so the loss was indiscriminate thinning
rather than targeted noise removal.
Four causes, all addressed here:
- The plan prompt lost its third severity tier. code_comment accepts
four severities but the plan offered only MUST/SHOULD, so nothing
could feed a `low` comment. Add CONSIDER.
- main_task_system.md lost its only positive instruction to produce
feedback ("pointing out areas for improvement"), leaving a prompt
made entirely of restrictions. Restore it and the obligation wording
in Role.
- Nothing obliged the model to cover every file in a group, so secondary
members were starved: .h files lost 79% of their comments and went
silent 68% of the time, against ~30% for implementation files at the
same comment density. Gate task_done on a per-<file> pass.
- PLAN_MODE_LINE_THRESHOLD was calibrated per file but is now compared
against a group's summed churn, making the plan phase effectively
unconditional. Gate on the largest single-file churn instead.
Also relax the [deep] abandonment threshold from 3-4 to 6-8 tool calls
(tool calls per file had fallen 4.12 -> 2.92) and make plan adherence
rule 4 an obligation rather than a permission.
The plan.skipped event keeps its original lines.changed attribute and
gains lines.changed.max_file, so existing dashboards keep resolving;
the telemetry docs are synced across all four locales, which also
picks up the file.path -> group.label rename from the grouping commit.
Round-budget and failure-attribution fixes are deliberately left out so
their effect can be measured separately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(llmloop): use per-comment path in re-location identity tests
These predate the code_comment schema migration (path moved from
tool-level to per-comment level in the grouping commit) and only
surfaced once the grouping commit was rebased ahead of them: the
tool-level path was silently ignored, comments were dropped for an
empty path, and TestReLocation_Identity got 0 requests instead of 1.
* refactor(prompt): emit the review plan as plain text instead of JSON
Nothing parses the plan output: executeGroupPlanPhase hands
resp.Content() straight into {{plan_guidance}}, so the JSON envelope
only cost output tokens and forced the model to escape newlines in the
code snippets every plan item quotes. Malformed JSON degraded silently
into the main-task prompt because no validation stood between them.
Keep the exact field set (change_summary, issues[].severity,
issues[].description, issues[].tool_guidance[].name/arguments/reason)
and carry it in plain text instead: a Summary: line, numbered issues
tagged [high|medium|low], and one arrow line per tool guidance. Headings
and code fences are now forbidden in the output, because the plan is
injected under a "### Review Plan" heading inside <user_task> and the
plan result is never fence-stripped. Align the plan user message, which
still asked for a JSON fence.
Restore the main_task system prompt to its pre-grouping wording, keeping
the trimmed Role section and dropping Review Plan Adherence, whose
MUST/SHOULD/CONSIDER and [quick]/[deep] vocabulary no longer exists in
the plan output. Two rules stay rescoped to the group instead: Strict
Focus Rules would otherwise tell the model to ignore its own group
members, and without the per-file pass requirement a group's secondary
files get marked completed in the manifest without ever being read.
* fix(agent): tag each rule block with the files it governs
A group spanning two languages resolved two rule sets and concatenated
them with a bare newline, so the model received (say) the Java checklist
immediately followed by the MyBatis one and nothing saying which file
each governs. A group holding exactly that mix is what the grouping
prompt asks for: interface plus implementation, i18n/config variants of
one resource.
Group the files by resolved rule text and wrap each block in
<rules for="...">, matching the <review_files>/<file path="..."> framing
already used for the diffs. A group covered by a single rule set — every
single-file group, and every group whose files share a language —
returns that text bare, so the rendered prompt stays byte-identical for
those runs and the prompt-cache prefix does not churn.
Resolve in path order as well. The diffs arrive in the grouping LLM's
response order, which varies between runs and would otherwise reorder
the blocks; clone before sorting, since the caller's slice is shared
with a.diffs.
Drop the four single-file helpers the grouping refactor left behind
(resolveSystemRule, buildChangeFilesExcept, executePlanPhase,
buildFilterCommentsJSON). All four had no caller outside their own
tests, and executePlanPhase still substituted {{current_file_path}} and
{{diff}}, placeholders no longer present in any template — reading it
suggested the single-file path was still live.
Their tests are ported to the group functions rather than deleted:
executeGroupPlanPhase, buildChangeFilesExceptGroup and
buildGroupFilterCommentsJSON had no coverage at all, so deleting the
tests with the code would have left the live path untested. The ports
assert what the group versions changed — plan records filed under the
group key, comment ids running globally across files, every group member
excluded from the change-file list including a renamed member's old
path.
* fix(agent): drop the stray trailing newline from the change-file list
buildChangeFilesExceptGroup appended the separator after each entry,
guarded by the entry's index into a.diffs. The loop skips binaries and
every group member, so whenever the final diff was one of the skipped
ones the last emitted entry still passed the index test and the list
ended with a newline. Excluding a whole group rather than a single file
makes that the common case, not the rare one.
Emit the separator before each entry instead, guarded on anything having
been written yet. Separators between entries were never affected — any
emitted entry followed by another necessarily sits below the final index
— so this only removes the trailing one.
Assert the exact string in three cases: a skipped final diff, a skip
between two entries, and everything excluded. The existing checks used
strings.Contains, which cannot see a trailing newline at all.
* fix(prompt): adapt the review filter to multi-file groups
The filter template still described its input as "one file's diff" and
fenced it with ```{{path}}, while executeGroupReviewFilter substitutes
the comma-joined group key and a multi-file <file> sequence. The fence
language read ```src/a.go,src/b.go, and the diff body is now XML that can
itself contain a fence and truncate the block.
The substantive cost was Ground A. Its shapes are inherently per-file —
"discusses the body of a function, on a file that only declares it",
"discusses host-language logic on a file that holds none" — but nothing
told the model to pair a comment with the <file> matching its path, so a
sibling file holding the construct could rescue a comment that its own
file refutes. The filter got more lenient, which is its safe direction,
but the ground stopped doing its job.
State the pairing rule up front, and keep the two grounds asymmetric:
Ground A is judged against the subject file alone, Ground B against any
file in the group, because a comment calling an identifier unused is
genuinely wrong once any of these files uses it. Sibling files stay
usable as evidence — that is what group-level filtering is for — they
just cannot substitute for the subject file.
Carry the diffs in <review_files> instead of a fenced block, matching
main_task_user.md, which also retires the bogus fence language and the
truncation risk. The system message loses "you can see only a single
diff" for the same reason. {{path}} is left substituted in agent.go: no
shipped template uses it now, but a user template still might.
* fix(agent): classify group coverage per file, not per group
A file group that stops before task_done (round/token budget
exhaustion) could still have produced usable comments for some of its
files. The dispatch loop was marking every file in the group as
Failed regardless, so a single stuck file dragged its whole group -
and, when it was the only dispatched group, the whole run - down to
terminal_state=failed even though real coverage existed.
Classify each file by whether it actually has comments: files with
output are marked Completed, files without stay Failed. This lets
computeTerminal report partial (exit 0) instead of failed (exit 1)
when a group is a genuine mix of the two.
Also fix subtaskFailed to count only the files marked failed in this
loop instead of assuming reportAsError implies the whole group
failed - a group can now legitimately end up partially completed.
* feat(agent): add multi-round review to improve recall coverage
Wrap the Main Loop + Review Filter in a per-group round loop
(default MAX_REVIEW_ROUNDS=2). Each round after the first injects
confirmed findings from prior rounds into the prompt, instructing the
model to find different issues rather than repeating known ones.
Key design points:
- Round 1 prompt is byte-identical to the previous single-round behavior
- Round 2+ strips the plan to avoid it acting as a coverage ceiling
- Per-path baselines (not global Snapshot) for concurrency safety
- Per-round filter ensures only verified findings enter confirmed set
- Early stop: zero new confirmed, or >= 30 confirmed, or budget exceeded
- Per-group timeout auto-scales 1.5x when rounds > 1
- budgetExceeded field converted to atomic.Bool to fix data race
- UTF-8 safe rune-based truncation for confirmed comment serialization
* feat(config): raise MAX_TOOL_REQUEST_TIMES to 100, minMaxTools to 50
Grouped file review needs more tool call rounds than single-file mode
since one RunPerFile call now serves multiple files. Raise the default
from 30 to 100 and the user-configurable minimum from 10 to 50.
* feat(config): add group plan threshold and separate prompt/completion token limits
- Add PLAN_MODE_GROUP_LINE_THRESHOLD (default 120) so multi-file groups
trigger the plan phase when their combined churn is substantial, even
if no single file exceeds the per-file threshold. This fixes round-1
review quality regression for grouped files.
- Separate MAX_TOKENS (prompt ceiling, raised to 200000) from
MAX_COMPLETION_TOKENS (output cap, 16384). Previously both roles were
conflated in a single value. This allows larger file groups to stay
intact without inflating the model's output budget.
- Add Template.PlanRequired() helper with dual-threshold logic
- Extract groupChurn() helper from executeGroupSubtask
- Remove runtime MaxCompletionTokens=MaxTokens assignment in review/scan
commands (now loaded from template JSON directly)
* feat(config): add --effort flag to control review rounds
Introduce a review effort preset (--effort low|medium|high) that maps
to the number of review rounds:
- low: 1 round (fast, suitable for large PRs or time-sensitive CI)
- medium (default): 2 rounds (best balance of recall and cost)
- high: 3 rounds (maximum coverage)
Supported via CLI flag, persistent config (ocr config set effort low),
and three-tier precedence (CLI > config > default).
Also reorder tools.json to place file_read after code_search.
* feat(output): include file group info in --format json output
Add a 'groups' field to the JSON output showing which files were
grouped together for review. Each entry has a label and file list.
This enables post-run analysis of grouping behavior for evaluation.
Also reorder Strict Focus Rules in main_task_system.md and move
file_read after code_search in tools.json.
* fix(prompt): rewrite Strict Focus Rules for clarity
- Lead with positive scope instruction (review every file individually)
- Encourage cross-file observations within <review_files>
- Clarify that context tools are for background info, and comments must
target code within <review_files> — avoids ambiguity that could
suppress legitimate findings discovered via context gathering
* style: gofmt -s output.go
* docs: sync documentation with semantic grouping and effort features
Update all four language variants (en, zh, ja, ru) to reflect:
- MAX_TOOL_REQUEST_TIMES default 30 → 100
- MAX_TOKENS 58888 → 200000 (prompt ceiling only)
- New MAX_COMPLETION_TOKENS = 16384 (output cap)
- New PLAN_MODE_GROUP_LINE_THRESHOLD = 100
- New --effort low|medium|high flag (1/2/3 rounds)
- Semantic file grouping pipeline
- --max-tools raise-only semantics (min 50)
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: xujiejie <80671406+yingjiexu2002@users.noreply.github.com>
|
||
|---|---|---|
| .. | ||
| apply_provider_field_test.go | ||
| arg_errors.go | ||
| arg_errors_test.go | ||
| background_file.go | ||
| background_file_test.go | ||
| bedrock_config_test.go | ||
| budget_output_test.go | ||
| color.go | ||
| color_test.go | ||
| compat_test.go | ||
| completion.go | ||
| config_cmd.go | ||
| config_cmd_test.go | ||
| config_dispatch_test.go | ||
| config_runset_test.go | ||
| config_unset_error_test.go | ||
| delegate_cmd.go | ||
| delegate_exec_test.go | ||
| delegate_helpers_test.go | ||
| emit_run_result_test.go | ||
| flag_suggest.go | ||
| flag_suggest_test.go | ||
| flags_test.go | ||
| git.go | ||
| git_test.go | ||
| llm_cmd.go | ||
| main.go | ||
| manual_e2e_retry_test.go | ||
| misc_helpers_test.go | ||
| output.go | ||
| output_color_test.go | ||
| output_file_test.go | ||
| output_helpers_test.go | ||
| output_manifest_test.go | ||
| output_test.go | ||
| parent_cmd_test.go | ||
| procattr_unix.go | ||
| procattr_windows.go | ||
| progress_stream_e2e_test.go | ||
| provider_cmd.go | ||
| provider_cmd_test.go | ||
| provider_config_apply_test.go | ||
| provider_tui.go | ||
| provider_tui_cpinput_test.go | ||
| provider_tui_customform_test.go | ||
| provider_tui_deleteconfirm_test.go | ||
| provider_tui_editsave_test.go | ||
| provider_tui_funcs_test.go | ||
| provider_tui_manualenter_test.go | ||
| provider_tui_modeltui_test.go | ||
| provider_tui_persist_test.go | ||
| provider_tui_rollback_test.go | ||
| provider_tui_savefail_test.go | ||
| provider_tui_test.go | ||
| retry_fake_llm_test.go | ||
| retry_report_e2e_test.go | ||
| retry_report_render_test.go | ||
| review_cmd.go | ||
| review_cmd_test.go | ||
| review_helpers_test.go | ||
| review_mcp_more_test.go | ||
| review_resume_more_test.go | ||
| root.go | ||
| rules_check_test.go | ||
| rules_cmd.go | ||
| sarif.go | ||
| sarif_test.go | ||
| scan_budget_json_test.go | ||
| scan_cmd.go | ||
| scan_cmd_test.go | ||
| scan_helpers_test.go | ||
| scan_resume_more_test.go | ||
| session_cmd.go | ||
| session_cmd_test.go | ||
| session_complete_test.go | ||
| session_display_more_test.go | ||
| shared.go | ||
| shared_flags.go | ||
| shared_llmruntime_test.go | ||
| shared_test.go | ||
| shell_unix.go | ||
| shell_windows.go | ||
| smallfiles_test.go | ||
| version.go | ||
| viewer_cmd.go | ||
| zero_args_test.go | ||