review-commit: interpret kai_view offset as the file tool does; unmappable view results are uncitable

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 {

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

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

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

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