Repository navigation
Conversation
Build the harness CLI with the fixture feature used by its runtimes, and correct the Lockr troubleshooting field.
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Thanks for this. Strict field schemas for every known integration (enabled and disabled), the rejection of unknown IDs when validating or pushing, and the warn-and-continue behaviour at runtime are the right split. Configs already stored keep loading, and new typos are caught before deploy.
I checked:
deny_unknown_fieldsis not combined with#[serde(flatten)]on any config struct. The flattenedMaps intestlight.rsare on request and response bodies, notTestlightConfig, and the datadome flatten case was already handled.- Nested config structs are already strict (
JsAssetProxyAsset, and the datadome, prebid and aps sub-structs). validate_disabled_schemastill skips onlymissing fielderrors. Serde reports unknown fields while visiting the map, before it checks for missing ones, so minimal disabled blocks with a typo are rejected.deploy_validation_rejects_unknown_fields_in_minimal_disabled_integrationscovers this.- The
test-utilsfixture ID is gated, so production CLI builds keep the production ID list, and the EOF harness builds with the feature on. - The Lockr docs table matches
LockrConfig:app_idrequired withlength(min = 1),enableddefaulting to true, the TTL range of 60–86400, andrewrite_sdk_hostas a compatibility field.
The CHANGELOG and configuration guide spell out the push-config-first, deploy-binary-second migration clearly.
Two minor notes inline. Neither blocks merging.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
The strict known-field schemas and deploy/runtime integration-ID handling are consistent with the intended contract. The rollout instructions need a Fastly propagation step to avoid activating strict validation against an older config blob.
The inline finding is prose-only because its documentation correction spans multiple files; no one-click suggestions are included.
Blocking
🔧 wrench
- Wait for Fastly config propagation before activating the strict binary — see inline at
docs/guide/configuration.md:192–196
Verification
All 21 changed files were read completely. git diff --check passed. Passing remote CI was reused rather than rerun locally. The propagation risk was source-traced through the pinned EdgeZero dependency; live Fastly propagation was not reproduced.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (axum native): PASS
- format-typescript: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo fmt: PASS (required)
- HTML EOF runtime (Fastly): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- cargo test: PASS (required)
- CLAUDE.md symlink guard: PASS
- format-docs: PASS (required)
- Analyze (rust): PASS
- docs-proposal scripts: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- prepare integration artifacts: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
- Analyze (actions): PASS
Require Fastly config propagation before strict binary activation. Keep only unknown/missing-field details in outer integration parse messages, retaining other serde details in the report cause chain. Document per-request unknown-ID warnings on ordinary Fastly sandboxes.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Rejects unknown integration IDs during deploy validation and unknown fields in known configuration schemas, while retaining runtime warnings for unknown IDs and minimal disabled blocks. Reviewed all 21 changed files at 9690c166cc1053f4bca976a06ed7fe7aeeb9fa59; no actionable findings.
No inline comments or suggestions. Existing CI results were reused; no local test suites were rerun. The isolated review worktree is clean.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- trace runtime and browser acceptance: PASS
- integration tests: PASS
- CodeQL: PASS
- cargo test: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- docs-proposal scripts: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- worker-deadlines (cloudflare): PASS
- HTML EOF runtime (Fastly): PASS
- Analyze (actions): PASS
- vitest: PASS
- cargo test (axum native): PASS
- worker-deadlines (spin): PASS
- format-typescript: PASS (required)
- ts dev lint domains (changed lines): PASS
- CI Node pin guard: PASS
- format-docs: PASS (required)
- prepare integration artifacts: PASS
- CLAUDE.md symlink guard: PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (cross-adapter parity): PASS
- cargo fmt: PASS (required)
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- .github/dependabot.yml: PASS
|
@prk-Jr @aram356, directionally this change is not compatible with V2 (the 55 pull requests tracked in #777, merged in order). It makes the V1 settings format stricter, with known-field schemas for each The V2 spec is settings-and-consumers in #1342, tracked as #1403. A V1 settings document is rewritten into the V2 shape by Could you evaluate the V2 spec and #1409 before progressing this pull request? |
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This adds strict known-field schemas to every integration table, plus [tester_cookie] and [tinybird], rejects unknown integration IDs at validate and push time, and warns about them at runtime, matching the spec's acceptance table. The PR now conflicts with main, which blocks merging. Two pre-pass parsers also bypass the new error redaction.
1 of the inline comments below carries a one-click GitHub
suggestion. Use Commit suggestion to apply it. The other comments describe the fix in prose because it touches multiple files or lines outside the diff.
Blocking
🔧 wrench
- Resolve the merge conflicts with main: see Cross-cutting below
Non-blocking
♻️ refactor
- The two pre-pass parsers still echo invalid values: see inline at
crates/trusted-server-core/src/config.rs:318
🤔 thinking
- A nested missing field hides unknown keys in a disabled block: see inline at
crates/trusted-server-core/src/settings.rs:386
⛏ nitpick
- The
enablerow also matches the expected-fields list: see inline atcrates/trusted-server-cli/tests/config_env_overlay.rs:477
Cross-cutting / body-level findings
-
🔧 Resolve the merge conflicts with main:
CHANGELOG.mdanddocs/guide/integrations/lockr.mdconflict with #1405 and #1406, so GitHub reports the PR as not mergeable. #1406 rewrotelockr.md, and its field table already matchesLockrConfig(app_idrequired, the documented defaults,rewrite_sdk_hostdeprecated and ignored). Suggested resolution:-
CHANGELOG.md: keep both bullets. -
lockr.md: take main's version, then add one note under its field table. The guide at the merge base told operators to setorganization_idandproject_id, and configs that still carry them now fail validation and startup:Unknown fields are rejected, including the `organization_id` and `project_id` fields that earlier versions of this guide listed. Remove them and set `app_id`.
A trial merge with that resolution passes the native core suite (3,349 tests) and
config_env_overlay(11 tests). Thelockr.mdrow in the PR description will need a refresh afterwards. -
-
📌 Typo'd env-overlay variables are still ignored silently:
TRUSTED_SERVER__INTEGRATIONS__GPT__ENABLE=falseandTRUSTED_SERVER__INTEGRATIONS__PREBDI__ENABLED=trueboth leavets config validatepassing, because the EdgeZero overlay only applies to leaves that already exist in the TOML. It's the same class of silent typo this PR closes for TOML, one layer up, and belongs in EdgeZero as a follow-up rather than in this PR. -
📌 Prebid docs still show keys that strict validation rejects: this predates the PR (
PrebidBundleBuildConfigwas already strict on main), but it's the same copy-the-docs-then-fail path.docs/guide/integrations/prebid.md:784-786shows[integrations.prebid.bundle]withadaptersanduser_id_modules; the schema is[integrations.prebid.bundle.modules]withbidderanduser_id, asprebid.md:259-270itself describes.prebid.md:765anddocs/guide/configuration.md:2026namebundle.user_id_modulesin prose..env.example:46-57lists commented Prebid variables (SERVER_URL,BIDDERS,BID_PARAM_*,SUPPRESS_NURL_BIDDERS,AUTO_CONFIGURE,TEST_MODE) for keysPrebidIntegrationConfigdoesn't have. -
📝 Release-candidate configs carry keys main doesn't have: tag
v0.1.1-rc.202610.1isn't on main and accepts[tinybird] auction_enabledand Lockrcapture_identity. Configs from that RC will fail on a main-based binary carrying this PR. The documented step to runts config validatewith the new CLI before upgrading catches them, so no change is needed here; it's worth keeping in mind when RC operators move to a main-based release.
Verification
- All 21 changed files read in full at
9690c166cc1053f4bca976a06ed7fe7aeeb9fa59. deny_unknown_fieldscoverage: every struct reachable fromSettingsand every nested integration config struct is strict, and none usesflatten. Every integration ID the runtime reads is inDEPLOY_VALIDATED_INTEGRATION_IDS.- Mutation checks: removing Sourcepoint's
deny_unknown_fields, theparse_errordetail, the runtime warning, Tinybird'sdeny_unknown_fields, or the deploy ID check each fails at least one of the new tests. - Real-binary probes with
ts config validateagainst unknown-ID, unknown-field, and invalid-value configs, plus env-overlay typos. - A docs and committed-config scan (162 files) found no key or ID that the newly strict schemas reject, apart from the pre-existing Prebid drift above.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- trace runtime and browser acceptance: PASS
- integration tests: PASS
- CodeQL: PASS
- cargo test: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- docs-proposal scripts: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- worker-deadlines (cloudflare): PASS
- HTML EOF runtime (Fastly): PASS
- Analyze (actions): PASS
- vitest: PASS
- cargo test (axum native): PASS
- worker-deadlines (spin): PASS
- format-typescript: PASS (required)
- ts dev lint domains (changed lines): PASS
- CI Node pin guard: PASS
- format-docs: PASS (required)
- prepare integration artifacts: PASS
- CLAUDE.md symlink guard: PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (cross-adapter parity): PASS
- cargo fmt: PASS (required)
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- .github/dependabot.yml: PASS
| ids | ||
| } | ||
|
|
||
| fn validate_gpt_diagnostics_config(settings: &Settings) -> Result<(), Report<TrustedServerError>> { |
There was a problem hiding this comment.
♻️ The two pre-pass parsers still echo invalid values
parse_error keeps type and enum errors out of the outer message, but validate_gpt_diagnostics_config and validate_js_asset_proxy_config run before get_typed in both deploy and runtime validation, and they still format the whole serde error. With trace_page_enabled = "fictional-private-value" under [integrations.gpt_diagnostics], ts config validate prints:
integration startup failed for `gpt_diagnostics`: configuration could not be parsed: invalid type: string "fictional-private-value", expected a boolean
js_asset_proxy does the same, and now that JsAssetProxyConfig denies unknown fields, its unknown-field errors take this path too. Reusing the helper closes the gap and gives all 15 IDs one message shape:
// settings.rs
pub(crate) fn parse_error(
integration_id: &str,
error: serde_json::Error,
) -> Report<TrustedServerError> {
// config.rs: import `IntegrationSettings` from `crate::settings`, then in
// validate_gpt_diagnostics_config
let config: GptDiagnosticsConfig = serde_json::from_value(raw_config.clone())
.map_err(|error| IntegrationSettings::parse_error(GPT_DIAGNOSTICS_INTEGRATION_ID, error))?;
// and in validate_js_asset_proxy_config
let config: JsAssetProxyConfig = serde_json::from_value(raw_config.clone())
.map_err(|error| IntegrationSettings::parse_error(JS_ASSET_PROXY_INTEGRATION_ID, error))?;Regression test for the config.rs tests module (it fails on the current head):
#[test]
fn prepass_integration_parse_errors_hide_invalid_values() {
for (id, config) in [
(
GPT_DIAGNOSTICS_INTEGRATION_ID,
serde_json::json!({ "enabled": "fictional-private-value" }),
),
(
JS_ASSET_PROXY_INTEGRATION_ID,
serde_json::json!({ "enabled": true, "cache_ttl_seconds": "fictional-private-value" }),
),
] {
let mut settings = valid_settings();
settings.integrations.insert(id.to_owned(), config);
let message = validate_settings_for_deploy(&settings)
.expect_err("should reject an invalid pre-pass integration value")
.to_string();
assert!(
message.contains(&format!("Integration '{id}' configuration could not be parsed")),
"should name the integration: {message}"
);
assert!(
!message.contains("fictional-private-value"),
"should not echo invalid values in the outer error message: {message}"
);
}
}With both changes, the config::tests and settings::tests suites (278 tests) pass and cargo clippy-fastly is clean. Apply manually: the fix touches settings.rs and lines of config.rs outside this diff, so it can't be a one-click suggestion.
| "Integration '{integration_id}' configuration could not be parsed" | ||
| ), | ||
| })?; | ||
| T::validate_disabled_schema(raw) |
There was a problem hiding this comment.
🤔 A nested missing field hides unknown keys in a disabled block
validate_disabled_schema accepts any missing field error, and serde raises a nested struct's missing field as soon as it reaches that value, before visiting the remaining keys. So this disabled Prebid block passes validate_settings_for_deploy, typo included:
{ "enabled": false, "managed_user_ids": [{ "params": { "example_option": true } }], "timout_ms": 500 }Add "name": "sharedId" to the entry and the same block is rejected with unknown field `timout_ms` . The runtime map is sorted, so any key after the nested one is hidden. The CLI keeps document order, so it happens there when an inline table comes before the typo. Not blocking: the block is disabled, and enabling it reports both errors. If the "including disabled blocks" contract should hold exactly, serde_path_to_error (already in Cargo.lock, though it would be a new direct dependency for core) can tell a root-level missing field from a nested one, so only root-level ones are tolerated. Otherwise, a sentence in the spec noting the limit would cover it.
| let cases = [ | ||
| ("prebdi", "enabled", true, "prebdi"), | ||
| ("gpt", "rewrite_scripts", false, "rewrite_scripts"), | ||
| ("gpt", "enable", false, "enable"), |
There was a problem hiding this comment.
⛏ The enable row also matches the expected-fields list
The parse error ends with expected one of `enabled`, ..., so stderr.contains("enable") holds even if the unknown key isn't named. Matching the whole phrase pins it:
| ("gpt", "enable", false, "enable"), | |
| ("gpt", "enable", false, "unknown field `enable`"), |
Summary
Known tables containing unknown fields will fail startup with the new binary. Run the new CLI's
ts config validate, correct the reported settings, push the cleaned config while the old binary runs. On Fastly, wait for the config-store propagation window to elapse before activating the new binary; a successful push does not mean every point of presence (POP) has the cleaned config.Changes
crates/trusted-server-core/src/config.rscrates/trusted-server-core/src/settings.rscrates/trusted-server-core/src/integrations/adserver_mock.rscrates/trusted-server-core/src/integrations/didomi.rscrates/trusted-server-core/src/integrations/google_tag_manager.rscrates/trusted-server-core/src/integrations/gpt.rscrates/trusted-server-core/src/integrations/js_asset_proxy.rscrates/trusted-server-core/src/integrations/lockr.rscrates/trusted-server-core/src/integrations/nextjs/mod.rscrates/trusted-server-core/src/integrations/permutive.rscrates/trusted-server-core/src/integrations/sourcepoint.rscrates/trusted-server-core/src/integrations/testlight.rscrates/trusted-server-core/src/test_support/async_html_finalization.rscrates/trusted-server-integration-tests/tests/html_eof_runtime.rscrates/trusted-server-cli/tests/config_env_overlay.rsdocs/guide/configuration.mddocs/guide/integrations/lockr.mdapp_id.docs/guide/error-reference.mdCHANGELOG.mddocs/superpowers/specs/2026-10-27-strict-integration-validation-design.mddocs/superpowers/plans/2026-10-27-strict-integration-validation.mdCloses
Closes #1201
Spec
Not a feature (bug fix). The technical design is in
docs/superpowers/specs/2026-10-27-strict-integration-validation-design.md.Test plan
Run with Rust 1.98.1, Node 24.20.0 and Viceroy 0.21.0. Initial sandbox listener/keychain-access failures were resolved by full reruns with the required local access.
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run— 1,232 tests passed with one worker, no type errorscd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo test-fastly-reuse,cargo test-cloudflare,cargo test-spin./scripts/async-html-finalization-local-test.sh axum --auction./scripts/async-html-finalization-local-test.sh fastly --release --auction— includes the test-utils release WASM buildfastly compute serve— not performedRegressions distinguish the original acceptance/diagnostic failures and cover all 15 production integration IDs, enabled/disabled fields, runtime warnings, open payload maps and deprecated compatibility fields.
The EOF harness builds its CLI with
trusted-server-core/test-utilsso validation recognizes the fixture integration selected by its runtimes. Ordinary production CLI builds retain the production ID list.Checklist
unwrap()in production codelogmacros (notprintln!)