mirror of
https://github.com/block/buzz.git
synced 2026-10-27 12:40:30 +00:00
fix(desktop): emit camelCase config-write payload fields (#6062)
Fixes #6015. `ConfigWriteMechanism` is internally tagged and carried only `rename_all = "camelCase"`. On an internally tagged enum that renames the **variants**, never the variants' fields, so the payload went out snake_case: ``` {"type":"respawnWithEnvVar","env_key":"K"} {"type":"acpSetConfigOption","config_id":"c"} {"type":"acpSetSessionModel"} {"type":"gooseNativeConfigWrite","config_key":"g"} {"type":"readOnly"} ``` against `envKey` / `configId` / `configKey` in `desktop/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 `type` discriminant and every `switch (writeVia.type)` behave; and the enclosing `NormalizedField`'s own fields (`writeVia`, `overriddenValue`, `isRequired`) rename fine too, because `rename_all` does apply to struct fields. Only the variant's field is wrong. Adding `rename_all_fields = "camelCase"` fixes it. `rename_all_fields` appeared zero times in `desktop/src-tauri` before this. **Severity, stated plainly: latent, not currently user-visible.** Nothing in `desktop/src/**` reads `.envKey`/`.configId`/`.configKey` off a `writeVia` — `AgentConfigPanel.tsx` is the only `RuntimeConfigSurface` consumer 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 get `undefined` with a green `tsc`. I also carried the attribute onto `ConfigFieldType`. Its only payload field is `options`, 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.ts` sites 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 with `api/types.ts` while 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): - every variant against the TypeScript spelling; - the nested `NormalizedField`, which is the shape the renderer actually receives; - a camelCase round-trip **plus** an assertion that the old `env_key` spelling is now rejected, so a revert cannot quietly keep deserializing; - `ConfigFieldType::Enum`. Removing `rename_all_fields` again 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 warnings` clean; `cargo fmt --manifest-path desktop/src-tauri/Cargo.toml --all -- --check`; in `desktop/`: `pnpm typecheck`, `pnpm check`, `pnpm test` 4954 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. --------- Signed-off-by: Taksh <takshkothari09@gmail.com>
This commit is contained in:
@@ -76,8 +76,21 @@ pub enum ConfigOrigin {
|
||||
}
|
||||
|
||||
/// How a config field can be written back to the runtime.
|
||||
///
|
||||
/// `rename_all_fields` is load-bearing, not decoration: on an internally
|
||||
/// tagged enum `rename_all` renames the *variants*, never the variants'
|
||||
/// fields, so without it `RespawnWithEnvVar` serializes as
|
||||
/// `{"type":"respawnWithEnvVar","env_key":"…"}` while
|
||||
/// `desktop/src/shared/api/types.ts` declares `envKey`. `invokeTauri<T>` is an
|
||||
/// unchecked cast, so `tsc` cannot see the mismatch — the reader just gets
|
||||
/// `undefined`. `wire_format_matches_typescript_contract` below pins the exact
|
||||
/// bytes.
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(tag = "type", rename_all = "camelCase")]
|
||||
#[serde(
|
||||
tag = "type",
|
||||
rename_all = "camelCase",
|
||||
rename_all_fields = "camelCase"
|
||||
)]
|
||||
pub enum ConfigWriteMechanism {
|
||||
/// Update record env vars, save, stop + restart agent.
|
||||
RespawnWithEnvVar { env_key: String },
|
||||
@@ -263,3 +276,105 @@ pub struct AcpModelEntry {
|
||||
pub name: Option<String>,
|
||||
pub description: Option<String>,
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod wire_format_tests {
|
||||
use super::*;
|
||||
use serde_json::json;
|
||||
|
||||
/// Every `ConfigWriteMechanism` variant, as `desktop/src/shared/api/types.ts`
|
||||
/// declares it. Whole-value comparison, not a key-set check: a key-set
|
||||
/// assertion still passes if the variant *name* regresses, and the `type`
|
||||
/// discriminant is what every `switch (writeVia.type)` reads. Compared as
|
||||
/// `serde_json::Value` rather than as text, because JSON object order is
|
||||
/// not semantic and the contract is the keys and values, not the encoder's
|
||||
/// field order.
|
||||
#[test]
|
||||
fn wire_format_matches_typescript_contract() {
|
||||
let cases = [
|
||||
(
|
||||
ConfigWriteMechanism::RespawnWithEnvVar {
|
||||
env_key: "GOOSE_MODE".into(),
|
||||
},
|
||||
json!({"type": "respawnWithEnvVar", "envKey": "GOOSE_MODE"}),
|
||||
),
|
||||
(
|
||||
ConfigWriteMechanism::AcpSetConfigOption {
|
||||
config_id: "model".into(),
|
||||
},
|
||||
json!({"type": "acpSetConfigOption", "configId": "model"}),
|
||||
),
|
||||
(
|
||||
ConfigWriteMechanism::AcpSetSessionModel,
|
||||
json!({"type": "acpSetSessionModel"}),
|
||||
),
|
||||
(
|
||||
ConfigWriteMechanism::GooseNativeConfigWrite {
|
||||
config_key: "goose.model".into(),
|
||||
},
|
||||
json!({"type": "gooseNativeConfigWrite", "configKey": "goose.model"}),
|
||||
),
|
||||
(ConfigWriteMechanism::ReadOnly, json!({"type": "readOnly"})),
|
||||
];
|
||||
for (mechanism, expected) in cases {
|
||||
assert_eq!(
|
||||
serde_json::to_value(&mechanism).expect("serialize"),
|
||||
expected
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// The renderer never sees a bare mechanism — it arrives nested inside
|
||||
/// `NormalizedField`, which is where the mismatch used to hide: the
|
||||
/// enclosing struct's `writeVia` / `overriddenValue` / `isRequired` all
|
||||
/// renamed correctly, so only the variant's own field was snake_case.
|
||||
#[test]
|
||||
fn nested_field_is_camel_case_all_the_way_down() {
|
||||
let field = NormalizedField {
|
||||
value: Some("v".into()),
|
||||
origin: ConfigOrigin::EnvVar,
|
||||
write_via: ConfigWriteMechanism::RespawnWithEnvVar {
|
||||
env_key: "GOOSE_MODE".into(),
|
||||
},
|
||||
overridden_value: Some("o".into()),
|
||||
overridden_origin: Some(ConfigOrigin::ConfigFile),
|
||||
is_required: true,
|
||||
};
|
||||
assert_eq!(
|
||||
serde_json::to_value(&field).expect("serialize"),
|
||||
json!({
|
||||
"value": "v",
|
||||
"origin": "envVar",
|
||||
"writeVia": {"type": "respawnWithEnvVar", "envKey": "GOOSE_MODE"},
|
||||
"overriddenValue": "o",
|
||||
"overriddenOrigin": "configFile",
|
||||
"isRequired": true,
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
/// The contract is singular: the shape the renderer sends back round-trips,
|
||||
/// and the old snake_case spelling is no longer accepted. Without the
|
||||
/// second half, a future revert would still deserialize and the read path
|
||||
/// would look healthy.
|
||||
#[test]
|
||||
fn camel_case_round_trips_and_snake_case_is_rejected() {
|
||||
let parsed: ConfigWriteMechanism =
|
||||
serde_json::from_str(r#"{"type":"respawnWithEnvVar","envKey":"GOOSE_MODE"}"#)
|
||||
.expect("the TypeScript shape must deserialize");
|
||||
assert_eq!(
|
||||
parsed,
|
||||
ConfigWriteMechanism::RespawnWithEnvVar {
|
||||
env_key: "GOOSE_MODE".into(),
|
||||
}
|
||||
);
|
||||
|
||||
assert!(
|
||||
serde_json::from_str::<ConfigWriteMechanism>(
|
||||
r#"{"type":"respawnWithEnvVar","env_key":"GOOSE_MODE"}"#
|
||||
)
|
||||
.is_err(),
|
||||
"the pre-fix snake_case spelling must not be accepted"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1817,6 +1817,12 @@ function buildMockConfigSurface(pubkey: string): {
|
||||
sources: Record<string, unknown>;
|
||||
} {
|
||||
// Goose running — mixed origins, override on model
|
||||
// The `writeVia` payloads below are camelCase because that is what the
|
||||
// backend emits — pinned by `wire_format_matches_typescript_contract` in
|
||||
// `desktop/src-tauri/src/managed_agents/config_bridge/types.rs`. This mock
|
||||
// agreed with `api/types.ts` while the real serializer emitted `env_key` /
|
||||
// `config_id` / `config_key`, so a test against it certified a contract
|
||||
// nothing produced. Change these only alongside that Rust test.
|
||||
const gooseSurface = {
|
||||
runtimeId: "goose",
|
||||
runtimeLabel: "Goose",
|
||||
|
||||
Reference in New Issue
Block a user