mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-09-14 19:59:52 +08:00
main
21 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 CowToTheSky
|
3bce7adf7e |
fix(preview): show provider-excluded diffs (#1223)
* fix(preview): show provider-excluded diffs * fix(preview): refine provider exclusions * feat: Apply suggestion from @lizhengfeng101 --------- Co-authored-by: Kite <254839944+lizhengfeng101@users.noreply.github.com> |
||
|
hyy321
|
4cecf1e763 |
fix(diff): propagate untracked file listing errors (#1140)
* fix(diff): propagate untracked file listing errors * test: Apply suggestion from @lizhengfeng101 --------- Co-authored-by: Kite <254839944+lizhengfeng101@users.noreply.github.com> |
||
|
ben7am1n
|
0c44f1049e |
fix(diff): surface git's own message when a diff command fails (#1039)
* fix(diff): surface git's own message when a diff command fails
GetDiff runs git through runGit, which captures stdout and stderr together,
then discarded that output on every failure path. A user whose `git show`
failed saw only:
Error: review failed: load diffs: get diffs: git show failed: exit status 129
The exit status alone cannot distinguish an unsupported option from a bad
revision or a permission problem, so diagnosing #972 meant asking the reporter
to re-run the command by hand to see what git actually said.
Quote git's output in the error for the three diff-producing paths (range,
commit, workspace-tracked). The same failure now reads:
... git show failed: exit status 129: error: unknown option `diff-merges=first-parent'
Output is capped, keeping the tail, because runGit's combined output means a
command that failed partway through carries a prefix of real diff along with
the diagnosis. The cap cuts on a rune boundary: git speaks the user's locale,
and #972 came from a Japanese-language Windows install, so a byte-wise cut
would replace a confusing error with an unreadable one.
The other runGit callers deliberately swallow errors and fall back, so they
are left alone.
* test(diff): pin which command speaks when both workspace diffs fail
Addresses review feedback on the two-stage fallback in workspaceTrackedDiff.
Reaching `git diff --staged` means `git diff HEAD` already failed, and in
the case the fallback exists for -- a repository with no commits -- it failed
with "bad revision 'HEAD'", which is expected rather than diagnostic.
Surfacing both would put that benign message ahead of the one describing what
actually blocked the review, so the behavior is deliberate and now has a test
saying so.
* fix(diff): quote stderr, not combined output, when git fails
Review feedback: a `git show` killed mid-write contributed a 2036-byte tail
made entirely of diff content, with no diagnosis anywhere in it. SIGKILL
leaves stderr empty, so keeping the tail kept repository source -- and
whatever that source contains.
That string does not stay local. reviewResultError hands it to
span.RecordError (review_cmd.go:259), and signal.NotifyContext (:99) puts
Ctrl-C on the path that reaches it, so the leak had a route to whatever
telemetry backend is configured. classifyItemError guards the run manifest
against raw error text for the same reason.
Add runGitSplit and give the three diff-producing callers stderr alone. Git
writes its diagnosis to stderr by construction, since die() writes there, so
this loses nothing a reader wants and cannot carry diff. Mirrors runGitGrep in
internal/tool/code_search.go, cancellation guard included: a signalled process
reports the signal rather than the reason, so a cancelled run now says
"context deadline exceeded" instead of "signal: killed" -- which also lets
classifyItemError reach its timeout class instead of the generic provider one.
The three failure modes this PR targets write to stderr, so their messages are
byte-identical and #972 still gets its diagnosis.
workspaceTrackedDiff returns stderr separately rather than overloading its
first return value, which also retires the dual-meaning the earlier review
flagged.
Also fix the regression test asserting "fatal:"/"error:", which git
translates: under zh_CN the line opens with a translated prefix, so it passed
only because CI runs in English. Anchor on the object name instead, the one
part no locale rewrites -- the same pairing isNotGitRepoError uses.
* docs(diff): correct the reasons recorded around gitFailure
Two comments described mechanisms that are not there.
The cancellation assertion in TestGetDiff_CancelledMidWriteReportsCancellation
credited classifyItemError with reading the error's type. It never sees it:
GetDiff is reached through loadDiffs, whose failure is recorded at agent.go:284
as a fixed SetRunFailure(RunFailureInput, "failed to resolve review input")
without inspecting err. classifyItemError has a single call site, agent.go:712,
for per-item subtask errors. Neither reviewResultError nor main.go branches on
the type either, so nothing downstream observes it today.
What the assertion actually holds is runGitSplit's cancellation guard: the leak
assertion above it passes either way, since quoting stderr alone keeps stdout
out of the error, so without this second assertion the guard could be removed
silently. Verified by dropping the guard -- only this assertion fails.
gitDiagLimit and gitFailure still justified themselves by runGit's combined
output, which no caller passes anymore. The ceiling and the keep-the-tail rule
both survive on stderr, but for a different reason: die() exits the process, so
the fatal is last, behind any warnings. Parameter renamed out -> stderr to match
what the three call sites pass.
No behavior change. make test, go vet, gofmt, english-only and coverage (94.0%)
all pass.
---------
Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>
|
||
|
Ziyang Guo
|
5b37b5f8e2 |
fix(diff): match gitignore directory patterns (#853)
Match directory-only patterns against ancestor paths with glob and root-anchor semantics aligned to Git. Add regression coverage for nested, globstar, component-glob, and anchored patterns. Test: make test |
||
|
kite
|
533b526b4c |
chore: add SPDX license headers and automated verification (#740)
* 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.
|
||
|
Gongyl01
|
0ce730a3c8 |
feat(manifest): run manifest coverage contract for review (#367) (#520)
* 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> |
||
|
Luis Rodriguez
|
6ab7e0f206 |
fix(diff): honor .gitignore negation patterns (#651)
* 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>
|
||
|
kite
|
0035d124bc |
fix: align Go module path with actual GitHub repository (#526)
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. |
||
|
Qiaochu Hu
|
3fe30274e9 |
fix(diff): review merge commits against their first parent (#450)
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.
|
||
|
chethanuk
|
5c6280c8c9 |
fix(diff): add --end-of-options guard and no-commit regression test for workspace diff (#376)
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 |
||
|
fuhui
|
43524cb157 | refactor(diff): normalize workspace git argument ordering (#381) | ||
|
fuhui
|
0ec3769d58 |
fix(diff): preserve non-ASCII paths (#365)
* 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. |
||
|
css521
|
18797f8c05 |
feat: add ocr scan for full-file code review (#93)
* 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>
|
||
|
MuoDoo
|
4b3f41df10 |
Fix workspace symlink diff containment (#125)
* Fix workspace symlink diff containment * Share repository path containment helpers |
||
|
ZheNing Hu
|
283ec8558c |
fix(diff): detect renamed and deleted files correctly in diff parsing (#105)
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 |
||
|
MuoDoo
|
64552aee9b | fix(security): block git ref option injection (#112) | ||
|
kite
|
cf32900ca1 |
fix: force standard diff prefixes to prevent diff.noprefix/mnemonicPrefix from breaking parsing (#82)
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. |
||
|
Eldar Shlomi
|
c9fae8d4e9 |
fix: pass --no-ext-diff --no-textconv to git diff/show so external diff tools don't break parsing (#86)
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. |
||
|
kite
|
ef46dfdac9 |
feat(tool): add global git subprocess concurrency limiter and propagate context.Context to diff layer
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. |
||
|
kite
|
99e6709603 |
fix(diff,tool): read file content at reviewed ref in range/commit mode
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. |
||
|
kite
|
7c8b8562aa | feat: init |