From 07c1a570b947e58b88adc4e541a8ea1880017366 Mon Sep 17 00:00:00 2001 From: kai CI Date: Wed, 9 Sep 2026 09:11:12 +0300 Subject: [PATCH] Tell the reviewer which repository it is reading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviews on kai-desktop#288 and kai-desktop#300 (2026-09-08) each stated the repository they were reading. Both were wrong — "the kai-engine repo" and "the kai-server working tree" — and nothing in either run could have told them otherwise. Three facts, and together they make the mistake inevitable: 1. The CI job clones into `mktemp -d`, so the workspace the agent runs in is a path like /tmp/tmp.aBc123. The directory name carries nothing. 2. The prompt is AUTHOR CONTEXT, CHANGED SYMBOLS, WHERE THESE LIVE, INTENT, DIFF. The repository is named in none of them, and rcReviewSystem does not name it either. 3. rcReviewSystem REQUIRES one in the output: "name the boundary you actually searched and put that boundary in the sentence — 'within this repo, the only caller is X' is honest". So the instructions demand an answer the input withholds, from a process running in an anonymous directory. The model did the only thing left and supplied a plausible sibling from the same ecosystem. It could not have recovered by looking, either: the run is ReadOnly with no bash, and finding out its own name should not cost a turn from a budget that already runs out. rcRepoIdentity resolves the slug the way resolveGitHubClient already does (autofix_cmd.go), so there is one answer to "which GitHub repo am I in" rather than two: GITHUB_REPOSITORY_FULLNAME first — the review workflow prefers it for exactly the case where the kai org name and the GitHub org name differ — then GITHUB_REPOSITORY, then the checkout's own origin remote, which is what makes this work for a human running review-commit locally. The header names the repo twice on purpose: once as a fact, and once inside the boundary sentence the system prompt asks the model to write. Naming it only at the top is how it kept getting ignored. Empty in, empty out. When nothing resolves the header is omitted entirely rather than guessed: an unnamed boundary is recoverable — the reviewer says "this repo" and the reader knows which PR they are on — while a confidently wrong one is not. Inventing a name here would rebuild the defect. This does not fix the anonymous clone directory; that is a kai-server workflow change and a separate risk. Co-Authored-By: Claude Opus 5 --- cmd/kai/review_commit.go | 58 ++++++++++++++++++++ cmd/kai/review_commit_repo_test.go | 85 ++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 cmd/kai/review_commit_repo_test.go diff --git a/cmd/kai/review_commit.go b/cmd/kai/review_commit.go index 1deab90..4a9ea63 100644 --- a/cmd/kai/review_commit.go +++ b/cmd/kai/review_commit.go @@ -15,10 +15,12 @@ import ( "github.com/spf13/cobra" + "kai/internal/autofix" "kai/internal/config" "github.com/kaicontext/kai-engine/agent" "github.com/kaicontext/kai-engine/finding" + "github.com/kaicontext/kai-engine/gitio" "github.com/kaicontext/kai-engine/message" "github.com/kaicontext/kai-engine/planner" "github.com/kaicontext/kai-engine/projects" @@ -559,6 +561,57 @@ func rcInferIntent(ctx context.Context, prov provider.Provider, model, subject, return out.String(), nil } +// rcRepoIdentity resolves the repository the review is about, as an +// "owner/name" GitHub slug. +// +// The reviewer used to have no way to know this, and it showed: reviews on +// kai-desktop#288 and kai-desktop#300 (2026-09-08) stated they were reading +// "the kai-engine repo" and "the kai-server working tree". Neither was true, +// and nothing in the run could have told them otherwise — the CI job clones +// into `mktemp -d`, so the workspace is a random path like /tmp/tmp.aBc123, +// and the prompt named the repository nowhere. Meanwhile rcReviewSystem +// REQUIRES a repository in the output ("name the boundary you actually +// searched … 'within this repo, the only caller is X' is honest"). The +// instructions demanded an answer the input withheld, so the model supplied a +// plausible sibling from the same ecosystem. +// +// The resolution order mirrors resolveGitHubClient (autofix_cmd.go), so this +// repo has one answer to "which GitHub repo am I in" rather than two: +// GITHUB_REPOSITORY_FULLNAME first — the CI workflow prefers it for exactly +// the case where the kai org name and the GitHub org name differ — then +// GITHUB_REPOSITORY, then the checkout's own origin remote, which is what +// makes this work for a human running review-commit locally. +// +// Returns "" when nothing resolves. The caller then says nothing rather than +// guessing, which is the whole point. +func rcRepoIdentity(dir string) string { + for _, env := range []string{"GITHUB_REPOSITORY_FULLNAME", "GITHUB_REPOSITORY"} { + if v := strings.TrimSpace(os.Getenv(env)); v != "" { + return v + } + } + if url, err := gitio.RemoteURL(dir, "origin"); err == nil { + return autofix.RepoSlugFromRemote(url) + } + return "" +} + +// rcRepoHeader is the prompt's opening line: which repository this is. +// +// Empty in, empty out. An unnamed boundary is recoverable — the reviewer says +// "this repo" and a reader knows which PR they are looking at — while a +// confidently wrong one is not, and inventing a name here would rebuild the +// exact defect this exists to close. +func rcRepoHeader(repo string) string { + if repo == "" { + return "" + } + return fmt.Sprintf("REPOSITORY: %s\n(The repository under review. Every path below is relative to its root. "+ + "This is the boundary to name when you write one — \"within %s, the only caller is X\". "+ + "Do not name a different repository as the one you are reading; sibling repos you cannot see "+ + "here are exactly the limit worth stating.)\n\n", repo, repo) +} + // rcRunReviewAgent runs the review through the shared harness runner, set up // the way the orchestrator sets up its executors: agent.ModeReview supplies // the harness's review personality + read-only tool whitelist, the graph @@ -570,6 +623,11 @@ func rcRunReviewAgent(ctx context.Context, set *projects.Set, prov provider.Prov gdb := asGraphDB(primary.DB) var user strings.Builder + // First line of the prompt, because everything after it is relative to + // this. rcReviewSystem asks the reviewer to name the boundary it searched; + // this is the name. Omitted entirely when it cannot be resolved — an + // unnamed boundary is recoverable, a confidently wrong one is not. + user.WriteString(rcRepoHeader(rcRepoIdentity(primary.Path))) if sc := strings.TrimSpace(sourceContext); sc != "" { if len(sc) > rcMaxAuthorContextBytes { sc = sc[:rcMaxAuthorContextBytes] + "\n... (context truncated)" diff --git a/cmd/kai/review_commit_repo_test.go b/cmd/kai/review_commit_repo_test.go new file mode 100644 index 0000000..cdd2d97 --- /dev/null +++ b/cmd/kai/review_commit_repo_test.go @@ -0,0 +1,85 @@ +package main + +import ( + "os/exec" + "strings" + "testing" +) + +// Reviews on kai-desktop#288 and kai-desktop#300 (2026-09-08) stated they were +// reading "the kai-engine repo" and "the kai-server working tree". Nothing in +// the run could have told them otherwise: the CI job clones into `mktemp -d`, +// so the workspace is a random path, and the prompt named the repository +// nowhere — while rcReviewSystem requires one in the output. These assert the +// answer is now supplied rather than guessed. +func TestRepoIdentityPrefersTheWorkflowsOwnAnswer(t *testing.T) { + // GITHUB_REPOSITORY_FULLNAME wins, because the CI workflow prefers it for + // exactly the case where the kai org name and the GitHub org name differ. + t.Setenv("GITHUB_REPOSITORY_FULLNAME", "kaicontext/kai-desktop") + t.Setenv("GITHUB_REPOSITORY", "kai/kai-desktop") + if got := rcRepoIdentity(t.TempDir()); got != "kaicontext/kai-desktop" { + t.Errorf("rcRepoIdentity = %q, want the GitHub-side slug", got) + } +} + +func TestRepoIdentityFallsBackToGithubRepository(t *testing.T) { + t.Setenv("GITHUB_REPOSITORY_FULLNAME", "") + t.Setenv("GITHUB_REPOSITORY", "kaicontext/kai-cli") + if got := rcRepoIdentity(t.TempDir()); got != "kaicontext/kai-cli" { + t.Errorf("rcRepoIdentity = %q, want kaicontext/kai-cli", got) + } +} + +// The local path: a human running `kai review-commit` has no GitHub +// environment at all, and the checkout's own remote is the answer. +func TestRepoIdentityReadsTheOriginRemote(t *testing.T) { + t.Setenv("GITHUB_REPOSITORY_FULLNAME", "") + t.Setenv("GITHUB_REPOSITORY", "") + dir := t.TempDir() + for _, args := range [][]string{ + {"init", "--quiet"}, + {"remote", "add", "origin", "git@github.com:kaicontext/kai-desktop.git"}, + } { + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Skipf("git %v unavailable here: %v (%s)", args, err, out) + } + } + if got := rcRepoIdentity(dir); got != "kaicontext/kai-desktop" { + t.Errorf("rcRepoIdentity = %q, want the slug from the origin remote", got) + } +} + +// Nothing resolves: say nothing. An unnamed boundary is recoverable, a +// confidently wrong one is not — inventing a name here would rebuild the +// defect this closes. +func TestRepoIdentityStaysSilentWhenItCannotTell(t *testing.T) { + t.Setenv("GITHUB_REPOSITORY_FULLNAME", "") + t.Setenv("GITHUB_REPOSITORY", "") + if got := rcRepoIdentity(t.TempDir()); got != "" { + t.Errorf("rcRepoIdentity = %q in a non-repo with no environment, want empty", got) + } + if got := rcRepoHeader(""); got != "" { + t.Errorf("rcRepoHeader(\"\") = %q, want empty", got) + } +} + +// The header has to name the repo where the system prompt asks for it: the +// boundary sentence. Naming it once at the top and not in the instruction the +// model is following is how it got ignored before. +func TestRepoHeaderNamesTheBoundary(t *testing.T) { + got := rcRepoHeader("kaicontext/kai-desktop") + for _, want := range []string{ + "REPOSITORY: kaicontext/kai-desktop", + `within kaicontext/kai-desktop, the only caller is X`, + "Do not name a different repository", + } { + if !strings.Contains(got, want) { + t.Errorf("header missing %q:\n%s", want, got) + } + } + if !strings.HasSuffix(got, "\n\n") { + t.Error("header must end with a blank line so it does not run into AUTHOR CONTEXT") + } +}