Prevent make test from polluting ~/.opencodereview/sessions/ with
thousands of var-folders-* directories by redirecting test session
writes to ~/.opencodereview/test-sessions/.
Introduce a package-level sessionSubDir variable (default "sessions")
and an exported UseTestSessions() function that switches it to
"test-sessions". Each test package that creates sessions calls
UseTestSessions() from init() in an init_test.go file.
* fix: add configurable HTTP timeout for LLM API calls
The LLM HTTP client had no user-configurable request timeout. When the
LLM API becomes unresponsive (e.g., rate limiting, network issues), the
ocr process hangs indefinitely with 0% CPU, holding an ESTABLISHED TCP
connection that never completes.
Changes:
- Add Timeout field to ResolvedEndpoint struct
- Add OCR_LLM_TIMEOUT environment variable (value in seconds)
- Add timeout_sec field to config.json llm and provider sections
- Pass timeout from ResolvedEndpoint to ClientConfig in NewLLMClient
The default timeout remains 5 minutes when not explicitly configured.
Users can now set a shorter timeout via:
- Environment: OCR_LLM_TIMEOUT=120
- Config file: {"llm": {"timeout_sec": 120}}
Fixes#237
* fix: address Copilot review feedback
- Make OCR_LLM_TIMEOUT a global override (works with any endpoint strategy)
- Add proper overflow detection for timeout values (check before multiply)
- Add validateTimeoutSec helper with negative/overflow guards
- Add comprehensive tests for timeout parsing, validation, and forwarding
- Reject negative timeout_sec in config files with explicit error
* fix: address review feedback for timeout configuration
- parseTimeoutEnv now returns errors instead of silent fallback for invalid values (negative, non-integer, overflow)
- Reuse validateTimeoutSec in parseTimeoutEnv to eliminate duplicated validation logic
- Remove redundant comment in tryOCREnv
- Add missing tests:
- TestResolveEndpoint_ProviderConfigTimeoutSec
- TestResolveEndpoint_ProviderConfigNegativeTimeoutSec
- TestResolveEndpoint_EnvTimeoutOverridesConfigTimeout
- TestResolveEndpoint_EnvTimeoutOverridesProviderTimeout
- TestResolveEndpoint_InvalidEnvTimeoutWithConfig
- TestResolveEndpoint_NegativeEnvTimeoutWithConfig
* feat: support inline content and file path resolution for rule field
- rule field auto-detects: .md/.txt/.markdown ending = file path, otherwise inline
- file paths: project-relative first, then as absolute path
- safety: stat before read (512KB cap), extension whitelist, symlink resolution
- tightened heuristic: values with spaces treated as inline to avoid false positives
- guard against empty repoDir to avoid CWD-relative resolution
- 5-language README docs updated with file path usage and first-match-wins behavior
- 15 new unit tests covering all resolution branches
Closes#67
Supersedes #87
* update readme
* feat: update rule resolution logic to clear rules for missing or invalid files
* feat: update rule field description to clarify file path detection criteria
* feat: enhance rule file resolution to block path traversal and improve validation
* fix: clean up tryReadRuleFile and add missing blank line
- Add blank line between matchProjectRuleEntry and allowedRuleExts (Issue 1)
- Remove dead code '|| repoDir == ' in tryReadRuleFile (Issue 2)
- Remove unnecessary warning when repoDir is empty but path is absolute (Issue 3)
Add comprehensive tests for events, metrics, provider, shutdown, span,
and exporter in the telemetry package. Update Makefile to exclude the
extensions directory from test, fmt, vet, and check targets.
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.
Add build provenance attestation to the release workflow using
actions/attest-build-provenance with OIDC keyless signing.
Document release signature verification in SECURITY.md.
git grep defaults to searching only tracked files, causing code_search
to miss untracked directories like internal/mcp/. Add --untracked flag
in workspace mode so both tracked and untracked files are searched.
Also fix non-git-directory detection to use exit code 128 instead of
hardcoded English error string, making it locale-independent.
Slow local models often cannot complete these sub-tasks within the
hardcoded 60-second limit, causing timeouts that the user-provided
--timeout flag cannot override. Raise both to 180s in task_template.json
and scan_template.json to give local models sufficient time.
* 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 tool_calls field to JSON output for tool usage statistics
Track per-tool invocation counts in llmloop.Runner and expose them
through the ResultProvider interface so both review and scan modes
report tool call statistics in --format json output.
* fix: correct tool_calls counting and ensure stable JSON schema
Move recordToolCall after lookupTool nil check so only actually-executed
tool calls are counted. Always emit tool_calls field in JSON output for
a consistent schema, initializing by_tool to empty map when nil.
* fix: remove omitempty from tool_calls to ensure stable JSON schema
Drop omitempty from the tool_calls struct tag and initialize ToolCalls
in outputJSONNoFiles so the field is always present in JSON output
regardless of execution path.
* 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>
* feat(llm): update provider model lists
Sync built-in model presets with latest vendor offerings.
* docs: update providers screenshot
Reflect the latest provider list in the config provider TUI.
PR #161 refactored matchProjectRule into matchProjectRuleEntry but
removed the empty-rule guard, causing entries with rule:"" to match
and return an empty string instead of falling through to the next
layer or system rule. This restores the skip-on-empty semantics while
honoring merge_system_rule:true with empty rule as "lock system rule".
The generic YAML rule only checked spelling in keys, which is insufficient
for CI/CD workflow files. Add layered rules with security, correctness,
reliability checks for .github/workflows/ and structure validation for
other .github/ config files (issue templates, release config).
Extract utility functions (stripEmptyPlanBlock, stripMarkdownFences,
buildMessageXML, copyMessages, countMessagesTokens, reviewModeString,
detectGitBranch) into util.go and all compression logic into compression.go
to improve readability (agent.go: 1489 → 1114 lines).
Fix pre-existing compression bugs from commit 14adf6f:
- Return unmodified messages on compression failure instead of truncating
to frozen zone (which discarded all conversation context)
- Check error in async compression goroutine to avoid applying failed results
- Track snapshot length to append post-snapshot messages when applying async
compression, preventing silent message loss
- Strip old <previous_review_summary> before appending new one to prevent
cumulative frozen zone bloat across multiple compressions
- Guard pendingJob cleanup to only clear when it matches the completed job,
preventing accidental clearing of a newly scheduled job
- Add context.Context and 5s timeout to detectGitBranch
- Deep copy ToolCalls in copyMessages to match session/history.go
- Reuse finalCount in addNextMessage to avoid redundant token counting
Move all LLM prompt content from inline JSON strings in task_template.json
to individual Markdown files under prompts/, following the same embed.FS
pattern used by internal/config/rules/. This improves maintainability by
enabling native syntax highlighting, cleaner diffs, and separation of
prompt content from runtime configuration.
Also removes two dead config fields (TOOL_REQUEST_WAIT_TIME_MS,
MAX_SUBTASK_EXECUTION_TIME_MINUTES) that had no runtime consumers.
Session directories were created with 0755 (rwxr-xr-x) and session
files with 0644 (rw-r--r--), making them readable by any local user
on shared systems such as multi-user servers and CI runners.
Session JSONL files contain full LLM request and response data,
including the complete source code diff, file contents read by the
file_read tool, and the model's analysis — all sensitive material.
Change MkdirAll mode from 0755 to 0700 and OpenFile mode from 0644
to 0600, restricting access to the file owner only. This matches the
0600 permission already used for the config file (which holds the API
key) in cmd/opencodereview/provider_cmd.go.
Fixes#144
Remove the duplicate kimi provider entry introduced by overlapping PRs
(#130 and #134). Keep the Kimi Moonshot API entry with MOONSHOT_API_KEY.
Set LC_ALL=C in Makefile test target to ensure git outputs English
messages regardless of system locale.
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
