Fix CI build and serialization warnings#27
Conversation
There was a problem hiding this comment.
Pull Request Overview
While this PR successfully addresses CI pathing and introduces a test suite for SubDirectoryFixerInstallerService, it contains a critical regression. Utilizing AppJsonContext.Default.Object for telemetry serialization results in empty JSON objects ({}) because source generation for System.Object does not capture properties of derived runtime types.
Furthermore, the PR introduces cross-platform compatibility issues by using hardcoded backslashes in file paths, which will fail on Linux and macOS environments. The PR description is also empty, which hinders reviewer context. These issues, particularly the telemetry breakage, should prevent merging in the current state.
About this PR
- The implementation of
SubDirectoryFixerInstallerServicerelies on Windows-style backslashes for path construction. This is a systemic portability issue that will cause failures in Linux-based CI/CD or production environments. - The PR description is empty. Please provide context for the changes, especially regarding the shift to source-generated serialization.
1 comment outside of the diff
src/GregModmanager.Core/Services/TelemetryService.cs
line 114🔴 HIGH RISK
Ensure that string operations explicitly specify culture to avoid unexpected behavior caused by system default culture dependency (e.g., the 'Turkish I' problem).return BitConverter.ToString(hash).Replace("-", "").Substring(0, 12).ToLowerInvariant();
Test suggestions
- Serialize telemetry payloads using the source-generated AppJsonContext metadata.
- SubDirectoryFixerInstallerService successfully installs the DLL and SHA256 marker file.
- SubDirectoryFixerInstallerService handles existing installations by skipping or updating on hash mismatch.
- SubDirectoryFixerInstallerService returns a failure status when the target file is locked.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Serialize telemetry payloads using the source-generated AppJsonContext metadata.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| DebugLogPayload debug => JsonSerializer.Serialize(debug, AppJsonContext.Default.DebugLogPayload), | ||
| string s => s, | ||
| _ => "{}" | ||
| _ => JsonSerializer.Serialize((object)payload, AppJsonContext.Default.Object) |
There was a problem hiding this comment.
🔴 HIGH RISK
Serializing using AppJsonContext.Default.Object will result in an empty JSON object {} for any runtime type because properties of derived classes are not included in the object metadata. This effectively breaks telemetry for any payload that isn't a base object. To fix this, you should explicitly handle known payload types in the switch expression or update AppJsonContext to include them via [JsonSerializable(typeof(YourType))].
| var payloadPath = Path.Combine(AppContext.BaseDirectory, "SubDirectoryFixer\\SubDirectoryFixer.dll"); | ||
| var dirPath = Path.GetDirectoryName(payloadPath); | ||
| if (!string.IsNullOrEmpty(dirPath)) | ||
| { | ||
| Directory.CreateDirectory(dirPath); | ||
| } | ||
| if (!File.Exists(payloadPath)) | ||
| { | ||
| File.WriteAllText(payloadPath, "dummy-payload-content"); | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Using a literal backslash in a path string passed to Path.Combine results in a filename containing a backslash on Linux rather than a directory structure. Update the production code and this test to use multiple arguments to Path.Combine to ensure the correct directory separator is used for the host OS.
| var payloadPath = Path.Combine(AppContext.BaseDirectory, "SubDirectoryFixer\\SubDirectoryFixer.dll"); | |
| var dirPath = Path.GetDirectoryName(payloadPath); | |
| if (!string.IsNullOrEmpty(dirPath)) | |
| { | |
| Directory.CreateDirectory(dirPath); | |
| } | |
| if (!File.Exists(payloadPath)) | |
| { | |
| File.WriteAllText(payloadPath, "dummy-payload-content"); | |
| } | |
| var payloadPath = Path.Combine(AppContext.BaseDirectory, "SubDirectoryFixer", "SubDirectoryFixer.dll"); |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
Description
Motivation and Context
Types of changes
Checklist: