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") + } +}