From a8bdaedba0a76c2814ef357ebc9bef3cb09e26d3 Mon Sep 17 00:00:00 2001 From: kai CI Date: Fri, 18 Sep 2026 08:08:39 +0300 Subject: [PATCH] review-commit: interpret kai_view offset as the file tool does; an unmappable view result is uncitable, never row-addressed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the declared-coordinates change. Its parser accepted only a plain non-negative integer for kai_view's offset and, on anything else, fell back to ROW coordinates for that source. The engine's file tool (flexInt, kai-engine v0.6.73) accepts an integer, null, "", "0.0", " 0 ", a float and negatives (clamped to 0). So a call written {"offset": "0.0"} produced a row-addressed source, and a citation of "file line 1" resolved — validly, by the row contract — to the tool-call header line. Reported by Jacob with a reproduction. rcViewOffset now mirrors flexInt exactly, plus the view's negative clamp. A kai_view result whose file mapping cannot be established for any reason — an offset the tool would refuse, no "N: text" rows, rows not starting at offset+1 — is UNMAPPED: rendered for context with a header saying it cannot be cited, and any citation into it is invalid (degrading its allegation as any invalid citation does). There is no fallback to rows. Regression tests drive the REAL file tool (tools.FileTools on a temp workspace): fourteen offset spellings map to the same first line the tool returned and "file line N" reaches the file's line, never the header; an offset the tool refuses is also refused by the parser; a slice past the end is unmapped; a truncated slice exposes only the returned lines. Co-Authored-By: Claude Fable 5.1 --- cmd/kai/review_commit_challenge.go | 84 ++++++++++++--- cmd/kai/review_commit_challenge_test.go | 9 +- cmd/kai/review_commit_coordinates_test.go | 22 ++-- cmd/kai/review_commit_viewtool_test.go | 121 ++++++++++++++++++++++ docs/review-evidence.md | 10 +- 5 files changed, 220 insertions(+), 26 deletions(-) create mode 100644 cmd/kai/review_commit_viewtool_test.go 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