core: refuse an entry name this system cannot write, instead of renaming it - #112
Merged
Conversation
Release v0.1.0
Release v0.2.0
Landing: ship the download page to production
Landing: ship the platform-centric redesign to production
Release 0.3.0
Release landing refresh to production
Release: bilingual landing (English and Spanish)
Hotfix release: make the language menu clickable
Release 0.4.0
Release v0.5.0
Release v0.5.1
Release v0.6.0
Release v0.7.0
Release v0.8.0
Release v0.9.0
Release v0.9.0 (final)
Release v0.10.0
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extraction used to negotiate over a name the host could not write. A refused
character became a question for the user and was replaced with their answer; a
trailing dot or space was truncated in silence; a reserved device name gained a
_. The entries were then written under whatever that arrived at.It worked, and what it produced was a directory tree whose names this program
had invented.
what?.txtlanded aswhat_.txtandCON.txtasCON_.txt, andnothing afterwards — not the returned listing, not a later
collapse compressof 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 now reproduces what the archive says, or it does not happen.
refuse_unwritable_namesjudges the whole listing againstNameRules::can_writebefore the first byte and fails on the first component the host cannot hold
exactly as spelled, naming the entry, the component and the reason:
The four faults have one ending now. Nothing is written at all, so an entry that
was perfectly writable does not arrive either — that is the half worth keeping,
and every rewritten test asserts it.
On Linux nothing changes in practice:
NameRules::unix()refuses only the NULbyte. This is a Windows-facing change.
Closes
Closes #95 — the names the user approved are not provably the names extracted.
There is nothing to approve.
unwritable_namesis advisory, and the listingextract_withjudges is the one it extracts, so the two-read gap has nosubject.
Closes #91 — the collision guard compares planned names byte for byte. The
guard is gone: with nothing renamed, a collision cannot be manufactured, and the
case the issue calls worst (
WHAT?.txtanswered with_landing on an existingwhat_.txt) is unreachable. One residual the issue documents is not fixed anddeserves a narrow successor: two entries of the archive's own differing only in
case are still both written, the second over the first on a case-insensitive
filesystem, with the listing naming two files where the disk holds one. That
half predates this and was explicitly allowed before too.
Supersedes #90 — an entry of only trailing characters makes the whole archive
unextractable. Verified still true, and now intended: it is the policy for every
unwritable name. Both fixes the issue proposes (a fallback name; scoping the
refusal to the entry) are rejected by this decision, which is recorded in
threat_model.md§4b. Worth closing by hand rather than by keyword, since it isa decision and not a fix.
What is deliberately left behind
Substitutions,ExtractOptions::with_replacements,NamePlan, theextract_*_plannedbackend variants, tar's second write path, the CLI'sadjustments/AdjustmentandOutcome::Extracted { adjusted }, and thedesktop's whole naming dialog with its
unwritable_namescommand are all inertand documented as such.
Removing them is follow-up, kept out of this PR on purpose: tar's second write
path carries the containment guard, and it should not move in a change about
naming. Deleting the public surface would also have stopped the test crates
compiling, and tests that fail to build cannot show what a policy change costs.
Tests
No test was deleted to make this pass. Five in
apps/core/tests/names.rs, two inapps/core/tests/security.rs, two inapps/cli/tests/names.rsand four in thedesktop suite were rewritten to assert the refusal, each keeping the guarantee it
was written for. Two cases were absorbed by table-driven replacements.
Two are worth pointing at:
an_answer_no_longer_rescues_an_entry_the_host_cannot_writeis the oldanswer-validation test turned around. Its subject is gone, so it now guards
the thing most likely to be quietly undone: the most reasonable answer to the
most ordinary question this ever asked must still extract nothing.
a_name_that_needs_no_answer_is_adjusted_rather_than_refusedwas passingwhile asserting the opposite of what the product does. It exercised a pure
function that still works, under a name describing behaviour that had been
removed. Rewritten to pin the criterion the CLI actually uses.
Documentation
threat_model.mdmeasure 4b is rewritten around the narrowing and keeps thereason for it, since the scheme it replaced looked like the kinder answer and
somebody will propose it again. Measure 4a loses the paragraph about following
the planned name.
architecture.mdanddesktop.mdfollow.A sweep of every internal link, anchor, cited file path and cited test name
across all eight documents came back clean apart from three gaps this branch did
not cause, fixed in
6043c07:--verifywas undocumented for the CLI (alongwith the fact that every local compression is already read back, and the flag
only chooses how deeply),
test (desktop, rust)was missing from the CI section,and the README's test counts were stale before this branch as well as after.