Stop the user-config tests writing to the real home directory - #34
Merged
Merged
Conversation
…tory `test_user_config_save_and_load` called `UserConfig::save()`, which writes `~/.lash/config.toml` on the developer's machine. It restored afterwards with `UserConfig::default().save()`, which was wrong twice over: on the happy path it replaced whatever the user actually had with defaults, and if the test panicked or the process was killed the restore never ran at all, leaving `color_scheme = "Test Theme"` behind. No theme resolves that name, so every later `lash` invocation on the machine failed with `Color scheme 'Test Theme' not found`. A mutation-testing run surfaced this on 2026-08-09 by killing the suite mid-execution hundreds of times, but any interrupted `cargo test` does the same. `load_from(path)` and `save_to(path)` now take the config path explicitly; `load()` and `save()` are thin wrappers that pass the home-directory path. The tests point at a `TempDir` and never resolve `$HOME`. Also covered: saving into a directory that does not exist yet, and rejecting invalid TOML on load. `test_user_config_load_nonexistent` no longer carries its "assumes ~/.lash/config.toml doesn't exist or is valid" caveat, since it now reads a path it created. Audited the rest of the workspace for the same pattern. lash-cli's `Config::user_config_path` resolves a different location (`dirs::config_dir`) and its test only inspects the path. lash-tui's `apply_selected_theme` calls `save()` for real, which is correct, and no test exercises it.
This was referenced Aug 9, 2026
fohara
added a commit
that referenced
this pull request
Aug 9, 2026
Two bugs, both of which made `lash format` unsafe to run on a file it did not fully model. `format_file` rebuilt the file from the parsed model alone, and the model holds the header, the Description section and the task tree and nothing else. Every other section was silently deleted: a file with `## Notes` and `## References` came back with only the header and the tasks, exit code 0, no warning. This turned out to be broader than filed. A section *above* `## Tasks` went too, because the parser folds it into "overview" text that `TaskFile` never stored and the formatter never emitted. `format_file` now takes the source alongside the parsed file. It regenerates only the spans it owns — the H1 plus annotation block, the Description section, the Tasks section — and copies every other line through unchanged. Section boundaries come from `parser::header::section_span`, which runs through pulldown-cmark, so a `##` inside a fenced code block neither opens nor closes a section. Passing an empty source keeps the old model-only behaviour, which is what the existing unit tests want. The second bug: the parser records inline labels in metadata without removing them from the title, and the formatter wrote both. Every run appended another copy of every label, so `- [ ] one #docs` became `#docs #docs` and then `#docs #docs #docs`, and `format --check` reported the file as needing formatting forever. The formatter strips inline labels from the title and writes the sorted metadata list as the only place they appear. The parsed title keeps them, so `lash list` and search behave as they do today. Each generated block ends with its own blank separator and the source's is skipped rather than copied. Emitting both would add a blank line per run, which is the same non-idempotence in a different coat. Tests: sections above and below `## Tasks` survive and keep their order, a fenced heading does not split a section, labels are emitted once, an already-canonical file comes back byte-identical, and an idempotence property test over six shapes asserting `format(format(x)) == format(x)`. All six fail against the old behaviour. Also ticks off three items in lash.index.md that were fixed in #34 and #35 but never marked done.
fohara
added a commit
that referenced
this pull request
Aug 9, 2026
Two bugs, both of which made `lash format` unsafe to run on a file it did not fully model. `format_file` rebuilt the file from the parsed model alone, and the model holds the header, the Description section and the task tree and nothing else. Every other section was silently deleted: a file with `## Notes` and `## References` came back with only the header and the tasks, exit code 0, no warning. This turned out to be broader than filed. A section *above* `## Tasks` went too, because the parser folds it into "overview" text that `TaskFile` never stored and the formatter never emitted. `format_file` now takes the source alongside the parsed file. It regenerates only the spans it owns — the H1 plus annotation block, the Description section, the Tasks section — and copies every other line through unchanged. Section boundaries come from `parser::header::section_span`, which runs through pulldown-cmark, so a `##` inside a fenced code block neither opens nor closes a section. Passing an empty source keeps the old model-only behaviour, which is what the existing unit tests want. The second bug: the parser records inline labels in metadata without removing them from the title, and the formatter wrote both. Every run appended another copy of every label, so `- [ ] one #docs` became `#docs #docs` and then `#docs #docs #docs`, and `format --check` reported the file as needing formatting forever. The formatter strips inline labels from the title and writes the sorted metadata list as the only place they appear. The parsed title keeps them, so `lash list` and search behave as they do today. Each generated block ends with its own blank separator and the source's is skipped rather than copied. Emitting both would add a blank line per run, which is the same non-idempotence in a different coat. Tests: sections above and below `## Tasks` survive and keep their order, a fenced heading does not split a section, labels are emitted once, an already-canonical file comes back byte-identical, and an idempotence property test over six shapes asserting `format(format(x)) == format(x)`. All six fail against the old behaviour. Also ticks off three items in lash.index.md that were fixed in #34 and #35 but never marked done.
Merged
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
1 participant
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.
What was wrong
test_user_config_save_and_loadin lash-types calledUserConfig::save(),which writes
~/.lash/config.tomlon the developer's machine. It restored atthe end with
UserConfig::default().save().That was wrong twice over. On the happy path it replaced whatever the user
actually had with defaults. And if the test panicked, was filtered out
mid-run, or the process was killed, the restore never ran, leaving
color_scheme = "Test Theme"in the real config. No theme resolves that name,so every later
lashinvocation failed witherror: Color scheme 'Test Theme' not found.Observed on 2026-08-09 after a mutation run killed the suite mid-execution
hundreds of times.
lash indexwas broken machine-wide until the line wasremoved by hand. Mutation testing amplified the bug rather than causing it:
any interrupted
cargo testdoes the same.The fix
UserConfig::load_from(path)andUserConfig::save_to(path)take the configpath explicitly.
load()andsave()become thin wrappers passing thehome-directory path, so production behaviour is unchanged. The tests point at
a
TempDirand never resolve$HOME.A
Dropguard would not have been enough on its own: a SIGKILL still skipsit, and it still could not restore settings the test never captured.
Tests
Both existing tests are rewritten against a temp path, plus two new ones for
saving into a parent directory that does not exist yet and for rejecting
invalid TOML on load.
test_user_config_load_nonexistentalso drops its"assumes ~/.lash/config.toml doesn't exist or is valid" caveat, since it now
reads a path it created itself.
Verified by running the full workspace suite and confirming the real
~/.lash/config.tomlcame out byte-identical.Audit
Checked the rest of the workspace for the same pattern. lash-cli's
Config::user_config_pathresolves a different location (dirs::config_dir)and its test only inspects the returned path without writing. lash-tui's
apply_selected_themecallssave()for real, which is correct, and no testexercises it.