Fix nesting

[muukii]

No description provided.

[claude]

Claude finished @muukii's task —— View job

---

Code Review Feedback ✅

Todo List

• Read and analyze the modified file
• Review code changes for bugs and issues
• Check performance and security considerations
• Assess test coverage needs
• Run build and tests to verify changes
• Provide comprehensive review feedback

Summary

This PR fixes a critical bug in the nesting functionality of withGraphTracking. The fix ensures that nested tracking operations (withGraphTrackingMap and withGraphTrackingGroup) properly register with their parent scope.

Analysis

What Changed:

// Before:
let subscriptions = ThreadLocal.subscriptions.withValue(.init()) {
  scope()
  return ThreadLocal.subscriptions.value!
}

// After:  
let subscriptions = ThreadLocal.subscriptions.withValue(.init()) {
  ThreadLocal.currentCancellable.withValue(nil) {
    scope()
  }
  return ThreadLocal.subscriptions.value!
}

Root Cause Fixed:
Without this change, withGraphTracking wasn't setting ThreadLocal.currentCancellable to nil, causing nested tracking functions to incorrectly think they had a parent cancellable when they should register with the root subscriptions collection.

Review Assessment

✅ Code Quality & Best Practices

• Excellent: Follows established patterns from withGraphTrackingMap.swift:162 and withGraphTrackingGroup.swift:114
• Consistent: Maintains the same ThreadLocal management style used throughout the codebase
• Minimal: Small, focused fix that doesn't introduce unnecessary complexity

✅ Bug Fixes

• Critical Fix: Resolves incorrect parent-child relationship setup in nested scenarios
• Behavior Alignment: Now matches documented behavior in extensive test suite (NestedGraphTrackingTests.swift)
• Registration Logic: Ensures proper fallback to root subscriptions when no parent exists

✅ Performance

• Zero Impact: Only adds proper scope management, no runtime overhead
• Memory Safety: Prevents potential subscription leaks through correct parent-child relationships

✅ Security

• Thread Safety: Uses existing ThreadLocal infrastructure correctly
• No Security Concerns: Pure internal state management change

✅ Test Coverage

• Comprehensive: Existing test suite in NestedGraphTrackingTests.swift covers this scenario extensively (28 test cases)
• Validation: Tests verify proper cancellation, nesting behavior, and conditional tracking
• Coverage: Tests include 3+ level nesting, conditional branches, and lifecycle management

Recommendation: APPROVE ✅

This is a well-crafted fix that resolves a critical nesting bug. The change aligns perfectly with existing patterns and is thoroughly covered by the test suite.

Files Reviewed:

• Sources/StateGraph/Observation/withGraphTracking.swift:60 - The fix
• Related pattern usage in tracking map/group implementations
• Comprehensive test coverage analysis