601 commits

This branch

This branch

All branches

Author	SHA1	Message	Date
Ben Younes	e93197d6e4	fix(llm): serve GPT-5.6 models via the OpenAI Responses API (#559) (#938) ... * fix(llm): serve GPT-5.6 models via the OpenAI Responses API (#559) The built-in "openai" preset listed gpt-5.6-sol/terra/luna but used the Chat Completions protocol (/v1/chat/completions). Selecting one of those models for `ocr review` fails with a 400 because GPT-5.6 tool calls with reasoning effort are only supported through the Responses API (/v1/responses). Move the GPT-5.6 models to a dedicated "openai-responses" preset that uses ProtocolOpenAIResponses, so a user can select them without knowing which OpenAI API protocol is required. Other openai models are unchanged. Fixes #559 * fix(llm): separate Responses API credentials Use OPENAI_RESPONSES_API_KEY for the built-in Responses preset and document it across the site translations.\n\nRED→GREEN: focused provider test failed on OPENAI_API_KEY, then passed with the separate key. Full local CI remained green at 91.3% coverage. * chore(llm): remove verbose provider comment * refactor(llm): inline OPENAI_RESPONSES_API_KEY literal in registry Remove the one-use openAIResponsesAPIKeyEnv constant and write the env-var name directly in the provider entry, consistent with every other built-in provider. --------- Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>	2026-08-25 15:03:48 +08:00
nolanchic	b17460a6a3	fix(cli): exit 128+signo when the native binary is killed by a signal (#1054) ... The launcher forwarded spawnSync's result as status ?? (error ? 1 : 0), so a child terminated by a signal (status=null, error=undefined) exited 0. Downstream pipelines gating on the exit code read an OOM kill or any signal death as a clean success. Exit 128+signo for signal deaths, keep the existing status/error behavior otherwise. The exit-code mapping moves to an exported launcherExitCode so it can be unit-tested; the launcher body is guarded by require.main to stay require-safe. Fixes #931	2026-08-25 14:32:39 +08:00
Kite	a66240084b	feat(agent): group semantically related files for multi-file review (#808) ... * 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>	2026-08-25 13:23:25 +08:00
ben7am1n	0c44f1049e	fix(diff): surface git's own message when a diff command fails (#1039) ... * fix(diff): surface git's own message when a diff command fails GetDiff runs git through runGit, which captures stdout and stderr together, then discarded that output on every failure path. A user whose `git show` failed saw only: Error: review failed: load diffs: get diffs: git show failed: exit status 129 The exit status alone cannot distinguish an unsupported option from a bad revision or a permission problem, so diagnosing #972 meant asking the reporter to re-run the command by hand to see what git actually said. Quote git's output in the error for the three diff-producing paths (range, commit, workspace-tracked). The same failure now reads: ... git show failed: exit status 129: error: unknown option `diff-merges=first-parent' Output is capped, keeping the tail, because runGit's combined output means a command that failed partway through carries a prefix of real diff along with the diagnosis. The cap cuts on a rune boundary: git speaks the user's locale, and #972 came from a Japanese-language Windows install, so a byte-wise cut would replace a confusing error with an unreadable one. The other runGit callers deliberately swallow errors and fall back, so they are left alone. * test(diff): pin which command speaks when both workspace diffs fail Addresses review feedback on the two-stage fallback in workspaceTrackedDiff. Reaching `git diff --staged` means `git diff HEAD` already failed, and in the case the fallback exists for -- a repository with no commits -- it failed with "bad revision 'HEAD'", which is expected rather than diagnostic. Surfacing both would put that benign message ahead of the one describing what actually blocked the review, so the behavior is deliberate and now has a test saying so. * fix(diff): quote stderr, not combined output, when git fails Review feedback: a `git show` killed mid-write contributed a 2036-byte tail made entirely of diff content, with no diagnosis anywhere in it. SIGKILL leaves stderr empty, so keeping the tail kept repository source -- and whatever that source contains. That string does not stay local. reviewResultError hands it to span.RecordError (review_cmd.go:259), and signal.NotifyContext (:99) puts Ctrl-C on the path that reaches it, so the leak had a route to whatever telemetry backend is configured. classifyItemError guards the run manifest against raw error text for the same reason. Add runGitSplit and give the three diff-producing callers stderr alone. Git writes its diagnosis to stderr by construction, since die() writes there, so this loses nothing a reader wants and cannot carry diff. Mirrors runGitGrep in internal/tool/code_search.go, cancellation guard included: a signalled process reports the signal rather than the reason, so a cancelled run now says "context deadline exceeded" instead of "signal: killed" -- which also lets classifyItemError reach its timeout class instead of the generic provider one. The three failure modes this PR targets write to stderr, so their messages are byte-identical and #972 still gets its diagnosis. workspaceTrackedDiff returns stderr separately rather than overloading its first return value, which also retires the dual-meaning the earlier review flagged. Also fix the regression test asserting "fatal:"/"error:", which git translates: under zh_CN the line opens with a translated prefix, so it passed only because CI runs in English. Anchor on the object name instead, the one part no locale rewrites -- the same pairing isNotGitRepoError uses. * docs(diff): correct the reasons recorded around gitFailure Two comments described mechanisms that are not there. The cancellation assertion in TestGetDiff_CancelledMidWriteReportsCancellation credited classifyItemError with reading the error's type. It never sees it: GetDiff is reached through loadDiffs, whose failure is recorded at agent.go:284 as a fixed SetRunFailure(RunFailureInput, "failed to resolve review input") without inspecting err. classifyItemError has a single call site, agent.go:712, for per-item subtask errors. Neither reviewResultError nor main.go branches on the type either, so nothing downstream observes it today. What the assertion actually holds is runGitSplit's cancellation guard: the leak assertion above it passes either way, since quoting stderr alone keeps stdout out of the error, so without this second assertion the guard could be removed silently. Verified by dropping the guard -- only this assertion fails. gitDiagLimit and gitFailure still justified themselves by runGit's combined output, which no caller passes anymore. The ceiling and the keep-the-tail rule both survive on stderr, but for a different reason: die() exits the process, so the fatal is last, behind any warnings. Parameter renamed out -> stderr to match what the three call sites pass. No behavior change. make test, go vet, gofmt, english-only and coverage (94.0%) all pass. --------- Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>	2026-08-24 21:47:47 +08:00
历代星辰	e95bdda4f2	feat(cli): add --output flag to write review/scan results to a file (#852) ... * feat(cli): add --output flag to write review/scan results to a file Add `--output <path>` / `-o` to `ocr review` and `ocr scan`, writing the result JSON or text directly to a UTF-8 file instead of stdout. The file is created lazily on the first write so a failed run never truncates an existing target; text-mode files are ANSI-stripped so terminal color codes never pollute the result file. A `[ocr] Results written to <path>` hint is printed to stderr once the file is actually created, and failure output keeps going to stderr so agents always find the failure reason. Closes #851 Signed-off-by: 历代星辰 * test(cli): cover --output flag parsing and file output behavior Add tests for --output/-o flag parsing, the stripAnsiWriter state machine (including escape sequences split across Write calls), lazy file creation (failed runs leave existing targets untouched, never-written targets are not created), and the Results-written stderr hint. Adapt existing emitRunResult / renderComment / outputPreview call sites to the new io.Writer parameter. Signed-off-by: 历代星辰 * fix(test): isolate USERPROFILE so Windows tests never touch the real OCR home os.UserHomeDir() prefers USERPROFILE over HOME on Windows, so t.Setenv("HOME", ...) alone left tests reading and writing the developer's real ~/.opencodereview: config tests overwrote config.json and session/agent tests polluted the sessions store. Add a setTestHome helper (per affected package) that also overrides USERPROFILE, and route every scattered HOME override through it. Signed-off-by: 历代星辰 * fix(cli): propagate output write failures and strip multi-byte ANSI escapes Address the ocr review findings on the --output feature: - lazyFileWriter now records the first write error (Err()) and emits the "Results written" hint only after a successful write; emitRunResult and outputPreview check it after text rendering, so a failed --output write (permission, disk full) exits non-zero like JSON mode already does instead of silently exiting 0 with no file. - stripAnsiWriter keeps multi-byte escapes (ESC + intermediate byte, DCS/PM/APC strings) inside the escape state so trailing bytes are discarded with the sequence instead of leaking into the result file. Signed-off-by: 历代星辰 * fix(cli): re-parse trailing byte after bare-ESC OSC termination and fix Write return semantics Addresses review comments on #852: - stripAnsiWriter ansiOSCEsc: a non-ST byte after ESC is no longer dropped. The OSC ends at a bare ESC terminator and the trailing byte is re-parsed — an ESC starts a new escape sequence, any other byte is forwarded as text. Previously the first byte after a bare-ESC-terminated OSC was silently lost. - stripAnsiWriter Write: report the underlying dst error but return len(p), since the state machine has already consumed the input; returning 0 made a caller retrying on n < len(p) feed the same bytes through twice. - Clarify --output help text: default is stdout and '-' also means stdout. Adds regression tests for both bugs (bare-ESC + trailing text, bare-ESC + new escape, and dst-failure return contract). Signed-off-by: 历代星辰 * fix(cli): reinforce ANSI stripper state machine and validate format flag * docs(cli): document output flag across localized references and READMEs * fix(cli): cap escape intermediate bytes and normalize format in output handlers --------- Signed-off-by: 历代星辰	2026-08-24 15:37:29 +08:00
Nefelibata	6612029127	fix(scan): join background memory compression before session finalization (#1026) ... * fix(scan): join background memory compression before session finalization * test(scan): exercise background compression join barrier in ScanAgent.Run - Rewrite TestScanAgent_WaitBackground_NoLeakOnRun using a blocking compression client pattern - Verify that ScanAgent.Run waits for in-flight memory compression before session finalization - Assert that memory_compression_task is recorded and session_end is the final record in session JSONL - Update Runner.WaitBackground doc comment in llmloop/loop.go to reflect scan mode usage * style: format internal/scan/agent_test.go with gofmt -s * test(scan): avoid premature cancel exit in blocking compression mock * fix(test): eliminate race in WaitBackground regression test The original test had two timing bugs that made it pass even when WaitBackground() was removed: 1. The mock's round-2 response (task_done) could return before the background compression goroutine reached the mock, allowing Run to finish before 'started' was ever closed. Fix: round 2 now waits on <-c.started so the main loop cannot complete until compression is in-flight. 2. The 'select { default: }' check fires instantly — if Run hadn't returned YET (still in cleanup, not because WaitBackground held it), the test passed spuriously. Fix: use time.After(200ms) which is vastly longer than the sub-millisecond cleanup path, so Run will have returned if WaitBackground is missing. Verified: test now correctly FAILs when WaitBackground is commented out and PASSes when it is present. --------- Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>	2026-08-22 10:58:55 +08:00
Wang shuaipeng	132de7d55b	fix(scan): persist safe dedup checkpoints on resume (#1035) ... * fix(scan): persist deduplicated comments on resume * fix(scan): preserve dedup checkpoint provenance * refactor(scan): return batch error last	2026-08-22 10:09:50 +08:00
Gongyl01	3b11812fff	feat(cli): clarify LLM request failures by review stage (#1022) ... * feat(cli): group the LLM retry report by review stage Make the text retry report easier to scan by grouping noteworthy LLM requests only by review stage. Replace internal task_type and request_no labels with human-readable stage names and error phrases, render each request on one line, and make the summary explicit that failed outcomes are request failures. Within each stage, list failed requests before recovered requests and sort matching outcomes by path. Cap each stage at five entries while keeping full details in JSON. Keep the change rendering-only: the retry report schema, manifest wording, JSON fields, warnings, and retry behavior remain unchanged. * test(cli): align retry report assertions * fix(cli): handle retry report edge cases * style(cli): keep the retry report terminal output ASCII-only The grouped report introduced U+2192 and U+2014 as separators. Both pass english-check (it tests for letters, not for non-ASCII bytes), but they render as replacement glyphs on a terminal that is not UTF-8 and they break a habitual grep for "-> failed". The arrow returns to "->", as it was before the regrouping. The two em dashes cannot both become "--" without reading oddly, so the entry separator becomes ":" -- the form the flat listing used, and the one that reads as "this file: this is what happened to it" -- while the summary line keeps a dash as "--". Terminal rendering only. The JSON report never went through this path: outputJSONWithWarnings encodes llm.RetryReport directly, so error_class stays the raw enum and attempts stays a structured array. --------- Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>	2026-08-21 20:04:33 +08:00
Wang shuaipeng	4d61c979a8	fix(vscode): list merge commit files (#1029)	2026-08-21 16:11:58 +08:00
Fanzzzd	4b6874bd23	fix(agent): name the main-loop stop trigger in item failure reasons (#855) ... * fix(agent): name the main-loop stop trigger in item failure reasons StopEmptyRounds and StopCompression previously collapsed into the same "main task stopped before completing" string as StopNone, so a failed item's manifest could not say whether the model spun without usable tool results or the context outgrew its compression threshold. In --format json runs the progress lines that name the trigger are discarded, which made the manifest reason the only diagnostic that leaves a CI runner — and it said the same thing for every stop. Keep the unknown failure class (the taxonomy has no fitting category) but give each stop its own reason, mirroring the StopMaxRounds precedent. Fixes #842 * fix(llmloop): share the main-loop stop reason with the scan path PR #855 named the empty-round and compression stops in the diff-review manifest reason, but two gaps remained. A MainLoopStop constant added later would fall through classifyMainLoopStop's default and inherit the collapsed "main task stopped before completing" text without failing a test — the exact shape of #842, reintroduced by the next enum addition. classifyMainLoopStop now keys only on StopMaxRounds for the budget class and delegates every reason to MainLoopStop.Reason(), whose own default names the unrecognized value. internal/scan discarded the MainLoopStop entirely and hardcoded a second opaque sentence. Scan sessions opt out of the run manifest, so that string is their whole diagnostic: warningsForOutput only drops scan_subtask_error warnings when a manifest exists, and under --format json the [ocr] progress lines that would say which exit fired are discarded. It now appends the shared Reason(), keeping the prefix that resume records and existing warnings match on. MainLoopStop also gains String(), so a stop no longer formats as a bare integer in telemetry, logs and test failure messages. Tests pin String() and Reason() for every stop, assert neither collides across stops, and fail when a constant is added past StopCompression — the prompt to give it its own case. The scan warning test now requires the trigger to be named rather than only that the task did not complete. --------- Co-authored-by: Fanzzzd <fanzzzd@users.noreply.github.com> Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>	2026-08-20 18:46:56 +08:00
xujiejie	43bbc48779	refactor(background): treat --background and --background-file as mutually exclusive with file precedence (#1016) ... * refactor(background): treat --background and --background-file as mutually exclusive with file precedence Replace mergeBackground with selectBackground: when both flags are provided, --background-file wins and --background is ignored (with a stderr warning). The commit-message fallback now fires only when neither entry point was used. This fixes the inconsistency where review and delegate produced different backgrounds for equivalent input (issue #1013), and makes the effective background deterministic regardless of which command is invoked. Closes #1013 * refactor(background): extract resolveBackground helper and fix tests Address reviewer feedback: - Extract resolveBackground() so review and delegate share one call site, making future drift impossible. - Rewrite tests to call resolveBackground directly instead of duplicating the if/else-if logic (which could never fail). - Fix stale comment on TestBackgroundFilePrecedenceOverCommit.	2026-08-20 18:03:28 +08:00
Kite	699c91444a	test(llm): cover tool_choice mapping for all three provider protocols (#1011) ... Add table-driven tests for buildOpenAIParams, buildAnthropicParams, and buildResponsesParams asserting how ChatRequest.ToolChoice="required" is translated into each SDK's own tool_choice representation, and that the mapping is skipped when no tools are attached. These parameter-building functions previously had no direct coverage for tool_choice, so a regression in any of the three branches would go unnoticed by CI.	2026-08-20 11:04:22 +08:00
Luis Rodriguez	9c5e90d0b1	feat(providers): add AWS Bedrock as a built-in provider with native SigV4 auth (#705) ... * feat(providers): add AWS Bedrock as a built-in provider Bedrock serves the same Messages API as api.anthropic.com, so this reuses AnthropicClient wholesale and lets the official SDK's bedrock middleware handle what differs: SigV4 signing, moving the model from the body into the URL path, injecting anthropic_version, and deriving the host from the region. No new protocol implementation, no AWS request plumbing. Configuration is an empty provider entry — there is no api_key to set: { "provider": "bedrock", "model": "us.anthropic.claude-sonnet-4-6", "providers": { "bedrock": { "aws_profile": "...", "aws_region": "..." } } } aws_profile and aws_region are optional; without them the standard AWS chain decides, as with any other AWS tool. Setting them makes a run reproducible without exporting AWS_PROFILE first. Model accepts a foundation model ID, an inference profile ID, or an application inference profile ARN when usage has to be attributed for cost allocation. Four things this needed beyond registering a provider, each found by running it rather than reading it: - The resolver required a non-empty api_key, and separately required both URL and Token to consider an endpoint complete. Bedrock has none of the three, so a correct config fell through every strategy and reported "no valid LLM endpoint configured" — the error for having configured nothing. Both gates now recognise ambient authentication, via an AmbientAuth flag on Provider and ResolvedEndpoint. Providers that do use api_key are unaffected, which TestNonAmbientProviderStillRequiresAPIKey pins. - bedrock.WithConfig prefers bearer auth over SigV4 whenever cfg.BearerAuthTokenProvider is non-nil, and LoadDefaultConfig populates that provider from the SSO token cache. An SSO-authenticated caller — most enterprise setups — therefore sent its OIDC access token and got 403 "Invalid API Key format: Must start with pre-defined prefix". The provider is cleared unless AWS_BEARER_TOKEN_BEDROCK was set deliberately, which restores SigV4 while leaving an explicit bearer token working. - The SDK would also attach an API-key header of its own, which Bedrock rejects even when empty. Authorization and X-Api-Key are removed before the signing middleware runs. - bedrock.WithLoadDefaultConfig panics when AWS config cannot be loaded. A CLI should not answer an expired session with a stack trace, so the config is loaded directly and the failure deferred to the first request as a sentence naming the likely fix. The preset's Models list is taken verbatim from `aws bedrock list-inference-profiles` on a live account rather than inferred: suffix conventions vary per family, so us.anthropic.claude-sonnet-5 is correct while us.anthropic.claude-sonnet-5-v1:0 is rejected with 400 "The provided model identifier is invalid." The global.* cross-region variants are listed alongside us.* since either is a valid routing target. That list only gates --model overrides; an application inference profile ARN still works via the model field. Two existing tests needed updating: the provider-order list gains "bedrock", and TestProviders_AllProtocolsCanonical now delegates to ValidateProtocol instead of re-listing the canonical names, so the next protocol added cannot silently leave it behind. Verified end-to-end against a live Bedrock account: reviews complete and return findings using SigV4 credentials from an SSO profile, with no AWS variables in the environment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(config): configure and diagnose an ambient-auth provider from the CLI Registering the provider was not enough to make it usable: every config-related path still assumed an api_key, and Bedrock's own error wording sends users after the wrong problem. - ProviderEntry gains aws_profile and aws_region. They were readable by the resolver but absent from the struct the CLI marshals, and config is unmarshalled into it and written back on every config command — so a hand-written aws_region was silently deleted the first time the user ran `ocr config model`, with no error and nothing to suggest why the next review reached a different region. - `ocr config set providers.<name>.aws_region|aws_profile` now works, for both the providers and custom_providers paths. Values are trimmed; whitespace inside one is rejected. Region names are deliberately not validated against a fixed list — AWS adds regions faster than an embedded list stays correct, and a wrong region already fails at request time. Setting either field on a provider that authenticates by api_key is an error rather than dead config that reads as applied. - The provider wizard treats the model step as final for an ambient provider instead of demanding a key. An API-key prompt that has to be left blank reads as a step the user failed to complete, and applyOfficialProviderConfig rejected the empty value anyway, so bedrock was unreachable through `ocr config provider` entirely. The gate is now a named check keyed off AmbientAuth, so key-based providers keep the requirement. - `ocr llm test` prints the resolved region and profile in place of the URL, which is empty for bedrock because the region decides the host. A request that reached the wrong region otherwise fails as though the model ID were malformed. - Bedrock rejections are translated into the action that fixes them, since two of them are actively misleading as the service words them: "Invalid API Key format" names a credential no bedrock user can configure (it means a bearer token reached the request), and a model merely absent from the region comes back as "The provided model identifier is invalid." Expired credentials point at `aws sso login` with the profile filled in; AccessDenied is named as an IAM gap on bedrock:InvokeModel rather than a bad credential; a rejected model points at `aws bedrock list-inference-profiles` and the -v1:0 suffix trap. Every other protocol shares this client type, so the translation is gated on the bedrock flag and returns other errors untouched. The unknown-config-key message is pinned byte-for-byte by an existing test; it is updated for the two new provider fields and for anthropic-bedrock as a protocol value. Verified against a live Bedrock account: `ocr llm test` reports region and profile and completes over SigV4; a -v1:0 model ID and an unresolvable profile each produce their intended message rather than a bare 400 or 403. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(bedrock): correct bearer-token precedence, error triage, and model gating Four defects found reviewing the two commits before this one. Each was verified by execution or against SDK source, not inferred. - AWS_BEARER_TOKEN_BEDROCK was unreachable for exactly the users it was meant to serve. The provider was cleared only when the variable was unset, on the strength of WithConfig's doc comment ("if the AWS_BEARER_TOKEN_BEDROCK environment variable is set, it is used"). The code disagrees with that comment: bedrock.go consults the variable only `if cfg.BearerAuthTokenProvider == nil`. So an SSO profile plus a deliberately configured Bedrock API key sent the SSO OIDC token instead of the key — the same silent substitution this patch exists to prevent, and explainError then blamed a token that never left the machine. Cleared unconditionally now, which is what gives the variable the precedence it documents. - A model that the account has not enabled was reported as an IAM problem. Bedrock answers both authorization failures with AccessDeniedException, and the fixes have nothing in common: "You don't have access to the model with the specified model ID" needs model access granted in the console, per account and per region, which no IAM policy provides. The specific wording is now matched ahead of the generic code, and the clause for it is no longer stranded in an unreachable branch. - A bare ValidationException match claimed every request-shape rejection was a model-ID problem: "Input is too long for requested model" sent the user off to list inference profiles. Only the model-identifier wording is matched now; everything else keeps the service's own message, which is the whole point of the function. The credential-expiry arm likewise no longer matches a bare "expired", which caught `x509: certificate has expired`. - --model rejected any Bedrock identifier absent from the preset's Models list, contradicting both the preset's own comment and this PR's description. A preset list cannot be an allowlist here: identifiers are scoped to an account and a region, and an application inference profile ARN — the value to use when spend has to be attributed — can never appear in a list compiled upstream. The list stays a picker for `ocr config model`; it no longer gates an override for an ambient-auth provider. Key-based providers keep the check, so a typo against a hosted API is still caught locally. Also: dropped a cfg.URL normalization block that could not have any effect, since WithConfig is appended last and installs its own base URL — the comment claimed a purpose the code did not have. Pinned AWS_CONFIG_FILE in the test that constructs a client, which was reading the developer's real ~/.aws/config. Fixed the column alignment of the region line in `ocr llm test`. Verified: `ocr llm test` still completes over SigV4 against a live account; an identifier the preset does not list now reaches Bedrock and returns Bedrock's own verdict rather than a local rejection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(bedrock): honour an entry-level protocol override for ambient auth OCR's own review of this PR found that ambient auth was read off the preset while the protocol could be overridden per entry, which left two ways to configure something that reads as applied and cannot work. `providers.bedrock.protocol = openai` resolved with no api_key and no URL: the key requirement was skipped because the preset declares AmbientAuth, but the endpoint then spoke a protocol with no SigV4 signing and carried nothing to authenticate with. Ambient auth is now derived from the protocol actually in force, after the override is applied, so such an entry needs a token again — and conversely an entry that selects the bedrock protocol explicitly signs its requests whatever preset it sits under. The same value gates the --model allowlist, which had the same preset-only assumption. `ocr config set providers.bedrock.aws_region` accepted AWS settings on that same overridden entry. The check now lets the entry's protocol decide whenever it sets one, falling back to the preset's flag only when the entry is silent. Also corrects two stale doc comments the review flagged: ValidateProtocol accepts four protocol names, not three, and the package comment now lists anthropic-bedrock among the supported protocols. The third finding in that review — a bare ValidationException match in explainError — was already fixed in the preceding commit; the bot reviewed the commit before it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * build(deps): bump aws-sdk-go-v2 to clear GO-2026-5764 govulncheck fails the CI test job because the pinned AWS SDK tree pulls in github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3, which is affected by GO-2026-5764 (fixed in v1.7.8). Upgrading the direct dependency aws-sdk-go-v2/config to current resolves eventstream to v1.7.16, past the fixed version. The diff is scoped to the AWS module tree (plus smithy-go, its runtime companion); no other dependencies move. The Bedrock provider's behavior is unchanged: the newer config module still populates BearerAuthTokenProvider from the SSO token cache, so the unconditional clearing in NewAnthropicBedrockClient remains necessary and correct, and it still does not consult AWS_BEARER_TOKEN_BEDROCK itself, so the anthropic-sdk-go re-read of that variable keeps its documented precedence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(bedrock): correct error triage, ambient credential handling, and the protocol's reach Addresses the six pre-merge items from review. - "not authorized to invoke this API operation" sat in the model-access branch. It is IAM's own wording, so it pointed at a console toggle when the fix is a policy change; it now falls to the AccessDenied branch, whose message names bedrock:InvokeModel. A test case carries the phrase without "AccessDenied" in the text, so the phrase itself is pinned to the authorization branch rather than the exception name. - TestExplainErrorClassifiesBedrockFailures read the ambient AWS_BEARER_TOKEN_BEDROCK and asserted the message produced when it is unset, so the suite failed on any machine that exports one. It pins the variable empty. Reproduced before the fix with AWS_BEARER_TOKEN_BEDROCK=sk-x go test -run TestExplainError. - api_key_cmd ran for an ambient-auth provider. The output is discarded by a signed request, and the command is typically a secret-manager read, so a bedrock user got a real 1Password / Touch ID prompt for nothing — while the comment above the call claimed it could not happen. Gated on !ambientAuth; the new test proves non-execution with a sentinel file, and fails without the gate. - llm.protocol and OCR_LLM_PROTOCOL validated anthropic-bedrock and then ignored it. Both strategies describe one URL and one token, have nowhere to carry a region or a profile, and bedrock uses neither value they do carry, so the request would have been signed and re-hosted with the rest of the block silently dropped. Both now reject it, as does `ocr config set llm.protocol`, at the point the value is typed. - The custom-provider contract was split in two: the TUI never offered bedrock, while the resolver demanded a url the bedrock client never reads. Settled toward supporting it — a provider entry is the one place with somewhere to put aws_region and aws_profile, which is what lets a second region or profile have its own entry. url is now required for every protocol except bedrock, the Custom form offers bedrock and ends at the protocol step (there is no url, api key or auth header to collect), and switching an existing entry to it clears the three fields the previous protocol needed rather than leaving them as dead config. The Manual form keeps the three-protocol list, since it writes llm.url and llm.auth_token. - Documented the bedrock.WithConfig append site: options wrap in order, so appending last leaves signing innermost — closest to the wire, and re-run on each retry rather than replaying a stale signature. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(pages): document the bedrock provider in all four locales The provider shipped with no documentation: no row in the built-in table, and no mention of aws_region or aws_profile, so the only way to learn either field existed was to read the resolver. Adds to en, ja, ru and zh: - a `bedrock` row in the built-in provider table, with the host derived from aws_region and no API key env var, since neither applies; - an "AWS Bedrock" section covering the two AWS fields and what each falls back to, why model IDs are not validated against the shipped list (they are scoped to an account and a region, and an application inference profile ARN can never appear in a list compiled upstream), the `-v1:0` suffix trap, and the region/profile lines `ocr llm test` prints in place of a URL; - anthropic-bedrock in the custom-provider protocol list, with the example that needs no url and takes the same AWS fields — the supported way to run a second region or profile; - a note that llm.protocol and OCR_LLM_PROTOCOL reject it, and why. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(bedrock): clear stale AWS settings on protocol switch, bound config load applyProviderField's "protocol" case only validated and wrote the new protocol, leaving aws_region/aws_profile behind when an entry switched away from anthropic-bedrock. Setting the AWS fields first (while the entry was still ambient) and only then switching protocol produced exactly the dead config the aws_region/aws_profile write path already guards against on the other ordering. Clear both fields when the new protocol isn't anthropic-bedrock, with a stderr warning, mirroring the TUI's cpAmbientProtocol() cleanup. Also bound NewAnthropicBedrockClient's awsconfig.LoadDefaultConfig with a context timeout instead of context.Background(), as defense in depth against network calls region auto-detection can still make. Credential resolution itself (SSO refresh, AssumeRole, credential_process) is lazy and already bounded by cfg.Timeout at request time, so this does not fix an observed hang, but removes an unbounded context where the AWS SDK's own defaults are the only guard. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>	2026-08-20 10:22:45 +08:00
超級の新人	756203c31a	fix(cmd): suppress ANSI color when stdout is not a TTY (#927) ... * fix(cmd): suppress ANSI color when stdout is not a TTY Text output hardcoded ANSI escape sequences with no terminal detection, so piping or redirecting a review leaked raw escapes into the consumer. Piping into `gh issue comment` produced a comment full of literal `^[[2m` sequences. Color is now resolved once per run, in this order: `--no-color`/`--color=never` turn it off, `--color=always` forces it on even through a pipe, `NO_COLOR` (any non-empty value, per no-color.org) and `TERM=dumb` turn it off, and otherwise it follows whether stdout is a terminal. Explicit flags outrank the environment because a flag is a per-invocation decision while the variable is a standing preference. Both text paths — review findings and `--preview` — route every escape through colorize(), so plain mode keeps all the information the color carried: the diff gutter still shows +/-/space and the status badges still read [A]/[M]/... Preview counts are padded before colorizing so the columns align in either mode. The flags are persistent on the root command, so `ocr --no-color review` and `ocr review --no-color` are equivalent, and an invalid `--color` value is rejected rather than silently treated as auto. Fixes #682 * refactor(cmd): drop --no-color flag and NO_COLOR env var support --no-color was just an alias for --color=never with no added capability, and NO_COLOR isn't a universal enough convention to bake in speculatively. --color <auto|always|never> alone already covers every case. TERM=dumb stays, since that closes a real gap in TTY detection rather than adding another way to configure the same toggle. --------- Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>	2026-08-19 21:51:38 +08:00
xujiejie	d8fc4cd2af	chore(examples): add explicit --audience agent to codeup_ci and action.yml (#1004) ... Since PR #929 made 'human' the default audience and redirected [ocr] progress lines to stderr, the two non-interactive CI call sites that omitted --audience now emit progress noise on stderr. Add --audience agent to match the other four examples. - examples/codeup_ci/post_review.py: add --audience agent to cmd - action.yml: add --audience agent to ARGS (also covers github_actions) - examples/codeup_ci/post_review_test.py: update exact cmd assertion Exit-code handling, JSON parsing, and comment-posting logic are unchanged. Closes #1003	2026-08-19 21:34:16 +08:00
xujiejie	f6e5e98564	refactor(skill): simplify review flow, use native severity (#1002) ... * refactor(skill): move prerequisites check to troubleshooting section - Remove upfront `which ocr` / `ocr llm test` from main workflow - Keep inline installation fallback hint in Step 2 - Add Troubleshooting section with install and LLM config guidance - Update Gotchas entry to reference Troubleshooting instead of requiring pre-run check * refactor(skill): use native OCR severity/category instead of manual classification - Step 3: remove manual High/Medium/Low classification; use OCR output severity/category directly - Step 4: reference severity field instead of custom priority levels - Output Format: add category/severity to field list; group template by severity - Remove duplicated Priority classification definition - Retain mispositioned comments handling and thinking field * refactor(skill): apply prerequisites/severity refactor to canonical skill Mirrors the plugin skill refactor: - Move prerequisites check to Troubleshooting section - Use native OCR severity/category instead of manual classification - Retain mispositioned comments handling and thinking field - Retain truncation guidance added in 4f7d78f * docs(skill): sync truncation guidance from canonical to plugin skill The plugin skill copy missed the output-truncation guidance added to the canonical skill in 4f7d78f (#809). Both files now diverge only by the plugin's self-descriptive mirror notice, as designed. * fix(skill): route command failures to troubleshooting section Step 2 had no failure directive, so the agent did not consult the Troubleshooting section when `ocr review` failed (e.g. LLM connection error). Add an explicit pointer so the agent looks up the matching fix before retrying. * docs(skill): address PR review feedback - Lead with the interactive `ocr config provider` wizard for LLM config (agent cannot obtain credentials); keep manual `ocr config set` as alternative; drop env-var option. - Fix the no-issues line to reflect that low-severity findings are discarded ("no critical, high, or medium severity issues remain after filtering"). - Apply to both canonical and plugin skill copies.	2026-08-19 20:19:02 +08:00
超級の新人	66d71b23eb	fix(cmd): stream review progress to stderr for json and sarif (#929) ... `--audience human --format json` ran completely silent: newQuietHandle replaced stdout with io.Discard whenever the format was machine-readable, without ever looking at the audience, so `--audience human` was ignored and the user watched a blank terminal until the document appeared at the end. `--format text` streamed progress but is not stable to parse, leaving no way to have both live progress and machine-readable output. Progress is now redirected to stderr instead of discarded when a human asked to watch a machine-readable run. This is safe because every result document (json, sarif, text) is encoded straight to os.Stdout and never travels through stdout.Writer(), so stdout remains a single parseable document while stderr carries the live [ocr] lines. Discarding was never necessary to protect stdout; the two streams were already separate. The three cases are now explicit: audience=agent discards progress regardless of format because the caller asked for none, a machine-readable format with a human audience redirects to stderr, and everything else leaves progress on stdout. Progress lines keep their existing text form. Emitting them as structured NDJSON events, which the report also asks for, would mean defining an event schema and reworking every call site; it is left for separate work. Tests cover where progress lands for each format/audience pair, that stdout stays empty and parseable while stderr receives the lines, and an end-to-end review asserting stdout unmarshals as JSON with no [ocr] line while stderr shows progress. Reverting the fix fails exactly the human-audience assertions and leaves the agent ones passing. The flag help and the CLI reference in all four locales are updated, along with the tip that implied `--format json` means a quiet terminal. Fixes #928	2026-08-19 19:05:53 +08:00
dependabot[bot]	f269d0ce00	chore(deps): bump the go-dependencies group across 1 directory with 18 updates (#880) ... Bumps the go-dependencies group with 14 updates in the / directory: | Package | From | To | | --- | --- | --- | | [charm.land/bubbles/v2](https://github.com/charmbracelet/bubbles) | `2.1.0` | `2.1.1` | | [charm.land/bubbletea/v2](https://github.com/charmbracelet/bubbletea) | `2.0.7` | `2.0.8` | | [charm.land/lipgloss/v2](https://github.com/charmbracelet/lipgloss) | `2.0.4` | `2.0.6` | | [github.com/anthropics/anthropic-sdk-go](https://github.com/anthropics/anthropic-sdk-go) | `1.55.1` | `1.63.1` | | [github.com/modelcontextprotocol/go-sdk](https://github.com/modelcontextprotocol/go-sdk) | `1.6.1` | `1.7.0` | | [github.com/openai/openai-go/v3](https://github.com/openai/openai-go) | `3.41.0` | `3.51.0` | | [github.com/spf13/pflag](https://github.com/spf13/pflag) | `1.0.9` | `1.0.10` | | [go.opentelemetry.io/otel](https://github.com/open-telemetry/opentelemetry-go) | `1.44.0` | `1.45.0` | | [go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc](https://github.com/open-telemetry/opentelemetry-go) | `1.44.0` | `1.45.0` | | [go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp](https://github.com/open-telemetry/opentelemetry-go) | `1.44.0` | `1.45.0` | | [go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc](https://github.com/open-telemetry/opentelemetry-go) | `1.44.0` | `1.45.0` | | [go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp](https://github.com/open-telemetry/opentelemetry-go) | `1.44.0` | `1.45.0` | | [go.opentelemetry.io/otel/exporters/stdout/stdoutmetric](https://github.com/open-telemetry/opentelemetry-go) | `1.44.0` | `1.45.0` | | [go.opentelemetry.io/otel/exporters/stdout/stdouttrace](https://github.com/open-telemetry/opentelemetry-go) | `1.44.0` | `1.45.0` | Updates `charm.land/bubbles/v2` from 2.1.0 to 2.1.1 - [Release notes](https://github.com/charmbracelet/bubbles/releases) - [Commits](https://github.com/charmbracelet/bubbles/compare/v2.1.0...v2.1.1) Updates `charm.land/bubbletea/v2` from 2.0.7 to 2.0.8 - [Release notes](https://github.com/charmbracelet/bubbletea/releases) - [Commits](https://github.com/charmbracelet/bubbletea/compare/v2.0.7...v2.0.8) Updates `charm.land/lipgloss/v2` from 2.0.4 to 2.0.6 - [Release notes](https://github.com/charmbracelet/lipgloss/releases) - [Commits](https://github.com/charmbracelet/lipgloss/compare/v2.0.4...v2.0.6) Updates `github.com/anthropics/anthropic-sdk-go` from 1.55.1 to 1.63.1 - [Release notes](https://github.com/anthropics/anthropic-sdk-go/releases) - [Changelog](https://github.com/anthropics/anthropic-sdk-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/anthropics/anthropic-sdk-go/compare/v1.55.1...v1.63.1) Updates `github.com/modelcontextprotocol/go-sdk` from 1.6.1 to 1.7.0 - [Release notes](https://github.com/modelcontextprotocol/go-sdk/releases) - [Commits](https://github.com/modelcontextprotocol/go-sdk/compare/v1.6.1...v1.7.0) Updates `github.com/openai/openai-go/v3` from 3.41.0 to 3.51.0 - [Release notes](https://github.com/openai/openai-go/releases) - [Changelog](https://github.com/openai/openai-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/openai/openai-go/compare/v3.41.0...v3.51.0) Updates `github.com/spf13/pflag` from 1.0.9 to 1.0.10 - [Release notes](https://github.com/spf13/pflag/releases) - [Commits](https://github.com/spf13/pflag/compare/v1.0.9...v1.0.10) Updates `go.opentelemetry.io/otel` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) Updates `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) Updates `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) Updates `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) Updates `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) Updates `go.opentelemetry.io/otel/exporters/stdout/stdoutmetric` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) Updates `go.opentelemetry.io/otel/exporters/stdout/stdouttrace` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) Updates `go.opentelemetry.io/otel/metric` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) Updates `go.opentelemetry.io/otel/sdk` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) Updates `go.opentelemetry.io/otel/sdk/metric` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) Updates `go.opentelemetry.io/otel/trace` from 1.44.0 to 1.45.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.44.0...v1.45.0) --- updated-dependencies: - dependency-name: charm.land/bubbles/v2 dependency-version: 2.1.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: go-dependencies - dependency-name: charm.land/bubbletea/v2 dependency-version: 2.0.8 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: go-dependencies - dependency-name: charm.land/lipgloss/v2 dependency-version: 2.0.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: go-dependencies - dependency-name: github.com/anthropics/anthropic-sdk-go dependency-version: 1.62.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: github.com/modelcontextprotocol/go-sdk dependency-version: 1.7.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: github.com/openai/openai-go/v3 dependency-version: 3.50.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: github.com/spf13/pflag dependency-version: 1.0.10 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/exporters/stdout/stdoutmetric dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/exporters/stdout/stdouttrace dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/metric dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/sdk dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/sdk/metric dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/trace dependency-version: 1.45.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>	2026-08-19 13:41:04 +08:00
Tao Xin	68492a5372	feat(installation): support asset download via OCR_GITHUB_MIRROR (#893) ... * rewrite mirrored install shell script * rewrite mirrored install pwsh script and code cmt * scripts updates * spelling qwq * docs: update * docs: powershell users * merge scripts * warn about security risks & docs * trim the DOMAIN string * fix spelling * docs: update fix naming * docs: refactor docs * fixes * apply suggestions * fix: download checksums from mirror * docs: clarify mirror security * fix(installation): normalize OCR_GITHUB_MIRROR input across platforms - Strip https://, http:// scheme prefix and trailing slash automatically - Unify whitespace handling: trim leading/trailing only, error on internal spaces - Both install.sh and install.ps1 now behave identically for edge inputs --------- Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>	2026-08-19 09:45:32 +08:00
Kite	ab9a83e36d	chore(ci): change dependabot schedule from weekly to monthly (#998)	2026-08-18 21:52:48 +08:00
Tao Xin	35a06a4951	feat(providers): add gemini to built-in providers (#930) ... * add gemini providers and tests * docs: update * chore: add more models * chore: reorder models * fix: Apply suggestion from @lizhengfeng101 --------- Co-authored-by: Kite <254839944+lizhengfeng101@users.noreply.github.com>	2026-08-18 21:20:03 +08:00
Tao Xin	24c2dd0cde	feat(language): add support for .ipynb files (#980) ... * chore: new extensions for `.ipynb` * docs: update site docs * chore(allowlist): skip Jupyter .ipynb_checkpoints autosaves Jupyter writes autosave copies of a notebook into a sibling .ipynb_checkpoints/ directory. Now that .ipynb is in the extension allowlist, an accidentally committed checkpoint would be reviewed as a regular file and produce comments duplicating those on the real notebook. Exclude the directory by default, alongside the other tool-generated artifacts. --------- Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>	2026-08-18 20:24:20 +08:00
chethanuk	6c4a89faaf	feat(allowlist): add Thrift and Cap'n Proto support (#960) ... Thrift IDL (.thrift) and Cap'n Proto schema (.capnp) files were dropped at the extension gate, so no rule could ever run on them. Both are IDLs whose main review risk is silent wire-compatibility breakage, which is exactly what protobuf.md already covers for .proto. Adds both extensions to the allowlist, registers thrift.md and capnp.md in path_rule_map, and excludes the compilers' generated output. The gen-* and kitex_gen excludes are scoped by extension rather than by directory because IsExcludedPath applies every pattern to every path with no language dispatch; a bare gen-*/** would drop unrelated files in other languages. Data only, no Go source changes.	2026-08-18 19:10:57 +08:00
Tao Xin	9a01ec0427	feat(lang): review support for R (#988) ... * feat: new support for R * docs: update * chore: update site docs * fixup: missing * for mapper * chore; switch r to uppercase * fix: `internal/config/allowlist/allowed_ext_test.go`	2026-08-18 17:10:03 +08:00
Minsu Lee	794a971a9a	i18n(pages): add Korean (ko) locale (#857) ... Add a complete Korean translation of the docs site UI strings and wire ko into the language switcher. - pages/src/i18n/ko.ts: all 273 keys from en.ts, in the same order. Terminology follows README.ko-KR.md (세션 뷰어, 텔레메트리, 위임 모드, 정밀도/재현율). Product names, protocol names, CLI flags and code identifiers are left in their original form. - Language union, translations record and SUPPORTED_LANGUAGES gain 'ko', so browser language detection picks up ko-KR automatically. - Navbar/Footer language menus list 한국어; the navbar badge glyph 한 gets a Korean font stack instead of falling back to the Chinese one. - docsMap gains an empty ko entry: doc pages fall back to English until pages/src/content/docs/ko/ is contributed in a follow-up. - styles/index.css: word-break: keep-all scoped to :lang(ko). Korean separates 어절 with spaces, and the default breaking split headings mid-word ("Agent 시스" / "템"). zh/ja depend on any-character breaking and are verified unchanged; keep-all is a no-op for Latin text. Verified with npm run typecheck, lint, test, build and size, plus headless screenshots of the ko locale at 1440px across home, features, benchmark, quickstart and docs. Refs #471	2026-08-18 16:49:53 +08:00
Ben Younes	c4d3f52b72	feat(allowlist): add Zig support (#937) ... Recognize .zig source files in the review allowlist, exclude conventional Zig test files, and add a Zig-specific review rule doc so .zig changes no longer fall back to the generic default checklist. - Add .zig to supported_file_types.json - Exclude **/test/**/*.zig and **/*_test.zig in default_exclude_patterns.json - Map .zig to a new rule_docs/zig.md via system_rules.json - Cover extension recognition, test-path exclusion, and rule resolution in tests	2026-08-18 09:53:37 +08:00
林SO	533f7367cd	fix(updater): discard stale version hints (#720)	2026-08-17 22:35:46 +08:00
Tao Xin	b712856403	docs(site): update for elm (#978)	2026-08-17 20:22:12 +08:00
Tao Xin	6cc10949a6	fix(ci): line endings check fails on symbolic links (#952) ... * fix(ci): line endings check fails on symbolic links * chore: update err msg	2026-08-17 20:10:58 +08:00
Tao Xin	ec5f6851d2	fix(pages): make install-channel command box scrollable (#912)	2026-08-17 19:53:04 +08:00
chethanuk	b741d7d0b5	feat(rules): add Jsonnet review rules (#470) (#962) ... Route `.jsonnet` and `.libsonnet` to a new rule doc. `.libsonnet` is a naming convention for importable libraries, not a separate language, so both share one doc (as .hs/.lhs and .nim/.nims/.nimble already do). The vendor exclude is extension-scoped rather than a bare `**/vendor/**`: IsExcludedPath applies every pattern to every path with no language dispatch, so an unscoped directory pattern would also drop vendored Go and PHP sources, whose extensions are allowlisted. No test-file pattern is added — real Jsonnet projects split between `test_*.libsonnet`, `tests/*.jsonnet` and `*_test.jsonnet`, and an over-broad glob silently drops handwritten files. Verified: go test ./... and gofmt -l internal/config clean.	2026-08-17 19:07:45 +08:00
chethanuk	1ae808e100	fix(scripts): stop license checks failing on valid headers (#971) ... `echo "$header" | grep -q ...` races. grep -q exits on the first match, so echo can die of SIGPIPE (141); `set -o pipefail` makes that the pipeline's status, and verify-license.sh reports a header that is present as missing. Measured on a 410-byte header under CPU load: 4 spurious failures in 3000 iterations, naming a different file each time. has_header() in add-license.sh has the same race, and there a false negative makes add_header() prepend a second copyright block to a file that already has one. Feed the header through a here-string instead, and read the year with bash's regex match rather than a grep | grep | head chain that can take SIGPIPE the same way.	2026-08-17 17:25:35 +08:00
Tao Xin	e288e1a33f	fix(pages): serve 404 pages on non-existent GET requests but render no React Route (#955) ... * core: NotFoundPage Page for 404 * chore: write tests(AI) * route 404 request to ./pages/NotFoundPage * chore: write tests (AI) * chore: translations sync * fixup: updated webpack cfg so unknown HTML paths are also rewritten to the app shell * refactor: enhance NotFoundPage tests with dynamic translation handling * docs: Apply suggestion from @lizhengfeng101 --------- Co-authored-by: Kite <254839944+lizhengfeng101@users.noreply.github.com>	2026-08-17 15:24:01 +08:00
chethanuk	e51d65706f	fix(allowlist): allow .properties, .po and .pot so their rules can run (#958) ... system_rules.json maps **/*.properties, **/*.po and **/*.pot to properties.md, po.md and pot.md, but none of the three extensions were in supported_file_types.json. Both filter paths (internal/scan/agent.go:483 and internal/agent/preview.go:51) reject a file on its extension before any rule is resolved, so the three docs were unreachable by default while pages/src/content/docs/en/review-rules.md advertised them. Adds the extensions, plus a TestSystemRulesIntegrity subtest that fails when any extension glob in path_rule_map names an extension IsAllowedExt rejects. Filename globs (**/pom.xml) and infix globs (**/*{mapper,dao}*.xml) make no extension claim and are skipped.	2026-08-17 15:05:41 +08:00
chethanuk	f75c43af45	feat(config): resolve api_key/auth_token from a command (#236) (#605) ... * feat(config): resolve api_key/auth_token from a command (#236) Add `api_key_cmd` (provider entries) and `auth_token_cmd` (legacy llm block) so the LLM credential can be fetched from a secret manager at review time instead of stored plaintext in config.json — same pattern as git credential.helper / AWS credential_process. Resolution precedence (single site, presets and custom providers alike): static api_key always wins (stderr warning if a command is also set) → api_key_cmd → preset env var → error. The legacy llm block gets a mirrored auth_token_cmd; an incomplete legacy block never executes the command, and a set-but-failing command on a complete block is a hard error (never a silent fallback). Command execution is a build-tag split (sh -c / cmd /C) with a 60s timeout; the child's stderr passes through so pinentry/1Password/op prompts stay visible. Stdout is trimmed and used in memory only — never written to config or logged. Empty, whitespace-only, multi-line, and timed-out output are all hard errors. No caching (resolution runs once per process). - config set: api_key_cmd/auth_token_cmd are settable and round-trip; not masked (they are command lines, not secrets). - TUI cloneProviderEntry preserves api_key_cmd. - docs: 'API key from a command' section in configuration.md (en/zh/ja). Tests: table-driven runner matrix (success/trim/non-zero/empty/ whitespace/multi-line/not-found/timeout) + resolver precedence and legacy-fallthrough rows. Coverage 81.3%; Windows arm compile-checked (CI is Linux-only). * fix(llm): harden the credential command and cover it on Windows Follow-up hardening on the api_key_cmd/auth_token_cmd path, plus the CI job that actually exercises its Windows arm. The 60s timeout was not a real bound. It killed the shell, but a helper that leaves a background process holding the inherited stdout pipe (gpg-agent, pinentry, a first-use `op` daemon) kept Cmd.Wait blocked on the read long after the context died — `api_key_cmd = "sleep 200 & printf tok"` hung for over 90s. Buffer stdout through a writer os/exec copies in its own goroutine and set WaitDelay, which is what lets Wait force the pipe closed; ErrWaitDelay on its own is not a failure, since the command exited and its output is already buffered. Three more ways a resolved value could not be used: - Stdin was /dev/null, so a helper needing a passphrase saw EOF or refused to prompt for lack of a tty. Wired to os.Stdin, which is safe because no path resolves an endpoint while the bubbletea TUI is reading stdin. - Output was unbounded; `cat /dev/urandom` grew the heap without limit. Capped at 64KiB, refusing the write so the child dies of SIGPIPE. - Control bytes reached the Authorization header, where net/http rejects them as an opaque `invalid header field value`. Rejected up front with the offending byte and offset, matching httpguts.ValidHeaderFieldValue. A lone interior CR survived both TrimRight and TrimSpace, so it is now caught as multi-line output. Ordering: the command ran before the rest of the config was known to be usable, so `ocr review --model nonexistent` fired a biometric prompt and only then failed on the model name. Execution is deferred past validation at both sites — the source selection in tryProviderConfig, and ResolveEndpointWithModelOverride, which parsed OCR_LLM_TIMEOUT and OCR_LLM_EXTRA_HEADERS after resolving the credential. A whitespace-only static api_key also used to win precedence over a working api_key_cmd and send `Authorization: Bearer `; it now normalizes to unset, and the Manual TUI tab trims its token like the other two tabs. `ocr config provider` rejected api_key_cmd-only providers in both directions: non-interactively applyOfficialProviderConfig demanded a static key or an env var, and interactively the API-key step could not be confirmed because the field renders blank for such a provider. Both now treat a configured command as satisfying the requirement, and the error messages name the option that would fix it. Windows: the command line goes to cmd.exe through SysProcAttr.CmdLine with /S rather than through Args, because os/exec quotes Args with syscall.EscapeArg, which targets CommandLineToArgvW; cmd.exe is a documented exception whose escaping mangles any command containing a double quote, so `op read "op://Private/My Vault/api-key"` arrived as a single literal filename. Args stays at its one-element default rather than nil (syscall.StartProcess ignores argv when CmdLine is set) so Cmd.String() cannot panic on Args[1:]. CI ran only self-hosted Linux, and the cross-compile job proves the windows arms compile but never runs them, so keycmd_windows.go had zero coverage on any platform. Adds a windows-latest job that vets, tests, builds and smoke-tests natively. It installs Go with setup-go instead of the shared golang:1.26.5 image because GitHub does not support `container:` on Windows runners (actions/runner#904); no -race, since the detector needs a C toolchain there and races are OS-independent; no coverage gate, since the //go:build !windows files legitimately put the total under the Linux job's 80%. Six existing tests needed a guard for that job, none a behavior change: three assert an unreadable path is skipped, but Chmod(0000) on Windows only sets the read-only bit (and their os.Getuid() == 0 guard cannot cover it, since Getuid returns -1 there); TestSaveConfig asserts the 0600 the config is written with, which Windows reports as 0666; the symlink-safety test needs a privilege an unelevated CI account lacks; and the "absolute unchanged" background-path case was passing a rooted but non-absolute path, so it had been exercising the relative branch. Running that job turned up more of the same, all of it in tests and none of it needing a production change. os.UserHomeDir reads USERPROFILE on Windows and never falls back to HOME, so every test that redirects a home dir was quietly reading the real profile: TestLoadGlobalRule, TestShellRCFiles, TestTryShellRC and the session writer-creation test now set both. So do the retry e2e helper and TestLoadLLMRuntime_BadAppConfig, where it had gone past reading the wrong profile to failing outright. The e2e test blocks session persistence by occupying $HOME/.opencodereview/ sessions with a regular file, and on Windows found the runner's real directory already sitting there, so the setup write died with "is a directory"; the config test wrote its invalid config.json into a temp home nothing read, so resolution reported a missing endpoint instead of the parse failure the test is named for. unwritableConfigPath put the config below a regular-file parent, which Windows reports as ERROR_PATH_NOT_FOUND; os.IsNotExist accepts that, so loadOrCreateConfig read it as "no config yet" and the six save-failure tests never reached the rollback they are named for. It now points at a directory, which fails both the write and the reload on every platform, so those six keep their coverage rather than taking a skip. Two do get one, the mechanism being absent rather than different: the chmod(0000) sniff error in internal/scan, and ReadDir on a regular file, which comes back as an empty listing on Windows instead of ENOTDIR. captureStdout and captureStderr -- and the two helpers shaped like them in the delegate and config tests -- drained their pipe only after the captured function returned, so that function could write one pipe buffer and then blocked forever. That is what hung TestReviewE2E_RecoveredAndFailedReachesJSONExit for the package's entire 10m budget. Linux only hid it: 1MiB through the old helper deadlocks there too. They now drain concurrently, which fixes the bug instead of skipping the test. Docs (en/zh/ja) spell out the failure modes, the 60s budget including the time spent answering a prompt, the inherited stdin/stderr, the extra 5s a daemon holding the pipe costs, and that config.json is trusted input because the value is executed as a shell command. Review follow-ups in the same pass. A whitespace-only api_key_cmd was the one credential field this path had not normalized: it is empty to `sh` but non-empty to Go, so it suppressed the env-var fallback and then failed with "produced empty output". It now reads as unset, the same as the equivalent typo in api_key. Same for auth_token_cmd on the legacy block. The wizard checked those same fields for emptiness without the trim, so `ocr config provider` would accept a command of " ", save a config with no static key, and leave the resolver to refuse it with "no api_key or api_key_cmd configured". Both gates read through apiKeyCmdForStep and manualAuthTokenCmd, so the trim goes in those two accessors and covers the render sites with them; applyOfficialProviderConfig reads the entry directly and gets its own. The TUI never showed that a command already satisfies the credential step, so the API-key field looked unconfigured on a provider that resolves fine; it now says so on both the provider tabs and the Manual tab. The hint names the config key rather than echoing the command. Usually the command is a bare reference to a secret manager, but nothing stops a user inlining a credential into it (`VAULT_TOKEN=hvs.xxx vault kv get ...`), and this wizard masks every other secret it puts on screen -- one user-authored string printed verbatim into screenshots and terminal recordings was the hole in that. There is exactly one command per provider, so the key name is enough to identify which one is configured. Left as it is, deliberately: SysProcAttr.Setpgid would let us SIGKILL the whole process group and so reap a grandchild the command backgrounded, which `sleep 200 & printf tok` does leak today. It would also put the child outside the terminal's foreground process group, where it takes SIGTTIN the moment it reads the tty -- measured, a child running `read -r x </dev/tty` answers in 7ms as written and returns nothing at all under Setpgid. That read is what pinentry and `op`'s fallback prompt do, which is the case c.Stdin = os.Stdin exists to support and the docs promise. The group has to be chosen at Start, so this cannot be narrowed to the timeout path, and reaping the grandchild properly needs tcsetpgrp-style job control. A process the user's own command asked to background, outliving a CLI that exits seconds later exactly as it would from their shell, is not worth a broken credential prompt. keycmd_unix.go records the measurement so the trade is not re-litigated. The static-key-wins tests asserted only on the resolved token, which would have held just as well if the command ran and its output were discarded — i.e. a spurious biometric prompt on every review of a config that keeps a command as a fallback. They now use a filesystem witness to assert non-execution. The docs note that a command written for `sh` is generally not portable to `cmd.exe`, since the Windows arm is where that bites. * fix(config): drop duplicated license header in testconnection The SPDX and copyright block was emitted twice at the top of internal/config/testconnection/testconnection.go, a rebase artifact from the first commit on this branch rather than an intentional change. The file is now byte-identical to main. make license-check passed throughout: it verifies a valid header is present, not that there is only one. * docs(i18n): sync api_key_cmd configuration docs to ru The en, ja and zh pages gained the "API key from a command" section; ru was left behind. Adds the same section, in the same position, with the config keys and shell snippets untranslated as the rest of the file does.	2026-08-17 14:40:37 +08:00
Waqas Ahmed	7ef8e50bd2	feat(allowlist): add Elm language support (#940) (#953)	2026-08-17 13:43:34 +08:00
Tethys0	5cbc8a1cd0	docs: fix code of conduct reporting links (#968) ... Co-authored-by: Bo Zhang <187063395+Tethys0@users.noreply.github.com>	2026-08-17 11:45:20 +08:00
ayxwi	ea1de41a4f	fix(plugin): separate client marketplace registrations (#908) ... Register the Claude Code and Codex integrations in their respective native marketplace manifests so each client only exposes its compatible plugin. Co-authored-by: Xwell <180168103+Xvvln@users.noreply.github.com>	2026-08-16 19:49:49 +08:00
chethanuk	75cb3d0c45	feat(scan): report token budget stop in JSON summary.budget_exceeded (#791) ... scan already detects the aggregate token-budget stop (it prints the "[ocr] token budget reached" line and records a token_budget_reached warning) but BudgetExceeded() was hard-coded false, so summary.budget_exceeded never appeared in `ocr scan --format json`. The write goes next to `budgetHit = true` in dispatchBatch's per-file gate. That is the only site that sets budgetHit, and it covers all three exits that carry the stop out of dispatchBatch: normal return, ctx-cancel return, and the caller's `if budgetHit { break }`. Setting it at the dispatchSubtasks break instead would lose it on the ctx-cancel path. Plain bool, no mutex: dispatchBatch's loop is the only writer, it runs on the caller's goroutine, and the value is read by emitRunResult after Run returns. The spawned subtask goroutines never touch it. Matches the existing internal/agent.Agent.budgetExceeded field. Status and exit code are untouched — reaching the budget is a controlled truncation, so out.Status stays the warning-derived value.	2026-08-16 19:21:18 +08:00
Magnus	b8bee97184	feat(providers): add kimi-global (Moonshot international API) to built-in providers (#914) ... Moonshot operates two first-party endpoints: api.moonshot.cn (mainland China, already registered as `kimi`) and api.moonshot.ai (international). Only the former is currently available, so users outside mainland China cannot select a Kimi model without hand-configuring a custom base URL. Registers `kimi-global` pointing at https://api.moonshot.ai/v1. The endpoint is OpenAI-compatible, so it uses the existing openai-chat-completions protocol with no new client code — the same shape as the existing global/CN provider pairs (siliconflow / siliconflow-cn, minimax / minimax-cn). - Base URL: https://api.moonshot.ai/v1 - Auth: MOONSHOT_GLOBAL_API_KEY (mirrors SILICONFLOW_GLOBAL_API_KEY naming) - Models: mirrors the existing `kimi` list; kimi-k3 is served on the international platform (kimi-k3 API live since 2026-07-16) The existing `kimi` provider is left untouched, so this is non-breaking for current users.	2026-08-15 22:35:52 +08:00
Kite	9a371c9b36	fix(diff): re-file comments whose code lives in another file (#921)	2026-08-15 19:20:41 +08:00
Vladimir	44a946a68d	feat(rules): add Swift review rules (#918)	2026-08-15 17:50:30 +08:00
Tao Xin	1abfb70fcb	docs(cli-reference): add missing ocr review flags examples (#920) ... * document flags * translations sync fixup fixup * remove conflicting `--exclude` in docs	2026-08-15 17:24:50 +08:00
ScarletCarpet	c8b6a390b8	fix(review-filter): add submit_filter_result func for review filter stage (#295) ... * fix(review-filter): add `submit_filter_result` func for review filter stage At review filter stage, LLM(test GLM5.2) may return natural language content such as: """ Looking at each comment... - commit 1: ... - commit 2: ... Summary: ```json [...]``` """ add function calling for LLM will get more stable result for some model of providers. and this tool only appears at review filter stage, so it does no harm for review quality. * fix(filter): log tool call argument parse failures for observability * refactor(filter): two-tool design with required choice for review filter - Replace single submit_filter_result tool with two mutually exclusive tools: report_incorrect_comments and approve_all_comments - Add ToolChoice field to ChatRequest, wired through OpenAI Chat, OpenAI Responses, and Anthropic client paths - Set ToolChoice to 'required' so the model must always make an explicit decision (no silent text-only fallback) - Aggregate results across multiple tool calls for robustness - Update prompt to instruct exactly-one-tool usage - Preserve text-based fallback for providers without tool support * fix(filter): stop the review filter from deleting real findings An A/B benchmark over 194 identical commit ranges (50 OSS repos, claude-opus-4-6) showed the filter removing 22 comments at 36% precision: 8 of them reported real defects — a heap overflow, an ignored LOCKMODE parameter, a static/non-static linkage conflict, a dropped success status, double URL encoding, an always-true condition. Two causes, one of them structural. Field order in report_incorrect_comments. Go serializes the parameter map alphabetically, so comment_ids was emitted before any reasoning field and the model had to commit before it had finished thinking. Replaying recorded sessions with a diagnostic field made this visible: it wrote "this is a protected subject, I should not remove it" while the id stayed in the list it had already produced. Adding an "analysis" array — alphabetically first, required — lets it reason before concluding. parseFilterToolCalls is unchanged; it reads comment_ids and ignores the rest. Prompt scope. Step 2 "Issue Classification" asked whether the comment "misidentifies clearly normal code as a defect", which invited a value judgement and was the entry point for most wrong removals. It is gone. Removal now needs one of two grounds: the code the comment targets is absent from the diff, or a single diff line literally contradicts its central claim. Protected subjects (memory safety, concurrency, linkage, behavioral change, unused parameters) and style-only comments that state something true are vetoed as the first two steps of the method, not as prose the model reads and then skips. Replaying all 455 recorded filter calls with the same inputs: precision 36% -> 88%, real findings deleted 8 -> 0, at +37% filter tokens and +7% mean latency. Caveat: the grounds were derived from this same dataset, so the figure is a training-set result and wants a hold-out range set before it is trusted. --------- Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>	2026-08-15 17:18:51 +08:00
Guiyang Yuan	96b2728ec2	docs(cli-reference): document missing review flags (#900)	2026-08-15 13:28:49 +08:00
温洛	f7344e798a	feat(providers): add xAI (Grok) to built-in providers (#899) ... * feat(llm): add xAI Grok provider preset * Update internal/llm/providers.go Co-authored-by: Tao Xin <wu2196674@icloud.com> * Update internal/llm/providers.go Co-authored-by: Tao Xin <wu2196674@icloud.com> * apply ccec85 for test * Update internal/llm/providers.go Co-authored-by: Tao Xin <wu2196674@icloud.com> --------- Co-authored-by: Tao Xin <wu2196674@icloud.com>	2026-08-15 12:21:46 +08:00
Abu Bakar Siddik	d8b222a180	refactor(telemetry): replace PrintTraceSummary positional params with TraceSummary struct (#909) ... * refactor(telemetry): replace PrintTraceSummary positional params with TraceSummary struct PrintTraceSummary had grown to nine positional parameters after the session ID landed in #870, making call sites hard to read and easy to get wrong. Introduce a TraceSummary struct and pass it as a single argument (Option A in the issue); printed output is unchanged. Also add stdout.Swap, which swaps the package writer under the existing mutex and returns a restore function, so tests can capture and assert output written through stdout.Writer(). The PrintTraceSummary tests now assert the exact summary, cache-token, and session lines instead of only verifying the call does not panic. Closes #906 * docs(stdout): clarify Swap concurrency doc comment The mutex in Swap and its restore closure already guarantees memory safety under concurrent access; the remaining hazard is semantic — concurrent swaps produce non-deterministic restore ordering. Rephrase the comment to state that distinction, as suggested in PR review.	2026-08-15 12:19:28 +08:00
Magnus	7ce7cf78a2	feat(providers): add glm-5.3 to the Z.AI Coding Plan provider (#915)	2026-08-15 10:45:49 +08:00
董艺宽	0c8c0670f2	docs(cli): document review no-filter option (#871)	2026-08-14 17:54:10 +08:00
Gongyl01	31db10fa63	fix(resume): preserve checkpoints after Ctrl-C (#902) ... * fix(resume): preserve checkpoints after Ctrl-C * fix(resume): tighten cancellation dispatch	2026-08-14 17:42:24 +08:00