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 17 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 →
|
|
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 →
|
…yed begin with other fields
…or the begin replay rule in the mock
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
| /** Each path turn's version among its siblings, keyed by its user row's message id. */ | ||
| export function lineageVersions( | ||
| turns: readonly ConversationGraphTurn[], | ||
| path: readonly ConversationGraphTurn[], | ||
| ): Map<string, TurnVersion> { | ||
| const versions = new Map<string, TurnVersion>(); | ||
| for (let i = 0, len = path.length; i < len; i++) { | ||
| const turn = path[i]; | ||
| const siblings = siblingsOf(turns, turn); | ||
| versions.set(userRowMessageId(turn.turnId), { | ||
| index: siblings.findIndex((sibling) => sibling.turnId === turn.turnId) + 1, | ||
| count: siblings.length, | ||
| state: turn.state === 'failed' || turn.state === 'cancelled' ? turn.state : null, | ||
| }); | ||
| } | ||
| return versions; | ||
| } |
![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 ｜ 𝕏
| code: 'invalid_request', | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
nit: the mirror is one step short of the daemon. On the daemon, commit calls forget(uploadId), which clears both live and begunByOperation — so a begin replayed after a successful commit mints a fresh upload. The mock never clears attachmentBegins on commit (only abortAttachmentUpload does, at line 2258), so the same replay resolves back to the committed upload instead.
Harmless in isolation, but this is the exact rule the commit set out to mirror, and the mock is maintained as a daemon-parity fixture. Clearing the matching attachmentBegins entries in commitAttachmentUpload alongside the existing abort-path loop would close it.
…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 ｜ 𝕏
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.Commits
Verification
Every commit passed
pnpm check:ciandpnpm testat its own tip; the stack tip (1d942a62; the same tree as the originally gated73fc5ff2plus the two review fixes below on 628/629) is atpnpm check:ci0 errors,pnpm test3408 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 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. Not verified here: the real-daemon edit path against live claude (needs a paid turn).Checklist
pnpm check:ciandpnpm testboth pass (no Rust changes)dev:mockin headless ChromeSessionRun.abandonedAt)AGENTS.mdand module docs in this branch)