feat(ui,client): version navigation and non-destructive prompt rewrite - #517
feat(ui,client): version navigation and non-destructive prompt rewrite#517Zerlight wants to merge 23 commits into
Conversation
|
Your Claude subscription has hit its usage limit. It resets at 11:30am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
Greptile SummaryThe PR adds conversation-version navigation and non-destructive prompt rewriting across the engine, client, workbench, and presentation layers.
Confidence Score: 5/5The PR appears safe to merge; no actionable new failure or outstanding repository-rule violation was identified. The follow-up changes correctly keep live error-reporting metadata transient while preserving deterministic operation replay, and they reject cross-session operation-ID reuse without affecting correctly generated submissions.
|
| Filename | Overview |
|---|---|
| packages/host/engine/src/conversation/turn-service.ts | Persists and settles graph turns, announces shape and terminal-state changes, and keeps live-only failure-reporting metadata out of replayed records. |
| packages/host/engine/src/session/lifecycle-service.ts | Adds explicit-parent submission admission, cross-session operation ownership checks, and failure cleanup for launch and dispatch paths. |
| packages/client/core/src/conversation-store.ts | Adds frozen projection stores so parked lineage reads do not consume events from the active run. |
| packages/client/workbench/src/surface/lineage.ts | Implements lineage traversal, sibling version selection, active-lineage detection, and safe continuation-parent selection. |
| packages/client/workbench/src/surface/workbench.tsx | Integrates parked-version navigation, explicit-parent rewrites and continuations, and return-to-default behavior. |
| packages/presentation/ui/src/chat/turn-version-nav.tsx | Adds the user-facing version navigation controls and turn-state presentation. |
Sequence Diagram
sequenceDiagram
participant UI as Workbench UI
participant Client as Client Core
participant Engine as Host Engine
participant Store as Conversation Store
UI->>Client: Read selected leafTurnId
Client->>Engine: conversation.read(leafTurnId)
Engine->>Store: Load root-to-leaf projection
Store-->>Engine: Lineage events and graph revision
Engine-->>Client: Projection seed
Client-->>UI: Live store or frozen parked store
alt Rewrite an existing prompt
UI->>Client: submitTurn(input, parent, revision)
Client->>Engine: turn.submit with explicit parent
Engine->>Store: Validate revision and persist sibling
Engine-->>Client: turn.submitted
Engine-->>UI: conversation.graph.changed
else Continue from a parked version
UI->>Client: submitTurn(input, last completed turn, revision)
Client->>Engine: turn.submit with explicit parent
Engine->>Store: Persist child and move default leaf
Engine-->>UI: conversation.graph.changed
end
Reviews (3): Last reviewed commit: "fix(workbench): mirror the daemon's sett..." | Re-trigger Greptile
73fc5ff to
992dff7
Compare
|
Your Claude subscription has hit its usage limit. It resets at 11:30am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
|
Warning Your Claude subscription has reached its daily usage limit, blocking all Pullfrog runs until it resets. Every Pullfrog run on arcboxlabs has failed since September 7 (5 runs, no successes), so this review did not happen. Anthropic's Claude subscription has hit its daily usage limit, which resets at 11:30am UTC. Since no ANTHROPIC_API_KEY is stored as a fallback, every run that tries to use the Claude model fails when the subscription window is exhausted. To fix it:
|
…and frozen parked stores
…ds in the dev mock
… through the turn graph
…nge and refresh the tree on park
…om the session's first root
… settles, and hide unrun turns
…y parked reads, and continue from the last completed turn
…h refusals, cancelled settles, and admit order in the dev mock
992dff7 to
1d942a6
Compare
|
Your Claude subscription has hit its usage limit. It resets at 11:30am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
…sks, pin read cursors to the item count, and match the mock's command text
|
Your Claude subscription has hit its usage limit. It resets at 11:30am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
There was a problem hiding this comment.
🟡 Changes recommended
The new graph-based prompt rewrite path still blocks (and strips) stored attachment refs in UserMessage, preventing non-destructive rewrites from preserving attachments as intended.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
packages/presentation/ui/src/chat/user-message.tsx:120
- When submitting an edit, stored attachment blocks are always stripped from
retainedBlocks, which would drop attachment refs on rewrite. With graph rewrites enabled, theseresource_linkattachment blocks should be preserved sopromptBlocksFromComposercan convert them intoattachment_refblocks on submit.
packages/presentation/ui/src/chat/user-message.tsx:93 - Edits are blocked whenever the prompt contains stored attachments (
resource_link→attachmentIdFromUri), but graph rewrites can submitattachment_refblocks (viapromptBlocksFromComposer) and should be allowed to rewrite prompts that include stored attachment refs. As-is, the UI showseditAttachmentsUnsupportedeven whenrewritesViaGraphis enabled and the message is a known graph node.
This issue also appears on line 115 of the same file.
- Files reviewed: 56/56 changed files
- Comments generated: 1
- Review effort level: Lite
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes — incremental re-review of the two commits since 4952b99:
fdda262fix(engine)—upload-service.tsgainsMAX_LIVE_UPLOADS = 32with areservedcounter released viaEffect.ensuring, aUPLOAD_IDLE_MS = 5 minidle reap that runs on the nextbegin, and asameDeclarationcheck that refuses a replayedoperationIdcarrying different declared fields instead of handing back another upload's id.8278289fix(client-core,workbench)— the attachment read walk now pins every page to the first page'sblobId/sizeBytes(plus offset, overrun, and empty-slice guards), anddev-mock-host.tsmirrors the daemon's begin-replay field check.
The bounding logic holds up. Effect.ensuring fires on success, failure, defect, and interruption, and the daemon forks each request through FiberSet.runtime, so exactly one reserve() pairs with one release() per admitted begin and hands off cleanly to live.size. sameDeclaration cannot false-refuse a legitimate replay either — attachment-store.beginUpload returns the caller's lease object verbatim, so name/mimeType round-trip unchanged. The new tests genuinely fail without their fixes: the cap test asserts exactly one refusal out of MAX_LIVE_UPLOADS + 1 concurrent begins (it would see zero without reserve()), and the reap test pins the exact UPLOAD_IDLE_MS - 1 boundary.
ℹ️ Stack sequencing
Both new commits fix the attachment-upload subsystem, which belongs to the PR below this one in the stack — the base here is ruocheng/code-637, not master. Merging bottom-up ships that PR with no live-upload cap, no idle stage reap, and an unpinned read walk, which are the exact surfaces these commits close. Worth confirming this is deliberate rather than a mis-targeted branch.
Technical details
Files touched by the two new commits and the layer that owns them:
packages/host/engine/src/attachment/upload-service.ts— attachment upload store (PR below)packages/client/core/src/client/attachment-channel.ts— attachment read channel (PR below)packages/client/workbench/src/mock/dev-mock-host.ts— mock parity for the above
Secondary: the PR body lists 10 commits against a commitCount of 16, and describes only version navigation / prompt rewrite. A reader landing here would not expect upload-bounding changes; refreshing the body would help whoever reviews the merge.
ℹ️ Nitpicks
packages/foundation/schema/src/wire/attachment.ts:36— the doc comment "a second begin with the same id returns the first" is now an understatement. Afterfdda262that holds only when the declared fields match; otherwise the begin is refused withinvalid_request. Not in this diff, so no inline anchor.
Claude Opus ｜ 𝕏
…donly_file projections to adapters as file links
There was a problem hiding this comment.
🔵 Needs a closer look
lineageParentKey can collide with valid TurnId values (e.g., 'root'), risking incorrect per-parent version memory and navigation behavior.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/client/workbench/src/surface/lineage.ts:9
lineageParentKeyuses the rawparentTurnId(or the sentinel'root') as an object key. SinceTurnIdSchemais any non-empty string, a real turn ID could be'root'(or collide with future sentinels), which would corruptpreferredChildBySessionlookups and make‹ ›navigation jump unpredictably. Encode non-null IDs with a prefix/length to avoid collisions while keeping'root'for the null parent.
- Files reviewed: 60/60 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
ℹ️ No new issues found — not approving only because one thread from the previous review is still open.
Reviewed changes — incremental re-review of the one commit since 8278289:
a8c1bb4fix(engine)—assertInlineAttachmentsSupportedgains animageCountcounter that refuses pastcapability.kinds.image.maxCount, andPromptMaterializer.toContentBlocksstops throwing on areadonly_fileprojection, emitting aresource_link(materialized path, name, MIME type, size) instead.
Both changes hold up, and I traced each for the failure mode that actually matters for it.
The new count cap cannot false-refuse. It is a new rejection on a path that previously had no count limit at all, so the risk is refusing something that used to work. Neither call site can reach it. session-input-dispatcher.ts:94 validates input.content before the engine appends its resource-context block, and that block is text, so nothing the engine adds is counted. On the history.branch rewrite path (lifecycle-service.ts:354) the content comes from a fresh edit composer, and the renderer strips stored-attachment resource_link blocks before sending — images are not accumulated across edit/retain cycles, so no pre-existing prompt can arrive carrying more than the 16-image cap. Counter placement is right too: it sits after the MIME check, which is correct because a MIME-rejected image already throws, and the post-increment comparison admits exactly maxCount.
The removed throw was dead code, not a loosened guard. PromptMaterializer.convert only emits a readonly_file block when the capability declares that representation. Every entry in AGENT_INPUT_CAPABILITIES uses INLINE_IMAGE_ATTACHMENT_CAPABILITY (representations: ['inline_image']) and grok-build declares no attachments, so intersectAttachmentCapability can never yield readonly_file. It is not injectable through capabilities-update either — that value lands on LiveSession for wire replay and is not what materializer.prepare receives — nor through any wire frame, nor through the dev mock. The only declaration in the repo is the test's FILE_CAPABILITY. When a real adapter eventually declares it, assertDeclaredAttachmentRepresentations (agent-adapter/src/base.ts:534-554) is the boundary check that catches a mismatch between what this emits and what the adapter accepts.
Both new tests genuinely fail without their fixes — the cap test pins the exact maxCount / maxCount + 1 boundary, and the materializer test asserts the emitted link by toEqual where the old code threw. I ran the two touched files locally: 17 tests pass.
Still open from the previous review
- The
dev-mock-host.ts:2101thread is unaddressed —attachmentBeginsis still cleared only inabortAttachmentUpload, never incommitAttachmentUpload, so a begin replayed after a successful commit resolves back to the committed upload where the daemon'sforget(uploadId)would mint a fresh one. Left open rather than re-raised. - Stack sequencing and the stale PR body (10 commits listed against a
commitCountof 17) are unchanged from the last review; noting them only so they are not lost, not as new findings.
Claude Opus ｜ 𝕏
… cap its name in the handler
…type before the begin frame
…lings once, and forget the mock's begin replay on commit
|
Review round on this PR. Both inline threads are fixed in 9176938 (top of the stack): |
There was a problem hiding this comment.
🔵 Needs a closer look
It spans wire/schema, engine session/turn attribution, and client UI/runtime state changes with large behavioral surface area that warrants final human verification.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/presentation/ui/src/chat/user-message.tsx:78
- The inline comment says graph rewrites resubmit “attachment refs”, but edits are explicitly blocked when the message includes attachment resource links (
hasPromptAttachmentdisables editing). This comment is misleading given the current behavior.
- Files reviewed: 67/67 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — incremental re-review of the three commits since a8c1bb4:
795f0b30revert(schema)+fix(engine)— the legacyresource.source.uploadwire variant goes back toname: z.string().min(1)/mimeType: z.string().min(1).optional(), andresource/service.tsupload()picks up the enforcement instead: the name is sliced toMAX_ATTACHMENT_NAME_LENGTH, an over-long MIME type gets a typedinvalid_request.bcd58057fix(client-core)— symmetric pre-send guard inattachment-channel.tsbeginUpload, rejecting an over-longname/mimeTypelocally rather than letting the modern (bounded) frame be dropped unanswered.91769388fix(workbench)— a module-levelurlGenerationcounter so a preview read started before a revoke can't mint an object URL after it;lineageVersionsgroups siblings by parent once instead of scanning per turn; and the dev mock'sforgetAttachmentBeginsnow runs on commit as well as abort.
The bound relocation is the right call and, importantly, it is a restore rather than a loosening — origin/master already carries the unbounded shape on that variant, so the tightening being reverted was introduced within this stack and never shipped. The rationale in the comment checks out end-to-end: a frame that fails the receiver's zod parse is dropped silently and never answered, hanging the sender's correlated promise, so tightening a v79 frame would strand an older peer with no error. Moving the check into the handler turns that silent hang into an answer. WIRE_PROTOCOL_VERSION 81 covers it and MIN_COMPATIBLE_WIRE_VERSION is correctly left alone, since loosening is non-breaking in the receive direction.
I traced the loosened path for anything that assumed the bound. Nothing does: every resource.source.upload still terminates in resource.uploaded or a typed request.failed via WireResponder.reply's Effect.catchCause, blobs.pathOf derives from the SHA-256 content address rather than the name, and SessionResourceSchema.name is itself unbounded — so no over-long value reaches a bounded schema, a DB column, or a filesystem path.
The urlGeneration guard is sound for the reason it needs to be: revokeAttachmentObjectUrls() and resetAttachmentPreviews() are the only production call sites of either counter, and they fire together in a single synchronous effect cleanup in workbench.tsx. That's what keeps attachment-preview.tsx's previews.set(id, result ?? {}) durable-miss cache from pinning a stale null across a session switch — the consumer's own generation check returns early before the write, and the retry timer is both cleared and generation-checked. StrictMode double-mount is fine.
Ran the touched suites locally — 49 tests across prompt-attachments, lineage, attachment-client, contract/wire/attachment, engine-resources, dev-mock-attachments, and dev-mock-conversation, all passing. The new assertions pin exact boundaries (toBe('n'.repeat(MAX_ATTACHMENT_NAME_LENGTH)), expect(createObjectURL).not.toHaveBeenCalled()) rather than asserting loosely around them.
The dev-mock-host.ts:2101 thread from the previous review is fixed and resolved, and the PR body is no longer stale — it now lists all 20 commits and describes the attachment hardening.
ℹ️ Stack sequencing (unchanged, not re-raising)
795f0b30 and bcd58057 join the earlier attachment commits in fixing a subsystem owned by the PR below this one — the base here is ruocheng/code-637, not master. Merging bottom-up ships that PR without these bounds. Flagged in an earlier review and still open; noting it only so the merge order is a deliberate choice rather than an accident.
Claude Opus ｜ 𝕏
|
Real-daemon verification of the edit path (live Claude Code, dev daemon, headless Chrome screenshots) found two projection bugs the dev mock cannot reproduce, both fixed on this branch:
Gates at b93989c: |
There was a problem hiding this comment.
🟡 Changes recommended
timelineLeftActiveLineage() can misclassify timelines as “off the active lineage” when activeLeafTurnId is absent, causing unnecessary resync behavior for graphs that don’t report an active leaf.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 67/67 changed files
- Comments generated: 1
- Review effort level: Lite
| export function timelineLeftActiveLineage( | ||
| userRowIds: readonly string[], | ||
| graph: ConversationGraphSnapshot, | ||
| ): boolean { | ||
| const known = new Set<string>(graph.turns.map((turn) => userRowMessageId(turn.turnId))); | ||
| const onPath = new Set<string>( | ||
| lineagePath(turnsById(graph.turns), graph.activeLeafTurnId).map((turn) => | ||
| userRowMessageId(turn.turnId), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes — incremental re-review of the two commits since 91769388. Both land entirely in packages/host/engine/src/conversation/projection-service.ts (+198/-84) and its test file; nothing else in the tree moved.
a5f406fffix(engine)—composeTailtakes a newownsTailflag. The watermark assignment is hoisted out of the journal block (a parked view still adopts the journal's high watermark), the entry walk and the CODE-35openRequestsbackstop are both gated onownsTail, and entries stamped for a turn off the read lineage are skipped via a newpathIdsset.b93989c6fix(engine)—composeDurablemoves from one corpus attribution per read to one per touched history. NewHistoryRead { attribution, partition, failed }pluschainOrder()order the turns of each history byparentTurnId(creation order as fallback), and each turn now renders fromread.partition.get(turn.turnId)/read.failed.get(turn.turnId)instead of a running index.leadingrows come only from the root turn's history read.
The core of b93989c6 holds up. The index maps line up with the attribution contract because the same hostTurns array is both handed to attributeLineage and iterated to build the maps, so partition.size/failed.size reproduce exactly CorpusAttribution.attributed[i] / .failed[j] (lineage-attribution.ts:19-33). The extra attributeLineage(…, liveHistoryId) backfill pass cannot clobber a turn's own binding — boundCut reads listBindings(turnId) and prefers the binding whose historyId matches the turn's own run, so bindings stay per (turnId, historyId). I also chased whether two same-parent siblings can ever share one historyId and land in one group: they cannot, except for a sibling that failed pre-dispatch. launch = { type: 'continue' } fires only when parentTurnId === record.activeLeafTurnId (lifecycle-service.ts:~700), activeLeafTurnId is written only by commitGraphMove ← commitRunning and never moves back, and the non-forkable resume fallback is explicitly refused when the parent already has a child. The pre-dispatch-failed case is handled correctly: the turn lands in the failed map, and chainOrder's creation-order fallback yields [A, B(failed), C], which is the order attributeAroundFailed expects. vitest run conversation-projection.test.ts → 30 pass locally.
ℹ️ Read cost of per-history attribution
composeDurable now issues one checkpoints.attributeLineage — and therefore one readCorpus — per touched history rather than one per read. That interacts with the cache invalidation in a way worth confirming: readCorpus passes freshAfter = this.turns.lastSettledAt(sessionId) (checkpoint-service.ts:140), which is session-wide, and history-service.ts:138 treats any entry with builtAt <= freshAfter as stale. So after any single turn settles, every one of the K touched histories rebuilds on the next conversation read, and conversation reads are driven by every graph announce/settle. On a session with several versions K is small, but the amplification is new. Not a blocker — just flagging in case a per-history freshAfter (or a probe keyed on the history's own last settle) is cheap to add later.
ℹ️ Also carried forward
- Stack sequencing (raised in run 2, unanswered): the attachment-subsystem commits earlier in this branch fix code owned by the PR below in the stack (base
ruocheng/code-637). These two new commits are correctly scoped to this PR, so nothing new here — noting only that the earlier question is still open. - The real-daemon edit path against live claude remains unverified (author-stated, carried from run 1).
b93989c6's fork-read path is the part that most wants a live confirmation, since the test fixture supplieshistoriesByIddirectly rather than exercising a real provider transcript.
Claude Opus ｜ 𝕏
| const ownsTail = | ||
| leafTurnId === record.activeLeafTurnId || | ||
| path.some((turn) => !TERMINAL_TURN_STATES.has(turn.state)); |
There was a problem hiding this comment.
The ownsTail gate itself has no test coverage — including from the test added alongside it.
I replaced this computation with const ownsTail = true; and re-ran conversation-projection.test.ts: all 30 tests still pass, including the new 'carries the live tail only into the lineage that owns the running turn'. Control: deleting the sibling filter (if (entry.turnId !== undefined && !pathIds.has(entry.turnId)) continue;) added in the same commit fails exactly 1 test. So the new test pins the pathIds filter, not the gate — its journal entries are all stamped for off-lineage turns, which the filter alone already drops.
The gate's two unique effects are the untested ones:
const openRequests = ownsTail ? this.openRequests(sessionId) : [];— without the gate a parked view surfaces the active run's open permission/question prompt, which thepathIdsfilter cannot catch (those come fromopenRequests, not the journal).- Journal entries with
entry.turnId === undefined— the filter short-circuits on those, so an unstamped live event still leaks into a parked read.
A case appending one unstamped journal entry plus one open permission request, then asserting the parked read carries neither, would pin the gate. Behaviour looks right as written; it's just load-bearing and currently free to regress.
…poser never falls back to defaults
|
76e383b — switching to an earlier version flickered the composer: the approval-policy button vanished and model/effort fell back to defaults. A parked store ( |
There was a problem hiding this comment.
🔵 Needs a closer look
It spans coordinated UI/workbench/client-core/engine/schema behavior changes (including a wire bump and lineage read semantics) where a final human pass is warranted despite strong test coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/presentation/ui/src/shell/lineage-notice.tsx:16
<AlertAction>wraps two buttons here but has no flex/gap styling. Other multi-action alerts in this codebase addclassName="flex gap-2"to keep buttons spaced and prevent awkward wrapping.
- Files reviewed: 67/67 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes — incremental re-review of the single commit since b93989c6. 76e383bf fix(client) touches three files, all in packages/client/core: src/conversation-store.ts (the only behavioral change), AGENTS.md, and tests/integration/conversation-store-projection.test.ts. Nothing else in the tree moved.
76e383bf— a frozen store (followLive: false) no longer skips the live buffer wholesale.sync()now always walks the entries pastconsumedSeq; while parked it folds only the ten types in the newSESSION_STATE_EVENT_TYPESset (status, mode/policy/model/effort, available commands/models, capabilities, both usage frames), bypassingadmit()so neither the watermark nor a resync applies, andsubscribe()keeps the event subscription and returns early only before the graph subscription.AGENTS.md's parked-view bullet is rewritten to match.
The premise is real, and I confirmed it rather than taking it on faith: status, currentModel and currentEffort are exactly what the composer renders from — send-vs-stop in composer.tsx:1124-1136 via shell-frame.tsx:184,256, and promptEditState: isRunning ? 'busy' : 'enabled' in workbench.tsx:697,721 — so a store that skipped those events genuinely did leave a parked composer at its seeded defaults. Bypassing admit() for these is the right call too: they carry no lineage content, so a watermark drop or an epoch/gap resync request from a read-only view would be wrong. And walking the buffer from consumedSeq is safe here because the EventBuffer is unbounded and cleared only on conversation-rewind / clearSession / clearAll (client/event-buffer.ts), so there is no truncated-history hazard; snapshot identity also stays stable when nothing folds, so React bails out of the re-render. npx vitest run conversation-store-projection.test.ts → 17 pass locally.
Two things inline: the notification half of the fix is untested (I reverted it and every test still passed), and 'status' is not purely session state — it reaches the parked timeline's own rendering.
ℹ️ Also carried forward
- The
ownsTailgate inprojection-service.ts:183-185still has no coverage (run 5, thread open). This commit doesn't touch that file, so the finding stands unchanged. - Stack sequencing (run 2, unanswered): the attachment-subsystem commits earlier in this branch fix code owned by the PR below in the stack (base
ruocheng/code-637). This commit is correctly scoped to this PR. - The real-daemon edit path against live claude remains unverified (author-stated, run 1). This commit's behaviour is client-local and fully exercised by the integration fixture, so it is not the part that wants a live confirmation.
Claude Opus ｜ 𝕏
| onStoreChange(); | ||
| }); | ||
| // A frozen store keeps its session state live but leaves graph changes to its owner. | ||
| if (!followLive) return unsubscribeEvents; |
There was a problem hiding this comment.
The subscription half of this fix has no test coverage — including from the test updated alongside it.
I restored the old if (!followLive) return noop; on this line, so a frozen store subscribes to nothing at all, and re-ran conversation-store-projection.test.ts: all 17 tests pass, including the renamed 'freezes a parked read’s content … but not the session’s state'. Control: neutering the fold branch above (lines 176-179 → a bare continue;) fails exactly 1 test — AssertionError: expected null to be 'claude-fable-5' at the new currentModel assertion. So the new case pins the fold, not the notification.
The blind spot is getSnapshot() calling sync() lazily: the test drives the store directly, so every read re-syncs whether or not a subscription exists. Under React, useSyncExternalStore re-reads only when the subscriber fires — without this line's change the parked composer would keep rendering its seeded defaults until some unrelated re-render happened to flush it, which is precisely the bug the commit set out to fix. Asserting that the store's own subscribe callback fires on a session-state event while parked would pin it.
Behaviour looks right as written; it's just load-bearing and currently free to regress.
| /** Session state, not lineage content: the latest of each wins, so a frozen store folds them | ||
| * without a watermark — a parked composer must not fall back to defaults. */ | ||
| const SESSION_STATE_EVENT_TYPES = new Set<AgentEvent['type']>([ | ||
| 'status', |
There was a problem hiding this comment.
'status' isn't purely session state — it moves the parked view's content too, which is the one thing the comment above says a frozen store won't do.
case 'status' (conversation.ts:583-591) also writes turnStopped, and snapshot() derives isSessionStreaming = !turnStopped && (status === 'running' || status === 'starting') (conversation.ts:758-768), overlaying isStreaming: true on the parked read's last assistant message and on any open reasoning item. Downstream, conversation-view.tsx:101 derives isThinking from conversation.status and feeds it to both the trailing <Spinner/> thinking… element and ended={index < segments.length - 1 || !isThinking}.
So while the active lineage is running, a parked ‹ 1/N › view of a settled version renders a "thinking…" spinner after its last turn, suppresses that turn's trailers (diff rollup / copy / reply actions), and re-animates its final assistant message through smoothText (turn-segment-view.tsx:175-188) — none of which belongs to that version.
Still a net improvement over a composer frozen at defaults, and status can't simply leave the set: it's what drives send-vs-stop and promptEditState: 'busy'. But if a parked view should read as settled, the seam is between the scalar and the derived overlay — pin isSessionStreaming to false on a parked snapshot (or fold status into the scalar without letting it clear turnStopped) and the composer stays live either way.
Summary
Phase 4 of CODE-627 — Conversation turn graph & immutable attachment store. Linear: https://linear.app/arcbox/issue/CODE-638/featuiclient-version-navigation-and-non-destructive-prompt-rewrite
Stack: #516 ← this PR (
ruocheng/code-638, baseruocheng/code-637) ← top of the stack. Merge bottom-up; this PR's diff is only its own commits.Phase 4:
‹ 1/N ›version navigation from the graph's sibling ordinals, a client-local parked view (pure reads that never move the host default), and the non-destructive rewrite — editing prompt T submits a sibling under parent(T) throughturn.submit, sends from a parked version continue from its last completed turn, and failed or cancelled turns keep their ordinal and badge. Engine side: inactive lineages read their own history, a failed leaf's shared prefix reads from the live history, every settle re-announces the tree at the same revision, and a relaunch whose dispatch failed is unwound so the thread keeps its own history.This PR also carries the review-round fixes for the whole stack (the code they touch moved in this branch, and the stack merges as a unit): failed-turn attribution, the delete-time launch guard, the preceding-checkpoint binding, the upload cap and idle reap, the read-walk identity check, the inline image count, the
readonly_fileconversion, and the legacy upload frame's wire shape. Each is recorded on its own PR's thread and on CODE-627.Commits
Verification
Every commit passed
pnpm check:ciandpnpm testat its own tip; the stack tip (76e383bf) is atpnpm check:ci0 errors,pnpm test3428 passed / 1 skipped. Adversarial reviewers (one per axis, isolated read-only worktrees) reviewed the branch; each P1/P2 was reproduced with a failing test or a probe step before its fix — the round-by-round record, including the six bot review rounds on #507–#517, is in the Linear issue's comments. Headless-Chrome probe ofdev:mock: two prompts → edit →2/2(zero stale alerts during the edit) →‹1/2+ parked notice → Back to latest →failedit3/3 Failed→refuseedit3/4 Failed→›4/4 Failed→ send →5/5under the first turn; no console errors. The real-daemon edit path was then verified against live Claude Code (dev daemon, headless Chrome screenshots on CODE-627): it found three bugs the mock cannot show — a parked read carried the active run's live tail, the shared prefix rendered from the fork's re-stamped copy ("Thought for 10 seconds" vs "1 second"), and the parked composer fell back to defaults — fixed in the last three commits, after which both versions render their shared turns identically and the composer keeps policy, model, and effort while parked.Checklist
pnpm check:ciandpnpm testboth pass (no Rust changes)dev:mockin headless ChromeSessionRun.abandonedAt);resource.source.uploadkeeps its v79 shapeAGENTS.mdand module docs in this branch)