From 72e2aaeb6bf8b691c4310547b31b95089c94d3ad Mon Sep 17 00:00:00 2001 From: kai CI Date: Wed, 9 Sep 2026 14:53:43 +0300 Subject: [PATCH] Read the moved dependency instead of guessing at it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit taught the reviewer to say "I could not read this" once instead of speculating five times. This is the part that makes that sentence unnecessary when the source can actually be had. TUI#92 bumped kai-engine to v0.6.59-0.20260908192613-5ab1b102fc2f and moved three call sites onto kaipath.UserPath, then spent most of its output asking whether that function is variadic, whether it accepts zero trailing components, and whether an empty override preserves the default — and called the arity question "a real defect". The answer is 491 bytes: func UserPath(home string, parts ...string) string { base := os.Getenv("KAI_DATA_DIR") if base == "" { base = filepath.Join(home, ".kai") } return filepath.Join(append([]string{base}, parts...)...) } Variadic; empty parts is fine; an empty override falls back. Every question answered, in the file whose commit the diff itself carried. ## The mechanism The pod has no Go toolchain and no module cache, so there is nothing on disk to read. But it does have GITHUB_TOKEN — an installation token for an app installed across the org, minted with no repository restriction — and the contents API serves a directory at an exact commit. That is the whole trick: the pinned commit out of the pseudo-version, the packages the diff imports, one API call each. Verified live against the real TUI#92 inputs: two files, 2,196 bytes, zero unresolved. ## What it will not do Only packages the diff actually imports, only github.com, only the plain three-segment module form, and only with a commit in hand — a version tag may not exist on the default branch, and reading the wrong revision of a contract is worse than reading none. Test files are skipped: the question is what the contract IS. Hard caps, because turn 0 waits on this: 10s for everything, 3 packages, 6 files, 48KB. Preflight exists to buy turns, not sell them. A dependency that cannot be read inside that budget is one the reviewer states as a limit, which is exactly what the previous commit built. ## One result, two blocks rcFetchDepSources returns what it read AND what it could not, and the caller renders both. A block telling the reviewer it cannot read a module, printed beside one containing that module's source, would be its own contradiction — so whatever is fetched is removed from what the limitation block claims. TestSourceAndLimitationBlocksDoNotOverlap holds that. Co-Authored-By: Claude Opus 5 --- cmd/kai/review_commit.go | 16 +- cmd/kai/review_commit_depsrc.go | 237 +++++++++++++++++++++++++++ cmd/kai/review_commit_depsrc_test.go | 80 +++++++++ 3 files changed, 329 insertions(+), 4 deletions(-) create mode 100644 cmd/kai/review_commit_depsrc.go create mode 100644 cmd/kai/review_commit_depsrc_test.go diff --git a/cmd/kai/review_commit.go b/cmd/kai/review_commit.go index 795fd33..2490d45 100644 --- a/cmd/kai/review_commit.go +++ b/cmd/kai/review_commit.go @@ -673,10 +673,18 @@ func rcRunReviewAgent(ctx context.Context, set *projects.Set, prov provider.Prov user.WriteString(lookups) user.WriteString("\n") } - // What the reviewer will NOT be able to read, named before it starts - // guessing. Sits after the lookups because it is the same kind of fact — - // resolved from the diff, ahead of turn 0 — pointed the other way. - user.WriteString(rcDepLimitsBlock(rcChangedDeps(diff))) + // The contracts this diff rests on that live in OTHER modules. Fetched at + // the pinned commit where that is possible, named as a limitation where it + // is not — both rendered from one result, so a block saying "you cannot + // read these" can never sit beside one containing the source. + if deps := rcChangedDeps(diff); len(deps) > 0 { + phase := time.Now() + src, unresolved := rcFetchDepSources(ctx, deps) + user.WriteString(rcDepSourceBlock(src)) + user.WriteString(rcDepLimitsBlock(unresolved)) + fmt.Fprintf(os.Stderr, " dependencies: %d file(s) fetched, %d module(s) unread (%s)\n", + len(src), len(unresolved), time.Since(phase).Round(time.Millisecond)) + } user.WriteString("INTENT:\n") user.WriteString(strings.TrimSpace(intent)) user.WriteString("\n\nDIFF:\n") diff --git a/cmd/kai/review_commit_depsrc.go b/cmd/kai/review_commit_depsrc.go new file mode 100644 index 0000000..9c34186 --- /dev/null +++ b/cmd/kai/review_commit_depsrc.go @@ -0,0 +1,237 @@ +package main + +// review_commit_depsrc.go — reading the contract instead of guessing at it. +// +// review_commit_deps.go names the modules a diff moves and tells the reviewer +// that an unread contract is a limitation, not a defect. That is the honest +// floor. This is the part that makes the floor unnecessary when it can. +// +// TUI#92 bumped kai-engine to v0.6.59-0.20260908192613-5ab1b102fc2f and moved +// three call sites onto kaipath.UserPath. The review then spent most of its +// output asking whether that function is variadic, whether it accepts zero +// trailing components, and whether an empty override preserves the default — +// and called the arity question "a real defect". The answer was 17 lines long, +// and the commit holding it was written in the diff the reviewer was handed. +// +// So fetch it. The pod has no Go toolchain and no module cache (see +// review_commit_deps.go), but it does have GITHUB_TOKEN — an installation +// token for an app installed across the org — and the GitHub contents API +// serves a directory at an exact commit. That is the whole mechanism. +// +// WHAT IT WILL NOT DO. It fetches only the packages the diff actually imports, +// only from github.com, only within hard byte and file caps, and only inside +// one short deadline. A review that spends ninety seconds pulling a large +// dependency has taken that time from reading the change itself, and the point +// of preflight is to buy turns, not sell them. Everything it does not fetch — +// too big, no token, third-party, a 404 — falls back to the limitation block, +// which is why the two are rendered from the same result. + +import ( + "context" + "encoding/base64" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "path" + "sort" + "strings" + "time" +) + +const ( + // rcDepFetchBudget bounds the whole preflight, every request together. + // Turn 0 waits on this, so it is deliberately short: a dependency that + // cannot be read in ten seconds is one the reviewer states as a limit. + rcDepFetchBudget = 10 * time.Second + // rcDepMaxFiles and rcDepMaxBytes bound what lands in the prompt. The + // contract a diff rests on is usually one small file; a package that + // needs more than this is not something to inline into every turn. + rcDepMaxFiles = 6 + rcDepMaxBytes = 48 << 10 + // rcDepMaxPkgFetch bounds how many packages are fetched per review. + rcDepMaxPkgFetch = 3 +) + +// rcDepSource is one file read out of a moved dependency, at the exact commit +// the diff pins. +type rcDepSource struct { + Module string + Pkg string + Path string // path within the repo, e.g. kaipath/user.go + Body string +} + +// rcGitHubRepo maps a module path to an owner/repo pair. +// +// Only the plain three-segment github.com form. A module with a major-version +// suffix, a module living in a subdirectory of its repo, or anything not on +// github.com is left alone: guessing a repo from a module path is how you +// fetch the wrong file and state it with confidence, which is worse than +// fetching nothing. +func rcGitHubRepo(module string) (owner, repo string, ok bool) { + parts := strings.Split(module, "/") + if len(parts) != 3 || parts[0] != "github.com" { + return "", "", false + } + if parts[1] == "" || parts[2] == "" { + return "", "", false + } + return parts[1], parts[2], true +} + +// rcGHContents fetches one contents-API path at a ref. Returns the raw JSON so +// the caller can decode either shape the endpoint serves — an object for a +// file, an array for a directory. +func rcGHContents(ctx context.Context, token, owner, repo, p, ref string) ([]byte, error) { + url := fmt.Sprintf("https://api.github.com/repos/%s/%s/contents/%s?ref=%s", owner, repo, p, ref) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, err + } + req.Header.Set("Accept", "application/vnd.github+json") + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + // 404 covers both "no such path" and "this token cannot see this + // repo", and the caller treats them the same: it did not read it. + return nil, fmt.Errorf("contents %s/%s/%s@%s: %s", owner, repo, p, ref, resp.Status) + } + return io.ReadAll(http.MaxBytesReader(nil, resp.Body, rcDepMaxBytes*4)) +} + +// ghContentEntry is the subset of a contents-API entry this needs. +type ghContentEntry struct { + Name string `json:"name"` + Path string `json:"path"` + Type string `json:"type"` + Size int `json:"size"` + Content string `json:"content"` + Encoding string `json:"encoding"` +} + +// rcFetchPkg reads the Go source of one package directory at a commit. +// +// Test files are skipped: they are usually the largest thing in a package and +// the reviewer is asking what the contract IS, not how it is exercised. +func rcFetchPkg(ctx context.Context, token, owner, repo, pkg, ref string, budget *int) []rcDepSource { + raw, err := rcGHContents(ctx, token, owner, repo, pkg, ref) + if err != nil { + return nil + } + var entries []ghContentEntry + if json.Unmarshal(raw, &entries) != nil { + return nil + } + sort.Slice(entries, func(i, j int) bool { return entries[i].Size < entries[j].Size }) + + var out []rcDepSource + for _, e := range entries { + if len(out) >= rcDepMaxFiles || *budget <= 0 { + break + } + if e.Type != "file" || !strings.HasSuffix(e.Name, ".go") || strings.HasSuffix(e.Name, "_test.go") { + continue + } + if e.Size <= 0 || e.Size > *budget { + continue + } + body := e.Content + if body == "" { + one, err := rcGHContents(ctx, token, owner, repo, e.Path, ref) + if err != nil { + continue + } + var f ghContentEntry + if json.Unmarshal(one, &f) != nil { + continue + } + body, e.Encoding = f.Content, f.Encoding + } + if e.Encoding == "base64" { + dec, err := base64.StdEncoding.DecodeString(strings.ReplaceAll(body, "\n", "")) + if err != nil { + continue + } + body = string(dec) + } + if body == "" { + continue + } + *budget -= len(body) + out = append(out, rcDepSource{Pkg: pkg, Path: e.Path, Body: body}) + } + return out +} + +// rcFetchDepSources resolves what it can of the moved dependencies and reports +// which ones it could not. +// +// The two returns are rendered together on purpose. A block saying "you cannot +// read these" beside one containing the source would be its own contradiction, +// so whatever is fetched here is removed from what the limitation block claims. +func rcFetchDepSources(ctx context.Context, deps []rcDepChange) (got []rcDepSource, unresolved []rcDepChange) { + token := strings.TrimSpace(os.Getenv("GITHUB_TOKEN")) + ctx, cancel := context.WithTimeout(ctx, rcDepFetchBudget) + defer cancel() + + budget := rcDepMaxBytes + fetched := 0 + for _, d := range deps { + owner, repo, ok := rcGitHubRepo(d.Module) + // Without a commit there is no exact source to ask for: a version tag + // may not exist on the default branch, and reading the wrong revision + // of a contract is worse than reading none. + if !ok || d.Commit == "" || len(d.Pkgs) == 0 || token == "" { + unresolved = append(unresolved, d) + continue + } + var forDep []rcDepSource + for _, pkg := range d.Pkgs { + if fetched >= rcDepMaxPkgFetch || budget <= 0 { + break + } + src := rcFetchPkg(ctx, token, owner, repo, pkg, d.Commit, &budget) + if len(src) == 0 { + continue + } + fetched++ + for i := range src { + src[i].Module = d.Module + } + forDep = append(forDep, src...) + } + if len(forDep) == 0 { + unresolved = append(unresolved, d) + continue + } + got = append(got, forDep...) + } + return got, unresolved +} + +// rcDepSourceBlock renders the fetched source into the prompt, in the same +// voice as the identifier lookups: an answer already obtained, not a place to +// go looking. +func rcDepSourceBlock(got []rcDepSource) string { + if len(got) == 0 { + return "" + } + var b strings.Builder + b.WriteString("SOURCE FROM THE DEPENDENCIES THIS DIFF MOVES (fetched at the pinned commit before\n") + b.WriteString("this review started — treat as already read; do not go looking for it, and do not\n") + b.WriteString("say you could not verify these contracts, because they are printed here):\n\n") + for _, s := range got { + fmt.Fprintf(&b, "--- %s/%s (package %s) ---\n", s.Module, path.Base(s.Path), s.Pkg) + b.WriteString(strings.TrimRight(s.Body, "\n")) + b.WriteString("\n\n") + } + return b.String() +} diff --git a/cmd/kai/review_commit_depsrc_test.go b/cmd/kai/review_commit_depsrc_test.go new file mode 100644 index 0000000..e95725c --- /dev/null +++ b/cmd/kai/review_commit_depsrc_test.go @@ -0,0 +1,80 @@ +package main + +import ( + "context" + "strings" + "testing" +) + +// Guessing a repo from a module path is how you fetch the wrong file and then +// state it with confidence, which is worse than fetching nothing. +func TestGitHubRepoOnlyAcceptsThePlainForm(t *testing.T) { + owner, repo, ok := rcGitHubRepo("github.com/kaicontext/kai-engine") + if !ok || owner != "kaicontext" || repo != "kai-engine" { + t.Errorf("= %q/%q ok=%v, want kaicontext/kai-engine", owner, repo, ok) + } + for _, m := range []string{ + "github.com/kaicontext/kai-engine/v2", // major-version suffix + "github.com/kaicontext", // not a repo + "golang.org/x/tools", // not github + "gopkg.in/yaml.v3", + "", + } { + if _, _, ok := rcGitHubRepo(m); ok { + t.Errorf("rcGitHubRepo(%q) accepted, want refused", m) + } + } +} + +// Without a token, a commit, an importable package, or a github.com module +// there is nothing to ask for — and the dependency has to come back as +// unresolved so the limitation block still covers it. Nothing here touches +// the network. +func TestFetchLeavesWhatItCannotAskForUnresolved(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "") + deps := []rcDepChange{ + {Module: "github.com/kaicontext/kai-engine", To: "v0.6.59-0.20260908192613-5ab1b102fc2f", Commit: "5ab1b102fc2f", Pkgs: []string{"kaipath"}}, + {Module: "golang.org/x/tools", To: "v0.1.0", Commit: "aaaaaaaaaaaa", Pkgs: []string{"go/packages"}}, + {Module: "github.com/kaicontext/kai-engine", To: "v0.6.58"}, // no commit, no pkgs + } + got, unresolved := rcFetchDepSources(context.Background(), deps) + if len(got) != 0 { + t.Errorf("fetched %d files with no token, want 0", len(got)) + } + if len(unresolved) != len(deps) { + t.Errorf("unresolved = %d, want all %d — the limitation block must still cover them", len(unresolved), len(deps)) + } +} + +// The source block and the limitation block are rendered from one result. A +// block telling the reviewer it cannot read a module, printed beside one +// containing that module's source, would be its own contradiction. +func TestSourceAndLimitationBlocksDoNotOverlap(t *testing.T) { + src := []rcDepSource{{ + Module: "github.com/kaicontext/kai-engine", + Pkg: "kaipath", + Path: "kaipath/user.go", + Body: "package kaipath\n\nfunc UserPath(home string, parts ...string) string { return \"\" }\n", + }} + unresolved := []rcDepChange{{Module: "golang.org/x/tools", From: "v0.1.0", To: "v0.2.0"}} + + source := rcDepSourceBlock(src) + for _, want := range []string{"user.go", "package kaipath", "func UserPath", "do not go looking for it"} { + if !strings.Contains(source, want) { + t.Errorf("source block missing %q:\n%s", want, source) + } + } + // The fetched module must not also be announced as unreadable. + limits := rcDepLimitsBlock(unresolved) + if strings.Contains(limits, "kai-engine") { + t.Errorf("limitation block claims a module whose source was fetched:\n%s", limits) + } + if !strings.Contains(limits, "golang.org/x/tools") { + t.Errorf("limitation block dropped the module that really was unread:\n%s", limits) + } + + // Nothing fetched: no source block at all, rather than an empty heading. + if got := rcDepSourceBlock(nil); got != "" { + t.Errorf("empty source block = %q, want nothing", got) + } +}