From 5b567e1f7962a0a9fd8a9b1c24d966f04db4856a Mon Sep 17 00:00:00 2001 From: kai CI Date: Wed, 9 Sep 2026 12:24:40 +0300 Subject: [PATCH] The coverage manifest counts files, not directories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "What I opened — 6 files" over a list beginning `frontend` and `frontend/dist` (kai-desktop#314). "4 files" over one containing `cmd/kai` (kai-cli#99). Two independent sightings within a day of the manifest shipping, so this is systematic, not incidental. A directory listing passes the same "path" argument a file read does, and rcFilesRead took any of them. The manifest exists so a reader can tell a clean verdict that read the whole change from one that read two files — a coverage report that miscounts its own coverage is the one thing it cannot afford. The filesystem is the authority, so ask it: the review runs inside the checkout, and the paths are right there. Only a CONFIRMED directory is dropped. A path stat cannot resolve — an absolute path from another tree, a bare name — is kept, because the cost is asymmetric. An extra entry slightly overstates what was read. A wrongly dropped one makes changedFilesNotListed accuse the review of skipping a file it actually opened, which is the same class of lie this whole line of work exists to remove, pointed the other way. TestFilesReadDropsDirectories fails on the parent with exactly the shape seen in production: rcFilesRead = [/elsewhere/vendored.go frontend frontend/dist frontend/dist/app.js], want [/elsewhere/vendored.go frontend/dist/app.js] Co-Authored-By: Claude Opus 5 --- cmd/kai/review_commit.go | 40 +++++++++++++++++++-- cmd/kai/review_commit_incomplete_test.go | 44 +++++++++++++++++++++++- 2 files changed, 80 insertions(+), 4 deletions(-) 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 + } + } +}