alibaba / alibaba/open-code-review

fix(preview): untracked provider-directory files are missing from workspace preview totals

Closed
#1,235 0 comments 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.