alibaba / alibaba/open-code-review
fix(preview): untracked provider-directory files are missing from workspace preview totals
- Dominant language
- Go
- Stars
- 24.4k
- Forks
- 1.8k
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 105
Description
## Description
#1223 made `--preview` list provider-directory exclusions (`vendor/`, `node_modules/`, `target/`, ...) so that preview totals account for the whole changeset instead of silently dropping those files. It fixes the tracked path, but **workspace mode still drops untracked files under those directories before any diff is built**, so they never reach `partitionDiffs` and never land in `DiffSet.Excluded`.
In `internal/diff/git.go`, `untrackedFilesList` filters with `isPathExcluded`, which combines two independent rules — the provider-directory blocklist and `.gitignore` matching:
```go
if !p.isPathExcluded(line, patterns) { // isProviderDirExcluded(line) || gitignore match
files = append(files, line)
}
```
Only the provider-directory half should be deferred to `partitionDiffs`; the `.gitignore` half must keep dropping files. (That half is nearly redundant here anyway, since `git ls-files --others --exclude-standard` has already dropped ignored files.)
Because of this, the comment added in #1223 to `internal/agent/preview.go` promises more than the code delivers:
> Preview lists them separately to make its file and line totals match the Git changeset without implying that include rules can make them reviewable.
That holds for tracked changes only.
## Reproduction
This repository gitignores `vendor/`, so `vendor/` cannot demonstrate the behaviour. `target/` is in `providerDirIgnoreDirs` and is not gitignored, so use that. Avoid path segments such as `dist/` or `node_modules`, which this repository's `.gitignore` matches at any depth.
Untracked — the file is silently dropped:
```console
$ mkdir -p target && printf 'package main\n\nfunc demo() {}\n' > target/demo.go
$ git status --short target/
?? target/
$ ocr review --preview
Preview: 1 file(s) changed | +2 -0
Excluded from review (1):
[M] internal/diff/git_test.go (default_path)
```
Staged — the same file is counted and reported:
```console
$ git add target/demo.go && git status --short target/
A target/demo.go
$ ocr review --preview
Preview: 2 file(s) changed | +5 -0
Excluded from review (2):
[A] target/demo.go (provider_directory)
[M] internal/diff/git_test.go (default_path)
```
Clean up with `git reset -- target/demo.go && rm -rf target/`.
(The `internal/diff/git_test.go` line is unrelated local noise. The relevant part is `target/demo.go` and the `1 -> 2` file / `+2 -> +5` line difference.)
## Possible fixes
The mechanical change is small, but there is a real cost to weigh, so this wants a maintainer decision.
1. **Defer the provider-directory check.** Split `isPathExcluded` into its two halves — #1223 already extracted `isProviderDirExcluded` — and have `untrackedFilesList` apply only the `.gitignore` half. The synthesized diffs then flow into `partitionDiffs`, which sorts them into `Excluded` with no further plumbing. Cost: `untrackedFileDiffs` reads every such file in full and builds a synthetic `+`-prefixed diff for it. In a repository where `node_modules/` is untracked and not gitignored that is tens of thousands of file reads, and this path runs for normal reviews too, not just `--preview` — which is exactly the cost the provider-level drop exists to avoid.
2. **Count lines without synthesizing a body.** Return the excluded paths separately, skip `combined`, and append lightweight `model.Diff{IsNew: true, NewPath: f, Insertions: }` records to `DiffSet.Excluded`. Still reads each file, but avoids building and retaining the diff text. Roughly 20 lines plus one merge point.
3. **Narrow the promise instead.** Reword the `preview.go` comment (and `Preview`'s doc comment) to say "tracked changes", and leave the untracked gap documented. Zero risk.
Option 3 may well be enough on its own. The scenario in #1197 — Rust or `go mod vendor` projects that commit their vendored tree — is *tracked*, and #1223 already covers it. The untracked gap needs a provider directory that is neither tracked nor gitignored, e.g. a fresh `npm install` before `.gitignore` is set up, which is a much narrower case than paying the read cost on every review.
## Scope
- File(s): `internal/diff/git.go` (`untrackedFilesList`, `isPathExcluded`), `internal/agent/preview.go` (comments)
- Also: `cmd/opencodereview/output_helpers_test.go`
## Acceptance Criteria
- [ ] Either untracked provider-directory files are counted in `total_files` / `total_insertions` and reported as `provider_directory`, or the "match the Git changeset" wording is narrowed to tracked changes
- [ ] `cmd/opencodereview/output_helpers_test.go` no longer pairs `vendor/lib.go` with `ExcludeDefaultPath` — after #1223 that combination cannot occur, and it should be `ExcludeProviderDirectory`
- [ ] Tests pass (`make test`)
- [ ] Code check passes (`make check`)
- [ ] Coverage threshold holds (`make coverage`)
A note for whoever writes the test: `vendor/` is gitignored in this repository, so use `target/` or a temporary repo. `initPreviewRepo` in `internal/agent/preview_run_test.go` is the existing harness.
## Context
Found while reviewing #1223 after it merged (3bce7ad). Related: #1197.
Worth a separate look: the FAQ wording added in #1223 — "never reviewable, even when `include`d" — is accurate for the current implementation, but it also closes the door on #1197's third request, a supported way to review a vendored tree that is genuinely part of the project. If that use case should stay open, the FAQ may want softer wording across all five locales.
---
*AI/LLM disclosure: the analysis and the reproduction above are my own. Claude Code (Claude Opus 5) was used to polish the English wording and to file this issue via `gh`.*
Contributor guide
Research direction
Start with internal/diff/git.go, especially untrackedFilesList and isPathExcluded, then trace how preview totals and DiffSet.Excluded are built in internal/agent/preview.go. Review cmd/opencodereview/output_helpers_test.go and the initPreviewRepo harness in internal/agent/preview_run_test.go; done means the chosen behavior is covered, make test, make check, and make coverage pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- cli, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100