Skip to content

Fork at message: re-roll a session from an earlier point in its history#4410

Merged
kevin-dp merged 14 commits into
mainfrom
kevin/fork-at-message
Jun 2, 2026
Merged

Fork at message: re-roll a session from an earlier point in its history#4410
kevin-dp merged 14 commits into
mainfrom
kevin/fork-at-message

Conversation

@kevin-dp
Copy link
Copy Markdown
Contributor

@kevin-dp kevin-dp commented May 26, 2026

Summary

Adds first-class support for forking a session at any earlier message, rather than only at HEAD. From the UI: hover any user-message bubble (after the first response has landed) and click the GitFork icon — a new entity is created whose main stream is the source's history truncated up to and including the previous completed agent response. The new entity boots idle, ready for a different prompt.

Implements the design in docs/fork-at-message.md (local-only RFC).

Phasing & commits

  • 089bcb7 — initial pin to a durable-streams PR build (originally new subscriptions api durable-streams/durable-streams#350; superseded — see "Upstream dependency" below).
  • 3a496b9 — add the EventPointer type + lexicographic order-token helpers in agents-runtime.
  • 347ea53 — widen __electricRowOffsets from Map<key, string> to Map<key, EventPointer>; thread through the 5 existing consumers (timeline, setup-context, context-factory).
  • b9c9ab8 — bump durable-streams pin to feat(server): Stream-Fork-Sub-Offset for arbitrary-position forks durable-streams/durable-streams#347 (rebased; that rebase plus a server-side fork-frame fix landed in feat(server): Stream-Fork-Sub-Offset for arbitrary-position forks durable-streams/durable-streams#347).
  • 665e3d0e — agents-server side: extend streamClient.fork with optional forkPointer headers, extend forkBodySchema with fork_pointer: { offset, sub_offset } (snake_case wire), thread through entityManager.forkSubtree, root-only pointer threading (Q2), manifest filter on the root (Q3 strict-on-root / loose-on-descendants), readJsonWithPointers helper.
  • f0bd4494 — agents-server-ui: hover-revealed GitFork button on UserMessage rows; ChatView computes the per-row fork-anchor map by walking the timeline backwards to the latest completed runs row and reading its pointer from __electricRowOffsets. Eligibility (no preceding completed run, in-flight run) suppresses the affordance.
  • 9dc46f25 — bugfix: the Phase-3 manifest filter compared pointers across coordinate systems (live delivery batches vs historical single-batch reads). Switched to canonical cumulative position from each event's stable headers.offset. Also tightened null-anchor handling.
  • 2f61c946 — bugfix: feat(server): Stream-Fork-Sub-Offset for arbitrary-position forks durable-streams/durable-streams#347's Stream-Fork-Sub-Offset addresses items within a single log entry, not items globally past the anchor. The runtime was minting sub-offsets across log-entry boundaries when delivery batches spanned multiple entries (typical during catch-up). Switched entity-stream-db's onBeforeBatch to group by item.headers.offset so pointers are server-compatible regardless of chunking.
  • c052ed96 — UX fix: skip the all-subtree-idle wait on the source root for pointer-forks. The runtime keeps a worker warm for 5 minutes after the last handler returned (status: "running" during the keep-alive window), and the fork POST default wait is 120 s, so clicking the button right after a response landed hung for two minutes and then 409'd. Since the pointer path reads the source's main historically up to a frozen offset, concurrent writes past the pointer can't tear our snapshot; we only need to wait+lock kept descendants (Q3 HEAD-clones) once they're identified. HEAD-clone forks (no pointer) keep the old full-idle requirement.
  • 5997c95 — flip @durable-streams/{client,server,state} from pkg.pr.new/...@347 to the published versions (^0.2.6 / ^0.3.5 / ^0.2.9) now that feat(server): Stream-Fork-Sub-Offset for arbitrary-position forks durable-streams/durable-streams#347 has merged and a release has been cut.

Upstream dependency

The upstream PR durable-streams/durable-streams#347 (Stream-Fork-Sub-Offset) provides the truncated-fork wire protocol this PR depends on.

It was originally rebased onto durable-streams/durable-streams#350, but that PR was deprecated by its author and split into four smaller PRs: durable-streams/durable-streams#359, durable-streams/durable-streams#360, durable-streams/durable-streams#361, and durable-streams/durable-streams#362, all of which merged into main. The state-package subscription hooks piece (queryOnce, createTransaction, getStreamDBCollectionId, coalesce, concat, createLiveQueryCollection, localOnlyCollectionOptions, toArray) merged via durable-streams/durable-streams#365.

durable-streams/durable-streams#347 has now merged (2026-06-01) and the wire protocol is available on the published packages:

  • @durable-streams/client@0.2.6
  • @durable-streams/server@0.3.5
  • @durable-streams/state@0.2.9

This PR depends on those versions directly — no more pkg.pr.new pins.

What's wired

  • POST /_electric/entities/<type>/<id>/fork accepts optional fork_pointer: { offset, sub_offset }. Omitted → existing HEAD-clone behavior.
  • Server-side validation surfaces 400 with descriptive errors on invalid offset or out-of-range sub-offset.
  • Root entity's main is truncated at the pointer; error and shared-state streams clone at HEAD (Q2).
  • Root's manifest filtered at the pointer; children whose manifest entry landed after X are dropped from the fork along with their subtrees (Q3 strict-on-root). Kept children come along at HEAD (Q3 loose-on-descendants).
  • For pointer-forks the source root doesn't need to be idle (only the kept descendants do); HEAD-clones still wait for the full subtree to be idle.
  • createForkEntity(url, { pointer }) client helper.
  • UserMessage per-row onForkFromHere prop + CSS hover/focus-within reveal. First message in a chat and in-flight runs suppress the button.

Known gaps / follow-ups

  1. createForkEntity swallows errors silently (.catch(() => {})). Now that pointer-forks no longer wait for the root to be idle, the 120 s hang case is gone — but other errors (manifest filter throws, 400 on bad pointer, etc.) still vanish without UI feedback. Surfacing this via a toast/inline state is a small UX follow-up.
  2. Mobile (Phase 5) — the affordance propagates automatically through the Expo DOM embed since ChatView is reused, but touch devices don't fire :hover so the button visibility rule needs an adjustment (always-visible at reduced weight, or long-press) once the embed is tested.
  3. Stratovolt — Stratovolt needs to bump to @durable-streams/server@^0.3.5 before fork-at-message works against Stratovolt-hosted servers; until then it only works against local agent servers.

Test plan

  • Pull, build, run ./scripts/dev.sh start --with-agents (needs ANTHROPIC_API_KEY + ELECTRIC_AGENTS_PULL_WAKE_RUNNER_ID=builtin-agents + ELECTRIC_AGENTS_REGISTER_PULL_WAKE_RUNNER=true in .env).
  • Spawn a Horton entity, send two messages, wait for both to complete.
  • Hover the second user-message bubble → GitFork button fades in.
  • Click → navigates to a new entity URL ending in -fork-<hash> with only the first exchange in history. Works immediately even while the source is still in the post-run 5-minute keep-alive window.
  • Send a new prompt in the fork → fresh response, source entity unchanged.
  • Negative: first user message gets no button. While Horton is streaming, the triggering message gets no button. Button returns after ✓ done.
  • Server-side via curl: POST /fork with bad fork_pointer.offset → 400; with overshooting sub_offset → 400; without fork_pointer → 201 full clone.

🤖 Generated with Claude Code

@netlify
Copy link
Copy Markdown

netlify Bot commented May 26, 2026

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit d465213
🔍 Latest deploy log https://app.netlify.com/projects/electric-next/deploys/6a195607ac7410000875a324
😎 Deploy Preview https://deploy-preview-4410--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@kevin-dp kevin-dp marked this pull request as ready for review May 26, 2026 11:50
Comment thread packages/agents-runtime/src/entity-stream-db.ts Outdated
Comment thread packages/agents-runtime/src/entity-stream-db.ts Outdated
Comment thread packages/agents-runtime/src/entity-stream-db.ts Outdated
Comment thread packages/agents-runtime/src/entity-stream-db.ts Outdated
Comment thread packages/agents-runtime/src/event-pointer.ts Outdated
Comment thread packages/agents-server/src/entity-manager.ts Outdated
Comment thread packages/agents-server/src/entity-manager.ts Outdated
Comment thread packages/agents-server/src/entity-manager.ts Outdated
Comment thread packages/agents-server/src/stream-client.ts Outdated
Comment thread packages/agents-server/src/stream-client.ts Outdated
@kevin-dp kevin-dp force-pushed the kevin/fork-at-message branch from f8313f9 to 0be3760 Compare May 27, 2026 06:45
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 27, 2026

Electric Agents Desktop Builds

Build artifacts for commit e2f1365.

Platform Status Artifact
macOS Apple Silicon Passed DMG
macOS Intel Passed DMG
Windows x64 Passed Installer
Linux x64 Passed AppImage / deb

Workflow run

@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

❌ Patch coverage is 41.80328% with 142 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.38%. Comparing base (1c6ebf9) to head (5997c95).
⚠️ Report is 20 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
packages/agents-server/src/entity-manager.ts 12.60% 103 Missing and 1 partial ⚠️
...agents-server-ui/src/components/views/ChatView.tsx 0.00% 18 Missing ⚠️
packages/agents-runtime/src/entity-stream-db.ts 76.66% 7 Missing ⚠️
...gents-server-ui/src/lib/ElectricAgentsProvider.tsx 0.00% 5 Missing ⚠️
packages/agents-runtime/src/context-factory.ts 0.00% 3 Missing ⚠️
packages/agents-server/src/stream-client.ts 89.65% 3 Missing ⚠️
...es/agents-server-ui/src/components/UserMessage.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4410      +/-   ##
==========================================
- Coverage   59.63%   59.38%   -0.25%     
==========================================
  Files         311      313       +2     
  Lines       32454    32733     +279     
  Branches     8920     9028     +108     
==========================================
+ Hits        19353    19439      +86     
- Misses      13083    13275     +192     
- Partials       18       19       +1     
Flag Coverage Δ
packages/agents 70.37% <ø> (ø)
packages/agents-mcp 77.54% <ø> (ø)
packages/agents-mobile 85.41% <ø> (ø)
packages/agents-runtime 81.99% <85.71%> (+0.14%) ⬆️
packages/agents-server 73.96% <28.18%> (-1.19%) ⬇️
packages/agents-server-ui 5.64% <0.00%> (-0.07%) ⬇️
packages/electric-ax 46.33% <ø> (ø)
packages/experimental 87.73% <ø> (ø)
packages/react-hooks 86.48% <ø> (ø)
packages/start 82.83% <ø> (ø)
packages/typescript-client 94.39% <ø> (ø)
packages/y-electric 56.05% <ø> (ø)
typescript 59.38% <41.80%> (-0.25%) ⬇️
unit-tests 59.38% <41.80%> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

Electric Agents Mobile Build

Android preview build for commit e2f1365.

Platform Profile Status Build
Android preview Passed EAS build

Workflow run

kevin-dp and others added 12 commits June 2, 2026 11:03
First step of fork-at-message. Introduces the { offset, subOffset }
pair that addresses one event on a Durable Stream - maps 1:1 to
PR #347's Stream-Fork-Offset + Stream-Fork-Sub-Offset header pair
when forking.

- EventPointer.offset = null represents "anchor at stream start"
  (translates to omitting Stream-Fork-Offset on the wire).
- formatPointerOrderToken produces lex-sortable
  stream:<padded-offset>:<padded-subOffset> tokens, kept format-
  compatible with the existing single-offset _timeline_order
  prefix so existing LIKE 'stream:%' query matchers keep working.
- comparePointers for explicit ordering.
- STREAM_START_POINTER sentinel for the no-batches-yet state.

The module's doc comment records why sub-offsets are computed
locally (PR #347 ships fork-side only; the read-side spec is
explicitly reserved for a future revision) and the limitation that
follows (local counter is correct for fresh reads from offset 0,
which is the only thing we use today).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Second step of fork-at-message. The per-row side-table on each
EntityStreamDB collection (__electricRowOffsets) now stores the
{ offset, subOffset } pair instead of a single offset string. The
pointer is computed in onBeforeBatch by pairing each item's
position-in-batch with the END offset of the *previous* batch — the
shape PR #347's Stream-Fork-Sub-Offset header expects when forking.

- entity-stream-db: track previousBatchOffset across batches;
  per-item pointer = { offset: previousBatchOffset, subOffset: i+1 }.
  Replaced legacy formatStreamTimelineOrder with the shared
  formatPointerOrderToken helper. applyEvent synthesizes a single-
  item pointer for in-process events (subOffset=1, offset from
  header or a monotonic local: token).
- entity-timeline: order-token derivation uses
  formatPointerOrderToken. orderTokenToHistoryOffset is now a no-op
  since the order token IS already a stable string representation
  of the pointer.
- setup-context: row comparison via comparePointers.
- context-factory: readContextHistoryOffset formats the pointer
  through the same formatter so loadContextHistory round-trips
  lookups.
- Tests updated to construct pointer objects from the existing
  offset(index) helpers; one superseded_at_offset assertion updated
  to match the new stream:<padded-offset>:<padded-subOffset> format.

All 519 existing tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #347 (Stream-Fork-Sub-Offset, the dependency for fork-at-message)
has been rebased onto PR #350. The rebase needed one extra fix: PR
size (payload + 5 bytes overhead), so the sub-offset prefix write in
PR #347 had to be updated to match or chained sub-offset forks fail.

PR #347 is now retargeted to base on PR #350's branch and its CI is
green. pkg.pr.new has been re-published with the combined PR #350 +
PR #347 + fork-frame fix under the @347 URL.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements Phase 3 of docs/fork-at-message.md. A fork PUT can now
carry an optional pointer `{ offset, sub_offset }` (snake_case on the
wire) addressing an event on the source root's `main` stream:

- `streamClient.fork(path, source, { forkPointer })` emits
  `Stream-Fork-Offset` + `Stream-Fork-Sub-Offset` per PR #347.
- `streamClient.readJsonWithPointers(path)` reads a JSON stream and
  yields each item with a `{ offset, subOffset }` pointer minted by
  counting items locally within each `JsonBatch`, mirroring how
  `entity-stream-db.ts`'s `onBeforeBatch` already mints them on the
  runtime side. Pointers are stable across the runtime and the server.
- `entity-manager.forkSubtree({ forkPointer })`:
  - validates the pointer against the root's `main` stream
    (`HTTP 400` on offset/sub-offset mismatch),
  - filters the root's manifest to entries with pointer ≤ X,
  - drops descendants whose manifest entry on the root was filtered
    out (their subtrees go with them — Q3 "strict on root"),
  - threads the pointer only to the root's `main` fork; `error` and
    shared-state streams clone at HEAD (Q2).
- `forkBodySchema` gains `fork_pointer`; the route handler translates
  snake_case → camelCase at the body-parse boundary.
- `createForkEntity(url, { pointer })` extends the UI client helper.

Phase 4 ("Fork from here" affordance on user-message rows) builds on
this in a follow-up commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements Phase 4 of docs/fork-at-message.md.

`UserMessage` gets a hover-revealed icon button (top-right of the
bubble) that re-rolls the conversation starting at the clicked
message. The conversation up to and including the previous completed
response is forked into a new entity; the new entity boots idle.

`ChatView` computes the fork anchor map by walking the timeline rows
in order, tracking the latest preceding `runs` row with
`status === 'completed'`, and looking up that run's pointer via
`db.collections.runs.__electricRowOffsets`. Inbox rows with no
preceding completed run (first message, in-flight run) get no entry
in the map, which suppresses the affordance — see Q4 "Affordance is
disabled when…".

UX matches the existing SearchPalette and SplitMenu fork flows:
silently swallow errors, navigate to the new entity URL on success.

Note: this hasn't been exercised in a browser yet — the agents-server
backend needs Docker, which isn't running locally. Type-checks clean
across the affected packages.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous implementation compared each event's runtime-minted
pointer to the fork pointer via `comparePointers`. That breaks because
the two pointers live in different coordinate systems:

- The runtime mints pointers from LIVE delivery-batch boundaries
  (`previousBatchOffset` = end of the previous HTTP response).
- The agents-server's pre-read (`readJsonWithPointers`) groups the
  same events into HISTORICAL batches, which can be arbitrarily
  chunked by the server — often into a single big batch on a
  one-shot `live:false` read.

A pointer minted live as `{X, N}` won't match any historical batch's
anchor on the server side, so the previous validation rejected every
real pointer with "fork_pointer.offset does not match any chunk
anchor", and the manifest filter let all events through (no event
pointer ever matched).

Switch both validation and filtering to canonical CUMULATIVE position
in the source's flattened history: walk events, group by their TRUE
`headers.offset` (the per-event Stream-Next-Offset, stored at write
time and stable across reads). Translate the fork pointer to "count
of events with headers.offset ≤ anchor, plus subOffset" — this matches
the PR #347 server's "N flattened messages past anchor" interpretation
regardless of how delivery was chunked.

Verified end-to-end with a running stack: two completed Horton runs,
fork at pointer `{offset: <run-0-completion-batch-end>, sub_offset:
3}` produced exactly the expected 10-event truncated history; bad
offset and out-of-range sub_offset both surface 400s; no-pointer
fork still produces a HEAD clone unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #347's `Stream-Fork-Sub-Offset` addresses items WITHIN A SINGLE
LOG ENTRY (the first one past the anchor), not items globally past
the anchor. The previous pointer minting used the live-delivery
batch's anchor + position-in-batch, which is only equivalent to the
server's semantic when each delivery batch contains exactly one log
entry.

During catch-up reads, the server can combine multiple log entries
into one HTTP response. The runtime's `onBeforeBatch` then minted
sub-offsets that span entry boundaries; when the UI later tried to
fork at one of those pointers, the durable-streams server interpreted
the sub-offset against only the FIRST entry past the anchor and
either picked the wrong event or returned 400 "overshoots".

Fix: group items in a batch by their `headers.offset` (the per-event
Stream-Next-Offset, stamped at write time). Items sharing an offset
belong to the same log entry; sub-offsets reset to 1 at each
entry boundary; the anchor for an entry is the END of the preceding
entry (`previousBatchOffset` for the first entry in a batch).

Also drops the now-unneeded `comparePointers` import in
entity-manager.ts (the manifest filter switched to canonical position
in the previous commit, and the position lookup no longer compares
pointers directly). Also tightens that filter to handle `null`-anchor
fork pointers correctly (stream-start: no events precede the anchor).

Verified end-to-end on a running stack with two completed Horton
runs, where the live read happened to chunk text/text_delta across
two separate batches. The runtime mints the run-completion pointer
exactly as the server's first-log-entry-past-anchor interpretation
expects, and the fork produces the right truncated history.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`forkSubtreeInner` previously called `waitForIdleSubtree` for every
fork, which polls until *every* entity in the subtree — including the
root — flips to `status: 'idle'`. The runtime keeps a worker warm for
five minutes after the last `handler returned` (so a follow-up
message hits a hot process instead of paying cold-start), so an
entity that's visibly "done" in the UI still reports `running` for
that whole window. The default fork wait is 120 s, which means
clicking the fork button right after the response lands hangs for two
minutes and then 409s — exactly the case the affordance is designed
for.

The full-subtree-idle constraint is necessary for HEAD-clones: those
read the source's state at "now," and concurrent writes during the
read produce a torn snapshot. For pointer-forks it's strictly
unnecessary: we read the source's `main` historically up to the
chosen offset, and concurrent writes past that offset can't affect
anything we capture. The Q3 manifest filter only keeps events with
`headers.offset ≤ pointer`, also frozen-in-time.

Skip the root-idle wait when `opts.forkPointer` is set. Kept
descendants (Q3 "loose on descendants") are still HEAD-cloned, so
they still need to be idle before we read their snapshots — wait+
lock only those once `computeEffectiveSubtree` has identified them.
The HEAD-clone path is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Remove pointers to the unmerged durable-streams PR (#347) and to the
local-only RFC doc (`docs/fork-at-message.md`, including its Q2/Q3/Q4
section callouts), and replace one special ellipsis character with
three dots. Comments now stand on their own — they describe the
behaviour, not where the spec lives.

No behaviour change.
…tests / electric-ax

The `Check Changeset` CI script scans every changed file (including
`package.json` pin bumps) and demands a changeset for each affected
package. Our existing `fork-at-message-*` changesets cover
`agents-runtime`, `agents-server`, and `agents-server-ui` — but the
durable-streams pin also moved in `agents`, `agents-server-conformance-tests`,
and `electric-ax`. Add a single patch entry for those three so the
check passes; the bump is mechanical (no other code changes in these
packages).
durable-streams#347 has merged and the fork-at-pointer wire protocol
(`Stream-Fork-Sub-Offset`) is now on the published packages:

- @durable-streams/client  ^0.2.6
- @durable-streams/server  ^0.3.5
- @durable-streams/state   ^0.2.9

Replaces the `pkg.pr.new/...@347` pins introduced in b9c9ab8.
@kevin-dp kevin-dp force-pushed the kevin/fork-at-message branch from 5997c95 to 2085be1 Compare June 2, 2026 09:14
@claude
Copy link
Copy Markdown

claude Bot commented Jun 2, 2026

Claude Code Review

Summary

Two follow-up commits land cleanly on top of the previous iteration: 1cb99d2e drops the dead readJsonWithPointers from stream-client.ts and its lone test caller, and e2f1365 wires fork failures into a danger toast via a new showForkFailureToast that mirrors the existing showSignalFailureToast shape. Nothing else changed in the PR; the remaining suggestions from iteration 1 still apply but are unchanged in scope.

What's Working Well

  • Toast helper is symmetric with signal flows. showForkFailureToast (packages/agents-server-ui/src/lib/ElectricAgentsProvider.tsx:372-393) reuses parseErrorResponse / compactToastText and matches showSignalFailureToast so the three error sites (network throw, non-2xx, invalid response shape) get consistent UX. The 400 surfaces from resolveForkPointerTarget (invalid offset / out-of-range sub-offset) now reach the user verbatim via parseErrorResponse(text).
  • Re-throw preserved. The action still rejects after toasting, so the ChatView .catch(() => {}) is now a deliberate "swallow because the toast already fired" rather than a silent drop — and a comment at ChatView.tsx:252-253 says exactly that.
  • Clean removal. readJsonWithPointers and its test (stream-client-fork.test.ts:81-104) deleted together, and grep confirms no remaining references in the tree.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

None new — items 1 and 2 from iteration 1 are now resolved (see Previous Review Status).

Suggestions (Nice to Have)

The carry-over suggestions from iteration 1 still stand — repeating only the headline so the author can decide whether to address before merge or punt to a follow-up:

  • Document the offset-format assumption in resolveForkPointerTarget (packages/agents-server/src/entity-manager.ts:1433) — the lexicographic eventOffset <= pointer.offset works because durable-streams offsets are fixed-width hex, but that's invisible from the code.
  • Manifest filter only considers kind === 'child' (packages/agents-server/src/entity-manager.ts:1468). Cross-checked against the rest of the module — the other manifest kinds (shared-state, source, attachment, schedule) use id / sourceType rather than entity_url, so this is probably correct. A one-line comment noting "only child entries carry subtree URLs" would lock in the assumption.
  • Fork anchor walks past failed runs (ChatView.tsx:245): only status === 'completed' updates anchor, so a failed run's downstream user message gets a button anchored at the previous successful run. Likely intentional ("fork from last good state") but undocumented.
  • orderTokenToHistoryOffset is an identity function (entity-timeline.ts:620). Leave as documentation or inline — minor.
  • LLM wire-format change for superseded_at_offset / load_context_history. Round-tripped consistently within a session; only a concern if persisted transcripts from before this PR's deploy need to lookup correctly.
  • Coverage on validation paths. entity-manager.ts patch coverage remains at 12.6%; resolveForkPointerTarget and computeEffectiveSubtree are the two methods most likely to regress silently.

Issue Conformance

No linked issue (flagged in iteration 1; PR description remains unusually thorough). The PR description's "Known gaps / follow-ups" section still lists createForkEntity swallows errors silently as gap #1 — that gap is closed by e2f1365 and the description should be updated to reflect that, otherwise a reader picking up the PR cold will think it's still open.

Previous Review Status

Iteration 1 issues addressed:

  • (1) readJsonWithPointers dead code — removed in 1cb99d2e along with its test.
  • (2) Fork error swallowed in UI — surfaced via showForkFailureToast in e2f1365, with re-throw preserved.

Iteration 1 suggestions (3–8) carried forward unchanged — see above.


Review iteration: 2 | 2026-06-02

Copy link
Copy Markdown
Contributor

@icehaunter icehaunter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server code looks sane. I think Claude has a valid point about dead code function

@icehaunter
Copy link
Copy Markdown
Contributor

Also Claude's point on the error - I've added some error surfacing with a toast - we can wire that in

kevin-dp and others added 2 commits June 2, 2026 14:11
The method had no production callers — the fork path in entity-manager
uses readJson + manual position math, and the only caller was its own
test. Drop both until a real caller lands.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fork failures used to be swallowed by the .catch(() => {}) at the
ChatView call site, so server-side validation errors (invalid
fork_pointer.offset, out-of-range sub_offset, etc.) never reached the
user. Emit a danger toast from createForkEntity for network failures,
non-2xx responses, and invalid response shape — matching the
kill/signal toast pattern from #4452. The action still re-throws so
existing callers continue to short-circuit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kevin-dp kevin-dp merged commit d5708c7 into main Jun 2, 2026
17 checks passed
@kevin-dp kevin-dp deleted the kevin/fork-at-message branch June 2, 2026 13:00
kevin-dp added a commit that referenced this pull request Jun 2, 2026
Follow-up to #4410: extends the per-message "Fork from here" affordance
to the mobile chat (and any other surface that mounts agents-server-ui
via an Expo "use dom" embed).

## What changes

Two source files, both in `packages/agents-server-ui`:

1. **`src/components/views/ChatView.tsx`** — `ChatLogView` (the view the
mobile embed mounts under `view='chat-log'`) now computes the same
`forkFromHereByInboxKey` map that `ChatView` already had on the web
side. It pulls `db` from `useEntityTimeline`, `forkEntity` from
`useElectricAgents`, walks `visibleRows` to find the latest preceding
completed `runs` row's pointer, and threads the per-row callback to
`EntityTimeline`. Without this, `UserMessage` rendered in the mobile
embed never received an `onForkFromHere` prop and the button stayed off
the DOM regardless of CSS.

2. **`src/components/UserMessage.module.css`** — adds a touch-targeted
reveal rule scoped via `:global(html[data-electric-mobile-dom='true'])
.forkButton { opacity: 1 }`. Touch devices don't fire `:hover`, and
`.bubble:focus-within` only kicks in after a tap, so without the
override the button stayed invisible until interaction. The reveal lives
inside the CSS module (rather than a sibling stylesheet that targets the
hashed class by substring) so whichever pipeline is in play — Vite for
the web/desktop bundles, Metro for the Expo DOM embed — hashes
`.forkButton` consistently with the JSX.

The mobile package itself doesn't change. The embed already mounts
`ChatView` → `EntityTimeline` → `UserMessage`, the fork POST already
routes through `serverFetch()` running in the embed's JS context, and
post-fork navigation already routes through `onRequestOpenEntity` →
`openSession(target)`. Once `ChatLogView` produces the callback map and
CSS makes the button visible, the whole flow lights up.

## Why split from #4410

#4410 was framed as the web/desktop release. The mobile surface depends
on the same wiring but ships separately so the web work can land without
being held by mobile-specific UX polish. Once both merge, the affordance
is uniform across web, desktop, and mobile.

## Test plan

- [ ] Build + run #4410's base. Start the mobile app with `pnpm -C
packages/agents-mobile ios`.
- [ ] Open any session with at least two completed runs.
- [ ] The second user-message bubble shows the GitFork icon at full
opacity, top-right corner of the bubble.
- [ ] Tap it. The app navigates to a new entity URL ending in
`-fork-<hash>`; the new session contains only the first exchange.
- [ ] Send a new prompt in the fork; it runs independently.
- [ ] First user message in a chat shows no button. While Horton is
streaming a reply, the triggering message shows no button. Button
returns after `✓ done`.
- [ ] On desktop and web, hover-reveal still works as before (the mobile
rule is scoped via the `data-electric-mobile-dom` attribute the embed
sets).

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

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants