Add option to fork from any message (user or assistant)

[Hrovatin]

Closes #439

Lets you fork a session from any message in the transcript — user or assistant — in the /fork selector, across the interactive TUI and the dashboard.

Role determines the branch semantics:

• Assistant message → the new branch includes that reply (and everything before it); the editor stays empty. "Continue from this answer." Forking at the last assistant message keeps the entire current state.
• User message → rewind to before the question and pre-fill the editor with its text. "Edit / re-ask this question." (unchanged from before)

This supersedes the earlier "fork from current state" approach per the maintainer discussion in the issue — forking at the last assistant message reproduces it, so the dedicated surface (forkFromCurrent, the fork_current RPC, and the dashboard fork-current button/endpoint) was removed.

See the revised plan comment for the full design and per-layer changes.

[Hrovatin]

Implementation Plan

Problem

The /fork selector only offers user messages, and fork() branches from selectedEntry.parentId — i.e. it rewinds to before the chosen user message. Because fork points are user-message-only and forking always excludes the selected message forward, the last model response can never be captured in a fork. For U1,A1,U2,A2,U3,A3, forking at U3 yields a branch whose tail is A2; U3 and A3 are dropped.

Approach

Add a dedicated "fork from current state" path that branches from the current leaf (getLeafId()), including everything up to and including the last entry (A3), with no editor pre-fill. The existing user-message rewind fork stays completely untouched. The only mechanical difference is which id is passed to createBranchedSession: existing fork uses selectedEntry.parentId; the new path uses the leaf id.

Deliverables

1. Core — packages/coding-agent/src/core/agent-session.ts

• New method forkFromCurrent(): Promise<{ cancelled: boolean }>:
  • leafId = sessionManager.getLeafId(); if null (empty session), no-op → return { cancelled: true } (surface a "nothing to fork" status in callers).
  • Emit session_before_fork with entryId = leafId (reuses existing event contract; respects cancel / skipConversationRestore).
  • Clear _pendingNextTurnMessages, call createBranchedSession(leafId) (includes the leaf), reload via buildSessionContext(), emit session_fork, replaceMessages (unless skipped).
  • No selectedText returned (no pre-fill).
• Refactor the shared tail of fork() and forkFromCurrent() into a private helper (e.g. _finalizeFork(previousSessionFile, { skipRestore })) to avoid duplication. fork()'s signature/behavior stays identical.

2. RPC layer

• modes/rpc/rpc-types.ts: add command { type: "fork_current" } and response { command: "fork_current"; data: { cancelled: boolean } }.
• modes/rpc/rpc-mode.ts: case "fork_current" → session.forkFromCurrent().
• modes/rpc/rpc-client.ts: async forkCurrent(): Promise<{ cancelled: boolean }>.

3. Interactive UI — modes/interactive/interactive-mode.ts + components/user-message-selector.ts

• Add a distinct leading row to the fork selector — e.g. ⎇ Fork from current state (include last response) — above the user-message list. Selecting it calls session.forkFromCurrent() (empty editor, status "Branched to new session including last response"). Selecting a user message keeps existing rewind + pre-fill behavior.
• UserMessageSelectorComponent gains an optional "current state" action row + callback; update the header text to describe both modes.
• /fork entry point and the double-Escape / app.session.fork bindings are unchanged (they just now show the extra row).

4. Dashboard

• packages/dashboard/src/server/server.ts: POST /api/runtimes/:key/fork-current → h.client.forkCurrent().
• packages/dashboard/src/client/api.ts: forkCurrent(key).
• packages/dashboard/src/client/screens/session.tsx: add a "fork from current state (include last response)" action in the fork modal (and/or a /fork-current command) → api.forkCurrent, then hydrateSession + refreshDiskSessions, no composer pre-fill.

Testing (mandatory)

• test/session-manager/tree-traversal.test.ts — unit: createBranchedSession(getLeafId()) produces a branch whose tail is the last assistant message (the core "include last response" guarantee), covering both in-memory and persisted-file paths.
• test/agent-session-branching.test.ts — non-live tests (no API key needed): build a session via sessionManager.appendMessage(userMsg/assistantMsg), then forkFromCurrent() — assert (a) branch includes the last assistant message, (b) empty-session no-op returns cancelled, (c) session_before_fork cancel path via createHarnessWithExtensions, (d) existing fork() behavior unchanged.
• RPC — wire test for fork_current with a mocked session (pattern from rpc-tree-commands.test.ts) + RpcClient.forkCurrent.
• Dashboard — test/client/screens.test.tsx: "fork from current state includes last response" mocking api.forkCurrent, asserting no composer pre-fill + hydrateSession called; test/server.test.ts: route forwards to forkCurrent.

Files touched (summary)

Layer	Files
Core	core/agent-session.ts
RPC	modes/rpc/rpc-types.ts, rpc-mode.ts, rpc-client.ts
TUI	modes/interactive/interactive-mode.ts, components/user-message-selector.ts
Dashboard	dashboard/src/server/server.ts, client/api.ts, client/screens/session.tsx
Tests	test/session-manager/tree-traversal.test.ts, test/agent-session-branching.test.ts, RPC fork test, dashboard/test/client/screens.test.tsx, dashboard/test/server.test.ts

Risks / open questions

• UI shape — in-selector row (chosen, matches issue AC "discoverable in the /fork UI") vs. a dedicated keybinding/command. Easy to switch to a separate key if preferred.
• Empty session — forkFromCurrent() on a fresh session is a no-op returning cancelled; confirm we want a status message rather than an error.
• Extension event — reuses session_before_fork with the leaf id, keeping the extension contract stable (no new event type). If extensions need to distinguish "current-state" forks, we'd add a field.

---

Plan created by mach6

[Hrovatin]

Progress Update

Implemented "fork from current state (including the last model response)" end-to-end across all four layers, plus a test-infra fix.

Architecture

The feature adds a second fork entry point that branches from the current session leaf (so the last assistant response is retained), complementing the existing "fork at a user message" flow which rewinds to before a selected user message.

• Core (core/agent-session.ts) is the anchor. fork(entryId) and the new forkFromCurrent() both delegate to a shared private _performFork(entryId, branch) helper, so the two paths share the snapshot/branch/reset/event tail and the existing fork() contract is unchanged. forkFromCurrent() differs only in its target: it branches from sessionManager.getLeafId() (last node, incl. the assistant reply) instead of the selected user message's parent, and returns { cancelled: true } for an empty session. Both reuse the cancellable session_before_fork / session_fork extension events.
• RPC wraps the core method: rpc-types.ts adds the fork_current command + { cancelled } response, rpc-mode.ts dispatches it to session.forkFromCurrent(), and rpc-client.ts exposes RpcClient.forkCurrent(). This is what the dashboard talks to.
• TUI (interactive-mode.ts + components/user-message-selector.ts) surfaces the action inside the existing /fork selector: a distinct action row (sentinel FORK_FROM_CURRENT_ID, isAction flag, ⎇ prefix, no "Message N of M") sits above the user-message list. Selecting it calls forkFromCurrent() with no composer pre-fill.
• Dashboard mirrors the TUI: server/server.ts adds POST /api/runtimes/:key/fork-current, client/api.ts adds forkCurrent(), and client/screens/session.tsx adds a "fork from current state" button to the fork modal (its own .fork-current-btn class so existing .fork-message selectors are unaffected), styled in app.css.
• Test infra (test.sh): unset the repo-location git vars (GIT_DIR, GIT_INDEX_FILE, GIT_WORK_TREE, …) at the top of the script. When the suite runs from the husky pre-commit hook, git exports these and they leak into tests that shell out to git in throwaway temp repos (git-update.test.ts, tools.test.ts), redirecting their git init/clone/commit at the parent repo. Identity vars are left intact.

New files

• packages/coding-agent/test/agent-session-fork-current.test.ts — core forkFromCurrent() tests: includes the last response, empty-session no-op, and extension-cancel path.
• packages/coding-agent/test/rpc-fork-current.test.ts — RpcClient.forkCurrent() wire-format tests.

Modified files

• packages/coding-agent/src/core/agent-session.ts — forkFromCurrent() + shared _performFork() helper.
• packages/coding-agent/src/modes/rpc/rpc-types.ts — fork_current command + response types.
• packages/coding-agent/src/modes/rpc/rpc-mode.ts — fork_current handler.
• packages/coding-agent/src/modes/rpc/rpc-client.ts — forkCurrent() client method.
• packages/coding-agent/src/modes/interactive/interactive-mode.ts — wire the action row into the fork selector.
• packages/coding-agent/src/modes/interactive/components/user-message-selector.ts — action-row sentinel, isAction flag, distinct rendering.
• packages/coding-agent/test/session-manager/tree-traversal.test.ts — branch-from-leaf retains the last assistant response.
• packages/dashboard/src/server/server.ts — POST /api/runtimes/:key/fork-current.
• packages/dashboard/src/client/api.ts — forkCurrent().
• packages/dashboard/src/client/screens/session.tsx — fork-modal current-state button.
• packages/dashboard/src/client/styles/app.css — .fork-current-btn styling.
• packages/dashboard/test/client/screens.test.tsx — client test for fork-from-current (no composer pre-fill).
• packages/dashboard/test/server.test.ts — /fork-current route forwarding.
• packages/dashboard/test/runtime-pool.test.ts — fake client gains forkCurrent/fork/getForkMessages.
• test.sh — unset leaked git-location env vars so hook-invoked runs of git-shelling tests pass.

Commit: c1c3b63

Verification

• Full npm run build (typechecks every package) — clean.
• biome check on all changed files — clean.
• Full suite via the pre-commit hook (bash test.sh --no-live-api): 5551 passed, 0 failed, 710 skipped.
• Fork-specific tests pass in isolation across core, RPC, and dashboard layers.

Migration notes

No schema/config changes. The fork_current RPC command and POST /api/runtimes/:key/fork-current endpoint are additive; existing fork / get_fork_messages behavior is unchanged.

---

Progress tracked by mach6

[Hrovatin]

Code Review

Five review agents evaluated the change against issue #439's acceptance criteria. Completeness is fully met — all 6 ACs map to real, working code, and the _performFork refactor was verified to preserve fork()'s exact event ordering and semantics. Findings below are ordered by severity.

Critical

None.

Important

A. _performFork can leave the session half-branched if buildSessionContext() throws after the branch switch (error-auditor, conf 85)
In _performFork (core/agent-session.ts), the sequence branches the session (mutating sessionManager + agent.sessionId) and only then calls buildSessionContext() / replaceMessages(), with no try/catch. If buildSessionContext() throws after branch() succeeds, the active session/id are already switched to the new branch while agent.messages still holds the old conversation — subsequent turns append to the new branch on top of stale content. This exact structure pre-existed for fork(), but the PR now makes it reachable via a second path (forkFromCurrent() → TUI action row, RPC fork_current, dashboard endpoint).

B. session_fork (after-fork) event is never asserted by any test (test-reviewer, conf 92)
_performFork emits session_fork after branching, and AC5 explicitly requires it to fire. A suite-wide grep for session_fork (excluding session_before_fork) returns zero references. Only the "before" cancellation path is covered. The existing extensionFactories harness makes a direct assertion straightforward.

C. Cross-session tree parenting (parentSession header) is never verified (test-reviewer, conf 88)
AC5 requires correct tree parenting. All new forkFromCurrent tests use SessionManager.inMemory() (persist: false), but createBranchedSession only sets parentSession when this.persist is true — so in-memory tests can't distinguish a correctly-parented new branch from "no branch happened." No test uses a file-backed manager to assert header.parentSession === previousSessionFile.

Suggestions

D. Dashboard "fork from current state" ignores the cancelled result and is shown unconditionally (code-reviewer conf 85 + error-auditor conf 90, medium)
forkFromCurrentState() (dashboard/src/client/screens/session.tsx) calls api.forkCurrent() and always hydrates/refreshes/closes the modal regardless of result.cancelled. forkFromCurrent() returns cancelled: true for an empty session (no leaf) or when a session_before_fork extension vetoes — both render as silent false-positive success. The TUI gates the action row on hasCurrentState and branches on cancelled; the dashboard button does neither (rendered unconditionally, result unchecked). Sibling selectForkMessage has the same latent gap but this PR copies rather than fixes the pattern.

E. TUI action-row selection path has zero test coverage (test-reviewer, conf 90, medium)
The new showUserMessageSelector logic (interactive-mode.ts) — building the action row when hasCurrentState, routing FORK_FROM_CURRENT_ID to forkFromCurrent(), success vs cancelled UI branches — is unexercised. Existing interactive-mode-*.test.ts files show this is testable without new infra.

F. skipConversationRestore result path from session_before_fork is untested (test-reviewer, conf 85, medium)
_performFork honors result?.skipConversationRestore to skip replaceMessages, but no test sets it true and asserts the conversation is left as-is. Shared by both fork paths.

G. Duplicated fork-completion logic in the TUI selector callback (simplifier, conf 88, low)
The two branches in the interactive-mode.ts selector callback are identical except for the fork method, editor pre-fill, and status text — collapsible into one path (~8 fewer duplicated lines), behavior-preserving.

H. Near-identical dashboard fork handlers could share a completion helper (simplifier, conf 85, low)
selectForkMessage and forkFromCurrentState share setup/cleanup/error-handling; a small finishFork(action) helper would dedupe them (and naturally skip pre-fill when the result has no text).

Strengths

• All 6 acceptance criteria genuinely met — completeness-checker mapped each to concrete code; the print-mode caveat in AC4 is correctly N/A (fork never existed in print mode).
• Refactor verified byte-for-byte — code-reviewer confirmed _performFork preserves fork()'s exact event ordering, pending-message clearing, and empty/cancelled early-return semantics; createBranchedSession(leafId) correctly includes the leaf so the last assistant response is retained.
• Dashboard tests are behavior-level and adequate — assert forkCurrent called with right key, fork not called, hydrate/refresh invoked, and no composer pre-fill.
• test.sh git-env fix is a genuine infra hardening (immunizes hook-invoked git-shelling tests).

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[Hrovatin]

Review Assessment

Assessed against the review comment: #440 (comment)

Each finding was verified by reading the actual code and run through two gates — factual (is it a real problem in current code?) and scope (must it be fixed to deliver issue #439 safely?). A finding is genuine only if both pass.

Classifications

Finding	Classification	Reasoning
B — session_fork event never asserted	genuine	Factual: Suite-wide grep for session_fork (excl. session_before_fork) returns zero refs; only the cancel path is tested. The emission was moved into the refactored shared _performFork (agent-session.ts). Scope: AC5 explicitly requires session_fork fire; the PR relocated the emission into new shared code, so a test proving the refactor preserved it is ship-with-PR coverage. Trivial with the existing createHarnessWithExtensions harness.
D — Dashboard ignores cancelled; button ungated	genuine	Factual: forkFromCurrentState() (session.tsx:1262) never reads { cancelled } — always hydrates/refreshes/closes; .fork-current-btn (:2328) has no hasCurrentState gate, unlike the TUI. forkFromCurrent() returns cancelled:true for empty session / extension veto. Scope: AC4 requires the feature work correctly on the dashboard; new dashboard code silently reports success when no branch was created — a correctness defect in PR-introduced code.
E — TUI action-row path zero coverage	genuine	Factual: New showUserMessageSelector logic (interactive-mode.ts:4298–4330: hasCurrentState gating, FORK_FROM_CURRENT_ID routing, cancelled-vs-success branches) has no test. Scope: Net-new PR code implementing AC3 (discoverable entry) + AC1 wiring; sibling interactive-mode-*.test.ts prove it's testable without new infra.
A — _performFork half-branched on buildSessionContext() throw	deferred	Factual: Confirmed — branch()/agent.sessionId set before buildSessionContext()/replaceMessages() with no try/catch. Scope: buildSessionContext() is a pure in-memory walk over validated entries; a throw is speculative. Unchanged pre-existing structure in fork(), not a PR-introduced regression. Optional hardening.
C — Persisted parentSession lineage never verified	deferred	Factual: Confirmed — forkFromCurrent tests use inMemory(), where createBranchedSession skips parentSession (session-manager.ts:1247). Scope: Parenting lives entirely in createBranchedSession, which this PR does not modify. The distinctive new guarantee (branch includes last response) is tested both in-memory and persisted (tree-traversal.test.ts:443). Covers unchanged shared behavior — real gap, not required for safe delivery.
F — skipConversationRestore path untested	deferred	Factual: Confirmed no test sets it true. Scope: Pre-existing fork() behavior merely relocated into _performFork; not new to this PR and not named by any AC.
G — Duplicated TUI fork-completion logic	nitpick	Behavior-preserving stylistic collapse; no correctness or AC impact.
H — Dashboard handlers could share finishFork()	nitpick	Pure refactor preference; no correctness or AC impact.

Action Plan

1. D — Dashboard forkFromCurrentState() must honor cancelled and gate the button (session.tsx:1262, :2328). Check result.cancelled before hydrating/closing (surface a notice on cancel); gate .fork-current-btn on current-state availability to match the TUI. Add a dashboard test for forkCurrent returning cancelled: true. (Correctness in PR-introduced code; AC4.)
2. E — Add a TUI test for the action-row selection path (interactive-mode.ts:4298–4330): hasCurrentState gating, FORK_FROM_CURRENT_ID → forkFromCurrent() routing, success vs cancelled UI branches. (New-code coverage; AC1/AC3.)
3. B — Assert session_fork fires (agent-session.ts:3718) via an extension handler in the existing fork-current harness test. (Refactored-code coverage explicitly named in AC5.)

Deferred (optional follow-up, not blocking merge): A (speculative throw-safety, pre-existing), C (persisted parentSession lineage — unchanged shared code), F (skipConversationRestore, pre-existing). G and H are nitpicks.

---

Assessment by mach6

[Hrovatin]

Progress Update

Addressed the three genuine review findings (D, E, B) and the two nitpicks (G, H) attributable to this PR. Deferred findings A/C/F were left untouched as out-of-scope (pre-existing / unchanged shared code).

Architecture

The change hardens and dedupes the two fork surfaces so a fork that produces no branch is now always surfaced to the user instead of appearing to succeed.

• TUI (interactive-mode.ts): the fork-selector callback in showUserMessageSelector was collapsed from two near-identical branches into one shared tail (finding G) — it now computes isCurrent once, calls forkFromCurrent() or fork(entryId), and on cancelled shows "Fork cancelled — no new branch was created" (finding D) rather than silently dismissing the selector. Editor pre-fill is taken from the fork result's optional selectedText (empty for the current-state branch, since it already includes the last response).
• Dashboard (client/screens/session.tsx): selectForkMessage and forkFromCurrentState were factored onto a shared finishFork(action, cancelMessage) helper (finding H). The helper runs the fork action, and on cancelled sets a fork-modal error message and keeps the modal open (finding D) instead of hydrating/refreshing/closing as if a branch had been created; on success it pre-fills the composer only when the action returns re-ask text, then refreshes and closes.
• Tests: agent-session-fork-current.test.ts gains assertions that session_fork fires exactly once on the forkFromCurrent path and does NOT fire when a session_before_fork handler vetoes (finding B / AC5). A new interactive-mode-fork.test.ts drives showUserMessageSelector via the prototype-call pattern with a mocked selector component to cover the previously-untested TUI action row (finding E): row gating (present with a leaf, "No messages to fork from" when empty, row-only when a leaf but no messages), FORK_FROM_CURRENT_ID → forkFromCurrent() routing, success vs cancelled UI branches, and fork() routing with editor pre-fill. The dashboard client test gains a cancelled-path case asserting the modal stays open with a message and no session churn.

New files

• packages/coding-agent/test/interactive-mode-fork.test.ts — 6 unit tests for the interactive /fork selector wiring (finding E).

Modified files

• packages/coding-agent/src/modes/interactive/interactive-mode.ts — collapse duplicated fork branches (G); inform on cancel (D).
• packages/dashboard/src/client/screens/session.tsx — shared finishFork helper (H); inform on cancel + keep modal open (D).
• packages/coding-agent/test/agent-session-fork-current.test.ts — assert session_fork fires / doesn't fire on veto (B).
• packages/dashboard/test/client/screens.test.tsx — cancelled-path dashboard test (D).

Verification

• biome clean on all changed files.
• Full npm run build (typechecks every package) — clean.
• Full suite via the pre-commit hook (bash test.sh --no-live-api): 5560 passed, 0 failed, 710 skipped (+9 net new fork tests over the prior 5551).

Known limitations

Deferred findings remain open as optional follow-up, intentionally out of scope for this PR: A (speculative _performFork throw-safety — pre-existing structure in fork()), C (persisted parentSession lineage assertion — covers unchanged createBranchedSession), F (skipConversationRestore path — pre-existing fork() behavior).

Commit: bfe37a0

---

Progress tracked by mach6

[Hrovatin]

Code Review

Re-review of the full PR at HEAD bfe37a0, after the fixes for the prior review's findings D/E/B/G/H. Five agents ran (code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier). All previously-fixed items hold up: _performFork event ordering is correct, the collapsed TUI callback and dashboard finishFork helper are functionally sound, and all six issue acceptance criteria map to working, tested code.

Note: this comment was corrupted by a gh api -f body=@file mistake (the literal filename was posted instead of the file body) and has been restored. The content below is reconstructed faithfully from the assessment; the classifications live in the mach6-assessment comment.

Important

Finding 1 — TUI fork selector callback has no error handling; a throw becomes an unhandled rejection that can crash the interactive session. interactive-mode.ts (~L4322-4345). The async onSelect is invoked with no await/.catch() at the call site and has no try/catch; fork()/forkFromCurrent() can throw via _performFork → createBranchedSession() or unguarded synchronous _rewriteFile()/writeFileSync. Under Node 22's default unhandledRejection: throw with no handler in packages/coding-agent, this terminates the TUI. Confidence 85, severity high.

Finding 2 — skipConversationRestore branch in shared _performFork has zero test coverage. agent-session.ts (~L3694-3729). No test sets skipConversationRestore: true and asserts the restore is skipped, for either fork path. Confidence 92, severity high (test gap).

Suggestions

Finding 3 — Dashboard finishFork cancelled-path untested for the regular (history-message) fork flow. session.tsx (~L1249-1275). The refactor changed selectForkMessage's cancel behavior (now keeps modal open, informs), but only the forkFromCurrentState cancel path is tested. Confidence 88, severity medium (test gap).

Finding 4 — Awkward as cast in the TUI fork callback. interactive-mode.ts (~L4327). Suggests "selectedText" in result narrowing. Caveat: that form is rejected by this repo's type checker (tsgo) with TS2345; the cast was the deliberate workaround. Confidence 92, severity low.

Finding 5 — Redundant per-row re-filtering. user-message-selector.ts (L52, L80-81). Per-row slice(0,i+1).filter(...); a running counter would avoid it. Low-impact (≤10 rows). Confidence 82, severity low.

Strengths

• All six issue acceptance criteria are met with tested implementation across core, RPC, TUI, and dashboard.
• _performFork preserves fork()'s exact event ordering and return shape.
• session_fork emission/non-emission on veto is now explicitly asserted.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[Hrovatin]

Review Assessment

Assessed against the review comment: #440 (comment)

Each finding was verified by reading the actual code (and, where relevant, running the repo's real type checker) and run through two gates — factual (is it a real problem in current code?) and scope (must it be fixed to deliver issue #439 safely?). A finding is genuine only if both pass. Regressions/safety failures introduced by the PR stay eligible; pre-existing or merely-relocated shared-code concerns are deferred.

Classifications

Finding	Classification	Reasoning
Finding 1 — TUI fork callback has no try/catch; a throw can crash the interactive session	deferred	Factual: Accurate. onSelect(selected.id) in user-message-selector.ts is fire-and-forget (not awaited/caught), the async onSelect in interactive-mode.ts has no try/catch, and there is no unhandledRejection handler in packages/coding-agent; _performFork → createBranchedSession/_rewriteFile/writeFileSync can throw. Scope: This gap is pre-existing and identical for the existing fork() path — verified at c1c3b63~1 the same un-caught async callback and fire-and-forget call site existed before this PR. forkFromCurrent() reuses the same machinery and fetches getLeafId() immediately before branching, adding no new structural crash path. Unchanged shared-code hardening; carries over prior deferred finding A. AC4 ("works consistently") is about feature parity, not adding crash guards absent from the baseline.
Finding 2 — skipConversationRestore branch in _performFork untested	deferred	Factual: Accurate — zero skipConversationRestore references in any test. Scope: This logic existed verbatim at c1c3b63~1 and was merely relocated into _performFork by the dedup refactor — behavior-preserving movement of pre-existing untested code, not new behavior. Test gaps for relocated pre-existing behavior are out of scope; carries over prior deferred finding F.
Finding 3 — Dashboard message-fork cancelled path untested	genuine	Factual: Accurate. At c1c3b63~1, selectForkMessage on cancel always hydrated/refreshed/closed (silent close + session churn); the bfe37a0 refactor routes it through shared finishFork, which on cancel now sets forkError and returns early (modal stays open, no churn) — a real behavior change. Only the forkCurrent cancelled path is tested; the api.fork (message) cancelled path has none. Scope: The cancel-inform behavior is authorized #439 work (prior genuine finding D, applied to both flows via the shared helper), and AC6 requires coverage of behavior this PR changed. Both gates pass. Low priority — regression risk is mitigated because finishFork is shared and already exercised via the forkCurrent cancel test.
Finding 4 — Replace the as cast with "selectedText" in result narrowing	false-positive	Factual: The assessor replaced the cast and ran the repo's actual checker: tsgo -p packages/coding-agent/tsconfig.build.json → interactive-mode.ts(4341,26): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'. The in-narrowing does not compile under tsgo; the simplifier validated only against tsc --strict, missing the repo's toolchain. The existing cast is the deliberate, correct workaround. Fails the factual gate.
Finding 5 — Redundant per-row re-filtering in the selector	nitpick	Factual: Accurate — totalRealMessages is computed once but position re-filters slice(0, i+1) per row; introduced by this PR. Scope: Behavior-preserving micro-optimization over at most maxVisible = 10 rendered rows — no correctness, safety, or requirement impact.

Counts

• Genuine: 1 (Finding 3)
• Nitpick: 1 (Finding 5)
• False positive: 1 (Finding 4)
• Deferred: 2 (Findings 1, 2)

Action Plan

1. Finding 3 (low priority) — Add a dashboard test asserting the message-fork cancelled path (api.fork → { cancelled: true } via selectForkMessage) surfaces forkError, keeps the fork modal open, and does not hydrate/refresh — mirroring the existing forkCurrent-cancelled test. Covers the cancel-inform behavior change this PR introduced for the message-fork flow (AC6).

Deferred (optional follow-up — no new tracking issue needed)

Findings 1 and 2 are factually valid but pre-existing/relocated shared-code concerns, already carried by prior deferred findings A and F. No additional tracking issue is warranted beyond what the prior review noted. Findings 4 and 5 require no action.

---

Assessment by mach6

[Hrovatin]

Progress Update

Addressed the sole genuine finding from the latest review assessment (finding 3). The other findings were correctly left unactioned per the assessment: findings 1 and 2 are deferred (pre-existing / merely-relocated shared code), finding 4 is a false positive (its in-narrowing suggestion is rejected by tsgo), and finding 5 is a nitpick.

Architecture

Test-only change in the dashboard client test suite. packages/dashboard/test/client/screens.test.tsx gains one test — "fork modal informs the user and stays open when a message fork is cancelled" — placed alongside the existing fork-modal tests. It mocks api.fork to resolve { text: "", cancelled: true }, opens the fork modal, clicks a .fork-message row, and asserts the shared finishFork helper's cancel behavior for the history-message flow: the modal stays open, .pair-error shows the cancel message, the composer is not pre-filled, and neither hydrateSession nor refreshDiskSessions is called. This closes the coverage gap where only the forkFromCurrent cancel path was tested even though the bfe37a0 refactor changed selectForkMessage's cancel behavior (previously it silently closed the modal; now it informs and stays open — AC6). No production code changed.

Modified files

• packages/dashboard/test/client/screens.test.tsx — add message-fork cancelled-path test (finding 3).

Verification

• biome clean; full npm run build green.
• Pre-commit hook full suite (bash test.sh --no-live-api): 5561 passed, 0 failed, 710 skipped (+1 over the prior 5560).
• Mutation-tested: temporarily bypassing the finishFork cancel guard makes the new test fail, confirming it asserts real behavior rather than echoing mocks. The guard and session.tsx were restored; only the test file is committed.

Known limitations

Deferred findings remain optional follow-up, out of scope for issue #439: finding 1 (TUI fork-callback throw-safety — a pre-existing gap identical on the existing fork() path) and finding 2 (skipConversationRestore test gap — behavior merely relocated by the dedup refactor). Both are carried by the prior review's deferred findings A and F; no new tracking issue was deemed necessary.

Commit: e5f7eb7

---

Progress tracked by mach6

[Hrovatin]

@aebrer Can you have a look?

[aebrer]

This is a good idea, but I do think the other approach we discussed in the issue is a better short and long term solution. Probably I'll try for that.

[Hrovatin]

Revised Implementation Plan — fork from any transcript position

This supersedes the earlier plan comment. Per the maintainer decision in the issue discussion ("Is 'fork from current state' a better solution than just allowing the fork process to select user OR assistant messages? ... let's go with that option") and the PR comment, we are pivoting away from the dedicated "fork from current state" surface toward forking at any message in the transcript. Forking at the last assistant message reproduces the "current state" behavior, so this strictly subsumes what was built.

New behavior

For U1, A1, U2, A2, U3, A3, the /fork selector offers every message — U1, A1, U2, A2, U3, A3 — with role-dependent semantics that match user intent:

• Fork at an assistant message A_k → the new branch includes everything up to and including A_k (branch anchored at A_k itself); empty editor. "Continue from this answer." Forking at the last assistant A3 is exactly the old "fork from current state."
• Fork at a user message U_k → unchanged existing behavior: rewind to before U_k (branch from its parent), pre-fill the editor with U_k's text. "Edit / re-ask this question."

This keeps both useful affordances (continue-from-answer vs. edit-a-question), makes every node a fork point, and lets us delete the redundant dedicated machinery added earlier in this PR.

Key design decision

fork(entryId) becomes role-aware rather than adding a parallel method. The anchor is entryId (inclusive) for assistant entries and parentId (exclusive) for user entries; selectedText is the message text for user entries and empty for assistant entries. The shared _performFork(entryId, branchStrategy) helper is retained.

Removals (redundant now that the last assistant is directly selectable)

• Core: forkFromCurrent().
• RPC: fork_current request/response (rpc-types.ts), its handler (rpc-mode.ts), and RpcClient.forkCurrent() (rpc-client.ts).
• TUI: the FORK_FROM_CURRENT_ID sentinel + action row (user-message-selector.ts), and the isCurrent branch in the selector callback (interactive-mode.ts).
• Dashboard: POST /api/runtimes/:key/fork-current (server.ts), api.forkCurrent() (api.ts), the .fork-current-btn button + forkFromCurrentState() handler (session.tsx), and .fork-current-btn CSS (app.css).

Changes by layer

Core (agent-session.ts)

• Broaden fork(entryId) to accept role === "user" || role === "assistant". Compute { anchor, selectedText } by role: user → { parentId (or newSession if root), text }; assistant → { entryId, "" }. Route through _performFork.
• Replace getUserMessagesForForking() with getForkableMessages(): Array<{ entryId; text; role }> returning both roles. For assistant entries, extract renderable text; entries with no renderable text (pure tool-call turns) fall back to a generic label (e.g. (assistant response)) — see open question.

RPC

• rpc-types.ts: drop fork_current; extend get_fork_messages response items to { entryId; text; role }; fork unchanged in shape.
• rpc-mode.ts: drop the fork_current case; get_fork_messages calls getForkableMessages().
• rpc-client.ts: drop forkCurrent(); getForkMessages() carries role.

TUI (interactive-mode.ts, user-message-selector.ts)

• Selector lists all messages with a role indicator; remove the action-row concept and gating. Selecting any entry calls session.fork(entryId) and pre-fills the editor with the returned text (empty for assistant). Collapse the callback back to a single fork() path (removes the isCurrent ternary and the as cast from finding 4). Update the "Message N of M" affordance to account for both roles.

Dashboard (server.ts, api.ts, session.tsx, app.css)

• Fork modal lists all messages, labeled by role; remove the fork-current button. Each row → selectForkMessage(entryId) through the shared finishFork helper. Guard finishFork to only setComposerText when the returned text is non-empty (so forking at an assistant doesn't clear a composer the user has typed into). Keep the cancel-inform behavior.

Tests (rework, not just add)

• agent-session-fork-current.test.ts → agent-session-fork.test.ts: fork-at-assistant (tail is the selected assistant; branch includes all prior; empty selectedText), fork-at-last-assistant (== full transcript), fork-at-user (rewind + prefill unchanged), session_fork emission/veto (retain), root/no-parent edge cases.
• interactive-mode-fork.test.ts: drop FORK_FROM_CURRENT_ID gating; assert all messages listed with roles, routing by entry, and editor pre-fill per role.
• rpc-fork-current.test.ts → rpc-fork.test.ts: fork-at-assistant via RPC; get_fork_messages returns roles; remove fork_current.
• session-manager/tree-traversal.test.ts: keep the persisted branch-from-entry (inclusive) coverage.
• Dashboard screens.test.tsx: remove fork-current button tests; add assistant-message fork (no prefill), user-message fork (prefill), and both cancelled paths via the message list.
• Dashboard server.test.ts / runtime-pool.test.ts: remove fork-current endpoint coverage.
• Retain the test.sh git-env unset (unrelated test-infra hardening).

Prior review findings, under the new design

• Finding 1 (TUI callback throw-safety): still worth addressing — wrap the (now single-path) fork() callback in try/catch → done() + status. Optional but cheap; carry as before.
• Finding 2 (skipConversationRestore untested): unchanged shared code; can gain coverage opportunistically in the reworked fork tests.
• Finding 3 (dashboard cancel path): the new message-list cancel tests cover this by construction.
• Findings 4 & 5: dissolved — the as cast and the action-row/position code are removed or rewritten.

Open questions

1. User-message semantics: keep rewind + editor pre-fill (recommended, preserves "edit this question"), or make all forks uniformly inclusive (drops pre-fill, changes long-standing behavior)? Plan assumes the former.
2. Assistant entries with no renderable text (pure tool-call turns): offer with a generic label, or hide them? Plan assumes offer-with-label.
3. Naming/diff size: rename getUserMessagesForForking → getForkableMessages and possibly the selector component for clarity, vs. minimize churn. Plan assumes the semantic rename, keeping the RPC fork command name.

Acceptance criteria (revised)

• Fork can be initiated at any user OR assistant message, discoverable in the /fork UI (role-labeled) across TUI and dashboard.
• Assistant fork includes the selected message (tail = that assistant); forking at the last assistant reproduces "fork from current state."
• User fork retains rewind + editor pre-fill.
• RPC fork/get_fork_messages carry role; the dedicated fork_current surface is removed.
• Session tree parenting correct; session_before_fork/session_fork fire as today.
• Full suite green (npm run build + test.sh --no-live-api).

---

Plan by mach6

[Hrovatin]

Progress Update

Implemented the maintainer-approved pivot: fork from any transcript message (user or assistant), replacing the dedicated "fork from current state" surface. Forking at the last assistant message reproduces the old behavior, so the parallel machinery was removed.

Architecture

The change is anchored in core/agent-session.ts. fork(entryId) is now role-aware: it looks up the entry, and for an assistant message branches from that entry inclusively (the reply is kept) with no editor pre-fill, while for a user message it rewinds to the parent (dropping the question) and returns the question's text as re-ask pre-fill. Both paths funnel through the existing shared _performFork(entryId, branch) helper (unchanged event flow: session_before_fork veto → branch → session_fork → conditional replaceMessages, honoring skipConversationRestore). getUserMessagesForForking() is replaced by getForkableMessages(), which walks the branch and emits {entryId, text, role} for every user and assistant message (tool-only assistant turns get a (assistant response) label); _extractUserMessageText is generalized to _extractMessageText.

This one method feeds three UIs. RPC (rpc-types.ts / rpc-mode.ts / rpc-client.ts) drops the fork_current request/response/handler and RpcClient.forkCurrent(); get_fork_messages now carries role. The TUI selector (components/user-message-selector.ts) lists all roles with [You]/[Assistant] badges and per-role hints instead of a synthetic "fork from current" action row; interactive-mode.ts builds items from getForkableMessages() and routes every selection through one fork() call wrapped in try/catch (surfacing errors and the cancelled case via status). The dashboard removes the /fork-current endpoint (server/server.ts), api.forkCurrent() (client/api.ts), and the fork-current button/handler/CSS; the fork modal (client/screens/session.tsx) renders role-labeled rows, and the shared finishFork helper only pre-fills the composer when re-ask text is present so assistant forks don't clobber it.

Modified files

• packages/coding-agent/src/core/agent-session.ts — role-aware fork(); getForkableMessages(); _extractMessageText; removed forkFromCurrent()
• packages/coding-agent/src/modes/rpc/rpc-types.ts — dropped fork_current; role on get_fork_messages
• packages/coding-agent/src/modes/rpc/rpc-mode.ts — removed fork_current handler
• packages/coding-agent/src/modes/rpc/rpc-client.ts — removed forkCurrent(); role on getForkMessages()
• packages/coding-agent/src/modes/interactive/components/user-message-selector.ts — all-roles list, role badges/hints; removed action row
• packages/coding-agent/src/modes/interactive/interactive-mode.ts — build from getForkableMessages(); single try/catch fork callback
• packages/dashboard/src/server/server.ts — removed /fork-current route
• packages/dashboard/src/client/api.ts — removed forkCurrent; role on fork messages
• packages/dashboard/src/client/screens/session.tsx — role-labeled rows; finishFork pre-fills only when re-ask text present; removed fork-from-current button/handler
• packages/dashboard/src/client/styles/app.css — removed .fork-current-btn; shared .fork-entry-id/.fork-role styles
• packages/ai/test/context-overflow.test.ts, packages/ai/test/total-tokens.test.ts — pre-existing hook failure fix: google catalog dropped gemini-2.0-flash; updated live-API tests to gemini-2.5-flash
• test reworks: agent-session-branching.test.ts, interactive-mode-fork.test.ts, dashboard screens.test.tsx / server.test.ts / runtime-pool.test.ts

New files

• packages/coding-agent/test/agent-session-fork.test.ts — role-aware fork coverage (fork-at-assistant incl. last, fork-at-user rewind+prefill, invalid entry, session_before_fork veto, skipConversationRestore, session_fork emission)
• packages/coding-agent/test/rpc-fork.test.ts — fork() + role-carrying getForkMessages()

Removed files

• packages/coding-agent/test/agent-session-fork-current.test.ts, packages/coding-agent/test/rpc-fork-current.test.ts — obsoleted by the role-aware design

Verification

biome clean; full npm run build green; test.sh --no-live-api → 5564 passed, 0 failed (pre-commit hook ran the full suite on commit).

Commit: 2b8fff1

---

Progress tracked by mach6

[Hrovatin]

Code Review

Five agents reviewed the role-aware fork implementation at 2b8fff1 (code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier — simplifier folded into code-reviewer's pass). The two core branching behaviors (assistant-fork keeps the reply; user-fork rewinds to before the question) are correctly implemented and solidly asserted. Findings below cluster around three themes: (a) forking at broken/non-terminal assistant entries, (b) the composer/editor no-clobber guard, and (c) test/doc coverage of the new listing logic.

Important

Finding 1 — TUI assistant-fork unconditionally clears the editor, wiping any in-progress draft (interactive-mode.ts:4328, confidence 88)
The success path calls this.editor.setText(result.selectedText) unconditionally; for assistant forks selectedText === "", so setText("") wipes whatever the user had typed. The dashboard added an explicit guard for exactly this (session.tsx:1262, if (result.text) setComposerText(...)), and the sibling showTreeSelector() callback in the same file already uses the correct pattern (if (result.editorText && !this.editor.getText().trim())). Not reachable via the two built-in /fork entry points (both guarantee an empty editor), but the app.session.fork keybinding is user-configurable and can be triggered mid-draft. The same unguarded pattern exists in the pre-existing extension-facing commandContextActions.fork handler (interactive-mode.ts:1326), which now inherits the clobber because this PR widened fork() to return "" for assistant entries.

Finding 2 — Dashboard composer no-clobber guard is asserted by a test that passes with or without the guard (screens.test.tsx:5657, confidence 96)
The "forks at an assistant message without prefilling" test asserts the textarea is "" afterward, but the composer starts empty in that test, so the assertion holds whether or not the guard exists. Mutation-verified: removing the guard (setComposerText(result.text ?? "")) leaves all 3 fork tests green. The one behavior the code comment promises to protect (don't wipe a draft) has zero coverage. Fix: type a draft into the composer before selecting the assistant message, then assert it survives.

Finding 3 — Forking at an aborted/errored assistant reply silently drops it from all future requests (agent-session.ts:3640,3971, confidence 88)
getForkableMessages() lists every assistant entry, including ones with stopReason: "error"/"aborted", with the same generic "(assistant response)" label. transformMessages() (used by every provider) unconditionally skips such messages, so the forked-at reply vanishes from context on the next turn — defeating "continue from this answer" with no warning, and potentially triggering back-to-back-user 400s on role-strict providers. Verified empirically. Fix: exclude or distinctly label error/aborted assistant entries as fork points.

Finding 4 — Forking at a non-terminal tool-call assistant row discards the real tool result (agent-session.ts:3652,3966; session-manager.ts:1081, confidence 85)
Pure tool-call assistant turns are offered as fork points with the same "(assistant response) · continue from here" copy as a real final answer. getBranch() only walks ancestors, so the toolResult that followed the selected entry is dropped, and transformMessages() substitutes a fabricated "No result provided", isError: true. Any multi-tool-call turn produces several such intermediate rows. Verified empirically. Fix: exclude assistant entries that still have tree children (non-terminal), or label them distinctly.

Suggestions

Finding 5 — getForkableMessages() has no direct offline unit test (agent-session.ts:3971, confidence ~90; raised by both test-reviewer and completeness-checker)
The listing method every UI depends on — including the new "(assistant response)" fallback and the "skip empty user messages" rule — is never invoked by a non-live test: interactive-mode-fork.test.ts mocks it, rpc-fork.test.ts uses a hand-built response, and the only real caller (agent-session-branching.test.ts) is skipIf(DREB_SKIP_LIVE_API) and only checks the user-filtered subset. Mutation-verified: dropping the fallback label leaves all 50 non-skipped tests green. The fork() tests already use an offline createHarnessWithExtensions() + appendMessage() pattern that would trivially cover this.

Finding 6 — Fork failure surfaced via showStatus (dim/info) instead of the showError convention (interactive-mode.ts:4331, confidence 85)
The catch block uses showStatus("Fork failed: ...") (muted, no "Error:" prefix). The analogous showTreeSelector() catch uses showError(...), as do dozens of other exception handlers in the file. Makes a real failure look like a routine status line.

Finding 7 — Selector component render (role badges, hints, width guard) has zero test coverage (user-message-selector.ts:36, confidence 85)
This PR added the [Assistant]/[You] badges, the "continue from here"/"rewind & re-ask" hints, and a Math.max(0, maxMsgWidth) guard — the only UI signal distinguishing two opposite-consequence actions. The sibling CopySelectorComponent has a getMessageList().render(width) test (copy-selector.test.ts); UserMessageSelectorComponent exposes the identical API but no test.

Finding 8 — "throws for non-message/invalid entry id" test only covers the missing-entry clause (agent-session-fork.test.ts:91, confidence 85)
The guard has three clauses (!entry, type !== "message", wrong role); the test only exercises a nonexistent id. Mutation-verified: collapsing the guard to if (!selectedEntry) leaves all 8 tests green.

Finding 9 — Docs not updated for role-aware fork (confidence 92)
No .md touched. Stale/incorrect: packages/coding-agent/README.md:246 (/fork described as rewind-and-re-ask only); docs/rpc.md:897 (fork = "from a previous user message", example always non-empty text); docs/rpc.md:925 (get_fork_messages = "user messages", example omits the new role field); docs/tree.md:12 ("Flat list of user messages"). AGENTS.md mandates doc updates on feature changes.

Finding 10 — Component/method names still say "User" though they now handle both roles (user-message-selector.ts, interactive-mode.ts:4298, confidence 90)
UserMessageItem/UserMessageList/UserMessageSelectorComponent/showUserMessageSelector() retain user-only naming despite dual-role docstrings. Cosmetic; getForkableMessages() was correctly renamed.

Strengths

• Core branching semantics verified against createBranchedSession/getBranch: assistant-fork retains the entry (createBranchedSession(entryId)), user-fork rewinds (createBranchedSession(parentId)), both asserted on message count + tail.
• _performFork handles cancel/veto/skipConversationRestore uniformly for both roles; cancel returns before mutation.
• TUI callback now wraps fork() in try/catch, surfacing cancel and thrown errors and never resetting editor/display on failure (closes a prior-round deferred gap).
• RPC/dashboard role plumbing consistent; exhaustive grep confirms zero dangling forkFromCurrent/fork_current/forkCurrent/FORK_FROM_CURRENT_ID/fork-current/getUserMessagesForForking references.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[Hrovatin]

Review Assessment

Assessment of the review findings, verified against the actual code at 2b8fff1. Authoritative scope = the revised plan (fork at any transcript position), which supersedes the original issue. The git diff 5a23988..HEAD confirms assistant fork points are entirely new (old getUserMessagesForForking() filtered to role === "user"), so correctness failures reachable only via assistant forks are regressions introduced by this PR, not pre-existing.

Classifications

Finding	Classification	Reasoning
3: Forking at error/aborted assistant silently drops the reply	genuine	Factual: Assistant entries persist regardless of stopReason (agent-session.ts L1096); getForkableMessages() lists them with no filter; transformMessages() unconditionally continues on stopReason error/aborted (transform-messages.ts L153), so the forked-at reply vanishes next turn (+ back-to-back-user risk on strict providers). Scope: Correctness failure newly reachable via this PR's assistant-fork; plan never considered these entries. A fork must not silently produce a different branch than shown.
4: Forking at a non-terminal tool-call assistant fabricates a result	genuine	Factual: Each message is a separate SessionMessageEntry chained by parentId; getBranch() walks only ancestors, dropping the child toolResult; transformMessages() then injects "No result provided", isError: true (L133-142). These rows carry the same "(assistant response) · continue from here" affordance. Scope: Correctness failure from the new capability. Plan open-q2 only sanctioned labeling pure-tool-call turns, not fabricating a tool failure. NB: the reviewer's "exclude entries with tree children" fix is over-broad (it would break the approved earlier-answer fork); correct filter is assistant entries with unresolved toolCall blocks + error/aborted stopReason.
5: getForkableMessages() has no offline unit test	genuine	Factual: Mocked in interactive-mode-fork.test.ts; hand-built response in rpc-fork.test.ts; the only real caller (agent-session-branching.test.ts L83/111/143) is skipIf(DREB_SKIP_LIVE_API) and filters to user role. Assistant-listing, "(assistant response)" fallback, and skip-empty-user are never exercised offline. Scope: Core new logic every UI depends on; AC6 applies. A proper offline test also forces the findings 3/4 decisions.
9: Docs not updated for role-aware fork	genuine	Factual: git diff --stat -- '*.md' is empty. README.md:246, rpc.md:897 ("from a previous user message"), rpc.md:926 ("Get user messages…", example omits the new role field), tree.md:12 ("Flat list of user messages") all describe the removed user-only behavior; the get_fork_messages RPC contract now returns role but is undocumented. Scope: Feature + RPC contract changed; AGENTS.md mandates doc updates. Integrator-facing docs are now actively wrong.
2: Dashboard no-clobber guard test is insensitive	genuine	Factual: The assistant-fork test mocks api.fork → {text:""}, composer starts empty, asserts textarea.value === ""; removing the guard still yields "", so it can't catch guard removal. The behavior the guard protects (don't wipe a typed draft) is unexercised. Scope: Guard is new code the plan explicitly required; AC6 requires effective coverage. Low priority.
7: Selector render (badges/hints/width guard) untested	genuine	Factual: The [Assistant]/[You] badges, hints, Math.max(0, maxMsgWidth) guard, and header are all new; UserMessageSelectorComponent is only ever vi.mocked, never rendered. Sibling CopySelectorComponent has a getMessageList().render() test and the identical API exists. Scope: New UI code; the badge is the only signal distinguishing two opposite-consequence actions. AC6 applies. Low priority.
1: TUI clears editor on assistant fork	deferred	Factual: True — editor.setText(result.selectedText) (L4328) runs unconditionally; "" for assistant forks wipes a draft. Dashboard and navigateTree both guard. Scope: Both built-in entry points are safe (double-Escape only fires when editor empty, L2292; /fork calls setText("") after). Only the non-default app.session.fork keybinding (defaultKeys: []) is exposed, and the plan specified this exact setText. Narrow opt-in edge; not required for approved scope.
8: "throws for invalid id" test only covers missing-entry clause	deferred	Factual: True — the 3-clause guard is only exercised via a nonexistent id; collapsing to if (!selectedEntry) survives. Scope: The PR-relevant change (accepting assistant entries) is directly covered; the uncovered clauses reject entries no UI ever surfaces. Defensive/pre-existing style.
6: Fork failure uses showStatus not showError	nitpick	Factual: True — catch uses showStatus while showTreeSelector uses showError. Scope: Error is still surfaced; only styling/prefix differs. No correctness/AC impact.
10: Component names still say "User"	nitpick	Factual: True — UserMessage*/showUserMessageSelector retain user-only names. Scope: Cosmetic; docstrings updated; plan open-q3 left the rename discretionary.

Counts: 6 genuine, 2 deferred, 2 nitpicks, 0 false-positives.

Action Plan

1. Correctness — stop offering assistant entries that produce broken branches (findings 4 + 3). In getForkableMessages() / the fork() assistant path, exclude (or correctly handle) assistant entries that (a) contain unresolved toolCall blocks (forking drops the descendant toolResult → fabricated isError result), and (b) have stopReason error/aborted (silently skipped by transformMessages → reply vanishes). Filter on toolCall presence + error/aborted stopReason — not "has tree children" (that would break the approved earlier-answer fork).
2. Coverage — add an offline unit test for getForkableMessages() (finding 5) via createHarnessWithExtensions() + appendMessage (assistant/user/tool-call/aborted); assert the list directly, including the item 1 exclusion decisions.
3. Docs — update for role-aware fork (finding 9): README.md:246, docs/rpc.md:897 & :926 (prose + add role to the get_fork_messages example), docs/tree.md:12; keep the root README in sync per AGENTS.md.
4. Coverage (low) — make the dashboard assistant-fork test mutation-sensitive (finding 2): pre-type a draft, assert it survives.
5. Coverage (low) — add a UserMessageSelectorComponent render test (finding 7) mirroring copy-selector.test.ts.

Deferred (optional, non-blocking): finding 1 (guard the TUI editor for consistency; only the non-default keybinding is exposed), finding 8 (test the reject clauses).
Nitpicks (skip): finding 6, finding 10.

---

Assessment by mach6

[Hrovatin]

Progress Update

Fixed the 6 genuine findings from the review assessment (findings 2, 3, 4, 5, 7, 9). Deferred findings (1, 8) and nitpicks (6, 10) were intentionally left as-is per the assessment.

Architecture

The correctness fix (findings 3 + 4) is centered on a new private guard in core/agent-session.ts, _isForkableAssistant(message), which returns false for assistant turns that cannot be safely branched from: those with stopReason "error"/"aborted" (silently dropped by @dreb/ai's transformMessages() before every request, so the reply would vanish) and those containing any toolCall block (their tool results are descendant entries a branch — which walks ancestors only via SessionManager.getBranch's parentId chain — cannot include, so transformMessages() would fabricate a "No result provided", isError result). This single predicate is the shared gate used by both getForkableMessages() (such turns are excluded from the fork list) and fork() (a direct RPC/dashboard call on one now throws a descriptive error instead of producing a misleading branch). The deliberately narrow filter (toolCall presence + error/aborted stopReason) preserves the approved earlier-answer fork, which a broader "has tree children" heuristic would have broken. The remaining changes are non-behavioral: offline tests that drive the real getForkableMessages()/fork() via createHarnessWithExtensions() + appendMessage(), a mutation-sensitised dashboard composer test, a new offline render test for the selector component, and doc corrections.

New files

• packages/coding-agent/test/user-message-selector.test.ts — offline render tests for UserMessageSelectorComponent (finding 7): [Assistant]/[You] badges, "continue from here"/"rewind & re-ask" hints, bottom-anchored selection, Escape-cancel, narrow-width clamp (no negative truncation), and empty-state — mirroring the existing copy-selector.test.ts convention.

Modified files

• packages/coding-agent/src/core/agent-session.ts — added _isForkableAssistant(); getForkableMessages() excludes unsafe assistant turns; fork() rejects a direct call on one (findings 3 + 4)
• packages/coding-agent/test/agent-session-fork.test.ts — new offline getForkableMessages suite (finding 5): role/text listing, empty-user skip, "(assistant response)" fallback, error/aborted exclusion, tool-call exclusion, and fork() rejection of both invalid kinds
• packages/dashboard/test/client/screens.test.tsx — the assistant-fork test now pre-types a composer draft and asserts it survives the fork (finding 2); mutation-verified that removing the if (result.text) guard fails it
• packages/coding-agent/README.md — /fork described as role-aware (finding 9)
• packages/coding-agent/docs/rpc.md — fork and get_fork_messages prose + the new role field + an assistant-fork response example + the invalid-target note (finding 9)
• packages/coding-agent/docs/tree.md — /fork vs /tree comparison row updated to "user and assistant messages" (finding 9)

Verification

biome clean; full npm run build green; test.sh --no-live-api → 5575 passed, 0 failed (+11 tests). The dashboard no-clobber guard and the assistant-exclusion logic were both mutation-tested (breaking each makes the new tests fail).

Commit: 7c1a14b

---

Progress tracked by mach6

[Hrovatin]

Code Review — round 4 (fix commit 7c1a14b)

Scope: the commit that fixes the 6 genuine findings from the prior assessment (findings 2, 3, 4, 5, 7, 9). Agents read the actual changed files plus surrounding code. Confidence-scored (≥80 reported).

Important

Finding 1 — Stale docstring contradicts the new exclusion behavior (code-reviewer, conf 92)
packages/coding-agent/src/core/agent-session.ts (~L3973-3981). The getForkableMessages() docblock still says "Assistant turns with no renderable text (pure tool-call turns) still appear as fork points, with a generic label," immediately above the new paragraph stating interrupted / tool-waiting turns are excluded. These are mutually exclusive: _isForkableAssistant()'s content.some(c => c.type === "toolCall") now filters pure-tool-call turns out entirely. The commit edited this exact docblock without reconciling the older sentence. Fix: rewrite to "Assistant turns with no renderable text but no excluded content (e.g. thinking-only turns) still appear … with a generic label."

Finding 3 — toolCall-exclusion tests don't actually pin .some() semantics (test-reviewer, conf 90)
packages/coding-agent/test/agent-session-fork.test.ts (excludes assistant turns with unresolved tool calls, fork() rejects … tool-call assistant entry). toolCallAssistant()'s content is a single homogeneous toolCall element, so .some() and .every() are indistinguishable there — mutating .some→.every left both toolCall tests passing (caught only incidentally by the unrelated empty-content test). Real assistant turns commonly mix text + toolCall in one content array; that shape isn't tested. Fix: add a fixture with content: [{type:"text",…},{type:"toolCall",…}] and assert it's still excluded.

Finding 5 — role/hint and role/badge pairing isn't verified (test-reviewer, conf 90)
packages/coding-agent/test/user-message-selector.test.ts (renders role badges and role-specific fork hints…). Assertions only check each string appears somewhere in the joined blob, not that it's attached to the correct role/line. Swapping the hint mapping (and separately the badge mapping) in the component left all 5 tests green. A real mislabel (e.g. "rewind & re-ask" on an assistant row) would slip through. Fix: tie [Assistant]↔continue from here (and [You]↔rewind & re-ask) to the same rendered line.

Suggestions

Finding 2 — docs/rpc.md cancelled-fork example wrong for the user-fork-cancel case (code-reviewer, conf 88)
packages/coding-agent/docs/rpc.md (~L925-932). The commit changed the cancelled-fork data.text from the prompt text to "". That's correct only for an assistant-message fork; a cancelled user-message fork still returns the real message text (selectedText is computed before _performFork and returned regardless of cancelled). No shipped client breaks (both finishFork paths ignore text on cancel), but it misleads direct RPC integrators. Fix: split the cancelled example by role, mirroring the two success examples.

Finding 4 — narrow-width test doesn't verify the clamp it names (test-reviewer, conf 88)
packages/coding-agent/test/user-message-selector.test.ts (does not throw … very narrow terminal). Removing Math.max(0, maxMsgWidth) in the component still passes all 5 tests, because truncateToWidth() has its own internal if (maxWidth <= 0) return "". The "doesn't throw" assertion was never at risk from that line. Fix: assert the rendered message portion is empty at width 10, or spy that truncateToWidth is called with 0.

Finding 6 — Finding 9 (docs) only partially done: stale /fork quick-reference row (completeness-checker, conf 88)
packages/coding-agent/README.md:178 — the Commands quick-reference table still reads | /fork | Create a new session from the current branch |, pre-#439 text with no role-aware framing. The detailed prose at L246, docs/rpc.md, and docs/tree.md are all correctly updated; this one table row was never touched. Fix: update the row to mention forking at any user/assistant message.

Strengths

• _isForkableAssistant() correctly matches the StopReason union and the ToolCall discriminant; getForkableMessages() and fork() call the same predicate, so the offered list and what fork() accepts cannot diverge.
• Error-auditor independently confirmed the fix's premise against transform-messages.ts across all six providers (error/aborted turns dropped; orphaned toolCalls get synthetic "No result provided", isError), verified _isForkableAssistant() cannot throw on a well-formed message (Array.isArray guard), and traced the new fork() throw to a surfaced error at every reachable call site (TUI, RPC, dashboard REST + client, extension ctx) — no findings.
• The dashboard no-clobber guard test was independently mutation-reproduced (removing if (result.text) fails it). The stopReason (error/aborted) exclusion tests are mutation-sensitive.
• New offline getForkableMessages suite and selector render tests exercise real (non-mocked) code paths, including the (assistant response) fallback.

Note (not a scored finding): error-auditor saw a single non-reproducible failure of excludes errored and aborted assistant turns on first run, not reproduced in ~160 subsequent attempts; the code under test is a pure sync function — likely environmental noise on this shared machine.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker (simplifier folded into code-reviewer)

---

Reviewed by mach6

[Hrovatin]

Review Assessment — round 4

Assessment of the round-4 review of fix commit 7c1a14b. Each finding verified against actual code at HEAD 7c1a14b (two gates: factual = real problem in current code; scope = required to deliver the authoritative PR scope safely).

Classifications

Finding	Classification	Reasoning
1 — getForkableMessages docblock contradiction	genuine	Factual: agent-session.ts:3977-3978 says pure tool-call turns "still appear as fork points, with a generic label," but _isForkableAssistant (L4027) returns false for any turn with a toolCall block — a flat contradiction with the code and with the docblock's own next paragraph. The sentence was true pre-#439; the fix commit edited this docblock without reconciling it. Scope: Self-contradictory documentation of the new safety guard, in the very surface the commit changed; AGENTS.md requires docs stay in sync.
2 — rpc.md cancelled-fork data.text	genuine (low)	Factual: git diff confirms the commit changed the cancelled example to text: "", but a cancelled user fork still returns the real message text — selectedText is computed before _performFork (agent-session.ts:3669) and returned regardless of cancelled (L3682), mapped unconditionally by rpc-mode.ts:2117. Example is now inaccurate for the user-cancel case. Scope: Fork RPC doc is in scope; the PR regressed a previously-correct example.
3 — toolCall fixture can't distinguish .some from .every	genuine	Factual: toolCallAssistant()'s content is a single homogeneous toolCall element, so .some/.every are indistinguishable — a .some→.every mutation leaves all exclusion tests green. No mixed [{text},{toolCall}] fixture (the discriminating, and dominant real, shape). Scope: This tool-call exclusion is the fix's core safety guard; the untested mixed case would silently corrupt branch context if it regressed. New PR code ⇒ in scope.
4 — narrow-width clamp has no regression signal	nitpick	Factual: TRUE — truncateToWidth guards if (maxWidth <= 0) return "" (tui/src/utils.ts:863-865), so removing the component's Math.max(0, …) is harmless and the test still passes. Scope: The clamp is redundant defensive code; its removal introduces no bug and the real contract (no crash, produces output) IS tested. Mutation-precision on a provably-harmless line, not a scoped requirement.
5 — role-badge/hint mapping not pinned to a line	genuine	Factual: The component renders badge+preview on line 1 and hint on line 2 per role, but the test only asserts each string appears somewhere in the joined blob; since the sample has both roles, swapping the badge or hint mapping leaves all toContain assertions green. interactive-mode-fork.test.ts only checks the data list via a mock. Scope: Role-labeling in the TUI is an explicit acceptance criterion; a swapped mapping (wrong fork semantics) would ship undetected. New PR UI ⇒ in scope.
6 — README:178 stale /fork quick-reference row	genuine (low)	Factual: git diff confirms the PR updated the prose at README.md:246 but left the quick-reference table at README.md:178 reading "Create a new session from the current branch" — untouched since the pre-#439 flatten, no role-aware framing (contrast the sibling /tree row). Scope: Authoritative scope invokes AGENTS.md's completeness rule ("fix ALL docs"); updating prose + rpc.md + tree.md but skipping this row is exactly the cherry-picking the rule forbids. Low priority but in scope.

Counts: genuine 5 (1, 2, 3, 5, 6) · nitpick 1 (4) · false-positive 0 · deferred 0.

Action Plan

1. Finding 3 — add a mixed content: [{type:"text",…},{type:"toolCall",…}] fixture; assert it's excluded from getForkableMessages() and rejected by fork(). Pins .some (not .every) and covers the dominant real tool-call shape, protecting the core safety guard.
2. Finding 5 — tie role assertions to lines: the "the assistant answer" line contains [Assistant] (not [You]) and is followed by continue from here; the "first question" line contains [You] and is followed by rewind & re-ask.
3. Finding 1 — rewrite the stale docblock sentence at agent-session.ts:3977-3978 to agree with _isForkableAssistant (keep the (assistant response) note for genuinely renderless-but-forkable turns, e.g. thinking-only).
4. Finding 2 — correct the rpc.md cancelled-fork example (~L927-932) to reflect that a cancelled user fork still returns the message text.
5. Finding 6 — update the README.md:178 quick-reference row to role-aware framing consistent with L246.

Deferred: none. Skip: Finding 4 (redundant with truncateToWidth's own guard; real behavior already tested).

---

Assessment by mach6

[Hrovatin]

Progress Update

Fixed the 5 genuine findings from the round-4 assessment (findings 1, 2, 3, 5, 6). Finding 4 (nitpick — the narrow-width clamp is redundant with truncateToWidth's own guard) was intentionally skipped per the assessment.

Architecture

All five fixes are doc/test-quality hardening; no runtime behavior changed. Two are mutation-hardened tests that pin the fix commit's safety guard. In packages/coding-agent/test/agent-session-fork.test.ts a new mixedToolCallAssistant() fixture (a content array with BOTH a text block and a toolCall block — the dominant real tool-call shape) is exercised by two tests asserting it is excluded from getForkableMessages() and rejected by fork(); this distinguishes _isForkableAssistant's .some(isToolCall) from .every(...), which the prior single-element fixture could not. In packages/coding-agent/test/user-message-selector.test.ts the role-label test now locates each message's rendered line and asserts the badge ([Assistant]/[You]) and the following hint line (continue from here/rewind & re-ask) are tied to the correct role, rather than merely appearing somewhere in the render blob. The remaining three are documentation corrections: the getForkableMessages() docblock in core/agent-session.ts no longer claims pure tool-call turns "still appear as fork points" (the fallback label now correctly applies only to renderless-but-forkable turns, e.g. thinking-only); docs/rpc.md's cancelled-fork example now reflects that a cancelled user fork still returns the message text (assistant forks return ""); and the /fork quick-reference row in packages/coding-agent/README.md gains role-aware framing consistent with the detailed prose already updated earlier in this PR.

Modified files

• packages/coding-agent/src/core/agent-session.ts — corrected the stale getForkableMessages() docblock (finding 1)
• packages/coding-agent/test/agent-session-fork.test.ts — added mixedToolCallAssistant() fixture + mixed-shape exclusion/rejection tests (finding 3)
• packages/coding-agent/test/user-message-selector.test.ts — badge/hint assertions now tied to the correct rendered line (finding 5)
• packages/coding-agent/docs/rpc.md — cancelled-fork example corrected for the user-fork case (finding 2)
• packages/coding-agent/README.md — /fork quick-reference row updated to role-aware framing (finding 6)

Verification

biome clean; full npm run build green; test.sh --no-live-api → 5577 passed, 0 failed (+2 tests). Both new safety-critical assertions were mutation-verified: flipping _isForkableAssistant's .some→.every fails the mixed-tool-call test, and swapping either the badge or the hint mapping in the selector fails the role-label test.

Commit: 4c562a5

---

Progress tracked by mach6

[Hrovatin]

Code Review — round 5 (fix commit 4c562a5)

Scope: the commit that fixes the 5 genuine round-4 findings (1, 2, 3, 5, 6). It changed only tests, docs, and one docblock comment — no runtime logic. Ran code-reviewer, test-reviewer, completeness-checker (error-auditor skipped — no error-handling/runtime code touched). Confidence-scored (≥80 reported).

Suggestions

Finding 1 — Cancelled fork at a user message (returns the original text) is undocumented-behavior-now-documented but untested (test-reviewer, conf 90, medium)
packages/coding-agent/test/agent-session-fork.test.ts. This commit's rpc.md change (fixing round-4 finding 2) newly documents a specific distinction: when a session_before_fork handler cancels, data.text still mirrors the successful-fork value — the user message's original text for a user fork, "" for an assistant fork. Verified accurate in agent-session.ts fork() (~L3667-3682): the user-path selectedText is computed via _extractMessageText before _performFork and returned unconditionally. But the suite's only cancellation test ("can be cancelled by a session_before_fork extension handler", ~L99) cancels at an assistant entry, which always returns selectedText: "" — so it cannot distinguish "text preserved" from "text always empty." The newly-documented user-cancel contract is unverified; a regression that zeroed selectedText on user-fork cancel would pass the full suite silently. Suggested test: append userMsg("q2") after an assistant turn, register a cancelling session_before_fork handler, session.fork(q2Id), assert cancelled === true AND selectedText === "q2".

Strengths

• code-reviewer: no findings. Traced fork() control flow and RPC mapping — the rewritten getForkableMessages() docblock, the rpc.md cancelled-fork prose/example, and the README /fork row are all factually accurate and mutually consistent (row matches the L246 prose; rpc.md matches the user-returns-text / assistant-returns-"" behavior).
• completeness-checker: no findings. All 5 findings fully resolved (docblock contradiction gone, mixed-fixture test present, selector test line-tied, rpc.md + README corrected). Searched root README, packages/coding-agent/README.md, and all docs/*.md for other stale user-only fork claims — none remain (categorical-completeness satisfied). Finding 4 correctly left as an out-of-scope nitpick.
• test-reviewer independently mutation-verified the two hardened tests: .some→.every in _isForkableAssistant fails both new mixed-toolCall tests; swapping either the badge or the hint mapping fails the role-line test. The findIndex/line+1 layout assumption matches the component's fixed 3-lines-per-message render and is not brittle. The mixedToolCallAssistant() fixture shape matches AssistantMessage/TextContent/ToolCall exactly.

Agents run: code-reviewer, test-reviewer, completeness-checker

---

Reviewed by mach6

[Hrovatin]

Review Assessment — round 5

Assessment of the round-5 review of fix commit 4c562a5. The single finding was verified against actual code at HEAD 4c562a5 (two gates: factual = real problem in current code; scope = required to deliver the authoritative PR scope safely).

Classifications

Finding	Classification	Reasoning
1 — cancelled user-fork returns original text is untested	deferred	Factual (passes): Confirmed against the worktree. fork() computes selectedText via _extractMessageText (agent-session.ts:3670) for the user path and returns {selectedText, cancelled} unconditionally (L3683), so a cancelled user fork returns the real text. Both cancellation tests fork at an assistant entry (always selectedText:""), and the only user-path selectedText assertion is on the non-cancelled path — so no test pins the cancelled-user-fork-returns-text contract. The gap is real. Scope (fails): This behavior is pre-existing, not introduced by this PR — the pre-#439 base fork() already returned {selectedText, cancelled:true} on cancel and only handled user messages; #439 preserved that semantics. The commit under review changed only tests/docs/a docblock, and its rpc.md edit merely documents the behavior. The authoritative acceptance criteria (user-fork rewind+prefill; session_before_fork firing "as today") are already covered on the success and cancel paths; none specifically requires asserting the cancelled-path return value for a user fork. Optional hardening of a pre-existing, PR-documented behavior — out of scope.

Counts: genuine 0 · nitpick 0 · false-positive 0 · deferred 1.

Action Plan

No genuine issues. The PR is clean and ready to merge.

The one deferred item (a one-line assertion for the pre-existing cancelled-user-fork return value) is low-value optional follow-up; it does not block merge and does not warrant a dedicated tracking issue.

---

Assessment by mach6

[Hrovatin]

@aebrer I made the changes, but still need to test manually

[Hrovatin]

@aebrer I tested manually and it looked good to me - please also try yourself

[m-aebrer]

Unverified Review Candidates — Pending Assessment

Review round: 6
Reviewed commit: db04a87

These are unverified candidates. Severity reflects reviewer confidence; do not treat any item as a merge blocker until the assessment comment is posted.

Critical

None.

Important

None.

Suggestions

Finding 1 — Single-use finishFork abstraction retains machinery for a removed second flow (packages/dashboard/src/client/screens/session.tsx:1287-1311, confidence 82)

finishFork(action, cancelMessage) is now called only by selectForkMessage(). Its generic action closure, configurable cancellation message, and comment about “both fork flows” were introduced when the dashboard also had a dedicated fork-current path; that path was removed when role-aware message forking superseded it. Inlining the logic into selectForkMessage(entryId) would preserve cancellation, composer no-clobber, hydration, refresh, modal-close, and error behavior while removing obsolete indirection and an inaccurate comment.

Strengths

• Core role-aware semantics are clean: user-message forks retain rewind-and-prefill behavior, while assistant-message forks branch inclusively.
• _isForkableAssistant() consistently prevents both listed and direct forks from producing vanished errored/aborted replies or orphaned tool-call results.
• RPC, TUI, and dashboard role propagation and error surfacing are coherent; dashboard assistant forks preserve an existing composer draft.
• Specialist verification found complete acceptance-criteria coverage and strong focused tests, including mixed text/tool-call turns, extension cancellation/events, role-linked selector hints, and both dashboard fork roles.
• The latest master merge introduced no conflicts or review-visible regression in the full PR diff.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[m-aebrer]

Review Assessment

#440 (comment)

Classifications

Finding	Classification	Reasoning
Finding 1 — Single-use finishFork abstraction	nitpick	Factual: finishFork has one call site and retains generic parameters originally useful to the removed dedicated fork-current path. However, “both fork flows” can still reasonably describe the current user-rewind and assistant-continue semantics, and single-use action helpers are common in this component. Scope: The helper became single-use through this PR's approved pivot, so the observation concerns changed code, but no acceptance criterion requires a particular helper shape. Practical: No supported user can trigger harm: both roles traverse the same tested completion path, and the existing tests pin user prefill, assistant draft preservation, cancellation, and session refresh. Inlining changes no observable behavior and offers only marginal reading-cost reduction. The independent assessor considered it a useful low-severity cleanup, but the developer's advocate found no material practical impact; there is no concrete harmful trigger-and-outcome sequence, so it is not a merge blocker.

Action Plan

No merge blockers. The PR is ready for publication.

The prior round's deferred cancelled-user-fork selectedText assertion remains optional test hardening and does not affect this PR's behavior or acceptance evidence.

---

Assessment by mach6