Make agent transcript groups honest when collapsed - #6537
Open
baxen wants to merge 5 commits into
Open
Conversation
A collapsed tool-run group used to present only its label, so a group whose child failed — or was still running — read as ordinary finished work, and a reader had to expand it to find out otherwise. Expansion state also lived in the DOM, so it was silently reverted whenever re-grouping remounted the card: a same-kind run that later absorbs one differing tool call is rebuilt as a mixed burst with a different summary id, discarding the reader's deliberate choice to keep it open. Group presentation now carries three properties: - Aggregate status leans toward the worst outcome (failed > executing > pending > completed) and renders beside the label, outside the collapsible body. Only non-clean outcomes show a badge; a badge on every finished group would train readers to ignore the badge that matters. - Expansion is mount-aware and sticky, keyed by a durable identity: history mounts collapsed, live and failed work mounts open, and an untouched group auto-collapses when it finishes cleanly. Once the reader touches a group, automatic policy never moves it again. - Files a group touched render as chips outside the collapsible body, so collapsing hides the steps without hiding the outcome. The durable key is `group:<turnId>:<first leaf item id>` rather than the summary id. Leaf ids derive from channel plus ACP tool-call id, so they survive both re-derivation over merged live/archive windows and the same-kind to mixed transition, while turnId keeps two turns from colliding. Low-signal suppression is deliberately conservative: only successful, finished, shell-flavored scaffolding folds away, and only when the group has at least four steps, at least two would hide, and at least one primary step remains. Failures, in-flight steps, file edits, relay ops, and anything the classifier could not identify always stay visible. Separately, an empty transcript no longer claims there was no activity when history was merely unreadable. Archived observer history requires an owner_p save subscription and a completed hydration pass; without either, the archive returns nothing, which is shaped exactly like a channel where the agent never ran. `useLoadArchivedObserverEvents` now reports history certainty, and the empty state says earlier activity may not be shown rather than asserting a fact it did not check. Group rendering moved to TranscriptToolRunGroup so AgentSessionTranscriptList stays under the 1000-line ceiling; the shared row timestamp moved alongside it to avoid an import cycle. Timestamp, animation, source-pill, and raw-feed behavior are unchanged. Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
Co-authored-by: ss-quality-00 <75789fffd704a5265cd65462cfc263f44561872fbebcea3a131a2a5f511465d2@buzz.block.builderlab.xyz> Signed-off-by: ss-quality-00 <75789fffd704a5265cd65462cfc263f44561872fbebcea3a131a2a5f511465d2@buzz.block.builderlab.xyz>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c176335fef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+82
to
+85
| onToggle={ | ||
| onOpenChange | ||
| ? (event) => onOpenChange(event.currentTarget.open) | ||
| : undefined |
There was a problem hiding this comment.
The toggle event also fires when React changes the controlled open property, not only when the reader clicks the summary. Consequently, mounting a live group with open={true} or automatically changing its expansion invokes onOpenChange, and setToolRunGroupExpanded records that transition as userInteracted: true; the reconciliation policy then returns early, so an untouched live group no longer auto-collapses after clean completion. Track interaction from the summary's pointer/keyboard action or otherwise suppress callbacks caused by controlled state synchronization.
Useful? React with 👍 / 👎.
Comment on lines
+79
to
+80
| if (COMPOUND_SHELL_MARKERS.some((marker) => command.includes(marker))) { | ||
| return true; |
There was a problem hiding this comment.
Any command containing &&, ||, ;, or a pipe is classified as low-signal without inspecting what the command actually does. In a qualifying group, successful commands such as git add . && git commit, a migration followed by a deploy, or a test followed by a release command are therefore hidden behind the “routine steps” disclosure even though they are the consequential work a supervisor needs to see. Compound commands should only be folded when their constituent commands are themselves known scaffolding, rather than treating the shell operator alone as sufficient.
Useful? React with 👍 / 👎.
The group-presentation units each had unit coverage against hand-built TranscriptItem fixtures, but nothing checked whether the states those fixtures describe are reachable from real ACP frames. Grouping eligibility is decided in agentSessionTranscriptGrouping, several layers away, so a fixture can happily exercise a branch that no observer frame can ever produce. Driving buildTranscript through buildTranscriptDisplayBlocks instead pins what a reader can actually end up looking at: - A failed tool call is never folded inside a summary. isGroupingEligible rejects isError items, so a failure breaks the run and lands as its own row — including a call that grouped cleanly while executing and only failed on a later update. That ejection, not the group's aggregate badge, is what actually keeps a failure visible in a collapsed transcript, and it is the invariant a future grouping change must not quietly drop. An in-flight case is asserted alongside as a positive control so "no summary holds a failure" cannot pass vacuously by grouping having stopped producing summaries at all. - Artifact chips have something to show: ACP rawInput survives into the path/file_path args collectToolRunArtifacts reads, and an edit outranks a read of the same path. - The durable group key really holds still across re-derivation for the two transitions it was designed for — a burst growing by appended live calls, and same-kind becoming mixed. The last test documents a gap rather than a guarantee: when the FIRST leaf is the call that fails, ejecting it makes the next leaf first, so `group:<turn>:<first leaf>` churns and useToolRunGroupViewState finds nothing under the new key — the reader's deliberate collapse is discarded and the group remounts open. Expansion surviving re-grouping therefore does not hold for first-leaf ejection. The test pins observed behavior and says so, so a later fix makes it fail loudly and replaces it with the durable assertion. Tests only; no product code changed. Signed-off-by: ss-dev-03 <4807721ff6da708730cf864aeef858d7cd47b2399fffc1ee584f0e7c9f5bc2af@buzz.block.builderlab.xyz> Co-authored-by: ss-dev-03 <4807721ff6da708730cf864aeef858d7cd47b2399fffc1ee584f0e7c9f5bc2af@buzz.block.builderlab.xyz>
Addresses the three review findings on the transcript presentation slice. Finding B (merge blocker) — a reader's collapse survives ejection. Grouping is re-derived every pass, so a group had no identity of its own and was keyed by its first leaf. When the FIRST call failed, isGroupingEligible ejected it, the next leaf became first, the key churned, and the store found nothing under the new key — silently reopening a group the reader had deliberately collapsed. Identity is now resolved once per transcript pass: a group keeps the identity of any leaf it still holds, an identity is claimed by at most one group, and on a mid-run split the half holding the earlier leaves inherits the reader's state. Identity is always content-derived from leaf keys, never positional, so archive prepends cannot renumber anything. The recognition table is scoped per transcript instance and retains only leaves present in the current pass, so it stays bounded. Fixing that surfaced a second, larger defect in the same seam: a `<details>` fires `toggle` when React sets `open` on mount, not only when a reader clicks, and the controlled row recorded that echo as a reader action. Every group that mounted open immediately latched the sticky userInteracted flag, which disabled automatic collapse entirely — the auto-collapse policy was dead in the real app while passing at the unit level. React also re-asserts the declared `open` before the async toggle is delivered, which loses a genuine click. Controlled rows now drive disclosure from the summary click and prevent the default, so the caller's state is the single source of truth. Finding A — history-certainty copy no longer drifts. getUncertainHistoryCopy existed but the UI inlined its own wording. The certainty type now names which check came up short rather than collapsing to a coarse "unknown", and that helper is the only place the wording lives. Per the product decision the uncertain message is correct when archive completeness cannot be established, so the channel E2E expectation was updated to the honest state instead of changing behavior. Finding C — unreachable failure detail removed. No observer frame can put a failed call inside a group, so the "N failed" plural badge was dead presentation. The badge is now one plain word and the failure-leaning fold is retained with a comment, so widening eligibility later cannot silently hide a failure. Verification (full desktop package, not scoped): 5428 unit tests pass / 0 fail, typecheck clean, pnpm check exit 0, transcript smoke spec 3/3, and channels.spec.ts 87/87 — including the previously red "shows and clears activity indicators" test. Both new browser tests were mutation-checked: restoring the toggle mechanism fails them. Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
Scope durable group disclosure keys to the rendered agent, channel, and transcript variant so simultaneous viewers cannot share reader choices. Co-authored-by: ss-quality-00 <75789fffd704a5265cd65462cfc263f44561872fbebcea3a131a2a5f511465d2@buzz.block.builderlab.xyz> Signed-off-by: ss-quality-00 <75789fffd704a5265cd65462cfc263f44561872fbebcea3a131a2a5f511465d2@buzz.block.builderlab.xyz>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A collapsed tool-run group in the agent activity transcript presented only its label. A group whose child failed — or was still running — read as ordinary finished work, so a reader had to expand it to find out otherwise. Expansion state also lived in the DOM, so it was silently reverted whenever re-grouping remounted the card: a same-kind run that absorbs one differing tool call is rebuilt as a mixed burst with a different summary id, discarding the reader's deliberate choice to keep it open.
Separately, an empty transcript claimed there was no activity even when history was merely unreadable.
These qualities were drawn from a study of Berd's chat experience. This is not a visual clone and deliberately does not port Berd's measurement architecture, unread model, replay system, or composer seams.
What changed
Failure-leaning group status. Children fold with precedence
failed > executing > pending > completed.isErroralone fails a group, and a lifecycleerrorrow inside the span fails it too. The badge renders outside the collapsible body, and only for non-clean outcomes — a badge on every finished group would train readers to ignore the badge that matters.Mount-aware sticky expansion. History mounts collapsed, live and failed work mounts open, and an untouched group auto-collapses when it finishes cleanly. Once the reader touches a group, automatic policy never moves it again. State is computed during render rather than in an effect, so the first paint after completion is already correct instead of flashing open for a frame.
Artifacts outside collapsed content. Every file a group touched renders as a chip outside the body, deduped per path with an edit outranking a read. Collapsing hides the steps, not the outcome.
Conservative low-signal suppression. Only successful, finished, shell-flavored scaffolding folds away, and only when the group has ≥4 steps, ≥2 would hide, and ≥1 primary step remains. Failures, in-flight steps, file edits, relay ops, and anything the classifier could not identify always stay visible. Steps the reader opened are pinned.
Empty vs. uncertain history. Archived observer history requires an
owner_psave subscription and a completed hydration pass. Without either, the archive returns nothing — shaped exactly like a channel where the agent never ran.useLoadArchivedObserverEventsnow reports history certainty,AgentSessionTranscriptEmptyStategained"unknown", and only"idle"may state emptiness as fact. Uncertain history renders "Earlier activity may not be shown"; a test asserts that copy never contains "no activity".Stable group identity
The durable key is
group:<turnId>:<first leaf item id>, not the summary id. Leaf ids derive from channel + ACPtoolCallId, so they survive re-derivation acrossmergeObserverEventWindowsand the same-kind → mixed transition, whileturnIdkeeps two turns from colliding. Empty groups fall back to the batch id rather than collapsing onto one shared key.Unchanged by design
Timestamp, animation, source-pill, and raw-feed behavior are untouched, as is
compactPreviewvariant semantics. Group rendering moved toTranscriptToolRunGroup.tsxto keepAgentSessionTranscriptList.tsxunder the 1000-line ceiling; the shared row timestamp was extracted to break the resulting import cycle.Verification
At exact commit
c176335fef0e0789b83014c3949e5062ebf063cc, rebased onto main074561233with no conflicts:pnpm typecheck: 0 errorspnpm check: exit 0 (biome + file-sizes + px-text + pubkey-truncation)transcript-tool-run-quality.smoke.spec.tsdrives the real app: a live group mounts open with a running badge, survives user collapse/expand, stays open through completion because the user touched it, and keeps a child step expanded across a group toggle cycleDeferred, explicitly
subagentAgentName/subagentTaskLabel), so carrying delegated identity needs a transcript-type change rather than an invented field.sameKindLabel.Review notes
Reviewed independently by ss-bugs-00 and ss-bugs-02 (explicit no-defects verdicts). ss-quality-00 contributed the second commit, which wires the expansion store into
resetCommunityState()and adds the Playwright acceptance coverage — both were open caveats on the first commit.