Skip to content

Ensure SSL_CERT_DIR messages are always shown and check for existing value#3

Open
tomerqodo wants to merge 6 commits intoaugment_full_base_ensure_ssl_cert_dir_messages_are_always_shown_and_check_for_existing_value_pr3from
augment_full_head_ensure_ssl_cert_dir_messages_are_always_shown_and_check_for_existing_value_pr3
Open

Ensure SSL_CERT_DIR messages are always shown and check for existing value#3
tomerqodo wants to merge 6 commits intoaugment_full_base_ensure_ssl_cert_dir_messages_are_always_shown_and_check_for_existing_value_pr3from
augment_full_head_ensure_ssl_cert_dir_messages_are_always_shown_and_check_for_existing_value_pr3

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#3

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Jan 26, 2026

🤖 Augment PR Summary

Summary: Ensures dotnet-dev-certs prints OpenSSL SSL_CERT_DIR guidance even when not running with --verbose.

Changes:

  • Always register ReporterEventListener and enable CertificateManager events at LogAlways (or Verbose when requested).
  • Update the OpenSSL “export …” example to use the actual environment variable name and quoting.
  • On Unix, inspect existing SSL_CERT_DIR to either confirm it already includes the certificate directory or suggest appending.
  • Add new EventSource events for the new messaging paths and refactor NssDb into an explicit class.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 3 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

if (!string.IsNullOrEmpty(existingSslCertDir))
{
var existingDirs = existingSslCertDir.Split(Path.PathSeparator);
var certDirFullPath = Path.GetFullPath(prettyCertDir);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

certDirFullPath is computed from prettyCertDir, which can contain the literal $HOME placeholder; Path.GetFullPath("$HOME/...") won’t resolve to the actual home directory and will likely prevent detecting that SSL_CERT_DIR already includes the cert dir.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

hasValidSslCertDir = false;
}

sawTrustFailure = !hasValidSslCertDir;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sawTrustFailure = !hasValidSslCertDir; overwrites any prior failures (e.g., dotnet/NSS trust issues) and can cause the method to return TrustLevel.Full even when earlier steps failed.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

}
else
{
listener.EnableEvents(CertificateManager.Log, System.Diagnostics.Tracing.EventLevel.LogAlways);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that LogAlways events are enabled by default, a mismatched EventSource message template/payload will crash dotnet-dev-certs because ReporterEventListener unconditionally calls string.Format(...); event 111 (UnixSuggestSettingEnvironmentVariableWithoutExample) currently uses {2} but only has 2 payload items.

Other Locations
  • src/Shared/CertificateGeneration/CertificateManager.cs:1307

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants