From 45bade96125fc448b8c14cfc0a8ef22a3eb337fb Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 7 Apr 2026 23:31:39 +0200 Subject: [PATCH] feat(chunk): added canonical chunk paths with CRC validation and selector resolution - Added fully-qualified anchor paths in chunk read output showing complete path hierarchy with CRC checksums. - Added exact chunk selector resolution with CRC validation to enforce canonical chunk paths in edit operations. - Added `?` selector option to read syntax-aware chunks by canonical path listing when anchor paths are unclear. - Improved tree-sitter range handling with `end_row_as_line()` function to correctly convert half-open ranges to 1-indexed line numbers. - Extracted anchor label logic into `chunk_anchor_label()` const function and consolidated style-based selection. - Removed --dev flag support from native build pipeline and dev:native npm scripts. --- crates/pi-natives/src/chunk/common.rs | 20 ++- crates/pi-natives/src/chunk/edit.rs | 151 ++++++++++++++++-- crates/pi-natives/src/chunk/mod.rs | 119 +++++++++++++- crates/pi-natives/src/chunk/render.rs | 19 ++- crates/pi-natives/src/chunk/resolve.rs | 63 +++++++- crates/pi-natives/src/chunk/state.rs | 22 ++- package.json | 1 - packages/coding-agent/CHANGELOG.md | 6 + .../src/prompts/tools/chunk-edit.md | 5 +- .../src/prompts/tools/read-chunk.md | 5 +- .../coding-agent/test/core/chunk-tree.test.ts | 109 ++++++------- .../test/tools/chunk-mode.test.ts | 111 +++++++++++-- packages/natives/CHANGELOG.md | 3 +- packages/natives/package.json | 1 - packages/natives/scripts/build-native.ts | 9 +- 15 files changed, 516 insertions(+), 128 deletions(-) diff --git a/crates/pi-natives/src/chunk/common.rs b/crates/pi-natives/src/chunk/common.rs index 0d6fd8bf1..1f931df5e 100644 --- a/crates/pi-natives/src/chunk/common.rs +++ b/crates/pi-natives/src/chunk/common.rs @@ -68,6 +68,24 @@ pub struct ChunkAccumulator { // ── Candidate constructors ─────────────────────────────────────────────── +/// Convert a tree-sitter `end_position` into a 1-indexed line number. +/// +/// Tree-sitter byte ranges are half-open, so `end_position` points to the byte +/// immediately *after* the last byte of the node. When that byte lands at +/// column 0 of a new row, the node's last byte actually sits on the previous +/// row and the 1-indexed last line is exactly `end.row` — not `end.row + 1`. +/// This matters for grammars whose container nodes terminate on the start of +/// the next sibling (tree-sitter-markdown sections, tree-sitter-toml tables, +/// etc.): the naive `end.row + 1` would claim the sibling's heading line and +/// make `replace_range_by_lines` clobber it. +const fn end_row_as_line(start: tree_sitter::Point, end: tree_sitter::Point) -> usize { + if end.column == 0 && end.row > start.row { + end.row + } else { + end.row + 1 + } +} + pub fn make_candidate<'tree>( node: Node<'tree>, base_name: String, @@ -89,7 +107,7 @@ pub fn make_candidate<'tree>( range_end_byte: node.end_byte(), checksum_start_byte: start_byte, range_start_line: start.row + 1, - range_end_line: end.row + 1, + range_end_line: end_row_as_line(start, end), signature: summary, error: name_style == NameStyle::Error, groupable: matches!(name_style, NameStyle::Group), diff --git a/crates/pi-natives/src/chunk/edit.rs b/crates/pi-natives/src/chunk/edit.rs index b8e4306b7..0b95ce022 100644 --- a/crates/pi-natives/src/chunk/edit.rs +++ b/crates/pi-natives/src/chunk/edit.rs @@ -6,7 +6,8 @@ use crate::chunk::{ normalize_leading_whitespace_char, reindent_inserted_block, strip_content_prefixes, }, resolve::{ - resolve_chunk_selector, resolve_chunk_with_crc, sanitize_chunk_selector, sanitize_crc, + resolve_exact_chunk_selector, resolve_exact_chunk_with_crc, sanitize_chunk_selector, + sanitize_crc, }, state::{ChunkState, ChunkStateInner}, types::{ @@ -62,9 +63,7 @@ pub fn apply_edits(state: &ChunkState, params: &EditParams) -> Result Result Result apply_delete( &mut state, &operation, &scheduled, - current_default_selector.as_deref(), + current_default_selector, current_default_crc.as_deref(), &mut touched_paths, - &mut warnings, ), ChunkEditOp::AppendChild | ChunkEditOp::PrependChild @@ -150,7 +147,7 @@ pub fn apply_edits(state: &ChunkState, params: &EditParams) -> Result, - warnings: &mut Vec, ) -> Result<(), String> { let anchor_selector = operation.sel.as_deref().or(default_selector); let crc = operation.crc.as_deref().or_else(|| { @@ -265,7 +261,7 @@ fn apply_replace( // When auto-accepted, strip CRC so resolution finds by path only (CRC is stale // from pre-batch). let resolve_crc = if batch_auto_accepted { None } else { crc }; - let resolved = resolve_chunk_with_crc(state, anchor_selector, resolve_crc, warnings)?; + let resolved = resolve_exact_chunk_with_crc(state, anchor_selector, resolve_crc)?; if !batch_auto_accepted { validate_batch_crc(resolved.chunk, resolved.crc.as_deref(), requires_checksum)?; } @@ -346,7 +342,6 @@ fn apply_delete( default_selector: Option<&str>, default_crc: Option<&str>, touched_paths: &mut Vec, - warnings: &mut Vec, ) -> Result<(), String> { let anchor_selector = operation.sel.as_deref().or(default_selector); let crc = operation.crc.as_deref().or_else(|| { @@ -359,7 +354,7 @@ fn apply_delete( let requires_checksum = operation.sel.is_some() || default_crc.is_some(); 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, anchor_selector, resolve_crc, warnings)?; + let resolved = resolve_exact_chunk_with_crc(state, anchor_selector, resolve_crc)?; if !batch_auto_accepted { validate_batch_crc(resolved.chunk, resolved.crc.as_deref(), requires_checksum)?; } @@ -394,7 +389,7 @@ fn apply_insert( }); 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, anchor_selector, resolve_crc, warnings)?; + let resolved = resolve_exact_chunk_with_crc(state, anchor_selector, resolve_crc)?; if !batch_auto_accepted { validate_batch_crc(resolved.chunk, resolved.crc.as_deref(), resolved.crc.is_some())?; } @@ -1339,4 +1334,130 @@ mod tests { result.diff_after ); } + + #[test] + fn edit_rejects_non_canonical_chunk_paths() { + let source = "class Worker { + run(): void { + console.log(this.name); + } +} +"; + let state = state_for(source, "typescript"); + let chunk = state + .inner() + .chunk("class_Worker.fn_run") + .expect("class_Worker.fn_run should exist"); + + let err = apply_edits(&state, &EditParams { + operations: vec![EditOperation { + op: ChunkEditOp::Replace, + sel: Some("run".to_owned()), + crc: Some(chunk.checksum.clone()), + content: Some( + "run(): void { + console.log(this.name); +}" + .to_owned(), + ), + line: None, + end_line: None, + }], + default_selector: None, + default_crc: None, + anchor_style: None, + cwd: ".".to_owned(), + file_path: "test.ts".to_owned(), + }) + .err() + .expect("edit should reject non-canonical selector"); + + assert!(err.contains("Chunk path not found: \"run\""), "{err}"); + } + + #[test] + fn selector_less_root_checksum_targets_file_root() { + let source = "# Contributing to uLua + +## Building and Testing + +Use just. + +## Code Style + +Use clang-format. +"; + let state = state_for(source, "markdown"); + let root = state.inner().chunk("").expect("root chunk should exist"); + let duplicate_count = state + .chunks() + .into_iter() + .filter(|chunk| chunk.checksum == root.checksum) + .count(); + assert!(duplicate_count > 1, "expected duplicate root checksum fixture"); + + let result = apply_edits(&state, &EditParams { + operations: vec![EditOperation { + op: ChunkEditOp::Replace, + sel: Some(String::new()), + crc: Some(root.checksum.clone()), + content: Some("# Updated guide".to_owned()), + line: Some(1), + end_line: Some(1), + }], + default_selector: None, + default_crc: None, + anchor_style: None, + cwd: ".".to_owned(), + file_path: "CONTRIBUTING.md".to_owned(), + }) + .expect("root checksum target should resolve the file root"); + + assert!( + result.diff_after.starts_with( + "# Updated guide +" + ), + "{}", + result.diff_after + ); + } + + #[test] + fn markdown_section_replace_preserves_next_sibling_heading() { + let source = "# Top\n\n## Building\n\nOld content.\n\n## Code Style\n\n- style one\n"; + let state = state_for(source, "markdown"); + let chunk = state + .inner() + .chunk("section_Top.section_Building") + .expect("section_Building"); + + let result = apply_edits(&state, &EditParams { + operations: vec![EditOperation { + op: ChunkEditOp::Replace, + sel: Some("section_Top.section_Building".to_owned()), + crc: Some(chunk.checksum.clone()), + content: Some("## Building\n\nNew content.\n".to_owned()), + line: None, + end_line: None, + }], + default_selector: None, + default_crc: None, + anchor_style: None, + cwd: ".".to_owned(), + file_path: "test.md".to_owned(), + }) + .expect("replace should succeed"); + + assert!( + result.diff_after.contains("## Code Style"), + "next sibling heading must survive section replace, got:\n{}", + result.diff_after, + ); + assert!( + result.diff_after.contains("New content."), + "replacement content must be present, got:\n{}", + result.diff_after, + ); + } } diff --git a/crates/pi-natives/src/chunk/mod.rs b/crates/pi-natives/src/chunk/mod.rs index 3cb8137a5..3f982867f 100644 --- a/crates/pi-natives/src/chunk/mod.rs +++ b/crates/pi-natives/src/chunk/mod.rs @@ -1728,6 +1728,30 @@ func (s *Server) GetAddress() string { assert!(!result.text.contains("return 1")); } + #[test] + fn read_renders_full_chunk_paths_in_full_anchor_style() { + let source = "class Worker { + run(): void { + work(); + } +} +"; + let state = ChunkState::parse(source.to_string(), "typescript".to_string()) + .expect("state should parse"); + let result = state + .render_read(ReadRenderParams { + read_path: "sample.ts".to_string(), + display_path: "sample.ts".to_string(), + language_tag: Some("ts".to_string()), + omit_checksum: false, + anchor_style: Some(ChunkAnchorStyle::Full), + absolute_line_range: None, + tab_replacement: Some(" ".to_string()), + }) + .expect("root read should succeed"); + assert!(result.text.contains("[class_Worker.fn_run#"), "{}", result.text); + } + #[test] fn read_missing_chunk_returns_error_with_suggestions() { let filler = (0..60) @@ -1740,7 +1764,7 @@ func (s *Server) GetAddress() string { try {{\n work();\n }} catch (error) {{\n throw error;\n }}\n}}\n" ); let state = ChunkState::parse(source, "typescript".to_string()).expect("state should parse"); - let error = state + let result = state .render_read(ReadRenderParams { read_path: "sample.ts:fn_loadSkills.try_2".to_string(), display_path: "sample.ts".to_string(), @@ -1750,12 +1774,15 @@ func (s *Server) GetAddress() string { absolute_line_range: None, tab_replacement: Some(" ".to_string()), }) - .err() - .expect("missing chunk should surface as an error"); - let message = error.to_string(); - assert!(message.contains("Chunk path not found: \"fn_loadSkills.try_2\""), "{message}"); - assert!(message.contains("Direct children of \"fn_loadSkills\""), "{message}"); - assert!(message.contains("fn_loadSkills.try"), "{message}"); + .expect("render_read should succeed"); + + let chunk = result.chunk.expect("should have a chunk target"); + assert_eq!(chunk.status, super::types::ChunkReadStatus::NotFound); + + let text = &result.text; + assert!(text.contains("Chunk path not found: \"fn_loadSkills.try_2\""), "{text}"); + assert!(text.contains("Direct children of \"fn_loadSkills\""), "{text}"); + assert!(text.contains("fn_loadSkills.try"), "{text}"); } #[test] @@ -2088,4 +2115,82 @@ end module.children ); } + + #[test] + fn adjacent_markdown_sections_do_not_overlap() { + let source = "# Top\n\n## A\n\na body\n\n## B\n\nb body\n\n## C\n\nc body\n"; + let tree = build_chunk_tree(source, "markdown").expect("markdown tree"); + + let a = tree + .chunks + .iter() + .find(|c| c.path == "section_Top.section_A") + .expect("section_A"); + let b = tree + .chunks + .iter() + .find(|c| c.path == "section_Top.section_B") + .expect("section_B"); + let c = tree + .chunks + .iter() + .find(|c| c.path == "section_Top.section_C") + .expect("section_C"); + + assert!( + a.end_line < b.start_line, + "section_A ({}-{}) must not overlap section_B ({}-{})", + a.start_line, + a.end_line, + b.start_line, + b.end_line, + ); + assert!( + b.end_line < c.start_line, + "section_B ({}-{}) must not overlap section_C ({}-{})", + b.start_line, + b.end_line, + c.start_line, + c.end_line, + ); + } + + #[test] + fn adjacent_toml_tables_do_not_overlap() { + let source = "[package]\nname = \"x\"\n\n[deps]\na = 1\n\n[tool]\nb = 2\n"; + let tree = build_chunk_tree(source, "toml").expect("toml tree"); + + let package = tree + .chunks + .iter() + .find(|c| c.path == "table_package") + .expect("table_package"); + let deps = tree + .chunks + .iter() + .find(|c| c.path == "table_deps") + .expect("table_deps"); + let tool = tree + .chunks + .iter() + .find(|c| c.path == "table_tool") + .expect("table_tool"); + + assert!( + package.end_line < deps.start_line, + "table_package ({}-{}) must not overlap table_deps ({}-{})", + package.start_line, + package.end_line, + deps.start_line, + deps.end_line, + ); + assert!( + deps.end_line < tool.start_line, + "table_deps ({}-{}) must not overlap table_tool ({}-{})", + deps.start_line, + deps.end_line, + tool.start_line, + tool.end_line, + ); + } } diff --git a/crates/pi-natives/src/chunk/render.rs b/crates/pi-natives/src/chunk/render.rs index 3ba7e96f3..b4c771667 100644 --- a/crates/pi-natives/src/chunk/render.rs +++ b/crates/pi-natives/src/chunk/render.rs @@ -236,6 +236,16 @@ fn chunk_body_anchor_indent( .map_or(String::new(), |line| leading_whitespace(line).replace('\t', tab_replacement)) } +const fn chunk_anchor_label(chunk: &ChunkNode, style: ChunkAnchorStyle) -> &str { + match style { + ChunkAnchorStyle::Full | ChunkAnchorStyle::FullOmit => chunk.path.as_str(), + ChunkAnchorStyle::Kind + | ChunkAnchorStyle::KindOmit + | ChunkAnchorStyle::Bare + | ChunkAnchorStyle::None => chunk.name.as_str(), + } +} + #[derive(Clone, Copy)] struct VisibleSpan { start: u32, @@ -619,7 +629,8 @@ fn emit_chunk_subtree( if !chunk.path.is_empty() { let anchor_indent = chunk_body_anchor_indent(ctx.source_lines, chunk, ctx.tab_replacement); let style = ctx.anchor_style.with_omit_checksum(ctx.omit_checksum); - push_meta(ctx, style.render(&anchor_indent, chunk.name.as_str(), chunk.checksum.as_str())); + let anchor_label = chunk_anchor_label(chunk, style); + push_meta(ctx, style.render(&anchor_indent, anchor_label, chunk.checksum.as_str())); } if !has_kids { if ctx.show_leaf_preview @@ -657,10 +668,8 @@ fn emit_chunk_subtree( if !chunk.path.is_empty() && chunk.line_count > 1 { let anchor_indent = chunk_body_anchor_indent(ctx.source_lines, chunk, ctx.tab_replacement); let style = ctx.anchor_style.with_omit_checksum(ctx.omit_checksum); - push_meta( - ctx, - style.render_close(&anchor_indent, chunk.name.as_str(), chunk.checksum.as_str()), - ); + let anchor_label = chunk_anchor_label(chunk, style); + push_meta(ctx, style.render_close(&anchor_indent, anchor_label, chunk.checksum.as_str())); } return; } diff --git a/crates/pi-natives/src/chunk/resolve.rs b/crates/pi-natives/src/chunk/resolve.rs index a6dce75e4..d0531dd22 100644 --- a/crates/pi-natives/src/chunk/resolve.rs +++ b/crates/pi-natives/src/chunk/resolve.rs @@ -75,6 +75,14 @@ pub fn resolve_chunk_selector<'a>( resolve_chunk_selector_impl(state, cleaned_selector.as_deref(), cleaned_crc.as_deref(), warnings) } +pub fn resolve_exact_chunk_selector<'a>( + state: &'a ChunkStateInner, + selector: Option<&str>, +) -> Result<&'a ChunkNode, String> { + let (cleaned_selector, cleaned_crc) = split_selector_and_crc(selector, None); + resolve_chunk_selector_exact_impl(state, cleaned_selector.as_deref(), cleaned_crc.as_deref()) +} + pub fn resolve_chunk_with_crc<'a>( state: &'a ChunkStateInner, selector: Option<&str>, @@ -94,6 +102,35 @@ pub fn resolve_chunk_with_crc<'a>( Ok(ResolvedChunk { chunk, crc: cleaned_crc }) } +pub fn resolve_exact_chunk_with_crc<'a>( + state: &'a ChunkStateInner, + selector: Option<&str>, + crc: Option<&str>, +) -> Result, String> { + let (cleaned_selector, cleaned_crc) = split_selector_and_crc(selector, crc); + + if cleaned_selector.is_none() { + let chunk = root_chunk(state)?; + if let Some(cleaned_crc) = cleaned_crc.as_deref() { + if chunk.checksum != cleaned_crc { + return Err(format!( + "Root checksum \"{cleaned_crc}\" did not match the current file root. Re-read the \ + file and copy the root checksum from the header line." + )); + } + return Ok(ResolvedChunk { chunk, crc: Some(cleaned_crc.to_owned()) }); + } + return Ok(ResolvedChunk { chunk, crc: None }); + } + + let chunk = resolve_chunk_selector_exact_impl( + state, + cleaned_selector.as_deref(), + cleaned_crc.as_deref(), + )?; + Ok(ResolvedChunk { chunk, crc: cleaned_crc }) +} + pub fn resolve_chunk_by_checksum<'a>( state: &'a ChunkStateInner, crc: &str, @@ -122,6 +159,12 @@ pub fn resolve_chunk_by_checksum<'a>( } } +fn root_chunk(state: &ChunkStateInner) -> Result<&ChunkNode, String> { + state + .chunk("") + .ok_or_else(|| "Chunk tree is missing the root chunk".to_owned()) +} + fn resolve_chunk_selector_impl<'a>( state: &'a ChunkStateInner, selector: Option<&str>, @@ -129,9 +172,7 @@ fn resolve_chunk_selector_impl<'a>( warnings: &mut Vec, ) -> Result<&'a ChunkNode, String> { let Some(cleaned) = selector else { - return state - .chunk("") - .ok_or_else(|| "Chunk tree is missing the root chunk".to_owned()); + return root_chunk(state); }; if let Some(chunk) = state.chunk(cleaned) { @@ -204,6 +245,22 @@ fn resolve_chunk_selector_impl<'a>( Err(build_not_found_error(state.tree(), cleaned)) } +fn resolve_chunk_selector_exact_impl<'a>( + state: &'a ChunkStateInner, + selector: Option<&str>, + crc: Option<&str>, +) -> Result<&'a ChunkNode, String> { + let Some(cleaned) = selector else { + return root_chunk(state); + }; + + let Some(chunk) = state.chunk(cleaned) else { + return Err(build_not_found_error(state.tree(), cleaned)); + }; + + match_crc_filter(cleaned, vec![chunk], crc) +} + fn match_crc_filter<'a>( cleaned: &str, matches: Vec<&'a ChunkNode>, diff --git a/crates/pi-natives/src/chunk/state.rs b/crates/pi-natives/src/chunk/state.rs index 8db1ef7b2..22dd22330 100644 --- a/crates/pi-natives/src/chunk/state.rs +++ b/crates/pi-natives/src/chunk/state.rs @@ -378,14 +378,20 @@ impl ChunkState { } let mut warnings = Vec::new(); - let Ok(resolved) = - resolve_chunk_with_crc(self.inner(), selector.as_deref(), crc.as_deref(), &mut warnings) - else { - let sel = selector.unwrap_or_default(); - return Ok(ReadResult { - text: format!("{}:{}\n\n[Chunk not found]", params.display_path, sel), - chunk: Some(ChunkReadTarget { status: ChunkReadStatus::NotFound, selector: sel }), - }); + let resolved = match resolve_chunk_with_crc( + self.inner(), + selector.as_deref(), + crc.as_deref(), + &mut warnings, + ) { + Ok(resolved) => resolved, + Err(err) => { + let sel = selector.unwrap_or_default(); + return Ok(ReadResult { + text: format!("{}:{}\n\n{}", params.display_path, sel, err), + chunk: Some(ChunkReadTarget { status: ChunkReadStatus::NotFound, selector: sel }), + }); + }, }; let chunk = resolved.chunk; diff --git a/package.json b/package.json index bfd8e0723..577f5cc86 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,6 @@ "fix:ts": "biome check --write --unsafe . && bun --cwd=packages/coding-agent run format-prompts && bun --cwd=packages/coding-agent run generate-docs-index", "fix:rs": "cargo fmt --all && cargo clippy --fix --allow-dirty --all-targets --no-deps --allow-staged --broken-code --allow-no-vcs", "build:native": "bun --cwd=packages/natives run build:native", - "dev:native": "bun --cwd=packages/natives run dev:native", "bench:gen-fixtures": "bun --cwd=packages/typescript-edit-benchmark run src/generate.ts --typescript-dir /tmp/typescript-source --count-per-type 8", "bench:edit": "bun --cwd=packages/typescript-edit-benchmark run start", "prepublishOnly": "bun run check", diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 73cda80e5..d0174152f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## [Unreleased] + ### Added - Chunk read formatting: `anchorStyle` (full / kind / bare), `read.anchorstyle` setting, and `chunked` flag on file display mode @@ -14,6 +15,9 @@ ### Changed +- Chunk read output now displays fully-qualified anchor paths (e.g., `[class_Worker.fn_run#CRC]`) instead of bare names, making targets unambiguous for edits +- Chunk edit tool documentation clarified: `target` must be the fully-qualified path with `#CRC` suffix; added guidance to run `read(path="file", sel="?")` for canonical target listings when anchor style is unclear +- Chunk read tool documentation updated: `sel` parameter now documents the `?` selector for canonical target listings, and clarifies that default output shows full paths - Chunk edit schema and tool contract: explicit `op` (`replace`, `delete`, `append`, `prepend`, `after`, `before`); sibling inserts use `anchor` instead of separate after/before target fields; `replace` supports optional `line` / `end_line` for line-scoped edits (former splice-style behavior); insert ops omit CRC where appropriate, mutations require checksum on target - Chunk path handling: parse selector and CRC separately, sanitize selectors (strip filename prefixes, uppercase checksums), accept embedded `#CRC` on targets, auto-accept stale CRC for later ops in the same batch on the same chunk - Chunk UX: streaming and final edit previews show chunk edits next to hashline edits with op-specific labels; prompt docs shortened with rules table, `…` in examples, and helper-based path/anchor samples @@ -34,6 +38,8 @@ ### Fixed +- Chunk edit error messages now consistently report checksum mismatches with the format `did not match checksum "XXXX"` instead of variable phrasing +- Chunk selector validation for edits now rejects non-canonical selectors (suffix-only like `fn_run` or prefix-stripped like `run`), requiring fully-qualified paths to prevent ambiguity - Plan review previews now re-append at the chat tail on refresh, keeping them adjacent to the active selector instead of updating off-screen - `log_experiment` validates and reverts run-scoped file changes without clobbering unrelated dirty worktree state - Chunk edit targets that embed CRC in the selector (e.g. `fn_foo#ABCD`) parse correctly diff --git a/packages/coding-agent/src/prompts/tools/chunk-edit.md b/packages/coding-agent/src/prompts/tools/chunk-edit.md index b00223764..d8e7c01e4 100644 --- a/packages/coding-agent/src/prompts/tools/chunk-edit.md +++ b/packages/coding-agent/src/prompts/tools/chunk-edit.md @@ -1,8 +1,9 @@ -Edits files via syntax-aware chunks. Run `read(path="file.ts")` first — it returns chunks with anchors `[name#CRC]` where `#CRC` is a 4-char hex checksum. Copy the full chunk path AND `#CRC` verbatim into `target`. +Edits files via syntax-aware chunks. Run `read(path="file.ts")` first — the default read output shows anchors like `[full.chunk.path#CRC]`, where `#CRC` is a 4-char hex checksum. Copy that exact `full.chunk.path#CRC` into `target`. - **MUST** `read` first. NEVER invent chunk names or CRCs — copy them from the latest read output or edit response. - `target` **MUST** be the **fully-qualified** path (e.g. `class_X.fn_y.if_2`, not `if_2`), ending with `#CRC` for replace/delete. +- If the exact path is unclear, or your anchor style omits full paths, run `read(path="file", sel="?")` and copy a canonical target from that listing. - Prefer `line`/`end_line` (absolute file line numbers from the read gutter) for small fixes over whole-chunk replace. - `content` must include the destination block's inner indentation. - Successful edits return refreshed anchors — use them for follow-ups, don't re-read just for new CRCs. @@ -16,7 +17,7 @@ Edits files via syntax-aware chunks. Run `read(path="file.ts")` first — it ret |`append` / `prepend`|`target`, `content`|insert as last/first child of target| |`after` / `before`|`target`, `anchor` (child name), `content`|insert at sibling position| -For file-root edits, `target` is the file CRC alone (e.g. `"#VSKB"`). +For file-root edits, `target` is the file header CRC alone (e.g. `"#VSKB"`). diff --git a/packages/coding-agent/src/prompts/tools/read-chunk.md b/packages/coding-agent/src/prompts/tools/read-chunk.md index 2522bcf76..2c2574d6f 100644 --- a/packages/coding-agent/src/prompts/tools/read-chunk.md +++ b/packages/coding-agent/src/prompts/tools/read-chunk.md @@ -2,10 +2,11 @@ Reads files using syntax-aware chunks. - `path` — file path or URL; may include `:selector` suffix -- `sel` — optional selector: `class_Foo`, `class_Foo.fn_bar`, `L50`, `L50-L120`, or `raw` +- `sel` — optional selector: `class_Foo`, `class_Foo.fn_bar`, `?`, `L50`, `L50-L120`, or `raw` - `timeout` — seconds, for URLs only -Each anchor `[name#CCCC]` in the output is a chunk ID. Copy `name#CCCC` into the edit tool's `target` field. +Each anchor `[full.chunk.path#CCCC]` in the default output is an exact chunk ID. Copy `full.chunk.path#CCCC` into the edit tool's `target` field. +If you need a canonical target list, or your anchor style omits full paths, run `read(path="file", sel="?")` and copy a path from that listing. Line numbers in the gutter are absolute — use them for `line`/`end_line` in edits. Chunk trees: JS, TS, TSX, Python, Rust, Go. Others use blank-line fallback. diff --git a/packages/coding-agent/test/core/chunk-tree.test.ts b/packages/coding-agent/test/core/chunk-tree.test.ts index 34a8898e1..f52b2a96a 100644 --- a/packages/coding-agent/test/core/chunk-tree.test.ts +++ b/packages/coding-agent/test/core/chunk-tree.test.ts @@ -124,7 +124,7 @@ describe("applyChunkEdits", () => { content: "replacement", }, ]), - ).toThrow(/mismatch/i); + ).toThrow(/did not match checksum/); }); test("append_child on branch inserts after existing members", () => { @@ -427,7 +427,7 @@ describe("edit safety invariants", () => { for (const operation of ["replace", "delete", "line-scoped replace"] as const) { test(`rejects stale checksum for ${operation} with current and provided checksums in the error`, () => { - const { source, staleChecksum, currentChecksum } = buildStaleRunFixture(); + const { source, staleChecksum } = buildStaleRunFixture(); const invoke = () => { if (operation === "replace") { @@ -461,7 +461,7 @@ describe("edit safety invariants", () => { ); }; - expect(invoke).toThrow(new RegExp(`expected "${currentChecksum}", got "${staleChecksum}"`)); + expect(invoke).toThrow(new RegExp(`did not match checksum "${staleChecksum}"`)); }); } @@ -582,7 +582,7 @@ describe("edit safety invariants", () => { crc: "ZZZZ", }, ]), - ).toThrow(/Edit operation 2\/2 failed.*Checksum mismatch/s); + ).toThrow(/Edit operation 2\/2 failed.*did not match checksum/s); }); test("keeps untouched sibling checksums stable after a nearby edit", () => { @@ -696,7 +696,7 @@ describe("formatChunkedRead", () => { expect(result.text).toContain("worker.ts·"); expect(result.text).toContain("[class_Worker#"); - expect(result.text).toContain("[fn_run#"); + expect(result.text).toContain("[class_Worker.fn_run#"); expect(result.text).not.toContain("ck:"); expect(result.text).not.toContain("[branch"); }); @@ -730,6 +730,7 @@ describe("formatChunkedRead", () => { }); expect(result.text).toContain("worker.ts:class_Worker.fn_run·"); + 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 {"); @@ -840,8 +841,8 @@ describe("addressable member rendering", () => { }); expect(result.text).toContain("[enum_LogLevel#"); - expect(result.text).toContain("[variant_Debug#"); - expect(result.text).toContain("[variant_Error#"); + expect(result.text).toContain("[enum_LogLevel.variant_Debug#"); + expect(result.text).toContain("[enum_LogLevel.variant_Error#"); }); test("renders a single-method Go interface inline on the parent chunk", async () => { @@ -880,9 +881,9 @@ describe("addressable member rendering", () => { }); expect(result.text).toContain("[type_Server#"); - expect(result.text).toContain("[field_Addr#"); - expect(result.text).toContain("[fn_Start#"); - expect(result.text).toContain("[fn_Stop#"); + expect(result.text).toContain("[type_Server.field_Addr#"); + expect(result.text).toContain("[type_Server.fn_Start#"); + expect(result.text).toContain("[type_Server.fn_Stop#"); }); test("line range filter shows receiver methods under a type even when the range skips the type header", async () => { @@ -901,8 +902,8 @@ describe("addressable member rendering", () => { }); expect(result.text).toContain("L7-L8"); - expect(result.text).toContain("[fn_Start#"); - expect(result.text).toContain("[fn_Stop#"); + expect(result.text).toContain("[type_Server.fn_Start#"); + expect(result.text).toContain("[type_Server.fn_Stop#"); }); test("renders trivial TypeScript enum variants as addressable children", async () => { @@ -918,8 +919,8 @@ describe("addressable member rendering", () => { }); expect(result.text).toContain("[enum_Status#"); - expect(result.text).toContain("[variant_Idle#"); - expect(result.text).toContain("[variant_Busy#"); + expect(result.text).toContain("[enum_Status.variant_Idle#"); + expect(result.text).toContain("[enum_Status.variant_Busy#"); }); test("keeps non-trivial containers expanded", () => { @@ -955,8 +956,8 @@ describe("grouped Go receiver chunk headers", () => { }); expect(result.text).toContain("server.go:type_Server·6L"); - expect(result.text).toContain("[fn_Start#"); - expect(result.text).toContain("[fn_Stop#"); + expect(result.text).toContain("[type_Server.fn_Start#"); + expect(result.text).toContain("[type_Server.fn_Stop#"); }); }); @@ -1103,7 +1104,7 @@ describe("Go receiver render ownership", () => { expect(result.responseText).toContain("func DefaultServer() *Server"); expect(result.responseText).toContain("[fn_DefaultServer#"); - expect(result.responseText).toContain("[fn_Start#"); + expect(result.responseText).toContain("[type_Server.fn_Start#"); }); }); @@ -1234,7 +1235,7 @@ describe("splice", () => { content: "replacement", }, ]), - ).toThrow(/mismatch/i); + ).toThrow(/did not match checksum/); }); test("splice rejects reversed ranges that are not zero-width gaps", () => { @@ -1303,51 +1304,31 @@ describe("prepend_child warnings", () => { }); }); -describe("chunk selector auto-resolution", () => { - test("warns on suffix auto-resolution", () => { - const result = edit([ - { - op: "replace", - sel: "fn_run", - crc: getChecksum(testSource, "class_Worker.fn_run"), - 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); - }); - - test("warns on prefix auto-resolution", () => { - const result = edit([ - { - op: "replace", - sel: "run", - crc: getChecksum(testSource, "class_Worker.fn_run"), - 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, - ); - }); - - test("errors on ambiguous suffix matches", () => { - // Source with two identically-named nested chunks under different parents - const source = `class Foo {\n\trun(): void {}\n}\nclass Bar {\n\trun(): void {}\n}\n`; +describe("chunk selector validation for edits", () => { + test("rejects suffix-only edit selectors", () => { expect(() => - applyEdit({ - source, - language: "typescript", - operations: [ - { - op: "delete", - sel: "fn_run", - crc: getChecksum(source, "class_Foo.fn_run"), - }, - ], - }), - ).toThrow(/Ambiguous chunk selector "fn_run" matches 2 chunks/); + edit([ + { + op: "replace", + sel: "fn_run", + crc: getChecksum(testSource, "class_Worker.fn_run"), + content: `run(): void {\n\tconsole.log(this.name);\n}`, + }, + ]), + ).toThrow(/Chunk path not found: "fn_run"/); + }); + + test("rejects prefix-stripped edit selectors", () => { + expect(() => + edit([ + { + op: "replace", + sel: "run", + crc: getChecksum(testSource, "class_Worker.fn_run"), + content: `run(): void {\n\tconsole.log(this.name);\n}`, + }, + ]), + ).toThrow(/Chunk path not found: "run"/); }); }); @@ -1394,7 +1375,7 @@ describe("tlaplus chunk rendering", () => { language: "tlaplus", }); - expect(result.text).toContain("[translation_12#"); + expect(result.text).toContain("[mod_Spec.translation_12#"); expect(result.text).toContain("\\* [translation hidden]"); expect(result.text).not.toContain("Next == pc' = pc"); }); @@ -1415,7 +1396,7 @@ describe("tlaplus chunk rendering", () => { }); expect(result.diffSourceAfter).toContain("Start == x = 0"); - expect(result.responseText).toContain("[translation_12#"); + expect(result.responseText).toContain("[mod_Spec.translation_12#"); expect(result.responseText).toContain("\\* [translation hidden]"); expect(result.responseText).not.toContain("Next == pc' = pc"); }); diff --git a/packages/coding-agent/test/tools/chunk-mode.test.ts b/packages/coding-agent/test/tools/chunk-mode.test.ts index 5a84e8e2e..c52d63436 100644 --- a/packages/coding-agent/test/tools/chunk-mode.test.ts +++ b/packages/coding-agent/test/tools/chunk-mode.test.ts @@ -110,8 +110,8 @@ describe("chunk mode tools", () => { expect(text).toContain("[Notice: chunk view scoped to requested lines L2-L4; non-overlapping lines omitted.]"); expect(text).toContain("server.ts·"); - expect(text).toContain("[fn_handleError#"); - expect(text).toContain("[var_total#"); + expect(text).toContain("[class_Server.fn_handleError#"); + expect(text).toContain("[class_Server.fn_handleError.var_total#"); expect(text).toContain("3|"); expect(text).toContain("4|"); expect(text).not.toContain("⋯"); @@ -127,9 +127,9 @@ describe("chunk mode tools", () => { expect(text).toContain("[Notice: chunk view scoped to requested lines L2-L4; non-overlapping lines omitted.]"); expect(text).toContain("server.ts·"); - expect(text).toContain("[fn_handleError#"); - expect(text).toContain("[var_total#"); - expect(text).toContain("[var_total#"); + expect(text).toContain("[class_Server.fn_handleError#"); + expect(text).toContain("[class_Server.fn_handleError.var_total#"); + expect(text).toContain("[class_Server.fn_handleError.var_total#"); expect(text).toContain("3|"); }); @@ -372,7 +372,7 @@ describe("chunk mode tools", () => { path: `${filePath}:class_Server.fn_missing`, }); - expect(getText(result)).toContain("[Chunk not found]"); + expect(getText(result)).toContain("Chunk path not found"); expect(result.details?.chunk).toEqual({ status: ChunkReadStatus.NotFound, selector: "class_Server.fn_missing", @@ -498,7 +498,7 @@ describe("chunk mode tools", () => { expect(await Bun.file(filePath).text()).toBe(originalSource); }); - it("auto-resolves chunk selectors with missing name prefixes", async () => { + it("rejects non-canonical edit selectors", async () => { const filePath = path.join(tmpDir, "server.ts"); const originalSource = buildLargeTypescriptFixture(); await Bun.write(filePath, originalSource); @@ -506,13 +506,100 @@ describe("chunk mode tools", () => { const editTool = new EditTool(session); const checksum = getChunkChecksum(originalSource, "typescript", "fn_main"); - // Use bare "main" instead of "fn_main" - const _result = await editTool.execute("chunk-edit-prefix-resolve", { + await expect( + editTool.execute("chunk-edit-prefix-resolve", { + path: filePath, + edits: [{ target: `main#${checksum}`, content: 'function main(): void {\n console.log("started");\n}\n' }], + }), + ).rejects.toThrow(/Chunk path not found: "main"/); + expect(await Bun.file(filePath).text()).toBe(originalSource); + }); + + it("resolves root-only checksum targets to the file root", async () => { + const filePath = path.join(tmpDir, "CONTRIBUTING.md"); + const source = `# Contributing to uLua + +## Building and Testing + +Use just. + +## Code Style + +Use clang-format. +`; + await Bun.write(filePath, source); + const session = createSession(tmpDir); + const editTool = new EditTool(session); + const language = getLanguageFromPath(filePath); + if (!language) { + throw new Error("expected markdown language"); + } + const state = ChunkState.parse(source, language); + const root = state.root(); + if (!root) { + throw new Error("expected root chunk"); + } + expect(state.chunks().filter(chunk => chunk.checksum === root.checksum).length).toBeGreaterThan(1); + + const _result = await editTool.execute("chunk-edit-root-checksum", { path: filePath, - edits: [{ target: `main#${checksum}`, content: 'function main(): void {\n console.log("started");\n}\n' }], + edits: [{ target: `#${root.checksum}`, line: 1, end_line: 1, content: "# Updated guide" }], }); + const updatedSource = await Bun.file(filePath).text(); - expect(updatedSource).toContain('console.log("started")'); - expect(updatedSource).not.toContain('console.log("boot")'); + expect(updatedSource.startsWith("# Updated guide\n")).toBe(true); + expect(updatedSource).toContain("## Building and Testing"); + }); + + it("preserves sibling headings when replacing a whole markdown section", async () => { + const filePath = path.join(tmpDir, "CONTRIBUTING.md"); + const source = [ + "# Contributing to uLua", + "", + "## Building and Testing", + "", + "```bash", + "cmake -S . -B build", + "```", + "", + "## Code Style", + "", + "- Follow .clang-format.", + "", + "## Commit Messages", + "", + "- Use imperative mood.", + "", + ].join("\n"); + await Bun.write(filePath, source); + const session = createSession(tmpDir); + const editTool = new EditTool(session); + const language = getLanguageFromPath(filePath); + if (!language) { + throw new Error("expected markdown language"); + } + const state = ChunkState.parse(source, language); + const building = state + .chunks() + .find(chunk => chunk.path === "section_Contributing_to_uLua.section_Building_and_Testing"); + if (!building) { + throw new Error("expected section_Building_and_Testing chunk"); + } + + await editTool.execute("chunk-edit-section-replace", { + path: filePath, + edits: [ + { + target: `${building.path}#${building.checksum}`, + content: "## Building and Testing\n\nUse `just verify` instead. It wraps cmake and ctest.\n", + }, + ], + }); + + const updatedSource = await Bun.file(filePath).text(); + expect(updatedSource).toContain("## Code Style"); + expect(updatedSource).toContain("## Commit Messages"); + expect(updatedSource).toContain("Use `just verify`"); + expect(updatedSource).not.toContain("cmake -S . -B build"); }); }); diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index 9d19c17de..a6d045815 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -1,7 +1,6 @@ # Changelog ## [Unreleased] - ### Breaking Changes - Moved package entry point from `src/index.ts` to `native/index.js` — consumers must update imports to use the new native module path @@ -17,6 +16,7 @@ ### Changed +- Simplified native build pipeline by removing `--dev` flag support; debug builds no longer available through npm scripts - Updated native module loader to check `XDG_DATA_HOME` environment variable for native addon location before falling back to `~/.omp/natives` - Removed native binding validation function that checked for required exports at load time - Refactored build pipeline to use napi-rs generated bindings instead of hand-written TypeScript wrappers @@ -25,6 +25,7 @@ ### Removed +- Removed `dev:native` npm script — use `build:native` for all build scenarios - Removed inline pi-utils helpers and dependency on `@oh-my-pi/pi-utils` from native module loader - Removed `logger.time()` wrapper calls from native module loading - Removed all TypeScript wrapper modules from `src/` directory (appearance, ast, chunk, clipboard, glob, grep, highlight, html, image, keys, projfs, ps, pty, shell, text, work) diff --git a/packages/natives/package.json b/packages/natives/package.json index 4b38f9eb8..3a47588da 100644 --- a/packages/natives/package.json +++ b/packages/natives/package.json @@ -30,7 +30,6 @@ "types": "./native/index.d.ts", "scripts": { "build:native": "bun scripts/build-native.ts", - "dev:native": "bun scripts/build-native.ts --dev", "embed:native": "bun scripts/embed-native.ts", "check": "biome check . && tsgo -p tsconfig.json", "fix": "biome check --write --unsafe .", diff --git a/packages/natives/scripts/build-native.ts b/packages/natives/scripts/build-native.ts index 26214608b..25d4a5ab0 100644 --- a/packages/natives/scripts/build-native.ts +++ b/packages/natives/scripts/build-native.ts @@ -8,7 +8,6 @@ const rustDir = path.join(repoRoot, "crates/pi-natives"); const nativeDir = path.join(import.meta.dir, "../native"); const packageJsonPath = path.join(import.meta.dir, "../package.json"); -const isDev = process.argv.includes("--dev"); const crossTarget = Bun.env.CROSS_TARGET; const targetPlatform = Bun.env.TARGET_PLATFORM || process.platform; const targetArch = Bun.env.TARGET_ARCH || process.arch; @@ -173,7 +172,7 @@ async function resolveBuiltAddonPath(canonicalFilename: string): Promise } const isCI = Boolean(Bun.env.CI); -const useLocalProfile = !isDev && !isCI && !isCrossCompile; +const useLocalProfile = !isCI && !isCrossCompile; // Build napi args const napiArgs = [ @@ -190,9 +189,7 @@ const napiArgs = [ nativeDir, ]; -if (isDev) { - // napi build defaults to debug, no flag needed -} else if (useLocalProfile) { +if (useLocalProfile) { napiArgs.push("--profile", "local"); } else { napiArgs.push("--release"); @@ -200,7 +197,7 @@ if (isDev) { if (crossTarget) napiArgs.push("--target", crossTarget); -const profileLabel = isDev ? " (debug)" : useLocalProfile ? " (local)" : ""; +const profileLabel = useLocalProfile ? " (local)" : ""; const canonicalAddonFilename = `pi_natives.${targetPlatform}-${targetArch}${variantSuffix}.node`; const canonicalAddonPath = path.join(nativeDir, canonicalAddonFilename);