feat(steps): inline step editor with main-protection + cascade-delete

[risjai]

Summary

• Add inline JSON editor for step request_body and response_body, with an always-confirm save modal that shows cascade-delete count up front
• Main-timeline steps are protected by default (git-like model) — saving triggers an auto-fork; REWIND_ALLOW_MAIN_EDITS=true env var unlocks direct editing
• Server recomputes request_hash via normalize_and_hash so cache replay-lookups remain stable after edits

Endpoints added

Method	Path	Description
PATCH	/api/steps/{id}/edit	Edit step blobs + cascade-delete downstream
GET	/api/steps/{id}/cascade-count	Dry-run: count downstream steps that would be deleted
POST	/api/sessions/{sid}/fork-and-edit-step	Atomic fork + edit in one transaction
GET	/api/health	Now includes allow_main_edits: bool

Test plan

• 9 new Rust integration tests (edit response, edit request + hash change, main blocked, main env bypass, cascade delete, cascade count match, body too large, missing body, fork-and-edit happy path + error cases)
• 6 new frontend tests (edit pencil visible, editor opens/cancels, invalid JSON disables save, unchanged text disables save, valid change enables save, response tab edit button)
• All 386 existing Rust tests pass
• All 183 web tests pass (was 177)
• TypeScript compiles cleanly
• Clippy clean
• Version bumped to 0.14.4 (Track 1: Cargo.toml + lock + CLI_VERSION x2 + MCP pyproject)

Made with Cursor

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
rewind	Ready	Preview, Comment	Apr 29, 2026 6:07am

[risjai]

Code review

Thanks for the quick turnaround on the plan. Vertical slices land cleanly and the Rust integration tests are well-scoped. Below are findings ordered by severity. Items marked HIGH should block; MEDIUM/LOW can be follow-ups but are worth a comment in the PR before merge.

---

HIGH 1 — fork-and-edit-step 400s for at_step = 1, breaking the most common auto-fork case

fork_and_edit_step calls engine.fork(..., body.at_step.saturating_sub(1), ...) (api.rs:1725).

When the user edits step #1 on main (the auto-fork branch), at_step = 1 → fork is called with at_step = 0. But ReplayEngine::fork explicitly rejects at_step == 0:

pub fn fork(&self, session_id: &str, source_timeline_id: &str, at_step: u32, label: &str) -> Result<Timeline> {
    let steps = self.get_full_timeline_steps(source_timeline_id, session_id)?;
    let total = u32::try_from(steps.len()).unwrap_or(u32::MAX);
    if at_step == 0 || at_step > total {
        bail!("Invalid fork step {}. Session has {} steps (use 1-{}).", at_step, steps.len(), steps.len());
    }

So clicking Save on the very first step of any session you didn't fork yourself gives the operator 400: Invalid fork step 0. Session has N steps (use 1-N). — confusing, and silently breaks the intended workflow.

There's no test covering it: test_fork_and_edit_step_at_zero tests at_step=0 → 400 (correct), but at_step=1 (the saturating_sub edge) isn't tested.

Fix options:

• Relax ReplayEngine::fork to permit at_step == 0 (an "empty" fork inheriting nothing) — then the existing handler logic works as written.
• OR special-case in fork_and_edit_step: if at_step == 1, create the fork with a different mechanism (build a fresh Timeline via Timeline::new_fork(_, 0, _) directly), then create the step.

Add a test for at_step=1 either way.

---

HIGH 2 — Neither edit path is wrapped in a SQLite transaction (plan T2 + T4 explicitly required this)

The plan committed to single-tx semantics:

Single transaction: BEGIN → update_step_blobs → delete_steps_after → COMMIT. Rolled back via store lock if any step fails.

patch_step does update_step_blobs and delete_steps_after as two separate autocommit calls (api.rs:1621-1628). Inside update_step_blobs itself, the two UPDATEs (request + response) are also separate autocommits (db.rs:830, 838).

fork_and_edit_step is worse: it does engine.fork() (writes timeline row), blobs.put() x2, then create_step() (api.rs:1725-1764). If create_step fails, you have an orphan fork timeline + orphan blobs and the operator sees a 500 — but the bogus fork is now visible in the timeline picker.

The Mutex<Store> makes the multi-statement sequence non-concurrent but does not make it atomic on crash, on a SQLite IO error mid-sequence, or on a constraint failure. The plan promised real txn boundaries.

Fix: wrap the multi-statement work in self.conn.transaction() (rusqlite gives you a Transaction guard). Make update_step_blobs itself transactional, and pass an existing &Transaction to it from the handler so PATCH = one tx and fork-and-edit = one tx.

---

HIGH 3 — StepUpdated WebSocket handler emits a fake SessionUpdate with hardcoded zeros — visible regression for any subscribed dashboard

StoreEvent::StepUpdated { session_id, .. } => {
    if subscribed_session.as_deref() == Some(session_id) {
        Some(ServerMessage::SessionUpdate {
            data: SessionUpdateData {
                session_id: session_id.clone(),
                status: "recording".to_string(),  // ← hardcoded
                total_steps: 0,                    // ← hardcoded
                total_tokens: 0,                   // ← hardcoded
            },
        })

When a subscribed dashboard receives this, the session-header card will reset to "recording / 0 steps / 0 tokens" — even if the session was completed and had 47 steps a moment ago. The step_id, timeline_id, and deleted_count fields you spent effort plumbing through StoreEvent::StepUpdated are dropped on the wire.

Fix (minimum): add a ServerMessage::StepUpdate { step_id, timeline_id, deleted_count } variant, serialize the real payload, and add a frontend handler that calls queryClient.invalidateQueries. As a stopgap, drop the broadcast (the hook already does react-query invalidation on save, so live subscribers won't miss the update — they'll just refresh on their own next poll).

Either way, the current emission is actively wrong because it overwrites correct session state with placeholders.

---

HIGH 4 — Auto-fork UX never navigates to the new fork (plan T8 promised it)

The hook returns forkTimelineId and autoForked, but StepDetailPanel.handleConfirm only calls cancelEditing() and shows a toast for cascade > 0 (StepDetailPanel.tsx:68-78):

const handleConfirm = useCallback(async () => {
  const result = await stepEdit.save()
  if (result) {
    if (result.deleted_downstream_count > 0 && step) {
      setToastMsg(...)
    }
    cancelEditing()
  }
}, [stepEdit, step, cancelEditing])

After the fork-and-edit succeeds, the panel still shows the original main step; the user has no idea the new fork exists unless they manually open the timeline picker. The plan's T8 said "navigate to the new fork via selectTimeline(forkId)" — and useStore().selectTimeline is already used by ForkModal and ReplaySetupModal for exactly this.

Fix: in handleConfirm, after await stepEdit.save(), if stepEdit.autoForked && stepEdit.forkTimelineId, call useStore.getState().selectTimeline(stepEdit.forkTimelineId). Add a vitest case asserting it.

---

MEDIUM 1 — fork-and-edit-step is missing the 5 MB body-size guard

patch_step enforces MAX_EDIT_BLOB_SIZE = 5MB (api.rs:1585-1596) but fork_and_edit_step does no such check before serde_json::to_vec and blobs.put. A 9 MB request body fits axum's 10 MB limit and silently lands on the new fork.

Same MAX_EDIT_BLOB_SIZE constant should apply.

---

MEDIUM 2 — serde_json::to_vec(...).unwrap_or_default() silently writes empty bytes on serialize failure

let request_bytes = body.request_body.as_ref()
    .map(|v| serde_json::to_vec(v).unwrap_or_default());
let response_bytes = body.response_body.as_ref()
    .map(|v| serde_json::to_vec(v).unwrap_or_default());

serde_json::to_vec on a parsed Value is essentially infallible, so this is mostly defensive — but unwrap_or_default() is the wrong default. If it ever did fail (e.g. someone wires a custom Serialize impl downstream), the user's edit becomes an empty blob with a request_hash of the empty string canonicalization. Same in fork_and_edit_step (line 1729-1731).

Use ? and surface 500, or expect("Value always serializable") to make the panic auditable.

---

MEDIUM 3 — fork_and_edit_step constructs every step as new_llm_call then overrides step_type

let mut new_step = Step::new_llm_call(&fork.id, &session.id, body.at_step, &original_step.model);
new_step.step_type = original_step.step_type;
new_step.status = original_step.status;
...

For an edit on a tool_call step, the constructor sets LLM-call-specific defaults (notably model = original_step.model which can be empty for tool calls), and then step_type is patched after the fact. This works today but is fragile: any new field added to new_llm_call could quietly leak the wrong default into tool-call edits.

Either add a generic Step::new(timeline, session, step_number, step_type) constructor, or branch on original_step.step_type and use the matching new_* constructor.

Tests don't cover the tool-call case (test_fork_and_edit_step_happy_path seeds LLM calls only).

---

MEDIUM 4 — Wrong status code for blob-write failures (507 INSUFFICIENT_STORAGE)

let req_blob = if let Some(ref rb) = request_bytes {
    store.blobs.put(rb).map_err(|e| (StatusCode::INSUFFICIENT_STORAGE, format!("Blob error: {e}")))?
} else { ... };

507 Insufficient Storage is a WebDAV-specific status meaning "the server can't store the resource needed to complete the request." A blob-write failure (IO, permission, hash collision) is not that — it's a generic server error. Use 500 Internal Server Error for consistency with the rest of the file.

---

MEDIUM 5 — Test coverage gaps vs the plan we agreed on

Plan T3 listed 9 scenarios; the implementation covers 8. Missing:

1. Canonical-form roundtrip (review #3 of the plan review). No test proves that a request edited with reordered keys produces the same request_hash as a directly-sorted edit. This is the single test that validates the entire reason normalize_and_hash is called on edits — without it, a future refactor that drops normalization would pass CI.

2. Concurrent-edit last-write-wins (review #9 of the plan review). Plan promised "two concurrent PATCHes on the same step → both succeed in some serialized order; final state is the last writer's."

3. at_step=1 for fork-and-edit (see HIGH 1 above).

4. Tool-call step edit (see MEDIUM 3 above).

Plan T10 listed 6 frontend scenarios; the implementation covers the 6 listed in the PR description but none of them exercise the actual save flow:

• No test that clicking Save → confirm → fork-edit step calls patchStep
• No test for the auto-fork branch (main step → calls forkAndEditStep)
• No test for allow_main_edits=true switching to the destructive copy
• No test for the cascade toast appearing when deleted_downstream_count > 0
• No test for navigation to the new fork after auto-fork (cf. HIGH 4)

The current frontend tests stop at "editor opens" / "Save is disabled correctly". The most error-prone flow (the modal + dispatch + invalidate + navigate sequence) is unverified.

---

LOW 1 — Tests use unsafe { std::env::set_var } mid-test → flake hazard if any other test starts touching REWIND_ALLOW_MAIN_EDITS

unsafe { std::env::remove_var("REWIND_ALLOW_MAIN_EDITS"); }
...
unsafe { std::env::set_var("REWIND_ALLOW_MAIN_EDITS", "true"); }
...
unsafe { std::env::remove_var("REWIND_ALLOW_MAIN_EDITS"); }

Cargo runs tests in parallel by default, and env-var mutations are process-global. Today only this one test touches REWIND_ALLOW_MAIN_EDITS, so it's safe, but the moment another test does, you get cross-test races (the existing runners_tests.rs already does this pattern with KEY_ENV_VAR and that's a known gotcha). Consider:

• A serial_test::serial attribute, or
• Plumbing the flag through AppState instead of reading std::env::var in the handler — that way tests can construct the flag explicitly and parallel-safe (and you remove the runtime env read on every PATCH).

The latter is the better long-term fix.

---

LOW 2 — patch_step does not bump session.updated_at

If an operator edits a step on a still-recording session and does so at minute 30+ since the last record event, the periodic cleanup_stale_sessions task (lib.rs:357) will auto-mark that session as completed. Edges only matter for live recording sessions (most edits target completed ones), so low severity.

Suggest: bump updated_at inside update_step_blobs (or do it in the handler).

---

LOW 3 — update_step_blobs always writes response_blob_format = 0 (FORMAT_NAKED_LEGACY)

The docstring is honest about this:

response_blob_format is always set to FORMAT_NAKED_LEGACY (0) for edited blobs since the caller provides a parsed JSON Value, not an HTTP envelope.

Worth calling out in the user-facing docs (DEPLOYMENT-GAPS.md or wherever the plan said it'd land — I don't see the docs slice in this PR, T11 is missing): an edited response loses the original envelope ({status, headers, body} from the proxy's format=2 recordings). The display surface still works because the format=0 reader returns the blob raw, but if anyone re-exports the session to OTel after editing, the trace will see the naked body where the original step had the full envelope.

This is acceptable for v1 but should be in the docs.

---

LOW 4 — Toast for stepEdit.error has a no-op onDismiss

{stepEdit.error && (
  <Toast msg={stepEdit.error} onDismiss={() => {}} />
)}

Toast's 5s timer will fire but onDismiss = () => {} does nothing, so the toast stays mounted forever (until the next render where stepEdit.error clears, which in the current hook only happens on the next openConfirm call). Use a local useState for the error message so the toast can actually dismiss.

---

LOW 5 — Switching tabs while editing silently discards in-progress edits

startEditing(field, data) resets editorText and originalText whenever invoked. If the user has typed half a JSON edit on the Request tab, switches to Response, and clicks Edit there, the Request edits are gone with no warning. Either preserve per-tab editor state or warn before discarding.

---

NIT — Endpoint path divergence from plan

The plan said PATCH /api/steps/{id}; this PR uses PATCH /api/steps/{id}/edit. Not wrong, but /edit as a suffix is unusual for REST PATCH (the verb already conveys "edit"). Mostly cosmetic — flagging in case you want to align before downstream consumers (SDK, MCP) start hard-coding the path.

---

NIT — patch_step ignores the timeline_id returned by update_step_blobs

let (_timeline_id, step_number, session_id) = store.update_step_blobs(...)?;
let deleted = store.delete_steps_after(&step.timeline_id, step_number)...;
let _ = state.event_tx.send(StoreEvent::StepUpdated {
    ...
    timeline_id: step.timeline_id,
    ...
});

The function returns the timeline_id but the handler reuses step.timeline_id from the earlier get_step call. Either drop the return value from update_step_blobs (it's redundant with the pre-read step) or use it.

---

NIT — Plan T11 (docs) and T13 (release/mirror) are not in this PR

Heads-up that the "Step editing & main-protection" section in DEPLOYMENT-GAPS.md and the release/mirror steps from the plan haven't landed yet. If those are intentionally a follow-up, mention it in the PR description so reviewers know not to look for them.

---

Verdict

The shape is right and the Rust scaffolding is solid. The HIGH items are real correctness/UX issues that would surface immediately in dev1 (HIGH 1 breaks the auto-fork flow for editing step #1 → that's the demo path; HIGH 3 will visibly reset session totals to 0 on every edit; HIGH 4 leaves users stranded on the unedited main timeline). HIGH 2 is the spec compliance the plan committed to. I'd rather not ship 0.14.4 until at least HIGH 1, 3, 4 are fixed; HIGH 2 (transactions) can land in 0.14.5 if it's a bigger refactor than expected.

Happy to pair on any of these — the auto-fork navigation in particular is a 5-line fix.