fix(web): stop showing red x summaries for ordinary tool failures - #8395

💭 I'm reviewing the changes to MessagesTimeline.tsx, where the collapsed summary icon lost its role/aria-label and that moved to the toggle button instead, while the failure marker was also removed—so I need to check where the "tool call failed" text now comes from to satisfy the accessibility test. I also want to check whether text-warning is a defined Tailwind token, since if --color-warning isn't set that class silently fails and warning indicators lose their color. And I should verify whether workEntrySignalsSevereFailure incorrectly matches ordinary tool activity failures via its .endsWith(".failed") check, which could keep red styling where it shouldn't apply—so I'm going to dig into the actual repo code now.

1. git_tools — text-warning|--color-warning|color-warning

2. git_tools — workEntryDisplayIndicatesToolFailure|workEntrySignalsSevereFailure

💭 Good, text-warning exists. Now I'll check MessagesTimeline.tsx around lines 2590-2660.

3. browse_code — apps/web/src/components/chat/MessagesTimeline.tsx

4. browse_code — apps/web/src/session-logic.ts

💭 I'm wondering whether .endsWith(".failed") would also match ordinary tool failure activity kinds like "tool.call.failed" or "command.execution.failed", which would undercut the PR's goal of toning down tool failure icons—so I need to grep for what activity kind strings actually exist for tool entries.

5. git_tools — .failed"

6. browse_code — What are all the possible values of OrchestrationThreadActivity kind (activity kinds)? List those ending in .failed, and indicate which ones correspond to tool/command execution failures vs orchestration/turn-level failures. Also explain how WorkLogEntry.sourceActivityKind is populated.

💭 The severity split looks sound since all .failed kinds are orchestration-level. Now I'm digging into the accessibility issue in WorkGroupToggleTimelineRow — the role="img" aria-label on the collapsed summary icon was removed, so I need to trace where the "tool call failed" text the test expects actually comes from, checking PlainWorkEntryRow's accessible text and whether the last entry row renders even when the group is collapsed.

7. browse_code — apps/web/src/components/chat/MessagesTimeline.tsx

💭 The collapsed summary still keeps its aria-label so the "tool call failed" accessibility test should pass. Looking at LiveActivityContent, the icon wrapper no longer gets the destructive text color when failed, but it still swaps to the "x" icon name, which matches the intended muted-but-marked behavior — I'm now tracking down where the announceFailure prop actually gets used. Let me check whether announceFailure is still referenced anywhere in the codebase, and I also need to confirm the actual function name around line 1482, since the diff labels it LiveActivityContent but I'm not sure that matches what's in the file — I'll look at the surrounding lines to pin down the real function boundary.

8. browse_code — apps/web/src/components/chat/MessagesTimeline.tsx

9. git_tools — announceFailure|LiveActivityRow|LiveActivityContent

💭 I notice the button already has an aria-label, so its inner content including the nested failure icon isn't separately announced — that's fine, pre-existing behavior. Now I'm checking the collapsed toggle row's new aria-label, which describes the hidden count of previous entries. Turning to the color styling: I see the diff only changed the iconWrapperClass to use text-warning when there's a warning indicator (previously it was text-destructive for both warning and failure cases), but I need to verify whether headingClass already had similar warning styling before this change or if that's a separate inconsistency worth flagging.

10. git_tools — a6797b3:apps/web/src/components/chat/MessagesTimeline.tsx

💭 I'm checking the accessibility regression around the toggle row's failure announcements. The "+N previous tool calls" toggle now properly moves the failure announcement to the button's aria-label with the hidden count, which is an improvement. But I'm noticing the summary row lost its role="img" aria-label on the icon span, though the button's aria-label may still cover that information. Now I'm looking at whether showDestructiveRowStyle switching to workEntrySignalsSevereFailure introduces a real behavioral change. The old logic checked for runtime errors or non-tool-like entries, while the new severe-failure helper seems to overlap heavily with the non-tool-like case since .failed kinds are typically already non-tool-like, so I'm trying to confirm whether this is truly redundant or if there's an edge case involving orchestration failures with different source activity kinds that the tests are specifically targeting. I also want to check the icon wrapper class logic more carefully. I notice the old code had an unreachable branch since destructive row style already implies the failed indicator, and the new version resolves this by making warning take priority, then destructive, then tool-tone or failed as muted, then falling back to the icon config. This actually fixes an inconsistency where a warning-tinted heading previously paired with a destructive-tinted icon, which is now unified so both use the warning color. For severe failures both icon and heading become destructive, which looks right, and ordinary tool failures get a muted icon with an "x" glyph alongside a secondary-label heading, which also looks correct. Now I need to check whether rows with tone "error" that aren't tool-like but also aren't failures -- like a denied-tool row -- get correctly classified by the failure-detection function, so I want to look at lines 230-260 to see how tone error is handled there.

11. browse_code — apps/web/src/session-logic.ts

12. browse_code — apps/web/src/components/chat/MessagesTimeline.logic.ts

💭 One candidate finding: the "+N previous log entries" toggle now only signals failure via an aria-label, removing the visible red X entirely, so sighted users get no indication that hidden entries include a failure. That seems to be an intentional design choice tied to toning down failure icons, so it likely falls under an aesthetic preference rather than a real bug. Checking the rest, the color logic in LiveActivityContent for failed icons is simplified consistently (highlighted vs muted) and matches the pattern of the other changes, nothing newly broken there. I'm also scanning for any leftover unused code from these edits that might now be dead. announceFailure remains meaningful since it still drives the role/aria-label, and row.hasFailure is still actively used for the toggle button's aria-labels in both expanded and collapsed states. The accessible name for the "+N previous" toggle now mentions the failure while the visible icon no longer shows an "x" for it, but that's intentional per the test. I'm checking whether the visible label text still matches the start of the aria-label (label-in-name accessibility rule) — it does for the "+N previous tool calls" case, and the expanded "Show fewer tool calls" case has no aria-label override, so that's consistent too. I traced the text-warning icon color choice back to showWarningIndicator, which is tied to runtime.warning activities and matches existing conventions elsewhere in the codebase like GitActionsControl, so nothing there seems newly broken. Now I'm checking whether the --warning CSS variable's contrast in index.css holds up against the background it's used on. The failed-tool-call row's accessibility handling still looks fine either way — the icon span carries the aria-label when the row itself can't expand, so no regression there. I'm also noting that isNoContentRuntimeWarning is duplicated between web and mobile session-logic files, but that's more of a code duplication concern than a UI consistency issue within the current review scope. Given the "smallest durable contract" guidance, since both apps already duplicate the whole deriveWorkLogEntries function, this filtering helper being duplicated too isn't really a new finding — it's consistent with the existing pattern. What's worth checking is whether the magic string suffix it matches against ("no displayable text content") is defined as a shared constant somewhere on the server side, which would mean the client apps are duplicating a literal that could drift out of sync. I should also check whether any existing tests assert the behavior that's now being removed, like an aria-label for "Hidden work includes a failure" — I'll grep the test files to see.

13. git_tools — Hidden work includes a failure|includes a failure|Tool call failed|tool call failed

14. git_tools — no displayable text content

💭 I'm checking the existing tests around lines 1150-1210 to verify whether removing the icon span aria-label would break assertions like "tool call failed" or the hidden work aria-label checks — these appear to be negative assertions that should still pass. I need to look further at lines 1140-1220 and 1329 to confirm which cases reference the aria-label directly.

15. browse_code — apps/web/src/components/chat/MessagesTimeline.test.tsx

💭 I'm checking that the mixed-success group test correctly stays neutral since it ends on a completed entry, then verifying the collapsed summary test expects the aria-label to read "Ran 2 commands, tool call failed". Now I'm confirming the muted failure marker test adds an entry-info item so the group avoids the collapsed summary path and renders PlainWorkEntryRow instead, and that the severe orchestration failure test relies on tone "error" plus the failed provider turn kind to trigger the red destructive styling. Then I trace through tool.denied: it has an error tone so the failure indicator shows, but since it's tool-like and its kind doesn't end in ".failed" nor equal "runtime.error", it doesn't count as severe, so it stays muted rather than destructive — matching the prior behavior. I move on to the runtime.warning case, which now correctly shows warning color instead of destructive, matching the heading's existing warning styling. I also want to check if anything else in the codebase, like tests or docs, still depends on the old red-X styling for denied tool calls. I check the aria-label on the "+N previous" toggle button matches the visible text and correctly notes when a failure is included — it does. I look for real issues but keep coming up empty: removing failed from the color ternary in LiveActivityContent doesn't affect anything since it's still used for icon name, and the highlighted/visible copy colors already diverged before. The announceFailure icon span nested inside a labeled button is pre-existing, not something introduced here. I check whether the mobile app duplicates workEntrySignalsSevereFailure logic separately, but the thread-work-log.tsx change removing red tint on failure rows looks out of scope for this review. The comment placement in PlainWorkEntryRow feels slightly off but is minor, and I'm now tracing through the conditional branches building iconWrapperClass to see if any branch is unreachable given warning, destructive, tool, or failed states. Working through the branch logic, non-tool error rows correctly stay destructive since showDestructiveRowStyle triggers when the entry isn't tool-like, so that path checks out. I also consider whether text-warning at reduced opacity gives enough contrast for the small icon, but it's used consistently elsewhere so it should be fine, and now I'm digging into how workEntrySignalsSevereFailure determines severity from the entry's source activity kind. I don't see an actual bug yet, just verifying the "defaults to failure, shows All clear when clean" expectation holds up. Checking accessibility, both branches now rely only on the button's aria-label instead of a separate role="img" span, which is actually more consistent with the other timeline row component's pattern. Now I'm looking at how hasFailure gets computed for the summary branch. I'm checking whether other parts of the codebase reference these styles, confirming index.css and the muted-text class are unaffected. Scanning the diff once more for genuinely new concerns, I note the new workEntrySignalsSevereFailure export looks fine, though the isNoContentRuntimeWarning logic is duplicated between session-logic.ts and the mobile file using the same magic string. That duplication also appears in the server's ClaudeAdapter with no shared constant, which raises a real maintainability concern — three copies of the same literal risk silently drifting apart and breaking the filter. Still, I'm weighing whether this counts as a violation under this UI-consistency check versus being out of scope, since the instructions caution against demanding broader cleanup beyond the check's focus. I want to verify whether deriveWorkLogEntries filters before or after grouping, since dropping the last entry in a group could leave an empty section — though WorkGroupSection already returns null for empty groups, so that's handled. I'm now tracing the placement of the no-content-runtime-warning filter inside that function to confirm it runs early in the loop and doesn't conflict with other pending filters.

16. browse_code — apps/web/src/session-logic.ts

💭 Everything looks coherent, so there are no findings to report. There's a slight tension between the instruction to respond exactly "All clear" and the requirement to output valid JSON, so I'll resolve it by setting the summary field to "All clear" and marking the result as success within the proper schema.