fix(store): keep pre-#530 runs loadable (serde default on run.prepare) (#554)

## Summary

Runs created before #530 disappear from the run list after upgrading,
because
their persisted `run.created` event can no longer be deserialized.

#530 renamed `RunPrepareSettings`'s field from `commands: Vec<String>`
to
`steps: Vec<PreparedStep>`. That struct is persisted inside the
`run.created`
event (`WorkflowSettings.run.prepare`). Events written by older versions
carry a
`prepare` object with a `commands` key and **no** `steps` key. Because
`steps`
had no serde default, deserializing such an event fails with:

```
Serialization error: missing field `steps`
```

`warm_projection_cache` catches that error per-run and **skips** the run
(`fabro_store::slate: Skipping run during projection cache warmup`), so
every
pre-#530 run silently vanishes from the run list. The event data is
intact on
disk — it just can't be read back.

This is an event-schema back-compat break: any type persisted in an
event must
stay readable across the field renames/additions that happen after it
was
written.

## Fix

Add `#[serde(default)]` at the container level on `RunPrepareSettings`,
so a
`prepare` object missing `steps` (and/or `timeout_ms`) falls back to the
existing `Default` impl (empty steps, product-default timeout) instead
of
failing the whole run. The unknown legacy `commands` key is ignored (the
struct
has no `deny_unknown_fields`).

- New runs always serialize explicit `steps`, so nothing changes for
them — the
  #530 feature is unaffected.
- Pre-#530 runs load again with an empty prepare phase, which is
faithful: those
  runs already executed; this only rebuilds a read model for display.

`#[serde(default)]` is already the evolution idiom in this same struct
tree
(e.g. `RunModelSettings.controls`).

## Test plan

- [x] `cargo test -p fabro-types` — added two regression tests that
deserialize
the exact pre-#530 event shape (`{ commands, timeout_ms }`, no `steps`)
      and an empty object, asserting both load instead of erroring.
- [x] Built the patched server and pointed it at a real
`~/.fabro/storage` that
had 119 pre-#530 runs being skipped. After the fix, 0 runs are skipped
and
      all 119 appear in `GET /api/v1/runs`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Bryan Helmkamp <bryan@brynary.com>
This commit is contained in:
André Mazoni 2026-07-08 11:50:44 +09:30 committed by GitHub
parent df4fee4dff
commit 6d55875645
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -717,7 +717,17 @@ pub struct GitAuthorSettings {
pub email: Option<String>,
}
// `#[serde(default)]` at the container level: these settings are persisted
// inside the `run.created` event, so they must stay readable for events written
// by older fabro versions. `steps` replaced a `commands: Vec<String>` field in
// #530, so runs created before that have a `prepare` object with no `steps`
// key. Without a default, the whole run fails to deserialize during projection
// cache warmup and silently disappears from the run list. Falling back to the
// `Default` impl (empty steps, product-default timeout) keeps historical runs
// loadable; new runs always serialize explicit values, so nothing changes for
// them.
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[serde(default)]
pub struct RunPrepareSettings {
pub steps: Vec<PreparedStep>,
pub timeout_ms: u64,
@ -1872,6 +1882,34 @@ mod resolve_step_env_tests {
}
}
// Regression for the projection-cache-warmup drop: runs created before #530
// persisted `prepare` inside the `run.created` event as
// `{ "commands": [...], "timeout_ms": N }` — no `steps` key. Those old
// events must still deserialize (as an empty prepare phase) instead of
// failing the whole run's projection and silently vanishing from the run
// list.
#[test]
fn deserializes_pre_530_prepare_without_steps() {
let old = serde_json::json!({
"commands": ["echo build", "echo test"],
"timeout_ms": 60_000,
});
let settings: RunPrepareSettings = serde_json::from_value(old).unwrap();
assert!(settings.steps.is_empty());
assert_eq!(settings.timeout_ms, 60_000);
}
// Any absent field falls back to the product default, so no missing field
// can ever hide a run.
#[test]
fn deserializes_prepare_missing_every_field() {
let settings: RunPrepareSettings = serde_json::from_value(serde_json::json!({})).unwrap();
assert_eq!(settings, RunPrepareSettings::default());
}
#[test]
fn literal_step_passes_through() {
let settings = RunPrepareSettings {