From 58c0d52679a3ea6f020a7a0d1ef08c2e68cc02fc Mon Sep 17 00:00:00 2001 From: can1357 Date: Wed, 8 Apr 2026 06:30:50 +0200 Subject: [PATCH] refactor: restructured chunk tree initialization and extraction logic for clarity - Refactored chunk tree initialization to use explicit prologue/epilogue byte boundaries instead of None values. - Extracted chunk resolution logic to use split_selector_crc_and_region() helper for improved code reuse. - Reorganized package.json exports across multiple modules (autoresearch, cli, dap, edit, modes) for better API surface clarity. - Updated test expectations for Go receiver method rendering and chunk selector formats to match refactored output. - Consolidated notebook conversion logic and removed unused _createErrorToolResult helper function. - Improved leading trivia preservation in chunk edits by detecting and maintaining line prefix whitespace. --- crates/pi-natives/src/chunk/ast_ipynb.rs | 4 +- crates/pi-natives/src/chunk/edit.rs | 42 ++++++++--- crates/pi-natives/src/chunk/mod.rs | 8 +-- packages/coding-agent/CHANGELOG.md | 18 ++++- packages/coding-agent/package.json | 72 +++++++++++++++---- packages/coding-agent/src/edit/index.ts | 1 + .../coding-agent/src/modes/rpc/host-tools.ts | 7 -- packages/coding-agent/src/tools/read.ts | 4 +- .../coding-agent/test/core/chunk-tree.test.ts | 67 +++++++++-------- .../coding-agent/test/core/hashline.test.ts | 7 +- packages/coding-agent/test/tools.test.ts | 4 +- .../test/tools/chunk-mode.test.ts | 15 ++-- packages/natives/package.json | 7 +- 13 files changed, 174 insertions(+), 82 deletions(-) diff --git a/crates/pi-natives/src/chunk/ast_ipynb.rs b/crates/pi-natives/src/chunk/ast_ipynb.rs index ff6e28687..04f9c75e5 100644 --- a/crates/pi-natives/src/chunk/ast_ipynb.rs +++ b/crates/pi-natives/src/chunk/ast_ipynb.rs @@ -406,8 +406,8 @@ pub fn build_notebook_tree_from_virtual( start_byte: 0, end_byte: virtual_source.len() as u32, checksum_start_byte: 0, - prologue_end_byte: None, - epilogue_start_byte: None, + prologue_end_byte: Some(0), + epilogue_start_byte: Some(virtual_source.len() as u32), checksum: root_checksum.clone(), error: false, indent: 0, diff --git a/crates/pi-natives/src/chunk/edit.rs b/crates/pi-natives/src/chunk/edit.rs index 3e550d2ff..6419d4e94 100644 --- a/crates/pi-natives/src/chunk/edit.rs +++ b/crates/pi-natives/src/chunk/edit.rs @@ -7,7 +7,7 @@ use crate::chunk::{ }, resolve::{ chunk_region_range, chunk_supports_region, resolve_chunk_selector, resolve_chunk_with_crc, - sanitize_chunk_selector, sanitize_crc, + sanitize_chunk_selector, sanitize_crc, split_selector_crc_and_region, }, state::{ChunkState, ChunkStateInner}, types::{ @@ -284,10 +284,18 @@ fn resolve_edit_target( None } }); - let batch_auto_accepted = ensure_batch_operation_target_current(scheduled, crc, touched_paths); - let resolve_crc = if batch_auto_accepted { None } else { crc }; - let resolved = resolve_chunk_with_crc(state, selector, resolve_crc, warnings)?; - let region = operation.region.unwrap_or(resolved.region); + let (cleaned_selector, cleaned_crc, parsed_region) = + split_selector_crc_and_region(selector, crc, operation.region)?; + let batch_auto_accepted = + ensure_batch_operation_target_current(scheduled, cleaned_crc.as_deref(), touched_paths); + let resolve_crc = if batch_auto_accepted { + None + } else { + cleaned_crc.as_deref() + }; + let resolved = + resolve_chunk_with_crc(state, cleaned_selector.as_deref(), resolve_crc, warnings)?; + let region = operation.region.unwrap_or(parsed_region); if !batch_auto_accepted { validate_batch_crc(resolved.chunk, resolved.crc.as_deref(), requires_checksum)?; } @@ -696,16 +704,32 @@ fn preserve_attached_leading_trivia( return replacement.to_owned(); } - let leading_trivia = &state.source[trivia_start..trivia_end]; + let line_start = state.source[..trivia_start] + .rfind('\n') + .map_or(0, |pos| pos + 1); + let line_prefix = &state.source[line_start..trivia_start]; + let mut leading_trivia = + if !line_prefix.is_empty() && line_prefix.chars().all(|ch| matches!(ch, ' ' | '\t')) { + format!("{line_prefix}{}", &state.source[trivia_start..trivia_end]) + } else { + state.source[trivia_start..trivia_end].to_owned() + }; + if let Some(last_newline) = leading_trivia.rfind('\n') + && leading_trivia[last_newline + 1..] + .chars() + .all(|ch| matches!(ch, ' ' | '\t' | '\r')) + { + leading_trivia.truncate(last_newline + 1); + } if leading_trivia.trim().is_empty() - || replacement.starts_with(leading_trivia) - || replacement_supplies_leading_trivia(leading_trivia, replacement) + || replacement.starts_with(&leading_trivia) + || replacement_supplies_leading_trivia(&leading_trivia, replacement) { return replacement.to_owned(); } let mut combined = String::with_capacity(leading_trivia.len() + replacement.len()); - combined.push_str(leading_trivia); + combined.push_str(&leading_trivia); combined.push_str(replacement); combined } diff --git a/crates/pi-natives/src/chunk/mod.rs b/crates/pi-natives/src/chunk/mod.rs index 08e39a45d..ad200ed68 100644 --- a/crates/pi-natives/src/chunk/mod.rs +++ b/crates/pi-natives/src/chunk/mod.rs @@ -136,8 +136,8 @@ pub(crate) fn build_chunk_tree(source: &str, language: &str) -> Result; }; -function _createErrorToolResult(message: string): AgentToolResult { - return { - content: [{ type: "text", text: message }], - details: {}, - }; -} - function isAgentToolResult(value: unknown): value is AgentToolResult { if (!value || typeof value !== "object") return false; const content = (value as { content?: unknown }).content; diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index ee04d734e..8b8881610 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -818,6 +818,8 @@ export class ReadTool implements AgentTool { const language = getLanguageFromPath(absolutePath); const skipChunksForExplore = !hasEditTool && !this.session.settings.get("read.explorechunks"); const skipChunksForProse = isProseLanguage(language) && !this.session.settings.get("read.prosechunks"); + const shouldConvertWithMarkit = + CONVERTIBLE_EXTENSIONS.has(ext) || (ext === ".ipynb" && (parsed.kind === "raw" || !chunkMode)); if (chunkMode && parsed.kind !== "raw" && !skipChunksForExplore && !skipChunksForProse) { const absoluteLineRange = @@ -928,7 +930,7 @@ export class ReadTool implements AgentTool { throw error; } } - } else if (CONVERTIBLE_EXTENSIONS.has(ext)) { + } else if (shouldConvertWithMarkit) { // Convert document or notebook via markit. const result = await convertFileWithMarkit(absolutePath, signal); if (result.ok) { diff --git a/packages/coding-agent/test/core/chunk-tree.test.ts b/packages/coding-agent/test/core/chunk-tree.test.ts index be59b7394..782a266cc 100644 --- a/packages/coding-agent/test/core/chunk-tree.test.ts +++ b/packages/coding-agent/test/core/chunk-tree.test.ts @@ -397,7 +397,7 @@ type Server struct { expect(result.diffSourceAfter).not.toMatch(/Addr string\n[ \t]+func \(s \*Server\) Ping/); }); - test("append on Go type_Server container keeps receiver method body indentation relative to column 0", () => { + test("append on Go type_Server container keeps receiver method body indentation relative to the anchor chunk", () => { const source = `package main type Server struct { @@ -419,9 +419,9 @@ type Server struct { expect(result.parseValid).toBe(true); expect(result.diffSourceAfter).toContain( - "\nfunc (s *Server) LogCount() int {\n\ts.mu.Lock()\n\tdefer s.mu.Unlock()\n\treturn 0\n}\n", + "\n\tfunc (s *Server) LogCount() int {\n\t\ts.mu.Lock()\n\t\tdefer s.mu.Unlock()\n\t\treturn 0\n\t}\n", ); - expect(result.diffSourceAfter).not.toContain("\n\tfunc (s *Server) LogCount() int {"); + expect(result.diffSourceAfter).not.toContain("\nfunc (s *Server) LogCount() int {"); }); test("keeps append separated from the closing delimiter when adding the last child", () => { const result = edit([ @@ -728,7 +728,7 @@ describe("formatChunkedRead", () => { expect(result.text).toMatch(new RegExp(`with-tail\\.ts·${totalLines}L`)); }); - test("leaf read shows absolute file lines and raw source indentation", async () => { + test("leaf read shows absolute file lines and canonical tab indentation", async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "chunk-tree-core-")); const filePath = path.join(tmpDir, "worker.ts"); await Bun.write(filePath, testSource); @@ -740,12 +740,10 @@ describe("formatChunkedRead", () => { language: "typescript", }); - expect(result.text).toContain("worker.ts:class_Worker.fn_run·"); + expect(result.text).toContain("worker.ts:class_Worker.fn_run@container·"); expect(result.text).toContain("[class_Worker.fn_run#"); - expect(result.text).toContain("run(): void {"); - expect(result.text).toContain("6| "); - expect(result.text).toContain("run(): void {"); - expect(result.text).toContain("7| "); + expect(result.text).toContain("6| \trun(): void {"); + expect(result.text).toContain("7| \t\tconsole.log(this.name);"); expect(result.text).toContain("console.log(this.name);"); }); @@ -763,8 +761,9 @@ describe("formatChunkedRead", () => { }); expect(result.text).not.toContain("to expand ⋮"); - expect(result.text).toContain("3| step(0);"); - expect(result.text).toContain("27| step(24);"); + expect(result.text).toContain("service.ts:class_Service.fn_handle@container·"); + expect(result.text).toContain("3| \t\tstep(0);"); + expect(result.text).toContain("27| \t\tstep(24);"); expect(result.text).toContain("done();"); }); }); @@ -873,10 +872,10 @@ describe("addressable member rendering", () => { expect(result.text).toContain("[type_Handler#"); expect(result.text).toContain("3| type Handler interface {"); - expect(result.text).toContain("4| Handle(method, path string) Result"); + expect(result.text).toContain("4| \tHandle(method, path string) Result"); }); - test("renders Go receiver methods beneath their receiver type", async () => { + test("renders Go receiver methods as top-level siblings", async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "chunk-tree-core-")); const filePath = path.join(tmpDir, "server.go"); await Bun.write( @@ -893,11 +892,13 @@ describe("addressable member rendering", () => { expect(result.text).toContain("[type_Server#"); expect(result.text).toContain("[type_Server.field_Addr#"); - expect(result.text).toContain("[type_Server.fn_Start#"); - expect(result.text).toContain("[type_Server.fn_Stop#"); + expect(result.text).toContain("[fn_Start#"); + expect(result.text).toContain("[fn_Stop#"); + expect(result.text).not.toContain("[type_Server.fn_Start#"); + expect(result.text).not.toContain("[type_Server.fn_Stop#"); }); - test("line range filter shows receiver methods under a type even when the range skips the type header", async () => { + test("line range filter shows top-level receiver methods even when the range skips the type header", async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "chunk-tree-core-")); const filePath = path.join(tmpDir, "server.go"); await Bun.write( @@ -913,8 +914,9 @@ describe("addressable member rendering", () => { }); expect(result.text).toContain("L7-L8"); - expect(result.text).toContain("[type_Server.fn_Start#"); - expect(result.text).toContain("[type_Server.fn_Stop#"); + expect(result.text).toContain("[fn_Start#"); + expect(result.text).toContain("[fn_Stop#"); + expect(result.text).not.toContain("[type_Server.fn_Start#"); }); test("renders trivial TypeScript enum variants as addressable children", async () => { @@ -950,8 +952,8 @@ describe("addressable member rendering", () => { }); }); -describe("grouped Go receiver chunk headers", () => { - test("reports grouped receiver chunk line counts from rendered lines", async () => { +describe("Go type chunk headers", () => { + test("reports Go type chunk line counts from the type body only", async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "chunk-tree-core-")); const filePath = path.join(tmpDir, "server.go"); await Bun.write( @@ -966,9 +968,9 @@ describe("grouped Go receiver chunk headers", () => { language: "go", }); - expect(result.text).toContain("server.go:type_Server·6L"); - expect(result.text).toContain("[type_Server.fn_Start#"); - expect(result.text).toContain("[type_Server.fn_Stop#"); + expect(result.text).toContain("server.go:type_Server@container·3L"); + expect(result.text).not.toContain("[fn_Start#"); + expect(result.text).not.toContain("[fn_Stop#"); }); }); @@ -992,7 +994,7 @@ describe("addressable member editing", () => { expect(result.diffSourceAfter).not.toContain('Busy = "busy"'); }); - test("after inserts beside an individually addressable enum variant", () => { + test("after inserts beside an individually addressable enum variant without extra blank lines", () => { const result = edit( [ { @@ -1004,7 +1006,7 @@ describe("addressable member editing", () => { enumSource, ); - expect(result.diffSourceAfter).toContain(' Idle = "idle",\n\n Paused = "paused",\n\n Busy = "busy",'); + expect(result.diffSourceAfter).toContain(' Idle = "idle",\n Paused = "paused",\n Busy = "busy",'); }); test("replace with empty content removes an individually addressable enum variant", () => { @@ -1017,7 +1019,7 @@ describe("addressable member editing", () => { }); describe("Go receiver render ownership", () => { - test("omits unrelated top-level siblings from grouped receiver output", () => { + test("before inserts beside a top-level Go receiver method", () => { const source = `package main\n\ntype Server struct {\n Addr string\n}\n\nfunc (s *Server) Start() {}\nfunc (s Server) Stop() {}\n`; const result = applyEdit({ source, @@ -1026,7 +1028,7 @@ describe("Go receiver render ownership", () => { operations: [ { op: "before", - sel: "type_Server.fn_Start", + sel: "fn_Start", content: "func DefaultServer() *Server {\n return &Server{}\n}", }, ], @@ -1034,7 +1036,8 @@ describe("Go receiver render ownership", () => { expect(result.responseText).toContain("func DefaultServer() *Server"); expect(result.responseText).toContain("[fn_DefaultServer#"); - expect(result.responseText).toContain("[type_Server.fn_Start#"); + expect(result.responseText).toContain("[fn_Start#"); + expect(result.responseText).not.toContain("[type_Server.fn_Start#"); }); }); @@ -1096,9 +1099,7 @@ describe("chunk selector auto-resolution", () => { content: "run(): void {\n\tconsole.log(this.name);\n}", }, ]); - expect( - result.warnings.some(w => w.includes('Auto-resolved chunk selector "fn_run" to "class_Worker.fn_run"')), - ).toBe(true); + expect(result.warnings.join("\n")).toMatch(/Auto-resolved chunk selector "fn_run" to "class_Worker\.fn_run#/); }); test("warns on prefix auto-resolution", () => { @@ -1109,9 +1110,7 @@ describe("chunk selector auto-resolution", () => { content: "run(): void {\n\tconsole.log(this.name);\n}", }, ]); - expect(result.warnings.some(w => w.includes('Auto-resolved chunk selector "run" to "class_Worker.fn_run"'))).toBe( - true, - ); + expect(result.warnings.join("\n")).toMatch(/Auto-resolved chunk selector "run" to "class_Worker\.fn_run#/); }); test("errors on ambiguous suffix matches", () => { diff --git a/packages/coding-agent/test/core/hashline.test.ts b/packages/coding-agent/test/core/hashline.test.ts index cc0c89d29..8e8929f18 100644 --- a/packages/coding-agent/test/core/hashline.test.ts +++ b/packages/coding-agent/test/core/hashline.test.ts @@ -12,10 +12,13 @@ import { stripNewLinePrefixes, validateLineRef, } from "@oh-my-pi/pi-coding-agent/edit"; -import { type Anchor, formatLineTag, type HashlineEdit } from "@oh-my-pi/pi-coding-agent/edit/modes/hashline"; +import type { Anchor, HashlineEdit } from "@oh-my-pi/pi-coding-agent/edit/modes/hashline"; function makeTag(line: number, content: string): Anchor { - return parseTag(formatLineTag(line, content)); + return { + line, + hash: computeLineHash(line, content), + }; } // ═══════════════════════════════════════════════════════════════════════════ diff --git a/packages/coding-agent/test/tools.test.ts b/packages/coding-agent/test/tools.test.ts index f1e117f0c..1b14d497c 100644 --- a/packages/coding-agent/test/tools.test.ts +++ b/packages/coding-agent/test/tools.test.ts @@ -266,7 +266,7 @@ describe("Coding Agent Tools", () => { expect(result.details?.truncation).toBeUndefined(); }); - it("should convert ipynb files through markit before rendering", async () => { + it("should convert ipynb files through markit for raw reads", async () => { const notebookPath = path.join(testDir, "notebook.ipynb"); const notebook = { cells: [ @@ -287,7 +287,7 @@ describe("Coding Agent Tools", () => { content: "# Notebook Title\n\nNotebook body\n", }); - const result = await readTool.execute("test-call-ipynb", { path: notebookPath }); + const result = await readTool.execute("test-call-ipynb", { path: notebookPath, sel: "raw" }); const output = getTextOutput(result); expect(convertSpy).toHaveBeenCalledTimes(1); diff --git a/packages/coding-agent/test/tools/chunk-mode.test.ts b/packages/coding-agent/test/tools/chunk-mode.test.ts index 38800a784..d06347675 100644 --- a/packages/coding-agent/test/tools/chunk-mode.test.ts +++ b/packages/coding-agent/test/tools/chunk-mode.test.ts @@ -4,7 +4,7 @@ import * as os from "node:os"; import * as path from "node:path"; import { _resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { EditTool } from "@oh-my-pi/pi-coding-agent/edit"; -import { HASHLINE_NIBBLE_ALPHABET } from "@oh-my-pi/pi-coding-agent/edit/modes/hashline"; +import { HASHLINE_NIBBLE_ALPHABET } from "@oh-my-pi/pi-coding-agent/edit/line-hash"; import { getLanguageFromPath } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import { GrepTool } from "@oh-my-pi/pi-coding-agent/tools/grep"; @@ -106,8 +106,9 @@ describe("chunk mode tools", () => { const text = getText(result); expect(text).not.toContain("to expand ⋮"); + expect(text).toContain("server.ts:class_Server.fn_handleError@container·"); expect(text).toContain("let total = 0;"); - expect(text).toContain("29| total += 25;"); + expect(text).toContain("29| \t\t\ttotal += 25;"); expect(text).toContain("return err.message + total;"); }); @@ -157,7 +158,7 @@ describe("chunk mode tools", () => { }); const text = getText(result); - expect(text).toContain("server.ts:class_Server.fn_handleError·"); + expect(text).toContain("server.ts:class_Server.fn_handleError@container·"); expect(text).not.toContain("[Warning: checksum #"); }); @@ -308,7 +309,13 @@ describe("chunk mode tools", () => { await editTool.execute("chunk-edit-string-content", { path: filePath, - edits: [{ target: "class_Server", op: "append", content: 'status(): string {\n return "ok";\n}\n' }], + edits: [ + { + target: "class_Server@body", + op: "append", + content: 'status(): string {\n return "ok";\n}\n', + }, + ], } as never); const updatedSource = await Bun.file(filePath).text(); diff --git a/packages/natives/package.json b/packages/natives/package.json index 3a47588da..5a4650de4 100644 --- a/packages/natives/package.json +++ b/packages/natives/package.json @@ -36,18 +36,17 @@ "test": "bun run build:native && bun test", "bench": "bun bench/grep.ts" }, - "devDependencies": { "@napi-rs/cli": "3.6.0", "@types/bun": "^1.3" }, + "engines": { + "bun": ">=1.3.7" + }, "napi": { "binaryName": "pi_natives", "triples": {} }, - "engines": { - "bun": ">=1.3.7" - }, "files": [ "src", "native",