assistant-ui fix(react): tolerate reordered tool call arguments

Closes #2613

Problem

The useToolInvocations hook would crash with a Tool call argsText can only be appended, not updated error. This was caused by a strict startsWith check that failed when the final, complete JSON for a tool's arguments had its keys in adifferent order than the partially streamed string

Solution

The logic is now more robust. It first checks if both the old and new argsText are complete JSON objects

If they are, it assumes the difference is due to harmless key reordering and proceeds without throwing an error
If they are not (i.e., the stream is still in progress), it retains the strict startsWith check to ensure the stream is valid

A regression test has been added to verify the fix

Dependency Changes

To enable testing and resolve a configuration issue, the following development dependencies were added:

@testing-library/react: Added to the packages/react package. This provides the renderHook utility for testing React hooks and aligns with testing patterns already used in other parts of the monorepo
@tsconfig/strictest: Added to the workspace root. This was a missing peer dependency required by a shared TypeScript configuration, and its absence was preventing the test suite from running

[!IMPORTANT] Fix useToolInvocations to handle reordered JSON keys in argsText and add tests for robustness.

Behavior:

Fix useToolInvocations in useToolInvocations.ts to handle reordered JSON keys in argsText without errors.

Adds logic to check if argsText is complete JSON and handle key reordering.

Retains strict startsWith check for incomplete JSON.

Testing:

Adds useToolInvocations.test.tsx with tests for reordered argsText, incomplete JSON, and complete JSON with different content.

Dependencies:

Adds @testing-library/react to packages/react/package.json for testing React hooks.

Adds @tsconfig/strictest to package.json to resolve TypeScript configuration issues.

^{This description was created by}^{for c21fbce1a47093ed5542ae4a289bdecb9baf1866. You can customize this summary. It will automatically update as commits are pushed.}

Nov 19 '25 18:11 ShobhitPatra

⚠️ No Changeset found

Latest commit: 6dd1ca655a4c1e4d398eaedf4575cc34c12efe58

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Nov 19 '25 18:11 changeset-bot[bot]

Claude finished @ShobhitPatra's task —— View job

Code Review

Summary of Changes

This PR fixes a critical bug where useToolInvocations crashed when tool call argument JSON keys were reordered during streaming. The fix adds logic to detect when both old and new argsText are complete JSON objects and treats key reordering as harmless rather than throwing an error.

Critical Issues

1. Logical Flaw: Valid Updates Will Be Silently Ignored

The fix has a serious bug on lines 176-179 in useToolInvocations.ts:176-179. When both isLastComplete and isCurrentComplete are true, the code does nothing—it doesn't update lastState.argsText or call controller.argsText.append(). This means:

Problem: If a complete JSON is legitimately updated to a different complete JSON (not just reordered), the update will be silently ignored. The controller will retain the old arguments instead of the new ones.

Why this matters: The current implementation assumes that if both are complete JSONs and different, it must be reordering. But there are valid scenarios where a complete JSON might be replaced:

Runtime updates/corrections to tool arguments
State synchronization issues
Re-renders with different complete argument sets

Expected behavior: The code should verify that the two JSON objects are semantically equivalent (same keys/values, different order) before deciding to ignore the difference.

Recommended Fix

if (isLastComplete && isCurrentComplete) {
  // Both are complete JSONs. Check if they're semantically equivalent.
  try {
    const lastParsed = JSON.parse(lastState.argsText);
    const currentParsed = JSON.parse(content.argsText);
    
    // Deep equality check - JSON.stringify normalizes key order
    if (JSON.stringify(lastParsed) !== JSON.stringify(currentParsed)) {
      // Different content - this is unexpected and should be logged
      if (process.env["NODE_ENV"] !== "production") {
        console.warn(
          "Complete argsText updated with different content:",
          { previous: lastState.argsText, next: content.argsText }
        );
      }
    }
    // In both cases (same or different), no append needed since args are complete
  } catch (e) {
    // JSON parsing failed unexpectedly - shouldn't happen since both passed isArgsTextComplete
  }
}

Note: Using JSON.stringify for comparison normalizes the key order, so JSON.stringify({a:1,b:2}) === JSON.stringify({b:2,a:1}) returns true.

2. Missing Changeset (CI Failure)

The changeset-bot flagged that this PR modifies source code in packages/react but lacks a changeset. According to the repository's assistant-ui requirements, all PRs that modify published packages need a changeset for version management.

Action required: Add a changeset using:

npx changeset add

Select @assistant-ui/react as the package and choose patch as the bump type since this is a bug fix.

Test Coverage Issues

3. Incomplete Test Coverage

The test in useToolInvocations.test.tsx:10-66 only validates the happy path (no error thrown). It doesn't verify:

What arguments are actually used: Does the controller receive the correct final arguments? The test should verify that the tool gets the right args, not just that it doesn't crash.
Edge cases:
- What if argsText changes from incomplete → complete with different key order?
- What if it goes from complete JSON → different complete JSON (semantically different)?
- What about partial streaming with key reordering mid-stream?
- Empty object {} reordering scenarios?

Recommended additions:

// Test that verifies final args are semantically correct
it("should preserve argument values when keys are reordered", () => {
  // ... setup ...
  // Assert that controller received args with correct values
  expect(onResult).toHaveBeenCalledWith(
    expect.objectContaining({
      // verify the parsed args match expected values
    })
  );
});

// Test incomplete -> complete with reordering
it("should handle streaming with final reordering", () => {
  // Stream: '{"name":"X"' -> '{"type":"Y","name":"X"}'
});

4. Test Data Doesn't Match Real-World Scenario

The test shows:

Initial: '{"name":"Test Operation Alpha","type":"equity"' (incomplete)
Final: '{"name":"Test Operation Alpha","surface":150,"type":"equity"}' (complete)

This isn't actually a reordering scenario! The final state has an additional field (surface: 150) that wasn't in the initial partial string. The test is passing but not testing what the PR description claims.

According to issue #2613 comments, the real scenario was:

Initial: {"name":"Test Operation Alpha","surface":150,"type":"equity"}
Final:   {"name":"Test Operation Alpha","type":"equity","surface":150}

The test should be updated to match this actual reordering case.

Security & Performance

5. Multiple JSON.parse() Calls

The isArgsTextComplete helper is called 4 times per update in lines 173, 174, 190, and potentially again in the close check. Each call does JSON.parse() on the same string. For large argument objects or high-frequency updates, this could impact performance.

Suggestion: Cache the parse results:

let lastParsed: unknown = null;
let currentParsed: unknown = null;

const isLastComplete = isArgsTextComplete(lastState.argsText);
if (isLastComplete) lastParsed = JSON.parse(lastState.argsText);

const isCurrentComplete = isArgsTextComplete(content.argsText);
if (isCurrentComplete) currentParsed = JSON.parse(content.argsText);

6. No Validation of JSON Structure

isArgsTextComplete accepts any valid JSON, including primitives like "hello", 123, null, etc. Should tool arguments always be objects? If so, add validation:

const isArgsTextComplete = (argsText: string) => {
  try {
    const parsed = JSON.parse(argsText);
    return typeof parsed === 'object' && parsed !== null;
  } catch {
    return false;
  }
};

Code Quality

7. Unclear Variable Naming

Lines 195-199: The state update uses spread operator but the intent isn't immediately clear:

lastToolStates.current[content.toolCallId] = {
  ...lastState,
  argsText: content.argsText,
  argsComplete: shouldClose || lastState.argsComplete,
};

This overwrites argsText and argsComplete while preserving other fields. Consider adding a comment explaining why the spread is necessary.

8. Dependency Array Completeness

The useEffect on line 236 depends on state, controller, and onResult, but also internally uses setToolStatuses (from props). While setToolStatuses is likely stable, it should either be in the dependency array or documented why it's excluded.

Good Practices Observed

Helper function extraction (isArgsTextComplete) for readability
Preservation of development warning for debugging
Non-breaking approach to fixing the issue
Regression test added
Proper test environment configuration (@vitest-environment jsdom)

Recommendations Summary

Must fix before merge:

⚠️ Add semantic equivalence check for complete JSON comparison (Critical Issue #1)
⚠️ Add changeset for version management (Critical Issue #2)
⚠️ Fix test to actually test reordering scenario (Issue #4)

Should fix: 4. Add test coverage for final args verification (Issue #3) 5. Optimize JSON.parse calls (Issue #5)

Nice to have: 6. Add JSON object type validation (Issue #6) 7. Improve code documentation (Issues #7, #8)

Additional Context

Issue #2613 reveals this primarily affects Gemini models, suggesting the AI SDK or Gemini's streaming behavior causes key reordering during serialization. The fix approach is sound, but the semantic equivalence check (Issue #1) is critical to ensure we're not masking other bugs where arguments legitimately change.

Nov 19 '25 18:11 github-actions[bot]

Claude finished @ShobhitPatra's task —— View job

PR Review: Fix for Reordered Tool Call Arguments

I've analyzed the changes in this PR along with the previous bot reviews. While the intention to fix the reordering issue is good, there are critical issues that need to be addressed before this can be merged.

Critical Issues Found

1. The fix is unreachable in the actual bug scenario (packages/react/src/legacy-runtime/runtime-cores/assistant-transport/useToolInvocations.ts:176-203)

The core issue: The new reordering logic at lines 176-203 can never be reached when the actual bug occurs. Here's why:

Current flow:

if (content.argsText !== lastState.argsText) {
  if (lastState.argsComplete) {  // Line 165
    // Warning branch - executes when argsComplete is true
    console.warn("argsText updated after controller was closed");
  } else {
    // Your new fix is here (lines 173-224)
    const isLastComplete = isArgsTextComplete(lastState.argsText);
    const isCurrentComplete = isArgsTextComplete(content.argsText);
    
    if (isLastComplete && isCurrentComplete) {
      // This branch tries to handle reordering
      // ...
    }
  }
}

The problem: When tool arguments are complete and then get reordered:

lastState.argsComplete is already true (set at line 223 on the previous render)
The code takes the first branch (line 165) and just warns
Your reordering fix at line 176 never executes because it's inside the else block

What happens in the actual bug:

First render: Stream completes with {"name":"Alpha","type":"equity","surface":150}
argsComplete is set to true
Second render: Backend sends reordered JSON {"name":"Alpha","surface":150,"type":"equity"}
Code hits line 165, sees argsComplete === true, logs warning, and exits
Bug still occurs (the startsWith error should have been thrown, but now it just warns and ignores the update)

The fix needs to be moved to line 165 where it checks lastState.argsComplete.

2. Test doesn't actually test the bug scenario (packages/react/src/legacy-runtime/runtime-cores/assistant-transport/useToolInvocations.test.tsx:15-43)

The test has a fundamental flaw in how it sets up the scenario:

const initialState: AssistantTransportState = {
  messages: [{
    content: [{
      type: "tool-call",
      toolCallId: "tool123",
      argsText: '{"name":"Test Operation Alpha","type":"equity","surface":150}'
    }]
  }]
};

The problem:

The hook initializes with the tool call already present in the state
Line 143-144 of useToolInvocations.ts shows: if (isInititialState.current) { ignoredToolIds.current.add(content.toolCallId); }
This means all tool calls present on initial render are ignored
The rerender never triggers the argsText comparison logic
The test will always pass, even without the fix

To properly test the bug, you need:

Start with an empty state (no tool calls)
First rerender: Add a tool call with incomplete args streaming in
Continue streaming until complete: {"name":"Alpha","type":"equity","surface":150}
Second rerender: Send the same data but with reordered keys: {"name":"Alpha","surface":150,"type":"equity"}

This simulates the actual streaming behavior where the bug occurs.

Suggested Improvements

3. Missing logic to properly close controller when both JSONs are complete

At line 201-203, there's a check to close the controller, but it should also ensure we don't attempt to append anything:

if (!lastState.argsComplete) {
  lastState.controller.argsText.close();
}

However, this doesn't update lastState.argsComplete to true, which could cause issues on subsequent renders. The state update at line 220-224 should handle this, but it would be clearer to explicitly set it.

4. Semantic equivalence check is good but could be simpler

The deep recursive key sorting (lines 180-187) is thorough, but for this use case, you could simplify by using a standard approach:

JSON.stringify(lastParsed, Object.keys(lastParsed).sort())

However, your current implementation is more robust for nested objects, so this is optional.

Missing Requirements

5. No changeset included

The changeset bot correctly identified that this PR modifies source code in packages/react but doesn't include a changeset. You need to:

Run pnpm changeset
Select @assistant-ui/react as the package
Choose patch as the version bump type
Add description: fix: tolerate reordered tool call arguments in useToolInvocations

Good Practices Observed

Added isArgsTextComplete helper function for JSON validation
Semantic equivalence checking to distinguish reordering from actual changes
Development-only console warnings for debugging
Added comprehensive inline comments explaining the logic

Recommendations

Before merging, you must:

Fix the unreachable code issue by moving the reordering check to handle the case where lastState.argsComplete === true
Rewrite the test to properly simulate the streaming scenario (start empty, stream incomplete → complete, then receive reordered)
Add a changeset for the version bump
Verify the fix by testing against the actual Gemini model scenario from issue #2613

The current implementation won't fix the reported bug. I recommend restructuring the logic to check for reordering before checking argsComplete, or as part of that check.

Nov 19 '25 19:11 github-actions[bot]

Claude finished @ShobhitPatra's task —— View job

Code Review Summary

This PR attempts to fix a crash in useToolInvocations when tool call arguments arrive with reordered JSON keys. Unfortunately, the fix does not work due to unreachable code and an inadequate test.

Critical Issues

1. The reordering fix is unreachable 🔴

Location: useToolInvocations.ts:173-203

The reordering logic is placed inside the else branch (line 172) that only executes when lastState.argsComplete === false. However, when reordering actually occurs:

First update: Complete JSON arrives → argsComplete is set to true (line 223)
Second update: Reordered JSON arrives → Code hits the early return at line 165 because argsComplete === true
Result: The reordering check never runs, and the bug persists

Why this happens: The conditional structure is:

if (content.argsText !== lastState.argsText) {
  if (lastState.argsComplete) {  // ← Takes this branch for reordering
    console.warn(...);
    return;  // ← Early exit, never reaches reordering logic below
  } else {
    // Reordering check is here ← Never reached when argsComplete is true
  }
}

Solution: Move the reordering check before the argsComplete check so it can handle complete → reordered complete transitions.

2. Test doesn't trigger the bug scenario 🔴

Location: useToolInvocations.test.tsx:27-51

The test provides complete JSON in the first rerender (line 39-40), which means:

The tool call is processed with argsComplete immediately set to true
The second rerender (reordered) hits the early return at line 165
The argsText comparison logic never executes
The test passes regardless of whether the fix works

A proper test needs three stages:

Empty state (initial render)
Incomplete streaming JSON
Complete JSON (original order)
Complete JSON (reordered) ← This is where the bug actually occurs

3. Missing changeset 🟡

This PR modifies source code in packages/react (a published package) but doesn't include a changeset. Per repository requirements, a changeset is needed for version bumping.

Suggested Improvements

Code structure improvement

The current implementation has the semantic equivalence check (lines 180-196) but doesn't update the state after validating reordering. The state update should happen after the semantic check passes.

Consider edge cases

What happens if the JSON keys are reordered multiple times?
Should the controller be closed if it's not already closed when both JSONs are complete?

Good Practices Observed

Added isArgsTextComplete helper function for cleaner code
Semantic equivalence check with recursive key sorting prevents false negatives
Development-only warnings don't pollute production logs
Comprehensive dependency additions documented in PR description

Recommendations

Fix the control flow: Move reordering check before argsComplete check
Fix the test: Start with incomplete JSON, then complete, then reordered
Add changeset: Run pnpm changeset add and select @assistant-ui/react with patch bump
Verify the fix: Ensure the test actually fails without the fix and passes with it

Example of proper control flow

if (content.argsText !== lastState.argsText) {
  // 1. Check for reordering FIRST (handles complete → reordered complete)
  const isLastComplete = isArgsTextComplete(lastState.argsText);
  const isCurrentComplete = isArgsTextComplete(content.argsText);
  
  if (isLastComplete && isCurrentComplete) {
    // Handle reordering with semantic check
    // Verify semantic equivalence
    try {
      const sortJsonKeys = (obj: any): any => {
        if (typeof obj !== 'object' || obj === null) return obj;
        if (Array.isArray(obj)) return obj.map(sortJsonKeys);
        return Object.keys(obj).sort().reduce((result, key) => {
          result[key] = sortJsonKeys(obj[key]);
          return result;
        }, {} as any);
      };

      const lastParsed = JSON.parse(lastState.argsText);
      const currentParsed = JSON.parse(content.argsText);

      if (JSON.stringify(sortJsonKeys(lastParsed)) !== JSON.stringify(sortJsonKeys(currentParsed))) {
        if (process.env["NODE_ENV"] !== "production") {
          console.warn("Complete argsText updated with different content:", { 
            previous: lastState.argsText, 
            next: content.argsText 
          });
        }
      }
    } catch (e) {
      // Parsing failed, fall through
    }
    
    // Update state with reordered JSON
    lastToolStates.current[content.toolCallId] = {
      ...lastState,
      argsText: content.argsText,
      argsComplete: true,
    };
    return; // Exit early - nothing to append
  }
  
  // 2. THEN check if already complete (for other updates)
  if (lastState.argsComplete) {
    if (process.env["NODE_ENV"] !== "production") {
      console.warn("argsText updated after controller was closed:", {
        previous: lastState.argsText,
        next: content.argsText,
      });
    }
    return;
  }
  
  // 3. Normal streaming logic
  if (!content.argsText.startsWith(lastState.argsText)) {
    throw new Error(
      `Tool call argsText can only be appended, not updated: ${content.argsText} does not start with ${lastState.argsText}`,
    );
  }
  
  const argsTextDelta = content.argsText.slice(lastState.argsText.length);
  lastState.controller.argsText.append(argsTextDelta);
  
  const shouldClose = isArgsTextComplete(content.argsText);
  if (shouldClose && !lastState.argsComplete) {
    lastState.controller.argsText.close();
  }
  
  lastToolStates.current[content.toolCallId] = {
    ...lastState,
    argsText: content.argsText,
    argsComplete: shouldClose || lastState.argsComplete,
  };
}

Nov 19 '25 19:11 github-actions[bot]

Claude finished @ShobhitPatra's task —— View job

Code Review: Fix for Tool Call Args Reordering

Summary

This PR attempts to fix issue #2613 where useToolInvocations crashes with "Tool call argsText can only be appended, not updated" when complete JSON arguments arrive with reordered keys. While the intention is correct, the implementation has critical flaws that prevent it from working.

Critical Issues

1. The reordering fix is unreachable 🔴

The core fix (lines 168-206 in useToolInvocations.ts) is placed inside the wrong conditional branch, making it impossible to execute when the bug actually occurs.

Execution flow when reordered JSON arrives:

First update: Complete JSON {"name":"X","type":"Y","surface":150} arrives
- Lines 230-237: Sets argsComplete: true and stores the argsText
Second update: Reordered JSON {"name":"X","surface":150,"type":"Y"} arrives
- Line 164: content.argsText !== lastState.argsText ✓ (different strings)
- Line 207: lastState.argsComplete is true → takes this branch
- Lines 209-214: Logs warning and returns early
- Lines 168-206 NEVER execute - the reordering fix is skipped!

The fix needs to check for complete JSON reordering before checking argsComplete.

See inline comment on lines 164-168 for detailed explanation and suggested fix.

2. Test doesn't validate the fix 🔴

The test in useToolInvocations.test.tsx cannot detect whether the fix works because:

It simulates: empty → incomplete JSON → complete JSON → reordered JSON
When the reordered JSON arrives (line 100), argsComplete is already true
The code hits the warning branch (lines 207-214) which doesn't throw - it just logs
Test passes even without any fix

The test needs to be restructured to actually trigger the error path that the original bug hits.

See inline comment on lines 48-72 for detailed analysis.

3. Missing changeset 🟡

This PR modifies source code in packages/react (a published package) but doesn't include a changeset. Per repository conventions, a changeset is required for version bumping.

Action needed: Run pnpm changeset and create a changeset describing this bug fix.

Additional Concerns

Semantic equivalence check may have edge cases

The sortJsonKeys function (lines 171-180) recursively sorts object keys to compare semantic equivalence. While this approach is sound, consider:

Nested objects: The implementation correctly handles nesting ✓
Arrays with objects: Arrays are mapped but not sorted - if array order changes, it would be detected ✓
Special values: null, undefined, NaN - should work correctly ✓
Performance: Deep cloning and sorting on every update could be expensive for large tool args. Consider adding a size check or early comparison before sorting.

Controller state management issue

Lines 204-206 close the controller if not already closed, but this happens after the semantic check. If the JSONs are semantically different (actual content change), the code still closes the controller without appending the new content. This could lead to lost updates.

The logic should probably be:

If semantically equivalent → close controller, update state, done
If semantically different → this is unexpected, maybe throw or handle specially?

Good Practices Observed

Helper function isArgsTextComplete for JSON validation
Comprehensive semantic equivalence checking with deep key sorting
Development-only console warnings to avoid production noise
Updated test starts from empty state (correct approach)

Recommendations

To fix this PR:

Restructure the conditional logic in useToolInvocations.ts:
- Move completeness checks (lines 165-206) to execute BEFORE the lastState.argsComplete check
- Ensure the reordering path executes when both strings are complete JSON
Fix the test to properly validate the scenario:
- The test should verify that when two complete JSONs with the same content but different key order arrive consecutively, no error is thrown
- Consider testing the actual error message from the original bug

Add a changeset:

pnpm changeset
# Select @assistant-ui/react
# Choose "patch" (bug fix)
# Describe: "fix: tolerate reordered tool call arguments"

Consider adding integration test with actual streaming behavior to catch this type of issue earlier

Test Coverage Assessment

❌ Current test doesn't validate the fix - passes even with the bug present
✅ Test structure (empty → incomplete → complete → reordered) is correct in principle
❌ Missing test for the actual error condition from original issue
❌ No test for semantic difference detection (what happens when content actually changes?)

Nov 19 '25 19:11 github-actions[bot]

Claude finished @ShobhitPatra's task —— View job

Code Review Complete

This PR fixes a crash in useToolInvocations when tool call arguments arrive as complete JSON with reordered keys. While the core approach is sound, there are several critical issues that need to be addressed.

Summary of Changes

Fixed Files:

useToolInvocations.ts: Added logic to detect complete JSON objects and handle key reordering
useToolInvocations.test.tsx: New comprehensive test suite (383 lines)
package.json & packages/react/package.json: Added required dependencies

CI Status: Code Quality check shows action required (likely due to missing changeset)

Critical Issues

1. Missing Changeset 🚨

The PR modifies source code in packages/react but lacks a changeset. This is required for version management.

Action Required:

npx changeset add
# Select: @assistant-ui/react
# Version bump: patch (this is a bug fix)
# Description: fix(react): tolerate reordered tool call arguments

2. State Update Missing in Non-Equivalent JSON Path 🚨

Location: useToolInvocations.ts:191-218

When two complete JSONs are semantically different (not just reordered), the code logs a warning but never updates the state. This means the old arguments persist even when new arguments arrive.

Current flow:

Lines 187-190: Check if JSONs are semantically equivalent
Lines 191-201: If NOT equivalent → log warning → no state update
Lines 209-218: If equivalent → close controller → update state ✓

Problem: The non-equivalent path exits without updating lastToolStates.current[content.toolCallId], leaving stale data.

Fix: After line 201, add state update:

if (!areSemanticallySame) {
  if (process.env["NODE_ENV"] !== "production") {
    console.warn(/* ... */);
  }
  // Add state update here:
  lastToolStates.current[content.toolCallId] = {
    ...lastState,
    argsText: content.argsText,
    argsComplete: true,
  };
}

Or better yet, restructure to update state regardless of equivalence (after line 207).

3. Deep Recursion Risk in sortJsonKeys ⚠️

Location: useToolInvocations.ts:173-182

The sortJsonKeys function recursively processes nested objects without depth limits. For deeply nested JSON (e.g., 100+ levels), this could cause stack overflow.

Suggested safeguard:

const sortJsonKeys = (obj: any, depth = 0): any => {
  if (depth > 100) throw new Error("Max depth exceeded");
  if (typeof obj !== "object" || obj === null) return obj;
  if (Array.isArray(obj)) return obj.map(item => sortJsonKeys(item, depth + 1));
  return Object.keys(obj)
    .sort()
    .reduce((result, key) => {
      result[key] = sortJsonKeys(obj[key], depth + 1);
      return result;
    }, {} as any);
};

Test Coverage Issues

4. Missing Behavioral Verification

Location: useToolInvocations.test.tsx:180-239

The test "should handle complete JSON with different content" only checks that it doesn't throw. It doesn't verify:

Which arguments are actually used (100 or 200?)
Whether the warning was logged
What the final state contains

This makes it unclear if the behavior is correct or just "doesn't crash."

5. Edge Cases Not Tested

Missing test scenarios:

Empty object reordering: {} → {}
Nested object reordering: {"a":{"x":1,"y":2}} → {"a":{"y":2,"x":1}}
Array order preservation: {"arr":[1,2,3]} should NOT equal {"arr":[3,2,1]}
Multiple sequential reorderings
Reordering mid-stream (incomplete → reordered incomplete → complete)

Implementation Concerns

6. Unclear Design Decision

Location: useToolInvocations.ts:169-218

The behavior when complete JSONs have different content (not just reordered) is ambiguous:

Should we accept the new value and log a warning? (current approach)
Should we reject it and throw an error?
Should we ignore it and keep the old value?

The current implementation logs a warning but the state update is missing (see Critical Issue #2), so it's unclear what the intended behavior is. This should be documented or clarified.

Security & Performance

7. JSON Parsing Performance

Location: useToolInvocations.ts:184-189

Every time argsText changes, the code parses both JSONs twice:

Once in isArgsTextComplete (lines 165-166)
Again for semantic comparison (lines 184-185)

For large JSON arguments during streaming, this could impact performance.

Optimization:

// Parse once and reuse
let lastParsed, currentParsed;
try {
  lastParsed = JSON.parse(lastState.argsText);
  currentParsed = JSON.parse(content.argsText);
  const isLastComplete = true;
  const isCurrentComplete = true;
  // ... rest of logic
} catch {
  // At least one is incomplete, use startsWith check
}

Dependency Changes

8. Dependencies Look Good ✓

@testing-library/react@^16.3.0: Appropriate for testing hooks
@tsconfig/strictest@^2.0.8: Correct missing peer dependency
Both additions are well-justified in the PR description

The pnpm-lock.yaml changes look clean (mostly de-duplication).

Good Practices Observed

Comprehensive test suite with multiple scenarios
Proper use of process.env["NODE_ENV"] guards for dev-only warnings
Semantic equivalence check using normalized JSON (sortJsonKeys)
Console spy properly set up/torn down in tests
Tests start from empty state to properly simulate streaming

Recommendations

Must Fix:

Add changeset for version management
Fix state update in non-equivalent JSON path (Critical Issue #2)
Add depth limit to sortJsonKeys recursion

Should Fix: 4. Add behavioral assertions to test #3 (verify which args are used) 5. Optimize JSON parsing to avoid duplicate work 6. Document intended behavior for semantically different complete JSONs

Nice to Have: 7. Add edge case tests (nested objects, arrays, etc.) 8. Consider caching parsed JSON if performance becomes an issue

Overall Assessment

The fix correctly addresses the core issue (reordered JSON keys causing crashes), and the test coverage is extensive. However, the incomplete state update path (Critical Issue #2) is a significant bug that must be fixed before merging. Once addressed, this will be a solid improvement to the robustness of the tool invocations system.

Nov 20 '25 11:11 github-actions[bot]

Claude finished @ShobhitPatra's task —— View job

Code Review Complete

Summary

This PR attempts to fix the issue where useToolInvocations crashes when tool call arguments arrive with reordered JSON keys. While the intention is correct, the implementation has a critical logic flaw that prevents the fix from working as intended.

Critical Issues

1. The reordering fix is unreachable in the actual bug scenario 🔴

Location: packages/react/src/legacy-runtime/runtime-cores/assistant-transport/useToolInvocations.ts:164-265

The reordering logic at lines 188-235 cannot execute when the actual bug occurs. Here's why:

Execution flow when reordered JSON arrives:

First render: Complete JSON arrives (e.g., {"name":"X","type":"Y","surface":150})
- Goes through normal streaming path (line 244-264)
- lastState.argsComplete is set to true at line 263
Second render: Same JSON with reordered keys arrives (e.g., {"name":"X","surface":150,"type":"Y"})
- Line 164: content.argsText !== lastState.argsText ✓ (condition met)
- Line 236: lastState.argsComplete is true (from step 1)
- Result: Takes the else if (lastState.argsComplete) branch at line 236-242 and logs a warning
- Lines 188-235 never execute - the reordering fix is bypassed!

Why this happens: The code checks lastState.argsComplete (line 236) BEFORE checking for semantic equivalence (lines 188-235). Once args are marked complete on the first render, subsequent updates with reordered keys hit the warning path instead of the reordering detection path.

Suggested fix: The semantic equivalence check must happen BEFORE the argsComplete check. See inline comment for detailed code suggestion.

2. Tests don't verify the actual bug scenario 🔴

Location: packages/react/src/legacy-runtime/runtime-cores/assistant-transport/useToolInvocations.test.tsx

All tests follow this pattern:

Start with empty state
Stream incomplete JSON
Complete the JSON
Send reordered JSON

However, due to issue #1 above, when step 4 executes:

lastState.argsComplete is already true from step 3
The code takes the warning branch (line 236-242 in implementation)
The tests pass because the warning branch doesn't throw - it just logs to console
The reordering detection logic (lines 188-235) is never tested!

The tests are passing for the wrong reason - they're exercising the warning code path, not the reordering fix.

3. Missing changeset 🔴

Issue: The changeset-bot correctly identified that this PR lacks a changeset file.

Impact: This fix won't trigger a version bump when merged, which is required for published packages in this monorepo.

Fix: Run npx changeset to add a patch-level changeset for @assistant-ui/react.

Additional Issues

4. Potential memory issue in semantic comparison

Location: useToolInvocations.ts:190-202

The sortJsonKeys function has a depth limit of 100, but the error handling doesn't update state properly:

if (depth > 100)
  throw new Error("Max depth exceeded in sortJsonKeys");

If this throws, the catch block at line 218-225 will log a warning, but the state won't be updated. This could cause subsequent renders to repeatedly attempt the same comparison.

Suggestion: Consider using a library like fast-json-stable-stringify or json-stable-stringify instead of the custom sortJsonKeys implementation.

5. Incomplete state update in non-equivalent case

Location: useToolInvocations.ts:204-217

When two complete JSONs are semantically different, the code logs a warning but doesn't update the state:

if (JSON.stringify(sortJsonKeys(lastParsed)) !== JSON.stringify(sortJsonKeys(currentParsed))) {
  if (process.env["NODE_ENV"] !== "production") {
    console.warn("Complete argsText updated with different content:", {...});
  }
}
// Missing: state update for the new argsText

The state update only happens after this check (line 227-235), meaning if content actually changes (not just reorders), the warning is logged but nothing else happens. Is this intended behavior?

Good Practices Observed

Well-structured test file with multiple scenarios
Environment-specific console warnings for debugging
Depth limit protection in recursive function
Proper use of vitest mocking for console.warn

Test Coverage Analysis

Current test scenarios

✅ Basic reordering with incomplete → complete → reordered
✅ Invalid update detection (incomplete JSON changed, not appended)
✅ Complete JSON with different content
✅ Normal streaming progression
✅ Comprehensive reordering test

All tests are well-written but don't actually exercise the reordering fix due to issue #1.

Recommendations

High Priority

Restructure the logic flow in useToolInvocations.ts to check for semantic equivalence BEFORE checking argsComplete
Add a changeset for this fix
Verify tests actually exercise the fix after restructuring the logic

Medium Priority

Consider using a stable JSON stringify library instead of custom sorting
Clarify intended behavior when complete JSONs have different content

Low Priority

Add inline code comments explaining the reordering scenario for future maintainers

Verdict

This PR cannot be merged as-is. The core fix is architecturally sound (checking semantic equivalence), but the execution order prevents it from working. The bug described in #2613 will still occur with this implementation.

Nov 20 '25 12:11 github-actions[bot]

Claude finished @ShobhitPatra's task —— View job

Code Review Complete

Summary

This PR fixes a crash in useToolInvocations when tool call arguments arrive with reordered JSON keys. The implementation is functionally correct and solves the reported issue. However, there are some improvements and clarifications needed.

Key Finding: Despite what several bot reviews claimed, the fix logic IS reachable and DOES work correctly. The check for both complete JSONs (line 188) happens before the argsComplete check (line 236), so reordered JSON is properly handled.

Critical Issues

1. Missing Changeset ⚠️

The PR modifies source code in packages/react but has no changeset. This fix should trigger a patch version bump.

Action needed: Add a changeset with:

npx changeset add
# Select @assistant-ui/react
# Select patch
# Description: "fix: tolerate reordered tool call arguments"

2. Overly Permissive Complete JSON Handling

When both JSONs are complete but have different content (not just reordering), the code updates the state and only logs a warning. This silently accepts scenarios like:

First: {"value": 100}
Second: {"value": 200} (different value, not just reordered)

Question: Is this intended behavior? Should complete arguments be allowed to change after being marked complete?

See inline comment at useToolInvocations.ts:217

3. Unnecessary `startsWith("{")` Checks

Lines 171 and 180 check if the JSON starts with { before parsing. This is redundant (JSON.parse handles this) and excludes valid JSON arrays starting with [.

Impact: Tool arguments that are arrays won't be recognized as complete JSON.

See inline comment at useToolInvocations.ts:186

Suggested Improvements

Performance Concern

The sortJsonKeys recursive function (lines 190-202) processes the entire object tree on every reordering check. For large/deeply nested tool arguments, this could impact performance.

Suggestions:

Memoize sorted results
Consider iterative approach
Document the 100-depth limit

See inline comment at useToolInvocations.ts:191

Test Coverage Analysis

The test suite is comprehensive and correctly structured:

✅ Good practices observed:

Tests start with empty state to avoid ignoredToolIds issue
Proper streaming sequence: incomplete → complete → reordered
Multiple scenarios covered (valid reordering, invalid updates, different content)
Edge cases tested (streaming progression, incomplete JSON modification)

Test scenarios covered:

✅ Reordered JSON (semantically equivalent) - doesn't throw
✅ Modified incomplete JSON - throws error
✅ Complete JSON with different content - handles gracefully
✅ Progressive streaming - works correctly
✅ Realistic reordering scenario - matches issue #2613

Dependency Changes

✅ Justified additions:

@testing-library/react - Standard for React hook testing
@tsconfig/strictest - Required peer dependency

Both additions are appropriate and align with monorepo patterns.

Security & Best Practices

✅ No security issues identified ✅ Error handling is appropriate ✅ State management follows React patterns ✅ Type safety maintained

Addressing Bot Review Comments

Several bot reviews incorrectly claimed the fix is "unreachable." This is incorrect.

Actual logic flow when reordering occurs:

Complete JSON arrives: '{"name":"X","type":"Y"}' → sets argsComplete: true
Reordered JSON arrives: '{"type":"Y","name":"X"}'
- Line 164: Strings differ ✓
- Lines 170-186: Both parse successfully ✓
- Line 188: isLastComplete && isCurrentComplete = true ✓
- Enters reordering fix block before checking lastState.argsComplete ✓

The fix works because the completeness check (line 188) executes before the argsComplete check (line 236).

Action Items

Required before merge:

[ ] Add changeset for version bump
[ ] Address or clarify the complete JSON content-change behavior
[ ] Fix startsWith("{") check to support JSON arrays

Recommended:

[ ] Consider performance optimization for sortJsonKeys
[ ] Add inline documentation explaining the reordering logic

Nov 20 '25 12:11 github-actions[bot]

Claude finished @ShobhitPatra's task —— View job

Code Review Complete

I've analyzed this PR which fixes a critical crash in useToolInvocations when tool arguments arrive with reordered JSON keys. Based on the implementation, tests, CI status, and issue context, here's my comprehensive review.

Summary of Changes Since Previous Reviews

The implementation has been significantly improved since earlier bot reviews. The code flow now correctly handles the reordering scenario:

Lines 164-187: Checks if both old and new argsText are complete JSON before checking argsComplete
Lines 188-235: Handles complete JSON reordering with semantic equivalence checking
Lines 236-242: Falls back to the argsComplete check only for non-complete JSON scenarios
Lines 243-265: Normal streaming logic with startsWith validation

Previous bot reviews incorrectly claimed the fix was unreachable - this is no longer true. The logic flow is now correct.

Critical Issues

1. Missing Changeset 🔴

The PR modifies source code in packages/react but doesn't include a changeset. This is required for version management per repository conventions.

Action required:

npx changeset add
# Select: @assistant-ui/react  
# Bump: patch (bug fix)
# Description: fix(react): tolerate reordered tool call arguments

Issues Requiring Attention

2. Incomplete handling of semantic differences 🟡

Location: useToolInvocations.ts:204-217

When both JSONs are complete but semantically different (actual content change, not reordering), the code logs a warning but doesn't update the state. This could cause stale arguments to persist.

Current behavior:

if (JSON.stringify(sortJsonKeys(lastParsed)) !== JSON.stringify(sortJsonKeys(currentParsed))) {
  console.warn("Complete argsText updated with different content:", {...});
  // No state update here - falls through to line 227-235
}

The state update at lines 231-235 happens regardless, so this might actually work correctly. However, the control flow is unclear. Consider restructuring for clarity:

if (isLastComplete && isCurrentComplete) {
  const areEquivalent = /* semantic check */;
  if (!areEquivalent) {
    console.warn(/* ... */);
  }
  // Always update state for complete JSONs (reordered or different)
  if (!lastState.argsComplete) {
    lastState.controller.argsText.close();
  }
  lastToolStates.current[content.toolCallId] = {
    ...lastState,
    argsText: content.argsText,
    argsComplete: true,
  };
  return; // Early return to make flow explicit
}

3. Potential performance issue with deep recursion 🟡

Location: useToolInvocations.ts:190-202

The sortJsonKeys function recursively processes objects with a depth limit of 100. While the limit is good, consider:

Large tool arguments will be parsed and sorted on every render
No memoization means repeated work for the same data

Suggestions:

Add depth limit parameter documentation
Consider caching parsed results if argsText hasn't changed
Alternatively, use iterative sorting for better performance

4. Edge case: Array arguments not handled 🟡

Location: useToolInvocations.ts:171, 180

The code checks if (lastState.argsText.trim().startsWith("{")) before parsing, which excludes valid JSON arrays like ["item1", "item2"]. While tool arguments are typically objects, the code should handle arrays consistently.

Current issue: If a tool's arguments are an array, the completeness check would fail even for valid JSON.

Fix:

try {
  lastParsed = JSON.parse(lastState.argsText);
  isLastComplete = true;
} catch (e) {
  // Is not complete JSON
}
// Remove the startsWith("{") check

Test Coverage Assessment

5. Excellent test coverage ✅

The test suite is comprehensive and well-structured:

✅ Tests reordering scenario (lines 20-115, 289-382)
✅ Tests incomplete JSON modification detection (lines 117-178)
✅ Tests different complete JSON content (lines 180-239)
✅ Tests normal streaming flow (lines 241-287)
✅ Starts from empty state to avoid ignoredToolIds issue
✅ Properly simulates streaming: empty → incomplete → complete → reordered

6. Minor test improvement suggestions 💡

Additional edge cases to consider:

Nested object reordering

it("should handle nested object reordering", () => {
  // Test: {"user":{"name":"X","age":30}} → {"user":{"age":30,"name":"X"}}
});

Empty object scenario

it("should handle empty object reordering", () => {
  // Test: {} → {} (should work without issues)
});

Array order preservation

it("should detect when array order changes", () => {
  // Arrays are not reordering - [1,2,3] vs [3,2,1] should be different
});

Good Practices Observed

✅ Helper function isArgsTextComplete for clean code organization
✅ Comprehensive semantic equivalence checking with recursive key sorting
✅ Development-only warnings to avoid production noise
✅ Proper test environment configuration
✅ Thorough test suite covering multiple scenarios
✅ Good inline comments explaining complex logic
✅ Depth limit protection in recursive function

Security & Code Quality

No security issues identified ✅

The code:

Properly validates JSON parsing with try/catch
Has depth limits to prevent stack overflow
Only logs to console in development mode
Doesn't expose sensitive data

Code organization is good ✅

The logic flow is now correct and handles all scenarios properly.

Recommendations

Must fix before merge:

✅ Fix is correct and working (contrary to earlier bot reviews)
🔴 Add changeset for version management

Should consider:

🟡 Add early return after handling complete JSON reordering (clarity)
🟡 Remove startsWith("{") check to support array arguments
🟡 Add performance optimization for repeated JSON parsing

Nice to have:

💡 Add test coverage for nested reordering and edge cases
💡 Document the depth limit in sortJsonKeys

Validation Against Issue #2613

The fix correctly addresses the reported issue:

Original error:

Tool call argsText can only be appended, not updated: 
{"name":"Test Operation Alpha","surface":150,"type":"equity"} does not start with 
{"name":"Test Operation Alpha","type":"equity","surface":150}

Root cause: JSON keys reordered during streaming (particularly with Gemini models)

Fix: Lines 164-235 now detect complete JSON objects and use semantic equivalence checking instead of strict startsWith validation

Test validation: Tests at lines 20-115 and 289-382 properly simulate the exact scenario from the issue

CI Status

Code Quality: Action required (likely due to missing changeset)
Implementation: Correct and functional
Tests: Comprehensive and passing

Final Assessment

This is a solid fix that correctly addresses the reported bug. The implementation is sound, tests are comprehensive, and the approach is robust. The only blocker is the missing changeset.

Status: ✅ Ready to merge after adding changeset

Nov 20 '25 12:11 github-actions[bot]

Claude finished @ShobhitPatra's task —— View job

PR Review: fix(react): tolerate reordered tool call arguments

Summary

This PR addresses issue #2613 where useToolInvocations crashed with "Tool call argsText can only be appended, not updated" when tool call argument JSON keys were reordered during streaming. The latest commit (6dd1ca6) has addressed most of the critical issues raised in previous reviews.

Status of previous review issues:

✅ RESOLVED: The reordering logic is now correctly placed BEFORE the argsComplete check (lines 185-237), making it reachable
✅ RESOLVED: Semantic equivalence check with deep key sorting has been implemented (lines 189-226)
✅ RESOLVED: Tests now properly exercise the bug scenario with incomplete→complete→reordered sequence
⚠️ NEEDS CHANGESET: Missing changeset file for version bump

Critical Issues

1. Missing Changeset ⚠️

Severity: Blocking

The changeset-bot has flagged that this PR modifies source code in packages/react but lacks a changeset. This is required for version management in the monorepo.

Action required:

npx changeset add
# Select: @assistant-ui/react
# Type: patch (this is a bug fix)
# Description: "fix(react): tolerate reordered tool call arguments"

Code Quality Issues

2. State Update in Semantic Equivalence Branch

Location: useToolInvocations.ts:232-236 (inline comment)

When both JSONs are complete and semantically equivalent (just reordered), the code correctly updates state with the new argsText. However, when they're semantically different (lines 210-220), only a warning is logged but state is still updated (lines 232-236 execute regardless).

Current behavior: If complete JSON legitimately changes from {"value":100} to {"value":200}, the code:

Logs a warning (good)
Updates state with new argsText (may or may not be desired)
Does not throw an error

Question: Is this the intended behavior? Should semantically different complete JSON updates be:

Accepted silently (current behavior)
Accepted with warning only (current behavior)
Rejected with an error (more strict)

The current implementation treats all complete→complete transitions the same way regardless of semantic equivalence, which may mask unexpected state changes.

3. Controller Close Timing

Location: useToolInvocations.ts:228-230

if (!lastState.argsComplete) {
  lastState.controller.argsText.close();
}

This correctly checks if the controller is already closed before closing it. However, the check happens inside the "both complete" branch, which means it's checking whether the previous state was complete, not whether the controller was actually closed. While these should align in practice, there's a subtle semantic mismatch.

Recommendation: This is working correctly but could be made more explicit with a comment explaining that argsComplete tracks whether the controller was closed.

4. Performance Consideration: Deep Recursion

Location: useToolInvocations.ts:189-204

The sortJsonKeys function recursively sorts nested objects up to 100 levels deep. This runs on every rerender where both JSONs are complete and different.

Concerns:

For large/deep tool argument objects, this could be expensive
The depth limit (100) is arbitrary and undocumented
No memoization means the same JSON gets re-sorted on every rerender

Recommendations:

Consider caching/memoizing sorted results if performance becomes an issue
Document the depth limit in comments
Consider using an iterative approach if deeply nested args are common

However, this is likely premature optimization for typical use cases. Tool arguments are usually small objects.

Test Coverage Assessment

Strengths ✅

The test suite is comprehensive and well-structured:

Core bug scenario: Test at line 20 properly simulates incomplete→complete→reordered sequence
Invalid updates: Test at line 117 verifies that truly invalid updates still throw
Different content: Test at line 180 covers complete→different complete scenario
Progressive streaming: Test at line 241 validates incremental streaming
Nested objects: Tests at lines 289 and 384 cover nested object reordering
Edge cases: Multiple reordering variations tested

Gaps/Suggestions

Missing verification of actual behavior: Tests only verify "no throw" but don't assert:
- What values end up in the tool invocation?
- Is onResult called with correct arguments?
- Is the warning actually logged when expected?

Example enhancement

it("should log warning for semantically different complete JSON", () => {
  // ... setup ...
  rerender({ state: firstCompleteState });
  rerender({ state: differentCompleteState });
  
  expect(consoleWarnSpy).toHaveBeenCalledWith(
    "Complete argsText updated with different content:",
    expect.objectContaining({
      previous: expect.stringContaining('"value":100'),
      next: expect.stringContaining('"value":200')
    })
  );
});

Array reordering: No test for JSON arrays as root (e.g., [{"a":1},{"b":2}]), though tool args are typically objects
Edge case - empty objects: No test for {} → {} scenario

These are nice-to-haves rather than blockers. The current test coverage is solid.

Security & Performance

Security ✅

No injection vulnerabilities
JSON parsing is safely wrapped in try-catch
Depth limit prevents infinite recursion attacks
No untrusted code execution

Performance ⚠️

Deep recursion in sortJsonKeys (see issue #4 above)
Generally acceptable for typical tool argument sizes
Could be optimized if needed in the future

Good Practices Observed

Reuse of parsed JSON objects to avoid redundant parsing
Proper error boundaries with try-catch
Development-only warnings that won't pollute production logs
Comprehensive test coverage with multiple edge cases
Clear comments explaining logic flow

Recommendations Summary

Must fix before merge:

⚠️ Add changeset (see issue #1)

Should consider: 2. Clarify intended behavior for semantically different complete JSON (issue #2) 3. Add test assertions for warning logs and final argument values 4. Document the 100-level depth limit for sortJsonKeys

Nice to have: 5. Performance optimization for deep object sorting (if needed) 6. Additional test coverage for array args and empty objects

Verdict

The implementation correctly solves the reordering bug and has been significantly improved from earlier versions based on bot feedback. The logic flow is now correct, semantic equivalence checking is robust, and tests are comprehensive.

Blocking issue: Missing changeset file

Once the changeset is added, this PR will be ready to merge. The other suggestions are minor improvements that can be addressed now or in follow-up work.

Nov 24 '25 16:11 github-actions[bot]