diff --git a/cmd/kai/review_commit.go b/cmd/kai/review_commit.go index 64e65ad..6a645dc 100644 --- a/cmd/kai/review_commit.go +++ b/cmd/kai/review_commit.go @@ -672,6 +672,10 @@ 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))) user.WriteString("INTENT:\n") user.WriteString(strings.TrimSpace(intent)) user.WriteString("\n\nDIFF:\n") diff --git a/cmd/kai/review_commit_deps.go b/cmd/kai/review_commit_deps.go new file mode 100644 index 0000000..cc1b081 --- /dev/null +++ b/cmd/kai/review_commit_deps.go @@ -0,0 +1,232 @@ +package main + +// review_commit_deps.go — naming what the reviewer cannot read, before it +// guesses. +// +// review_commit_lookups.go answers the questions the reviewer would otherwise +// spend a turn on. This file does the opposite job for the questions it can +// never answer at all, because the answer is not in this checkout. +// +// CLI#95 and TUI#92 (2026-09-08) both moved kai-engine forward and switched +// four call sites onto kaipath.UserPath. Neither review could read that +// function — 17 lines, in another module — and both spent most of their output +// on it anyway: whether it is variadic, whether zero trailing components are +// allowed, whether an empty override preserves the default, whether the +// one-argument call is valid. TUI#92 escalated the arity question to "That's a +// real defect, not a...". Every one of those was speculation about a file +// nobody could open, presented as review. +// +// The reviewer was not being careless. It was told to ground every claim, and +// it was handed a diff whose correctness genuinely rests on a contract it had +// no way to see. What it lacked was permission to say so once and move on. +// +// NOTHING HERE FETCHES. The review pod is debian:bookworm-slim carrying two +// binaries — no Go toolchain, no module cache — and the workflow never runs +// `go mod download` (which for a private module would need GOPRIVATE and auth +// besides). So sibling module source is not merely unfetched, it is absent, +// and pretending otherwise would produce a second kind of wrong answer. What +// this does instead is tell the reviewer exactly which contracts are out of +// reach and that ONE scoped sentence is the whole correct response. +// +// The parsing is the same parsing a real fetch would need — module path, +// version, and the commit inside a pseudo-version — so this is the floor under +// that work, not a detour around it. + +import ( + "fmt" + "path" + "regexp" + "sort" + "strings" +) + +const ( + // rcMaxDepsShown bounds the block. A diff that moves thirty modules is a + // dependency bump, and listing all of them would bury the one or two the + // code actually reaches. + rcMaxDepsShown = 6 + // rcMaxPkgsPerDep bounds the packages named per module, same reasoning. + rcMaxPkgsPerDep = 5 +) + +// rcDepChange is one module whose version this diff moves. +type rcDepChange struct { + Module string // github.com/kaicontext/kai-engine + From string // "" when the diff ADDS the dependency + To string + Commit string // the 12-hex commit inside a pseudo-version, "" otherwise + Pkgs []string // packages under this module that the diff imports +} + +// rcPseudoCommit pulls the commit out of a Go pseudo-version. +// +// v0.6.59-0.20260908192613-5ab1b102fc2f names commit 5ab1b102fc2f. That is the +// exact source the reviewer would need, and it is sitting in the diff it was +// handed — which is what makes the speculation on TUI#92 so costly. It is +// reported so the limitation can name a commit rather than a version range, +// and so a later fetch has the identifier already parsed. +func rcPseudoCommit(version string) string { + parts := strings.Split(version, "-") + if len(parts) < 3 { + return "" + } + last := parts[len(parts)-1] + if len(last) != 12 { + return "" + } + for _, r := range last { + if !strings.ContainsRune("0123456789abcdef", r) { + return "" + } + } + return last +} + +// rcGoModRequire matches a require line in a go.mod diff hunk: optional +// "require ", a module path, a version, and whatever trailing comment. +var rcGoModRequire = regexp.MustCompile(`^[+-]\s*(?:require\s+)?([a-zA-Z0-9._~-]+\.[a-zA-Z0-9._~/-]+)\s+(v[0-9][^\s]*)`) + +// rcChangedDeps reads the diff's go.mod hunks and reports which modules move. +// +// Deliberately schema-loose, like rcChangedSymbols: it reads require lines and +// ignores everything else a go.mod can hold. A missed module only shortens a +// list, and a wrong one would only over-declare a limitation the reviewer +// already has. +func rcChangedDeps(diff string) []rcDepChange { + type versions struct{ from, to string } + moved := map[string]*versions{} + + inGoMod := false + for _, line := range strings.Split(diff, "\n") { + // Track which file the hunk belongs to. Only go.mod carries module + // versions; go.sum restates them twice each and would double every + // entry with nothing added. + if strings.HasPrefix(line, "+++ b/") || strings.HasPrefix(line, "--- a/") { + inGoMod = path.Base(strings.TrimSpace(line[6:])) == "go.mod" + continue + } + if strings.HasPrefix(line, "diff --git ") { + inGoMod = strings.HasSuffix(strings.TrimSpace(line), "go.mod") + continue + } + if !inGoMod || len(line) == 0 { + continue + } + if line[0] != '+' && line[0] != '-' { + continue + } + m := rcGoModRequire.FindStringSubmatch(line) + if m == nil { + continue + } + mod, ver := m[1], m[2] + if moved[mod] == nil { + moved[mod] = &versions{} + } + if line[0] == '+' { + moved[mod].to = ver + } else { + moved[mod].from = ver + } + } + + var out []rcDepChange + for mod, v := range moved { + // A module with no "+" line was REMOVED, and a removed dependency is + // not a contract this diff rests on. + if v.to == "" || v.to == v.from { + continue + } + out = append(out, rcDepChange{ + Module: mod, + From: v.from, + To: v.to, + Commit: rcPseudoCommit(v.to), + Pkgs: rcImportedPkgs(diff, mod), + }) + } + sort.Slice(out, func(i, j int) bool { + // Modules the diff actually imports first: those are the ones whose + // contract the change depends on, as opposed to a transitive bump. + if (len(out[i].Pkgs) > 0) != (len(out[j].Pkgs) > 0) { + return len(out[i].Pkgs) > 0 + } + return out[i].Module < out[j].Module + }) + if len(out) > rcMaxDepsShown { + out = out[:rcMaxDepsShown] + } + return out +} + +// rcImportedPkgs finds packages under mod that appear as import paths anywhere +// in the diff — added, removed or context lines alike, because an import the +// diff does not touch still names a contract the changed code below it uses. +func rcImportedPkgs(diff, mod string) []string { + seen := map[string]bool{} + var out []string + for _, line := range strings.Split(diff, "\n") { + i := strings.Index(line, `"`+mod) + if i < 0 { + continue + } + rest := line[i+1+len(mod):] + j := strings.Index(rest, `"`) + if j < 0 { + continue + } + pkg := strings.Trim(rest[:j], "/") + if pkg == "" { + pkg = path.Base(mod) // the module root is itself a package + } + if seen[pkg] { + continue + } + seen[pkg] = true + out = append(out, pkg) + if len(out) >= rcMaxPkgsPerDep { + break + } + } + sort.Strings(out) + return out +} + +// rcDepLimitsBlock is the prompt section: what cannot be read, and what the +// single correct response to that is. +// +// The instruction is as important as the list. Left to itself the reviewer +// produces a concern per call site, because each one genuinely is unverified — +// and a reader cannot tell that speculation from the findings around it. One +// scoped sentence carries the same information and costs the reader nothing. +func rcDepLimitsBlock(deps []rcDepChange) string { + if len(deps) == 0 { + return "" + } + var b strings.Builder + b.WriteString("DEPENDENCIES THIS DIFF MOVES (resolved from the diff before this review started).\n") + b.WriteString("You cannot read these. They are other modules, this checkout holds none of their\n") + b.WriteString("source, and no tool you have will open them — do not spend a turn trying.\n") + for _, d := range deps { + b.WriteString(" ") + b.WriteString(d.Module) + if d.From != "" { + fmt.Fprintf(&b, " %s -> %s", d.From, d.To) + } else { + fmt.Fprintf(&b, " added at %s", d.To) + } + if d.Commit != "" { + fmt.Fprintf(&b, " (commit %s)", d.Commit) + } + b.WriteString("\n") + if len(d.Pkgs) > 0 { + fmt.Fprintf(&b, " imported here: %s\n", strings.Join(d.Pkgs, ", ")) + } + } + b.WriteString("\nAn unread contract is a LIMITATION, not a defect. Say it ONCE — name the module and\n") + b.WriteString("what rests on it — then review what you can actually see. Do not open a concern per\n") + b.WriteString("call site, do not ask the author to confirm a signature you could not read, and do\n") + b.WriteString("not call it a defect: you have no evidence either way, and a reader cannot tell that\n") + b.WriteString("speculation from the findings around it.\n\n") + return b.String() +} diff --git a/cmd/kai/review_commit_deps_test.go b/cmd/kai/review_commit_deps_test.go new file mode 100644 index 0000000..9ae0e13 --- /dev/null +++ b/cmd/kai/review_commit_deps_test.go @@ -0,0 +1,121 @@ +package main + +import ( + "strings" + "testing" +) + +// The TUI#92 diff, reduced to the parts that matter. That review spent most of +// its output speculating about kaipath.UserPath — a 17-line function in another +// module — and escalated one guess to "a real defect". The commit holding the +// answer was sitting in the pseudo-version below the whole time. +const tui92Diff = `diff --git a/go.mod b/go.mod +index 1111111..2222222 100644 +--- a/go.mod ++++ b/go.mod +@@ -67,7 +67,7 @@ require ( + github.com/charmbracelet/bubbletea/v2 v2.0.0 +- github.com/kaicontext/kai-engine v0.6.58 ++ github.com/kaicontext/kai-engine v0.6.59-0.20260908192613-5ab1b102fc2f + github.com/spf13/cobra v1.8.0 +diff --git a/go.sum b/go.sum +--- a/go.sum ++++ b/go.sum ++github.com/kaicontext/kai-engine v0.6.59-0.20260908192613-5ab1b102fc2f h1:deadbeef= +diff --git a/internal/tui/app.go b/internal/tui/app.go +--- a/internal/tui/app.go ++++ b/internal/tui/app.go +@@ -12,6 +12,7 @@ import ( + "os" ++ "github.com/kaicontext/kai-engine/kaipath" + ) +@@ -1360,7 +1361,7 @@ func logTUIPanic() { +- dir := filepath.Join(home, ".kai") ++ dir := kaipath.UserPath(home) +` + +func TestChangedDepsFindsThePinnedCommit(t *testing.T) { + deps := rcChangedDeps(tui92Diff) + if len(deps) != 1 { + t.Fatalf("got %d deps, want exactly kai-engine: %+v", len(deps), deps) + } + d := deps[0] + if d.Module != "github.com/kaicontext/kai-engine" { + t.Errorf("Module = %q", d.Module) + } + if d.From != "v0.6.58" || d.To != "v0.6.59-0.20260908192613-5ab1b102fc2f" { + t.Errorf("versions = %q -> %q", d.From, d.To) + } + // The whole point: the source the reviewer needed is identified by a + // commit the diff already carried. + if d.Commit != "5ab1b102fc2f" { + t.Errorf("Commit = %q, want the pseudo-version's commit", d.Commit) + } + if len(d.Pkgs) != 1 || d.Pkgs[0] != "kaipath" { + t.Errorf("Pkgs = %v, want [kaipath]", d.Pkgs) + } +} + +// go.sum restates every version and would double each entry while adding +// nothing. Only go.mod is read. +func TestChangedDepsReadsOnlyGoMod(t *testing.T) { + onlySum := `diff --git a/go.sum b/go.sum +--- a/go.sum ++++ b/go.sum ++github.com/kaicontext/kai-engine v0.6.59 h1:deadbeef= +` + if deps := rcChangedDeps(onlySum); len(deps) != 0 { + t.Errorf("go.sum alone produced %+v, want nothing", deps) + } +} + +// A module that only appears on "-" lines was removed, and a removed +// dependency is not a contract this change rests on. +func TestChangedDepsSkipsRemovedAndUnchanged(t *testing.T) { + d := `diff --git a/go.mod b/go.mod +--- a/go.mod ++++ b/go.mod +- github.com/old/gone v1.0.0 +- github.com/same/pinned v2.0.0 ++ github.com/same/pinned v2.0.0 +` + if deps := rcChangedDeps(d); len(deps) != 0 { + t.Errorf("got %+v, want nothing: one removal and one no-op", deps) + } +} + +func TestPseudoCommitOnlyAcceptsARealOne(t *testing.T) { + if got := rcPseudoCommit("v0.6.59-0.20260908192613-5ab1b102fc2f"); got != "5ab1b102fc2f" { + t.Errorf("pseudo-version commit = %q", got) + } + for _, v := range []string{"v0.6.58", "v1.2.3-rc1", "v0.0.0-20260101010101-nothexdigits"} { + if got := rcPseudoCommit(v); got != "" { + t.Errorf("rcPseudoCommit(%q) = %q, want empty", v, got) + } + } +} + +// The instruction is the load-bearing half. Without it the reviewer produces a +// concern per call site, each genuinely unverified, and a reader cannot tell +// that speculation from the findings around it. +func TestDepLimitsBlockSaysItOnceOrNotAtAll(t *testing.T) { + if got := rcDepLimitsBlock(nil); got != "" { + t.Errorf("no dependency changes should print nothing, got %q", got) + } + // Matched against whitespace-collapsed text: these assert what the block + // SAYS, and should not break the day a sentence rewraps. + got := strings.Join(strings.Fields(rcDepLimitsBlock(rcChangedDeps(tui92Diff))), " ") + for _, want := range []string{ + "github.com/kaicontext/kai-engine", + "v0.6.58 -> v0.6.59-0.20260908192613-5ab1b102fc2f", + "commit 5ab1b102fc2f", + "imported here: kaipath", + "LIMITATION, not a defect", + "Say it ONCE", + "do not call it a defect", + } { + if !strings.Contains(got, want) { + t.Errorf("block missing %q:\n%s", want, got) + } + } +}