Skip to content

refactor(web,server): match a turn to its reply by execution id, and clamp text through one primitive#545

Merged
gbasin merged 4 commits into
masterfrom
fix/turn-identity-and-clamp
Jul 16, 2026
Merged

refactor(web,server): match a turn to its reply by execution id, and clamp text through one primitive#545
gbasin merged 4 commits into
masterfrom
fix/turn-identity-and-clamp

Conversation

@gbasin

@gbasin gbasin commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

The two follow-ups worth doing from the #538/#539 thread, built in parallel lanes and reviewed here. The third (tokenizing WorkFold's ml-[43px]) I dropped after investigating — see the bottom.

1. Fold→reply matching by identity, not position

session.replied carried {session_id, text, broadcast} and no turn identity, so buildSpineRows matched by ordinal: the Nth reply-producing fold in the SSE stream was assumed to be the Nth session.replied in the thread. True only while they stay 1:1 — and a failed execution posts no session.replied, so the two clocks drift and a turn's work can nest under someone else's answer. (Prod thread 575 has exactly this shape: 5 executions, 4 replies. It doesn't currently mis-attribute only because the failed turn produced no fold.)

The terminal execution_state frame that drives completeSession already knows which execution answered, so it now says so:

  • serversession.replied payload carries execution_id
  • reducerSessionState tracks the current execution; every item is stamped with the execution that produced it (following the existing Artifact.executionId precedent)
  • foldsFoldedTurnRow.executionId
  • wireChatMessage.sessionExecutionId, decoded-with-default so legacy events yield null rather than throwing
  • placement — matches on identity; the ordinal machinery is deleted

No fallback, deliberately (product call): replies written before this change have no execution id and simply don't nest — their work keeps a standalone row. One code path, paid for with nesting on existing threads. Where a steer splits a running execution into several folds, the execution's answer is the last one — that nests, earlier ones stay standalone. No fold is ever dropped.

2. One clamp primitive

#538's bug was two clamps nesting: CompactReply's line-clamp-3 wrapped MessageText's own max-h-80, so "Show more" shrank the message (360px → 87px in a real browser). #538 fixed it with a collapsible opt-out — a convention, and conventions get forgotten.

ClampedBlock puts the clamp in React context, so an inner block declines to clamp: the nesting is now unrepresentable, not merely discouraged. It also measures overflow rather than trusting a length heuristic, so a message that trips the threshold but fits on screen no longer offers a toggle that does nothing (the one visible behaviour change).

Adopted at 2 of 4 sites, on purpose. EntryQuoteCard collapses a changes list behind a domain-specific action row; SessionCapabilitiesPopover slice()s an item array and has no overflowing box to measure. Forcing either through the primitive would bend it out of shape for no benefit — the duplication there is honest.

Verification

  • 729 web · 171 centaur-client · 428 shared · 893 server ✓ · biome ✓ · typecheck ✓
  • Real browser: cluster-steer-interleave (incl. "Show more grows a long agent reply" — jsdom has no layout and cannot judge it) and session-pane-ux work-fold specs all pass through the new primitive.
  • Local full-suite e2e is flaky on my machine right now and CI is the arbiter. I baselined it: stock master with zero changes fails 1–2 tests per full run (addressability-shell:81, nav-discoverability:23), a rotating cast that all pass in isolation. This branch shows the same rate and character. I nearly mis-attributed that to these changes.

Dropped: the WorkFold gutter token

ml-[43px] / calc(100%-59px) appear exactly once in the codebase, already behind a nested prop with an explanatory comment. There's no duplication to remove, and the numbers don't align to anything I could name truthfully (MessageRow's content column computes to 60px, not 43). Naming a single-use constant would be indirection for its own sake.

@gbasin gbasin closed this Jul 16, 2026
@gbasin gbasin reopened this Jul 16, 2026
gbasin added 3 commits July 16, 2026 15:50
…clamp text through one primitive

Two follow-ups to #538/#539.

`session.replied` carried no turn identity, so the client matched a fold to a
reply by ordinal: the Nth reply-producing fold in the stream was assumed to be
the Nth reply in the thread. That holds only while the two stay 1:1, and a
failed execution posts no session.replied — so the clocks could drift and nest a
turn's work under someone else's answer.

The terminal execution_state frame that drives completeSession already knows
which execution answered, so it now says so: session.replied carries
execution_id, the reducer stamps each item with the execution that produced it,
and folds carry it through. Placement matches on identity, and the ordinal
machinery is gone.

Deliberately no fallback: replies written before this change have no execution
id and do not nest — their work keeps a standalone row. One code path, at the
cost of nesting on existing threads. When a steer splits a running execution
into several folds, the execution's answer is the last of them; that one nests
and the earlier ones stay standalone.

MessageText's own max-height, so "Show more" shrank the message. That was fixed
with an opt-out prop — a convention, and conventions get forgotten. ClampedBlock
carries the clamp in context, so an inner block declines to clamp and the
nesting is unrepresentable rather than merely discouraged.

It also measures overflow instead of trusting a length heuristic, so a message
long enough to trip the threshold but short enough to fit no longer offers a
toggle that does nothing.

Adopted by the two sites that genuinely share the shape. EntryQuoteCard and
SessionCapabilitiesPopover keep their own: the first collapses a changes list
behind a domain action row, and the second slices an item array rather than
clamping a box. Forcing them through the primitive would bend it out of shape to
no benefit.
…it came from

Making `executionId` required is what surfaced these: the workspace typecheck
found two constructors the change had missed, in packages the lane's scope
excluded. Tests didn't catch them because tests don't typecheck.

The server's session-records projection replays stored events, which carry no
execution boundary — unlike the live stream, where execution_state frames stamp
each item — so null is the honest value there. Mobile's fixtures just had to say
so; mobile does no fold-to-reply placement, so nothing in its logic moves.
GitHub dropped the pull_request synchronize event for b4e07dc — only the
pull_request_target CLA workflow fired. Reopening the PR did not re-fire it
either, so this nudges a fresh SHA.
@gbasin
gbasin force-pushed the fix/turn-identity-and-clamp branch from 2c4eb4f to ef12d6a Compare July 16, 2026 20:05
ToolCallItem gained a required executionId, but this hand-built literal
was never updated. vitest does not typecheck, so only the build caught it.
@gbasin
gbasin added this pull request to the merge queue Jul 16, 2026
Merged via the queue into master with commit 9d0614f Jul 16, 2026
16 checks passed
@gbasin
gbasin deleted the fix/turn-identity-and-clamp branch July 16, 2026 20:39
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.

1 participant