* 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.
* fix(review-filter): add `submit_filter_result` func for review filter stage
At review filter stage, LLM(test GLM5.2) may return natural language content such as:
"""
Looking at each comment...
- commit 1: ...
- commit 2: ...
Summary:
```json [...]```
"""
add function calling for LLM will get more stable result for some model of providers.
and this tool only appears at review filter stage, so it does no harm for review quality.
* fix(filter): log tool call argument parse failures for observability
* refactor(filter): two-tool design with required choice for review filter
- Replace single submit_filter_result tool with two mutually exclusive tools:
report_incorrect_comments and approve_all_comments
- Add ToolChoice field to ChatRequest, wired through OpenAI Chat,
OpenAI Responses, and Anthropic client paths
- Set ToolChoice to 'required' so the model must always make an
explicit decision (no silent text-only fallback)
- Aggregate results across multiple tool calls for robustness
- Update prompt to instruct exactly-one-tool usage
- Preserve text-based fallback for providers without tool support
* fix(filter): stop the review filter from deleting real findings
An A/B benchmark over 194 identical commit ranges (50 OSS repos,
claude-opus-4-6) showed the filter removing 22 comments at 36% precision:
8 of them reported real defects — a heap overflow, an ignored LOCKMODE
parameter, a static/non-static linkage conflict, a dropped success status,
double URL encoding, an always-true condition.
Two causes, one of them structural.
Field order in report_incorrect_comments. Go serializes the parameter map
alphabetically, so comment_ids was emitted before any reasoning field and
the model had to commit before it had finished thinking. Replaying recorded
sessions with a diagnostic field made this visible: it wrote "this is a
protected subject, I should not remove it" while the id stayed in the list
it had already produced. Adding an "analysis" array — alphabetically first,
required — lets it reason before concluding. parseFilterToolCalls is
unchanged; it reads comment_ids and ignores the rest.
Prompt scope. Step 2 "Issue Classification" asked whether the comment
"misidentifies clearly normal code as a defect", which invited a value
judgement and was the entry point for most wrong removals. It is gone.
Removal now needs one of two grounds: the code the comment targets is
absent from the diff, or a single diff line literally contradicts its
central claim. Protected subjects (memory safety, concurrency, linkage,
behavioral change, unused parameters) and style-only comments that state
something true are vetoed as the first two steps of the method, not as
prose the model reads and then skips.
Replaying all 455 recorded filter calls with the same inputs: precision
36% -> 88%, real findings deleted 8 -> 0, at +37% filter tokens and +7%
mean latency.
Caveat: the grounds were derived from this same dataset, so the figure is
a training-set result and wants a hold-out range set before it is trusted.
---------
Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>
* 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(viewer): add review comment tag filters
* fix(viewer): normalize filter chip state values
Use the same empty-string fallback when updating filter-chip active state as
when handling clicks, preventing filters without a value attribute from
appearing inactive after selection.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* fix(viewer): improve comment tag filter controls
* fix(viewer): support combined comment filters
* fix(viewer): simplify active filter chip ring
---------
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* 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.
* feat(viewer): add defense-in-depth security headers
Wrap the local viewer with a middleware that sets a strict
Content-Security-Policy (default-src 'self', no unsafe-inline) plus
X-Content-Type-Options, X-Frame-Options, Referrer-Policy, and
Permissions-Policy on every response. HSTS is intentionally omitted
since the viewer serves plain HTTP on loopback.
To keep the CSP strict without an 'unsafe-inline' relaxation, the
formerly-inline session script is externalized to static/session.js
(it uses no template variables). Update the assurance case with a
CWE-79 countermeasure row documenting these headers.
Adds tests covering header presence, HSTS omission, and CSP strictness.
* fix: Apply suggestions from code review
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
---------
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.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>
Parse review_item_done/review_item_reused JSONL records to extract
LLM-generated review comments. Display them in the session detail page
grouped by file, with severity/category badges, line ranges, and
side-by-side code diff panels (existing vs suggested). Also add a
comment count column to the sessions list page.
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>
Redesign the OCR viewer interface with a modern developer-tool aesthetic:
- New color system with layered surfaces and indigo accents
- Dark mode with deeper tones and refined contrast
- Branded navigation with official OCR logo (inline SVG)
- Improved card, table, and accordion components
- Subtle animations with prefers-reduced-motion support
- System font stack (no external dependencies)
- color-mix() with proper fallbacks for older browsers
- Responsive layout improvements
- 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
renderTemplate logged its template-execution error with a [viewer] prefix
while the rest of the project logs under the [ocr prefix family, making
the line invisible to a grep '\[ocr' triage pass.
A census of bracketed prefixes across all Go sources shows [viewer] was
the only stdout prefix sharing no prefix with [ocr:
"[ocr] 100 sites
"[ocr session] 2 sites (internal/session) - deliberate sub-namespace,
left unchanged
"[viewer] 1 site - the outlier fixed here
Remaining bracketed literals are not log prefixes and are untouched:
[bug]/[low] are buildBadge() category/severity expectations and
[A][M][D][R][B][S] are statusBadge() file-status badges, both in
cmd/opencodereview tests.
Log text only: no test, parser, doc, or CI check consumes the string.
TestRenderTemplate_ExecutionError exercises this path but asserts only
the Content-Type header, so it passes unchanged.
Closes#415
Enhance the embedded viewer stylesheet per #304:
- Add automatic dark mode via `prefers-color-scheme` (no JS/toggle).
The full colour palette — surfaces, text, borders, accents, task/badge
brand colours — lives in `:root` custom properties, with a single dark
block overriding the values. Light-mode colours are unchanged.
- Declare `color-scheme: light dark` so native scrollbars and form
controls follow the active theme.
- Use the system monospace stack (`ui-monospace, ...`) via a `--mono`
variable, replacing the hardcoded font stacks.
- Dark tokens chosen to clear WCAG AA (>=4.5:1) for small text, and dim
brand colours (task-main, task-default) lifted for the dark surface.
Verified by rendering all three viewer pages in light and dark with a
headless browser: computed colours, contrast ratios, resolved theme
variables, and `color-scheme` asserted programmatically.
The test relied on filesystem ModTime for sorting repos, but files
created in rapid succession can share the same mtime on CI, making
the sort order non-deterministic. Use os.Chtimes to guarantee repo-b
has a strictly later mtime than repo-a.
Add integration-style tests with fake LLM clients for agent dispatch and
llmloop runner, plus new unit test files for gitcmd, session/history,
tool/code_comment, tool/filereader_read, and viewer/store packages.
The session page was showing significantly lower token counts than the console
summary because it used local tiktoken estimation while the console used actual
API-reported usage data. Now SetResponse prefers resp.Usage when available
(falling back to tiktoken), persists cache token fields to JSONL, and the
viewer displays cache read/write stats alongside prompt/completion totals.
Store ReviewMode, DiffFrom, DiffTo, and DiffCommit in SessionHistory
and persist them to JSONL (conditionally, only when non-empty). Display
mode in viewer list page and show version details (from/to or commit)
on the session detail page based on review mode.
The viewer's HTTP server (StartServer in internal/viewer/server.go) had no
Host-header validation, so a web page the user visits could DNS-rebind its
own origin to 127.0.0.1:5483 and read every session JSONL — which contains
LLM request messages (= source code being reviewed) and the LLM's analysis.
Adds a default-deny host guard that always allows loopback names plus the
concrete bind host. Wildcard binds and extra hostnames go through the new
OCR_VIEWER_ALLOWED_HOSTS env var, so an operator who binds the viewer on a
LAN interface has to acknowledge the exposure explicitly.
Detected by Aeon + manual review.
Severity: high
CWE-346 (Origin Validation Error), CWE-200 (Exposure of Sensitive Info)
Co-authored-by: aeonframework <aeonframework@users.noreply.github.com>
Re-location LLM calls were invisible in session history and their token
consumption was unaccounted for. Refactor the code_comment handling path
to record re-location requests/responses into session JSONL, accumulate
token usage into global counters, and display them in the viewer.
Also fixes: async context cancellation risk (WithoutCancel), duplicate
telemetry recording, missing timeout enforcement, missing {existing_code}
placeholder in the re-location prompt, and renames Parse to ParseComments
for clarity.
Unify the config folder name to `.opencodereview` across all runtime paths,
tests, docs, and i18n strings. Also make defaultConfigPath() return an error
instead of silently falling back to a current-directory file when $HOME is
unresolvable.
