feat(steps): edit on selected timeline — promote-and-mutate for inherited steps

[risjai]

Summary

• When a fork shows an inherited step and the user clicks Save, the server now promotes the step to an owned step on the fork (or in-place edits it if already owned), cascade-deleting only the fork's downstream
• Fixes the screenshot bug where the "Create fork and edit" modal appeared even when the user was already on a fork viewing an inherited step
• The fork_and_edit_step endpoint remains for the explicit "Create a sub-fork" workflow; the dashboard's default save flow now uses PATCH with target_timeline_id

Changes

Layer	What
Store	upsert_step_on_timeline_and_cascade — single-tx promote with explicit 18-column INSERT
PATCH /api/steps/{id}/edit	New target_timeline_id field routes edit to a different timeline; main-protection follows the effective timeline; returns resolved_step_id
GET /api/steps/{id}/cascade-count	New ?target_timeline_id query param so the confirm modal shows the fork's count, not the parent's
useStepEdit hook	Accepts contextTimelineId; routes onMain, PATCH payload, and cascade-count through it
StepDetailPanel	Falls back to root timeline (not step.timeline_id) when no timeline is selected

Test plan

• 7 new Rust integration tests: promote inherited step, idempotent re-edit, visibility 400 with actionable message, main-protection follows target, step_type+tool_name preservation, cascade-count target-aware, cascade-count matches PATCH
• 394 Rust tests pass (was 386)
• 183 web tests pass
• TypeScript compiles cleanly
• Clippy clean with -D warnings
• Version bumped to 0.14.6 (Track 1)

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 8:58am

[risjai]

Code review (with local repro)

Built the binary at HEAD and exercised it on a fresh local store. Cargo test 65/65, npm test 183/183. Functional happy path works. One HIGH-severity correctness/integrity bug confirmed live, and several gaps from the plan.

---

HIGH 1 — Visibility check matches by step_number, not step.id → cross-fork mutation by request

crates/rewind-web/src/api.rs:1669:

if !visible.iter().any(|s| s.step_number == step.step_number) {
    return Err((StatusCode::BAD_REQUEST, format!("step #{} not visible …", …)));
}

The check passes whenever the target timeline has any step at the same step_number — not necessarily the same step. So if two unrelated forks both own a step at step_number=N, you can clobber one fork's content by PATCHing the other's step with target_timeline_id set to the victim.

Live repro on the PR's binary (just ran on 127.0.0.1:4810):

# Setup: 2 main steps, two parallel forks both at_step=2,
# each fork records its own step #1 with distinct content.
A3=<fork_a's owned step id>     # response_body: {"owned-by": "fork-A"}
B3=<fork_b's owned step id>     # response_body: {"owned-by": "fork-B"}

# Attack: edit A3 but target fork_b
curl -X PATCH /api/steps/$A3/edit -d '{
  "response_body": {"sentinel":"CROSS_FORK_MUTATION"},
  "target_timeline_id": "<fork_b>"
}'
# → HTTP 200, resolved_step_id = $B3

# Aftermath:
GET /api/steps/$B3 → response_body: {"sentinel":"CROSS_FORK_MUTATION"}   ← OVERWRITTEN
GET /api/steps/$A3 → response_body: {"owned-by":"fork-A"}                ← untouched

Existing test_patch_promote_visibility_400 doesn't catch this because it constructs forks at different at_step values (2 and 4) so the step_numbers don't overlap. With overlapping step_numbers (the common case for parallel forks off the same point), the visibility check rubber-stamps the cross-fork write and the upsert "in-place edits the existing owned step at #N" — which is the wrong row.

Fix: change the check to match by id:

if !visible.iter().any(|s| s.id == step.id) {
    return Err((StatusCode::BAD_REQUEST, format!(
        "step '{}' (id={}) is not visible on target timeline '{}' …",
        step.step_number, step.id, effective_timeline,
    )));
}

This catches the cross-fork case because fork_b's union view contains B3.id, not A3.id.

Add a regression test that mirrors my repro: two parallel forks both owning step #N with different content; PATCH fork_a.stepN with target=fork_b → expect 400. Without that test, this same bug class will recur.

---

HIGH 2 — cascade_count skips visibility & session validation; arbitrary cross-session probe

crates/rewind-web/src/api.rs:1712-1738:

let effective_timeline = q.target_timeline_id
    .as_deref()
    .unwrap_or(&step.timeline_id);

let count = store.count_steps_after(effective_timeline, step.step_number)?;
let on_main = store.is_main_timeline(effective_timeline)?;

The PATCH handler validates that effective_timeline is in the same session and the step is visible on it. The dry-run handler skips both checks. Consequences:

• An operator can probe the downstream count of any timeline in any session by passing its UUID — no auth tied to ownership.
• The modal's cascade-count value can be wildly wrong (it returns the foreign timeline's downstream count instead of "what would be deleted on confirm"), which is exactly the HIGH 1 finding from the plan review that the implementation was supposed to fix.

The plan explicitly called out: "Validate: effective_timeline must exist in the same session AND the step must be visible on it. Re-use the same validators as patch_step." That validation is missing — the new query param landed without the gate.

Fix: mirror the PATCH handler's two checks (timeline-in-session + step-visible-on-target), reusing the same engine call. Add a test that GET cascade-count with a foreign-session timeline returns 400.

---

MEDIUM 1 — No frontend tests added

PR's diff for web/:

StepDetailPanel.tsx | 10 ++++++++++
use-step-edit.ts    | 26 +++++++++++++++++++-------
lib/api.ts          | 10 ++++++----

StepDetailPanel.test.tsx is unchanged. The plan's M8 listed three frontend tests that were the whole point of the screenshot-bug fix:

1. Inherited-on-fork shows "Save edit" modal (not "Create fork and edit")
2. PATCH carries target_timeline_id
3. Null-selectedTimelineId falls back to root, NOT step.timeline_id

None of these are added. The screenshot bug fix isn't actually verified at the React level — npm test going 183/183 just means the existing tests still pass. A future regression that re-introduces step.timeline_id as the fallback or drops target_timeline_id from the payload would slip through.

---

MEDIUM 2 — Visibility-check Vec materialization on every PATCH

Same perf concern from the PR #160 review (S1) but now with a wider blast radius — it runs on every promote-PATCH, which is the dashboard's default save path. For sessions with hundreds of steps, the handler loads the entire union into a Vec<Step> just to check existence. Plan said "shares the recursive-walk limitation noted in PR #160 review" — same SQL helper that fixes that should also turn this into a point lookup.

Acceptable for v0.14.6; flag for the recursive-walk follow-up.

---

LOW 1 — Plan said the StoreEvent::StepUpdated payload now carries resolved_step_id (good), but the WS transport still drops it

The handler at api.rs:1687-1692 emits StoreEvent::StepUpdated { step_id: resolved_step_id, … } — good, that's the right wiring. But the WS handler in crates/rewind-web/src/ws.rs (left as None from the v0.14.4 fix) still doesn't forward StepUpdated to subscribers. So multi-tab sync still relies entirely on the in-tab react-query invalidation. Not a regression from this PR, but worth a mention since the resolved_step_id work is wasted on the wire.

---

LOW 2 — useStepEdit has an unused-arg pattern smell

timelineId is now only consumed to compute promoted = contextTimelineId !== timelineId. If contextTimelineId always equals the user's selection, this is fine — but if the panel ever passes the same value for both (defensive bug), promoted becomes false silently and the user re-experiences the "saves on the wrong timeline" issue with no error. Consider adding a debug-only assertion or renaming timelineId to physicalTimelineId to make the intent explicit.

---

LOW 3 — onMain returns false when sessionDetail hasn't loaded yet

web/src/hooks/use-step-edit.ts:44-48:

const onMain = (() => {
  if (!sessionDetail) return false
  …
})()

If a user manages to hit Save before sessionDetail resolves (rare with react-query but possible on first paint), onMain is false → PATCH path runs → with selectedTimelineId === null, contextTimelineId is step.timeline_id (per the fallback) → for an inherited step this is main → server returns 409 (correct) → user sees the error toast. Functional but confusing. Consider gating Save's disabled state on sessionDetail.isSuccess to avoid the race entirely.

---

NIT — Test name test_patch_promote_visibility_400 reads as "tests visibility check works" but actually documents the gap

After HIGH 1 above, that test's pass becomes misleading: it proves only that non-overlapping step_number triggers 400, not that cross-fork visibility is enforced. Either rename to test_patch_promote_disjoint_step_numbers_rejected or strengthen it to also cover overlapping-step_number cross-fork (which is what HIGH 1 wants).

---

NIT — target_timeline_id query param naming inconsistent with body field

PATCH body: target_timeline_id. GET cascade-count query: also target_timeline_id. Frontend lib param: targetTimelineId. Consistent names, but worth documenting the convention in use-cases/ray-agent/DEPLOYMENT-GAPS.md "Step Editing" section since the API has now grown a meaningful new contract.

---

What works (verified live)

• v0.14.6 binary builds clean; /api/health reports {"status":"ok","version":"0.14.6","allow_main_edits":false}.
• cargo test -p rewind-web --test recording_api_tests → 65/65 pass.
• npm test → 183/183 pass.
• Server endpoints: PATCH happy path, cascade-count target-aware (count differs based on ?target_timeline_id=), idempotent re-edit returns same resolved_step_id, main-protection follows target.
• Frontend wiring: StepDetailPanel reads selectedTimelineId from the store and passes a sensible fallback chain (selectedTimelineId ?? rootTimelineId ?? step.timeline_id).

---

Verdict

Block on HIGH 1. The cross-fork mutation is a correctness + integrity bug — operators can silently corrupt unrelated forks' data via a request the server validates as legal. Repro is one curl command on the PR's binary. Fix is a single character change in the visibility predicate (step_number → id) plus a regression test.

HIGH 2 is a 5-line gate addition (validate target is in session + visible) that the plan explicitly required.

MEDIUM 1 (frontend tests) is the difference between "the bug is fixed" and "the fix is verified to stay fixed." Without the React tests, future refactors will silently re-introduce the screenshot bug.

The rest are follow-up-grade. After HIGH 1+2 and MEDIUM 1 land, this is mergeable.

[risjai]

Code review (with local repro)

Built the binary at HEAD and exercised it on a fresh local store. Cargo test 65/65, npm test 183/183. Functional happy path works. One HIGH-severity correctness/integrity bug confirmed live, and several gaps from the plan.

---

HIGH 1 — Visibility check matches by step_number, not step.id → cross-fork mutation by request

crates/rewind-web/src/api.rs:1669:

if !visible.iter().any(|s| s.step_number == step.step_number) {
    return Err((StatusCode::BAD_REQUEST, format!("step #{} not visible …", …)));
}

The check passes whenever the target timeline has any step at the same step_number — not necessarily the same step. So if two unrelated forks both own a step at step_number=N, you can clobber one fork's content by PATCHing the other's step with target_timeline_id set to the victim.

Live repro on the PR's binary (just ran on 127.0.0.1:4810):

# Setup: 2 main steps, two parallel forks both at_step=2,
# each fork records its own step #1 with distinct content.
A3=<fork_a's owned step id>     # response_body: {"owned-by": "fork-A"}
B3=<fork_b's owned step id>     # response_body: {"owned-by": "fork-B"}

# Attack: edit A3 but target fork_b
curl -X PATCH /api/steps/$A3/edit -d '{
  "response_body": {"sentinel":"CROSS_FORK_MUTATION"},
  "target_timeline_id": "<fork_b>"
}'
# → HTTP 200, resolved_step_id = $B3

# Aftermath:
GET /api/steps/$B3 → response_body: {"sentinel":"CROSS_FORK_MUTATION"}   ← OVERWRITTEN
GET /api/steps/$A3 → response_body: {"owned-by":"fork-A"}                ← untouched

Existing test_patch_promote_visibility_400 doesn't catch this because it constructs forks at different at_step values (2 and 4) so the step_numbers don't overlap. With overlapping step_numbers (the common case for parallel forks off the same point), the visibility check rubber-stamps the cross-fork write and the upsert "in-place edits the existing owned step at #N" — which is the wrong row.

Fix: change the check to match by id:

if !visible.iter().any(|s| s.id == step.id) {
    return Err((StatusCode::BAD_REQUEST, format!(
        "step '{}' (id={}) is not visible on target timeline '{}' …",
        step.step_number, step.id, effective_timeline,
    )));
}

This catches the cross-fork case because fork_b's union view contains B3.id, not A3.id.

Add a regression test that mirrors my repro: two parallel forks both owning step #N with different content; PATCH fork_a.stepN with target=fork_b → expect 400. Without that test, this same bug class will recur.

---

HIGH 2 — cascade_count skips visibility & session validation; arbitrary cross-session probe

crates/rewind-web/src/api.rs:1712-1738:

let effective_timeline = q.target_timeline_id
    .as_deref()
    .unwrap_or(&step.timeline_id);

let count = store.count_steps_after(effective_timeline, step.step_number)?;
let on_main = store.is_main_timeline(effective_timeline)?;

The PATCH handler validates that effective_timeline is in the same session and the step is visible on it. The dry-run handler skips both checks. Consequences:

• An operator can probe the downstream count of any timeline in any session by passing its UUID — no auth tied to ownership.
• The modal's cascade-count value can be wildly wrong (it returns the foreign timeline's downstream count instead of "what would be deleted on confirm"), which is exactly the HIGH 1 finding from the plan review that the implementation was supposed to fix.

The plan explicitly called out: "Validate: effective_timeline must exist in the same session AND the step must be visible on it. Re-use the same validators as patch_step." That validation is missing — the new query param landed without the gate.

Fix: mirror the PATCH handler's two checks (timeline-in-session + step-visible-on-target), reusing the same engine call. Add a test that GET cascade-count with a foreign-session timeline returns 400.

---

MEDIUM 1 — No frontend tests added

PR's diff for web/:

StepDetailPanel.tsx | 10 ++++++++++
use-step-edit.ts    | 26 +++++++++++++++++++-------
lib/api.ts          | 10 ++++++----

StepDetailPanel.test.tsx is unchanged. The plan's M8 listed three frontend tests that were the whole point of the screenshot-bug fix:

1. Inherited-on-fork shows "Save edit" modal (not "Create fork and edit")
2. PATCH carries target_timeline_id
3. Null-selectedTimelineId falls back to root, NOT step.timeline_id

None of these are added. The screenshot bug fix isn't actually verified at the React level — npm test going 183/183 just means the existing tests still pass. A future regression that re-introduces step.timeline_id as the fallback or drops target_timeline_id from the payload would slip through.

---

MEDIUM 2 — Visibility-check Vec materialization on every PATCH

Same perf concern from the PR #160 review (S1) but now with a wider blast radius — it runs on every promote-PATCH, which is the dashboard's default save path. For sessions with hundreds of steps, the handler loads the entire union into a Vec<Step> just to check existence. Plan said "shares the recursive-walk limitation noted in PR #160 review" — same SQL helper that fixes that should also turn this into a point lookup.

Acceptable for v0.14.6; flag for the recursive-walk follow-up.

---

LOW 1 — Plan said the StoreEvent::StepUpdated payload now carries resolved_step_id (good), but the WS transport still drops it

The handler at api.rs:1687-1692 emits StoreEvent::StepUpdated { step_id: resolved_step_id, … } — good, that's the right wiring. But the WS handler in crates/rewind-web/src/ws.rs (left as None from the v0.14.4 fix) still doesn't forward StepUpdated to subscribers. So multi-tab sync still relies entirely on the in-tab react-query invalidation. Not a regression from this PR, but worth a mention since the resolved_step_id work is wasted on the wire.

---

LOW 2 — useStepEdit has an unused-arg pattern smell

timelineId is now only consumed to compute promoted = contextTimelineId !== timelineId. If contextTimelineId always equals the user's selection, this is fine — but if the panel ever passes the same value for both (defensive bug), promoted becomes false silently and the user re-experiences the "saves on the wrong timeline" issue with no error. Consider adding a debug-only assertion or renaming timelineId to physicalTimelineId to make the intent explicit.

---

LOW 3 — onMain returns false when sessionDetail hasn't loaded yet

web/src/hooks/use-step-edit.ts:44-48:

const onMain = (() => {
  if (!sessionDetail) return false
  …
})()

If a user manages to hit Save before sessionDetail resolves (rare with react-query but possible on first paint), onMain is false → PATCH path runs → with selectedTimelineId === null, contextTimelineId is step.timeline_id (per the fallback) → for an inherited step this is main → server returns 409 (correct) → user sees the error toast. Functional but confusing. Consider gating Save's disabled state on sessionDetail.isSuccess to avoid the race entirely.

---

NIT — Test name test_patch_promote_visibility_400 reads as "tests visibility check works" but actually documents the gap

After HIGH 1 above, that test's pass becomes misleading: it proves only that non-overlapping step_number triggers 400, not that cross-fork visibility is enforced. Either rename to test_patch_promote_disjoint_step_numbers_rejected or strengthen it to also cover overlapping-step_number cross-fork (which is what HIGH 1 wants).

---

NIT — target_timeline_id query param naming inconsistent with body field

PATCH body: target_timeline_id. GET cascade-count query: also target_timeline_id. Frontend lib param: targetTimelineId. Consistent names, but worth documenting the convention in use-cases/ray-agent/DEPLOYMENT-GAPS.md "Step Editing" section since the API has now grown a meaningful new contract.

---

What works (verified live)

• v0.14.6 binary builds clean; /api/health reports {"status":"ok","version":"0.14.6","allow_main_edits":false}.
• cargo test -p rewind-web --test recording_api_tests → 65/65 pass.
• npm test → 183/183 pass.
• Server endpoints: PATCH happy path, cascade-count target-aware (count differs based on ?target_timeline_id=), idempotent re-edit returns same resolved_step_id, main-protection follows target.
• Frontend wiring: StepDetailPanel reads selectedTimelineId from the store and passes a sensible fallback chain (selectedTimelineId ?? rootTimelineId ?? step.timeline_id).

---

Verdict

Block on HIGH 1. The cross-fork mutation is a correctness + integrity bug — operators can silently corrupt unrelated forks' data via a request the server validates as legal. Repro is one curl command on the PR's binary. Fix is a single character change in the visibility predicate (step_number → id) plus a regression test.

HIGH 2 is a 5-line gate addition (validate target is in session + visible) that the plan explicitly required.

MEDIUM 1 (frontend tests) is the difference between "the bug is fixed" and "the fix is verified to stay fixed." Without the React tests, future refactors will silently re-introduce the screenshot bug.

The rest are follow-up-grade. After HIGH 1+2 and MEDIUM 1 land, this is mergeable.