Conversation
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#ykxh
Kata: shinychat#bfq8
Kata: shinychat#bfq8
Kata: shinychat#bfq8
Kata: shinychat#bfq8
Kata: shinychat#bfq8
Kata: shinychat#bfq8
Kata: shinychat#bfq8
Kata: shinychat#xt5q
Kata: shinychat#xt5q
Kata: shinychat#xt5q
Kata: shinychat#xt5q
Kata: shinychat#xt5q
Kata: shinychat#xt5q
Kata: shinychat#redv
Refs: shinychat#redv
Refs: shinychat#redv
Refs: shinychat#vqzf
Refs: shinychat#vqzf
Refs: shinychat#vqzf
Update the legacy HistoryController test double for the current transcript and destructive-mutation boundary. Keep the initial v2 readiness test focused on admission before and after the authoritative decision.\n\nRefs: shinychat#vqzf
Patch the shared os module in the atomic-write failure test instead of reaching through a private module export.\n\nRefs: shinychat#vqzf
Wait for the history drawer to publish both saved conversations before selecting a branch, and for initial input admission before submitting in the navigation example.\n\nRefs: shinychat#73pm
There was a problem hiding this comment.
First, the important part: this is the strongest design the package has had
for its hardest problem. The exchange tree plus server-authoritative
transcript deletes the echo, the ui_offset arithmetic, the positional
alignment heuristic, and the chat_restore()/history duality in one stroke —
and turns bookmarks into branch pointers, which the old model couldn't even
express. None of the comments below dispute the direction; they're about
where complexity has concentrated, a few behavioral edges worth sharpening,
and one process ask — so keep going. Everything below is a recommendation —
some strongly encouraged, none a merge condition.
The process ask: land this as multiple PRs, along the cut lines the plan
itself already defines. At 100 commits / ~29k insertions across 146
files, this branch is still past what a reviewer can hold in mind — and
the phases are natural, independent review units. A concrete mapping of
the current diff (PR 1 is already done — #360 landed it on main while
this review was being drafted):
| PR | Contents (plan phase) | Notes |
|---|---|---|
| 1 | Trust-provenance rendering (P1b) | Done — landed on main via #360 mid-review; no longer in this diff |
| 2 | Echo deletion, transactional send-then-commit, synchronous Chat.messages() (P2) |
The core behavioral change; ChatExpress + client bundle advance together |
| 3 | Exchange record + capture + store (P3) | Stacks on 2; _history_store.py changes belong here |
| 4 | Restore + branching + bookmark pointer (P4) | Stacks on 3 |
| 5 | Startup gate + Phase 5 hardening | Stacks on 4 |
Two notes on the split. Squash the process/docs commits out of history as
each PR is cut (they're valuable — as the plan's archive, not as review
context; _plan/ rides the branch until its own scheduled cleanup, which is
fine). And let comments 2 and 3 (admission-gate centralization, the
_history.py splits) ride the earliest PR they touch — they shrink every
later diff and the R port. When _history.py splits, its tests should split
with it: test_history_controller.py grew by ~7,300 lines in this branch,
and a file-per-class split keeps the R port's parity fixtures reviewable
too.
| # TODO: deprecate messages once we start promoting managing LLM message | ||
| # state through other means | ||
| async def _append_init_messages(): | ||
| async def _append_init_messages( |
There was a problem hiding this comment.
1. Chat(messages=...) + history enabled: fail loudly instead of suppressing
Location: Chat.__init__ (_chat.py:324) — history defaults to True
(line 329), messages warns deprecated (line 344), and the startup appends
(_append_init_messages / _init_chat, ~440–453) run straight into the Q1
suppression contract; the bare returns are at _chat.py:2234, 2255,
1250.
The messages parameter's original purpose was letting developers hand-roll
their own history feature. The Q1 suppression contract silently invalidates
that whenever history is enabled — which is the default. Today an app passing
messages gets a deprecation warning (so it knows the parameter is on the way
out) and silently loses the content (so it doesn't know it's already gone on
this branch). That's the worst of both, and it quietly removes a supported
capability rather than migrating it.
Fix: rather than flat-out deprecating the parameter, raise a loud error
when messages is provided and history is enabled — both are constructor
arguments, so the check is static and fails immediately, not in a startup
race. The error should guide developers to the two supported alternatives:
Chat(messages=...)requireshistory=False: startup messages can't be
recorded by the conversation-history feature. Use thegreetingparameter
for a startup message, or sethistory=Falseif you're managing
conversation state yourself.
With history=False there is no recorder and no suppression, so messages
keeps working exactly as on main — the hand-rolled path becomes a supported
mode rather than an accident. It'd also be worth revisiting the blanket
deprecation warning at line 344: the incompatible combination is now an
error, and the
remaining use is the supported one. This is technically breaking for apps
that pass messages and never touched history, but loudly, with a one-line
fix — which beats silent loss by any measure. And worth carrying the same
contract into Phase 6's chat(messages=, history=).
Secondary: with the messages path failing
loudly, the remaining suppressed appends are programmatic
chat_append()/stream calls during the init window — those still deserve a
warning when suppression fires (short and constant, easy to filter). The
drop is deliberate, but it should never be invisible.
| if ( | ||
| controller is not None | ||
| and controller._exchange_recorder is not None | ||
| and not self.history._initial_history_initialized |
There was a problem hiding this comment.
2. The admission gate is one decision copied into seven places
Location: the controller is not None and controller._exchange_recorder is not None and not self.history._initial_history_initialized guard, repeated at
pkg-py/src/shinychat/_chat.py:519, 1136, 1250, 1361, 1663, 2234,
and 2255.
The plan gives message capture a single choke point (Chat._send_action) for
exactly the right reason — every wire action funnels through one place, so
nothing can bypass it. The restore-admission decision (Q1's
disabled-until-restore-decision) deserves the same treatment, but it's
enforced by a hand-copied triple guard at each call site instead. Any future
code path that accepts input or starts a stream has to remember the guard;
there's no single place whose job it is. Under R6 the R port mirrors this
structure, so seven guards become fourteen, and the next admission-seam
change is a two-language archaeology dig.
Fix: one predicate on the controller (e.g. _history_action_admitted()),
each site asking it once. This is also the natural home for the warning in
comment 1. Cheap now, much cheaper than after the port.
| pass | ||
|
|
||
|
|
||
| class _ExchangeRecorder: |
There was a problem hiding this comment.
3. _history.py carries three lifecycles in two god classes
Location: _ExchangeRecorder (_history.py:293-1003, ~710 lines —
capture hook registry + restore planning + state materialization);
HistoryController (_history.py:1005-2269, ~1,260 lines — session
orchestration + record CRUD + UI actions + bookmark integration);
switch_to() (_history.py:1627+) interleaves the v1 and v2 record paths in
nested conditionals instead of dispatching on record type.
Each piece is individually followable; the problem is concentration. A
maintainer can't answer "what happens on conversation switch" without
tracing both record versions at once through three nesting levels, and
_ExchangeRecorder forces capture, restore-plan, and materialization
concerns to be understood together. The R port will mirror whatever shape
this is in — mirroring a 1,250-line orchestrator is the single biggest
mental-model cost the port could inherit.
Fix (before the port, while the test suite pins behavior): split
_ExchangeRecorder into its three natural roles, and make v1/v2 dispatch
structural (separate methods or a record-type dispatch) rather than
interleaved. This is the one I'd push for most strongly, ideally before
Phase 6 starts — sequencing advice, not a merge condition.
|
|
||
| def _invalidate_turn_baseline(self) -> None: | ||
| # Canonical turn fingerprints always serialize a dictionary, never "". | ||
| self._turn_baseline = [""] |
There was a problem hiding this comment.
4. Turn-baseline states are encoded in folklore
Location: self._turn_baseline in _ExchangeRecorder
(_history.py:337, 341, 360).
The baseline is a list of canonical-JSON fingerprints with two undocumented
sentinel states: [] (uninitialized) and [""] (broken — set by
_invalidate_turn_baseline(), permanently forcing snapshot mode from then
on). Nothing in the code says that [""] means "never try delta again," and
once tripped there's no path back even if turns become compatible again.
Fingerprints are also recomputed on every capture, and the surrounding
validation exists twice (_validate_turn_entries at 428 vs
_validate_restore_state_entry at 511) as do the preflight builders
(_preflight_restore_state at 532 vs _preflight_rewind_state at 605).
Fix: make the state explicit — a small dataclass/enum (pristine,
tracking(fingerprints), broken) instead of list-shape conventions — and
unify the duplicated validation/preflight helpers. The snapshot escape hatch
is the right design; its state machine just shouldn't be inferred from list
shapes by the fourth maintainer to read it.
| type="warning", | ||
| ) | ||
|
|
||
| async def _clear_failed_restore(self) -> None: |
There was a problem hiding this comment.
5. Restore failure handling is all-or-nothing
Location: _clear_failed_restore() (_history.py:1403), called from the
restore path's except BaseException handlers (1519, 1599).
A single rewind-hook exception wipes the recorder's record, turns,
messages, active id, and greeting. As a last-resort invariant restoration
it's defensible; the concern is blast radius — one flaky third-party
restore/rewind hook (the §3.7 extension contract is meant to invite
those) can erase the whole session, with no partial-recovery option and,
as far as I can tell, no specific warning distinguishing "hook failed,
session reset" from any other restore failure.
Fix (minimum): document the all-or-nothing rationale at the function,
and log which hook failed before clearing. Fix (better): isolate hook
consumption per entry — a failing entry degrades with a warning (the same
contract stored-turn replay already uses) rather than failing the restore.
Either is fine; the part I'd most want addressed is the silence — that's
the whole ask.
|
|
||
| def ui_message_count(self) -> int: | ||
| # Client-facing message count for this node. Must mirror replay_ui's | ||
| # Rendered message count for this node. Must mirror replay_ui's |
There was a problem hiding this comment.
6. An index-math invariant held by a comment, not a test
Location: ConversationNode.ui_message_count() in
pkg-py/src/shinychat/_history_types.py (~92) — "must mirror replay_ui's
node.ui or [<fallback>]" so a missing/empty ui still renders one
fabricated message.
If the fallback rendering and this count ever drift, positional alignment
breaks silently. This is the class of invariant (same shape as the old
extend_record_linear alignment heuristic this branch deliberately
deleted) that the design works hard to avoid — it deserves a test pinning
the pair together, not a comment.
Fix: one test asserting ui_message_count() against the replay
fallback for the ui-absent, ui-empty, and ui-present cases. Three
assertions, permanent insurance.
| } | ||
| return | ||
| } | ||
| if (action.type === "history_edit_projection") { |
There was a problem hiding this comment.
7. Client: edit-projection logic duplicated across layers
Location: the history_edit_projection handling in
js/src/chat/ChatApp.tsx (274-304) vs the reducer case in
js/src/chat/state.ts (1236-1239), which duplicate the INPUT_SENT /
TRUNCATE_MESSAGES sequencing.
Small, and the client is deliberately thin (the plan's "no client-side
transcript state machine" claim holds up — the store, reducer, and exchange
metadata are each individually coherent). But the projection path is the
one place client code now constructs transcript state rather than
mirroring it, so it's the one place where a divergence between the two
copies would be user-visible mid-edit.
Fix: extract the truncation+input dispatch into one shared helper used
by both the live submission path and the projection action. Anytime; not
port-blocking.
What this PR asks you to review
This PR asks whether the Python exchange-tree design is the right reference for the R implementation. It does not ask for final merge, release, or documentation approval. No R port begins until reviewers agree that the record shape, lifecycle, and Python/R contract are sound.
If you see a structural problem, leave a review comment before the R work begins. Structural comments include changes to the record model, capture or restore hooks, wire protocol, state ownership, initialization behavior, or the cross-language contract. I will turn accepted changes into scoped work before porting.
The central design decision
This is the structural observation that makes the exchange tree possible. It defines where an alternative can begin and gives edit, retry, and regenerate one shared operation.
What we learned from the previous design
The previous history redesign treated the displayed transcript and the provider turns as two histories that had to stay aligned. It tried to restore that alignment after the fact with cursors, prefix checks, positional matching, settlement work, and admission gates. Those mechanisms could each look reasonable in isolation but did not form a reliable system. A correction in one path created new failure cases in another.
The replacement has three non-negotiable constraints.
These constraints rule out both display-only and turns-only history. They also rule out another attempt to keep separate records synchronized after the conversation has moved on.
Do not invent a second lifecycle
Startup and restore boundaries tempt an implementation to hold work for later, add a timer, or introduce an intermediate state. That creates another lifecycle with its own owner, release condition, cancellation path, and recovery behavior. It can drift from the conversation lifecycle that history must preserve.
The exchange-tree design makes a narrower choice. Until shinychat has made the authoritative history decision, it does not accept user submissions or capture-eligible startup appends. The browser preserves the user draft and attachments. The server rejects premature input and suppresses premature appends instead of buffering or replaying them. After the decision completes, ordinary work proceeds. Greetings remain ambient presentation.
This keeps ordering in the observed conversation events. It does not ask a timer or held payload to decide which exchange owns later work.
Why change history
The key observation is that a conversation branches only when the user submits new input. Server output can stream, call tools, or append several messages, but it does not offer a competing continuation. The next user input is the first moment at which the conversation can take another path.
That makes an exchange the right history unit. An exchange starts with one user input and contains every server message until the next user action. It is one node in a tree. Editing an input, retrying a failure, and regenerating a response all create a sibling exchange. The history model follows the interaction model instead of trying to build separate branching rules for messages, turns, retries, and edits.
The design
A conversation is a tree of exchange nodes and an active-leaf pointer. Each node records its identity and status, optional input, the wire-message specs sent to the browser, named state entries, and bounded error data. Input-less nodes record server-initiated output but are not branch points.
The server captures display messages at its send choke point. It stores the attached client turns beside those messages in the registered
shinychat:turnsentry. These are related facts, not two views that must match. Restore replays messages through the ordinary display pipeline and applies turn state throughset_turns. The user view and model view can therefore differ without making history inconsistent.Turn capture writes a delta while the earlier turns remain a prefix. It writes a snapshot if the application replaced that prefix. The first user input closes the root node with the initial snapshot. Restore validates the stored graph before it mutates the live chat. If a provider turn can no longer replay, shinychat warns and retains the restored display history. Failed and cancelled exchanges keep their input and any output already sent.
One operation handles edit, retry, and regenerate. It creates a sibling at the chosen exchange, rewinds registered state to the parent, and submits the input. During the initial history decision, the client blocks user input, the server rejects premature input, and shinychat suppresses unadmitted startup work. Greetings are ambient presentation and do not enter the history record.
Why this is an improvement
The exchange tree makes the branching rule explicit and gives each branch a stable record of both what the user saw and what the model received. The design captures those facts when they happen. It does not reconstruct or reconcile them later.
The design records the display transcript universally, so manual and multi-agent apps retain useful history without depending on a provider client. The attached-client path adds resumable model state through the same exchange lifecycle. Neither path needs a second transcript or a later alignment pass.
Evidence to review
test_chat_transcript.pycovers send-before-commit, admission exclusion, stream-to-exchange attribution, and terminal preservation.test_history_controller.pycovers capture and restore, durable failures, degraded replay, rewind and resubmit, and validation before mutation.test_history_store.pycovers atomic write recovery, ordering and partition trust, and cache coherence.The test-quality audit independently reviewed these clusters and removed weaker duplicate cases.
make py-checkpasses on this branch with Ruff, Pyright, 211 Playwright tests, and 961 Python tests with one existing skip.R port and remaining work
The R port must use the same exchange-node structure, fixture matrices, and error conditions. The two packages keep independent stores and provider-owned turn serialization. They do not need byte-identical records.
The R work includes fail-closed graph validation, isolated R ID generation, invalidation of late cancelled chunks, restored terminal metadata, atomic and malformed-record behavior, the no-queue startup rule, and the ambient greeting contract.
Release work includes legacy v1 import, removal of
Chat(messages=...), and the remaining persisted-values contract. The backlog retains shared-client concurrent-turn attribution, browser completion provenance, and multi-writer support. ellmer and chatlas own replay continuity across adjacent provider releases. Shinychat warns and degrades genuinely incompatible stored entries.This PR does not include the R implementation, final documentation, release cleanup, cross-language stored-record loading, greeting/history coupling, MarkdownStream capture, retroactive re-rendering, or multi-writer support.
Review boundary
Ignore
_plan/. Those files are retained working history for this branch. They are not part of the requested source review, and final merge cleanup removes them.Refs:
shinychat#73pm