# fix(core): resolve macOS issues in 1.4.0-alpha — deadlock, GhJSON validation, and JSON parsing#389
Conversation
…idation, and JSON parsing - Fix ComponentStateManager deadlock by firing events outside stateLock using Monitor.Exit/Enter - Skip SHA-256 hash verification on non-Windows platforms (source-built hash mismatch) - Set GhJSON component Id when InstanceGuid is null to satisfy validation - Add SanitizeAndParseJson for markdown/HTML-wrapped AI responses - Improve AIProvider error messages for non-JSON API responses
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple macOS compatibility issues in SmartHopper 1.4.0-alpha, including a state manager deadlock, SHA-256 hash verification problems for source builds, GhJSON validation errors, and JSON parsing robustness for AI responses.
Changes:
- Modified
ComponentStateManagerto fire events outside the lock usingMonitor.Exit/Enterto prevent macOS deadlocks - Added platform checks to skip SHA-256 hash verification on non-Windows platforms
- Fixed GhJSON component validation by setting
Id = 1whenInstanceGuidis null - Added
SanitizeAndParseJsonhelper to handle AI responses with markdown/HTML wrapping - Improved error messages when AI providers return non-JSON content
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ComponentStateManager.cs | Refactored event firing to use Monitor.Exit/Enter pattern to prevent re-entrant lock deadlocks on macOS |
| ProviderManager.cs | Added platform checks to skip SHA-256 hash verification on non-Windows platforms, reducing UI hangs during Settings dialog load |
| AIProvider.cs | Enhanced error messages to detect and report HTML responses from proxies/server errors |
| script_generate.cs | Added SanitizeAndParseJson helper and set GhJSON component Id fallback to fix validation |
| script_edit.cs | Added SanitizeAndParseJson helper and set GhJSON component Id fallback to fix validation |
| CHANGELOG.md | Documented all fixes in Unreleased section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try extracting first JSON object from the response | ||
| var firstBrace = trimmed.IndexOf('{'); | ||
| var lastBrace = trimmed.LastIndexOf('}'); | ||
| if (firstBrace >= 0 && lastBrace > firstBrace) | ||
| { | ||
| try | ||
| { | ||
| return JObject.Parse(trimmed.Substring(firstBrace, lastBrace - firstBrace + 1)); |
There was a problem hiding this comment.
Potential bug in JSON extraction logic: Using IndexOf('{') and LastIndexOf('}') to extract JSON may fail for responses containing nested objects or multiple JSON objects. For example, if the response is "Some text { "outer": { "inner": "value" } } more text", this will correctly extract the outer braces. However, if the response contains text after with more braces like "{ "json": "data" } and { "other": "stuff" }", it will incorrectly combine them into { "json": "data" } and { "other": "stuff" } which is invalid JSON.
Consider using a more robust approach such as iterating character-by-character to track brace depth and extract the first complete JSON object, or using a JSON parser in lenient mode.
| // Try extracting first JSON object from the response | |
| var firstBrace = trimmed.IndexOf('{'); | |
| var lastBrace = trimmed.LastIndexOf('}'); | |
| if (firstBrace >= 0 && lastBrace > firstBrace) | |
| { | |
| try | |
| { | |
| return JObject.Parse(trimmed.Substring(firstBrace, lastBrace - firstBrace + 1)); | |
| // Local helper to extract the first complete JSON object by tracking brace depth | |
| string ExtractFirstJsonObject(string text) | |
| { | |
| if (string.IsNullOrEmpty(text)) | |
| { | |
| return null; | |
| } | |
| bool inString = false; | |
| bool escape = false; | |
| int depth = 0; | |
| int startIndex = -1; | |
| for (int i = 0; i < text.Length; i++) | |
| { | |
| char c = text[i]; | |
| if (escape) | |
| { | |
| // Current character is escaped; skip special handling | |
| escape = false; | |
| continue; | |
| } | |
| if (c == '\\') | |
| { | |
| if (inString) | |
| { | |
| escape = true; | |
| } | |
| continue; | |
| } | |
| if (c == '"') | |
| { | |
| inString = !inString; | |
| continue; | |
| } | |
| if (inString) | |
| { | |
| continue; | |
| } | |
| if (c == '{') | |
| { | |
| if (depth == 0) | |
| { | |
| startIndex = i; | |
| } | |
| depth++; | |
| } | |
| else if (c == '}' && depth > 0) | |
| { | |
| depth--; | |
| if (depth == 0 && startIndex >= 0) | |
| { | |
| return text.Substring(startIndex, i - startIndex + 1); | |
| } | |
| } | |
| } | |
| return null; | |
| } | |
| // Try extracting first JSON object from the response using brace depth tracking | |
| var jsonCandidate = ExtractFirstJsonObject(trimmed); | |
| if (!string.IsNullOrEmpty(jsonCandidate)) | |
| { | |
| try | |
| { | |
| return JObject.Parse(jsonCandidate); |
| var lastBrace = trimmed.LastIndexOf('}'); | ||
| if (firstBrace >= 0 && lastBrace > firstBrace) | ||
| { | ||
| try | ||
| { | ||
| return JObject.Parse(trimmed.Substring(firstBrace, lastBrace - firstBrace + 1)); | ||
| } | ||
| catch (JsonException) | ||
| { | ||
| // Continue |
There was a problem hiding this comment.
Potential bug in JSON extraction logic: Using IndexOf('{') and LastIndexOf('}') to extract JSON may fail for responses containing nested objects or multiple JSON objects. For example, if the response is "Some text { "outer": { "inner": "value" } } more text", this will correctly extract the outer braces. However, if the response contains text after with more braces like "{ "json": "data" } and { "other": "stuff" }", it will incorrectly combine them into { "json": "data" } and { "other": "stuff" } which is invalid JSON.
Consider using a more robust approach such as iterating character-by-character to track brace depth and extract the first complete JSON object, or using a JSON parser in lenient mode.
| var lastBrace = trimmed.LastIndexOf('}'); | |
| if (firstBrace >= 0 && lastBrace > firstBrace) | |
| { | |
| try | |
| { | |
| return JObject.Parse(trimmed.Substring(firstBrace, lastBrace - firstBrace + 1)); | |
| } | |
| catch (JsonException) | |
| { | |
| // Continue | |
| if (firstBrace >= 0) | |
| { | |
| int depth = 0; | |
| bool inString = false; | |
| bool escape = false; | |
| int endIndex = -1; | |
| for (int i = firstBrace; i < trimmed.Length; i++) | |
| { | |
| char c = trimmed[i]; | |
| if (escape) | |
| { | |
| // Current character is escaped; skip special handling | |
| escape = false; | |
| continue; | |
| } | |
| if (c == '\\') | |
| { | |
| if (inString) | |
| { | |
| // Start escape sequence inside string | |
| escape = true; | |
| } | |
| continue; | |
| } | |
| if (c == '"') | |
| { | |
| // Toggle string state when encountering an unescaped quote | |
| inString = !inString; | |
| continue; | |
| } | |
| if (!inString) | |
| { | |
| if (c == '{') | |
| { | |
| depth++; | |
| } | |
| else if (c == '}') | |
| { | |
| depth--; | |
| if (depth == 0) | |
| { | |
| endIndex = i; | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| if (endIndex > firstBrace) | |
| { | |
| try | |
| { | |
| return JObject.Parse(trimmed.Substring(firstBrace, endIndex - firstBrace + 1)); | |
| } | |
| catch (JsonException) | |
| { | |
| // Continue | |
| } |
| System.Threading.Monitor.Exit(this.stateLock); | ||
| try | ||
| { | ||
| this.TransitionRejected?.Invoke(evt.oldState, evt.newState, evt.rejectionMessage); | ||
| foreach (var evt in pendingEvents) | ||
| { | ||
| if (evt.isRejection) | ||
| { | ||
| this.TransitionRejected?.Invoke(evt.oldState, evt.newState, evt.rejectionMessage); | ||
| } | ||
| else | ||
| { | ||
| this.FireTransitionEvents(evt.oldState, evt.newState); | ||
| } | ||
| } | ||
| } | ||
| else | ||
| finally | ||
| { | ||
| this.FireTransitionEvents(evt.oldState, evt.newState); | ||
| System.Threading.Monitor.Enter(this.stateLock); | ||
| } |
There was a problem hiding this comment.
Potential thread safety issue: After releasing the lock via Monitor.Exit() at line 380, if another thread calls RequestTransition() before this thread re-acquires the lock at line 397, that other thread could observe isTransitioning = false (set at line 371) and start its own ProcessTransitionQueue() execution. This could lead to concurrent processing of the transition queue from multiple threads.
The pattern assumes the calling thread still "owns" the lock semantically even after Monitor.Exit(), but that's not guaranteed. Consider one of these approaches:
- Set a separate flag before releasing the lock to indicate events are being fired
- Use a recursive lock pattern instead of Monitor.Exit/Enter
- Accept that new transitions will be queued and processed by the next ProcessTransitionQueue call
The current implementation may work in practice if event handlers typically don't trigger transitions from other threads, but the race condition exists.
| if (string.IsNullOrWhiteSpace(response)) | ||
| { | ||
| throw new JsonException("AI response is empty."); | ||
| } | ||
|
|
||
| // Try direct parse first | ||
| try | ||
| { | ||
| return JObject.Parse(response); | ||
| } | ||
| catch (JsonException) | ||
| { | ||
| // Continue with sanitization attempts | ||
| } | ||
|
|
||
| // Try extracting JSON from markdown code blocks (```json ... ``` or ``` ... ```) | ||
| var trimmed = response.Trim(); | ||
| var jsonBlockPattern = new System.Text.RegularExpressions.Regex( | ||
| @"```(?:json)?\s*\n?(.*?)\n?\s*```", | ||
| System.Text.RegularExpressions.RegexOptions.Singleline); | ||
| var match = jsonBlockPattern.Match(trimmed); | ||
| if (match.Success) | ||
| { | ||
| try | ||
| { | ||
| return JObject.Parse(match.Groups[1].Value.Trim()); | ||
| } | ||
| catch (JsonException) | ||
| { | ||
| // Continue | ||
| } | ||
| } | ||
|
|
||
| // Try extracting first JSON object from the response | ||
| var firstBrace = trimmed.IndexOf('{'); | ||
| var lastBrace = trimmed.LastIndexOf('}'); | ||
| if (firstBrace >= 0 && lastBrace > firstBrace) | ||
| { | ||
| try | ||
| { | ||
| return JObject.Parse(trimmed.Substring(firstBrace, lastBrace - firstBrace + 1)); | ||
| } | ||
| catch (JsonException) | ||
| { | ||
| // Continue | ||
| } | ||
| } | ||
|
|
||
| // All attempts failed - provide a descriptive error | ||
| var preview = response.Length > 200 ? response.Substring(0, 200) + "..." : response; | ||
| if (trimmed.StartsWith("<", StringComparison.Ordinal)) | ||
| { | ||
| throw new JsonException( | ||
| $"AI returned HTML/XML instead of JSON. This may indicate a provider error. Preview: {preview}"); | ||
| } | ||
|
|
||
| throw new JsonException($"AI response is not valid JSON. Preview: {preview}"); |
There was a problem hiding this comment.
Code duplication: The SanitizeAndParseJson method is duplicated identically in both script_generate.cs and script_edit.cs. Consider extracting this to a shared utility class to improve maintainability. If one implementation needs to be updated in the future (e.g., to fix the firstBrace/lastBrace issue), it will need to be updated in both places.
| if (string.IsNullOrWhiteSpace(response)) | |
| { | |
| throw new JsonException("AI response is empty."); | |
| } | |
| // Try direct parse first | |
| try | |
| { | |
| return JObject.Parse(response); | |
| } | |
| catch (JsonException) | |
| { | |
| // Continue with sanitization attempts | |
| } | |
| // Try extracting JSON from markdown code blocks (```json ... ``` or ``` ... ```) | |
| var trimmed = response.Trim(); | |
| var jsonBlockPattern = new System.Text.RegularExpressions.Regex( | |
| @"```(?:json)?\s*\n?(.*?)\n?\s*```", | |
| System.Text.RegularExpressions.RegexOptions.Singleline); | |
| var match = jsonBlockPattern.Match(trimmed); | |
| if (match.Success) | |
| { | |
| try | |
| { | |
| return JObject.Parse(match.Groups[1].Value.Trim()); | |
| } | |
| catch (JsonException) | |
| { | |
| // Continue | |
| } | |
| } | |
| // Try extracting first JSON object from the response | |
| var firstBrace = trimmed.IndexOf('{'); | |
| var lastBrace = trimmed.LastIndexOf('}'); | |
| if (firstBrace >= 0 && lastBrace > firstBrace) | |
| { | |
| try | |
| { | |
| return JObject.Parse(trimmed.Substring(firstBrace, lastBrace - firstBrace + 1)); | |
| } | |
| catch (JsonException) | |
| { | |
| // Continue | |
| } | |
| } | |
| // All attempts failed - provide a descriptive error | |
| var preview = response.Length > 200 ? response.Substring(0, 200) + "..." : response; | |
| if (trimmed.StartsWith("<", StringComparison.Ordinal)) | |
| { | |
| throw new JsonException( | |
| $"AI returned HTML/XML instead of JSON. This may indicate a provider error. Preview: {preview}"); | |
| } | |
| throw new JsonException($"AI response is not valid JSON. Preview: {preview}"); | |
| return JsonResponseParser.SanitizeAndParseJson(response); | |
| } | |
| /// <summary> | |
| /// Helper for sanitizing and parsing AI JSON responses so that the logic | |
| /// is centralized and can be reused from multiple call sites. | |
| /// </summary> | |
| private static class JsonResponseParser | |
| { | |
| public static JObject SanitizeAndParseJson(string response) | |
| { | |
| if (string.IsNullOrWhiteSpace(response)) | |
| { | |
| throw new JsonException("AI response is empty."); | |
| } | |
| // Try direct parse first | |
| try | |
| { | |
| return JObject.Parse(response); | |
| } | |
| catch (JsonException) | |
| { | |
| // Continue with sanitization attempts | |
| } | |
| // Try extracting JSON from markdown code blocks (```json ... ``` or ``` ... ```) | |
| var trimmed = response.Trim(); | |
| var jsonBlockPattern = new System.Text.RegularExpressions.Regex( | |
| @"```(?:json)?\s*\n?(.*?)\n?\s*```", | |
| System.Text.RegularExpressions.RegexOptions.Singleline); | |
| var match = jsonBlockPattern.Match(trimmed); | |
| if (match.Success) | |
| { | |
| try | |
| { | |
| return JObject.Parse(match.Groups[1].Value.Trim()); | |
| } | |
| catch (JsonException) | |
| { | |
| // Continue | |
| } | |
| } | |
| // Try extracting first JSON object from the response | |
| var firstBrace = trimmed.IndexOf('{'); | |
| var lastBrace = trimmed.LastIndexOf('}'); | |
| if (firstBrace >= 0 && lastBrace > firstBrace) | |
| { | |
| try | |
| { | |
| return JObject.Parse(trimmed.Substring(firstBrace, lastBrace - firstBrace + 1)); | |
| } | |
| catch (JsonException) | |
| { | |
| // Continue | |
| } | |
| } | |
| // All attempts failed - provide a descriptive error | |
| var preview = response.Length > 200 ? response.Substring(0, 200) + "..." : response; | |
| if (trimmed.StartsWith("<", StringComparison.Ordinal)) | |
| { | |
| throw new JsonException( | |
| $"AI returned HTML/XML instead of JSON. This may indicate a provider error. Preview: {preview}"); | |
| } | |
| throw new JsonException($"AI response is not valid JSON. Preview: {preview}"); | |
| } |
| Name = CreateComponentName(languageKey), | ||
| NickName = nickname, | ||
| InstanceGuid = instanceGuid, | ||
| Id = instanceGuid.HasValue ? null : 1, |
There was a problem hiding this comment.
Type mismatch: Setting Id = 1 when creating a GhJSON component may be incorrect. According to GhJSON.Core conventions, Id is typically used as a sequential document-level identifier (1, 2, 3, etc.) when components don't have GUIDs, not as a constant fallback value. Using a hardcoded 1 for all generated components without GUIDs could cause issues if multiple components are created in the same document.
However, since this code creates single-component documents (line 479-481 shows AddComponent(component) with a single component), using Id = 1 is acceptable for this specific use case. If this function is ever used to create multi-component documents, the Id logic will need to be updated to use sequential IDs.
| }, | ||
| NickName = nickname, | ||
| InstanceGuid = instanceGuid, | ||
| Id = instanceGuid.HasValue ? null : 1, |
There was a problem hiding this comment.
Type mismatch: Setting Id = 1 when creating a GhJSON component may be incorrect. According to GhJSON.Core conventions, Id is typically used as a sequential document-level identifier (1, 2, 3, etc.) when components don't have GUIDs, not as a constant fallback value. Using a hardcoded 1 for all generated components without GUIDs could cause issues if multiple components are created in the same document.
However, since this code creates single-component documents (line 508-510 shows AddComponent(component) with a single component), using Id = 1 is acceptable for this specific use case. If this function is ever used to create multi-component documents, the Id logic will need to be updated to use sequential IDs.
| // Log warning but don't block on macOS (source builds have different hashes) | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| Debug.WriteLine($"[ProviderManager] SHA-256 mismatch on non-Windows platform for {Path.GetFileName(assemblyPath)} (expected: {hashResult.PublicHash}, actual: {hashResult.LocalHash}). Allowing load."); | ||
| RhinoApp.WriteLine($"WARNING: Provider '{Path.GetFileName(assemblyPath)}' SHA-256 hash does not match published hash. This is expected for source-built assemblies."); | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
Dead code detected: The non-Windows check on line 193 will never be true because it's already inside a Windows-only block (line 174). This means the macOS/Linux fallback logic for hash mismatches is unreachable.
The entire SHA-256 verification block starting at line 174 is wrapped in if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)), so checking !RuntimeInformation.IsOSPlatform(OSPlatform.Windows) inside that block at line 193 can never be true.
Since line 237 ends the Windows-only block and skips all SHA-256 verification on non-Windows platforms, this unreachable code should simply be removed.
| // Log warning but don't block on macOS (source builds have different hashes) | |
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | |
| { | |
| Debug.WriteLine($"[ProviderManager] SHA-256 mismatch on non-Windows platform for {Path.GetFileName(assemblyPath)} (expected: {hashResult.PublicHash}, actual: {hashResult.LocalHash}). Allowing load."); | |
| RhinoApp.WriteLine($"WARNING: Provider '{Path.GetFileName(assemblyPath)}' SHA-256 hash does not match published hash. This is expected for source-built assemblies."); | |
| break; | |
| } |
…d code, improve brace matching Address Copilot review comments on PR architects-toolkit#389: - Remove unreachable non-Windows check inside Windows-only SHA-256 block in ProviderManager.cs (dead code) - Extract duplicated SanitizeAndParseJson to shared AIResponseParser class in Utils/Parsing/, eliminating code duplication between script_generate.cs and script_edit.cs - Replace IndexOf/LastIndexOf JSON extraction with brace-depth tracking algorithm that correctly handles nested objects and multiple JSON fragments
2 participants
fix(core): resolve macOS issues in 1.4.0-alpha — deadlock, GhJSON validation, and JSON parsing
Description
Thank you for integrating the macOS fixes and for the great new features in 1.4.0-alpha! I tested it on macOS and found a few remaining issues. This PR attempts to address them.
1.
ComponentStateManagerdeadlock (macOS-specific)It appears that the
ProcessTransitionQueue()refactoring in 1.4.0 may have reintroduced a similar lock reentrancy pattern to the one addressed in PR #382. On macOS, event handlers fired while holdingstateLockcan re-enterRequestTransition(), causing a deadlock that manifests as the UI hanging with an infinite spinner.Fix: Applied the same
Monitor.Exit/Monitor.Enterapproach from PR #382, adapted to the 1.4.0 code structure — events are collected inside the lock, then fired outside it.2. SHA-256 hash verification (macOS-specific)
The new SHA-256 hash verification downloads hashes from GitHub Pages and compares them against local assemblies. When building from source on macOS, the hashes naturally differ from official releases, which triggers a blocking error dialog. The HTTP timeout (10s per URL) also caused the Settings dialog to appear unresponsive.
Fix: Skip SHA-256 hash verification on non-Windows platforms using
RuntimeInformation.IsOSPlatform(OSPlatform.Windows), with a debug log message. This mirrors the existing Authenticode guard.3. GhJSON validation error (all platforms)
When generating new scripts,
CreateScriptGhJson()inscript_generate.csandscript_edit.cscreates aGhJsonComponentwithInstanceGuid = nulland does not setId. GhJSON.Core 1.0.0 requires at least one of these identifiers, resulting in:"Component must have either 'id' or 'instanceGuid'".Fix: Set
Id = 1as fallback whenInstanceGuidis null:Id = instanceGuid.HasValue ? null : 1. The existing edit-mode behavior (which passes a validInstanceGuid) is unaffected.4. JSON parsing of AI responses (all platforms)
Some AI models return responses wrapped in markdown code blocks (
```json ... ```) or other formatting, causingJObject.Parse(response)to fail. Additionally, when a provider API returns an HTML error page (e.g., proxy error), the error message is not very informative.Fix:
SanitizeAndParseJson()helper in both script tools — tries direct parse first, then extracts from markdown code blocks, then finds the first{...}object, with descriptive error messages for each case.AIProvider.CallApi()to detect and clearly report HTML responses.Breaking Changes
None.
Testing Done
Files Changed
ComponentStateManager.csstateLockusingMonitor.Exit/Monitor.EnterProviderManager.csscript_generate.csIdwhenInstanceGuidis null; addSanitizeAndParseJson()script_edit.csIdwhenInstanceGuidis null; addSanitizeAndParseJson()AIProvider.csChecklist