Skip to content

Commit d6df2a1

Browse files
committed
Address Gemini review feedback
- Close truncated string literals before appending missing brackets in TryFixBraces (fixes recovery of JSON cut mid-string) - Remove ineffective ToJsonString() validation loop since JsonNode.Parse already validates syntax; the typed deserializer handles type mismatches via Settings property initializer defaults - Extract duplicated recovery logic from LoadSettings/LoadSettingsAsync into shared DeserializeOrRecoverSettings helper - Add test for truncated-inside-string recovery https://claude.ai/code/session_01Gej4ey7eUsbPRDqFWwxKyZ
1 parent 06645ea commit d6df2a1

3 files changed

Lines changed: 67 additions & 95 deletions

File tree

StabilityMatrix.Core/Services/SettingsJsonSanitizer.cs

Lines changed: 15 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,17 @@ public static string TryFixBraces(string jsonText)
118118
}
119119
}
120120

121-
// Append missing closing brackets in correct LIFO order
122121
var sb = new StringBuilder(trimmed);
122+
123+
// If truncated inside a string literal, close it first
124+
if (inString)
125+
{
126+
if (escaped)
127+
sb.Append('\\');
128+
sb.Append('"');
129+
}
130+
131+
// Append missing closing brackets in correct LIFO order
123132
while (stack.Count > 0)
124133
{
125134
sb.Append('\n').Append(stack.Pop());
@@ -208,38 +217,17 @@ public static string TryFixBraces(string jsonText)
208217
return null;
209218
}
210219

211-
// Remove properties that can't be individually serialized back
212-
var propsToRemove = new List<string>();
213-
foreach (var (key, value) in rootObject)
214-
{
215-
try
216-
{
217-
// Validate each property by serializing it to a string
218-
_ = value?.ToJsonString();
219-
}
220-
catch (Exception)
221-
{
222-
propsToRemove.Add(key);
223-
}
224-
}
225-
226-
foreach (var key in propsToRemove)
227-
{
228-
logger?.LogWarning("Removing corrupt property from settings: {Key}", key);
229-
rootObject.Remove(key);
230-
}
231-
232-
// Re-serialize the cleaned node tree and attempt typed deserialization
220+
// Re-serialize the cleaned node tree and attempt typed deserialization.
221+
// JsonNode.Parse with lenient options may accept JSON that the typed deserializer
222+
// can handle, and any properties with incompatible types will get their defaults
223+
// from the Settings class property initializers.
233224
try
234225
{
235226
var cleanedJson = rootObject.ToJsonString();
236227
var settings = JsonSerializer.Deserialize(cleanedJson, SettingsSerializerContext.Default.Settings);
237228
if (settings is not null)
238229
{
239-
logger?.LogInformation(
240-
"Settings recovered via property-level recovery ({RemovedCount} properties removed)",
241-
propsToRemove.Count
242-
);
230+
logger?.LogInformation("Settings recovered via property-level recovery");
243231
return settings;
244232
}
245233
}

StabilityMatrix.Core/Services/SettingsManager.cs

Lines changed: 40 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -477,41 +477,7 @@ protected virtual void LoadSettings(CancellationToken cancellationToken = defaul
477477
return;
478478
}
479479

480-
// Try normal deserialization first
481-
try
482-
{
483-
var loadedSettings = JsonSerializer.Deserialize(
484-
rawBytes,
485-
SettingsSerializerContext.Default.Settings
486-
);
487-
488-
if (loadedSettings is not null)
489-
{
490-
Settings = loadedSettings;
491-
return;
492-
}
493-
}
494-
catch (JsonException ex)
495-
{
496-
logger.LogWarning(ex, "Failed to deserialize settings, attempting recovery");
497-
}
498-
499-
// Recovery path
500-
BackupCorruptedFile(rawBytes);
501-
502-
var jsonText = Encoding.UTF8.GetString(SettingsJsonSanitizer.SanitizeBytes(rawBytes));
503-
var recovered = SettingsJsonSanitizer.TryDeserializeWithRecovery(jsonText, logger);
504-
505-
if (recovered is not null)
506-
{
507-
Settings = recovered;
508-
logger.LogInformation("Settings recovered from corrupted file");
509-
}
510-
else
511-
{
512-
logger.LogWarning("Could not recover settings from corrupted file, using defaults");
513-
Settings = new Settings();
514-
}
480+
Settings = DeserializeOrRecoverSettings(rawBytes);
515481
}
516482
finally
517483
{
@@ -546,50 +512,56 @@ protected virtual async Task LoadSettingsAsync(CancellationToken cancellationTok
546512
return;
547513
}
548514

549-
// Try normal deserialization first
550-
try
551-
{
552-
var loadedSettings = JsonSerializer.Deserialize(
553-
rawBytes,
554-
SettingsSerializerContext.Default.Settings
555-
);
515+
Settings = DeserializeOrRecoverSettings(rawBytes);
516+
}
517+
finally
518+
{
519+
fileLock.Release();
556520

557-
if (loadedSettings is not null)
558-
{
559-
Settings = loadedSettings;
560-
return;
561-
}
562-
}
563-
catch (JsonException ex)
564-
{
565-
logger.LogWarning(ex, "Failed to deserialize settings, attempting recovery");
566-
}
521+
isLoaded = true;
567522

568-
// Recovery path
569-
BackupCorruptedFile(rawBytes);
523+
Loaded?.Invoke(this, EventArgs.Empty);
524+
}
525+
}
570526

571-
var jsonText = Encoding.UTF8.GetString(SettingsJsonSanitizer.SanitizeBytes(rawBytes));
572-
var recovered = SettingsJsonSanitizer.TryDeserializeWithRecovery(jsonText, logger);
527+
/// <summary>
528+
/// Attempts to deserialize settings from raw bytes, falling back to sanitization
529+
/// and recovery if the JSON is corrupted. Returns default settings as a last resort.
530+
/// </summary>
531+
private Settings DeserializeOrRecoverSettings(byte[] rawBytes)
532+
{
533+
// Try normal deserialization first
534+
try
535+
{
536+
var loadedSettings = JsonSerializer.Deserialize(
537+
rawBytes,
538+
SettingsSerializerContext.Default.Settings
539+
);
573540

574-
if (recovered is not null)
575-
{
576-
Settings = recovered;
577-
logger.LogInformation("Settings recovered from corrupted file");
578-
}
579-
else
541+
if (loadedSettings is not null)
580542
{
581-
logger.LogWarning("Could not recover settings from corrupted file, using defaults");
582-
Settings = new Settings();
543+
return loadedSettings;
583544
}
584545
}
585-
finally
546+
catch (JsonException ex)
586547
{
587-
fileLock.Release();
548+
logger.LogWarning(ex, "Failed to deserialize settings, attempting recovery");
549+
}
588550

589-
isLoaded = true;
551+
// Recovery path: backup corrupted file, sanitize, and attempt recovery
552+
BackupCorruptedFile(rawBytes);
590553

591-
Loaded?.Invoke(this, EventArgs.Empty);
554+
var jsonText = Encoding.UTF8.GetString(SettingsJsonSanitizer.SanitizeBytes(rawBytes));
555+
var recovered = SettingsJsonSanitizer.TryDeserializeWithRecovery(jsonText, logger);
556+
557+
if (recovered is not null)
558+
{
559+
logger.LogInformation("Settings recovered from corrupted file");
560+
return recovered;
592561
}
562+
563+
logger.LogWarning("Could not recover settings from corrupted file, using defaults");
564+
return new Settings();
593565
}
594566

595567
private void BackupCorruptedFile(byte[] rawBytes)

StabilityMatrix.Tests/Core/SettingsJsonSanitizerTests.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,18 @@ public void TryFixBraces_MixedBrackets_ClosesInCorrectOrder()
8181
Assert.IsTrue(result.TrimEnd().EndsWith('}'));
8282
}
8383

84+
[TestMethod]
85+
public void TryFixBraces_TruncatedInsideString_ClosesStringAndBraces()
86+
{
87+
// JSON truncated in the middle of a string value
88+
var input = """{"Theme": "Da""";
89+
90+
var result = SettingsJsonSanitizer.TryFixBraces(input);
91+
92+
// Should close the string literal, then the brace
93+
Assert.IsNotNull(JsonDocument.Parse(result));
94+
}
95+
8496
[TestMethod]
8597
public void TryDeserializeWithRecovery_ValidJson_ReturnsSettings()
8698
{

0 commit comments

Comments
 (0)