refactor: reshape three oversized files into focused modules - #93
Merged
Conversation
main.rs re-declared all 15 modules as private `mod`s while also importing `claude_code_sync::VerbosityLevel` from its own library. The tree therefore compiled twice -- once into the lib, once into the bin -- producing two incompatible type universes that happened to share names. Make the binary a consumer of the library: add `pub mod handlers` to lib.rs and let main.rs import from `claude_code_sync::*`. Imports are explicit rather than glob: the crate root and `handlers` both export `config`, so two globs would be an ambiguity error at every `config::` call site. This lets both `#[allow(unused_imports)]` in undo/mod.rs go. They were not cargo-culted -- in the lib those `pub use`s are real public API, but in the bin `mod undo` was private, so they were unreachable and warned. The attribute silenced the duplicate copy. Removing the duplicate removes the need for it. The `#[allow(dead_code)]` attributes are unrelated (they serve the non-test lib build) and stay. `cargo test` now reports 336 tests where it previously reported 490. No test was lost: 154 of those entries were the bin harness re-running the lib's own tests. The set of unique test paths is unchanged at 336, verified by diffing `cargo test -- --list` before and after. The `src/main.rs` unittest target now correctly reports 0 tests.
undo/mod.rs was 1397 lines, of which 1373 were a single `#[cfg(test)] mod tests`. The production code had already been split into cleanup/operations/ preview/restore/snapshot; the tests never followed, so every one of them sat in the parent testing its siblings through the public re-exports. Redistribute all 32 to the module each actually exercises, matching the convention preview.rs already followed. mod.rs is now 21 lines of module declarations and re-exports. Split snapshot.rs (542 lines) into snapshot.rs and differential.rs along a seam that was already there: full snapshots (struct, base64 serde, create, save/load) versus differential chains (create_differential*, find_latest_ snapshot, reconstruct_full_state*). The differential half builds on the full half and never the reverse. This mirrors restore.rs, which already keeps an `impl Snapshot` block in a sibling file. Without it, snapshot.rs plus its tests would have been ~790 lines. Add undo/test_support.rs with the fixtures the tests genuinely share -- most importantly a HistoryBuilder, which collapses the ~30 lines of snapshot + OperationHistory + OperationRecord setup that all ten operations tests previously spelled out longhand. Drop five of the six `#[allow(dead_code)]`. Their comment claimed "the bin compiles this module separately", which stopped being true in the previous commit: `create`, the three `create_differential*`, and `find_latest_snapshot` are all reachable public API in the lib and were only ever dead in the binary's private copy of the tree. The one that survives, `reconstruct_full_state`, is genuinely pub(crate) with no production caller. No test was lost: `cargo test -- --list` diffs to 32 renames, each with a matching leaf name, and the unique test-path count is unchanged at 336.
…config.rs
BEHAVIOR CHANGE: `config` and `config --wizard` now reject a non-positive or
non-finite max file size instead of silently accepting it.
handlers/config.rs was 1090 lines holding four unrelated commands, and the two
editing modes each carried their own copy of the same value logic: the
comma-separated pattern parser appeared four times (include/exclude x
interactive/wizard) and the megabyte parser twice. Neither copy validated
anything, and neither could be tested -- the parsing was welded directly to the
`inquire` prompt, so it only ran with a TTY attached.
Split by concern:
fields.rs pure value logic, no I/O -- and now unit-tested
prompts.rs the terminal glue the two modes share
interactive.rs MultiSelect, then edit the chosen settings
wizard.rs Confirm-gated walk through every setting
repo_select.rs the no-argument repo menu
export.rs --export, plus its six tests
`prompt_artifact_toggle_selection` lives in prompts.rs, not fields.rs: it runs
a MultiSelect, so it is not TTY-free and would have made that module's whole
premise false.
The two modes keep their distinct prompt flows. They are not accidentally
different: interactive treats empty input as "clear this setting" and says so,
while the wizard asks "Do you want to ...?" first and has no clear affordance.
Only the value logic is shared, not the prompt shape.
The file-size fix: both copies did `parse::<f64>()` then
`(mb * 1024.0 * 1024.0) as u64`. Rust's float-to-int cast saturates, so `-5`
became a 0-byte limit -- which makes FilterConfig::should_include reject every
file -- and `1e30` became u64::MAX. The `.context("Must be a positive number")`
already claimed a check that was never performed. parse_file_size_mb now
actually performs it, and eight tests pin the behaviour down.
Also give the export tests an RAII guard. They mutate the process cwd and an
env var, and previously restored both in a trailing statement that a failed
assertion would skip, leaving the harness pointed at a deleted temp dir.
Tests: 336 -> 347 (11 new in fields.rs). The 6 export tests are renamed, not
lost.
…safe
tests/test_onboarding.rs was 1088 lines covering five unrelated subjects.
Split it by what is actually under test:
test_onboarding.rs 12 tests init_from_onboarding, init_sync_repo,
cloning, InitConfig validation
test_config_state.rs 9 tests ConfigManager paths, FilterConfig, SyncState
test_multi_repo.rs 13 tests MultiRepoState v1->v2, active-repo switching
Add tests/common/mod.rs with a ConfigEnv guard that unsets
CLAUDE_CODE_SYNC_CONFIG_DIR on Drop, replacing 21 hand-written set_var/
remove_var pairs. The cleanup used to be a bare statement at the end of the
function body -- and these tests return Result<()> and use `?` throughout, so
*any* early return, not only a panic, skipped it and left the variable pointing
at a TempDir that was about to be deleted. Every later test in the same binary
then resolved its config against a path that no longer existed. Drop runs on
the early-return and unwind paths both.
Delete setup_test_config_env(). Despite the name it set up no environment --
it was TempDir::new() and nothing else, and all 28 callers went on to set the
variable by hand anyway.
Four tests ran with no override and no #[serial] at all, and two of them
(ensure_config_dir, config_directory_structure) mkdir'd in the developer's real
~/.config -- a comment in the old file admitted as much. They are now guarded
like the rest. Their assertions (contains("claude-code-sync"), ends_with(...))
hold identically under the override.
34 tests in, 34 tests out, same names.
`Snapshot::restore` validated a path only *after* creating it. The security check needs a canonical path, `Path::canonicalize` requires the path to exist, so the code created the parent directories and wrote an empty file first and checked second -- and then returned early on rejection, leaving both behind. A snapshot naming a path outside the home directory therefore got directories and an empty file placed there before being refused. The existing traversal test only passed because it aimed at /etc/passwd, which already exists, so the create-first branch never ran. Resolve the path without creating anything instead: reject `..` up front, walk up to the deepest ancestor that does exist, canonicalize *that* (which resolves symlinks in the prefix), and re-attach the remaining components literally. They don't exist, so they can't be symlinks, and they aren't `..` because we just checked. Only then create directories and write. Existence is probed with `symlink_metadata` rather than `exists`. `exists` follows symlinks and so reports false for a dangling one; treating a dangling symlink as merely absent would let us re-attach its name to a canonical in-base prefix, pass the check, and then have `fs::write` follow it out of the sandbox. Three tests: a rejected out-of-base path creates neither the file nor its parent directories; `..` in a non-existent tail is refused; and a dangling symlink inside the base is not followed out of it.
`FilterConfig::validate()` checked the LFS backend and the reserved sync subdirectory but never the file size limit, so `max_file_size_bytes = 0` was accepted -- and 0 makes `should_include` reject every non-empty file, meaning sync silently does nothing at all. Strict on write, forgiving on read: - `validate()` now rejects 0, so it can never be introduced through the CLI. - `load()` repairs a 0 to the default and warns on stderr, rather than erroring. The asymmetry is deliberate. Configs in the wild already contain a 0, because until the previous commit entering a negative size at the `config` prompt wrote exactly that (the `as u64` cast saturates). Hard-failing on load would be a trap: `handle_config_interactive` opens by calling `FilterConfig::load()`, so an error there would take down the one command that can fix the value and leave hand-editing TOML as the only way out.
The Architecture list linked to a `git` module that does not exist -- the module is `scm`, which abstracts over Git *and* Mercurial. `cargo doc` warned about the unresolved link but CI does not deny rustdoc warnings, so it stayed. Fixing it surfaced two more, both of which actively mislead: - The `///` block describing "Platform-agnostic configuration directory management ... XDG on Linux, Application Support on macOS" sat directly above `pub mod artifacts;`, so rustdoc rendered it as the documentation for `artifacts` -- while `config`, which it actually describes, had none at all. Reattached to `config`. `artifacts` documents itself in artifacts/mod.rs. - `scm`'s own doc claimed "a unified interface for Git". The whole point of the abstraction is that it isn't Git-only: src/scm/hg.rs exists and CI runs a Mercurial matrix. Also list `artifacts` and `handlers` in the Architecture section; `handlers` became public in this branch and was missing. Verified with `RUSTDOCFLAGS="-D rustdoc::broken_intra_doc_links" cargo doc`, which is stricter than the `doc` CI task -- the crate is now clean under it.
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.
Splits the three largest hand-written files in the repo. Each was large for a different reason, and two of them were large because of an actual defect.
src/undo/mod.rssrc/handlers/config.rstests/test_onboarding.rsFour commits, each independently reviewable. Commit 1 must land first — commit 3 depends on it.
The defects this surfaced
A
-5at the max-file-size prompt silently disabled all syncingBoth prompt modes did
parse::<f64>()then(mb * 1024.0 * 1024.0) as u64. Rust's float→int cast saturates, so-5becamemax_file_size_bytes = 0, which makesFilterConfig::should_includereject every file.NaNdid the same;1e30becameu64::MAX.The existing
.context("Invalid number. Must be a positive number.")already claimed a positivity check that was never performed. Neither of the two copies caught it, and neither could be tested — the parsing was welded directly to theinquireprompt, so it only ran with a TTY attached.This is a behaviour change, not a pure refactor, and it's typed
fix(config):so it reaches the changelog.The entire module tree compiled twice
src/main.rsre-declared all 15 modules as privatemods while also importingclaude_code_sync::VerbosityLevelfrom its own library. The tree compiled once into the lib and again into the bin, producing two incompatible type universes that happened to share names.This is also why
undo/mod.rscarried#[allow(unused_imports)]: in the lib thosepub uses are real public API, but in the binmod undowas private, so they were unreachable and warned. The attribute was load-bearing — and a symptom.main.rsis now a consumer of the library. Five of the six#[allow(dead_code)]inundo/turned out to be artifacts of the same thing and are gone; the compiler confirmed which one was real.Test env cleanup didn't survive
?21 tests set
CLAUDE_CODE_SYNC_CONFIG_DIRand removed it in a bare trailing statement. Those functions returnResult<()>and use?throughout, so any early return — not just a panic — skipped the cleanup and left the variable pointing at aTempDirabout to be deleted. Every later test in the same binary then resolved config against a path that no longer existed. Now an RAII guard whoseDropruns on the unwind path too.Separately, four tests ran with no override and no
#[serial], and two of themmkdir'd in the developer's real~/.config— a comment in the old file admitted it. Guarded now.Verification
mise run ciis green (fmt,clippy --all-targets -- -D warnings, build, tests).Test count drops 490 → 347, and that is the fix working. 154 of those entries were the bin harness re-running the lib's own tests. The invariant checked was the set of unique test paths, baselined before any change:
Unique paths went 336 → 347. Every original test still exists — the 32
undo::and 6config::changes are renames with matching leaf names — plus 11 new unit tests, the firstfields.rshas ever had.Reviewer notes
interactiveandwizardkeep separate prompt flows on purpose. They are not accidentally different: interactive treats empty input as "clear this setting" and says so; the wizard asks "Do you want to…?" first and has no clear affordance. Only the value logic is shared, not the prompt shape.snapshot.rswas split intosnapshot.rs+differential.rs— a fifth module beyond the original plan. With its tests it would have hit ~790 lines, worse than what this PR is fixing. The seam (full snapshots vs. differential chains) was already there, andrestore.rsalready used the same sibling-impl Snapshotidiom.prompt_artifact_toggle_selectionlives inprompts.rs, notfields.rs— it runs aMultiSelect, so it isn't TTY-free and would have falsified that module's whole premise.operations.rs(525),differential.rs(472),snapshot.rs(450),test_multi_repo.rs(404). The production code in each is comfortably under; colocated#[cfg(test)]blocks push the totals over. Flagging rather than contorting the structure.fix(config):type is lost and the file-size fix won't appear in the changelog. A merge commit preserves it.Two further fixes, added on review
Snapshot::restorecreated files outside the home directory before rejecting themThe traversal check needs a canonical path;
canonicalizerequires the path to exist. So the code created the parent directories and wrote an empty file, then validated — and on rejection returned early, leaving both behind. A snapshot naming a path outside home got directories and a file placed there before being refused.The existing traversal test only passed because it aimed at
/etc/passwd, which already exists — so the create-first branch never ran.Paths are now resolved without touching the filesystem: reject
..up front, walk to the deepest ancestor that does exist, canonicalize that (resolving symlinks in the prefix), re-attach the remaining components literally. They don't exist so they can't be symlinks, and they aren't..because we just checked.Existence is probed with
symlink_metadata, notexists—existsfollows symlinks and reportsfalsefor a dangling one, so treating it as merely absent would let us re-attach its name to a canonical in-base prefix, pass the check, and havefs::writefollow it straight out of the sandbox. There's a test for exactly that.FilterConfig::validate()never checked the file size limitmax_file_size_bytes = 0was accepted, and 0 makesshould_includereject every non-empty file — sync silently does nothing.Strict on write, forgiving on read.
validate()now rejects 0 so it can't be introduced via the CLI;load()repairs a 0 to the default and warns on stderr rather than erroring.That asymmetry is deliberate, and it's the interesting part: configs in the wild already contain a 0, because the
-5bug above wrote it there. Hard-failing on load would be a trap —handle_config_interactiveopens by callingFilterConfig::load(), so erroring would take down the one command that can fix the value, leaving hand-editing TOML as the only way out.7 new tests. Unique test paths: 336 → 354.
Crate docs
The Architecture list linked to a
gitmodule that doesn't exist (it'sscm, which abstracts over Git and Mercurial). Fixing it surfaced two more, both actively misleading:pub mod artifacts;, so rustdoc rendered it as the documentation forartifacts— whileconfig, which it actually describes, had none at all.scm's own doc claimed "a unified interface for Git". The point of the abstraction is that it isn't Git-only —src/scm/hg.rsexists and CI runs a Mercurial matrix.Verified with
RUSTDOCFLAGS="-D rustdoc::broken_intra_doc_links", which is stricter than thedocCI task (that one doesn't deny rustdoc warnings, which is why the link survived). The crate is clean under it.Everything I flagged as out-of-scope has now been fixed. Nothing outstanding.