Repository navigation
fix(tools): prevent Edit MV from deleting its source through path aliases - #1451
Conversation
Writing an aliased destination and then removing the source can delete the same file. Reject self-moves after canonical permission checks and before any filesystem mutation, including mixed content edits. Cover Windows case aliases, lexical paths and directory links through the tool entry point, and verify Read provenance through the Host RPC.
muzimu217
left a comment
muzimu217
left a comment
There was a problem hiding this comment.
Verified locally on macOS (arm64, current main 278e929ca + this branch).
The bug is real and the fix is right. On main, the MV branch writes the destination and then removes the source with no same-path check (tools/mod.rs, the if let Some(dest) = &success.moved_to block), and the apply layer's no-change guard explicitly exempts MV (hashline/apply.rs:596, plan.mv.is_none()). The new equality check sits after the sensitivity gate and before create_dir_all/write/remove, so a rejected self-move performs zero filesystem mutation — which is what makes the combined PUT + MV rejection atomic. Reusing EDIT_NO_CHANGE keeps the error-table surface small and the two spec tables updated to match.
Tests: cargo test -p host-core edit_ → 14 passed, 1 failed locally on macOS. The passing 14 include the directory-symlink alias case, the distinct-destination move, and the RPC-level test (relative alias + PUT+MV + source bytes preserved + original tag still usable). CI green on Linux.
One new test fails on macOS — edit_move_to_self_preserves_source_bytes, the absolute-destination iteration:
expected EDIT_NO_CHANGE, got PATH_OUTSIDE_WORKSPACE
Cause: macOS tempdir() lives under /var, a symlink to /private/var. The absolute destination canonicalizes to /private/var/... while the workspace root spelling stays /var/..., so the workspace-boundary check rejects the destination before the equality check is ever reached. No data loss either way — but the test as written cannot pass on a stock macOS checkout. Suggest canonicalizing the root once in the test (std::fs::canonicalize(dir.path()) passed as the workspace root) so the absolute alias resolves inside the workspace and hits the intended EDIT_NO_CHANGE.
Non-blocking notes:
- The same-path guarantee only holds when the destination survives resolution; a symlinked workspace root makes absolute self-aliases surface as
PATH_OUTSIDE_WORKSPACEinstead. Rare in practice, but worth a sentence in §7.2 if you want the contract exhaustive. - The case-only destination is only exercised on Windows (
cfg!(windows)). On a case-insensitive macOS volume canonicalization should produce the same rejection; enabling that destination behind a runtime case-sensitivity probe (the approach #1452 needs for its alias tests) would cover it. - Docs: this PR and #1452 both touch
18-line-anchored-edit-contract.mdand04-e2e-test-plan.md— whoever merges second will need a rebase.
fix scope, fits the current contribution window; the fix itself gets a clean pass from me once the macOS test assumption is fixed.
Keep the published repair branch current without rewriting its history.
Canonicalize both the temporary workspace and absolute destination so macOS temporary-directory aliases reach the intended self-move guard. Probe case sensitivity instead of assuming it from the operating system, and clarify that permission and path checks precede the rejection.
|
Thanks for the macOS reproduction. Fixed in An independent follow-up review found no blockers. All 15 related Rust tests pass on Windows, including the tool and RPC regressions; formatting and Clippy pass. I haven't run macOS locally—could you please recheck there? |
The renderer extension shipped without reaching the documentation that decides what an author can discover. The permission matrix had no renderer.extension row, the manifest schema never named renderer, rendererActions or rendererCallMethods, and the built-in development guide mentioned no slot at all, so a plugin that draws in the composer or on a message's action bar had no documented way to exist. The matrix gains the permission with its risk tier and its display copy, the manifest schema gains the three fields with the slot contract and the whitelist vocabulary, and the zero-to-one guide and the built-in skill guide each gain a renderer slots section. The zh-CN mirrors follow.
…contract docs(plugins): document the renderer slot contract
The Jev card invited users to paste a TypeSafe key with no way to know whether it worked, and the classifier sat next to the AI service list as a special section instead of being one of the services. Jev is now offered by "Add service" in its own Classifiers group, on the add path only, because it owns no provider row and no model list: changing an existing row's service can never turn it into a classifier. Adding it is one action with two steps. The key is checked against the same TypeSafe System One address and classifier the Agent's `JevClassify` tool calls, and only a key that answered is stored and turns the classifier on, so an enabled Jev always has a key the Agent can spend. A refused key writes nothing and reports TypeSafe's status, and closing the dialog while the check is in flight cancels the whole action, so an abandoned dialog cannot leave a credential behind. The credential is an API key; this integration has no OAuth path. The card becomes the state of the integration rather than a second place to enter a key: the stored-key status, the Agent-mode switch, and the actions that add, replace or remove the key. Removing the key still turns Jev off before it is deleted.
feat(settings): add Jev from the service chooser with a checked key
The Jev card sat on the model configuration page of every install, offering "Add key" to users who had never added TypeSafe Jev at all. Jev is one of the services "Add service" provides, so an install without it now has nothing on that page: the card appears once a key is stored, and it leaves again when the key is removed, after the switch that removal already turned off. Two states keep the card: a Host that cannot answer the secret lookup, and an enabled setting that contradicts a missing key. Hiding a configured install would be the worse failure. The status read stays with the card, so the dialog's report of a stored key is what makes the card appear; the card-visibility test covers both directions.
…added fix(settings): show the Jev card only once Jev has been added
muzimu217
left a comment
muzimu217
left a comment
There was a problem hiding this comment.
Confirmed on macOS (arm64): cargo test -p host-core edit_ now passes 15/15 on 5bb56226a. The canonicalized workspace root plus the filesystem case probe resolve both the symlinked tempdir (/var → /private/var) and the case-alias assumption. Nothing blocking from my side.
The former 4 DIP default looked nearly square and did not follow the shared design scale. Use the 12 DIP radius token as the native default while preserving authorized theme overrides.
Markdown destinations with drive letters were filtered as URI schemes before the existing file opener could handle them. Normalize parsed absolute Windows paths during the Markdown pass and decode them through the current click path, keeping sanitization and file access checks intact.
|
Thanks for fixing the macOS symlink and case-sensitivity assumptions. Merged as 177223a after the latest-main candidate passed: 15 Rust Edit tests, cargo fmt, Clippy, the self-move RPC E2E (atomic rejection, source bytes preserved, original Read tag still usable), and all five GitHub checks. No follow-up blockers. |
3 participants
Problem
An Edit
MVcan delete the source file when the destination resolves to the same path. The tool reports success even though the file disappears.For example:
source.txtand callReadto obtain its tag.Editwith that tag,path: "source.txt", andops: "MV ./source.txt\n".source.txtno longer exists.This also occurs with identical paths, absolute paths,
..aliases, and case-only spellings on ordinary Windows filesystems. A model attempting a case-only rename can therefore cause data loss.Cause and fix
The MV branch writes the destination and then removes the source. Although both paths pass the existing canonical resolution and permission checks, it does not check whether they resolve to the same path. In that case, removing the source deletes the file just written.
Reject the move with the existing
EDIT_NO_CHANGEerror when the resolved paths are equal, before any filesystem mutation. A combinedPUT + MVcall is rejected as a whole, leaving the original bytes and Read provenance intact.Moves to distinct paths remain supported. Case-only renames on case-insensitive filesystems now fail safely; this change does not implement case-only rename support.
Validation
tools.executeRPC test verifies unchanged bytes and a subsequent valid Edit using the original Read tag.The Edit contract, error-code documentation and E2E-138 scenario are updated. No schema migration or RPC shape change is required.
macOS review follow-up
Canonical temporary roots and a filesystem case-sensitivity probe replace the platform assumptions. The self-move assertion remains EDIT_NO_CHANGE. All 15 related Rust tests pass on Windows; formatting and Clippy pass. Independent follow-up review passed. macOS has not been rerun locally; reviewer revalidation was requested. Validation applies to 5bb5622.