feat(tools): add selectable edit formats - #820
Conversation
|
Warning Review limit reached
Next review available in: 31 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 (20)
📝 WalkthroughWalkthroughThe PR adds configurable ChangesConfigurable edit-tool platform
Publish preparation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Config
participant SDKRegistry
participant SelectedEditTool
participant Workspace
participant Presenter
Config->>SDKRegistry: provide selected edit format
SDKRegistry->>SelectedEditTool: register one edit adapter
SelectedEditTool->>Workspace: validate and mutate files
Workspace-->>SelectedEditTool: return diffs and snapshots
SelectedEditTool-->>Presenter: emit mutation metadata
Presenter->>Presenter: render format-specific edit card
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
crates/rho-tools/src/apply_patch_tests.rs (1)
274-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for out-of-order and overlapping chunks.
compute_replacementsraises "patch chunks overlap or apply out of order" from three separate guards. No test reaches that error. A model that emits chunks in the wrong order hits this path often, so the message and the fail-closed behavior are user-visible. Add one case with two update chunks whose contexts appear in reverse file order, and assert both the error message and that the file is unchanged.As per coding guidelines: "Prefer behavior or integration tests for user-visible logic and unit tests for focused pure logic."
🤖 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/apply_patch_tests.rs` around lines 274 - 331, Add an integration-style test near the existing apply_hunks tests using a single file with two update chunks whose contexts are supplied in reverse file order. Assert that apply_hunks returns the exact “patch chunks overlap or apply out of order” error and verify the target file retains its original contents, covering fail-closed behavior for compute_replacements.Source: Coding guidelines
crates/rho-tools/src/apply_patch/seek_sequence_tests.rs (1)
11-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the no-match case and the Unicode fallback.
The test covers the trailing-whitespace fallback and the EOF anchor. Two branches stay untested: the
Noneresult when no window matches, and thenormalisefallback that maps smart quotes and Unicode dashes. Thenormalisebranch decides whether a model's typographic drift silently rewrites a file, so it deserves a case. The table style already used inedit_file_tests.rsfits here.As per coding guidelines: "Use the
rho-test-selectionskill when adding, expanding, reviewing, or deleting tests, including its failure-mode, ownership, tier, determinism, table-driven, and assertion guidance."🤖 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/apply_patch/seek_sequence_tests.rs` around lines 11 - 34, The seek_sequence tests need coverage for both unmatched input and Unicode normalization fallback. Expand matches_normalized_context_and_eof_position or add a table-driven test using the existing lines and seek_sequence helpers, asserting None when no window matches and asserting the expected position when smart quotes or Unicode dashes normalize to the source text; follow the table style established by edit_file_tests.rs.Source: Coding guidelines
crates/rho-tools/src/hashline/execute.rs (1)
200-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLabel the two adjacent content arguments at every
rewrite_locked_filecall site.rewrite_locked_file(file, display_path, original, updated)takes two adjacent&strcontent parameters. A swap compiles and silently reverts the file instead of updating it. The coding guidelines require an inline parameter-name comment for this case.
crates/rho-tools/src/hashline/execute.rs#L200-L205: annotate&file.originalas/*original*/and&file.outcome.textas/*updated*/.crates/rho-tools/src/edit_file.rs#L145-L145: annotate&originalas/*original*/and&updatedas/*updated*/.crates/rho-tools/src/apply_patch/apply.rs#L668-L668: annotate&liveas/*original*/and&updatedas/*updated*/.As per coding guidelines: "comment unavoidable opaque positional arguments with the exact parameter name."
🤖 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/execute.rs` around lines 200 - 205, Annotate the adjacent content arguments at every rewrite_locked_file call site with exact parameter-name comments: in crates/rho-tools/src/hashline/execute.rs:200-205, mark &file.original as /*original*/ and &file.outcome.text as /*updated*/; in crates/rho-tools/src/edit_file.rs:145-145, mark &original as /*original*/ and &updated as /*updated*/; and in crates/rho-tools/src/apply_patch/apply.rs:668-668, mark &live as /*original*/ and &updated as /*updated*/.Source: Coding guidelines
crates/rho-tools/src/edit_file_tests.rs (1)
83-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
replace_all.The tests cover the single-match, ambiguous-match, and CRLF paths. No test exercises
replace_all = true. That branch changes bothvalidate_match_countand the multi-span rewrite inreplace_spans, and multi-span offset mapping is the most intricate logic in this file. Add one case that replaces two occurrences and asserts the resulting file content and the reported occurrence count.As per coding guidelines: "Prefer behavior or integration tests for user-visible logic and unit tests for focused pure logic."
🤖 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/edit_file_tests.rs` around lines 83 - 129, Add an integration-style async test alongside rejects_ambiguous_match_without_mutation that calls edit_file_content with replace_all set to true, replaces two occurrences in the file, and verifies both the final file content and the reported occurrence count in the result. Exercise the multi-span rewrite through the public edit_file_content path rather than testing validate_match_count or replace_spans directly.Source: Coding guidelines
crates/rho-tools/src/apply_patch/apply.rs (1)
1-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSplit
apply.rsinto focused modules.The file is 930 lines and holds six separable concerns: the
FileChangeplan model, hunk planning and reads, path validation, content derivation and chunk matching, commit and platform identity checks, and rollback. The coding guidelines require extraction here.Suggested split:
plan.rsforplan_hunk,check_path_conflicts, and the read helpers;paths.rsforvalidate_patch_pathandreject_symlink_entry;content.rsforderive_new_contents,compute_replacements, andapply_replacements;commit.rsforcommit_changes,rewrite_current,remove_current, and the identity checks;rollback.rsforrollback_applied,rollback_one, androllback_move.As per coding guidelines: "In Rust, avoid large files and extract separable behavior into focused modules while keeping tests and invariant documentation close to the 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/apply_patch/apply.rs` around lines 1 - 30, Split the monolithic apply implementation into focused sibling modules while preserving behavior and keeping related tests and invariant documentation with their implementations: move the FileChange plan model, plan_hunk, check_path_conflicts, and read helpers to plan.rs; validate_patch_path and reject_symlink_entry to paths.rs; derive_new_contents, compute_replacements, and apply_replacements to content.rs; commit_changes, rewrite_current, remove_current, and platform identity checks to commit.rs; and rollback_applied, rollback_one, and rollback_move to rollback.rs. Update apply.rs and module imports/re-exports so the existing patch application flow remains unchanged.Source: Coding guidelines
crates/rho-tools/src/file_mutation.rs (1)
89-100: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
File::flushdoes not persist data to disk.The
Writeimplementation forstd::fs::Filehas a no-opflush. The current code reports success before the OS writes the data. If durability after a tool edit matters, callsync_datainstead.♻️ Proposed change
file.write_all(contents.as_bytes()) .map_err(|error| ToolError::Message(format!("could not write {display_path}: {error}")))?; - file.flush() + file.sync_data() .map_err(|error| ToolError::Message(format!("could not write {display_path}: {error}")))🤖 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/file_mutation.rs` around lines 89 - 100, Update the final persistence step in rewrite to call File::sync_data instead of File::flush, while preserving the existing error mapping and display_path context for failures.crates/rho-tools/src/sdk_support.rs (1)
123-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve non-validation error kinds.
map_invalid_app_errormaps everyAppToolErrorvariant toInvalidArguments. If a caller ever passesAppToolError::CancelledorAppToolError::Io, the error kind becomes wrong and cancellation stops being reported as cancellation.map_invalid_edit_argsincrates/rho-tools/src/sdk_adapter.rsat Line 922 already handles this by matchingMessageand delegating other variants tomap_app_error. Use the same shape here, and consider replacing the duplicate helper insdk_adapter.rswith this one.♻️ Proposed change
pub(crate) fn map_invalid_app_error(error: AppToolError) -> ToolError { - ToolError::new(ToolErrorKind::InvalidArguments, error.to_string()) + match error { + AppToolError::Message(message) => ToolError::new(ToolErrorKind::InvalidArguments, message), + other => map_app_error(other), + } }🤖 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_support.rs` around lines 123 - 125, Update map_invalid_app_error to match AppToolError variants, preserving InvalidArguments only for validation/message errors and delegating non-validation variants such as Cancelled and Io to map_app_error. Align the behavior with map_invalid_edit_args in sdk_adapter.rs, and reuse this helper there if that removes the duplicate mapping logic.crates/rho-tools/src/sdk_adapter.rs (1)
581-587: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the boolean argument at the call site.
path_set.collecttakes two positional booleans. Line 585 annotatesrequire_existing, and Line 593 annotates both. Line 586 passesmutates_source_entry, which does not match the parameter namereject_symlink_leaf. Add the parameter-name comment so all call sites read the same way.♻️ Proposed change
- mutates_source_entry, + /*reject_symlink_leaf*/ mutates_source_entry,As per coding guidelines: "comment unavoidable opaque positional arguments with the exact parameter name."
🤖 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 581 - 587, Add the exact parameter-name comment `reject_symlink_leaf` before the `mutates_source_entry` argument in the `path_set.collect` call within the surrounding hunk-processing function, matching the annotations used by the other call sites.Source: Coding guidelines
crates/rho/src/tools/sdk_registry_tests.rs (1)
73-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the selected edit schema for each configuration.
The aggregated name list passes if every configuration exposes all three edit tools. This misses the requirement that Rho exposes only the selected edit schema.
Keep names separate per
EditTool. Assert that each set contains its selected tool and excludes the other two edit-tool names.🤖 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/tools/sdk_registry_tests.rs` around lines 73 - 95, Update the test loop around AppToolSet::unfiltered_names so names remain separate for each EditTool configuration instead of being aggregated. For every configuration, assert the selected edit tool name is present and the other two edit-tool names are absent, while preserving the existing advisor registration setup.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.
Inline comments:
In `@crates/rho-tools/src/apply_patch/apply.rs`:
- Around line 786-788: Distinguish restore-failure errors in the rewrite flow:
have rewrite_locked_file return a marker when restoring the truncated file
fails, propagate it through rewrite_current, and map that marker to
ApplyFailure::after_mutation at the rewrite_current call site. Keep ordinary
pre-write failures mapped to ApplyFailure::before_mutation so commit_changes and
rollback_applied retain their existing behavior.
- Around line 444-456: Update derive_new_contents so line-ending conversion is
limited to lines affected by apply_replacements, preserving the original endings
of untouched lines in mixed-ending files. Avoid applying replace('\n',
line_ending) to the complete joined output, while retaining the existing
trailing-newline behavior.
In `@crates/rho-tools/src/apply_patch/parser.rs`:
- Around line 334-339: Update the empty-line check in the patch parser to test
the untrimmed line content, so only genuinely empty lines enter this branch.
Allow whitespace-only lines to fall through to the existing strip_prefix(' ')
context handling, preserving their indentation in old_lines and new_lines.
In `@crates/rho-tools/src/edit_file.rs`:
- Around line 126-145: Update edit_file_content_locked to perform the existing
symlink/identity validation used by apply_patch immediately after
lock_for_rewrite and before reading or modifying the file. Reject the operation
unless the locked path is the live regular file addressed by the request,
preventing symlink targets outside the request from being edited.
In `@crates/rho-tools/src/sdk_adapter.rs`:
- Around line 639-645: Update the request-level cache handling in collect so
cached entries retain whether they were validated with reject_symlink_leaf and
require_existing. When a later call requires stricter validation, re-run
reject_symlink_entry and the require_existing resolution before returning;
preserve the fast path only when the cached validation satisfies the current
request.
- Around line 518-535: Update EditFileTool::prepare to resolve args.path with
workspace.resolve_for_write instead of resolve_for_read, preserving the existing
path-error mapping and write/read capability requests so the target matches
execute_prepared_string_edit mutation behavior.
In `@crates/rho/src/prompt.rs`:
- Around line 94-107: Update the prompt-building logic around the
tools.iter().any check to emit edit guidance when any of “edit”, “apply_patch”,
or “edit_file” is selected. Keep the existing grep, hashline TAG, locator, and
PUT-specific guidance exclusively under the “edit” branch, and add separate
generic guidance cases for “apply_patch” and “edit_file” that direct their use
instead of shell or Python rewrites.
In `@docs/configuration.md`:
- Line 37: Update the settings timing description near the `edit_tool` row to
say it applies on the next Rho startup instead of the next session, keeping the
existing timing for `enable_subagents` unchanged.
---
Nitpick comments:
In `@crates/rho-tools/src/apply_patch_tests.rs`:
- Around line 274-331: Add an integration-style test near the existing
apply_hunks tests using a single file with two update chunks whose contexts are
supplied in reverse file order. Assert that apply_hunks returns the exact “patch
chunks overlap or apply out of order” error and verify the target file retains
its original contents, covering fail-closed behavior for compute_replacements.
In `@crates/rho-tools/src/apply_patch/apply.rs`:
- Around line 1-30: Split the monolithic apply implementation into focused
sibling modules while preserving behavior and keeping related tests and
invariant documentation with their implementations: move the FileChange plan
model, plan_hunk, check_path_conflicts, and read helpers to plan.rs;
validate_patch_path and reject_symlink_entry to paths.rs; derive_new_contents,
compute_replacements, and apply_replacements to content.rs; commit_changes,
rewrite_current, remove_current, and platform identity checks to commit.rs; and
rollback_applied, rollback_one, and rollback_move to rollback.rs. Update
apply.rs and module imports/re-exports so the existing patch application flow
remains unchanged.
In `@crates/rho-tools/src/apply_patch/seek_sequence_tests.rs`:
- Around line 11-34: The seek_sequence tests need coverage for both unmatched
input and Unicode normalization fallback. Expand
matches_normalized_context_and_eof_position or add a table-driven test using the
existing lines and seek_sequence helpers, asserting None when no window matches
and asserting the expected position when smart quotes or Unicode dashes
normalize to the source text; follow the table style established by
edit_file_tests.rs.
In `@crates/rho-tools/src/edit_file_tests.rs`:
- Around line 83-129: Add an integration-style async test alongside
rejects_ambiguous_match_without_mutation that calls edit_file_content with
replace_all set to true, replaces two occurrences in the file, and verifies both
the final file content and the reported occurrence count in the result. Exercise
the multi-span rewrite through the public edit_file_content path rather than
testing validate_match_count or replace_spans directly.
In `@crates/rho-tools/src/file_mutation.rs`:
- Around line 89-100: Update the final persistence step in rewrite to call
File::sync_data instead of File::flush, while preserving the existing error
mapping and display_path context for failures.
In `@crates/rho-tools/src/hashline/execute.rs`:
- Around line 200-205: Annotate the adjacent content arguments at every
rewrite_locked_file call site with exact parameter-name comments: in
crates/rho-tools/src/hashline/execute.rs:200-205, mark &file.original as
/*original*/ and &file.outcome.text as /*updated*/; in
crates/rho-tools/src/edit_file.rs:145-145, mark &original as /*original*/ and
&updated as /*updated*/; and in
crates/rho-tools/src/apply_patch/apply.rs:668-668, mark &live as /*original*/
and &updated as /*updated*/.
In `@crates/rho-tools/src/sdk_adapter.rs`:
- Around line 581-587: Add the exact parameter-name comment
`reject_symlink_leaf` before the `mutates_source_entry` argument in the
`path_set.collect` call within the surrounding hunk-processing function,
matching the annotations used by the other call sites.
In `@crates/rho-tools/src/sdk_support.rs`:
- Around line 123-125: Update map_invalid_app_error to match AppToolError
variants, preserving InvalidArguments only for validation/message errors and
delegating non-validation variants such as Cancelled and Io to map_app_error.
Align the behavior with map_invalid_edit_args in sdk_adapter.rs, and reuse this
helper there if that removes the duplicate mapping logic.
In `@crates/rho/src/tools/sdk_registry_tests.rs`:
- Around line 73-95: Update the test loop around AppToolSet::unfiltered_names so
names remain separate for each EditTool configuration instead of being
aggregated. For every configuration, assert the selected edit tool name is
present and the other two edit-tool names are absent, while preserving the
existing advisor registration setup.
🪄 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: bdb49f04-497b-487e-843d-75ca37d5d648
📒 Files selected for processing (45)
crates/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/hashline/execute.rscrates/rho-tools/src/lib.rscrates/rho-tools/src/sdk_adapter.rscrates/rho-tools/src/sdk_adapter_tests.rscrates/rho-tools/src/sdk_support.rscrates/rho-tools/src/write_file.rscrates/rho-tui-pty/src/scenarios/config.rscrates/rho/src/agent/definition.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/builtin_skills/rho-config/SKILL.mdcrates/rho/src/config.rscrates/rho/src/config_format.rscrates/rho/src/config_load.rscrates/rho/src/config_load_tests.rscrates/rho/src/diagnostics.rscrates/rho/src/diagnostics_tests.rscrates/rho/src/prompt.rscrates/rho/src/tools/coding.rscrates/rho/src/tools/mod.rscrates/rho/src/tools/sdk_registry.rscrates/rho/src/tools/sdk_registry_tests.rscrates/rho/src/tui/config_actions.rscrates/rho/src/tui/config_picker.rsdocs/configuration.mddocs/configuration/full-example.mddocs/hooks.mddocs/tools-workspace.mddocs/tools-workspace/documents-and-images.mddocs/tools-workspace/edit-format.mddocs/tools-workspace/search.md
Package verify resolves published rho-agent-tools 0.12.6 from crates.io, which lacks EditFormat and apply_patch. Bump the workspace tools cut so publish prep path-patches the unpublished surface used by the agent.
When only one leaf crate is unpublished, path-patching that leaf alone loads a second rho-sdk through its path edges and breaks type identity. Patch the full internal dependency closure whenever any direct internal dep is missing from crates.io.
Make each edit format expose its own model-facing name so presenters and prompts classify by name alone. Rename the string-replace surface to StrReplace, align agent capability aliases, reuse mutation_output for apply_patch, and require locked path identity on hashline commit and rollback.
Pad the high-volume stdout flood with keep_alive control frames so the stream mapper does no journal work per line, and raise the deadlock timeout above the artifact finish-join budget. Assistant flood lines were racing macOS CI disk load and looking like pipe deadlocks.
Mirror apply_patch layout with a str_replace/ module, content helper, and sibling tests. Rename the public content entry point to str_replace_content and keep legacy edit_file tool-name aliases.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
crates/rho/src/app/interactive_presenter_format.rs (1)
255-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffExtract the apply-patch renderer into a focused module.
apply_patch_cardadds a separate presentation policy to an already large module. Move this renderer and its focused tests into a cohesive private module.As per coding guidelines, “In Rust, avoid large files and extract separable behavior into focused modules while keeping tests and invariant documentation close to the 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/src/app/interactive_presenter_format.rs` around lines 255 - 306, Move apply_patch_card and its focused tests from the large presenter module into a cohesive private module dedicated to apply-patch rendering. Preserve the existing parsing, path formatting, change mapping, statistics, truncation, and empty-diff behavior, and update the caller to use the extracted renderer while keeping related tests and invariant documentation alongside it.Source: Coding guidelines
crates/rho-tools/src/edit_file.rs (1)
79-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment the positional boolean argument at the call site.
edit_file_contenttakesreplace_allas a bare positionalbool. The tests inedit_file_tests.rsalready annotate this argument as/*replace_all*/. Apply the same annotation here for consistency with the coding guidelines.♻️ Proposed change
&args.old_string, &args.new_string, - args.replace_all, + /*replace_all*/ args.replace_all, ctx.max_output_bytes,As per coding guidelines: "Make Rust call sites self-documenting by preferring enums, named methods, builders, or newtypes over ambiguous boolean or
Optionparameters; comment unavoidable opaque positional arguments with the exact parameter name."🤖 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/edit_file.rs` around lines 79 - 87, Add the exact `/*replace_all*/` inline annotation to the `args.replace_all` argument in the `edit_file_content` call, matching the existing test call sites and leaving the surrounding arguments unchanged.Source: Coding guidelines
crates/rho-tools/src/apply_patch/apply.rs (1)
39-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the two
Nonefault arguments.The call passes two positional
Nonevalues forrewrite_faultandcreate_fault. The parameter identity is not visible at the call site. Add the parameter-name comments used elsewhere in this crate.♻️ Proposed change
apply_hunks_inner( hunks, resolve_path, display_path, max_output_bytes, - None, - None, + /*rewrite_fault*/ None, + /*create_fault*/ None, )As per coding guidelines: "comment unavoidable opaque positional arguments with the exact parameter name."
🤖 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/apply_patch/apply.rs` around lines 39 - 46, Annotate both positional None arguments in the apply_hunks_inner call with comments identifying them exactly as rewrite_fault and create_fault, matching the parameter-name comment style used elsewhere in the crate.Source: Coding guidelines
crates/rho-tools/src/apply_patch/transaction.rs (1)
496-501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOpaque positional arguments at three new call sites. The crate already comments positional arguments with their parameter name, for example
/*original*/and/*updated*/. Three new call sites pass ambiguousbool,Option, and same-typed string arguments without that annotation.
crates/rho-tools/src/apply_patch/transaction.rs#L496-L501: annotatenew_contentas/*expected*/,old_contentas/*updated*/, and&Noneas/*rewrite_fault*/in the rollbackrewrite_currentcall.crates/rho-tools/src/apply_patch/apply.rs#L39-L46: annotate the two trailingNonearguments as/*rewrite_fault*/and/*create_fault*/.crates/rho-tools/src/edit_file.rs#L79-L87: annotateargs.replace_allas/*replace_all*/, matching the tests.As per coding guidelines: "comment unavoidable opaque positional arguments with the exact parameter name."
🤖 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/apply_patch/transaction.rs` around lines 496 - 501, Annotate the opaque positional arguments at all three call sites with their exact parameter names: in crates/rho-tools/src/apply_patch/transaction.rs:496-501, mark new_content as /*expected*/, old_content as /*updated*/, and &None as /*rewrite_fault*/ in rewrite_current; in crates/rho-tools/src/apply_patch/apply.rs:39-46, mark the trailing None arguments as /*rewrite_fault*/ and /*create_fault*/; in crates/rho-tools/src/edit_file.rs:79-87, mark args.replace_all as /*replace_all*/.Source: Coding guidelines
crates/rho-tools/src/file_mutation.rs (2)
86-92: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse
symlink_metadatain the Unix identity check for parity with Windows.The Unix
same_file_identitycallsstd::fs::metadata(path), which follows symlinks. The Windows implementation opens the live path withFILE_FLAG_OPEN_REPARSE_POINT, which does not follow reparse points. Today the asymmetry is masked becauselocked_path_identity_matchesrejects non-regular entries at lines 78-80 before it calls this helper. If a second caller invokessame_file_identitywithout that guard, the Unix path will compare against the symlink target and report a match for a substituted path.Make the helper safe on its own.
🛡️ Proposed change
#[cfg(unix)] fn same_file_identity(file: &std::fs::File, path: &Path) -> std::io::Result<bool> { use std::os::unix::fs::MetadataExt; let left = file.metadata()?; - let right = std::fs::metadata(path)?; + let right = std::fs::symlink_metadata(path)?; Ok(left.dev() == right.dev() && left.ino() == right.ino()) }🤖 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/file_mutation.rs` around lines 86 - 92, Update the Unix same_file_identity helper to obtain the path metadata with std::fs::symlink_metadata instead of std::fs::metadata, so identity comparison does not follow symlinks and matches the Windows behavior independently of callers such as locked_path_identity_matches.
553-711: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the platform install backends into a focused submodule.
file_mutation.rsnow holds locking, path-identity checks, rewrite and restore, newline normalization, atomic creation, parent creation, staging, and four platform-specific install backends in one file of over 700 lines. The install backends at lines 553-711 are a clean seam: they depend only onInstallOutcome,AtomicInstallMethod, andAtomicCreateFaultInjector. Moving them tofile_mutation/install.rsshortens the file and collects everyunsafeFFI block in one reviewable place.As per coding guidelines: "In Rust, avoid large files and extract separable behavior into focused modules while keeping tests and invariant documentation close to the 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/file_mutation.rs` around lines 553 - 711, Extract InstallOutcome, install_no_replace, install_platform_no_replace, and install_with_hard_link into a focused file_mutation/install.rs submodule, preserving their existing visibility, cfg gates, platform-specific behavior, and unsafe FFI implementations. Update file_mutation.rs to declare and use the submodule, passing the existing AtomicInstallMethod and AtomicCreateFaultInjector dependencies without changing installation semantics or moving unrelated logic.Source: Coding guidelines
scripts/crate_publish_prep.py (1)
257-259: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDo not let an unknown
package_namesilently produce an empty closure.
adjacencyis keyed only byINTERNAL_PACKAGE_NAMES. Line 259 usesadjacency.get(package_name, ()), so apackage_nameoutside that set yields an empty closure andselect_path_patchesreturns zero patches. That happens after line 305 already determined that patches are required, so the publish dry-run would proceed unpatched and could pass while the real publish fails. The failure is silent.
internal_dependencies_forraisesRuntimeErrorfor an unknown package at line 143. Match that behavior here so the mismatch fails loudly.🛡️ Proposed change
ordered: list[InternalDependency] = [] seen: set[str] = set() - stack = list(reversed(adjacency.get(package_name, ()))) + if package_name not in adjacency: + raise RuntimeError( + f"package {package_name!r} is not an internal released package" + ) + stack = list(reversed(adjacency[package_name]))🤖 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 `@scripts/crate_publish_prep.py` around lines 257 - 259, Update the dependency-closure logic around `select_path_patches` to validate `package_name` against the supported internal package names before reading `adjacency`. Raise `RuntimeError` for unknown packages, matching `internal_dependencies_for`, instead of allowing `adjacency.get(package_name, ())` to return an empty closure.crates/rho-tools/src/edit_format.rs (2)
87-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the tool-name lookups from
ALLso a new variant cannot drift.
is_edit_tool_nameandfrom_tool_nameeach hold a hand-written name list that duplicatestool_name().EditFormatis#[non_exhaustive]and expected to grow. If someone adds a variant,tool_name()anddetail()fail to compile, but these two functions compile and silently returnfalseandNonefor the new tool name.Derive both from
ALLand keep only the legacy alias as an explicit special case.♻️ Proposed refactor
/// Whether `name` is a model-facing built-in edit tool name. /// /// Includes the legacy `edit_file` name so older transcripts and agent /// frontmatter still classify as edit. pub fn is_edit_tool_name(name: &str) -> bool { - matches!(name, "edit" | "apply_patch" | "str_replace" | "edit_file") + Self::from_tool_name(name).is_some() } /// Resolves a model-facing edit tool name. /// /// Names are unique per format. The legacy model-facing name `edit_file` /// still maps to [`Self::StrReplace`]. pub fn from_tool_name(name: &str) -> Option<Self> { - match name { - "edit" => Some(Self::Hashline), - "apply_patch" => Some(Self::ApplyPatch), - "str_replace" | "edit_file" => Some(Self::StrReplace), - _ => None, - } + Self::ALL + .iter() + .copied() + .find(|format| format.tool_name() == name) + // Legacy model-facing name retained for older transcripts. + .or_else(|| (name == "edit_file").then_some(Self::StrReplace)) }🤖 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/edit_format.rs` around lines 87 - 106, Update is_edit_tool_name and from_tool_name to derive current tool-name matching and resolution by iterating over EditFormat::ALL and comparing each variant’s tool_name(), preserving the existing mapping behavior. Keep "edit_file" as the only explicit legacy alias mapping to Self::StrReplace, so newly added variants cannot be omitted.
108-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the one-use
EditFormat::build_sdk_toolwrapper.
EditFormat::build_sdk_toolonly forwards tobuild_edit_sdk_tool. Keep tool construction withsdk_adapter, which already owns the edit tool wiring and call path; useEditFormatonly for tool identity such astool_name.🤖 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/edit_format.rs` around lines 108 - 114, Remove the one-use EditFormat::build_sdk_tool method and update its callers to construct the edit tool through sdk_adapter, which owns the existing build_edit_sdk_tool wiring. Preserve EditFormat only for tool identity, including tool_name, and eliminate the now-unnecessary forwarding wrapper.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.
Inline comments:
In `@crates/rho-tools/src/apply_patch/planning.rs`:
- Around line 66-80: Update the Hunk::Add branch to inspect the requested target
entry itself, including dangling symlinks, before relying on read_optional.
Reject any existing symlink leaf with the existing “file already exists”
ToolError, then retain the current read_optional check for regular files and
other existing entries before constructing FileChange::Add.
In `@crates/rho-tools/src/file_mutation.rs`:
- Around line 527-549: Update stage_file after file.flush() to call
file.sync_data() and propagate errors using the same ToolError and staged-file
context as the existing write and flush failures. Keep the permission
preservation and subsequent installation flow unchanged, matching rewrite’s
durability guarantee.
- Around line 610-617: Update the renameat2 fallback error matching in the
install flow to also treat EOPNOTSUPP and ENOTSUP as unsupported-operation
cases. Preserve the existing ENOSYS and EINVAL handling, and continue invoking
install_no_replace with AtomicInstallMethod::HardLink for all supported fallback
errors.
In `@crates/rho-tools/src/sdk_adapter/mutation.rs`:
- Around line 30-37: Update the match handling in the mutation result flow so
the (Ok(_), Err(capture_error)) case returns an error explicitly stating that
the mutation succeeded but capturing the resulting workspace state failed, while
preserving the existing direct operation-error handling and combined-error
context.
---
Nitpick comments:
In `@crates/rho-tools/src/apply_patch/apply.rs`:
- Around line 39-46: Annotate both positional None arguments in the
apply_hunks_inner call with comments identifying them exactly as rewrite_fault
and create_fault, matching the parameter-name comment style used elsewhere in
the crate.
In `@crates/rho-tools/src/apply_patch/transaction.rs`:
- Around line 496-501: Annotate the opaque positional arguments at all three
call sites with their exact parameter names: in
crates/rho-tools/src/apply_patch/transaction.rs:496-501, mark new_content as
/*expected*/, old_content as /*updated*/, and &None as /*rewrite_fault*/ in
rewrite_current; in crates/rho-tools/src/apply_patch/apply.rs:39-46, mark the
trailing None arguments as /*rewrite_fault*/ and /*create_fault*/; in
crates/rho-tools/src/edit_file.rs:79-87, mark args.replace_all as
/*replace_all*/.
In `@crates/rho-tools/src/edit_file.rs`:
- Around line 79-87: Add the exact `/*replace_all*/` inline annotation to the
`args.replace_all` argument in the `edit_file_content` call, matching the
existing test call sites and leaving the surrounding arguments unchanged.
In `@crates/rho-tools/src/edit_format.rs`:
- Around line 87-106: Update is_edit_tool_name and from_tool_name to derive
current tool-name matching and resolution by iterating over EditFormat::ALL and
comparing each variant’s tool_name(), preserving the existing mapping behavior.
Keep "edit_file" as the only explicit legacy alias mapping to Self::StrReplace,
so newly added variants cannot be omitted.
- Around line 108-114: Remove the one-use EditFormat::build_sdk_tool method and
update its callers to construct the edit tool through sdk_adapter, which owns
the existing build_edit_sdk_tool wiring. Preserve EditFormat only for tool
identity, including tool_name, and eliminate the now-unnecessary forwarding
wrapper.
In `@crates/rho-tools/src/file_mutation.rs`:
- Around line 86-92: Update the Unix same_file_identity helper to obtain the
path metadata with std::fs::symlink_metadata instead of std::fs::metadata, so
identity comparison does not follow symlinks and matches the Windows behavior
independently of callers such as locked_path_identity_matches.
- Around line 553-711: Extract InstallOutcome, install_no_replace,
install_platform_no_replace, and install_with_hard_link into a focused
file_mutation/install.rs submodule, preserving their existing visibility, cfg
gates, platform-specific behavior, and unsafe FFI implementations. Update
file_mutation.rs to declare and use the submodule, passing the existing
AtomicInstallMethod and AtomicCreateFaultInjector dependencies without changing
installation semantics or moving unrelated logic.
In `@crates/rho/src/app/interactive_presenter_format.rs`:
- Around line 255-306: Move apply_patch_card and its focused tests from the
large presenter module into a cohesive private module dedicated to apply-patch
rendering. Preserve the existing parsing, path formatting, change mapping,
statistics, truncation, and empty-diff behavior, and update the caller to use
the extracted renderer while keeping related tests and invariant documentation
alongside it.
In `@scripts/crate_publish_prep.py`:
- Around line 257-259: Update the dependency-closure logic around
`select_path_patches` to validate `package_name` against the supported internal
package names before reading `adjacency`. Raise `RuntimeError` for unknown
packages, matching `internal_dependencies_for`, instead of allowing
`adjacency.get(package_name, ())` to return an empty closure.
🪄 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: ec7473e9-542a-4825-b1f0-c37524b9e878
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (53)
.release-please-manifest.jsoncrates/rho-tools/Cargo.tomlcrates/rho-tools/README.mdcrates/rho-tools/src/apply_patch/apply.rscrates/rho-tools/src/apply_patch/content.rscrates/rho-tools/src/apply_patch/mod.rscrates/rho-tools/src/apply_patch/model.rscrates/rho-tools/src/apply_patch/parser.rscrates/rho-tools/src/apply_patch/planning.rscrates/rho-tools/src/apply_patch/seek_sequence_tests.rscrates/rho-tools/src/apply_patch/transaction.rscrates/rho-tools/src/apply_patch_tests.rscrates/rho-tools/src/edit_file.rscrates/rho-tools/src/edit_file_tests.rscrates/rho-tools/src/edit_format.rscrates/rho-tools/src/file_mutation.rscrates/rho-tools/src/hashline/execute.rscrates/rho-tools/src/lib.rscrates/rho-tools/src/sdk_adapter.rscrates/rho-tools/src/sdk_adapter/edit.rscrates/rho-tools/src/sdk_adapter/mutation.rscrates/rho-tools/src/sdk_adapter/registry.rscrates/rho-tools/src/sdk_adapter_tests.rscrates/rho-tools/src/sdk_support.rscrates/rho-tui-pty/src/scenarios/config.rscrates/rho/Cargo.tomlcrates/rho/src/agent/agent_tests.rscrates/rho/src/agent/definition.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/builtin_skills/rho-config/SKILL.mdcrates/rho/src/claude_runtime/session_process_tests.rscrates/rho/src/config.rscrates/rho/src/config_load_tests.rscrates/rho/src/diagnostics_tests.rscrates/rho/src/hooks/catalog.rscrates/rho/src/hooks/config_tests.rscrates/rho/src/hooks/matcher_tests.rscrates/rho/src/prompt.rscrates/rho/src/tools/coding.rscrates/rho/src/tools/mod.rscrates/rho/src/tools/sdk_registry_tests.rscrates/rho/src/tui/config_picker.rsdocs/configuration.mddocs/configuration/full-example.mddocs/development.mddocs/hooks.mddocs/tools-workspace.mddocs/tools-workspace/edit-format.mdscripts/crate_publish_prep.pyscripts/tests/test_crate_publish_prep.py
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/rho-tui-pty/src/scenarios/config.rs
- docs/hooks.md
- crates/rho-tools/README.md
- crates/rho/src/diagnostics_tests.rs
- docs/configuration/full-example.md
- crates/rho-tools/src/apply_patch/seek_sequence_tests.rs
- crates/rho-tools/src/sdk_support.rs
- crates/rho/src/tools/sdk_registry_tests.rs
- crates/rho-tools/src/hashline/execute.rs
- crates/rho/src/prompt.rs
- crates/rho-tools/src/apply_patch/mod.rs
- crates/rho/src/tui/config_picker.rs
- crates/rho-tools/src/sdk_adapter_tests.rs
| let error = std::io::Error::last_os_error(); | ||
| if !matches!( | ||
| error.raw_os_error(), | ||
| Some(libc::ENOSYS) | Some(libc::EINVAL) | ||
| ) { | ||
| return InstallOutcome::NotInstalled(error); | ||
| } | ||
| install_no_replace(staged, target, AtomicInstallMethod::HardLink, fault) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
renameat2 RENAME_NOREPLACE overlayfs EOPNOTSUPP fallback
💡 Result:
When using the renameat2(2) system call with the RENAME_NOREPLACE flag, the operation requires explicit support from the underlying filesystem [1][2]. If a filesystem does not support this flag, the kernel returns EOPNOTSUPP [3][4]. Overlayfs does not inherently support the RENAME_NOREPLACE flag, and there is no automatic kernel-level "fallback" mechanism that translates a failed RENAME_NOREPLACE attempt into an equivalent operation [1][2][5]. Applications that require this functionality on overlayfs must implement their own user-space fallback logic [5]. To implement a reliable fallback, an application should typically: 1. Attempt the renameat2(2) operation with RENAME_NOREPLACE. 2. If the operation fails with EOPNOTSUPP, catch the error. 3. Perform a manual check (e.g., using stat(2) or access(2)) to determine if the destination path already exists. 4. If the destination does not exist, proceed with a standard rename(2) call. 5. If the destination exists, handle the conflict appropriately (e.g., return EEXIST) to mimic the behavior of RENAME_NOREPLACE [1][2]. Note that because overlayfs is a union filesystem, performing manual checks followed by a rename is not atomic, which can introduce race conditions if other processes are modifying the directory concurrently [1][6]. Application developers should be aware of these potential races when designing fallback strategies [5].
Citations:
- 1: https://www.man7.org/linux/man-pages/man2/renameat2.2.html
- 2: https://manpages.debian.org/bookworm/manpages-dev/renameat2.2.en.html
- 3: https://man.freebsd.org/renameat(2)
- 4: https://lkml.indiana.edu/hypermail/linux/kernel/1401.1/05450.html
- 5: https://lwn.net/Articles/574380/
- 6: https://man7.org/linux/man-pages/man2/rename.2.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant source section without executing repository code.
if [ -f crates/rho-tools/src/file_mutation.rs ]; then
wc -l crates/rho-tools/src/file_mutation.rs
sed -n '540,705p' crates/rho-tools/src/file_mutation.rs | cat -n
else
echo "crates/rho-tools/src/file_mutation.rs not found"
fd -a file_mutation.rs .
fi
# Check the exact raw_os_error match expression and Linux target fallback references.
rg -n "matches!\\(|raw_os_error\\(|EOPNOTSUPP|ENOTSUP|R(?:E\\{||ENAME|NAME_NOREPLACE|ENAME_NOREPLACE)|install_platform_no_replace|install_no_replace" crates/rho-tools/src/file_mutation.rs || true
# Inspect crate target defaults / cfg flags related to file_mutation.
if [ -f crates/rho-tools/Cargo.toml ]; then
sed -n '1,220p' crates/rho-tools/Cargo.toml | cat -n
fi
# Read-only static verifier: identify exact matches on raw_os_error and whether the fallback
# includes unsupported-operation errors for Unix/Linux syscall failures.
python3 - <<'PY'
from pathlib import Path
p = Path("crates/rho-tools/src/file_mutation.rs")
if not p.exists():
raise SystemExit("missing source")
s = p.read_text()
needle = "error.raw_os_error()"
idx = s.find(needle)
print("needle_offset", idx)
if idx >= 0:
snippet = s[max(0, idx-250): idx+350]
print("snippet:")
print(snippet)
for err in ["EOPNOTSUPP", "ENOTSUP", "ENOSYS", "EINVAL"]:
print(f"{err}_in_snippet", err in snippet)
PYRepository: matthewyjiang/rho
Length of output: 12812
🌐 Web query:
RUSTUP_TARGET linux-gnu libc ENOTSUP EOPNOTSUPP values
💡 Result:
On Linux targets, including those used by rustup (such as x86_64-unknown-linux-gnu), ENOTSUP and EOPNOTSUPP represent the same error condition and have identical integer values [1][2][3]. Specifically, both constants are defined as 95 in the Rust libc crate [1][4][5]. This equivalence often appears in technical contexts because certain system calls, notably copy_file_range, may return EOPNOTSUPP on older Linux kernels or specific file systems (like NFS) when the operation is not supported [6][7]. When rustup or other Rust tools encounter this error, it is frequently a result of the underlying operating system or file system lacking support for the specific system call being attempted, rather than a configuration error within rustup itself [6][7]. In the Rust standard library and ecosystem, these error codes are treated as synonymous on Linux platforms [2][3]. Consequently, many libraries map both to the same error category, such as ErrorKind::Unsupported, to provide a consistent interface across different environments [2][8].
Citations:
- 1: https://docs.rs/libc/latest/libc/constant.ENOTSUP.html
- 2: std: map ENOTSUP to ErrorKind::Unsupported rust-lang/rust#158580
- 3: Add ENOTSUP to Linux and Android nix-rust/nix#969
- 4: https://docs.rs/libc/0.2.178/libc/constant.ENOTSUP.html
- 5: https://docs.rs/rs-streams/latest/rs_streams/constant.ENOTSUP.html
- 6: rustup appears to depend on new-ish system call in linux rust-lang/rustup#2472
- 7: Rustup fails to install on RHEL 7 if /home is mapped to an NFS filesystem rust-lang/rustup#2452
- 8: https://shadow.github.io/docs/rust/linux_api/errno/struct.Errno.html
Extend the renameat2 fallback to unsupported-operation errors.
renameat2 with RENAME_NOREPLACE can fail with EOPNOTSUPP/ENOTSUP when the filesystem does not support this flag, such as overlayfs. The fallback only handles ENOSYS and EINVAL, so file creation reports NotInstalled instead of using the hard-link fallback.
Add the unsupported-operation errors to the fallback set.
🤖 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/file_mutation.rs` around lines 610 - 617, Update the
renameat2 fallback error matching in the install flow to also treat EOPNOTSUPP
and ENOTSUP as unsupported-operation cases. Preserve the existing ENOSYS and
EINVAL handling, and continue invoking install_no_replace with
AtomicInstallMethod::HardLink for all supported fallback errors.
Keep the 1.x edit_file tool-name alias for transcripts and agent frontmatter, and mark it NEXT_MAJOR so 2.0 drops it in favor of str_replace only.
Reject dangling symlink add targets, fsync staged creates, and clarify post-mutation capture failures. Extract install/apply-patch helpers and drive edit tool-name resolution from EditFormat::ALL.
1 participant
Summary
Rho previously exposed only the hash-line
edittool. This PR lets users selectedit,apply_patch, oredit_filewith[behavior].edit_tool; Rho exposes only the selected schema after restart.apply_patchand exact-stringedit_fileimplementations with shared mutation output./config, prompts, diagnostics, hooks, policies, rewind metadata, and tool presentation.Validation
apply_patchtransaction suite: 18 passedrho-coding-agent --lib: 1607 passed, 2 ignoredopen_config_picker: passedDocs TUI proof plate
bash scripts/check_docs_ui_demo.sh --check(throughpython3 scripts/validate.py full)Test gate
rho-test-selection(failure mode, owner layer, gap).crates/rho/src/tuiunit tests cover pure formatting and mapping logic..containschecks cover model-facing tool protocol output rather than UI copy.New tests
/configor see the restart requirementPTY exception (if any)
Next-major debt
rho-next-major-debt(ideal shape considered; compromise intentional).NEXT_MAJOR(<surface>): <cleanup>marker on the API.Markers added
NEXT_MAJOR(rho-tools): remove the EditToolKind alias and use EditFormat directlyEditFormat.Summary by CodeRabbit
New Features
Bug Fixes
Documentation