Repository navigation
feat(tools): rework apply_patch diagnostics, paths, and workspace root - #60
Merged
Merged
Conversation
Skill frontmatter was scanned line by line, so a `metadata:` map with indented keys failed with `invalid frontmatter line` and the whole skill was dropped from the catalog. Real skill files carry structured metadata, so that silently hid them from the model. - Read frontmatter with serde_norway and keep only the typed `name` and `description` fields; unknown top-level keys are ignored on purpose. - Accept plain multi-line scalars, `|`/`>` blocks, quoted values, trailing comments, CRLF line endings, and a leading byte-order mark. - Normalize name and description in `SkillMetadata::new`, so the stable prefix and the CLI completion list share one single-line invariant instead of depending on the renderer. - Type loader failures as `FrontmatterError` and `SkillLoadWarningReason` instead of `String` messages. - Move frontmatter parsing into `skill/frontmatter.rs` and share `text::collapse_whitespace` with memory canonicalization. Verified with cargo fmt --all --check, cargo clippy --all-targets --all-features -- -D warnings, and cargo test --all.
…ne counts apply_patch failures were hard to localize and the documented DSL did not match what the tool really accepts. Patch-text failures now carry their own `apply_patch_syntax` and `apply_patch_noop` codes instead of repeating the workspace path contract, a preimage miss names the closest line and the first difference, an ambiguous preimage lists the matching lines, and CRLF line endings are reported as the cause when byte-exact matching fails. Repeated Update sections for one file now merge instead of being rejected, context-only hunks are dropped and counted next to real edits, and an envelope with no `+`/`-` line at all fails as a no-op rather than writing nothing silently. `*** Delete File:` is a supported operation that verifies the preimage, enforces the same path scope, and confirms the path is absent afterwards. Success envelopes report `op` plus line and byte counts, and the TUI renders Created/Edited/Deleted with line counts before bytes. The provider-visible tool description and coding capability summary are updated in the same change, deliberately, to document the real rules. Verification: cargo fmt --all --check (clean), cargo clippy --all-targets --all-features -- -D warnings (clean), cargo test --all (58 targets, exit 0).
…aths Deletion bypassed two write-side invariants that its own documentation claimed. The preimage comparison ran only for updates, so a file replaced between planning and unlinking was removed anyway while the recorded evidence still described the planned bytes; the delete path also opened the target read-write, which made removing a write-protected file fail with an unrelated "could not open workspace file for patching" error even though unlinking only needs write permission on the parent directory. Both operations now share one immediate pre-mutation preimage check, and a delete opens its target read-only. The patch-execution test hook is generalized to install at either execution point, and the new changed-before-mutation test fails deterministically when that guard is removed. The TUI call detail now names the file in a `*** Delete File:` section instead of showing only the payload size, and the tool description, coding capability summary, and syntax guidance describe the merged-update rule the parser actually implements. Verification: cargo fmt --all --check (clean), cargo clippy --all-targets --all-features -- -D warnings (clean), cargo test --all (58 targets, exit 0), plus a manual flip test proving both new delete tests fail without their fix.
Workspace tools only accepted paths relative to a configured root, so a caller holding a path from process output or an earlier tool result had to translate it before reading or patching, and a copied path failed even though it named a file the workspace already allowed. A path may now be workspace-relative or absolute inside a root; dot segments are resolved lexically, and a path that escapes every root, reaches the root itself, or names a file outside the workspace is denied with an explicit reason. The relaxation changes spelling only, never scope: normalized components still pass the hidden-path rule, the patch write scope, forbidden paths, per-component symlink rejection, and canonical containment, and the runtime keeps rejecting evidence whose `relative_path` is not relative. Failures report the normalized workspace-relative form and never echo a host root, because failure text is provider-visible. Provider-visible text changes with the contract: the `read_text` and `apply_patch` descriptions, the coding capability summary, and the path contract and recovery guidance. The change is static and session-stable, so it does not break prompt or KV cache reuse within a session. Verification: cargo fmt --all --check (clean), cargo clippy --all-targets --all-features -- -D warnings (clean), cargo test --all (58 targets, 2132 tests, exit 0), including new coverage for absolute paths inside and outside the workspace, dot-segment normalization, escape denial, root and prefix-lookalike denial, and hidden paths after normalization.
The success payload existed twice: the tool crate serialized internal structs while the CLI re-declared the same fields as serde mirrors, so a renamed or added field could silently stop rendering. The types now live in one public `patch::envelope` module that the tool serializes and the CLI deserializes, with `serde(default)` keeping an envelope recorded by an older build readable and `serde(other)` keeping an operation or line kind from a newer build readable instead of discarding the whole change. Round trip, legacy shape, and exact wire shape are covered by tests. The failure classifier no longer matches the same codes twice: one `FailureClass` decides both the guidance and whether the workspace path contract applies, and the test helper states the expectation as data per code instead of re-running the production predicate. Planned line counts are stored beside the byte counts so a proposal summary cannot disagree with the change it describes. `crates/merry-tools/src/tests/patch.rs` is split into focused submodules under `tests/patch/`, with every test, assertion, and string literal preserved. Delete also gains runtime-level coverage: an opted-in delete removes the file through the runtime and reports `op: delete` in the provider-visible continuation, a policy-denied delete leaves the file, and a delete outside the write scope is denied with the file intact. Verification: cargo fmt --all --check (clean), cargo clippy --all-targets --all-features -- -D warnings (clean), cargo test --all (58 targets, 2140 tests, exit 0), plus a mechanical check that the split keeps all 33 tests, 124 assertions, and the exact multiset of string literals.
Workspace tools re-decided path policy that the sandbox and trusted path configuration already own, which turned legitimate spellings into tool failures: an absolute path outside a configured root, a relative path that climbs with `..`, and any dot-prefixed component such as `.github/workflows` were all rejected before the target was even inspected. `validate_workspace_path_argument` now accepts every spelling. A relative argument still resolves against the configured roots, an absolute argument is used as named and may address a file outside every root, `..` is resolved lexically with the escape kept visible in the reported path, and a leading dot is ordinary spelling. `WorkspaceToolsConfig::allow_hidden` and its plumbing through the coding profile, CLI, PyO3 binding, and Python SDK are removed rather than left as a switch that no longer means anything. Reachability and writability are the sandbox's decision, so the tools stop duplicating it. Three deliberate boundaries remain: opens keep `O_NOFOLLOW` so the leaf cannot be swapped for a link, components inside a configured root are still walked without following symlinks, and a child agent cannot leave the write scope its parent gave it because those patterns are root-relative and an outside target has no spelling that matches. Runtime patch evidence accepted only root-relative paths, which would have blocked an outside-workspace change after the tools allowed it. It now also accepts an absolute path while still rejecting blank text, control characters, empty segments, and dot segments, so one target cannot compare unequal under two spellings. Tool descriptions, the coding capability summary, the path contract, and recovery guidance now say that every spelling is accepted and that the sandbox decides reachability, so provider-visible text stops promising a denial that no longer happens. Verification: cargo fmt --all --check (clean), cargo clippy --all-targets --all-features -- -D warnings (clean), cargo test --all (58 targets, 2143 passed), git diff --check (clean), and the Python gates in the CI form (ruff check, ruff format --check, ty check, pytest 53 passed). The two new boundary tests were inverted to confirm they fail against the old behavior.
Addressing a file through a root list made an absolute path ambiguous: it was stripped to root-relative components and then joined against every root in order, so naming <root-b>/note.txt could read or delete <root-a>/note.txt when both roots held the file. A workspace has one root now, so a path never has to be guessed between candidates, and the read-only resource roots that hold skill directories stay separate. - WorkspaceToolsConfig::new takes one root, WorkspaceToolState stores it, and the patch planner and read_text resolve against that root instead of looping over a root list with first-match-wins ordering. - An absolute path below the workspace root is still rewritten to its relative spelling, so results stay free of host paths and root-relative write-scope and forbidden-path rules keep matching. Every other absolute path is used exactly as written and is never re-anchored. - A relative path resolves under the workspace root first and then under each read-only resource root, which is what lets a skill's own relative SKILL.md path resolve. - resolve_existing_path and resolve_new_file_path now own path resolution and return the resolved path, so read_text and the patch planner no longer repeat the candidate loop. NewWorkspacePath::ParentMissing is gone because a single root has no fallback root left to try. - CodingAgentProfileBuilder::new takes the root; with_roots and the Python Roots sequence are removed. The profile hash keeps the same workspace-root field, so a single-root profile hashes unchanged. - The path contract, capability summary, module docs, and the Python README now describe one workspace root plus read-only resource roots. Reproducing the old collapse makes read_text_absolute_resource_path_is_not_shadowed_by_a_workspace_file fail, so the regression test is genuine. Verified with cargo fmt --all --check, cargo clippy --all-targets --all-features -- -D warnings, cargo test --all, and the Python SDK gates (ruff check, ruff format --check, ty check, pytest, uv build).
The call detail and the TUI projector each matched their own subset of the `*** Add File:`, `*** Update File:`, and `*** Delete File:` headers, so the two readers could disagree about which files a patch names. - Add one `apply_patch_argument` module that owns section-header recognition for presentation, with tests that pin every operation and that directives and hunk lines are never headers. - Both readers use it. The projector now numbers an add section from its first line and starts update and delete sections at their hunk headers, which is the previous behavior expressed as one match. merry-tools still owns the grammar that validates and applies a patch. Verified with cargo fmt --all --check, cargo clippy -p merry-cli --all-targets --all-features -- -D warnings, and cargo test -p merry-cli.
Control characters were filtered or replaced in the call detail, the tool result preview, and both timeline renderers, so each new renderer had to remember the rule and the rules had started to differ. - Add one `text` module with the three intents the CLI actually has: drop control characters for single-line text, replace them with spaces so words stay separated, and keep newlines when a preview shows lines. - The call detail, the compacted tool output, and the timeline renderers now call those helpers instead of open-coding the filter. The trailing-space rule of a displayed shell command stays where it is, because trimming is a presentation decision of that field rather than a control-character policy. Verified with cargo fmt --all --check, cargo clippy -p merry-cli --all-targets --all-features -- -D warnings, and cargo test -p merry-cli.
`apply_patch` read a complete file twice, once while planning a preimage and once immediately before mutating the file, and each copy repeated the same regular-file check, byte budget, seek, and cancellation steps. Two copies of a limit can drift, and the pre-write copy existed only because the planning copy lived inside the planner. - Add a `file` module that owns reading every byte of an open workspace file under the configured budget, plus decoding that read as UTF-8. - The patch planner and the pre-write verification call it instead of keeping their own copies. The planner still rejects a NUL byte, because editing a binary file is a patch decision rather than a read policy. - `read_text` keeps its line-oriented reader: it streams a range under a smaller budget and must not read the whole file. Verified with cargo fmt --all --check, cargo clippy -p merry-tools --all-targets --all-features -- -D warnings, and cargo test -p merry-tools, which covers the oversized, binary, non-UTF-8, and cancellation paths.
`skill.rs` was 727 lines, and 280 of them were two `#[cfg(test)]` modules that the file had outgrown. The module is now 451 lines of production code, with the tests in `skill/tests.rs` grouped by the rendering and loader responsibilities they cover. Only the test module declaration changed; every test moved unchanged. Verified with cargo fmt --all --check, cargo clippy -p merry-runtime --all-targets --all-features -- -D warnings, and cargo test -p merry-runtime, which still runs all 17 skill tests.
`path.rs` mixed two responsibilities with different risk: lexical validation of a caller's argument, which never touches the filesystem, and resolution plus file opening, which owns the symlink rule and every `O_NOFOLLOW` open. The file had reached 565 lines across both. - `path/validate.rs` owns `ValidatedToolPath`, its normalization, and the reported spelling. It performs no filesystem access. - `path/open.rs` owns resolution under the roots, new-file probing, parent directory creation, and the open helpers. - `path.rs` is now a 27-line module root that documents the split and re-exports the crate-internal surface, so no call site changed. The behavior, error codes, and symlink boundary are unchanged; every merry-tools test still passes. Verified with cargo fmt --all --check, cargo clippy -p merry-tools --all-targets --all-features -- -D warnings, cargo test -p merry-tools, and cargo test --all.
The failure-code expectation table ended in a catch-all, so a new code silently inherited "no guidance, but repeat the path contract" instead of forcing a decision about what the model should do next. - Declare every code through one `workspace_error_codes!` invocation that also registers it, so registration cannot drift from the constants. - `expected_recovery_for_code` returns `Option`, and a new coverage test fails for any registered code without an explicit entry; the entries for the read, write, and UTF-8 codes are now written down instead of implied. - Removing the entry for `workspace_file_not_utf8` makes the test fail with `ERROR_NOT_UTF8 (workspace_file_not_utf8) needs an explicit expected recovery entry`, so the check is genuine. Verified with cargo fmt --all --check, cargo clippy -p merry-tools --all-targets --all-features -- -D warnings, and cargo test -p merry-tools.
…not need `PatchOperationView` is constructed by the projector and matched by the renderer, so the allowance only hid a future real dead-code warning for the enum. merry-cli builds and lints without it. Verified with cargo check -p merry-cli --all-targets, cargo clippy -p merry-cli --all-targets --all-features -- -D warnings, and cargo test -p merry-cli.
The two tests added with single-root support placed their second root inside the workspace root, so the absolute path they used was also a workspace path and collapsed to its relative spelling. They passed without ever exercising the anchor they were meant to protect. - The resource-root test now uses a sibling directory tree with a same-named `demo/SKILL.md`, which is what makes the read answer depend on the anchor instead of on the root order. - The patch test now names a sibling file that shares a name with a workspace file, so an anchored write must change the named file and leave the workspace copy alone. - Making `anchors` treat an absolute argument as anchored at every root, which is the collapsed behavior the fix removed, now fails both tests. Verified with cargo fmt --all --check, cargo clippy -p merry-tools --all-targets --all-features -- -D warnings, and cargo test -p merry-tools.
`merry-coding/src/lib.rs` owned the builder, the profile types, and 245 lines of hand-assembled hash material, so the file that defines the public surface also had to be read to learn what identifies a profile. - Add `profile_hash.rs`, which owns `CodingAgentProfileHash` and the material folded into it, and re-export the type from the crate root so the public path is unchanged. - Extract the retry block and the repeated `"on"/"off"` spelling into `append_model_retry_policy` and `on_or_off`, which removes the copy of that decision that lived in each boolean field. - Document that field names and their order are the compatibility contract, because they are what keep two adjacent values from hashing alike. The hashed bytes are unchanged: a fixed-input probe prints `fnv1a64:1a6ae97057d8c5d1` both before and after this move, so a resumed session sees the same stable profile identity. Verified with cargo fmt --all --check, cargo clippy -p merry-coding --all-targets --all-features -- -D warnings, and cargo test -p merry-coding.
10 of 12 tasks
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
Reworks the workspace tools (mainly
apply_patch) around three findings: thetool contract did not state its own grammar, a failed preimage match was not
localizable, and path/root policy was decided in the wrong layer.
Diagnostics and grammar (
027b937)apply_patch_syntaxandapply_patch_noopcodes. A preimage miss nowreports the closest line, the first differing line, and whether CRLF is why
bytes do not match; an ambiguous preimage lists its match lines.
*** Update File:sections for one file merge into one changeinstead of being rejected, context-only hunks are dropped and counted when
the envelope contains real edits, and an all-context envelope fails with
apply_patch_noopinstead of a generic invalid-arguments error.*** Delete File:is now a supported section with the same preimage, scope,and verification contract as an update.
Result envelope (
4f00a8d,a250f17)merry-tools::patch::envelope) for the success envelope, used bythe tool to write and by the CLI to read;
serde(default)/serde(other)keep older and newer envelopes readable.
lines_before/lines_after),delete renders as a deletion rather than
Edited (+0 -0), and approval andaudit summaries lead with lines.
Path policy (
a27cd64,d2a134f)inside the workspace, absolute paths outside it, and dot-prefixed components
are all accepted, and the sandbox, accepted process profile, and trusted path
rules decide what exists and what is writable.
ambiguous: it was stripped to root-relative components and rejoined against
every root in order, so naming
<root-b>/note.txtcould read or delete<root-a>/note.txtwhen both roots held that file. A workspace now has oneroot; read-only resource roots (skills) stay separate.
Breaking change
refactor(tools)!: support one workspace root instead of manychanges theworkspace surface.
WorkspaceToolsConfig::new,WorkspaceToolState,CodingAgentProfileBuilder::with_roots(removed), the PyO3with_workspaceargument list, and Python
WorkspaceConfigall take one root. Read-onlyresource roots are unchanged. The hashed profile material is unchanged for a
single-root profile, so a resumed session keeps its stable profile identity.
Review follow-ups
apply_patchnow has one owner for each concern it had duplicated: CLI sectionheader recognition, CLI control-character sanitizing, bounded whole-file reads,
path validation versus file opening, and profile hash material.
skill.rsandmerry-coding/src/lib.rswere split at their real responsibility boundaries.Every workspace error code must now declare its expected recovery in the test
table, and the two regression tests for absolute-path anchoring were rewritten
because the first version placed its second root inside the workspace root and
therefore never exercised the anchor.
Verification
cargo fmt --all --checkcleancargo clippy --all-targets --all-features -- -D warningscleancargo test --allexit 0, 2151 passed, 0 failedgit diff --checkcleanmaturin develop --uv --features test-utils,ruff check,ruff format --check,ty check,pytest(53 passed),uv buildBehavior checks that were made to fail on purpose, so the tests are known to be
load-bearing:
apply_patch_delete_refuses_file_changed_before_mutation.child_write_scope_denies_absolute_paths_outside_the_workspace.workspace_file_not_utf8fails the new error-code coverage test.Notes for the reviewer
b5d6318 fix(runtime): parse skill frontmatter as YAML, isalso the head of fix(runtime): parse skill frontmatter as YAML #59. That PR should merge first; this branch then carries the
remaining 15 commits.
forbidden_pathsstill includes.gitfor aroot-relative target only, so an outside path spelling
.gitis not coveredby that default. Changing it is a separate decision.
failure names CRLF and suggests converting the file or editing outside
apply_patch).