From a71ee2e2c1932f9ec99f4f067fbf91bb0cfc2233 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fiere?= Date: Wed, 17 Jun 2026 21:32:40 +0200 Subject: [PATCH] fix: open agent file links that carry line-range selectors (#1412) --- .../server/adapter/transport/acp/normalize.go | 50 +++++-- .../adapter/transport/acp/normalize_test.go | 87 ++++++++++++ .../server/process/workspace_files.go | 50 +++++++ .../server/process/workspace_tracker_test.go | 113 ++++++++++++++++ .../common/readselector/readselector.go | 122 +++++++++++++++++ .../common/readselector/readselector_test.go | 81 +++++++++++ .../shared/markdown-components.test.tsx | 23 ++++ .../components/shared/markdown-components.tsx | 6 +- .../task/chat/messages/tool-edit-message.tsx | 30 ++++- .../task/chat/messages/tool-read-message.tsx | 49 ++++++- .../components/task/file-viewer-content.tsx | 40 ++++++ .../e2e/tests/task/mobile-file-viewer.spec.ts | 127 ++++++++++++++++++ apps/web/hooks/use-file-editors.ts | 37 +++++ apps/web/hooks/use-lsp-file-opener.ts | 35 +---- 14 files changed, 798 insertions(+), 52 deletions(-) create mode 100644 apps/backend/internal/common/readselector/readselector.go create mode 100644 apps/backend/internal/common/readselector/readselector_test.go diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go index 0a86ff1127..d17936dda8 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go @@ -6,6 +6,7 @@ import ( "github.com/kandev/kandev/internal/agentctl/server/adapter/transport/shared" "github.com/kandev/kandev/internal/agentctl/types/streams" + "github.com/kandev/kandev/internal/common/readselector" ) // Tool operation type constants. @@ -313,9 +314,24 @@ func updateModifyFileInput(mf *streams.ModifyFilePayload, supplemental, inputMap } } +// updateReadFileInput fills a ReadFile payload from a tool_call_update: it sets +// FilePath (when still empty) to the selector-stripped path and populates +// Offset/Limit from the parsed line range when those fields are unset. func updateReadFileInput(rf *streams.ReadFilePayload, supplemental, inputMap map[string]any) { - if path := pathFromArgs(supplemental, inputMap); path != "" && rf.FilePath == "" { - rf.FilePath = path + // Parse the selector on every update — a later tool_call_update can carry the + // line range (e.g. "main.go:50+150") even when an earlier frame already set + // FilePath, so range metadata must not be gated on FilePath being empty. + if path := pathFromArgs(supplemental, inputMap); path != "" { + clean, startLine, lineCount := readselector.Split(path) + if rf.FilePath == "" { + rf.FilePath = clean + } + if rf.Offset == 0 { + rf.Offset = startLine + } + if rf.Limit == 0 { + rf.Limit = lineCount + } } } @@ -383,13 +399,11 @@ func (n *Normalizer) normalizeEdit(args map[string]any) *streams.NormalizedPaylo Type: streams.MutationPatch, } - // Add line numbers if available - if startLine, ok := rawInput["old_str_start_line_number_1"].(float64); ok { - mutation.StartLine = int(startLine) - } - if endLine, ok := rawInput["old_str_end_line_number_1"].(float64); ok { - mutation.EndLine = int(endLine) - } + // Line numbers vary by agent: Claude's str-replace editor uses + // old_str_*_line_number_1, OMP uses startLine/endLine. Accept both + // (plus snake_case) so edit links can navigate to the changed lines. + mutation.StartLine = firstInt(rawInput, "old_str_start_line_number_1", "startLine", "start_line") + mutation.EndLine = firstInt(rawInput, "old_str_end_line_number_1", "endLine", "end_line") // Generate unified diff when at least one string is provided if oldStr != "" || newStr != "" { @@ -403,6 +417,18 @@ func (n *Normalizer) normalizeEdit(args map[string]any) *streams.NormalizedPaylo return streams.NewModifyFile(path, mutations) } +// firstInt returns the first key whose value is a non-zero int, handling JSON +// numbers (decoded as float64). Line/column numbers are 1-based, so a missing +// or zero value is treated as absent. +func firstInt(m map[string]any, keys ...string) int { + for _, k := range keys { + if v := shared.GetInt(m, k); v != 0 { + return v + } + } + return 0 +} + // normalizeRead converts ACP read tool data. // If rawInput.type is "directory", this becomes a code search (file listing) operation. func (n *Normalizer) normalizeRead(args map[string]any) *streams.NormalizedPayload { @@ -412,13 +438,17 @@ func (n *Normalizer) normalizeRead(args map[string]any) *streams.NormalizedPaylo } path := pathFromArgs(args, rawInput) + // OMP's read tool embeds a line/range/mode selector in the path + // (e.g. "foo.go:43-94"); strip it so the file link stays openable and + // carry the parsed range via offset/limit. No-op for other agents. + path, startLine, lineCount := readselector.Split(path) // Check if this is a directory read - treat as code search (file listing) if readType := shared.GetString(rawInput, "type"); readType == readTypeDirectory { return streams.NewCodeSearch("", "", path, "") } - return streams.NewReadFile(path, 0, 0) + return streams.NewReadFile(path, startLine, lineCount) } // normalizeExecute converts ACP execute/bash tool data. diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize_test.go index aab8be43a2..a5907787e9 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize_test.go @@ -393,6 +393,40 @@ func TestNormalizerEdit(t *testing.T) { t.Errorf("expected FilePath 'src/main.go', got %q", result.ModifyFile().FilePath) } }) + + t.Run("extracts omp startLine/endLine into mutation", func(t *testing.T) { + result := normalizer.NormalizeToolCall("edit", map[string]any{ + "kind": "edit", + "raw_input": map[string]any{ + "path": "main.go", + "old_str_1": "a", + "new_str_1": "b", + "startLine": float64(43), + "endLine": float64(94), + }, + }) + m := result.ModifyFile().Mutations[0] + if m.StartLine != 43 || m.EndLine != 94 { + t.Errorf("expected StartLine=43 EndLine=94, got %d/%d", m.StartLine, m.EndLine) + } + }) + + t.Run("still extracts claude old_str line numbers", func(t *testing.T) { + result := normalizer.NormalizeToolCall("edit", map[string]any{ + "kind": "edit", + "raw_input": map[string]any{ + "path": "main.go", + "old_str_1": "a", + "new_str_1": "b", + "old_str_start_line_number_1": float64(10), + "old_str_end_line_number_1": float64(12), + }, + }) + m := result.ModifyFile().Mutations[0] + if m.StartLine != 10 || m.EndLine != 12 { + t.Errorf("expected StartLine=10 EndLine=12, got %d/%d", m.StartLine, m.EndLine) + } + }) } // TestNormalizerRead tests the normalizer's read handling. @@ -453,6 +487,59 @@ func TestNormalizerRead(t *testing.T) { t.Errorf("expected Path '.', got %q", result.CodeSearch().Path) } }) + + t.Run("strips omp line selector from path and records range", func(t *testing.T) { + args := map[string]any{ + "kind": "read", + "raw_input": map[string]any{ + "path": "apps/backend/internal/sentry/handlers.go:43-94", + }, + } + result := normalizer.NormalizeToolCall("read", args) + rf := result.ReadFile() + if rf == nil { + t.Fatal("expected ReadFile to be set") + } + if rf.FilePath != "apps/backend/internal/sentry/handlers.go" { + t.Errorf("expected stripped FilePath, got %q", rf.FilePath) + } + if rf.Offset != 43 || rf.Limit != 52 { + t.Errorf("expected Offset=43 Limit=52, got Offset=%d Limit=%d", rf.Offset, rf.Limit) + } + }) + + t.Run("strips omp selector arriving on a tool_call_update", func(t *testing.T) { + payload := normalizer.NormalizeToolCall("read", map[string]any{"kind": "read"}) + normalizer.UpdatePayloadInput(payload, map[string]any{"path": "main.go:50+150"}, nil) + rf := payload.ReadFile() + if rf == nil { + t.Fatal("expected ReadFile to be set") + } + if rf.FilePath != "main.go" { + t.Errorf("expected stripped FilePath 'main.go', got %q", rf.FilePath) + } + if rf.Offset != 50 || rf.Limit != 150 { + t.Errorf("expected Offset=50 Limit=150, got Offset=%d Limit=%d", rf.Offset, rf.Limit) + } + }) + + t.Run("populates range from update even when FilePath already set", func(t *testing.T) { + payload := normalizer.NormalizeToolCall("read", map[string]any{ + "kind": "read", + "raw_input": map[string]any{"path": "main.go"}, + }) + if payload.ReadFile().FilePath != "main.go" { + t.Fatalf("expected initial FilePath 'main.go', got %q", payload.ReadFile().FilePath) + } + normalizer.UpdatePayloadInput(payload, map[string]any{"path": "main.go:50+150"}, nil) + rf := payload.ReadFile() + if rf.FilePath != "main.go" { + t.Errorf("expected FilePath unchanged 'main.go', got %q", rf.FilePath) + } + if rf.Offset != 50 || rf.Limit != 150 { + t.Errorf("expected Offset=50 Limit=150 from update, got Offset=%d Limit=%d", rf.Offset, rf.Limit) + } + }) } // TestNormalizerResult tests the normalizer's result handling. diff --git a/apps/backend/internal/agentctl/server/process/workspace_files.go b/apps/backend/internal/agentctl/server/process/workspace_files.go index d02e41bc97..f0e505a7ad 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files.go @@ -19,6 +19,7 @@ import ( "unicode/utf8" "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/common/readselector" "github.com/kandev/kandev/internal/common/subproc" "go.uber.org/zap" ) @@ -279,6 +280,37 @@ func resolveNonExistentPath(path string) (string, error) { // If the file is not valid UTF-8, it is base64-encoded and isBinary is true. // If the file is a symlink, resolvedPath contains the target path relative to the workspace root. func (wt *WorkspaceTracker) GetFileContent(reqPath string) (string, int64, bool, string, error) { + // Try the path exactly as requested first, so a real workspace file whose + // name contains a colon (e.g. "notes.txt:2-3") or a literal "~" path + // segment still opens. Only if the literal open fails do we treat a + // trailing piece as an omp read selector (e.g. "foo.go:43-94", multi-range) + // and/or a "~"-home shorthand, strip/expand it, and retry — so agent links + // that embed a selector open without breaking valid filenames. + content, size, isBinary, resolved, err := wt.readResolvedPath(reqPath) + if err == nil { + return content, size, isBinary, resolved, nil + } + if alt := expandHomePath(stripReadSelector(reqPath)); alt != reqPath { + if c, s, b, r, altErr := wt.readResolvedPath(alt); altErr == nil { + return c, s, b, r, nil + } + } + // Neither the literal path nor the stripped/expanded fallback opened; + // surface the original (literal-path) error, which is the most meaningful. + return content, size, isBinary, resolved, err +} + +// stripReadSelector removes a trailing omp read selector from reqPath (the line +// range itself is irrelevant to serving content); paths without a selector are +// returned unchanged. +func stripReadSelector(reqPath string) string { + clean, _, _ := readselector.Split(reqPath) + return clean +} + +// readResolvedPath resolves reqPath within the workspace, falling back to the +// read-only external-absolute-path path (ADR 0016), and returns its content. +func (wt *WorkspaceTracker) readResolvedPath(reqPath string) (string, int64, bool, string, error) { safePath, err := wt.resolveSafePath(reqPath) if err != nil { if errors.Is(err, errPathTraversal) { @@ -299,6 +331,24 @@ func (wt *WorkspaceTracker) GetFileContent(reqPath string) (string, int64, bool, return content, size, isBinary, resolvedPath, err } +// expandHomePath expands a leading "~" or "~/" to the current user's home +// directory. omp emits read paths like "~/.kandev/…"; a literal tilde is not a +// real filesystem path, so it must be expanded before stat/serve. Other paths +// (absolute, workspace-relative) are returned unchanged. +func expandHomePath(path string) string { + if path != "~" && !strings.HasPrefix(path, "~/") && !strings.HasPrefix(path, `~\`) { + return path + } + home, err := os.UserHomeDir() + if err != nil || home == "" { + return path + } + if path == "~" { + return home + } + return filepath.Join(home, path[2:]) +} + func readFileContent(safePath string) (string, int64, bool, error) { // Check if file exists and is a regular file info, err := os.Stat(safePath) diff --git a/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go b/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go index 22bbf9bcbb..5d2e75e232 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go @@ -556,6 +556,119 @@ func TestGetFileContent_ExternalAbsolutePath(t *testing.T) { } } +// TestGetFileContent_StripsReadSelector covers the file-open boundary fix: a +// path carrying an omp read-selector (e.g. ":43-94") must still open, +// both for an in-workspace file and for an external absolute file (the +// CLAUDE.md:93-113 "path traversal detected" repro). +func TestGetFileContent_StripsReadSelector(t *testing.T) { + workDir, wt := setupTestDir(t) + + inside := filepath.Join(workDir, "notes.txt") + insideContent := "l1\nl2\nl3\nl4\n" + if err := os.WriteFile(inside, []byte(insideContent), 0o644); err != nil { + t.Fatalf("write in-workspace file: %v", err) + } + content, _, _, _, err := wt.GetFileContent("notes.txt:2-3") + if err != nil { + t.Fatalf("GetFileContent(in-workspace + selector) error: %v", err) + } + if content != insideContent { + t.Fatalf("content = %q, want full file %q", content, insideContent) + } + + externalDir := t.TempDir() + externalPath := filepath.Join(externalDir, "CLAUDE.md") + externalContent := "global config\n" + if err := os.WriteFile(externalPath, []byte(externalContent), 0o644); err != nil { + t.Fatalf("write external file: %v", err) + } + content, _, _, _, err = wt.GetFileContent(externalPath + ":93-113") + if err != nil { + t.Fatalf("GetFileContent(external absolute + selector) error: %v", err) + } + if content != externalContent { + t.Fatalf("content = %q, want %q", content, externalContent) + } +} + +// TestGetFileContent_ExpandsTildeWithMultiRange covers OMP reads that arrive as +// a "~"-prefixed home path with a multi-range selector +// (e.g. "~/.kandev/x/README.md:16-20,32-40,69-69,85-96"): the tilde must be +// expanded and the whole multi-range selector stripped for the open to succeed. +func TestGetFileContent_ExpandsTildeWithMultiRange(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) // os.UserHomeDir() on unix/macOS + t.Setenv("USERPROFILE", home) // os.UserHomeDir() on Windows + sub := filepath.Join(home, "notes") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + want := "global config\n" + if err := os.WriteFile(filepath.Join(sub, "GLOBAL.md"), []byte(want), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + _, wt := setupTestDir(t) + got, _, _, _, err := wt.GetFileContent("~/notes/GLOBAL.md:16-20,32-40,69-69,85-96") + if err != nil { + t.Fatalf("GetFileContent(tilde + multi-range) error: %v", err) + } + if got != want { + t.Fatalf("content = %q, want %q", got, want) + } +} + +// TestGetFileContent_PrefersLiteralColonPath verifies a real workspace file +// whose name literally contains a colon (e.g. "notes.txt:2-3") opens as-is and +// is NOT mistaken for a "notes.txt" + read selector. +func TestGetFileContent_PrefersLiteralColonPath(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("colon is not a legal filename character on Windows") + } + workDir, wt := setupTestDir(t) + if err := os.WriteFile(filepath.Join(workDir, "notes.txt"), []byte("plain\n"), 0o644); err != nil { + t.Fatalf("write notes.txt: %v", err) + } + literal := "literal-colon-file\n" + if err := os.WriteFile(filepath.Join(workDir, "notes.txt:2-3"), []byte(literal), 0o644); err != nil { + t.Fatalf("write literal: %v", err) + } + got, _, _, _, err := wt.GetFileContent("notes.txt:2-3") + if err != nil { + t.Fatalf("GetFileContent error: %v", err) + } + if got != literal { + t.Fatalf("expected literal colon file %q, got %q (selector wrongly stripped)", literal, got) + } +} + +// TestGetFileContent_PrefersLiteralTildePath verifies a workspace file under a +// literal "~" directory opens as-is and is NOT expanded to the user's home. +func TestGetFileContent_PrefersLiteralTildePath(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + // Decoy at the home-expanded location to prove expansion is not used. + if err := os.WriteFile(filepath.Join(home, "config.txt"), []byte("home-decoy\n"), 0o644); err != nil { + t.Fatalf("write decoy: %v", err) + } + workDir, wt := setupTestDir(t) + tildeDir := filepath.Join(workDir, "~") + if err := os.MkdirAll(tildeDir, 0o755); err != nil { + t.Fatalf("mkdir ~: %v", err) + } + want := "workspace-tilde\n" + if err := os.WriteFile(filepath.Join(tildeDir, "config.txt"), []byte(want), 0o644); err != nil { + t.Fatalf("write tilde file: %v", err) + } + got, _, _, _, err := wt.GetFileContent("~/config.txt") + if err != nil { + t.Fatalf("GetFileContent error: %v", err) + } + if got != want { + t.Fatalf("expected workspace tilde file %q, got %q (path wrongly home-expanded)", want, got) + } +} + // setupTestDir creates a temp directory with a WorkspaceTracker (no git required). func setupTestDir(t *testing.T) (string, *WorkspaceTracker) { t.Helper() diff --git a/apps/backend/internal/common/readselector/readselector.go b/apps/backend/internal/common/readselector/readselector.go new file mode 100644 index 0000000000..71dedfbd8e --- /dev/null +++ b/apps/backend/internal/common/readselector/readselector.go @@ -0,0 +1,122 @@ +// Package readselector parses Oh-My-Pi (omp) style read paths of the form +// "path:" into the bare file path plus the parsed line range. +// +// OMP's read tool embeds a line/range/mode selector in the path argument +// (e.g. "foo.go:43-94", "foo.go:50+150", "foo.go:5-16,960-973", "foo.go:raw", +// "foo.go:2-4:raw"). Kandev opens files by stat'ing the path, so a selector +// left on the path makes the open fail ("Failed to open file … no such file" +// for workspace files, or "path traversal detected" for absolute external +// files whose stat'd ":" simply doesn't exist). +// +// Both the ACP normalizer (so new file links are stored clean and the line +// range is surfaced separately) and the file-open boundary (so already-persisted +// links and any normalization gap still open) call Split — a single source of +// truth shared by both. +package readselector + +import ( + "strconv" + "strings" +) + +// Split splits "path:" into the bare file path plus the parsed line +// range. startLine is the first line referenced by the selector; lineCount is +// the contiguous span when a closed range is given ("N-M" or "N+K"), and 0 when +// the selector is open-ended ("N", "N-") or carries no line numbers ("raw"). +// When no valid selector is present the path is returned unchanged with zero +// line info, so paths from non-omp agents (which never match the grammar) and +// ordinary filenames are unaffected. +func Split(raw string) (path string, startLine, lineCount int) { + // Only inspect colons in the final path segment so a directory component + // that legitimately contains a colon is never mistaken for a selector. + // Consider both '/' and '\\' so Windows paths — and their drive-letter + // colon, e.g. "C:\\dir\\file.go" — are not misread as a selector boundary. + lastSep := strings.LastIndexByte(raw, '/') + if b := strings.LastIndexByte(raw, '\\'); b > lastSep { + lastSep = b + } + rel := strings.IndexByte(raw[lastSep+1:], ':') + if rel < 0 { + return raw, 0, 0 + } + colon := lastSep + 1 + rel + base, suffix := raw[:colon], raw[colon+1:] + if base == "" || suffix == "" { + return raw, 0, 0 + } + start, count, ok := parseReadSelector(suffix) + if !ok { + return raw, 0, 0 + } + return base, start, count +} + +// parseReadSelector validates the full selector tail (everything after the +// first ':' in the final segment). Selector parts are joined by ':' so combos +// such as "2-4:raw" and "raw:2-4" are accepted; each part must be a line-spec +// list or a recognized mode keyword. The first line-spec encountered drives the +// returned startLine/lineCount. +func parseReadSelector(suffix string) (startLine, lineCount int, ok bool) { + gotLine := false + for _, part := range strings.Split(suffix, ":") { + if part == "raw" || part == "conflicts" { + continue + } + s, c, valid := parseLineSpecList(part) + if !valid { + return 0, 0, false + } + if !gotLine { + startLine, lineCount, gotLine = s, c, true + } + } + return startLine, lineCount, true +} + +// parseLineSpecList parses a comma-separated list of line specs ("5-16,960-973") +// and reports the start/count of the first spec. +func parseLineSpecList(part string) (startLine, lineCount int, ok bool) { + segs := strings.Split(part, ",") + for i, seg := range segs { + s, c, valid := parseLineSpec(seg) + if !valid { + return 0, 0, false + } + if i == 0 { + startLine, lineCount = s, c + } + } + return startLine, lineCount, true +} + +// parseLineSpec parses a single line spec: "N", "N-", "N-M", or "N+K". +func parseLineSpec(seg string) (startLine, lineCount int, ok bool) { + if i := strings.IndexByte(seg, '+'); i >= 0 { + start, errA := strconv.Atoi(seg[:i]) + count, errB := strconv.Atoi(seg[i+1:]) + if errA != nil || errB != nil || start <= 0 || count < 0 { + return 0, 0, false + } + return start, count, true + } + if i := strings.IndexByte(seg, '-'); i >= 0 { + start, errA := strconv.Atoi(seg[:i]) + if errA != nil || start <= 0 { + return 0, 0, false + } + rest := seg[i+1:] + if rest == "" { // "N-" — open-ended + return start, 0, true + } + end, errB := strconv.Atoi(rest) + if errB != nil || end < start { + return 0, 0, false + } + return start, end - start + 1, true + } + start, err := strconv.Atoi(seg) + if err != nil || start <= 0 { + return 0, 0, false + } + return start, 0, true +} diff --git a/apps/backend/internal/common/readselector/readselector_test.go b/apps/backend/internal/common/readselector/readselector_test.go new file mode 100644 index 0000000000..f3cb221b3e --- /dev/null +++ b/apps/backend/internal/common/readselector/readselector_test.go @@ -0,0 +1,81 @@ +package readselector + +import "testing" + +func TestSplit(t *testing.T) { + tests := []struct { + name string + raw string + wantPath string + wantStart int + wantCount int + }{ + {"no selector", "config.json", "config.json", 0, 0}, + {"single line", "apps/web/lib/utils.ts:50", "apps/web/lib/utils.ts", 50, 0}, + {"open ended", "main.go:50-", "main.go", 50, 0}, + {"closed range", "apps/backend/internal/sentry/handlers.go:43-94", "apps/backend/internal/sentry/handlers.go", 43, 52}, + {"plus span", "main.go:50+150", "main.go", 50, 150}, + {"anchor", "main.go:20+1", "main.go", 20, 1}, + {"multi range", "main.go:5-16,960-973", "main.go", 5, 12}, + {"raw mode", "main.go:raw", "main.go", 0, 0}, + {"conflicts mode", "main.go:conflicts", "main.go", 0, 0}, + {"range then raw", "main.go:2-4:raw", "main.go", 2, 3}, + {"raw then range", "main.go:raw:2-4", "main.go", 2, 3}, + {"absolute path", "/home/u/.kandev/x/handlers.go:43-94", "/home/u/.kandev/x/handlers.go", 43, 52}, + {"absolute external", "/home/clem/.claude/CLAUDE.md:93-113", "/home/clem/.claude/CLAUDE.md", 93, 21}, + {"no extension single line", "Makefile:10", "Makefile", 10, 0}, + // Real OMP reads observed in the wild (multi-range, zero-length range, + // range+mode combo, and a "~"-prefixed home path). + {"omp multi range zero-len", "x/README.md:16-20,32-40,69-69,85-96", "x/README.md", 16, 5}, + {"omp range plus raw combo", "x/README.md:69+1:raw", "x/README.md", 69, 1}, + {"omp tilde multi range", "~/.kandev/x/README.md:27-28,35-36,66-66", "~/.kandev/x/README.md", 27, 2}, + // Windows path: drive-letter colon must not be read as the selector + // boundary; the trailing "\\" segment + ":43-94" selector is stripped. + {"windows absolute", `C:\Users\me\handlers.go:43-94`, `C:\Users\me\handlers.go`, 43, 52}, + // "N+0" is intentionally accepted (count 0, anchor at N) and stripped, so + // the link still opens; rejecting it would leave the selector on the path. + {"plus zero anchors", "main.go:50+0", "main.go", 50, 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path, start, count := Split(tt.raw) + if path != tt.wantPath { + t.Errorf("path = %q, want %q", path, tt.wantPath) + } + if start != tt.wantStart { + t.Errorf("startLine = %d, want %d", start, tt.wantStart) + } + if count != tt.wantCount { + t.Errorf("lineCount = %d, want %d", count, tt.wantCount) + } + }) + } +} + +// TestSplit_NoFalsePositives verifies that paths whose colon suffix is not a +// valid omp selector are returned untouched, so legitimate filenames and +// non-omp agent paths never lose data. +func TestSplit_NoFalsePositives(t *testing.T) { + unchanged := []string{ + "", + "src/file.ts", + "foo.go:bar", // non-numeric, non-keyword suffix + "foo.go:1.2", // float-like, not a line spec + `C:\Users\me\a.go`, // windows drive letter: suffix is not a line spec + "dir:1/file.go", // colon lives in a directory component, not the file + ":43-94", // empty base + "main.go:", // empty selector + "main.go:-5", // negative / malformed start + "main.go:0", // zero is not a valid 1-based line + "main.go:10-5", // descending range + "main.go:5,abc", // one invalid segment invalidates the list + } + for _, raw := range unchanged { + t.Run(raw, func(t *testing.T) { + path, start, count := Split(raw) + if path != raw || start != 0 || count != 0 { + t.Errorf("Split(%q) = (%q, %d, %d), want (%q, 0, 0)", raw, path, start, count, raw) + } + }) + } +} diff --git a/apps/web/components/shared/markdown-components.test.tsx b/apps/web/components/shared/markdown-components.test.tsx index 91c33be6d9..d0b0b0dac0 100644 --- a/apps/web/components/shared/markdown-components.test.tsx +++ b/apps/web/components/shared/markdown-components.test.tsx @@ -198,6 +198,29 @@ describe("markdownComponents", () => { }); }); +describe("markdownComponents omp read selectors", () => { + afterEach(() => { + cleanup(); + openFile.mockClear(); + appState.value.tasks.activeSessionId = "session-1"; + appState.value.taskSessions.items = { + "session-1": { worktree_path: "/root/.kandev/tasks/example/kandev" }, + }; + }); + + it("opens file links with omp range and multi-range selectors in the editor", () => { + render({"[readme](docs/README.md:16-20)"}); + fireEvent.click(screen.getByRole("link", { name: "readme" })); + expect(openFile).toHaveBeenCalledWith("docs/README.md"); + }); + + it("opens file links with omp multi-range and mode selectors in the editor", () => { + render({"[readme](docs/README.md:5-16,960-973:raw)"}); + fireEvent.click(screen.getByRole("link", { name: "readme" })); + expect(openFile).toHaveBeenCalledWith("docs/README.md"); + }); +}); + describe("normalizeMarkdown", () => { it("splits a glued 4-backtick close", () => { const input = "````go\nfunc f() {\n ...\n}````\nprose continues here"; diff --git a/apps/web/components/shared/markdown-components.tsx b/apps/web/components/shared/markdown-components.tsx index bd6ffbb486..55b27aa87f 100644 --- a/apps/web/components/shared/markdown-components.tsx +++ b/apps/web/components/shared/markdown-components.tsx @@ -80,8 +80,12 @@ function stripHashAndQuery(href: string): string { return href.split(/[?#]/, 1)[0] ?? ""; } +// Strip a trailing source-location / omp read selector so a file link resolves +// to the bare path: ":42", ":42:5", and omp ranges like ":16-20", ":50+150", +// ":5-16,960-973", ":2-4:raw", ":raw", ":conflicts". function stripSourceLocationSuffix(path: string): string { - return path.replace(/:\d+(?::\d+)?$/, ""); + const part = String.raw`\d+(?:[-+]\d+)?(?:,\d+(?:[-+]\d+)?)*|raw|conflicts`; + return path.replace(new RegExp(`:(?:${part})(?::(?:${part}))*$`), ""); } function decodeHrefPath(href: string): string | null { diff --git a/apps/web/components/task/chat/messages/tool-edit-message.tsx b/apps/web/components/task/chat/messages/tool-edit-message.tsx index 9c1263788d..70b39bc41b 100644 --- a/apps/web/components/task/chat/messages/tool-edit-message.tsx +++ b/apps/web/components/task/chat/messages/tool-edit-message.tsx @@ -1,6 +1,6 @@ "use client"; -import { useState, memo } from "react"; +import { useState, memo, useCallback } from "react"; import { IconCheck, IconX, @@ -16,6 +16,7 @@ import { DiffViewBlock } from "./diff-view-block"; import { ExpandableRow } from "./expandable-row"; import { transformFileMutation, type FileMutation } from "@/lib/diff"; import { useExpandState } from "./use-expand-state"; +import { setPendingCursorPosition, scrollEditorIfMounted } from "@/hooks/use-file-editors"; type ModifyFilePayload = { file_path?: string; @@ -34,6 +35,7 @@ type ToolEditMessageProps = { onOpenFile?: (path: string) => void; }; +// EditStatusIcon renders the status glyph (complete/error/running) for an edit card. function EditStatusIcon({ status }: { status: string | undefined }) { if (status === "complete") return ; if (status === "error") return ; @@ -41,6 +43,7 @@ function EditStatusIcon({ status }: { status: string | undefined }) { return null; } +// getEditSummary returns the short header label for an edit/write card. function getEditSummary( content: string, worktreePath: string | undefined, @@ -62,6 +65,7 @@ type FileActionButtonProps = { onCopyPath: (e: React.MouseEvent) => void; }; +// FileActionButton renders the file path with an optional open-in-editor action. function FileActionButton({ filePath, worktreePath, @@ -109,6 +113,7 @@ type EditExpandedContentProps = { writeContent: string | undefined; }; +// EditExpandedContent renders the expanded body of an edit card: a diff or write content. function EditExpandedContent({ diffData, writeContent }: EditExpandedContentProps) { if (diffData?.diff) return ; if (writeContent) { @@ -123,6 +128,8 @@ function EditExpandedContent({ diffData, writeContent }: EditExpandedContentProp return null; } +// parseEditMetadata pulls the edit status, file path, first-changed line, +// diff/write content, and expandability out of a tool_edit message's metadata. function parseEditMetadata(comment: Message) { const metadata = comment.metadata as ToolEditMetadata | undefined; const status = metadata?.status; @@ -138,6 +145,7 @@ function parseEditMetadata(comment: Message) { return { status, filePath, + startLine: mutation?.start_line, writeContent, isWriteOperation, diffData, @@ -147,6 +155,7 @@ function parseEditMetadata(comment: Message) { }; } +// ToolEditMessage renders an edit/write tool call: the file link and expandable diff. export const ToolEditMessage = memo(function ToolEditMessage({ comment, worktreePath, @@ -155,6 +164,7 @@ export const ToolEditMessage = memo(function ToolEditMessage({ const { status, filePath, + startLine, writeContent, isWriteOperation, diffData, @@ -177,6 +187,22 @@ export const ToolEditMessage = memo(function ToolEditMessage({ } }; + // Navigate the editor to the first changed line. For a newly opened file the + // pending position is consumed on editor mount; for an already-open file no + // mount happens, so scroll the live editor directly. + const handleOpenFile = useCallback( + (path: string) => { + if (startLine && startLine > 0) { + setPendingCursorPosition(path, startLine, 1); + onOpenFile?.(path); + scrollEditorIfMounted(path, worktreePath ?? null, startLine, 1); + return; + } + onOpenFile?.(path); + }, + [onOpenFile, startLine, worktreePath], + ); + return ( } @@ -190,7 +216,7 @@ export const ToolEditMessage = memo(function ToolEditMessage({ diff --git a/apps/web/components/task/chat/messages/tool-read-message.tsx b/apps/web/components/task/chat/messages/tool-read-message.tsx index 7e10415341..ad6466f774 100644 --- a/apps/web/components/task/chat/messages/tool-read-message.tsx +++ b/apps/web/components/task/chat/messages/tool-read-message.tsx @@ -1,12 +1,13 @@ "use client"; -import { memo } from "react"; +import { memo, useCallback } from "react"; import { IconCheck, IconX, IconFileCode2 } from "@tabler/icons-react"; import { GridSpinner } from "@/components/grid-spinner"; import { FilePathButton } from "./file-path-button"; import type { Message } from "@/lib/types/http"; import { ExpandableRow } from "./expandable-row"; import { useExpandState } from "./use-expand-state"; +import { setPendingCursorPosition, scrollEditorIfMounted } from "@/hooks/use-file-editors"; type ReadFileOutput = { content?: string; @@ -36,6 +37,7 @@ type ToolReadMessageProps = { onOpenFile?: (path: string) => void; }; +// ReadStatusIcon renders the status glyph (complete/error/running) for a read card. function ReadStatusIcon({ status }: { status: string | undefined }) { if (status === "complete") return ; if (status === "error") return ; @@ -43,30 +45,62 @@ function ReadStatusIcon({ status }: { status: string | undefined }) { return null; } +// getReadSummary returns the short "Read N lines" header label for the card. function getReadSummary(lineCount: number | undefined): string { if (lineCount) return `Read ${lineCount} line${lineCount !== 1 ? "s" : ""}`; return "Read"; } +// formatLineRange renders the range the agent read (carried separately from +// the file path so the link stays openable). offset is the 1-based start line; +// limit is the line count (0 when open-ended / a single anchor). +function formatLineRange(offset: number | undefined, limit: number | undefined): string { + if (!offset || offset <= 0) return ""; + if (limit && limit > 0) return `:${offset}-${offset + limit - 1}`; + return `:${offset}`; +} + +// parseReadMetadata pulls the read status, file path, line range, and output +// out of a tool_read message's normalized metadata for rendering. function parseReadMetadata(comment: Message) { const metadata = comment.metadata as ToolReadMetadata | undefined; const status = metadata?.status; const readFile = metadata?.normalized?.read_file; const readOutput = readFile?.output; const filePath = readFile?.file_path; + const startLine = readFile?.offset; + const lineRange = formatLineRange(readFile?.offset, readFile?.limit); const hasOutput = !!readOutput?.content; const isSuccess = status === "complete"; - return { status, readOutput, filePath, hasOutput, isSuccess }; + return { status, readOutput, filePath, startLine, lineRange, hasOutput, isSuccess }; } +// ToolReadMessage renders a read tool call: the file link, line-range badge, and +// the (expandable) read output. export const ToolReadMessage = memo(function ToolReadMessage({ comment, worktreePath, onOpenFile, }: ToolReadMessageProps) { - const { status, readOutput, filePath, hasOutput, isSuccess } = parseReadMetadata(comment); + const { status, readOutput, filePath, startLine, lineRange, hasOutput, isSuccess } = + parseReadMetadata(comment); const autoExpanded = status === "running"; const { isExpanded, handleToggle } = useExpandState(status, autoExpanded); + // Navigate the editor to the line the agent read (offset). For a newly opened + // file the pending position is consumed on editor mount; for an already-open + // file no mount happens, so scroll the live editor directly. + const handleOpenFile = useCallback( + (path: string) => { + if (startLine && startLine > 0) { + setPendingCursorPosition(path, startLine, 1); + onOpenFile?.(path); + scrollEditorIfMounted(path, worktreePath ?? null, startLine, 1); + return; + } + onOpenFile?.(path); + }, + [onOpenFile, startLine, worktreePath], + ); return ( } {filePath && ( - + + {lineRange && ( + + {lineRange} + + )} )} {readOutput?.truncated && ( diff --git a/apps/web/components/task/file-viewer-content.tsx b/apps/web/components/task/file-viewer-content.tsx index 847f8dd279..01a98d77e2 100644 --- a/apps/web/components/task/file-viewer-content.tsx +++ b/apps/web/components/task/file-viewer-content.tsx @@ -1,10 +1,12 @@ "use client"; +import { useCallback, useEffect, useRef } from "react"; import CodeMirror from "@uiw/react-codemirror"; import { vscodeDark } from "@uiw/codemirror-theme-vscode"; import { EditorView } from "@codemirror/view"; import type { Extension } from "@codemirror/state"; import { getCodeMirrorExtensionFromPath } from "@/lib/languages"; +import { consumePendingCursorPosition } from "@/hooks/use-file-editors"; import { cn } from "@/lib/utils"; type FileViewerContentProps = { @@ -20,6 +22,43 @@ export function FileViewerContent({ path, content, className }: FileViewerConten extensions.push(langExt); } + const viewRef = useRef(null); + + // Consume a pending cursor position (set by chat read/edit file links before + // they open the file) and scroll CodeMirror so the target 1-based line is + // centered. No pending entry → no scroll, so normal file browsing stays at + // the top. Desktop's Monaco consumes the same entry; on mobile there is no + // Monaco, so this CodeMirror viewer must consume it instead. + const scrollToPendingLine = useCallback((view: EditorView, filePath: string) => { + const pending = consumePendingCursorPosition(filePath); + if (!pending) return; + const totalLines = view.state.doc.lines; + const clampedLine = Math.min(Math.max(pending.line, 1), totalLines); + const pos = view.state.doc.line(clampedLine).from; + view.dispatch({ + selection: { anchor: pos }, + effects: EditorView.scrollIntoView(pos, { y: "center" }), + }); + }, []); + + const handleCreateEditor = useCallback( + (view: EditorView) => { + viewRef.current = view; + scrollToPendingLine(view, path); + }, + [path, scrollToPendingLine], + ); + + // The same FileViewerContent instance is reused when switching files (the + // path prop changes without a remount), and a pending entry may be set after + // the editor already mounted. Re-consume + scroll whenever path changes and + // the EditorView exists, so the second open of a different file still jumps. + useEffect(() => { + if (viewRef.current) { + scrollToPendingLine(viewRef.current, path); + } + }, [path, scrollToPendingLine]); + return ( { // Header must show full relative path, not just the basename. await expect(viewer.getByText(filePath)).toBeVisible(); }); + + // A chat read/edit file link can target a line deep in a file. Tapping it sets + // a pending cursor position (use-file-editors), then on mobile the file opens + // through MobileFileViewerPanel → FileViewerContent (CodeMirror), which must + // consume that pending entry and scroll the target line into view. Regression + // guard for the mobile half of "open-and-scroll-to-line": before the fix + // CodeMirror ignored the pending map and the file opened pinned at the top. + test("tapping a chat read link scrolls the CodeMirror viewer to the target line", async ({ + testPage, + apiClient, + seedData, + backend, + }) => { + test.setTimeout(90_000); + + const fileName = `chat-read-${Date.now()}-${Math.random().toString(36).slice(2, 8)}.ts`; + const filePath = fileName; + const git = new GitHelper( + path.join(backend.tmpDir, "repos", "e2e-repo"), + makeGitEnv(backend.tmpDir), + ); + git.createFile(filePath, createLongFileContent(500)); + git.stageAll(); + git.commit(`add ${filePath}`); + + const profile = await createStandardProfile(apiClient, `mobile-fv-scroll-${Date.now()}`); + // `e2e:message(...)` emits a single non-activity text message — no thinking + // and no tool calls — so the seeded read card below is the only activity + // message and renders as a standalone, directly tappable card (a turn group + // only forms with ≥2 activity messages in the same turn). + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Mobile FV Scroll To Line", + profile.id, + { + description: 'e2e:message("ready")', + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id"); + + // Seed a real read card pointing deep into the file. The chat read-message + // component sets the pending cursor position from `offset` before opening, + // keyed by this exact `file_path` — the same string the mobile viewer keys + // FileViewerContent on, so the consume matches. + const targetLine = 300; + await apiClient.seedSessionMessage(task.session_id, { + type: "tool_read", + content: "Read file", + metadata: { + status: "complete", + tool_call_id: "tc-chat-read-scroll", + normalized: { + read_file: { + file_path: filePath, + offset: targetLine, + limit: 20, + output: { content: 'export const line_299 = "line 299";', line_count: 20 }, + }, + }, + }, + }); + + await testPage.goto(`/t/${task.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await session.waitForChatIdle({ timeout: 45_000 }); + + // FilePathButton renders the openable link with the seeded path as `title`. + const chat = session.activeChat(); + const fileLink = chat.locator(`button[title="${filePath}"]`); + await expect(fileLink).toBeVisible({ timeout: 15_000 }); + await fileLink.tap(); + + const viewer = testPage.getByTestId("mobile-file-viewer-panel"); + await expect(viewer).toBeVisible({ timeout: 10_000 }); + + const cmScroller = viewer.locator(".cm-scroller").first(); + await expect(cmScroller).toBeVisible(); + + // FileViewerContent consumed the pending position on mount and scrolled the + // target line to centre — the scroller is no longer pinned at the top. + await expect + .poll(async () => cmScroller.evaluate((element) => element.scrollTop), { timeout: 10_000 }) + .toBeGreaterThan(0); + + // The target line's gutter number is rendered and on-screen, proving the + // scroll landed on the requested line rather than an arbitrary offset. + await expect( + viewer.locator(".cm-lineNumbers .cm-gutterElement", { hasText: "300" }), + ).toBeInViewport(); + }); + + // Opening a file with NO pending cursor position (normal file browsing) must + // not scroll — the CodeMirror viewer stays pinned at the top. + test("opening a file without a pending cursor position stays at the top", async ({ + testPage, + apiClient, + seedData, + backend, + }) => { + test.setTimeout(90_000); + + const { filePath } = await setupMobileFileViewerTest({ + testPage, + apiClient, + seedData, + backend, + taskTitle: "Mobile FV No Scroll", + options: { extension: "ts", content: createLongFileContent(500) }, + }); + + await testPage.getByRole("button", { name: "Files" }).tap(); + const fileNode = testPage.locator(`[data-testid="file-tree-node"][data-path="${filePath}"]`); + await expect(fileNode).toBeVisible({ timeout: 15_000 }); + await fileNode.tap(); + + const viewer = testPage.getByTestId("mobile-file-viewer-panel"); + await expect(viewer).toBeVisible({ timeout: 5_000 }); + + const cmScroller = viewer.locator(".cm-scroller").first(); + await expect(cmScroller).toBeVisible(); + // No pending entry → no scroll-to-line dispatch → scroller stays at the top. + expect(await cmScroller.evaluate((element) => element.scrollTop)).toBe(0); + }); }); diff --git a/apps/web/hooks/use-file-editors.ts b/apps/web/hooks/use-file-editors.ts index 77925e3454..0841ff460c 100644 --- a/apps/web/hooks/use-file-editors.ts +++ b/apps/web/hooks/use-file-editors.ts @@ -18,6 +18,7 @@ import type { FileContentResponse } from "@/lib/types/backend"; import { useSaveDeleteActions } from "./use-file-save-delete"; import { PREVIEW_FILE_EDITOR_ID } from "@/lib/state/dockview-panel-actions"; import { useOpenFileWorkspaceSync } from "./file-editors-sync"; +import { getMonacoInstance } from "@/components/editors/monaco/monaco-init"; // Module-level guard: ensures restoration only runs once across all hook instances let _restoredSessionId: string | null = null; @@ -39,6 +40,42 @@ export function consumePendingCursorPosition( return pos; } +/** + * Scroll an already-mounted Monaco editor for `path` to the given position. + * Returns true if an editor was found and scrolled. The editor's model URI is + * built as `${worktreePath}/${path}` (see use-monaco-editor-lsp), so the same + * `path` used for setPendingCursorPosition resolves the right editor here. + * + * Used for the already-open case: when a file link is clicked but its tab is + * already open, no editor mounts, so handleEditorDidMount never consumes the + * pending position — this scrolls the live editor instead (and consumes the + * pending entry so a later remount doesn't double-apply it). + */ +export function scrollEditorIfMounted( + path: string, + worktreePath: string | null, + line: number, + column: number, +): boolean { + const monaco = getMonacoInstance(); + if (!monaco) return false; + + const monacoPath = worktreePath ? `${worktreePath}/${path}` : path; + for (const editor of monaco.editor.getEditors()) { + const model = editor.getModel(); + if (!model) continue; + const modelPath = model.uri.path; + if (modelPath === `/${monacoPath}` || modelPath === monacoPath) { + consumePendingCursorPosition(path); + editor.setPosition({ lineNumber: line, column }); + editor.revealLineInCenter(line); + editor.focus(); + return true; + } + } + return false; +} + /** Read openFiles from the store without subscribing to changes. */ function getOpenFiles() { return useDockviewStore.getState().openFiles; diff --git a/apps/web/hooks/use-lsp-file-opener.ts b/apps/web/hooks/use-lsp-file-opener.ts index b008bda146..d44d7d0841 100644 --- a/apps/web/hooks/use-lsp-file-opener.ts +++ b/apps/web/hooks/use-lsp-file-opener.ts @@ -5,42 +5,9 @@ import { useAppStore } from "@/components/state-provider"; import { useFileEditors, setPendingCursorPosition, - consumePendingCursorPosition, + scrollEditorIfMounted, } from "@/hooks/use-file-editors"; import { lspClientManager } from "@/lib/lsp/lsp-client-manager"; -import { getMonacoInstance } from "@/components/editors/monaco/monaco-init"; - -/** - * Try to scroll an already-mounted editor to the given position. - * Returns true if the editor was found and scrolled, false otherwise. - * If successful, consumes the pending cursor position so handleEditorDidMount - * doesn't double-apply it. - */ -function scrollEditorIfMounted( - relativePath: string, - worktreePath: string | null, - line: number, - column: number, -): boolean { - const monaco = getMonacoInstance(); - if (!monaco) return false; - - const monacoPath = worktreePath ? `${worktreePath}/${relativePath}` : relativePath; - - for (const editor of monaco.editor.getEditors()) { - const model = editor.getModel(); - if (!model) continue; - const modelPath = model.uri.path; - if (modelPath === `/${monacoPath}` || modelPath === monacoPath) { - consumePendingCursorPosition(relativePath); - editor.setPosition({ lineNumber: line, column }); - editor.revealLineInCenter(line); - editor.focus(); - return true; - } - } - return false; -} /** * Connects LSP Go-to-Definition / Find References navigation to dockview file tabs.