fix: correctness bug fixes from the three-axis audit#63
Merged
Conversation
Case-folded deploy-name collision detection via confine.rs fold_path; link<->copy mode transitions classify Modified/Foreign conflicts; merged_with drops stale url digests on url change; prune keeps records for cross-binding overlaps; parse_full_url handles nested groups and trailing slashes; bind --local checks the merged config; downloads follow redirects manually with a scheme allowlist and RFC 3986 joins. Tasks: BUG-CASE-001, BUG-MODE-002, BUG-DIGEST-003, BUG-PRUNE-004, BUG-ADDURL-006, BUG-BIND-008, BUG-HTTP-009 Batch: 1/3
Interpret target bytes relative to the symlink's deployed parent and normalize lexically; reject absolute targets (unix, drive-letter, UNC, root-relative backslash) and any '..' step escaping the artifact root at any prefix, splitting on both separators. In-root '..' targets still materialize; materialize_symlink stays dumb. Tasks: BUG-SYMLINK-005 Batch: 2/3
is_local_path recognizes a single ASCII letter followed by ':' as a path on all platforms, so add C:/Users/foo/project no longer falls through to colon-alias parsing fabricating host="C". Tasks: BUG-WINPATH-007 Batch: 3/3
There was a problem hiding this comment.
Pull request overview
This PR implements a set of audited correctness fixes across sync/export, config merging, URL parsing, and HTTP download handling, with extensive regression tests to lock in behavior.
Changes:
- Harden HTTP downloads by disabling auto-redirects and manually enforcing a redirect scheme allowlist with RFC 3986 relative-Location resolution.
- Fix sync correctness around prune overlap behavior (retain records when overlap prevents deletion) and mode-transition conflict detection (don’t silently bypass Modified/Foreign).
- Strengthen export/source safety and parsing correctness (case-folded deployed-name dedup, symlink escape validation, Windows drive-letter local-path classification, URL/digest merge semantics), with new targeted tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/http_redirect_scheme.rs | Adds regression tests for relative redirect resolution and disallowed-scheme refusal. |
| tests/bind_local_merged.rs | Adds CLI integration tests for bind --local merged target lookup and error remedy text. |
| src/sync/tests.rs | Adds regression tests for prune overlap invariants and mode-transition behavior. |
| src/sync/target.rs | Fixes conflict classification so mode transitions don’t swallow Modified/Foreign conflicts; adds unit tests. |
| src/sync/prune.rs | Refines prune overlap logic to keep registry records when overlap with a different binding prevents deletion. |
| src/sync/mod.rs | Exposes sync::confine internally for shared folding logic. |
| src/sync/confine.rs | Adds fold_path helper for consistent path folding. |
| src/source.rs | Implements case-folded deployed-name collision detection; adds symlink escape validation; improves local-path classification; adds tests. |
| src/http.rs | Implements manual redirect handling with scheme allowlist, hop limit, and relative redirect resolution helpers. |
| src/config/source.rs | Fixes url/digest merge semantics to drop stale digest only when url changes without a restated digest; adds tests. |
| src/cli/tests.rs | Adds tests for URL parse edge cases (GitLab subgroup naming, trailing slash normalization, tree URL handling). |
| src/cli/bind.rs | Uses merged config for bind validation; improves missing-target hint to include --local when applicable. |
| src/cli/add.rs | Fixes scheme URL parsing to derive source name from last segment and normalize .git handling across nested paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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
Fixes all nine verified correctness defects from the 2026-07-02 three-axis audit (bugs axis). Every fix landed TDD: a failing regression test reproducing the defect first, then the minimal fix. Scope records:
scopes/done/correctness-bug-fixes/.High severity
BUG-CASE-001) —register_deployed_namenow keys its dedup on a case-folded path via a newpub(crate) confine::fold_path(NFC + full Unicode lowercase, folding defined only in confine.rs);README.md+readme.mdto one target is a hard collision error naming both paths on all platforms.BUG-MODE-002) — themode_transitionarm insideconflict_kind_foris narrowed: clean-like states stay silent, Modified/Foreign fall through to the standard conflict path (--forceoverrides). Full 2×2 matrix regression-tested; local edits survive on disk.Medium
BUG-DIGEST-003) —merged_withnulls the digest only when the override changesurlwithout restatingdigest; restating the same url keeps a valid pin (pinned by test).BUG-SYMLINK-005) — rejects absolute targets (unix, drive-letter, UNC, root-relative backslash) and any..step escaping the artifact root at any prefix, splitting on both separators; in-root..(dir/link -> ../sibling) still materializes.BUG-PRUNE-004) — records are kept when the orphan's path overlaps a live destination of a different binding (file stays tracked); same-binding granularity flips still drop superseded records; already-absent paths still drop theirs.BUG-ADDURL-006,BUG-WINPATH-007) — GitLab subgroup URLs name the source from the last segment; trailing slashes normalize before.git;C:/Users/...classifies as a filesystem path on all platforms instead of fabricating ahost="C"alias.Low
BUG-BIND-008) — target existence checks the merged config; the missing-target remedy includes--localwhen writing locally.BUG-HTTP-009) — downloads disable auto-redirect and follow hops manually with RFC 3986 relative-Location resolution; https always allowed, http only when the original URL was http; refusals name the offending scheme.Review
Each batch cleared a four-harness pipeline (claude-opus, claude-sonnet, codex, gemini): test-quality gate before implementation, per-batch code review, fix rounds with targeted re-verification. Final review: ready_to_merge; every acceptance criterion is pinned by a named regression test (see
scopes/done/correctness-bug-fixes/review.yaml). Three medium nits deferred with rationale (recorded as D1–D3 in review.yaml).Test plan
mise run check— clippy-D warnings, fmt, full suite (1379 tests) greenb04c283(pre-fix) and passes after