Fix/issue 1330 taskid null render crash #1330 - #1331
Merged
Conversation
filterToolProgressMessages 放行 data:null 的 progress 消息（结构漏洞）， UserToolSuccessMessage 的工具结果渲染不在嵌套边界内（任何字段访问崩溃 都会撕裂整个 MessagesBoundary）。补齐两层防线： - Tool.ts: data != null 过滤 + UserToolSuccessMessage 结果槽位加 SentryErrorBoundary（name=ToolResultMessage） - BashTool/PowerShellTool/MonitorTool/AttachmentMessage/ UserTeammateMessage/TaskAssignmentMessage 的 taskId 读取改可选链 - 新增 filterToolProgressMessages data:null 回归测试 Co-Authored-By: deepseek-v4-flash <deepseek-ai@claude-code-best.win>
review 后移除 8 处纯防御性可选链改动（原有守卫已完整），仅保留: - filterToolProgressMessages 拒绝 data 为 null 的 progress 消息 - ToolResultMessage 边界包裹，隔离工具结果 UI 渲染崩溃 - AssistantToolUseMessage / messages.ts 同源 data 空值守卫 Co-Authored-By: deepseek-v4-flash <deepseek-ai@claude-code-best.win>
Contributor
📝 WalkthroughWalkthroughThe change adds null checks for tool-progress message data and wraps tool-result rendering in a named ChangesTool message safety
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utils/messages.ts (1)
1279-1283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a type guard instead of asserting the nullable shape.
The assertion does not validate the runtime object or
hookEvent. If a malformedhook_progresspayload lacks a validhookEvent, the code inserts an invalid key intoinProgressHookCounts. Reuse a guard that validates bothtypeandhookEventbefore updating the map.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/messages.ts` around lines 1279 - 1283, Replace the nullable type assertion in the progress-data handling with the existing type guard that validates both the payload type and hookEvent before entering the hook_progress branch. Update the condition around inProgressHookCounts so malformed payloads are ignored and only validated HookEvent values become map keys.Source: Coding guidelines
src/__tests__/Tool.test.ts (1)
207-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the lookup-builder path.
This test covers
filterToolProgressMessagesonly. Add or confirm tests forbuildMessageLookupswithdata: nullanddata: undefined, so the changedprogressData.typepath also remains protected from regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/Tool.test.ts` around lines 207 - 221, Add tests for buildMessageLookups covering messages whose data is null and undefined, verifying they are safely ignored without accessing progressData.type or throwing. Keep the existing filterToolProgressMessages regression test unchanged and assert the lookup-builder output remains valid for the remaining data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/messages/UserToolResultMessage/UserToolSuccessMessage.tsx`:
- Around line 112-121: Move the synchronous tool renderer call into a dedicated
child component, then render that child inside the named SentryErrorBoundary in
UserToolSuccessMessage so renderer exceptions are caught. Add a regression test
covering a synchronous throw from renderToolResultMessage and verify the
boundary fallback replaces only the tool-result row.
---
Nitpick comments:
In `@src/__tests__/Tool.test.ts`:
- Around line 207-221: Add tests for buildMessageLookups covering messages whose
data is null and undefined, verifying they are safely ignored without accessing
progressData.type or throwing. Keep the existing filterToolProgressMessages
regression test unchanged and assert the lookup-builder output remains valid for
the remaining data.
In `@src/utils/messages.ts`:
- Around line 1279-1283: Replace the nullable type assertion in the
progress-data handling with the existing type guard that validates both the
payload type and hookEvent before entering the hook_progress branch. Update the
condition around inProgressHookCounts so malformed payloads are ignored and only
validated HookEvent values become map keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e8bbf9d-590f-49f0-a059-31bf3e63df48
📒 Files selected for processing (5)
src/Tool.tssrc/__tests__/Tool.test.tssrc/components/messages/AssistantToolUseMessage.tsxsrc/components/messages/UserToolResultMessage/UserToolSuccessMessage.tsxsrc/utils/messages.ts
Comment on lines
+112
to
+121
| {/* | ||
| Tool-provided result UIs are rendered from runtime data | ||
| (message.toolUseResult). Resumed transcripts deserialize it via raw | ||
| JSON.parse (parseJSONL), so a partial/corrupt/old-format result can | ||
| crash renderToolResultMessage on first field access | ||
| (anthropics/claude-code#39817, claude-code-best/claude-code#1330). | ||
| Keep the result slot behind its own boundary so a bad result only | ||
| degrades that row instead of tearing down the whole Messages tree. | ||
| */} | ||
| <SentryErrorBoundary name="ToolResultMessage">{wrappedMessage}</SentryErrorBoundary> |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'UserToolSuccessMessage\.tsx$' . || true
echo "== file outline =="
ast-grep outline src/components/messages/UserToolResultMessage/UserToolSuccessMessage.tsx --view expanded || true
echo "== relevant lines =="
sed -n '1,180p' src/components/messages/UserToolResultMessage/UserToolSuccessMessage.tsx
echo "== search renderToolResultMessage definitions/usages =="
rg -n "renderToolResultMessage|function renderToolResultMessage|const renderToolResultMessage|export .*renderToolResultMessage" src || true
echo "== search SentryErrorBoundary =="
rg -n "SentryErrorBoundary|class SentryErrorBoundary|function SentryErrorBoundary" src || trueRepository: claude-code-best/claude-code
Length of output: 11212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SentryErrorBoundary implementation =="
sed -n '1,90p' src/components/SentryErrorBoundary.tsx
echo "== Tool renderToolResultMessage signature and comments =="
sed -n '550,610p' src/Tool.ts
echo "== test files mentioning UserToolSuccessMessage or ToolResultMessage =="
rg -n "UserToolSuccessMessage|ToolResultMessage|SentryErrorBoundary.*ToolResult|toolUseResult" src/utils src/components/messages/src src --glob '*test*' --glob '*Test*' || trueRepository: claude-code-best/claude-code
Length of output: 5381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
import re
path = Path("src/components/messages/UserToolResultMessage/UserToolSuccessMessage.tsx")
src = path.read_text()
# Deterministic check based on source order and lexical assignment to JSX children.
try:
tree = ast.parse(src)
except Exception as e:
print(f"parse_error: {type(e).__name__}: {e}")
raise
assignments = {}
calls = []
boundary = None
func = None
for node in tree.body:
if isinstance(node, ast.FunctionDef) and node.name == "UserToolSuccessMessage":
func = node
for child in ast.walk(node):
if isinstance(child, ast.Return) and child.value is not None and isinstance(child.value, ast.Subscript):
# JSX subscripts: expressions in the JSX tree appear as Subscript nodes in this TypeScript-like AST if imported/typed.
# Use text pattern for this simple check rather than parsing JSX into AST.
pass
calls = []
boundary = None
for child in ast.walk(node):
if isinstance(child, ast.Call):
calls.append(ast.get_source_segment(src, child))
if isinstance(child, ast.Assign) and len(child.targets) == 1 and isinstance(child.targets[0], ast.Name) and child.targets[0].id == "wrappedMessage":
assignments[child.targets[0].id] = ast.get_source_segment(src, child.value)
# Find boundary text and its child expression.
text = src[node.lineno-1:tree.body.index(func)+1]
for m in re.finditer(r"<SentryErrorBoundary[^>]*>(.*?)</SentryErrorBoundary>", text, flags=re.S):
boundary = ast.get_source_segment(src, node).find(m.group(0))
print("boundary_text", m.group(0))
render_call = next((x for x in calls if "renderToolResultMessage" in str(x)), None)
wrapped_text = assignments.get("wrappedMessage")
print("render_call_present", render_call is not None)
print("wrapped_message", wrapped_text)
print("boundary_text_contains_render_call", boundary is not None and render_call in boundary if render_call else False)
print("boundary_text_contains_wrappedMessage", boundary is not None and wrapped_text is not None and "wrappedMessage" in m.group(0) if render_call else False)
break
PYRepository: claude-code-best/claude-code
Length of output: 517
Move the synchronous tool renderer under SentryErrorBoundary.
tool.renderToolResultMessage runs before React creates SentryErrorBoundary, so a synchronous exception still leaves UserToolSuccessMessage. Move the renderer call into a child component and render that child inside the named boundary. Add a regression test where renderToolResultMessage throws synchronously.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/messages/UserToolResultMessage/UserToolSuccessMessage.tsx`
around lines 112 - 121, Move the synchronous tool renderer call into a dedicated
child component, then render that child inside the named SentryErrorBoundary in
UserToolSuccessMessage so renderer exceptions are caught. Add a regression test
covering a synchronous throw from renderToolResultMessage and verify the
boundary fallback replaces only the tool-result row.
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.
Summary by CodeRabbit
Bug Fixes
Tests