fix(desktop): emit camelCase config-write payload fields - #6062
Conversation
themiguelamador
left a comment
There was a problem hiding this comment.
I found and fixed two minor review issues:
- P3 — Keep the patch scoped to the broken wire contract.
rename_all_fieldswas also added toConfigFieldType, but its only payload field (options) is already a single camel-case word. That change has no effect today and silently chooses serialization behavior for future fields unrelated to this bug, so I removed it and its non-regression test. - P3 — Assert JSON semantics, not serializer property order. The new tests compared exact serialized strings, coupling the contract tests to object-key order even though JSON object order is not semantic. I changed them to whole-value
serde_json::Valuecomparisons, which still verify every key and value (including nested fields) without overconstraining the encoder.
Fix branch: https://github.com/Complear/buzz/tree/review/pr-6062-fix
Commit: d0f57d1de
Validation: full Tauri library suite (2,443 passed, 15 ignored), strict Tauri workspace/all-target clippy, focused wire-format tests, cargo fmt, desktop typecheck, full desktop checks, and git diff --check all passed. The desktop checks reported only the four pre-existing advisory Biome diagnostics outside this PR.
Review finding (P3, themiguelamador on block#6062): `rename_all_fields` was also added to `ConfigFieldType`, whose only payload field is `options` — already a single camel-case word. Verified inert: the other three variants are unit variants, and `desktop/src/shared/api/types.ts` declares the enum as `{ type: "enum"; options: string[] }`, so the attribute changes no byte the renderer sees today. It only pre-decides serialization for fields that do not exist yet and are not part of this bug. Removed it and the test that pinned it — that test was explicitly a shape pin, not a non-regression; nothing it asserted could have failed. `ConfigWriteMechanism` keeps the attribute: there it is the fix. Signed-off-by: Taksh <takshkothari09@gmail.com>
Review finding (P3, themiguelamador on block#6062): the wire-format tests compared exact serialized strings, which couples them to the encoder's object-key order even though JSON object order is not semantic. A field reordered in the struct would fail these tests without changing anything a reader sees. They now compare whole `serde_json::Value`s built with `json!`. That still verifies every key and value, nested ones included, and still catches a renamed variant or field — which a key-set assertion would not — without overconstraining the encoder. The round-trip test keeps its string literals: there the exact input bytes are the point, since it asserts what the renderer sends is accepted and the pre-fix snake_case spelling is not. Signed-off-by: Taksh <takshkothari09@gmail.com>
|
Both applied, one commit each ( P3 — scope. Verified inert before removing it: Worth stating the trade-off you're accepting, since it's the same footgun this PR exists to close: a future multi-word field on P3 — JSON semantics. Agreed, key order isn't semantic. The two serialization tests now compare whole Verification: full Tauri lib suite (2,443 passed, 15 ignored), strict workspace/all-target clippy, and |
|
🤖 LGTM at reviewed head This is a narrow, correct serialization-contract fix: it makes the nested Rust fields ( Local verification passed the full Desktop JavaScript suite (4,954 tests), full Desktop Tauri suite (2,544 passed, 16 ignored), and causal mutations that restore the original bug when the Serde attribute is removed. No merge blockers found in the code review. GitHub CI is still finishing at the time of this comment. |
ravarora2
left a comment
There was a problem hiding this comment.
lgtm, will wait for owners for final merge
On an internally tagged enum, serde's rename_all renames the variants, not
the variants' fields. ConfigWriteMechanism carried only rename_all, so it
serialized as
{"type":"respawnWithEnvVar","env_key":"GOOSE_MODE"}
while desktop/src/shared/api/types.ts declares envKey — likewise configId
and configKey. The variant names were right, so the type discriminant and
every switch on it behaved, and the enclosing NormalizedField's own fields
renamed correctly; only the variant payload was snake_case.
invokeTauri<T> is an unchecked cast, so tsc reports nothing: whoever wires
the config write-back path would read writeVia.envKey, get undefined, and
have a green typecheck.
rename_all_fields is added to ConfigFieldType too. Its only payload field is
single-word today, so that is not a fix — it is the attribute the next
multi-word field would silently need.
Refs block#6015
Signed-off-by: Taksh <takshkothari09@gmail.com>
Whole-value assertions against the TypeScript contract, not key-set checks: a key-set assertion still passes when the variant name regresses, and the type discriminant is what every switch (writeVia.type) reads. Four cases: all five ConfigWriteMechanism variants; the nested NormalizedField, which is the shape the renderer actually receives and where the mismatch hid, since the enclosing struct renamed correctly; a round-trip that also asserts the old snake_case spelling is now rejected, so a revert cannot quietly keep deserializing; and ConfigFieldType's payload variant. Removing rename_all_fields turns three of the four red. Refs block#6015 Signed-off-by: Taksh <takshkothari09@gmail.com>
The mock already hand-wrote the camelCase shape at 20 sites, so it agreed with api/types.ts while the real serializer emitted env_key — a test against it certified a contract nothing produced. Name the Rust test that now pins the bytes, so the two move together. Refs block#6015 Signed-off-by: Taksh <takshkothari09@gmail.com>
Review finding (P3, themiguelamador on block#6062): `rename_all_fields` was also added to `ConfigFieldType`, whose only payload field is `options` — already a single camel-case word. Verified inert: the other three variants are unit variants, and `desktop/src/shared/api/types.ts` declares the enum as `{ type: "enum"; options: string[] }`, so the attribute changes no byte the renderer sees today. It only pre-decides serialization for fields that do not exist yet and are not part of this bug. Removed it and the test that pinned it — that test was explicitly a shape pin, not a non-regression; nothing it asserted could have failed. `ConfigWriteMechanism` keeps the attribute: there it is the fix. Signed-off-by: Taksh <takshkothari09@gmail.com>
Review finding (P3, themiguelamador on block#6062): the wire-format tests compared exact serialized strings, which couples them to the encoder's object-key order even though JSON object order is not semantic. A field reordered in the struct would fail these tests without changing anything a reader sees. They now compare whole `serde_json::Value`s built with `json!`. That still verifies every key and value, nested ones included, and still catches a renamed variant or field — which a key-set assertion would not — without overconstraining the encoder. The round-trip test keeps its string literals: there the exact input bytes are the point, since it asserts what the renderer sends is accepted and the pre-fix snake_case spelling is not. Signed-off-by: Taksh <takshkothari09@gmail.com>
935d6a4 to
0f526ff
Compare
* origin/main: (43 commits) perf(desktop): parallelize relay agent directory rebuild (block#6258) Refine the mobile emoji picker (block#5853) fix(desktop): exclude archived agents from nest, order regeneration (block#5905) Add font size and conversation density preferences (block#5644) fix(desktop): emit camelCase config-write payload fields (block#6062) fix(desktop): downscale large avatars for agent-share PNG body (block#6260) fix(desktop): preserve early relay auth challenges (block#3320) Polish mobile message actions (block#5873) Refine mobile pairing confirmation (block#6018) chore(scripts): add buzz-adopt-prod-agents.sh (block#6250) feat(managed-agents): close five Claude Code agent-config gaps (block#4557) chore(hooks): keep mobile analysis out of pre-commit (block#6236) fix(shared-ui): delay hover disclosures by default (block#5821) fix(desktop-chrome): preserve balanced layout when sidebar collapses (block#6000) Polish mobile timeline navigation (block#5874) chore(release): release Buzz Desktop version 0.5.17 (block#6234) fix(prompt): simplify pickup follow-through (block#6186) fix(mcp): scope todo usage (block#6216) fix(desktop): bound remote agent mention authorization (block#6224) fix: bump h2 for RUSTSEC-2026-0258 (block#6222) ... Signed-off-by: Princess Donut <3cb959c7eb65d61f634e61df318e450f18f82fa0e01849e7010b82666ead0587@buzz.block.builderlab.xyz> # Conflicts: # desktop/src/main.tsx # mobile/ios/Podfile.lock
Fixes #6015.
ConfigWriteMechanismis internally tagged and carried onlyrename_all = "camelCase". On an internally tagged enum that renames the variants, never the variants' fields, so the payload went out snake_case:against
envKey/configId/configKeyindesktop/src/shared/api/types.ts:615-620. That output is a probe run of the real module before the fix, not a reading of the code.What makes it read as correct is the asymmetry: the variant names rename fine, so the
typediscriminant and everyswitch (writeVia.type)behave; and the enclosingNormalizedField's own fields (writeVia,overriddenValue,isRequired) rename fine too, becauserename_alldoes apply to struct fields. Only the variant's field is wrong.Adding
rename_all_fields = "camelCase"fixes it.rename_all_fieldsappeared zero times indesktop/src-tauribefore this.Severity, stated plainly: latent, not currently user-visible. Nothing in
desktop/src/**reads.envKey/.configId/.configKeyoff awriteVia—AgentConfigPanel.tsxis the onlyRuntimeConfigSurfaceconsumer and never touches the field, and no Rust code deserializes the type either. The write-back path these fields exist for is not wired yet. The hazard is for whoever wires it:invokeTauri<T>is an unchecked cast, so they getundefinedwith a greentsc.I also carried the attribute onto
ConfigFieldType. Its only payload field isoptions, single-word, so that half is not a fix — it is the attribute the next multi-word field would silently need.One divergence from the issue's suggested step 3: the 20
e2eBridge.tssites already emit camelCase, and camelCase is the contract, so they are correct as written — changing them would have been wrong. What they lacked was provenance, since agreeing withapi/types.tswhile the backend emitted something else is exactly what let this sit. They now name the Rust test that pins the bytes.Tests (
wire_format_tests, 4 cases, whole-value not key-set — a key-set assertion still passes when a variant name regresses):NormalizedField, which is the shape the renderer actually receives;env_keyspelling is now rejected, so a revert cannot quietly keep deserializing;ConfigFieldType::Enum.Removing
rename_all_fieldsagain turns three of the four red.Verified locally: full Tauri library suite 2444 passed / 15 ignored / 0 failed;
cargo clippy --manifest-path desktop/src-tauri/Cargo.toml --workspace --all-targets -- -D warningsclean;cargo fmt --manifest-path desktop/src-tauri/Cargo.toml --all -- --check; indesktop/:pnpm typecheck,pnpm check,pnpm test4954 passed;git diff --check. Not run: the app itself — there is no UI path to this field yet, which is the same reason the bug is latent.