feat: provision --local + gitignored adapter manifests + hardened path safety#287
Open
aram356 wants to merge 104 commits into
Open
feat: provision --local + gitignored adapter manifests + hardened path safety#287aram356 wants to merge 104 commits into
aram356 wants to merge 104 commits into
Conversation
Empty tracking commit for the implementation work tracked in: docs/superpowers/plans/2026-06-27-provision-local.md Issues: - Epic: <epic-issue-url> - Per-section sub-issues linked from the epic. This PR opens as a DRAFT and stays draft until Section 1 lands its first real commit. Each section opens as its own follow-up PR that lands here before the umbrella merges to main.
This was referenced Jun 29, 2026
run_shared_checks iterates every declared adapter and dispatches validate_adapter_manifest, which for Spin does fs::read_to_string(manifest_root.join(rel)). With the containment guard sitting after run_shared_checks, a manifest declaring [adapters.spin.adapter].manifest = "../outside/spin.toml" could trigger a filesystem read outside the project root before the guard rejected it — a spec violation of §"Path containment (MUST)" which requires the helper run BEFORE any manifest-path use. Fix by relocating the check to fire immediately after load_push_context, and looping over every declared adapter (not just ctx.adapter) since run_shared_checks reads all of them. Also close Task 7's Minor about the duplicate adapter_entry call by removing the now-redundant per-adapter guard block. Regression test: config_push_local_rejects_parent_traversal_in_ sibling_spin_adapter declares a poisoned Spin adapter alongside the pushed axum adapter, and asserts the error names the containment violation (not Spin's "failed to read spin manifest" message that would surface under the old ordering). Also tighten copy_tree's else-branch to explicitly gate on is_regular_file() rather than "everything non-dir non-symlink", add a Unix symlink-skip test, and drop a stale #[expect(dead_code)] on ValidationContext::manifest_path that now has real callers.
…to feature/provision-local-impl
The bare-cwd variant of the accept-test (--manifest edgezero.toml)
previously wrote the manifest to a tempdir and set EDGEZERO_MANIFEST,
but run_provision reads args.manifest directly (no env fallback).
The test therefore failed on manifest load ("failed to load
edgezero.toml") and its negative !contains(path-safety-markers)
assertion vacuously passed — no actual coverage of the
`args.manifest.parent() == ""` fallback.
Fix by adding a CwdGuard RAII helper that chdirs into the tempdir
under the manifest_guard() serialisation lock and restores the
previous cwd on drop. Both accept-tests now also assert positively
that the error is the (true, true) dispatch stub ("local dry-run
staging lands in Task 10/11"), proving the manifest loaded AND
path-safety passed AND we reached the dispatch matrix. Drop the
now-unnecessary EnvOverride from both tests.
Reviewer: reviewer of Task 9 pushed this as a Low ahead of Task 10
because run_with_staging depends on manifest-root/cwd correctness.
Six #[expect(clippy::missing_trait_methods)] removed by adding explicit method implementations that match the trait defaults — same code, opinion stated instead of inherited. The workspace-wide restriction group requires every trait impl to spell every method, so a no-op explicit impl beats a documented expect. ## Adapter cli/mod.rs (4 crates) Each Adapter impl now spells out every trait method: - axum: adds deployed_fields, merged_id_kinds, validate_adapter_manifest, validate_app_config_keys, validate_typed_secrets (all no-op — axum is local-only). - cloudflare: adds validate_adapter_manifest, validate_app_config_keys, validate_typed_secrets (wrangler validates its own schema at deploy). - fastly: adds merged_id_kinds, validate_adapter_manifest, validate_app_config_keys, validate_typed_secrets (fastly's stores are independent namespaces; key rules relaxed enough that no adapter-side check applies). The rationale block-comment above the impl is preserved verbatim so future readers understand why each no-op is intentional. - spin: adds deployed_fields (Fermyon Cloud addresses by name, not by id) and validate_app_config_keys (Stage 6 KV cutover dropped the rule). ## edgezero-core manifest.rs (3 Deserialize impls) HttpMethod, BodyMode, LogLevel each get an explicit deserialize_in_place that forwards to Self::deserialize — same body serde would have generated as the trait default. All 4 CI gates pass: - cargo fmt --all -- --check - cargo clippy --workspace --all-targets --all-features -- -D warnings - cargo test --workspace --all-targets - cargo check --workspace --all-targets --features 'fastly cloudflare spin'
The final else-branch was documented as unreachable but wrapped in panic!(), forcing a #[expect(clippy::panic)] on the whole function. Restructured to fold the impossible-fourth-shape case into the existing f64 branch: n.as_f64() falls back to NaN, and the downstream is_finite() assert fires the same diagnostic message either way. Two runtime panics collapsed into one assert (which isn't caught by clippy::panic — only the panic! macro is). Same behaviour, no lost signal, no more per-item expect.
The ConfigPushArgs.no_env field moved into a flattened ConfigPushSuppressions sub-struct (commit 22964f5). This test constructs args by field assignment on a Default value, so it missed the workspace-scoped rebuild. Update to args.suppress.no_env. Note args.no_env stays direct on ConfigValidateArgs (unchanged).
…y_tree
Three issues surfaced in a post-merge review of the provision-local
branch:
## 1. Axum manifest generation was not actually single-source
The Axum blueprint was registering `axum.toml` as a scaffold template
(via AXUM_TEMPLATE_REGISTRATIONS + AXUM_FILE_SPECS) that wrote the
file at `edgezero new` time — BEFORE scaffold-time provision ran.
Provision's `write_baseline_to_disk` skips existing files, so:
- `edgezero new` produced axum.toml from the .hbs template
- clean `git clone` + `edgezero provision --local` produced axum.toml
from Adapter::synthesise_baseline_manifest
Two divergent baselines for what the spec asserts is one file. The
template also lacked the `# edgezero-provision: v1` header the
synthesiser emits, so the drift was visible on inspection.
Fix: remove axum.toml from the Axum blueprint entirely, delete the
now-orphaned axum.toml.hbs, and add a comment above the file spec
list explaining why the manifest is intentionally absent. The
scaffold-time provision loop (generator::provision_all_selected_adapters)
becomes the single writer.
Regression test in generator.rs asserts that after `edgezero new`
the scaffolded axum.toml starts with `# edgezero-provision: v1`
— proving it came from the synthesiser and not a stray template.
## 2. Docs still said "Axum stays tracked"
Six guide pages and multiple spec/plan sections still described
axum.toml as tracked/scaffold-owned even though the code moved
axum.toml into the gitignored-generated set. Updated every
occurrence to the current model: all five adapter manifests
(axum.toml, wrangler.toml, fastly.toml, spin.toml,
runtime-config.toml) are provision-generated + gitignored.
The historical carve-out inside the Amendment 2026-07 block on
the spec's Axum subsection is preserved as motivation, but the
prescriptive wording is aligned across:
- docs/superpowers/specs/2026-06-23-provision-local.md
- docs/superpowers/plans/2026-06-27-provision-local.md
- docs/guide/configuration.md
- docs/guide/getting-started.md
- docs/guide/cli-reference.md
- docs/guide/cli-walkthrough.md
- docs/guide/kv.md
- docs/guide/manifest-store-migration.md
- docs/guide/adapters/{cloudflare,fastly,spin}.md
## 3. Dry-run copy treated FIFOs / sockets / device files as copyable
A previous refactor (commit 19c2180) replaced
`file_type.is_file()` with `!file_type.is_dir() && !file_type.is_symlink()`
to satisfy `clippy::filetype_is_file` structurally, but that
predicate also returns true for FIFOs, sockets, and block/character
devices — for which fs::copy will hang indefinitely or error.
Revert to `file_type.is_file()` — which IS the intended
symlink-safe positive form here because `read_dir` yields
FileType from symlink_metadata (never follows symlinks). Wrap
the tiny helper in a #[expect(clippy::filetype_is_file)] with a
reason line that pins the invariant ("read_dir → symlink_metadata").
Adds a Unix regression test that mkfifos into the source tree
and asserts (a) copy_dir_recursive doesn't hang and (b) the FIFO
is not reproduced in the staged dest. mkfifo is invoked via
Command::new (no new libc dep).
…src/bin + docs Reviewer verified after the prior batch (0703a3f) that only Axum was correctly single-source; Cloudflare / Fastly / Spin still had scaffold templates registered, so clean-clone provision produced different bytes than `edgezero new`. Spin was the most acute: its template used the adapter crate name (`<app>-adapter-spin`), but the synth defaulted to the top-level `[app].name` — so a clean clone's regenerated `spin.toml` pointed at `<app>.wasm`, an artifact Cargo never produced under a `<app>-adapter-spin` package. ## 1. All five adapter manifests are now truly single-source Removed the scaffold .hbs templates and file specs for `wrangler.toml`, `fastly.toml`, `spin.toml`, and `runtime-config.toml` — matching the fix applied to `axum.toml` in 0703a3f. Each adapter's `synthesise_baseline_manifest` is now the single writer, invoked by scaffold-time provision AND clean-clone provision, so both paths produce byte-identical output. Synths were extended to match the pre-2026-07 scaffold template content: - **Spin** (`synthesise_spin_toml`): unset `component` now derives `<app>-adapter-spin` (scaffold-convention crate name) instead of falling back to bare `app_name`. That's the fix for the wrong wasm path. Also folds in `allowed_outbound_hosts = ["https://*:*"]` (Spin defaults outbound HTTP to deny-all) and `[component.<id>.build]` (so `spin build` still knows how to compile the crate). `runtime-config.toml` synth is unchanged (header-only) — the template's `[key_value_store.app_config]` block was defensive dead code for the commented-out `[stores.config]` case; enabling that store adds the block via provision's merge path either way, so removing the template loses nothing. - **Cloudflare** (`synthesise_wrangler_toml`): `name` field now spells `<app>-adapter-cloudflare` (was bare `app_name`) so wrangler builds the crate it actually finds. `[build]` table with `command = "worker-build --release"` added so `wrangler deploy` still knows how to compile. - **Fastly** (`synthesise_fastly_toml`): `name` field now spells `<app>-adapter-fastly` (was bare `app_name`) so `fastly compute` builds the crate it actually finds. Also adds `authors = [""]` (scaffold parity — the fastly CLI requires the key present). Synth tests updated to reflect the new naming; the escaping tests still cover the pathological-app-name path. **Regression:** `generate_new_provisions_cloudflare_and_spin_scaffold_artifacts` extended with `assert_regenerated_manifests_match_baseline` which snapshots every generated manifest, deletes it, re-runs `provision --local` for that adapter, and asserts byte-equality. That's the check that would have caught the prior divergence. ## 2. Scaffold .gitignore re-includes Cargo src/bin dirs The scaffold template's broad `bin/` pattern (line 2) was blocking Cargo's `src/bin/` directories — the same bug the repo-root `.gitignore` fixes with `!**/src/bin/` + `!**/src/bin/**`. Copied the two re-include lines into `gitignore.hbs`. Regression test `assert_scaffold_src_bin_not_ignored` initialises a temporary git repo inside the generated project, plants `crates/demo-app-cli/src/bin/tool.rs`, and asks `git check-ignore -v` whether it's ignored. Parses the matched rule to distinguish ignore matches from re-include matches (exit 0 covers BOTH — the distinction is the `!` prefix on the printed pattern). Bypasses the test-only git PATH stub by capturing the real git binary path BEFORE the stub is installed. ## 3. Final docs sweep Reviewer flagged three files with stale "axum.toml stays tracked" wording. Also folded in the plan / spec's migration runbook updates so all five manifests are covered: - docs/guide/blob-app-config-migration.md - docs/guide/cli-reference.md (two occurrences: `--local` flag docs + `--local` behaviour table row for axum) - docs/superpowers/specs/2026-06-23-provision-local.md (migration runbook `git rm --cached` sed pattern now includes `axum`) - docs/superpowers/plans/2026-06-27-provision-local.md (Task 32 gitignore assertion list, Task 33 migration runbook, Task 37 CI gate description + regex, plus the `default_adapter_manifest_for` snippet now includes an axum arm)
…er-<id>) Reviewer verified that the prior single-source fix (5e19f4f) still mispointed on any project that renamed its adapter crate. Given [adapters.axum.adapter].crate = "crates/server" [adapters.axum.adapter].manifest = "crates/server/axum.toml" # crates/server/Cargo.toml [package] name = "server" ...clean-clone `provision --local` still wrote `crate = "demo-app-adapter-axum"` because the synth derived the name from the scaffold convention `<app>-adapter-<id>` instead of reading the adjacent Cargo.toml. Same story for Spin, where the resulting `source = ".../demo_app_adapter_spin.wasm"` pointed at an artefact Cargo never produced under the `server` package. Not legacy support — the [adapters.<name>.adapter].crate contract IS the operator-facing knob for adapter-crate placement, and it must round-trip through the synthesiser. ## Fix New helper `cli_support::read_adapter_crate_name` reads `<manifest_root>/<adapter_manifest_path>/../Cargo.toml` `[package].name`. Each adapter's `synthesise_baseline_manifest` in cli/mod.rs threads that value into the synthesiser call: cli_support::read_adapter_crate_name(manifest_root, adapter_manifest_path) .unwrap_or_else(|| format!("{app_name}-adapter-<id>")) Fall-through fallback stays for the first-run scaffold path where the adapter Cargo.toml isn't on disk yet — the scaffold-time provision loop runs BEFORE the crate files land, so first-generation of e.g. `axum.toml` still succeeds. Every non-scaffold path (clean clone, operator-renamed crate) picks up the real name. The three synth signatures (`synthesise_wrangler_toml`, `synthesise_fastly_toml`, `synthesise_spin_toml`) rename their `app_name` parameter to `crate_name` and drop their internal convention fallbacks — that logic now lives in one place (the mod.rs caller). The Spin synth also drops the awkward `derived_component` scoping since `component.unwrap_or(crate_name)` is now the pure form. ## Regression tests - `cli_support::tests`: three new tests for `read_adapter_crate_name` (happy path, missing Cargo.toml, None manifest path). - `edgezero-adapter-axum` provision_local: `synthesised_axum_toml_honors_renamed_adapter_crate` — plants `crates/server/Cargo.toml` with `name = "server"` and asserts the synthesiser emits `crate = "server"` (not `demo-app-adapter-axum`). - `edgezero-adapter-spin` provision_local: `synthesised_spin_toml_honors_renamed_adapter_crate` — plants `crates/spin-server/Cargo.toml` with `name = "spin-server"` and asserts `[application].name = "spin-server"`, the trigger component id, and the underscored wasm path `spin_server.wasm`. - New synth-level test `synthesises_spin_toml_uses_renamed_crate_name` pins the synth half of the invariant. ## Docs - `docs/superpowers/specs/2026-06-23-provision-local.md`: Spin <component_id> resolution now describes the three-step precedence ([adapters.spin.adapter].component → Cargo.toml [package].name → scaffold convention) and calls out why the pre-2026-07 fallback to bare `<app_name>` broke renamed adapter crates. - `docs/superpowers/plans/2026-06-27-provision-local.md`: Task 41's test list renames `provision_local_does_not_touch_axum_toml` to `provision_local_preserves_existing_axum_toml` (the actual name in the tree) and drops the "Axum exception" wording. - `crates/edgezero-adapter/src/registry.rs` `synthesise_baseline_manifest` doc: all four bundled adapters now override this hook; the "Axum has no synthesised local state" carve-out is gone. All local CI gates pass: fmt, clippy -D warnings, wasm32-wasip2 check for Spin, workspace tests.
…al-impl # Conflicts: # .github/workflows/test.yml # crates/edgezero-adapter/src/registry.rs # crates/edgezero-core/src/manifest.rs
…from wasm basename Two remaining review findings on top of 2bb6487 (which established the read-Cargo.toml-not-scaffold-convention behaviour): ## 1. read_adapter_crate_name walks up nested paths Previous shape only checked the manifest's immediate parent for Cargo.toml. Spec §"[adapters.<name>.adapter]" allows the adapter manifest to resolve anywhere inside the adapter crate — including nested paths like `crates/server/config/axum.toml` where the Cargo.toml sits one directory above the manifest. The helper now walks upward from the manifest's parent, taking the first Cargo.toml it finds. Bounded at manifest_root (inclusive) so we never leak up into the user's home directory or workspace parents when no Cargo.toml exists inside the project. Regression tests: - `read_adapter_crate_name_walks_up_to_nested_manifest_crate_root`: seeds `crates/server/Cargo.toml` with `name = "server"` and passes `crates/server/config/axum.toml`; asserts `Some("server")`. - `read_adapter_crate_name_stops_at_manifest_root`: no Cargo.toml under tempdir; asserts `None` (must not walk above manifest_root). - Per-adapter nested integration tests: `synthesised_axum_toml_honors_renamed_adapter_crate_with_nested_manifest`, `synthesised_wrangler_toml_honors_renamed_adapter_crate` (nested), `synthesised_fastly_toml_honors_renamed_adapter_crate` (nested), `synthesised_spin_toml_honors_renamed_adapter_crate_with_nested_manifest`. ## 2. Spin: decouple component id from wasm artifact basename Previously `synthesise_spin_toml` used `component.unwrap_or(crate_name)` and then derived the wasm source path from that single value. That broke when the operator set `[adapters.spin.adapter].component = "worker"` on a Cargo package named `spin-server`: the synthesiser emitted `source = ".../worker.wasm"` while Cargo actually produced `spin_server.wasm`, and Spin failed at startup with a missing-artifact error. The two identities are runtime-distinct: - `[application].name` and the wasm source basename follow the CARGO PACKAGE NAME (`<crate_name_underscored>.wasm`). - `[[trigger.http]].component` and `[component.<id>]` table key follow the SPIN COMPONENT SELECTOR (or default to the crate name when unset). Regression test: - `synthesised_spin_toml_component_selector_does_not_leak_into_wasm_basename`: crate `spin-server` + component selector `"worker"` → `[[trigger.http]].component = "worker"`, `[component.worker]`, BUT `source = ".../spin_server.wasm"` and `[application].name = "spin-server"`. - Existing `synthesises_spin_toml_honors_component_selector` updated to assert both invariants (selector reroutes trigger/section keys; wasm basename stays crate-anchored). ## Spec update `docs/superpowers/specs/2026-06-23-provision-local.md` Spin subsection rewritten to describe the two-identity model (`<crate_name>` vs. `<component_id>`) and pin the wasm basename to the Cargo package name. All local CI gates pass: fmt, clippy --all-features -D warnings, workspace tests, Spin wasm32-wasip2 check.
…odel Three doc-only findings from the 39bcb5e review pass: ## cli-reference.md `--local per-adapter behaviour` table The prior single-column table conflated two entry points: - **Base**: bundled `edgezero provision --local` (no downstream typed `AppConfig`) — synthesises adapter manifests, merges per-store bindings, and writes the runtime env-label lines. - **Typed**: downstream `<app>-cli provision --local` (routes through `run_provision_typed::<AppConfig>`) — runs base first, then additionally emits `#[secret]` / `[variables]` / `SPIN_VARIABLE_*` placeholder lines derived from the typed `AppConfig`. Split into two columns and prefaced with a paragraph naming the distinction, so operators running the bundled binary don't expect placeholder emission that only the generated CLI can produce. ## plan Task 24 (Spin synth) Both the test fixtures (line 3496) and the implementation sketch (line 3529) still encoded the pre-2026-07-v3 model where `component_id.unwrap_or(app_name)` drove BOTH the trigger keys AND the wasm source basename. Under that model, setting `[adapters.spin.adapter].component = "worker"` on a Cargo package named `spin-server` produced `source = ".../worker.wasm"`, while Cargo actually built `spin_server.wasm` — the fix now landed as 39bcb5e in the implementation. Rewrote the Task 24 code+tests to: - Take `crate_name` (from adapter-crate Cargo.toml `[package].name` via `cli_support::read_adapter_crate_name`), NOT `app_name`. - Emit `[application].name = crate_name` and `source = ".../<crate_name_under>.wasm"` regardless of the component selector. - Emit `[[trigger.http]].component` and `[component.<id>]` from the component selector (or `crate_name` when unset). ## plan Task 37 CI gate wording Line 4895/4916 still referred to "NOT axum.toml" and an "axum.toml exempt" commit message, but the actual workflow gate that landed at Task 37 correctly includes `axum.toml` in the regex (the 2026-07 amendment folded axum.toml into the gitignored set). Updated the Step 3 header, sanity-check description, and commit message so future readers of the plan see the current enforcement scope.
…d .env, Axum remnants Four doc-only findings from the a1e000e review pass: ## Spec: primitive-synth output names come from <crate_name> The 'Primitive synthesiser output' section (§) still described <app_name> from [app].name and used name = "<app_name>" in the Cloudflare/Fastly/Spin example blocks. Current code (39bcb5e) derives these from the adapter crate's Cargo.toml [package].name via cli_support::read_adapter_crate_name, with <app>-adapter-<id> only as a fallback when no Cargo.toml is reachable. Rewrote the section to define <crate_name> as the adapter-crate Cargo package name (with the upward-walk semantics for nested manifests), name the fallback contract, and reference the existing Spin two-identity subsection for the runtime split. ## Plan Section 5 (Spin override) uses read_adapter_crate_name Step 5's Spin synthesise_baseline_manifest snippet still passed app_name directly to synthesise_spin_toml(...). An implementer following the plan verbatim would reintroduce the exact component-drives-wasm-basename bug 39bcb5e fixed. Rewrote the snippet to resolve crate_name via read_adapter_crate_name(...) with the scaffold-convention fallback and thread THAT into the synth. ## configuration.md .env files: base vs typed split The 'Where does each setting live?' bullet for .edgezero/.env (Axum) + <spin_crate>/.env (Spin) said 'provision --local appends #[secret] placeholder lines' — same conflation the cli-reference table already fixed. Split into a two-sub-bullet base/typed description so operators know the bundled binary writes only the env-label lines and only <app>-cli emits secret placeholders. ## Plan: 'Axum keeps the default synthesiser' + migration regex - Task 8 step: 'Axum keeps the default' → all four bundled adapters override synthesise_baseline_manifest per the 2026-07 amendment. - Task 33 migration regex: '(fastly|spin|wrangler|runtime-config)' → '(axum|fastly|spin|wrangler|runtime-config)' to match the Task 37 CI gate. Also updated the surrounding comment, Step 3 verify (now includes app-demo-adapter-axum), and Step 5 commit message. All docs prettier-clean.
… + typed dry-run gates Four code fixes + one doc sweep for the review pass on 6589957. ## H1: Strict-local requires [adapters.<name>.adapter].manifest Previously `assert_provision_paths_contained` only enforced "manifest inside crate" when BOTH `.crate` AND `.manifest` were set. A hand-authored adapter with `crate = "crates/server"` and no `.manifest` fell through — the synth's `PathBuf::from("<default>.toml")` fallback wrote generated manifests at the PROJECT root (outside the crate dir), and `read_adapter_crate_name` couldn't walk up to the crate's Cargo.toml (it has no manifest path to start from). Both are the exact containment leak the strict-local model was supposed to close. Fix: strict-local now requires `.manifest`. Cloud (`assert_provision_paths_safe`) still permits missing `.manifest` for adapters that manage manifests via their vendor CLI. Regression tests: - `rejects_missing_manifest_when_crate_declared_in_local_mode` - `rejects_missing_manifest_even_without_declared_crate_in_local_mode` - `accepts_manifest_without_crate_declaration_in_local_mode` - `safe_variant_still_allows_missing_manifest` Existing test fixtures updated to declare `.manifest` inside the crate dir (`PROVISION_MANIFEST` in test_support.rs, the mixed-case-adapter-key fixture in provision.rs, and the sibling-adapter traversal fixture in config.rs). ## H2: run_config_diff_typed runs the containment loop `run_config_push_typed` (config.rs:356) already looped over all adapters and called `assert_provision_paths_contained` / `assert_provision_paths_safe` before shared checks and adapter dispatch. `run_config_diff_typed` only ran `run_shared_checks`, skipping containment on the argument that a read-only command can't corrupt the tree. But the adapter's `read_config_entry` / `read_config_entry_local` STILL joins `manifest_root` with `[adapters.<name>.adapter].manifest` and reads (and prints) the resulting file. Without the guard a poisoned `manifest = "../../etc/shadow"` surfaces as a diff line rather than an early error. Fix: copied the push loop into `run_config_diff_typed` before `run_shared_checks`. Spec §"Path containment (MUST)" also updated so the "read-only stays unchecked" bullet no longer excuses diff. ## H3: Dry-run redacts .env / .dev.vars values `context_radius(2)` scoped context to two lines around each hunk, but if provision appended new lines at EOF, the LAST two pre-existing lines still surfaced as unchanged context — leaking real operator secrets from `.dev.vars` / `.env` into CI logs, screen recordings, and terminal scrollback. Fix: for env-shaped paths (`.env`, `.dev.vars`), rewrite each `KEY=value` line as `KEY=<redacted>` on BOTH sides BEFORE diffing. Comment lines and blank lines pass through so structural drift (added/removed keys) is still visible. When the redacted bodies match (pure value churn), the renderer emits a single redacted-header line instead of a value-carrying diff hunk. Regression tests: - `dry_run_diff_redacts_env_values_and_never_leaks_context`: seeds an `.edgezero/.env` with a real-shaped secret value on both sides, asserts the value never appears in the report and both existing + newly-appended lines surface in redacted form. - `dry_run_diff_emits_redacted_placeholder_when_only_values_changed` seeds two different secret values on the same key, asserts neither leaks and the header names the redaction reason. - `dry_run_diff_leaves_non_env_files_unredacted`: proves the filter is scoped to `.env` / `.dev.vars` only. - `redact_env_body_preserves_comments_and_blank_lines`: pins the line-by-line filter shape. ## M1: Typed local dry-run runs full manifest shape gates `run_provision_typed` dry-run only called `enforce_single_store_capability` + `strict_handler_paths`. Real typed runs eventually call `run_provision_inner`, which uses the full `run_manifest_shape_gates` (adds `validate_deployed_field_ownership`). Dry-run could therefore accept a manifest that the real run rejects. Fix: swapped the two individual calls for `run_manifest_shape_gates(ctx.manifest(), &args.adapter)` so dry-run and real-write share the same preflight surface. ## L1: Migration bullets include axum.toml Spec §"Migration for the in-tree examples/app-demo/" and plan Task 33's file list dropped `axum.toml` before it was folded into the gitignored set. Added the axum.toml bullet to both so the runbook matches the CI gate + the .gitignore. Also noted that `axum.toml` has NO scaffold `.hbs` template so scaffold-time provision is the single writer. All CI gates locally clean: fmt, clippy -D warnings, workspace tests (all 1516 pass), Spin wasm32-wasip2 check.
…st+crate Three follow-ups on top of 7a92e57: ## H: Commented KEY=value env lines still leaked The pre-fix filter treated ANY `#`-starting line as a pure comment and passed it through verbatim. But: - The adapter provisioners emit commented placeholder env entries (e.g. `# EDGEZERO__STORES__CONFIG__APP_CONFIG__KEY=` in `edgezero-adapter-axum/src/cli/provision_local.rs`). - Operators frequently stash real values behind a leading `#` for later use. - `edgezero-adapter/src/env_file.rs`'s dedup helper treats single-hash `# KEY=value` as a real commented env entry, i.e. part of the secret-carriage surface. Fix: extend the redactor to also match `#[whitespace]?KEY=value` and rewrite it as `#[whitespace]?KEY=<redacted>`. The leading indent + `#` shape is preserved via char-by-char iteration so the diff still renders the line as a comment. Pure comments (no `=` after the `#`) still pass through unchanged. Regression: `redact_env_body_redacts_commented_key_value_lines` seeds all three shapes — `# KEY=value`, `#KEY=value` (no space), and ` # KEY=value` (indented) — and asserts NONE of the raw values survive. Also pins the pure-comment banner `# edgezero-provision: v1` as unchanged. ## M: Strict-local now requires BOTH .manifest AND .crate Previously the guard required `.manifest` but tolerated a missing `.crate`. That meant `[adapters.cloudflare.adapter].manifest = "wrangler.toml"` was still legal for `--local`: containment couldn't be proven, generated wrangler.toml landed at the project root, and `read_adapter_crate_name`'s upward walk had no crate-root anchor. Fix: strict-local rejects a missing `.crate` outright — the same way it already rejects a missing `.manifest`. Cloud (`assert_provision_paths_safe`) still permits both fields absent for adapters that manage manifests via their vendor CLI. All four bundled adapters declare both fields in generated projects; there is no legacy case that legitimately omits `.crate` in local mode. Regression: `rejects_missing_crate_when_manifest_declared_in_local_mode`. Renamed `accepts_manifest_without_crate_declaration_in_local_mode` (the incorrect prior-behaviour lock-in) to the negative case, and extended `safe_variant_still_allows_missing_manifest_and_missing_crate` to prove cloud dispatch keeps the permissive shape. Existing test at `path_safety::tests::accepts_empty_root_string_as_dot` updated to pass `Some("crates/edgezero-adapter-spin")` for the crate arg per the new both-required rule. ## L: Spec path-safety snippet + containment promise updated `docs/superpowers/specs/2026-06-23-provision-local.md`: - The 'Never creates files outside the adapter crate' bullet now spells out that `--local` REQUIRES both `.crate` and `.manifest`, and calls out the no-legacy stance. - The embedded path-safety code snippet rewritten to reflect the new hard cutoff: distinct errors for missing `.manifest`, missing `.crate`, and manifest-outside-crate. The pre-fix 'when both are set' + 'crate = None is legal for Axum' wording is gone. All CI gates locally clean: fmt, clippy -D warnings, workspace tests (1516 pass), Spin wasm32-wasip2 check.
… writeback path)
Reviewer regression on nested Spin manifests:
- `Spin provision --local` writes .env next to the RESOLVED spin.toml
(`edgezero-adapter-spin/src/cli/provision_local.rs`:
`env_path = spin_dir.join(".env")` where
`spin_dir = manifest_root/adapter_manifest_path.parent()`).
- `run_serve` used to derive Spin's .env from
`[adapters.spin.adapter].crate` -> `<crate>/.env`.
With `[adapters.spin.adapter]`:
crate = "crates/spin"
manifest = "crates/spin/config/spin.toml"
provision writes `crates/spin/config/.env` while serve loaded
`crates/spin/.env`. Spin thus started without the runtime env
labels (`EDGEZERO__STORES__...__NAME`) and typed
`SPIN_VARIABLE_*` placeholders provision had just generated.
Spec §"Adapter-scoped env-file load" already prescribes the
correct derivation (line 1639-1641: "the parent of
`[adapters.spin.adapter].manifest`, defaulting to the in-tree
per-adapter crate when unset"). Code was lagging the spec.
Fix: `resolve_serve_env_file`'s spin arm now derives `.env` from
`adapter.manifest.parent()` when set, falling back to
`adapter.crate` and then `manifest_root` for out-of-tree
fixtures that predate the 2026-07 both-required strict-local
rule. That fallback is a strict superset of the pre-fix
resolution, so the existing SPIN_MANIFEST_LOWER /
SPIN_MANIFEST_MIXED_CASE fixtures (both declare only .crate) keep
resolving to `<crate>/.env`.
Regression test:
- `resolve_serve_env_file_spin_honors_nested_manifest_parent`:
seeds `SPIN_MANIFEST_NESTED` with
`manifest = "crates/spin/config/spin.toml"` and asserts
`resolve_serve_env_file` returns
`crates/spin/config/.env` — matching where provision writes.
Existing 5 Spin/Axum/Cloudflare/Fastly/case-insensitive tests
continue to pass unchanged.
All CI gates locally clean: fmt, clippy --workspace --all-targets
--all-features -D warnings, workspace tests (1517 pass).
Two review items on top of c38cb54. ## M — Path-safety guard before adapter env-file read `resolve_serve_env_file` joins `manifest_root` with `[adapters.spin.adapter].manifest` and `run_serve` reads the resulting `.env` if it exists. Core manifest validation only length-checks the manifest string, so a poisoned `manifest = "/etc/spin.toml"` or `manifest = "../../../secrets/env"` would resolve to an out-of-tree `.env` — and `env_file::load_into_process_env` would happily read it and inject its `KEY=VALUE` lines into the process env, which the spawned adapter would then inherit. Provision and config already run `assert_provision_paths_safe` for this exact class of manifest-declared path resolution (`path_safety.rs`). `run_serve` now runs the same guard BEFORE the resolve-and-read step, matching the third-party surface `run_config_diff_typed` also grew in v7a92e57. Regression tests: - `run_serve_rejects_absolute_spin_adapter_manifest_path`: poisons `manifest = "/etc/spin.toml"`; asserts the "must be a project-relative path" error surfaces BEFORE the read. - `run_serve_rejects_parent_traversal_in_spin_adapter_manifest`: poisons `manifest = "../../../outside/spin.toml"`; asserts the "must not contain `..` traversal" error surfaces before the read. ## L/M — Contract test that run_serve loads the .env before spawn Spec §"Adapter-scoped env-file load" (line 1654) prescribes a contract test: run `edgezero serve --adapter spin`, intercept the spawned child env, and prove the Spin `.env` value reaches it. Prior tests only exercised `resolve_serve_env_file(...)` in isolation — the resolver's output was correct but nothing proved `run_serve` actually loaded the file before dispatching to the adapter's serve command. New test `run_serve_loads_env_file_into_process_env_before_spawning_child`: - Seeds a manifest with a nested `manifest = "crates/spin/config/spin.toml"`. - Writes `crates/spin/config/.env` with a unique marker (`EDGEZERO_TEST_SERVE_ENV_LOADED_MARKER=spin-nested-manifest-parent`). - Also writes a DECOY `crates/spin/.env` with a different value so a regression to the pre-c38cb54 crate-based lookup would surface as a wrong-value assertion (not a silent pass). - Runs `run_serve` (serve command is `echo` — the manifest `[adapters.spin.commands].serve = "echo"` makes the actual child spawn trivial and hermetic). - Reads `env::var(marker_key)` and asserts it equals the nested-parent `.env` value. The test is Unix-only (`#[cfg(not(windows))]`) because it depends on `echo` being on PATH — same convention the existing `run_serve_executes_manifest_command` test uses. All CI gates locally clean: fmt, clippy `--all-features -D warnings`, workspace tests (1519 pass — 3 new here), Spin wasm32-wasip2 check.
…nt's Test fidelity gap on top of 57826a9. The previous version checked `env::var(marker_key)` in the PARENT process after run_serve returned — that proves run_serve loaded the .env into the parent's env, but not that the spawned serve child actually inherited the value before executing. Spec § "Adapter-scoped env-file load" (line 1654) asks specifically for intercepting the spawned child's env. The implementation order in `run_serve` was already correct (load .env into process env BEFORE `adapter::execute` spawns the child, see lib.rs:211), so this is a test-fidelity fix, not a runtime bug. Fix: make the serve command a small `sh -c` script that writes the CHILD'S OWN observed value of the marker to a file inside the tempdir. The test reads that file after run_serve and asserts the child saw the expected value. serve = 'sh -c "printf %s \"\${MARKER:-<unset>}\" > <path>"' Now the test has TWO assertions: 1. Parent-side (necessary): `env::var(marker_key)` on the current process returns `Some(marker_value)` — proves run_serve populated the parent env before dispatch. 2. Child-side (the spec's real ask): the file the child wrote contains `marker_value` (not `<unset>`, and not the decoy `crates/spin/.env` value) — proves the spawned process inherited the env at execution time. If a regression skipped the .env load, the parent env stays unset, the child inherits nothing, `printf \"\${MARKER:-<unset>}\"` writes literal `<unset>` to disk, and the child-side assertion fails with a useful diff. If a regression restored the pre-c38cb54 crate-based lookup, the child inherits the DECOY value ("THIS_MUST_NOT_BE_LOADED_from_crate_root") and again the assertion fails loud. All CI gates locally clean: fmt, clippy --workspace --all-targets --all-features -D warnings, workspace tests (1517 pass — the same 228-test edgezero-cli set, updated contract test).
aram356
marked this pull request as ready for review
July 8, 2026 22:09
aram356
changed the title
prk-Jr
requested changes
Jul 9, 2026
prk-Jr
left a comment
prk-Jr
left a comment
Contributor
There was a problem hiding this comment.
📝 ## PR Review
Summary
This is a thorough implementation with strong adapter-specific coverage, dry-run fidelity tests, and clear ownership boundaries. I found three blocking safety gaps: lexical containment can be bypassed through symlinks, run_serve mutates the process environment, and config validate remains outside the new path-safety perimeter.
😃 Praise
- 😃 The dry-run renderer redacts both sides before diffing and handles commented assignments plus value-only changes without leaking secret values.
Findings
Blocking
- 🔧 Apply path safety to
config validate: Bothrun_config_validateandrun_config_validate_typedcallrun_shared_checks, which reachesrun_adapter_shared_checksand invokesvalidate_adapter_manifestwithout first applyingassert_provision_paths_safe. Spin's validator then readsmanifest_root.join(adapter_manifest_path), so an absolute or traversing path rejected by provision/push/diff remains readable through validation (crates/edgezero-cli/src/config.rs:1421). Centralize the safe variant immediately before each adapter validator; local push can retain its stricter contained check.
Non-blocking
- 🤔 Document Axum's generated-manifest workflow:
docs/guide/adapters/axum.mdstill presentsaxum.tomlas an ordinary project file without explaining that it is now gitignored and absent after a fresh clone. Add the regeneration command (<app>-cli provision --adapter axum --local) near the manifest description (docs/guide/adapters/axum.md:201).
CI Status
- 📝 fmt: PASS
- 📝 clippy: PASS
- 📝 workspace tests: PASS
- 📝 feature compilation: PASS
- 📝 Spin wasm32-wasip2 check: PASS
- 📝 GitHub checks: PASS
| ("[adapters.<name>.adapter].crate", adapter_crate_path), | ||
| ] { | ||
| let Some(raw) = maybe_raw else { continue }; | ||
| let candidate = Path::new(raw); |
Contributor
There was a problem hiding this comment.
🔧 Symlinks bypass the containment guarantee: these checks are purely lexical. For example, crate = "crates/worker" passes when crates/worker is a symlink to a directory outside the project; the subsequent adapter fs::write calls follow that symlink and mutate the external target. Dry-run makes this harder to notice because copy_dir_recursive deliberately skips symlinks, so its staged result differs from the real write path.
Please canonicalize and verify each existing path component (or reject symlink components) before writing. For complete TOCTOU protection, perform directory-relative opens/writes beneath a trusted root rather than checking a path and reopening it by name.
Contributor
Author
There was a problem hiding this comment.
Resolved in 44c9a2d (crates/edgezero-cli/src/path_safety.rs).
assert_provision_paths_impl now walks each component of the resolved path from project_root inward via new reject_symlink_components helper. Any existing intermediate whose symlink_metadata reports is_symlink() == true is rejected before dispatch. Broken/dangling symlinks are caught too (they still report is_symlink() == true). Applies to both the safe and strict-local variants — symlink following would let cloud dispatch read from outside the project as well.
Four regression tests (Unix, std::os::unix::fs::symlink): rejects_symlinked_adapter_crate_directory, rejects_symlink_in_manifest_intermediate_component, safe_variant_also_rejects_symlinked_paths, accepts_regular_directory_and_first_run_missing_paths (proves NotFound intermediates for first-run scaffold still pass).
Documented as best-effort against current filesystem state, not full TOCTOU protection — openat + O_NOFOLLOW threaded through every adapter is flagged as follow-up scope in the code comment.
| .strip_prefix('"') | ||
| .and_then(|stripped| stripped.strip_suffix('"')) | ||
| .unwrap_or(trimmed_val); | ||
| env::set_var(key, value); |
Contributor
There was a problem hiding this comment.
🔧 Do not mutate the process environment from this library path: on Unix, setenv/getenv access is not thread-safe, and this crate cannot guarantee that no other thread is reading the environment. The repository's own shared_test_guards.rs explicitly documents this constraint, while run_serve can be called from a multithreaded downstream process.
Parse the file into an owned env overlay and apply it only to the spawned child with Command::envs. The existing adapter::run_shell path already builds a Command; registered-adapter execution should receive the same overlay through an execution context rather than process-global mutation. Validate keys and values while parsing so malformed NUL/= input returns an error instead of panicking.
Contributor
Author
There was a problem hiding this comment.
Resolved in c38cb54 → 7d6d886 → e0688aa (crates/edgezero-cli/src/env_file.rs, adapter.rs, lib.rs).
env_file::load_into_process_env (which called std::env::set_var) removed. Replaced by env_file::parse_env_overlay which returns an owned Vec<(String, String)> — no process-env mutation, no shared environ access, safe from any multithreaded caller. The parser also rejects lines whose key contains NUL or =, and values that contain NUL (both are Command::env contract violations that would otherwise panic or misroute downstream).
run_serve threads the overlay into the spawned child via new adapter::execute_with_env_overlay → Command::env per entry. Each Command::env call stores the entry in the Commands private map — no shared state, no setenv. Existing-env-wins preserved by checking std::env::var_os before each set (mirrors the manifest [environment.variables] block and the adapter bind hint).
Three parser tests + three integration tests:
env_file::tests::parses_key_value_lines_into_ordered_overlay(pure parser)env_file::tests::rejects_nul_byte_in_key/rejects_nul_byte_in_value(Command::env contract)env_file::tests::missing_file_yields_readable_errorrun_serve_does_not_mutate_parent_process_env_yet_child_sees_env_file_value(the load-bearing thread-safety assertion — child observes the marker via a file it writes; parent env asserted to beNoneafterrun_servereturns)run_serve_existing_parent_env_wins_over_env_file(opposite contract — parents KEY wins over the .env)
| // through as-is so structural drift is still visible. | ||
| let is_env_like = is_env_secret_carriage_path(proj_path); | ||
| let old_render = if is_env_like { | ||
| redact_env_body_for_diff(&old) |
Contributor
There was a problem hiding this comment.
😃 Good secret-handling boundary: redacting both the old and staged bodies before constructing the diff prevents unchanged context lines from leaking operator values. The separate value-only-change marker also preserves useful signal without exposing keys or values.
ChristianPavilonis
requested changes
Jul 9, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Contributor
There was a problem hiding this comment.
Summary
Requesting changes. The implementation has strong coverage and all current checks pass, but the inline findings identify three high-impact correctness/security regressions plus dry-run, nested-layout, and public-API compatibility gaps. I have not repeated the symlink-containment, process-global environment, validation-ordering, or Axum-documentation findings from the existing review.
…verlay via Command::env, axum docs All four findings from #287 review (@prk-Jr) addressed. ## Blocking #1 — config validate outside path-safety perimeter `run_config_validate` / `run_config_validate_typed` called `run_shared_checks` -> `run_adapter_shared_checks` which invoked `adapter.validate_adapter_manifest(manifest_root, path, ...)` without `assert_provision_paths_safe`. Spin's validator then reads `manifest_root.join(adapter_manifest_path)`, so a poisoned `manifest = "/etc/spin.toml"` would surface that file's contents as the validation error message. Fix: call `assert_provision_paths_safe` per-adapter INSIDE `run_adapter_shared_checks`, before each adapter's `validate_adapter_manifest`. This makes validate share the same minimum-bar guard push/diff/provision already run; local push/diff still layer their stricter `contained` variant on top. Regression tests: - `raw_rejects_absolute_adapter_manifest_path_before_adapter_validators_run` - `raw_rejects_parent_traversal_adapter_manifest_before_adapter_validators_run` ## Blocking #2 — Symlink bypass in lexical containment The lexical guard let `crate = "crates/worker"` pass when `crates/worker` is a symlink to a directory outside the project; subsequent adapter `fs::write` calls followed the symlink and mutated the external target. Dry-run `copy_dir_recursive` deliberately skips symlinks, so its staged result differed from the real write path — the operator wouldn't notice. Fix: new `reject_symlink_components(start, candidate, label)` helper walks each component of the resolved path and rejects any existing intermediate that `symlink_metadata` reports as `is_symlink()`. `NotFound` intermediates (first-run scaffold where the adapter crate isn't materialised yet) are fine — the adapter creates them from EdgeZero-owned code. Missing directories along the way are ok; a broken symlink still reports `is_symlink() == true`, so dangling links get caught too. Fires on BOTH the safe and strict-local variants; symlink following would let cloud dispatch read from outside the project too. Note: this closes the ambient-file surface but is not a full TOCTOU guard. A concurrent attacker who plants the symlink between check and adapter write can still race. Fully closing that requires directory-relative opens (`openat` + `O_NOFOLLOW`) threaded through every adapter — flagged as follow-up scope. Regression tests (Unix-only, use `std::os::unix::fs::symlink`): - `rejects_symlinked_adapter_crate_directory` - `rejects_symlink_in_manifest_intermediate_component` - `safe_variant_also_rejects_symlinked_paths` - `accepts_regular_directory_and_first_run_missing_paths` ## Blocking #3 — run_serve mutated process env `env_file::load_into_process_env` wrote every KEY=VALUE via `std::env::set_var`. On Unix, `setenv`/`getenv` operate on the shared `environ` array without synchronisation — the C standard flags concurrent access as UB. A multithreaded downstream process calling `edgezero_cli::run_serve` from one thread while another thread reads `std::env::var` observes a torn read. Fix: - `env_file::load_into_process_env` -> `parse_env_overlay`, which returns `Vec<(String, String)>`. No process-env mutation. - Parser rejects lines with NUL bytes in the key or value (`Command::env` contract violations) rather than panicking or silently misrouting downstream. - New `adapter::execute_with_env_overlay` accepts the owned overlay; `run_shell` applies it via `Command::env` (per-Command private map — no shared state, no setenv). Existing-env-wins preserved by checking `env::var_os` before each set, same rule as the manifest `[environment.variables]` block. - `run_serve` swaps the load path for `parse_env_overlay` + `execute_with_env_overlay`. Regression tests: - `env_file::tests::parses_key_value_lines_into_ordered_overlay` (pure parser: comments, quotes, indented lines, malformed lines skipped). - `env_file::tests::rejects_nul_byte_in_key` / `rejects_nul_byte_in_value` (Command::env contract). - `env_file::tests::missing_file_yields_readable_error`. - `run_serve_does_not_mutate_parent_process_env_yet_child_sees_env_file_value`: child observes the overlay via a file it writes; the PARENT's `env::var(marker)` is asserted to be None afterward. The load-bearing thread-safety assertion. - `run_serve_existing_parent_env_wins_over_env_file`: opposite contract — parent's KEY wins over the .env's KEY. - `run_serve_loads_env_file_into_process_env_before_spawning_child` (existing) updated: dropped the parent-env-observed assertion (which is now inverted from the new contract); kept the child-observed nested-manifest-parent assertion. The old `load_into_process_env_*` tests in lib.rs are deleted — parser behaviour is now covered in env_file.rs's own module tests, and the process-env observation was the exact pattern this fix removed. ## Non-blocking — Axum docs `docs/guide/adapters/axum.md` presented `axum.toml` as an ordinary project file. Added a warning block under the Configuration section documenting the 2026-07 amendment: it is provision-generated + gitignored, teammates regenerate via `<app>-cli provision --adapter axum --local`, operator edits survive re-run, and the Axum blueprint has no scaffold `.hbs` template so scaffold and clean-clone produce byte-identical output. All CI gates locally clean: fmt, clippy --all-features -D warnings, workspace tests (1521 pass — 4 new symlink + 4 new env-file parser + 2 new run_serve overlay + 2 new config-validate path-guard tests minus 2 obsolete load_into_process_env tests), Spin wasm32-wasip2 check, docs prettier.
…al-impl # Conflicts: # crates/edgezero-adapter-spin/src/cli.rs # crates/edgezero-cli/src/bin/check_no_nested_app_config.rs # crates/edgezero-cli/src/config.rs
…, dry-run diff, ConfigPushArgs source-compat All six findings from the second review pass (@ChristianPavilonis) addressed. ## P1a — Windows path shapes bypass containment `Path::is_absolute()` only catches drive-absolute (`C:\foo`) on Windows and Unix-absolute (`/foo`) on Unix. Two shapes slipped through on every host, regardless of authoring platform: - `\outside\spin.toml` — Windows rooted-without-drive; on Unix it looked like a relative component with embedded backslashes. - `D:outside\spin.toml` — Windows drive-relative (`D:`'s cwd); `Component::Prefix` only fires on Windows. Path safety now rejects any raw string that starts with `\` or `/` and any string matching a byte-level Windows drive prefix (`[A-Za-z]:`), regardless of host OS. Belt-and-braces: `Component::Prefix(_)` is also rejected. 5 new regression tests covering rooted-no-drive, forward-slash rooted, drive-relative, drive-absolute, and a legitimate mid-string-backslash accept case. ## P1b — Cloudflare retry loses existing namespace IDs If attempt 1 creates namespace A then fails on B, the outcome is discarded. On retry, A is skipped (already present in wrangler.toml) but was NOT recorded in `created_kv_ns`, so `ProvisionOutcome.deployed` only carries B. That permanently drops A from `[adapters.cloudflare.deployed].kv_namespaces` in edgezero.toml; teammates cloning fresh silently point at a missing namespace. Fix: when skipping an already-provisioned namespace, ALSO record its id in `created_kv_ns` so the outcome surfaces the full set (existing + newly-created). Regression test `skipped_existing_namespace_ids_still_surface_in_deployed_outcome` seeds a real id and asserts `outcome.deployed.sub_tables ["kv_namespaces"][logical]` matches. ## P1c — Nested Spin manifest emits wrong wasm relative path `synthesise_spin_toml` hard-coded `../../target/...` — correct for the 2-deep scaffold convention `crates/<crate>/spin.toml` but WRONG for nested layouts like `crates/spin-server/config/spin.toml` (needs `../../../target/...`; `../../target/` resolves to `crates/target/`, not the workspace's `target/`). New `workspace_relative_target_prefix(manifest_rel)` computes the prefix from the depth of `manifest_rel.parent()`. The synth signature grows a `manifest_rel: &Path` arg; the caller in `cli/mod.rs` passes `spin_rel` through. Existing tests updated to pass `Path::new("crates/x/spin.toml")` (or the seeded manifest path) to keep the same emitted prefix. Nested-manifest regression test tightened to assert the exact 3-deep `../../../target/...` prefix (previously only checked the basename, masking the bug). ## P2a — Cloud dry-run suppressed the edgezero.toml writeback diff `merge_deployed_into_manifest` returned silently in dry-run. Fastly's cloud dry-run can discover an existing `service_id` via a read-only `fastly service describe`, and the corresponding real run mutates tracked `edgezero.toml` without any preview. Dry-run now renders the original-vs-updated unified diff via `similar::TextDiff` when the doc changes, or logs a "no change" line otherwise. Existing dry-run test still passes (still no file mutation), just now with a log line the operator can read. ## P2b — Nested Axum synth emits crate_dir="." (loader breaks) For `crates/server/config/axum.toml`, the synthesiser found the crate name via the upward walk but still emitted `crate_dir = "."` — which points the axum loader at `crates/server/config/Cargo.toml` (manifest parent), not `crates/server/Cargo.toml` (crate root). `edgezero serve --adapter axum` then errored out with "expected Cargo.toml next to the manifest". New `cli_support::read_adapter_crate_root` returns the DIRECTORY holding the discovered Cargo.toml (sibling of the existing `read_adapter_crate_name`). `derive_axum_crate_dir` in `edgezero-adapter-axum/src/cli/mod.rs` computes the relative path from the manifest's parent to the crate root — `"."` for scaffold-convention, `".."` (or deeper) for nested. The synth gains a `crate_dir: &str` arg. Two regression tests: nested manifest emits `crate_dir = ".."`; scaffold-convention manifest still emits `crate_dir = "."`. ## P2c — ConfigPushArgs source break (`no_diff` / `no_env`) Prior commit hoisted `no_diff` / `no_env` under a nested `ConfigPushSuppressions` sub-struct to satisfy `clippy::struct_excessive_bools`, breaking every downstream CLI that constructs `ConfigPushArgs` via `Default::default()` + field assignment (`args.no_diff = true`) with E0609. `#[non_exhaustive]` does not insulate against moving or removing existing public fields. Fields restored as top-level on `ConfigPushArgs`. Total bool count is now 5, so the struct gets a per-item `#[expect(clippy::struct_excessive_bools)]` with a reason line pinning the source-compat rationale. `ConfigPushSuppressions` kept as an empty compat-stub for any out-of-tree code that reached for it. Call sites (config.rs, args.rs tests, app-demo test) updated back to `args.no_diff` / `args.no_env`. All CI gates locally clean: fmt, clippy `--workspace --all-targets --all-features -D warnings`, workspace tests (1622 pass), Spin wasm32-wasip2 check, examples/app-demo workspace tests.
Contributor
Author
Review responses — first-pass findingsThe two summary-body findings without inline threads: Blocking — Apply path safety to Non-blocking — Document Axum's generated-manifest workflow. Resolved in 7d6d886 ( Individual replies posted on each of the six second-review inline threads (P1a Windows paths, P1b CF retry, P1c Spin nested wasm, P2a dry-run diff, P2b Axum crate_dir, P2c ConfigPushArgs source break). All CI gates locally clean; branch at e0688aa on top of the main merge 3f340ef. |
prk-Jr
self-requested a review
July 14, 2026 06:21
…al-impl # Conflicts: # crates/edgezero-adapter-axum/src/cli/run.rs # crates/edgezero-adapter-cloudflare/src/cli.rs # crates/edgezero-adapter-fastly/src/cli.rs # crates/edgezero-adapter-spin/src/cli.rs # crates/edgezero-adapter-spin/src/cli/push_sqlite.rs # crates/edgezero-cli/Cargo.toml # crates/edgezero-cli/src/adapter.rs # crates/edgezero-cli/src/config.rs # crates/edgezero-cli/src/generator.rs # crates/edgezero-cli/src/provision.rs
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
3 participants
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.
Adds
edgezero provision --local, folds all five adapter manifests into the gitignored-generated model, and hardens the surrounding CLI (path safety, env redaction, adapter-scoped env-file load) acrossprovision,config push,config diff, andserve.Source artifacts
docs/superpowers/specs/2026-06-23-provision-local.mddocs/superpowers/plans/2026-06-27-provision-local.md— 43 tasks across 9 sectionsThe 9 plan sections all landed on this branch (rather than as separate sub-PRs merging in); each closes on merge below.
What ships
provision --local. NewProvisionMode::Localarm threads throughAdapter::provision. Local mode:toml_edit::DocumentMut(CLI-owned bootstrap runs before validation).EDGEZERO__STORES__<KIND>__<ID>__NAME=…) so the runtime resolves stores at startup..edgezero/.env,.dev.vars,<spin_crate>/.env).Dry-run stages a real
fs::copyinto atempfile::TempDirand diffs the result back. The tree stays byte-identical either way.Typed provision (
run_provision_typed::<C>). Generated<app>-clibinaries dispatch the typed variant, which runs the base flow then additionally emits per-secret placeholder lines derived fromAppConfig's#[secret]/#[secret(store_ref)]fields (Axum:<key>=; Cloudflare:<key>=""in.dev.vars; Fastly:[[local_server.secret_stores.<store_id>]]; Spin: lowercased[variables]+SPIN_VARIABLE_<NAME>=in.env). The bundlededgezerobinary intentionally does not emit placeholders — it has no downstreamAppConfigtype.All five adapter manifests are provision-generated and gitignored.
axum.tomljoinedwrangler.toml/fastly.toml/spin.toml/runtime-config.tomlin the 2026-07 amendment; the scaffold.hbstemplate foraxum.tomlwas removed so scaffold-time provision is the single writer. A CI grep gate enforces the whole set.Path safety. New
path_safety::{assert_provision_paths_safe, assert_provision_paths_contained}helpers guard every CLI entry point that joinsmanifest_rootwith an operator-declared adapter path. Cloud dispatch runs the "safe" variant (absolute-path rejection +..traversal rejection);--localruns the stricter "contained" variant that additionally requires BOTH[adapters.<name>.adapter].manifestAND[adapters.<name>.adapter].crateto be declared, with the manifest resolving inside the crate dir. Wired intorun_provision,run_provision_typed,run_config_push_typed,run_config_diff_typed, andrun_serve.Renamed / nested adapter crate support.
cli_support::read_adapter_crate_namewalks upward from the manifest's parent to the first Cargo.toml insidemanifest_rootand reads[package].name. All four bundled adapter synthesisers use this to name generated manifests correctly under operator renames ([adapters.axum.adapter].crate = "crates/server"→crate = "server"inaxum.toml; Cargo packagespin-server→ wasm pathspin_server.wasm). Nested manifests likecrates/server/config/spin.tomlresolve correctly too.Spin component / crate decoupling.
synthesise_spin_tomlnow takes two distinct identities —crate_name(from Cargo.toml, drives[application].nameand the wasm source basename) andcomponent(from[adapters.spin.adapter].component, drives[[trigger.http]].componentand the[component.<id>]table key). Fixes the pre-2026-07 conflation where setting a component selector would silently mispoint the wasm artifact.Env-value redaction in dry-run.
.envand.dev.varsbodies are rewritten asKEY=<redacted>before diffing so operator secrets never surface in dry-run output. CommentedKEY=valuelines (adapter provisioners emit these as# EDGEZERO__STORES__…__KEY=placeholders, and operators stash real values behind#for later use) get the same treatment; pure comments and blank lines pass through so structural drift stays readable.Adapter-scoped env-file load in
run_serve. Axum reads<manifest_root>/.edgezero/.env; Spin reads.envnext to the resolvedspin.toml(matches where provision writes it — a nestedmanifest = "crates/spin/config/spin.toml"correctly resolves tocrates/spin/config/.env). Both run through the path-safety guard before the read. Contract test spawns a real serve child, inspects its inherited environment via a file the child writes, and asserts the marker propagates.Section breakdown
Each section closes on merge:
ManifestAdapterDeployedschema + writeback (Tasks 14–16, 16b–16c)Post-plan additions (from external review iterations)
Multiple review passes drove refinements past the original plan:
.gitignore+ CI gate + spec-plan-guide docs updated.read_adapter_crate_nameupward walk added to support nested manifests (v5 review).[application].nameand wasm basename decoupled from the component selector (v5 review)..manifestAND.crate;run_config_diff_typedgets the guard (previously read-only justified skipping);run_servegets the guard on the Spin env-file load.# KEY=valuelines (v6/v7 reviews).run_manifest_shape_gates(addsvalidate_deployed_field_ownershipalongside capability + handler-path checks).run_servederives.envpath from[adapters.spin.adapter].manifest.parent()(was.crate— broke on nested manifests). Contract test observes the spawned child's env (v8 review).#[expect(...)]restructure sweep acrossedgezero-cli,edgezero-core/manifest.rs(Deserialize impls), and adaptercli/mod.rsfiles (removed via explicit trait method overrides,ConfigPushSuppressionssub-struct,streammodule, structural code fixes rather than workspace-levelallows).CI gates (all pass locally)
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --all-targetscargo check --workspace --all-targets --features "fastly cloudflare spin"cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spinexamples/app-demoworkspace testscargo test -p edgezero-cli --test generated_project_builds -- --ignored)Test plan
provision_local_*cases + Spin's env-label alignment quartet (Section 9)run_serve,run_config_diff_typed,run_config_push_typed,run_provisionMigration notes
<app>-cli provision --adapter <name> --localafter cloning to regenerate the five adapter manifests (.gitignorecovers all of them plus.dev.vars).[adapters.<name>.adapter].manifestand[adapters.<name>.adapter].crateare now both required forprovision --localandconfig push/diff --local. Every scaffolded project already sets both. Cloud dispatch remains permissive.run_provision_typed::<AppConfig>(not the untypedrun_provision) so#[secret]fields reach the adapters'provision_typedimpls.