From 6d55875645147a356abae2d4eafa0c3a6ac3079f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Mazoni?= <mazoni.andre@gmail.com>
Date: Wed, 8 Jul 2026 11:50:44 +0930
Subject: [PATCH] fix(store): keep pre-#530 runs loadable (serde default on
 run.prepare) (#554)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## 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>
---
 lib/crates/fabro-types/src/settings/run.rs | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs
index 95b3a2282..6d00f7e2b 100644
--- a/lib/crates/fabro-types/src/settings/run.rs
+++ b/lib/crates/fabro-types/src/settings/run.rs
@@ -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 {