diff --git a/cmd/kai/review_commit_challenge.go b/cmd/kai/review_commit_challenge.go index d20cdb7..f47104a 100644 --- a/cmd/kai/review_commit_challenge.go +++ b/cmd/kai/review_commit_challenge.go @@ -222,6 +222,12 @@ func rcSubmitReviewToolInfo() tools.ToolInfo { const ( rcCoordRows = "rows" rcCoordFile = "file" + // rcCoordNone: a kai_view result whose file line mapping could not be + // established — no "N: text" rows, rows that do not start where the call's + // offset says, an offset the engine would have rejected. It is shown for + // context but CANNOT be cited: falling back to row numbers would let a + // citation of "file line 1" resolve to the tool-call header, silently. + rcCoordNone = "unmapped" ) type rcSource struct { @@ -236,6 +242,8 @@ type rcSource struct { Path string First int Rows []string + // Why is set when Coord is rcCoordNone: the reason no mapping exists. + Why string } // rcPromptSource wraps the review prompt (the fast pass's only source). @@ -278,33 +286,71 @@ func rcFileViewRows(content string, offset int) (first int, rows []string) { return offset + 1, rows } -// rcToolSource classifies one retained tool result. The kai_view call's own -// arguments — not the rendered text — say where the slice starts; the rows the -// tool returned say how far it goes. Structured, but tolerant of the model -// writing "offset": "100" (the engine accepts that too). +// rcViewOffset interprets a kai_view "offset" argument exactly as the engine's +// file tool does (tools/file.go flexInt at kai-engine v0.6.73): a JSON integer +// as is; null → 0; a string, trimmed — "" → 0, otherwise a %g float truncated +// ("0.0", " 0 ", "1e1"); a JSON float truncated; anything else is an error +// the tool itself would have refused. The view then clamps a negative start +// to 0, so this does too. A parser that interprets offset differently from +// the tool maps citations onto the wrong lines. +func rcViewOffset(raw json.RawMessage) (int, error) { + if len(raw) == 0 || string(raw) == "null" { + return 0, nil + } + var n int + if err := json.Unmarshal(raw, &n); err == nil { + return max(n, 0), nil + } + var str string + if err := json.Unmarshal(raw, &str); err == nil { + str = strings.TrimSpace(str) + if str == "" { + return 0, nil + } + var f float64 + if _, err := fmt.Sscanf(str, "%g", &f); err == nil { + return max(int(f), 0), nil + } + return 0, fmt.Errorf("cannot interpret %q as int", str) + } + var f float64 + if err := json.Unmarshal(raw, &f); err == nil { + return max(int(f), 0), nil + } + return 0, fmt.Errorf("cannot unmarshal %s into int", string(raw)) +} + +// rcToolSource classifies one retained tool result. For kai_view the call's +// own arguments — not the rendered text — say where the slice starts, and the +// "N: text" rows the tool returned say how far it goes. When that mapping +// cannot be established the source is UNMAPPED and cannot be cited; it is +// never silently re-addressed by rows. func rcToolSource(name, input, content string) rcSource { src := rcSource{Text: name + " " + input + "\n" + content, Tool: name, Coord: rcCoordRows} if name != "kai_view" { return src } + unmapped := func(why string) rcSource { + src.Coord, src.Why = rcCoordNone, why + return src + } var args struct { FilePath string `json:"file_path"` Offset json.RawMessage `json:"offset"` } - if err := json.Unmarshal([]byte(input), &args); err != nil || args.FilePath == "" { - return src + if err := json.Unmarshal([]byte(input), &args); err != nil { + return unmapped("the kai_view call's arguments could not be read") } - offset := 0 - if len(args.Offset) > 0 { - if n, err := strconv.Atoi(strings.Trim(string(args.Offset), `"`)); err == nil && n >= 0 { - offset = n - } else { - return src - } + if args.FilePath == "" { + return unmapped("the kai_view call names no file") + } + offset, err := rcViewOffset(args.Offset) + if err != nil { + return unmapped("the kai_view call's offset could not be interpreted: " + err.Error()) } first, rows := rcFileViewRows(content, offset) if rows == nil { - return src + return unmapped(fmt.Sprintf("the result carries no file lines starting at line %d (offset %d)", offset+1, offset)) } src.Coord, src.Path, src.First, src.Rows = rcCoordFile, args.FilePath, first, rows return src @@ -350,6 +396,13 @@ func rcSourceLines(body string) []string { // file lines it actually contains. func rcRenderSource(n int, src rcSource) string { var b strings.Builder + if src.Coord == rcCoordNone { + fmt.Fprintf(&b, "SOURCE %d (kai_view result whose file line mapping could not be established — %s; shown for context only, it CANNOT be cited):\n%s", n, src.Why, src.Text) + if !strings.HasSuffix(src.Text, "\n") { + b.WriteString("\n") + } + return b.String() + } if src.Coord == rcCoordFile { last := src.First + len(src.Rows) - 1 fmt.Fprintf(&b, "SOURCE %d (kai_view %s — file lines %d-%d returned; cite FILE line numbers exactly as printed below):\n%s", n, src.Path, src.First, last, src.Text) @@ -378,6 +431,9 @@ func rcExtractCitation(sources []rcSource, ev rcCheckEvidence) (text, reason str return "", "source number is out of range", false } src := sources[ev.Source-1] + if src.Coord == rcCoordNone { + return "", "this source cannot be cited: " + src.Why, false + } if src.Coord == rcCoordFile { last := src.First + len(src.Rows) - 1 if ev.LineStart < src.First || ev.LineEnd < ev.LineStart || ev.LineEnd > last { diff --git a/cmd/kai/review_commit_challenge_test.go b/cmd/kai/review_commit_challenge_test.go index 97f41b7..a022904 100644 --- a/cmd/kai/review_commit_challenge_test.go +++ b/cmd/kai/review_commit_challenge_test.go @@ -120,9 +120,10 @@ func TestReviewChallengeReceivesFullEvidenceAndFreshConversation(t *testing.T) { for _, src := range sources { joined.WriteString(src.Text) } - // This kai_view result carries no "N: " file rows, so it has no file - // coordinates and is a row-addressed source like any other. - if len(sources) != 2 || !strings.Contains(sources[1].Text, file) || sources[1].Coord != rcCoordRows || strings.Contains(joined.String(), "unsupported model assertion") { + // This kai_view result carries no "N: " file rows, so its file mapping + // cannot be established: it is UNMAPPED — shown in full for context, not + // citable — never silently re-addressed by rows. + if len(sources) != 2 || !strings.Contains(sources[1].Text, file) || sources[1].Coord != rcCoordNone || strings.Contains(joined.String(), "unsupported model assertion") { t.Fatalf("sources lost evidence or included speculation: %v", sources) } p := rcChallengeProvider{send: func(ctx context.Context, req provider.Request) (provider.Response, error) { @@ -135,7 +136,7 @@ func TestReviewChallengeReceivesFullEvidenceAndFreshConversation(t *testing.T) { text = req.Messages[0].Parts[0].(message.TextContent).Text } // Source 2 = the tool-call header line + 500 preamble lines + the last line. - if !strings.Contains(text, "SOURCE 2 (502 rows; cite the ROW numbers printed at the left):") || !strings.Contains(text, " 502| critical source at the end") { + if !strings.Contains(text, "SOURCE 2 (kai_view result whose file line mapping could not be established") || !strings.Contains(text, "\ncritical source at the end") { t.Fatal("challenge did not get full evidence in a fresh conversation") } return provider.Response{}, errors.New("provider failed") diff --git a/cmd/kai/review_commit_coordinates_test.go b/cmd/kai/review_commit_coordinates_test.go index 6c5a124..016f2fe 100644 --- a/cmd/kai/review_commit_coordinates_test.go +++ b/cmd/kai/review_commit_coordinates_test.go @@ -91,16 +91,26 @@ func TestKaiViewSliceAndTruncationBounds(t *testing.T) { if short.First != 1 || len(short.Rows) != 3 { t.Fatalf("short file: %+v", short) } - // Results with no file rows have no file coordinates. + // A kai_view result with no file rows has NO coordinates: it is unmapped + // and cannot be cited. Falling back to rows would let "file line 1" + // resolve to the tool-call header. for _, c := range []string{"(empty: offset 900 past end of 300-line file)", "(binary file: a.bin — 12 bytes, not displayed; first NUL byte at offset 0)", ""} { - if s := rcToolSource("kai_view", `{"file_path":"a","offset":900}`, c); s.Coord != rcCoordRows { - t.Fatalf("rows expected for %q, got %s", c, s.Coord) + s := rcToolSource("kai_view", `{"file_path":"a","offset":900}`, c) + if s.Coord != rcCoordNone || s.Why == "" { + t.Fatalf("unmapped expected for %q, got %s", c, s.Coord) + } + if _, reason, ok := rcExtractCitation([]rcSource{s}, rcCheckEvidence{Source: 1, LineStart: 1, LineEnd: 1}); ok || !strings.Contains(reason, "cannot be cited") { + t.Fatalf("unmapped source was citable: %q %v", reason, ok) } } // Rows must start at offset+1: a result whose numbering does not match the - // call's offset is not trusted as file coordinates. - if s := rcToolSource("kai_view", `{"file_path":"a","offset":10}`, "1: x\n2: y\n"); s.Coord != rcCoordRows { - t.Fatalf("mismatched offset accepted as file coordinates: %+v", s) + // call's offset is unmapped, not re-addressed. + if s := rcToolSource("kai_view", `{"file_path":"a","offset":10}`, "1: x\n2: y\n"); s.Coord != rcCoordNone { + t.Fatalf("mismatched offset accepted: %+v", s) + } + // Unmapped sources are still shown, with a header saying they cannot be cited. + if r := rcRenderSource(3, rcToolSource("kai_view", `{"file_path":"a","offset":"x"}`, "1: x\n")); !strings.Contains(r, "CANNOT be cited") || !strings.Contains(r, "offset could not be interpreted") { + t.Fatalf("unmapped rendering: %s", r) } } diff --git a/cmd/kai/review_commit_viewtool_test.go b/cmd/kai/review_commit_viewtool_test.go new file mode 100644 index 0000000..6f7cd34 --- /dev/null +++ b/cmd/kai/review_commit_viewtool_test.go @@ -0,0 +1,121 @@ +package main + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/kaicontext/kai-engine/projects" + "github.com/kaicontext/kai-engine/tools" +) + +// The citation parser against the REAL file tool, not a hand-written imitation +// of its output. Whatever kai_view returns for an offset spelled any way the +// tool accepts, the parser must map the same rows to the same file lines — +// and a citation of "file line 1" must reach the file's first line, never the +// tool-call header. Reproduces the case where `"offset": "0.0"` made the parser +// fall back to row numbering and file line 1 resolved to the header. +func rcRealView(t *testing.T, dir, input string) string { + t.Helper() + ft := &tools.FileTools{Set: projects.Single(dir)} + resp, err := ft.View().Run(context.Background(), tools.ToolCall{ID: "v", Name: "kai_view", Input: input}) + if err != nil { + t.Fatalf("view %s: %v", input, err) + } + if resp.IsError { + t.Fatalf("view %s refused: %s", input, resp.Content) + } + return resp.Content +} + +func TestKaiViewOffsetSpellingsMatchTheRealTool(t *testing.T) { + dir := t.TempDir() + var body strings.Builder + for i := 1; i <= 30; i++ { + body.WriteString("line " + itoa2(i) + "\n") + } + if err := os.WriteFile(filepath.Join(dir, "f.txt"), []byte(body.String()), 0o644); err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + offset string // the JSON as the model wrote it; "" = key absent + wantFirst int + }{ + {"", 1}, {"0", 1}, {"0.0", 1}, {`"0.0"`, 1}, {"null", 1}, {`""`, 1}, {`" 0 "`, 1}, {"-5", 1}, {`"-5"`, 1}, + {"3", 4}, {`"3"`, 4}, {"3.9", 4}, {`" 3 "`, 4}, {`"1e1"`, 11}, + } { + input := `{"file_path":"f.txt","limit":5` + if tc.offset != "" { + input += `,"offset":` + tc.offset + } + input += "}" + t.Run(input, func(t *testing.T) { + content := rcRealView(t, dir, input) + src := rcToolSource("kai_view", input, content) + if src.Coord != rcCoordFile { + t.Fatalf("not file-addressed (%s: %s) for tool output:\n%s", src.Coord, src.Why, content) + } + if src.First != tc.wantFirst || len(src.Rows) != 5 || src.Rows[0] != "line "+itoa2(tc.wantFirst) { + t.Fatalf("mapping: first=%d rows=%d row0=%q; want first %d", src.First, len(src.Rows), src.Rows[0], tc.wantFirst) + } + // The first returned file line is citable and is the file's line, + // never the header; the line before the slice is not citable. + sources := []rcSource{rcPromptSource("p"), src} + got, _, ok := rcExtractCitation(sources, rcCheckEvidence{Source: 2, LineStart: tc.wantFirst, LineEnd: tc.wantFirst}) + if !ok || got != "line "+itoa2(tc.wantFirst) { + t.Fatalf("citation of file line %d: %q %v", tc.wantFirst, got, ok) + } + if tc.wantFirst > 1 { + if _, _, ok := rcExtractCitation(sources, rcCheckEvidence{Source: 2, LineStart: tc.wantFirst - 1, LineEnd: tc.wantFirst - 1}); ok { + t.Fatalf("line %d before the slice was citable", tc.wantFirst-1) + } + } + }) + } + // An offset the tool refuses yields an error response and therefore no + // source at all — the parser is never asked. Confirm the tool's behavior so + // the parser's own rejection list stays aligned with it. + ft := &tools.FileTools{Set: projects.Single(dir)} + resp, _ := ft.View().Run(context.Background(), tools.ToolCall{ID: "v", Name: "kai_view", Input: `{"file_path":"f.txt","offset":"abc"}`}) + if !resp.IsError { + t.Fatalf("the tool accepted offset \"abc\": %s", resp.Content) + } + if src := rcToolSource("kai_view", `{"file_path":"f.txt","offset":"abc"}`, "1: line 1\n"); src.Coord != rcCoordNone { + t.Fatalf("parser accepted an offset the tool refuses: %+v", src) + } +} + +// The real tool on a slice past the end and on a truncated slice. +func TestKaiViewRealToolEdgesAreMappedOrUnmapped(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "g.txt"), []byte("a\nb\nc\n"), 0o644); err != nil { + t.Fatal(err) + } + // Past the end: the tool returns a notice, not rows → unmapped, uncitable. + content := rcRealView(t, dir, `{"file_path":"g.txt","offset":40}`) + src := rcToolSource("kai_view", `{"file_path":"g.txt","offset":40}`, content) + if src.Coord != rcCoordNone { + t.Fatalf("past-the-end result mapped: %+v\n%s", src, content) + } + // Truncated: two rows returned of four (the trailing newline makes a + // phantom empty fourth); the trailer names line 3 but it is not citable. + content = rcRealView(t, dir, `{"file_path":"g.txt","offset":0,"limit":2}`) + src = rcToolSource("kai_view", `{"file_path":"g.txt","offset":0,"limit":2}`, content) + if src.Coord != rcCoordFile || src.First != 1 || len(src.Rows) != 2 || !strings.Contains(content, "(truncated;") { + t.Fatalf("truncated slice: %+v\n%s", src, content) + } + if _, _, ok := rcExtractCitation([]rcSource{src}, rcCheckEvidence{Source: 1, LineStart: 3, LineEnd: 3}); ok { + t.Fatal("a line the tool did not return was citable") + } + // The response the tool actually produced, kept on the record for review. + raw, _ := json.Marshal(map[string]any{"input": `{"file_path":"g.txt","offset":0,"limit":2}`, "content": content}) + t.Logf("real view output: %s", raw) +} + +func itoa2(n int) string { + b, _ := json.Marshal(n) + return string(b) +} diff --git a/docs/review-evidence.md b/docs/review-evidence.md index e8c5df6..9e82407 100644 --- a/docs/review-evidence.md +++ b/docs/review-evidence.md @@ -60,8 +60,14 @@ source's header says which numbers to cite: experiment results — is shown with **row numbers** at the left and cited by those. -Validation uses only the declared system; it never guesses the other one, and -a citation that does not resolve there is invalid. This replaced a rendering +The call's `offset` is interpreted exactly as the file tool interprets it +(integer, `null`, `"0.0"`, `" 3 "`, a float — truncated; a negative clamped +to 0). A `kai_view` result whose file mapping cannot be established — no file +rows, rows that do not start where the offset says, an offset the tool would +refuse — is **unmapped**: shown for context, citable by nothing; it is never +silently re-addressed by rows, which would let "file line 1" resolve to the +tool-call header. Validation uses only the declared system; it never guesses +another one, and a citation that does not resolve there is invalid. This replaced a rendering that stacked the system's row numbers in front of the tool's file line numbers; the model cited file lines, and on large files viewed in slices they fell outside the row range, which withheld whole reviews (kai-cli#119's own