* 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.
* feat(telemetry): add OTLP HTTP exporter and print TraceID
- Add HTTP/protobuf exporter support alongside existing gRPC exporter
- Route based on OTEL_EXPORTER_OTLP_PROTOCOL config (http/protobuf vs grpc)
- Print TraceID to stderr when telemetry is enabled for easier correlation
- Add corresponding unit tests
* feat(telemetry): add span coverage for LLM calls, tool execution, plan/filter phases
- Add StartLLMSpan / RecordLLMResult helpers (span.go), symmetric with
existing StartToolSpan / RecordToolResult
- Wrap LLM completion calls in llmloop.RunPerFile with llm.request spans
- Wrap all three tool execution paths in executeToolCall with
tool.execute.* spans (dynamic tools, code_comment sync/async, other tools)
- Add plan.execute span around executePlanPhase
- Add main.loop span around RunPerFile call in executeSubtask
- Add review_filter.execute span around executeReviewFilter, with
comments.before / comments.filtered attributes
- Record llm.error attribute on LLM failures for diagnosability
- Record review.repo / review.from / review.to / review.model on the
top-level review.run span
- Metrics (RecordLLMRequest / RecordToolCall) are preserved alongside
the new spans — they serve different purposes (aggregate dashboards
vs per-run diagnosis)
Verified end-to-end against Sunfire (OTLP HTTP gateway): full span tree
observed for review.run -> subtask.execute -> plan.execute/main.loop/
review_filter.execute -> llm.request/tool.execute.*
* fix(telemetry): address CR findings — span error handling, async span lifecycle, protocol robustness
- Add span.RecordError(err) to RecordLLMResult and RecordToolResult for
consistency with EndSpan
- Use OTel standard pattern (span.SetStatus + span.RecordError) in error
paths of review.run, plan.execute, main.loop, review_filter.execute
- Move async code_comment span end into pool.Submit callback so span
duration reflects actual execution time
- Unify time.Since(startTime) in code_comment error path to a single dur
- Remove http/json from supported OTLP protocols (not actually implemented)
- Add stderr warning when unknown OTLP protocol falls back to gRPC
* feat(telemetry): include trace_id in JSON output, restrict stderr to text format
- Add trace_id as top-level field in jsonOutput struct (omitempty)
- JSON format: trace_id in structured response for programmatic extraction
- Text format: TraceID printed to stderr for human debugging
- Telemetry disabled: trace_id field omitted entirely
* fix: address PR review findings
- loop.go: wrap async span lifecycle in defer to prevent leak on panic
- exporter.go: update parseOTLPEndpoint comment to reflect gRPC+HTTP usage
- scan_cmd.go: align traceID extraction and OTel error handling with review_cmd
- output.go/shared.go: propagate traceID to outputJSONNoFiles for consistency
- agent.go: move comments.filtered attribute before early return so 0 is
distinguishable from not-executed
* feat(telemetry): address PR review — http/json routing, LLM span coverage, trace_id tests
- Route http/json to HTTP exporter (Go OTel SDK HTTP transport only
supports protobuf serialization; users need HTTP transport, not JSON encoding)
- Add llm.request spans to executePlanPhase, executeReviewFilter, and
ReLocateComment with Usage nil-safety consistent with loop.go
- Add trace_id assertions to output helper tests and emitRunResult
end-to-end tests using real TracerProvider
* docs: add OTLP protocol selection and endpoint format to telemetry section
Sync across all 5 README language versions (en, zh-CN, ja-JP, ko-KR, ru-RU).
* fix: unify time.Since in async code_comment defer to single dur variable
* fix(tool): resolve file_read paths against git top-level in monorepos
ocr review from a monorepo subdirectory failed with "file not found" (#287):
git reports diff and `git show HEAD:<path>` paths relative to the repo root,
but RepoDir was scoped to the invocation subdirectory, producing a double
prefix. resolveWorkingDir now anchors RepoDir at `git rev-parse
--show-toplevel` on the review path (requireGit=true); scan keeps the CWD so
its `git ls-files` walk stays scoped.
The top-level lookup uses a stdout-only git helper so stderr notices can't
pollute the path, and fails loudly if --show-toplevel errors or is empty
(e.g. a bare repo) instead of silently reusing the subdirectory. Adds
regression tests for the subdir hoist, the scan-path scoping, git-show
resolution of root-relative paths, and the bare-repo failure.
* docs(rules): document repo-root rule.json resolution in monorepos
Since #287 anchored RepoDir at the git top-level, ocr review from a
monorepo subdirectory loads the repo-root .opencodereview/rule.json
rather than a subdir-local one. Call out this user-visible behavior at
loadProjectRule so the scope change isn't a surprise (review feedback).
Add two structured fields, category and severity, to every review finding
so CI integrations can sort, group, filter, or gate builds without
re-parsing natural-language comment text.
- Tool schema (tools.json): add category/severity as enum-constrained,
required properties of code_comment. severity is limited to
critical/high/medium/low (info dropped, since LLMs struggle to
distinguish low from info).
- System prompt (task_template.json) is intentionally left untouched to
avoid the review-quality regression observed on the benchmark suite;
the tool schema alone drives field population.
- JSON output: category/severity are flat siblings of content/start_line,
omitted entirely when empty (backward compatible).
- CLI output: render an inline [category - severity] badge before the
comment, colored by severity.
- Sync docs across all five README locales.
* fix(tui): persist official-tab models and refine saved secret hint
Persist user-added models to providers.<name>.models on the official tab.
When an API key or auth token is already saved, show a replace hint with a
prefix/suffix fingerprint (skipped for short keys), use a fixed mask placeholder,
and ensure typing or paste replaces the saved value instead of re-saving it.
* fix(tui): model add/delete UX and config wizard hardening
- Add model add/delete in config provider and config model (official + custom)
- Show d Delete only on model rows; green highlight when selected
- Improve Esc cancel text; track savedInSession to avoid misleading messages
- Reload config on save failure; read registry models fresh after reload
- Export llm.ModelListContains; fix config model persist using registry-only check
* fix(tui): defer provider config until confirm and harden API key UX
Only persist provider/model on wizard confirm; keep in-session picks via
sessionModelPick. Validate API key before quit, clear saved keys when emptied,
and improve official env-var hints and custom edit clear behavior.
* feat(tui): show active model suffix on official provider list
Align Official tab with Custom: display (model) next to the active preset
when cfg.Provider matches and a global model is configured.
* feat(mcp): add Model Context Protocol server support
Add MCP client and provider packages that allow integrating external
MCP tool servers into the review loop. Includes config commands for
managing MCP servers, stdio subprocess integration tests, and
comprehensive test coverage.
* refactor(mcp): rename loop variable in contentToText to avoid shadowing Client receiver
* fix(mcp): use platform-specific shell for setup command
The MCP server setup command was hardcoded to use `sh -c`, which
fails on Windows. Extract a `shellCommand` helper behind build tags
to use `cmd /c` on Windows and `sh -c` elsewhere.
* docs(mcp): add MCP server documentation to all README locales
