Ensure SSL_CERT_DIR messages are always shown and check for existing value

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#3

Summary by CodeRabbit

• New Features

  • Enhanced verbose logging for Unix OpenSSL certificate directory configuration, including detection of already-configured directories and guidance for environment variable setup.

• Improvements

  • Refined event listener initialization for better control over verbose output during certificate operations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This pull request enhances certificate generation logging with improved OpenSSL SSL_CERT_DIR validation on Unix platforms. It adds two new EventSource events to log directory configuration status, refactors event listener initialization in the dev-certs tool, and strengthens the Unix certificate manager's validation logic for environment variable handling.

Changes

Cohort / File(s)	Summary
EventSource Event Enhancements src/Shared/CertificateGeneration/CertificateManager.cs	Added UnixOpenSslCertificateDirectoryAlreadyConfigured() and UnixSuggestAppendingToEnvironmentVariable() events to CertificateManagerEventSource. Updated event 110 message format to use parameterized environment variable names. Removed documentation remarks block.
OpenSSL SSL_CERT_DIR Validation src/Shared/CertificateGeneration/UnixCertificateManager.cs	Implemented robust SSL_CERT_DIR validation in TrustCertificateCore() that checks if the directory is already configured, logs appropriate guidance, and falls back to existing discovery logic. Refactored NssDb nested type to use private fields with public accessors.
Event Listener Centralization src/Tools/dotnet-dev-certs/src/Program.cs	Introduced ReporterEventListener upfront in command execution path with conditional activation based on --verbose flag. Consolidated event emission control into a single listener instance.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 SSL paths need care and grace,
So certificates find their place,
We log each step along the way,
Unix now knows what to say,
Trust flows clear, no more dismay! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is largely incomplete, missing required sections like detailed summary, description details, and issue reference per the template.	Follow the PR template: add a summary under 80 characters, include detailed description section, and reference the upstream issue with 'Fixes #' format.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main changes: checking for existing SSL_CERT_DIR values and ensuring related messages are displayed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/Shared/CertificateGeneration/UnixCertificateManager.cs`:
- Around line 362-382: The comparison uses prettyCertDir (which may contain
"$HOME" or "~") and performs a case-insensitive compare; fix by first expanding
environment variables and tilde in prettyCertDir (e.g., use
Environment.ExpandEnvironmentVariables and replace a leading "~" with HOME) to
produce the real path, then call Path.GetFullPath on that expanded path (update
certDirFullPath to the expanded/full path), and in the isCertDirIncluded lambda
compare using case-sensitive comparison (StringComparison.Ordinal) when checking
Path.GetFullPath(dir) against certDirFullPath; update code in
UnixCertificateManager where prettyCertDir, certDirFullPath and
isCertDirIncluded are used.

🧹 Nitpick comments (1)

src/Shared/CertificateGeneration/UnixCertificateManager.cs (1)

994-1007: Consider using a record or auto-properties for brevity.

This private class could be simplified using modern C# features while maintaining immutability.

♻️ Optional simplification using record

-    private sealed class NssDb
-    {
-        private readonly string _path;
-        private readonly bool _isFirefox;
-
-        public NssDb(string path, bool isFirefox)
-        {
-            _path = path;
-            _isFirefox = isFirefox;
-        }
-
-        public string Path => _path;
-        public bool IsFirefox => _isFirefox;
-    }
+    private sealed record NssDb(string Path, bool IsFirefox);

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a0c9bd and 07d6e09.

📒 Files selected for processing (3)

• src/Shared/CertificateGeneration/CertificateManager.cs
• src/Shared/CertificateGeneration/UnixCertificateManager.cs
• src/Tools/dotnet-dev-certs/src/Program.cs

🧰 Additional context used

🧬 Code graph analysis (1)

src/Tools/dotnet-dev-certs/src/Program.cs (1)

src/Shared/CommandLineUtils/CommandLine/CommandOption.cs (1)

• HasValue (93-96)

🔇 Additional comments (3)

src/Shared/CertificateGeneration/CertificateManager.cs (1)

1302-1320: LGTM!

The new events are well-designed:

• Event 113 at Verbose level correctly logs when the directory is already configured (informational)
• Event 114 at LogAlways level ensures users always see the guidance to append to an existing SSL_CERT_DIR
• The shell example in event 114 (export {1}=\"{0}:${1}\") correctly uses ${1} as shell variable expansion to preserve the existing value

src/Tools/dotnet-dev-certs/src/Program.cs (2)

127-135: LGTM!

The event listener initialization change correctly ensures that EventLevel.LogAlways events (including the new SSL_CERT_DIR guidance messages) are always captured and displayed to users, even when --verbose is not specified. This aligns well with the PR objective to ensure SSL_CERT_DIR messages are always shown.

---

139-141: No functional change - formatting only.

The || operator moved to the previous line is a stylistic change with no impact on behavior.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Bug: Using display string prettyCertDir instead of actual path for comparison.

prettyCertDir may contain the literal string $HOME (from lines 354-356) which Path.GetFullPath won't expand. This causes incorrect path comparisons.

Additionally, StringComparison.OrdinalIgnoreCase is used on line 375, but Unix/Linux filesystems are case-sensitive, which could cause false matches.

🐛 Proposed fix

             if (!string.IsNullOrEmpty(existingSslCertDir))
             {
                 var existingDirs = existingSslCertDir.Split(Path.PathSeparator);
-                var certDirFullPath = Path.GetFullPath(prettyCertDir);
+                var certDirFullPath = Path.GetFullPath(certDir);
                 var isCertDirIncluded = existingDirs.Any(dir =>
                 {
                     if (string.IsNullOrWhiteSpace(dir))
                     {
                         return false;
                     }

                     try
                     {
-                        return string.Equals(Path.GetFullPath(dir), certDirFullPath, StringComparison.OrdinalIgnoreCase);
+                        return string.Equals(Path.GetFullPath(dir), certDirFullPath, StringComparison.Ordinal);
                     }
                     catch
                     {
                         // Ignore invalid directory entries in SSL_CERT_DIR
                         return false;
                     }
                 });

🤖 Prompt for AI Agents

In `@src/Shared/CertificateGeneration/UnixCertificateManager.cs` around lines 362
- 382, The comparison uses prettyCertDir (which may contain "$HOME" or "~") and
performs a case-insensitive compare; fix by first expanding environment
variables and tilde in prettyCertDir (e.g., use
Environment.ExpandEnvironmentVariables and replace a leading "~" with HOME) to
produce the real path, then call Path.GetFullPath on that expanded path (update
certDirFullPath to the expanded/full path), and in the isCertDirIncluded lambda
compare using case-sensitive comparison (StringComparison.Ordinal) when checking
Path.GetFullPath(dir) against certDirFullPath; update code in
UnixCertificateManager where prettyCertDir, certDirFullPath and
isCertDirIncluded are used.