Skip to content

Honor EdgeZero app config store default - #879

Merged
ChristianPavilonis merged 11 commits into
mainfrom
fix/honor-ez-app-config
Sep 28, 2026
Merged

ChristianPavilonis merged 11 commits into
mainfrom
fix/honor-ez-app-config

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Jul 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Derive the Trusted Server runtime app-config store ID and blob key from [stores.config].default in edgezero.toml at build time.
  • Preserve trusted_server_config as the current default while retaining EdgeZero store name/key overrides.
  • Align runtime defaults, Cloudflare bindings, integration fixtures, and operator documentation with the manifest-defined default.

Fastly already consumes DEFAULT_CONFIG_STORE_ID and resolves service-scoped overrides from edgezero_runtime_env on the base branch. This PR changes the constant's source from a literal to the build-time manifest value; it does not change Fastly adapter source or add its override behavior.

Changes

File Change
crates/trusted-server-core/build.rs Validate edgezero.toml and expose its default config-store ID as a compile-time value.
crates/trusted-server-core/src/config_payload.rs, settings_data.rs Use the manifest-derived ID for the default runtime store and blob key, with regression coverage for manifest alignment and environment overrides.
crates/trusted-server-adapter-cloudflare/ Read the manifest-default blob key while retaining compatibility with the legacy app_config key.
Integration fixtures and docs/guide/configuration.md Generate and document app-config stores and keys from the shared manifest-derived constant.

Closes

Closes #866

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: cargo test-cloudflare; cargo clippy-cloudflare && cargo clippy-cloudflare-wasm; integration config generator and non-ignored integration tests; Spin, CLI, and parity tests

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!) — no runtime logging added; build-script println! calls emit required Cargo directives
  • New code has tests
  • No secrets or credentials committed

@ChristianPavilonis
ChristianPavilonis requested review from aram356 and prk-Jr and removed request for aram356 July 9, 2026 21:12

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Replacing the Fastly hardcode with a manifest-derived constant is the right direction — Spin already resolved its config store through ctx.config_store_default() (adapter-spin/src/platform.rs:726), so Fastly's literal was the last one holding out, and the Cloudflare key alignment keeps a legacy fallback rather than breaking existing Workers. Read all 13 changed files.

Two blocking items. One is the red cargo fmt job, a one-line import reorder. The other is the documentation paragraph introducing the EDGEZERO__STORES__CONFIG__<ID>__KEY override: ts config push does not honor that variable, so following the instruction as written puts the blob at one key and the runtime read at another, and the service fails closed at startup. I confirmed both halves with dry-run probes against this branch's own CLI (details inline). The rest is non-blocking — mostly a question about whether the new build script earns its coupling, given the test added alongside it already catches the same drift.

Blocking

🔧 wrench

  • cargo fmt fails, CI red: import ordering (crates/trusted-server-core/src/settings_data.rs:6). Only formatting diff in the workspace.
  • The newly documented …__KEY override fails the deploy closed: the runtime reads it, ts config push ignores it, so the push and the read disagree on the blob key (docs/guide/configuration.md:1473-1476). Probe output and the edgezero-cli call site are in the inline comment.

Non-blocking

🤔 thinking

  • Does the build script earn its coupling? config_defaults_match_edgezero_manifest already catches manifest drift, while the build script adds a host build of edgezero-core and its transitive deps, a hard repo-layout dependency on the library crate, and full-manifest validation as a build gate — so an error in an unrelated [adapters.spin] block now breaks the build of every adapter (crates/trusted-server-core/build.rs:14).
  • Cloudflare gets a legacy fallback; Fastly and Axum do not: undocumented asymmetry, and LEGACY_CONFIG_BLOB_KEY has no deprecation note or removal condition (crates/trusted-server-adapter-cloudflare/src/app.rs:106).
  • A Cloudflare startup error can name a key the operator never set: manifest key absent plus a non-string at app_config reports "missing string value at trusted_server_config" (crates/trusted-server-adapter-cloudflare/src/app.rs:97).

♻️ refactor

  • CONFIG_BLOB_KEY now fills five roles and is named for one: it is derived from a logical store id, then used as env-override lookup id, platform store name, blob key, Cloudflare JSON object key, and Viceroy store name and key. Suggest a DEFAULT_CONFIG_STORE_ID const with CONFIG_BLOB_KEY defined from it (crates/trusted-server-core/src/config_payload.rs:15).
  • Build-script panics omit the path they looked at (crates/trusted-server-core/build.rs:15).

⛏ nitpick

  • Comment line overruns the block's wrap width (edgezero.toml:19).

📝 note

  • Base is two commits behind origin/main (d744b764 vs f6a2fb85) — worth a refresh before merge so CI runs against current main.

👍 praise

  • config_defaults_match_edgezero_manifest asserts both directions, so an accidental edgezero.toml edit fails a test instead of silently repointing every adapter (crates/trusted-server-core/src/settings_data.rs:248).
  • cloudflare_config_does_not_mask_malformed_manifest_value pins the one behavior an or_else chain would have gotten wrong (crates/trusted-server-adapter-cloudflare/src/app.rs:646).
  • The docs correct a claim that was wrong: the config store is not optional-and-possibly-empty; an absent entry fails startup closed. Both new CLI invocations check out against edgezero-cli v0.0.4's argument definitions (docs/guide/configuration.md:1479).

CI Status

  • fmt: FAIL (crates/trusted-server-core/src/settings_data.rs:6)
  • clippy (fastly / axum / cloudflare native + wasm / spin native + wasm): PASS
  • rust tests (fastly, axum, cloudflare, spin, cross-adapter parity, ts CLI): PASS
  • js tests (vitest): PASS
  • browser + integration + Fastly EC lifecycle: PASS
  • CodeQL: PASS

Comment thread crates/trusted-server-core/src/settings_data.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/build.rs Outdated
Comment thread crates/trusted-server-core/build.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs
Comment thread edgezero.toml Outdated

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Re-review of 2760e429 against 044cd7e9. All eight findings from the previous round are answered: the cargo fmt regression is gone (verified locally, exit 0, matching the now-green CI job), the build script's load failure carries the attempted path, DEFAULT_CONFIG_STORE_ID is split from CONFIG_BLOB_KEY and threaded through the store-identity call sites, the Cloudflare fallback gained a typed error plus three new tests, and the manifest comment is rewrapped.

One documentation change introduced in the fix commit reproduces the failure mode the previous round flagged, this time as a copy-pasteable command, and one sentence next to it conflicts with the Fastly deployment mechanism documented elsewhere in this repo. Everything else is non-blocking.

Blocking

🔧 wrench

  • --key example sets the runtime override on the push process: ts config push never reads store_key (verified in the pinned CLI at edgezero-cli/src/config.rs:435), so the env prefix is inert and the runtime never sees it — the operator lands in the fail-closed startup described three lines below (docs/guide/configuration.md:1480-1483)

❓ question

  • Does the Fastly runtime honor EDGEZERO__STORES__CONFIG__<ID>__NAME?: .claude/skills/deploying-trusted-server-to-fastly/SKILL.md:28-34 documents fastly resource-link as the runtime mapping instead, which would make NAME push-side only on this adapter (docs/guide/configuration.md:1473-1476)

Non-blocking

♻️ refactor

  • Missing [stores.config] still panics without the path: the load-failure path was fixed, the section-missing path one line later was not (crates/trusted-server-core/build.rs:32)
  • normalize_env_segment hand-rolled in the integration harness: duplicates adapter-axum/src/platform.rs:24, doubled by this commit, and already diverges on to_ascii_uppercase() vs to_uppercase() (crates/trusted-server-integration-tests/tests/environments/axum.rs:37-44)
  • Dead match guard: when the two keys are equal, the guarded arm and the final arm produce the same Missing (crates/trusted-server-adapter-cloudflare/src/app.rs:149)

🤔 thinking

  • KEY override is adapter-dependent: Fastly and Axum resolve through default_config_key(); Cloudflare reads the constant directly, so the override does not reach it (crates/trusted-server-core/src/config_payload.rs:20)

⛏ nitpick

  • Comment between #[cfg] and item: belongs above the attribute, and reads better as /// (crates/trusted-server-adapter-cloudflare/src/app.rs:78-82)

👍 praise

  • Typed Cloudflare envelope error: names the key that is actually malformed rather than the one the operator was expected to set, and the malformed-legacy test asserts the rendered message, not just the variant (crates/trusted-server-adapter-cloudflare/src/app.rs:84-110)
  • Formatting regression fixed: cargo fmt --all -- --check is clean locally

CI Status

  • fmt: PASS (also verified locally)
  • clippy (cloudflare native + wasm32-unknown-unknown, spin native + wasm32-wasip1): PASS
  • rust tests: PASS (cargo test, axum native, cloudflare, spin, cross-adapter parity, ts CLI native)
  • integration tests: PASS (Fastly EC lifecycle, browser, general)
  • js tests: PASS (vitest)
  • format-typescript / format-docs: PASS
  • Analyze (javascript-typescript): FAIL — GitHub infrastructure, not code. The CodeQL init action died with No server is currently available to service your request before producing any artifacts. Analyze (rust) and Analyze (actions) passed on the same commit.

Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/build.rs Outdated
Comment thread crates/trusted-server-integration-tests/tests/environments/axum.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-core/src/config_payload.rs
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Third pass on this branch. All eight findings from the previous round check out as addressed in 391fbb52: the runtime KEY example is gone from the Fastly guide, the resource-link runtime model is documented, a missing [stores.config] now emits cargo::error with the resolved manifest path (reproduced locally by corrupting the manifest), the axum harness calls config_env_var, the dead equality guard is gone, and the legacy-key rationale is a /// doc above the #[cfg]. The Rust side of this PR is clean; what is left is one operator-facing gap in the new Fastly documentation plus a scope note on where the EDGEZERO__* overrides can actually be read.

2 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch for both) to apply them as commits on this branch. Both were verified in a scratch worktree at this head: cargo fmt --all -- --check, cargo check --tests and cargo clippy --all-targets -- -D warnings on the integration-tests manifest, and the pinned docs/ Prettier, with pre/post-verification patch snapshots byte-identical. The remaining comments describe the change in prose.

Blocking

🔧 wrench

  • fastly resource-link create sequence cannot run on a deployed service, and never takes effect — see inline at docs/guide/configuration.md:1481-1484

Non-blocking

🤔 thinking

  • EDGEZERO__…__NAME / __KEY are inert on Fastly, and the new test reads as a guarantee that they are not — see inline at crates/trusted-server-core/src/settings_data.rs:32
  • This section now forks the deploy skill's multi-service guidance, minus its guardrails — see inline at docs/guide/configuration.md:1487-1490

♻️ refactor

  • Hand-built Map where the same PR uses json! with a const key — see inline at crates/trusted-server-integration-tests/tests/common/config.rs:39-43

👍 praise

  • main.rs and app.rs now resolve the same store name — see inline at crates/trusted-server-adapter-fastly/src/main.rs:52
  • Override test injects EnvConfig instead of mutating global env — see inline at crates/trusted-server-core/src/settings_data.rs:258

Cross-cutting / body-level findings

  • 📌 Last shipped app_config reference points the JWKS store at the app-config store — trusted-server.example.toml:38 and crates/trusted-server-integration-tests/fixtures/configs/trusted-server.integration.toml:31 both set [request_signing].config_store_id = "app_config". That field is the Fastly Config Store ID used for JWKS (docs/guide/configuration.md:516 documents it as such, and docs/guide/request-signing.md:219 uses jwks_store), so the value is wrong on two counts: wrong store, and a store name where an ID belongs. Inert today because [request_signing].enabled = false in both files, and genuinely outside this PR's scope — but after this sweep it is the only app_config left in shipped configuration, so it is now the one place an operator could still read the old name as current. Worth a follow-up issue rather than a change here.

CI Status

  • browser integration tests: PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS
  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • vitest: PASS
  • format-docs: PASS (required)
  • format-typescript: PASS (required)

Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs
Comment thread crates/trusted-server-integration-tests/tests/common/config.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-adapter-fastly/src/main.rs Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs Outdated

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Fourth pass. All four findings from the previous round check out as addressed in 953f0df2: fastly resource-link create now carries --autoclone and is followed by service-version activate, the hand-built Map in the integration harness became json! with the const key, and both default_config_store_name / default_config_key carry a /// note that the process-env path is native-adapter-only. The Rust side of this PR reads clean and its test coverage pins the manifest-to-constant relationship in the right place. What is left is one operator-facing correctness bug in the new Fastly deployment sequence, plus a merge conflict against current main.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on this branch. It was verified in a scratch worktree at this head against the pinned docs/ Prettier, with pre/post-verification patch snapshots byte-identical. The remaining comments describe the change in prose.

Blocking

🔧 wrench

  • Shared-account first-deployment path links the store under the physical name, not the logical one — see inline at docs/guide/configuration.md:1503-1519
  • Merge conflict against main — see the cross-cutting section below

Non-blocking

🤔 thinking

  • __KEY override is honoured on read but never on write — see inline at crates/trusted-server-core/src/settings_data.rs:46

♻️ refactor

  • Hand-built Value::Object helper where the same file uses json! with a const key — see inline at crates/trusted-server-adapter-cloudflare/src/app.rs:685
  • Error type does not follow the derive_more::Display + impl Error convention — see inline at crates/trusted-server-adapter-cloudflare/src/app.rs:92

👍 praise

  • Build script fails closed with the resolved manifest path — see inline at crates/trusted-server-core/build.rs:20
  • Manifest-to-constant alignment is pinned by a test — see inline at crates/trusted-server-core/src/settings_data.rs:237

Cross-cutting / body-level findings

  • 🔧 Merge conflict against main — mergeStateStatus is DIRTY. One file conflicts: crates/trusted-server-adapter-cloudflare/src/app.rs, at the import block (lines 13-18 of the merged result), against 42a34ae3 (#860, configurable cache header policies) which landed on main after this branch's merge commit. The two sides add adjacent use lines and the resolution is keep-both:

    #[cfg(any(test, target_arch = "wasm32"))]
    use trusted_server_core::config_payload::CONFIG_BLOB_KEY;
    use trusted_server_core::cache_policy::EdgeCacheHeader;

    Flagged at the body level because the conflict is not inside this PR's diff, so it has no inline anchor. Note that CI is green at 29cd794e, but that run predates 42a34ae3; the checks have not seen the two changes combined.

CI Status

  • integration tests: PASS
  • browser integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS
  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • vitest: PASS
  • format-docs: PASS (required)
  • format-typescript: PASS (required)

Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs
Comment thread crates/trusted-server-core/build.rs
Comment thread crates/trusted-server-core/src/settings_data.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Small functional delta wrapped in a large docs rewrite, and the functional part is a genuine fix: the Fastly entry point previously opened a hardcoded trusted_server_config in main.rs while app.rs resolved the same store through default_config_store_name() — under a __NAME override those two would have opened different stores. The Cloudflare JSON key migration (app_config → the manifest default) is done carefully, with the legacy fallback firing only on absence of the primary key. CI is green across all 19 checks; cargo test -p trusted-server-core --lib settings_data and cargo clippy-cloudflare re-run clean locally against this head.

Approving. Nothing here blocks merge — the notes below are a redundancy question about the new build script plus a few doc/comment sharpenings.

3 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. Each was applied to a scratch worktree at this head and verified with cargo fmt --all -- --check, cargo clippy-fastly, cargo doc -p trusted-server-core --no-deps, and the pinned docs Prettier, individually and as a batch, with a byte-exact pre/post drift check.

Non-blocking

🤔 thinking / ♻️ refactor

  • CONFIG_BLOB_KEY and default_config_key() are not interchangeable — see inline at crates/trusted-server-core/src/config_payload.rs:20
  • Doc comment understates why Fastly can't take overrides — see inline at crates/trusted-server-core/src/settings_data.rs:30
  • Local-dev step inherits the shared-account env export, and writes secrets to a tracked file — see inline at docs/guide/configuration.md:1780

Cross-cutting / body-level findings

  • ♻️ The build script is redundant with the alignment test it ships alongside — config_defaults_match_edgezero_manifest already fails if [stores.config].default drifts from the compiled constant, and its third assertion re-pins the literal "trusted_server_config" anyway, so the manifest value still cannot change without editing Rust. Against that, the build script costs a host edgezero-core build-dependency, a ../.. path escape out of the crate directory, and a new failure mode: every compile of trusted-server-core now hard-fails if the repo-root edgezero.toml is missing or trips a future EdgeZero ManifestLoader::validate() tightening. Previously a manifest-validation change could only break ts commands; now it breaks cargo build for the whole workspace. A plain const DEFAULT_CONFIG_STORE_ID: &str = "trusted_server_config"; with the new test as the gate gets the same guarantee for none of that. Not blocking — just worth a second look before the machinery settles in.

  • 🌱 Per-environment config overrides remain unreachable on Fastly. EdgeZero's own Fastly adapter reads EDGEZERO__* from a config store named edgezero_runtime_env (created by ts provision --adapter fastly, which even prints a fastly config-store-entry update ... --key=EDGEZERO__STORES__CONFIG__..__KEY hint). Trusted Server's Fastly entry point is bespoke — it never calls edgezero_adapter_fastly::run_app — so that store is inert here and a staging-style __KEY=trusted_server_config_staging cannot work by any route. Wiring edgezero_runtime_env into default_config_store_name() / default_config_key() would close it. Follow-up issue, not this PR.

  • 🌱 Stale app_config literals in the Cloudflare environment tests — crates/trusted-server-integration-tests/tests/environments/cloudflare.rs:206,210,221,232 still hardcode {"app_config":"blob"}. They only exercise template injection so they still pass, but they now read as though app_config is the current key. tests/common/config.rs was switched to CONFIG_BLOB_KEY in this PR; these could follow. (No inline comment — the file isn't in the diff.)

  • 👍 Two things worth calling out. First, unifying main.rs with app.rs on default_config_store_name() closes a real latent split that only a __NAME override would have exposed. Second, cloudflare_config_envelope falls back to the legacy key only when the primary key is absent, never when it's present-but-malformed — and cloudflare_config_does_not_mask_malformed_manifest_value pins exactly that. That's the right call and the easy thing to get wrong.

CI Status

  • browser integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • integration tests: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test: PASS
  • vitest: PASS
  • format-typescript: PASS
  • format-docs: PASS
  • cargo fmt: PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • prepare integration artifacts: PASS

Comment thread crates/trusted-server-core/src/config_payload.rs
Comment thread crates/trusted-server-core/src/settings_data.rs Outdated
Comment thread docs/guide/configuration.md Outdated
@jwrosewell

Copy link
Copy Markdown
Collaborator

A merge simulation flags a real conflict between this PR and the provider stack in #1043–#1047 and #1094. Merging the two, they collide on three files:

  • crates/trusted-server-core/build.rs
  • crates/trusted-server-adapter-fastly/src/main.rs
  • crates/trusted-server-adapter-cloudflare/src/app.rs

Whichever merges second has to resolve those by hand. Could this merge behind the stack? The stack is six stacked PRs across 34 commits, so a conflict there has to be re-resolved up the whole chain, whereas resolving once here is contained to a single PR. That makes stack-first the cheaper order.

If the stack can't be merged quickly and #879 needs to go first, the alternative we put to Rowena, Jason and Shailley is a release branch model, keeping the current live and next release branches in sync so neither blocks the other.

Flagging it now with the specific files so whoever merges first knows exactly what the other will hit.

@aram356 aram356 added this to the 202609 milestone Sep 14, 2026

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

This PR derives the Trusted Server app-config store ID from [stores.config].default in edgezero.toml at build time, replacing a hardcoded literal. The mechanism is sound and I verified it empirically in a worktree: editing edgezero.toml correctly re-triggers the bake via rerun-if-changed, an invalid default fails the build with a clear cargo::error, and EdgeZero's loader validates default is a member of ids. Tests are green (2662 core, 27+22 cloudflare) and all 20 CI checks pass.

The blocking findings are not about that mechanism. They are about claims in the new documentation and doc comments that contradict the code, plus a PR description that describes a change the diff does not contain.

Findings 1, 2, and 4 are the same underlying misunderstanding about how Fastly resolves store-name overrides, surfacing in three places. Fixing that one thing resolves most of the blocking set.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because the change spans multiple paragraphs, targets lines outside the diff, or depends on an answer to an open question.

Blocking

🔧 wrench

  • Docs assert Fastly cannot honor a __NAME override, but the code and this repo's own docs say it can — see inline at docs/guide/configuration.md:2112
  • Shared-account recipe sets only the write-side variable, producing a split-brain deployment — see inline at docs/guide/configuration.md:2127
  • PR description claims a Fastly adapter change that is not in the diff — see Cross-cutting below

❓ question

  • Doc comments repeat the "Fastly has no process environment" inaccuracy — see inline at crates/trusted-server-core/src/settings_data.rs:58

Non-blocking

♻️ refactor / 🏕 camp site / 📌 out of scope / 📝 note / 🌱 seedling / 🤔 thinking

  • Missing error says "missing string values" when the keys are simply absent — see inline at crates/trusted-server-adapter-cloudflare/src/app.rs:114
  • Stale "app_config" store literals in the file being cleaned up — see inline at crates/trusted-server-core/src/settings_data.rs:290
  • cloudflare.toml is a second manifest with a duplicate hardcoded default — see Cross-cutting below
  • CONFIG_BLOB_KEY aliased to a store id without an explanatory comment — see inline at crates/trusted-server-core/src/config_payload.rs:26
  • Nothing ties the CLI's push-time resolution to the build-time constant — see Cross-cutting below
  • config_env_var made pub solely for a test — see inline at crates/trusted-server-adapter-axum/src/platform.rs:31

Cross-cutting / body-level findings

  • 🔧 PR description claims a Fastly adapter change that is not in the diff — The description's change table says crates/trusted-server-adapter-fastly/src/main.rs was changed to "Open the resolved manifest-default config store instead of a separate hardcoded name." That file is not in the diff. git diff 6cae7f5da..HEAD -- crates/trusted-server-adapter-fastly/ is empty — the entire directory is byte-identical to the merge base. DEFAULT_CONFIG_STORE_ID already existed on base at crates/trusted-server-core/src/settings_data.rs:13 (as a literal), and the Fastly adapter already consumed it at app.rs:167-168. This PR moves that constant and changes its source from a literal to a build-script env!; it does not change Fastly's store selection behavior at all. Please correct the description — a reviewer trusting it would look for a Fastly change that isn't there, and it overstates the PR's blast radius.

  • 📌 cloudflare.toml duplicates the default the PR just centralized — crates/trusted-server-adapter-cloudflare/cloudflare.toml:15 declares [stores.config] name = "trusted_server_config", a second manifest carrying the same default as a hardcoded literal. The new parity test (settings_data.rs:290-316) only checks edgezero.toml, so if [stores.config].default ever changes, this file silently diverges and nothing catches it. The same gap applies to the wrangler.toml:28 placeholder this PR hand-edited from {"app_config":""} to {"trusted_server_config":""} — it is now correct, but nothing ties it to CONFIG_BLOB_KEY. Both are TOML and cannot reference a Rust const, so a test asserting the parsed value matches the constant is the realistic guard.

  • 🌱 No test spans the compile-time / run-time seam — The runtime bakes the logical id at compile time via build.rs. ts config push re-derives it from edgezero.toml at run time (edgezero-cli resolves declaration.default_id() on each invocation). Same source of truth, two independent mechanisms, and no test asserts they agree. The new manifest-parity test is a genuine improvement here — it catches a stale build cache — but it only proves the compiled constant matches the manifest. A --dry-run assertion in the integration tests comparing the CLI's resolved store/key against DEFAULT_CONFIG_STORE_ID / CONFIG_BLOB_KEY would close the loop.

CI Status

  • integration tests: PASS
  • browser integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • CodeQL: PASS
  • Analyze (actions): PASS
  • Analyze (rust): PASS
  • Analyze (javascript-typescript): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test: PASS (required)
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • cargo fmt: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • vitest: PASS
  • CLAUDE.md symlink guard: PASS
  • prepare integration artifacts: PASS

Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs Outdated
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs
Comment thread crates/trusted-server-core/src/config_payload.rs
Comment thread crates/trusted-server-adapter-axum/src/platform.rs
@ChristianPavilonis

Copy link
Copy Markdown
Collaborator Author

Pushed fd290ac and replied to all seven outstanding inline threads, then resolved them.

The review's body-level findings are also addressed:

  • Corrected the PR description. Fastly already consumed the shared constant and supported service-scoped overrides on the base branch; this PR changes the constant's source, not Fastly adapter behavior.
  • Added parsed parity checks for cloudflare.toml and the Wrangler config JSON placeholder.
  • Added CLI integration tests that copy the repository's store declarations into a temporary Axum project and inspect the resulting local file and blob key against the compiled constants. Coverage includes physical-name resolution, explicit --key, and the fact that a runtime-only __KEY override does not move the CLI write destination.

Local validation passed: Fastly, Axum, Cloudflare, and CLI tests; Fastly, Axum, Cloudflare native/WASM, and CLI Clippy; Rust formatting; docs formatting, lint, and build; and git diff --check. The Cloudflare diagnostic assertion failed before the wording fix and passed afterward.

Live Fastly provisioning was not tested. Rustdoc completed with warnings in unchanged code. The new push's CI results are separate from these local checks.

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Fifth pass, against 65ddf859. All seven findings from the previous round check out as genuinely addressed in fd290ac6 — I verified each against the pinned EdgeZero v0.0.8 source rather than the replies: the Fastly __NAME docs now describe the real edgezero_runtime_env path (edgezero-adapter-fastly/src/lib.rs:217-257 reads only service-scoped keys), the shared-account recipe persists the mapping through ts provision (edgezero-adapter-fastly/src/cli.rs:1617-1620 rejects non-default mappings without a service namespace, :1603 enforces manifest / FASTLY_SERVICE_ID agreement, :3744-3748 confirms reconciliation), the settings_data doc comments no longer claim Fastly ignores overrides, the Cloudflare Missing diagnostic names absent properties and asserts its rendered text, all four stale "app_config" store-name literals are gone, and CONFIG_BLOB_KEY's intentional equality is documented.

The build-time bake is sound and I probed it directly: editing [stores.config].default re-triggers rerun-if-changed and the compiled constant follows, while the tripwire test fails as designed; deleting [stores.config] fails the build with a clear cargo::error. Test coverage now pins both ends — the compiled constant against the manifest, and the CLI's actual write destination under __NAME, __KEY, and --key, which is exactly where the earlier rounds found divergence.

Approving. Nothing here blocks merge — three low-severity notes below.

2 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch for both) to apply them as commits on the PR branch. Each was applied individually to a scratch worktree at this head and verified (cargo fmt --all -- --check and cargo check -p trusted-server-core plus a re-probe of both build-script error paths for the build.rs one; the pinned docs/node_modules Prettier for the docs one), with byte-exact pre/post-verification patch snapshots. The third comment is prose-only.

Non-blocking

🤔 thinking

  • Empty primary property shadows a working legacy value — see inline at crates/trusted-server-adapter-cloudflare/src/app.rs:166

⛏ nitpick

  • Rollback paragraph misfiled under "Local development" — see inline at docs/guide/configuration.md:2314
  • Build-script error prints an unnormalized ../.. path — see inline at crates/trusted-server-core/build.rs:10-14

CI Status

  • cargo fmt: PASS
  • cargo test: PASS
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • integration tests: PASS
  • browser integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • vitest: PASS
  • format-typescript: PASS
  • format-docs: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (javascript-typescript): PASS
  • Analyze (actions): PASS
  • CLAUDE.md symlink guard: PASS

Comment thread docs/guide/configuration.md
Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs
Comment thread crates/trusted-server-core/build.rs Outdated

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Second review pass, against head 65ddf859d (the previous pass reviewed c60468d10).

Every finding from my previous review is resolved, and I verified each against the new head rather than relying on the commit message. The doc comments and the rewritten Fastly section are now accurate: I re-checked all eleven claims in the rewritten ## Fastly Runtime Config Store section against edgezero at the pinned rev 5679641, including the service-scoped selector format, the config-only __KEY rule, the service-ID mismatch rejection, and the reconcile-deletes-omitted-overrides warning. All hold. docs/guide/fastly.md now cross-references configuration.md instead of contradicting it. The two new test files genuinely run and pass (3 CLI, 2 Cloudflare manifest), and the CLI test's env_clear() isolation is real — I re-ran it with EDGEZERO__…__NAME/__KEY set to garbage and it still passed.

One blocking defect remains, in code this PR introduces. cloudflare_config_envelope treats an empty-string primary property as present, so it shadows a populated legacy app_config value — defeating the fallback in exactly the migration case the fallback exists to serve. This PR also changes wrangler.toml's placeholder to {"trusted_server_config":""}, which is the precise value that triggers it.

1 of the inline comments below carries a one-click GitHub suggestion. The remaining comments describe the fix in prose because the change targets lines outside the diff hunks or is an observation rather than a request.

Blocking

🔧 wrench

  • Blank primary property shadows the legacy fallback — see inline at crates/trusted-server-adapter-cloudflare/src/app.rs:165

Non-blocking

⛏ nitpick / 🌱 seedling

  • build.rs renders an unnormalized path in its error messages — see inline at crates/trusted-server-core/build.rs:14
  • Rollback paragraph absorbed into the local-development subsection — see Cross-cutting below
  • Manifest-parity test pins the literal value — see inline at crates/trusted-server-core/src/settings_data.rs:313

Cross-cutting / body-level findings

  • ⛏ Rollback paragraph absorbed into the local-development subsection — Promoting **Local development** to a ### Local development heading left the closing rollback paragraph (docs/guide/configuration.md:2314, "Rollback to the legacy entry point is no longer controlled by runtime config keys…") reading as the end of the local-dev subsection, when it is about production service-version rollback. A sibling ### Rollback heading restores the original section-level reading. This duplicates an existing unresolved thread from @prk-Jr; noting it here for completeness rather than re-posting inline.

  • 📝 Previous-pass findings, all verified resolved at this head — For the record, since the verdict rests on what remains rather than what was fixed: the false "do not use ts provision" claim and its rationale are gone; the shared-account recipe now documents the service-scoped selectors and both required resource links; the PR description no longer lists a Fastly adapter change that was not in the diff; both settings_data.rs doc comments now describe the edgezero_runtime_env path correctly; the Cloudflare Missing message is corrected and now has a message assertion; all four stale StoreName::from("app_config") literals now use DEFAULT_CONFIG_STORE_ID; cloudflare.toml and the wrangler.toml placeholder are pinned by the new tests/config_defaults.rs; CONFIG_BLOB_KEY's alias is explained; tests/config_store_defaults.rs closes the compile-time/run-time seam by driving the real ts binary; and the getting-started.md triple-literal is now explained with unset guards.

  • 📝 Local verification at this head — cargo fmt --all -- --check clean; cargo clippy-cloudflare and cargo clippy-cloudflare-wasm clean; cargo test-fastly (2696 core + 191 + others), cargo test-axum, cargo test-cloudflare all pass; the new CLI test file passes on aarch64-apple-darwin.

CI Status

All 20 checks reported by gh pr checks are PASS:

  • integration tests: PASS
  • browser integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • CodeQL: PASS
  • Analyze (actions): PASS
  • Analyze (rust): PASS
  • Analyze (javascript-typescript): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test: PASS (required)
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • cargo fmt: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • vitest: PASS
  • CLAUDE.md symlink guard: PASS
  • prepare integration artifacts: PASS

Comment thread crates/trusted-server-adapter-cloudflare/src/app.rs Outdated
Comment thread crates/trusted-server-core/build.rs Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs
aram356 added a commit that referenced this pull request Sep 24, 2026
# Conflicts:
#	crates/trusted-server-integration-tests/fixtures/configs/viceroy-template.toml
#	docs/guide/fastly.md
aram356 added a commit that referenced this pull request Sep 24, 2026
This reverts commit 9b4a994, reversing
changes made to eda2379.
aram356 added a commit that referenced this pull request Sep 24, 2026
aram356 added a commit that referenced this pull request Sep 24, 2026
EdgeZero PR 381's store-selector work made a runtime
EDGEZERO__STORES__CONFIG__<ID>__KEY override move the CLI's config push
destination on its own, with no --key flag. Under v0.0.8 it did not, which
is what this test asserted when PR #879 was written.

Record the pinned runtime's behavior so the release candidate is green, and
flag the change for upstream review rather than leaving the suite red.

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Third review pass, against head c15938688 (previous passes reviewed c60468d10 and 65ddf859d).

Verdict: approving. Every finding from the previous two passes is resolved, and I verified each empirically rather than reading the diff. All 20 CI checks pass, every review thread on this PR is resolved, and cargo fmt, cargo test-fastly (2756 core tests), cargo test-cloudflare (28 lib + 23 integration), and both Cloudflare clippy targets are clean locally at this head.

Verification of the round-two blocking fix:

  • cloudflare_config_treats_blank_primary_as_absent is a genuine regression guard, not just a passing test. I reverted only the .filter(...) and re-ran it: it fails (left: Ok(""), right: Ok("legacy-envelope")), then passes again once restored.
  • The build.rs path fix works. I triggered the error path by deleting [stores.config] from the manifest, and the diagnostic now renders /…/pr-879-review/edgezero.toml with no ../.. segment, while the missing-section guard still fires correctly.
  • ### Rollback is now a sibling heading under ## Fastly Runtime Config Store, so the production-rollback paragraph no longer reads as the tail of ### Local development.

The two comments below are not objections to this PR. They record a forward-compatibility coupling with edgezero PR #381 ("Bind Fastly stores per deployment environment"), which I read at 3c82f338b. Both are follow-up work; neither blocks merging, and both are marked resolvable so they can be closed once a tracking issue exists.

Non-blocking

📌 out of scope

  • edgezero #381 retires the Fastly __KEY workflow this section documents — see inline at docs/guide/configuration.md:2717
  • Doc comment describes a Fastly mechanism edgezero #381 removes — see inline at crates/trusted-server-core/src/settings_data.rs:57

Cross-cutting / body-level findings

  • 📌 Follow-up: migrate off runtime_env_config when edgezero #381 lands — This PR's compiled-constant design is forward-compatible and, under #381, becomes the required semantics rather than a convention: the Fastly config registry there does FastlyConfigStore::try_open(id) with default_key: (*id).to_owned(), making logical-ID-as-key a hard rule. The manifest API this PR's build.rs depends on (ManifestLoader::from_path, manifest().stores.config, StoreDeclaration { ids, default }, default_id()) is byte-identical on #381, so the build script needs no change.

    What does break is outside this PR's diff. runtime_env_config and edgezero_runtime_env are deleted outright on #381, and dispatch_with_registries drops its env: &EnvConfig parameter, so the Fastly runtime reads no environment override at all. Trusted Server calls runtime_env_config in four places, none of them touched here: crates/trusted-server-adapter-fastly/src/main.rs:5,93 and crates/trusted-server-adapter-fastly/src/app.rs:93,1346. edgezero ships migration guidance for exactly this in docs/guide/adapters/fastly.md under "Migrating a custom entrypoint" (remove the call, drop the env argument, initialise logging from [adapters.fastly.logging]).

    Worth filing a single tracking issue covering the four call sites plus the documentation updates in the two inline comments, so the coupling is recorded rather than rediscovered at bump time. Also note for whoever owns the deploy pipeline: #381 requires the release-backed --application-release form for any app declaring config/KV/secret stores, which Trusted Server does.

  • 📝 The new CLI tests are unaffected by #381 — Recording this because it is the non-obvious half. crates/trusted-server-cli/tests/config_store_defaults.rs exercises __KEY overrides, which #381 makes a hard error on Fastly via a new Adapter::validate_config_key_for_target hook. These tests pass --adapter axum, the trait's default implementation returns Ok(()), and only the Fastly adapter overrides it. Axum, Cloudflare, and Spin all still honour __KEY at runtime, so these tests remain valid and should not be pre-emptively rewritten.

  • 📝 Local verification at this head — cargo fmt --all -- --check clean; cargo test-fastly 2756 core tests pass; cargo test-cloudflare 28 lib + 23 integration pass, including the new blank-primary guard; cargo clippy-cloudflare and cargo clippy-cloudflare-wasm clean. The build.rs error path and the blank-primary regression were both re-derived by deliberately breaking the tree and restoring it.

CI Status

All 20 checks reported by gh pr checks are PASS:

  • integration tests: PASS
  • browser integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • CodeQL: PASS
  • Analyze (actions): PASS
  • Analyze (rust): PASS
  • Analyze (javascript-typescript): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test: PASS (required)
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • cargo fmt: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • vitest: PASS
  • CLAUDE.md symlink guard: PASS
  • prepare integration artifacts: PASS

Comment thread docs/guide/configuration.md
Comment thread crates/trusted-server-core/src/settings_data.rs
@ChristianPavilonis
ChristianPavilonis merged commit d2dee4c into main Sep 28, 2026
20 checks passed
@aram356
aram356 deleted the fix/honor-ez-app-config branch September 29, 2026 14:59
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.

Honor edgezero.toml config store id for Trusted Server app config

4 participants