fix(convo): relativize file change keys against path.base#124
Open
akesling wants to merge 7 commits into
Open
Conversation
|
🔍 Preview deployed: https://b0a28189.toolpath.pages.dev |
…e derive/extract mechanism
…relativized change keys #124 relativized file-change artifact keys against path.base but only had one integration test and no property coverage. Adds: a 9-case derive->JSON->extract->re-derive identity/idempotency table (nested, exact-root, outside-base, trailing-slash base, deep nesting, unicode, no-base absolute/relative, prefix-not-boundary); relativize_key/resolve_key edge-case units (exact root, trailing-slash base, empty root, case sensitivity, URI passthrough); a files_changed-consistency test; and a backward-compat characterization pinning that documents shared before relativization (absolute keys) still extract unchanged.
…onversation providers Extends each provider's real-fixture test with a ground-truth check for #124's key relativization: for every derived file.write change, the key is relative iff its original path genuinely sat under path.base at a path-component boundary (independently recomputed via std::path, not by calling derive.rs's own relativize_key) -- catching both an absolute-under-base leak and a wrongly-relativized outside-base key -- plus extract -> re-derive idempotency of the file.write key set. claude and cursor fixtures carry real absolute file mutations under a recorded base, so stripping is exercised directly. gemini and pi's fixtures record already-relative tool-call paths, so a synthetic absolute mutation/tool-call is injected to genuinely exercise stripping alongside the fixture's own untouched relative paths. codex already had a relativize-and-assert-membership test in fidelity.rs; this adds the leak/idempotency half next to it. opencode's plain to_view() (no snapshot resolver) still runs the tool-input fallback unconditionally, so its real fixture's absolute paths exercise stripping directly. cursor gets a new real_fixture_roundtrip.rs (it previously only had a live-DB-gated sanity test and a cross-harness projection test, neither of which exercised this fixture's key relativization).
…uments path p validate keeps accepting a pre-#124 document whose file.write keys are verbatim absolute paths -- relativization is a derive_path write-side behavior, not a schema requirement, so old documents stay valid forever. path p merge concatenates paths verbatim with no cross-document key unification (see cmd_merge.rs::merge_into_graph), so two documents that touch the "same" file under different key conventions -- one absolute, one base-relative -- merge with both keys surviving as distinct artifacts. Pins this as the intended (if unglamorous) cross-version behavior rather than a bug to fix. Neither test needs the $TOOLPATH_CONFIG_DIR/TEST_ENV_LOCK sandbox the resume/share/import suites use: validate and merge operate purely on the file paths given to them and never touch $HOME or the cache.
Review follow-up on the #124 provider key-stability tests. Finding 1: codex, opencode, and cursor had the derive+assert block inlined in the #[test] body while claude/gemini/pi factored it into a local assert_file_write_keys_match_base helper. Extract each into a matching per-crate local helper (no shared test-util crate — the cross-crate sameness is accepted). codex now sources ground truth from the view's FileMutation paths (Codex populates these from patch_apply_end) and uses BTreeSet like the others instead of HashSet. Finding 2: the no-leak assertion could silently no-op — a fixture with no absolute-under-base path never enters the `if under_base` branch, so the test would pass without ever exercising the exact case #124 relativizes. All six helpers now track a saw_under_base bool and assert it fired at least once. Verified per provider: claude, codex, opencode, and cursor real fixtures already carry absolute paths under their recorded base, so the branch fires on real data with no injection; gemini and pi record provider-relative tool paths (and derive no usable base), so they keep the synthetic absolute-under-base FileMutation/tool-call injection that makes the branch genuinely run.
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.
Root cause
The RFC keys
changeartifacts as URLs, with bare paths relative topath.base— butderive_pathinserted provider paths verbatim. Claude tool inputs are absolute (/Users/x/proj/src/main.rs); Codex/Cursor paths are workspace-relative. Derived documents therefore mixed absolute and relative keys depending on harness, breaking the RFC contract and making cross-harness merge/diff/resume ambiguous.Fix
One canonical rule at the shared choke point, so all six providers are fixed at once:
toolpath-convo::derive_path): when the resolvedbase.uriisfile://<root>and a file path is absolute and under<root>(component-boundary match —/a/bdoes not swallow/a/bc/x), the change key is stored base-relative. Applies to bothfile.writekey branches andmeta.extra["files_changed"]. Paths outside the base, non-file bases, and theprovider://idconversation self-key pass through verbatim.extract_conversation): a relative key (no leading/, no://) resolves againstview.base.working_dir, soFileMutation.pathandfiles_changedcome back absolute for projectors. The IR convention onFileMutation.pathis now documented: absolute when the workspace root is known.Provider-crate test assertions (codex ×2, cursor, opencode) updated to the new canonical relative keys — each keeps its original invariant (raw/operation/tool_id assertions untouched).
Tests
TDD: helper unit tests red (E0425) before implementation, green after; new round-trip integration test; full workspace suite (1644 tests) + clippy green.
toolpath-convobumped 0.11.1 → 0.11.2 (0.11.1 was already taken by the step-id dedup fix on main).Notes
file:///(filesystem-root) base conservatively never relativizes — safe and round-trip-stable.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.