Skip to content

Expose runtime_env_config for custom Fastly entry points - #351

Merged
aram356 merged 6 commits into
mainfrom
expose-fastly-runtime-env-config
Sep 1, 2026
Merged

Expose runtime_env_config for custom Fastly entry points#351
aram356 merged 6 commits into
mainfrom
expose-fastly-runtime-env-config

Conversation

@aram356

@aram356 aram356 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Closes #349.

edgezero-adapter-fastly resolved EDGEZERO__* runtime overrides from the edgezero_runtime_env Config Store only inside the private loader behind run_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_config and exports it, keeping the missing-store warning and empty-EnvConfig fallback so custom entry points resolve staged selectors identically to run_app. The key-derivation rules move into a pure runtime_env_keys helper with a unit test pinning them: a __NAME selector per declared store id, __KEY for config stores only. The helper is gated cfg(any(feature = "fastly", test)) because the crate's default features exclude fastly, 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, and cargo check --workspace --all-targets --features "fastly cloudflare spin".

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 prk-Jr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_keys as a pure function and gating it #[cfg(any(feature = "fastly", test))] is the right trick, and it pays off concretely: I ran cargo test -p edgezero-adapter-fastly runtime_env_keys with fastly off 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:55 line-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:154 included.

Findings

Blocking

  • The __KEY selector still cannot be wired through the public builder (crates/edgezero-adapter-fastly/src/request.rs:366) — not inlineable, request.rs is unchanged in this PR.

    FastlyService is the only public builder for a custom entry point that wires stores explicitly. Its config path hardcodes default_key: "default".to_owned(), while the crate-private registry path used by run_app resolves default_key: env.store_key("config", id) (request.rs:427). The builder exposes with_config(name) and with_config_handle(handle), neither of which accepts a key. dispatch_with_registries — the only function that threads an EnvConfig into bindings — is pub(crate).

    This matters because the __KEY selector is the specific root cause in IABTechLab/trusted-server#940: a staged deploy redirects EDGEZERO__STORES__CONFIG__<ID>__KEY to <id>_staging, and a staged version reading the production key silently exercises production config. __NAME is fully solved by this PR — downstream reads env.store_name("config", id) and passes it to with_config(...). __KEY is only half solved: the value is reachable via the public EnvConfig::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 the run_app path need a FastlyService::with_config_key(key) (or a public registry-dispatch entry point)? If manual application is the intent, the runtime_env_config doc 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_env stays private (crates/edgezero-adapter-fastly/src/lib.rs:89) — not inlineable, unchanged region.

    run_app_with_config requires a &FastlyLogging. FastlyLogging is public with public fields, and runtime_env_config now yields the EnvConfig needed 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: the log::LevelFilter::from_str parse with its Info fallback (line 92-95), and the use_fastly_logger = endpoint.is_some() rule, which exists specifically so Viceroy does not reject the reserved stdout endpoint (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_config completes 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_extensions becomes let logging = FastlyLogging::from(&env); and a downstream entry point gets run_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/PORT are axum-only; LOGGING__USE_FASTLY_LOGGER/ECHO_STDOUT are 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 before init_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 at lib.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 test arm in the cfg gate needs a one-line why — see inline comment at lib.rs:209.

  • #[inline] on a non-trivial pub fn — see inline comment at lib.rs:179.

  • One stale reference to the old name: docs/superpowers/plans/2026-07-04-edgezero-fastly-dispatch-fidelity.md:815 still shows let 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, same to_ascii_uppercase, same __NAME-always / __KEY-for-CONFIG-only rule, same filter_map collection 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-260 already documents the edgezero_runtime_env store, the fastly config-store list lookup, 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 the runtime_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.

Comment thread crates/edgezero-adapter-fastly/src/lib.rs
Comment thread crates/edgezero-adapter-fastly/src/lib.rs Outdated
Comment thread crates/edgezero-adapter-fastly/src/lib.rs
Comment thread crates/edgezero-adapter-fastly/src/lib.rs Outdated
Comment thread crates/edgezero-adapter-fastly/src/lib.rs
Comment thread crates/edgezero-adapter-fastly/src/lib.rs
Comment thread crates/edgezero-adapter-fastly/src/lib.rs Outdated
Comment thread scripts/smoke_test_config_key_override.sh

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/edgezero-adapter-fastly/src/lib.rs Outdated
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.
@aram356

aram356 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

On the blocking question — manual env.store_key() at call sites was not the intended contract. 85bfbc0 closes the gap by making request::dispatch_with_registries public, taking StoresMetadata whole: a custom entry point pairs it with runtime_env_config and gets the exact wiring run_app uses, including the __KEY selector into ConfigStoreBinding.default_key, multi-id registries, and the KV/secrets __NAME resolution. I chose this over a FastlyService::with_config_key(...) because the builder route would close only the single-config-store case and still leave the other selectors as manual duplication; one public parity path keeps one source of truth. It stays at its module path rather than a crate-root re-export because the workspace denies clippy::pub_use.

The logging_from_env suggestion is also in: it is now impl From<&EnvConfig> for FastlyLogging, so the run_app_with_config path needs no reimplemented rules either.

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.

@aram356
aram356 requested a review from prk-Jr August 31, 2026 21:25

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at request.rs:320. [resolve_kv_handle] is private; legal from a pub(crate) doc, rejected from a pub one. Confirmed introduced here (this branch errors, origin/main does not). Tagged campsite rather than wrench only because cargo doc is 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 at lib.rs:142 so the crate can be rustdoc-clean on the commit that first publishes this API.

  • ♻️ The __KEY trap is now documented, but not where a reader walks into it. dispatch_with_registries carries an excellent contrast note — "Contrast FastlyService, whose bare-handle path binds default_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) and with_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 "Use run_app for 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 synthesised SecretRegistry binds the handle to platform store name "default" … Use the manifest-aware run_app if 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: __KEY is 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:195 and lib.rs:278 are 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. Your runtime_env_keys cfg trick applies verbatim, since FastlyLogging needs nothing from the fastly crate.

  • ♻️ Doc example omits FastlyLogging::from(&env) / init_logger — inline at lib.rs:209. Matters beyond completeness: the example's failure modes report only through log, so as written it reproduces #359's inertness in the snippet meant to be copied.

  • Third From rule undocumented — inline at lib.rs:92. echo_stdout is hardcoded true, discarding a key the loader resolves; the sibling From<ResolvedLoggingConfig> honours it.

  • rust,ignore example is never compiled — inline at lib.rs:195. Correct today (I checked it against app.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 have pub(crate) dispatch_with_registries on the old three-Option signature, and both still carry the byte-identical key-derivation loop (adapter-cloudflare/src/lib.rs:67-80) — same ("CONFIG", …)/("KV", …)/("SECRETS", …) iteration, same to_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 into EnvConfig::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-260 already documents the edgezero_runtime_env store, the fastly config-store list lookup, 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 grepped docs/: no reference to runtime_env_config anywhere. Since the entire motivation is an external consumer who has to discover this API exists, and the rustdoc example is ignore, 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.

Comment thread crates/edgezero-adapter-fastly/src/lib.rs
Comment thread crates/edgezero-adapter-fastly/src/lib.rs Outdated
Comment thread crates/edgezero-adapter-fastly/src/lib.rs
Comment thread crates/edgezero-adapter-fastly/src/lib.rs
Comment thread crates/edgezero-adapter-fastly/src/lib.rs Outdated
Comment thread crates/edgezero-adapter-fastly/src/request.rs Outdated
Comment thread crates/edgezero-adapter-fastly/src/request.rs
@aram356

aram356 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Round-two follow-up is in 6d2a7ae.

  • FastlyService::with_config, with_config_handle, and run_app_with_config now state that their bare-handle path does not apply the EnvConfig overlay and binds the config default key to "default"; the docs point parity-sensitive custom entry points to runtime_env_config plus dispatch_with_registries.
  • The custom-entry example now includes FastlyLogging/init_logger, and the three EnvConfig conversion rules run in the default-feature unit-test suite.
  • The new public API rustdoc is clean with warnings denied.

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.

@aram356
aram356 merged commit 5c9886e into main Sep 1, 2026
22 checks passed
@aram356
aram356 deleted the expose-fastly-runtime-env-config branch September 1, 2026 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose a public runtime-env EnvConfig API for custom Fastly entry points

3 participants