feat(core): extract command logic into a cookcli-core library crate - #434
Open
dubadub wants to merge 45 commits into
Open
feat(core): extract command logic into a cookcli-core library crate#434dubadub wants to merge 45 commits into
dubadub wants to merge 45 commits into
Conversation
Spec 1 of 2: extract the recipe, shopping-list, search, doctor, pantry and report commands into a cookcli-core crate with a typed API, leaving cookcli as a clap formatting shell. Spec 2 (consumer wiring + persistent-list CLI verbs) follows separately.
14 tasks: workspace scaffold, core types, parser, formatters, then one command per task with the CLI reduced to a shell. Task 6 adds characterization snapshots for shopping-list before Task 7 refactors it: all 26 ignored tests in the suite are shopping-list tests and 11 fail when run, so the command had no regression coverage.
…n plan build.rs hard-fails without static/css/output.css and static/js/editor.bundle.js, which are gitignored, so a fresh worktree cannot build until they are generated or copied.
The repo root is both the workspace root and a member package, so bare cargo commands scope to cookcli alone. cookcli-core's tests therefore ran nowhere in CI, which would have silently skipped every test added by the extraction work.
Task 5: CoreError::Parse now has a single-line Display per library convention, so the CLI must re-attach cooklang's rendered report to keep its output unchanged. Task 14: release.yaml runs a bare 'cargo publish', which will fail once cookcli path-depends on cookcli-core. Publish core first, and verify release-please's workspace handling rather than assuming it.
Address API-durability review findings ahead of Task 2, while the crate
still has no consumers and these changes are not yet breaking:
- name the span type (Span { start, end }) instead of an opaque tuple
- mark Severity, RecipeSource and ConfigSource #[non_exhaustive]
- keep CoreError Display to a single line; the parse report stays in
the rendered field for the CLI to print
- give CoreError::Io the path it failed on, dropping #[from] so every
call site must name it
- make CoreError::Config path optional, for inline configurations
- skip null file/span in Location JSON, pinned by roundtrip tests
- add Diagnostic::hint for symmetry with warning/error
- rename ConfigSource::is_none to is_unset
- derive PartialEq on both source enums
- document the Ok-with-error-diagnostics contract on Outcome
- warn(missing_docs) and document the whole public surface
- trim the manifest to the dependencies the crate actually uses
- add crates/core/LICENSE to match the declared license
Fresh subagents implement each task, so the conventions settled during Task 1's review need to live in the plan rather than in one agent's context.
CoreError::Io covers reads and writes, but its Display said "failed to
read". Task 7's shopping-list store and Task 11's pantry mutations both
write through it, and would have reported a failed write as a failed
read. Change the message to "i/o error on {path}" while ConfigSource::read
is still the only caller and the string is not snapshot-pinned.
Also make every_display_is_a_single_line live up to its name: its input
list is hand-maintained, so add a compile-time exhaustiveness guard that
breaks when a variant is added, and assert the documented lowercase half
of the contract, not just the single-line half. Both guards were verified
by temporarily introducing a variant and an uppercase message.
Extend the diagnostic roundtrip test with the empty-location case.
Task 2 adds a third copy of config resolution alongside main.rs's and lib.rs's. Drift between those two is the bug Task 2 repairs, so the cleanup needs to be an explicit step rather than assumed.
Tasks 2 and 3 each shipped tests that passed while asserting nothing, both caught only in review. The failure mode is consistent: assertions check shape rather than value.
Strengthen parser tests from shape assertions to value assertions: the previous suite passed under severity inversion, report truncation, a wrong scale factor, a wrong label index and an ignored ansi flag. Also capture cooklang's `hints` (often a ready-to-apply fix the CLI's `warn!` discarded), mark `Diagnostic`/`Location` non_exhaustive, take `&SourceReport` in `render_report` so it serves metadata parses, re-export `cooklang`, reject non-finite scale, and replace the `into_result` panic with an error. Drops `Diagnostic::at_span`, which had no caller outside its own tests.
The human, markdown, cooklang, latex, typst, schema.org and number formatters move to `cookcli-core::format`, and `cookcli` gains a dependency on core so it can call them. Core's `PARSER` becomes the single definition; the CLI re-exports it and the `format` module under their old paths. The human formatter no longer relies on yansi's global colour switch: colour is an explicit `format::Style` argument, and `Style::Plain` strips escapes at the writer. The CLI passes `Style::Ansi`, so its output is unchanged. `anyhow` is gone from the moved files — core does not depend on it — and the formatters return `std::io::Result` instead.
src/util/format.rs held the only three tests among the moved formatter files, and they compiled into both the lib and bin targets, so they counted twice. Verified lib 72->69, bin 72->69, integration targets unchanged.
Task 4 moved 2800 lines faithfully and still shipped three API problems: a &str where an enum existed, 130 lines of untested public config frozen into semver, and a format_number that loses the sign of negatives.
Follow-up to 04abf60, applying the review's lens: the code was moved faithfully but not reconsidered as a public surface. - print_latex/print_typst take PaperSize instead of &str, so a wrong paper name is a compile error rather than a document that will not typeset. - print_human is generic over its writer again, like its six siblings; StripWriter became generic, which removes the reason the signature was &mut dyn. - The markdown options (Options, Headings, DescriptionStyle, FrontMatterName, print_md_with_options) are pub(crate): ~130 lines of untested configuration with one in-repo caller, and deriving serde would pin the wire format too. - format_number no longer mangles negatives. `-0.25` rendered as "3/4", losing both sign and magnitude, because floor() rounds away from zero. - Dropped the yansi global from the tests and replaced it with direct StripWriter tests, including an escape split across two writes, which the type's doc claimed and nothing checked. - Added a cooklang round-trip test. It found that formatting is not idempotent: a wrapped step accumulates one leading space per pass. That is a pre-existing defect and a behaviour change to fix, so it is recorded as an ignored test rather than fixed here. - Removed textwrap, humantime, anstyle and anstyle-yansi from the root manifest; 04abf60 orphaned them.
…ction All 26 ignored tests in the suite are shopping-list tests and 11 fail when run, so the command had no regression coverage (#415). These snapshots record current behaviour so the cookcli-core extraction can be verified as behaviour-preserving. They assert what the output IS, not what it should be.
Moves the whole of `cook shopping-list` below the argument layer into `cookcli-core`, so the editor addon and CookBot can build the same list without reimplementing it. - `core::find::get_recipe` resolves a name or path, distinguishing genuine absence (`RecipeNotFound`) from found-but-unusable (`Io`). - `core::shopping_list::generate` loads the aisle and pantry configuration from the `Context`, expands recipe references, merges duplicates, and subtracts the pantry, returning warnings as attributed `Diagnostic`s instead of logging them. `extract_ingredients` stays public for the web server, which accumulates one recipe at a time. - The output builders move verbatim into `core::format::shopping_list`, including the missing `plain` parameter on the YAML writer (#419). - `shopping_list::run` is now a shell: directory expansion, `name:factor` splitting and output writing only. Behaviour is pinned by the 14 characterization snapshots, which are unmoved, and by new unit tests in core. Two defects found while re-reading the moved code as library surface: - `cli_error` labelled every `CoreError::Io` a recipe, so an unreadable `config/pantry.conf` reported "Failed to read recipe". The variant carries a path and no file kind, so the wording is now resource-neutral. - Relying on discovery's `is_file()` probe to distinguish an explicitly named pantry from a discovered one covered a missing file but not an unreadable one, which newly hard-errored. The shell now probes the discovered pantry and warns, as before. Mutation testing: aggregation, pantry subtraction, `ignore_references`, diagnostics, aisle categorisation, scaling, unit joining and the lookup error mapping are all caught by tests. Two survivors are recorded in comments rather than fixed: the `CircularReference` guard is unreachable because expansion is bounded rather than recursive (as it was before), and `PantryConf::rebuild_index` is redundant because `parse_lenient` already builds the index.
Reference expansion is bounded rather than recursive, so the guard that built `CoreError::CircularReference` could never fire: `extract_ingredients` started every call with an empty `seen` map and never recursed into itself. Mutation testing confirmed it — disabling the guard failed no test. A public error variant nothing can return is a lie in the API. A consumer matching on it writes dead code, and its presence implies a cycle-safety property the crate does not have: two recipes referencing each other silently double-count the ingredients of the one they start from (#424). Removes the variant, the guard, and the `seen` map that existed only to feed it, which collapses `extract_into` back into `extract_ingredients`. Both exhaustiveness tests are updated. The `Cycles are not detected` section on `extract_ingredients` and a comment where the variant used to live point at #424 and say what reintroducing it would take, so the fix is not rediscovered from scratch. Also pins the pantry distinction restored in the previous commit: a pantry named with `--pantry` that cannot be read is fatal, while one merely discovered in `config/` warns and is skipped. Nothing covered this, and the two branches are one line apart. Permission bits are the only way to make a file `is_file()` accepts and `read_to_string` rejects — testing mere existence is what missed the case originally — so the test probes whether the mode is enforced and returns early when it is not, rather than asserting something untrue under root. Permissions are restored before any assertion runs, so a failure cannot orphan the temp directory. Mutation results for that branch: removing the discovered-pantry probe and making an explicitly named pantry non-fatal are each caught by the new test.
Task 7 added a unix-only pantry permissions test, so Windows compiles one fewer test. CI runs windows-latest, so the difference needs to be in the contract rather than discovered as a false regression.
Moves `cook search` into `cookcli_core::search::search`, leaving the CLI with argument joining, delegation and printing. Output is unchanged: quoted, one per line, relative to the search root, in relevance order. `SearchHit` carries the path it was found at and the recipe's title alongside the relative path. Both are already computed by the walk — the title comes from front matter it has to parse anyway — and an editor listing results needs a path it can open and a name it can show, neither of which `relative_path` alone provides. `query` is a single string rather than a list of terms, because the underlying scoring matches the whole query against file names and its whitespace-separated parts against contents. A list would imply a per-term structure that no scoring rule can observe. Corrects the help text and docs, which described multiple terms as AND. They are not: a file scoring above zero on any one term is a hit, so an extra term widens the results. That is `cooklang-find`'s behaviour and is now pinned by a test rather than misdescribed. Adds `CoreError::Search` for a root that cannot be searched at all, which a directory whose name contains glob syntax reaches. Reporting it as a failed read would send the user looking at permissions. CLI tests 312 -> 313: nothing covered the space-join, since the existing multi-term test searched two words from the same recipe and passed with the trailing terms discarded.
`doctor validate` is the first command whose payload *is* its diagnostics, so `cookcli_core::doctor::validate` returns them as data: `Ok` even for a collection of entirely broken recipes, `Err` only when the walk could not start. `RecipeValidation` carries both halves — structured `Diagnostic`s for consumers and cooklang's own source-quoting report for the CLI to print verbatim. Unlike `CoreError::Parse.rendered`, which is always plain, that report's colour is chosen by the caller through `ValidateRequest::style`, so a NAPI consumer showing it in a web view does not get escape codes. The CLI asks for `Style::Ansi` and prints it unchanged. The five totals are methods on `ValidationReport` rather than stored counters: they hold nothing `recipes` does not, and deriving them is what keeps a total from disagreeing with the recipes it counts. The reference map is a derived view for the same reason. Two visible changes fall out of the move. Recipes now come back in path order, where the walk previously leaked `cooklang-find`'s `HashMap` ordering into a report that changed between runs. And a root that cannot be walked is reported as `Cannot search '<root>': no such directory` rather than in cooklang-find's words. `doctor aisle` and `doctor pantry` are untouched.
The plan claimed a malformed aisle.conf fell back silently. main already logged both the warnings and the failure through warn!. The real gap was that the information was unstructured and unreachable by a library consumer, which Task 7 closed.
The five read-only `cook pantry` subcommands — list, depleted, expiring, recipes and plan — now build a request, call `cookcli-core::pantry` and format what comes back. Pantry parsing, the low-stock rules, date reading and the greedy coverage algorithm all move into the library; the wording, the emoji and the `--format` matching stay here. `load` reads through `Context::pantry`, a `ConfigSource`, so an editor can hand over pantry text it has not saved rather than a path. A context with no pantry at all is the new `CoreError::MissingConfig`, which the CLI words as it always has. `plan` needs no pantry: it works out what to stock from the recipe collection alone. Two things change on the way, both of them ordering that was previously left to a `HashMap`: - `pantry recipes` sorts its matches by name, and the missing ingredients within each, rather than reporting them in walk order. - `pantry plan` breaks a tie between equally-wanted ingredients alphabetically. Its output used to differ on every single run. Also fixes a panic: `pantry expiring -d 4294967295` ran off the end of the calendar and unwound through `NaiveDate + TimeDelta`. Adding, removing and updating items are untouched, and still own the pantry file's loader and writer.
Adds `cookcli_core::report::render`, the last command extraction. The CLI keeps argument parsing, the prototype warning, template file reading, path resolution, the exit(1) on a render failure and the println! of the result; everything else moves to core. A library must never end the host process, so core returns CoreError::Render instead of calling std::process::exit. That variant gains a `rendered` field alongside `message`, mirroring CoreError::Parse: cooklang-reports' format_with_source() is a multi-line report with source context and hints, and putting it in Display would break the single-line convention every other variant keeps. The CLI prints `rendered` verbatim, so its output is byte-identical. Inline aisle and pantry configuration is honoured. cooklang-reports' Config only takes paths, but its aisled(), excluding_pantry() and from_pantry() functions read `aisle_content` / `pantry_content` out of the template state, so a ConfigSource::Inline is injected there via Config::with_context rather than being rejected or written to a temp file. Non-finite scale factors are now rejected with CoreError::InvalidScale. `cook report recipe.cook:nan` previously produced NaN quantities; it now fails the way `cook recipe recipe.cook:nan` already did. Adds tests/report_test.rs: `cook report` had no end-to-end coverage at all. All ten tests were verified to pass against the pre-refactor implementation as well, and nineteen scenarios were diffed before and after for byte-identical output.
The .shopping-list / .shopping-checked store was behind the `server` cargo feature — exactly the feature a library consumer compiles out — while the Cooklang editor keeps a near-identical wrapper over the same cooklang::shopping_list API and the same file pair. Move it to cookcli-core so both can share one implementation. Its writes were not fully atomic: save_list used std::fs::write, which truncates before writing, so a failed write left a truncated .shopping-list. Only compact() staged and renamed. Both now go through one shared write_atomically, hoisted out of the pantry writer (Task 11b had duplicated it there) into a crate-private fs_atomic module that also fsyncs the staged file before the rename. anyhow is replaced by CoreError, with a new InvalidShoppingList variant for a .shopping-list that cannot be parsed — treating one as empty would silently discard the user's list. ShoppingListApiItem becomes StoredEntry: there is no API layer in a library.
Delete the CLI's two divergent Context copies (#417). src/main.rs's aisle()/pantry() fell back to the platform config directory and src/lib.rs's did not, and the lib.rs copy was the one the test suite exercised, which is why the drift went unnoticed. Both now re-export cookcli_core::Context, built with Context::discover so the CLI keeps ambient discovery; the base-path canonicalisation and is_dir check stay in configure_context, since core deliberately does neither. global_file_path went the same way: the session.json and sync.db call sites use cookcli_core::global_config_path. Add crates/core/README.md with the call shape, the paths-or-text contract and the editor consumer-coverage mapping, wired as a doctest under cfg(doctest) so the example cannot rot, plus readme = "README.md". Publish cookcli-core before cookcli in the release workflow: crates.io rejects a package whose path+version dependency is unpublished, so the first release after this merges would otherwise fail. The core step is gated on the version not already being on crates.io rather than continue-on-error, so a genuine failure still fails the job.
ScaledRecipe named a String, resolved against Context::base_path through
cooklang-find, so shopping_list::generate could only aggregate recipes
that already existed on disk. That made the editor's
generateShoppingList — an array of { content, scale } that never touches
the filesystem — inexpressible, and it is the single function most
responsible for this extraction existing.
Replace the name with a RecipeSource, matching recipe::read, which has
resolved both a path and a buffer since Task 5. A Content recipe is
parsed from its text and attributed to its supplied name; a Path recipe
is looked up exactly as before.
Only the starting recipe can come from memory. A Content recipe's
@./sauce{} reference is still resolved from disk under base_path,
because a reference names a file and nothing in the request carries a
second buffer to resolve it against. Documented on ScaledRecipe and
extract_ingredients, and pinned both ways: the reference expands when
the file exists, and fails with RecipeNotFound when it does not.
Nine core tests cover the new path, including two mutation guards the
coordinator called for — that Content never reads the file its name
happens to match, and that the supplied name is what identifies a buffer
that will not parse. Seven mutations were killed, including dropping the
scale, fabricating a file path for a buffer's diagnostics, and skipping
reference expansion for Content.
CLI behaviour is unchanged: src/shopping_list.rs and the two server
handlers now build RecipeSource::Path, and all 14 shopping-list
characterization snapshots are untouched.
The criterion said no command module contains parsing logic, but the plan only ever scoped doctor validate. doctor pantry, doctor aisle and 12 parse_recipe_from_entry callers were never in scope.
…core `cook doctor aisle` and `cook doctor pantry` were the last command logic left in the CLI's command modules: both walked the recipe tree, parsed every recipe and aggregated ingredients themselves. Both are now one core call each — `doctor::aisle_coverage` and `doctor::pantry_coverage` — over a shared collection walk lifted out of `pantry` into `find`. `doctor validate` no longer resolves recipe references itself either; `doctor::broken_references` does, against the root the report records. Neither subcommand had any test coverage before; thirteen characterization tests pin what they print.
Addresses review of the doctor extraction. - `config_error` returned a constant, so `cook doctor pantry` and `cook pantry list` gave different messages for one unparseable file. `parse_failure` moves to `diagnostic` and both now use it. - `pantry_coverage`'s diagnostic collection had no test: deleting the line left every doctor test passing. Mirrored the aisle test. - `cook doctor` with no subcommand no longer aborts on a check it cannot run; it prints the failure and runs the rest. - Documented that a broken reference is not a diagnostic, so `has_errors()` alone is not the verdict, and that the ingredient order is by code point rather than alphabetical.
Moving the comparison from `eq_ignore_ascii_case` to `to_lowercase` on both sides made non-ASCII names match case-insensitively, so `@Öl` against an aisle entry `öl` is no longer reported as uncategorised. That was an undeclared behaviour change; pin it and say so.
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.
Extracts CookCLI's command logic into a new
cookcli-corelibrary crate, so the Cooklang editor (via its NAPI-RS addon) and CookBot can call it directly instead of maintaining parallel reimplementations. The CLI becomes a clap-and-print shell over it.Design doc:
docs/superpowers/specs/2026-08-10-cookcli-core-library-design.mdPlan:
docs/superpowers/plans/2026-08-10-cookcli-core-extraction.mdWhat changed
crates/coreis a new workspace member holding:recipe::read,shopping_list::generate,search,doctor(validate,aisle,pantry, plus broken-reference resolution),pantry(all eight subcommands),reportContextwith opt-in configuration discovery, replacing two divergent copies in the CLIserverfeature that a library consumer compiles outEvery command has the shape:
Two things that shape follows from:
Outcome<T>carries diagnostics.cooklangparses leniently, and the CLI previously logged those warnings and discarded them. They are now returned — including hints like a ready-to-apply frontmatter replacement, which is the payload an editor quick-fix needs.RecipeSource/ConfigSource, so recipes and configs can be paths or in-memory text. An editor works on unsaved buffers; a path-only API cannot serve its main use case.CoreErroris a#[non_exhaustive]thiserrorenum rather thananyhow, so consumers can distinguish "recipe not found" from "parse failed" from "malformed config". Core contains noanyhow, and nopanic!/unwrap/exitoutside tests — a panic crossing a NAPI boundary kills the editor.Behaviour
The CLI's test suite is the contract, and it is unchanged: zero pre-existing snapshots were modified across the whole branch.
Deliberate, recorded changes:
>>-titled recipe on disk now shows its declared title rather than its filename. The title rule was implemented twice against two metadata parsers, so identical bytes gave different results depending on how they arrived.cook report x.cook:nanerrors instead of emitting NaN quantities, matchingcook recipe.cli_error; core keeps single-lowercase-lineDisplay.Bugs found and fixed here
save_listand the pantry writer usedstd::fs::write, which truncates before writing — a failure mid-write destroyed the file. Both now stage-fsync-rename. (pantry add/remove/update corrupts top-level items and silently drops comments and attributes #429, shopping list writes are not atomic: a failed write truncates .shopping-list #431)cook pantry expiring -d 4294967295(pantry expiring panics on a large --days value #428)HashMapiteration reaching stdout:doctor validate,pantry plan,pantry recipes(doctor validate reports recipes in non-deterministic order #427)format_numberlosing the sign of negatives (format_number returns a positive fraction for negative numbers #418)Contextimplementations, one missing the global config fallback (Two divergent Context implementations: src/lib.rs misses the global aisle/pantry fallback #417)cargo testscoped tocookclialoneBugs found and left alone
Filed rather than fixed, because each is a product decision or out of scope: #414 (
-f cooklangcorrupts recipes on round-trip), #415 (all 26 ignored tests are shopping-list tests, 11 fail when run), #416, #419, #424 (mutual references silently double-count), #425, #430, #437.Test plan
cargo test— 347 passed, 0 failed, 26 ignored (346 on Windows; one test is#[cfg(unix)])cargo test --workspace— 676 passedcargo test --no-default-features— 270 passedcargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warningscargo rustc -p cookcli-core --lib -- -W missing_docs— zerocargo publish -p cookcli-core --dry-rungit diff main --name-status -- tests/snapshots/shows 14 lines, allA, allshopping_list_characterization_test__*. Use--name-status, not--stat, which truncates filenamesRelease note
release-please'sruststrategy manages both crates in lockstep when the root manifest declares a[workspace]— verified from its source.cookcli-corewill therefore first publish at the CLI's next version, not the0.1.0currently in its manifest, and the release workflow now publishes core before the CLI.Scope note
cook doctor pantryandcook doctor aislewere not in the original plan — onlydoctor validatewas — but they were the last command logic left in the CLI, so they are included.src/doctor.rsis now clap definitions and printing only.Still CLI-resident and deliberately out of scope:
util::parse_recipe_from_entry's remaining callers inweb/,server/,build/andutil/menu_scale.rs, andsrc/server/handlers/pantry.rs's parallel pantry CRUD, which has its own serialiser and non-atomic writes.