Claude finished @thomhurst's task —— View job

PR Review: Global After/AfterEvery hooks not executing ✅

Todo List:

• Read and analyze the TestExecutor.cs changes
• Review the new test file for coverage adequacy
• Check for potential bugs or edge cases
• Evaluate code quality and best practices
• Assess performance implications
• Review security considerations
• Provide comprehensive feedback

Code Review Summary

This PR successfully fixes a critical bug where global After/AfterEvery hooks weren't executing. The fix is minimal, targeted, and well-implemented.

✅ What's Good

🎯 Correct Root Cause Identification

• The issue was correctly identified: TestExecutor.cs:104 calls OnTestStartingAsync() for Before hooks but was missing the corresponding OnTestCompletedAsync() call for After hooks
• The fix addresses the exact problem without over-engineering

🏗️ Excellent Implementation Quality

• Proper Exception Handling: The try-finally pattern in TestExecutor.cs:117-127 ensures RouteTestResult() always executes even if hooks fail
• Resource Safety: Uses proper disposal patterns and doesn't introduce resource leaks
• Consistent with Framework: Follows existing patterns in the codebase

📋 Comprehensive Test Coverage

• GlobalHooksExecutionTest.cs provides excellent validation for all hook types:
  • Test-level: Before(Test), After(Test), BeforeEvery(Test), AfterEvery(Test)
  • Class-level: Before(Class), After(Class), BeforeEvery(Class), AfterEvery(Class)
  • Assembly-level: Before(Assembly), After(Assembly), BeforeEvery(Assembly), AfterEvery(Assembly)
  • Session/Discovery hooks for regression testing
• Observability: Includes helpful console output and hook execution tracking
• Validation Logic: Checks for missing hooks and provides clear failure messages

🔍 Technical Analysis

Performance Impact: ⚡ Minimal

• Only adds necessary hook orchestration calls
• No additional allocations or unnecessary overhead
• Maintains existing parallel execution patterns

Security: 🔒 No Concerns

• No user input processing or external resource access
• Maintains existing security boundaries
• No credential or sensitive data handling

Thread Safety: 🧵 Maintained

• Uses ConcurrentBag<string> for thread-safe hook tracking
• Maintains existing synchronization patterns
• No race condition introductions

🤔 Minor Observations

1. Test Organization (GlobalHooksExecutionTest.cs:7,21): Two separate ExecutedHooks collections exist but only one is used. This could be consolidated but doesn't affect functionality.

2. Hook Failure Handling: The implementation correctly prioritizes test result reporting over hook failures, which is the right design choice for a testing framework.

📊 Code Quality Score: 9/10

Strengths:

• Minimal, surgical fix that addresses the root cause
• Excellent error handling with try-finally pattern
• Comprehensive test coverage with clear validation
• Follows existing code patterns and conventions
• Well-documented commit message explaining the fix

Areas for Future Enhancement:

• Could add metrics for hook execution failures (not required for this fix)
• Test file could be slightly cleaner with single ExecutedHooks collection

🎯 Recommendation

✅ APPROVE - This is a high-quality fix that:

• Correctly addresses the reported issue
• Maintains backwards compatibility
• Includes excellent test coverage
• Uses proper error handling patterns
• Has minimal performance impact

The fix is ready for merge and will properly restore the execution of global After/AfterEvery hooks at Test, Class, and Assembly levels.

Claude finished @thomhurst's task —— View job

PR Review: Global After/AfterEvery hooks not executing ✅

Todo List:

• Read and analyze the TestExecutor.cs changes
• Review the new test file for coverage adequacy
• Check for potential bugs or edge cases
• Evaluate code quality and best practices
• Assess performance implications
• Review security considerations
• Provide comprehensive feedback

Code Review Summary

This PR successfully fixes a critical bug where global After/AfterEvery hooks weren't executing. The fix is minimal, targeted, and well-implemented.

✅ What's Good

🎯 Correct Root Cause Identification

• The issue was correctly identified: TestExecutor.cs:104 calls OnTestStartingAsync() for Before hooks but was missing the corresponding OnTestCompletedAsync() call for After hooks
• The fix addresses the exact problem without over-engineering

🏗️ Excellent Implementation Quality

• Proper Exception Handling: The try-finally pattern in TestExecutor.cs:117-127 ensures RouteTestResult() always executes even if hooks fail
• Resource Safety: Uses proper disposal patterns and doesn't introduce resource leaks
• Consistent with Framework: Follows existing patterns in the codebase

📋 Comprehensive Test Coverage

• GlobalHooksExecutionTest.cs provides excellent validation for all hook types:
  • Test-level: Before(Test), After(Test), BeforeEvery(Test), AfterEvery(Test)
  • Class-level: Before(Class), After(Class), BeforeEvery(Class), AfterEvery(Class)
  • Assembly-level: Before(Assembly), After(Assembly), BeforeEvery(Assembly), AfterEvery(Assembly)
  • Session/Discovery hooks for regression testing
• Observability: Includes helpful console output and hook execution tracking
• Validation Logic: Checks for missing hooks and provides clear failure messages

🔍 Technical Analysis

Performance Impact: ⚡ Minimal

• Only adds necessary hook orchestration calls
• No additional allocations or unnecessary overhead
• Maintains existing parallel execution patterns

Security: 🔒 No Concerns

• No user input processing or external resource access
• Maintains existing security boundaries
• No credential or sensitive data handling

Thread Safety: 🧵 Maintained

• Uses ConcurrentBag<string> for thread-safe hook tracking
• Maintains existing synchronization patterns
• No race condition introductions

🤔 Minor Observations

1. Test Organization (GlobalHooksExecutionTest.cs:7,21): Two separate ExecutedHooks collections exist but only one is used. This could be consolidated but doesn't affect functionality.

2. Hook Failure Handling: The implementation correctly prioritizes test result reporting over hook failures, which is the right design choice for a testing framework.

📊 Code Quality Score: 9/10

Strengths:

• Minimal, surgical fix that addresses the root cause
• Excellent error handling with try-finally pattern
• Comprehensive test coverage with clear validation
• Follows existing code patterns and conventions
• Well-documented commit message explaining the fix

Areas for Future Enhancement:

• Could add metrics for hook execution failures (not required for this fix)
• Test file could be slightly cleaner with single ExecutedHooks collection

🎯 Recommendation

✅ APPROVE - This is a high-quality fix that:

• Correctly addresses the reported issue
• Maintains backwards compatibility
• Includes excellent test coverage
• Uses proper error handling patterns
• Has minimal performance impact

The fix is ready for merge and will properly restore the execution of global After/AfterEvery hooks at Test, Class, and Assembly levels.

Claude finished @thomhurst's task —— View job

PR Review: Global After/AfterEvery hooks not executing ✅

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze TestExecutor.cs changes for code quality and potential issues
• Review the new test file for coverage adequacy
• Evaluate performance implications of the changes
• Check for security concerns
• Assess overall implementation approach
• Provide comprehensive feedback

Code Review Summary

This PR successfully fixes a critical bug where global After/AfterEvery hooks weren't executing after test completion. The implementation is clean, targeted, and well-tested.

✅ What's Good

🎯 Precise Problem Resolution

• Root Cause Correctly Identified: The issue was in TestExecutor.cs:104 where OnTestStartingAsync() was called for Before hooks, but the corresponding OnTestCompletedAsync() call for After hooks was missing
• Minimal, Surgical Fix: The solution adds exactly what was needed without over-engineering or unnecessary changes

🏗️ Excellent Implementation Quality

• Proper Exception Handling: The try-finally pattern in TestExecutor.cs:117-127 ensures RouteTestResult() always executes even if cleanup hooks fail, which is critical for test result reporting
• Resource Safety: Uses appropriate disposal patterns and doesn't introduce resource leaks
• Framework Consistency: Follows existing patterns in the codebase for hook orchestration and error handling

📋 Comprehensive Test Coverage
The new test file GlobalHooksExecutionTest.cs provides excellent validation:

• Complete Hook Coverage: Tests all hook types at Test, Class, Assembly, TestSession, and TestDiscovery levels
• Clear Observability: Includes helpful console output showing hook execution order and timing
• Robust Validation Logic: Checks for missing hooks and provides clear failure messages with comprehensive summary reporting
• Thread Safety: Uses ConcurrentBag<string> for tracking hook execution across parallel tests

🔍 Technical Analysis

Performance Impact: ⚡ Minimal

• Only adds necessary hook orchestration calls
• No additional memory allocations or computational overhead
• Maintains existing parallel execution patterns without blocking

Security: 🔒 No Concerns

• No user input processing or external resource access
• Maintains existing security boundaries and doesn't introduce attack vectors
• No handling of credentials or sensitive data

Thread Safety: 🧵 Maintained

• Uses thread-safe collections (ConcurrentBag) for hook tracking
• Maintains existing synchronization patterns from the framework
• No race conditions introduced by the changes

🤔 Minor Observations

1. Test File Organization (GlobalHooksExecutionTest.cs:7,21): Two separate ExecutedHooks collections exist but only one is actively used. While this doesn't impact functionality, it could be consolidated for cleaner code organization.

2. Hook Failure Handling: The implementation correctly prioritizes test result reporting over hook execution failures, which is the right design choice for a testing framework to maintain test execution reliability.

3. Package Updates (Directory.Packages.props): The EnumerableAsyncProcessor version bump appears unrelated to the core fix but is likely a necessary dependency update.

📊 Code Quality Assessment

Code Quality Score: 9/10

Strengths:

• ✅ Minimal, targeted fix addressing the exact root cause
• ✅ Robust error handling with proper try-finally resource cleanup
• ✅ Comprehensive test coverage validating all hook execution scenarios
• ✅ Consistent with existing framework patterns and conventions
• ✅ Clear, descriptive commit messages explaining the fix purpose

Areas for Future Enhancement:

• Could add telemetry/metrics for hook execution failures (not required for this fix)
• Test file could be slightly cleaner with unified ExecutedHooks collection
• Could benefit from integration test verifying fix in real test execution scenarios

🎯 Recommendation

✅ APPROVE - This is a high-quality fix that:

• ✅ Correctly addresses the critical issue described in Global After hooks not executing (v0.50.30) #2804
• ✅ Maintains full backwards compatibility with existing test suites
• ✅ Includes thorough test coverage with clear validation logic
• ✅ Uses proper error handling patterns that prioritize test result reliability
• ✅ Has minimal performance impact and no security concerns
• ✅ Follows established framework patterns and best practices

The fix is ready for merge and will properly restore the execution of global After/AfterEvery hooks at Test, Class, and Assembly levels, resolving the critical testing framework bug.

Claude finished @thomhurst's task —— View job

PR Review: Global After/AfterEvery hooks not executing

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze TestExecutor.cs changes for code quality and potential issues
• Review the new test file for coverage adequacy
• Evaluate performance implications of the changes
• Check for security concerns
• Assess overall implementation approach
• Provide comprehensive feedback

Code Review Summary

This PR successfully fixes a critical bug where global After/AfterEvery hooks weren't executing after test completion. The implementation is clean, targeted, and well-tested.

✅ What's Good

🎯 Precise Problem Resolution

• Root Cause Correctly Identified: The issue was in TestExecutor.cs:104 where OnTestStartingAsync() was called for Before hooks, but the corresponding OnTestCompletedAsync() call for After hooks was missing
• Minimal, Surgical Fix: The solution adds exactly what was needed without over-engineering or unnecessary changes

🏗️ Excellent Implementation Quality

• Proper Exception Handling: The try-finally pattern in TestExecutor.cs:117-127 ensures RouteTestResult() always executes even if cleanup hooks fail, which is critical for test result reporting
• Resource Safety: Uses appropriate disposal patterns and doesn't introduce resource leaks
• Framework Consistency: Follows existing patterns in the codebase for hook orchestration and error handling

📋 Comprehensive Test Coverage
The new test file GlobalHooksExecutionTest.cs provides excellent validation:

• Complete Hook Coverage: Tests all hook types at Test, Class, Assembly, TestSession, and TestDiscovery levels
• Clear Observability: Includes helpful console output showing hook execution order and timing
• Robust Validation Logic: Checks for missing hooks and provides clear failure messages with comprehensive summary reporting
• Thread Safety: Uses ConcurrentBag<string> for tracking hook execution across parallel tests

🔍 Technical Analysis

Performance Impact: ⚡ Minimal

• Only adds necessary hook orchestration calls
• No additional memory allocations or computational overhead
• Maintains existing parallel execution patterns without blocking

Security: 🔒 No Concerns

• No user input processing or external resource access
• Maintains existing security boundaries and doesn't introduce attack vectors
• No handling of credentials or sensitive data

Thread Safety: 🧵 Maintained

• Uses thread-safe collections (ConcurrentBag) for hook tracking
• Maintains existing synchronization patterns from the framework
• No race conditions introduced by the changes

🤔 Minor Observations

1. Test File Organization (GlobalHooksExecutionTest.cs:7,21): Two separate ExecutedHooks collections exist but only one is actively used. While this doesn't impact functionality, it could be consolidated for cleaner code organization.

2. Hook Failure Handling: The implementation correctly prioritizes test result reporting over hook execution failures, which is the right design choice for a testing framework to maintain test execution reliability.

3. Package Updates (Directory.Packages.props): The EnumerableAsyncProcessor version bump appears unrelated to the core fix but is likely a necessary dependency update.

📊 Code Quality Assessment

Code Quality Score: 9/10

Strengths:

• ✅ Minimal, targeted fix addressing the exact root cause
• ✅ Robust error handling with proper try-finally resource cleanup
• ✅ Comprehensive test coverage validating all hook execution scenarios
• ✅ Consistent with existing framework patterns and conventions
• ✅ Clear, descriptive commit messages explaining the fix purpose

Areas for Future Enhancement:

• Could add telemetry/metrics for hook execution failures (not required for this fix)
• Test file could be slightly cleaner with unified ExecutedHooks collection
• Could benefit from integration test verifying fix in real test execution scenarios

🎯 Recommendation

✅ APPROVE - This is a high-quality fix that:

• ✅ Correctly addresses the critical issue described in Global After hooks not executing (v0.50.30) #2804
• ✅ Maintains full backwards compatibility with existing test suites
• ✅ Includes thorough test coverage with clear validation logic
• ✅ Uses proper error handling patterns that prioritize test result reliability
• ✅ Has minimal performance impact and no security concerns
• ✅ Follows established framework patterns and best practices

The fix is ready for merge and will properly restore the execution of global After/AfterEvery hooks at Test, Class, and Assembly levels, resolving the critical testing framework bug.

View job

@claude investigate these changes causing hanging

Claude finished @thomhurst's task —— View job

Critical Issue Found - Hanging Root Cause Identified 🚨

Todo List:

• Analyze TestExecutor.cs changes for potential deadlocks/hanging
• Review GlobalHooksExecutionTest.cs for blocking operations
• Check async/await patterns in the hook execution
• Look for resource contention or infinite waits
• Identify root cause of hanging behavior ✅
• Provide specific findings and recommendations ✅

🎯 Root Cause: Counter Race Condition

The Issue: The hanging is caused by a race condition in the counter logic in HookOrchestrator.cs. Here's what's happening:

The Problem Flow:

1. OnTestStartingAsync (line ~376): Increments counters with AddOrUpdate(key, 1, (_, count) => count + 1)
2. OnTestCompletedAsync (line ~391): Decrements counters with AddOrUpdate(key, 0, (_, count) => count - 1)
3. Race Condition: If a test calls OnTestCompletedAsync before another test calls OnTestStartingAsync, the counter can go negative or become inconsistent

Critical Code Lines:

// OnTestStartingAsync - HookOrchestrator.cs:~376
_assemblyTestCounts.AddOrUpdate(assemblyName, 1, (_, count) => count + 1);
_classTestCounts.AddOrUpdate(testClassType, 1, (_, count) => count + 1);

// OnTestCompletedAsync - HookOrchestrator.cs:~391  
var classTestsRemaining = _classTestCounts.AddOrUpdate(testClassType, 0, (_, count) => count - 1);
var assemblyTestsRemaining = _assemblyTestCounts.AddOrUpdate(assemblyName, 0, (_, count) => count - 1);

Why This Causes Hanging:

• Tests wait for classTestsRemaining == 0 to execute AfterClass hooks
• Tests wait for assemblyTestsRemaining == 0 to execute AfterAssembly hooks
• If counters become inconsistent due to race conditions, they may never reach 0
• Tests hang indefinitely waiting for cleanup hooks that never execute

Specific Race Scenario:

1. Test A starts: counter = 1
2. Test B starts: counter = 2
3. Test A completes: counter = 1
4. Test C starts: counter = 2
5. Test B completes: counter = 1
6. Test C completes: counter = 0 ✅ (cleanup executes)
7. BUT if timing is wrong, Test A could complete before Test C starts, leading to inconsistent state

🔧 Recommended Immediate Fix:

The core problem: Using AddOrUpdate(key, 0, (_, count) => count - 1) in the decrement operation is incorrect. If the key doesn't exist when decrementing, it initializes to 0, then decrements to -1, causing the counter to never reach 0 for cleanup.

Solution: Change the decrement logic in OnTestCompletedAsync:

// BEFORE (problematic):
var classTestsRemaining = _classTestCounts.AddOrUpdate(testClassType, 0, (_, count) => count - 1);

// AFTER (fix):
var classTestsRemaining = _classTestCounts.AddOrUpdate(testClassType, -1, (_, count) => Math.Max(0, count - 1));

This ensures counters never go negative and cleanup hooks execute properly.

🚨 Immediate Action Required:

This is a critical bug that will cause test hangs in parallel execution scenarios. The fix should be applied before merging this PR.

Claude finished @thomhurst's task —— View job

Critical Hanging Issue Identified - Race Condition in Counter Management 🚨

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze HookOrchestrator.cs changes for race conditions and hanging issues
• Review TestExecutor.cs changes for potential deadlocks
• Examine GlobalHooksExecutionTest.cs for blocking operations
• Check async/await patterns in hook execution
• Identify root cause of hanging behavior ✅
• Provide specific fixes and recommendations ✅

🎯 Root Cause: Counter Lifecycle Race Condition

I've identified the exact cause of the hanging issue. The problem is in the counter management logic in HookOrchestrator.cs.

The Critical Flaw

HookOrchestrator.cs:255-273 - The counter removal logic creates a race condition:

// Problem code:
var classCounter = _classTestCounts.GetOrAdd(testClassType, _ => new Counter());
var classTestsRemaining = classCounter.Decrement();

// Execute AfterClass hooks if last test in class
if (classTestsRemaining == 0 && _beforeClassTasks.TryGetValue(testClassType, out _))
{
    await ExecuteAfterClassHooksAsync(testClassType, cancellationToken);
    _classTestCounts.TryRemove(testClassType, out _);  // ⚠️ PROBLEM: Removes counter
}

Race Condition Scenario:

1. Test A starts: Counter = 1
2. Test B starts: Counter = 2
3. Test A completes: Decrements to 1
4. Test B completes: Decrements to 0, executes cleanup, removes counter
5. Test C starts: GetOrAdd creates NEW counter = 1 (should be 3!)
6. Test C completes: Decrements to 0, tries to execute cleanup again
7. Tests hang waiting for cleanup that will never complete properly

The Core Issues:

1. Counter Removal Too Early

• Line 265: _classTestCounts.TryRemove(testClassType, out _)
• Line 272: _assemblyTestCounts.TryRemove(assemblyName, out _)
• Problem: Removes counters immediately after cleanup, but new tests might still be starting

2. GetOrAdd Creates New Counters

• If a test starts after counter removal, GetOrAdd creates a fresh counter starting at 0
• This breaks the count continuity and can cause cleanup hooks to execute multiple times or hang waiting

3. No Coordination Between Start/Complete Phases

• OnTestStartingAsync and OnTestCompletedAsync operate on potentially different counter instances
• This violates the fundamental counting contract

🔧 Immediate Fix Required

Option 1: Never Remove Counters (Safest)

// In OnTestCompletedAsync - REMOVE these lines:
// _classTestCounts.TryRemove(testClassType, out _);     // DELETE THIS
// _assemblyTestCounts.TryRemove(assemblyName, out _);   // DELETE THIS

Option 2: Better Counter Reset Logic

// Instead of removing, reset counters safely
if (classTestsRemaining == 0 && _beforeClassTasks.TryGetValue(testClassType, out _))
{
    // Execute cleanup first
    await ExecuteAfterClassHooksAsync(testClassType, cancellationToken);
    
    // Reset counter to handle potential new tests, but don't remove
    if (_classTestCounts.TryGetValue(testClassType, out var counter))
    {
        // Only reset if still at 0 (no new tests started during cleanup)
        counter.CurrentCount; // Check current value safely
    }
}

🚨 Impact Assessment

This is a critical production bug that will cause:

• ✅ Test hangs in parallel execution
• ✅ Inconsistent hook execution
• ✅ Resource leaks from waiting threads
• ✅ Flaky test behavior depending on timing

📋 Recommended Actions

1. Immediate: Remove the TryRemove calls (lines 265, 272) to prevent new counter creation
2. Follow-up: Implement proper counter lifecycle management
3. Testing: Add stress tests with high concurrency to verify fix

This explains why the tests are hanging - threads are waiting for cleanup conditions that can never be met due to counter instance inconsistency.

Claude finished @thomhurst's task —— View job

PR Review: Global After/AfterEvery hooks not executing ✅

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze TestExecutor.cs changes for code quality and potential issues
• Review HookOrchestrator.cs changes for the hanging issue mentioned in previous comments
• Examine the new test file for coverage adequacy
• Check Directory.Packages.props changes
• Evaluate performance implications of the changes
• Check for security concerns
• Assess overall implementation approach and provide comprehensive feedback

Code Review Summary

This PR successfully fixes a critical bug where global After/AfterEvery hooks weren't executing after test completion. The implementation is clean, targeted, and well-tested.

✅ What's Good

🎯 Precise Problem Resolution

• Root Cause Correctly Identified: The issue was in TestExecutor.cs:104 where OnTestStartingAsync() was called for Before hooks, but if an exception occurred after incrementing counters, OnTestCompletedAsync() was never called
• Excellent Fix Strategy: The solution uses a hookStarted flag to track if counters were incremented and ensures symmetrical cleanup in the catch block

🏗️ Excellent Implementation Quality

• Proper Exception Handling: The try-finally pattern in TestExecutor.cs:119-129 ensures RouteTestResult() always executes even if cleanup hooks fail, which is critical for test result reporting
• Resource Safety: Uses appropriate disposal patterns and doesn't introduce resource leaks
• Framework Consistency: Follows existing patterns in the codebase for hook orchestration and error handling
• Thread Safety: The Counter class in HookOrchestrator.cs:256-259 uses proper locking to prevent race conditions

📋 Comprehensive Test Coverage
The new test file GlobalHooksExecutionTest.cs provides excellent validation:

• Complete Hook Coverage: Tests all hook types at Test, Class, Assembly, TestSession, and TestDiscovery levels
• Clear Observability: Includes helpful console output showing hook execution order and timing
• Robust Validation Logic: Checks for missing hooks and provides clear failure messages with comprehensive summary reporting
• Thread Safety: Uses ConcurrentBag<string> for tracking hook execution across parallel tests

🔍 Technical Analysis

Performance Impact: ⚡ Minimal

• Only adds necessary hook orchestration calls and a boolean flag
• No additional memory allocations or computational overhead
• The Counter class uses efficient locking and maintains existing parallel execution patterns

Security: 🔒 No Concerns

• No user input processing or external resource access
• Maintains existing security boundaries and doesn't introduce attack vectors
• No handling of credentials or sensitive data

Thread Safety: 🧵 Excellent

• Uses thread-safe Counter class with proper locking in HookOrchestrator.cs
• Uses thread-safe collections (ConcurrentBag, ConcurrentDictionary) for hook tracking
• The fix ensures counter symmetry preventing the race conditions that caused hanging

🚨 Critical Bug Resolution

Hanging Issue Resolved: The commits show this PR addresses the previously identified hanging issues:

• TestExecutor.cs:141-152: Added proper cleanup in exception handling to ensure OnTestCompletedAsync is always called when OnTestStartingAsync completes
• HookOrchestrator.cs:256-259: Replaced unsafe AddOrUpdate operations with thread-safe Counter class
• Counter Symmetry: Every increment now has a guaranteed corresponding decrement, preventing counters from getting stuck and causing infinite waits

🤔 Minor Observations

1. Test File Organization (GlobalHooksExecutionTest.cs:7,21): Two separate ExecutedHooks collections exist but only one is actively used. While this doesn't impact functionality, it could be consolidated for cleaner code organization.

2. Package Updates (Directory.Packages.props:14): The EnumerableAsyncProcessor version bump from 3.3.2 to 3.3.3 appears unrelated to the core fix but is likely a necessary dependency update.

📊 Code Quality Assessment

Code Quality Score: 9/10

Strengths:

• ✅ Correctly addresses the exact root cause with minimal, targeted changes
• ✅ Robust error handling with proper resource cleanup patterns
• ✅ Comprehensive test coverage validating all hook execution scenarios
• ✅ Excellent commit messages explaining both the problem and solution
• ✅ Thread-safe implementation preventing race conditions
• ✅ Maintains full backwards compatibility

Areas for Future Enhancement:

• Could add telemetry/metrics for hook execution failures (not required for this fix)
• Test file could be slightly cleaner with unified ExecutedHooks collection

🎯 Recommendation

✅ APPROVE - This is a high-quality fix that:

• ✅ Correctly addresses the critical issue described in Global After hooks not executing (v0.50.30) #2804
• ✅ Resolves the hanging issues identified in previous comments through proper counter management
• ✅ Maintains full backwards compatibility with existing test suites
• ✅ Includes thorough test coverage with clear validation logic
• ✅ Uses proper error handling patterns that prioritize test result reliability
• ✅ Has minimal performance impact and no security concerns
• ✅ Follows established framework patterns and best practices

The fix is ready for merge and will properly restore the execution of global After/AfterEvery hooks at Test, Class, and Assembly levels while preventing the counter race conditions that could cause hanging.