Skip to content

Add option to fork from current state (include last model response) - #440

Open
Hrovatin wants to merge 4 commits into
aebrer:masterfrom
Hrovatin:feature/issue-439-fork-from-current-state
Open

Add option to fork from current state (include last model response)#440
Hrovatin wants to merge 4 commits into
aebrer:masterfrom
Hrovatin:feature/issue-439-fork-from-current-state

Conversation

@Hrovatin

@Hrovatin Hrovatin commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Closes #439

Adds a "fork from current state" option that branches from the current leaf — including the last model response — as a complement to the existing rewind-to-user-message fork.

Implementation plan posted as a comment below.

@Hrovatin

Hrovatin commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

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-currenth.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.
  • Dashboardtest/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 sessionforkFromCurrent() 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

Hrovatin commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

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.tsRpcClient.forkCurrent() wire-format tests.

Modified files

  • packages/coding-agent/src/core/agent-session.tsforkFromCurrent() + shared _performFork() helper.
  • packages/coding-agent/src/modes/rpc/rpc-types.tsfork_current command + response types.
  • packages/coding-agent/src/modes/rpc/rpc-mode.tsfork_current handler.
  • packages/coding-agent/src/modes/rpc/rpc-client.tsforkCurrent() 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.tsPOST /api/runtimes/:key/fork-current.
  • packages/dashboard/src/client/api.tsforkCurrent().
  • 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
Hrovatin marked this pull request as ready for review August 9, 2026 08:01
@Hrovatin

Hrovatin commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

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

Hrovatin commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

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_IDforkFromCurrent() 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

Hrovatin commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

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_IDforkFromCurrent() 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

Hrovatin commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

@/tmp/gh-comment.XXXXXX.md

@Hrovatin

Hrovatin commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

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; _performForkcreateBranchedSession/_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.jsoninteractive-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

Hrovatin commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

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

Hrovatin commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

@aebrer Can you have a look?

@aebrer

aebrer commented Aug 10, 2026

Copy link
Copy Markdown
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add option to fork from current state (include last model response)

2 participants