fix(steps): fork-and-edit-step accepts inherited step numbers

[risjai]

Summary

POST /api/sessions/{sid}/fork-and-edit-step rejected step numbers that lived on the parent timeline (i.e. inherited via a previous fork), even though the dashboard timeline picker happily showed those steps. Repro from dev1 v0.14.4 smoke testing on 2026-04-29:

1. fork-and-edit at_step=5 from main → creates fork F (owns step 5)
2. fork-and-edit at_step=3 from fork F → 400: no step #3 on timeline 'F' (step Add MCP server for AI assistant integration #3 is inherited from main, not owned by F)

The handler used store.get_step_by_number(timeline_id, n) which only matches steps OWNED by that timeline. Routed the lookup through engine.get_full_timeline_steps — the same union view engine.fork already uses for at_step validation — so inherited steps resolve correctly.

Changes

• crates/rewind-web/src/api.rs: fork_and_edit_step now resolves the source step via the engine's union view; the engine instance is created earlier in the function and reused for the subsequent engine.fork() call.
• crates/rewind-web/tests/recording_api_tests.rs: adds test_fork_and_edit_step_inherited_step covering the repro above.
• Track 1 version bump 0.14.4 → 0.14.5 across all 5 files per CLAUDE.md.

Adjacent issue spotted (separate fix)

While writing the regression test I noticed engine.get_full_timeline_steps only walks ONE level up the parent chain — store.get_steps(parent_id) returns the parent's OWNED steps, not its own union. So a fork-of-a-fork loses every step inherited via the grandparent. This breaks /api/sessions/{sid}/steps?timeline=X for nested forks (visible in the dev1 smoke run as a 1-step view where 2 were expected).

This PR's regression test sidesteps it by asserting only the directly-created step's invariants (step_number, timeline_id) instead of the full fork view. Worth opening a follow-up to make get_full_timeline_steps recursive.

Test plan

• cargo test -p rewind-web --test recording_api_tests — 58/58 pass (was 57/57 before; +1 new regression test)
• cargo build -p rewind-web — clean
• Bug repro verified live in dev1 against v0.14.4 (separate from the unit-test repro): hitting POST /sessions/.../fork-and-edit-step with at_step pointing at an inherited step returned HTTP 400 no step #N on timeline 'X'
• Live verification of the fix in dev1 will happen post-merge once CI builds the v0.14.5 release binary
• Version bumped to 0.14.5 (Track 1: Cargo.toml + lock + 2x CLI_VERSION + MCP pyproject)

[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 7:38am

[risjai]

Review: fix(steps): fork-and-edit-step accepts inherited step numbers

Clean, well-motivated bugfix caught from a real dev1 smoke test. The root cause is correctly identified and the fix is sound. The adjacent nested-fork limitation is honestly documented. One finding, two suggestions.

Findings summary

#	Severity	Finding
1	Suggestion	get_full_timeline_steps loads the full step list into memory for a single-step lookup
2	Suggestion	Test covers the fork-boundary case but not a step below the fork boundary
3	Info	ReplayEngine is constructed even in the at_step == 1 branch where it's unused

Details

S1 — Full-list load for single-step lookup (low priority)

The old code did store.get_step_by_number(timeline_id, n) — a single SQL WHERE with an index hit. The new code does engine.get_full_timeline_steps() which loads the entire parent + owned step set into a Vec, then into_iter().find() scans it linearly.

For current session sizes (< 200 steps) this is negligible. If sessions grow to 1K+ steps (e.g., long-running agent loops), a targeted SQL query that unions parent and own steps with a step_number = ? filter would be more efficient. Fine for v0.14.5; flag for the follow-up where get_full_timeline_steps gets made recursive anyway.

S2 — Test could also cover a step below the fork boundary

The test forks at step 2 and edits step 2 (the last inherited step). The original repro from dev1 was editing step 3 from a fork created at step 5 — i.e., a step well below the fork boundary. Consider adding a variant:

// Fork at step 4, then fork-and-edit at step 2 (two steps below fork boundary)
let (_, fork) = post_json(&app, &format!("/api/sessions/{sid}/fork"), json!({
    "at_step": 4, "label": "deep-inherit-fork"
})).await;
let fork_tid = fork["fork_timeline_id"].as_str().unwrap();
let (status, _) = post_json(&app, &format!("/api/sessions/{sid}/fork-and-edit-step"), json!({
    "source_timeline_id": fork_tid,
    "at_step": 2,
    "response_body": {"edit": "deep-inherited"},
})).await;
assert_eq!(status, StatusCode::CREATED);

This exercises the "step is clearly visible in the picker but lives 2 levels above the fork point" path. Not blocking — the existing test covers the core regression.

I3 — Engine created but unused in at_step == 1 branch

After the refactor, let engine = ReplayEngine::new(&store) is created before the if body.at_step == 1 branch, where it's used for the step lookup. But inside the at_step == 1 body, the timeline is created directly via Timeline::new_fork — the engine is allocated but never calls engine.fork(). Trivial; ReplayEngine::new is just a borrow, so there's no runtime cost.

Verdict

Ship it. The fix correctly routes through the union view for step resolution. The nested-fork limitation (get_full_timeline_steps only walks one parent level) is a pre-existing issue and appropriately deferred. Version bump is correct.

[risjai]

Self-review (treating as a fresh reviewer)

The diff is small and the bug it fixes is real. Posting the same scrutiny applied to #155/#159, ordered by severity. Findings 1 and 2 are worth addressing before merge; the rest can be follow-ups.

---

HIGH 1 — User-visible workflow remains broken on nested forks; PR fixes the API but the resulting fork is unusable

This is the real cost of the "adjacent issue" mentioned in the PR body. Concretely, the new flow after this PR for the dev1 repro:

1. Dashboard shows fork F1 (parent=main, fork_at_step=2) with 5 visible steps (1-2 inherited, 3-5 inherited from main extending past the fork point — actually no, those are not visible either, but dashboard shows them through the union endpoint)
2. User clicks Edit on inherited step #3 → Save
3. Endpoint creates new fork F2 (parent=F1, fork_at_step=2), inserts edited step #3 into F2 — PR fix lets this succeed
4. Frontend auto-navigates to F2 and queries /api/sessions/{sid}/steps?timeline=F2
5. Returns [step #3] only — steps #1, #2 from grandparent main are gone from the view

So the user clicks Save expecting "create a fork like F1 but with my edit", and gets a fork that visually appears to start at step #3 with no upstream context. The replay engine reads the same incomplete view (steps come from engine.get_full_timeline_steps everywhere, not just /api/sessions/.../steps) — meaning a Run replay against F2 will start from step #3 with no inherited cache, almost certainly diverging.

The PR comment dismisses this as "a separate fix" but I'd argue it's the same bug: the symptom is "fork-and-edit on inherited steps doesn't work for the user." The 400 → 201 change moves the failure from a clear error to a silent data-loss-shaped UX. Either:

• (a) Block this PR until get_full_timeline_steps is recursive, OR
• (b) Land this fix but explicitly disable the Edit pencil on inherited steps in the dashboard until the recursive lookup ships, OR
• (c) Document the limitation in the response toast ("Edit applied, but nested-fork view is incomplete — see #issue") so the operator understands.

The PR currently does none of these. Recommend (a).

---

MEDIUM 2 — Test asserts step_number + timeline_id but never reads the edited body back

let s = _store.lock().unwrap();
let new_step = s.get_step(new_step_id).unwrap().unwrap();
assert_eq!(new_step.step_number, 2);
assert_eq!(new_step.timeline_id, new_fork_tid);

The test sends response_body: {"edit": "of-an-inherited-step"} but never verifies the value persisted. A future refactor that drops the response_body plumbing through this code path (or accidentally reuses original_step.response_blob instead of the edited bytes) would pass this test.

Add an assertion that reads back the response_body and matches the input:

let blob = s.blobs.get(&new_step.response_blob).unwrap();
let body: serde_json::Value = serde_json::from_slice(&blob).unwrap();
assert_eq!(body, json!({"edit": "of-an-inherited-step"}));

---

MEDIUM 3 — engine.get_full_timeline_steps materializes the full Vec just to find one step by number

For sessions with hundreds/thousands of steps, this is wasteful per-edit. The original store.get_step_by_number(timeline_id, n) was an indexed point lookup; the replacement is O(n) with a Vec allocation.

For the immediate fix it's fine — fork-and-edit isn't on a hot path. But the right long-term shape is a store helper:

fn get_step_by_number_with_inheritance(
    &self,
    timeline_id: &str,
    session_id: &str,
    step_number: u32,
) -> Result<Option<Step>>

…that walks the parent chain via SQL (recursive CTE) without loading every sibling step into memory. This also dovetails with the fix needed for HIGH 1 — the recursive walk is shared logic.

Worth filing as the same follow-up.

---

LOW 1 — Pre-existing redundancy not removed

let timelines = store.get_timelines(&session.id).map_err(|e| {
    (StatusCode::INTERNAL_SERVER_ERROR, format!("DB error: {e}"))
})?;
let source_exists = timelines.iter().any(|t| t.id == body.source_timeline_id);
if !source_exists {
    return Err((StatusCode::BAD_REQUEST, format!(
        "source_timeline_id '{}' not found in session '{}'",
        body.source_timeline_id, session.id
    )));
}

engine.get_full_timeline_steps (now called immediately after) does its own store.get_timelines(session_id) and would error out with Timeline not found. The explicit pre-check is now an extra DB round-trip purely for a friendlier error message. Acceptable, but worth a comment explaining why the duplication is intentional, or dropping it and letting the engine error propagate (mapping it to 400 with the friendlier message).

---

LOW 2 — No test for at_step > source.steps.len()

Existing tests cover at_step=0, bad source_timeline_id, happy path, and (now) inherited step. There's no test for an explicitly-out-of-range at_step (e.g. at_step=99 on a 3-step source). The fix's find() returning None will surface as the same "no step #N on timeline 'X'" 400, but the missing test means a future change that loosens the check (e.g. auto-padding) wouldn't be caught.

Add:

#[tokio::test]
async fn test_fork_and_edit_step_at_step_beyond_max() {
    let (app, _store, _tmp) = setup();
    let (sid, root_tid) = seed_session(&app, 3).await;

    let (status, body) = post_json(&app, &format!("/api/sessions/{sid}/fork-and-edit-step"), json!({
        "source_timeline_id": root_tid,
        "at_step": 99,
        "response_body": {"e": 1}
    })).await;

    assert_eq!(status, StatusCode::BAD_REQUEST);
    let raw = body.get("raw").and_then(|v| v.as_str()).unwrap_or("");
    assert!(raw.contains("no step #99"), "got: {raw}");
}

---

LOW 3 — PR body links to "follow-up" but no actual issue exists

The "Adjacent issue spotted" section names the bug clearly but doesn't link to a tracking issue. If you want HIGH 1 to be resolved separately, file the issue first and link it from this PR (and from the test's TODO comment).

---

LOW 4 — Test's inline comment about the deeper bug should be a TODO(#issue) reference

// (Note: the new fork's full-step view is currently incomplete for
// nested forks because `get_full_timeline_steps` only walks one
// parent level up — that's a separate bug worth its own fix. Here
// we assert the direct invariants of the edit itself.)

Long comments rot. Convert to // TODO(#XYZ): once nested-fork inheritance is fixed, also assert the new fork has [step1_inherited, step2_edited] once the issue exists. That way the test naturally catches the regression as soon as the deeper fix ships.

---

NIT — fork_and_edit_step could use a docstring documenting the "inherited steps are valid" semantics

After this PR, fork_and_edit_step accepts step numbers that don't physically live on the source timeline (they're inherited from the parent chain). That's a non-obvious API guarantee. A 3-line doc comment above the function would save the next reader from re-deriving it from the engine call.

---

NIT — Test name + comment reference "dev1 v0.14.4 smoke testing" — fine for now, will be stale soon

The test docstring references a specific date and the dev1 environment. That's a useful provenance trail for now but will become noise within a release. After v0.14.5 ships, consider trimming to just the conceptual repro recipe.

---

Verdict

Block on HIGH 1. The fix is necessary but creates a worse-feeling failure mode (looks-like-data-loss in the dashboard) than the one it removes (clear 400 error). Either fix the recursive-walk bug in this PR, or gate the dashboard pencil on inherited steps until that fix ships, or warn the user in the cascade-toast.

MEDIUM 2 is a 5-line improvement that meaningfully strengthens the regression test.

MEDIUM 3 + LOW 1 together suggest the right shape is a single store helper that does the recursive walk and returns one step — that helper would resolve HIGH 1, MEDIUM 3, and LOW 1 in one go and replace both get_full_timeline_steps (one parent level) and get_step_by_number (no inheritance) with the unified primitive. Probably worth scoping in this PR rather than splitting.

[risjai]

Thanks for the review. Folded in the actionable bits in 8599676:

• S2 (deeper-inherit test variant) → added test_fork_and_edit_step_inherited_step_below_fork_boundary: fork at step 4, edit step 2 — exercises the original dev1 repro shape (step well below the fork boundary).
• MEDIUM 2 from my self-review (response-body roundtrip) → both inherited-step tests now read the edited bytes back through the blob store and assert against the input. Future refactors that reuse the original step's response_blob will fail loudly.

Punted (intentionally, with notes):

• S1 (perf — Vec materialization for a single-step lookup) → as you noted, fine for v0.14.5; will fold into the recursive-walk follow-up where we collapse get_full_timeline_steps + get_step_by_number into a single get_step_by_number_with_inheritance SQL helper.
• I3 (engine allocated in the at_step==1 branch) → trivial, no-op cost.
• HIGH 1 from my self-review (nested-fork inheritance via get_full_timeline_steps) and the frontend step.timeline_id vs selectedTimelineId UX bug both belong to the same Model B (promote-and-mutate) follow-up. Plan is staged at .cursor/plans/edit-on-selected-timeline_9740df3e.plan.md — to land separately as v0.14.6 once this merges.

5/5 tests pass locally. Awaiting CI green to merge.