* 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.
* 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>
`--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
* 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.
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.
* 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.
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>
* 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
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
* 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>
* feat(llm): support custom retry status codes via retry_codes config
Add a retry_codes configuration field that allows users to specify
additional HTTP status codes (4xx) that should trigger exponential
backoff retry. This targets self-hosted LLM clusters that misuse
status codes like 403 or 400 for rate limiting.
Implementation uses SDK middleware to inject the x-should-retry: true
response header when a configured status code is encountered, leveraging
the existing SDK retry mechanism (up to 5 retries with exponential
backoff) without any SDK modifications.
Configuration is supported via:
- Provider config: providers.<name>.retry_codes / custom_providers.<name>.retry_codes
- Legacy llm config: llm.retry_codes
- CLI: ocr config set providers.<name>.retry_codes 403,400
Also fixes cloneProviderEntry to copy previously missing fields
(TimeoutSec, ExtraHeaders, RetryCodes) for TUI rollback correctness.
* test(llm): add unit tests for retry_codes feature
Cover ParseRetryCodes validation, retryCodesMiddleware behavior,
resolver integration (provider and legacy config paths, invalid codes),
and end-to-end retry via httptest for both OpenAI and Anthropic clients.
* feat(llm): silently filter redundant retry_codes (408/409/429) instead of erroring
Replace validateRetryCodes with sanitizeRetryCodes that filters out
SDK-default codes and returns warnings. The resolve phase silently
drops redundant codes without interrupting startup. The config set
command prints warnings to stderr so users are informed.
This makes the tool friendlier for users migrating from other tools
who habitually configure 429 and similar codes.
* test(tui): add coverage for cloneProviderEntry deep-copy of TimeoutSec/RetryCodes/ExtraHeaders
Verify that the clone correctly copies these fields and that
mutations to the clone do not affect the original. Also pin the
nil-stays-nil behavior for RetryCodes and ExtraHeaders.
* docs(llm): fix contradictory ParseRetryCodes comment
* test(config): cover retry_codes warning output in config set path
Verify that redundant codes (429, 408) emit a WARNING to stderr while
still writing the valid codes to config. Also verify that valid-only
codes produce no warning output.
runPreview, runScanPreview, and delegate's preview each built a throwaway
agent via agent.New / scan.NewAgent purely to reach Preview. Both
constructors auto-create a session, and session.New opens persistence and
buffers a session_start record, so every preview created a JSONL file
under the OCR home. Preview never runs or finalizes a review, so that
file was left unfinalized and usually empty.
Make the exported entry point a package-level Preview(ctx, args) in both
internal/agent and internal/scan. It builds only what file selection
needs, so there is no session, manifest, or runner to leak. The existing
bodies stay as unexported methods, keeping the in-package tests
(including scan's regression test that Preview must not mutate a.items).
Deleting the file afterwards was rejected: it would still leak on crash
and would keep the wrong abstraction.
Tests assert at the CLI boundary, with a temporary OCR home, that no
session store is created by any of the three preview commands. They also
neutralize global git config, which git resolves via XDG_CONFIG_HOME
independently of HOME.
Both commands advertise `--format text|json` and parse `-f json -p`
without error, but the preview paths called outputPreviewText
unconditionally and never consulted the requested format, so automation
asking for JSON got the human view instead.
Route runPreview and runScanPreview through a small dispatcher so
`--format json` emits the existing model.Preview DTO as a single JSON
value on stdout. Only "json" opts out of the human view, so text output
is unchanged.
Agent.Preview also left Entries nil for an empty diff, which marshals as
`"files":null`. Scan preview already preallocated the slice; do the same
for review so both emit `"files":[]` and consumers can iterate
unconditionally.
Note the new behavior on both `--preview` rows of the CLI reference, in
all four locales.
* test: raise statement coverage to 90% and enforce it in CI
Add unit tests across the cmd and internal packages to bring total
statement coverage above 90%, and gate future regressions.
- Cover CLI helpers, provider TUI handlers, resume/manifest paths, and
error branches in config, llm, llmloop, scan, session, agent, viewer,
mcp, pathutil, and telemetry.
- Raise the coverage threshold from 80% to 90% in the Makefile
(COVERAGE_THRESHOLD) and in the CI "Check coverage threshold" step.
- Ignore generated coverage.out and coverage.html artifacts.
Total statement coverage is now 90.5%, measured consistently by both
`make coverage` and the CI `go test ./...` scope.
* test: widen statement coverage margin with environment-independent unit tests
Add table-driven unit tests for pure, environment-independent functions to
raise the statement-coverage safety margin above the 90% threshold:
- session.ResumeState.ValidateScanOptions (70% -> 100%)
- rules.SystemRule.UnmarshalJSON error branches (71% -> 82%)
- llmloop.stripMarkdownFences no-newline branch (82% -> 100%)
- diff.firstLine empty/blank-input branch
- diff.extractCodeBlock missing-newline and no-closing-fence branches
- main.truncate n<=1 and normalization branches
- agent.Agent nil-receiver accessor guards
* chore: add SPDX license headers to all source files
Add Apache-2.0 SPDX license identifiers and copyright notices to all
tracked .go, .sh, .js, .mjs, .ts, and .tsx source files.
Introduce scripts/verify-license.sh and scripts/add-license.sh for
automated verification and bulk addition of license headers. Integrate
the check into CI (ci.yml) and the Makefile (license-check target as
a prerequisite of the existing check target).
This satisfies the OpenSSF Best Practices Badge requirements for
copyright_per_file and license_per_file.
* fix: restore execute permissions on scripts
* docs: add license header instructions to CONTRIBUTING guides
* docs: add license header instructions to pages contributing guides
* fix(pages): strip unclosed HTML comment markers to satisfy CodeQL
* fix: apply code review suggestions for license scripts
- Fix portability: detect macOS vs Linux stat for permission copy
- Fix has_header: check both SPDX and copyright (match verify logic)
- Fix is_ignored: match on path boundaries to avoid false positives
- Fix year extraction: use consistent pipeline across both scripts
- Fix Bash 3.2 compat: quote array length expansion for set -u
* fix(pages): use loop-until-clean for HTML comment stripping (CodeQL)
* fix(pages): use split/join instead of replace to avoid CodeQL false positive
CodeQL's js/incomplete-multi-character-sanitization rule flags any
.replace() that removes multi-character sequences like '<!--...-->',
regardless of context. The data here comes from readFileSync on the
project's own index.html (no untrusted input), making this a false
positive. Using split(regex).join('') achieves the same result without
triggering the taint-tracking rule.
Parent commands (session, config, delegate, llm, rules) were defined
without a RunE function. In Cobra, when a parent command has no RunE
and receives an unrecognized subcommand, it falls back to displaying
help and returns nil (exit 0). This is inconsistent with the root
command and leaf commands, which correctly return exit code 1 with an
error message.
Add RunE: func(cmd *cobra.Command, args []string) error { return cmd.Help() }
to all five parent commands so Cobra properly reports "unknown command"
errors instead of silently succeeding.
Includes tests verifying:
- unknown subcommands produce a non-nil error containing "unknown command"
- known subcommands still route correctly
- bare parent commands (no args) still print help and return nil
Closes#641
Adds a new 'ocr session comments <session-id>' subcommand that prints the
review comments persisted in a session, rendered in the same style as
'ocr review' terminal output (path, line range, severity badge, suggestion
diff). Supports --json for machine-readable output and --severity/--category
comma-separated filters.
Comments are read from the review_item_done / review_item_reused checkpoint
records via a new session.LoadComments, mirroring resume replay semantics:
a later checkpoint for the same fingerprint supersedes the earlier one and a
subsequent failure drops it.
Also adds shell tab completion for session ids (session show, session
comments, and review --resume) and for the --severity/--category values.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* feat(session): add run manifest coverage data model and builder
First slice of issue #367 (run manifest coverage contract): the data
model and state machine only. Not yet wired into the agent or CLI, so
existing review/scan output is unchanged.
Introduce the versioned, immutable RunManifest (schema ocr.run-manifest/v1)
and a concurrency-safe ManifestBuilder that tracks per-file coverage
(selected/completed/reused/failed/waived) and freezes into a terminal
state.
- terminal state derived solely from coverage sets, never comments/warnings
(complete/partial/failed/skipped)
- Finalize sweeps any undecided selected item to failed/unknown so no item
is silently dropped
- single-mutex builder: first terminal state wins, frozen after Finalize,
nil-receiver safe
- fixed failure classification enum with an unknown catch-all
- redaction floor on failure/waive reasons (strip secrets, cap length) as a
single write entry so callers cannot bypass it
- 22 unit tests, race-clean
Refs: issue #367
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(session): harden run manifest per adversarial review
Address findings from the concurrency / JSON-contract / PR#306-coupling
adversarial review of the manifest data model (still slice 1; not wired to
agent or CLI).
- SetSweepClass: Finalize can classify undispatched items as cancelled/budget
instead of a blanket unknown (the one real model gap the review found)
- ItemID(fingerprint)=SHA-256 canonical mint helper; an item_id is never a raw
fingerprint, keeping the resume cross-reference explicit and mix-ups caught
- sanitizeReason: strip control/ANSI chars, coerce valid UTF-8, redact quoted
secret values, guarantee single line
- Finalize returns deep-copied coverage slices so the frozen snapshot is never
aliased across the two outlets
- RegisterSelected: nil-safe (lazy-init map) + documents that only the
post-deletion/post-filter dispatchable set may be registered
+7 unit tests (29 total), race-clean.
Refs: issue #367
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(manifest): wire input identity, config hashes and run-level failure (shard ②d)
- Freeze per-mode input identity (mode + resolved_base/head + exact_range +
source_artifact_sha256) via diff.ResolveInput/commitParents, and repository
identity via RemoteIdentity/canonicalRemote (credential-free).
- Add rule_config_sha256 and runtime_config_sha256 over an allowlist of
non-secret fields using a length-prefixed SHA-256 framework (no tokens/URLs).
- Replace SetRunLevelFailure(bool) with structured SetRunFailure(class, reason)
and set ManifestInput.mode; fill execution.* (ocr version, provider, model,
concurrency, config hashes).
- Thread error returns through Finalize/WriteSessionEnd (main review path
surfaces them; skip/all-failed/scan paths hardened in follow-up).
- Tests: manifest_hash, canonical_config, git_resolve.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(manifest): propagate persistence errors and harden remote/error classification
Merged review themes A/B/E from the 07-22 consolidated assessment.
Theme A — Finalize / session_end delivery errors no longer swallowed:
- agent.go no-files path returns the Finalize error instead of nil (A1)
- agent.go loadDiffs failure joins the Finalize error via errors.Join (A2)
- session.Finalize uses sync.Once + cached finalizeErr: written exactly
once, concurrency-safe, and every caller replays the same result so a
retry cannot falsely report success (A3)
- scan/agent.go wires both Finalize call sites to surface the error (A4)
Theme B — canonicalRemote rewritten (internal/diff/git.go):
- keep the port (u.Host, not u.Hostname) so endpoints differing only by
port stay distinct (B1)
- split scp syntax on the first ':' so an '@' inside the path survives (B2)
- recognize local/file/Windows/UNC remotes and omit identity rather than
misparsing a path as a host (B3; local-remote policy still open)
Theme E — main_task-empty is now a sentinel (errMainTaskEmpty) classified
via errors.Is instead of matching error text.
Theme D (TOCTOU) deferred to shard 4 per issue #367 open-issues OI-12.
Tests: go build ./... + go vet + go test ./... all green (23 pkgs).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(manifest): report both dispatch and persistence errors on the normal path
The success-path Finalize wiring used `ferr != nil && err == nil`, so when the
review (or scan) failed AND session_end also failed to persist, the persistence
error was dropped and only the dispatch error surfaced — the caller never
learned the session/manifest was not saved.
Join both with errors.Join when both occur (matching the loadDiffs path), so a
persistence failure is always reported even alongside a dispatch failure. This
closes the last gap in the OI-10 contract.
- internal/agent/agent.go: review normal path
- internal/scan/agent.go: scan normal path (+ errors import)
Tests: go build ./... + go vet + go test ./... all green (23 pkgs).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(manifest): 接入 CLI 与 viewer 并补齐验收用例
- 使用冻结 manifest 统一 review JSON、文本与退出状态\n- session CLI 和 viewer 展示五集合覆盖并兼容 legacy/aborted\n- 补充本地 mock、跨出口一致性及安全验收用例
* test(manifest): 补齐验收矩阵缺口并修复审核发现的缺陷
验收用例：configuration 分类（run 级 sweep + item 级映射）、budget/timeout/panic 混合 partial 隔离、跨出口一致性改为规范化原始字节比对、flag 校验失败无产物断言。
代码修复：sanitizeReason 先剥控制字符再脱敏（堵控制字节绕过）、失败项异分类二次标记报冲突错误、source_artifact_sha256 按 item_id 去重并稳定排序、sortItems 改 SliceStable 对齐设计用词。
全仓 go test 23 包通过。
* test(manifest): 补充 provider transition resume 测试用例
覆盖 issue #367 验收标准 provider transition：resume 时 provider/model 改变后，子 manifest 记录当前值而非继承父运行，并经 parent_run_id 链接父会话以支持审计。用 mock client，不依赖真实 provider key。
* fix(manifest): 对齐预算终态与持久化语义
统一聚合预算停止时的 coverage、status 与退出码。传播 session writer 初始化错误，并补齐 merge first-parent 输入身份及回归测试。移除代码注释中的外部设计文档引用。
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: kite <254839944+lizhengfeng101@users.noreply.github.com>
The supported top-level config keys lived both in the switch/case in setConfigValue and, hardcoded again, in the unknown-key error message, so adding a key meant updating two places. Move them into a supportedConfigKeys slice and generate the error message from it. The message content is unchanged.
Closes#637
runConfig manually dispatched config subcommands and parseConfigArgs did manual arg slicing, duplicating the routing the production config command already defines with cobra. Rebuild runConfig as a fresh cobra tree mirroring production (matching the runSession pattern), and drop parseConfigArgs, configAction, and configParseError, which are no longer used. Dispatch behavior is unchanged and stays covered by the existing runConfig/config_dispatch tests.
Closes#638
* refactor(cli): migrate to Cobra framework for shell completion support
Replace the hand-rolled ocrFlagSet + switch dispatch with spf13/cobra,
enabling native bash/zsh/fish/powershell completion via `ocr completion`.
Key changes:
- Add root.go (rootCmd definition, version flag with -V shorthand)
- Add completion.go (ocr completion [bash|zsh|fish|powershell])
- Add shared_flags.go (reusable flag registration helpers + validation)
- Rewrite all *_cmd.go to use cobra.Command with RunE
- Delete flags.go (ocrFlagSet, expandShortFlags, parseXxxFlags)
- Add compat_test.go (test compatibility wrappers for existing tests)
- Promote github.com/spf13/cobra from indirect to direct dependency
Behavioral improvements over the previous implementation:
- Shell completion for all commands, flags, and enum values
- "Did you mean?" suggestions for misspelled commands
- cobra.NoArgs on review/scan prevents silent positional arg ignoring
- Cleaner error messages on unknown flags (no full flag dump)
- Consistent help output format across all subcommands
Closes#576
* fix(cli): add Args: cobra.NoArgs to viewerCmd
Prevents `ocr viewer localhost:3000` from silently ignoring the
positional argument and starting on the default address.
Consistent with reviewCmd and scanCmd.
* feat(cli): add "Did you mean?" suggestions for misspelled flags
Cobra only suggests corrections for unknown subcommands, not flags.
Add a levenshtein-distance based suggestion that fires when cobra
returns an "unknown flag" error, matching the same UX pattern.
Examples:
--hel → Did you mean this? --help
--audienc → Did you mean this? --audience
--comit → Did you mean this? --commit
* fix(deps): promote spf13/pflag to direct dependency
After the Cobra migration, pflag is directly imported but was still
marked as indirect in go.mod, causing CI's go-mod-tidy check to fail.
* refactor(cli): use idiomatic cobra patterns for args validation and flag errors
Replace hand-rolled argument validation in configSetCmd/configUnsetCmd
with cobra.ExactArgs, and move flag typo suggestions from post-hoc error
string parsing into SetFlagErrorFunc where cobra provides the command
context directly.
Treat FAILED and missing completion as review failures, and retry
invalid task_done states instead of accepting them as success.
Keep partial scan output and session history aligned with those states.
Co-authored-by: 4-1-1 <4401981+4-1-1@users.noreply.github.com>
Co-authored-by: kite <254839944+lizhengfeng101@users.noreply.github.com>
Add a runtime token-budget guardrail to the diff-review path so a large MR
stops itself before runaway cost (issue #409: ~90.4M tokens in one failed
attempt), instead of timing out and losing all structured output.
New `ocr review` flag (mirror scan's proven --max-tokens-budget):
- --max-tokens-budget N : cap aggregate token usage; dispatch stops once the
running total + a per-file look-ahead would exceed it (0 = unlimited).
Mechanism mirrors scan/agent.go's dispatchBatch gate exactly: read the
existing atomic Runner counters (no duplicate counter — a second one would be
a drift bug) before acquiring the semaphore, and break the dispatch loop on
exceed. In-flight workers are allowed to finish (overrun bounded by the
in-flight count, <= concurrency), matching scan's documented contract.
Budget exhaustion returns the partial comments already produced with a nil
error (not a Go error — the failure path suppresses output), and signals
budget-exceeded out-of-band so the output layer sets a typed `budget_exceeded`
status distinct from success / completed_with_warnings / completed_with_errors.
Additional changes tied to the invariants:
- Pre-review scale warning (files, diff tokens, configured budget) printed
before any model spend; non-blocking, warn-only.
- Structured usage emitted on the failure path (stderr) so the cost of a
failed attempt is never lost. Carries only token/tool tallies — no
credentials or prompts. Reports the agent's actual BudgetExceeded() state so
the residual budget-trip + all-dispatched-fail edge can never contradict the
typed status.
- summary.budget_exceeded JSON field (additive, omitempty) so old parsers are
unaffected; default 0/unlimited restores prior behavior.
Internal/scan gains only the BudgetExceeded() accessor (returns false) so the
shared ResultProvider interface compiles; scan keeps its own token budget and
its JSON output is unchanged.
Tests mirror internal/scan/budget_test.go: token-budget gate stops dispatch
early and sets BudgetExceeded(); unlimited default runs all files; estimate
helpers project sane values (with a humanTokens parity table so the two copies
cannot diverge); JSON output asserts the typed status and the failure-path
usage record. Full `make test` (-race) green.
- implement the documented `ocr v` alias for viewer in dispatch
- list the implemented --exclude flag in `ocr review -h`
- mention mcp_servers.<name> in the `config unset` usage error
- add missing delegate/viewer -h hints to top-level usage
- print localhost instead of wildcard hosts in viewer banner URLs
* feat(mcp): add remote MCP server support via Streamable HTTP transport
Support connecting to remote MCP servers using the Streamable HTTP
transport from go-sdk. Users configure remote servers with type=remote,
a URL, and optional headers (with environment variable expansion for
secrets). Authentication stays outside OCR — users provide tokens via
headers, matching CLI tool conventions.
Closes#335
* docs(mcp): document remote MCP server support and shell expansion warning
Add remote transport (Streamable HTTP) documentation to MCP Server
sections in all READMEs and CLI help text. Include type/url/headers
field descriptions, remote server examples (CLI and JSON config),
and a tip about using single quotes to prevent shell expansion.
* fix(mcp): harden remote MCP client validation and error handling
- Reject URLs with missing host (e.g. "http://") in config validation
- Make empty-after-expansion header values a fatal error instead of a warning
- Detect HTTP 401/403 responses and surface clear authentication error messages
- Drain response body before closing to preserve connection pool reuse
* test(mcp): add tests for header env var expansion failure in NewRemoteClient
Verify that NewRemoteClient returns a clear error when header values
expand to empty strings, covering both explicitly empty env vars and
completely unset env vars.
The module path was github.com/open-code-review/open-code-review but the
repo lives at github.com/alibaba/open-code-review. This mismatch prevents
pkg.go.dev indexing and breaks Go Report Card resolution.
The LLM resolver honors providers.<name>.timeout_sec and llm.timeout_sec
(and the docs tell users to hand-edit them into config.json), but the
cmd-side ProviderEntry/LlmConfig structs lacked the field. Every
loadOrCreateConfig + saveConfig cycle — any `ocr config set`,
`ocr config model`, or interactive provider setup — rewrites the whole
file from those structs, silently dropping the key and reverting the
per-request timeout to the 300s default.
Add the field to both structs so the value survives round-trips. This
intentionally does not add `ocr config set` write support for the key,
matching the documented behavior.
* feat(delegate): add delegation mode for host-agent driven code review
Add `ocr delegate` subcommand that provides deterministic file selection
and rule resolution without calling any LLM. This enables AI coding agents
to perform reviews themselves using OCR only for engineering scaffolding
(preview which files to review, resolve grouped rules by path).
Includes:
- `ocr delegate preview` — outputs reviewable file list with mode/ref metadata
- `ocr delegate rule <path...>` — outputs review rules grouped by content
- Claude Code plugin command (delegate-review.md)
- Skill definitions for Claude Code, Codex, and Cursor
- Unit tests for internal/delegate package
- README documentation synced across all 5 locales
* fix(delegate): group rules by source, pattern and text
GroupRules keyed groups on rule text alone, so files sharing identical
rule text but resolved from different sources or matched by different
patterns were merged into one group that kept only the first file's
Source/Pattern metadata. Use a composite (source, pattern, text) key so
each group's provenance is accurate for every file it contains.
* feat(llm): add OpenAI Responses API support
Add `openai-responses` as a third LLM protocol alongside `anthropic` and
`openai-chat-completions`, enabling code review via the OpenAI Responses
API (/v1/responses) for GPT-5.x / o-series models.
Protocol naming refactor (backward-compatible):
- Canonicalize "openai" -> "openai-chat-completions" (alias still accepted)
- Add NormalizeProtocol / ValidateProtocol / IsAnthropicProtocol helpers
- Registry uses canonical constants; resolver normalizes everywhere
New OpenAIResponsesClient (stateless replay, per DESIGN_STATE_CACHE_PHASE.md):
- system messages -> Instructions; tool calls -> function_call items keyed
by CallID so the agent loop pairs results correctly
- store=false (privacy); prompt_cache_key = sha256(instructions)[:32]
- Phase fields (commentary/final_answer) dropped with TODO for gpt-5.3-codex+
Config plumbing:
- llm.protocol field + OCR_LLM_PROTOCOL env (priority over use_anthropic /
OCR_USE_ANTHROPIC); TUI exposes all three protocols in Custom & Manual
- anthropic-vertex rejected with friendly "not yet implemented" message
Docs: protocol reference, config examples, env var table, and Responses API
notes (store=false caching caveat, cache key derivation, Phase TODO) updated
across en/zh-CN/ko-KR/ja-JP/ru-RU READMEs.
* refactor(llm): switch PromptCacheKey to precomputed scheme via ChatRequest.CacheKey
Replace per-turn sha256 computation inside buildResponsesParams with a
precomputed cache key that callers compute once per session and pass
through ChatRequest.CacheKey (json:"-"). The key now incorporates the
first user message alongside instructions, so different files under
review land in distinct cache buckets — the previous instructions-only
key was identical across all files.
Changes:
- ChatRequest gains CacheKey string field (json:"-", zero impact on
Chat Completions / Anthropic clients which never read it)
- New llm.ComputeCacheKey helper: sha256(instructions + "\x00" +
firstUser)[:32]
- responses_client.go: reads req.CacheKey directly, removes promptCacheKey
function and first-user-message scanning
- loop.go: RunPerFile computes cacheKey once before the loop, reuses
every turn
- All 8 remaining call sites (agent, scan, relocation, compression,
llm_cmd) compute once at request construction
- Update PLAN_RESPONSES_SUPPORT.md and DESIGN_STATE_CACHE_PHASE.md to
reflect the precomputed scheme
- Update tests: passthrough tests for client, dedicated TestComputeCacheKey
* refactor(llm): use canonical protocol name "openai" and UUID-based session ID for cache key
Two changes to maximize backward compatibility and simplify the design:
1. Protocol naming: revert ProtocolOpenAIChatCompletions value from
"openai-chat-completions" back to "openai". Old config files with
protocol: "openai" are now identical to what new configs write —
zero behavioral difference. The alias direction in NormalizeProtocol
is reversed: "openai-chat-completions" -> "openai" (for configs
written during this branch's testing phase only).
2. Cache key: replace content-based sha256 hash (ComputeCacheKey) with a
random UUID session ID. The agent loop generates one UUID per file in
RunPerFile and passes it via ChatRequest.SessionID; the Responses
client uses it as prompt_cache_key. Single-turn call sites no longer
set a cache key (no multi-turn caching benefit). This removes the
need to scan messages or compute hashes, and eliminates collision
risk between files with similar content.
ChatRequest.CacheKey is renamed to SessionID to reflect its actual
semantic — a per-session identifier that the Responses client
repurposes as prompt_cache_key.
Also updates PLAN_RESPONSES_SUPPORT.md, all 5 README translations,
test expectations, and promotes google/uuid to a direct dependency.
* refactor(llm): remove IsAnthropicProtocol helper and anthropic-vertex special case
* refactor(llm): remove openai-chat-completions branch-internal alias
* docs: remove DESIGN_STATE_CACHE_PHASE and PLAN_RESPONSES_SUPPORT design notes
* fix(llm): address code review findings on Responses API support
- provider_cmd: clear stale use_anthropic when switching to openai-responses
- resolver: validate preset protocol with ValidateProtocol for consistency
- responses_client: swap usage mapping to resolveUsage-first (matches OpenAIClient)
- responses_client: map failed/cancelled statuses to 'error' finish reason
- usage_resolver: add Responses API field paths (input_tokens, output_tokens,
input_tokens_details.cached_tokens)
- add tests for all four fixes
* fix(llm): mirror use_anthropic when setting llm.protocol
- config_cmd: 'ocr config set llm.protocol' now mirrors use_anthropic
(anthropic -> true, OpenAI family -> false) for backward compat with
older binaries that predate llm.protocol
- provider_cmd: openai-responses now sets use_anthropic=false instead of
nil, so older binaries fall back to the OpenAI family rather than
wrongly defaulting to anthropic
- update tests for both write paths
* fix(llm): mirror protocol when setting llm.use_anthropic
- config_cmd: 'ocr config set llm.use_anthropic' now mirrors protocol
(true -> anthropic, false -> openai) so the two fields never disagree,
matching the reverse llm.protocol mirroring added previously
- without this, setting use_anthropic=true while protocol=openai-responses
left a contradictory config that misled older binaries into using the
anthropic protocol against an OpenAI endpoint
- extend tests to cover both values and stale-protocol overwrite
* docs(llm): fix NormalizeProtocol comment to match lowercasing behavior
The comment claimed unknown values are 'returned unchanged', but the
default branch lowercases and trims them (corroborated by the
'gRPC -> grpc' test). Update the wording to describe the actual
behavior so callers aren't misled about round-trip fidelity.
* docs(llm): drop OpenAI Responses API implementation notes from READMEs
* fix(llm): address code review findings on Responses API support
- config: preserve openai-responses when setting llm.use_anthropic=false
(only mirror to openai when protocol is unset or a legacy anthropic/openai)
- config: add Protocol values guidance to unknown-key error message
- protocol: extract normalized local var in NormalizeProtocol
- responses_client: align SDK base URL trimming with NewOpenAIClient
- responses_client: drop unused test-only sdkBaseURL method
* fix(llm): use protocol constants consistently
- providers: edenai now uses ProtocolOpenAIChatCompletions like the rest
of the registry instead of the "openai" string literal
- provider_cmd: print the normalized protocol variable (what is actually
saved) instead of the raw TUI value
* fix(llm): drop stream key and surface non-completed status in Responses client
Address two PR review comments on OpenAI Responses API support:
1. extra_body.stream=true was forwarded to Responses.New, making the API
return SSE while the SDK expects JSON and breaking every call. Skip the
'stream' key (like OpenAIClient treats it as a non-forwarded key) while
still forwarding other extra_body entries.
2. The Responses API returns HTTP 200 even for failed/cancelled (terminal)
and queued/in_progress (background) states, so the SDK reports nil error.
Surface these as real errors so callers branching on err != nil (ocr llm
test, review loop) fail instead of treating a dead response as success.
Add table-driven tests covering both fixes.
- Add ContextWithTraceParentFromEnv to extract TRACEPARENT env var and
inject upstream span context via OTel TextMapPropagator.
- Register TraceContext+Baggage composite propagator in Init().
- Wire trace parent propagation into review and scan entry points.
- Add tests for valid, absent, disabled, and malformed TRACEPARENT.
