feat(tools): replace edit_file with hashline edit and session recovery - #743
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces hashline-based ChangesHashline editing and tool integration
Canonical tool names and application flows
Explicit boxed tool futures
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthewyjiang
force-pushed
the
feat/hashline-edit-tool
branch
from
August 5, 2026 01:12
0a6a13a to
69aa8dc
Compare
matthewyjiang
changed the title
matthewyjiang
force-pushed
the
feat/hashline-edit-tool
branch
2 times, most recently
from
August 5, 2026 05:42
2ad6f58 to
6063cf1
Compare
matthewyjiang
marked this pull request as ready for review
August 5, 2026 09:29
Owner
Author
|
@coderabbitai review |
Contributor
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (11)
crates/rho/src/agent/agent_tests.rs (1)
38-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover every compatibility alias.
This test covers only
write_file. The parser also mapsedit_fileandapply_patchtoToolCapability::Edit. Add assertions that both aliases produce the canonicaleditcapability and the same fingerprint asedit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho/src/agent/agent_tests.rs` around lines 38 - 54, Extend write_file_capability_alias_matches_write with parsed definitions using the edit_file and apply_patch tool names, then assert each has the same fingerprint as a canonical edit definition. Also assert ToolCapability::parse for both aliases returns the canonical "edit" string, covering all compatibility aliases.crates/rho/src/app/interactive_presenter_tests.rs (1)
70-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompare the notice with the exported constant.
Line 73 locks the test to a copy fragment. Compare
text.as_str()withrho_tools::hashline::EDIT_DOCUMENT_ONLY_NOTICEinstead.As per coding guidelines, “Do not lock copy behind string-contains tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho/src/app/interactive_presenter_tests.rs` around lines 70 - 74, Update the fact assertion in the interactive presenter test to compare text.as_str() directly with rho_tools::hashline::EDIT_DOCUMENT_ONLY_NOTICE, replacing the string-contains check while preserving the existing Meta fact matching.Source: Coding guidelines
docs/dev/hashline-edit-eval.md (1)
31-31: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winScope Suite J to non-structural chaining.
The workspace contract requires a fresh read after structural edits because those previews contain no numbered body lines. State that Suite J uses a non-structural first edit, and add a separate structural-edit re-read case.
As per workspace tool contract, structural edits require a fresh read before further anchored operations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dev/hashline-edit-eval.md` at line 31, Update the Suite J documentation to specify that its first edit is non-structural and its chained second edit uses the post-edit preview tag and lines. Add a separate case covering a structural first edit that requires a fresh read before any further anchored operation, reflecting the workspace tool contract.crates/rho-tools/src/hashline/apply.rs (1)
163-190: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd guards for
start == 0andend < startinadd_span.
apply_opsispub(crate)and accepts a caller-builtVec<Op>. The parser validates ranges today, butadd_spandoes not. Two malformed inputs behave badly:
start == 0creates a slot thatemitnever visits, becauseemitstarts at index 1. The op is dropped silently.end < startmakesemitsetindex = span.end + 1, which is less than or equal to the slot key. Thewhileloop then re-enters the same slot and never advances, so the blocking task hangs.A cheap range check keeps the module fail-closed for every caller.
🛡️ Proposed guard
if line_count == 0 { return Err(ApplyError::message( "cannot replace or delete lines in an empty file", )); } + if start == 0 || end < start { + return Err(ApplyError::message(format!( + "invalid line range {start}.={end}; ranges are 1-indexed and must not end before they start" + ))); + } if start > line_count || end > line_count {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho-tools/src/hashline/apply.rs` around lines 163 - 190, Update add_span to reject ranges with start == 0 or end < start before inserting into self.slots, returning an ApplyError consistent with the existing invalid-range checks. Preserve the current line_count bounds validation and ensure malformed caller-provided operations fail closed instead of being stored for emit.crates/rho-tools/src/hashline/format_tests.rs (1)
92-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests lock user-facing copy behind string-contains assertions. The three sites assert literal preview text, footer text, and tool-description text. A copy change breaks each test without a behavior change. Export the strings as constants next to the code that produces them, then assert against those constants or compare whole objects.
crates/rho-tools/src/hashline/format_tests.rs#L92-L106: replace the "structural edit" and "no chainable body lines" substring assertions with a constant exported fromformat.rs; keep the assertion that numbered body lines are absent.crates/rho-tools/src/hashline/format_tests.rs#L126-L135: replace the "showing" footer substring assertion with the exported footer template or anassert_eqon the whole snapshot.crates/rho-tools/src/hashline/mod_tests.rs#L270-L277: drop the description length and "PUT 12:" / "never `PUT 12.:`" substring assertions; keepassert_eq!(spec.name, "edit")and cover locator rules inparser_tests.rs.As per coding guidelines: "Do not test static constants or removed behavior, and do not lock copy behind string-contains tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho-tools/src/hashline/format_tests.rs` around lines 92 - 106, Export the relevant preview and footer strings from format.rs and update crates/rho-tools/src/hashline/format_tests.rs lines 92-106 to assert against those constants while retaining the check that numbered body lines are absent; update crates/rho-tools/src/hashline/format_tests.rs lines 126-135 to use the exported footer template or compare the complete snapshot; in crates/rho-tools/src/hashline/mod_tests.rs lines 270-277, remove description-length and “PUT 12:” substring assertions, retain assert_eq!(spec.name, "edit"), and cover locator rules in parser_tests.rs.Source: Coding guidelines
crates/rho-tools/src/write_file.rs (1)
194-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not assert on the notice wording.
assert!(result.content.contains("showing"))locks the footer copy. A wording change informat_chain_snapshotthen breaks this test without any contract change. The surrounding assertions on1:line-1,80:line-80, and the absent40:line-40already prove that the snapshot is bounded. Remove this assertion, or replace it with a structural check such as the count of emittedN:rows.As per coding guidelines: "Do not test static constants or removed behavior, and do not lock copy behind string-contains tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho-tools/src/write_file.rs` around lines 194 - 198, Remove the assertion checking result.content for the literal “showing” text in the write-file test. Keep the existing line-presence and absence assertions around format_chain_snapshot, which already verify truncation structurally; do not replace them with another wording-dependent check.Source: Coding guidelines
crates/rho-tools/src/read_file_tests.rs (1)
133-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the ranged output as a literal.
The expectation is produced by
format_hashline_view, the same function that produces the value under test. A defect in the formatter satisfies both sides and the test still passes. Assert the exact expected string instead, for example the[sample.txt#TAG]header, the2:twoand3:threerows, and the window footer. The tag can stay computed withcompute_file_hash.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho-tools/src/read_file_tests.rs` around lines 133 - 152, The test’s expected value in ranged_plain_text_reads_use_hashline_window is generated by the formatter under test, making it unable to catch formatter defects. Keep compute_file_hash available for the dynamic tag, but replace format_hashline_view with a literal expected string containing the sample.txt hashline header, the 2:two and 3:three rows, and the range window footer.crates/rho-tools/src/sdk_adapter_tests.rs (1)
462-474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the capability contents, not only the count.
assert_eq!(prepared.capabilities().len(), 4)passes for any four requests. Two read requests fora.txtand none forb.txtwould also satisfy it. Assert that each ofa.txtandb.txthas one read request and one write request, in the same loop that already checks the accesses.As per coding guidelines: "Prefer
pretty_assertions::assert_eqand whole-object comparisons when available in Rust tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho-tools/src/sdk_adapter_tests.rs` around lines 462 - 474, Update the capability assertions in the existing loop over a.txt and b.txt to verify each path has exactly one read request and one write request, rather than relying on prepared.capabilities().len(). Use pretty_assertions::assert_eq with whole-object comparisons where the existing capability representation supports it, and remove or replace the count-only assertion.Source: Coding guidelines
crates/rho-tools/src/hashline/proposed_tests.rs (1)
108-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompare the projected rows as one object.
The test checks row membership and uses
removed >= 2/added >= 1. A whole-object comparison of the(kind, text)row vector and offile.statslocks the exact projection and catches ordering or duplication regressions. The first test in this file already uses that shape.As per coding guidelines: "Prefer
pretty_assertions::assert_eqand whole-object comparisons when available in Rust tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho-tools/src/hashline/proposed_tests.rs` around lines 108 - 131, Update the test assertions around the projected rows and stats to use whole-object comparisons, matching the pattern established by the first test in the file. Compare the complete ordered `(kind, text)` row vector and compare `file.stats` exactly, replacing the membership checks and lower-bound assertions while preserving the expected projection.Source: Coding guidelines
crates/rho-tools/src/read_file.rs (1)
131-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the image branch into a helper.
read_file_contentnow holds four branches and about 130 lines: ranged text, image preview, plain UTF-8, and rich document. The image branch (lines 131-194) is self-contained and carries its own size limits, spawn-blocking call, and three fallback outcomes. Move it into a privateread_image_content(file, display_path, source_len, header)helper. The remaining function then reads as a short dispatch.As per coding guidelines: "Avoid large Rust files by extracting separable behavior into focused modules, keeping tests and invariant documentation close to implementation."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho-tools/src/read_file.rs` around lines 131 - 194, Extract the self-contained image-preview branch from read_file_content into a private read_image_content helper accepting file, display_path, source_len, and header. Move its size checks, thumbnail_png spawn_blocking call, and fallback handling unchanged, then have read_file_content dispatch to the helper when supported_image_mime_type matches while leaving the text and document paths intact.Source: Coding guidelines
crates/rho-tools/src/sdk_adapter.rs (1)
465-486: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse the write resolver for existing edit targets.
resolve_existingusesresolve_for_read, andresolve_for_writehandles an existing target throughsymlink_metadataplus canonicalization before assigning scope. For the edit request, resolve once withresolve_for_writeto keep the writer and read authorizations on the same resolution result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho-tools/src/sdk_adapter.rs` around lines 465 - 486, Update push_existing to resolve existing edit targets with workspace.resolve_for_write instead of resolve_for_read, retaining the single resolved result for canonical path creation, access tracking, and both write/read capabilities.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rho-providers/src/providers/tui_fixture/edit.rs`:
- Around line 64-67: Update the fixture setup before the edit call to propagate
failures from both std::env::current_dir and std::fs::write instead of ignoring
them. In the surrounding fixture function, convert each setup error into the
existing ProviderError type with context identifying the failed fixture file
setup, and only emit the edit call after the target has been successfully
written.
In `@crates/rho-tools/src/hashline/execute.rs`:
- Around line 208-214: Update AppliedFile to retain the committed outcome text
as applied_text, and have rollback_applied verify each live file still matches
that text before restoring file.original. Continue processing all entries in
reverse order after any lock, validation, or rewrite failure, collecting and
returning every failure instead of exiting on the first error.
- Around line 216-232: Raise the rho-sdk rust-version declaration to at least
1.89 before relying on File::lock in lock_for_rewrite. Replace the blocking lock
call there with bounded non-blocking retries using backoff, or an equivalent
timed lock, and return the existing ToolError once the timeout is exceeded.
In `@crates/rho-tools/src/hashline/mod_tests.rs`:
- Around line 159-175: The multi-file commit test around apply_prepared_sections
is using a readonly permission change on b_path, which can still succeed under
elevated privileges and make the test flaky. Replace that failure injection with
a user-independent failure mode in the same test setup, such as turning b.txt
into a directory before commit or routing the failure through the
apply_sections_locked / commit_planned_file path, so the expected rollback/error
assertion remains deterministic.
In `@crates/rho-tools/src/hashline/mod.rs`:
- Line 57: Refactor the Tool trait’s call method and every implementation to
return an explicit Send + '_ future, preserving their current async behavior,
then remove the #[async_trait::async_trait] attribute from the Tool definition
in hashline/mod.rs.
In `@crates/rho-tools/src/hashline/parser.rs`:
- Around line 287-304: Update take_body to preserve trailing spaces and tabs in
body rows by removing only a terminal wire carriage return before stripping the
leading “+” marker; do not call trim_end or otherwise normalize body content.
Keep blank-line handling and body termination behavior unchanged.
In `@crates/rho-tools/src/hashline/proposed.rs`:
- Around line 117-150: Update planned_edit’s read_path integration to ensure
each section.path resolves within the caller’s workspace before exposing file
contents. The resolver should canonicalize the workspace and candidate path,
verify the candidate remains contained (including traversal and symlink
escapes), then read only the verified path; return None for paths outside the
workspace.
In `@crates/rho-tools/src/read_file.rs`:
- Around line 108-129: Update the ranged-read branch in the read-file flow to
reuse the already-open file handle instead of calling
tokio::fs::read_to_string(path). Read through file with a .take(...) cap
matching the non-image branch’s MAX_DOCUMENT_INPUT_BYTES plus one-byte overflow
allowance, then convert the bounded bytes to UTF-8 text while preserving the
existing error mapping and hashline formatting.
In `@crates/rho/src/app/interactive_presenter_format.rs`:
- Around line 215-217: Bound file loading in the edit-card planning around
rho_tools::hashline::planned_edit by enforcing both a maximum referenced-file
count and maximum per-file size before read_to_string; for paths exceeding
either budget, return the document-only preview instead of loading full content.
Ensure planning cannot perform unbounded synchronous reads on the interactive
presentation path, moving it off that path if necessary.
In `@docs/tools-workspace.md`:
- Around line 60-86: Align stale-tag documentation with the SnapshotStore
contract: in docs/tools-workspace.md lines 60-86, document conditional
session-wide remapping for consistent anchor shifts and reveal-snapshot
rejection for unsafe or unseen edits; in
docs/dev/hashline-edit-dogfood-report.md lines 208-212, distinguish historical
removal from shipped behavior; in lines 230-235, update the recommended action
order; and in docs/dev/hashline-edit-eval.md line 25, add remap-success and
remap-failure evaluation cases.
---
Nitpick comments:
In `@crates/rho-tools/src/hashline/apply.rs`:
- Around line 163-190: Update add_span to reject ranges with start == 0 or end <
start before inserting into self.slots, returning an ApplyError consistent with
the existing invalid-range checks. Preserve the current line_count bounds
validation and ensure malformed caller-provided operations fail closed instead
of being stored for emit.
In `@crates/rho-tools/src/hashline/format_tests.rs`:
- Around line 92-106: Export the relevant preview and footer strings from
format.rs and update crates/rho-tools/src/hashline/format_tests.rs lines 92-106
to assert against those constants while retaining the check that numbered body
lines are absent; update crates/rho-tools/src/hashline/format_tests.rs lines
126-135 to use the exported footer template or compare the complete snapshot; in
crates/rho-tools/src/hashline/mod_tests.rs lines 270-277, remove
description-length and “PUT 12:” substring assertions, retain
assert_eq!(spec.name, "edit"), and cover locator rules in parser_tests.rs.
In `@crates/rho-tools/src/hashline/proposed_tests.rs`:
- Around line 108-131: Update the test assertions around the projected rows and
stats to use whole-object comparisons, matching the pattern established by the
first test in the file. Compare the complete ordered `(kind, text)` row vector
and compare `file.stats` exactly, replacing the membership checks and
lower-bound assertions while preserving the expected projection.
In `@crates/rho-tools/src/read_file_tests.rs`:
- Around line 133-152: The test’s expected value in
ranged_plain_text_reads_use_hashline_window is generated by the formatter under
test, making it unable to catch formatter defects. Keep compute_file_hash
available for the dynamic tag, but replace format_hashline_view with a literal
expected string containing the sample.txt hashline header, the 2:two and 3:three
rows, and the range window footer.
In `@crates/rho-tools/src/read_file.rs`:
- Around line 131-194: Extract the self-contained image-preview branch from
read_file_content into a private read_image_content helper accepting file,
display_path, source_len, and header. Move its size checks, thumbnail_png
spawn_blocking call, and fallback handling unchanged, then have
read_file_content dispatch to the helper when supported_image_mime_type matches
while leaving the text and document paths intact.
In `@crates/rho-tools/src/sdk_adapter_tests.rs`:
- Around line 462-474: Update the capability assertions in the existing loop
over a.txt and b.txt to verify each path has exactly one read request and one
write request, rather than relying on prepared.capabilities().len(). Use
pretty_assertions::assert_eq with whole-object comparisons where the existing
capability representation supports it, and remove or replace the count-only
assertion.
In `@crates/rho-tools/src/sdk_adapter.rs`:
- Around line 465-486: Update push_existing to resolve existing edit targets
with workspace.resolve_for_write instead of resolve_for_read, retaining the
single resolved result for canonical path creation, access tracking, and both
write/read capabilities.
In `@crates/rho-tools/src/write_file.rs`:
- Around line 194-198: Remove the assertion checking result.content for the
literal “showing” text in the write-file test. Keep the existing line-presence
and absence assertions around format_chain_snapshot, which already verify
truncation structurally; do not replace them with another wording-dependent
check.
In `@crates/rho/src/agent/agent_tests.rs`:
- Around line 38-54: Extend write_file_capability_alias_matches_write with
parsed definitions using the edit_file and apply_patch tool names, then assert
each has the same fingerprint as a canonical edit definition. Also assert
ToolCapability::parse for both aliases returns the canonical "edit" string,
covering all compatibility aliases.
In `@crates/rho/src/app/interactive_presenter_tests.rs`:
- Around line 70-74: Update the fact assertion in the interactive presenter test
to compare text.as_str() directly with
rho_tools::hashline::EDIT_DOCUMENT_ONLY_NOTICE, replacing the string-contains
check while preserving the existing Meta fact matching.
In `@docs/dev/hashline-edit-eval.md`:
- Line 31: Update the Suite J documentation to specify that its first edit is
non-structural and its chained second edit uses the post-edit preview tag and
lines. Add a separate case covering a structural first edit that requires a
fresh read before any further anchored operation, reflecting the workspace tool
contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf004d6e-1dc5-4df1-9b12-7e75e7a76d00
⛔ Files ignored due to path filters (1)
docs/assets/rho-ui-demo.svgis excluded by!**/*.svg
📒 Files selected for processing (77)
.agents/skills/rho-tui-herdr-testing/SKILL.md.agents/skills/rho-tui-pty-testing/SKILL.mdcrates/rho-providers/src/providers/anthropic/provider_tests.rscrates/rho-providers/src/providers/automation_fixture.rscrates/rho-providers/src/providers/google/provider_tests.rscrates/rho-providers/src/providers/openai_compatible_tests.rscrates/rho-providers/src/providers/tui_fixture.rscrates/rho-providers/src/providers/tui_fixture/edit.rscrates/rho-sdk/src/hooks/envelope_tests.rscrates/rho-sdk/src/orchestration/stream_capture_tests.rscrates/rho-sdk/src/workspace_tests.rscrates/rho-tools/README.mdcrates/rho-tools/src/apply_patch/apply.rscrates/rho-tools/src/apply_patch/mod.rscrates/rho-tools/src/apply_patch/parser.rscrates/rho-tools/src/apply_patch/proposed_diff.rscrates/rho-tools/src/apply_patch/seek_sequence.rscrates/rho-tools/src/apply_patch/seek_sequence_tests.rscrates/rho-tools/src/apply_patch_tests.rscrates/rho-tools/src/edit_file.rscrates/rho-tools/src/edit_file_tests.rscrates/rho-tools/src/file_mutation.rscrates/rho-tools/src/grep.rscrates/rho-tools/src/grep_format.rscrates/rho-tools/src/grep_tests.rscrates/rho-tools/src/hashline/apply.rscrates/rho-tools/src/hashline/apply_tests.rscrates/rho-tools/src/hashline/execute.rscrates/rho-tools/src/hashline/format.rscrates/rho-tools/src/hashline/format_tests.rscrates/rho-tools/src/hashline/mod.rscrates/rho-tools/src/hashline/mod_tests.rscrates/rho-tools/src/hashline/parser.rscrates/rho-tools/src/hashline/parser_tests.rscrates/rho-tools/src/hashline/proposed.rscrates/rho-tools/src/hashline/proposed_tests.rscrates/rho-tools/src/lib.rscrates/rho-tools/src/read_file.rscrates/rho-tools/src/read_file_tests.rscrates/rho-tools/src/sdk_adapter.rscrates/rho-tools/src/sdk_adapter_tests.rscrates/rho-tools/src/tool_card.rscrates/rho-tools/src/write_file.rscrates/rho-tui-pty/src/scenarios.rscrates/rho-tui-pty/src/scenarios/edit_diff.rscrates/rho/src/agent/agent_tests.rscrates/rho/src/agent/definition.rscrates/rho/src/agent/serializer_tests.rscrates/rho/src/app/agent_binding_tests.rscrates/rho/src/app/interactive_presenter.rscrates/rho/src/app/interactive_presenter_format.rscrates/rho/src/app/interactive_presenter_results.rscrates/rho/src/app/interactive_presenter_tests.rscrates/rho/src/app/policy_tests.rscrates/rho/src/app/workflow_runtime/runner.rscrates/rho/src/builtin_agents/explorer.mdcrates/rho/src/builtin_agents/reviewer.mdcrates/rho/src/builtin_agents/worker.mdcrates/rho/src/builtin_skills/rho-agent-creator/SKILL.mdcrates/rho/src/hooks/catalog_tests.rscrates/rho/src/hooks/diagnostics_tests.rscrates/rho/src/hooks/hooks_tests.rscrates/rho/src/permission_tests.rscrates/rho/src/prompt.rscrates/rho/src/tools/coding.rscrates/rho/src/tools/mod.rscrates/rho/src/tui/event_adapter_tests.rscrates/rho/src/tui/theme.rscrates/rho/src/tui/tool_card_render.rscrates/rho/tests/automation_cli.rscrates/rho/tests/tui_pty.rsdocs/configuration.mddocs/dev/hashline-edit-dogfood-report.mddocs/dev/hashline-edit-eval.mddocs/hooks.mddocs/subagents.mddocs/tools-workspace.md
💤 Files with no reviewable changes (9)
- crates/rho-tools/src/apply_patch/seek_sequence_tests.rs
- crates/rho-tools/src/edit_file_tests.rs
- crates/rho-tools/src/edit_file.rs
- crates/rho-tools/src/apply_patch_tests.rs
- crates/rho-tools/src/apply_patch/proposed_diff.rs
- crates/rho-tools/src/apply_patch/parser.rs
- crates/rho-tools/src/apply_patch/mod.rs
- crates/rho-tools/src/apply_patch/seek_sequence.rs
- crates/rho-tools/src/apply_patch/apply.rs
Add a line-anchored multi-hunk editor that pairs with hashline read_file views so models can edit by original line numbers and a file snapshot tag instead of reproducing old text. Keep edit_file and apply_patch, and document a model-in-the-loop eval design without shipping a runner.
Match tool path display on macOS and Windows where TempDir paths differ from the canonical workspace root used by read_file.
Review follow-ups on hashline_edit. The apply loop split ops into five buckets and scanned each one per line. An insert anchored inside a range that another op replaced or deleted was never reached, so it was dropped without any error while the tool reported success. Ops are now bucketed onto the line they anchor to and emitted in a single pass, which makes that case a visible conflict instead of a silent drop, and removes the per-line scans. The SDK adapter parsed the document in prepare to authorize each path, threw the result away, and passed the raw string down so the core parsed it again behind two closures. Prepare now hands the resolved sections to the core, so both parses, both closures, and the "was not prepared" fallbacks are gone, along with a revalidation pass the locked commit already covered. Also: - guard range reads with the document size limit, which the new hashline branch skipped - widen the snapshot tag from 16 to 32 bits - show added/removed counts on the approval card, matching apply_patch, via one parser that can skip malformed lines instead of a second lenient one - name the planned/applied tuples and note hidden head lines in ranged views - replace two tests that computed their expectation from the code under test
Make `edit` the only multi-hunk workspace edit tool. `read_file` and `write_file` return bounded hashline chain snapshots, failed edits include a live recovery snapshot, and unified diffs stay on tool metadata for UI. Agent frontmatter still accepts the old capability names as aliases.
Mint chainable 4-hex tags from grep and read, share a session SnapshotStore across coding tools, remap stale anchors when safe, and reject unseen lines.
Steer agents toward in-process grep/glob over shell rg/fd so content-mode [path#TAG] snapshots can feed edit without a separate read_file.
Keep the direct tagged-read → apply → live-snapshot loop. Remove SnapshotStore, stale-tag remap, and seen-line provenance so edit fails closed with a copyable live snapshot instead of soft session memory. Simplify format APIs, SDK wiring, docs, and the edit_diff PTY assertion to match the card presenter.
Make hashline the sole chainable header emitter, keep grep match text as non-copyable previews, project proposed edits as document op summaries, and move FileMutationOutcome to a neutral module.
Address thermo-nuclear review findings: give proposed op locators a real DiffRowKind::Meta instead of Skip, collapse format selection policy into callers with a simpler hunk budget, and surface ops summaries plus structural re-read footers on successful edit results.
Address thermo-nuclear review: approval dry-runs live content diffs with removals, streaming stays document-only, structural edits drop chainable body lines, shrink tool schema policy into prompt/docs, drop dead apply tags, own write-time uniqueness comments, and test the production apply path plus multi-step structural PUT→CUT cleanup.
Make narrowed grep headers workspace-relative for edit chaining, emit DiffCardFile previews directly, keep pure CUT as content stats, and surface document-only fallback on approval cards.
Align event_adapter expectations with document-only planned cards and pure-CUT DiffStat mapping, and refresh builtin agent legacy v1 goldens after explorer/worker prompt edits.
Match the short edit name, cut a token from model tool lists, and keep write_file as a capability/UI alias until the next major cleanup.
Replace the overloaded document_only flag with EditPreviewKind so stream cards stay quiet and only planned fallbacks warn. Own path uniqueness in claim_unique_path for prepare and execute, lift the multi-file transaction into execute.rs, and share PUT/CUT locator formatting.
Clippy -D warnings failed CI after the execute split left the re-export unused in hashline/mod.rs.
Propagate fixture setup errors, make multi-file rollback verify applied text and collect failures, bound edit locks and plan-time file reads, preserve body trailing whitespace, and drop async_trait from app tools.
Drop the temporary dogfood/eval pages under docs/dev and the stale link from tools-workspace.
Exclusive locks are mandatory on Windows and blocked the plan-phase read, so the commit-failure path never ran. Use a read-only target file instead.
Avoid clippy::permissions_set_readonly_false by restoring the file's original mode via Drop instead of set_readonly(false).
matthewyjiang
force-pushed
the
feat/hashline-edit-tool
branch
from
August 6, 2026 00:34
cf7fd43 to
2f89722
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/rho/src/app/interactive_presenter_format.rs (1)
406-423: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe interrupted
Editcard loses the partial-argument fallback.The
_arm at Lines 410-422 setscard.bodyfrompartial_argumentswhen the card has no body and no facts. The newEditarm at Line 409 bypasses that logic.If a tool call is interrupted before the arguments complete,
view.argumentscan lackinput.edit_planned_cardthen returns a bareeditheader card, andpartial_argumentsis discarded. The user sees less than the generic path would show.Apply the same fallback to the
Editarm.♻️ Proposed change to keep the fallback
- ToolKind::Edit => edit_planned_card(&view.arguments, cwd, ToolStatus::Interrupted), + ToolKind::Edit => { + let mut card = edit_planned_card(&view.arguments, cwd, ToolStatus::Interrupted); + if !partial_arguments.is_empty() && card.body.is_empty() && card.facts.is_empty() { + card.body = ToolBody::Lines(vec![partial_arguments.to_string()]); + } + card + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho/src/app/interactive_presenter_format.rs` around lines 406 - 423, Update the ToolKind::Edit arm in the view.kind match to preserve the partial-argument fallback used by the generic arm: build the interrupted edit card, then populate card.body with partial_arguments when it is non-empty and both body and facts are empty. Keep the existing edit_planned_card behavior for complete arguments.crates/rho-tools/src/hashline/apply.rs (1)
21-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider typed variants instead of
ApplyError::Message.The doc comment states that callers match variants instead of English substrings.
Message(String)reintroduces string-encoded failures for range errors, empty-file errors, and overlap errors. A downstream caller cannot distinguish "range outside file" from "overlapping ops" without substring matching.Two related follow-ups, both optional in this PR:
- Split
Messageinto named variants such asRangeOutsideFile,OverlappingOps, andEmptyFile, and keep the human text inDisplay.- Replace
SpanEdit.kind: &'static str(Line 87) with a small enum, so op kinds are modeled explicitly rather than as strings.As per coding guidelines: "In Rust, model concepts such as selected, current, unavailable, warning, or detail explicitly rather than inferring them from encoded strings or suffixes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rho-tools/src/hashline/apply.rs` around lines 21 - 33, Replace ApplyError::Message with named variants for each distinct failure condition, including range-outside-file, overlapping operations, and empty-file cases, and update the ApplyError::message callers to construct those variants. Keep human-readable wording in the Display implementation while preserving TagMismatch and EmptyOps behavior; optionally replace SpanEdit.kind with an explicit enum rather than a string.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/rho-tools/src/hashline/apply.rs`:
- Around line 21-33: Replace ApplyError::Message with named variants for each
distinct failure condition, including range-outside-file, overlapping
operations, and empty-file cases, and update the ApplyError::message callers to
construct those variants. Keep human-readable wording in the Display
implementation while preserving TagMismatch and EmptyOps behavior; optionally
replace SpanEdit.kind with an explicit enum rather than a string.
In `@crates/rho/src/app/interactive_presenter_format.rs`:
- Around line 406-423: Update the ToolKind::Edit arm in the view.kind match to
preserve the partial-argument fallback used by the generic arm: build the
interrupted edit card, then populate card.body with partial_arguments when it is
non-empty and both body and facts are empty. Keep the existing edit_planned_card
behavior for complete arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21908bdf-4eb9-4529-b49f-536c5031a983
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockdocs/assets/rho-ui-demo.svgis excluded by!**/*.svg
📒 Files selected for processing (84)
.agents/skills/rho-tui-herdr-testing/SKILL.md.agents/skills/rho-tui-pty-testing/SKILL.mdcrates/rho-providers/src/providers/anthropic/provider_tests.rscrates/rho-providers/src/providers/automation_fixture.rscrates/rho-providers/src/providers/google/provider_tests.rscrates/rho-providers/src/providers/openai_compatible_tests.rscrates/rho-providers/src/providers/tui_fixture.rscrates/rho-providers/src/providers/tui_fixture/edit.rscrates/rho-sdk/src/hooks/envelope_tests.rscrates/rho-sdk/src/orchestration/stream_capture_tests.rscrates/rho-sdk/src/workspace_tests.rscrates/rho-tools/Cargo.tomlcrates/rho-tools/README.mdcrates/rho-tools/src/apply_patch/apply.rscrates/rho-tools/src/apply_patch/mod.rscrates/rho-tools/src/apply_patch/parser.rscrates/rho-tools/src/apply_patch/proposed_diff.rscrates/rho-tools/src/apply_patch/seek_sequence.rscrates/rho-tools/src/apply_patch/seek_sequence_tests.rscrates/rho-tools/src/apply_patch_tests.rscrates/rho-tools/src/bash.rscrates/rho-tools/src/edit_file.rscrates/rho-tools/src/edit_file_tests.rscrates/rho-tools/src/file_mutation.rscrates/rho-tools/src/grep.rscrates/rho-tools/src/grep_format.rscrates/rho-tools/src/grep_tests.rscrates/rho-tools/src/hashline/apply.rscrates/rho-tools/src/hashline/apply_tests.rscrates/rho-tools/src/hashline/execute.rscrates/rho-tools/src/hashline/format.rscrates/rho-tools/src/hashline/format_tests.rscrates/rho-tools/src/hashline/mod.rscrates/rho-tools/src/hashline/mod_tests.rscrates/rho-tools/src/hashline/parser.rscrates/rho-tools/src/hashline/parser_tests.rscrates/rho-tools/src/hashline/proposed.rscrates/rho-tools/src/hashline/proposed_tests.rscrates/rho-tools/src/lib.rscrates/rho-tools/src/list_dir.rscrates/rho-tools/src/powershell.rscrates/rho-tools/src/read_file.rscrates/rho-tools/src/read_file_tests.rscrates/rho-tools/src/sdk_adapter.rscrates/rho-tools/src/sdk_adapter_tests.rscrates/rho-tools/src/tool.rscrates/rho-tools/src/tool_card.rscrates/rho-tools/src/write_file.rscrates/rho-tui-pty/src/scenarios.rscrates/rho-tui-pty/src/scenarios/edit_diff.rscrates/rho/src/agent/agent_tests.rscrates/rho/src/agent/definition.rscrates/rho/src/agent/serializer_tests.rscrates/rho/src/app/agent_binding_tests.rscrates/rho/src/app/interactive_presenter.rscrates/rho/src/app/interactive_presenter_format.rscrates/rho/src/app/interactive_presenter_results.rscrates/rho/src/app/interactive_presenter_tests.rscrates/rho/src/app/policy_tests.rscrates/rho/src/app/workflow_runtime/runner.rscrates/rho/src/builtin_agents/explorer.mdcrates/rho/src/builtin_agents/reviewer.mdcrates/rho/src/builtin_agents/worker.mdcrates/rho/src/builtin_skills/rho-agent-creator/SKILL.mdcrates/rho/src/hooks/catalog_tests.rscrates/rho/src/hooks/diagnostics_tests.rscrates/rho/src/hooks/hooks_tests.rscrates/rho/src/permission_tests.rscrates/rho/src/prompt.rscrates/rho/src/tools/coding.rscrates/rho/src/tools/mod.rscrates/rho/src/tools/process/tools.rscrates/rho/src/tools/rho.rscrates/rho/src/tools/skill.rscrates/rho/src/tools/web/adapters.rscrates/rho/src/tui/event_adapter_tests.rscrates/rho/src/tui/theme.rscrates/rho/src/tui/tool_card_render.rscrates/rho/tests/automation_cli.rscrates/rho/tests/tui_pty.rsdocs/configuration.mddocs/hooks.mddocs/subagents.mddocs/tools-workspace.md
💤 Files with no reviewable changes (10)
- crates/rho-tools/Cargo.toml
- crates/rho-tools/src/apply_patch/proposed_diff.rs
- crates/rho-tools/src/apply_patch_tests.rs
- crates/rho-tools/src/apply_patch/seek_sequence_tests.rs
- crates/rho-tools/src/apply_patch/parser.rs
- crates/rho-tools/src/apply_patch/seek_sequence.rs
- crates/rho-tools/src/apply_patch/apply.rs
- crates/rho-tools/src/edit_file.rs
- crates/rho-tools/src/edit_file_tests.rs
- crates/rho-tools/src/apply_patch/mod.rs
🚧 Files skipped from review as they are similar to previous changes (61)
- crates/rho-providers/src/providers/google/provider_tests.rs
- docs/configuration.md
- crates/rho/src/hooks/diagnostics_tests.rs
- crates/rho-sdk/src/orchestration/stream_capture_tests.rs
- crates/rho/tests/tui_pty.rs
- crates/rho-sdk/src/hooks/envelope_tests.rs
- crates/rho-tools/src/file_mutation.rs
- crates/rho-providers/src/providers/openai_compatible_tests.rs
- crates/rho/src/builtin_agents/explorer.md
- crates/rho-providers/src/providers/anthropic/provider_tests.rs
- crates/rho/src/app/interactive_presenter_results.rs
- crates/rho/src/builtin_agents/worker.md
- docs/hooks.md
- crates/rho/src/permission_tests.rs
- crates/rho/src/tools/coding.rs
- crates/rho/src/tui/theme.rs
- crates/rho-tools/README.md
- crates/rho-tui-pty/src/scenarios.rs
- crates/rho/src/tui/tool_card_render.rs
- crates/rho-tools/src/hashline/apply_tests.rs
- .agents/skills/rho-tui-herdr-testing/SKILL.md
- crates/rho-providers/src/providers/tui_fixture.rs
- crates/rho-tools/src/lib.rs
- crates/rho/src/hooks/catalog_tests.rs
- crates/rho-providers/src/providers/automation_fixture.rs
- crates/rho-tui-pty/src/scenarios/edit_diff.rs
- crates/rho-tools/src/hashline/format_tests.rs
- crates/rho-tools/src/grep_format.rs
- crates/rho/src/agent/definition.rs
- crates/rho/src/tools/mod.rs
- crates/rho/tests/automation_cli.rs
- crates/rho/src/prompt.rs
- crates/rho-tools/src/grep.rs
- crates/rho/src/hooks/hooks_tests.rs
- crates/rho-tools/src/grep_tests.rs
- crates/rho/src/builtin_agents/reviewer.md
- crates/rho/src/app/policy_tests.rs
- crates/rho-tools/src/hashline/mod.rs
- crates/rho-tools/src/hashline/parser_tests.rs
- crates/rho/src/agent/serializer_tests.rs
- .agents/skills/rho-tui-pty-testing/SKILL.md
- crates/rho/src/tui/event_adapter_tests.rs
- crates/rho/src/app/interactive_presenter.rs
- crates/rho-tools/src/hashline/mod_tests.rs
- crates/rho/src/app/workflow_runtime/runner.rs
- crates/rho/src/app/agent_binding_tests.rs
- crates/rho-tools/src/tool_card.rs
- crates/rho-tools/src/hashline/proposed.rs
- crates/rho-tools/src/hashline/proposed_tests.rs
- crates/rho-tools/src/hashline/execute.rs
- crates/rho-tools/src/sdk_adapter_tests.rs
- crates/rho-tools/src/hashline/parser.rs
- crates/rho-tools/src/read_file_tests.rs
- crates/rho-sdk/src/workspace_tests.rs
- crates/rho-tools/src/write_file.rs
- docs/subagents.md
- crates/rho/src/app/interactive_presenter_tests.rs
- crates/rho-tools/src/read_file.rs
- crates/rho-tools/src/hashline/format.rs
- crates/rho-tools/src/sdk_adapter.rs
- crates/rho-providers/src/providers/tui_fixture/edit.rs
WaitText("edit(") matched the completed first card and let Esc race
ahead of the second stream under CI load. Wait for unique cancel-fixture
content and assert both payloads remain on screen.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ship the hashline edit stack end state: sole
edittool, chainable snapshots, session recovery, and grep tags.edit_file/apply_patchwith hashlineedit(PUT/CUT against original line numbers).read_filealways returns[path#TAG]+N:linefor UTF-8 sources.grepcontent mode mints[path#TAG]and hashlineN:textrows for edit chaining.SnapshotStoreshared across coding tools records full-file text + seen lines on read/grep/write/edit.N-M,N..M).Not in this cut
N*), registers,REM,MVwrite_file)Validation
All passed.
Test gate
rho-test-selection(failure mode, owner layer, gap).crates/rho/src/tuiunit tests are pure logic or justified below..containsonly for redaction, wire format, or security escaping.New tests
editmust recover via store when tag is stale but anchors still mapeditmust reject unseen anchors from partial readsedit/write_file/read_filechain previews and multi-file commitsPTY exception (if any)
Breaking changes
read_filetext/source output is hashline ([path#TAG]+ numbered lines).grepcontent mode output is hashline-shaped ([path#TAG]+N:text).edit_fileandapply_patchare removed; useedit/write_file.Next-major debt
None for dual API shapes. Capability aliases for removed tool names, if any, stay greppable as
NEXT_MAJORwhere already marked.Summary by CodeRabbit
editandwritetools with hashline-based file snapshots.