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.
- 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
* 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>
* 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.
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.
* 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>
* 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>
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
* 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.
`--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
* 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>
