From b25395a7a8a49aec34cf988babe3ac83ba51d6ec Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 15:28:33 +0200 Subject: [PATCH 01/12] core: refuse a name this system cannot write instead of renaming it Extraction used to negotiate. A character Windows refuses became a question for the user, a trailing dot was dropped without asking, and a reserved device name gained a `_`; the entries were then written under whatever name that arrived at. The tree on disk was this crate's invention rather than the archive's, and nothing downstream could tell which files those were. The policy is now the narrow one: all of the archive under its own names, or none of it. `refuse_unwritable_names` judges the whole listing against `NameRules::can_write` before the first byte, and the first component the host cannot hold exactly as spelled fails the extraction with `NameError::Unwritable`, naming the entry, the component and why. All four faults answer the same way, because from the caller's side they are one failure: the name they asked for is not the name they would get. Consequences worth stating rather than discovering: - `extract` and `extract_with` now refuse archives they used to extract. On Windows that includes ordinary Linux tarballs holding `aux.log` or `notes.txt.`, which is the whole point and also the cost. - `ExtractOptions::with_replacements` is inert. Kept, with `Substitutions`, so the two front-end dialogs still compile while they are taken out. - `check_replacements` no longer runs from `extract_with`: validating an input that is ignored is worse than not taking one. - `refuse_overwriting_the_archive` drops its plan argument. It followed the rewritten name because a substitution could land an entry on the archive that the archive's own name did not match; nothing rewrites a name any more. - `NamePlan` is now always the identity and `extract_with` calls the plain backends. The type, the `extract_*_planned` variants and tar's second write path are dead and deliberately left in place: that path carries the containment guard and should not move in a commit about naming. The tests are untouched on purpose, so the ones that pinned the old behaviour fail. They are the record of what was decided before and rewriting them in the same commit would hide the size of the change. --- apps/core/src/compression.rs | 109 ++++++++++++-------- apps/core/src/compression/names.rs | 158 ++++++++++++++++++++--------- 2 files changed, 175 insertions(+), 92 deletions(-) diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index 4137783..9ef50fc 100644 --- a/apps/core/src/compression.rs +++ b/apps/core/src/compression.rs @@ -16,7 +16,7 @@ pub use self::tar::{compress_tar, compress_tar_dir, extract_tar}; pub use self::verify::{verify_archive, Verify}; pub use self::zip::{compress_zip, compress_zip_dir, extract_zip}; -pub(crate) use self::names::{plan_names, NamePlan}; +pub(crate) use self::names::{refuse_unwritable_names, NamePlan}; pub(crate) use self::sevenz::{list_7z_entries, read_7z_entries}; pub(crate) use self::tar::{list_tar_entries, read_tar_entries}; pub(crate) use self::walk::walk_tree; @@ -371,9 +371,9 @@ pub fn compress_dir( /// /// A separate type rather than more arguments on [`extract`], and /// [`extract_with`] rather than a replacement for it: every existing caller -/// (the CLI, the server, the desktop, this crate's own tests) has no -/// substitutions to offer and should not have to say so, and the next knob -/// extraction grows should not add a third function. +/// (the CLI, the server, the desktop, this crate's own tests) has nothing to +/// say here and should not have to say so, and the next knob extraction grows +/// should not add a third function. #[derive(Debug, Clone, Default)] #[non_exhaustive] pub struct ExtractOptions { @@ -394,7 +394,13 @@ impl ExtractOptions { self } - /// The caller's answers for the characters the host cannot write. + /// **Inert.** Kept so the callers that pass answers still compile while + /// their own naming dialogs are being taken out; nothing reads it. + /// + /// Extraction no longer substitutes anything: a character this filesystem + /// cannot write fails the extraction rather than being replaced, so there + /// is no answer for a caller to supply. Removing this, [`Substitutions`] + /// and the two front-end dialogs that fill it is follow-up work. pub fn with_replacements(mut self, replacements: Substitutions) -> Self { self.replacements = replacements; self @@ -404,6 +410,8 @@ impl ExtractOptions { self.rules } + /// The answers this was handed. Read by nobody; see + /// [`Self::with_replacements`]. pub fn replacements(&self) -> &Substitutions { &self.replacements } @@ -471,11 +479,14 @@ fn list_entries(archive: &Path, algorithm: Algorithm) -> Result, Com /// What an archive holds that this machine cannot write as ordinary files. /// /// Reads the listing and nothing else: no entry is decompressed and nothing is -/// created, so a front end can ask this before it asks the user anything. Feed -/// the answers back through [`ExtractOptions::with_replacements`]. +/// created, so a front end can ask this before it makes anyone wait. /// -/// An empty report ([`NameReport::is_empty`]) means extraction has no naming -/// question to ask, which on Unix is nearly always the case. +/// **This is now a prediction, not a questionnaire.** A non-empty report +/// ([`NameReport::is_empty`]) means [`extract`] will refuse this archive on +/// this machine, and the report says which entries and why so a front end can +/// explain it. There is nothing to answer: the entries it names are the reason +/// the extraction will not happen, not a form to fill in. On Unix the report is +/// nearly always empty. pub fn unwritable_names(archive: &Path) -> Result { unwritable_names_with(archive, NameRules::host()) } @@ -493,44 +504,55 @@ pub fn unwritable_names_with( /// Extract an archive into `output_dir`. /// -/// Returns the list of extracted file paths (relative to `output_dir`), which -/// are the names **as written**: an entry the host had to be given a different -/// name for is reported under the name that is on disk, never under the -/// archive's, or a front end would list files nobody can find. +/// Returns the list of extracted file paths, relative to `output_dir` and +/// spelled exactly as the archive spells them. +/// +/// **All of the archive, under its own names, or none of it.** An entry this +/// filesystem cannot hold as spelled fails the whole extraction +/// ([`CompressionError::Name`]) before anything is written, rather than being +/// adjusted to fit: a renamed file is not the file the archive named, and +/// nothing downstream can tell the two apart afterwards. Ask +/// [`unwritable_names`] first if the answer is worth showing someone before +/// they wait for it. /// /// The algorithm is detected from the archive file extension. pub fn extract(archive: &Path, output_dir: &Path) -> Result, CompressionError> { extract_with(archive, output_dir, &ExtractOptions::default()) } -/// [`extract`], with the caller's answers for the entry names this machine -/// cannot write. +/// [`extract`], against a chosen set of [`NameRules`] rather than the host's. /// -/// Naming is settled over the whole listing before the first byte is written, -/// and a listing that cannot be read stops it there (issue #89), so the answers -/// nothing can recover from (a character with no replacement, two entries that -/// would land on one name, an archive too damaged to read) leave the output -/// directory as they found it. +/// Same policy as [`extract`], which is the point of it: this exists so a test +/// on one machine can ask what another machine would do, not so a caller can +/// soften the answer. +/// +/// Every name is judged against the whole listing before the first byte is +/// written, and a listing that cannot be read stops it there (issue #89), so +/// an archive this system cannot hold leaves the output directory exactly as +/// it found it. pub fn extract_with( archive: &Path, output_dir: &Path, options: &ExtractOptions, ) -> Result, CompressionError> { let algorithm = algorithm_of(archive)?; - // Before the archive is even opened: an answer that is itself unwritable is - // wrong whether or not any entry needs it. - options.rules().check_replacements(options.replacements())?; - let (names, plan) = plan_for(archive, algorithm, options)?; - refuse_overwriting_the_archive(archive, output_dir, &names, &plan)?; - + let names = listing_for(archive, algorithm)?; + // Judged against the whole listing before a byte is written, so an archive + // this host cannot hold leaves the output directory as it found it. + refuse_unwritable_names(&names, options.rules())?; + refuse_overwriting_the_archive(archive, output_dir, &names)?; + + // The plain backends, because there is no longer any plan to hand them: + // every entry is written under the name the archive spells or the run + // stopped above. match algorithm { - Algorithm::SevenZ => self::sevenz::extract_7z_planned(archive, output_dir, &plan), - Algorithm::Tar => self::tar::extract_tar_planned(archive, output_dir, &plan), - Algorithm::Zip => self::zip::extract_zip_planned(archive, output_dir, &plan), + Algorithm::SevenZ => self::sevenz::extract_7z(archive, output_dir), + Algorithm::Tar => self::tar::extract_tar(archive, output_dir), + Algorithm::Zip => self::zip::extract_zip(archive, output_dir), } } -/// Work out what every entry will be called, from the listing. +/// The archive's listing, read before anything is written. /// /// **A listing that cannot be read stops the extraction here**, before anything /// is written. @@ -540,8 +562,8 @@ pub fn extract_with( /// layer would only replace that message with a worse one. The reasoning was /// right about the message and wrong about the timing (issue #89): the /// extractor fails **while streaming**, so by the time it notices it has -/// already written every entry before the fault, and written them with no plan -/// at all, which means no rewriting, no refusal and no collision check. An +/// already written every entry before the fault, and written them with nothing +/// judged at all, so no name was refused however badly it fitted the host. An /// archive holding `notes.txt:hidden` was refused outright when its listing was /// intact and written as an invisible NTFS stream when it was not: the harm of /// issue #63, performed without the user ever being asked. One bad 512 byte @@ -558,27 +580,26 @@ pub fn extract_with( /// and so never come through here; `recovering_from_a_damaged_archive_is_still_ /// possible_through_the_backend` pins that. /// -/// It costs one listing per extraction, paid even when nothing needs renaming, +/// It costs one listing per extraction, paid even when every name is ordinary, /// because the only way to know that is to read the names. For zip and 7z that /// is a header read; for tar it is a second walk over the headers, seeking past /// each member rather than reading it. -fn plan_for( - archive: &Path, - algorithm: Algorithm, - options: &ExtractOptions, -) -> Result<(Vec, NamePlan), CompressionError> { - let names = list_entries(archive, algorithm).map_err(unreadable_archive)?; - let plan = plan_names(&names, options.rules(), options.replacements())?; - Ok((names, plan)) +fn listing_for(archive: &Path, algorithm: Algorithm) -> Result, CompressionError> { + list_entries(archive, algorithm).map_err(unreadable_archive) } /// Refuse an extraction that would write one of the archive's own entries over /// the archive. /// /// Free to run, in the sense that matters: the listing it needs has already -/// been read and paid for by the planning pass, so this adds one identity check +/// been read and paid for by [`listing_for`], so this adds one identity check /// per entry and no extra pass over the file. /// +/// It compares the name the archive spells, because that is now the only name +/// an entry can be written under. It used to have to follow a rewritten name +/// as well, since a substitution could land an entry on the archive that the +/// archive's own name did not match; nothing rewrites a name any more. +/// /// By **file identity**, not by path. A hardlink is a second name for one file /// and never resolves to the same string, which is exactly how `--force` used /// to be able to overwrite its own source on the compression side before that @@ -587,7 +608,6 @@ fn refuse_overwriting_the_archive( archive: &Path, output_dir: &Path, names: &[String], - plan: &NamePlan, ) -> Result<(), CompressionError> { for name in names { let Some(natural) = sanitize_entry_path(name) else { @@ -595,8 +615,7 @@ fn refuse_overwriting_the_archive( // that is the message worth keeping. continue; }; - let rel = plan.written_as(name).map_or(natural, Path::to_path_buf); - if crate::paths::same_file(&output_dir.join(&rel), archive) { + if crate::paths::same_file(&output_dir.join(&natural), archive) { return Err(CompressionError::WouldOverwriteArchive { archive: archive.to_path_buf(), entry: name.clone(), diff --git a/apps/core/src/compression/names.rs b/apps/core/src/compression/names.rs index 551e225..80dbdc0 100644 --- a/apps/core/src/compression/names.rs +++ b/apps/core/src/compression/names.rs @@ -1,5 +1,4 @@ -//! Which entry names the host can write, and what to do with the ones it -//! cannot. +//! Which entry names the host can write, and which ones stop an extraction. //! //! Extraction takes names from an archive, which is to say from another //! machine. Containment ([`sanitize_entry_path`](super::sanitize_entry_path)) @@ -10,6 +9,18 @@ //! directory, and `notes.txt:hidden` is accepted as the `hidden` stream of //! `notes.txt` rather than as a file, with no error at all. //! +//! **The answer to all four is the same: refuse the archive.** This module used +//! to negotiate instead. It asked the user to supply a character in place of +//! the `?`, dropped the trailing dot on its own and suffixed the device name, +//! then extracted under the names it had arrived at. The output was then a tree +//! whose names were this crate's invention rather than the archive's, with no +//! way for anything downstream to tell which files those were. An extraction +//! that cannot reproduce what the archive says is one that should not happen, +//! so [`refuse_unwritable_names`] stops it before the first byte and says which +//! entry and why. The machinery for the old answer is still here and unused, +//! because the front ends still hand it answers; see +//! [`super::ExtractOptions::with_replacements`]. +//! //! The rules are **data** ([`NameRules`]) rather than `#[cfg]`, and that is the //! point: [`NameRules::windows`] can be asked for from any machine, so every //! rule here is exercised on a Mac and on Linux CI as well as on the Windows @@ -633,10 +644,59 @@ pub enum NameError { result: String, }, + /// The host cannot hold this name exactly as the archive spells it. + /// + /// The whole extraction stops here. Extraction writes what the archive + /// says or it writes nothing: a name this filesystem would refuse, or + /// silently store under a different name, is reported rather than adjusted, + /// because the adjusted file is not the file the archive named and nothing + /// downstream can tell the difference afterwards. + #[error( + "the archive entry {entry:?} cannot be written on this system: {component:?} {}. \ + Nothing was extracted; extract it on a system that can hold the name.", + describe(.problems) + )] + Unwritable { + entry: String, + component: String, + problems: Vec, + }, + #[error("{key:?} is not a single character, so there is nothing to replace")] NotOneCharacter { key: String }, } +/// Why one component cannot be written, as a phrase that follows its name. +/// +/// Rendered here rather than on [`NameProblem`] itself: this is the sentence a +/// refusal reads in, and the same data has to render differently in the desktop +/// dialog, which shows it as structure rather than as prose. +fn describe(problems: &[NameProblem]) -> String { + problems + .iter() + .map(|problem| match problem { + NameProblem::Character { + character, + fault: CharacterFault::Rejected, + } => format!("contains {character:?}, which this system refuses in a file name"), + NameProblem::Character { + character, + fault: CharacterFault::Reinterpreted, + } => format!( + "contains {character:?}, which this system reads as something other than part \ + of the name" + ), + NameProblem::TrailingCharacters { removed } => { + format!("ends in {removed:?}, which this system does not preserve") + } + NameProblem::ReservedDevice { device } => { + format!("is the reserved device name {device:?}") + } + }) + .collect::>() + .join("; and ") +} + impl NameError { /// Re-point an error raised over one component at the whole entry, which is /// the only name the person reading it has ever seen. @@ -661,10 +721,14 @@ impl NameError { /// The name every entry will be written under, for the entries whose name /// changes. /// -/// Built from the whole listing before anything is written, because two of the -/// three answers it can give ("no replacement for this" and "these two entries -/// collide") must stop the extraction while the output directory is still -/// empty, and a collision cannot be seen one entry at a time. +/// **Always the identity now, and kept only so the write paths do not have to +/// change in the same commit as the policy.** Extraction no longer renames an +/// entry ([`refuse_unwritable_names`]), so nothing constructs a plan that +/// says anything: every backend receives [`Self::identity`] and every +/// [`Self::written_as`] answers `None`. Removing this type, the +/// `extract_*_planned` variants that take it and tar's second write path is +/// follow-up work, deliberately separate because that path carries the +/// containment guard and should not be moved by a commit about naming. #[derive(Debug, Clone, Default)] pub(crate) struct NamePlan { /// Only the entries whose name changed. An entry that is absent is written @@ -685,55 +749,55 @@ impl NamePlan { } } -/// Work out what each name in `names` becomes, and refuse the two situations -/// nothing downstream could recover from. -pub(crate) fn plan_names>( +/// Refuse a listing holding a name this filesystem cannot write exactly as the +/// archive spells it. +/// +/// **Nothing is renamed.** This used to be a planning pass: it applied the +/// caller's replacements, dropped a trailing dot or space and suffixed a +/// reserved device, so an archive Windows could not hold was extracted anyway +/// under names it could. That traded one problem for a worse one. The file on +/// disk was then not the file the archive named, nothing downstream could tell +/// the two apart, and the listing handed back was the only record that a +/// substitution had happened at all. An extraction either reproduces what the +/// archive says or it does not happen. +/// +/// So the question is now the narrow one [`NameRules::can_write`] answers, and +/// the answer is yes or no rather than a rewrite: can this host hold this +/// component, spelled this way. A component it would refuse (`what?.txt` on +/// Windows), silently store elsewhere (`notes.txt:hidden`, an NTFS stream), +/// silently rename (`notes.txt.`, whose trailing dot is dropped) or resolve to +/// a device (`CON`) all fail the same way, because from the caller's side they +/// are the same failure: the name they asked for is not the name they would +/// get. +/// +/// The whole listing is judged before a byte is written, so a refusal leaves +/// the output directory exactly as it found it. +/// +/// The first offending component stops it. Reporting every one at once is a +/// front end's job, and both have the listing to do it from +/// ([`NameReport::of`]), which is also what lets them ask before extracting +/// rather than after failing. +pub(crate) fn refuse_unwritable_names>( names: &[S], rules: NameRules, - replacements: &Substitutions, -) -> Result { - let mut rewritten = HashMap::new(); - // planned name -> the first entry that claimed it, and whether that entry's - // name had to change to claim it. - let mut claimed: HashMap = HashMap::new(); - +) -> Result<(), NameError> { for name in names { let name = name.as_ref(); - let planned = rules.rewrite_entry(name, replacements)?; - if planned.as_os_str().is_empty() { - // Nothing normal in it (`.`, a bare root). Containment decides what - // becomes of those, per format, and it is not this pass's business. - continue; - } - let changed = planned != natural_path(name); - - if let Some((first, first_changed)) = claimed.get(&planned) { - // Two entries spelled the same way is an archive that was already - // like that, and extraction has always let the second win. Refusing - // it here would start rejecting archives that have nothing to do - // with this feature; a collision is only ours when a rewrite caused - // it. - if *first != name && (changed || *first_changed) { - return Err(NameError::Collision { - first: (*first).to_string(), - second: name.to_string(), - name: planned.to_string_lossy().into_owned(), + // Component by component, and split on `/` rather than by the host's + // rules: an archive entry name means the same thing on every machine, + // which is the property `entry_components` exists to preserve. + for component in entry_components(name) { + let problems = rules.problems(component); + if !problems.is_empty() { + return Err(NameError::Unwritable { + entry: name.to_string(), + component: component.to_string(), + problems, }); } } - claimed.insert(planned.clone(), (name, changed)); - if changed { - rewritten.insert(name.to_string(), planned); - } } - - Ok(NamePlan { rewritten }) -} - -/// The relative path an extractor derives from an entry name with no rules -/// applied: its `Normal` components and nothing else. -fn natural_path(name: &str) -> PathBuf { - entry_components(name).collect() + Ok(()) } /// Split an archive entry name into its components, the same way on every host. From db36a9d69d8589595fcd4946f3676e73948f96f3 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 15:28:47 +0200 Subject: [PATCH 02/12] cli: refuse every naming problem, not only the ones with an answer `run_extract` refused an archive whose listing needed a character replaced and waved through a trailing dot or a reserved device, because those two had one correct answer that needed nobody. Core applied them silently and extraction continued. Core refuses all three now, so the split has nothing behind it: an archive with `notes.txt.` in it will not extract on Windows whatever this check says. Widened to the whole report. Refusing here as well is still not redundant. Core stops at the first offending component; this has the same listing in hand and names every entry at fault at once, which is the message someone can act on. `adjustments` is left in place and is now provably empty by the time it is reached, since the report above it was empty. It goes when `Outcome::Extracted` loses the field. Tests untouched, so the cases that pinned a trailing dot extracting fail. --- apps/cli/src/lib.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/apps/cli/src/lib.rs b/apps/cli/src/lib.rs index 4df923d..945b564 100644 --- a/apps/cli/src/lib.rs +++ b/apps/cli/src/lib.rs @@ -437,17 +437,18 @@ fn run_extract(archive: PathBuf, output_dir: PathBuf) -> Result Date: Fri, 28 Aug 2026 15:28:47 +0200 Subject: [PATCH 03/12] desktop: say that the replacement exchange is now inert `NameInspection`'s doc claimed the extraction validates the answers again before opening the archive. It did, and no longer does: core neither validates nor applies a substitution, so an answer cannot reach a file name at all. The dialog still runs and still collects answers that go nowhere. Removing it, along with the `unwritable_names` command it is built from, is the follow-up. --- apps/desktop/src-tauri/src/names.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apps/desktop/src-tauri/src/names.rs b/apps/desktop/src-tauri/src/names.rs index 59f7ab4..d66d887 100644 --- a/apps/desktop/src-tauri/src/names.rs +++ b/apps/desktop/src-tauri/src/names.rs @@ -47,9 +47,11 @@ pub struct NameInspection { /// /// Sent as data rather than reimplemented in JavaScript. The dialog only /// picks the wording; whether a character is acceptable is answered here by - /// the same ruleset the extraction will use, and the extraction checks - /// again regardless (`extract_with` validates the answers before it opens - /// the archive). + /// the same ruleset the extraction will use. + /// + /// Nothing downstream re-checks it any more, and nothing needs to: core no + /// longer substitutes anything, so an answer cannot reach a file name. This + /// whole exchange is inert until the dialog is taken out. pub rejected_in_replacement: String, } From 0a95e2e2c569fb8558501981595c7f4338bee173 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 17:06:24 +0200 Subject: [PATCH 04/12] core: hold the tests to the refusal, not to the rewriting The five that failed asserted the answers being applied. Rewritten to assert what happens instead, keeping each one pointed at the guarantee it was written for rather than deleting it and losing the record. - `every_fault_stops_the_whole_archive_the_same_way` replaces `the_answers_are_written_and_the_listing_names_what_is_on_disk`, and is now a table over all four faults, since the point of the policy is that they have one ending. `summary.txt` rides along in each archive: it is writable everywhere and must still not reach disk. - `a_colon_entry_is_refused_and_its_neighbour_is_never_written` keeps issue #63 and moves its sharpest assertion. `notes.txt` used to have to survive intact; now it must not exist at all. - `two_entries_that_used_to_collide_are_refused_for_their_own_names` records that a collision cannot be manufactured any more, and asserts no planned name appears in the message, which is what would show something still working one out. - `a_trailing_dot_is_refused_rather_than_folded_onto_the_name_beside_it` is the renamed collision-with-an-untouched-entry case. It was passing under a name that had stopped being true. - `an_answer_no_longer_rescues_an_entry_the_host_cannot_write` is the old answer-validation test turned around. Its subject is gone, so it now guards the policy most likely to be quietly undone: the most reasonable answer to the most ordinary question must still extract nothing. `an_entry_with_no_answer_stops_before_anything_is_written` is dropped, absorbed whole by the first of these. In security.rs, two tests depended on the renamed write path that no longer exists. Both keep their real assertion (the archive intact, the file outside the output directory absent) and say in the body that the reason changed: refusal now comes before the write rather than the guard catching it during. `a_replacement_cannot_carry_an_entry_out_of_the_output_directory` still offers the hostile answers and still must see nothing move. --- apps/core/tests/names.rs | 239 ++++++++++++++++++------------------ apps/core/tests/security.rs | 61 ++++++--- 2 files changed, 165 insertions(+), 135 deletions(-) diff --git a/apps/core/tests/names.rs b/apps/core/tests/names.rs index 73619f4..304adbe 100644 --- a/apps/core/tests/names.rs +++ b/apps/core/tests/names.rs @@ -124,6 +124,19 @@ fn as_windows(replacements: Substitutions) -> ExtractOptions { .with_replacements(replacements) } +/// The message of the refusal an unwritable entry must produce. +/// +/// Panics naming what happened instead, because the two ways this goes wrong +/// need telling apart: extracting anything at all is the policy being broken, +/// while a different error is usually the fixture failing to build the name. +fn refusal(result: Result, CompressionError>, context: &str) -> String { + match result { + Err(CompressionError::Name(problem @ NameError::Unwritable { .. })) => problem.to_string(), + Err(other) => panic!("{context}: expected a naming refusal, got {other}"), + Ok(files) => panic!("{context}: expected a refusal, extracted {files:?}"), + } +} + // ------------------------------------------------------------- the ruleset -- #[test] @@ -564,55 +577,62 @@ fn inspecting_this_machine_s_own_archives_asks_nothing() { // -------------------------------------------- extracting with the answers --- #[test] -fn the_answers_are_written_and_the_listing_names_what_is_on_disk() { - // Issue #64 end to end, for all three formats. The listing is the half that - // matters most: returning the archive's names would have a front end show - // `what?.txt` next to a file called `what_.txt`. +fn every_fault_stops_the_whole_archive_the_same_way() { + // Issue #64 end to end, for all three formats, under the policy that + // replaced the answers. The four faults used to have four different + // endings: a question put to the user, a trailing run truncated in + // silence, a `_` appended to a device name. They have one ending now, and + // that is the whole of what this pins. + // + // `summary.txt` rides along in every archive and is the assertion that + // matters most. It is perfectly writable on any host, and it must still not + // be on disk: the refusal is judged over the listing before a byte is + // written, so a good entry beside a bad one goes nowhere either. for format in FORMATS { - let dir = tempfile::TempDir::new().unwrap(); - let archive = archive_with( - dir.path(), - format, - &[ - ("summary.txt", b"fine"), - ("what?.txt", b"question"), - ("notes.txt.", b"trailing"), - ("CON.txt", b"device"), - ], - ); - let out = dir.path().join("out"); - - let written = extract_with( - &archive, - &out, - &as_windows(Substitutions::new().with('?', "_")), - ) - .unwrap(); - - let expected = vec![ - "CON_.txt".to_string(), - "notes.txt".to_string(), - "summary.txt".to_string(), - "what_.txt".to_string(), - ]; - assert_eq!(sorted(written), expected, "{format}: the returned listing"); - assert_eq!(files_under(&out), expected, "{format}: what is on disk"); - assert_eq!( - fs::read(out.join("what_.txt")).unwrap(), - b"question", - "{format}: the renamed entry kept its content" - ); + for (bad, fault) in [ + ("what?.txt", "a character the host refuses"), + ("notes.txt:hidden", "a character the host reinterprets"), + ("notes.txt.", "a trailing dot"), + ("CON.txt", "a reserved device"), + ] { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[("summary.txt", b"fine"), (bad, b"trouble")], + ); + let out = dir.path().join("out"); + let context = format!("{format}, {fault}"); + + let message = refusal( + extract_with(&archive, &out, &as_windows(Substitutions::new())), + &context, + ); + + assert!(message.contains(bad), "{context}: {message}"); + assert!( + files_under(&out).is_empty(), + "{context}: {:?} was written before the refusal", + files_under(&out) + ); + } } } #[test] -fn a_colon_entry_becomes_a_file_of_its_own_and_leaves_its_neighbour_alone() { +fn a_colon_entry_is_refused_and_its_neighbour_is_never_written() { // Issue #63. On Windows the unfixed path writes these bytes into the // `hidden` stream of `notes.txt`, which changes nothing about `notes.txt` // that `dir` can see and leaves the listing naming a file that exists - // nowhere. Here the answer turns it into a file, and the assertion that - // `notes.txt` still holds its own bytes is what would fail if a future - // "simplification" let the colon through. + // nowhere. + // + // The answer used to be a substitution that turned it into a file of its + // own. That is gone: `notes.txt-hidden` is not the file the archive named + // either, and inventing it hid the problem rather than reporting it. + // + // `notes.txt` is what would catch a future "simplification" letting the + // colon through. It is the innocent half of the archive, and it must not + // exist: on a host that refuses the other entry, nothing is written. for format in FORMATS { let dir = tempfile::TempDir::new().unwrap(); let archive = archive_with( @@ -625,59 +645,34 @@ fn a_colon_entry_becomes_a_file_of_its_own_and_leaves_its_neighbour_alone() { ); let out = dir.path().join("out"); - let written = extract_with( - &archive, - &out, - &as_windows(Substitutions::new().with(':', "-")), - ) - .unwrap(); - - assert_eq!( - sorted(written), - vec!["notes.txt".to_string(), "notes.txt-hidden".to_string()], - "{format}" - ); - assert_eq!(fs::read(out.join("notes.txt")).unwrap(), b"the real file"); - assert_eq!( - fs::read(out.join("notes.txt-hidden")).unwrap(), - b"the payload" - ); - } -} - -#[test] -fn an_entry_with_no_answer_stops_before_anything_is_written() { - // The pre-pass earning its keep: judged one entry at a time, `summary.txt` - // would already be on disk when `what?.txt` was refused, and the user would - // be left with half a directory and no list of what is in it (issue #64's - // other complaint). - for format in FORMATS { - let dir = tempfile::TempDir::new().unwrap(); - let archive = archive_with( - dir.path(), + let message = refusal( + extract_with(&archive, &out, &as_windows(Substitutions::new())), format, - &[("summary.txt", b"fine"), ("what?.txt", b"question")], ); - let out = dir.path().join("out"); - let err = extract_with(&archive, &out, &as_windows(Substitutions::new())).unwrap_err(); - - let message = err.to_string(); - assert!(message.contains("what?.txt"), "{format}: {message}"); - assert!(message.contains('?'), "{format}: {message}"); + assert!(message.contains("notes.txt:hidden"), "{format}: {message}"); + assert!(message.contains(':'), "{format}: {message}"); assert!( - files_under(&out).is_empty(), - "{format}: {:?} was written before the refusal", - files_under(&out) + !out.join("notes.txt").exists(), + "{format}: the neighbour was written despite the refusal" ); + assert!(files_under(&out).is_empty(), "{format}"); } } #[test] -fn two_entries_that_would_land_on_one_name_are_refused_by_name() { - // Deliberately not disambiguated: renaming one of them to `a_b (2).txt` is - // how a user ends up with a file they never look at again. Both names are - // in the message so the answer can be changed. +fn two_entries_that_used_to_collide_are_refused_for_their_own_names() { + // These two were the collision case: `?` and `*` both answered with `_` + // made one name out of two, and the refusal named all three spellings so + // the answer could be changed. + // + // A collision can no longer be manufactured, because nothing is renamed, so + // the guard that looked for one has gone with it. The archive is still + // refused, for the plainer reason that this host cannot write either name. + // + // Only the first offending entry is reported, and the assertion that + // `a_b.txt` is absent is the one that matters: a planned name appearing in + // a message would mean something is still working one out. for format in FORMATS { let dir = tempfile::TempDir::new().unwrap(); let archive = archive_with( @@ -688,24 +683,26 @@ fn two_entries_that_would_land_on_one_name_are_refused_by_name() { let out = dir.path().join("out"); let answers = Substitutions::new().with('?', "_").with('*', "_"); - let err = extract_with(&archive, &out, &as_windows(answers)).unwrap_err(); + let message = refusal(extract_with(&archive, &out, &as_windows(answers)), format); - let message = err.to_string(); assert!(message.contains("a?b.txt"), "{format}: {message}"); - assert!(message.contains("a*b.txt"), "{format}: {message}"); - assert!(message.contains("a_b.txt"), "{format}: {message}"); assert!( - files_under(&out).is_empty(), - "{format}: a collision must leave the output alone" + !message.contains("a_b.txt"), + "{format}: nothing is renamed, so no planned name should appear: {message}" ); + assert!(files_under(&out).is_empty(), "{format}"); } } #[test] -fn a_renamed_entry_colliding_with_an_untouched_one_is_refused_too() { - // The case a "compare the rewritten names to each other" check would miss: - // only one of these two changes, and it lands on a name the archive already - // uses. +fn a_trailing_dot_is_refused_rather_than_folded_onto_the_name_beside_it() { + // `notes.txt.` used to lose its dot and land on `notes.txt`, which the + // archive already holds, and the collision guard existed to catch exactly + // that. Now the dot is never dropped, so the two names stay two names and + // the archive is refused for the one the host will not preserve. + // + // The neighbour is ordinary and still goes nowhere, which is the all-or- + // nothing half of the policy. let dir = tempfile::TempDir::new().unwrap(); let archive = archive_with( dir.path(), @@ -714,9 +711,11 @@ fn a_renamed_entry_colliding_with_an_untouched_one_is_refused_too() { ); let out = dir.path().join("out"); - let err = extract_with(&archive, &out, &as_windows(Substitutions::new())).unwrap_err(); + let message = refusal( + extract_with(&archive, &out, &as_windows(Substitutions::new())), + "zip", + ); - let message = err.to_string(); assert!(message.contains("notes.txt."), "{message}"); assert!(files_under(&out).is_empty()); } @@ -745,26 +744,32 @@ fn an_archive_that_already_names_one_entry_twice_still_extracts() { } #[test] -fn an_answer_the_host_cannot_write_is_refused_before_the_archive_is_opened() { - // Checked against a path that does not exist: if the answer were validated - // per entry instead of up front, this would fail with "no such file" - // instead, and a user would only learn their replacement was no good after - // choosing an archive. - let dir = tempfile::TempDir::new().unwrap(); - let missing = dir.path().join("nowhere.zip"); - let err = extract_with( - &missing, - &dir.path().join("out"), - &as_windows(Substitutions::new().with('?', "<")), - ) - .unwrap_err(); - assert!( - matches!( - err, - collapse_core::CompressionError::Name(NameError::UnwritableReplacement { .. }) - ), - "{err}" - ); +fn an_answer_no_longer_rescues_an_entry_the_host_cannot_write() { + // What this test used to guarantee — that a replacement the host could not + // write was refused up front, before the archive was even opened — has no + // subject left: nothing validates an answer, because nothing applies one. + // + // It is kept, turned around to face the policy itself, because that is the + // part most likely to be quietly undone. `?` answered with `_` is the most + // reasonable answer anyone could give to the most ordinary question this + // ever asked, and it must still extract nothing at all. + for format in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with(dir.path(), format, &[("what?.txt", b"question")]); + let out = dir.path().join("out"); + + let message = refusal( + extract_with( + &archive, + &out, + &as_windows(Substitutions::new().with('?', "_")), + ), + format, + ); + + assert!(message.contains("what?.txt"), "{format}: {message}"); + assert!(files_under(&out).is_empty(), "{format}"); + } } #[test] diff --git a/apps/core/tests/security.rs b/apps/core/tests/security.rs index 28c433c..db8c587 100644 --- a/apps/core/tests/security.rs +++ b/apps/core/tests/security.rs @@ -11,7 +11,7 @@ use collapse_core::compression::{ compress_7z_dir, compress_tar_dir, compress_zip_dir, extract_7z, extract_tar, extract_zip, NameRules, Substitutions, }; -use collapse_core::{extract, extract_with, Algorithm, ExtractOptions, Verify}; +use collapse_core::{extract, extract_with, Algorithm, CompressionError, ExtractOptions, Verify}; use sevenz_rust2::{SevenZArchiveEntry, SevenZWriter}; use tar::{Builder, EntryType, Header}; use zip::write::SimpleFileOptions; @@ -628,10 +628,17 @@ fn no_format_writes_an_entry_name_with_a_colon_as_a_stream() { #[test] fn a_replacement_cannot_carry_an_entry_out_of_the_output_directory() { - // The answer the user gives is put inside a name that containment has - // already cleared, so an unchecked replacement is a traversal by the back - // door: `?` answered with `../..` would write above the output directory - // with nothing left to notice it. + // The answer the user gave used to be pushed inside a name that containment + // had already cleared, so an unchecked replacement was a traversal by the + // back door: `?` answered with `../..` would write above the output + // directory with nothing left to notice it. `check_replacements` stood in + // front of that. + // + // The hole is now closed a layer earlier and by construction rather than by + // a check: an answer is never applied to anything, so it cannot reach a + // path at all, and the entry that would have carried it is refused for its + // own name. The hostile answers are still offered here, and must still + // change nothing. for (ext, build) in [ ("zip", malicious_zip as fn(&Path, &str)), ("7z", malicious_7z), @@ -737,7 +744,17 @@ fn no_format_writes_through_a_symlink_already_in_the_output() { /// planned rename must not be able to land outside either. #[cfg(unix)] #[test] -fn a_renamed_entry_cannot_be_written_through_such_a_symlink() { +fn an_entry_that_cannot_be_named_is_refused_before_any_symlink_is_followed() { + // This entry used to take a second write path. Windows rules made the `?` a + // question, the answer renamed it, and the renamed branch wrote through a + // route that `unpack_in`'s own containment check never saw — so it needed + // its own proof that a symlink already sitting in the output could not be + // followed. + // + // That branch is unreachable now: the name is refused and nothing is + // written by any path. The test stays because the thing it guards is the + // file outside the output directory, and that assertion does not care which + // of the two reasons kept it safe. for ext in ["zip", "7z", "tar"] { let dir = tempfile::TempDir::new().unwrap(); let outside = dir.path().join("outside"); @@ -753,12 +770,8 @@ fn a_renamed_entry_cannot_be_written_through_such_a_symlink() { _ => malicious_tar(&archive, "link/ev?l.txt"), } - // Windows rules make the `?` a question, so this entry is renamed and - // takes the planned write path rather than the untouched one. - let options = ExtractOptions::new() - .with_rules(NameRules::windows()) - .with_replacements(Substitutions::new().with('?', "i")); - let escaped = outside.join("evil.txt"); + let options = ExtractOptions::new().with_rules(NameRules::windows()); + let escaped = outside.join("ev?l.txt"); assert_contained(extract_with(&archive, &out, &options), &escaped); } } @@ -894,18 +907,30 @@ fn the_same_archive_extracts_normally_somewhere_else() { /// lands exactly on the archive. Checking the archive's own spelling would /// miss it. #[test] -fn the_check_follows_the_renamed_name_not_the_archive_s() { +fn an_entry_that_cannot_be_named_never_reaches_the_archive_it_would_overwrite() { + // This used to be the case for following the *planned* name: `v?.zip` + // answered with `_` became `v_.zip`, which is the archive being read, and + // the overwrite guard had to see that even though the archive's own name + // matched no entry. + // + // Nothing is renamed any more, so that arrangement cannot be built. What + // is left is the ordering, and it is worth pinning: the naming refusal + // comes first, so the archive is never opened for writing at all. The + // guarantee a person cares about is unchanged and is the last assertion — + // the archive is still there, byte for byte. let dir = tempfile::TempDir::new().unwrap(); let archive = dir.path().join("v_.zip"); malicious_zip(&archive, "v?.zip"); let before = std::fs::read(&archive).unwrap(); - let options = ExtractOptions::new() - .with_rules(NameRules::windows()) - .with_replacements(Substitutions::new().with('?', "_")); + let options = ExtractOptions::new().with_rules(NameRules::windows()); let err = extract_with(&archive, dir.path(), &options) - .expect_err("the planned name lands on the archive"); - assert!(err.to_string().contains("over the archive itself"), "{err}"); + .expect_err("a name this host cannot write is refused"); + assert!( + matches!(err, CompressionError::Name(_)), + "refused for the naming reason, before the overwrite check: {err}" + ); assert_eq!(std::fs::read(&archive).unwrap(), before); } + From 768f1dc7fbcb020eabd976d506a44cfb4d9743a0 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 17:06:24 +0200 Subject: [PATCH 05/12] cli: test the widened refusal instead of the adjustments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `a_name_that_needs_no_answer_is_adjusted_rather_than_refused` was passing while asserting the opposite of what the CLI does, which is the worst state a test can be in: it exercised `adjustments`, a pure function that still works, and its name and doc comment described a product behaviour that is gone. Both adjustment tests now pin the criterion `run_extract` actually uses. The report's `characters` half is empty for a trailing dot or a device name — that is why they used to extract — and `is_empty()` is not, which is the whole of the one-line change. Judged through `NameReport` rather than by running the command, since these faults are Windows-only and this suite runs on Linux. `adjustments` and `Adjustment` lose their only tests and their import here. They are dead code, reached only where the report is provably empty, and go when `Outcome::Extracted` loses the field. --- apps/cli/tests/names.rs | 66 ++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/apps/cli/tests/names.rs b/apps/cli/tests/names.rs index f12fb9a..c8e27d1 100644 --- a/apps/cli/tests/names.rs +++ b/apps/cli/tests/names.rs @@ -18,7 +18,7 @@ use std::io::Write as _; use std::path::{Path, PathBuf}; use clap::Parser; -use collapse_cli::{adjustments, run, Adjustment, Cli, CliError}; +use collapse_cli::{run, Cli, CliError}; use collapse_core::{NameReport, NameRules}; fn run_err(args: &[&str]) -> CliError { @@ -143,50 +143,50 @@ fn the_refusal_counts_the_entries_each_character_holds_up() { ); } -/// A trailing dot and a device name have one correct answer and nobody to ask, -/// so they are not what the refusal is about. Refusing them too would leave a -/// Windows user unable to extract an archive whose only fault is a file called -/// `aux.log`, which the desktop app would open without a word. +/// A trailing dot and a device name used to be adjusted rather than refused, on +/// the reasoning that they had one correct answer and nobody to ask. Core +/// adjusts nothing now, so `run_extract` refuses on the whole report instead of +/// on its `characters` half, and these three entries are exactly what that +/// widening is about: an archive whose only fault is a file called `aux.log` is +/// refused on Windows rather than arriving as `aux_.log`. +/// +/// Judged through `NameReport` rather than by running the command, because the +/// faults are Windows-only and this suite runs on Linux. #[test] -fn a_name_that_needs_no_answer_is_adjusted_rather_than_refused() { +fn a_name_that_needs_no_answer_is_refused_like_any_other() { let windows = NameRules::windows(); let report = NameReport::of(&["notes.txt.", "CON.txt", "aux.log", "fine.txt"], windows); + // The half the CLI used to consult is empty: not one of these is a question + // anybody could be asked. Under the old rule the archive extracted. + assert!(report.characters.is_empty(), "none of these is a question"); + // The half it consults now is not, which is the whole of the change. + assert!(!report.is_empty(), "the archive is refused all the same"); + + let named: Vec<&str> = report.entries.iter().map(|e| e.entry.as_str()).collect(); assert_eq!( - adjustments(&report, windows), - vec![ - Adjustment { - entry: "notes.txt.".to_string(), - written: "notes.txt".to_string(), - }, - Adjustment { - entry: "CON.txt".to_string(), - written: "CON_.txt".to_string(), - }, - Adjustment { - entry: "aux.log".to_string(), - written: "aux_.log".to_string(), - }, - ], - "the device keeps the extension that says what the file is, and the writable name is absent" + named, + ["notes.txt.", "CON.txt", "aux.log"], + "every offending entry, and the writable one absent" ); } -/// An entry needing an answer has no adjustment to report, because there is no -/// answer to apply. That is what keeps the two lists disjoint: what `run` -/// refuses is exactly what this cannot rewrite. +/// The two kinds of fault now arrive at the same place, which is what stops the +/// refusal being half a list. A report mixing one of each names both, so a +/// user is not refused for `what?.txt`, fixed nothing, and then refused again +/// for `notes.txt.`. #[test] -fn an_entry_needing_a_replacement_has_no_adjustment() { +fn a_report_mixing_both_kinds_of_fault_names_all_of_them() { let windows = NameRules::windows(); let report = NameReport::of(&["what?.txt", "notes.txt."], windows); - assert_eq!( - adjustments(&report, windows), - vec![Adjustment { - entry: "notes.txt.".to_string(), - written: "notes.txt".to_string(), - }], - ); + assert!(!report.is_empty()); + let named: Vec<&str> = report.entries.iter().map(|e| e.entry.as_str()).collect(); + assert_eq!(named, ["what?.txt", "notes.txt."]); + + let message = windows_refusal("mixed.zip", &["what?.txt", "notes.txt."]); + assert!(message.contains("what?.txt"), "{message}"); + assert!(message.contains("notes.txt."), "{message}"); } // ------------------------------------------------------ end to end, on any host -- From 39a1fcaeb3bcbecb4b4754f5a4766ffac6c97ee4 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 17:06:24 +0200 Subject: [PATCH 06/12] desktop: test that the dialog's answers no longer do anything MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The host-rules half of this suite drove the NUL byte through the whole exchange: ask, answer, extract, check the name on disk. The answer reaches nothing now, so those assertions had no subject. - `no_answer_rescues_a_name_this_computer_cannot_write` replaces both the substantive answer and the empty one with a table over the two, since they were the same test with a different string. It is the one to look at if the dialog is ever wired back up by accident. - `a_hostile_answer_changes_nothing_because_no_answer_is_applied` keeps the `../escaped` fixture from the separator test. That answer used to be refused by the ruleset; now it is simply inert, and the archive extracts while nothing called `escaped` appears anywhere. - `the_refusal_says_which_character_is_the_problem` keeps the reason travelling to the dialog, which matters more here than anywhere: a NUL prints as nothing, so an entry named without its character looks fine. - `an_entry_beside_the_name_it_would_have_taken_is_still_just_refused` records that the collision it was built for cannot happen, and holds on to the part that still matters — the writable neighbour is not written either. --- apps/desktop/src-tauri/tests/names.rs | 121 ++++++++++++++------------ 1 file changed, 65 insertions(+), 56 deletions(-) diff --git a/apps/desktop/src-tauri/tests/names.rs b/apps/desktop/src-tauri/tests/names.rs index 4941c3c..56d2706 100644 --- a/apps/desktop/src-tauri/tests/names.rs +++ b/apps/desktop/src-tauri/tests/names.rs @@ -316,21 +316,27 @@ fn an_ordinary_archive_extracts_with_no_answers_at_all() { } #[test] -fn an_answer_containing_a_separator_is_refused_before_anything_is_written() { - // `?` is writable on this host, so nothing in this archive needs answering: - // the answer is refused on its own account, by the ruleset, before the - // archive is opened. That is what keeps a bad answer from being discovered - // half way through an extraction. +fn a_hostile_answer_changes_nothing_because_no_answer_is_applied() { + // This used to be the guard on the answer itself: `?` replaced with + // `../escaped` would have carried an entry out of the output directory, so + // the ruleset refused the answer before the archive was opened. + // + // There is no answer to refuse any more. Nothing in this archive is + // unwritable, so it extracts, and the point is what does *not* happen: the + // replacement reaches no name, creates no directory and moves nothing. A + // regression here would show up as `escaped` existing somewhere. let dir = TempDir::new().unwrap(); let archive = zip_with(dir.path(), &[("notes.txt", b"hello")]); let out = dir.path().join("out"); - let problem = refusal(extract_answering(&archive, &out, &[("?", "../escaped")]).unwrap()); + let outcome = extract_answering(&archive, &out, &[("?", "../escaped")]).unwrap(); - assert!(problem.contains("path separator"), "{problem}"); + assert_eq!(written(outcome), ["notes.txt"]); + assert_eq!(files_under(&out), ["notes.txt"]); assert!( - !out.exists(), - "the output directory was created before the answer was judged" + !dir.path().join("escaped").exists() && !out.join("escaped").exists(), + "the answer reached a path: {:?}", + files_under(dir.path()) ); } @@ -381,38 +387,36 @@ fn a_name_this_computer_cannot_write_is_reported_with_the_character_to_ask_about #[cfg(unix)] #[test] -fn the_answer_is_written_and_the_listing_names_what_is_on_disk() { - let dir = TempDir::new().unwrap(); - let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); - let out = dir.path().join("out"); - - let outcome = extract_answering(&archive, &out, &[("\u{0}", "_")]).unwrap(); - - // The name on disk, never the archive's: reporting `bad\0name.txt` would - // name a file that exists nowhere and send the user looking for it. - assert_eq!(written(outcome), ["bad_name.txt"]); - assert_eq!(files_under(&out), ["bad_name.txt"]); - assert_eq!(fs::read(out.join("bad_name.txt")).unwrap(), b"hello"); -} - -#[cfg(unix)] -#[test] -fn an_empty_answer_removes_the_character() { - let dir = TempDir::new().unwrap(); - let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); - let out = dir.path().join("out"); - - let outcome = extract_answering(&archive, &out, &[("\u{0}", "")]).unwrap(); - - assert_eq!(written(outcome), ["badname.txt"]); +fn no_answer_rescues_a_name_this_computer_cannot_write() { + // Both halves of what the dialog used to offer, and neither works now: a + // character to put in its place, and an empty answer meaning "just drop + // it". The name the archive gave is the only name that may be written, so + // an entry holding a NUL does not arrive under some other spelling — it + // does not arrive. + // + // This is the test to look at if the dialog is ever wired back up by + // accident: it is the one that says the answers are inert. + for answer in ["_", ""] { + let dir = TempDir::new().unwrap(); + let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); + let out = dir.path().join("out"); + + let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", answer)]).unwrap()); + + assert!( + problem.contains("cannot be written on this system"), + "answer {answer:?}: {problem}" + ); + assert!(!out.exists(), "answer {answer:?} wrote {:?}", files_under(&out)); + } } #[cfg(unix)] #[test] -fn a_name_left_unanswered_stops_before_anything_is_written() { +fn a_name_this_computer_cannot_write_stops_before_anything_is_written() { // The other entry is perfectly writable and still does not get written: - // extraction settles every name from the listing before the first byte, so - // a user who dismissed the dialog is not left with half an archive. + // every name is judged from the listing before the first byte, so a user is + // never left with half an archive and no way to tell which half. let dir = TempDir::new().unwrap(); let archive = zip_with( dir.path(), @@ -423,40 +427,46 @@ fn a_name_left_unanswered_stops_before_anything_is_written() { let problem = refusal(extract_answering(&archive, &out, &[]).unwrap()); assert!( - problem.contains("no replacement for it was given"), + problem.contains("cannot be written on this system"), "{problem}" ); assert!(problem.contains("bad"), "the entry names itself: {problem}"); assert!( !out.exists(), - "an unanswered name wrote {:?}", + "an unwritable name wrote {:?}", files_under(&out) ); } #[cfg(unix)] #[test] -fn an_answer_this_computer_cannot_write_either_is_refused_with_the_reason() { +fn the_refusal_says_which_character_is_the_problem() { + // The reason has to travel, because the dialog renders it and it is all the + // user gets: naming the entry without naming the character leaves them + // staring at a file name that looks fine, since a NUL prints as nothing. let dir = TempDir::new().unwrap(); let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); let out = dir.path().join("out"); - // Replacing the NUL with a NUL is not an answer, and it is caught before - // the archive is opened rather than by the write failing. - let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", "a\u{0}b")]).unwrap()); + let problem = refusal(extract_answering(&archive, &out, &[]).unwrap()); - assert!(problem.contains("cannot write"), "{problem}"); + assert!(problem.contains("refuses in a file name"), "{problem}"); + assert!(problem.contains("\\0"), "the character is shown escaped: {problem}"); assert!(!out.exists()); } #[cfg(unix)] #[test] -fn two_entries_that_would_land_on_one_name_are_refused_naming_both() { - // Renaming one of them behind the user's back is how a file disappears - // without anyone noticing, so the answer is refused and both names are - // said out loud. The second entry here is one the host could write - // perfectly well: the collision is created by the answer, not found in the - // archive. +fn an_entry_beside_the_name_it_would_have_taken_is_still_just_refused() { + // This was the collision case: answering the NUL with `_` made this entry + // into `bad_name.txt`, which the archive already holds, and one of the two + // would have been written over the other. Both names went into the refusal + // so the answer could be changed. + // + // Nothing is renamed now, so the two names stay two names and there is no + // collision to find. The archive is refused for the NUL alone, and + // `bad_name.txt` — an entry this host writes perfectly well — is not + // written either, which is the part worth keeping. let dir = TempDir::new().unwrap(); let archive = zip_with( dir.path(), @@ -466,11 +476,10 @@ fn two_entries_that_would_land_on_one_name_are_refused_naming_both() { let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", "_")]).unwrap()); - assert!(problem.contains("bad_name.txt"), "{problem}"); - assert!(problem.contains("both be written as"), "{problem}"); - assert!(!out.exists(), "a collision wrote {:?}", files_under(&out)); - - // And an answer that keeps them apart goes through. - let outcome = extract_answering(&archive, &out, &[("\u{0}", "-")]).unwrap(); - assert_eq!(written(outcome), ["bad-name.txt", "bad_name.txt"]); + assert!( + problem.contains("cannot be written on this system"), + "{problem}" + ); + assert!(!out.exists(), "wrote {:?}", files_under(&out)); } + From 74caaa18e6a825b2c06c6303451d451ffaf9ea7c Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 17:07:35 +0200 Subject: [PATCH 07/12] tests: drop the trailing blank line rustfmt refuses Left behind by rewriting the last test in two files. `fmt/check` is CI's first gate and everything waits on it, so this cost a whole run. --- apps/core/tests/security.rs | 1 - apps/desktop/src-tauri/tests/names.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/apps/core/tests/security.rs b/apps/core/tests/security.rs index db8c587..f84ca8f 100644 --- a/apps/core/tests/security.rs +++ b/apps/core/tests/security.rs @@ -933,4 +933,3 @@ fn an_entry_that_cannot_be_named_never_reaches_the_archive_it_would_overwrite() ); assert_eq!(std::fs::read(&archive).unwrap(), before); } - diff --git a/apps/desktop/src-tauri/tests/names.rs b/apps/desktop/src-tauri/tests/names.rs index 56d2706..8aa796c 100644 --- a/apps/desktop/src-tauri/tests/names.rs +++ b/apps/desktop/src-tauri/tests/names.rs @@ -482,4 +482,3 @@ fn an_entry_beside_the_name_it_would_have_taken_is_still_just_refused() { ); assert!(!out.exists(), "wrote {:?}", files_under(&out)); } - From 7f35b5a666ade3a554f99ab22770b98ba18f0aed Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 17:08:41 +0200 Subject: [PATCH 08/12] desktop: break two asserts the way rustfmt wants them rustfmt breaks a call whose arguments exceed 60 columns, not 100, which is the width the eye checks against. Both of these fit a line and still get split. --- apps/desktop/src-tauri/tests/names.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src-tauri/tests/names.rs b/apps/desktop/src-tauri/tests/names.rs index 8aa796c..8540d25 100644 --- a/apps/desktop/src-tauri/tests/names.rs +++ b/apps/desktop/src-tauri/tests/names.rs @@ -407,7 +407,11 @@ fn no_answer_rescues_a_name_this_computer_cannot_write() { problem.contains("cannot be written on this system"), "answer {answer:?}: {problem}" ); - assert!(!out.exists(), "answer {answer:?} wrote {:?}", files_under(&out)); + assert!( + !out.exists(), + "answer {answer:?} wrote {:?}", + files_under(&out) + ); } } @@ -451,7 +455,10 @@ fn the_refusal_says_which_character_is_the_problem() { let problem = refusal(extract_answering(&archive, &out, &[]).unwrap()); assert!(problem.contains("refuses in a file name"), "{problem}"); - assert!(problem.contains("\\0"), "the character is shown escaped: {problem}"); + assert!( + problem.contains("\\0"), + "the character is shown escaped: {problem}" + ); assert!(!out.exists()); } From 8df2e3009c43e80a47741dc2ae20f40c840a42f7 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 17:26:35 +0200 Subject: [PATCH 09/12] docs: record that extraction refuses a name rather than adjusting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three documents describing the naming layer all still described the scheme that was replaced, which is worse than describing nothing: threat_model.md is the stated source of truth for why each guard exists, and it was explaining a guard that had stopped existing. **threat_model.md**, measure 4b, is rewritten around the narrowing and keeps the reason for it, since the scheme it replaced looked like the kinder answer and somebody will propose it again. What it produced was a tree of names this program had invented, indistinguishable afterwards from the archive's own. The `unwritable_names` report is now described as what it became: a prediction of a refusal rather than a questionnaire. Measure 4a loses the paragraph about following the *planned* name. That guard had to, while a substitution could land an entry on the archive under a name the archive did not spell; with nothing renamed the two names are one, and the guard got simpler rather than weaker. Two test names it cited no longer existed, both renamed in this branch, and a sweep of every identifier the docs cite says the rest resolve. The new pair that pins the policy itself is cited too. **architecture.md**, the names.rs section, is rewritten the same way, and now says which surface is inert (`Substitutions`, `with_replacements`, `NamePlan`, the `extract_*_planned` variants) so the next reader does not take dead machinery for live design. The CLI section gains the pre-flight refusal and why it duplicates core's: core stops at the first offending component, the CLI has the whole listing and names every entry at once. Also corrected along the way, drift that predates this branch: both architecture.md and desktop.md said the desktop exposes four Tauri commands and listed four. There are five — `unwritable_names` arrived later — and desktop.md's layout block never gained `src/names.rs` either. --- docs/architecture.md | 65 ++++++++++++++++++++++++++++++++------------ docs/desktop.md | 7 ++++- docs/threat_model.md | 55 +++++++++++++++++++++++++++++-------- 3 files changed, 97 insertions(+), 30 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 438eb23..cdac90a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -82,19 +82,35 @@ any platform, so every Windows rule is tested from a Mac, and only `NameRules::host()` is chosen by the compiler. A rule reachable solely under `#[cfg(windows)]` is a rule this repository cannot test. -It answers with structure rather than a string, because a front end has to -render it: each offending character (and whether the host **rejects** it or -**reinterprets** it, which is the difference between a colon failing and a -colon quietly becoming an NTFS stream), a trailing dot or space, and a reserved -device name. Only the first is a question for the user; the other two are -adjustments that need explaining, not answering. - -`unwritable_names` inspects a listing without extracting, and `extract_with` -takes the answers. Replacements are applied **before** the structural -adjustments, or `CO?1` answered with `M` would be left as the device `COM1`. -An answer that is itself unwritable, carries a path separator, or empties a -component is refused, and a collision is refused naming both entries rather -than silently renaming one. +**A name that does not fit stops the extraction.** Nothing is renamed to make it +fit, and nothing is put to the user as a question: an archive arrives under the +names it carries or it does not arrive. `refuse_unwritable_names` judges the +whole listing before the first byte and fails on the first component +`NameRules::can_write` rejects, so the output directory is left as it was found, +the writable entries included. + +The four faults it knows about are one fault as far as a caller is concerned — +a character the host **rejects** (`?`), one it **reinterprets** (`:`, which is +the difference between a write failing and a write quietly becoming an NTFS +stream), a trailing dot or space, and a reserved device name. Each would leave a +file under a name the archive did not ask for, and that is the whole of what is +being prevented. +[threat_model.md](threat_model.md#4b-entry-names-this-host-cannot-write) records +why this replaced a scheme that adjusted and asked instead. + +It still answers with **structure** rather than a string, because the refusal has +to be rendered: `unwritable_names` inspects a listing without extracting +anything and reports every entry this host would refuse and why, so a front end +can say what will not work before anyone waits for it. The CLI names every +offending entry at once from that report; core stops at the first, the run being +over either way. + +`Substitutions` and `ExtractOptions::with_replacements` survive as inert +surface, so the two front ends still compile while their naming dialogs are +taken out. `NamePlan` and the `extract_*_planned` backend variants are likewise +vestigial: nothing constructs a plan that says anything. Removing all of it, +tar's second write path included, is follow-up work kept out of the commit that +changed the policy, because that path carries the containment guard. ### `src/paths.rs` — the guards both front ends share @@ -207,6 +223,15 @@ Extraction (`run_extract`) resolves the output directory (default the current directory) and calls `collapse_core::extract`, which creates the directory tree as needed. +Ahead of that it reads the listing once through `unwritable_names_with` and +refuses an archive holding any name this host cannot write, which core would +refuse anyway. The duplication is deliberate and is the reason the CLI's message +is worth having: core stops at the first offending component, while this has the +whole listing and names every entry at fault in one go, so a user is not refused +four times over four runs. A listing this build cannot read is deliberately not +this check's business — extraction is about to open the same archive and fail on +it in the extractor's own vocabulary, which is the message worth keeping. + ## collapse-remote — the client for a remote server A small crate holding the client side of the server's job flow: @@ -438,15 +463,21 @@ HTTP, same engine as the CLI. It compresses files and folders and extracts archives, in the cervantic visual style (warm cream + terracotta, monospace), and targets macOS, Windows and Linux from one codebase. -The backend exposes four Tauri commands: `is_directory` (UI icon/name hint), +The backend exposes five Tauri commands: `is_directory` (UI icon/name hint), `compress_path` (dispatches file vs. folder, refuses to overwrite its own source or a file inside the folder being compressed, replaces an existing output only when `overwrite` says the user agreed to it in the save dialog, and hands the work to a remote server when one is chosen), -`extract_archive`, and `check_server` (a health probe for the settings panel). +`extract_archive`, `check_server` (a health probe for the settings panel), and +`unwritable_names`, which reports the entry names this machine cannot write. + +That last one, and the dialog it feeds, are **inert**: extraction refuses such +an archive outright rather than taking answers, so the replacements the dialog +collects reach nothing. Both go when the dialog does. -All three of those carry `#[tauri::command(async)]`. A bare `#[tauri::command]` -on a synchronous function runs the body on the thread handling the IPC message, +All four of the blocking ones carry `#[tauri::command(async)]`. A bare +`#[tauri::command]` on a synchronous function runs the body on the thread +handling the IPC message, so the window stops repainting until it returns: a compression froze it for its whole duration, and a mistyped server address froze it for the 30 seconds of ureq's connect timeout. `is_directory` is the exception, being a single `stat`. diff --git a/docs/desktop.md b/docs/desktop.md index bde473b..4a04ff0 100644 --- a/docs/desktop.md +++ b/docs/desktop.md @@ -25,9 +25,14 @@ apps/desktop/ src-tauri/ Rust backend src/lib.rs app wiring: the plugin, the command registry, run() src/commands.rs the commands the webview invokes: is_directory, - compress_path, extract_archive, check_server + compress_path, extract_archive, check_server, + unwritable_names src/paths.rs same_file and inside, the guards that stop an archive overwriting its own source or a file within it + src/names.rs the entry-name exchange the extract dialog was built on. + Inert: extraction refuses a name this host cannot write + rather than taking a replacement for it, so the answers + the dialog collects reach nothing. Goes when it does. tests/ Cargo integration tests (see below) tauri.conf.json window, bundle, identifier (com.cervantic.collapse) capabilities/ window permissions (core + dialog) diff --git a/docs/threat_model.md b/docs/threat_model.md index 709b6b7..d79d56e 100644 --- a/docs/threat_model.md +++ b/docs/threat_model.md @@ -91,7 +91,7 @@ still be exactly one `Normal` component, which closes that at the source. `zip_symlink_entry_is_not_materialized_as_symlink`, `tar_symlink_entry_is_not_materialized`, `no_format_writes_through_a_symlink_already_in_the_output`, -`a_renamed_entry_cannot_be_written_through_such_a_symlink`, +`an_entry_that_cannot_be_named_is_refused_before_any_symlink_is_followed`, `a_colon_in_a_later_component_cannot_clear_the_path_being_built`. ### 3. Hardlink escape (tar) @@ -135,9 +135,12 @@ hardlink is a second name for one file and never resolves to the same string, which is precisely how `--force` was once able to overwrite its own source on the compression side. -The check follows the **planned** name rather than the archive's spelling, since -a rename can land an entry on the archive that the archive's own name does not -match. +The check compares the name the archive spells, because that is the only name an +entry can be written under. It used to have to follow a *planned* name as well: +while extraction still renamed entries to fit the host, a substitution could +land one on the archive that the archive's own name did not match. Nothing +renames an entry any more (see [4b](#4b-entry-names-this-host-cannot-write)), so +the two names are one and the guard got simpler rather than weaker. This is the mirror of a guard compression has always had (`OutputIsSource`, and an output inside the folder being archived, neither of which `--force` unlocks). @@ -146,7 +149,7 @@ positions on the same question (issue #96). **Covered by** `no_format_writes_an_entry_over_the_archive_it_is_reading`, `a_hardlink_to_the_archive_is_not_a_way_around_it`, -`the_check_follows_the_renamed_name_not_the_archive_s`, and +`an_entry_that_cannot_be_named_never_reaches_the_archive_it_would_overwrite`, and `the_same_archive_extracts_normally_somewhere_else`, which is the one that stops the guard from being fixed by refusing too much. @@ -162,12 +165,34 @@ same family: the host silently gives the file a different name than the archive asked for. **Prevention.** Extraction judges every entry name against the host's rules -before anything is written, and refuses rather than guessing. A character the -host cannot hold is a question for the user, who supplies a replacement; the two -adjustments nobody can be asked about (a trailing run, a device name) are -applied and reported. The whole listing is planned before the first byte, so an -unanswerable name or two entries that would collide leave the output directory -as they were found. +before anything is written, and **refuses**. Not adjusts, and not asks: an +archive arrives under the names it carries or it does not arrive. + +That is a narrowing of what shipped first, and the reason is worth recording, +because the original answer looked like the kinder one. A character the host +could not hold was put to the user as a question and replaced with their answer; +a trailing run and a reserved device name were adjusted without asking, since +neither has anything anyone could be asked about. It worked. What it produced +was a directory tree whose names this program had invented: `what?.txt` arrived +as `what_.txt` and `CON.txt` as `CON_.txt`, and nothing afterwards — not the +returned listing, not a later `collapse compress` of the same folder, not the +person reading it a month later — could tell which names came from the archive +and which from us. An extraction that cannot reproduce what the archive says is +one that should not happen, and saying so is cheaper to live with than a rename +nobody remembers agreeing to. + +So the four faults have one ending now. `NameRules::can_write` is the whole +question — can this host hold this component, spelled exactly this way — and the +first component that fails it stops the run with `NameError::Unwritable`, naming +the entry, the component and the reason. The listing is judged in full before +the first byte, so a refusal leaves the output directory exactly as it found it, +the entries that were perfectly writable included. + +What replaces the question is a prediction. `unwritable_names` reads a listing +and reports every entry this host would refuse, decompressing nothing and +creating nothing, so a front end can say what will not work before anybody waits +for it. The CLI surveys the whole listing and names every offending entry at +once; core stops at the first, since by then the run is over either way. The rules are **data**, not `#[cfg]`, so a Mac can be asked what a Windows host would refuse, which is what makes them testable at all: nobody working on this @@ -194,7 +219,7 @@ the user ever seeing the question (issue #89). Recovering what a damaged archive still holds is not gone, it is simply no longer the default: the backends (`extract_tar` and friends) take no options and -never come through the planning pass. +never come through this pass. **Covered by** `apps/core/tests/names.rs`, in particular `an_entry_splits_the_same_way_on_every_host`, @@ -204,6 +229,12 @@ never come through the planning pass. `a_damaged_archive_writes_nothing_rather_than_writing_raw_names` and `the_same_names_are_refused_whether_or_not_the_archive_is_damaged`. +The policy itself is pinned by `every_fault_stops_the_whole_archive_the_same_way`, +which drives all four faults through all three formats and checks that a +writable entry sitting beside a bad one is not written either, and by +`an_answer_no_longer_rescues_an_entry_the_host_cannot_write`, which is the guard +against the substitutions being quietly reintroduced. + ## Compression measures ### 5. Symlinks are never followed out of the tree From 20654f357e2b6d734e1b6f3c1c24152e6e1ec76c Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 17:34:10 +0200 Subject: [PATCH 10/12] docs: three gaps a sweep of the whole documentation turned up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit None of these come from this branch; they are drift the review found while checking the naming rewrite, and they are worth the same commit because they are all "the documentation describes a smaller product than the one that ships". **`--verify` was undocumented for the CLI.** The flag exists, is shipped, and `docs/server.md` documents its server-side twin in full, while README.md and architecture.md's command surface never mentioned it. Worse, neither said the thing a user would actually want to know: *every* local compression is already read back, and `--verify` only chooses how deeply. A compressor here finalises on drop, so a run that dies partway through leaves an archive that opens cleanly and is silently short — the default check is what catches that, and it was invisible in the docs. Also recorded: the flag is refused with `--server`, because the archive is built on the far side. **`test (desktop, rust)` was missing from the CI section**, which exists to describe the job graph and listed every other job. It is the only Linux job that compiles the Tauri crate, and it needs Node for the bundle `generate_context!()` embeds. **The README's test counts were stale**, and were stale before this branch: 620/505 written against 623/507 on `main`. Now 621/506, which this branch's own net of two removed tests produced. A number nothing verifies will drift again; worth either dropping or adding a check, and it is not obvious which. A sweep of every internal link, anchor, cited file path and cited test name across all eight documents came back clean otherwise. --- README.md | 18 ++++++++++++++---- docs/architecture.md | 17 +++++++++++++++-- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 5638742..999fccf 100644 --- a/README.md +++ b/README.md @@ -60,8 +60,18 @@ Formats: `zip` (default, or inferred from `-o`'s extension), `7z`, `tar`. Level next to the source; override with `-o`. It **won't overwrite** an existing archive unless you pass `--force`, and `--force` still refuses to write onto its own source or onto a file inside the folder being compressed, because either -would destroy the data instead of archiving it. Run `collapse --help` for the -full surface. +would destroy the data instead of archiving it. + +**Every local compression is read back before it is reported.** A compressor +here finalises on drop, so a run that dies partway through still closes out an +archive that opens cleanly and is quietly missing whatever had not been written +yet — nothing about the file itself says so. By default the check is the +archive's own listing, which is what catches that. `--verify` upgrades it to +reading every entry back, roughly twice the work, which also checks the +per-entry checksums zip and 7z store (tar stores none). It cannot be combined +with `--server`, since the archive is built on the far side. + +Run `collapse --help` for the full surface. Prebuilt macOS binaries (Apple Silicon and Intel tarballs, with sha256 checksums) are attached to every @@ -188,8 +198,8 @@ Requires **Rust 1.88+** (2021 edition). ```bash make build # build the Rust crates -make test # run every suite (620 Rust tests + 116 Vitest cases) -make test/rust # only the Rust tests that need no Node toolchain (505) +make test # run every suite (621 Rust tests + 116 Vitest cases) +make test/rust # only the Rust tests that need no Node toolchain (506) ``` `make test` includes the desktop app's own Rust suite, which compiles Tauri, so diff --git a/docs/architecture.md b/docs/architecture.md index cdac90a..363f6b1 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -176,7 +176,8 @@ effects — no subprocess needed. ### Command surface ``` -collapse compress [-f 7z|zip|tar] [-l 1-5] [-o ] [--force] [--server ] +collapse compress [-f 7z|zip|tar] [-l 1-5] [-o ] [--force] + [--verify] [--server ] collapse extract [-o ] ``` @@ -218,6 +219,15 @@ Aliases `c` / `e`. The CLI-local `Format` enum (`clap::ValueEnum`) converts to handles files and directories alike; the archive lands at the same output path local mode would use. The safety guards in step 4 run before any network I/O. Extraction has no remote mode. +6. **Read the archive back.** Every local compression is checked, and `--verify` + only chooses how deeply: `Verify::Index` by default, `Verify::Contents` with + the flag. The depth is bound once and then both handed to the engine and + reported, so an `Outcome` cannot name a check that did not happen. The flag + is refused alongside `--server`, because the archive is built on the far side + and the protocol has no way to ask for the deeper check; the server takes its + own `verify=` parameter instead. What each depth buys per format is in + `compression/verify.rs`, and the difference is not cosmetic: tar stores no + checksum over an entry's data at all. Extraction (`run_extract`) resolves the output directory (default the current directory) and calls `collapse_core::extract`, which creates the directory tree @@ -552,7 +562,10 @@ for four minutes. It is there because merging two individually well formatted branches can still produce an unformatted tree, and only a check on the merged result sees that. Per app, tests gate the build: `test (core)` (`make core/test`) gates `test (remote)`, `test (cli)`, `test (server-backend)` and -`test (desktop)` (the Tauri IPC is mocked, so that one needs Node only), while +`test (desktop)` (the Tauri IPC is mocked, so that one needs Node only) and +`test (desktop, rust)` (the `src-tauri` suite, which needs the Node toolchain to +build the bundle `generate_context!()` embeds, plus the webkit system libraries, +and is the only Linux job that compiles the Tauri crate at all), while `test (server-frontend)` is independent of the Rust engine and waits only on `fmt`. **Linux runs everything, on every push and every pull request.** macOS and Windows run the whole Rust suite too, as `test (rust, macos)` and From dd37a669ec993e39eaeea07f584665c413d1f77a Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 19:40:39 +0200 Subject: [PATCH 11/12] chore: move the development branch from dev to develop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One logical change across CI and the documentation, kept in one commit because splitting it would leave a state where the workflows enforce one branch name and the documents describe another. **Workflows.** `gitflow.yml` is the one that matters: it only triggers on PRs whose base is in `branches:`, and its two jobs are the required status checks on the rulesets. Left pointing at `dev`, a PR into `develop` would never run it, so the required check would never report and the PR would be unmergeable — blocked waiting on something that cannot arrive. Its trigger, both `base_ref` guards, the release head-branch check and every message now say `develop`. `test-and-build.yml` gets the push trigger and the `head_ref` gate that lets the macOS and Windows suites run on the release path. `deploy-landing.yml` gets its push trigger. `release.yml` only mentioned the old name in a comment. **The staging branch keeps its name.** `pages/landing-dev` is what serves the staging site, so renaming it would change that URL and buy nothing. The name describes the environment, not the branch the build came from, and both `deployment.md` and the workflow now say so rather than leaving it looking like something that was missed. **Documentation.** 41 references across `git_flow.md`, `deployment.md`, `architecture.md` and `README.md`. The rename was applied only to the backticked branch reference, so `pages/landing-dev`, the `collapse-server-aio:dev` image tag, `npm run tauri dev` and the word "development" were left alone; the ASCII flow diagram and a table's column alignment were fixed by hand afterwards. **Not in this commit, and required before `dev` is deleted:** the rulesets. `protect-dev` still targets `refs/heads/dev`. The documentation now calls it `protect-develop`, which is a promise this commit cannot keep on its own. --- .github/workflows/deploy-landing.yml | 10 ++++++---- .github/workflows/gitflow.yml | 18 +++++++++--------- .github/workflows/release.yml | 2 +- .github/workflows/test-and-build.yml | 18 +++++++++--------- README.md | 4 ++-- docs/architecture.md | 14 +++++++------- docs/deployment.md | 25 +++++++++++++++---------- docs/git_flow.md | 25 ++++++++++++++----------- 8 files changed, 63 insertions(+), 53 deletions(-) diff --git a/.github/workflows/deploy-landing.yml b/.github/workflows/deploy-landing.yml index fd36709..855b5fa 100644 --- a/.github/workflows/deploy-landing.yml +++ b/.github/workflows/deploy-landing.yml @@ -3,13 +3,13 @@ name: Deploy landing # Build the Nuxt landing and publish the compiled static site to a branch that # holds only the build output: # - push to main → pages/landing (production) -# - push to dev → pages/landing-dev (staging, to preview before main) +# - push to develop → pages/landing-dev (staging, to preview before main) on: push: - branches: [main, dev] + branches: [main, develop] workflow_dispatch: -# One deploy per source branch at a time; don't let main and dev cancel each other. +# One deploy per source branch at a time; don't let main and develop cancel each other. concurrency: group: deploy-landing-${{ github.ref_name }} cancel-in-progress: true @@ -39,7 +39,9 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - # main goes to production; every other branch (dev) goes to staging. + # main goes to production; every other branch (develop) goes to staging. + # The target branch keeps its -dev name: it is what serves the staging + # site, and renaming it would change that URL for no gain. if [ "$GITHUB_REF_NAME" = "main" ]; then TARGET="pages/landing" else diff --git a/.github/workflows/gitflow.yml b/.github/workflows/gitflow.yml index 2d21507..49734b3 100644 --- a/.github/workflows/gitflow.yml +++ b/.github/workflows/gitflow.yml @@ -1,19 +1,19 @@ name: Gitflow # Enforce where changes come from: -# - PRs into dev must come from a gitflow branch (feature/*, hotfix/* or +# - PRs into develop must come from a gitflow branch (feature/*, hotfix/* or # release/*), or from main itself (a post-release back-merge). -# - PRs into main are releases and must come from dev, only. +# - PRs into main are releases and must come from develop, only. # Both checks are required by the branch rulesets, so a PR from any other # source branch cannot be merged. on: pull_request: - branches: [dev, main] + branches: [develop, main] jobs: branch-name: name: gitflow branch name - if: github.base_ref == 'dev' + if: github.base_ref == 'develop' runs-on: ubuntu-latest steps: - name: Check the head branch follows gitflow @@ -24,7 +24,7 @@ jobs: feature/*|hotfix/*|release/*|main) echo "ok: $HEAD_REF" ;; *) - echo "PRs into dev must come from a feature/*, hotfix/* or release/* branch (got: $HEAD_REF)" + echo "PRs into develop must come from a feature/*, hotfix/* or release/* branch (got: $HEAD_REF)" exit 1 ;; esac @@ -33,13 +33,13 @@ jobs: if: github.base_ref == 'main' runs-on: ubuntu-latest steps: - - name: Check the release comes from dev + - name: Check the release comes from develop env: HEAD_REF: ${{ github.head_ref }} run: | - if [ "$HEAD_REF" = "dev" ]; then - echo "ok: release PR from dev" + if [ "$HEAD_REF" = "develop" ]; then + echo "ok: release PR from develop" else - echo "PRs into main are releases and must come from dev (got: $HEAD_REF)" + echo "PRs into main are releases and must come from develop (got: $HEAD_REF)" exit 1 fi diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f6eb7d3..fb99cd9 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -30,7 +30,7 @@ concurrency: jobs: # A release must correspond to a released state of the code: refuse any # v* tag whose commit is not part of main's history (e.g. a tag placed on - # dev by mistake). The publish job requires this to pass. + # develop by mistake). The publish job requires this to pass. verify: name: tag is on main if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags/') diff --git a/.github/workflows/test-and-build.yml b/.github/workflows/test-and-build.yml index 5f02cab..b2e92af 100644 --- a/.github/workflows/test-and-build.yml +++ b/.github/workflows/test-and-build.yml @@ -5,7 +5,7 @@ name: test-and-build # landing has no test suite and is built by deploy-landing.yml instead. on: push: - branches: [main, dev] + branches: [main, develop] # `labeled` is not in the default set, and without it the `full-matrix` label # below would be a documented escape hatch that never fires: adding a label # would change nothing until the next push. @@ -245,11 +245,11 @@ jobs: # times a Linux one in billed minutes and feature work does not need them. # # They run when the code is on, or heading to, a shipping branch: - # - any push to dev or main (the `on:` block already limits pushes to those) - # - a pull request FROM dev (the release PR into main), from main (a + # - any push to develop or main (the `on:` block already limits pushes to those) + # - a pull request FROM develop (the release PR into main), from main (a # back-merge) or from a release/* branch # A pull request from feature/* or hotfix/* therefore stays Linux only, and - # picks these up when it lands on dev. When such a branch does touch + # picks these up when it lands on develop. When such a branch does touch # platform-specific code, label the pull request `full-matrix` to ask for # them anyway rather than renaming the branch. # @@ -263,7 +263,7 @@ jobs: if: >- github.event_name == 'push' || github.event_name == 'workflow_dispatch' || - github.head_ref == 'dev' || + github.head_ref == 'develop' || github.head_ref == 'main' || startsWith(github.head_ref, 'release/') || contains(github.event.pull_request.labels.*.name, 'full-matrix') @@ -534,22 +534,22 @@ jobs: run: make desktop/compile # The same compile on the other two platforms it ships to, on the release - # path only: a release/* branch, and the dev to main pull request itself. + # path only: a release/* branch, and the develop to main pull request itself. # # That second one is not optional. The pull request into main IS the release # gate, the last thing that runs before a tag exists, so a desktop build # skipping there would mean the app was never compiled for macOS or Windows - # on the commit about to ship. Its head branch is dev, not release/*, so it + # on the commit about to ship. Its head branch is develop, not release/*, so it # needs naming separately. # - # Not on dev, deliberately. The test jobs above already compile this crate on + # Not on develop, deliberately. The test jobs above already compile this crate on # both platforms: `make desktop/test-rust` runs `cargo test` inside src-tauri, # which goes through generate_context!(), runs build.rs (so rc.exe, the icon # and the manifest on Windows) and links executables. All this adds is the # release profile, which for this crate means lto, codegen-units = 1 and # panic = "abort": a real difference, but a narrow one, and the slowest kind # of build there is. Paying ten to fifteen minutes per platform on every push - # to dev for that is not a good trade. + # to develop for that is not a good trade. # # On a release branch it is, because the alternative is finding out after the # tag is pushed. Note what it still does not cover: it compiles rather than diff --git a/README.md b/README.md index 999fccf..79720cb 100644 --- a/README.md +++ b/README.md @@ -254,9 +254,9 @@ make fmt make lint # format / clippy both Rust workspaces make fmt/check # fail instead of reformatting (the tree is rustfmt clean) ``` -Work happens on `dev`, merged into `main` per release (see +Work happens on `develop`, merged into `main` per release (see [git flow](docs/git_flow.md)). CI invokes these same `make` targets on every -pull request and on pushes to `dev` and `main`. +pull request and on pushes to `develop` and `main`. ## License diff --git a/docs/architecture.md b/docs/architecture.md index 363f6b1..d15f712 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -550,7 +550,7 @@ outside. Source files carry no inline `#[cfg(test)] mod tests`. ## CI -`.github/workflows/test-and-build.yml` runs on every push to `main`/`dev` and +`.github/workflows/test-and-build.yml` runs on every push to `main`/`develop` and on pull requests, entirely on Linux runners. Every job is named `test ()` or `build ()`, so a check's name says what it does and which app it belongs to; the job ids match those names (`test-cli`, @@ -569,10 +569,10 @@ and is the only Linux job that compiles the Tauri crate at all), while `test (server-frontend)` is independent of the Rust engine and waits only on `fmt`. **Linux runs everything, on every push and every pull request.** macOS and Windows run the whole Rust suite too, as `test (rust, macos)` and -`test (rust, windows)`, but only for the branches that ship: any push to `dev` -or `main`, and any pull request from `dev`, from `main`, or from a `release/*` +`test (rust, windows)`, but only for the branches that ship: any push to `develop` +or `main`, and any pull request from `develop`, from `main`, or from a `release/*` branch. A `feature/*` or `hotfix/*` pull request stays Linux only and picks -them up when it lands on `dev`, because those runners cost several times a +them up when it lands on `develop`, because those runners cost several times a Linux one per minute. When such a branch does touch platform-specific code, labelling the pull request `full-matrix` asks for them anyway. @@ -585,13 +585,13 @@ a developer's own machine ever ran the desktop suite there. The desktop app is compiled on Linux everywhere as `build (desktop)`, and on the other two platforms as `build (desktop, macos)` and `build (desktop, -windows)` **on the release path only**: a `release/*` branch, and the `dev` to +windows)` **on the release path only**: a `release/*` branch, and the `develop` to `main` pull request itself. That second one matters, since that pull request is -the release gate and its head branch is `dev` rather than `release/*`, so +the release gate and its head branch is `develop` rather than `release/*`, so skipping there would mean the app was never compiled for macOS or Windows on the commit about to ship. -Not on `dev`, deliberately. The cross-platform test jobs already compile this +Not on `develop`, deliberately. The cross-platform test jobs already compile this crate on both platforms, since `make desktop/test-rust` runs `cargo test` inside `src-tauri`, which goes through `generate_context!()`, runs `build.rs` and links executables. All the extra build adds is the release profile (`lto`, diff --git a/docs/deployment.md b/docs/deployment.md index a9310e0..4971476 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -13,23 +13,28 @@ pipeline and [desktop.md](desktop.md) for the desktop bundle specifics. The landing is published to a **build branch** that holds only the compiled site (no source). Which branch depends on where the change landed: -| Source branch | Deploys to | Environment | Purpose | -|---------------|----------------------|-------------|-------------------------------------------| -| `dev` | `pages/landing-dev` | staging | preview changes before they reach `main` | -| `main` | `pages/landing` | production | the live site | +| Source branch | Deploys to | Environment | Purpose | +|---------------|---------------------|-------------|------------------------------------------| +| `develop` | `pages/landing-dev` | staging | preview changes before they reach `main` | +| `main` | `pages/landing` | production | the live site | -So the normal flow is: iterate on `dev` → check the result on `pages/landing-dev` -→ merge `dev` into `main` (a release) → production updates on `pages/landing`. +The staging branch keeps its `-dev` name now that the source branch is +`develop`. It is the branch that serves the staging site, so renaming it would +change that URL and buy nothing; the name is a label for the environment rather +than for the branch it is built from. + +So the normal flow is: iterate on `develop` → check the result on `pages/landing-dev` +→ merge `develop` into `main` (a release) → production updates on `pages/landing`. ``` -commit to dev ──▶ CI build ──▶ pages/landing-dev (staging) -merge to main ──▶ CI build ──▶ pages/landing (production) +commit to develop ──▶ CI build ──▶ pages/landing-dev (staging) +merge to main ──▶ CI build ──▶ pages/landing (production) ``` ## How it works The workflow is [`.github/workflows/deploy-landing.yml`](../.github/workflows/deploy-landing.yml). -On every push to `main` or `dev` (and via manual **Run workflow** / `workflow_dispatch`): +On every push to `main` or `develop` (and via manual **Run workflow** / `workflow_dispatch`): 1. **Build** — `make landing/build`, which runs `npm ci` then `nuxt generate`, producing the static site in `apps/landing/.output/public`. @@ -39,7 +44,7 @@ On every push to `main` or `dev` (and via manual **Run workflow** / `workflow_di branch: `main` → `pages/landing`, anything else → `pages/landing-dev`. The deploy commit is authored as the repo owner (via the account's GitHub noreply -email), not a bot. Concurrency is keyed per source branch, so a `dev` deploy and a +email), not a bot. Concurrency is keyed per source branch, so a `develop` deploy and a `main` deploy never cancel each other. Because each deploy force-pushes an orphan-style commit, the build branches never diff --git a/docs/git_flow.md b/docs/git_flow.md index 01686ee..d765b74 100644 --- a/docs/git_flow.md +++ b/docs/git_flow.md @@ -7,15 +7,15 @@ This repository follows a **gitflow** branching model, kept deliberately simple. Where humans commit code: - **`main`** — released versions only. Every merge into `main` marks a version and gets a tag (`vX.Y.Z`). Changes arrive only through pull requests (see [Branch protections](#branch-protections)). -- **`dev`** — the integration branch where real day-to-day work happens. All work lands here first, always through a pull request from a gitflow branch; direct pushes are blocked. -- **`feature/`** — branched off `dev` for each unit of work, merged back into `dev` via pull request when done. Since direct pushes to `dev` are blocked, even small changes travel on a short-lived feature branch. +- **`develop`** — the integration branch where real day-to-day work happens. All work lands here first, always through a pull request from a gitflow branch; direct pushes are blocked. +- **`feature/`** — branched off `develop` for each unit of work, merged back into `develop` via pull request when done. Since direct pushes to `develop` are blocked, even small changes travel on a short-lived feature branch. - **`release/`** — the version bump and any last touches before a - release, branched off `dev` and merged back into `dev` by pull request, after - which `dev` goes to `main` as usual. Mechanically a feature branch; the name + release, branched off `develop` and merged back into `develop` by pull request, after + which `develop` goes to `main` as usual. Mechanically a feature branch; the name is what makes CI run the macOS and Windows suites on it, which is the point of using it for a release rather than `feature/release-`. -- **`hotfix/`** — an urgent fix for a released version. Mechanically a feature branch (off `dev`, merged into `dev` by PR); the name signals urgency, and a release (`dev` → `main`, new patch version + tag) follows immediately. PRs into `main` accept only `dev`, so there is no direct fix-to-main path; keep `dev` releasable. +- **`hotfix/`** — an urgent fix for a released version. Mechanically a feature branch (off `develop`, merged into `develop` by PR); the name signals urgency, and a release (`develop` → `main`, new patch version + tag) follows immediately. PRs into `main` accept only `develop`, so there is no direct fix-to-main path; keep `develop` releasable. ## Deploy branches (auto-generated — never commit by hand) @@ -23,7 +23,10 @@ CI force-pushes the built landing site to these branches; they contain only buil output (no source), and each deploy replaces the branch wholesale: - **`pages/landing`** — the **production** landing build, published from `main`. -- **`pages/landing-dev`** — the **staging** landing build, published from `dev`, to preview changes before they reach production. +- **`pages/landing-dev`** — the **staging** landing build, published from + `develop`, to preview changes before they reach production. It keeps its + `-dev` name: it is what serves the staging site, and the name describes the + environment rather than the branch the build came from. A ruleset blocks deleting these branches; not pushing to them by hand is a convention (see [Branch protections](#branch-protections)). @@ -39,8 +42,8 @@ bind everyone, including the repo owner: (0 required approvals, solo maintainer); force pushes and deletion are blocked. The required **release source branch** status check ([`gitflow.yml`](../.github/workflows/gitflow.yml)) fails any PR whose head - branch is not `dev`: releasing from `dev` is the only way into `main`. -- **`protect-dev`**: the same for `dev`, plus the required **gitflow branch + branch is not `develop`: releasing from `develop` is the only way into `main`. +- **`protect-develop`**: the same for `develop`, plus the required **gitflow branch name** status check ([`gitflow.yml`](../.github/workflows/gitflow.yml)), which fails any PR whose head branch is not `feature/*`, `hotfix/*`, `release/*`, or @@ -54,9 +57,9 @@ bind everyone, including the repo owner: ## Releasing -When `dev` reaches a state worth shipping, open a pull request from `dev` into `main`, merge it, and tag the merge commit with the version number. `main` therefore only ever moves forward one version at a time. +When `develop` reaches a state worth shipping, open a pull request from `develop` into `main`, merge it, and tag the merge commit with the version number. `main` therefore only ever moves forward one version at a time. -Pushing the tag triggers [`release.yml`](../.github/workflows/release.yml), which builds the CLI binaries (macOS arm64 and Intel tarballs) and the desktop app (one universal macOS `.dmg`, `.deb`/`.rpm`/`.AppImage` builds for x86_64 Linux, and an `.msi` plus NSIS setup `.exe` for x64 Windows), all unsigned for now, and publishes a GitHub release with them, their sha256 checksums, and auto-generated notes. The build also runs the Rust test suite once on the macOS runner — the one shipped OS regular CI never exercises, since CI is Linux-only — so a macOS-only test failure blocks the release. Only exact `vX.Y.Z` tags trigger it, and a guard job refuses to publish when the tagged commit is not on `main` (a tag placed on `dev` by mistake never becomes a release) or when the tag does not match the versions in `apps/cli/Cargo.toml` and `apps/desktop/src-tauri/tauri.conf.json` — bump **both** before tagging, or the CLI would report the wrong `--version` and the desktop bundles (`.dmg`, `.deb`, `.rpm`, `.AppImage`, `.msi`, setup `.exe`) would carry the wrong version in their names. The `workflow_dispatch` trigger dry-runs the builds without publishing anything, even if pointed at a tag. +Pushing the tag triggers [`release.yml`](../.github/workflows/release.yml), which builds the CLI binaries (macOS arm64 and Intel tarballs) and the desktop app (one universal macOS `.dmg`, `.deb`/`.rpm`/`.AppImage` builds for x86_64 Linux, and an `.msi` plus NSIS setup `.exe` for x64 Windows), all unsigned for now, and publishes a GitHub release with them, their sha256 checksums, and auto-generated notes. The build also runs the Rust test suite once on the macOS runner — the one shipped OS regular CI never exercises, since CI is Linux-only — so a macOS-only test failure blocks the release. Only exact `vX.Y.Z` tags trigger it, and a guard job refuses to publish when the tagged commit is not on `main` (a tag placed on `develop` by mistake never becomes a release) or when the tag does not match the versions in `apps/cli/Cargo.toml` and `apps/desktop/src-tauri/tauri.conf.json` — bump **both** before tagging, or the CLI would report the wrong `--version` and the desktop bundles (`.dmg`, `.deb`, `.rpm`, `.AppImage`, `.msi`, setup `.exe`) would carry the wrong version in their names. The `workflow_dispatch` trigger dry-runs the builds without publishing anything, even if pointed at a tag. ## Commits @@ -64,5 +67,5 @@ We favor **small commits with few changes** over large ones: - One logical change per commit — a commit should be explainable in one sentence. - Prefer several small commits over one big commit, even within a single task. -- Red commits are fine on `feature/*`/`hotfix/*` branches while work is in progress (say so in the message); whatever merges into `dev` must pass `cargo test`, so every commit on `dev` and `main` is green. +- Red commits are fine on `feature/*`/`hotfix/*` branches while work is in progress (say so in the message); whatever merges into `develop` must pass `cargo test`, so every commit on `develop` and `main` is green. - Message format: a short imperative summary line, optionally prefixed by area (`core:`, `docs:`, `chore:`), with a body only when the *why* is not obvious from the diff. From c9ea38ad2e03fc9ea4408ae63084120adba7790c Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 19:52:57 +0200 Subject: [PATCH 12/12] chore: take the assistant tooling out of the repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two lines, and they were the only mentions left in tracked files. `.gitignore` listed `CLAUDE.md`, a file that was never committed — the entry existed so it could not be. With the file gone from the working copy too, the entry has nothing to ignore, and leaving it would keep a name in the repository for a file the repository does not have. `.dockerignore` excluded `.claude/` under "never ship history or local tooling state into the context", beside `.git/`, `.github/` and `node_modules/`. **That second one has a small cost, stated rather than buried:** if a `.claude/` directory ever appears at the root it now enters the Docker build context. It does not exist today, and `*.md` further down already keeps stray markdown out, so nothing changes for the images as they stand. The exclusion was hygiene, not a dependency — no build reads that path — but it was doing something, and this gives it up on purpose. Re-adding the line is the fix if a context ever grows for that reason. Nothing here touches the build: `cargo`, the Dockerfiles and the workflows never read either path. --- .dockerignore | 1 - .gitignore | 1 - 2 files changed, 2 deletions(-) diff --git a/.dockerignore b/.dockerignore index 052f643..f682a8c 100644 --- a/.dockerignore +++ b/.dockerignore @@ -14,7 +14,6 @@ apps/landing/ # Never ship history or local tooling state into the context. .git/ .github/ -.claude/ **/node_modules/ **/.DS_Store diff --git a/.gitignore b/.gitignore index 51f92f8..e08c794 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,2 @@ target/ .DS_Store -CLAUDE.md