Expose runtime_env_config for custom Fastly entry points - #351
Conversation
Rename the private env_config_from_runtime_dictionary loader to runtime_env_config and export it from edgezero-adapter-fastly, so custom Fastly entry points that bypass run_app can resolve staged EDGEZERO__* store selectors identically instead of duplicating the store name and key-derivation rules. Split the key derivation into a pure runtime_env_keys helper and pin its rules (a __NAME selector per store id, __KEY for config stores only) with a unit test that runs without a Fastly host. Closes #349
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Exports the Fastly runtime-env loader as runtime_env_config and splits its key derivation into a pure, unit-tested runtime_env_keys helper. This is a clean, well-scoped fix that lands exactly the signature issue #349 proposed. Findings below are about how far the exported surface actually carries a custom entry point, plus documentation and test-strength polish. One blocking question; nothing structurally wrong with the change.
I verified the PR's central claim before reviewing the rest: hardcoding "edgezero_runtime_env" is correct for staged deploys because relink_staged_runtime_env (cli.rs:4814-4912) links the per-service twin edgezero_runtime_env_staging_<service-id> under that same name. A fixed name is what makes staged resolution work, not a bug.
😃 Praise
- Extracting
runtime_env_keysas a pure function and gating it#[cfg(any(feature = "fastly", test))]is the right trick, and it pays off concretely: I rancargo test -p edgezero-adapter-fastly runtime_env_keyswithfastlyoff and the test passes in the plain default-feature suite. The key-derivation rules are now pinned in CI with no Fastly host and no Viceroy — which is the difference between a rule that is documented and a rule that is enforced. - The rewritten doc comment is a genuine improvement beyond the rename: it drops the stale "spec 5.2/5.4/12.7" cross-references and the Cloudflare
lib.rs:55line-number pointer (both already drifted) in favour of stating the actual operator-facing contract. - Rename propagated to every reference,
scripts/smoke_test_config_key_override.sh:154included.
Findings
Blocking
-
❓ The
__KEYselector still cannot be wired through the public builder (crates/edgezero-adapter-fastly/src/request.rs:366) — not inlineable,request.rsis unchanged in this PR.FastlyServiceis the only public builder for a custom entry point that wires stores explicitly. Its config path hardcodesdefault_key: "default".to_owned(), while the crate-private registry path used byrun_appresolvesdefault_key: env.store_key("config", id)(request.rs:427). The builder exposeswith_config(name)andwith_config_handle(handle), neither of which accepts a key.dispatch_with_registries— the only function that threads anEnvConfiginto bindings — ispub(crate).This matters because the
__KEYselector is the specific root cause in IABTechLab/trusted-server#940: a staged deploy redirectsEDGEZERO__STORES__CONFIG__<ID>__KEYto<id>_staging, and a staged version reading the production key silently exercises production config.__NAMEis fully solved by this PR — downstream readsenv.store_name("config", id)and passes it towith_config(...).__KEYis only half solved: the value is reachable via the publicEnvConfig::store_key(...), but it cannot be attached to the binding, so downstream has to apply it by hand at each extraction site rather than getting it from the resolved config store.So the question is about whether #349 actually closes: is manual
env.store_key()at the call site the intended contract for custom entry points, or does parity with therun_apppath need aFastlyService::with_config_key(key)(or a public registry-dispatch entry point)? If manual application is the intent, theruntime_env_configdoc should say so explicitly — the asymmetry between the two config paths is invisible from the outside and silently reintroduces exactly the drift #349 is about. Happy to be told this is a deliberate follow-up.
Non-blocking
-
♻️
logging_from_envstays private (crates/edgezero-adapter-fastly/src/lib.rs:89) — not inlineable, unchanged region.run_app_with_configrequires a&FastlyLogging.FastlyLoggingis public with public fields, andruntime_env_confignow yields theEnvConfigneeded to populate it — but the only code in the crate that derives one from the other is private. A custom entry point on that path must therefore reimplement two non-obvious rules: thelog::LevelFilter::from_strparse with itsInfofallback (line 92-95), and theuse_fastly_logger = endpoint.is_some()rule, which exists specifically so Viceroy does not reject the reservedstdoutendpoint (documented at lines 96-100). Both are internals that can drift — the same category of duplication #349 objects to, one layer up.Exporting it alongside
runtime_env_configcompletes the pair, and an impl reads better than a free function:#[cfg(feature = "fastly")] impl From<&EnvConfig> for FastlyLogging { #[inline] fn from(env: &EnvConfig) -> Self { /* current logging_from_env body */ } }
Then
run_app_with_request_extensionsbecomeslet logging = FastlyLogging::from(&env);and a downstream entry point getsrun_app_with_config::<A>(&FastlyLogging::from(&runtime_env_config(A::stores())), req, name)without copying any rule. -
🤔 Four of six fixed keys have no Fastly consumer — see inline comment at
lib.rs:211.ADAPTER__HOST/PORTare axum-only;LOGGING__USE_FASTLY_LOGGER/ECHO_STDOUTare read by nothing. Now pinned as contract by the new test. -
🤔 The missing-store warning is inert on Compute — see inline comment at
lib.rs:144. Emitted beforeinit_logger, so it never reaches the Fastly logs its own doc points operators at. Pre-existing. -
♻️ Doc omits
run_app_with_config, which does not call this — see inline comment atlib.rs:174. -
♻️ Test asserts membership, not the exact set — see inline comment at
lib.rs:311;StoresMetadata::default()also uncovered. -
♻️ Store name still duplicated across three sites — see inline comment at
lib.rs:183. -
♻️ The
testarm in the cfg gate needs a one-line why — see inline comment atlib.rs:209. -
⛏
#[inline]on a non-trivialpub fn— see inline comment atlib.rs:179. -
⛏ One stale reference to the old name:
docs/superpowers/plans/2026-07-04-edgezero-fastly-dispatch-fidelity.md:815still showslet env = env_config_from_runtime_dictionary(stores);. It is a dated historical plan document, so leaving it as a record of what the code looked like then is a defensible call — noting it only so the choice is deliberate rather than missed.
📌 Out of Scope
-
Cloudflare carries a byte-identical derivation loop (
crates/edgezero-adapter-cloudflare/src/lib.rs:67-80). Same("CONFIG", stores.config) / ("KV", ...) / ("SECRETS", ...)iteration, sameto_ascii_uppercase, same__NAME-always /__KEY-for-CONFIG-only rule, samefilter_mapcollection shape. Only the fixed-key prefix differs (Cloudflare omits the three Fastly logging keys). Its doc comment at lines 55-58 even restates the__KEY-is-config-only rule in prose, so the rule is now written down in three places and enforced in one.This PR extracts the rule Fastly-side, which is the right call for its scope, but the cross-adapter drift risk it leaves is the more durable version of the same problem #349 reported. Natural home is core, where the spec rule belongs and where the new test could cover both adapters at once:
// edgezero-core/src/env_config.rs impl EnvConfig { #[must_use] pub fn store_keys(stores: StoresMetadata) -> Vec<String> { /* the shared loop */ } }
Each adapter then concatenates its own fixed keys. Worth a tracking issue rather than scope creep here.
-
🌱 The new public API is not reflected in the docs site.
docs/guide/blob-app-config-migration.md:236-260already documents theedgezero_runtime_envstore, thefastly config-store listlookup, the[local_server.config_stores.edgezero_runtime_env]local block, and even the missing-store warning text — so it is the natural place for a short "custom entry points" subsection showing theruntime_env_config(A::stores())call. Since the whole motivation is an external consumer (Trusted Server) who has to discover this exists, docs are arguably load-bearing for the fix rather than optional. Fine as a follow-up.
CI Status
All gates run locally on 7060da12:
| Gate | Result |
|---|---|
cargo fmt --all -- --check |
PASS |
cargo clippy --workspace --all-targets --all-features -- -D warnings |
PASS (exit 0, zero warnings) |
cargo test --workspace --all-targets |
PASS |
cargo check --workspace --all-targets --features "fastly cloudflare spin" |
PASS |
cargo check -p edgezero-adapter-fastly --no-default-features |
PASS (zero warnings — confirms the cfg gating leaves a --no-default-features build dead-code clean) |
cargo test -p edgezero-adapter-fastly runtime_env_keys |
PASS (1 passed, 53 filtered — runs with fastly off, as the PR claims) |
GitHub checks: 21/21 passing.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed 7060da1292ec2776b69cc20da9ca3e496da9a38f against 8ff0c3b3b26edd48e32923569fae1a211618f628. The extraction preserves the existing Fastly selector behavior and all relevant local and CI checks pass. I left one P2 inline concerning the custom-entry-point guidance for the specific Trusted Server consumer.
Make dispatch_with_registries public, taking StoresMetadata whole, so a custom Fastly entry point pairs it with runtime_env_config for the same store wiring run_app uses, including the config-only __KEY selector that FastlyService's bare-handle path cannot express. Replace the private logging_from_env with From<&EnvConfig> for FastlyLogging so the level-parse fallback and the endpoint-derived use_fastly_logger rule are not reimplemented downstream. Add the ungated RUNTIME_ENV_STORE_NAME const as the single source of the store name; document that the fixed name is what staged relinking relies on. Document the empty-default-stores() trap for handwritten Hooks impls and the entry points that do not resolve the env overlay themselves. Explain the test arm of the runtime_env_keys cfg gate, and pin the key derivation with exact-set assertions plus an empty-metadata case.
|
On the blocking question — manual The Follow-ups filed rather than grown into this PR: #359 (the inert missing-store warning), #360 (hoisting the shared key derivation into core, covering the Cloudflare twin), #361 (the docs-site custom-entry-point section). The stale name in the 2026-07-04 plan doc stays as a historical record, per your framing. |
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review — round 2 (3d30ae52)
Summary
Re-review of 85bfbc01 against my CHANGES_REQUESTED on 7060da12. Approving. All nine prior findings are resolved, and the blocking one is resolved substantively rather than by documentation: making request::dispatch_with_registries public with StoresMetadata taken whole gives a custom entry point the real __KEY resolution path, not a written-down workaround. That is a better answer than the FastlyService::with_config_key setter I proposed — it reuses build_config_registry instead of adding a second resolution path to keep in sync.
I verified the parity claim directly rather than taking the reply: build_config_registry binds default_key: env.store_key("config", id) (request.rs:437), against the hardcoded default_key: "default" on the bare-handle path (request.rs:376). A caller pairing runtime_env_config with dispatch_with_registries now resolves EDGEZERO__STORES__CONFIG__<ID>__KEY exactly as run_app does. #349 closes.
Everything below is non-blocking. The findings cluster on the seam the promotion created — a doc link that was legal while the function was private, and rules that became public contract without becoming tested.
Prior round — verified resolved
| Prior finding | How it landed | Verified |
|---|---|---|
❓ __KEY unreachable from any public builder |
dispatch_with_registries made pub, taking StoresMetadata |
env.store_key("config", id) at request.rs:437 — real parity, not documented parity |
♻️ logging_from_env stays private |
impl From<&EnvConfig> for FastlyLogging (lib.rs:75) |
Sole caller updated at lib.rs:161; free function gone |
| ♻️ Store name duplicated across three sites | Ungated pub const RUNTIME_ENV_STORE_NAME (lib.rs:44) |
Old RUNTIME_ENV_STORE gone; all six sites route through it (lib.rs:217, cli.rs:551/4850/4855/4885/4908) |
♻️ Doc omits run_app_with_config |
Named, plus the hand-built FastlyService path |
Went past the ask — also covers the empty-stores() trap with a worked example |
| ♻️ Test asserts membership, not the exact set | Sorted assert_eq!; contains helper gone |
Both tests run under default features: 2 passed; 53 filtered out |
♻️ StoresMetadata::default() uncovered |
Second test pinning the six fixed keys | Same run |
♻️ test arm in the cfg gate needs a why |
Comment added, following the file-top precedent | lib.rs:247-249 |
| 🤔 Four fixed keys have no Fastly consumer | Doc corrected to distinguish what the runtime consumes from what it resolves for downstream | Keys kept — right call, changing the resolved EnvConfig mid-PR would have been the riskier edit |
| 🤔 Missing-store warning inert on Compute | Filed #359 | Correctly scoped out |
One I am withdrawing: ⛏ #[inline] on a non-trivial pub fn. The reply is correct and I was wrong — the workspace denies the whole clippy restriction group (Cargo.toml:108), so missing_inline_in_public_items rejects a public fn without it, and dropping the attribute would cost an #[expect]. Not worth it for an advisory hint.
Findings — all non-blocking
-
🏕 Broken rustdoc link on the newly-public
dispatch_with_registries— inline atrequest.rs:320.[resolve_kv_handle]is private; legal from apub(crate)doc, rejected from apubone. Confirmed introduced here (this branch errors,origin/maindoes not). Tagged campsite rather than wrench only becausecargo docis not a CI gate and these are warnings without-D warnings— the 21/21 green run is accurate. Grouped with the pre-existing redundant-link warning atlib.rs:142so the crate can be rustdoc-clean on the commit that first publishes this API. -
♻️ The
__KEYtrap is now documented, but not where a reader walks into it.dispatch_with_registriescarries an excellent contrast note — "ContrastFastlyService, whose bare-handle path bindsdefault_key: "default"and ignores those selectors" — but a reader only finds it having already discovered the function. The two doors a custom entry point actually opens carry nothing:FastlyService::with_config(request.rs:195-198) andwith_config_handle(request.rs:206-208): no mention that neither applies the env overlay, so both silently bind"default".run_app_with_config(lib.rs:278-281): says "Userun_appfor the manifest-driven flow that resolves stores automatically", which is true but reads as a convenience note rather than a correctness one. Its own doc never says the overlay is unresolved on this path.
The crate already sets the precedent, which is what makes the gap worth closing:
with_secrets(request.rs:230-241) carries exactly this warning — "Platform-name binding: the synthesisedSecretRegistrybinds the handle to platform store name"default"… Use the manifest-awarerun_appif your account uses a different store name — it routes through the env-overlay resolution path instead." The config methods need the mirror of that paragraph, and they are the higher-stakes pair:__KEYis the specific root cause in IABTechLab/trusted-server#940, whereas secrets only carry__NAME. Right now the store kind with the extra failure mode is the one without the warning.Not inlineable —
request.rs:195andlib.rs:278are outside this PR's diff. -
♻️ The two newly-public logging rules have no test that CI runs — inline at
lib.rs:70, with the measurement. Yourruntime_env_keyscfg trick applies verbatim, sinceFastlyLoggingneeds nothing from thefastlycrate. -
♻️ Doc example omits
FastlyLogging::from(&env)/init_logger— inline atlib.rs:209. Matters beyond completeness: the example's failure modes report only throughlog, so as written it reproduces #359's inertness in the snippet meant to be copied. -
⛏ Third
Fromrule undocumented — inline atlib.rs:92.echo_stdoutis hardcodedtrue, discarding a key the loader resolves; the siblingFrom<ResolvedLoggingConfig>honours it. -
⛏
rust,ignoreexample is never compiled — inline atlib.rs:195. Correct today (I checked it againstapp.rs:83-94); noting the drift cost since this is the external consumer's onboarding path.
😃 Praise
Two inline, at lib.rs:44 and request.rs:327. The short version: the const collapse reached all six sites including the four staged-relink strings whose agreement is what makes staged resolution work, and ungating it costs nothing (--no-default-features stays clean). And collapsing three same-typed positional Option<StoreMetadata> parameters into StoresMetadata before publishing removed a silent-transposition footgun at the last moment it was free to remove.
Worth adding: the response to round 1 consistently went past the literal ask — the run_app_with_config doc note grew to cover the hand-built FastlyService path and the empty-stores() trap from a separate thread, and the exact-set test gained the StoresMetadata::default() case. The one finding declined was declined with a correct reason.
📌 Out of Scope
-
Cross-adapter drift is now wider than before this PR, in two dimensions. Cloudflare (
adapter-cloudflare/src/request.rs:298) and Spin (adapter-spin/src/request.rs:183) both still havepub(crate) dispatch_with_registrieson the old three-Optionsignature, and both still carry the byte-identical key-derivation loop (adapter-cloudflare/src/lib.rs:67-80) — same("CONFIG", …)/("KV", …)/("SECRETS", …)iteration, sameto_ascii_uppercase, same__NAME-always /__KEY-config-only rule. Fastly-only was the right scope for #349, but the rule is now written in three places, enforced in one, and the ergonomic fix exists in one. Still a tracking issue: hoisting the loop intoEnvConfig::store_keys(StoresMetadata)in core would let one test cover every adapter. -
🌱 The docs site still does not mention any of this. Carried over from round 1 and not claimed as addressed.
docs/guide/blob-app-config-migration.md:236-260already documents theedgezero_runtime_envstore, thefastly config-store listlookup, the local[local_server.config_stores.edgezero_runtime_env]block and the missing-store warning text — so the "custom entry points" subsection has an obvious home. I greppeddocs/: no reference toruntime_env_configanywhere. Since the entire motivation is an external consumer who has to discover this API exists, and the rustdoc example isignore, the docs page is closer to load-bearing than optional here. Fine as a follow-up, worth not losing.
CI Status
All five project gates run locally on 3d30ae52:
| Gate | Result |
|---|---|
cargo fmt --all -- --check |
PASS |
cargo clippy --workspace --all-targets --all-features -- -D warnings |
PASS (exit 0, zero warnings) |
cargo test --workspace --all-targets |
PASS (all suites green) |
cargo check --workspace --all-targets --features "fastly cloudflare spin" |
PASS |
cargo check -p edgezero-adapter-fastly --no-default-features |
PASS (zero warnings — the ungated const leaves a default build dead-code clean) |
cargo test -p edgezero-adapter-fastly runtime_env_keys |
PASS (2 passed, 53 filtered — both new tests run with fastly off, as claimed) |
Not a gate, reported for the finding above: RUSTDOCFLAGS="-D warnings" cargo doc -p edgezero-adapter-fastly --features fastly --no-deps fails on this branch with the private-item link error, and fails on origin/main only with the pre-existing redundant-link error.
|
Round-two follow-up is in 6d2a7ae.
Verification passed: format, workspace clippy with all features and -D warnings, workspace all-target tests, workspace feature check, Spin wasm32-wasip2 check, Fastly no-default check, and Fastly rustdoc with -D warnings. |
Closes #349.
edgezero-adapter-fastlyresolvedEDGEZERO__*runtime overrides from theedgezero_runtime_envConfig Store only inside the private loader behindrun_app. A downstream app with a custom Fastly entry point (Trusted Server builds its app directly) could not reach it, so a staged version silently loaded production config selectors (IABTechLab/trusted-server#940).This renames the loader to
runtime_env_configand exports it, keeping the missing-store warning and empty-EnvConfigfallback so custom entry points resolve staged selectors identically torun_app. The key-derivation rules move into a pureruntime_env_keyshelper with a unit test pinning them: a__NAMEselector per declared store id,__KEYfor config stores only. The helper is gatedcfg(any(feature = "fastly", test))because the crate's default features excludefastly, which is what lets the test run in the plain workspace test suite.Verified in this branch:
cargo test -p edgezero-adapter-fastly(54 passed, new test included),cargo fmt --check,cargo clippy --all-targets --all-features -D warnings, andcargo check --workspace --all-targets --features "fastly cloudflare spin".