feat(paths): keep user files out of the directory a package manager owns - #125
Merged
Conversation
A package manager owns the directory it installs into. WinGet records an extracted archive's top-level directory as one entry and removes every recorded entry before installing the new ones, so a portable zip whose archive has a single top-level directory loses everything written beside the executable on each upgrade, and a portable package has no script hook a manifest could use. Chocolatey keeps files it never installed, but its package folder grants plain users read and execute only, so a non-elevated run cannot save there at all. Frozen builds now resolve the profiles, the window state, both CSV exports and the crash log against %LOCALAPPDATA%\<TOOL_ID>. Running from sources is unchanged, and BEAN_DATA_DIR overrides both for a genuinely portable copy. - paths: app_dir() becomes user_data_dir(); add ensure_data_dir(), migrate_user_files() and prepare_user_data() - migration copies rather than moves, so a rollback still finds its files, and only takes a file the target lacks, so a stale copy cannot overwrite newer data. It parses nothing on the way and skips a directory carrying a user file's name - the location does not probe whether the exe directory is writable: that would make it depend on elevation, and one install would silently keep two sets of profiles - gui: prepare_user_data() runs before the first store, since both stores bind their path at import; problems are logged through _report_storage_problems - new key log.data_files_problem in both language files - the two CSV exports move to gui/csv_export.py so app.py stays under the size ratchet, which drops 1287 -> 1202 Guard: tests/test_user_data_location.py, eleven checks, with the location mutation registered in tests/test_mutation_registry.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… READMEs The prose that read "next to the executable" was true until the previous commit moved the files, and nothing guards prose (convention 5), so it is swept in the same chunk as the behaviour. - both CSV exports log the WHOLE path instead of the file name: the name was an answer only while the file landed next to the executable, and nothing else on screen names the directory - module docstrings: jsonfile, gui/profiles, gui/ui_state (which used the old location to justify why these files are hand-edited) - both READMEs: the CSV section now names the folder, says why the files left the program folder, and documents BEAN_DATA_DIR; gui/csv_export.py joins the module list Guard: tests/test_conns_export.py::test_both_exports_tell_the_user_the_whole_path, proved by mutation and registered - putting the basename back reddens it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…as to retype
Everything under packaging/ is a template with {{PLACEHOLDER}} markers, filled by
tools/build_packages.py from the one place that owns each value: the version from
VERSION.txt, the checksum AND the archive name from the release's own
SHA256SUMS.txt, the release date from the dated changelog section, the URLs from
site/site.json, and the identity and nested exe path from appinfo. A version
typed into a manifest is a second source of truth (convention 34), and a manifest
with a stale one still parses.
Nothing is published and nothing is wired into release.yml: submitting to a feed
is a human step, because a bad manifest costs somebody else's moderation queue.
packaging/README.md carries the order the steps go in, including the one that
matters most - the first published manifest must point at a release that already
keeps user files in %LOCALAPPDATA%, or WinGet's upgrade destroys them before the
version that migrates them ever runs.
Checked at the source rather than taken from notes: the newest manifest schema is
1.28.0, and ArchiveBinariesDependOnPath adds the directory HOLDING the nested file
to PATH, which is what the implementation does and what the field's description
does not say.
Verified with the real tools against the published 0.4.0 checksum: winget
validate reports success and choco pack builds the nupkg. That run also found a
real bug - sha256sum's binary-mode star was riding into the installer URL.
Guard: tests/test_packaging.py, ten checks, three mutations registered. One of
them survived at first, because the check was satisfied by a comment after the
code it named was gone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uard Answering "should packaging/ be ignored like internal_tools/?" with a test rather than with a memory, because the failure of getting it wrong does not show up on the machine that has the files. Measured with the directory absent: the renderer wrote nothing and the tests failed with a bare KeyError - a missing file instead of a reason, which is the exact failure mode the tools/ versus internal_tools/ rule exists for. Three separate things need these files tracked: the packaging tests and the mutation registry read them and CI runs on a fresh clone, packageSourceUrl has to point at a source a moderator can open (CPMR0040, a Guideline), and a package source is there so somebody other than us can see what the package does to their machine. - build_packages refuses an empty template set instead of writing nothing - the render tests no longer depend on the changelog being closed for the current version: the date reader has its own test, so a version bumped mid-release reddens one test that names itself rather than five that do not Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The folder belongs to the Windows account the program runs as. On an account without administrator rights, agreeing to the elevation prompt runs the program as the administrator account whose password was entered, so it uses that account's folder - and nothing said so, which made saved profiles look lost. Measured rather than reasoned about: the same code resolves the folder from the account it runs as, not from the account that launched it, so the two really do differ. Passing the launcher's folder into the elevated copy was rejected: a standard user's AppData is fully controlled by that user, and pointing an elevated process at a directory a plain user controls is the documented precondition for redirecting a privileged write through a directory junction, which needs no privilege to create. Today that exposure does not exist, because the elevated copy writes into its own account's folder. - About shows it in the selectable box, next to the licence path - --doctor prints a "user files:" line and carries data_dir in its JSON report - both READMEs explain that the folder follows the account, and that an administrator can set BEAN_DATA_DIR system-wide for one shared folder - new key about.data_dir in both language files The path sits on its own line for a measured reason: that box does not wrap and has no horizontal scrollbar, and on real Tk the label and a realistic frozen path came to 644 px in a 583 px box, hiding the end of the path. Split, the widest line is 413 px. Guards: test_the_about_window_says_where_the_users_files_are and test_doctor_says_where_the_users_own_files_are, both proved by mutation and registered. The first was VACUOUS at first - it asserted the path as a substring, which another line satisfied - and the mutation is what said so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
os.path.relpath raises ValueError on Windows when the two paths sit on different drives, and build_packages used it only to print what it had written. Found by CI rather than here: the Windows runner keeps the repository on one drive and the temporary directory on another, so six packaging tests failed there while passing on this machine, where both are on C:, and on Linux, where drives do not exist. Rendering into a directory on another volume is a legitimate thing to ask a build tool for. Guard: test_rendering_onto_another_drive_is_not_a_crash. The runner's condition was also reproduced locally, by making only that one call raise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g the guard The changelog said "guard: test_rendering_onto_another_drive_is_not_a_crash", and an unproven claim of that shape is exactly what the registry exists to stop: it should either be re-runnable or be listed as unproven out loud. Proved: removing the fallback in display_path reddens that test and nothing else. Co-Authored-By: Claude Opus 5 <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.
Why
A package manager owns the directory it installs into, and this program was writing the user's own files there.
Read in WinGet's source rather than its documentation: an archive install records the extracted top-level directory as one entry, and an upgrade removes every recorded entry before installing the new ones -
remove_allon that directory, with no diffing (PortableInstaller.cppcarries its ownTODO: Optimize by applying the difference). Our archive has exactly one top-level directory, so profiles, window state and both CSV exports would be destroyed on everywinget upgrade, and a portable package has no script hook a manifest could use to save them.Chocolatey is the opposite, which is worth stating because the two are usually lumped together:
RemoveInstallationFilesUnsafeskips any file absent from the package snapshot, so runtime-written files survive an upgrade. But its package folder grants plain users read and execute only, so a non-elevated run cannot save there at all.What changed
User files leave the install directory. A frozen build now resolves profiles, window state, both CSV exports and the crash log against
%LOCALAPPDATA%\<TOOL_ID>. Running from sources is unchanged, andBEAN_DATA_DIRoverrides both for a genuinely portable copy. Files from an earlier version are copied over once, on first start.Migration copies rather than moves, so rolling back to an older build still finds its files; it only takes a file the target does not have, so a stale copy can never overwrite newer data; it parses nothing on the way, so a corrupt file is copied verbatim and quarantined by the store exactly as before; and it skips a directory carrying a user file's name, which is what Scoop's persist leaves behind.
The location does not probe whether the executable's directory is writable. That was the obvious shape and it is the wrong one: it would make the location depend on elevation - the window elevates itself,
--simulatedoes not - so one install would silently keep two sets of profiles.The folder follows the Windows account, and the program now says which one it is using. About shows it in its selectable box,
--doctorprints it and carriesdata_dirin its JSON report, and both READMEs explain that elevating on an account without administrator rights uses that administrator's folder, and that an administrator can setBEAN_DATA_DIRsystem-wide to give every account one shared folder.Passing the launcher's folder into the elevated copy was measured, considered and rejected: a standard user's AppData is fully controlled by that user, and pointing an elevated process at a directory a plain user controls is the documented precondition for redirecting a privileged write through a directory junction, which needs no privilege to create. That exposure does not exist today, because the elevated copy writes into its own account's folder.
Package sources, as templates.
packaging/holds the Chocolatey and WinGet manifests with{{PLACEHOLDER}}markers, filled bytools/build_packages.pyfrom the one place that owns each value: the version fromVERSION.txt, the checksum and the archive name from the release's ownSHA256SUMS.txt, the release date from the dated changelog section, URLs fromsite/site.json, identity and the nested executable path fromappinfo. Nothing is published and nothing is wired intorelease.yml- submitting to a feed stays a human step.packaging/README.mdrecords the order, including the part that matters most: the first published manifest must point at a release that already keeps user files in%LOCALAPPDATA%, or the upgrade destroys them before the version that migrates them ever runs.Verified
python -m pytest tests- 1119 passed,python smoke_gui.pyOK,tools/ci_gui_render.pyOK on real Tk in both languages.winget validate --manifestreports success,choco packbuilds the package. That run found a real bug on the way -sha256sumwrites<hash> *<file>and the binary-mode star was riding into the installer URL.ArchiveBinariesDependOnPathadds the directory holding the nested file toPATH- what the implementation does, not what the field's one-line description implies.Guards
New:
tests/test_user_data_location.py(11 checks - the location had none at all, because the harness redirects every store to a temp directory) andtests/test_packaging.py(13 checks on files nobody here reviews twice, including one that keeps the package sources tracked).Eight mutations are registered in
tests/test_mutation_registry.py. Two of them survived their first attempt and both were defects in the test: one guard was satisfied by the comment explaining the code it named, the other by a substring another line already contained.gui/app.pysat exactly on the size ratchet, so the two CSV exports moved togui/csv_export.pyrather than the ceiling moving up. It went down, 1287 to 1202.One defect was found by CI rather than here, and is fixed in this branch:
os.path.relpathraises on Windows across drives, and the Windows runner keeps the repository on one drive and
the temporary directory on another. Six packaging tests failed there while passing on a machine
where both are on
C:and on Linux, where drives do not exist.Not done here
Nothing is published to either feed and no account exists on them. No package has been installed locally from these manifests - that changes
PATHand installs for real, so it waits. The end-to-end case behind the account split (a real standard account going through the UAC credential prompt) is not reproduced: this machine has one user profile.