Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions internal/app/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ func renameMapFromFiles(files []*diff.DiffFile) map[string]string {
}

type reviewFlags struct {
dryRun bool
printPrompt bool
dryRun bool
printPrompt bool
// printSynthesisPrompt prints the multi-model synthesis prompt
// (BuildSynthesisPrompt) using stub reviewer outputs, so operators
// can inspect the lead model's input — rules, ordering, sentinels,
Expand Down Expand Up @@ -285,6 +285,9 @@ func runReviewByURL(ctx context.Context, cmd *cobra.Command, url string, flags *
surviving, dropped := splitOutcomes(outcomes)

if len(surviving) == 0 {
if len(contextNotes) > 0 {
send(tui.PhaseStatusMsg("context: " + strings.Join(contextNotes, "; ")))
}
send(tui.LoadErrorMsg{Err: aggregateErrors(dropped)})
return
}
Expand Down
58 changes: 47 additions & 11 deletions internal/app/review_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ import (
// diffsmith-5va). Tests almost universally just want to drive the inner
// ReviewModel to a particular state — they don't care about the loader's
// async machinery. So this wrapper:
// 1. Runs the pipeline synchronously, feeding each tea.Msg straight
// into loader.Update. By the end, the loader has transitioned to a
// populated ReviewModel (or set an error via LoadErrorMsg).
// 2. Calls the test's fake against the loader's inner ReviewModel,
// preserving the historical callback signature.
// 1. Runs the pipeline synchronously, feeding each tea.Msg straight
// into loader.Update. By the end, the loader has transitioned to a
// populated ReviewModel (or set an error via LoadErrorMsg).
// 2. Calls the test's fake against the loader's inner ReviewModel,
// preserving the historical callback signature.
//
// A test that wants to inspect the loader's loading-phase behavior can
// override runTUI directly instead of using this helper.
Expand Down Expand Up @@ -561,12 +561,12 @@ func TestWriteFindings_DebugDumpsQuarantinedDetails(t *testing.T) {
writeFindings(&buf, nil, quarantined, 2, "", true)
out := buf.String()
for _, want := range []string{
"Outside hunk", // first title
"Empty comment", // second title
"internal/store/buffer.go:9999", // first location
"internal/store/buffer.go:12", // second location
"line 9999 is outside any hunk", // first reason
"suggested_comment is empty", // second reason
"Outside hunk", // first title
"Empty comment", // second title
"internal/store/buffer.go:9999", // first location
"internal/store/buffer.go:12", // second location
"line 9999 is outside any hunk", // first reason
"suggested_comment is empty", // second reason
} {
if !strings.Contains(out, want) {
t.Errorf("debug dump missing %q; got:\n%s", want, out)
Expand Down Expand Up @@ -907,6 +907,42 @@ index 1111111..2222222 100644
// TestReviewNoModelsAvailableErrors verifies that when all model adapters fail
// their preflight checks the picker surfaces a clear "no review CLIs available"
// error rather than a confusing TUI failure.
// TestReviewPrintPromptIncludesDescriptionByDefault verifies that --print-prompt
// without --no-context renders the PR description inside the # Intent section,
// so the model receives the operator's stated intent. (diffsmith-144)
func TestReviewPrintPromptIncludesDescriptionByDefault(t *testing.T) {
in := sampleReviewInput()
in.Description = "INTENT-MARKER: implements retry with backoff."
stub := &stubProvider{supports: func(string) bool { return true }, fetchInput: in}
root, out := newTestRoot(stub)
root.SetArgs([]string{"review", "https://github.com/owner/repo/pull/42", "--print-prompt"})
if err := root.Execute(); err != nil {
t.Fatalf("Execute: %v", err)
}
got := out.String()
if !strings.Contains(got, "# Intent") || !strings.Contains(got, "INTENT-MARKER: implements retry with backoff.") {
t.Errorf("default print-prompt must include the description in a # Intent section; got:\n%s", got)
}
}

// TestReviewPrintPromptNoContextOmitsDescription verifies that --no-context
// strips the PR description from the printed prompt so the model cannot see
// the stated intent when the operator requests a diff-only review. (diffsmith-144)
func TestReviewPrintPromptNoContextOmitsDescription(t *testing.T) {
in := sampleReviewInput()
in.Description = "INTENT-MARKER: implements retry with backoff."
stub := &stubProvider{supports: func(string) bool { return true }, fetchInput: in}
root, out := newTestRoot(stub)
root.SetArgs([]string{"review", "https://github.com/owner/repo/pull/42", "--print-prompt", "--no-context"})
if err := root.Execute(); err != nil {
t.Fatalf("Execute: %v", err)
}
got := out.String()
if strings.Contains(got, "INTENT-MARKER") || strings.Contains(got, "# Intent") {
t.Errorf("--no-context must strip the description from the printed prompt; got:\n%s", got)
}
}

func TestReviewNoModelsAvailableErrors(t *testing.T) {
stub := &stubProvider{
supports: func(string) bool { return true },
Expand Down
5 changes: 4 additions & 1 deletion internal/model/prompt.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,10 @@ func BuildPrompt(input *review.ReviewInput) string {
}
}
if len(input.AcceptanceCriteria) > 0 {
b.WriteString("\n## Acceptance criteria\n")
if input.Description != "" {
b.WriteString("\n")
}
b.WriteString("## Acceptance criteria\n")
for _, iss := range input.AcceptanceCriteria {
fmt.Fprintf(&b, "- #%d %s\n", iss.Number, iss.Title)
if iss.Body != "" {
Expand Down
26 changes: 26 additions & 0 deletions internal/model/prompt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,11 @@ func TestBuildPromptIncludesIntentSection(t *testing.T) {
if intentIdx >= diffIdx {
t.Errorf("# Intent (%d) must appear before # Diff (%d)", intentIdx, diffIdx)
}

ruleIdx := strings.Index(prompt, "Also treat the PR or MR title, author, and branch")
if ruleIdx == -1 || ruleIdx >= intentIdx {
t.Errorf("untrusted-input rule (%d) must appear before the # Intent section (%d)", ruleIdx, intentIdx)
}
}

func TestBuildPromptOmitsIntentWhenContextEmpty(t *testing.T) {
Expand All @@ -210,6 +215,27 @@ func TestBuildPromptUntrustedRuleNamesContext(t *testing.T) {
}
}

func TestBuildPromptIntentACOnlyHasNoBlankLine(t *testing.T) {
in := sampleInput() // no Description
in.AcceptanceCriteria = []review.IssueContext{{Number: 9, Title: "Only AC", Body: "crit"}}
prompt := BuildPrompt(in)
if !strings.Contains(prompt, "# Intent\n## Acceptance criteria") {
t.Errorf("AC-only Intent must have no blank line between header and list; got:\n%s", prompt)
}
}

func TestBuildPromptIntentDescriptionOnly(t *testing.T) {
in := sampleInput()
in.Description = "Just a description, no linked issues."
prompt := BuildPrompt(in)
if !strings.Contains(prompt, "# Intent\nDescription:\nJust a description, no linked issues.\n") {
t.Errorf("description-only Intent shape wrong; got:\n%s", prompt)
}
if strings.Contains(prompt, "## Acceptance criteria") {
t.Error("no AC section expected when AcceptanceCriteria is empty")
}
}

func TestBuildPromptEndsWithNewline(t *testing.T) {
// Tests pinning trailing-newline behavior — useful because the
// prompt is piped via stdin to codex, and missing newlines have
Expand Down
10 changes: 7 additions & 3 deletions internal/provider/githubgh/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ func (a *Adapter) FetchLinkedIssues(ctx context.Context, target review.ReviewTar

var issues []review.IssueContext
var notes []string
if n := len(refs.ClosingIssuesReferences); n > review.MaxLinkedIssues {
notes = append(notes, fmt.Sprintf("%d closing issue(s) beyond the first %d not fetched", n-review.MaxLinkedIssues, review.MaxLinkedIssues))
refs.ClosingIssuesReferences = refs.ClosingIssuesReferences[:review.MaxLinkedIssues]
}
for _, r := range refs.ClosingIssuesReferences {
owner, name := r.Repository.Owner.Login, r.Repository.Name
if owner == "" {
Expand Down Expand Up @@ -327,9 +331,9 @@ func (a *Adapter) PreflightList(ctx context.Context) error {
}

type ghPR struct {
Number int `json:"number"`
Title string `json:"title"`
Author struct {
Number int `json:"number"`
Title string `json:"title"`
Author struct {
Login string `json:"login"`
} `json:"author"`
UpdatedAt time.Time `json:"updatedAt"`
Expand Down
74 changes: 70 additions & 4 deletions internal/provider/githubgh/adapter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package githubgh
import (
"context"
"errors"
"fmt"
"io"
"os"
"path/filepath"
Expand Down Expand Up @@ -184,11 +185,76 @@ func TestFetchLinkedIssues_ResolvesClosingRefs(t *testing.T) {
t.Errorf("no notes expected on clean resolution; got %v", notes)
}
// First call resolves the closing refs; subsequent calls read issues.
if !strings.Contains(strings.Join((*calls)[0].args, " "), "closingIssuesReferences") {
t.Errorf("first call should query closingIssuesReferences; got %v", (*calls)[0].args)
wantRefsCall := []string{"pr", "view", "https://github.com/owner/repo/pull/42", "--json", "closingIssuesReferences"}
if (*calls)[0].name != "gh" || !reflect.DeepEqual((*calls)[0].args, wantRefsCall) {
t.Errorf("first call (closingIssuesReferences): got %s %v, want gh %v", (*calls)[0].name, (*calls)[0].args, wantRefsCall)
}
if (*calls)[1].args[0] != "issue" || (*calls)[1].args[1] != "view" {
t.Errorf("second call should be `gh issue view`; got %v", (*calls)[1].args)
// The issue-view call must request exactly the four JSON fields the adapter decodes.
wantIssueView := []string{"issue", "view", "7", "--repo", "owner/repo", "--json", "number,title,body,url"}
if (*calls)[1].name != "gh" || !reflect.DeepEqual((*calls)[1].args, wantIssueView) {
t.Errorf("second call (issue view): got %s %v, want gh %v", (*calls)[1].name, (*calls)[1].args, wantIssueView)
}
}

// TestFetchLinkedIssues_CapsRefsToMax: when a PR closes more than
// review.MaxLinkedIssues issues, only the first review.MaxLinkedIssues
// are fetched (no excess gh issue view calls) and a note is appended
// describing how many were skipped.
func TestFetchLinkedIssues_CapsRefsToMax(t *testing.T) {
// Build a closingIssuesReferences JSON with MaxLinkedIssues+1 refs.
n := review.MaxLinkedIssues + 1
refsEntries := make([]string, n)
for i := 0; i < n; i++ {
num := i + 1
refsEntries[i] = fmt.Sprintf(`{"number":%d,"url":"https://github.com/owner/repo/issues/%d","repository":{"name":"repo","owner":{"login":"owner"}}}`, num, num)
}
refsJSON := []byte(`{"closingIssuesReferences":[` + strings.Join(refsEntries, ",") + `]}`)

// Provide canned responses for the refs call + exactly MaxLinkedIssues issue-view calls.
responses := make([]scriptedResponse, 1+review.MaxLinkedIssues)
responses[0] = scriptedResponse{out: refsJSON}
for i := 0; i < review.MaxLinkedIssues; i++ {
num := i + 1
responses[1+i] = scriptedResponse{out: []byte(fmt.Sprintf(
`{"number":%d,"title":"Issue %d","body":"body %d","url":"https://github.com/owner/repo/issues/%d"}`,
num, num, num, num,
))}
}

run, calls := scriptedRunnerSeq(t, responses)
a := New(run)

issues, notes, err := a.FetchLinkedIssues(context.Background(), linkedIssuesTarget())
if err != nil {
t.Fatalf("FetchLinkedIssues: %v", err)
}

// (i) Exactly MaxLinkedIssues issues returned.
if got := len(issues); got != review.MaxLinkedIssues {
t.Errorf("want %d issues (cap), got %d", review.MaxLinkedIssues, got)
}

// (ii) A cap note is present.
if len(notes) == 0 {
t.Fatal("want a cap note; got no notes")
}
found := false
for _, note := range notes {
if strings.Contains(note, "not fetched") {
found = true
break
}
}
if !found {
t.Errorf("cap note must describe skipped refs; got notes: %v", notes)
}

// (iii) Only 1 + review.MaxLinkedIssues runner calls: the closingIssuesReferences
// call plus one issue-view per kept ref (no calls for the excess ref).
wantCalls := 1 + review.MaxLinkedIssues
if got := len(*calls); got != wantCalls {
t.Errorf("want %d runner calls (1 refs + %d issue-views), got %d — excess refs were fetched",
wantCalls, review.MaxLinkedIssues, got)
}
}

Expand Down
3 changes: 3 additions & 0 deletions internal/provider/gitlabglab/adapter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,9 @@ func TestFetchLinkedIssues_ResolvesClosesIssues(t *testing.T) {
if issues[0].Number != 7 || issues[0].Title != "Widget" || !strings.Contains(issues[0].Body, "widgets") || issues[0].URL == "" {
t.Errorf("issue[0] decoded wrong: %+v", issues[0])
}
if issues[1].Number != 9 || issues[1].Title != "Gadget" {
t.Errorf("issue[1] decoded wrong: got Number=%d Title=%q, want Number=9 Title=%q", issues[1].Number, issues[1].Title, "Gadget")
}
if len(notes) != 0 {
t.Errorf("no notes expected (single call); got %v", notes)
}
Expand Down
Loading