Test FastlyPlatformSecretStore read path and write stubs#557
Test FastlyPlatformSecretStore read path and write stubs#557prk-Jr wants to merge 9 commits intofeature/edgezero-pr3-config-storefrom
Conversation
aram356
left a comment
There was a problem hiding this comment.
Summary
This PR introduces a full platform abstraction layer (+2553/-530 lines, 25 files) including crates/trusted-server-core/src/platform/, a new storage module, RuntimeServices threading, geo refactor, backend validation, and config store hardening. The code is well-structured with good documentation and proper error handling. Main concerns are the PR description accuracy and code duplication between adapter and core storage.
Blocking
🔧 wrench
-
PR description severely understates scope: The description claims "No implementation code is added — all production code already existed from prior PRs; this PR is tests only" touching one file. The actual diff is +2553/-530 across 25 files with significant production code changes (platform abstraction, storage module, RuntimeServices wiring,
fastly_storage.rsdeletion, geo refactor, backend validation). This makes the PR difficult to review accurately and could mislead approvers. Please rewrite the PR body to accurately describe the full scope. -
Code duplication between adapter and core storage: See inline comment on
config_store.rs:17.
❓ question
- URL-encoding behavioral change: See inline comment on
api_client.rs:22.
Non-blocking
🤔 thinking
UnsupportedvsNotImplementeddistinction (platform/error.rs:27-32): Test doubles returnUnsupported, adapter stubs returnNotImplemented. The distinction is documented but callers may not realistically branch on it.
♻️ refactor
LazyLockfor trivial conversion (request_signing/jwks.rs:17-18):LazyLockforStoreName::from(JWKS_CONFIG_STORE_NAME)is heavyweight for aStringfrom&str. Could be simplified ifStoreNamesupportedconst fnor ifget()accepted&str.
🌱 seedling
PlatformHttpClient: Send + Syncwith?Sendfutures (platform/http.rs:203-204): Correct for wasm32 and well-documented. May need revisiting if a multi-threaded adapter (e.g., Axum) is added.
👍 praise
- Newtype
StoreName/StoreId(platform/types.rs): Prevents accidental swaps between runtime edge names and management API identifiers. Clean implementation. - Graceful KV store degradation (
adapter-fastly/src/main.rs:66-79): Fallback toUnavailableKvStorekeeps non-synthetic routes working when KV is unavailable. - Control character validation in
BackendConfig(backend.rs:107-116): Prevents log/header injection attacks. Good security hardening. - Geo deduplication via
geo_from_fastly(geo.rs): Eliminates duplication between legacy and new paths. - Comprehensive test coverage: Platform stubs are well-tested including edge cases (empty host, custom timeout, nocert suffix, store failures).
⛏ nitpick
- PR checklist says "Uses
tracingmacros": Project convention in CLAUDE.md specifieslogmacros (withlog-fastlybackend), nottracing.
📝 note
- CLAUDE.md removes
attach_with()guidance:attach_with()is a validerror-stackAPI for lazy attachment via closure. Fine to remove if unused in this project. test_supportmodule ispub(crate)+#[cfg(test)]: Adapter crate can't reuse it, which is fine since it has its own stubs.
CI Status
- All checks: PASS
…o-pr4-secret-store
Summary
FastlyPlatformSecretStorein the Fastly adapter to prove the read path and write stubs satisfy issue Secret store trait (read-only) #485's "Done when" criteriaPlatformError::SecretStore),create()stub (PlatformError::NotImplemented), anddelete()stub (PlatformError::NotImplemented)Changes
crates/trusted-server-adapter-fastly/src/platform.rs#[cfg(test)]blockCloses
Closes #485
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest run— N/A (no JS changes)cd crates/js/lib && npm run format— N/A (no JS changes)cd docs && npm run format— N/A (no docs changes)cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1— N/A (#[cfg(test)]code is excluded from WASM binary)fastly compute serve— N/A (tests only)Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)