Repository navigation
fix(config): let a UI read config from a newer daemon - #413
Draft
hyperb1iss wants to merge 3 commits into
Draft
hyperb1iss wants to merge 3 commits into
hyperb1iss wants to merge 3 commits into
Conversation
Add HypercolorConfig::deserialize_strict, which wraps any deserializer in serde_ignored, collects every key the derive skipped, and refuses the document naming each one by its dotted path. Root extension sections and driver entries are open maps, so their keys are captured rather than skipped and keep loading. Route every decode that reads an authoritative source through it: the TOML file loader (ConfigManager::parse_toml, the only path that turns hypercolor.toml into a config), the env and CLI overlay pipeline, and both decodes in the REST config write handler. The section types still deny unknown fields here, so behavior is unchanged; this moves the rule to the decoders so the types can stop enforcing it on clients. serde_ignored is dtolnay's MIT OR Apache-2.0 crate and depends only on serde_core, which the lockfile already carries. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The config section types and SessionConfig carried deny_unknown_fields, so any UI older than the daemon it talks to failed the whole GET /api/v1/config decode on the first new key (state_history_generations was the one that surfaced it) and lost the entire Settings surface. That bites in production, not only in dev: the remote UI embeds this crate and connects to daemons of other versions. Drop the attribute everywhere in the config schema. Strictness now lives in HypercolorConfig::deserialize_strict, which the file loader, the startup overlays, and REST config writes already use, so a typo in hypercolor.toml, an unknown overlay key, or a PUT or PATCH naming a key the daemon does not model is still refused, now with the full dotted key path instead of a bare field name. The typo-rejection tests decode through the strict path, and new tests cover a client decoding an envelope that carries unknown keys in several sections, the loader naming the file and every key path, an unknown env overlay key failing the load, and the REST write handler refusing unknown keys on PUT and PATCH without touching the config. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The capture registry validator decoded its section with plain serde_json, so once the section types stopped denying unknown fields it silently accepted the retired monitor key it used to refuse. Move the serde_ignored decode into a config::strict module and expose a section-scoped twin, deserialize_section_strict, that names unknown keys under the section root (capture.monitor rather than monitor). HypercolorConfig::deserialize_strict and the capture validator share it, so both report through the same message. Client decodes are untouched and stay tolerant. Capture is the only section row with a validator; giving other rows one would flip their published has_validator flag, which promises checks beyond the type. The new tests cover the capture validator and the shared helper across audio, capture, and a nested rendering key, each asserting the plain decode of the same value still succeeds. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A web UI that is older than the daemon it talks to loses its whole Settings surface. The UI decodes
GET /api/v1/configintoHypercolorConfig, and every config section type carried#[serde(deny_unknown_fields)], so the moment a daemon gains a config key (daemon.state_history_generationsmost recently) the older UI's fetch fails withunknown fieldand nothing in Settings loads. That happens to anyone running a remote UI against another computer's daemon, and it happened on the owner's own dev stack, where a stale UI build logged the failure on every socket open. After this PR, clients decode the daemon's config tolerantly, and every authoritative source (the config file, startup overlays, REST writes) still refuses a key it does not model, now naming it by its full path.What this changes
deny_unknown_fieldsattribute is gone from the 13 config section types andSessionConfig, so the plainDeserializeimpl that clients use skips fields it does not know.HypercolorConfig::deserialize_strictanddeserialize_section_strict(in the newhypercolor-types/src/config/strict.rs), decode throughserde_ignored, collect every key the derive skipped, and refuse the document with a message such asunknown config keys `daemon.prot`, `rendering.servo_gpu_import.mdoe`.ConfigManager::parse_toml(the only path fromhypercolor.toml, used by load, reload, and the daemon's startup parse), dotted overlays inconfig/sources.rs, both decodes in the daemon's REST config writes, and the config registry's section validator.PUTandPATCH /api/v1/config/keys/{key}) still answer 422validation_error, now with the key path.extensionsmap and driversettingsmaps were already open and still accept arbitrary keys.Why
The repo's agent guide (
CLAUDE.md) asks clients to consume tolerant subsets of daemon payloads. The strictness exists for a reason: a typo inhypercolor.tomlshould fail loudly instead of vanishing on the next rewrite. That rule belongs to the loaders, so this PR moves it there. Astrict-configcargo feature gatingdeny_unknown_fieldswas considered and rejected: serde behavior that depends on a feature can change silently when Cargo unifies features across crates.Found while rehearsing the demo film against a live rig.
Verification
just verifypasses locally (Rust fmt + lint + test)just denypasses (required for dependency or license changes)just ui-testandjust ui-buildpass (required forcrates/hypercolor-ui/)just sdk-lint,just sdk-check, andjust sdk-buildpass (required forsdk/)just python-verifypasses (required forpython/)just compat-checkpasses (required fordata/drivers/vendors/*.toml)just docs-buildpasses (required for docs or README changes)cd docs && zola checkpasses (required for docs link/content changes)scripts/orpackaging/)just e2e-buildpasses with the normal Servo stack (required for daemon/UI/effect integration changes)just e2e-build-cpupasses when validating the CPU smoke fallbackjust e2epasses against the Servo stack (required for end-to-end behavior changes; starts daemon/browser)The full
just verifygate runs the daemon suite in parallel, which deadlocks on GPU tests on this machine, so the gates ran per crate instead:cargo test -p hypercolor-typesconfig_tests,config_sources_tests,config_persistence_tests(core)api_testsfiltered toconfig_, single-threaded-D warningson types, core, daemon (--all-targets)cargo check --workspaceand the UI--libcheck forwasm32-unknown-unknowncargo deny check,cargo fmt --checkThe UI's own code did not change (its decode becomes tolerant through the shared types), so
just ui-testandjust ui-builddid not run; only its lockfile gainedserde_ignored. The client regression test decodes a config payload carrying invented future keys in the daemon, audio, rendering, and session sections and checks the known values survive. An independent review audited every Rust file that references the config types for a remaining lenient authoritative path and found none; the one regression it raised (the registry validator) is fixed in the last commit.Notes for reviewers
x, expected one of ..." to the dotted key path, so the list of valid fields no longer appears. Nothing in the repo matched the old text.serde_ignoredcrate cannot see keys inside#[serde(flatten)]or untagged and internally tagged content. The config graph only flattens open maps today; a future closed struct placed behind one would lose strictness without a test failing.PATCH /api/v1/config/keys/session?live=falsewith{"not_a_real_key": null}still returns 200, because the merge treats null as "remove this member" and the unchanged-value check returns before any decode. The API response envelope types (ApiResponse,ResponseMeta) still deny unknown fields too, which is the same version-skew bug one level up. Both are tracked separately.🤖 Generated with Claude Code