fix: clear the help-wanted backlog - #444
Merged
Merged
Conversation
`doctor validate` counted a broken recipe reference into its error total but never into its recipe total, so two dangling references reported "2 error(s) found in 0 recipe(s)" — a summary that contradicts itself exactly when someone is trying to work out what to fix. References are resolved in a pass after validation, so neither total sees them. Both are now topped up there, the recipe total as a set: a recipe carrying two broken references is still one recipe, and one that both fails to parse and references something missing is not counted twice. The tripwire test that pinned the old arithmetic now asserts the fixed arithmetic, and a second test covers the two-references-one-recipe case. Closes #437
The last `into_vec()` left on the API surface. `GroupedQuantity` keeps components the converter cannot combine in a `HashMap`, whose iteration order Rust randomises per process, so an ingredient measured two ways — `1 cup` and `100 g` of flour — came back in a different order on every request, and disagreed with what the CLI printed for the same list. Routes through the same `ordered_components` helper the recipe handler and every core writer already use. Refs #441
`chrono`'s `Add<Duration> for NaiveDate` panics rather than erroring when the result runs off the end of the calendar, so a large `days` took down the request handler. `days` comes straight off the query string here, so this is more exposed than the CLI path, which core already saturates. Refs #428
`cook report` read its RECIPE argument with a plain `fs::read_to_string`, where `recipe`, `shopping-list`, `doctor` and the rest resolve it through `cooklang-find`. So `cook report -t t.jinja pancakes` failed with "Failed to read recipe file" while `cook recipe pancakes` rendered it, and `--base-path` had no bearing on the one argument it most looked like it should steer. Passes a `RecipeSource::Path`, which core already resolves under the request's base path — so a bare name works, and `--base-path` scopes the argument as well as the template's references, matching how `shopping-list --base-path` is documented. The two tests that pinned the old behaviour now assert the new, and a third covers the not-found message, which names the name asked for rather than a file the user never mentioned. Closes #430
`--plain` is documented as "Display ingredients without aisle categories", and the human, JSON and Markdown writers all honour it. `build_yaml_value` took no `plain` parameter, so the flag could not reach it and `-f yaml` always categorised — anyone scripting against `-f yaml --plain` got a different document shape from `-f json --plain`, with no error and no warning. The YAML writer now takes `plain` and emits the flat array of items that the JSON writer does, so the two agree. A unit test asserts that agreement directly rather than restating the shape twice, and a characterization snapshot covers the CLI path. Closes #419
`doctor` joined a reference's components with `/` while the shopping list
and the writers joined with `std::path::MAIN_SEPARATOR`, so on Windows the
same broken reference was reported as `./absent` by one command and
`.\absent` by the other — and the `./`-stripping in `get_recipe` only
matches the forward-slash form, so the prefix survived into the reported
name.
Two of these paths are not diagnostics at all, which makes this more than
cosmetic: the Cooklang writer re-emits the reference as *source*, where a
backslash is not valid syntax, and the Markdown writer puts it in a link
target. One `REFERENCE_SEPARATOR` constant now carries the rule and the
reason, and every builder uses it.
Also fixes a defect found while pinning this down: a reference's
components already begin with `.`, and the Cooklang writer prepended a
second `./` on top, so `cook recipe -f cooklang` emitted
`@././sub/sauce{}` and each further pass added another — an unbounded
prefix on every rewrite, the same corrupt-on-round-trip symptom as #414
from a different cause. Both now go through `RecipeRef::path`.
On Unix the separator was already `/`, so no existing output changes.
Closes #442
`cook recipe -f cooklang` added one leading space to any step line that wrapped immediately after an ingredient, cookware or timer, and the growth was unbounded: one space after the first pass, two after the second, three after the third. Any workflow that rewrites recipes through the formatter — normalising a collection, an editor's "format document", a pre-commit hook — corrupted the source a little more on each run. A `textwrap::Word` carries the whitespace that *trails* it, and that is what gets dropped when a line breaks there. `component_word_separator` emitted the component with `Word::from` over the component text alone, so its whitespace was empty and the following space fell to the next word instead — as an empty word whose whitespace is that space. When the break landed exactly there, the space moved to the start of the wrapped line, where a reparse reads it as ordinary step text. The component now takes its trailing whitespace with it, which is all `Word::from` needs to put the space in the right field. `formatting_is_idempotent` is un-ignored, and joined by one that runs six passes — the defect grew by a space per pass, so two passes prove nothing about the sixth — and one asserting the specific shape that broke. Closes #414
Five independent features gate a command in the top-level help, so there are thirty-two possible command lists. The snapshot branched on `self-update` alone, which left `help_output_no_update` — recorded without `sync`, and so without `login`/`logout` — asserted against any build that had `sync` but not `self-update`. `--features sync,server,import,lsp` failed for that reason. Split into two tests that hold for every combination: one snapshots the help text with the gated command lines filtered out, the other asserts that each optional command is listed exactly when its feature is on. The second is what the old snapshot was really checking, and it also catches the opposite mistake — a command offered by a build that cannot run it. `help_output_no_update.snap` is deleted; it has no combination left to describe. The `with_settings` import was gated on the same stale combination and would not compile without it, so it is unconditional now. Verified against no-default-features, each optional feature alone, the combination from the issue, and the full default set. Closes #440
`cook search`'s help text has always promised "Multiple terms are treated as AND (all must match)", but the behaviour was OR: `cooklang-find` scores a file for any term it contains and keeps everything above zero, so adding a term widened the results. `cook search chicken rice` returned recipes with no rice in them, and a query of common words matched most of a collection. Resolved in favour of the documented intent, which is also the convention users expect from a search box. The library still returns the union and its ranking; core now intersects it, keeping a hit only when every term appears in the recipe text or the file name. Single-term queries are unchanged — AND over one term is that term — so this only narrows. The web API's `GET /api/search` called `cooklang_find::search` directly and so would have kept the old semantics; it goes through core now, so the web UI and the CLI cannot disagree about what a query means. The CLI test proving trailing terms are not dropped had to be turned round: it relied on a second term *adding* a hit, which under AND is no longer distinguishable from the term being ignored. It now checks that the second term removes one. Help text and docs/search.md updated to describe AND. Closes #425
Two recipes referencing each other silently double-counted. Expansion is bounded rather than recursive, so a cycle did not hang — it walked the cycle a fixed number of times and added every ingredient it met. With `a` referencing `b` and `b` referencing `a`, the salt declared once in `a` came out as `2 tsp`, exit code 0, no warning. The deeper the mutual references, the further the quantities drifted from the truth. Expansion now carries the chain of recipes it is inside, by resolved file path. A reference resolving to one of them is not followed, and raises a warning naming the whole cycle rather than just its end. The list is still produced — a wrong list is worse than an unhelpful error, but so is refusing to shop because two recipes cross-reference. The chain is the ancestors, not everything seen, which is the distinction that keeps this from under-counting: a dressing that both the pasta and the salad call for is still counted for each, because neither contains the other. There is a test for that as well as for the cycle, since getting it wrong in that direction would be just as silent. Closes #424
Any `cook pantry add`, `remove` or `update` re-parsed the whole file into
`cooklang`'s model, applied itself, and serialised that model back — so
everything the model does not carry was gone the first time anything was
changed, silently, on a file people hand-write and hand-comment.
The worst of it was not loss but corruption. The parser reads
salt = { quantity = "1%kg", expire = "2027-01-01" }
as a *section* named `salt` holding items `quantity` and `expire`, and the
rewrite emitted it as one — so `cook pantry add fridge butter` destroyed
an item it had nothing to do with, exit code 0, success message, no
warning.
Changes are now applied to the file as a TOML document, so a write touches
only the entry asked for. Comments, blank lines, indentation, key order,
the short `x = "1%kg"` form, attributes `cooklang` does not model and
non-string values all survive, because nothing re-emits them.
Two shapes still need a decision rather than a rewrite:
- A top-level item can only be written `name = "quantity"`. Giving one an
attribute is refused, naming the fix, rather than dropped silently — and
emphatically rather than written as the inline table it would need,
which is the corruption above.
- A section written `fridge = ["milk"]` has nowhere to put a quantity, so
an edit converts it to a `[fridge]` table, keeps every name in it, and
says so.
`update` also refuses a value it cannot interpret — a hand-written
`salt = 3` — rather than guessing which attribute it meant.
The CLI dropped these diagnostics entirely, so a pantry with something the
parser did not understand was edited with no word about it; all three
commands log them now.
Six tests pinned the old destructive behaviour and now assert the new,
comparing whole file contents so that anything reformatted anywhere fails.
Closes #429
Every `#[ignore]`d test in the repository was a shopping-list test — the whole of `shopping_list_test.rs` plus cases in three other files — each with a bare `#[ignore]` and no reason, and 13 of the 26 failed when run. The most intricate command in CookCLI had no regression coverage at all. Three things had gone stale under them, none of them the tests' fault: - The shared fixture gained a `config/pantry.conf`, and pantry items are subtracted by default, so assertions were looking for ingredients the command was right to leave out. Those pass `--ignore-pantry` now, except where the pantry is the subject. - `--exclude-pantry` was renamed `--ignore-pantry`. - The `Shopping List` header is no longer printed, and the JSON and YAML documents are arrays of categories rather than objects. Two tests rested on false premises rather than stale ones: - `test_shopping_list_exclude_pantry` compared output lengths using `simple.cook`, whose ingredients the pantry cannot subtract at all — it holds `salt` as `1%kg` against a recipe's `1%tsp`, and no conversion between them exists since #433. It now uses the unitless `tomatoes`, where the subtraction really happens, and asserts which ingredient appears rather than how many bytes do. - `test_shopping_list_ingredient_modifiers` read `@pepper{}/hidden` as a hidden-ingredient marker. `/x` is Cooklang's *alias* separator, and CookCLI parses with `Extensions::empty()`, so no modifier syntax exists to hide anything. Rewritten to pin what is true: an aliased ingredient is listed once, under its name. The menu fixture's `- @./with_ref` had no `{}`, so it was never a recipe reference and the menu silently drew on two recipes rather than three. The three `snapshot_test.rs` cases are deleted rather than repaired: `shopping_list_characterization_test` already snapshots the same three invocations, so repairing them would re-record the same output twice. 26 ignored, 13 of them failing -> 0 ignored, 753 passing Closes #415
The LaTeX and Typst writers had no test that looked at the document body. The five that named them assert only that the paper-size and margin substrings appear in the preamble, so a regression that broke `\begin`/`\end` pairing, emitted a malformed preamble, mangled ingredient markup or dropped a whole section passed the suite. The Cooklang writer was no better served: one test checked that its output contains `@water` and a `#`. Adds a golden-output snapshot per format over a fixture built to exercise what these writers have to handle — front matter, multi-word ingredient names, an ingredient with no quantity, cookware with and without an amount, named and unnamed timers, a text block, a recipe reference, and both an unnamed and a named section. Each snapshot is backed by structural assertions, because a snapshot can be re-recorded wrong and some things are not a matter of taste: balanced environments and braces in LaTeX, balanced content blocks in Typst, and for Cooklang the stronger contract that its output is *source* — it must re-read as the same recipe, and formatting it again must change nothing. That last one covers #414 and #442 from the CLI end: the round trip exercises the real terminal wrap width, which the crate-level tests cannot. Closes #416
…dom one
`shopping-list-copy.spec.ts` expected `salt 1 tbsp, pinch`. That was the
order `GET /api/shopping_list` happened to serve when the test was
written: it read the components straight out of a `HashMap`, so the real
order was random per request, and it disagreed with the `salt pinch,
1 tbsp` that `cook shopping-list` printed for the same recipe.
Routing the handler through `ordered_components` settled that — unitless
first, then by unit name — so the expectation now describes a rule rather
than a coincidence. Verified against both surfaces for the seed recipe:
API -> salt pinch, 1 tbsp
CLI -> salt pinch, 1 tbsp
Refs #441
ReviewReviewed all 13 commits (help-wanted backlog cleanup). Overall this is unusually well-documented work — each commit explains its root cause, the fix is scoped tightly, and coverage is added alongside almost every fix. Two issues below look like genuine regressions/gaps worth a look before merge; the rest are minor. Correctness1. today.checked_add_signed(chrono::Duration::days(days)).unwrap_or(NaiveDate::MAX)
2. if !doc.get(section).is_some_and(Item::is_table_like) {
let mut table = Table::new();
table.set_implicit(false);
doc.insert(section, Item::Table(table)); // <-- unconditional overwrite
}
Minor / follow-up-worthy
Nice to see
Nothing above blocks the correctness/coverage fixes (#437, #430, #419, #442, #414, #415, #416, #440, #425, #424) — those all look solid. The two flagged issues are both in newer, less-covered code paths (the pantry TOML editor and the server's pantry-expiring handler) and both have narrow, testable fixes. |
This was referenced Aug 14, 2026
Closed
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.
Clears every open
help wantedissue: 13 fixed here, 4 already fixed onmainby thecookcli-coreextraction (#434) and verified rather than assumed.Tests: 760 passing, 0 failing, 0 ignored (was 696 / 0 / 27).
cargo fmt --checkandcargo clippy --workspace --all-targetsclean.One commit per issue, so this can be reviewed — or reverted — a piece at a time.
Already fixed on
mainVerified by running the reproduction, not by reading the diff:
Context, re-exported from core; the divergent copy is goneformat_number(-0.25)→"-0.25", not"3/4"doctor validateemits an identical order across five runssave_listroutes throughwrite_atomicallyFixed here
Correctness
doctor validatecounted a broken reference into the error total but never the recipe total, so two dangling references read2 error(s) found in 0 recipe(s). Both totals are topped up now, the recipe total as a set.into_vec()on the API surface.grouped_quantitykeeps inconvertible components in aHashMap, so an ingredient measured two ways came back in a different order on every request, and disagreed with what the CLI printed for the same list.pantry expiring --dayscame off the query string straight intochrono's panickingAdd, taking down the request handler.cook reportread itsRECIPEargument withfs::read_to_string, socook report -t t.jinja pancakesfailed wherecook recipe pancakesworked, and--base-pathhad no bearing on the one argument it most looked like it should steer.build_yaml_valuetook noplainparameter, so-f yaml --plainsilently produced a different document shape from-f json --plain.doctorjoined reference components with/while the shopping list and the writers usedMAIN_SEPARATOR. OneREFERENCE_SEPARATORconstant now carries the rule and the reason.cook recipe -f cooklangadded one leading space per pass to any step wrapping just after a component. Unbounded: rewriting a collection degraded it every run.Behaviour changes — the four you picked; each is the deliberate half of this PR
cook search chicken ricereturned recipes with no rice. Single-term queries are unchanged.GET /api/searchcalledcooklang_find::searchdirectly and would have kept OR — routed through core, so the web UI and the CLI cannot disagree about what a query means.a→b→acounteda's salt twice, exit 0, no warning. Expansion now carries its ancestor chain and refuses a reference that leads back up it, warning with the whole cycle named.add/remove/updateused to re-serialise the whole file fromcooklang's model. Comments, layout and unmodelled keys were lost, and a top-level item written with attributes was corrupted into a section by a command that had nothing to do with it. Now edited as a TOML document: a write touches only the entry asked for.#[ignore]d tests were shopping-list tests; 13 failed when run.Coverage
@waterand a#. Golden snapshots plus structural assertions (balanced environments and braces; for Cooklang, that its output re-reads as the same recipe).syncbut notself-update#440 — the help snapshot branched onself-updatealone across five independent features. Split into a snapshot of the invariant text and a check that each optional command is listed exactly when its feature is on. Verified against no-default-features, each feature alone, the combination from the issue, and the default set.Six things not in any issue
./on every format pass —@././sub/sauce{},@./././sub/sauce{}— because the writer prepended a.thatRecipeRef::pathalready supplies. Different cause, same corruption.@.\sub\sauce{}, which is not valid syntax. The Markdown writer puts the same path in a link target.GET /api/searchbypassed core (see search: multiple terms are OR, but the help text documents AND #425 above).- @./with_refhad no{}, so it was never a reference — the menu silently drew on two recipes rather than three.use insta::with_settingswas gated on a stale feature combination and would not compile under--no-default-features.Two judgement calls worth a close look
--expiresilently as before,addandupdatenow refuse it there and name the fix. A section writtenfridge = ["milk"]is converted to a table with a warning, since the array form has nowhere to put a quantity.Review notes
doctor_validate_output(0 recipe(s)→1 recipe(s)), a newyaml_plain_output, three new document-format snapshots.help_output_no_update.snapis deleted — it has no feature combination left to describe./. It is the Windows leg of the CI matrix that it fixes.toml_editis added as a direct dependency ofcookcli-core; it was already in the tree viacooklang.Closes #414, #415, #416, #419, #424, #425, #428, #429, #430, #437, #440, #441, #442