* 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>
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.
* 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`
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
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
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.
`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.
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.
* 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.
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>
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.
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.
* 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>
* 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.
Add paths-ignore to the push and pull_request triggers so changes that
no CI step scans (markdown files, LICENSE, .gitignore) no longer spin
up the full suite. Source-relevant paths (.yml/.yaml, workflow files,
action.yml) stay triggerable.
Closes#905
Add platform-specific install options (curl|sh for MacOS/Linux,
irm|iex for Windows) to the 'More' dropdown in the homepage hero.
- Add apple.svg, linux.svg, windows.svg icons
- Add three new secondary install channels with OS-specific commands
- Reorder MacPorts to the bottom of the dropdown
- Add i18n keys for all four locales
- Fix dropdown clipping by removing overflow:hidden on hero section
- Fix dropdown alignment (left:0 instead of right:0)
Print the review session UUID on a separate line after the terminal
summary so users can copy it for `ocr resume` without --format json or
filesystem browsing. The line is omitted when the session ID is empty
(e.g. when session persistence is unavailable). JSON output is unchanged.
Co-authored-by: Kite <254839944+lizhengfeng101@users.noreply.github.com>
* fix(opencode): separate per-file and overall timeouts
* fix(opencode): default to 30-minute overall timeout instead of no timeout
Defense in depth: when overallTimeoutMinutes is not configured,
apply a 30-minute watchdog so genuinely stuck processes are reaped
even if the abort signal never fires.
---------
Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>
* feat(provider): add editable Base URL step to official provider wizard
The official-provider tab in `ocr config provider` only captured API key
and model, with no way to override a preset provider's Base URL. The
resolver already honored `entry.URL` over `preset.BaseURL`, but the TUI
never exposed it — litellm (a self-hosted gateway rarely at
http://localhost:4000/v1) was the canonical pain point.
Add a Base URL step to the official-tab flow (stepModel -> stepBaseURL ->
stepAPIKey), pre-filled with the effective URL (configured override or
preset default). Persist `providers.<name>.url` only when the entered
value differs from the preset default, so the preset remains the fallback
and configs without an explicit url are unchanged. Custom/manual tabs are
unaffected.
Add resolver regression tests (litellm override + default fallback) and
TUI tests (pre-fill with preset/override, Esc navigation, persistence of
override vs. clearing on preset default). Update the four official-tab
tests that assumed stepModel -> stepAPIKey to traverse the new step.
* feat(provider): surface override Base URL in model picker and document it
With the wizard now able to set a Base URL override for built-in
providers, make the override visible and discoverable.
- `ocr config model` shows the effective Base URL for a preset provider
(the configured `providers.<name>.url` override, or the preset default
when none is set) so users can confirm their gateway is in use.
- The provider-wizard model-selection step shows the same effective URL
via a tab-aware `effectiveBaseURL()` helper (official override/preset,
or custom provider URL).
- Document `providers.<name>.url` as a built-in provider override in the
configuration docs, with a litellm example and the preset-as-default
semantics; note the wizard's editable Base URL step.
Add tests covering the model-selector display (override vs preset
default) and the wizard's effectiveBaseURL resolution.
* fix(provider): address PR review — URL trim, validation, dead code, Esc display
Address 4 of 5 code review findings on PR #729:
1. URL trim consistency (provider_cmd.go): trim the Base URL once and use
the trimmed value for both comparison and persistence, preventing
whitespace-polluted URLs from being written to config.
2. URL format validation (provider_cmd.go): validate that the Base URL
has an http/https scheme and non-empty host before persisting, giving
immediate feedback instead of a runtime failure. Rejects malformed
values like bare hosts or ftp:// schemes.
3. Dead code removal (provider_tui.go): remove the init-time pre-fill of
officialURLInput that is always overwritten by loadOfficialURL() when
the user enters the Base URL step. Pre-fill logic now lives in a
single place.
4. effectiveBaseURL reflects pending edit (provider_tui.go): when the
user edits the Base URL and presses Esc back to model selection,
effectiveBaseURL() now returns the in-progress value from
officialURLInput instead of the stale on-disk config.
The SSRF/private-IP finding (#2 in review) is not addressed — it is a
false positive for a local CLI tool where localhost and private network
endpoints are the primary use case (the litellm preset default is
http://localhost:4000/v1).
* feat(provider): implement URL trimming and validation for provider configuration
* fix(provider): remove obsolete official URL handling
---------
Co-authored-by: Kite <254839944+lizhengfeng101@users.noreply.github.com>
* feat(resume): verify input identity before reusing checkpoints
`ocr review --resume` admitted a session whenever the ref text matched, which
is neither sufficient nor necessary evidence about the input: `abc1234` and
`abc1234def` name the same commit, while a branch name that did not change can
name a new one. Resuming then mixed comments computed from one input with
comments computed from another, and nothing in the report distinguished them.
Compare the resolved input identity instead. agent.ResolveIdentity replays the
run's own selection — the same diff load and the same two filter passes — and
returns the identity a real run would record, so the parent manifest and the
child candidate are directly comparable. Any mismatch on mode, repository,
source artifact or rule config rejects the whole resume rather than degrading
to partial reuse. A provider or model change must be asked for with --provider
or --model; one that arrived through config or the environment is rejected.
Two behaviour changes fall out of this. Ref text no longer decides admission,
so ValidateOptions only checks the review mode. A parent that completed zero
items is now admitted: its manifest is verifiable, so its selected set is
simply re-dispatched, which is the case resume exists for.
Reuse is then gated on the parent manifest rather than on the checkpoint lines:
only a fingerprint the manifest claims as completed or reused is reused, which
keeps the manifest the single source of coverage truth. That gate is also what
makes an unreadable checkpoint survivable, so review loads through
LoadReviewResumeState, which drops lines it cannot parse — the file such a line
described is simply reviewed again, instead of one truncated write costing every
other file its checkpoint. Scan keeps the strict LoadResumeState, because with
no manifest to arbitrate, a dropped line cannot be told apart from a checkpoint
that was never written.
Rejection happens strictly before agent.New, because session.New writes
session_start the moment it is called — validating any later would leave an
orphan session behind every rejection. Keeping it there needs the run to review
exactly what was admitted, so the pre-flight hands back the commit endpoints it
resolved that identity from, and a resumed run loads its diff from those instead
of from the refs the user typed; file_read reads at the same sealed head. Both
loads then see the same immutable objects, so a ref moving after admission can
no longer change what the run reviews, and no mid-run re-check is needed to
discover that it did. An accepted resume records one
resume_lineage event naming the parent run and the provider/model endpoints,
surfaced by `ocr session show`; it carries non-secret labels only.
Interrupted runs become unresumable, since session_end is the sole carrier of
run_manifest. That is deliberate: an unverifiable input is exactly what this
change refuses to build on, and the error says so rather than reporting the
parent as unproductive.
Refs #786
* fix(resume): freeze refs before loading identity diff
* test(resume): cover sealed input resolution
* feat(llm): task-scoped session affinity via {ocr_session_key} template variable
Derive a prompt-cache affinity key per LLM conversation, scoped to the
review session and the task within it (<session-id>-<task-type>-<hash>).
Review/scan runs bind the session ID into the request context and each
task conversation refines it where it starts, so every request carries
the real OCR session's key at per-conversation granularity — the
granularity provider prompt caches reuse prefixes at.
Embedding the {ocr_session_key} placeholder in extra_headers or
extra_body values is the opt-in: clients expand it per request, and
requests without it are unchanged. OCR never enforces a parameter or
header name, so any provider convention works with existing config
fields, e.g.:
extra_body: {"prompt_cache_key": "{ocr_session_key}"} (OpenAI)
extra_headers: x-session-affinity={ocr_session_key} (gateways)
Closes#229
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* add license headers
* edit docs
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Normalize position-dependent trailing line endings before hashing per-file diffs so unchanged files can be reused across resumed reviews.
Co-authored-by: Kite <254839944+lizhengfeng101@users.noreply.github.com>
govulncheck now fails on golang:1.26.5: the Go vulnerability database
lists fixes in 1.26.6 for seven standard-library issues reachable from
this module (GO-2026-6218, -6091, -6090, -6089, -6088, -5972, -5026),
via net/url, html/template, crypto/tls, net/http, encoding/xml and
encoding/asn1. Nothing in the module's own code changed — the step
turned red when the database was updated, and because Govulncheck runs
before "Test with coverage", every PR is now blocked before a single
test executes.
Bump the pinned image in the test, cross-compile and release jobs, and
refresh the stale version reference in the translation-sync comment so
it keeps matching the other jobs.
Verified locally under go1.26.6: govulncheck reports no vulnerabilities
and exits 0, gofmt -s and go vet are clean, go mod tidy is a no-op, and
the full test suite passes (23 packages).
Cobra's default message for a wrong positional-argument count ("accepts
2 arg(s), received 1") names neither the command nor what it expects, and
the root command sets SilenceUsage, so no usage block follows it. Running
"ocr config set provider" gave the user nothing to act on.
Add exactArgs and minimumArgs, drop-in replacements for cobra.ExactArgs
and cobra.MinimumNArgs that build the message from metadata the command
already declares: the positional signature in Use, plus Example and
ValidArgs where present. The guidance therefore cannot drift from the
command's own help output, and no command carries a hand-written error
string. This mirrors how flagErrorWithSuggestion handles the analogous
flag-error case.
Wire the seven commands that take positional arguments: config set,
config unset, rules check, session show, session comments, delegate rule
and completion. Exit codes are unchanged; only the message text differs.
The supplied count is deliberately not echoed back, since it adds nothing
the user cannot see in the line they just typed.
A tree walk over the command tree fails if a command declaring positional
placeholders still reports the raw count message, so wiring a new command
to cobra.ExactArgs directly is caught by tests.
Fixes#890
Replace the plain-text badge line in GitHub review and summary comments
with a single static shields.io badge in the format
img.shields.io/badge/<category>-<severity>-<color>, with the color keyed
off severity (low green, medium orange, high red, critical darkred).
The image renders only when both fields are known enum members; missing
or unknown metadata falls back to the existing plain-text badge so
arbitrary model output is never hotlinked into an image URL. The image
alt text keeps the plain-text badge content for screen readers and
image-load failures, the hidden ocr-id marker stays first, and the CLI's
buildBadge output is unchanged.
* feat(llmloop): add grace round after tool-request budget exhausted
When RunPerFile exits because MaxToolRequestTimes reaches zero, perform
one additional LLM call with only code_comment and task_done available.
This gives the model a final chance to submit findings it identified but
had not yet reported, preventing loss of review comments on budget stop.
* fix(llmloop): address review comments on grace round
- Check ctx.Err() before making the grace round LLM call to avoid
wasted API calls when the context is already cancelled.
- Pass messages copy to AppendTaskRecord and call rec.SetResponse so
the grace round interaction is visible in session/debug logs.
* test(llmloop): add unit tests for grace round
Cover three scenarios:
- Grace round fires and collects code_comment on budget exhaustion
- Grace round is skipped when context is already cancelled
- Grace round is NOT triggered on StopEmptyRounds
* fix(prompt): replace the fullwidth colon in the file_read tool description
tools.json advertised the example output as "File：path/to/example.go" with
a fullwidth colon (U+FF1A), while file_read.go actually emits "File: %s".
The description is sent to the model on every review, so the example did
not match the output it was describing.
Also switches action.yml's OCR_LANGUAGE example from 中文 to Chinese, for
the same reason as #861: the value is fed to the LLM and Chinese is what
the rest of the project uses.
* chore(ci): fail CI when CJK characters appear in source files
Comments, identifiers and strings in this repository are meant to be
English, but nothing enforced it — #861 had to clean up leftovers by hand,
and the same drift keeps arriving through generated code and contributions
written internally.
scripts/verify-cjk.go walks the index plus untracked files and reports Han
ideographs, kana, CJK punctuation and fullwidth forms. Written in Go rather
than shell so it does not depend on the container's grep having PCRE, and so
`unicode.Is` decides what counts as CJK instead of a byte range that would
flag the em dashes used throughout the comments. `//go:build ignore` keeps
it out of ./..., so it does not affect go vet, go build or the coverage
threshold.
Untracked files are included (--others --exclude-standard) so a new file is
checked before it lands: while writing this, the script's own comment used
Chinese punctuation as an example and went unreported until it was staged.
Two escape hatches, preferring the narrow one: an `allow-cjk: <reason>`
marker comment on a single line, or a prefix in allowedPrefixes for a whole
tree. 23 existing lines get markers (UTF-8 encoding fixtures, multibyte
truncation fixtures, language-switcher labels, the fullwidth bar used as a
terminal cursor). pages/src/i18n/ is allowlisted as translated UI copy;
extensions/vscode/ is allowlisted TEMPORARILY — its comments, test names
and zh-cn NLS bundle are still Chinese and need a follow-up pass.
Wired into CI next to the license and action-pin checks, plus
`make cjk-check` and `make check` for local runs.
* chore(ci): generalise the CJK check to all non-English text
Addresses the review feedback, and widens the rule that the feedback
exposed.
Review feedback:
- exemptMarker requires its colon, so a bare "allow-cjk" can no longer
exempt a line without giving a reason.
- The script is named for CJK but missed Hangul.
- git ls-files gains -z, so paths that are not plain ASCII arrive
unquoted, and its stderr is reported rather than a bare exit status.
- main discarded run()'s error entirely and only called os.Exit(1),
which is what made the lost stderr invisible in the first place.
- The CI step and AGENTS.md say "unapproved", since escape hatches exist.
The check was skewed by writing system rather than by language. In one
array the 'zh' and 'ja' labels each needed a marker while the adjacent
'ru' label passed untouched, and nine lines of Russian sat in the tree
unflagged: two language-switcher labels and the heading-ID fixtures.
Contributors writing Chinese had to justify every line; contributors
writing Russian had nothing to justify.
The rule is now "a letter outside ASCII", since written English needs no
letter beyond the ASCII 26 -- Cyrillic and Han as much as the diacritics
that spell German or Vietnamese. Scripts are not enumerated, so one
nobody has contributed in yet is covered when it arrives. Common and
Inherited pass, so letterlike symbols (U+2139, U+2113) are not mistaken
for prose, and combining accents are caught, so the decomposed spelling
of an accented letter cannot slip through. Symbols and emoji stay out of
scope by construction: they are not letters.
Renamed to scripts/verify-english-only.go and make english-check, and
the marker to allow-non-english:. Text spelled entirely in ASCII still
takes a dictionary to identify and stays a matter for review.
* docs(agents): restate the English-only rule as rule, homes, hatches
The rule was one dense bullet that led with the detection mechanism and
mentioned the exemptions only in passing, which is the wrong order for
the reader: an agent needs to know where a translation may go before it
needs to know which Unicode scripts are flagged. Split into three.
The homes are now spelled out from what the tree actually holds, rather
than left as "<locale> docs or an i18n table": README and CONTRIBUTING
in zh-CN, ja-JP, ko-KR and ru-RU; the doc pages under
pages/src/content/docs/ in en, zh, ja and ru; the UI copy tables in
pages/src/i18n/. Also why the two are exempt for different reasons --
Markdown by extension, the i18n tables by prefix because they are .ts --
since that decides where a new translation can safely go.
Drops the enumerated list of what "make check" runs. It duplicated the
Makefile, went stale the moment a check was added (this PR had to edit
it), and told an agent nothing it would not read in the output anyway.
What is worth saying is that the target writes to the tree.
* fix(ci): detect U+FE10–FE6F CJK punctuation in english-only check
The vertical forms (U+FE10–FE19), CJK compatibility forms (U+FE30–FE4F)
and small form variants (U+FE50–FE6F) were not caught, even though their
fullwidth counterparts (U+FF00–FFEF) already were. A small question mark
(U+FE56 ﹖) or vertical comma (U+FE10 ︐) left in source reads as correct
English punctuation and is invisible in review — the same class of typo
the fullwidth range already defends against.
Skip U+FE20–FE2F (Combining Half Marks) which are used in Latin text.
* feat(llm): add retry report data layer
Add the internal data layer for an explicit LLM request retry report: request
identity, attempt classification, and a per-run collector that freezes into an
immutable report. No behavior change — nothing is mounted on any client and no
output is produced, so this is inert until the observer is wired up.
- RequestMeta identifies one logical request (provider, model, file path, task
type, request no) and travels through the request context, so the
single-method LLMClient interface and every call site stay unchanged.
- logical_request_id is SHA-256 over a canonical NUL-terminated encoding of
run_id plus the meta. It is computed in Freeze, so the collector can be
constructed before the session exists.
- classifyAttempt derives error_class and failure_phase from the HTTP status
and the Go error type only, never from error message text. A non-2xx status
outranks the error, since it is the stronger fact.
- RetryCollector is created per run with no package-level state, is safe for
concurrent use, and drops attempts that carry no identity, which is how scan
and llm test requests stay out of the report.
- The request outcome is decided once, in Finalize, from the attempt sequence
plus the returned error and the parent context state, rather than inferred
from the last attempt: cancelling during backoff produces no new attempt, so
the sequence still ends in an error while the outcome is cancelled.
- Freeze recomputes every aggregate from the listed requests and returns a
construction error instead of publishing self-contradictory numbers. Ordering
bugs (double Finalize, mutation after Finalize) are recorded as violations
and surface there.
The report has no free-text field, so there is nothing to redact: no bodies,
prompts, URLs or raw SDK error strings. A test pins the exact set of plain
string fields so adding one has to be argued for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: 艺临 <gongyiling.gyl@alibaba-inc.com>
* feat(llm): observe retry attempts via SDK middleware
Mount a shared observer on all three LLM clients (Anthropic, OpenAI Chat
Completions, OpenAI Responses) through option.WithMiddleware, so every real
HTTP attempt the SDK retry loop makes is recorded against the logical request
that issued it.
The observer reads response headers only -- status code, request-id /
x-request-id, Retry-After (all three forms, at the SDK's own precedence),
x-should-retry -- and never touches the body, which the SDK owns and closes
before retrying. Attempts without a RequestMeta on the context are dropped
whole, which is how scan and `ocr llm test` stay out of the report.
RecordAttempt now takes the attempt's start and end timestamps instead of
pre-computed durations. observed_backoff_ms spans two attempts, so only the
collector can derive it; deriving both durations there also means the observer
cannot desynchronize numbering from the real call order. No clock abstraction
is needed and the values stay deterministic in tests.
The collector is reached through an unexported ClientConfig field rather than
new constructor parameters, keeping the three exported constructors unchanged.
It is created per run in loadLLMRuntime, not package-level, so two runs in one
process cannot share data. Nothing consumes it yet -- P5 calls Freeze at the
run boundary.
The roadmap's X-Stainless-Retry-Count cross-check is deliberately not
implemented: the SDK stops maintaining that header once ExtraHeaders overrides
it, so the mismatch branch is only reachable from a legitimate configuration,
and the desync it guards against is already caught at build time by the
exhaustion and recovery tests asserting exact attempt counts.
WithMaxRetries(5) and WithRequestTimeout are untouched; the SDK's retry
decisions are observed, never overridden.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(llm): correct attempts and finalize requests at the client boundary
The middleware can only observe real HTTP attempts, so an HTTP 200 that
carried a truncated body, undecodable JSON, a mid-stream failure, or a dead
Responses object was recorded as a success. Each client now corrects its last
attempt before returning and finalizes the logical request exactly once.
- add retry_boundary.go: classifyBoundaryError (unrecognized errors are left
alone rather than bucketed as unknown, since the only way left to tell them
apart would be message text), classifyStreamError, reviseAttempt,
finalizeRequest, streamIntegrityError and the panic sentinel
- defer the boundary on all three CompletionsWithCtx, which now use named
results; correction runs before Finalize, as the reverse order would be a
"revised after Finalize" violation and drop the whole run's report
- correct both EOF branches ahead of their ctx early return, so a parent
cancel between the two SDK calls cannot leave a truncated attempt as success
- split completionsStreaming into a wrapper with a single exit, so the four
inner returns need no correction call of their own
- replace the three bare fmt.Errorf stream integrity errors with a dedicated
type, messages unchanged
- parentCancelled reads only context.Canceled: the per-attempt deadline from
WithRequestTimeout must surface as failed, not as a user abort
- drop finalizeForTest from the observer tests; every case now reaches Freeze
through a client, so a missing defer fails that case instead of passing
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(llm): stamp request identity on review LLM requests
review 的五类逻辑请求在调用 SDK 前建立 RequestMeta，使 observer 能按请求身份收集 attempt；scan 的六类请求保持无 meta、不进报告。
- Deps 增加 NewRequestMeta 工厂字段：review 在 agent.New 注入闭包，scan 保持 nil；不用空 provider 当开关，空串是 unnamed endpoint 的合法值
- main_task / memory compression / re-location / plan / review filter 五个落点遵循固定顺序：AppendTaskRecord -> requestCtx -> 请求
- compression 的记录创建移到请求之前，使 request_no 在请求发起时即存在；orphan llm_request 对 resume 无害（applyResumeLine 无该分支），补回归断言
- ReLocateComment 拆出纯 prompt 构造 BuildReLocationMessages，internal/diff 不接触 session / meta；Duration 口径保持含 prompt 构造时间不变
- 导出 RequestMetaFromContext，供 llmloop / agent / scan 三包的测试跨包验收请求身份
* feat(cmd): publish the frozen retry report at the run boundary
在 review 运行边界冻结重试报告并经两个出口发布；scan 与 llm test 输出不变，session JSONL 与 run manifest 契约不动。
- Runner 增加后台 WaitGroup 与 WaitBackground()：agent.Run 在 dispatchSubtasks 之后、finalizeManifest 之前收口 async compression，消除 Freeze 见到未 Finalize 请求而吞掉整份报告的竞态；不加第二个超时，等待依赖 SDK 遵守取消契约
- review_cmd.go 在 ag.Run 返回后调用 Freeze，run_id 取 session 内存 UUID 而非持久化门控的 SessionID()；构造错误并入 emitErr 而非 runErr，不包装成 review failed、不触发失败 usage、不打 --resume 提示
- 报告以末位参数传给 emitRunResult / outputJSONWithWarnings，不扩展 ResultProvider；双出口去重：emitRunResult 已执行时 emitFailureUsage 不重复携带
- 终端摘要走 stdout，位于评审结果与项目摘要之间，全量渲染不截断，file_path / task_type 经 sanitizeTerminal 防控制字符注入
- JSON 在 jsonOutput 末位追加 retry_report（omitempty），直接复用 llm.RetryReport 的字段与 tag；首次成功运行输出逐字节不变
- 端到端：假 Anthropic server + 真 git 仓库驱动 runReview，覆盖干净运行、recovered+failed、全失败去重、Freeze 构造错误、session 持久化失败五个场景；manual_e2e tag 保留写码前的手工验证夹具
* test(cmd): consolidate retry report tests by responsibility
The retry-report tests for #368 P5 split coverage of emitRunResult and
emitFailureUsage into their own file, leaving the review-run emit
functions tested in two places. Move those emit-boundary cases into
emit_run_result_test.go beside the pre-existing emitRunResult tests, and
rename the remaining file to retry_report_render_test.go so it holds only
the report-rendering cases (outputRetryReportText, the JSON key-set
allowlist, retryAttemptChain). The shared retryReportFixture stays with
the rendering tests; both files are package main so it is still reachable.
No test logic changes; only relocation.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: separate cancelled retry requests
---------
Signed-off-by: 艺临 <gongyiling.gyl@alibaba-inc.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
