diff --git a/cmd/kai/review_commit.go b/cmd/kai/review_commit.go index 64e65ad..7f5c749 100644 --- a/cmd/kai/review_commit.go +++ b/cmd/kai/review_commit.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "sort" "strconv" "strings" @@ -760,7 +761,7 @@ func rcRunReviewAgent(ctx context.Context, set *projects.Set, prov provider.Prov FinishReason: string(res.FinishReason), Elapsed: time.Since(started), Turns: len(res.Transcript), - FilesRead: rcFilesRead(res.Transcript), + FilesRead: rcFilesRead(res.Transcript, primary.Path), } raw := strings.TrimSpace(res.FinalText) // A run that ran out of road — the soft time budget fired, or the loop @@ -836,11 +837,24 @@ func rcCoverageOf(inc *rcIncomplete) *rcCoverage { } } -// rcFilesRead pulls the distinct paths the run actually opened out of its tool +// rcFilesRead pulls the distinct FILES the run actually opened out of its tool // calls. Deliberately cheap and schema-loose, like rcChangedSymbols: any tool // that names a file names it in a "path" or "file_path" field, and a missed one // only shortens a list that is already a courtesy. -func rcFilesRead(transcript []message.Message) []string { +// +// Directories are dropped, and root is how. The same "path" argument that a +// file read uses is also what a directory listing passes, so the manifest +// published "What I opened — 6 files" over a list whose first two entries were +// `frontend` and `frontend/dist` (kai-desktop#314), and "4 files" over one +// containing `cmd/kai` (kai-cli#99). A coverage report that miscounts its own +// coverage is the one thing this feature cannot afford. +// +// Only a CONFIRMED directory is dropped. A path that does not resolve — an +// absolute path from another tree, a bare name, anything stat cannot answer — +// is kept, because the cost is asymmetric: an extra entry slightly overstates +// what was read, while a wrongly dropped one makes changedFilesNotListed +// accuse the review of skipping a file it actually opened. +func rcFilesRead(transcript []message.Message, root string) []string { seen := map[string]bool{} var out []string for _, m := range transcript { @@ -861,6 +875,9 @@ func rcFilesRead(transcript []message.Message) []string { continue } seen[p] = true + if rcIsDir(root, p) { + continue + } out = append(out, p) } } @@ -869,6 +886,23 @@ func rcFilesRead(transcript []message.Message) []string { return out } +// rcIsDir reports whether p names a directory in the reviewed checkout. +// +// Answers only what the filesystem confirms: a stat that fails, for any reason, +// is not a directory as far as this is concerned. See rcFilesRead for why the +// uncertain case keeps the entry rather than dropping it. +func rcIsDir(root, p string) bool { + full := p + if !filepath.IsAbs(full) { + if root == "" { + return false + } + full = filepath.Join(root, p) + } + fi, err := os.Stat(full) + return err == nil && fi.IsDir() +} + // rcIncompleteProse is the review of last resort: not a review at all, but an // honest account of a run that read code and ran out of road. It exists so the // finding still carries something a human can act on — how long it ran, why it diff --git a/cmd/kai/review_commit_incomplete_test.go b/cmd/kai/review_commit_incomplete_test.go index c82a598..4cfe2e2 100644 --- a/cmd/kai/review_commit_incomplete_test.go +++ b/cmd/kai/review_commit_incomplete_test.go @@ -2,6 +2,8 @@ package main import ( "encoding/json" + "os" + "path/filepath" "strings" "testing" "time" @@ -75,7 +77,9 @@ func TestFilesReadDedupesAcrossToolCallSpellings(t *testing.T) { message.ToolCall{ID: "5", Name: "kai_view", Input: `not json`}, }, }} - got := rcFilesRead(tr) + // Empty root: nothing resolves, so nothing is confirmed a directory and + // every named path is kept. Directory filtering has its own test below. + got := rcFilesRead(tr, "") if len(got) != 2 || got[0] != "a.go" || got[1] != "b.go" { t.Errorf("rcFilesRead = %v, want [a.go b.go] (deduped, sorted, non-file calls ignored)", got) } @@ -158,3 +162,41 @@ func TestCoverageShipsOnEveryGroundedRun(t *testing.T) { t.Error("rcCoverageOf(nil) invented a manifest for a pass that opened nothing") } } + +// "What I opened — 6 files" listed `frontend` and `frontend/dist` +// (kai-desktop#314); "4 files" listed `cmd/kai` (kai-cli#99). A directory +// listing passes the same "path" argument a file read does, so the manifest +// counted directories as files it had read. A coverage report that miscounts +// its own coverage is the one thing this feature cannot afford. +func TestFilesReadDropsDirectories(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "frontend", "dist"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "frontend", "dist", "app.js"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + tr := []message.Message{{ + Role: message.RoleAssistant, + Parts: []message.ContentPart{ + message.ToolCall{ID: "1", Name: "kai_grep", Input: `{"path":"frontend"}`}, + message.ToolCall{ID: "2", Name: "kai_grep", Input: `{"path":"frontend/dist"}`}, + message.ToolCall{ID: "3", Name: "kai_view", Input: `{"file_path":"frontend/dist/app.js"}`}, + // Never existed here at all: unresolvable, so it is kept. Dropping + // it would make changedFilesNotListed accuse the review of + // skipping a file it opened. + message.ToolCall{ID: "4", Name: "kai_view", Input: `{"file_path":"/elsewhere/vendored.go"}`}, + }, + }} + got := rcFilesRead(tr, root) + want := []string{"/elsewhere/vendored.go", "frontend/dist/app.js"} + if len(got) != len(want) { + t.Fatalf("rcFilesRead = %v, want %v — directories must not count as files", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("rcFilesRead = %v, want %v", got, want) + break + } + } +}