refactor: split recipe resolution into fetch and render stages - #710
Draft
raymondk wants to merge 2 commits into
Draft
refactor: split recipe resolution into fetch and render stages#710raymondk wants to merge 2 commits into
raymondk wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Separates recipe retrieval from rendering to improve testability and error reporting.
Changes:
- Adds dedicated fetch and render stages.
- Updates recipe resolution interfaces and initialization.
- Distinguishes fetch failures from render failures.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/icp/src/project.rs |
Integrates staged resolution and errors. |
crates/icp/src/lib.rs |
Updates test resolver. |
crates/icp/src/context/init.rs |
Initializes RecipeFetcher. |
crates/icp/src/canister/recipe/render.rs |
Adds rendering logic and tests. |
crates/icp/src/canister/recipe/mod.rs |
Exposes staged recipe API. |
crates/icp/src/canister/recipe/handlebars.rs |
Removes combined implementation. |
crates/icp/src/canister/recipe/fetch.rs |
Adds fetching, validation, and caching. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| recipe::{Recipe, RecipeType}, | ||
| }; | ||
|
|
||
| /// Context passed to a recipe resolver, describing the canister being built. |
`canister::recipe::handlebars` did two unrelated jobs in one method: it fetched a recipe's Handlebars template (local read, HTTP download, package cache) and then rendered that template into build/sync steps. Rendering was therefore only reachable through something that owned an HTTP client and a package cache, and could not be tested without writing a temp file first. Split it in two: - `recipe::fetch::RecipeFetcher` (was `Handlebars`) retrieves the template text and nothing else, behind the `Resolve` seam, which now returns a `String` instead of `(BuildSteps, SyncSteps)`. - `recipe::render_recipe` is a pure function from template text to steps. Its five tests are plain `#[test]`s over string inputs; the four they replace each needed a tempdir, a package cache and a tokio runtime. Two behavioural notes: - A rendered document that is not a valid build/sync manifest was a `panic!`; it is now `RenderRecipeError::Parse`. The rendered YAML moves to a `debug!` line, so `--debug` still shows it. - A remote template is now cached once its checksum verifies, rather than after it also renders. The checksum is what establishes the bytes are the bytes we asked for, and rendering depends on per-canister context that has nothing to do with the template's cacheability. `ConsolidateManifestError::Recipe` splits into `FetchRecipe` and `RenderRecipe` accordingly, so a failure names the stage that failed. Salvaged from PR 660, originally authored by Adam Spofford, re-executed on current `main`: that branch introduced this split as part of a larger crate reorganization, and the split stands on its own.
Review of the fetch/render split caught a behaviour regression the split
introduced, confirmed by experiment against the pre-split implementation.
Splitting fetch from render moved the cache write into the fetch stage, which
runs before anything knows whether the template is usable. `sha256` is
optional, so an unpinned remote response that is valid UTF-8 but fails to
render was written to the cache anyway — and because `read_cached_uri_recipe`
returns the stored entry when no checksum is given, every later resolution read
those bad bytes back instead of refetching. One bad response became sticky.
Measured, serving a template that fails Handlebars strict mode once and then
answering 500:
- pre-split (`main`): cache holds only `.lock` after the failed render, and the
second resolution goes back to the network (gets the 500).
- split as submitted: cache holds `recipes/<sha>/recipe.hbs`, and the second
resolution succeeds from cache with the same unusable bytes.
Caching is therefore a third step, not part of fetching:
- A download carrying a `sha256` is still cached during the fetch. The checksum
is what establishes the bytes are the ones that were asked for, and a refetch
would produce the same bytes, so there is nothing to gain by waiting.
- An unpinned download comes back as a `PendingCache` that the caller commits
through `Resolve::commit` once `render_recipe` succeeds. Until then nothing is
written, so a bad response is refetched rather than replayed.
`Resolve::resolve` now returns `Fetched { template, pending_cache }`. `commit`
defaults to doing nothing: only `RecipeFetcher` caches, and only it can
construct a `PendingCache`, so a resolver that never defers a write never has
one to commit.
Three tests cover it; the first two fail if the pending write is committed
eagerly. Also fixes the `RecipeContext` doc comment, which still described the
context as passed to a resolver after it became render-only input.
raymondk
force-pushed
the
fm/icp-recipe-fetch-render-split
branch
from
August 13, 2026 19:31
8f89015 to
d53a11c
Compare
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.
Salvaged from draft PR 660 (
spofford/deploylib), originally authored by Adam Spofford.That branch is being superseded for architectural reasons, but two of its splits stand on
their own. This is the first. It is re-executed on current
mainrather than cherry-picked,since the branch is 33 commits behind; the branch's file layout and module docstrings were
used as the specification.
What this changes
canister::recipe::handlebarsdid two unrelated jobs in one method: it fetched a recipe'sHandlebars template (local read, HTTP download, package cache) and then rendered that
template into build/sync steps. Rendering was therefore only reachable through something
that owned an HTTP client and a package cache, and could not be tested without writing a
temp file first.
recipe::fetch::RecipeFetcher(wasHandlebars) retrieves the template text andnothing else, behind the
Resolveseam — which now returns aStringinstead of(BuildSteps, SyncSteps).recipe::render_recipeis a pure function from template text to steps. Its five testsare plain
#[test]s over string inputs; the four they replace each needed a tempdir, apackage cache and a tokio runtime.
ConsolidateManifestError::Recipesplits intoFetchRecipeandRenderRecipe, so afailure names the stage that failed.
Two behavioural changes, called out for review
panic!; it is nowRenderRecipeError::Parse. The rendered YAML moves to adebug!line, so--debugstill shows it.
renders. The checksum is what establishes the bytes are the bytes we asked for; rendering
depends on per-canister context that has nothing to do with the template's cacheability.
Validation
cargo build,cargo fmt --checkandcargo clippy --workspace --all-targetsare cleanwith zero warnings.
cargo test --workspace: 38 targets pass. The 7 failures(
identity_link_hsm*,canister_snapshot_{download,upload}_resume) are environmental —they need SoftHSM2 and mitmproxy, and fail identically on unmodified
main.Review notes
This is independent of the
ScriptRunnersalvage PR — different code, no shared commits,both based on
main. Either can be reviewed, merged or reverted without the other.