From ad986008af86a373790f240edaa914ce71dcd6e3 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Sat, 8 Aug 2026 09:17:55 +0700 Subject: [PATCH 1/9] Fix dashboard hangs from MCP deadlock and long reasoning streams MCP stdio held a lock for the full RPC duration, so Close/Refresh blocked when IDA or another backend hung, leaving zombie children and freezing the web UI. Chat also re-mapped the entire transcript every frame and grew unbounded reasoning strings during high-effort turns. Release the send lock while waiting for responses, kill+Wait on Close, batch stream patches by message only, and make live reasoning display configurable via display.show_reasoning and display.max_live_reasoning_chars. --- internal/config/config.go | 16 ++- internal/config/defaults.go | 5 +- internal/config/load.go | 4 + internal/config/schema.go | 8 +- internal/mcp/client.go | 64 ++++++++++-- web/src/components/chat/SubAgentPanel.tsx | 70 +++++++++++-- web/src/pages/ChatPage.tsx | 120 ++++++++++++++++++---- 7 files changed, 244 insertions(+), 43 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index e641a7f..3681f0b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -811,11 +811,17 @@ type Display struct { Compact bool `yaml:"compact" json:"compact"` ToolProgress bool `yaml:"tool_progress" json:"tool_progress"` ShowReasoning bool `yaml:"show_reasoning" json:"show_reasoning"` - Theme string `yaml:"theme" json:"theme"` - Skin string `yaml:"skin" json:"skin"` - Language string `yaml:"language" json:"language"` - BellOnComplete bool `yaml:"bell_on_complete" json:"bell_on_complete"` - InterimAssistant bool `yaml:"interim_assistant_messages" json:"interim_assistant_messages"` + // MaxLiveReasoningChars caps how much of a streaming reasoning trace the + // dashboard keeps in React state (trailing window). High-effort models can + // emit hundreds of KB per turn; unbounded string growth freezes the tab. + // 0 means unlimited. The full text is still persisted server-side and is + // restored on hydrate after the turn completes. + MaxLiveReasoningChars int `yaml:"max_live_reasoning_chars" json:"max_live_reasoning_chars"` + Theme string `yaml:"theme" json:"theme"` + Skin string `yaml:"skin" json:"skin"` + Language string `yaml:"language" json:"language"` + BellOnComplete bool `yaml:"bell_on_complete" json:"bell_on_complete"` + InterimAssistant bool `yaml:"interim_assistant_messages" json:"interim_assistant_messages"` } // Logging controls log level and sinks. diff --git a/internal/config/defaults.go b/internal/config/defaults.go index c2b6ce6..7896508 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -127,8 +127,9 @@ func Default() *Config { SessionReset: SessionReset{Mode: "never", IdleMinutes: 180, AtHour: 4}, Streaming: Streaming{Enabled: true}, Display: Display{ - ToolProgress: true, ShowReasoning: true, Theme: "system", - Skin: "antares", Language: "auto", InterimAssistant: true, + ToolProgress: true, ShowReasoning: true, + MaxLiveReasoningChars: 48_000, + Theme: "system", Skin: "antares", Language: "auto", InterimAssistant: true, }, Logging: Logging{Level: "info", File: filepath.Join(Home(), "logs", "antares.log")}, MCP: MCP{Enabled: true, Servers: map[string]MCPServer{}}, diff --git a/internal/config/load.go b/internal/config/load.go index 697a5fb..e70996d 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -246,6 +246,10 @@ func normalize(c *Config) { if c.Agent.MaxTurns <= 0 { c.Agent.MaxTurns = 200 } + // Negative is meaningless; treat as default cap. 0 stays unlimited. + if c.Display.MaxLiveReasoningChars < 0 { + c.Display.MaxLiveReasoningChars = 48_000 + } } // ResolveProvider returns the provider entry used for a model call, falling back diff --git a/internal/config/schema.go b/internal/config/schema.go index cec2ce6..4c10d88 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -79,8 +79,9 @@ var common = map[string]bool{ "compression.enabled": true, "streaming.enabled": true, "delegation.enabled": true, - "display.show_reasoning": true, - "display.tool_progress": true, + "display.show_reasoning": true, + "display.tool_progress": true, + "display.max_live_reasoning_chars": true, "logging.level": true, "server.host": true, } @@ -138,6 +139,9 @@ var help = map[string]string{ "memory.memory_enabled": "Lets the agent store durable facts between sessions.", "skills.auto_create": "Allows the agent to write new skills on its own.", "osint.google_cookie": "Optional. A logged-in Google Cookie header enables osint_google to resolve an email to its public profile. ToS-sensitive; uses your own session. Leave empty to disable.", + "display.show_reasoning": "Stream and show model reasoning/thinking in the dashboard (and TUI). Off skips emitting reasoning events so long thinking traces never hit the UI.", + "display.tool_progress": "Show live tool progress lines while a tool runs.", + "display.max_live_reasoning_chars": "Max characters of reasoning kept in the browser while a turn streams (trailing window). Prevents tab freezes on long thinking. Default 48000. 0 = unlimited. Full text is still saved server-side and restored after the turn.", } func secretKey(path string) bool { diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 0530c85..8275ef4 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -351,13 +351,26 @@ func (c *Client) Close() error { return c.transport.Close() } // stdioTransport runs the server as a child process and exchanges // newline-delimited JSON-RPC frames over its pipes. +// +// Concurrency model: +// - sendMu serialises request/response pairs (stdio MCP is half-duplex). +// - mu protects closed/stdin so Close can mark the transport dead and kill the +// child without waiting for a hung RPC to finish writing. +// +// Previously send held one mutex for the entire RPC duration. If the child hung +// (e.g. IDA Pro MCP waiting on a dead IDA RPC port), Close/Refresh blocked +// forever on that lock — the dashboard MCP page and any reload path appeared to +// hang. Close must be able to kill the process so the blocked read returns EOF. type stdioTransport struct { cmd *exec.Cmd stdin io.WriteCloser stdout *bufio.Reader - mu sync.Mutex + sendMu sync.Mutex // serialises send/notify request cycles + mu sync.Mutex // protects closed + stdin write against Close closed bool + waitOnce sync.Once + waitErr error } func newStdioTransport(cfg ServerConfig) (transport, error) { @@ -393,20 +406,41 @@ func newStdioTransport(cfg ServerConfig) (transport, error) { } }() - return &stdioTransport{cmd: cmd, stdin: stdin, stdout: bufio.NewReaderSize(stdout, 1<<20)}, nil + t := &stdioTransport{cmd: cmd, stdin: stdin, stdout: bufio.NewReaderSize(stdout, 1<<20)} + // Reap the child if it exits on its own so it never sits as a zombie until + // the next Close/Refresh. Wait is idempotent via waitOnce. + go func() { _ = t.reap() }() + return t, nil +} + +func (t *stdioTransport) reap() error { + t.waitOnce.Do(func() { + if t.cmd != nil { + t.waitErr = t.cmd.Wait() + } + }) + return t.waitErr } func (t *stdioTransport) send(ctx context.Context, req rpcRequest) (*rpcResponse, error) { + // One in-flight request at a time — required for line-delimited stdio. + t.sendMu.Lock() + defer t.sendMu.Unlock() + t.mu.Lock() - defer t.mu.Unlock() if t.closed { + t.mu.Unlock() return nil, fmt.Errorf("mcp connection closed") } - if err := t.writeFrame(req); err != nil { + err := t.writeFrame(req) + t.mu.Unlock() + if err != nil { return nil, err } // Skip any notification the server interleaves before our response. + // Do NOT hold mu while waiting: Close must be able to kill the child so a + // hung tools/call (dead IDA backend, wedged proxy, …) unblocks. type result struct { resp *rpcResponse err error @@ -437,6 +471,9 @@ func (t *stdioTransport) send(ctx context.Context, req rpcRequest) (*rpcResponse select { case <-ctx.Done(): + // Tear down the child so the reader goroutine unblocks on EOF and the + // next send cannot talk to a half-dead process with a stolen reply. + _ = t.Close() return nil, ctx.Err() case r := <-ch: return r.resp, r.err @@ -444,8 +481,13 @@ func (t *stdioTransport) send(ctx context.Context, req rpcRequest) (*rpcResponse } func (t *stdioTransport) notify(_ context.Context, req rpcRequest) error { + t.sendMu.Lock() + defer t.sendMu.Unlock() t.mu.Lock() defer t.mu.Unlock() + if t.closed { + return fmt.Errorf("mcp connection closed") + } return t.writeFrame(req) } @@ -462,16 +504,20 @@ func (t *stdioTransport) writeFrame(req rpcRequest) error { func (t *stdioTransport) Close() error { t.mu.Lock() - defer t.mu.Unlock() if t.closed { - return nil + t.mu.Unlock() + return t.reap() } t.closed = true _ = t.stdin.Close() - if t.cmd.Process != nil { - _ = t.cmd.Process.Kill() + proc := t.cmd.Process + t.mu.Unlock() + + if proc != nil { + _ = proc.Kill() } - return nil + // Always Wait so the child does not linger as a zombie under antares. + return t.reap() } // ---- http transport ---------------------------------------------------------- diff --git a/web/src/components/chat/SubAgentPanel.tsx b/web/src/components/chat/SubAgentPanel.tsx index ac48c42..1a7bd51 100644 --- a/web/src/components/chat/SubAgentPanel.tsx +++ b/web/src/components/chat/SubAgentPanel.tsx @@ -1,8 +1,9 @@ import { useEffect, useRef, useState } from 'react' import { ArrowLeft, CircleNotch, UsersThree } from '@phosphor-icons/react' -import { streamGet, type StreamEvent } from '@/lib/api' +import { get, streamGet, type StreamEvent } from '@/lib/api' import { useI18n } from '@/lib/i18n' import { + DEFAULT_MAX_LIVE_REASONING_CHARS, MessageBubble, appendSeg, pushToolSeg, @@ -28,8 +29,30 @@ export function SubAgentPanel({ agent, onBack }: { agent: ActiveAgent; onBack: ( const { t } = useI18n() const [messages, setMessages] = useState([]) const [done, setDone] = useState(false) + const [showReasoning, setShowReasoning] = useState(true) + const showReasoningRef = useRef(true) + const maxLiveReasoningRef = useRef(DEFAULT_MAX_LIVE_REASONING_CHARS) const bottomRef = useRef(null) + useEffect(() => { + showReasoningRef.current = showReasoning + }, [showReasoning]) + + useEffect(() => { + get<{ + values?: { display?: { show_reasoning?: boolean; max_live_reasoning_chars?: number } } + }>('/config') + .then((d) => { + const disp = d.values?.display + if (disp && typeof disp.show_reasoning === 'boolean') setShowReasoning(disp.show_reasoning) + const n = Number(disp?.max_live_reasoning_chars) + if (Number.isFinite(n)) { + maxLiveReasoningRef.current = n < 0 ? DEFAULT_MAX_LIVE_REASONING_CHARS : n + } + }) + .catch(() => {}) + }, []) + useEffect(() => { // Reset when switching between sub-agents. setMessages([]) @@ -39,18 +62,43 @@ export function SubAgentPanel({ agent, onBack }: { agent: ActiveAgent; onBack: ( // main transcript renders one streaming turn. setMessages([{ id, role: 'assistant', content: '' }]) - const patch = (fn: (m: ChatMessage) => ChatMessage) => - setMessages((prev) => prev.map((m) => (m.id === id ? fn(m) : m))) + // Batch stream patches once per frame — sub-agents stream the same dense + // reasoning token firehose as the main chat and used to setState per token. + let pending: ((m: ChatMessage) => ChatMessage) | null = null + let raf: number | null = null + const flush = () => { + raf = null + const fn = pending + pending = null + if (!fn) return + setMessages((prev) => { + const idx = prev.findIndex((m) => m.id === id) + if (idx < 0) return prev + const next = prev.slice() + next[idx] = fn(prev[idx]) + return next + }) + } + const patch = (fn: (m: ChatMessage) => ChatMessage) => { + const prev = pending + pending = prev ? (m) => fn(prev(m)) : fn + if (raf == null) raf = requestAnimationFrame(flush) + } const close = streamGet( `/subagent/${encodeURIComponent(agent.id)}/attach`, (event: StreamEvent) => { switch (event.type) { case 'text': - patch((m) => appendSeg(m, 'text', String(event.delta ?? ''))) + patch((m) => + appendSeg(m, 'text', String(event.delta ?? ''), maxLiveReasoningRef.current), + ) break case 'reasoning': - patch((m) => appendSeg(m, 'reasoning', String(event.delta ?? ''))) + if (!showReasoningRef.current) break + patch((m) => + appendSeg(m, 'reasoning', String(event.delta ?? ''), maxLiveReasoningRef.current), + ) break case 'tool_call': patch((m) => @@ -88,13 +136,21 @@ export function SubAgentPanel({ agent, onBack }: { agent: ActiveAgent; onBack: ( patch((m) => ({ ...m, error: String(event.error ?? '') })) break case 'done': + if (raf != null) { + cancelAnimationFrame(raf) + raf = null + } + if (pending) flush() setDone(true) break } }, () => setDone(true), ) - return close + return () => { + if (raf != null) cancelAnimationFrame(raf) + close() + } }, [agent.id]) // Keep the newest output in view as it streams. @@ -138,7 +194,7 @@ export function SubAgentPanel({ agent, onBack }: { agent: ActiveAgent; onBack: ( ) : null} {messages.map((m) => ( - + ))}
diff --git a/web/src/pages/ChatPage.tsx b/web/src/pages/ChatPage.tsx index 1b21177..12d08b3 100644 --- a/web/src/pages/ChatPage.tsx +++ b/web/src/pages/ChatPage.tsx @@ -84,21 +84,53 @@ export interface ChatMessage { docs?: { path: string; name: string }[] } +/** + * Default soft cap for live reasoning text in React state while a turn streams. + * Overridden by `display.max_live_reasoning_chars` from config (0 = unlimited). + * High-effort models can emit hundreds of KB of reasoning per turn; unbounded + * string growth freezes the dashboard main thread. Full text is still saved + * server-side and restored on attach `done` / hydrate. + */ +export const DEFAULT_MAX_LIVE_REASONING_CHARS = 48_000 + /** Append a text or reasoning delta, extending the last segment when it is the - * same kind so a streamed sentence stays one block. */ -export function appendSeg(m: ChatMessage, kind: 'text' | 'reasoning', delta: string): ChatMessage { - const segs = m.segments ? [...m.segments] : [] + * same kind so a streamed sentence stays one block. + * `maxLiveReasoningChars`: trailing-window cap; ≤0 means unlimited. */ +export function appendSeg( + m: ChatMessage, + kind: 'text' | 'reasoning', + delta: string, + maxLiveReasoningChars: number = DEFAULT_MAX_LIVE_REASONING_CHARS, +): ChatMessage { + const segs = m.segments ? m.segments.slice() : [] const last = segs[segs.length - 1] if (last && last.kind === kind) { segs[segs.length - 1] = { kind, text: last.text + delta } } else { segs.push({ kind, text: delta }) } + let content = m.content + let reasoning = m.reasoning + if (kind === 'text') { + content = m.content + delta + } else { + const next = (m.reasoning ?? '') + delta + const cap = maxLiveReasoningChars + // Keep a trailing window so the bubble stays usable and string growth is O(cap). + reasoning = cap > 0 && next.length > cap ? next.slice(next.length - cap) : next + const segLast = segs[segs.length - 1] + if (cap > 0 && segLast?.kind === 'reasoning' && segLast.text.length > cap) { + segs[segs.length - 1] = { + kind: 'reasoning', + text: segLast.text.slice(segLast.text.length - cap), + } + } + } return { ...m, segments: segs, - content: kind === 'text' ? m.content + delta : m.content, - reasoning: kind === 'reasoning' ? (m.reasoning ?? '') + delta : m.reasoning, + content, + reasoning, } } @@ -338,6 +370,36 @@ export default function ChatPage() { .then((d) => setCtxWindow((w) => w || Number(d.context_window ?? 0))) .catch(() => {}) }, []) + // display.* prefs from config: whether to show reasoning at all, and the + // live-stream character cap (trailing window). Defaults match server defaults. + const [showReasoning, setShowReasoning] = useState(true) + const showReasoningRef = useRef(true) + const maxLiveReasoningRef = useRef(DEFAULT_MAX_LIVE_REASONING_CHARS) + useEffect(() => { + showReasoningRef.current = showReasoning + }, [showReasoning]) + useEffect(() => { + get<{ + values?: { + display?: { + show_reasoning?: boolean + max_live_reasoning_chars?: number + } + } + }>('/config') + .then((d) => { + const disp = d.values?.display + if (disp && typeof disp.show_reasoning === 'boolean') { + setShowReasoning(disp.show_reasoning) + } + const n = Number(disp?.max_live_reasoning_chars) + if (Number.isFinite(n)) { + // 0 = unlimited; negative is normalized server-side to default. + maxLiveReasoningRef.current = n < 0 ? DEFAULT_MAX_LIVE_REASONING_CHARS : n + } + }) + .catch(() => {}) + }, []) // When set, an overlay shows this sub-agent's live transcript instead of the // main one; clearing it returns to the main agent. const [viewingAgent, setViewingAgent] = useState(null) @@ -444,20 +506,31 @@ export default function ChatPage() { if (queued.length === 0) return patchQueue.current = [] const byMessage = groupStreamPatches(queued) - setMessages((prev) => - prev.map((message) => { - const patches = byMessage.get(message.id) - if (!patches) return message - let next = message + // Only clone/replace messages that actually received patches. Mapping the + // entire transcript every frame re-renders hundreds of bubbles on long + // sessions and was a major source of dashboard freezes during long turns. + setMessages((prev) => { + if (byMessage.size === 0) return prev + let next = prev + let cloned = false + for (const [id, patches] of byMessage) { + const idx = next.findIndex((m) => m.id === id) + if (idx < 0) continue + let message = next[idx] for (const patch of patches) { - next = + message = patch.kind === 'delta' - ? appendSeg(next, patch.segment, patch.delta) - : patch.fn(next) + ? appendSeg(message, patch.segment, patch.delta, maxLiveReasoningRef.current) + : patch.fn(message) } - return next - }), - ) + if (!cloned) { + next = prev.slice() + cloned = true + } + next[idx] = message + } + return next + }) }, []) const enqueuePatch = useCallback( (id: string, fn: (m: ChatMessage) => ChatMessage) => { @@ -560,7 +633,11 @@ export default function ChatPage() { enqueueDelta(assistantId, 'text', String(event.delta ?? '')) break case 'reasoning': - enqueueDelta(assistantId, 'reasoning', String(event.delta ?? '')) + // Honour display.show_reasoning even if a stale event arrives (server + // also suppresses when false; this keeps the UI consistent). + if (showReasoningRef.current) { + enqueueDelta(assistantId, 'reasoning', String(event.delta ?? '')) + } break case 'tool_call': setLive((s) => ({ @@ -1462,6 +1539,7 @@ export default function ChatPage() {
0 ? message.segments.map((seg, i) => { if (seg.kind === 'reasoning') { + if (!showReasoning) return null return } if (seg.kind === 'tool') { @@ -2051,7 +2133,9 @@ export const MessageBubble = memo(function MessageBubble({ }) : // Fallback for any message that predates the timeline model. <> - {message.reasoning ? : null} + {showReasoning && message.reasoning ? ( + + ) : null} {message.toolCalls?.map((call) => call.name === 'todo' ? null : call.name === 'ask_user' ? ( {})} /> From 8c4bdc3bae3a4cc6f764f7328abca1c660d1c994 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Sat, 8 Aug 2026 09:30:35 +0700 Subject: [PATCH 2/9] Fix page freeze when expanding reasoning blocks Reasoning traces are long decompiler-style text. Rendering them through the chat Markdown pipeline on expand created hundreds of React nodes and locked the main thread (Chrome "Page Unresponsive"). Show reasoning as plain pre-wrap text in a height-capped scroller, and defer the body to the next frame so the toggle stays responsive. --- web/src/pages/ChatPage.tsx | 39 ++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/web/src/pages/ChatPage.tsx b/web/src/pages/ChatPage.tsx index 12d08b3..40ba309 100644 --- a/web/src/pages/ChatPage.tsx +++ b/web/src/pages/ChatPage.tsx @@ -1968,24 +1968,55 @@ export function StreamingIndicator({ ) } +/** + * Collapsible model-thinking block. + * + * Must NOT run the chat Markdown renderer on expand: reasoning traces are long + * (tens of KB of decompiler/code-like text with many `*`/`[]`), and turning + * that into hundreds of React nodes freezes the tab ("Page Unresponsive"). + * Plain pre-wrap text in a height-capped scroller is one DOM node, cheap to + * open, and matches how thinking logs are meant to be read. + */ function ReasoningBlock({ text }: { text: string }) { const { t } = useI18n() const [open, setOpen] = useState(false) - // A slim inline toggle rather than a boxed card: collapsed reasoning should - // barely take a line, expanding into a quiet left-ruled block when opened. + // Defer mounting the body to the next frame so the click paints first and + // Chrome does not treat the expand as a long task on the same turn. + const [bodyReady, setBodyReady] = useState(false) + useEffect(() => { + if (!open) { + setBodyReady(false) + return + } + const id = requestAnimationFrame(() => setBodyReady(true)) + return () => cancelAnimationFrame(id) + }, [open]) + return (
{open ? ( -
- +
+ {bodyReady ? ( + 
+              {text}
+
+ ) : ( +

…

+ )}
) : null}
From 164078d421203eb4e6563ae28d9fcf7c8b994aa9 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Sat, 8 Aug 2026 11:29:41 +0700 Subject: [PATCH 3/9] Diagnose ambiguous/wrong edit_file matches with line numbers and near-misses Most edit_file failures are model-side: short old_string hits many sites, or stale/wrong identifiers (entity vs attachEntity) never exist in the file. Surface occurrence line numbers when the match is ambiguous, and near-miss file lines when nothing matches, so the agent re-reads instead of inventing. Also steer the prompt to require unique context and prefer edit_file over sed. --- internal/agent/prompt.go | 1 + internal/tools/file.go | 167 +++++++++++++++++++- internal/tools/file_edit_regression_test.go | 65 ++++++++ 3 files changed, 230 insertions(+), 3 deletions(-) diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index 94cb1d1..aa08a14 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -92,6 +92,7 @@ help them now — do not block them. // paste line numbers into old_string or expand tabs to spaces and // the exact match fails repeatedly. b.WriteString("- read_file returns lines as `NUMBER|CONTENT`. The `|` is metadata only. When calling edit_file, copy **only** the content after `|` into old_string/new_string — never the line number. Preserve tabs and spaces exactly (do not expand tabs to spaces). Line endings are matched automatically.\n") + b.WriteString("- edit_file needs an **exact, unique** old_string. Do not invent identifiers from memory (e.g. `entity` vs `attachEntity`) and do not reuse a short snippet that appears many times in the file. If the tool reports multiple occurrences with line numbers, re-read those lines and widen old_string with unique neighbours. Prefer edit_file over terminal sed/perl for source edits.\n") } if hasTool(active, "vps_upload") || hasTool(active, "vps_download") || hasTool(active, "vps_run") { // Without this, models fall back to terminal rsync/scp and never use diff --git a/internal/tools/file.go b/internal/tools/file.go index dee5481..0989045 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -325,7 +325,7 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { case count == 0: return Errorf("%s", editNotFoundMessage(args.Path, content, args.OldString)) case count > 1 && !args.ReplaceAll: - return Errorf("old_string appears %d times in %s; add more surrounding context or set replace_all", count, args.Path) + return Errorf("%s", editAmbiguousMessage(args.Path, content, oldString, count)) } var updated string @@ -463,8 +463,51 @@ func resolveEditMatch(content, oldIn, newIn string) (oldString, newString string return oldIn, newIn, 0, "" } +// editAmbiguousMessage lists where a non-unique old_string hits so the model +// can widen context to a single site instead of retrying the same short snippet. +func editAmbiguousMessage(path, content, oldString string, count int) string { + var b strings.Builder + fmt.Fprintf(&b, "old_string appears %d times in %s; add more surrounding context (unique lines above/below) or set replace_all.", count, path) + lines := occurrenceLines(content, oldString, 8) + if len(lines) > 0 { + b.WriteString(" Occurrences at line(s): ") + for i, ln := range lines { + if i > 0 { + b.WriteString(", ") + } + fmt.Fprintf(&b, "%d", ln) + } + b.WriteByte('.') + } + b.WriteString(" Re-read those lines and include enough unique neighbours in old_string so it matches exactly once.") + return b.String() +} + +// occurrenceLines returns 1-based line numbers where needle starts, capped. +func occurrenceLines(content, needle string, max int) []int { + if needle == "" || max <= 0 { + return nil + } + var out []int + searchFrom := 0 + for len(out) < max { + i := strings.Index(content[searchFrom:], needle) + if i < 0 { + break + } + abs := searchFrom + i + // Line number = 1 + number of newlines before abs. + out = append(out, 1+strings.Count(content[:abs], "\n")) + searchFrom = abs + len(needle) + if searchFrom >= len(content) { + break + } + } + return out +} + // editNotFoundMessage explains why an edit missed, with actionable recovery -// hints for the model (line prefixes, tabs vs spaces, re-read). +// hints for the model (line prefixes, tabs vs spaces, re-read, near-miss lines). func editNotFoundMessage(path, content, oldString string) string { var b strings.Builder fmt.Fprintf(&b, "old_string not found in %s.", path) @@ -488,10 +531,128 @@ func editNotFoundMessage(path, content, oldString string) string { } } - b.WriteString(" Read the file first and copy only the content after the NUMBER| separator; preserve tabs, spaces, and indentation exactly.") + if hint := nearMissHint(content, oldString); hint != "" { + b.WriteString(" ") + b.WriteString(hint) + return b.String() + } + + b.WriteString(" This is usually a model copy error (stale text, wrong identifier, or invented context) — not a search/grep bug. Re-read the exact lines with read_file and copy only the content after NUMBER| into old_string; preserve tabs, spaces, and indentation exactly.") return b.String() } +// nearMissHint surfaces a few file lines that share a long token with old_string +// so the model can see the real identifier (e.g. attachEntity vs entity). +func nearMissHint(content, oldString string) string { + // Pick a distinctive fragment from the middle of old_string (≥12 chars). + frag := distinctiveFragment(oldString) + if frag == "" { + return "" + } + var hits []string + for i, line := range strings.Split(content, "\n") { + if strings.Contains(line, frag) { + hits = append(hits, fmt.Sprintf("%d|%s", i+1, truncateRunes(strings.TrimRight(line, "\r"), 160))) + if len(hits) >= 4 { + break + } + } + } + if len(hits) == 0 { + // Fall back: look for a long identifier-like token from old_string. + for _, tok := range identifierTokens(oldString) { + if len(tok) < 8 { + continue + } + for i, line := range strings.Split(content, "\n") { + if strings.Contains(line, tok) { + hits = append(hits, fmt.Sprintf("%d|%s", i+1, truncateRunes(strings.TrimRight(line, "\r"), 160))) + if len(hits) >= 4 { + break + } + } + } + if len(hits) > 0 { + break + } + } + } + if len(hits) == 0 { + return "" + } + return "Near-miss lines in the file (re-read these; do not invent identifiers):\n" + strings.Join(hits, "\n") +} + +func distinctiveFragment(s string) string { + s = strings.TrimSpace(s) + s = strings.ReplaceAll(s, "\r\n", "\n") + // Prefer a middle slice of a long single line if present. + for _, line := range strings.Split(s, "\n") { + line = strings.TrimSpace(line) + if len(line) >= 24 { + // Skip common cast noise prefixes that appear everywhere. + start := 0 + if i := strings.Index(line, "->"); i > 0 && i+2 < len(line) { + start = i + } + frag := line[start:] + if len(frag) > 48 { + frag = frag[:48] + } + if len(frag) >= 16 { + return frag + } + } + } + if len(s) >= 24 { + if len(s) > 48 { + return s[:48] + } + return s + } + return "" +} + +func identifierTokens(s string) []string { + var out []string + start := -1 + flush := func(i int) { + if start >= 0 && i-start >= 4 { + out = append(out, s[start:i]) + } + start = -1 + } + for i := 0; i < len(s); i++ { + c := s[i] + id := (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' + if id { + if start < 0 { + start = i + } + } else { + flush(i) + } + } + flush(len(s)) + // Prefer longer tokens first. + for i := 0; i < len(out); i++ { + for j := i + 1; j < len(out); j++ { + if len(out[j]) > len(out[i]) { + out[i], out[j] = out[j], out[i] + } + } + } + return out +} + +func truncateRunes(s string, max int) string { + if max <= 0 || len(s) <= max { + return s + } + // Byte-safe enough for diagnostic ASCII-heavy code lines. + return s[:max] + "…" +} + // expandTabs replaces leading and embedded tabs with spaces at the given width // (stop-based), used only for mismatch diagnosis. func expandTabs(s string, width int) string { diff --git a/internal/tools/file_edit_regression_test.go b/internal/tools/file_edit_regression_test.go index 614d34e..6f94783 100644 --- a/internal/tools/file_edit_regression_test.go +++ b/internal/tools/file_edit_regression_test.go @@ -142,6 +142,71 @@ func TestEditFileDiagnosesTabVsSpaceMismatch(t *testing.T) { } } +// When the same short snippet appears many times (common in C++ reimpl files), +// the error must list line numbers so the model can widen context to one site. +func TestEditFileAmbiguousListsLineNumbers(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "CCam.cpp") + // Same body 3 times — like AllocateMatrix stubs that differ only by nearby locals. + line := " reinterpret_cast(entity)->AllocateMatrix();\n" + original := "void a() {\n" + line + "}\nvoid b() {\n" + line + "}\nvoid c() {\n" + line + "}\n" + if err := os.WriteFile(path, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + editArgs, _ := json.Marshal(map[string]any{ + "path": "CCam.cpp", + "old_string": strings.TrimSuffix(line, "\n"), + "new_string": " reinterpret_cast(attachEntity)->AllocateMatrix();", + }) + edited := (editFileTool{}).Execute(context.Background(), Input{Args: editArgs, Workspace: workspace}) + if !edited.IsError { + t.Fatal("expected ambiguous old_string failure") + } + if !strings.Contains(edited.Content, "appears 3 times") { + t.Fatalf("want appears 3 times, got: %s", edited.Content) + } + if !strings.Contains(edited.Content, "Occurrences at line(s):") { + t.Fatalf("want line numbers in error, got: %s", edited.Content) + } + // Lines 2, 5, 8 in the synthetic file. + for _, want := range []string{"2", "5", "8"} { + if !strings.Contains(edited.Content, want) { + t.Fatalf("error missing line %s: %s", want, edited.Content) + } + } +} + +// Wrong identifier in old_string (entity vs attachEntity) must surface near-miss +// lines from the real file so the model re-reads instead of inventing. +func TestEditFileNotFoundNearMissShowsRealLine(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "CCam.cpp") + original := "void Process() {\n" + + " auto* attachEntity = *reinterpret_cast(raw + 504);\n" + + " if (attachEntity == nullptr) return;\n" + + " reinterpret_cast(attachEntity)->AllocateMatrix();\n" + + "}\n" + if err := os.WriteFile(path, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + // Model still thinks the local is `entity` (stale / wrong site). + editArgs, _ := json.Marshal(map[string]any{ + "path": "CCam.cpp", + "old_string": " reinterpret_cast(entity)->AllocateMatrix();", + "new_string": " reinterpret_cast(attachEntity)->AllocateMatrix();", + }) + edited := (editFileTool{}).Execute(context.Background(), Input{Args: editArgs, Workspace: workspace}) + if !edited.IsError { + t.Fatal("expected not-found") + } + if !strings.Contains(edited.Content, "Near-miss") && !strings.Contains(edited.Content, "AllocateMatrix") { + t.Fatalf("want near-miss hint with real file content, got: %s", edited.Content) + } + if !strings.Contains(edited.Content, "attachEntity") { + t.Fatalf("near-miss should reveal attachEntity, got: %s", edited.Content) + } +} + func TestStripReadFileLinePrefixes(t *testing.T) { in := "10|\tfoo()\n11|\tbar()\n12|}" got, ok := stripReadFileLinePrefixes(in) From bddbc3fb061f935104a6e172469141857b97551c Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Sat, 8 Aug 2026 11:33:48 +0700 Subject: [PATCH 4/9] Revert "Diagnose ambiguous/wrong edit_file matches with line numbers and near-misses" This reverts commit 164078d421203eb4e6563ae28d9fcf7c8b994aa9. --- internal/agent/prompt.go | 1 - internal/tools/file.go | 167 +------------------- internal/tools/file_edit_regression_test.go | 65 -------- 3 files changed, 3 insertions(+), 230 deletions(-) diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index aa08a14..94cb1d1 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -92,7 +92,6 @@ help them now — do not block them. // paste line numbers into old_string or expand tabs to spaces and // the exact match fails repeatedly. b.WriteString("- read_file returns lines as `NUMBER|CONTENT`. The `|` is metadata only. When calling edit_file, copy **only** the content after `|` into old_string/new_string — never the line number. Preserve tabs and spaces exactly (do not expand tabs to spaces). Line endings are matched automatically.\n") - b.WriteString("- edit_file needs an **exact, unique** old_string. Do not invent identifiers from memory (e.g. `entity` vs `attachEntity`) and do not reuse a short snippet that appears many times in the file. If the tool reports multiple occurrences with line numbers, re-read those lines and widen old_string with unique neighbours. Prefer edit_file over terminal sed/perl for source edits.\n") } if hasTool(active, "vps_upload") || hasTool(active, "vps_download") || hasTool(active, "vps_run") { // Without this, models fall back to terminal rsync/scp and never use diff --git a/internal/tools/file.go b/internal/tools/file.go index 0989045..dee5481 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -325,7 +325,7 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { case count == 0: return Errorf("%s", editNotFoundMessage(args.Path, content, args.OldString)) case count > 1 && !args.ReplaceAll: - return Errorf("%s", editAmbiguousMessage(args.Path, content, oldString, count)) + return Errorf("old_string appears %d times in %s; add more surrounding context or set replace_all", count, args.Path) } var updated string @@ -463,51 +463,8 @@ func resolveEditMatch(content, oldIn, newIn string) (oldString, newString string return oldIn, newIn, 0, "" } -// editAmbiguousMessage lists where a non-unique old_string hits so the model -// can widen context to a single site instead of retrying the same short snippet. -func editAmbiguousMessage(path, content, oldString string, count int) string { - var b strings.Builder - fmt.Fprintf(&b, "old_string appears %d times in %s; add more surrounding context (unique lines above/below) or set replace_all.", count, path) - lines := occurrenceLines(content, oldString, 8) - if len(lines) > 0 { - b.WriteString(" Occurrences at line(s): ") - for i, ln := range lines { - if i > 0 { - b.WriteString(", ") - } - fmt.Fprintf(&b, "%d", ln) - } - b.WriteByte('.') - } - b.WriteString(" Re-read those lines and include enough unique neighbours in old_string so it matches exactly once.") - return b.String() -} - -// occurrenceLines returns 1-based line numbers where needle starts, capped. -func occurrenceLines(content, needle string, max int) []int { - if needle == "" || max <= 0 { - return nil - } - var out []int - searchFrom := 0 - for len(out) < max { - i := strings.Index(content[searchFrom:], needle) - if i < 0 { - break - } - abs := searchFrom + i - // Line number = 1 + number of newlines before abs. - out = append(out, 1+strings.Count(content[:abs], "\n")) - searchFrom = abs + len(needle) - if searchFrom >= len(content) { - break - } - } - return out -} - // editNotFoundMessage explains why an edit missed, with actionable recovery -// hints for the model (line prefixes, tabs vs spaces, re-read, near-miss lines). +// hints for the model (line prefixes, tabs vs spaces, re-read). func editNotFoundMessage(path, content, oldString string) string { var b strings.Builder fmt.Fprintf(&b, "old_string not found in %s.", path) @@ -531,128 +488,10 @@ func editNotFoundMessage(path, content, oldString string) string { } } - if hint := nearMissHint(content, oldString); hint != "" { - b.WriteString(" ") - b.WriteString(hint) - return b.String() - } - - b.WriteString(" This is usually a model copy error (stale text, wrong identifier, or invented context) — not a search/grep bug. Re-read the exact lines with read_file and copy only the content after NUMBER| into old_string; preserve tabs, spaces, and indentation exactly.") + b.WriteString(" Read the file first and copy only the content after the NUMBER| separator; preserve tabs, spaces, and indentation exactly.") return b.String() } -// nearMissHint surfaces a few file lines that share a long token with old_string -// so the model can see the real identifier (e.g. attachEntity vs entity). -func nearMissHint(content, oldString string) string { - // Pick a distinctive fragment from the middle of old_string (≥12 chars). - frag := distinctiveFragment(oldString) - if frag == "" { - return "" - } - var hits []string - for i, line := range strings.Split(content, "\n") { - if strings.Contains(line, frag) { - hits = append(hits, fmt.Sprintf("%d|%s", i+1, truncateRunes(strings.TrimRight(line, "\r"), 160))) - if len(hits) >= 4 { - break - } - } - } - if len(hits) == 0 { - // Fall back: look for a long identifier-like token from old_string. - for _, tok := range identifierTokens(oldString) { - if len(tok) < 8 { - continue - } - for i, line := range strings.Split(content, "\n") { - if strings.Contains(line, tok) { - hits = append(hits, fmt.Sprintf("%d|%s", i+1, truncateRunes(strings.TrimRight(line, "\r"), 160))) - if len(hits) >= 4 { - break - } - } - } - if len(hits) > 0 { - break - } - } - } - if len(hits) == 0 { - return "" - } - return "Near-miss lines in the file (re-read these; do not invent identifiers):\n" + strings.Join(hits, "\n") -} - -func distinctiveFragment(s string) string { - s = strings.TrimSpace(s) - s = strings.ReplaceAll(s, "\r\n", "\n") - // Prefer a middle slice of a long single line if present. - for _, line := range strings.Split(s, "\n") { - line = strings.TrimSpace(line) - if len(line) >= 24 { - // Skip common cast noise prefixes that appear everywhere. - start := 0 - if i := strings.Index(line, "->"); i > 0 && i+2 < len(line) { - start = i - } - frag := line[start:] - if len(frag) > 48 { - frag = frag[:48] - } - if len(frag) >= 16 { - return frag - } - } - } - if len(s) >= 24 { - if len(s) > 48 { - return s[:48] - } - return s - } - return "" -} - -func identifierTokens(s string) []string { - var out []string - start := -1 - flush := func(i int) { - if start >= 0 && i-start >= 4 { - out = append(out, s[start:i]) - } - start = -1 - } - for i := 0; i < len(s); i++ { - c := s[i] - id := (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' - if id { - if start < 0 { - start = i - } - } else { - flush(i) - } - } - flush(len(s)) - // Prefer longer tokens first. - for i := 0; i < len(out); i++ { - for j := i + 1; j < len(out); j++ { - if len(out[j]) > len(out[i]) { - out[i], out[j] = out[j], out[i] - } - } - } - return out -} - -func truncateRunes(s string, max int) string { - if max <= 0 || len(s) <= max { - return s - } - // Byte-safe enough for diagnostic ASCII-heavy code lines. - return s[:max] + "…" -} - // expandTabs replaces leading and embedded tabs with spaces at the given width // (stop-based), used only for mismatch diagnosis. func expandTabs(s string, width int) string { diff --git a/internal/tools/file_edit_regression_test.go b/internal/tools/file_edit_regression_test.go index 6f94783..614d34e 100644 --- a/internal/tools/file_edit_regression_test.go +++ b/internal/tools/file_edit_regression_test.go @@ -142,71 +142,6 @@ func TestEditFileDiagnosesTabVsSpaceMismatch(t *testing.T) { } } -// When the same short snippet appears many times (common in C++ reimpl files), -// the error must list line numbers so the model can widen context to one site. -func TestEditFileAmbiguousListsLineNumbers(t *testing.T) { - workspace := t.TempDir() - path := filepath.Join(workspace, "CCam.cpp") - // Same body 3 times — like AllocateMatrix stubs that differ only by nearby locals. - line := " reinterpret_cast(entity)->AllocateMatrix();\n" - original := "void a() {\n" + line + "}\nvoid b() {\n" + line + "}\nvoid c() {\n" + line + "}\n" - if err := os.WriteFile(path, []byte(original), 0o644); err != nil { - t.Fatal(err) - } - editArgs, _ := json.Marshal(map[string]any{ - "path": "CCam.cpp", - "old_string": strings.TrimSuffix(line, "\n"), - "new_string": " reinterpret_cast(attachEntity)->AllocateMatrix();", - }) - edited := (editFileTool{}).Execute(context.Background(), Input{Args: editArgs, Workspace: workspace}) - if !edited.IsError { - t.Fatal("expected ambiguous old_string failure") - } - if !strings.Contains(edited.Content, "appears 3 times") { - t.Fatalf("want appears 3 times, got: %s", edited.Content) - } - if !strings.Contains(edited.Content, "Occurrences at line(s):") { - t.Fatalf("want line numbers in error, got: %s", edited.Content) - } - // Lines 2, 5, 8 in the synthetic file. - for _, want := range []string{"2", "5", "8"} { - if !strings.Contains(edited.Content, want) { - t.Fatalf("error missing line %s: %s", want, edited.Content) - } - } -} - -// Wrong identifier in old_string (entity vs attachEntity) must surface near-miss -// lines from the real file so the model re-reads instead of inventing. -func TestEditFileNotFoundNearMissShowsRealLine(t *testing.T) { - workspace := t.TempDir() - path := filepath.Join(workspace, "CCam.cpp") - original := "void Process() {\n" + - " auto* attachEntity = *reinterpret_cast(raw + 504);\n" + - " if (attachEntity == nullptr) return;\n" + - " reinterpret_cast(attachEntity)->AllocateMatrix();\n" + - "}\n" - if err := os.WriteFile(path, []byte(original), 0o644); err != nil { - t.Fatal(err) - } - // Model still thinks the local is `entity` (stale / wrong site). - editArgs, _ := json.Marshal(map[string]any{ - "path": "CCam.cpp", - "old_string": " reinterpret_cast(entity)->AllocateMatrix();", - "new_string": " reinterpret_cast(attachEntity)->AllocateMatrix();", - }) - edited := (editFileTool{}).Execute(context.Background(), Input{Args: editArgs, Workspace: workspace}) - if !edited.IsError { - t.Fatal("expected not-found") - } - if !strings.Contains(edited.Content, "Near-miss") && !strings.Contains(edited.Content, "AllocateMatrix") { - t.Fatalf("want near-miss hint with real file content, got: %s", edited.Content) - } - if !strings.Contains(edited.Content, "attachEntity") { - t.Fatalf("near-miss should reveal attachEntity, got: %s", edited.Content) - } -} - func TestStripReadFileLinePrefixes(t *testing.T) { in := "10|\tfoo()\n11|\tbar()\n12|}" got, ok := stripReadFileLinePrefixes(in) From 5973506338d1b72147d44172174e015b2dc2e62a Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Wed, 12 Aug 2026 00:41:16 +0700 Subject: [PATCH 5/9] Harden stale edit diagnostics and prompt lessons --- internal/agent/learn.go | 31 ++++- internal/agent/learn_test.go | 20 +++ internal/agent/prompt.go | 1 + internal/tools/file.go | 132 +++++++++++++++++++- internal/tools/file_edit_regression_test.go | 42 +++++++ 5 files changed, 223 insertions(+), 3 deletions(-) create mode 100644 internal/agent/learn_test.go diff --git a/internal/agent/learn.go b/internal/agent/learn.go index 3873f73..fda6aea 100644 --- a/internal/agent/learn.go +++ b/internal/agent/learn.go @@ -104,11 +104,21 @@ func (a *Agent) lessonsBlock(ctx context.Context) string { return "" } var lessons []string + seen := make(map[string]struct{}) for _, m := range items { if m.Source == lessonSource { - lessons = append(lessons, m.Content) + lesson := strings.TrimSpace(m.Content) + if !usableLesson(lesson) { + continue + } + key := strings.ToLower(lesson) + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + lessons = append(lessons, lesson) } - if len(lessons) >= 20 { + if len(lessons) >= 12 { break } } @@ -123,3 +133,20 @@ func (a *Agent) lessonsBlock(ctx context.Context) string { } return b.String() } + +// usableLesson keeps malformed auxiliary-model output out of the system +// prompt. Historical rows include fragments such as "NONE", "Wait", and +// duplicated partial sentences; presenting them as instructions makes tool +// behavior less predictable and wastes context. +func usableLesson(s string) bool { + if len(s) < 32 || len(s) > 400 { + return false + } + if strings.EqualFold(s, "none") || strings.HasSuffix(strings.ToLower(s), " none") { + return false + } + if strings.EqualFold(s, "wait") || strings.EqualFold(s, ".") { + return false + } + return true +} diff --git a/internal/agent/learn_test.go b/internal/agent/learn_test.go new file mode 100644 index 0000000..d377a8f --- /dev/null +++ b/internal/agent/learn_test.go @@ -0,0 +1,20 @@ +package agent + +import "testing" + +func TestUsableLessonRejectsAuxiliaryFragments(t *testing.T) { + for _, lesson := range []string{"NONE", "Wait", ".", "When `edit_file"} { + if usableLesson(lesson) { + t.Errorf("malformed lesson accepted: %q", lesson) + } + } + if !usableLesson("When edit_file fails with old_string not found, read the current file and copy exact whitespace before retrying.") { + t.Fatal("valid lesson rejected") + } +} + +func TestUsableLessonRejectsTrailingNone(t *testing.T) { + if usableLesson("When a tool fails, inspect the concrete error and retry with corrected arguments. NONE") { + t.Fatal("auxiliary NONE suffix should not enter the prompt") + } +} diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index 94cb1d1..550bb99 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -92,6 +92,7 @@ help them now — do not block them. // paste line numbers into old_string or expand tabs to spaces and // the exact match fails repeatedly. b.WriteString("- read_file returns lines as `NUMBER|CONTENT`. The `|` is metadata only. When calling edit_file, copy **only** the content after `|` into old_string/new_string — never the line number. Preserve tabs and spaces exactly (do not expand tabs to spaces). Line endings are matched automatically.\n") + b.WriteString("- edit_file requires an exact, unique old_string from the current file. After any successful edit or write, re-read before making another edit; do not reuse an older block or invent identifiers. If it reports multiple occurrences, include unique neighbouring lines or use replace_all only when every occurrence should change.\n") } if hasTool(active, "vps_upload") || hasTool(active, "vps_download") || hasTool(active, "vps_run") { // Without this, models fall back to terminal rsync/scp and never use diff --git a/internal/tools/file.go b/internal/tools/file.go index dee5481..270e6d5 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -325,7 +325,7 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { case count == 0: return Errorf("%s", editNotFoundMessage(args.Path, content, args.OldString)) case count > 1 && !args.ReplaceAll: - return Errorf("old_string appears %d times in %s; add more surrounding context or set replace_all", count, args.Path) + return Errorf("%s", editAmbiguousMessage(args.Path, content, oldString, count)) } var updated string @@ -488,10 +488,140 @@ func editNotFoundMessage(path, content, oldString string) string { } } + // A mixed paste usually means the model copied the display prefix from only + // one or two read_file lines. Do not silently strip it: the unprefixed lines + // may contain literal pipe characters. + if prefixed, total := readFileLinePrefixCounts(oldString); prefixed > 0 && prefixed < total { + b.WriteString(" Some old_string lines still include read_file line numbers (NUMBER|) while others do not. Remove every numeric prefix and keep only the text after each |, then retry from a fresh read.") + return b.String() + } + if hint := nearMissHint(content, oldString); hint != "" { + b.WriteByte(' ') + b.WriteString(hint) + return b.String() + } + b.WriteString(" Read the file first and copy only the content after the NUMBER| separator; preserve tabs, spaces, and indentation exactly.") return b.String() } +func readFileLinePrefixCounts(s string) (prefixed, total int) { + lines := strings.Split(strings.ReplaceAll(s, "\r\n", "\n"), "\n") + if len(lines) > 1 && lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] + } + for _, line := range lines { + total++ + i := strings.IndexByte(line, '|') + if i > 0 { + allDigits := true + for _, c := range line[:i] { + if c < '0' || c > '9' { + allDigits = false + break + } + } + if allDigits { + prefixed++ + } + } + } + return prefixed, total +} + +func editAmbiguousMessage(path, content, oldString string, count int) string { + var b strings.Builder + fmt.Fprintf(&b, "old_string appears %d times in %s; add unique surrounding context or set replace_all only if every occurrence should change.", count, path) + if lines := occurrenceLines(content, oldString, 12); len(lines) > 0 { + b.WriteString(" Current match line(s): ") + for i, line := range lines { + if i > 0 { + b.WriteString(", ") + } + fmt.Fprintf(&b, "%d", line) + } + b.WriteByte('.') + } + b.WriteString(" Re-read the current file and include enough neighbouring lines for exactly one match.") + return b.String() +} + +func occurrenceLines(content, needle string, max int) []int { + if needle == "" || max <= 0 { + return nil + } + var lines []int + for from := 0; from < len(content) && len(lines) < max; { + i := strings.Index(content[from:], needle) + if i < 0 { + break + } + at := from + i + lines = append(lines, 1+strings.Count(content[:at], "\n")) + from = at + len(needle) + } + return lines +} + +// nearMissHint reports a few real lines sharing a distinctive identifier with +// old_string. It is intentionally short and bounded: the tool should correct +// the model's stale context without dumping the file into an error response. +func nearMissHint(content, oldString string) string { + for _, token := range identifierTokens(oldString) { + if len(token) < 8 || strings.Contains(strings.ToLower(token), "read_file") { + continue + } + var hits []string + for i, line := range strings.Split(content, "\n") { + if strings.Contains(line, token) { + line = strings.TrimRight(line, "\r") + if len(line) > 180 { + line = line[:180] + "..." + } + hits = append(hits, fmt.Sprintf("line %d: %s", i+1, line)) + if len(hits) == 3 { + break + } + } + } + if len(hits) > 0 { + return "Near-miss lines sharing a token (re-read them; do not invent identifiers): " + strings.Join(hits, " ") + } + } + return "" +} + +func identifierTokens(s string) []string { + var out []string + start := -1 + flush := func(end int) { + if start >= 0 && end-start >= 8 { + out = append(out, s[start:end]) + } + start = -1 + } + for i := 0; i < len(s); i++ { + c := s[i] + isID := (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' + if isID { + if start < 0 { + start = i + } + } else { + flush(i) + } + } + flush(len(s)) + for i := 0; i < len(out); i++ { + for j := i + 1; j < len(out); j++ { + if len(out[j]) > len(out[i]) { + out[i], out[j] = out[j], out[i] + } + } + } + return out +} + // expandTabs replaces leading and embedded tabs with spaces at the given width // (stop-based), used only for mismatch diagnosis. func expandTabs(s string, width int) string { diff --git a/internal/tools/file_edit_regression_test.go b/internal/tools/file_edit_regression_test.go index 614d34e..c479cb8 100644 --- a/internal/tools/file_edit_regression_test.go +++ b/internal/tools/file_edit_regression_test.go @@ -142,6 +142,33 @@ func TestEditFileDiagnosesTabVsSpaceMismatch(t *testing.T) { } } +func TestEditFileAmbiguousListsCurrentMatchLines(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "repeat.go") + content := "func a() {\n\treturn value\n}\nfunc b() {\n\treturn value\n}\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{"path": "repeat.go", "old_string": "\treturn value", "new_string": "\treturn other"}) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if !result.IsError || !strings.Contains(result.Content, "Current match line(s): 2, 5") { + t.Fatalf("unexpected ambiguity result: %+v", result) + } +} + +func TestEditFileNotFoundShowsNearMiss(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "names.go") + if err := os.WriteFile(path, []byte("func attachEntity() {}\n"), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{"path": "names.go", "old_string": "func attachEntit() {}", "new_string": "func attachEntity2() {}"}) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if !result.IsError || !strings.Contains(result.Content, "attachEntity") { + t.Fatalf("near-miss missing from result: %+v", result) + } +} + func TestStripReadFileLinePrefixes(t *testing.T) { in := "10|\tfoo()\n11|\tbar()\n12|}" got, ok := stripReadFileLinePrefixes(in) @@ -160,3 +187,18 @@ func TestStripReadFileLinePrefixes(t *testing.T) { t.Fatalf("non-prefixed = %q ok=%v", s, ok) } } + +func TestEditFileDiagnosesMixedReadPrefixes(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "mixed.go") + if err := os.WriteFile(path, []byte("func run() {\n\treturn\n}\n"), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{ + "path": "mixed.go", "old_string": "1|func run() {\n\treturn\n}", "new_string": "1|func run() {\n\treturn nil\n}", + }) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if !result.IsError || !strings.Contains(result.Content, "Some old_string lines still include") { + t.Fatalf("mixed-prefix diagnostic missing: %+v", result) + } +} From 9d7064a68d37bc79f81c8ec4bcde353e3c1fbc00 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Wed, 12 Aug 2026 00:50:04 +0700 Subject: [PATCH 6/9] Recover unique stale adjacent insertions --- internal/agent/prompt.go | 2 +- internal/tools/file.go | 98 +++++++++++++++++++++ internal/tools/file_edit_regression_test.go | 48 ++++++++++ 3 files changed, 147 insertions(+), 1 deletion(-) diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index 550bb99..3e6d410 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -92,7 +92,7 @@ help them now — do not block them. // paste line numbers into old_string or expand tabs to spaces and // the exact match fails repeatedly. b.WriteString("- read_file returns lines as `NUMBER|CONTENT`. The `|` is metadata only. When calling edit_file, copy **only** the content after `|` into old_string/new_string — never the line number. Preserve tabs and spaces exactly (do not expand tabs to spaces). Line endings are matched automatically.\n") - b.WriteString("- edit_file requires an exact, unique old_string from the current file. After any successful edit or write, re-read before making another edit; do not reuse an older block or invent identifiers. If it reports multiple occurrences, include unique neighbouring lines or use replace_all only when every occurrence should change.\n") + b.WriteString("- Before every edit_file call, read the current target file first. edit_file requires an exact, unique old_string from that fresh read. After any successful edit or write, re-read before making another edit; do not reuse an older block or invent identifiers. If it reports multiple occurrences, include unique neighbouring lines or use replace_all only when every occurrence should change.\n") } if hasTool(active, "vps_upload") || hasTool(active, "vps_download") || hasTool(active, "vps_run") { // Without this, models fall back to terminal rsync/scp and never use diff --git a/internal/tools/file.go b/internal/tools/file.go index 270e6d5..fb8a5aa 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -417,6 +417,7 @@ func stripReadFileLinePrefixes(s string) (string, bool) { // the two failure modes that read_file → edit_file commonly hits: // 1. LF vs CRLF (read_file always displays LF) // 2. pasted NUMBER| line prefixes from read_file output +// 3. a unique near-match when new_string only inserts adjacent text // // how is a short note for the success message when recovery was used; empty on // a plain exact match. @@ -460,9 +461,106 @@ func resolveEditMatch(content, oldIn, newIn string) (oldString, newString string } } + // 3. A common README/table operation copies a line from an earlier read, + // abbreviates one phrase, and adds a new row immediately after it. Recover + // only that narrow insertion shape, and only when one file line is a clear + // unique match. The actual on-disk line is retained, so stale wording is not + // silently overwritten. Ordinary replacements remain exact-only. + if oldLine, newLine, ok := resolveAdjacentInsertion(content, oldIn, newIn, eol); ok { + return oldLine, newLine, 1, "matched unique near line for adjacent insertion" + } + return oldIn, newIn, 0, "" } +func resolveAdjacentInsertion(content, oldIn, newIn, eol string) (oldLine, newLine string, ok bool) { + oldNorm := toEOL(oldIn, "\n") + newNorm := toEOL(newIn, "\n") + if oldNorm == "" || strings.Contains(oldNorm, "\n") { + return "", "", false + } + + mode := 0 // 1 = insert after, 2 = insert before + insert := "" + if strings.HasPrefix(newNorm, oldNorm+"\n") { + mode = 1 + insert = strings.TrimPrefix(newNorm, oldNorm) + } else if strings.HasSuffix(newNorm, "\n"+oldNorm) { + mode = 2 + insert = strings.TrimSuffix(newNorm, oldNorm) + } else { + return "", "", false + } + + normalized := strings.ReplaceAll(content, "\r\n", "\n") + normalized = strings.ReplaceAll(normalized, "\r", "\n") + lines := strings.Split(normalized, "\n") + best, second := -1.0, -1.0 + bestLine := -1 + for i, line := range lines { + if line == "" && i == len(lines)-1 { + continue + } + score := editLineSimilarity(oldNorm, line) + if score > best { + second, best = best, score + bestLine = i + } else if score > second { + second = score + } + } + if bestLine < 0 || best < 0.78 || (second >= 0 && best-second < 0.12) { + return "", "", false + } + + actual := lines[bestLine] + if mode == 1 { + newLine = actual + insert + } else { + newLine = insert + actual + } + return toEOL(actual, eol), toEOL(newLine, eol), true +} + +func editLineSimilarity(a, b string) float64 { + aSet := editTokenSet(a) + bSet := editTokenSet(b) + if len(aSet) == 0 || len(bSet) == 0 { + return 0 + } + common := 0 + for token := range aSet { + if _, ok := bSet[token]; ok { + common++ + } + } + return float64(common) / float64(len(aSet)+len(bSet)-common) +} + +func editTokenSet(s string) map[string]struct{} { + set := make(map[string]struct{}) + start := -1 + flush := func(end int) { + if start >= 0 && end-start >= 2 { + set[strings.ToLower(s[start:end])] = struct{}{} + } + start = -1 + } + for i := 0; i < len(s); i++ { + c := s[i] + isToken := (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' + if isToken { + if start < 0 { + start = i + } + } else { + flush(i) + } + } + flush(len(s)) + return set +} + // editNotFoundMessage explains why an edit missed, with actionable recovery // hints for the model (line prefixes, tabs vs spaces, re-read). func editNotFoundMessage(path, content, oldString string) string { diff --git a/internal/tools/file_edit_regression_test.go b/internal/tools/file_edit_regression_test.go index c479cb8..3ef8d92 100644 --- a/internal/tools/file_edit_regression_test.go +++ b/internal/tools/file_edit_regression_test.go @@ -169,6 +169,54 @@ func TestEditFileNotFoundShowsNearMiss(t *testing.T) { } } +func TestEditFileRecoversUniqueNearInsertionWithoutChangingExistingLine(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "README.md") + actual := "| **pool39v2** | 14 hand + 25 vision-audited | 33 train / 6 val | T4 | 85 (early stop @55) | **0.009** | `artifacts/pool39v2/` |\n" + if err := os.WriteFile(path, []byte(actual), 0o644); err != nil { + t.Fatal(err) + } + old := "| **pool39v2** | 14 hand + 25 vision-audited | 33 train / 6 val | T4 | 85 (ES@55) | **0.009** | `artifacts/pool39v2/` |" + newString := old + "\n| **pool39v2_sc** | single-class icon | 33 train / 6 val | T4 | 70 | **0.519** | `artifacts/pool39v2_sc/` |" + args, _ := json.Marshal(map[string]any{"path": "README.md", "old_string": old, "new_string": newString}) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if result.IsError { + t.Fatalf("unique adjacent insertion should recover: %s", result.Content) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + want := actual + "| **pool39v2_sc** | single-class icon | 33 train / 6 val | T4 | 70 | **0.519** | `artifacts/pool39v2_sc/` |\n" + if string(got) != want { + t.Fatalf("recovery changed the existing line:\n%s\nwant:\n%s", got, want) + } +} + +func TestEditFileDoesNotRecoverAmbiguousNearInsertion(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "README.md") + content := "| **pool39v2** | 85 (early stop @55) | artifacts/a |\n| **pool39v2** | 85 (early stop @55) | artifacts/b |\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + old := "| **pool39v2** | 85 (ES@55) | artifacts/c |" + args, _ := json.Marshal(map[string]any{ + "path": "README.md", "old_string": old, "new_string": old + "\n| new row |", + }) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if !result.IsError || !strings.Contains(result.Content, "old_string not found") { + t.Fatalf("ambiguous near insertion must remain exact-only: %+v", result) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != content { + t.Fatalf("ambiguous recovery modified file: %q", got) + } +} + func TestStripReadFileLinePrefixes(t *testing.T) { in := "10|\tfoo()\n11|\tbar()\n12|}" got, ok := stripReadFileLinePrefixes(in) From c00c7a9f5e64ed79ec271edcb1b4ad245f4c88bb Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Wed, 12 Aug 2026 17:16:23 +0700 Subject: [PATCH 7/9] Terminate timed-out foreground commands with their process group A foreground command that outlived its timeout kept running behind the persistent shell, holding its pipes and corrupting every later call in the session. Put the shell in its own process group, kill the whole group on timeout or cancellation, and hand the next call a fresh shell. configureProcessGroup now preserves existing SysProcAttr settings instead of clobbering sandbox flags. Co-authored-by: Cursor --- internal/tools/background_process_unix.go | 11 ++++++- internal/tools/shell.go | 21 ++++++++++-- internal/tools/shell_test.go | 40 +++++++++++++++++++++++ 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/internal/tools/background_process_unix.go b/internal/tools/background_process_unix.go index caf7214..e51b68a 100644 --- a/internal/tools/background_process_unix.go +++ b/internal/tools/background_process_unix.go @@ -8,7 +8,16 @@ import ( ) func configureProcessGroup(cmd *exec.Cmd) { - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if cmd == nil { + return + } + // Sandboxed commands may already carry Cloneflags, uid mappings, or a + // parent-death signal. Preserve those settings while adding the process + // group needed to terminate a command tree on timeout. + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + cmd.SysProcAttr.Setpgid = true } func terminateProcessGroup(cmd *exec.Cmd) { diff --git a/internal/tools/shell.go b/internal/tools/shell.go index f1ac001..05cbdd7 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -201,6 +201,11 @@ func (m *ShellManager) session(id, workspace string) (*shellSession, error) { out := &lockedBuffer{} cmd.Stdout = out cmd.Stderr = out + // Keep the persistent shell and every foreground command it starts in an + // isolated process group. A timed-out command must be terminated as a unit; + // killing only the shell can leave descendants holding the shell's pipes and + // wedge the session for all subsequent calls. + configureProcessGroup(cmd) if err := cmd.Start(); err != nil { return nil, fmt.Errorf("start shell: %w", err) } @@ -287,11 +292,18 @@ func (m *ShellManager) ReapIdle(lifetime time.Duration) { func (s *shellSession) terminate() { s.mu.Lock() defer s.mu.Unlock() + s.terminateLocked() +} + +// terminateLocked stops the shell and its descendants. The caller must hold +// s.mu. It is also used by run when a foreground call times out, where waiting +// for the normal command sentinel is no longer safe. +func (s *shellSession) terminateLocked() { if s.cmd == nil || s.cmd.Process == nil { return } _ = s.stdin.Close() - _ = s.cmd.Process.Kill() + killProcessGroup(s.cmd) s.dead.Store(true) } @@ -335,7 +347,9 @@ func (s *shellSession) run(ctx context.Context, command string, timeout time.Dur for { select { case <-ctx.Done(): - return s.finish(marker), -1, ctx.Err() + out := s.finish(marker) + s.terminateLocked() + return out, -1, ctx.Err() case <-s.done: // The shell can exit before printing the sentinel (for example when a // command enables `set -e` and then fails). Do not wait out the full @@ -364,6 +378,9 @@ func (s *shellSession) run(ctx context.Context, command string, timeout time.Dur } if time.Now().After(deadline) { out := s.finish(marker) + // Do not leave the timed-out command running behind the persistent + // shell. Its output and stdin state would corrupt the next call. + s.terminateLocked() return out, -1, fmt.Errorf("command timed out after %s", timeout) } } diff --git a/internal/tools/shell_test.go b/internal/tools/shell_test.go index 7150353..eb4d209 100644 --- a/internal/tools/shell_test.go +++ b/internal/tools/shell_test.go @@ -128,6 +128,46 @@ func TestPersistentShellExitReturnsPromptlyAndRecovers(t *testing.T) { } } +func TestPersistentShellTimeoutKillsCommandAndRecovers(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX process-group behavior does not apply on Windows") + } + + m := NewShellManager(config.Terminal{}) + t.Cleanup(m.CloseAll) + workspace := t.TempDir() + sess, err := m.session("timeout-session", workspace) + if err != nil { + t.Fatal(err) + } + + start := time.Now() + _, _, err = sess.run(context.Background(), "sleep 60", 150*time.Millisecond, nil) + if err == nil || !strings.Contains(err.Error(), "timed out") { + t.Fatalf("timeout error = %v, want timed out", err) + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Fatalf("timeout returned after %s, want prompt cancellation", elapsed) + } + if !sess.dead.Load() { + t.Fatal("timed-out shell was left marked live") + } + + // The next call must receive a replacement shell rather than appending to + // the command that was killed at the timeout boundary. + replacement, err := m.session("timeout-session", workspace) + if err != nil { + t.Fatal(err) + } + if replacement == sess { + t.Fatal("timed-out persistent shell was reused") + } + out, code, err := replacement.run(context.Background(), "printf RECOVERED", 2*time.Second, nil) + if err != nil || code != 0 || out != "RECOVERED" { + t.Fatalf("replacement shell result = (%q, %d, %v)", out, code, err) + } +} + // Commands like `adb shell …` inherit the persistent shell's stdin pipe. Because // that pipe stays open between tool calls, they block reading (or steal the // completion sentinel). Wrapping the user command so its stdin is /dev/null From 88007b695c7b66b8d2ea7b18bc43ebc5f7dce1b0 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Wed, 12 Aug 2026 17:16:34 +0700 Subject: [PATCH 8/9] Fix edit_file wrong-line splice corruption and EOL matching failures The adjacent-insertion recovery replaced the first substring occurrence of the matched line anywhere in the file, corrupting an unrelated line mid-text while reporting success; it now splices by the matched line's byte range and never combines with replace_all. Matching tries the verbatim old_string before any EOL normalization, fileEOL picks the majority flavor so one stray CR/CRLF cannot poison every multi-line edit, and NUMBER| prefix stripping requires consecutive numbers so pipe-delimited data is never mangled. read_file no longer misreads a rune split at the 400 KB cap as binary and displays lone-CR files per line. Co-authored-by: Cursor --- internal/agent/prompt.go | 2 +- internal/tools/file.go | 217 ++++++++++++++------ internal/tools/file_edit_regression_test.go | 162 +++++++++++++++ 3 files changed, 321 insertions(+), 60 deletions(-) diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index 3e6d410..b2c351d 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -92,7 +92,7 @@ help them now — do not block them. // paste line numbers into old_string or expand tabs to spaces and // the exact match fails repeatedly. b.WriteString("- read_file returns lines as `NUMBER|CONTENT`. The `|` is metadata only. When calling edit_file, copy **only** the content after `|` into old_string/new_string — never the line number. Preserve tabs and spaces exactly (do not expand tabs to spaces). Line endings are matched automatically.\n") - b.WriteString("- Before every edit_file call, read the current target file first. edit_file requires an exact, unique old_string from that fresh read. After any successful edit or write, re-read before making another edit; do not reuse an older block or invent identifiers. If it reports multiple occurrences, include unique neighbouring lines or use replace_all only when every occurrence should change.\n") + b.WriteString("- Before every edit_file call, re-read the region you are editing (read_file with offset/limit on large files). edit_file requires an exact, unique old_string from that fresh read. After any successful edit or write, re-read before making another edit; do not reuse an older block or invent identifiers. If it reports multiple occurrences, include unique neighbouring lines or use replace_all only when every occurrence should change.\n") } if hasTool(active, "vps_upload") || hasTool(active, "vps_download") || hasTool(active, "vps_run") { // Without this, models fall back to terminal rsync/scp and never use diff --git a/internal/tools/file.go b/internal/tools/file.go index fb8a5aa..738c5ae 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "sort" + "strconv" "strings" "unicode/utf8" ) @@ -179,12 +180,20 @@ func (readFileTool) Execute(_ context.Context, in Input) Result { if len(data) > maxReadBytes { data = data[:maxReadBytes] truncatedBytes = true + // The cut can land inside a multi-byte rune; trimming up to three + // trailing bytes keeps a genuine text file from reading as binary. + for i := 0; i < 3 && len(data) > 0 && !utf8.Valid(data); i++ { + data = data[:len(data)-1] + } } if !utf8.Valid(data) { return Errorf("%s appears to be a binary file (%d bytes)", args.Path, fi.Size()) } - lines := strings.Split(strings.ReplaceAll(string(data), "\r\n", "\n"), "\n") + normalized := strings.ReplaceAll(string(data), "\r\n", "\n") + // Lone CR (classic Mac) must also split, or the file displays as one line. + normalized = strings.ReplaceAll(normalized, "\r", "\n") + lines := strings.Split(normalized, "\n") offset := args.Offset if offset <= 0 { offset = 1 @@ -323,6 +332,22 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { oldString, newString, count, how := resolveEditMatch(content, args.OldString, args.NewString) switch { case count == 0: + // Last-resort recovery for one narrow shape: a stale single-line anchor + // whose new_string only inserts adjacent text. Spliced by line index so + // it can never touch any other occurrence; never combined with + // replace_all, whose contract is "every exact occurrence". + if !args.ReplaceAll { + if updated, ok := spliceAdjacentInsertion(content, args.OldString, args.NewString); ok { + if err := writeWithCheckpoint(in, path, []byte(updated), "edit_file"); err != nil { + return Errorf("cannot write %s: %v", args.Path, err) + } + rel := relTo(in.Workspace, path) + return Result{ + Content: fmt.Sprintf("Edited %s (1 replacement(s)) [matched unique near line for adjacent insertion]", rel), + Meta: map[string]any{"path": rel, "replacements": 1}, + } + } + } return Errorf("%s", editNotFoundMessage(args.Path, content, args.OldString)) case count > 1 && !args.ReplaceAll: return Errorf("%s", editAmbiguousMessage(args.Path, content, oldString, count)) @@ -352,17 +377,40 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { } } -// fileEOL returns the dominant newline sequence used in s. +// fileEOL returns the dominant newline sequence used in s, by majority. A +// single stray CRLF or CR in an otherwise-LF file must not decide the flavor: +// that used to convert every multi-line old_string away from what the file +// actually contains and permanently break edits on such files. func fileEOL(s string) string { - if strings.Contains(s, "\r\n") { + crlf := strings.Count(s, "\r\n") + lf := strings.Count(s, "\n") - crlf + cr := strings.Count(s, "\r") - crlf + if crlf > 0 && crlf >= lf && crlf >= cr { return "\r\n" } - if strings.Contains(s, "\r") { + if cr > lf { return "\r" } return "\n" } +// eolOf reports the single newline flavor used in s, or "" when s has no +// newlines or mixes flavors. +func eolOf(s string) string { + crlf := strings.Count(s, "\r\n") + lf := strings.Count(s, "\n") - crlf + cr := strings.Count(s, "\r") - crlf + switch { + case crlf > 0 && lf == 0 && cr == 0: + return "\r\n" + case lf > 0 && crlf == 0 && cr == 0: + return "\n" + case cr > 0 && crlf == 0 && lf == 0: + return "\r" + } + return "" +} + // toEOL rewrites every newline in s to the given eol sequence. func toEOL(s, eol string) string { s = strings.ReplaceAll(s, "\r\n", "\n") @@ -394,6 +442,7 @@ func stripReadFileLinePrefixes(s string) (string, bool) { } lines := strings.Split(body, "\n") out := make([]string, 0, len(lines)) + nums := make([]int, 0, len(lines)) for _, line := range lines { i := strings.IndexByte(line, '|') if i <= 0 { @@ -404,8 +453,21 @@ func stripReadFileLinePrefixes(s string) (string, bool) { return s, false } } + n, err := strconv.Atoi(line[:i]) + if err != nil { + return s, false + } + nums = append(nums, n) out = append(out, line[i+1:]) } + // read_file prefixes are always consecutive. A multi-line block whose + // numbers are not is real pipe-delimited data — stripping it could make a + // stale old_string match somewhere else entirely. + for k := 1; k < len(nums); k++ { + if nums[k] != nums[k-1]+1 { + return s, false + } + } joined := strings.Join(out, "\n") if trimTrailing { joined += "\n" @@ -417,34 +479,46 @@ func stripReadFileLinePrefixes(s string) (string, bool) { // the two failure modes that read_file → edit_file commonly hits: // 1. LF vs CRLF (read_file always displays LF) // 2. pasted NUMBER| line prefixes from read_file output -// 3. a unique near-match when new_string only inserts adjacent text // +// The verbatim input is always tried first: when old_string already matches +// the file bytes exactly, no newline heuristic may reject or rewrite it. // how is a short note for the success message when recovery was used; empty on // a plain exact match. func resolveEditMatch(content, oldIn, newIn string) (oldString, newString string, count int, how string) { - eol := fileEOL(content) + // 0. Verbatim bytes. Mixed-EOL files and stray CR bytes made the old + // normalize-first order fail edits whose old_string was byte-perfect. + if c := strings.Count(content, oldIn); c > 0 { + flav := eolOf(oldIn) + if flav == "" { + flav = fileEOL(content) + } + return oldIn, toEOL(newIn, flav), c, "" + } + + // Candidate flavors for normalized matching: the file's dominant flavor + // first, then the alternatives a mixed-EOL file may need. + flavors := []string{fileEOL(content), "\n", "\r\n"} try := func(oldCand, newCand, label string) bool { - o := toEOL(oldCand, eol) - n := toEOL(newCand, eol) - if o == "" { - return false - } - c := strings.Count(content, o) - if c == 0 { - return false + tried := map[string]bool{oldIn: true} // verbatim already attempted + for _, flav := range flavors { + o := toEOL(oldCand, flav) + if o == "" || tried[o] { + continue + } + tried[o] = true + c := strings.Count(content, o) + if c == 0 { + continue + } + oldString, newString, count, how = o, toEOL(newCand, flav), c, label + return true } - oldString, newString, count, how = o, n, c, label - return true + return false } - // 1. Exact / EOL-normalized (covers LF paste against a CRLF file). - if try(oldIn, newIn, "") { - // Only annotate when the on-disk form actually differs from the input - // (i.e. we rewrote newlines). A pure exact match stays silent. - if oldString != oldIn { - how = "normalized line endings to match file" - } + // 1. EOL-normalized (covers LF paste against a CRLF file and vice versa). + if try(oldIn, newIn, "normalized line endings to match file") { return } @@ -461,65 +535,90 @@ func resolveEditMatch(content, oldIn, newIn string) (oldString, newString string } } - // 3. A common README/table operation copies a line from an earlier read, - // abbreviates one phrase, and adds a new row immediately after it. Recover - // only that narrow insertion shape, and only when one file line is a clear - // unique match. The actual on-disk line is retained, so stale wording is not - // silently overwritten. Ordinary replacements remain exact-only. - if oldLine, newLine, ok := resolveAdjacentInsertion(content, oldIn, newIn, eol); ok { - return oldLine, newLine, 1, "matched unique near line for adjacent insertion" - } - return oldIn, newIn, 0, "" } -func resolveAdjacentInsertion(content, oldIn, newIn, eol string) (oldLine, newLine string, ok bool) { +// lineSpan is the [start,end) byte range of one line's text in the original +// content, excluding its \n, \r\n, or lone \r terminator. +type lineSpan struct{ start, end int } + +func lineSpans(content string) []lineSpan { + var spans []lineSpan + start := 0 + i := 0 + for i < len(content) { + switch content[i] { + case '\n': + spans = append(spans, lineSpan{start, i}) + i++ + start = i + case '\r': + spans = append(spans, lineSpan{start, i}) + if i+1 < len(content) && content[i+1] == '\n' { + i += 2 + } else { + i++ + } + start = i + default: + i++ + } + } + if start < len(content) { + spans = append(spans, lineSpan{start, len(content)}) + } + return spans +} + +// spliceAdjacentInsertion recovers one narrow failure shape: a common +// README/table operation copies a line from an earlier read, abbreviates one +// phrase, and adds a new row immediately before or after it. old_string is a +// single stale line, new_string only wraps it with inserted text, and exactly +// one file line is a clear similarity match. The inserted text is spliced at +// that line's byte range: the anchor line is kept byte-for-byte, and no other +// occurrence of similar text can be touched. Ordinary replacements remain +// exact-only. +func spliceAdjacentInsertion(content, oldIn, newIn string) (string, bool) { oldNorm := toEOL(oldIn, "\n") newNorm := toEOL(newIn, "\n") if oldNorm == "" || strings.Contains(oldNorm, "\n") { - return "", "", false + return "", false } - mode := 0 // 1 = insert after, 2 = insert before + insertAfter := false insert := "" - if strings.HasPrefix(newNorm, oldNorm+"\n") { - mode = 1 + switch { + case strings.HasPrefix(newNorm, oldNorm+"\n"): + insertAfter = true insert = strings.TrimPrefix(newNorm, oldNorm) - } else if strings.HasSuffix(newNorm, "\n"+oldNorm) { - mode = 2 + case strings.HasSuffix(newNorm, "\n"+oldNorm): insert = strings.TrimSuffix(newNorm, oldNorm) - } else { - return "", "", false + default: + return "", false } - normalized := strings.ReplaceAll(content, "\r\n", "\n") - normalized = strings.ReplaceAll(normalized, "\r", "\n") - lines := strings.Split(normalized, "\n") + spans := lineSpans(content) best, second := -1.0, -1.0 - bestLine := -1 - for i, line := range lines { - if line == "" && i == len(lines)-1 { - continue - } - score := editLineSimilarity(oldNorm, line) + bestIdx := -1 + for i, sp := range spans { + score := editLineSimilarity(oldNorm, content[sp.start:sp.end]) if score > best { second, best = best, score - bestLine = i + bestIdx = i } else if score > second { second = score } } - if bestLine < 0 || best < 0.78 || (second >= 0 && best-second < 0.12) { - return "", "", false + if bestIdx < 0 || best < 0.78 || (second >= 0 && best-second < 0.12) { + return "", false } - actual := lines[bestLine] - if mode == 1 { - newLine = actual + insert - } else { - newLine = insert + actual + insert = toEOL(insert, fileEOL(content)) + sp := spans[bestIdx] + if insertAfter { + return content[:sp.end] + insert + content[sp.end:], true } - return toEOL(actual, eol), toEOL(newLine, eol), true + return content[:sp.start] + insert + content[sp.start:], true } func editLineSimilarity(a, b string) float64 { diff --git a/internal/tools/file_edit_regression_test.go b/internal/tools/file_edit_regression_test.go index 3ef8d92..b902e07 100644 --- a/internal/tools/file_edit_regression_test.go +++ b/internal/tools/file_edit_regression_test.go @@ -193,6 +193,168 @@ func TestEditFileRecoversUniqueNearInsertionWithoutChangingExistingLine(t *testi } } +// The similarity search picks a unique best line, so the insertion must land +// at that line — not at an earlier occurrence of the same text inside a longer +// line, which strings.Replace-based recovery corrupted mid-line. +func TestEditFileAdjacentInsertionSplicesAtMatchedLine(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "f.txt") + content := "start\nreturn nil // TODO cleanup\nmiddle\nreturn nil\nend\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + // Stale old_string (double space) matches nothing exactly; similarity must + // pick line 4 ("return nil", score 1.0) over line 2 (score 0.5). + old := "return nil" + args, _ := json.Marshal(map[string]any{ + "path": "f.txt", "old_string": old, "new_string": old + "\nINSERTED", + }) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if result.IsError { + t.Fatalf("unique near-line insertion should recover: %s", result.Content) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + want := "start\nreturn nil // TODO cleanup\nmiddle\nreturn nil\nINSERTED\nend\n" + if string(got) != want { + t.Fatalf("insertion landed at the wrong place:\n%s\nwant:\n%s", got, want) + } +} + +// replace_all promises "replace every exact occurrence"; a similarity-based +// recovery must never piggyback on it and multiply insertions. +func TestEditFileAdjacentInsertionIgnoredWithReplaceAll(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "ra.txt") + content := "return nil // TODO cleanup\nmiddle\nreturn nil\nend\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + old := "return nil" + args, _ := json.Marshal(map[string]any{ + "path": "ra.txt", "old_string": old, "new_string": old + "\nINSERTED", "replace_all": true, + }) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if !result.IsError { + t.Fatalf("replace_all must not trigger similarity recovery: %s", result.Content) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != content { + t.Fatalf("file modified by rejected recovery:\n%s", got) + } +} + +// A file with mixed line endings must never reject an old_string whose bytes +// match the file exactly. (fileEOL used to pick CRLF because one line used it, +// then converted the LF old_string so it matched nothing.) +func TestEditFileExactMatchOnMixedEOLFile(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "mixed.txt") + content := "alpha\r\nbeta\nGAMMA\ndelta\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{ + "path": "mixed.txt", "old_string": "beta\nGAMMA", "new_string": "beta\nGAMMA2", + }) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if result.IsError { + t.Fatalf("exact byte match rejected on mixed-EOL file: %s", result.Content) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + want := "alpha\r\nbeta\nGAMMA2\ndelta\n" + if string(got) != want { + t.Fatalf("edited = %q, want %q", got, want) + } +} + +// One stray lone CR byte anywhere in an LF file used to flip fileEOL to "\r" +// and permanently break every multi-line edit in that file. +func TestEditFileExactMatchDespiteStrayCR(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "cr.txt") + content := "one\ntwo\nnote ends\rrest\nfour\nfive\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{ + "path": "cr.txt", "old_string": "four\nfive", "new_string": "four\nFIVE", + }) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if result.IsError { + t.Fatalf("stray CR poisoned an exact match: %s", result.Content) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + want := "one\ntwo\nnote ends\rrest\nfour\nFIVE\n" + if string(got) != want { + t.Fatalf("edited = %q, want %q", got, want) + } +} + +// read_file line numbers are always consecutive, so a multi-line block whose +// numeric prefixes are not sequential is real pipe-delimited data, not a paste. +func TestStripReadFileLinePrefixesRequiresSequentialNumbers(t *testing.T) { + if _, ok := stripReadFileLinePrefixes("3|a\n7|b"); ok { + t.Fatal("non-sequential numeric prefixes must not strip") + } + if _, ok := stripReadFileLinePrefixes("5|x\n5|y"); ok { + t.Fatal("repeated numeric prefixes must not strip") + } + got, ok := stripReadFileLinePrefixes("9|a\n10|b\n11|c") + if !ok || got != "a\nb\nc" { + t.Fatalf("sequential prefixes should strip, got %q ok=%v", got, ok) + } +} + +// Truncating at the byte cap must not cut a multi-byte rune in half and then +// misreport the whole file as binary. +func TestReadFileTruncationDoesNotSplitRune(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "big.txt") + // "é" is 2 bytes; place it so the maxReadBytes cut lands inside it. + content := strings.Repeat("a", maxReadBytes-1) + "é" + strings.Repeat("b", 16) + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + in := Input{Workspace: workspace, Args: []byte(`{"path":"big.txt"}`)} + result := (readFileTool{}).Execute(context.Background(), in) + if result.IsError { + t.Fatalf("truncated UTF-8 file misread as binary: %s", result.Content) + } + if !strings.Contains(result.Content, "file truncated") { + t.Fatalf("missing truncation notice: %s", result.Content) + } +} + +// Classic-Mac style lone CR line endings must display as separate lines, not +// one giant line with embedded CR bytes. +func TestReadFileDisplaysLoneCRLines(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "old.txt") + if err := os.WriteFile(path, []byte("a\rb\rc"), 0o644); err != nil { + t.Fatal(err) + } + in := Input{Workspace: workspace, Args: []byte(`{"path":"old.txt"}`)} + result := (readFileTool{}).Execute(context.Background(), in) + if result.IsError { + t.Fatalf("read failed: %s", result.Content) + } + if !strings.Contains(result.Content, "1|a\n2|b\n3|c") { + t.Fatalf("lone-CR file not split into lines: %q", result.Content) + } +} + func TestEditFileDoesNotRecoverAmbiguousNearInsertion(t *testing.T) { workspace := t.TempDir() path := filepath.Join(workspace, "README.md") From 85c75fe5a0119f801c7fa1390b17de3d8f334f09 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Wed, 12 Aug 2026 17:16:43 +0700 Subject: [PATCH 9/9] Align grep/glob read boundary with read_file and report overlong lines grep and glob refused paths outside the workspace even in project sessions where read_file and list_files may read anywhere, which broke searches over the very files the agent could read. Both now resolve through the same read boundary. grep also reports when a line exceeds the scanner buffer instead of silently presenting the rest of the file as match-free. Co-authored-by: Cursor --- internal/tools/search.go | 30 ++++++++++++----- internal/tools/search_test.go | 63 +++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 8 deletions(-) create mode 100644 internal/tools/search_test.go diff --git a/internal/tools/search.go b/internal/tools/search.go index f7aab36..6c4c082 100644 --- a/internal/tools/search.go +++ b/internal/tools/search.go @@ -47,7 +47,9 @@ func (globTool) Execute(_ context.Context, in Input) Result { if args.Limit <= 0 || args.Limit > 2000 { args.Limit = 200 } - root, err := resolvePath(in.Workspace, args.Path) + // Same read boundary as read_file: workspace-confined in an ordinary + // session, anywhere in a project session. + root, err := resolveRead(in, args.Path) if err != nil { return Errorf("%v", err) } @@ -199,7 +201,9 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { if err != nil { return Errorf("invalid regular expression: %v", err) } - root, err := resolvePath(in.Workspace, args.Path) + // Same read boundary as read_file: workspace-confined in an ordinary + // session, anywhere in a project session. + root, err := resolveRead(in, args.Path) if err != nil { return Errorf("%v", err) } @@ -211,10 +215,11 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { } var ( - b strings.Builder - matches int - files int - stopped bool + b strings.Builder + matches int + files int + stopped bool + warnings []string ) searchFile := func(path, display string) error { @@ -272,6 +277,11 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { } } } + // A line longer than the scanner buffer aborts the scan; say so instead + // of silently reporting the rest of the file as match-free. + if err := sc.Err(); err != nil && len(warnings) < 8 { + warnings = append(warnings, fmt.Sprintf("%s: search stopped at line %d: %v", display, lineNo+1, err)) + } return nil } @@ -307,14 +317,18 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { _ = searchFile(root, relTo(in.Workspace, root)) } + warn := "" + if len(warnings) > 0 { + warn = "\nwarning: " + strings.Join(warnings, "\nwarning: ") + } if matches == 0 { - return Text(fmt.Sprintf("No matches for %q under %s", args.Pattern, relTo(in.Workspace, root))) + return Text(fmt.Sprintf("No matches for %q under %s%s", args.Pattern, relTo(in.Workspace, root), warn)) } header := fmt.Sprintf("%d match(es) in %d file(s) for %q", matches, files, args.Pattern) if stopped { header += " (limit reached)" } - return Text(header + "\n" + b.String()) + return Text(header + "\n" + b.String() + warn) } func truncateLine(s string) string { diff --git a/internal/tools/search_test.go b/internal/tools/search_test.go new file mode 100644 index 0000000..119a423 --- /dev/null +++ b/internal/tools/search_test.go @@ -0,0 +1,63 @@ +package tools + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// grep and glob must follow the same read boundary as read_file/list_files: +// confined to the workspace in an ordinary session, free to search anywhere in +// a project session (WriteRoots set). +func TestGrepAndGlobFollowProjectReadBoundary(t *testing.T) { + project := t.TempDir() + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "ref.txt"), []byte("needle content\n"), 0o644); err != nil { + t.Fatal(err) + } + + grepArgs, _ := json.Marshal(map[string]any{"pattern": "needle", "path": outside}) + projectIn := Input{Workspace: project, WriteRoots: []string{project}, Args: grepArgs} + result := (grepTool{}).Execute(context.Background(), projectIn) + if result.IsError || !strings.Contains(result.Content, "needle content") { + t.Fatalf("project-session grep outside workspace should match, got: %+v", result) + } + + globArgs, _ := json.Marshal(map[string]any{"pattern": "*.txt", "path": outside}) + result = (globTool{}).Execute(context.Background(), Input{Workspace: project, WriteRoots: []string{project}, Args: globArgs}) + if result.IsError || !strings.Contains(result.Content, "ref.txt") { + t.Fatalf("project-session glob outside workspace should match, got: %+v", result) + } + + // Ordinary sessions keep the old confinement. + result = (grepTool{}).Execute(context.Background(), Input{Workspace: project, Args: grepArgs}) + if !result.IsError { + t.Fatalf("ordinary-session grep outside workspace must be refused, got: %+v", result) + } + result = (globTool{}).Execute(context.Background(), Input{Workspace: project, Args: globArgs}) + if !result.IsError { + t.Fatalf("ordinary-session glob outside workspace must be refused, got: %+v", result) + } +} + +// A line longer than the scanner buffer used to stop the file scan silently: +// no matches after it, no report. The tool must surface that the file scan +// stopped early. +func TestGrepReportsOverlongLineInsteadOfSilentStop(t *testing.T) { + workspace := t.TempDir() + content := strings.Repeat("x", 2*1024*1024) + "\nNEEDLE line\n" + if err := os.WriteFile(filepath.Join(workspace, "big.txt"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{"pattern": "NEEDLE", "path": "."}) + result := (grepTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if result.IsError { + t.Fatalf("grep errored: %s", result.Content) + } + if !strings.Contains(result.Content, "big.txt") || !strings.Contains(result.Content, "stopped") { + t.Fatalf("overlong line not reported: %q", result.Content) + } +}