From 19c2db16a8aa4f3731382d19fbf3a77c3ebc96b9 Mon Sep 17 00:00:00 2001 From: kai CI Date: Wed, 9 Sep 2026 12:10:45 +0300 Subject: [PATCH] Name the contracts the reviewer cannot read, before it guesses at them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 supported, 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, rendered indistinguishably from the findings around it. The reviewer was not being careless. It is 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. ## Why this does not fetch the source The review pod is debian:bookworm-slim carrying `kai` and `kit`. There is no Go toolchain and no module cache, and the workflow never runs `go mod download` — which for a private module would need GOPRIVATE and auth besides. Sibling module source is not merely unfetched, it is absent. So this names the gap instead of pretending to close it. The block lists each module the diff moves, the version change, the commit inside a pseudo-version, and which of its packages this diff imports — then says that one scoped sentence is the whole correct response: An unread contract is a LIMITATION, not a defect. Say it ONCE [...] Do not open a concern per call site, do not ask the author to confirm a signature you could not read, and do not call it a defect: you have no evidence either way. The instruction is the load-bearing half. The list alone would just tell the reviewer what it already knew it could not see. ## Groundwork, not a detour The parsing is the parsing a real fetch needs: module path, version, and the commit inside the pseudo-version. TUI#92's diff carried v0.6.59-0.20260908192613-5ab1b102fc2f — the exact source that would have answered every one of its questions, identified in the input it was given. When there is a way to read it, that commit is already resolved. Reads go.mod only; go.sum restates every version and would double each entry. A module that appears only on a "-" line was removed, and a removed dependency is not a contract the change rests on. Co-Authored-By: Claude Opus 5 --- cmd/kai/review_commit.go | 4 + cmd/kai/review_commit_deps.go | 232 +++++++++++++++++++++++++++++ cmd/kai/review_commit_deps_test.go | 121 +++++++++++++++ 3 files changed, 357 insertions(+) create mode 100644 cmd/kai/review_commit_deps.go create mode 100644 cmd/kai/review_commit_deps_test.go 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) + } + } +}