* 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>
* fix(diff): honor .gitignore negation patterns
Patterns were resolved with a first-match-wins scan that discarded any
`!` line outright ("negation patterns are not needed for exclusion
purposes"). That holds for a blocklist .gitignore, but inverts the result
for the allow-list idiom github/gitignore ships per language: `*` to
ignore everything, then `!` lines to re-include. Because a bare `*`
basename-matches every file, every path in such a repository resolved as
excluded.
The failure is silent. `ocr review` reports "0 reviewable / 0 total" and
`--preview` prints "No files changed", both of which read as a clean
review of a repository that was never looked at. It reaches `ocr scan`
and the agent's file_find tool too, since both filter through the same
matcher.
Resolve patterns the way git does — in file order, last match wins, `!`
inverting that pattern's verdict — and while in here support the two
constructs the allow-list idiom needs: a leading `/` anchoring a pattern
to the repository root, and `**`, routed through doublestar (already a
dependency) since filepath.Match cannot express it.
Two deliberate asymmetries:
- The hardcoded directory blocklist (.git/, node_modules/, vendor/…)
still short-circuits, so a negation cannot re-admit those.
- A negated directory-only pattern is inert. Git uses `!*/` to keep
descending into subdirectories, not to re-admit the files inside
them; honouring it against file paths would readmit everything below
the root. Positive directory-only patterns now also match on
directory components only, so `vendor/` no longer matches a file
named `vendor`.
MatchGitignorePattern keeps its existing contract: a negated pattern
reports false, so callers testing one pattern in isolation still read it
as "does this exclude the path". Ordered resolution, where negations
carry meaning, lives in isPathExcluded.
Verified against a repository using Go.AllowList.gitignore: file
discovery goes from 0 reviewable / 0 total to 9 reviewable / 18 total,
with the ignore file untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(diff): anchor the path-suffix match on a component boundary
The suffix fallback compared raw strings, so a pattern containing "/" also
matched a path whose directory merely ends in the pattern's first component:
"src/main.go" excluded "othersrc/main.go", which git never matches — a
pattern with a "/" is anchored to the repository root.
Requiring the suffix to start at "/" keeps the intentionally loose
"generated/api.go" matches "src/generated/api.go" behaviour while dropping
the partial-component case. Two cases added to TestMatchGitignorePattern.
Pre-existing rather than introduced here; the line is in this diff because
of the anchored-pattern guard, and the fix is a one-liner, so it is folded
in rather than deferred.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Java implementation referenced by these comments
(processTargetLineCode, TaskCheckPoint, subtaskExecutor, the Java-side
LlmConversation) does not exist in this repository, so the
cross-references cannot be verified. Trim or reword each comment to keep
the behavioral explanation without the stale Java reference.
Comment-only change; no behavior is affected.
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.
* fix: remove hardcoded 180s timeout from REVIEW_FILTER_TASK
The filter task had a hidden inner timeout (ft.Timeout) that always
fired before the global OCR_LLM_TIMEOUT (300s) could apply. Removing
it lets the task flow through the same two timeout knobs
(OCR_LLM_TIMEOUT / --timeout) as every other task type.
Also cleaned up unused timeout fields from task_template.json that
were never read in production code.
* fix: remove dead timeout block from RE_LOCATION_TASK per review feedback
Plain `git show` renders a merge commit as a combined diff ("diff --cc"
sections with @@@ hunks), which ParseDiffText does not handle: every
section is silently dropped, so `ocr review --commit <merge>` exits 0
with "No supported files changed" even when the merge contains conflict
resolutions — precisely the content that most needs review. Worse, a
combined section following a regular one is absorbed into the previous
file's diff text.
Pass --diff-merges=first-parent to the ModeCommit git show call so a
merge commit is diffed against its first parent in regular unified
format. Non-merge commits are unaffected (verified: root commits still
diff against the empty tree). The flag requires git >= 2.31; the project
already requires git >= 2.41.
ParseDiffText classified lines with two unanchored heuristics:
- binaryRe ("Binary files ") matched anywhere in a file's diff section,
including hunk content, so a text file whose change merely mentions the
phrase (docs, comments, log strings) was marked binary and silently
excluded from review.
- Insertions/Deletions counted "+"/"-" lines while excluding anything
starting with "+++"/"---" to skip file headers — but that guard also
drops real content lines, e.g. an added line "++i" renders as "+++i".
Besides skewing the reported stats, the undercounted changeLines total
gates the plan phase in the review agent.
Track hunk state instead: a line belongs to a hunk only after the "@@"
header, and inside a hunk every content line carries a leading marker, so
"+"/"-" lines always count and the header strings can never appear.
Anchor binaryRe to column 0 (git always emits the marker there) and
restrict the "/dev/null" header markers to the pre-hunk region, where an
added line "++ /dev/null" cannot be misread as a deleted-file header.
Follow-up to #381, which normalized the argument ordering in
workspaceTrackedDiff (options -> positional ref -> --) but stopped short of
the --end-of-options guard. This adds it before HEAD in the first runGit
call, bringing workspace mode fully in line with the canonical range/commit/
merge-base calls in this file (git.go:119/126/256), which already require
git >= 2.24.
The --staged fallback call is untouched: it has no positional ref (only the
-- pathspec separator), so --end-of-options would guard nothing there.
Also documents why the --staged fallback is load-bearing (repos with no
commits have no HEAD, so `git diff HEAD` fails while `git diff --staged`
still surfaces staged changes against the empty tree) and pins it with
TestWorkspaceDiffNoCommitsUsesStagedFallback.
Closes#374
* fix(diff): preserve non-ASCII paths
Disable Git path quoting when generating diffs so parsed paths can be read back correctly. Add regression coverage for workspace, commit, and range modes.
* fix(diff): preserve untracked non-ASCII paths
Disable Git path quoting when listing untracked files and add regression coverage for non-ASCII workspace paths.
* 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: ensure current file path is always injected for code_comment and improve line number tracking in resolver
* test: Cover blank-line matching in resolveFromFileContent fallback
Add a regression test for snippets that omit blank lines while the
source file retains them, and document that consecutive matching skips
blank lines on both sides.
* test: Add tests for resolving line numbers with blank lines and CRLF
* feat: add ocr scan for full-file code review
Introduce a new top-level subcommand `ocr scan` (alias `s`) that reviews
whole files instead of git diffs. Use cases include reviewing unfamiliar
codebases, pre-migration audits, and ad-hoc per-directory reviews.
Architecture splits scan and diff review at the package level so the two
pipelines can evolve independently:
- internal/scan/ new package: file enumeration via `git ls-files`,
full-scan agent, FULL_SCAN_TASK rendering, preview
- internal/llmloop/ new package: shared LLM tool-use loop, three-zone
memory compression, CommentWorkerPool, AgentWarning.
Both internal/agent and internal/scan delegate to
llmloop.Runner; agent and scan never import each other
- internal/agent/ slimmed: LLM loop / compression / token aggregation
moved to llmloop; review-only orchestration remains
- internal/model/ new ScanItem (full-file payload) + Preview /
PreviewEntry / ExcludeReason shared by both modes
- internal/diff/ new gitignore.go exporting helpers reused by scan
- cmd/opencodereview/ new scan_cmd.go; shared.go consolidates startup
(loadCommonContext / loadLLMRuntime), output
(emitRunResult, ResultProvider) and stdout silencing
(quietHandle); review_cmd.go follows the same shape
Template additions:
- FULL_SCAN_TASK: dedicated prompt with Tool-call discipline guidance to
reduce gratuitous tool calls per file
- FULL_SCAN_MAX_TOOL_REQUEST_TIMES (default 60): scan-only per-file budget,
raised over diff's 30 to fit multi-finding files; --max-tools still
composes (only raise, never lower)
In scan mode, file_read_diff is filtered out of MainToolDefs since it has
no useful semantics without a diff.
Tests cover provider enumeration (with temp git repo), template rendering,
filter passes, dependency budget, flag validation, and excludeToolDef.
* feat(scan): v2 — exclude / non-git / split template / plan / batch / dedup / project-summary
Address design-review feedback by evolving `ocr scan` along seven axes
while keeping `ocr review` behavior unchanged:
1. File size cap is now configurable (ScanTemplate.MaxFileSizeBytes,
default 2 MiB; previously a hard-coded 5 MiB). The cap exists only to
bound memory reading; the real review-feasibility gate is the per-file
token budget downstream.
2. Drop the `--all` flag. Bare `ocr scan` now scans the whole repo;
`--path` narrows. Less ceremony, fewer redundant flags.
3. New `--exclude` flag on both review and scan. Comma-separated
gitignore-style patterns; merged with rule.json's exclude layer via
the new shared.applyCLIExcludes helper.
4. Scan supports non-git directories. internal/scan.Provider chooses
between `git ls-files` (full .gitignore semantics) and a
filepath.WalkDir fallback (root .gitignore + ExcludedDirs blocklist)
per isGitRepo probe. loadCommonContext takes a requireGit bool; review
keeps the hard requirement, scan relaxes it.
5. Scan configuration lives in its own file. internal/config/template:
- new ScanTemplate type with LoadScanDefault/ApplyLanguage/Validate
- new embedded scan_template.json
- Template loses the FULL_SCAN_* fields (review template unaffected)
scan.Agent.Args.Template now holds a ScanTemplate; toLoopTemplate
adapts it for llmloop.Runner.
6. New scan phases — each nil-able in the template and toggleable via a
CLI flag, so users can revert to v1 behavior trivially:
* PLAN_TASK (--no-plan): per-file pre-pass that outputs a JSON
summary + checkpoints, embedded into MAIN_TASK as {{plan_guidance}}.
formatPlanGuidance renders to markdown; malformed JSON falls back
to raw text. PLAN_TASK failure never blocks the main loop.
* BATCH_STRATEGY (--batch): files are grouped before dispatch.
"none" preserves v1, "by-language" (default) groups by extension,
"by-directory" groups by first-level subdir. BatchSize caps natural
groups so a single language with 500 files doesn't form one giant
batch. Batches are processed sequentially; files within a batch
remain concurrent up to MaxConcurrency.
* DEDUP_TASK (--no-dedup): per-batch postprocess that asks the LLM
to cluster near-duplicate comments. Output is a `groups` JSON;
every input id must appear exactly once or the result is rejected
and originals are kept (safety: never silently lose comments).
CommentCollector grows Snapshot/Since/ReplaceSince for this.
* PROJECT_SUMMARY_TASK (--no-summary): once-per-run cross-file
summary appended to text output and surfaced as `project_summary`
in JSON output. ResultProvider grows ProjectSummary(); agent.Agent
returns "" (review mode has no project summary).
All four new LLM steps record token usage via runner.RecordUsage so
aggregate counters stay accurate.
7. Tests cover the new pure code paths:
- batch_test.go: 3 strategies, BatchSize cap, language-key edge cases
- dedup_test.go: groups parser, malformed shapes, fence stripping,
payload field selection
- agent_test.go: formatPlanGuidance variants, buildSummaryCommentsList
truncation, maybeRunPlan skip paths
- provider_test.go: non-git directory walker fallback
- template_test.go: ScanTemplate loads / ApplyLanguage / review
template no longer contains scan fields
The seven phases can be reverted independently by toggling flags or
clearing the corresponding optional template fields; nothing forces the
new behavior on existing review users.
* fix(scan): three real bugs surfaced by SCAN_PLAN_TASK self-review
A v2 end-to-end test (ocr scan --path internal/scan/preview.go) had the
PLAN_TASK phase flag three concrete bugs in the scan package itself.
This commit fixes them and adds regression tests.
1. Preview() mutated a.items as a side-effect.
Both Preview and Run wrote to a.items. Calling Preview before Run
silently primed Run with the preview's enumeration instead of
triggering a fresh listFiles. Preview is documented as a read-only
dry-run; uphold that. Local variable now; a.items stays nil after
Preview returns.
2. Preview.result.Entries was nil when there were no items.
With no items at all the loop never ran, so Entries remained nil and
JSON marshalling produced "files":null. Pre-allocate to a non-nil
empty slice so the JSON contract stays "files":[] regardless.
3. Provider.Enumerate and listFilesViaWalk never checked ctx.Done().
On a large repo a cancelled context would still complete the full
walk before the caller saw an error (every iteration costs a stat or
ReadFile syscall). Add the check at the top of each iteration in
both the git-ls-files path and the walker fallback path; the walker
returns ctx.Err() so filepath.WalkDir propagates the cancellation.
Three new regression tests pin the contracts:
- TestPreview_DoesNotMutateAgentItems
- TestPreview_EmptyResultEntriesIsNonNilSlice
- TestProvider_Enumerate_RespectsContextCancellation
* feat(scan): cost estimate + token budget cap; fix file_find on non-git dirs
Two cost-control features and one robustness fix, all surfaced by running
the scanner against a real ~870K-token repository.
Cost estimate (internal/scan/estimate.go):
- Before dispatch, Run prints an order-of-magnitude projection of token
usage (input/output/total), derived from per-file content size × an
assumed round count, plus the optional plan/dedup/summary phases.
- Deliberately reports tokens only, not dollars — pricing varies per
provider/model and a precise figure would mislead. Actual usage is still
reported from the API after the run.
Token budget cap (--max-tokens-budget / ScanTemplate.MaxTokensBudget):
- Caps total token usage for one scan. The gate is checked per file inside
dispatchBatch, right before acquiring a concurrency slot: if tokens
already spent plus a look-ahead estimate of the next file would exceed
the budget, that file and all remaining files are skipped and a
token_budget_reached warning is recorded.
- An earlier batch-level gate was too coarse: with the default by-language
batching, a Go-heavy repo puts most files in one batch, so the gate only
fired between batches and overran the budget ~2.4×. The per-file gate
bounds overrun to roughly one in-flight file per worker (~1.3× at
concurrency=1 in testing).
- 0 = unlimited (unchanged default behavior).
Phase-gate helpers (planEnabled/dedupEnabled/summaryEnabled) consolidate
the "template defines it AND --no-* flag not set" checks so the cost
estimate and the dispatch path agree on which phases will actually run.
file_find non-git fallback (internal/tool/file_find.go):
- `git ls-files` exits 128 in a non-git directory, which spammed failures
when scanning plain directories (scan already supports non-git repos via
the provider's walker, but the file_find tool did not). Now falls back
to filepath.WalkDir honoring the root .gitignore and the default
excluded-dir blocklist when git fails and no specific ref is requested.
Tests:
- estimate_test.go: humanTokens formatting, per-file vs aggregate estimate
consistency, phase scaling, phase-gate tri-state.
- budget_test.go: fake LLM client drives the gate deterministically —
verifies dispatch stops before exceeding budget and that 0 = unlimited.
- file_find_test.go: non-git directory fallback finds files, honors
.gitignore / blocklist, and returns the not-found sentinel correctly.
* docs(readme): document ocr scan subcommand and flags
ocr scan existed but was undiscoverable from the README. Add it to the
intro blurb, Quick Start, the Commands table, Examples, and a dedicated
flags table (path / exclude / preview / max-tokens-budget / no-plan /
no-dedup / no-summary / batch / format / concurrency / rule / repo).
Note non-git support and the pre-run cost estimate. Also backfill the
--exclude flag in the ocr review flags table (added during the v1.3 merge
but never documented).
Flag names and defaults verified against `ocr scan -h`.
* fix(scan): code_search works in non-git directories via git grep --no-index
code_search relied on `git grep`, which exits 128 in a non-git directory —
so `ocr scan` on a plain directory (already supported by file enumeration and
file_find) silently returned errors instead of search results. Detect that
failure and retry with `git grep --no-index --exclude-standard`, which searches
the working tree directly while still honoring .gitignore. Reuses all existing
grep flag/parsing logic; ref-based search still requires a real repo.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(scan): preserve context on compression failure; fix NUL parsing in gitLs
Address three real bugs from the PR #93 automated review that regressed when
compression moved into internal/llmloop:
- Sync compression failure / empty summary now return the original messages
instead of truncating to the frozen zone, which discarded the whole
per-file conversation context.
- Async compression now abandons the job on error instead of applying a
truncated snapshot, and re-applies messages appended while it ran
(snapshotLen), so concurrent tool results are no longer lost.
- scan Provider.gitLs uses cmd.Output() instead of CombinedOutput() so
stderr can't corrupt the NUL-delimited (-z) filename parsing.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
When a file was renamed on the target branch, ocr review emitted
'[ocr] WARNING: cannot read file <old path> at ref <to>: exit status 128'.
Two compounding bugs:
1. The parser required 'a/'/'b/' prefixes when matching '--- /dev/null' /
'+++ /dev/null', but git emits these lines without prefixes, so
IsNew/IsDeleted were never set and deleted files fell through to a
doomed 'git show ref:<old path>'.
2. 'rename from' / 'rename to' extended headers were never parsed, and
git diff/show call sites did not force rename detection, so renames
degraded to delete+add whenever the user had diff.renames=false.
Fixes:
- Parse 'rename from'/'rename to', 'new file mode', 'deleted file mode'
and unprefixed /dev/null markers in ParseDiffText.
- Pass --find-renames to all git diff/show invocations so rename
detection no longer depends on user config.
- Add IsRenamed to model.Diff (json: is_renamed) and prefer it in
diffStatus.
- Add parser unit tests and a range-mode rename regression test.
Fixes#99
Add --src-prefix=a/ --dst-prefix=b/ to all git diff/show calls so that
user config (diff.noprefix, diff.mnemonicPrefix) cannot alter the prefix
format the parser depends on. Also add missing ModeCommit test coverage.
When a user has configured a global external diff tool (diff.external /
GIT_EXTERNAL_DIFF) or a textconv filter, git diff/show emit the tool's
output instead of unified diff text. The provider's parser keys off
`diff --git` headers, so it parses zero diffs and the review silently
reports "No files changed".
Add --no-ext-diff --no-textconv to all four git diff/show call sites in
internal/diff/git.go (ModeRange diff, ModeCommit show, and both
workspaceTrackedDiff calls). merge-base and ls-files are left untouched
since they don't run the diff machinery.
Adds an integration test that initializes a real git repo, activates a
garbage GIT_EXTERNAL_DIFF script, and asserts the provider still parses
a non-empty diff (fails before this change, passes after).
Closes#82
AI-assisted contribution.
Introduce gitcmd.Runner with channel-based semaphore to cap concurrent
git subprocesses (default 16, configurable via --max-git-procs). Route
all tool-layer and diff-layer git calls through the shared runner.
Also fix diff.Provider.runGit lacking context.Context — now the full
chain (GetDiff → MergeBase → ParseDiffText → finalizeDiff) propagates
the caller's context for proper cancellation and timeout support.
Migrated internal/llm from manual net/http implementation to anthropic-sdk-go v1.47.0
and openai-go/v3 v3.39.0, reducing ~400 lines of retry, request-building, and streaming
code. Simplified LLMClient interface to single CompletionsWithCtx method. Fixed Anthropic
auth to use WithAuthToken (Bearer) instead of WithAPIKey (X-Api-Key) for proxy compatibility.
Fixed OpenAI message builder to use ExtractText() for proper []ContentBlock handling.
StreamCompletion used http.NewRequest without context and retry with
context.Background(), making streaming calls uncancellable. Memory
compression called Completions() without context, leaving async
goroutines unable to be cancelled or timed out.
- Add StreamCompletionWithCtx to LLMClient interface and both
OpenAI/Anthropic implementations
- Propagate context through runCompression, addNextMessage, and
triggerAsyncCompression call chain
- Add 5-minute timeout cap on async compression goroutines
- Cancel in-flight compression HTTP requests when job is superseded
file_find and finalizeDiff always read from the working tree, even in
range/commit mode where the review targets a specific git ref. This
caused inconsistent file versions compared to file_read and code_search
which correctly used git show. Fix file_find to use git ls-tree and
finalizeDiff to use git show when a ref is specified.
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.
This change refactors the `resolveFromHunk` function by introducing several helper functions (`extractSideLines`, `matchConsecutive`) to improve readability and maintainability. The new implementation attempts to match against both new and old sides of the diff hunk, making comment resolution more robust.
