Handled and changed RepoFolderManager#4307
Conversation
WalkthroughAdds a per-user GingerRepos folder and a minimal variable record; hardens directory-clear logic; disables TLS certificate validation in HTTP utilities; changes RepoFolderManager constructor/defaults and persistence location; updates CLI usage; extends Git source-control progress/cancellation and adds recovery for corrupted repos; adds tests for RepoFolderManager. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLIHelper
participant RFM as RepoFolderManager
participant FS as FileSystem
participant GEN as General
CLI->>RFM: new RepoFolderManager(processId)
alt baseWorkingFolder == null
RFM->>GEN: Request DefaultGingerReposFolder
GEN-->>FS: Ensure "GingerRepos" exists under UserProfile
end
RFM->>FS: Open lock & assignments in LocalUserAppData/RepoFolderManagerData
RFM->>RFM: LoadAssignments (tolerant to IO/JSON/Access errors)
RFM->>FS: Acquire lock (retries/backoff)
RFM->>FS: Assign or reuse repo folder
RFM-->>CLI: Return temp repo folder path
sequenceDiagram
autonumber
participant SCI as SourceControlIntegration
participant SC as SourceControl (Git/SVN)
participant GitLib as LibGit2Sharp
participant FS as FileSystem
participant PN as ProgressNotifier
SCI->>SCI: CreateSourceControl() / ConfigureSourceControl()
SCI->>SCI: GetSolutionInfo() (detect `.git`/`.svn`)
alt Local repo exists
SCI->>SC: GetLatest(progressNotifier?, cancellation)
alt Success
SC-->>SCI: true
else Repo invalid / GetLatest failed with repo error
SCI->>SC: Disconnect()
SCI->>FS: ClearDirectoryContent(path) with retries
SCI->>SC: GetProject(projectURI, PN, cancellationToken)
end
else No local repo
SCI->>SC: GetProject(projectURI, PN, cancellationToken)
end
note right of GitLib: Checkout/clone progress reported via PN
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Ginger/GingerCoreCommon/GeneralLib/HttpUtilities.cs (1)
30-50: Missing TLS certificate validation bypass in Download method.The
Downloadmethod lacks the TLS certificate validation bypass that was added to theGetAsyncmethod, creating inconsistent behavior between the two HTTP utility methods. If certificate bypass is needed forGetAsync, it's likely also needed forDownload.Apply this diff to add consistent certificate validation bypass to the
Downloadmethod:try { HttpClientHandler handler = new HttpClientHandler { Proxy = WebRequest.GetSystemWebProxy(), UseProxy = true }; + handler.ServerCertificateCustomValidationCallback += (_, _, _, _) => { return true; }; using HttpClient httpClient = new HttpClient(handler);Ginger/GingerCoreNET/SourceControl/GITSourceControl.cs (1)
324-351: Consider simplifying the control flow.The current implementation has redundant conditions. Since you're checking
result.Status != MergeStatus.Conflictsand thenresult.Status == MergeStatus.Conflicts, the finalreturn falseat line 351 is unreachable. Consider simplifying:- if (result.Status != MergeStatus.Conflicts) + if (result.Status == MergeStatus.Conflicts) + { + conflictsPaths = GetConflictPaths(); + + if (supressMessage == true) + { + Reporter.ToLog(eLogLevel.INFO, "Merge Conflict occurred while getting latest changes."); + } + else + { + Reporter.ToUser(eUserMsgKey.SourceControlUpdateFailed, "Merge Conflict occurred while getting latest changes."); + } + return false; + } + else { using var repo = new Repository(RepositoryRootFolder); if (supressMessage == true) { Reporter.ToLog(eLogLevel.INFO, $"The solution was updated successfully, Update status: {result.Status}, to Revision :{repo.Head.Tip.Sha}"); } else { Reporter.ToUser(eUserMsgKey.GitUpdateState, result.Status, repo.Head.Tip.Sha); } return true; } - else if (result.Status == MergeStatus.Conflicts) - { - conflictsPaths = GetConflictPaths(); - - if (supressMessage == true) - { - Reporter.ToLog(eLogLevel.INFO, "Merge Conflict occurred while getting latest changes."); - } - else - { - Reporter.ToUser(eUserMsgKey.SourceControlUpdateFailed, "Merge Conflict occurred while getting latest changes."); - } - return false; - } - return false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
Ginger/GingerCoreCommon/GeneralLib/General.cs(2 hunks)Ginger/GingerCoreCommon/GeneralLib/HttpUtilities.cs(2 hunks)Ginger/GingerCoreCommon/WorkSpaceLib/RepoFolderManager.cs(6 hunks)Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs(1 hunks)Ginger/GingerCoreNET/RunLib/CLILib/CLIHelper.cs(3 hunks)Ginger/GingerCoreNET/RunLib/CLILib/CLIProcessor.cs(1 hunks)Ginger/GingerCoreNET/SourceControl/GITSourceControl.cs(5 hunks)Ginger/GingerCoreNET/SourceControl/SourceControlIntegration.cs(4 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-03-30T15:18:06.262Z
Learnt from: amitamir
PR: Ginger-Automation/Ginger#4151
File: Ginger/GingerCoreNET/RunLib/CLILib/CLIDynamicFile.cs:0-0
Timestamp: 2025-03-30T15:18:06.262Z
Learning: In CLIDynamicFile.cs, when using the UseTempFolder feature, we should use the cliHelper.GetTempFolderPathForRepo() method instead of manually constructing the temporary path, as this method already handles proper repository name extraction from URLs.
Applied to files:
Ginger/GingerCoreNET/RunLib/CLILib/CLIProcessor.csGinger/GingerCoreNET/RunLib/CLILib/CLIHelper.cs
📚 Learning: 2024-10-15T07:06:51.444Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#3958
File: Ginger/Ginger/App.xaml.cs:246-246
Timestamp: 2024-10-15T07:06:51.444Z
Learning: In `Ginger/Ginger/App.xaml.cs`, the `cliProcessor` field is used in multiple methods (`ParseCommandLineArguments` and `RunNewCLI`), so it should remain as a class-level field.
Applied to files:
Ginger/GingerCoreNET/RunLib/CLILib/CLIProcessor.cs
📚 Learning: 2025-03-10T06:43:33.588Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4140
File: Ginger/GingerCoreNET/Drivers/WebServicesDriver/HttpWebClientUtils.cs:299-303
Timestamp: 2025-03-10T06:43:33.588Z
Learning: The codebase maintains two similar SSL certificate validation bypass approaches for backward compatibility: (1) "AllSSL" which sets both the global ServicePointManager.ServerCertificateValidationCallback and the handler-specific ServerCertificateCustomValidationCallback, and (2) "Ignore" which only sets the handler-specific callback.
Applied to files:
Ginger/GingerCoreCommon/GeneralLib/HttpUtilities.cs
📚 Learning: 2025-08-25T09:27:30.249Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:1433-1441
Timestamp: 2025-08-25T09:27:30.249Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs, SetProxy(dynamic options) should not force options.AcceptInsecureCertificates = false. Only set AcceptInsecureCertificates = true when UseZAP is enabled to avoid breaking setups that permit self-signed/dev certificates.
Applied to files:
Ginger/GingerCoreCommon/GeneralLib/HttpUtilities.cs
📚 Learning: 2025-02-06T07:15:58.225Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4085
File: Ginger/GingerCoreNET/SourceControl/GITSourceControl.cs:317-317
Timestamp: 2025-02-06T07:15:58.225Z
Learning: In GITSourceControl class, the progressNotifier parameter can be null - in such cases the progress reporting is simply skipped without any issues as null checks are properly implemented in the code.
Applied to files:
Ginger/GingerCoreNET/SourceControl/GITSourceControl.cs
📚 Learning: 2025-05-09T10:02:58.761Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4201
File: Ginger/Ginger/SourceControl/CheckInPage.xaml.cs:60-64
Timestamp: 2025-05-09T10:02:58.761Z
Learning: The `GetCurrentWorkingBranch()` method in Ginger source control module has been confirmed by the developer to work reliably without null checks in the current implementation.
Applied to files:
Ginger/GingerCoreNET/SourceControl/GITSourceControl.cs
📚 Learning: 2025-03-20T11:10:30.816Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:30.816Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Applied to files:
Ginger/GingerCoreNET/SourceControl/SourceControlIntegration.cs
📚 Learning: 2025-03-20T11:17:00.659Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/GingerCoreCommon/UserCofig/UserProfile.cs:748-765
Timestamp: 2025-03-20T11:17:00.659Z
Learning: The GetSolutionSourceControlInfo method in UserProfile class, which retrieves or creates a GingerSolution entry for a specific solution GUID, has been confirmed by the developer to be working as expected without issues of duplicate entries.
Applied to files:
Ginger/GingerCoreNET/SourceControl/SourceControlIntegration.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/GingerCoreNET/SourceControl/SourceControlIntegration.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Applied to files:
Ginger/GingerCoreNET/SourceControl/SourceControlIntegration.cs
📚 Learning: 2025-06-16T10:37:13.073Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.073Z
Learning: In Ginger codebase, both `using amdocs.ginger.GingerCoreNET;` and `using Amdocs.Ginger.CoreNET;` are valid and serve different purposes. The first (lowercase) contains the WorkSpace class and related workspace functionality, while the second (proper case) contains drivers like GenericAppiumDriver and other core functionality. Both may be required in files that use types from both namespaces.
Applied to files:
Ginger/GingerCoreNET/SourceControl/SourceControlIntegration.cs
🧬 Code graph analysis (6)
Ginger/GingerCoreNET/RunLib/CLILib/CLIProcessor.cs (1)
Ginger/GingerCoreNET/RunLib/CLILib/CLIHelper.cs (1)
GetTempFolderPathForRepo(1068-1099)
Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs (1)
Ginger/GingerCoreCommon/WorkSpaceLib/RepoFolderManager.cs (5)
RepoFolderManager(18-315)RepoFolderManager(38-62)AssignFolder(71-139)ReleaseFolder(144-162)UpdateHeartbeat(168-186)
Ginger/GingerCoreCommon/GeneralLib/General.cs (1)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
Ginger/GingerCoreNET/SourceControl/SourceControlIntegration.cs (8)
Ginger/GingerCore/GeneralLib/General.cs (4)
System(883-887)System(889-893)General(54-1572)ClearDirectoryContent(834-837)Ginger/GingerCoreCommon/SourceControlLib/SourceControlBase.cs (5)
SourceControlBase(35-252)CreateConfigFile(112-112)GetLatest(127-127)Disconnect(149-149)GetProject(129-129)Ginger/Ginger/DotNetFrameworkHelper.cs (3)
SourceControlBase(550-553)Type(138-160)GetLatest(540-543)Ginger/GingerRuntime/DotnetCoreHelper.cs (3)
SourceControlBase(195-198)Type(148-160)GetLatest(185-188)Ginger/GingerCoreCommon/UserCofig/UserProfile.cs (2)
UserProfile(82-1010)UserProfile(326-391)Ginger/GingerCoreNET/UserCofig/UserProfileOperations.cs (3)
UserProfile(284-319)UserProfileOperations(34-394)UserProfileOperations(37-41)Ginger/GingerCoreNET/SourceControl/GitSourceControlShellWrapper.cs (6)
GitSourceControlShellWrapper(30-284)GitSourceControlShellWrapper(34-46)CreateConfigFile(78-81)GetLatest(118-123)Disconnect(88-91)GetProject(135-156)Ginger/GingerCoreNET/SourceControl/GITSourceControl.cs (5)
GITSourceControl(36-1929)CreateConfigFile(1764-1833)GetLatest(306-365)Disconnect(1376-1378)GetProject(451-475)
Ginger/GingerCoreNET/RunLib/CLILib/CLIHelper.cs (3)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (2)
Reporter(28-357)ToLog(41-87)Ginger/GingerCoreCommon/WorkSpaceLib/RepoFolderManager.cs (2)
RepoFolderManager(18-315)RepoFolderManager(38-62)Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs (1)
RepoFolderManager(59-62)
Ginger/GingerCoreCommon/WorkSpaceLib/RepoFolderManager.cs (3)
Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs (1)
RepoFolderManager(59-62)Ginger/GingerCoreCommon/GeneralLib/General.cs (1)
General(36-714)Ginger/GingerCoreNET/RunLib/CLILib/CLIHelper.cs (2)
Dispose(1117-1121)Dispose(1123-1142)
🔇 Additional comments (24)
Ginger/GingerCoreCommon/GeneralLib/HttpUtilities.cs (1)
97-97: LGTM: Minor formatting improvement.The trailing comma addition aligns with modern C# formatting conventions and improves code consistency.
Ginger/GingerCoreCommon/GeneralLib/General.cs (3)
118-135: Good addition for per-user repo folder management.The new
DefaultGingerReposFolderproperty provides a clean, centralized location for Git repositories. The implementation correctly uses lazy initialization, caching, and creates the directory if it doesn't exist.
299-382: Robust improvements to directory clearing with comprehensive error handling.The enhanced
ClearDirectoryContentmethod now includes:
- Input validation for null/empty paths
- Attribute resetting to handle ReadOnly/Hidden/System files
- Retry mechanism with exponential backoff for transient failures
- Per-item error logging for better diagnostics
- Depth-first directory traversal to handle nested structures like
.gitThese changes significantly improve reliability when clearing directories, especially those containing version control metadata.
655-655: New VariableMinimalRecord looks good.The record provides a clean, immutable data structure for variable tracking with name, initial value, and current value properties.
Ginger/GingerCoreNET/SourceControl/GITSourceControl.cs (4)
319-323: Add null check for Pull result to prevent potential issues.The new null check prevents downstream errors when the Pull operation returns null. This defensive programming practice improves robustness.
1467-1467: Good API extension with progress support.The updated signature properly adds optional
progressNotifierandcancellationTokenparameters while maintaining backward compatibility through default values.
1509-1513: Progress reporting for checkout phase added successfully.The new
OnCheckoutProgresscallback ensures users receive feedback during the checkout phase of cloning, improving the user experience for large repositories.
1626-1626: OK to throw — Pull is private and internal callers handle exceptions.Pull is a private method; all calls are inside GITSourceControl.cs (lines ~112, 130, 162, 168, 317) and are wrapped in try/catch (GetLatest at ~317 also catches/logs and returns false). No external callers, so switching from returning null to throwing does not break external callers.
Ginger/GingerCoreCommon/WorkSpaceLib/RepoFolderManager.cs (3)
38-44: Good API improvement with sensible defaults.The constructor reordering to make
processIdthe first required parameter andbaseWorkingFolderoptional with a default toGeneral.DefaultGingerReposFolderimproves the API usability. This is a cleaner design that reduces boilerplate for callers.
53-61: Well-structured data folder isolation.Moving coordination files to a dedicated
RepoFolderManagerDatasubfolder underLocalUserApplicationDataFolderPathprovides better organization and separation of concerns. This prevents clutter in the main app data folder and makes it easier to manage permissions.
276-284: Robust error handling for corrupted data files.The enhanced
LoadAssignmentsmethod now gracefully handles corrupted or inaccessible assignment files by resetting to an empty dictionary instead of propagating exceptions. This ensures the system can recover from file corruption scenarios.Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs (2)
40-46: Clever test isolation using reflection.The test setup appropriately overrides the static field to ensure tests don't interfere with the user's actual app data folder. Good use of reflection to maintain test isolation.
64-191: Comprehensive test coverage for RepoFolderManager.The test suite thoroughly covers:
- Basic folder assignment and uniqueness
- Cross-process isolation
- Folder release and reuse
- Heartbeat mechanism
- Stale assignment cleanup
- Concurrent access scenarios
- Lock timeout handling
- Resilience to corrupted data
All tests have appropriate timeouts and assertions. Well done!
Ginger/GingerCoreNET/RunLib/CLILib/CLIProcessor.cs (1)
533-533: Minor formatting change looks good.The added space after the comma improves code readability.
Ginger/GingerCoreNET/RunLib/CLILib/CLIHelper.cs (3)
649-653: Good improvement: Success log is now conditional on actual successMoving the success log inside the success path ensures it only logs when the operation actually succeeds, eliminating false positive success messages.
655-657: LGTM: Added exception parameter to error loggingGood practice to include the exception object in the error log for better debugging information.
1087-1087: Constructor signature change aligns with refactored RepoFolderManager APIThe change to use the single-parameter constructor
RepoFolderManager(_processId)correctly matches the updated constructor signature in RepoFolderManager.cs, which now defaults the base working folder toGeneral.DefaultGingerReposFolderwhen not specified.Ginger/GingerCoreNET/SourceControl/SourceControlIntegration.cs (7)
527-564: Well-structured refactoring of source control creation logicGood separation of concerns by extracting the source control instantiation logic into a dedicated method. The logic correctly handles Git vs SVN and shell client preferences.
566-583: Good centralization of source control configurationExtracting the configuration logic improves maintainability by consolidating all source control settings in one place.
585-604: Correct implementation of solution info extractionThe method properly checks for .svn or .git directories to determine if the solution exists locally and correctly extracts the repository folder name.
606-640: Complex URL construction logic for SVN - verify edge casesThe GetProjectURI method has intricate SVN URL construction logic with multiple conditional branches. Please verify that all URL patterns are handled correctly, especially:
- URLs starting with "SVN" (line 610)
- URLs with various combinations of "/svn" and trailing slashes (lines 619-631)
Consider adding unit tests to cover the various SVN URL patterns to ensure consistent behavior across different repository configurations.
694-736: Robust error recovery for corrupted Git repositoriesExcellent implementation of error recovery when encountering corrupted local Git repositories. The solution:
- Specifically catches Git repository corruption errors
- Only applies cleanup in execution mode and within the default Ginger repos folder (safety constraints)
- Properly disconnects the source control before cleanup
- Implements retry logic with backoff for IO and access errors
- Falls back to GetProject after cleanup
716-726: Good retry implementation with progressive backoffThe retry logic properly handles both IOException and UnauthorizedAccessException with:
- Limited retry attempts (2)
- Progressive backoff (300ms * attempt number)
- Appropriate warning logs for each retry
712-712: Consider using the imported GeneralLib namespaceSince you've added
using Amdocs.Ginger.Common.GeneralLib;at line 22, you can simplify the call to justGeneralLib.ClearDirectoryContent.- General.ClearDirectoryContent(SolutionFolder); + GeneralLib.ClearDirectoryContent(SolutionFolder);⛔ Skipped due to learnings
Learnt from: GokulBothe99 PR: Ginger-Automation/Ginger#4188 File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23 Timestamp: 2025-04-25T13:29:45.059Z Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
| ServicePointManager.ServerCertificateValidationCallback += (_, _, _, _) => true; | ||
| handler.ServerCertificateCustomValidationCallback += (_, _, _, _) => { return true; }; | ||
| using var client = new HttpClient(handler); |
There was a problem hiding this comment.
🧹 Nitpick
Consider making certificate validation configurable.
The unconditional bypass of TLS certificate validation should ideally be configurable to maintain security in production environments while allowing development/testing flexibility.
Consider adding a configuration parameter to control certificate validation bypass:
+ if (ShouldIgnoreCertificateValidation()) // Add configuration check
+ {
+ handler.ServerCertificateCustomValidationCallback += (_, _, _, _) => { return true; };
+ }
- ServicePointManager.ServerCertificateValidationCallback += (_, _, _, _) => true;
- handler.ServerCertificateCustomValidationCallback += (_, _, _, _) => { return true; }; This would require adding a configuration setting and helper method to determine when certificate validation should be bypassed.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Ginger/GingerCoreCommon/GeneralLib/HttpUtilities.cs around lines 66-68, the
code unconditionally bypasses TLS certificate validation; make this behavior
configurable by adding a configuration setting (e.g., EnableInsecureTls or
BypassCertificateValidation) defaulting to false, expose a helper method to read
that setting, and only set ServerCertificateCustomValidationCallback (and avoid
changing the global ServicePointManager callback) when the configuration flag is
true; update the code to respect the flag and document the setting so production
remains secure while tests/dev can opt-in.
| ServicePointManager.ServerCertificateValidationCallback += (_, _, _, _) => true; | ||
| handler.ServerCertificateCustomValidationCallback += (_, _, _, _) => { return true; }; |
There was a problem hiding this comment.
Security risk: Global TLS certificate validation bypass.
The global ServicePointManager.ServerCertificateValidationCallback bypass affects all HTTP(S) requests across the entire application domain, not just this specific request. This creates a significant security vulnerability by disabling certificate validation for all subsequent HTTPS connections in the process.
Based on the retrieved learnings, the codebase maintains dual SSL bypass approaches for backward compatibility. However, the global bypass should be avoided in favor of handler-specific validation.
Apply this diff to remove the global bypass and rely only on the handler-specific bypass:
- ServicePointManager.ServerCertificateValidationCallback += (_, _, _, _) => true;
handler.ServerCertificateCustomValidationCallback += (_, _, _, _) => { return true; };Additionally, consider making SSL certificate validation bypass configurable rather than unconditional to maintain security in production environments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ServicePointManager.ServerCertificateValidationCallback += (_, _, _, _) => true; | |
| handler.ServerCertificateCustomValidationCallback += (_, _, _, _) => { return true; }; | |
| handler.ServerCertificateCustomValidationCallback += (_, _, _, _) => { return true; }; |
🤖 Prompt for AI Agents
In Ginger/GingerCoreCommon/GeneralLib/HttpUtilities.cs around lines 66-67,
remove the global TLS bypass assignment to
ServicePointManager.ServerCertificateValidationCallback and keep only the
handler-specific ServerCertificateCustomValidationCallback; update the code to
apply the handler callback conditionally based on a configuration flag (e.g., a
boolean "AllowUntrustedCertificates" from app settings or environment) so the
bypass is not unconditional in production, and ensure the global
ServicePointManager line is deleted to avoid affecting other requests.
🧩 Analysis chain
Verify the necessity of TLS certificate validation bypass.
Please confirm that bypassing TLS certificate validation is actually required for the intended use cases, as this significantly reduces security. Consider whether this change is needed for all scenarios or only specific development/testing environments.
Based on the web search results, I have confirmed several security concerns and best practices regarding TLS certificate validation bypass:
Key Security Findings:
-
Global bypasses are extremely dangerous: "once you're done with testing you may forget to clean it up in the source code, and it silently paves it's way out to production… What it results into is that all TLS certificates will be accepted and all HTTPS traffic will be allowed, even the malicious one! This can be extremely destructive from the security perspective and can open up for the man-in-the-middle vulnerability"
-
Production risks: "Bypassing SSL certificate validation should never be done in production environments as it exposes your application to man-in-the-middle attacks. Remember: SSL certificate validation is a critical security feature. Only bypass it when absolutely necessary and never in production environments."
-
Better alternatives exist: ".NET a new HttpClientHandler property - HttpClientHandler.DangerousAcceptAnyServerCertificateValidator - makes deactivation of certificate validation for development purposes more secure and scoped to development environment only"
-
Configuration-based approach recommended: Examples show bypassing certificate validation only when configuration explicitly enables it, with comments like "!DO NOT DO IT IN PRODUCTION!! GO AND CREATE VALID CERTIFICATE!"
This confirms that the current implementation poses significant security risks, particularly the global ServicePointManager bypass.
🏁 Script executed:
#!/bin/bash
# Description: Search for other usages of SSL certificate validation bypass in the codebase
# Expected: Find if there are other similar patterns or configurations
echo "=== Searching for ServicePointManager certificate validation bypasses ==="
rg -n "ServicePointManager\.ServerCertificateValidationCallback" --type cs -A2 -B2
echo -e "\n=== Searching for ServerCertificateCustomValidationCallback usage ==="
rg -n "ServerCertificateCustomValidationCallback" --type cs -A2 -B2
echo -e "\n=== Searching for DangerousAcceptAnyServerCertificateValidator usage ==="
rg -n "DangerousAcceptAnyServerCertificateValidator" --type cs -A2 -B2
echo -e "\n=== Searching for SSL/TLS related configuration settings ==="
rg -n "(AllSSL|IgnoreSSL|BypassSSL|TLS|SSL)" --type cs -C2 | head -20Length of output: 10866
Remove global TLS cert-validation bypass; scope accept-any-cert to explicit dev/test configs.
Global ServicePointManager callbacks are present in multiple places and create a serious MITM risk — do not leave this enabled in production.
-
Fix these locations (remove/avoid global ServicePointManager assignment; scope to per-HttpClient handler and guard by config + logging):
- Ginger/GingerCoreCommon/GeneralLib/HttpUtilities.cs:66-67
- Ginger/GingerCoreNET/Drivers/WebServicesDriver/HttpWebClientUtils.cs:249-251, 274-276, 299-302
- Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServiceXML.cs:32-34
- Ginger/GingerCoreNET/GeneralLib/EmailOperations.cs:161
- Ginger/GingerCoreNET/ActionsLib/Webservices/ActREST.cs:277-279
- Ginger/GingerCoreNET/ActionsLib/ActeMail.cs:674-676
- Ginger/GingerCoreNET/SourceControl/SVNSourceControl.cs:1001-1003
- Ginger/GingerCore/SourceControl/SVNSourceControl.cs:56-60
- Unit tests referencing AllSSL: Ginger/GingerCoreNETUnitTest/Webservice/WebServicesTest.cs:303, 355, 406, 454
-
Recommended fixes:
- Remove/avoid ServicePointManager.ServerCertificateValidationCallback global changes.
- If accept-any-cert is required for development/tests, gate it behind an explicit config/environment flag (default = disabled), enable only per-HttpClient handler (or HttpClientHandler.DangerousAcceptAnyServerCertificateValidator) for the shortest scope, and log a prominent warning when enabled.
- Replace unit tests that set AllSSL with test-scoped feature flags/fixtures that enable the bypass only for test duration.
🤖 Prompt for AI Agents
Ginger/GingerCoreCommon/GeneralLib/HttpUtilities.cs lines 66-67: remove the
global ServicePointManager.ServerCertificateValidationCallback assignment and do
not set global accept-any-cert behavior; instead, if you need to allow untrusted
certs for dev/tests, gate it behind an explicit config/env flag (default false),
apply the bypass only to the specific HttpClient/HttpClientHandler instance (or
use HttpClientHandler.DangerousAcceptAnyServerCertificateValidator) for the
shortest scope, and emit a prominent warning log when the flag is enabled;
update related places and unit tests to use test-scoped fixtures or feature
flags that temporarily enable the per-client bypass during test execution rather
than changing global ServicePointManager behavior.
| bool inUse = _assignments.Values.Any(assignment => | ||
| string.Equals(assignment.FolderPath, subFolderPath, StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
🧹 Nitpick
Consider using StringComparer for path comparison.
The current case-insensitive comparison might not be appropriate for all file systems. Consider using the appropriate StringComparer based on the OS:
bool inUse = _assignments.Values.Any(assignment =>
- string.Equals(assignment.FolderPath, subFolderPath, StringComparison.OrdinalIgnoreCase));
+ string.Equals(assignment.FolderPath, subFolderPath,
+ RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
+ ? StringComparison.OrdinalIgnoreCase
+ : StringComparison.Ordinal));Committable suggestion skipped: line range outside the PR's diff.
| using var holder = new FileStream(lockPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None); | ||
|
|
||
| // Act: attempt to assign with very short timeout and few retries | ||
| var manager = new RepoFolderManager( "timeoutProc", Path.Combine(_tempRoot, "Repos"), lockAcquisitionTimeout: TimeSpan.FromMilliseconds(200), maxLockRetryAttempts: 5, initialRetryDelay: TimeSpan.FromMilliseconds(20)); |
There was a problem hiding this comment.
🧹 Nitpick
Fix spacing in constructor call.
Minor formatting issue with spacing around constructor parameters.
- var manager = new RepoFolderManager( "timeoutProc", Path.Combine(_tempRoot, "Repos"), lockAcquisitionTimeout: TimeSpan.FromMilliseconds(200), maxLockRetryAttempts: 5, initialRetryDelay: TimeSpan.FromMilliseconds(20));
+ var manager = new RepoFolderManager("timeoutProc", Path.Combine(_tempRoot, "Repos"), lockAcquisitionTimeout: TimeSpan.FromMilliseconds(200), maxLockRetryAttempts: 5, initialRetryDelay: TimeSpan.FromMilliseconds(20));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var manager = new RepoFolderManager( "timeoutProc", Path.Combine(_tempRoot, "Repos"), lockAcquisitionTimeout: TimeSpan.FromMilliseconds(200), maxLockRetryAttempts: 5, initialRetryDelay: TimeSpan.FromMilliseconds(20)); | |
| var manager = new RepoFolderManager("timeoutProc", Path.Combine(_tempRoot, "Repos"), lockAcquisitionTimeout: TimeSpan.FromMilliseconds(200), maxLockRetryAttempts: 5, initialRetryDelay: TimeSpan.FromMilliseconds(20)); |
🤖 Prompt for AI Agents
In Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs around
line 168, the constructor call has inconsistent spacing around parameters;
adjust spacing to follow project style (no extra space after opening parenthesis
and single spaces after commas) so the call becomes uniformly formatted, e.g.,
remove the extra space before the first string argument and ensure single spaces
after commas between parameters.
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Failed to get repo folder path from Repo Folder Manager, using the random temp folder", ex); | ||
| Reporter.ToLog(eLogLevel.WARN, $"Failed to get repo folder path from Repo Folder Manager, using the random temp folder", ex); |
There was a problem hiding this comment.
🧹 Nitpick
Consider keeping error level as ERROR for folder assignment failures
While the log level was lowered from ERROR to WARN, failures to get a repo folder path could impact execution reliability. Since this falls back to a random temp folder (line 1097), which may not have the same cleanup guarantees as managed folders, consider keeping this as ERROR level to maintain visibility of potential resource management issues.
- Reporter.ToLog(eLogLevel.WARN, $"Failed to get repo folder path from Repo Folder Manager, using the random temp folder", ex);
+ Reporter.ToLog(eLogLevel.ERROR, $"Failed to get repo folder path from Repo Folder Manager, falling back to random temp folder", ex);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Reporter.ToLog(eLogLevel.WARN, $"Failed to get repo folder path from Repo Folder Manager, using the random temp folder", ex); | |
| Reporter.ToLog(eLogLevel.ERROR, $"Failed to get repo folder path from Repo Folder Manager, falling back to random temp folder", ex); |
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/RunLib/CLILib/CLIHelper.cs around line 1096, the log
call for failing to get the repo folder path was downgraded to WARN but should
remain ERROR; change the Reporter.ToLog invocation back to eLogLevel.ERROR and
keep the existing message and exception parameter so the failure to fall back to
a temp folder is logged at error level for visibility and resource-management
tracking.
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs (1)
Ginger/GingerCoreCommon/WorkSpaceLib/RepoFolderManager.cs (5)
RepoFolderManager(18-315)RepoFolderManager(38-62)AssignFolder(71-139)ReleaseFolder(144-162)UpdateHeartbeat(168-186)
🔇 Additional comments (1)
Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs (1)
169-169: Fix spacing in constructor call.Inconsistent space after the opening parenthesis.
- var manager = new RepoFolderManager( "timeoutProc", Path.Combine(_tempRoot, "Repos"), lockAcquisitionTimeout: TimeSpan.FromMilliseconds(200), maxLockRetryAttempts: 5, initialRetryDelay: TimeSpan.FromMilliseconds(20)); + var manager = new RepoFolderManager("timeoutProc", Path.Combine(_tempRoot, "Repos"), lockAcquisitionTimeout: TimeSpan.FromMilliseconds(200), maxLockRetryAttempts: 5, initialRetryDelay: TimeSpan.FromMilliseconds(20));
| using System; | ||
| using System.IO; | ||
| using System.Linq; | ||
| using System.Reflection; | ||
| using System.Threading.Tasks; | ||
| using Amdocs.Ginger.Common.WorkSpaceLib; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
| using GingerTestHelper; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Add missing usings for strongly-typed reflection and JSON parsing.
This enables typeof(General) usage (safer than string-based reflection) and robust heartbeat assertions via JSON parsing.
using System;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Threading.Tasks;
using Amdocs.Ginger.Common.WorkSpaceLib;
+using Amdocs.Ginger.Common.GeneralLib;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using GingerTestHelper;
+using System.Text.Json;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using System; | |
| using System.IO; | |
| using System.Linq; | |
| using System.Reflection; | |
| using System.Threading.Tasks; | |
| using Amdocs.Ginger.Common.WorkSpaceLib; | |
| using Microsoft.VisualStudio.TestTools.UnitTesting; | |
| using GingerTestHelper; | |
| using System; | |
| using System.IO; | |
| using System.Linq; | |
| using System.Reflection; | |
| using System.Threading.Tasks; | |
| using Amdocs.Ginger.Common.WorkSpaceLib; | |
| using Amdocs.Ginger.Common.GeneralLib; | |
| using Microsoft.VisualStudio.TestTools.UnitTesting; | |
| using GingerTestHelper; | |
| using System.Text.Json; |
| // We need to override General.LocalUserApplicationDataFolderPath backing field so the manager writes into our temp folder | ||
| // The LocalUserApplicationDataFolderPath implementation caches the value in a private static field 'mAppDataFolder'. | ||
| var generalType = Type.GetType("Amdocs.Ginger.Common.GeneralLib.General, GingerCoreCommon"); | ||
| var field = generalType.GetField("mAppDataFolder", BindingFlags.Static | BindingFlags.NonPublic); | ||
| _originalAppDataPathBackup = (string)field.GetValue(null); | ||
| field.SetValue(null, _tempRoot); // force usage of temp folder | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Replace string-based reflection with strongly-typed reflection and guard nulls.
Avoid fragile assembly-qualified names and fail fast if the field layout changes.
- // We need to override General.LocalUserApplicationDataFolderPath backing field so the manager writes into our temp folder
- // The LocalUserApplicationDataFolderPath implementation caches the value in a private static field 'mAppDataFolder'.
- var generalType = Type.GetType("Amdocs.Ginger.Common.GeneralLib.General, GingerCoreCommon");
- var field = generalType.GetField("mAppDataFolder", BindingFlags.Static | BindingFlags.NonPublic);
- _originalAppDataPathBackup = (string)field.GetValue(null);
- field.SetValue(null, _tempRoot); // force usage of temp folder
+ // We need to override General.LocalUserApplicationDataFolderPath backing field so the manager writes into our temp folder
+ // The LocalUserApplicationDataFolderPath implementation caches the value in a private static field 'mAppDataFolder'.
+ var generalType = typeof(General);
+ var field = generalType.GetField("mAppDataFolder", BindingFlags.Static | BindingFlags.NonPublic);
+ Assert.IsNotNull(field, "General.mAppDataFolder field not found; tests cannot override app-data folder.");
+ _originalAppDataPathBackup = (string)field.GetValue(null);
+ field.SetValue(null, _tempRoot); // force usage of temp folderRun this to confirm the backing field exists and hasn’t been renamed:
🏁 Script executed:
#!/bin/bash
rg -nP -C3 '\bclass\s+General\b' --type cs
rg -nP 'mAppDataFolder' --type cs
rg -nP 'LocalUserApplicationDataFolderPath' --type csLength of output: 6969
🏁 Script executed:
#!/bin/bash
sed -n '1,30p' Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs
rg -n -n 'using ' -n 1,50 Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.csLength of output: 1963
Use compile-time type reference and guard null for mAppDataFolder
Add the General namespace import, replace the assembly-qualified lookup with typeof(General), and assert the field exists before use.
+ using Amdocs.Ginger.Common.GeneralLib;
...
- var generalType = Type.GetType("Amdocs.Ginger.Common.GeneralLib.General, GingerCoreCommon");
+ var generalType = typeof(General);
var field = generalType.GetField("mAppDataFolder", BindingFlags.Static | BindingFlags.NonPublic);
+ Assert.IsNotNull(field, "General.mAppDataFolder field not found; tests cannot override app-data folder.");
_originalAppDataPathBackup = (string)field.GetValue(null);
field.SetValue(null, _tempRoot); // force usage of temp folder🤖 Prompt for AI Agents
Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs lines 40-46:
replace the runtime Type.GetType("Amdocs.Ginger.Common.GeneralLib.General,
GingerCoreCommon") lookup with a compile-time reference using typeof(General)
(add the appropriate using/import for its namespace at top of the file), then
retrieve the private static field mAppDataFolder via
typeof(General).GetField(...); add a null-guard after GetField to assert or
throw a clear exception if the field is null before calling GetValue/SetValue so
the test fails fast with a helpful message.
| var generalType = Type.GetType("Amdocs.Ginger.Common.GeneralLib.General, GingerCoreCommon"); | ||
| var field = generalType.GetField("mAppDataFolder", BindingFlags.Static | BindingFlags.NonPublic); | ||
| field.SetValue(null, _originalAppDataPathBackup); | ||
|
|
||
| try { Directory.Delete(_tempRoot, true); } catch { } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden cleanup: strongly-typed reflection and null-guard.
Prevents NullReferenceException if the field layout changes.
- // Restore cached path (avoid leaking temp path into other tests)
- var generalType = Type.GetType("Amdocs.Ginger.Common.GeneralLib.General, GingerCoreCommon");
- var field = generalType.GetField("mAppDataFolder", BindingFlags.Static | BindingFlags.NonPublic);
- field.SetValue(null, _originalAppDataPathBackup);
+ // Restore cached path (avoid leaking temp path into other tests)
+ var generalType = typeof(General);
+ var field = generalType.GetField("mAppDataFolder", BindingFlags.Static | BindingFlags.NonPublic);
+ if (field != null)
+ {
+ field.SetValue(null, _originalAppDataPathBackup);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var generalType = Type.GetType("Amdocs.Ginger.Common.GeneralLib.General, GingerCoreCommon"); | |
| var field = generalType.GetField("mAppDataFolder", BindingFlags.Static | BindingFlags.NonPublic); | |
| field.SetValue(null, _originalAppDataPathBackup); | |
| try { Directory.Delete(_tempRoot, true); } catch { } | |
| // Restore cached path (avoid leaking temp path into other tests) | |
| var generalType = typeof(General); | |
| var field = generalType.GetField("mAppDataFolder", BindingFlags.Static | BindingFlags.NonPublic); | |
| if (field != null) | |
| { | |
| field.SetValue(null, _originalAppDataPathBackup); | |
| } | |
| try { Directory.Delete(_tempRoot, true); } catch { } |
🤖 Prompt for AI Agents
In Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs around
lines 52 to 56, replace the loose string-based reflection and unguarded SetValue
with strongly-typed reflection and a null-check: obtain the Type via
typeof(Amdocs.Ginger.Common.GeneralLib.General) (or ensure Type.GetType returned
non-null) then get the non-public static field and check the FieldInfo is not
null before calling SetValue; if either the type or field is null, skip the
SetValue (or assert/fail with a clear message) and keep the Directory.Delete in
the try/catch as-is.
| private RepoFolderManager CreateManager(string processId = null) | ||
| { | ||
| return new RepoFolderManager(processId ?? Guid.NewGuid().ToString("N"), Path.Combine(_tempRoot, "Repos")); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Optional: add a test for default baseWorkingFolder resolution.
Currently all tests pass an explicit base path. Add one that passes baseWorkingFolder: null to exercise General.DefaultGingerReposFolder resolution.
I can add a test method AssignFolder_UsesDefaultBaseFolder_WhenNull that asserts the assigned path is rooted under General.DefaultGingerReposFolder. Want me to draft it?
🤖 Prompt for AI Agents
In Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs around
lines 59-62, add a unit test named AssignFolder_UsesDefaultBaseFolder_WhenNull
that constructs a RepoFolderManager with baseWorkingFolder set to null (instead
of passing an explicit temp path), invokes the same folder-assignment code used
by existing tests, and asserts the resulting assigned path is rooted under
General.DefaultGingerReposFolder; specifically, create the manager with new
RepoFolderManager(processId, null), call the method that returns/assigns the
repo folder, normalize with Path.GetFullPath, and assert the
path.StartsWith(General.DefaultGingerReposFolder) (or use Path.GetRelativePath
to ensure it is under that root) so the test verifies default base folder
resolution.
| { | ||
| return new RepoFolderManager(processId ?? Guid.NewGuid().ToString("N"), Path.Combine(_tempRoot, "Repos")); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
DRY: centralize assignments file path.
Avoids path duplication and typos across tests.
+
+ private string AssignmentsPath =>
+ Path.Combine(_tempRoot, "RepoFolderManagerData", "repo_folder_pool_assignments.json");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private string AssignmentsPath => | |
| Path.Combine(_tempRoot, "RepoFolderManagerData", "repo_folder_pool_assignments.json"); |
🤖 Prompt for AI Agents
In Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs around
line 63, tests currently duplicate the assignments file path; introduce a single
shared variable (e.g., private const or private static readonly string
AssignmentsFilePath = "<relative-or-combined-path>") at the top of the test
class and replace all hard-coded occurrences with that variable to avoid
duplication and typos; prefer constructing the path with Path.Combine if it uses
segments and update all tests in this file to reference AssignmentsFilePath.
| [TestMethod] | ||
| [Timeout(60000)] | ||
| [Ignore] | ||
| public void UpdateHeartbeat_RefreshesTimestamp() |
There was a problem hiding this comment.
🧹 Nitpick
Enable and stabilize the heartbeat test (remove Ignore).
The test can be made deterministic by asserting on the JSON value instead of file mtime.
- [Ignore]
public void UpdateHeartbeat_RefreshesTimestamp()🤖 Prompt for AI Agents
In Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs around
lines 103-106, remove the [Ignore] attribute so the test runs, and change the
assertion to make the test deterministic by reading and parsing the heartbeat
JSON file and asserting on the timestamp value inside the JSON rather than
relying on the file mtime; specifically capture the JSON before the update, call
UpdateHeartbeat, re-read and parse the JSON, and assert the heartbeat field was
updated (e.g., new timestamp > previous timestamp or equals the expected value)
so the test no longer depends on filesystem modification times.
| // Capture assignment file content pre-update | ||
| string assignmentsPath = Path.Combine(_tempRoot, "RepoFolderManagerData", "repo_folder_pool_assignments.json"); | ||
| DateTime before = DateTime.UtcNow; | ||
| manager.UpdateHeartbeat(); | ||
|
|
||
| string json = File.ReadAllText(assignmentsPath); | ||
| Assert.IsTrue(json.Contains("\"hb1\""), "Assignment record exists"); | ||
| // Not parsing full JSON (simplicity) but ensure file timestamp updated | ||
| Assert.IsTrue(File.GetLastWriteTimeUtc(assignmentsPath) >= before, "Heartbeat caused rewrite"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Assert heartbeat via JSON value rather than file timestamp.
Parsing LastHeartbeatUtc avoids flaky mtime checks and makes the test meaningful.
- // Capture assignment file content pre-update
- string assignmentsPath = Path.Combine(_tempRoot, "RepoFolderManagerData", "repo_folder_pool_assignments.json");
- DateTime before = DateTime.UtcNow;
- manager.UpdateHeartbeat();
-
- string json = File.ReadAllText(assignmentsPath);
- Assert.IsTrue(json.Contains("\"hb1\""), "Assignment record exists");
- // Not parsing full JSON (simplicity) but ensure file timestamp updated
- Assert.IsTrue(File.GetLastWriteTimeUtc(assignmentsPath) >= before, "Heartbeat caused rewrite");
+ // Capture LastHeartbeatUtc before update and assert it advances
+ var json1 = File.ReadAllText(AssignmentsPath);
+ using var doc1 = JsonDocument.Parse(json1);
+ var before = doc1.RootElement.GetProperty(procId).GetProperty("LastHeartbeatUtc").GetDateTime();
+
+ Task.Delay(10).Wait(); // ensure a different timestamp
+ manager.UpdateHeartbeat();
+
+ var json2 = File.ReadAllText(AssignmentsPath);
+ using var doc2 = JsonDocument.Parse(json2);
+ var after = doc2.RootElement.GetProperty(procId).GetProperty("LastHeartbeatUtc").GetDateTime();
+ Assert.IsTrue(after > before, "Heartbeat timestamp should advance");Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs lines
112-121: the test currently asserts the heartbeat update by checking file
LastWriteTimeUtc which is flaky; instead read and parse the JSON in
repo_folder_pool_assignments.json, deserialize to a suitable object/collection
(or parse into a dynamic/JObject), locate the assignment entry for "hb1" and
assert that its LastHeartbeatUtc (parse to DateTime, using Utc) is >= the
recorded 'before' timestamp; replace the file timestamp assertion with this JSON
value check and keep the existing assertion that the assignment record exists.
| public void CleanupStaleAssignments_RemovesOld() | ||
| { | ||
| // Use very short stale timeout by constructing with parameter | ||
| var manager = new RepoFolderManager("stale1", Path.Combine(_tempRoot, "Repos"), staleAssignmentTimeout: TimeSpan.FromMilliseconds(10)); | ||
| var folder = manager.AssignFolder("repoS"); | ||
| // Wait for staleness | ||
| Task.Delay(30).Wait(); | ||
| // Force new manager to trigger cleanup | ||
| var manager2 = new RepoFolderManager("newProc", Path.Combine(_tempRoot, "Repos"), staleAssignmentTimeout: TimeSpan.FromMilliseconds(10)); | ||
| manager2.AssignFolder("repoS"); | ||
|
|
||
| string assignmentsPath = Path.Combine(_tempRoot, "RepoFolderManagerData", "repo_folder_pool_assignments.json"); | ||
| string json = File.ReadAllText(assignmentsPath); | ||
| Assert.IsFalse(json.Contains("stale1"), "Stale assignment removed"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Minor: reduce flakiness by widening the staleness gap.
On loaded agents, 30ms can be tight versus 10ms timeout. Bump the delay to make the cutoff unambiguous.
- Task.Delay(30).Wait();
+ Task.Delay(50).Wait();If CI remains flaky, consider 100–150ms.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void CleanupStaleAssignments_RemovesOld() | |
| { | |
| // Use very short stale timeout by constructing with parameter | |
| var manager = new RepoFolderManager("stale1", Path.Combine(_tempRoot, "Repos"), staleAssignmentTimeout: TimeSpan.FromMilliseconds(10)); | |
| var folder = manager.AssignFolder("repoS"); | |
| // Wait for staleness | |
| Task.Delay(30).Wait(); | |
| // Force new manager to trigger cleanup | |
| var manager2 = new RepoFolderManager("newProc", Path.Combine(_tempRoot, "Repos"), staleAssignmentTimeout: TimeSpan.FromMilliseconds(10)); | |
| manager2.AssignFolder("repoS"); | |
| string assignmentsPath = Path.Combine(_tempRoot, "RepoFolderManagerData", "repo_folder_pool_assignments.json"); | |
| string json = File.ReadAllText(assignmentsPath); | |
| Assert.IsFalse(json.Contains("stale1"), "Stale assignment removed"); | |
| } | |
| public void CleanupStaleAssignments_RemovesOld() | |
| { | |
| // Use very short stale timeout by constructing with parameter | |
| var manager = new RepoFolderManager("stale1", Path.Combine(_tempRoot, "Repos"), staleAssignmentTimeout: TimeSpan.FromMilliseconds(10)); | |
| var folder = manager.AssignFolder("repoS"); | |
| // Wait for staleness | |
| Task.Delay(50).Wait(); | |
| // Force new manager to trigger cleanup | |
| var manager2 = new RepoFolderManager("newProc", Path.Combine(_tempRoot, "Repos"), staleAssignmentTimeout: TimeSpan.FromMilliseconds(10)); | |
| manager2.AssignFolder("repoS"); | |
| string assignmentsPath = Path.Combine(_tempRoot, "RepoFolderManagerData", "repo_folder_pool_assignments.json"); | |
| string json = File.ReadAllText(assignmentsPath); | |
| Assert.IsFalse(json.Contains("stale1"), "Stale assignment removed"); | |
| } |
🤖 Prompt for AI Agents
In Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs around
lines 125 to 139, the test uses a 10ms staleAssignmentTimeout but only waits
30ms before checking, which can be flakey on loaded agents; increase the wait to
a larger value (e.g., 100ms) to ensure the assignment becomes stale reliably
and, if CI still flakes, use 150ms instead.
| Parallel.For(0, count, i => | ||
| { | ||
| var m = CreateManager($"pc{i}"); | ||
| folders[i] = m.AssignFolder(repo); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Dispose managers in the parallel test.
Use using to proactively release any resources and reduce cross-test coupling.
Parallel.For(0, count, i =>
{
- var m = CreateManager($"pc{i}");
- folders[i] = m.AssignFolder(repo);
+ using var m = CreateManager($"pc{i}");
+ folders[i] = m.AssignFolder(repo);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Parallel.For(0, count, i => | |
| { | |
| var m = CreateManager($"pc{i}"); | |
| folders[i] = m.AssignFolder(repo); | |
| }); | |
| Parallel.For(0, count, i => | |
| { | |
| using var m = CreateManager($"pc{i}"); | |
| folders[i] = m.AssignFolder(repo); | |
| }); |
🤖 Prompt for AI Agents
In Ginger/GingerCoreCommonTest/WorkSpaceLib/RepoFolderManagerTests.cs around
lines 149 to 153, the test creates RepoFolderManager instances in a Parallel.For
loop but never disposes them; change the loop to dispose each manager after use
(e.g., use "using var m = CreateManager($\"pc{i}\"); folders[i] =
m.AssignFolder(repo);" or wrap creation and AssignFolder inside a using block /
try/finally that calls m.Dispose()) so each manager is proactively released and
resources aren't shared across tests.
e001fa9
into
Releases/Beta-Published/Beta-2025.4.1
1 participant
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests